Skip to content

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

Merged
kuyazee merged 2 commits into
mainfrom
task/key-broken-expiry
Aug 12, 2026
Merged

fix: a key killed by an unreadable expiresAt now reads as broken (T1.2.21)#46
kuyazee merged 2 commits into
mainfrom
task/key-broken-expiry

Conversation

@kuyazee

@kuyazee kuyazee commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

The item

T1.2.21, backlog 1 epic 1.2, filed 2026-08-11 from the T1.2.20 UX lens.

T1.2.20 made a key with an unreadable expiresAt answer 401. That is the right answer, and it said
nothing to anyone. usableKey reads hash and scopes only, so publicKey's broken flag stayed
false and the boot warning never named the record. publicKey also coerces a non-string expiresAt
to null, so {}, true and [] drew as a key with no expiry at all, and "garbage" drew as an
expiry date. Both clients offered Disable, which changes nothing.

The operator's loop was: key 401s, open the key screen, see a healthy key, press Disable, nothing
changes, hand-edit auth.json or re-mint without ever learning why.

What changed

  • keyExpiryUnreadable in lib/auth.js is the test: an expiresAt that is present and cannot be
    read as a date. publicKey folds it into broken, which both clients already render, so the row
    loses Disable, keeps Revoke, and says the key always answers 401.
  • A second boot warning names those records, separately from the no-hash-or-scopes line, because the
    fix is different: a value in the file rather than a missing field.
  • keyExpired and keyExpiryUnreadable both refuse a non-string before calling Date.parse.
  • Both warnings quote the ids they print and fall back to the record position when the id is not a
    string, matching what GET /api/keys is able to list.
  • The Revoke dialog says something true for a broken record, and artifacts keys list drops the
    rest of the line the way the dashboard does.
  • docs/auth.md carries the second warning.

An expiry that has simply passed is untouched. It reads back, both clients print the date, and the
key stops working for the reason it says.

One change beyond the item text

keyExpired is T1.2.20's function and this PR edits it, one line. Three lenses reproduced the same
hole independently: Date.parse stringifies its argument, so 2020, 0 and ["2020-01-01"] all
read as valid past dates. The key was rejected and drawn as healthy, which is the exact loop above,
so flagging it in the UI without fixing the parse would have put a live-looking key in the broken row
or left a dead one out of it. Both functions now treat any non-string as unreadable, so
broken implies 401 by construction. parseKeyInput only ever writes an ISO string, so nothing an
API caller can send changes behaviour.

Review

Four lenses, because the diff touches lib/auth.js. Fourteen findings, seven fixed here, one filed,
the rest verified clean.

Fixed here, each reproduced first:

  1. A non-string that Date.parse reads reproduced the original bug. Adversarial, security and
    QA, independently, all rating it the top finding. Measured on a live instance below.
  2. The boot warning could name a record no client can show. Adversarial. k.id || 'entry i'
    prints a truthy non-string id, while GET /api/keys lists only records whose id is a string, so
    the operator went looking for a row that was not on the screen. Both warnings now use the same
    rule the route does. The pre-existing warning had the same shape and is fixed with it.
  3. k.id was interpolated raw into a console line. Security, low, and mitigated by needing
    write access to auth.json in the first place. An id holding a newline split the warning into
    three lines, the middle one indistinguishable from the real admin account created from env
    line. Both warnings quote now.
  4. The Revoke dialog argued against the action the row had just recommended. UX. The row says
    "always answers 401. Revoke it and mint a new key.", then the dialog said "Any CLI or MCP client
    using this key stops working." Nothing is using it.
  5. artifacts keys list printed the junk as if it were a date. QA, UX and adversarial. The
    flags array was joined rather than replaced, so a record read
    [broken, always answers 401, expires garbage, never used]. The dashboard already suppressed the
    rest of the line; the CLI now matches.
  6. Both boot warnings ended with the same sentence, and it named a button that does not exist.
    UX. Neither client has Delete for a key; both have Revoke.
  7. docs/auth.md documented one warning class and quoted it verbatim. QA, security and UX. An
    operator whose record has both a hash and scopes read that and concluded the problem was
    elsewhere.

