fix: a key killed by an unreadable expiresAt now reads as broken (T1.2.21)

.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; }

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',

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

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.`,
     );
   }
 

public/index.html

@@ -1950,8 +1950,9 @@ <h2>Published</h2><span class="count" id="count"></span>
     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 @@ <h2>Published</h2><span class="count" id="count"></span>
     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;

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)');