Skip to content

review-checklist: add "authorization gates key on provenance, not credential format" — earned by the #1405 /refresh bypass #1411

Open

Description

@lilyshen0722

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions