fix(api): retire the placebo scopes and make token issue prove itself - #115
Merged
Conversation
`threads:write` and `admin:read` were offered by the issue form and required by no route: ticking them granted nothing. Both are removed from SCOPES, so the form cannot offer them; a token already stored with one loses it as the row is read, because `parseScopes` filters through `isScope`. A malformed "Expires in (days)" fell through to `expiresAt = null` — a token that never expires. It is now refused with a form error and mints nothing; an empty field still means no expiry, as the label says. Issuing a bearer credential now needs `requireFreshAdmin()`, like every other destructive admin operation and like the panel's own promise. Revoking keeps `requireAdmin()`: it is the containment step during an incident, and it is undone by issuing again. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014kP43A5shJWmAKqkBovhaF
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the three audited defects on the API-token screen (MEI-52). They live in the same two files, so they ship together.
1. Placebo scopes (finding B)
SCOPESofferedthreads:writeandadmin:read;ROUTESrequires neither, and never has. A token issued with only those two passed creation and could call nothing — a checkbox that reads as a permission and is not one.Decision: removed, not implemented. Inventing REST endpoints to justify a checkbox is a feature, not a bug fix, and the scopes that do exist enforce genuinely — none of that enforcement is touched.
What that means for tokens already stored with them: nothing breaks.
PostgresApiTokenRepositoryparsesscopesthroughisScopeon every read (parseScopes,packages/db/src/api-repo.ts), so a row containing["forums:read","threads:write","admin:read"]loads as["forums:read"]— no throw, no 500. The token keeps authenticating and keeps the scopes that still exist; anything it reached only through a retired scope would answermissing_scope, and nothing did, because no route consumed one. The panel's scope column and the authenticating path go through the same filter, so they cannot disagree.Scopeis not used in a discriminated union or an exhaustive switch, so nothing else followed.2. Expiry failed open (finding F)
issueApiTokenActiondidNumber(field(...))and treated anything failingNumber.isSafeInteger(days) && days > 0as "no expiry" — so30.5,abcand30 dayssilently minted a token that never expires, rounding a typo to the most dangerous outcome.Now a non-empty value must be digits only and a safe integer above zero, or the action returns a form error and mints nothing (no token, no admin-log entry, no revalidation). An empty or whitespace-only field still means no expiry, which is what the field's own label promises.
1e2is refused rather than quietly read as 100 days.3. Re-authentication inconsistency (finding E)
Issuing a bearer credential — optionally never-expiring, optionally carrying
posts:write— required onlyrequireAdmin(), whilebanMemberAction,mergeStepAction,pruneMembersAction,startMassMailAction,moveForumAction,copyForumPermissionsAction,deleteGroupAction,moveMembersActionandapplyPromotionsActionall demandrequireFreshAdmin(). The admin layout promises "Anything destructive will ask again". Minting an out-of-band credential is at least as destructive as moving a forum, soissueApiTokenActionnow callsrequireFreshAdmin().Revoke deliberately still uses
requireAdmin(). Revoking is the containment step — the thing an operator does the moment a token leaks — and putting a password prompt in front of it buys nothing an attacker cares about while slowing down the defender. It is also recoverable: the cost of a mistaken revoke is issuing a new token, not lost data. The asymmetry is now documented rather than accidental.Docs
docs/rest-api.mdis generated, so the prose changes are inscripts/api-docs.mjsand the page was regenerated withpnpm api:docs(7 endpoints, 6 scopes;pnpm api:docs:checkgreen). It now states that every scope is consumed by a route, that retired scopes degrade rather than break, and — in a new "Issuing a token" section — that issuing re-asks for the password while revoking does not, and what the expiry field accepts. The form's own hint text says the same thing next to the field.docs/operating.mdhas no API-token section to update;rest-api.mdis where tokens are documented.Tests
All new tests were proved to fail against the old behaviour and pass against the new one.
packages/api/src/api.test.ts— SCOPES ⊆ the scopes ROUTES actually requires, so a future dead scope cannot be added silently;isScoperefuses the two retired names; the "no administrative scope" pin updated.apps/community/src/server/api-token-actions.test.ts(new) — issue asks for fresh proof and revoke does not; a stale proof mints nothing and logs nothing; seven malformed expiries (30.5,abc,1e2,-7,0,9007199254740993,30 days) are each refused with a form error and mint nothing; an empty field and an absent field both mint a non-expiring token; a valid30lands 30 days out.packages/db/src/client.pg.test.ts— against real Postgres, a row stored withthreads:writeandadmin:readstill loads, carrying onlyforums:read, through bothfindByLookupandlistAll.Red-then-green check: restoring
threads:writetoSCOPESturns the scope-set test and the stored-token test red; restoring the oldrequireAdmin()+Number()expiry parse turns nine of the fifteen action tests red.Validation
pnpm verify— green (only the two warnings already onmain).packages/db/src/client.pg.test.tsrun against a real PostgreSQL 16 withTEST_DATABASE_URL— 8 passed; it is skipped without one, and CI supplies it.DATA_SOURCE=fixture pnpm build— exit 0 (a"use client"component changed; it still value-imports no@meith/*package).pnpm test:e2enot run. The existing API specs sign into the panel with a password immediately before issuing, so the fresh-proof clock is satisfied.🤖 Generated with Claude Code
Generated by Claude Code