diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 30d5d2f..7b8cee3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -227,7 +227,14 @@ jobs: const fs = require("fs"); const p = process.env.DATA_DIR + "/artifacts/auth.json"; const a = JSON.parse(fs.readFileSync(p, "utf8")); - a.keys = [null, "ak_live_oops", { id: "k_broken", name: "broken" }]; + a.keys = [ + null, + "ak_live_oops", + { id: "k_broken", name: "broken" }, + // Whole record, one unreadable field. The bearer path 401s it and the key + // screen used to draw it as a healthy key with no expiry (T1.2.21). + { id: "k_expiry", name: "junk expiry", prefix: "ak_live_j", hash: "x".repeat(64), scopes: ["publish"], expiresAt: "garbage" }, + ]; fs.writeFileSync(p, JSON.stringify(a, null, 2)); ' node server.js & @@ -238,6 +245,18 @@ jobs: grep -q '"broken":true' /tmp/keys.json || { echo "FAIL: broken record not flagged"; cat /tmp/keys.json; exit 1; } if grep -q '"id":null' /tmp/keys.json; then echo "FAIL: unaddressable entry listed"; cat /tmp/keys.json; exit 1; fi echo "ok: GET /api/keys lists the broken record and skips the unaddressable entries" + # Per record, because a single grep for "broken":true passes on k_broken alone and + # would say nothing about the record whose only fault is the expiry. + node -e ' + const rows = JSON.parse(require("fs").readFileSync("/tmp/keys.json", "utf8")); + const row = rows.find((k) => k.id === "k_expiry"); + if (!row) { console.error("FAIL: k_expiry is not listed", rows); process.exit(1); } + if (row.broken !== true) { console.error("FAIL: k_expiry is not flagged broken", row); process.exit(1); } + console.log("ok: a record with an unreadable expiresAt lists as broken"); + ' + code=$(curl -s -o /dev/null -w '%{http_code}' -X DELETE -H "$AUTH" http://localhost:3000/api/keys/k_expiry) + [ "$code" = "200" ] || { echo "FAIL: DELETE /api/keys/k_expiry -> $code"; exit 1; } + echo "ok: the operator can revoke it without hand-editing auth.json" code=$(curl -s -o /dev/null -w '%{http_code}' -X PATCH -H "$AUTH" -H 'Content-Type: application/json' \ -d '{"disabled":true}' http://localhost:3000/api/keys/k_broken) [ "$code" = "200" ] || { echo "FAIL: PATCH /api/keys/k_broken -> $code"; exit 1; } diff --git a/cli.js b/cli.js index 2d2d78e..da3ebfa 100644 --- a/cli.js +++ b/cli.js @@ -301,10 +301,12 @@ switch (command) { if (sub === 'list') { const keys = await apiJson('GET', '/api/keys'); for (const k of keys) { - const flags = [ - // A record missing the hash or the scopes the bearer path reads. It answers 401 - // whatever you do with it, so say so here as well as on the dashboard. - k.broken && 'broken, always answers 401', + // A record missing the hash or the scopes the bearer path reads, or carrying an + // expiresAt nothing can read. It answers 401 whatever you do with it, so say that + // and nothing else: the dashboard drops the rest of the line for the same reason, + // and "expires garbage" next to "always answers 401" reads like a working key with + // an odd date on it. + const flags = k.broken ? ['broken, always answers 401'] : [ k.disabled && 'disabled', k.expiresAt && `expires ${k.expiresAt.slice(0, 10)}`, k.lastUsedAt ? `used ${k.lastUsedAt.slice(0, 10)}` : 'never used', diff --git a/docs/auth.md b/docs/auth.md index 2ad8e63..d9a0542 100644 --- a/docs/auth.md +++ b/docs/auth.md @@ -77,9 +77,21 @@ hand edit or a crash mid-write can leave behind. The server skips a record with scopes and names it at boot: ``` -auth.json: ignoring 1 key record(s) with no hash or no scopes (k_7fa2c1). Those keys return 401 until you delete or re-create them. +auth.json: ignoring 1 key record(s) with no hash or no scopes ("k_7fa2c1"). Revoke them on the key screen and mint new ones. ``` +An `expiresAt` the server cannot read is the second way a record dies quietly. Anything that is +not an ISO date string counts, including a number, an object and a value in the wrong format. +The key answers `401` from that point on, and the key screen shows it as broken with Revoke as +the only action: + +``` +auth.json: 1 key record(s) carry an expiresAt nothing can read ("k_7fa2c1"). They return 401 from here on. +``` + +An expiry that has simply passed is not this. It reads back, the key screen prints the date, and +the key stops working for the reason it says. + ## A corrupt auth.json A single broken record is skipped. A file that does not parse at all is different: the server diff --git a/lib/auth.js b/lib/auth.js index 091f03b..156520f 100644 --- a/lib/auth.js +++ b/lib/auth.js @@ -155,16 +155,38 @@ export function usableKey(k) { // only reaches here through a hand-edited or restored auth.json, since parseKeyInput rejects a // bad expiresAt at mint time. Absent, null and empty still mean "no expiry"; anything else that // does not parse is treated as expired, which is the direction that fails closed. +// Non-strings are expired without asking Date.parse, which stringifies its argument first and +// then accepts more than an operator would expect: `2020` reads as the year 2020 and +// `["2099-01-01"]` reads as the string inside the array. Both are hand edits, and letting +// Date.parse rule on them put a key back in the state T1.2.21 is about, rejected by the bearer +// path and drawn as healthy by both clients. parseKeyInput only ever writes an ISO string. export function keyExpired(k) { if (k.expiresAt === undefined || k.expiresAt === null || k.expiresAt === '') return false; + if (typeof k.expiresAt !== 'string') return true; const t = Date.parse(k.expiresAt); return Number.isNaN(t) || t <= Date.now(); } +// True when a record carries an expiry nothing can read. keyExpired rejects these, which is the +// right answer and says nothing to the operator: publicKey coerces a non-string expiresAt to +// null, so `{}`, `true` and `[]` render as a key with no expiry at all, and "garbage" renders as +// an expiry date. Either way the key screen shows a working key while every request with it +// answers 401. A date that has simply passed is not this: it reads back, both clients print it, +// and the operator can see what happened. +export function keyExpiryUnreadable(k) { + const value = k.expiresAt; + if (value === undefined || value === null || value === '') return false; + if (typeof value !== 'string') return true; + return Number.isNaN(Date.parse(value)); +} + // The key screen renders whatever this returns, and it is the screen an operator opens after // the boot warning tells them a record is broken. So it has to survive the same records // usableKey() screens off the bearer path, and say which ones they are: a `broken` record // authenticates nothing, and the only action worth offering on it is Revoke. +// +// An unreadable expiresAt is broken by the same test: the record authenticates nothing and +// Disable cannot change that, so it belongs in the same row treatment as a hashless one. export function publicKey(k) { return { id: typeof k.id === 'string' ? k.id : null, @@ -178,7 +200,7 @@ export function publicKey(k) { expiresAt: typeof k.expiresAt === 'string' ? k.expiresAt : null, lastUsedAt: typeof k.lastUsedAt === 'string' ? k.lastUsedAt : null, disabled: !!k.disabled, - broken: !usableKey(k), + broken: !usableKey(k) || keyExpiryUnreadable(k), }; } @@ -287,16 +309,39 @@ export async function createAuthStore(storage, { apiKey, baseUrl }) { const auth = await loadAuth(); + // Both warnings name records by id. GET /api/keys lists only records whose id is a string, + // so a record with any other id has to be named by position instead: naming it by its id + // would send the operator looking for a row that is not on the screen. + const recordName = (k, i) => (typeof k?.id === 'string' && k.id ? k.id : `entry ${i}`); + + // An id is only ever a nanoid this server generated. A hand-edited one can hold a newline, + // which would split the line and let the file write whatever it likes into the log. Quoting + // escapes it and costs a pair of quotes. + const nameList = (ids) => ids.map((id) => JSON.stringify(id)).join(', '); + // resolveApiKey skips these, so they cannot take the instance down. Name them once at boot // anyway: a bearer 401 is logged nowhere, so without this line a key that stopped working // looks like a client problem. const badKeys = auth.keys - .map((k, i) => (usableKey(k) ? null : k?.id || `entry ${i}`)) + .map((k, i) => (usableKey(k) ? null : recordName(k, i))) .filter(Boolean); if (badKeys.length) { console.warn( `auth.json: ignoring ${badKeys.length} key record(s) with no hash or no scopes ` + - `(${badKeys.join(', ')}). Those keys return 401 until you delete or re-create them.`, + `(${nameList(badKeys)}). Revoke them on the key screen and mint new ones.`, + ); + } + + // Same reason, different field: keyExpired counts an expiry it cannot read as passed, so the + // record 401s from boot with nothing said anywhere. Named separately from the line above + // because the fix is different: this one is a value in the file, not a missing field. + const unreadableExpiry = auth.keys + .map((k, i) => (usableKey(k) && keyExpiryUnreadable(k) ? recordName(k, i) : null)) + .filter(Boolean); + if (unreadableExpiry.length) { + console.warn( + `auth.json: ${unreadableExpiry.length} key record(s) carry an expiresAt nothing can read ` + + `(${nameList(unreadableExpiry)}). They return 401 from here on.`, ); } diff --git a/public/index.html b/public/index.html index 817174f..be765b2 100644 --- a/public/index.html +++ b/public/index.html @@ -1950,8 +1950,9 @@

Published

const bits = [k.prefix + '…']; if (k.expiresAt) bits.push('expires ' + k.expiresAt.slice(0, 10)); bits.push(k.lastUsedAt ? ('used ' + k.lastUsedAt.slice(0, 10)) : 'never used'); - // A broken record is missing the hash or the scopes the bearer path reads, so it answers - // 401 no matter what. Enabling it would change nothing; revoking it is the whole fix. + // A broken record is missing the hash or the scopes the bearer path reads, or carries an + // expiresAt nothing can read. Either way it answers 401 no matter what: enabling it would + // change nothing, and revoking it is the whole fix. sub.textContent = k.broken ? 'always answers 401. Revoke it and mint a new key.' : bits.join(' · '); meta.append(name, sub); @@ -1968,7 +1969,12 @@

Published

del.addEventListener('click', async () => { const ok = await dialog({ title: 'Revoke “' + k.name + '”?', - body: 'Any CLI or MCP client using this key stops working.', + // The row already told the operator this key answers 401 for everything. Warning them + // that clients will stop working reads as an argument against the action they were + // just sent here to take. + body: k.broken + ? 'This key already answers 401 for every request. Revoking removes the record.' + : 'Any CLI or MCP client using this key stops working.', confirmText: 'Revoke', danger: true, }); if (!ok) return; diff --git a/test/auth.test.js b/test/auth.test.js index 1f599eb..3e1ba2a 100644 --- a/test/auth.test.js +++ b/test/auth.test.js @@ -8,6 +8,7 @@ import { AuthFileError, capTtlDays, hashKey, + keyExpired, publicKey, signSession, } from '../lib/auth.js'; @@ -281,6 +282,57 @@ test('publicKey drops timestamps and an id that are not strings', () => { assert.equal(row.createdAt, null); }); +// keyExpired treats an expiry it cannot read as passed, so the record 401s. Before this, both +// clients still drew it as a working key: a non-string expiresAt comes out of publicKey as null, +// which reads as "no expiry", and "garbage" reads as an expiry date. The operator's loop was +// key 401s, open the key screen, see a healthy key, press Disable, nothing changes. +test('publicKey flags a record whose expiresAt cannot be read', () => { + // The last three are the ones Date.parse reads after stringifying: 2020 and 0 become years, + // and a one-element array becomes the string inside it. All three are past dates, so + // keyExpired already rejected the key while publicKey drew it as a key with no expiry. + for (const junk of ['garbage', '2026-13-45', {}, true, [], 2020, 0, ['2020-01-01']]) { + const row = publicKey({ ...goodKey(), expiresAt: junk }); + assert.equal(row.broken, true, `${JSON.stringify(junk)} should read as broken`); + assert.equal(keyExpired({ expiresAt: junk }), true, `${JSON.stringify(junk)} should also 401`); + } +}); + +// An expiry that has passed is not broken. It reads back, both clients print the date, and the +// operator can see for themselves why the key stopped working. +test('publicKey leaves a readable expiry alone, past or future', () => { + const past = new Date(Date.now() - 60_000).toISOString(); + const future = new Date(Date.now() + 60_000).toISOString(); + + assert.equal(publicKey({ ...goodKey(), expiresAt: past }).broken, false); + assert.equal(publicKey({ ...goodKey(), expiresAt: future }).broken, false); + assert.equal(publicKey({ ...goodKey(), expiresAt: null }).broken, false); + assert.equal(publicKey({ ...goodKey(), expiresAt: '' }).broken, false); +}); + +// The boot warning exists because a bearer 401 is logged nowhere. A record killed by an +// unreadable expiresAt was the one class it did not name. +test('the boot warning names a record with an unreadable expiresAt', async () => { + const lines = []; + const warn = console.warn; + console.warn = (msg) => lines.push(msg); + try { + await createAuthStore( + stubStorage({ + version: 1, + keys: [goodKey(), { ...goodKey(), id: 'k_junk', expiresAt: 'garbage' }], + }), + { apiKey: 'bootstrap', baseUrl: 'http://localhost:3000' }, + ); + } finally { + console.warn = warn; + } + + const named = lines.filter((l) => l.includes('k_junk')); + assert.equal(named.length, 1, `expected one warning naming k_junk, got ${JSON.stringify(lines)}`); + assert.match(named[0], /expiresAt nothing can read/); + assert.equal(named[0].includes('k_good'), false); +}); + // A name that is not a string reached the dashboard as "undefined · " in the row title. test('publicKey names an unnamed record rather than passing undefined through', () => { assert.equal(publicKey({ id: 'k_bad' }).name, '(unnamed)');