security(oauth): OAuth 2.1 / MCP authorization-server hardening (#516)#693
Merged
Conversation
- Refresh-token reuse response (OAuth 2.1 §6.1 / RFC 9700 §4.14.2): a genuine, already-rotated refresh token replayed is now detected and revokes the whole live session family, not a no-op invalid_grant. Rotation itself already retired the old token; this adds the breach response. Reuse is only flagged for a signed, client-bound refresh token whose session record is gone, so a forged/foreign token can't force a revocation. - RFC 8707 resource indicator on the authorization_code flow: optional single `resource` at /authorize is bound to the code, must be echoed and matched at /oauth/token (invalid_grant on mismatch), and sets the access token `aud`. id_token / refresh_token audiences stay the client. - RFC 8414: /.well-known/oauth-authorization-server alias of the OIDC discovery handler; advertise resource_indicators_supported. - --oauth21-strict flag (default false, opt-in). When on, reject the implicit/hybrid-with-token response types and PKCE plain (S256 only), reflected in code_challenge_methods_supported. Off is unchanged. Refs #516
…mily Fix four findings from the adversarial review of PR #693. Fix 1 (HIGH) — refresh-token reuse was an unauthenticated forced-logout DoS. On reuse detection the token endpoint called DeleteAllUserSessions(userID), wiping every session of the user across every login method. Since the refresh grant needs no client secret, anyone holding any retired refresh token (logs, history, backups) could keep a victim perpetually logged out. Approach taken: full family-scoping via a per-lineage family record — NOT the grace-window+cooldown fallback. Investigating the memory_store backends showed the subkey-encoding sketch (refresh_token_<family>_<nonce>) would force family_id into access/session tokens + validation and ~40 Set/Get/Delete call sites — far more invasive than described. Instead: - Refresh tokens carry a stable family_id claim, minted once at first issuance and carried forward unchanged across every rotation (nonce still rotates). Generation lives in CreateRefreshToken, so every issuance path gets one for free; family_id is reserved against custom-script forgery. - On rotation the handler writes a family index record ("refresh_family_<familyID>" -> {live, prev, rotated_at}) via the existing SetUserSession — no new interface method, works uniformly across the in-memory, redis and db backends. - On reuse, revocation deletes only that family's live session via the existing DeleteUserSession(sessionKey, liveNonce); other lineages and login-methods of the same user are untouched. A family-less (legacy) or live-lineage-absent replay revokes nothing — never the whole user. - Grace window (10s): a replay of the immediate predecessor within the window is a benign double-submit (multi-tab/retry), not a breach — the live session is not revoked. Any older nonce, or a predecessor replayed after the window, is genuine reuse and revokes the family. Fix 2 (MEDIUM) — strict mode now rejects every response_type delivering a bearer token into the fragment. The check is component-exact on the canonical response_type, catching the front-channel-token hybrids "code token" and "code id_token token" alongside "token"/"id_token token", while leaving pure "id_token" (substring only) untouched. Fix 3 (LOW) — the /authorize resource parameter is validated per RFC 8707 §2 (absolute URI, no fragment); an invalid value is rejected with invalid_target. Fix 4 (LOW/MEDIUM) — ValidateAccessToken rejects a token whose aud is a resource indicator (absolute URI other than the default audience) at Authorizer's own protected endpoints (/userinfo, GraphQL, gRPC). Introspection keeps its own ParseJWTToken path and is unaffected, so an external RS can still validate resource-bound tokens. Tests: family-scoped revocation kills only its lineage (unrelated same-user session survives); grace window benign within / breach after; strict-mode rejection of the two hybrids plus pure-id_token allowance; resource validation; and the aud restriction at /userinfo. All pre-existing OAuth 2.1 tests updated and passing.
"oauth21" reads ambiguously (looks like a made-up version 21); the dash between "2" and "1" makes it unambiguously OAuth 2.1. Go field name (OAuth21Strict) and test file names are unaffected - Go identifiers can't carry dots/hyphens anyway.
Found in a fresh adversarial re-review of the family-scoped reuse fix. handleRefreshTokenReuse couldn't tell "this nonce was genuinely rotated away" from "the store had a transient blip looking up the CURRENT live token's own session entry" - GetUserSession collapses both to a plain error. Without a guard, a transient lookup failure on a still-legitimate live token fell straight into the genuine-reuse branch and revoked the user's own session over what should have been a retryable fault. Fix: if the presented nonce equals the family record's LiveNonce, this can't be a rotated-away token by definition - skip revocation and let it fail as an ordinary invalid_grant instead. Regression test simulates the failure by expiring just the refresh_token_ <nonce> store entry (not via DeleteUserSession, which cascades to the sibling session/access token entries for that nonce and would mask the bug) and asserts the sibling access_token_<nonce> entry survives. Verified the test actually catches the regression: temporarily disabled the guard, confirmed the test fails (access token wiped), restored it.
pull Bot
pushed a commit
to bhardwajRahul/authorizer
that referenced
this pull request
Jul 22, 2026
Added to README: - WebAuthn / passkey login - Enterprise SSO (SAML 2.0 IdP & SP, OIDC broker, verified domains, home realm discovery) - SCIM 2.0 provisioning - Multi-tenant / org-scoped admin - Migration guides (planned) Added to CHANGELOG [Unreleased]: - Service accounts as FGA subjects (authorizerdev#665) - SAML ACS CSRF exemption fix (authorizerdev#666) - Trusted-issuer token review config (authorizerdev#667) - Session revocation on password reset (authorizerdev#669, authorizerdev#673) - Multi-tenant SSO phases (authorizerdev#672, authorizerdev#674, authorizerdev#675) - OIDC discovery caching + Twitter PKCE fix (authorizerdev#668) - WebAuthn passkey support (authorizerdev#671) - Per-user TOTP lockout + hashed recovery codes (authorizerdev#670) - Server-side user search + org membership (authorizerdev#678, authorizerdev#680) - Per-method MFA availability signals (authorizerdev#681) - Consolidated MFA redesign narrative (authorizerdev#682-686) - SAML 2.0 IdP role (authorizerdev#691) - OAuth 2.1 / MCP hardening (authorizerdev#693) - SCIM group provisioning + FGA + SAML assertions (authorizerdev#694) - Provider template parity (authorizerdev#695) - Recent fixes (async tracking, AGENTS.md rules, security fixes, storage parity, error typing, panics, frontend fixes) (authorizerdev#696-702)
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.
Summary
Closes #516 (RFC: OAuth 2.1 Authorization Server for MCP), scoped to what the current MCP spec (2025-11-25) and codebase actually require rather than the issue's original text, which was written against an older spec revision.
invalid_grant.resourceon the primaryauthorization_codeflow. Previously only existed inside the token-exchange/delegation path./authorizenow accepts and bindsresource;/oauth/tokenrequires it be echoed and matched, and sets the access token'saudto it (id_token/refresh_token audiences stay the client, per OIDC/RFC 6749 §6).GET /.well-known/oauth-authorization-server— thin alias of the existing OIDC discovery document (not a compliance gap; MCP clients probe this path first).--oauth21-strictflag, default off. Gates implicit-grant removal and PKCE-plain removal behind opt-in — zero behavior change for existing deployments unless explicitly enabled.Explicitly out of scope (see PR discussion / linked issue for citations): RFC 7591 Dynamic Client Registration (downgraded to MAY in the current MCP spec), OAuth Client ID Metadata Documents (early WG draft), RFC 9728 Protected Resource Metadata (that's the MCP Resource Server's responsibility, not the AS's —
examples/with-mcpalready demonstrates the boundary correctly).Judgment calls flagged for reviewer attention
reset_password/deactivate_accountrevocation.refresh_tokengrant — reverts to client-id audience after the first refresh. Noted as a deferral, not a silent gap.Test plan
internal/integration_tests/oauth21_mcp_test.go(5 tests / 10 subtests) — resource/aud binding, mismatch rejection, refresh-reuse-revokes-family, strict-mode gating (default-off regression + on-behavior), AS metadata.token_audience,refresh_token_client_binding,oidc_hybrid,oauth_standards_complianceregressions all pass).make test-all-db— all 7 DB backends (couchbase, postgres, sqlite, mongodb, arangodb, scylladb, dynamodb), 31 packages, 0 failures.go vet ./...clean,golangci-lint run0 issues on touched packages,gofmt -sapplied.