Repository navigation
review-checklist: add "authorization gates key on provenance, not credential format" — earned by the #1405 /refresh bypass #1411
Description
Reviewer-checklist rule owed, from a live incident on #1405. Filing rather than PRing because the incident is still being fixed and the rule should cite a settled outcome — but the pattern is general and I do not want it to evaporate into a chat log.
The rule
Authorization gates key on provenance, never on credential format. When a guard says "shape X implies trusted," the review question is not whether the check is correct — it usually is — but who else can mint an X. Enumerate the minting sites, not the checking sites.
Belongs in docs/development/review-checklist.md under Guards and predicates, as a sibling to rule 8 ("Collapse aliases at the semantic layer, not the transport layer"). Rule 8 says keys derive from the thing that is the identity, not the transport that carried it; this is the authorization-side twin: gates derive from how a credential was obtained, not what it looks like.
The incident
#1405 adds CLI device-code login. A device bearer (cm_) was able to authorize another device, minting a successor credential — an escalation from a narrow, individually-revocable principal to a broader one. The fix at e17b07e7 added:
if (req.authType !== 'jwt') {
return res.status(403).json({ error: 'Device authorization requires a signed-in browser session' });
}Correct as written, and it works. But authType === 'jwt' is the token's shape, and the comment reads it as "an interactive browser session." POST /refresh takes bare auth and signs a fresh 7-day JWT off req.userId without consulting authType, so the weaker principal simply mints the stronger shape and walks back through the gate. Measured end-to-end at that head:
device bearer -> /device/authorize -> 403 (control: the gate works)
device bearer -> POST /refresh -> 200 JWT issued
laundered JWT -> /device/authorize -> 200 authorized
attacker polls second bearer -> GRANTED
Found code-level by @pod-architect; reproduced against the harness here.
Why it belongs in the checklist rather than in the PR alone
Three reviewers (myself included, three times) passed the original escalation. What made it survivable was that every local reading was correct: the middleware correctly reports the format, the gate correctly checks the format, and /refresh correctly refreshes a session. The defect is only visible when you ask which other routes produce the value being checked — a question no single-file review asks by default.
This is the same family as rule 7's phantom cross-layer contract: a promise held together by a comment ("jwt means browser session") that no type enforces and no test covers. It differs in direction — rule 7 is about a value nobody sets; this is about a value too many places set.
Durable fix shape (also worth recording)
Consistent with rule 7's "make the dependency a value the caller must pass, not a convention the caller must remember": carry provenance in the credential rather than inferring it. Stamp the issued JWT with its origin ({ id, src: 'password' | 'device' | 'refresh' }) and have privileged gates require an origin, not a format. Refusing at /refresh closes this specific hop but leaves the unenforced equivalence standing for the next minting route someone adds.
Blocked on
#1405 landing a fix, so the rule can cite a resolved incident with a stable sha.
Activity