Filed as T1.2.27, because it changes the shape of what the key API returns rather than patching
behaviour: a broken record says it is broken and never says why. {}, true and a hashless record
all render byte for byte the same, and the row drops the scopes, the prefix and the last-used date
to make room for the words, so the prefix that would match the row to the key in a CI config is
gone. For a record broken only by its expiry all three are valid. Raised by the UX lens as three
separate findings that share one fix.

Verified clean by the security lens, with proofs: the GET /api/keys gate is still admin-only, so
the flag leaks nothing new; publicKey still never carries the hash out; broken is a pure OR, so
it can only add records, never remove one; no key is hidden; no live key can be marked broken, since
unreadable implies expired; and the bearer path's three functions are untouched apart from the
one line above.

Tests

  • npm test: 49 to 52. Three new cases covering eight junk shapes (each checked for both broken
    and a 401), a readable expiry past and future, and the boot warning naming exactly the junk
    record and not the healthy one.

  • bash .github/workflows/smoke.sh http://localhost:3421 test: 154 ok-lines, all pass, same count
    as origin/main.

  • ci.yml gains a fourth planted record in the step T1.2.19 added, checked by id rather than by a
    grep any broken record satisfies, then revoked over the API.

  • Six records planted in auth.json on two local servers. artifacts keys list:

    record expiresAt origin/main this branch
    ci-deploy "garbage" expires garbage, never used broken, always answers 401
    backup-runner {} never used broken, always answers 401
    old-laptop 2020 never used broken, always answers 401
    last-quarter a real past date expires 2020-01-01, never used unchanged
    half-written no hash broken, always answers 401 unchanged
    healthy-key none never used unchanged

    Boot warnings on the branch, both classes and disjoint counts:

    auth.json: ignoring 1 key record(s) with no hash or no scopes ("k_nohash"). Revoke them on the key screen and mint new ones.
    auth.json: 3 key record(s) carry an expiresAt nothing can read ("k_junkstr", "k_junkobj", "k_numyear"). They return 401 from here on.
    

    On main only the first line printed.

  • Browser at 1280x900 on the same instance: the three junk rows show broken record with Revoke
    only, last-quarter keeps its date and both buttons, the Revoke dialog on a broken row reads
    "This key already answers 401 for every request. Revoking removes the record.", and the page logs
    0 console errors and 0 warnings.

Screenshot: https://artifacts.zonily.cloud/a/1decmlu0x1

Merging

Checked with git merge-tree. This branch merges clean with #43, #44 and #45, in any order.

T1.2.20 made a key with an expiresAt nothing can read answer 401. Nothing told the operator. publicKey coerces a non-string expiresAt to null, so a record carrying {}, true or [] drew as a key with no expiry at all, and "garbage" drew as an expiry date; both clients offered Disable, which changes nothing, and the boot warning named neither.

keyExpiryUnreadable is the test, and publicKey folds it into the broken flag the dashboard and `artifacts keys list` already render, so the row loses Disable, keeps Revoke and says the key always answers 401. A second boot warning names the records, separately from the no-hash-or-scopes line because the fix is different: a value in the file rather than a missing field.

An expiry that has simply passed is untouched. It reads back, both clients print the date, and the operator can see what happened.
Four lenses on the first cut. What changed here:

- keyExpired and keyExpiryUnreadable both refuse a non-string before asking Date.parse. Date.parse stringifies first, so 2020, 0 and ["2020-01-01"] all read as past dates: the bearer path rejected the key while publicKey dropped the value to null, and the screen drew a working key with no expiry. That is the loop this item exists to close, and it survived the first cut.
- Both boot warnings quote the ids they name and fall back to the record position when the id is not a string, which is the same rule GET /api/keys uses to decide what it can list. Naming a record by an id no client shows sent the operator looking for a row that is not there.
- The second warning stops repeating the first one's closing sentence, and both point at Revoke, which is the button that exists.
- The Revoke dialog no longer warns that clients will stop working when the row just said the key answers 401 for everything.
- artifacts keys list drops the rest of the line for a broken record, the way the dashboard already does, so "expires garbage" cannot sit next to "always answers 401".
- ci.yml plants a whole record with one unreadable field alongside the three shapes it already plants, checks that row's broken flag by id rather than by a grep that any broken record satisfies, and revokes it over the API.
- docs/auth.md carries the second warning and says what separates it from an expiry that has simply passed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant