Skip to content

feat(auth): scope and expire system API tokens, and fix two secret-handling bugs (fixes #217) - #250

Merged
mikeknight85 merged 4 commits into
mainfrom
feat/system-token-scopes
Oct 11, 2026
Merged

mikeknight85 merged 4 commits into
mainfrom
feat/system-token-scopes

Conversation

@mikeknight85

Copy link
Copy Markdown
Owner

Implements #217, following @stevene1919's decision on that thread exactly: backfill-as-* with a deprecation grace period rather than a hard break.

Two real bugs found beyond the brief

1. The token creation response was returning the token's bcrypt hash. system_api_token.repository.ts does INSERT ... RETURNING *, and routes/admin/tokens.ts sends systemToken: result.systemToken straight to the client. So every token creation leaked token_hash. CLAUDE.md is explicit that secrets are write-only over the API — this was a direct violation sitting in main.

2. || 1 had a real caller. I expected to find none. backend/src/scripts/generate-api-token.ts passed admin_id: undefined, so every token minted by the documented CLI route relied on the fallback to silently become user 1. Rather than just breaking that script, it now takes --user <email> (validated as an admin) or links to the longest-standing administrator and prints which. createSystemToken's admin_id is now a required parameter, so the invariant is type-enforced rather than hoped for. Unlinked tokens already in a database are refused with a 401 and a logged warning naming the label.

Scope vocabulary

Seven entries. mcp:read / mcp:write / mcp:extract come straight from ADR 001 §4.1 rather than being invented around it. The three admin:* entries exist because the ADR's list has nothing expressing "read the admin tree but do not run migrations" — which is the exact harm described in the issue.

scope covers
* everything; what legacy tokens hold, deprecated for new ones
mcp:read read-only product/price/history plus admin diagnostics
mcp:write mutating the tracked catalogue
mcp:extract POST /api/admin/debug/* — the arbitrary-URL SSRF surface from #165
admin:read / admin:write reading / mutating instance config
admin:system POST /api/admin/command including run-migration

Namespace wildcards like admin:* are deliberately not grantable — a grant that widens as the vocabulary grows hands a token privileges nobody consented to at issue time. Routes accepting more than one scope list them as an OR, so mcp:read satisfies admin reads without any implication logic.

Two judgement calls:

  • requireScope is a no-op for non-system tokens. Scopes describe machine credentials; a JWT grants none, so enforcing them on browser sessions would lock out every user. Pinned by a test.
  • It is applied beyond /api/admin, method-aware. Otherwise mcp:read would be a lie: a "read-only" token could DELETE /api/products/:id, or PUT /api/profile to change the linked admin's email and then trigger a password reset to it. Profile writes require * for that reason. Legacy * tokens and human sessions are unaffected.

Prefix design, with the security reasoning

token_prefix varchar(16) holding ps_ plus 8 hex, partial index WHERE token_prefix IS NOT NULL. The prefix narrows the candidate set; bcrypt still decides every match.

  • Secrecy: 8 hex is 32 bits of a 256-bit crypto.randomBytes(32) secret. 224 bits remain, still behind bcrypt. A prefix lets an operator identify a token they already hold — which is the point, the same job ghp_ does — and does not help guess one. Safe to show in the UI; token_hash still never leaves the service.
  • Collisions: no UNIQUE constraint, on purpose. A unique index would turn a 1-in-8600-at-1000-tokens event into randomly failing token creation. findByPrefix returns a set and bcrypt-compares each; tested with two rows sharing a prefix.
  • Timing: an indexed equality on a non-secret substring reveals only whether some token with that prefix exists, and you need a valid prefix to probe it. The meaningful win runs the other way: a bogus token previously cost one bcrypt per row in the table — a CPU-exhaustion vector on an unauthenticated path. The fallback scan is now restricted to token_prefix IS NULL rows only, so the cost of garbage input shrinks to zero as legacy tokens are replaced.

Also included: expiry

The binding comment asked for it. The column already existed but was never surfaced; it is now settable (30/90/365/never), and refused in authMiddleware before scopes or admin status, with a clear Token expired. NULL for existing rows.

Migration 023, tested against real Postgres

PostgreSQL 16.15 on a throwaway Homebrew cluster. Full 000→023 chain applies clean; down drops both columns and the index; a legacy row inserted before the migration backfills to {*} with a null prefix; re-running with the columns present reports 0 backfilled and — importantly — does not promote a post-migration {mcp:read} row to *, because the guard is token_prefix IS NULL AND cardinality(scopes) = 0. Down-then-up with data present works and tokens keep authenticating. The real service was also round-tripped against that database, confirming no token_hash in any response.

Renumbered from 022 to 023 — #249 took 022 and merged first. The runner tracks by filename so both would have run, but sequence matters and this had not shipped.

Admin UI

Legacy tokens (token_prefix === null) render a DEPRECATED / UNSCOPED / FULL ACCESS badge with "Issued before scopes. Regenerate to restrict it.", and a banner above the table counts them and explains they cannot be narrowed in place. Status now distinguishes EXPIRED from ACTIVE. Creation fetches the vocabulary from the API as a single source of truth, defaults to mcp:read, and the token is still shown exactly once.

Verification

tsc clean. 877 backend tests (836 on main, +41) and 66 frontend. Playwright 20 passed on private port 4403, bundle freshness proved by grepping the built admin chunk for the deprecation badge text. No as any added — the only one in the diff is the deleted listAll() as any[].

For whoever picks up #163

ADR 001 §4.1 and §4.2 are now implemented. I left the ADR untouched, but its status line and those two sections could use a "landed in #217" marker.

System API tokens carried no authorisation information. A token minted for
one integration reached every /api/admin route, run-migration included --
missing granularity rather than an admin bypass, since admin.ts still
checks is_admin against the database. Verification was also an O(n) bcrypt
scan over every row on every request presenting a non-JWT bearer token,
plus a discarded listAll() cast `as any[]`.

Migration 022 adds `scopes text[]` and an indexed, non-secret
`token_prefix`, backfilling existing rows with '*' so nothing in the wild
breaks. The prefix cannot be backfilled -- only the bcrypt hash was ever
stored -- so null means "legacy", and verification keeps the scan for
exactly those rows while new tokens resolve through the index to a single
bcrypt comparison. Each successful legacy verification logs a deprecation
warning naming the token's label.

requireScope / requireScopeByMethod sit beside adminMiddleware and
constrain system tokens only; '*' behaves exactly as today. Minting a
token now requires '*', so a narrow token cannot widen itself.

authMiddleware no longer resolves `admin_id || 1`: a token linked to no
user is refused instead of silently acting as user 1. The token CLI script
resolved that way, so it now links to --user or to the longest-standing
administrator, and takes --scopes.

Expired tokens are refused before scopes or admin status are considered,
and the creation response no longer carries the bcrypt hash.
The API Tokens panel showed every token as ACTIVE with no indication of
what it could do. It now lists each token's scopes and its public prefix,
marks a token issued before scopes existed as DEPRECATED / UNSCOPED / FULL
ACCESS with a banner explaining that it cannot be narrowed in place and
has to be replaced, and distinguishes EXPIRED from ACTIVE.

Creation offers the scope vocabulary (read from the backend, so the two
cannot disagree) defaulting to mcp:read, and an expiry choice defaulting
to 90 days. Selecting full access clears the narrower scopes rather than
showing both. The token itself is still shown exactly once.
Records the scope vocabulary, what a deprecated unscoped token is and how
to replace it, and the expiry behaviour. Also corrects two stale entries:
the token CLI invocation (it never took the documented `--user <username>`
form) and the system_api_tokens column list in DATABASE.md, which named a
`token` and a `user_id` column that do not exist.
022 was taken by the default-check-interval migration (#249), which merged
first. The runner tracks applied migrations by filename, so two 022s would
both have run -- but CLAUDE.md says number the next migration in sequence,
and this one has not shipped, so renaming is still allowed.
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