feat(api): draw the OAuth admin boundary around durable privilege - #244
Open
Waiel5 wants to merge 4 commits into
Open
feat(api): draw the OAuth admin boundary around durable privilege#244Waiel5 wants to merge 4 commits into
Waiel5 wants to merge 4 commits into
Conversation
This deployment already issues OAuth access tokens, but only `/mcp` accepted them. `/api/*` string-matched the `Bearer sk_` API-key prefix and rejected anything else, so a client could complete authorization, be granted `email:read`, and then find that the scope reached nothing. A third-party or native client had no usable path to the API at all. Tokens become a third credential alongside the session cookie and API keys. **One resolver, both surfaces.** Access tokens verify offline against the JWKS, so a signature proves only that this deployment minted the token at some point; everything that can revoke a live one — disabling the client, deleting or banning the user, removing their passkey — lives in the database and must be re-checked per request. `/mcp` already did all of that. Rather than write it twice, `resolveOAuthPrincipal` holds it and both callers use it, so the two bearer surfaces cannot drift apart. `/mcp` keeps its narrower audience and its `WWW-Authenticate` challenge; its handler loses about 2,700 characters of inline validation and behaves identically. **Scopes, not audience, are the boundary.** No `/api` audience is introduced. `@better-auth/oauth-provider` at the pinned version is subject to GHSA-p2fr-6hmx-4528, where the authorization-time resource is dropped and the token endpoint will mint a token for another allowlisted audience — so a distinct API audience would look like an authorization boundary without being one, which is worse than not having it. Scopes do the work instead, checked per route. **The policy classifies on method plus exact path.** Three routes send mail from under a router whose other routes do not — template send, sequence enroll and outbox retry — so a prefix rule would file them as `email:manage` and let a client that was never granted `email:send` send mail. Anything unclassified is denied, so adding a route without classifying it breaks an integration rather than quietly widening every existing token. **Two things are closed to tokens outright.** The credential surface (`/api/api-keys`, `/api/user/passkeys`, `/api/auth/*`), because minting an unscoped API key would convert a narrow mail consent into the user's whole account and destroy whatever key they already had. And a set of admin operations that escalate the principal or open a standing channel: rewriting inbox assignments (a token could grant itself every inbox, then read them), changing a user's role, minting or listing invite tokens, revoking OAuth clients, repointing the webhook, and setting an inbox's `forwardTo`. These are a different risk from an admin doing the same in a browser — a token is held by software, acts with no human present, and may be compromised without anyone noticing. `admin:manage` is for operating the deployment, not for rewriting who may operate it. Session and API-key callers are untouched and remain unscoped; the scope middleware returns immediately unless the request authenticated as `oauth`. Also here because the same clients need them: - `admin:manage`, never implied and required in addition to `role === "admin"`. - `GET /api/user/me`, so a client can decide whether to offer admin screens without probing an admin route and reading the 403. - `Authorization` in the CORS allow-list. Hono's default is empty, so a cross-origin client that preflighted a bearer request had the header stripped and the request blocked.
The bearer deny list refused nine operations for looking like they rewrote the
deployment. Meanwhile `DELETE /api/admin/users/{id}` deletes an account and
`DELETE /api/blocklist/mail` permanently deletes every hidden message from
blocked senders across every inbox, and both were open at `admin:manage` the
whole time. A line drawn around a category rather than around damage ends up
both too broad and too narrow, and this one did: it cost a client the ability
to rename an inbox while leaving account deletion available.
One question decides membership now: does the route create a privilege that
survives revoking the client and expiring the token, or open a standing copy of
future mail? Nothing else.
**Four open outright.** Revoking an invite destroys a capability and was denied
only because the rule was a `(\/|$)` prefix over the whole invites surface — an
accident of pattern shape. `POST /api/webhook/test` takes no URL; it posts a
synthetic payload to whatever `PUT` already configured, which is guarded below.
`GET /api/oauth-apps` is read-only inventory and
`DELETE /api/oauth-apps/{clientId}` is revocation. Replacing an inbox's
assignments opens for the reason the denial gave for closing it: the caller is
already an admin who can read every inbox, so a self-grant is moot, and the
strictly more destructive account deletion was never denied.
**Four are clamped rather than denied,** because the clamp is what removes the
durable property:
- A bearer-minted invite must be `role: "member"`, must name an email address
(`POST /api/invites/accept` refuses a mismatch), and expires within seven
days, so one call produces a single-target, short-lived, non-escalating
credential. Admin invites stay browser-only.
- `PATCH /api/admin/users/{id}/role` may only demote. De-privileging cannot
escalate, and it is the weaker sibling of a deletion already open.
- An inbox's `forwardTo` and the webhook URL may be cleared but not set. That
returns the display fields on those two routes — the old denial cost a client
the ability to rename an inbox because path alone could not separate them —
and leaves an integration a kill switch it can never use to arm. The webhook
is the sharper of the pair: its payload carries subject and body text, so a
URL set from a token copies every future inbound message to an address of the
caller's choosing, without the token ever holding `email:read`.
**The clamps are allowlists.** A field the guard does not list is refused, which
is the body-level counterpart of `classifyRoute`'s trailing `return denied`: a
field added to one of those schemas next year reaches token callers only once
someone classifies it, and a test enumerates each route's zod shape so that
decision cannot be skipped. The guards run only for `authMethod === "oauth"`;
session and API-key callers are untouched, which is asserted rather than
assumed.
**Two things stay closed.** `/api/api-keys`, because an `sk_` key authenticates
as `authMethod: "apiKey"` — skipping this policy and `requirePasskey` both —
never expires, and is not revoked by `DELETE /api/oauth-apps/{clientId}`, so
minting one buys permanent unscoped access; `/api/user/passkeys` and
`/api/auth/*` are the rest of that surface. And `GET /api/admin/invites`, the
one line of the invites surface that survives: it returns the token of every
live invite, and an admin-role invite created in a browser need not pin an
email, so reading that list can be as good as minting an admin account. No
clamp on creation can un-print a token a human already made.
`/api/oauth-apps` also carried two rules, a deny shadowing an admin, so which
line an edit happened to delete decided the outcome. It has one rule now, and
the test pins the classification rather than the line — as it does for every
path that moved, since `classifyRoute` is first-match-wins and a rule that
lands one line too low fails open with nothing to see in a diff.
An adversarial review of the reclassification proved a chain that crossed the line the policy draws, using only calls the policy permits. Mint a member invite pinned to an address the caller controls — the clamp allows exactly that, and the 201 body carries the token. Redeem it through the public accept route. Then hand the fresh member every inbox. The result is a standing account with mail access that outlives revoking the client and expiring the token: durable privilege, assembled entirely out of allowed steps. The last step is the one that gives, so it is the one that gives way. A bearer caller may now take an inbox away from someone but never grant one. Shrink-only rather than closing the route, because revocation is the de-privileging half and locking someone out from a phone is exactly when that matters; granting joins promotion and admin invites in the browser, for the same reason all three are there. Checked in the handler rather than in `BODY_GUARDS` because the question is not what the body says but how it compares to what is stored, and the guard table is deliberately syntactic with no database behind it. Five tests: adding is refused and writes nothing, clearing works, a no-op replace works, swapping one member for another is refused (the case a naive length comparison would pass), and an API-key admin is unaffected. The pre-existing "replaces inbox assignments" case asserted the behaviour this removes and now asserts the clearing half.
`BODY_GUARDS` marks the webhook `secret` free, with a comment saying rotating it escalates nothing. True, and irrelevant: the route gave the permission nowhere to land, so "free" described a capability the server did not have. Three bodies exhausted `PUT /api/webhook` and none rotated anything. Sending the current URL back with a new secret is refused, correctly — setting a non-empty URL is the exfiltration half. Omitting the URL failed validation, because the schema declared it required. And sending a blank URL was accepted but wiped the whole configuration: the handler branches on the empty URL first and returns before it ever reads `secret`. `url` is now optional, and absent means "leave the destination alone". That cannot arm anything, so the guard refusing a non-empty URL still covers the path it exists to cover. A secret-only PUT against a deployment with no webhook configured is a 400 rather than a silent no-op — there is nothing for the secret to sign. Found by an agent writing the app's webhook screen, which is the useful part: it went looking for the control the policy said it was allowed to draw, could not make the request work, and reported the permission as dead rather than shipping a button that quietly deleted the webhook it claimed to re-key. The existing coverage missed it because it called `guardBody()` directly and asserted the policy function rather than the route. Four tests now go through the API with a real token: rotation keeps the destination, clearing the secret keeps it too, repointing is still refused and leaves the stored URL untouched, and a rotation against nothing configured 400s.
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
1 participant
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.
Stacked on #238 — review the last three commits. Depends on that PR's scope policy existing at all.
Why
#238 refused nine things to OAuth bearer callers. Building a native admin client against it, the line turned out to be drawn by category rather than by damage, and to be wrong in both directions:
DELETE /api/admin/invites/{id}is denied, and it destroys a capability. It was only ever caught by a(\/|$)prefix over the whole invites surface — an accident of pattern shape.DELETE /api/blocklist/mailis wide open atadmin:manage, and it hard-deletes mail and R2 objects across every inbox in the deployment, irreversibly, with no dry-run.The rule
One test, stated in the file header: does this create a durable privilege, or open a standing exfiltration channel? Durable meaning it survives revoking the client and expiring the token.
Where an operation failed the test only because of how much it could grant, it is clamped rather than closed — the clamp is what removes the durable property:
POST /api/admin/invitesrole: "member"+ pinnedemail+ ≤7 days. One call yields a single-target, short-lived, non-escalating credential.PATCH /api/admin/users/{id}/rolePATCH /api/admin/inboxes/{email}forwardTo. Clearing stays open, so a client keeps a kill switch it can never use to arm.PUT /api/webhookurl. Secret rotation now works (see below).PUT .../assignmentsDELETE /api/admin/invites/{id},POST /api/webhook/test,GET+DELETE /api/oauth-apps/api/api-keys,/api/user/passkeys,/api/auth/*,GET /api/admin/invites/api/api-keysstays shut permanently: ansk_key resolves asauthMethod:"apiKey", which skips the scope middleware andrequirePasskey, has no expiry, and is not revoked byDELETE /api/oauth-apps/{clientId}. One call would buy permanent unscoped access.GET /api/admin/invitesstays shut because it spreads...rowand returns the raw, still-usabletokenof every live invite — including admin-role ones a human created in a browser.Guards are allowlists
BODY_GUARDSrefuses any body key it does not classify, mirroringclassifyRoute's trailingreturn denied. A test enumerates each route's zod schema and asserts every key is either allowed or explicitly refused, so a field added next year is refused to tokens by default. Guards readauthMethodand are invisible to session and API-key callers, asserted directly.Two holes found by review, closed here
Assignments were a composition hole. Every step permitted, and together durable: mint a member invite pinned to an address you control (the clamp allows exactly that, and the 201 body carries the token), redeem it through the public accept route, then hand that member every inbox. The resulting access outlives revoking the client. Assignments are now shrink-only — the analogue of demote-only.
Webhook secret rotation was dead permission. The guard marks
secret: "free", but the route made it impossible: sending the current URL back with a new secret is refused, omitting the URL failed validation (urlwas required), and a blank URL wiped the configuration beforesecretwas read.urlis now optional and absent means "leave the destination alone" — which cannot arm anything, so the guard still covers its path. Found by an agent writing the client's webhook screen, which went looking for the control the policy said it could draw and reported the permission as dead rather than shipping a button that silently deleted the webhook it claimed to re-key.Verification
Non-vacuity checked by mutation: making the invite clamp permissive and moving the invites deny rule one line below the general admin rule made 7 tests fail. Full suite 827 passed / 1 skipped (needs
--fileParallelism=falsehere; the default deadlocks ~60 workerd children at 0% CPU, pre-existing).Known, not fixed here
/api/blocklistis mounted withoutrequireAdmininindex.ts, so this policy is stricter for OAuth than the session path is for anyone. Worth a separate fix; flagging rather than widening this diff.