diff --git a/app/controllers/oauth/authorizations_controller.rb b/app/controllers/oauth/authorizations_controller.rb index e13fdedf98..7aec59607b 100644 --- a/app/controllers/oauth/authorizations_controller.rb +++ b/app/controllers/oauth/authorizations_controller.rb @@ -37,7 +37,11 @@ def new def create if denial? redirect_to error_redirect_uri("access_denied", "User denied the request"), allow_other_host: true - else + # Consent is use, so it restarts a self-registered client's retention + # period: the sweep (Oauth::Client.cleanup) mustn't remove the client while + # the code it is about to receive is still redeemable. A touch that finds no + # row means the sweep got there first, and a code would be dead on arrival. + elsif @client.touch code = Oauth::AuthorizationCode.generate \ client_id: @client.client_id, identity_id: Current.identity.id, @@ -46,6 +50,8 @@ def create scope: Oauth.canonical_scope(params[:scope].presence || "read") redirect_to success_redirect_uri(code), allow_other_host: true + else + oauth_error "invalid_request", "Unknown client" end end diff --git a/app/controllers/oauth/tokens_controller.rb b/app/controllers/oauth/tokens_controller.rb index ee696c2dbb..6756e65235 100644 --- a/app/controllers/oauth/tokens_controller.rb +++ b/app/controllers/oauth/tokens_controller.rb @@ -45,7 +45,7 @@ def create granted = Oauth.canonical_scope(@auth_code.scope) permission = granted.split.include?("write") ? "write" : "read" - if access_token = @identity.access_tokens.redeem(@auth_code, oauth_client: @client, permission: permission) + if access_token = @client.redeem(@auth_code, identity: @identity, permission: permission) render json: token_response(access_token, scope: granted) else oauth_error "invalid_grant", "Authorization code already used" @@ -60,6 +60,8 @@ def create else oauth_error "invalid_grant", "Invalid refresh token" end + rescue ActiveRecord::RecordNotFound + oauth_error "invalid_grant", "Unknown client" end private @@ -138,11 +140,12 @@ def set_refreshable_access_token end # The code or refresh token must have been issued to the client_id in the - # request (RFC 6749 §4.1.3, §6). + # request (RFC 6749 §4.1.3, §6). A grant whose client has been swept away + # (Oauth::Client.cleanup) was issued to no client that still exists. # One issued to another client gets the same answer as a dead one, so # naming a public client tells nobody whether someone else's grant is live. def validate_client_id - unless oauth_client_id == (@client || @access_token.oauth_client).client_id + unless oauth_client_id == (@client || @access_token.oauth_client)&.client_id oauth_error "invalid_grant", authorization_code_grant? ? "Invalid or expired authorization code" : "Invalid refresh token" end end diff --git a/app/models/identity/access_token.rb b/app/models/identity/access_token.rb index dd7ac7b693..62210c8f3a 100644 --- a/app/models/identity/access_token.rb +++ b/app/models/identity/access_token.rb @@ -34,6 +34,13 @@ class Identity::AccessToken < ApplicationRecord before_create :set_expiry_and_refresh_token, if: :oauth_client_id? + # Issuing or destroying a grant restarts its client's retention period + # (Oauth::Client.cleanup). The touch waits until the grant's own transaction + # commits. A revocation locks the grant, and touching the client in the same + # transaction would then wait on the client's lock. A replayed code exchange + # holds that lock while it waits on the grant, so the two would deadlock. + after_commit :touch_oauth_client, on: %i[ create destroy ] + class << self # The token a bearer request may authenticate with: active and honored. # Whether it permits the request's method is the caller's question. @@ -113,6 +120,10 @@ def refresh(permission: self.permission) end private + def touch_oauth_client + Oauth::Client.where(id: oauth_client_id).touch_all if oauth_client_id? + end + def lock_grant self.class.lock.where(id: id).pluck(:id) end diff --git a/app/models/oauth/client.rb b/app/models/oauth/client.rb index 538f55bbfb..77852bc059 100644 --- a/app/models/oauth/client.rb +++ b/app/models/oauth/client.rb @@ -4,10 +4,20 @@ class Oauth::Client < ApplicationRecord SECRET_AUTH_METHODS = %w[ client_secret_post client_secret_basic ] AUTH_METHODS = %w[ none ] + SECRET_AUTH_METHODS - has_many :access_tokens, class_name: "Identity::AccessToken", foreign_key: :oauth_client_id, inverse_of: :oauth_client + # Registration is open, and MCP clients often register afresh on every + # install or reconnect, so a self-registered client left without a grant + # this long is abandoned (RFC 7591 §5). + UNUSED_RETENTION = 30.days + + has_many :access_tokens, class_name: "Identity::AccessToken", foreign_key: :oauth_client_id, inverse_of: :oauth_client, dependent: :destroy has_secure_token :client_id, length: 32 + # A code exchange locks the client and then issues a grant (#redeem). + # Destroying a client takes the same lock before its grants cascade, in the + # same order, so no grant can land after the cascade and be stranded. + before_destroy :lock_client, prepend: true + validates :name, presence: true, length: { maximum: 255 } validates :client_id, uniqueness: true, allow_nil: true # Registration is open and unauthenticated, so what it stores is bounded: @@ -27,6 +37,36 @@ class Oauth::Client < ApplicationRecord scope :trusted, -> { where trusted: true } scope :dynamically_registered, -> { where dynamically_registered: true } + # A grant touches its client when it is created or destroyed, so updated_at + # is the last grant activity: an app the user disconnected yesterday is not + # swept for having registered long ago. + scope :stale, -> { dynamically_registered.where.missing(:access_tokens).where(updated_at: ...UNUSED_RETENTION.ago) } + + def self.cleanup + stale.find_each(&:destroy_if_still_unused) + end + + # The sweep and #redeem take the same row lock, so a grant either lands + # before the recheck, which then spares the client, or finds the client + # already gone and is never issued. There is no foreign key to refuse a + # token for a deleted client. The recheck is the whole stale test, not just + # the absence of tokens: a grant issued and revoked since the sweep picked + # this client has touched it, and restarted its retention period. + def destroy_if_still_unused + with_lock do + destroy if self.class.stale.exists?(id) + end + rescue ActiveRecord::RecordNotFound + # Already gone, deleted since the sweep picked it. + end + + # Raises ActiveRecord::RecordNotFound if the client has been swept since it + # was read. + def redeem(authorization_code, identity:, permission:) + with_lock do + identity.access_tokens.redeem authorization_code, oauth_client: self, permission: permission + end + end def confidential? token_endpoint_auth_method.in?(SECRET_AUTH_METHODS) @@ -54,6 +94,14 @@ def allows_scope?(requested_scope) end private + # Takes the row lock without reloading, since lock! refuses a record with + # unsaved changes and destroy otherwise ignores them. Resetting the grants + # makes the cascade see every grant committed before the lock. + def lock_client + self.class.lock.where(id: id).pluck(:id) + access_tokens.reset + end + def generate_client_secret self.client_secret ||= self.class.generate_unique_secure_token end diff --git a/config/recurring.yml b/config/recurring.yml index 66c8d1a1bf..046489c489 100644 --- a/config/recurring.yml +++ b/config/recurring.yml @@ -39,6 +39,9 @@ production: &production cleanup_imports: command: "Account::Import.cleanup" schedule: every hour at minute 25 + cleanup_oauth_clients: + command: "Oauth::Client.cleanup" + schedule: every day at 04:12 incineration: class: "Account::IncinerateDueJob" schedule: every 8 hours at minute 16 diff --git a/docs/oauth/spec_conformance.md b/docs/oauth/spec_conformance.md new file mode 100644 index 0000000000..fd89a1ad9c --- /dev/null +++ b/docs/oauth/spec_conformance.md @@ -0,0 +1,188 @@ +# OAuth spec conformance and Basecamp parity + +This covers Fizzy's OAuth authorization server (AS) and the API acting as resource +server. It records, for each requirement, whether we conform, where a fix landed, +where it is planned, or why we deliberately depart from it. Basecamp's AS (bc3) is +the reference for parity. Where bc3 has a gap of its own, we don't copy it. + +Snapshot: 2026-10-06, at the heads of the OAuth train, rebased onto main: + +| PR | Branch | Scope | +|----|--------|-------| +| [#2296](https://github.com/basecamp/fizzy/pull/2296) | `oauth` | Code + PKCE, discovery, DCR (loopback), revocation, Connected Apps, availability switches | +| [#3080](https://github.com/basecamp/fizzy/pull/3080) | `oauth-dcr-https` | DCR with https redirects | +| [#3081](https://github.com/basecamp/fizzy/pull/3081) | `oauth-refresh-tokens` | 1h access-token expiry, refresh rotation | +| [#3082](https://github.com/basecamp/fizzy/pull/3082) | `oauth-confidential-clients` | `client_secret_post` and `client_secret_basic`; client authentication at revocation | +| [#3158](https://github.com/basecamp/fizzy/pull/3158) | `oauth-bearer-challenge` | Resource-server Bearer challenge (on #3082) | +| [#3160](https://github.com/basecamp/fizzy/pull/3160) | `oauth-refresh-replay` | Refresh-token replay detection (on #3082) | +| [#3161](https://github.com/basecamp/fizzy/pull/3161) | `oauth-grant-idle-expiry` | 90-day idle expiry of grants (on #3160) | +| [#3159](https://github.com/basecamp/fizzy/pull/3159) | `oauth-client-sweep` | Sweep of abandoned DCR clients (on #3161) | + +Commits are cited by subject, with the short SHA as of the snapshot. Rebases change +the SHAs of #3080 and above; the subjects stay the same. + +The hosted MCP plan is +[fizzy-mcp-server#5 `docs/hosted-plan.md`](https://github.com/basecamp/fizzy-mcp-server/pull/5). +Its phases (A merge dark; B audience + account binding; C RFC 8693 exchange; C′ DPoP +and CIMD; D–G MCP server and light-up) are what "Planned" refers to below. + +Status key: + +- **Conformant**: holds at the snapshot, with no change needed in this round. +- **Fixed**: the PR and commit that made it hold. +- **Planned**: not built yet; names the hosted-plan phase. +- **Deliberate**: a recorded posture, with the reason given. +- **Open decision**: a departure that needs a call (see [Decisions](#decisions)). +- **N/A**: does not apply to Fizzy. + +## Availability (ship dark) + +| Requirement | Status | +|---|---| +| The AS merges dark: discovery, DCR, authorize and token answer 404 unless `OAUTH_ACCEPTANCE_ENABLED=true`; OAuth bearer tokens are refused while dark; personal access tokens are unaffected (hosted plan, Phase A and Decision 6) | **Fixed** #2296 `1183b2bee` "Ship the OAuth authorization server dark behind availability switches". Port of bc3's `Oauth::Availability`. | +| Issuance can be paused (`OAUTH_ISSUANCE_ENABLED=false`) while existing tokens are still honored; minting and refresh answer 503 | **Fixed** #2296 `1183b2bee`; refresh covered in #3081 `0568c1392` | +| Pilot client ids are exempt by exact `client_id`; registration can't be used to claim a pilot id | **Fixed** #2296 `1183b2bee`, `9a9d3ce7a` "Keep registration dark when a request names a pilot client" | +| Revocation, Connected Apps and consent denial are never gated, so people can always shed access | **Conformant** (by design in `Oauth::Availability`) | +| Unset means dark, which differs from bc3's dev default; dark answers 404 where bc3 answers 503; protected-resource metadata is also 404 while dark | **Deliberate**. Fizzy has no other AS to name, and OSS installs that never mention OAuth must look like they have none. | + +## RFC 6749 / OAuth 2.1: authorization endpoint + +| Requirement | Status | +|---|---| +| `response_type=code` only | **Conformant** | +| PKCE required, `S256` only (OAuth 2.1 §4.1.1, RFC 7636) | **Conformant** | +| `code_verifier` is 43–128 unreserved characters (RFC 7636 §4.1); a malformed one fails like a wrong one | **Fixed** #2296 `694eb8d7b` "Refuse a PKCE verifier outside 43 to 128 unreserved characters". Matches bc3's `Oauth::Pkce::VERIFIER_PATTERN` | +| Exact `redirect_uri` match against the registered list; never redirect to an unregistered URI (§3.1.2, §4.1.2.1) | **Conformant** | +| Loopback redirects: only the port may vary, host and query must match exactly, http only (RFC 8252 §7.3) | **Fixed** #2296 `51155c5e3` "…vary only the loopback port" (exact host and query; restores a fix a resolved Copilot thread had recorded as done but that was lost), `771ed906d` "Vary the port only for http loopback redirects"; #3080 `3a52a06ac` (no port variance onto a fragment), `6685905d3` | +| `state` is optional; when sent it's a single string of at most 2048 characters, echoed back exactly | **Fixed** #2296 "Accept an authorization request without state, and bound one that has it". Was **Deliberate** (required). PKCE is the CSRF defence (OAuth 2.1 §7.5.1), and the rubric's GP-7 makes `state` optional. | +| Consent is CSRF-protected (§10.12) | **Conformant**. The app-wide Origin check (`forgery_protection_origin_check`) refuses same-site sibling origins as well as cross-site ones. A finding that it didn't was refuted. | +| Consent is always shown; `trusted` doesn't skip it | **Deliberate** (#2296 body) | +| CSP `form-action` lets the consent POST redirect to the validated redirect origin; Chrome had been stranding the flow | **Fixed** #2296 `d468283d7` "Let consent redirect past CSP…". Ported from bc3. Covered by a Chrome system test (`test/system/oauth_consent_test.rb`). | +| Canonical scope: "Read + Write" is `read write`; a `read write` request preselects it instead of silently downgrading to read | **Fixed** #2296 `d468283d7`; refresh reports the same string in #3081 `89e22441d` | + +## RFC 6749 / OAuth 2.1: token endpoint + +| Requirement | Status | +|---|---| +| Authorization codes are single-use; reuse is refused and the grant minted from that code is revoked (§4.1.2, §10.5; OAuth 2.1 §4.1.3) | **Fixed** #2296 `bd447dd7a` "Redeem each authorization code once…". A `jti` in the code plus a unique `identity_access_tokens.authorization_code_jti`. Reuse is checked only after PKCE, `redirect_uri` and `client_id` pass, so a leaked code without its verifier can't revoke the grant. Limit: if the grant is revoked within the code's 60s life, the row is gone and one more redemption is possible. No tombstones were added. | +| `client_id` required and matched to the code for public clients (§4.1.3) | **Fixed** #2296 `bd447dd7a`; one check shared by both grants in #3081 `f9a1885ff` | +| Missing or malformed parameters answer `invalid_request`, not `invalid_grant` or `unsupported_grant_type` (§5.2) | **Fixed** #2296 `bd447dd7a`, `ac884df5f` "Reject non-string token request parameters as malformed" (an array `refresh_token` had been accepted as an `IN` list; a hash had raised a 500); #3081 `f9a1885ff`, `2b2b7f2d6` | +| Exact `redirect_uri` equality with the code (§4.1.3) | **Conformant** | +| Responses, errors included, carry `Cache-Control: no-store` (§5.1) | **Fixed** #3082. Every authorization server response, discovery, consent, its redirects and revocation included, carries `Cache-Control: no-store` and `Referrer-Policy: no-referrer`, from a callback prepended ahead of every one that can halt ("Keep every authorization server response out of caches and Referers"; rubric SC-5) | +| Authorization codes, PKCE verifiers, client assertions, DPoP proofs, tokens and secrets are filtered from logs, the logged redirect to the client included | **Fixed** #2296 "Filter authorization codes, PKCE verifiers and DPoP proofs from logs" (rubric SC-6). SQL and slow-query logs are separate work. | +| A confidential client authenticates before anything about the grant is looked up, so a wrong secret reveals nothing about whether a code or refresh token is live | **Fixed** #3082 `a1c6aad55`, `d7795f51b` "Authenticate the named client before resolving the grant…" | +| A client that fails authentication gets `invalid_client`: 401 + `WWW-Authenticate: Basic` only for a failed Basic attempt, and 400 with no challenge for anything else (a body credential, a public client presenting a secret, a secret that authenticates no client, a body failure beside another scheme's header) (§5.2) | **Fixed** #3082 `b6b83296a` "Answer invalid_client with 400 unless Basic authentication failed". The same at the token endpoint and revocation. A bare 401 breaks RFC 9110 §15.5.2; a Basic challenge to a request that never tried Basic applies to nothing; oauth4webapi and openid-client surface a challenged 401 as `WWWAuthenticateChallengeError`, not `invalid_client`; and a Basic challenge on a form POST can pop a browser auth dialog. | +| Client credentials are read from the request body only, never the query string | **Conformant** (#3082 `38fd57bbc`) | +| Per-IP rate limit of 20/min on the token endpoint | **Conformant**. A per-client budget after confidential authentication is **Planned** (Phase C); the per-IP limit blocks hosted use. | + +## RFC 6749 §2.3.1: client authentication + +| Requirement | Status | +|---|---| +| `client_secret_post` | **Conformant** (#3082) | +| HTTP Basic (`client_secret_basic`) MUST be supported for clients issued a password; a failed Basic attempt gets 401 + `WWW-Authenticate` (§5.2) | **Fixed** #3082 `d65d0117c` "Accept client_secret_basic at the token endpoint". Either method authenticates any confidential client; both at once is `invalid_request`. DCR and metadata advertise it. bc3 closes the same gap in bc3#13678. | +| Secrets stored in plaintext, as access tokens are | **Deliberate** (#3082 body) | +| An OAuth grant can't reach a credential or consent surface: personal access tokens, Connected Apps, passkey registration, transfer links, email changes or OAuth consent. Anything it minted there would outlive disconnecting the app, and anything it removed would belong to another app | **Fixed** #2296 `196b9b0b3` "Keep OAuth grants out of every credential and consent surface" (`Authentication.disallow_oauth_grants`, declared on each such controller). Personal access tokens may still manage personal access tokens (fizzy-cli) | + +## Refresh tokens (RFC 6749 §6, OAuth 2.1 §4.3, RFC 9700 §4.14) + +| Requirement | Status | +|---|---| +| Rotation on every refresh, atomic (compare-and-swap). A concurrent loser is a retry of the winning rotation: within the 60-second grace it gets the winner's pair, as a retry would (#3160); past it, it's a replay | **Conformant** (#3081, #3160) | +| Refresh may narrow scope, never widen it; blank or `null` scope is refused | **Conformant** (#3081). | +| A refresh token issued after narrowing keeps its predecessor's scope (RFC 6749 §6) | **Deliberate**: a grant is one row, so narrowing on refresh narrows the grant for good, and a later refresh can't ask for the wider scope again. Strictly more restrictive than the rubric (RT-5 forbids only widening); signing in to the app again restores a broader scope. No shipped client narrows on refresh. | +| `client_id` required and must match the grant | **Conformant** (#3081 `f9a1885ff`) | +| Public clients' refresh tokens MUST be sender-constrained, or rotated **with replay detection**: presenting a rotated-out token revokes the live one (OAuth 2.1 §4.3.1, RFC 9700 §4.14.2) | **Fixed** #3160 `cf4aa3192` "Revoke the grant when a rotated refresh token is replayed, with a retry grace". A replay revokes the grant. A retry within 60s, while the successor is still current, gets that successor, as in bc3. | +| Refresh tokens SHOULD expire after client inactivity (RFC 9700 §4.14.2) | **Fixed** #3161 `f97c959e0` "Lapse OAuth grants left idle for 90 days", `d6bfcc2b1`. This matches bc3's 90-day `refresh_token_ttl`. A grant that lapses restarts its client's 30-day sweep clock (#3159). | + +## RFC 7009: revocation + +| Requirement | Status | +|---|---| +| Revokes an access or refresh token, destroying the whole grant; always 200 for unknown tokens (§2.2) | **Conformant** | +| Missing token answers `invalid_request` JSON (§2.2.1) | **Fixed** #2296 `51155c5e3` | +| Confidential clients authenticate, and the token must have been issued to the requesting client (§2.1) | **Fixed** #3082 `e80b5b615` "Authenticate clients at revocation and revoke only their own tokens". A public client names its `client_id`. Unknown tokens, and tokens belonging to another client, get 200 and nothing is revoked. | +| A presented client secret that authenticates no client is refused with 400 `invalid_client` (401 + Basic if it came in a Basic header), whatever the token | **Deliberate**, and stricter than bc3: #3082 `c9375f186`, `29a5d118c`. A 200 would tell a client with a mistyped `client_id` that a live grant was dead. bc3 is being aligned to this. | +| A personal access token can be revoked here by whoever holds it. It has no client, so holding it is the credential; client credentials sent alongside must still authenticate | **Fixed** #3082 `98619ab8f` "Revoke personal access tokens at the revocation endpoint again". Revocation works the same for every kind of token, which helps incident response such as revoking a leaked token. It also keeps the endpoint ready if personal access tokens become OAuth tokens, as in bc3. | +| Revocation is throttled like the token endpoint: 20 per minute per IP, then 429 `slow_down` (RFC 6749 §2.3.1, RFC 7009 §5) | **Fixed** #3082 `11e8e1ad8` "Throttle revocation like the token endpoint". Once revocation authenticated confidential clients, it became a place to guess secrets | + +## RFC 8414 / RFC 9728: metadata + +| Requirement | Status | +|---|---| +| Issuer, endpoints, grant types, PKCE methods and scopes advertised | **Conformant** | +| `revocation_endpoint_auth_methods_supported` matches behaviour (`["none", "client_secret_post", "client_secret_basic"]`; an omitted value would mean `client_secret_basic` alone) | **Fixed** #2296 `51155c5e3`; #3082 `e80b5b615` | +| `response_modes_supported: ["query"]` (an omitted value implies fragment, which is unsupported) | **Fixed** #2296 `51155c5e3`. Unknown `response_mode` is ignored, per RFC 6749 §3.1, not rejected. | +| `token_endpoint_auth_methods_supported` matches behaviour | **Conformant** (`none`, `client_secret_post`, `client_secret_basic`) | +| RFC 9728 protected-resource metadata | **Conformant** (#2296). It has one app-wide resource; MCP resources come in Phase B. | + +## RFC 9207: issuer in the authorization response + +| Requirement | Status | +|---|---| +| `iss` on success and error redirects, equal to the metadata issuer; `authorization_response_iss_parameter_supported: true` (mix-up defense, RFC 9700 §4.4) | **Fixed** #2296 `d468283d7`. A single `Oauth::BaseController#oauth_issuer` feeds both the redirect and the metadata. | + +## RFC 9700: open redirector + +| Requirement | Status | +|---|---| +| Never auto-redirect errors to an untrusted URI (§4.11.2). Self-registered https hosts get the local error page for pre-consent errors; operator clients and loopback still redirect; deny after consent still redirects | **Fixed** #3080 `35c8d0de9` "Render pre-consent errors here for self-registered https clients", `4809449a0` (tests for every pre-consent error) | + +## RFC 7591: dynamic client registration + +| Requirement | Status | +|---|---| +| Loopback and https redirect URIs; fragments, plain-http public hosts and custom schemes rejected | **Conformant** (#2296, #3080) | +| A self-registered redirect names its authority plainly: no userinfo, no percent-encoded host, a port from 1 to 65535. Consent names a non-default port, and loopback port variation keeps userinfo fixed | **Fixed** #3080 `91b975e6e` "Register only redirects whose authority is plain, and show their port on consent". One rule at registration, in place of canonicalizing wherever the URI is shown or compared | +| Errors answer `invalid_client_metadata` or `invalid_redirect_uri`, never 500 (§3.2.2). An over-long or non-String `client_name` had raised a 500 on both databases | **Fixed** #2296 `51155c5e3` | +| Open, unauthenticated registration of any https host (no initial access token, no host vetting) | **Deliberate** (#3080, hosted plan): Cursor and other generic MCP clients need it. bc3 vets hosts; Fizzy will cover the recognized-host tier with CIMD (Phase C′). Impersonating names on consent go to the consent design pass (D5). | +| Abandoned registrations are reclaimed (§5 abuse mitigation) | **Fixed** #3159 `de3de6185`, `c3a296cec`. A daily sweep removes self-registered clients with no grant activity for 30 days, matching bc3's retention. It row-locks and shares that lock with issuance, so it can't race a code exchange. Under the lock it rechecks the whole stale test, and consent touches the client, so a client isn't swept between consent and its code exchange (`3398ff00e`). A refresh whose client row is gone gets a 400, not a 500. Groundwork in #2296 `65e4d3e7c` fixed the `access_tokens` association and a schema-only foreign key that migrated databases never had. | +| RFC 7592 management endpoint, initial access token, global registration cap | **Deliberate**: not built. RFC 7592 is optional. A global cap would turn one actor's abuse into an outage for everyone. | + +## RFC 6750 / RFC 9110: resource-server challenge + +| Requirement | Status | +|---|---| +| Challenges use the `Bearer` scheme (§3). Rails' default was `Token realm=` | **Fixed** #3158 `cc7d07659` "Answer API authentication failures with a Bearer challenge" | +| A presented but invalid or expired token gets 401 `error="invalid_token"` (§3.1); clients refresh on this hourly since #3081 | **Fixed** #3158 `cc7d07659` | +| A valid read token on a write gets 403 `error="insufficient_scope", scope="write"`, so clients stop refreshing in a loop | **Fixed** #3158 for OAuth grants. A read-only personal access token keeps the 401 shipped clients have always had, with the same `insufficient_scope` challenge (**Deliberate**, pinned by #3191). | +| A JSON request with no credentials gets 401 plus a challenge instead of a 302 to sign-in (§3) | **Deliberate** (D6): it keeps the 302. Nothing in a request without credentials tells the native apps or fizzy-cli, which rely on the redirect (pinned by #3191), from an OAuth client. Hosted OAuth clients discover Fizzy's authorization server through fizzy-mcp-server's own 401 + `resource_metadata` and Fizzy's `/.well-known` metadata, not a credential-less call to Fizzy's API, so it costs them little. **Exit:** once the oldest supported native app and fizzy-cli handle a 401 challenge on an unauthenticated JSON request. | +| The scheme is matched case-insensitively (RFC 9110 §11.1) | **Fixed** #3158 `cc7d07659` | +| `resource_metadata` in the challenge (RFC 9728 §5.1), omitted while dark | **Fixed** #3158 `cc7d07659` | +| A Bearer header on an HTML request: 401, not the SHOULD-level 400 for `invalid_request` | **Deliberate** (#3158 summary); preserves the existing refusal. | + +## Parity with bc3 not yet built + +| Capability | Status | +|---|---| +| RFC 8707 resource indicators and audience stamping; the API refuses audienced tokens | **Planned**: Phase B. The `resource` parameter is ignored today, which RFC 6749 §3.1 permits. Must ship before any MCP-audienced token is issued. | +| One account per connection (account-bound grants, account picker at consent) | **Planned**: Phase B, Decision 4. OAuth tokens are identity-wide today, like personal access tokens. | +| RFC 8693 token exchange for the MCP server | **Planned**: Phase C, on top of B. Copy bc3's error table. | +| Per-client token rate limit after confidential authentication | **Planned**: Phase C | +| DPoP-bound delegated tokens, replay store (port the bc3#12535 fix), `htu` behind kamal-proxy | **Planned**: Phase C′. #3158 already parses the scheme, so the DPoP branch can reuse it. The binding check must be keyed on the token row, not on the header. | +| CIMD (URL client ids, recognized hosts, SSRF-guarded via Surfguard) | **Planned**: Phase C′, after DPoP | +| Device authorization grant (RFC 8628) | **N/A for now**: there's no consumer. fizzy-cli uses personal access tokens, and a CLI can use a loopback redirect with PKCE. Revisit if fizzy-cli moves to OAuth login. | +| Token introspection (RFC 7662) | **N/A**: exchange is the chosen model. | + +## Decisions + +These need Jeremy. Everything else above is either conformant, fixed in the +train, or planned. + +D1 through D4 were decided and are built: replay detection (#3160), idle +expiry (#3161), `client_secret_basic` (#3082), and client authentication at +revocation (#3082). For D4, personal access tokens stay revocable by whoever +holds them, and a client secret that authenticates no client gets 400 +`invalid_client` (401 + Basic only when it came in a Basic header). + +5. **D5: consent for self-registered clients.** For the consent design pass: + lead with the redirect host rather than the self-asserted `client_name`, and + refuse impersonating names (Fizzy, Basecamp, HEY, Claude, ChatGPT) for + self-registered clients. +6. **D6 (#3158), decided: shipped clients keep their answers.** A JSON + request with no credentials keeps the 302 (DS-3 deviation, row above), and + a read-only personal access token writing keeps its 401; only an OAuth + grant gets the 403. + +The train is rebased onto main, which carries rubyzip 3.7.0, so the Security +audit no longer flags rubyzip 3.3.0 (CVE-2026-85396). diff --git a/test/integration/oauth_flow_test.rb b/test/integration/oauth_flow_test.rb index 5146325e15..3530307d3d 100644 --- a/test/integration/oauth_flow_test.rb +++ b/test/integration/oauth_flow_test.rb @@ -157,6 +157,39 @@ class OauthFlowTest < ActionDispatch::IntegrationTest assert_select "strong", text: "connector.example.com:8443" end + test "consent restarts a self-registered client's retention period, so the sweep spares it while the code is live" do + sign_in_as :david + client = travel_to(31.days.ago) { Oauth::Client.create!(name: "Returning", redirect_uris: %w[ http://127.0.0.1:8888/callback ], dynamically_registered: true) } + assert_includes Oauth::Client.stale, client + + post_consent client: client + + assert_response :redirect + assert_not_includes Oauth::Client.stale, client + end + + test "consent issues no code for a client swept while consent was submitted" do + sign_in_as :david + client = Oauth::Client.create!(name: "Swept", redirect_uris: %w[ http://127.0.0.1:8888/callback ], dynamically_registered: true) + + # The sweep deletes the client between consent reading it and touching it. + Oauth::Client.any_instance.stubs(:allows_redirect?).with { client.delete }.returns(true) + Oauth::AuthorizationCode.expects(:generate).never + + post_consent client: client + + assert_response :bad_request + end + + test "denying consent leaves a self-registered client's retention period alone" do + sign_in_as :david + client = travel_to(31.days.ago) { Oauth::Client.create!(name: "Returning", redirect_uris: %w[ http://127.0.0.1:8888/callback ], dynamically_registered: true) } + + post_consent client: client, error: "access_denied" + + assert_includes Oauth::Client.stale, client + end + test "denying a self-registered https client still redirects, after the consent screen named its host" do sign_in_as :david client = Oauth::Client.create!(name: "Hosted", redirect_uris: %w[ https://connector.example.com/callback ], dynamically_registered: true) @@ -521,6 +554,30 @@ class OauthFlowTest < ActionDispatch::IntegrationTest assert_equal "read write", response.parsed_body["scope"] end + test "token exchange issues nothing for a client swept after the exchange read it" do + code_verifier = "dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk" + client = oauth_clients(:mcp_client) + code = authorization_code_for(client, code_verifier: code_verifier) + + # The sweep deletes the client between the exchange's lookup and its issuance. + Oauth::AuthorizationCode.stubs(:valid_pkce?).with { client.delete }.returns(true) + + assert_no_difference "Identity::AccessToken.count" do + untenanted do + post oauth_token_path, params: { + grant_type: "authorization_code", + client_id: client.client_id, + code: code, + redirect_uri: "http://127.0.0.1:8888/callback", + code_verifier: code_verifier + }, as: :json + end + end + + assert_response :bad_request + assert_equal "invalid_grant", response.parsed_body["error"] + end + test "token exchange rejects invalid code" do untenanted do post oauth_token_path, params: { @@ -1109,6 +1166,23 @@ class OauthFlowTest < ActionDispatch::IntegrationTest assert_not_equal old_refresh_token, body["refresh_token"] end + test "refresh grant for a token whose client is gone fails as invalid_grant" do + client = oauth_clients(:mcp_client) + token = identities(:david).access_tokens.create!(oauth_client: client) + client.delete + + untenanted do + post oauth_token_path, params: { + grant_type: "refresh_token", + refresh_token: token.refresh_token, + client_id: client.client_id + }, as: :json + end + + assert_response :bad_request + assert_equal "invalid_grant", response.parsed_body["error"] + end + test "refresh grant requires refresh_token and client_id" do client = oauth_clients(:mcp_client) token = identities(:david).access_tokens.create!(oauth_client: client) diff --git a/test/models/oauth/client_test.rb b/test/models/oauth/client_test.rb index c0961e62eb..5354b8cad6 100644 --- a/test/models/oauth/client_test.rb +++ b/test/models/oauth/client_test.rb @@ -307,4 +307,156 @@ class Oauth::ClientTest < ActiveSupport::TestCase assert_not Oauth::Client.new(name: "Long", redirect_uris: [ "http://127.0.0.1/#{"a" * 2000}" ]).valid? assert Oauth::Client.new(name: "Ten", redirect_uris: (1..10).map { |port| "http://127.0.0.1:#{port}/cb" }).valid? end + + test "cleanup destroys dynamically registered clients left without a grant for the retention period" do + stale = travel_to(31.days.ago) { register_client } + + Oauth::Client.cleanup + + assert_not Oauth::Client.exists?(stale.id) + end + + test "cleanup keeps recent, granted and operator-provisioned clients" do + recent = travel_to(29.days.ago) { register_client } + granted = travel_to(31.days.ago) { register_client.tap { identities(:david).access_tokens.create!(oauth_client: it) } } + provisioned = travel_to(31.days.ago) { register_client(dynamically_registered: false, redirect_uris: %w[ https://app.example.com/cb ]) } + + Oauth::Client.cleanup + + assert Oauth::Client.exists?(recent.id) + assert Oauth::Client.exists?(granted.id) + assert Oauth::Client.exists?(provisioned.id) + end + + test "cleanup counts the retention period from the client's last grant, not its registration" do + client = travel_to(60.days.ago) { register_client } + token = travel_to(40.days.ago) { identities(:david).access_tokens.create!(oauth_client: client) } + travel_to(1.day.ago) { token.destroy } + + Oauth::Client.cleanup + assert Oauth::Client.exists?(client.id), "a client disconnected yesterday is not abandoned" + + travel 30.days do + Oauth::Client.cleanup + assert_not Oauth::Client.exists?(client.id) + end + end + + test "a grant that lapses restarts its client's retention period, like one the user disconnects" do + client = travel_to(200.days.ago) { register_client } + travel_to(100.days.ago) { identities(:david).access_tokens.create!(oauth_client: client) } + + Identity::AccessToken.cleanup + Oauth::Client.cleanup + assert Oauth::Client.exists?(client.id), "swept the day its last grant lapsed" + assert_not client.access_tokens.exists? + + travel 31.days do + Oauth::Client.cleanup + assert_not Oauth::Client.exists?(client.id) + end + end + + test "cleanup carries on past a client deleted after it was picked" do + gone, stale = travel_to(31.days.ago) { [ register_client, register_client ] } + picked = Oauth::Client.stale.find(gone.id) + gone.delete + + assert_nothing_raised { assert_not picked.destroy_if_still_unused } + + Oauth::Client.cleanup + assert_not Oauth::Client.exists?(stale.id) + end + + test "destroy_if_still_unused spares a client whose grant came and went after it was picked" do + client = travel_to(31.days.ago) { register_client } + picked = Oauth::Client.stale.find(client.id) + identities(:david).access_tokens.create!(oauth_client: client).destroy + + assert_not picked.destroy_if_still_unused + assert Oauth::Client.exists?(client.id) + end + + test "destroy_if_still_unused spares a client that got a grant after it was picked" do + client = travel_to(31.days.ago) { register_client } + picked = Oauth::Client.stale.find(client.id) + identities(:david).access_tokens.create!(oauth_client: client) + + assert_not picked.destroy_if_still_unused + assert Oauth::Client.exists?(client.id) + end + + # Revoking a grant locks it, and touching the client inside that transaction + # would then wait on the client's lock. A replayed code exchange holds the + # client's lock and waits on the grant, so the two would deadlock. + test "a grant touches its client outside its own transaction" do + client = register_client + grant = identities(:david).access_tokens.create!(oauth_client: client) + transactions = {} + record = ->(*, payload) do + if table = payload[:sql][/\A(?:UPDATE|DELETE FROM) [`"]?(oauth_clients|identity_access_tokens)\b/, 1] + transactions[table] ||= Identity::AccessToken.connection.current_transaction + end + end + + ActiveSupport::Notifications.subscribed(record, "sql.active_record") { grant.destroy } + + assert transactions.key?("oauth_clients"), "the client was never touched" + assert_not transactions["identity_access_tokens"].equal?(transactions["oauth_clients"]), "the client was touched inside the grant's transaction" + end + + # A code exchange locks the client, then issues a grant. Destroying a client + # takes the client's lock first too, so no grant can land after the cascade + # has run and be stranded by the client's deletion. + test "destroying a client locks it before destroying its grants" do + client = register_client + identities(:david).access_tokens.create!(oauth_client: client) + statements = [] + record = ->(*, payload) { statements << payload[:sql] } + + ActiveSupport::Notifications.subscribed(record, "sql.active_record") { client.destroy } + + lock = statements.index { it.match?(/\ASELECT .*FROM [`"]?oauth_clients\b/) } + cascade = statements.index { it.match?(/\ADELETE FROM [`"]?identity_access_tokens\b/) } + assert lock, "the client was never locked" + assert_operator lock, :<, cascade + end + + test "destroying a client takes a grant issued after its grants were loaded" do + client = register_client + client.access_tokens.load + late = identities(:david).access_tokens.create!(oauth_client: Oauth::Client.find(client.id)) + + client.destroy + + assert_not Identity::AccessToken.exists?(late.id) + end + + test "destroying a client takes its grants' retired refresh tokens too" do + client = register_client + grant = identities(:david).access_tokens.create!(oauth_client: client) + assert grant.refresh + + assert_difference -> { Oauth::RetiredRefreshToken.count }, -1 do + client.destroy + end + assert_not Identity::AccessToken.exists?(grant.id) + end + + + test "destroying a client with unsaved changes still destroys it and its grants" do + client = Oauth::Client.create!(name: "Dirty", redirect_uris: %w[ http://127.0.0.1:8888/callback ]) + identities(:david).access_tokens.create!(oauth_client: client) + client.name = "Renamed but never saved" + + assert_difference -> { Identity::AccessToken.count }, -1 do + client.destroy + end + assert_not Oauth::Client.exists?(client.id) + end + + private + def register_client(**attributes) + Oauth::Client.create! name: "Abandoned", redirect_uris: %w[ http://127.0.0.1:8888/callback ], dynamically_registered: true, **attributes + end end