diff --git a/CHANGELOG.md b/CHANGELOG.md index 976fe2b..68f8e9b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,190 @@ +## [0.6.0] - 2026-07-16 + +An OAuth 2.1 authorization bridge for the authority role, so hosted MCP clients +that will only authenticate by discovering an authorization server and running a +browser flow can reach a server whose tokens are issued out-of-band. Additive and +opt-in: a host that configures nothing behaves exactly as it did on 0.5.0. + +### Added + +- **OAuth authorization bridge (authority-only, opt-in).** A standards-shaped + envelope around the tokens a host ALREADY issues — not an identity provider. Its + authorization page asks an operator to paste an existing access token, and the + `access_token` it returns IS that token, verified through the same + `config.token_authenticator` the transport uses. Scopes, expiry, revocation and + tenancy stay entirely with the host; the bridge widens nobody's reach. + + Deliberately not implemented, because none of it gates anything here: client + registration returns an identifier and stores nothing (no endpoint reads a + `client_id`); there is no consent step (pasting a token you already hold is the + grant); no refresh token is issued (the pasted token's own expiry is the real + lifetime, so a client re-runs the flow rather than refreshing a shadow of it). + + Deliberately NOT mocked, because faking either would create a real vulnerability + rather than skip a ceremony: `redirect_uri` is checked against the host's policy + on BOTH legs (below), and the PKCE `code_verifier` is verified (constant-time) + against the stored S256 `code_challenge`. + + **Which clients may receive a code.** The authorization page is served from the + host's own origin, under its own certificate, and asks an operator to paste a + live token — so an unvetted `redirect_uri` does not merely add an open redirect, + it makes the host's own domain a credential-phishing page: an attacker sends the + operator an authorize link carrying the attacker's own `code_challenge`, the + operator pastes, and the code is delivered to the attacker, who redeems it with + the verifier they chose. PKCE cannot help — they own the verifier. + + So every redirect target must be named by exact string in + `config.oauth_allowed_redirect_uris`, with exactly ONE exception: + **loopback** (`http://127.0.0.1:*`, `localhost`, `[::1]`), enabled by + `config.oauth_allow_loopback_redirects`. It is the exception because it is the + one target that CANNOT be named even in principle — an MCP client on an + operator's machine listens on an ephemeral port chosen at runtime, so no list + could enumerate it (RFC 8252 §7.3 exists for this) — and because a loopback + address resolves on the operator's OWN machine, so the attack above, which needs + the code to reach a REMOTE attacker, does not work through it. + + A private-use scheme (`cursor://…`, §7.1) is NOT covered: its redirect URI is a + fixed string, so it just goes in the allowlist. There is no forcing reason to + accept one unnamed, and whole schemes cannot be accepted generically anyway — + separating a private-use scheme from a registered network one (`ssh:`, `ldap:`, + `gopher:`, each naming a REMOTE host) would mean enumerating the IANA registry, + and a denylist of the ones you thought of is the shape that fails open. + + Loopback is judged on the PARSED URI: `http://127.0.0.1@evil.example/` has host + `evil.example` and is remote, as is `http://127.0.0.1.evil.example/`; a fragment + is refused. + + Endpoints — `GET`/`POST` `/oauth/authorize`, `POST /oauth/token`, + `POST /oauth/register`, plus the two metadata documents. A `/.well-known/*` + path cannot be drawn by an engine mounted under a path, so a host adds one line at + the top level of its route set: `McpToolkit.draw_oauth_metadata_routes(self)` (a + no-op unless the bridge is configured). Every identifier is derived from the live + request origin, so each host name an app answers on works without further + configuration. + + **Additive to a host's own OAuth provider, and it claims nothing origin-global.** + The flow endpoints live under the engine's mount (`/oauth/*`), so a host + already serving OAuth at the conventional top-level `/oauth/*` — as an app with + Doorkeeper for its own API does — keeps every one of those routes. The metadata + documents are PATH-SCOPED to the mount + (`/.well-known/oauth-protected-resource/mcp`), never the bare + `/.well-known/oauth-authorization-server`: the bare paths are origin-global and + mean "the authorization server of this whole origin", which belongs to that + pre-existing provider. RFC 8414 §3.1 exists for exactly this ("Using path + components enables supporting multiple issuers per host"), and the MCP + authorization spec (2025-11-25) requires a client given a path-ful issuer to try + the path-INSERTED URLs with no root fallback — so the issuer is the MCP endpoint + URL itself. A host mounted AT its origin root has no path to insert and gets the + bare paths, which is correct there. +- `config.oauth_allowed_redirect_uris` (default `[]`; entries are validated at + assignment and the list is frozen — an unparseable, scheme-less, + fragment-bearing or *opaque* URI raises, as does cleartext `http://` to a remote + host and the `javascript:`/`data:`/`file:` schemes a browser treats as script. + Naming bad schemes is sound here and nowhere else: this list is what the HOST + wrote, so there is no unlisted scheme for an attacker to slip through — unlike + the request-time policy, which is why that one takes the opposite shape), + `config.oauth_allow_loopback_redirects` (default `false`), + `config.oauth_resource_path` (default `"/mcp"` — must match the engine's mount + point), `config.oauth_authorization_code_ttl` (default `60`), + `config.oauth_signing_secret` (defaults to the Rails app's `secret_key_base`), + and `config.oauth_parent_controller` (default `"ActionController::Base"`). +- `config.oauth_bridge?` — whether the bridge is live. Gated on the authority role; + on a `token_authenticator` being set, since the bridge verifies the pasted token + through it on both legs and drawing no route beats an authorization page that + takes an operator's token and then errors; and on at least one redirect target + being named (an allowlist entry or the loopback switch), so it cannot run + without a bound answer to who may receive a code. A satellite — whose tokens + belong to its central app — never draws it. +- The token response is served `Cache-Control: no-store` + `Pragma: no-cache`, a + MUST of RFC 6749 §5.1 for any response carrying a token. Both metadata documents + get the same headers for a subtler reason: they name the + `authorization_endpoint` an operator will be sent to and are built from the + caller-influenced request origin (`request.base_url` honours `X-Forwarded-Host`), + so a shared cache holding one could hand every client an origin an attacker + chose, with the document itself vouching for it. Hosts should also pin + `config.hosts`, which Rails leaves empty in production by default. +- `POST /oauth/authorize` answers **303**, not Rails' default 302. That POST + carried the operator's token in its body, and only 303 unambiguously tells the + browser to fetch the callback with GET and no body (RFC 9700 §4.12). +- The authority transport's 401 now carries + `WWW-Authenticate: Bearer resource_metadata="..."` when the bridge is configured — + the header a hosted client waits for before it will start a flow at all. Absent + otherwise, so an opted-out host's 401 is unchanged. + +### Notes + +- **An authorization code leaves nothing usable in the cache.** The entry is keyed + by the code's SHA256, and its payload is sealed with `MessageEncryptor` + (AES-256-GCM) under a key HMAC'd from `config.oauth_signing_secret` **and** the + code. Worth the few lines because what is parked there for the code's lifetime + is not the short-lived credential an authorization server would normally hold: + it is the operator's pre-existing, long-lived, full-scope token, in a store + hosts are told to point at their shared `Rails.cache`. + + The secret is in the key for a specific reason: **the code alone must not be + it.** Rails logs an authorization code twice per flow at INFO — `Redirected to + ...?code=...` (only `config.filter_redirect` touches that line) and the token + endpoint's `Parameters:` (no stock `filter_parameters` entry matches `code`) — + so keying on the code alone would leave the key to the cache sitting in the one + artifact that is more widely read, longer retained and more replicated than the + 60-second entry it protects. With the secret mixed in, the cache, the logs and + the code together still open nothing. + + The serializer is pinned to `NullSerializer` for a similar reason: every + ActiveSupport default across the supported range (`:marshal`, and 7.1+'s + `:json_allow_marshal`) reaches `Marshal.load`, so a host with cache-write access + forging one blob would have had code execution. `JSON.parse` is now the only + parser that sees the payload. + + Codes are also single-use by the DELETE rather than the read, so of two + concurrent redemptions exactly one proceeds (verified on MemoryStore, Redis and + MemCache stores). +- The bridge's controller is built from its own `config.oauth_parent_controller` + rather than the `parent_controller` the transport uses. The transport is a + JSON-only endpoint whose parent is typically `ActionController::API`, which + cannot render an HTML view — and the authorization page is one. Keeping them + separate means enabling the bridge changes nothing about the transport. Point + `oauth_parent_controller` at your own `ApplicationController` to inherit app + branding; the page renders with `layout: false` either way. +- The engine adds `access_token` and `code_verifier` to `config.filter_parameters` + itself. The bridge takes a live token in a POST body and Rails logs parameters + at INFO, so filtering it is the gem's business — a `rails new` app happens to + ship a `:token` entry that covers `access_token` by substring, but that is a + host default the gem does not own and an `--api` host may not have. +- **Serve the bridge over HTTPS** (`config.force_ssl`). The authorization page + receives a live access token; on cleartext it is on the wire. A cleartext remote + `redirect_uri` is refused in the allowlist for the same reason, but the gem + cannot make a host's own origin HTTPS. +- The bad-paste page answers **422 as an integer**, not a symbol: the gemspec pins + no Rack floor and neither symbol spans the supported range + (`:unprocessable_content` raises below Rack 3.1, `:unprocessable_entity` is + deprecated above it), so a symbol would turn a mistyped paste into an + unauthenticated 500 on Rails 7.x. +- `config.oauth_allowed_redirect_uris` is **frozen** once assigned, and + `config.oauth_signing_secret=` validates its input. Both are the same lesson: + validation that can be bypassed (`<<` onto the reader) or skipped (a bare + writer) is a suggestion, and both failures surface at request time — after an + operator has pasted a live token. +- **A host MUST pin `config.hosts`.** Every identifier the bridge publishes is + derived from `request.base_url`, which honours `X-Forwarded-Host`. Rails does + not pin it for you — it populates `config.hosts` in development and leaves it + empty in production, where empty means no host checking at all. +- **A host MUST point `config.cache_store` at a shared store** before running the + bridge on more than one worker. The default is an in-process MemoryStore, which + cannot carry a code from the worker that issued it to the worker that redeems + it: the flow then fails roughly (N-1)/N of the time, intermittently, and only + after the operator has pasted a live token. Warned about once at boot rather + than gated, because a MemoryStore is correct in a single process and + `Rails.cache` IS one in a stock development environment. +- The bridge's two browser legs are guarded by a `before_action` rather than a + check inside each action, so the guard cannot be routed around: `authorize` is + a common method name, and a gem defining one on `ActionController::Base` would + drop the action from Rails' `action_methods` and have Rails serve the template + by implicit render — skipping a guard that lived in the body. +- A host restyles the page by defining its own + `app/views/mcp_toolkit/oauth/authorize.html.erb`, which takes precedence over the + engine's. + ## [0.5.0] - 2026-07-14 Authority-path discoverability + backward-compatibility work (driven by an diff --git a/README.md b/README.md index 53e4bee..3ffbc28 100644 --- a/README.md +++ b/README.md @@ -403,6 +403,180 @@ mount McpToolkit::Engine => "/mcp" # POST /mcp/tokens/introspect now works Drawing it is safe even on an app that is not an authority: with no `token_authenticator`, it simply answers `{ "valid": false }`. +### OAuth authorization bridge (authority-only, opt-in) + +Some MCP clients will not accept a token you hand them. They authenticate one way +only: discover an authorization server, run an authorization-code + PKCE flow in a +browser, and use whatever `access_token` comes back. The MCP authorization spec +also forbids a token in the request URI, so `?token=<...>` is not a fallback for +them either. If your tokens are issued out-of-band — an admin UI, a CLI, a support +process — those clients cannot reach your server at all. + +The bridge is a standards-shaped **envelope around the tokens you already issue**. +It is not an identity provider: its authorization page asks the operator to paste +an access token they already hold, and the `access_token` it returns **is that +token**, verified through the same `token_authenticator` your transport uses. +Scopes, expiry, revocation and tenancy stay exactly where you put them, and it +creates no new way to obtain a token. + +```ruby +# config/initializers/mcp_toolkit.rb +McpToolkit.configure do |c| + c.auth_role = :authority + c.token_authenticator = ->(plaintext) { AccessToken.authenticate(plaintext) } + + # REQUIRED for the bridge on any multi-worker deployment. The default is an + # in-process MemoryStore, which cannot carry an authorization code from the + # worker that issues it to the worker that redeems it — the flow then fails + # intermittently, *after* the operator has pasted their token. + c.cache_store = Rails.cache + + # Naming who may receive an authorization code is what switches the bridge on. + c.oauth_allowed_redirect_uris = ["https://client.example/callback"] + c.oauth_resource_path = "/mcp" # must match the engine's mount point + + # Optional: let any MCP client running on your operators' OWN machines connect + # without an allowlist entry each (RFC 8252 — see below). This is an opt-in + # signal in its own right, so it alone can switch the bridge on. + c.oauth_allow_loopback_redirects = true +end +``` + +```ruby +# config/routes.rb — the helper call must be TOP LEVEL. A `/.well-known/*` path +# cannot be drawn by an engine mounted under a path, so the metadata routes have +# to live in your own route set. A no-op unless the bridge is configured. +Rails.application.routes.draw do + McpToolkit.draw_oauth_metadata_routes(self) + mount McpToolkit::Engine => "/mcp" +end +``` + +That yields the whole flow — `GET /.well-known/oauth-protected-resource/mcp`, +`GET /.well-known/oauth-authorization-server/mcp`, `POST /mcp/oauth/register`, +`GET`/`POST /mcp/oauth/authorize`, `POST /mcp/oauth/token` — plus a +`WWW-Authenticate: Bearer resource_metadata="..."` header on the transport's 401, +which is what makes a client start the flow at all. Every identifier is derived +from the live request origin, so each host name your app answers on works with no +further configuration. + +**What is deliberately absent**, because none of it gates anything here: client +registration returns an identifier and stores nothing (no endpoint reads a +`client_id`); there is no consent step (pasting a token you hold *is* the grant); +no refresh token is issued (the pasted token's own expiry is the real lifetime, so +a client re-runs the flow instead of refreshing a shadow of it). + +**What is not faked**, because faking either would be a real vulnerability rather +than a skipped ceremony: `redirect_uri` is checked against your policy on both +legs (below), and the PKCE `code_verifier` is verified against the stored S256 +challenge in constant time. + +### Which clients may receive a code + +This is the bridge's load-bearing control, so it is worth knowing why it is shaped +the way it is. The authorization page is served from **your** origin under **your** +certificate and asks an operator to paste a live token. So an unvetted +`redirect_uri` does not merely add an open redirect — it makes your own domain a +credential-phishing page: an attacker sends the operator an authorize link +carrying the attacker's own `code_challenge`, the operator pastes, the code is +delivered to the attacker, and they redeem it with the verifier they chose. PKCE +does not help (they own the verifier), nor does the single-use code, nor +re-verifying the token. A full authorization server blocks this with a consent +screen naming the client plus an authenticated session; this bridge mocks both +away, which is exactly what the redirect policy compensates for. + +So **every target must be named by exact string**, with exactly one exception: + +| Target | Rule | Why | +|---|---|---| +| Anything remote (`https://client.example/cb`) | Exact string, in `oauth_allowed_redirect_uris` | The phishing vector. Never opened up. | +| Private-use scheme (`cursor://…`, `com.example.app:/cb`) | Exact string, in `oauth_allowed_redirect_uris` | Keeps the code on the device, but its URI is a fixed string — so just name it. | +| Loopback (`http://127.0.0.1:*`, `localhost`, `[::1]`) | `oauth_allow_loopback_redirects` | The only target that **cannot** be named: the client picks an ephemeral port at runtime (RFC 8252 §7.3). And it resolves on the operator's own machine, so the attack above cannot reach it. | + +The loopback exception exists because an allowlist entry is *impossible* there, +not because native clients are trusted. A private-use scheme keeps the code on the +device too, but nothing forces it to be unnamed — and whole **schemes** cannot be +accepted generically anyway: telling a private-use scheme from a registered +network one (`ssh:`, `ldap:`, `gopher:` — each naming a **remote** host) would +mean enumerating the IANA registry, and a denylist of the ones you happened to +think of is the shape that fails open. + +Loopback is judged on the *parsed* URI, so `http://127.0.0.1@evil.example/` (host +`evil.example`) and `http://127.0.0.1.evil.example/` are both correctly seen as +remote, and a fragment is refused. + +**What the allowlist does not cover.** It binds which URL a code may be sent to — +not *whose session at that URL* receives it. A hosted MCP client is one callback +shared by every one of its users, so an attacker can start a flow in their own +account there, send an operator the resulting authorize link, and have the code +land back at that client carrying the attacker's `state`. Whether the operator's +token then ends up in the attacker's account is decided by whether **the client** +binds `state` to the browser session that began the flow (RFC 6819 §4.4.1.7). An +authorization server cannot bind a code to a session it never saw, so this is not +something the bridge — or a full authorization server, which has the identical +exposure — can close. **Only allowlist clients you believe handle `state` +correctly.** + +### Deployment note + +Every identifier the bridge publishes is derived from the live request origin +(`request.base_url`), which honours `X-Forwarded-Host`. **You MUST pin +`config.hosts`** so Rails' `HostAuthorization` rejects a forged header before it +reaches the bridge — Rails does *not* do this for you: it populates `config.hosts` +in development and leaves it **empty in production**, where empty means no +checking at all. Both metadata documents are served +`Cache-Control: no-store` regardless, so no shared cache can hand one client an +origin another client chose. + +**Serve it over HTTPS** (`config.force_ssl = true`). The authorization page +receives a live access token in a POST body; on cleartext that token is on the +wire. The gem refuses a cleartext remote `redirect_uri` in the allowlist for the +same reason, but it cannot make your own origin HTTPS for you. + +The engine adds `access_token` and `code_verifier` to `config.filter_parameters` +itself, so the pasted token stays out of your logs even on a host that ships no +filter list of its own — nothing to configure. + +**It is additive to an OAuth provider you already run, and it claims nothing +origin-global.** The flow endpoints live under the engine's mount +(`/mcp/oauth/*`), so if you already serve OAuth at the conventional top-level +`/oauth/*` — as an app with Doorkeeper for its own API does — you keep every one of +those routes. + +The metadata documents are **path-scoped** to the mount +(`/.well-known/oauth-protected-resource/mcp`), never the bare +`/.well-known/oauth-authorization-server`. That matters: the bare paths are +origin-global and mean *"the authorization server of this whole origin"*, which +belongs to a provider you already run, not to an MCP server sharing the host. +RFC 8414 §3.1 exists for exactly this — *"Using path components enables supporting +multiple issuers per host"* — and the MCP authorization spec (2025-11-25) requires +a client given a path-ful issuer to try the path-**inserted** URLs, with no root +fallback. So the issuer is your MCP endpoint URL, and both documents hang off it. + +If your MCP endpoint IS its origin root (a dedicated MCP domain), there is no path +to insert and you get the bare paths — correct there, since your server really is +that origin's only authorization server. Set `oauth_resource_path = "/"`. + +`oauth_allowed_redirect_uris` is empty and `oauth_allow_loopback_redirects` +is off by default, which leaves `config.oauth_bridge?` false and the routes +undrawn — the bridge cannot run without bounds on where codes may go. A satellite +never draws it at all (its tokens belong to its central app, so there is nothing +for it to authorize against), and neither does an authority with no +`token_authenticator`, since the bridge verifies the pasted token through it on +both legs and could not work without one. + +The bridge's controller has its **own** parent, `config.oauth_parent_controller` +(default `ActionController::Base`), deliberately separate from the +`parent_controller` your transport uses. The transport is a JSON-only endpoint you +may well have pointed at `ActionController::API`, which cannot render an HTML view +— and the authorization page is one. Keeping them apart means enabling the bridge +changes nothing about your transport. Point it at your own `ApplicationController` +to inherit branding; the page renders with `layout: false` either way, so an app +layout that needs asset-pipeline context is not pulled in. + +To restyle the page, define your own `app/views/mcp_toolkit/oauth/authorize.html.erb` +— your app's view path takes precedence over the engine's. + ## Authority + gateway server (own tools + upstreams, no SDK) Beyond the SDK-backed satellite path, the toolkit also ships a **hand-rolled diff --git a/app/views/mcp_toolkit/oauth/authorize.html.erb b/app/views/mcp_toolkit/oauth/authorize.html.erb new file mode 100644 index 0000000..b189d9e --- /dev/null +++ b/app/views/mcp_toolkit/oauth/authorize.html.erb @@ -0,0 +1,83 @@ +<%# + The bridge's only human-facing surface: paste an access token you already hold. + + Rendered with `layout: false`, so this is a whole document and carries its own + styles inline — it must not depend on the host's asset pipeline. A host that + wants its own branding defines app/views/mcp_toolkit/oauth/authorize.html.erb + in its own tree, which takes precedence over this one. + + The authorization parameters ride through as hidden fields because the bridge + keeps no state between the two legs. They are re-validated on POST — the + redirect_uri against the allowlist — so a tampered field cannot widen anything. +%> + + + + + + + Connect to <%= McpToolkit.config.server_name %> + + + +
+

Connect to <%= McpToolkit.config.server_name %>

+

Paste your access token to finish connecting. It is not shown again after this step.

+ + <% if @mcp_oauth_error.present? %> + + <% end %> + + <%= form_tag request.path, method: :post do %> + <%= hidden_field_tag :redirect_uri, params[:redirect_uri] %> + <%= hidden_field_tag :state, params[:state] %> + <%= hidden_field_tag :code_challenge, params[:code_challenge] %> + <%= hidden_field_tag :code_challenge_method, params[:code_challenge_method] %> + + + <%= password_field_tag :access_token, nil, id: "access_token", autocomplete: "off", + autocapitalize: "off", autocorrect: "off", spellcheck: false, + autofocus: true, required: true %> + + + <% end %> +
+ + diff --git a/config/routes.rb b/config/routes.rb index ec6ab3f..c43c8a7 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -30,4 +30,25 @@ # is true whenever `auth_role == :authority`). The controller also fails safe # (no `token_authenticator` => `{ valid: false }`), so this is defence in depth. post "tokens/introspect", to: "tokens#introspect" if McpToolkit.config.authority? + + # The OAuth authorization bridge (McpToolkit::Oauth::ControllerMethods). Drawn + # only when the bridge is configured — `oauth_bridge?` is authority-only AND + # requires a redirect-uri allowlist — so a satellite, or any host that has not + # opted in, gets no such routes at all. Same reasoning as the introspection + # route above: the routes file is evaluated through the routes_reloader, after + # the host's initializers/to_prepare, so the config is already set. + # + # The two metadata documents are NOT here: a client looks for them at the origin + # root, which an engine mounted under a path cannot draw. The host draws them + # with `McpToolkit.draw_oauth_metadata_routes(self)`. + # `format: false` on each, as on the metadata routes the host draws: without it + # Rails' optional `(.:format)` segment matches, so `/mcp/oauth/authorize.json` + # reaches the action, finds no JSON template, and 500s — an unauthenticated + # error on a public endpoint, for a format the bridge never speaks. + if McpToolkit.config.oauth_bridge? + get "oauth/authorize", to: "oauth#authorize", format: false + post "oauth/authorize", to: "oauth#approve", format: false + post "oauth/token", to: "oauth#token", format: false + post "oauth/register", to: "oauth#register", format: false + end end diff --git a/lib/mcp_toolkit.rb b/lib/mcp_toolkit.rb index 55ea3df..ab0f403 100644 --- a/lib/mcp_toolkit.rb +++ b/lib/mcp_toolkit.rb @@ -8,12 +8,15 @@ # subfiles that happen to touch them: # # json - JSON.parse / JSON.generate (introspection parse, tools, transport) -# digest - Digest::SHA256 (introspection cache key) +# digest - Digest::SHA256 (introspection cache key; OAuth bridge PKCE digest) # time - Time.iso8601 / Time.parse (introspection expiry parsing) -# securerandom - SecureRandom.uuid (Session ids) +# securerandom - SecureRandom.uuid (Session ids; OAuth bridge codes/client ids) +# uri - URI.parse / encode_www_form (OAuth bridge redirect construction) # mcp - the official MCP SDK (Server wraps it; Tools::Base subclasses MCP::Tool) # active_support/concern - Transport::ControllerMethods is an includable concern # active_support/cache - the default MemoryStore cache_store +# active_support/security_utils - constant-time compare (OAuth bridge PKCE) +# active_support/message_encryptor - encrypts the OAuth bridge's cached code payload # # Two third-party libs are the exception to the centralize-here rule: each is # required alongside its owner file rather than up front. @@ -25,9 +28,12 @@ require "digest" require "time" require "securerandom" +require "uri" require "mcp" require "active_support/concern" require "active_support/cache" +require "active_support/security_utils" +require "active_support/message_encryptor" # External dependencies (NOT autoloaded by Zeitwerk — only the gem's own tree is). # ActiveSupport's specific core extensions are required up front (rather than full diff --git a/lib/mcp_toolkit/authority/controller_methods.rb b/lib/mcp_toolkit/authority/controller_methods.rb index 6bfe165..8722e1f 100644 --- a/lib/mcp_toolkit/authority/controller_methods.rb +++ b/lib/mcp_toolkit/authority/controller_methods.rb @@ -381,6 +381,7 @@ def mcp_candidate_account_id(request_data) # ---- error renders -------------------------------------------------- def mcp_render_unauthorized(message) + mcp_set_authenticate_challenge render json: { jsonrpc: McpToolkit::Protocol::JSONRPC_VERSION, id: nil, @@ -391,6 +392,29 @@ def mcp_render_unauthorized(message) }, status: :unauthorized end + # Points an unauthenticated caller at the OAuth bridge's protected-resource + # metadata (RFC 9728). This header is what a hosted MCP client waits for before + # it will start an authorization flow at all — without it, a 401 is just a + # failure. It also makes the metadata's location OURS to state rather than the + # client's to guess: RFC 9728 has the client fetch this URL directly, so the + # path-scoped location is found without probing the origin's bare well-known + # path. Emitted only when the bridge is configured, so a host that has not opted + # in keeps its 401 byte-identical. + # + # `request.base_url` honours `X-Forwarded-Host`, so it is caller-influenced and + # cannot be interpolated into a quoted-string parameter unescaped: a host + # carrying a `"` would close the quotes and let a caller append auth-params of + # their own. A URL has no business containing one, so refuse rather than escape + # — an origin that odd is a misconfiguration to notice, not to render. + def mcp_set_authenticate_challenge + return unless mcp_config.oauth_bridge? + + metadata_url = "#{request.base_url}#{mcp_config.oauth_protected_resource_path}" + return if metadata_url.include?('"') + + response.headers["WWW-Authenticate"] = %(Bearer resource_metadata="#{metadata_url}") + end + def mcp_render_session_not_found render json: { jsonrpc: McpToolkit::Protocol::JSONRPC_VERSION, diff --git a/lib/mcp_toolkit/configuration.rb b/lib/mcp_toolkit/configuration.rb index 1773234..a2bd5dc 100644 --- a/lib/mcp_toolkit/configuration.rb +++ b/lib/mcp_toolkit/configuration.rb @@ -112,6 +112,214 @@ class McpToolkit::Configuration # @return [#call, nil] attr_accessor :token_authenticator + # --- auth: OAuth authorization bridge (authority-only) --------------------- + # + # An OAuth 2.1 authorization-code + PKCE envelope around the tokens the host + # ALREADY issues, for hosted MCP clients that will only authenticate by + # discovering an authorization server and running a browser flow (and that + # cannot be handed a token in the request URI, which the MCP authorization spec + # forbids). It authenticates nobody: its authorization page asks the operator + # to paste an existing access token and hands that same token back. See + # McpToolkit::Oauth::ControllerMethods. + + # The exact redirect URIs an authorization code may be handed to — the + # allowlist a REMOTE client's `redirect_uri` is matched against by exact + # string. This is the bridge's load-bearing control: without it the authorize + # endpoint would be an open redirect that emits authorization codes. + # + # Why it cannot just be opened up to "any client": the page is served from the + # host's OWN origin under its own certificate and asks for a live token, so an + # unvetted `redirect_uri` makes it a credential-phishing page hosted by the host + # itself — an attacker sends the operator an authorize link carrying the + # attacker's `code_challenge`, the operator pastes, and the code goes to the + # attacker, who redeems it with the verifier they chose. PKCE cannot help; they + # own the verifier. This list IS the redirect-URI registration a real + # authorization server does, and it is the only thing standing in for it. + # + # Be precise about what it does NOT cover, because the boundary is easy to + # overstate. It binds which URL a code may be sent to — not whose session at + # that URL receives it. Point it at a MULTI-TENANT client (which is the usual + # case: a hosted MCP client is one callback shared by every user) and an + # attacker can still start a flow in their OWN account there, send the operator + # the resulting authorize link, and have the code land back at that client + # carrying the attacker's `state`. Whether the operator's token then ends up in + # the attacker's account is decided entirely by whether the CLIENT binds + # `state` to the browser session that began the flow (RFC 6819 §4.4.1.7; RFC + # 9700 §4.7.1) — an authorization server cannot bind a code to a session it + # never saw, so no amount of consent or authentication here would close it. A + # real authorization server has exactly the same exposure. Only allowlist + # clients you believe handle `state` correctly. + # + # EMPTY BY DEFAULT. Empty, and with `oauth_allow_loopback_redirects` off, + # the bridge is DISABLED entirely (see `oauth_bridge?`) — so it cannot be + # switched on without naming who may receive a code, and a host that wants + # nothing to do with it sets nothing. + # + # c.oauth_allowed_redirect_uris = ["https://client.example/callback"] + # + # @return [Array] + attr_reader :oauth_allowed_redirect_uris + + # Assigns the allowlist, rejecting an entry the bridge must not, or could not, + # redirect to. Validated at CONFIG time for the same reason + # `filter_operator_overrides=` is: the alternative is a request-time failure, + # and here that failure lands at the worst possible moment — the authorize page + # renders, the operator pastes a live token, the token is verified, a code is + # written to the cache, and only THEN does the callback URL turn out to be + # unusable. A typo would cost the operator their paste. + # + # The result is FROZEN, so validation is a gate rather than a suggestion: an + # `<<` onto the reader would otherwise slip an unchecked entry straight past + # this method. (`config.oauth_allowed_redirect_uris << "…#frag"` used to work, + # and a fragment is not a harmless typo — the emitted `…/cb#frag?code=…` puts + # the code in the browser's HASH, so it never reaches the client's server and + # is readable by any script on the page.) + # + # See `oauth_redirect_uri_problem` for what is rejected and why. + def oauth_allowed_redirect_uris=(uris) + entries = Array(uris).map { |uri| uri.to_s.dup.freeze } + entries.each do |uri| + problem = oauth_redirect_uri_problem(uri) + next unless problem + + raise ArgumentError, "oauth_allowed_redirect_uris contains #{uri.inspect}: #{problem}" + end + @oauth_allowed_redirect_uris = entries.freeze + end + + # Permits LOOPBACK redirect targets on any port without naming each one: + # `http://127.0.0.1:54321/cb`, `http://localhost:*/cb`, `http://[::1]:*/cb`. + # + # This is the one target that cannot be allowlisted even in principle — an MCP + # client on an operator's machine listens on an ephemeral port chosen at + # runtime, so no list could enumerate it (RFC 8252 §7.3 exists for exactly + # this). And it is safe to accept unnamed for the same reason the list above + # cannot be opened up: a loopback address resolves on the operator's OWN + # machine, so the phishing described above — which needs the code to reach a + # REMOTE attacker — does not work through it. + # + # Note what this deliberately does NOT cover: a private-use scheme + # (`cursor://…`, RFC 8252 §7.1). Those keep the code on the device too, but + # their redirect URI is a FIXED STRING, so it simply goes in the list above — + # there is no forcing reason to accept one unnamed. And whole schemes cannot be + # accepted generically anyway: separating a private-use scheme from a + # registered network one (`ssh:`, `ldap:`, `gopher:` — each naming a remote + # host) would mean enumerating the IANA registry, and a denylist of the ones + # you happened to think of is the shape that fails open. + # + # A remote `https://` callback is never covered by this, whatever it is set to. + # + # OFF BY DEFAULT: switching it on says "any client on my operators' machines may + # receive a code", which is a decision, not a default — and so is an opt-in + # signal in its own right. + # + # c.oauth_allow_loopback_redirects = true + # + # @return [Boolean] + attr_accessor :oauth_allow_loopback_redirects + + # The path McpToolkit::Engine is mounted at, used to build the `resource` + # identifier, the issuer, the two metadata locations, and the bridge's own + # endpoint URLs (their origin comes from the live request, so every host name + # the app answers on works). MUST match the actual mount point, and the + # `resource` it yields MUST equal the MCP endpoint URL as an operator types it + # into their client. + # + # This path is ALSO what keeps the bridge out of the origin's global namespace + # — see `oauth_protected_resource_path`. + # + # @return [String] + attr_accessor :oauth_resource_path + + # Seconds an issued authorization code stays redeemable. Codes are single-use + # (read-and-deleted at exchange); this only bounds a code that is never + # redeemed. Short by design — a client exchanges immediately. + # + # @return [Integer] + attr_accessor :oauth_authorization_code_ttl + + # The server-held secret mixed into the key that seals a cached authorization + # code's payload (McpToolkit::Oauth::ControllerMethods#mcp_oauth_encryptor). + # + # It exists because the code alone must NOT be the key: Rails logs an + # authorization code twice per flow at INFO, so the logs would otherwise carry + # the key to the cache entry. This secret never reaches a log or a response, so + # the cache and the logs together still open nothing without it. + # + # Defaults to the Rails app's `secret_key_base` (lazily, so load order and a + # non-Rails host are both fine), which is exactly the "server-held, in ENV, + # never logged" property wanted — so a Rails host configures nothing. A + # non-Rails host MUST set it; the bridge refuses to run without one + # (`oauth_bridge?`), rather than silently sealing with a weak key. + # + # c.oauth_signing_secret = ENV.fetch("MCP_OAUTH_SIGNING_SECRET") + # + # Use `fetch`, not `ENV["..."]`: a nil from a missing var falls back to + # `secret_key_base` silently, leaving a host believing it runs a dedicated + # secret when it does not. + # + # Rotating it invalidates in-flight codes (a 60s window), nothing else — the + # tokens themselves are the host's and are untouched. + # + # @return [String, nil] + # + # Validated at assignment, like `oauth_allowed_redirect_uris=` — a non-String + # here passed `oauth_bridge?`, drew the routes, and then raised a TypeError out + # of `OpenSSL::HMAC` at request time: after the operator pasted a live token and + # a code was already cached. That is the exact failure the sibling setter exists + # to prevent, so it gets the same treatment. + def oauth_signing_secret=(secret) + raise ArgumentError, "oauth_signing_secret must be a String, got #{secret.class}" if secret && !secret.is_a?(String) + + if secret.is_a?(String) && !secret.empty? && secret.bytesize < MINIMUM_SIGNING_SECRET_BYTES + raise ArgumentError, + "oauth_signing_secret is #{secret.bytesize} bytes; use at least " \ + "#{MINIMUM_SIGNING_SECRET_BYTES} (a real secret_key_base is 128)" + end + + @oauth_signing_secret = secret + end + + # Short enough to admit any real secret, long enough to catch a placeholder. + # + # Deliberately NOT applied to the `secret_key_base` fallback, which is checked + # for presence only. The minimum exists to catch a placeholder a host typed into + # THIS setting; `secret_key_base` is Rails' own, is 128 chars in a real + # deployment, and is short only in environments where Rails generates a throwaway + # (a stock test env is ~15 bytes). A genuinely weak `secret_key_base` is an + # app-wide problem — signed cookies, message verifiers, Active Record encryption + # — and not this gem's to police from the outside. + MINIMUM_SIGNING_SECRET_BYTES = 32 + + # Reads the configured secret, else the Rails app's `secret_key_base`. Resolved + # lazily rather than in the initializer: the gem loads before a Rails app is + # fully configured, and a non-Rails host has no `Rails` constant at all. + def oauth_signing_secret + return @oauth_signing_secret if @oauth_signing_secret + + return nil unless defined?(::Rails) && ::Rails.respond_to?(:application) && ::Rails.application + + ::Rails.application.secret_key_base + end + + # The parent class of the bridge's controller, SEPARATE from + # `parent_controller` and defaulting to ActionController::Base. + # + # They are separate because the two controllers have opposite needs. The MCP + # transport is a JSON-only endpoint, so a host quite reasonably points + # `parent_controller` at `ActionController::API` — which cannot render an HTML + # view. The bridge's authorization page IS an HTML view. Deriving it from + # `parent_controller` would therefore force a host to weaken its transport's + # superclass just to switch the bridge on; keeping them apart means enabling + # the bridge changes nothing about the transport. + # + # Point this at your own `ApplicationController` to inherit app branding (the + # page renders with `layout: false` regardless, so an app layout that needs + # asset-pipeline context is not pulled in). + # + # @return [String] + attr_accessor :oauth_parent_controller + # --- caching --------------------------------------------------------------- # The cache store backing sessions and introspection results. Must satisfy the @@ -434,6 +642,8 @@ def initialize @token_authenticator = nil + initialize_oauth_bridge_defaults + @cache_store = ActiveSupport::Cache::MemoryStore.new initialize_data_path_defaults @@ -454,6 +664,18 @@ def initialize @upstreams = McpToolkit::Gateway::UpstreamRegistry.new end + # OAuth bridge defaults. The empty redirect allowlist AND the off native-client + # switch are jointly what keep the bridge OFF (`oauth_bridge?`), so a host that + # never configures it is unaffected. + def initialize_oauth_bridge_defaults + @oauth_allowed_redirect_uris = [] + @oauth_allow_loopback_redirects = false + @oauth_resource_path = "/mcp" + @oauth_authorization_code_ttl = 60 + @oauth_parent_controller = "ActionController::Base" + @oauth_signing_secret = nil # falls back to Rails' secret_key_base — see the reader + end + # Session-TTL and list-executor defaults: the :tokenized / :created_at # data-path semantics (a host preserving a pre-gem contract overrides these — # see each accessor's docs). @@ -556,6 +778,149 @@ def authority? auth_role.to_sym == :authority end + # The well-known prefixes the two metadata documents hang off. The resource + # path is INSERTED after these, never appended to the origin — see + # `oauth_protected_resource_path`. + PROTECTED_RESOURCE_WELL_KNOWN = "/.well-known/oauth-protected-resource" + AUTHORIZATION_SERVER_WELL_KNOWN = "/.well-known/oauth-authorization-server" + + # `oauth_resource_path` normalized for URL building: no trailing slash, and + # empty when the MCP endpoint IS the origin root (where there is no path + # component to insert). + # + # @return [String] e.g. "/mcp", or "" for a root-mounted endpoint. + def oauth_resource_path_component + path = oauth_resource_path.to_s.chomp("/") + path == "/" ? "" : path + end + + # Where the protected-resource metadata (RFC 9728) answers, and where + # `WWW-Authenticate` points. + # + # Path-SCOPED (`/.well-known/oauth-protected-resource/mcp`), never the bare + # path, because the bare ones are ORIGIN-GLOBAL: they describe the authorization + # server of the whole origin, which on a host already running an unrelated OAuth + # provider is that provider's claim to make, not an MCP server's. RFC 8414 §3.1 + # exists for this — "Using path components enables supporting multiple issuers + # per host" — and MCP's 2025-11-25 authorization spec gives a path-ful issuer no + # root fallback, so scoping is the correct reading rather than a workaround. + # + # A root-mounted endpoint has no path to insert and gets the bare paths, which is + # correct there: it really is that origin's only authorization server. + # + # @return [String] + def oauth_protected_resource_path + "#{PROTECTED_RESOURCE_WELL_KNOWN}#{oauth_resource_path_component}" + end + + # Where the authorization-server metadata (RFC 8414) answers. Path-inserted for + # the same reason, and it MUST agree with the issuer: a client constructs this + # URL from the issuer it was given. + # + # @return [String] + def oauth_authorization_server_path + "#{AUTHORIZATION_SERVER_WELL_KNOWN}#{oauth_resource_path_component}" + end + + # Whether the OAuth authorization bridge is live: its routes are drawn, and the + # authority transport advertises it on a 401 via `WWW-Authenticate`. + # + # Gated on three conditions, each for its own reason. + # + # AUTHORITY-ONLY, because the flow hands back a token this app itself + # authenticates — a satellite's tokens belong to its central app, so there is + # nothing here for it to authorize against. + # + # A `token_authenticator` must be set, because the bridge cannot function + # without one: it verifies the pasted token through it on both legs. Gated + # rather than left to fail at request time so a misconfigured host serves no + # bridge at all, instead of an authorization page that accepts an operator's + # token and then errors — the sibling introspection endpoint fails safe the + # same way. + # + # An `oauth_signing_secret` must resolve, for the same reason: without one the + # bridge cannot seal a code's payload, and it must not fall back to sealing + # with something weaker. A Rails host gets `secret_key_base` for free. + # + # And at least one redirect target must be named — an allowlist entry, or the + # loopback switch — so the bridge cannot be running without a bound answer to + # "who may receive a code". Both are empty/off by default, which is what makes + # an unconfigured host byte-identical to one without the bridge. + # + # NOT gated on a shared `cache_store`, deliberately, even though a per-worker + # one breaks the flow (see `oauth_per_process_cache_store?`): a MemoryStore is + # correct in a single process, `Rails.cache` IS a MemoryStore in a stock + # development environment, and the gem cannot see the worker count. Gating + # would make the bridge undevelopable locally to prevent a production mistake, + # so that one is a loud warning at boot instead. + # + # @return [Boolean] + def oauth_bridge? + return false unless authority? + return false if token_authenticator.nil? + return false if oauth_signing_secret.to_s.empty? + + Array(oauth_allowed_redirect_uris).any? || !!oauth_allow_loopback_redirects + end + + # Why a given allowlist entry must not or cannot receive a code, or nil if it + # may. A lint over values the HOST wrote, not a boundary against an attacker — + # which is why naming bad schemes is sound here and would not be at request + # time: nothing an attacker sends reaches this list, so there is no unlisted + # scheme for them to slip through. (Request-time policy takes the opposite + # shape for exactly that reason — see + # McpToolkit::Oauth::ControllerMethods#mcp_oauth_loopback_redirect_uri?.) + def oauth_redirect_uri_problem(uri) + parsed = oauth_parse_redirect_uri(uri) + return "it is not a valid URI" if parsed.nil? + + scheme = parsed.scheme&.downcase + return "it names no scheme" if scheme.nil? + return "it carries a fragment, which OAuth forbids on a redirect_uri" unless parsed.fragment.nil? + unless parsed.opaque.nil? + return "it is opaque (#{scheme}:#{parsed.opaque}) so it cannot carry the code — " \ + "write it with a path, e.g. #{scheme}:/#{parsed.opaque}" + end + + oauth_redirect_scheme_problem(scheme, parsed) + end + + # Schemes a browser may treat as script or local content. A code handed to one + # is at best lost and at worst executed; no client legitimately registers one. + ACTIVE_CONTENT_SCHEMES = %w[javascript data vbscript file blob about view-source].freeze + + def oauth_redirect_scheme_problem(scheme, parsed) + if ACTIVE_CONTENT_SCHEMES.include?(scheme) + return "#{scheme}: is not a redirect target — a browser treats it as script or local content" + end + return nil unless scheme == "http" + # RFC 8252 §7.3: cleartext is fine to a loopback address, which never leaves + # the operator's machine. Anywhere else it puts the code on the wire. + return nil if McpToolkit::Oauth.loopback_host?(parsed.host) + + "it is cleartext http to a remote host, which would put the authorization code on the wire — " \ + "use https (cleartext is only accepted for loopback)" + end + + def oauth_parse_redirect_uri(uri) + URI.parse(uri) + rescue URI::InvalidURIError + nil + end + + # Whether `cache_store` is per-process, which the bridge cannot survive on a + # multi-worker deployment: a code written on the worker that ran leg 1 is + # invisible to the worker that runs leg 2, so the exchange reads nil and + # answers `invalid_grant` — roughly (N-1)/N of the time, AFTER the operator has + # pasted a live token, and intermittently enough to read as a fluke rather than + # a misconfiguration. (It also quietly voids the payload sealing: a per-process + # store has no snapshot to steal, and its key would be in the same heap.) + # + # Fine in one process, which is why this warns rather than gates. + def oauth_per_process_cache_store? + cache_store.is_a?(ActiveSupport::Cache::MemoryStore) + end + # Full introspection URL the satellite POSTs to. Raises a clear error if the # central URL was never configured. def introspect_url diff --git a/lib/mcp_toolkit/engine.rb b/lib/mcp_toolkit/engine.rb index 5d9304a..5c70a31 100644 --- a/lib/mcp_toolkit/engine.rb +++ b/lib/mcp_toolkit/engine.rb @@ -36,4 +36,19 @@ class McpToolkit::Engine < Rails::Engine # reload so a changed parent (or a reloaded app parent class) takes effect on the # next reference. Runs before `:eager_load!`, so the fresh classes exist for it. config.to_prepare { McpToolkit.reset_engine_controllers! } + + # The OAuth bridge takes the operator's live access token in a POST body, and + # Rails logs request parameters at INFO. Filtering it is therefore the gem's + # business, not a deployment note: a `rails new` app happens to ship a `:token` + # entry that covers `access_token` by substring, but that is a host default this + # gem does not own and an `--api` or hand-rolled host may not have. Additive, so + # a host's own list is untouched, and harmless when the bridge is off. + # + # `code_verifier` is belt-and-braces (a logged verifier is worthless once the + # code is spent, and the code is burnt on read). `code` is deliberately NOT + # filtered: it is single-use, useless without the verifier, and matching it + # would filter every `country_code`/`state_code` a host logs. + initializer "mcp_toolkit.filter_oauth_parameters" do |app| + app.config.filter_parameters += %i[access_token code_verifier] + end end diff --git a/lib/mcp_toolkit/engine_controllers.rb b/lib/mcp_toolkit/engine_controllers.rb index a6e3d62..db0568b 100644 --- a/lib/mcp_toolkit/engine_controllers.rb +++ b/lib/mcp_toolkit/engine_controllers.rb @@ -28,7 +28,7 @@ module McpToolkit # The controllers built directly under McpToolkit (the engine's routes point at # these). McpToolkit::Authority::ServerController is built alongside them but is # fetched through McpToolkit::Authority's own const_missing. - ENGINE_CONTROLLER_NAMES = %i[ServerController TokensController].freeze + ENGINE_CONTROLLER_NAMES = %i[ServerController TokensController OauthController].freeze # (Re)builds the engine controllers + the authority base from the current # config. Idempotent: an existing constant is replaced so a rebuild reflects a @@ -37,6 +37,11 @@ def self.build_engine_controllers! parent = config.parent_controller.constantize define_controller(self, :ServerController, build_server_controller(parent)) define_controller(self, :TokensController, build_tokens_controller(parent)) + # Only when the bridge is on — matching its routes, which are equally gated. + # Its parent (default ActionController::Base) would otherwise be constantized + # on every host, pulling view machinery into an API-only app that never + # enables the bridge, and breaking a non-Rails host outright. + define_controller(self, :OauthController, build_oauth_controller) if config.oauth_bridge? define_controller(Authority, :ServerController, build_authority_server_controller(parent)) ServerController end @@ -44,7 +49,7 @@ def self.build_engine_controllers! # Undefines the built controllers so the next reference rebuilds them from the # then-current config. Called from the engine's `to_prepare` on every reload. def self.reset_engine_controllers! - [[self, :ServerController], [self, :TokensController], [Authority, :ServerController]].each do |mod, name| + ENGINE_CONTROLLER_NAMES.map { |name| [self, name] }.push([Authority, :ServerController]).each do |mod, name| mod.send(:remove_const, name) if mod.const_defined?(name, false) end end @@ -96,12 +101,74 @@ def mcp_extract_token end end + # The OAuth authorization bridge the engine mounts at `/oauth/*` and the + # host draws at the two `.well-known` metadata paths. + # + # Built from `config.oauth_parent_controller`, NOT the `parent_controller` its + # siblings use: the transport is a JSON-only endpoint that a host rightly points + # at ActionController::API, which cannot render an HTML view — and this + # controller's authorization page is one. Sharing the parent would force a host + # to weaken its transport's superclass just to enable the bridge. Read lazily + # here, like the rest, so the host's initializer/to_prepare has already run. + def self.build_oauth_controller + warn_about_per_process_cache_store + Class.new(config.oauth_parent_controller.constantize) { include McpToolkit::Oauth::ControllerMethods } + end + + # Said once per boot (this runs from the engine's `to_prepare`), because the + # failure it predicts is otherwise hard to read as a misconfiguration at all: + # with a per-process cache the two legs of a flow land on different workers, so + # the exchange fails (N-1)/N of the time — intermittently, and only AFTER the + # operator has already pasted a live token. A warning rather than a gate, + # because one process is genuinely fine and `Rails.cache` is a MemoryStore in a + # stock development environment. + def self.warn_about_per_process_cache_store + return unless config.oauth_per_process_cache_store? + + config.logger&.warn( + "[mcp_toolkit] The OAuth bridge is enabled but config.cache_store is an in-process MemoryStore. " \ + "That is safe in a single process only: on a multi-worker deployment an authorization code issued " \ + "by one worker is invisible to the worker that redeems it, so the flow fails intermittently AFTER " \ + "the operator has pasted their token. Point config.cache_store at a shared store (Rails.cache " \ + "backed by Redis/Memcached) before running the bridge in production." + ) + end + # The AUTHORITY base controller a host subclasses (the recommended path for a # host whose rate-limit/usage/account hooks touch app models). def self.build_authority_server_controller(parent) Class.new(parent) { include McpToolkit::Authority::ControllerMethods } end + # Draws the OAuth bridge's two metadata documents. A `/.well-known/*` path + # cannot be drawn by an engine mounted under a path, so it has to live in the + # host's own route set. The host calls this at the TOP LEVEL — not inside a + # locale/format/constraint scope, which would prefix the paths out of view: + # + # # config/routes.rb + # Rails.application.routes.draw do + # McpToolkit.draw_oauth_metadata_routes(self) + # mount McpToolkit::Engine => "/mcp" + # # ... + # end + # + # The paths are PATH-SCOPED to the engine's mount + # (`/.well-known/oauth-protected-resource/mcp`), so this claims NOTHING + # origin-global and cannot collide with an OAuth provider the host already runs + # — see Configuration#oauth_protected_resource_path for why that matters. A host + # mounted at its origin root gets the bare paths instead, which is correct there. + # + # A no-op unless the bridge is configured, so the call can sit in a host's routes + # unconditionally across environments. + def self.draw_oauth_metadata_routes(mapper) + return unless config.oauth_bridge? + + mapper.get config.oauth_protected_resource_path, + to: "mcp_toolkit/oauth#protected_resource", format: false + mapper.get config.oauth_authorization_server_path, + to: "mcp_toolkit/oauth#authorization_server", format: false + end + # Removes an existing same-named constant (avoiding a redefinition warning on a # rebuild) before setting the freshly-built class. def self.define_controller(mod, name, klass) diff --git a/lib/mcp_toolkit/oauth.rb b/lib/mcp_toolkit/oauth.rb new file mode 100644 index 0000000..50a2775 --- /dev/null +++ b/lib/mcp_toolkit/oauth.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +# Namespace for the OAuth authorization bridge (McpToolkit::Oauth::ControllerMethods), +# and the home of the one policy value both the request path and the config path +# need to agree on. +module McpToolkit::Oauth + # RFC 8252 §7.3 loopback hosts — the only hosts a code may be sent to over + # cleartext, and the only redirect target accepted without being named. The RFC + # prefers the IP literals over the name (a name is only as trustworthy as the + # resolver — §8.3), but real clients use all three, and RFC 6761 has OS + # resolvers and browsers hardcode `localhost` to loopback. + # + # Lives here rather than on the concern because Configuration reads it too, to + # decide whether an allowlisted `http://` entry is a mistake: cleartext is fine + # to an address that never leaves the operator's machine, and puts the code on + # the wire anywhere else. One list, so the two paths cannot drift apart. + LOOPBACK_HOSTS = ["127.0.0.1", "::1", "localhost"].freeze + + # `[::1]` arrives bracketed from `URI`; compare against the bare form. + def self.loopback_host?(host) + LOOPBACK_HOSTS.include?(host.to_s.downcase.delete_prefix("[").delete_suffix("]")) + end +end diff --git a/lib/mcp_toolkit/oauth/controller_methods.rb b/lib/mcp_toolkit/oauth/controller_methods.rb new file mode 100644 index 0000000..22f44e0 --- /dev/null +++ b/lib/mcp_toolkit/oauth/controller_methods.rb @@ -0,0 +1,426 @@ +# frozen_string_literal: true + +# The AUTHORITY-side OAuth 2.1 authorization bridge (routes: config/routes.rb; +# setup + rationale: README). +# +# NOT an identity provider, and reading it as a half-built one will mislead. It +# mints no credential, stores no client, models no consent, issues no refresh +# token. It is a standards-shaped envelope around tokens the host ALREADY issues +# by its own means, for clients that will only authenticate by discovering an +# authorization server and running a browser flow: the page asks an operator to +# paste a token they hold, and the `access_token` returned IS that token, verified +# through the same `config.token_authenticator` the transport uses. Scopes, +# expiry, revocation and tenancy stay with the host; nothing here widens reach. +# +# So the stubs are deliberate, not unfinished: no endpoint reads the `client_id` +# it hands out (a public client's identifier is self-asserted and gates nothing); +# pasting a token you already hold IS the grant; and the pasted token's own expiry +# is the real lifetime, so a client re-runs the flow rather than refreshing a +# shadow of it. +# +# Two things are NOT mocked, because faking them would be a vulnerability rather +# than a skipped ceremony: `redirect_uri` is checked against the host's policy on +# BOTH legs (an unvetted REMOTE target is an open redirect handing out +# authorization codes — see Configuration#oauth_allowed_redirect_uris for the +# attack it stops), and the PKCE `code_verifier` is verified. +module McpToolkit::Oauth::ControllerMethods + extend ActiveSupport::Concern + + CODE_CACHE_PREFIX = "mcp_toolkit:oauth:code:" + CODE_BYTES = 32 + + # Query parameters the callback response owns: whatever a client put in its own + # redirect_uri, these are set by the redirect and not carried over from it. + RESPONSE_OWNED_QUERY_KEYS = %w[code state].freeze + + # RFC 7636 §4.1: 43–128 unreserved characters. The challenge is §4.2's + # base64url of a SHA-256, which is always exactly 43 of the same alphabet. + PKCE_VALUE = /\A[A-Za-z0-9\-._~]{43,128}\z/ + + included do + # Safe to disable: the token endpoint is called server-to-server without a CSRF + # token, and `approve` never acts on ambient authority — it reads no session and + # no cookie, only a pasted token, redirecting to a host-permitted URI. (The GET + # leg does SET a session cookie, because `form_tag` emits an authenticity + # token; nothing ever reads it back.) + protect_from_forgery with: :null_session if respond_to?(:protect_from_forgery) + + # A before_action, not a guard clause in each action, because a callback runs + # even when the action itself does not. `authorize` is a common enough method + # name that a gem patching ActionController::Base with one would knock this + # action out of Rails' `action_methods` — and Rails would then serve + # `authorize.html.erb` by implicit render, skipping a body guard entirely and + # showing an attacker's `redirect_uri` a paste page. Here the check cannot be + # routed around. + # Raises rather than skipping when the parent has no filter chain. Skipping + # would leave both browser legs unguarded — an open redirect issuing codes to + # any URI — and it is the same shape as the scheme denylist this branch + # removed for failing open. A guard that installs itself conditionally is not + # a guard. + unless respond_to?(:before_action) + raise McpToolkit::Errors::ConfigurationError, + "#{name || "The OAuth bridge's parent"} has no `before_action`; " \ + "config.oauth_parent_controller must be an ActionController with a filter chain." + end + + before_action :mcp_oauth_validate_request!, only: %i[authorize approve] + end + + # `resource` MUST equal the MCP endpoint URL as the operator typed it into the + # client, hence derived from the live request origin rather than pinned. + def protected_resource + mcp_oauth_forbid_caching + render json: { + resource: mcp_oauth_resource_url, + authorization_servers: [mcp_oauth_issuer], + bearer_methods_supported: ["header"] + } + end + + # S256 because clients send a `code_challenge` regardless; `none` because the + # clients here are public and unverified. + def authorization_server + mcp_oauth_forbid_caching + render json: { + issuer: mcp_oauth_issuer, + authorization_endpoint: mcp_oauth_endpoint_url("authorize"), + token_endpoint: mcp_oauth_endpoint_url("token"), + registration_endpoint: mcp_oauth_endpoint_url("register"), + response_types_supported: ["code"], + grant_types_supported: ["authorization_code"], + code_challenge_methods_supported: ["S256"], + token_endpoint_auth_methods_supported: ["none"] + } + end + + # Stateless: persisting a `client_id` nothing reads would only grow a table of + # strings the bridge never consults. + def register + render json: { + client_id: SecureRandom.uuid, + token_endpoint_auth_method: "none", + grant_types: ["authorization_code"], + response_types: ["code"] + }, status: :created + end + + # `formats: [:html]` because there is only an HTML template and `Accept` picks + # the format just as a `.json` suffix would — without it, `Accept: + # application/json` raises MissingTemplate on an unauthenticated endpoint. + def authorize + render :authorize, layout: false, formats: [:html] + end + + # The token is verified here, not only at exchange, so a typo fails on the page + # the operator is looking at. + def approve + access_token = params[:access_token].to_s + return mcp_oauth_reject_paste if mcp_oauth_authenticate(access_token).nil? + + # 303, not Rails' default 302: this POST carried the operator's token in its + # body, and only 303 unambiguously tells the browser to fetch the redirect + # target with GET and no body. A 302 leaves re-sending it to the client's + # discretion, which would hand the token itself to the callback (RFC 9700 + # §4.12). + redirect_to mcp_oauth_callback_url(mcp_oauth_issue_code(access_token)), + allow_other_host: true, status: :see_other + end + + def token + return mcp_oauth_render_token_error("unsupported_grant_type") unless params[:grant_type] == "authorization_code" + # Shape first, and BEFORE the code is consumed: a request that could never + # verify shouldn't cost a legitimate client its code. A well-formed but WRONG + # verifier still burns it below — that is deliberate, and the only thing + # stopping someone who intercepted a code from retrying verifiers against it. + return mcp_oauth_render_token_error("invalid_request") unless mcp_oauth_verifier_well_formed? + + payload = mcp_oauth_consume_code(params[:code].to_s) + return mcp_oauth_render_token_error("invalid_grant") if payload.nil? + return mcp_oauth_render_token_error("invalid_grant") unless mcp_oauth_exchange_valid?(payload) + + access_token = payload[:access_token].to_s + return mcp_oauth_render_token_error("invalid_grant") if mcp_oauth_authenticate(access_token).nil? + + mcp_oauth_forbid_caching + render json: { access_token:, token_type: "Bearer" } + end + + private + + def mcp_oauth_config + McpToolkit.config + end + + # ---- request validation --------------------------------------------------- + + # Halts both legs before their action runs. Both problems RENDER rather than + # bounce an OAuth error back to the caller: a disallowed redirect_uri must never + # be redirected TO — that is the attack. + def mcp_oauth_validate_request! + problem = mcp_oauth_request_problem + mcp_oauth_render_bad_request(problem) if problem + end + + def mcp_oauth_request_problem + return mcp_oauth_reject_redirect_uri unless mcp_oauth_redirect_uri_allowed? + return "Missing or unsupported PKCE code_challenge." unless mcp_oauth_code_challenge_supported? + + nil + end + + # Exact matching makes a legitimate client rejected over a trailing slash look + # identical to an attack, so log the offered value (a public callback, never a + # credential) — otherwise an operator is left guessing what to allowlist. + def mcp_oauth_reject_redirect_uri + mcp_oauth_config.logger&.warn( + "[mcp_toolkit] OAuth authorize rejected: redirect_uri #{params[:redirect_uri].inspect} is not in " \ + "config.oauth_allowed_redirect_uris (#{Array(mcp_oauth_config.oauth_allowed_redirect_uris).inspect}) " \ + "and is not a native-client target permitted by config.oauth_allow_loopback_redirects " \ + "(#{mcp_oauth_config.oauth_allow_loopback_redirects ? "enabled" : "disabled"})" + ) + "Unregistered redirect_uri." + end + + # Every target must be named exactly, with ONE exception: loopback, whose port + # cannot be named ahead of time. See `mcp_oauth_loopback_redirect_uri?`. + def mcp_oauth_redirect_uri_allowed? + redirect_uri = params[:redirect_uri].to_s + return false if redirect_uri.empty? + return true if Array(mcp_oauth_config.oauth_allowed_redirect_uris).include?(redirect_uri) + + mcp_oauth_loopback_redirect_uri?(redirect_uri) + end + + # RFC 8252 §7.3 loopback — the one target accepted unnamed, and only http(s) to + # a loopback host: NOT private-use schemes, however local they look. See + # Configuration#oauth_allow_loopback_redirects for why that line is drawn there. + # + # Judged on the PARSED URI, never the string, because `host` is what a browser + # resolves: `http://127.0.0.1@evil.example/` (userinfo — host is evil.example) + # and `http://127.0.0.1.evil.example/` are both correctly remote. A fragment is + # refused because OAuth forbids one on a redirect_uri. + def mcp_oauth_loopback_redirect_uri?(redirect_uri) + return false unless mcp_oauth_config.oauth_allow_loopback_redirects + + uri = mcp_oauth_parse_uri(redirect_uri) + return false if uri.nil? + return false unless %w[http https].include?(uri.scheme&.downcase) + return false unless uri.fragment.nil? + + mcp_oauth_loopback_host?(uri.host) + end + + def mcp_oauth_parse_uri(value) + URI.parse(value) + rescue URI::InvalidURIError + nil + end + + # `URI` keeps the brackets on an IPv6 literal (`[::1]`); strip them so the + # literal compares against the bare form clients actually send. + def mcp_oauth_loopback_host?(host) + McpToolkit::Oauth.loopback_host?(host) + end + + # RFC 7636 §4.1/§4.2 shapes, not just presence. This cannot make a client's + # PKCE strong — the challenge is a 43-char digest whatever the verifier was, so + # a client that chose a one-character verifier is indistinguishable here and has + # only defeated its own protection. It is enforced because a public gem should + # not quietly accept what the spec forbids, and because a malformed value can + # then be refused early rather than after a paste. + def mcp_oauth_code_challenge_supported? + params[:code_challenge].to_s.match?(PKCE_VALUE) && params[:code_challenge_method].to_s == "S256" + end + + def mcp_oauth_verifier_well_formed? + params[:code_verifier].to_s.match?(PKCE_VALUE) + end + + # ---- authorization codes -------------------------------------------------- + + # The whole "authorization server" state: one cache entry, short-lived, bound + # to the challenge and redirect it was issued for. + # + # The entry is keyed by the code's DIGEST and its payload is sealed, so a dump + # of the store yields nothing on its own. Worth the few lines because what is + # parked there is not the short-lived credential an authorization server would + # normally hold: it is the operator's pre-existing, long-lived, full-scope + # token, in a store a host is told to point at its shared `Rails.cache`. + def mcp_oauth_issue_code(access_token) + code = SecureRandom.urlsafe_base64(CODE_BYTES) + payload = { + access_token:, code_challenge: params[:code_challenge].to_s, redirect_uri: params[:redirect_uri].to_s + } + mcp_oauth_config.cache_store.write( + mcp_oauth_code_key(code), + mcp_oauth_encryptor(code).encrypt_and_sign(JSON.generate(payload)), + expires_in: mcp_oauth_config.oauth_authorization_code_ttl + ) + code + end + + # Single-use for real: the DELETE decides, not the read. `Cache::Store#delete` + # answers whether the entry was still there, so of two concurrent redemptions + # exactly one proceeds. (The race was never exploitable — both would return the + # same token, and both need the verifier — but the guarantee is cheap to keep + # honest, and a code is burnt even when the exchange that follows fails.) + def mcp_oauth_consume_code(code) + return nil if code.empty? + + key = mcp_oauth_code_key(code) + blob = mcp_oauth_config.cache_store.read(key) + return nil if blob.nil? + return nil unless mcp_oauth_config.cache_store.delete(key) + + mcp_oauth_decrypt_payload(code, blob) + end + + def mcp_oauth_decrypt_payload(code, blob) + JSON.parse(mcp_oauth_encryptor(code).decrypt_and_verify(blob), symbolize_names: true) + rescue ActiveSupport::MessageEncryptor::InvalidMessage, JSON::ParserError + nil + end + + def mcp_oauth_code_key(code) + "#{CODE_CACHE_PREFIX}#{Digest::SHA256.hexdigest(code)}" + end + + # HMAC because two independent inputs are being combined (why the secret is one + # of them: Configuration#oauth_signing_secret). No password-stretching — both + # are already high-entropy, so a PBKDF2 run per request would buy nothing. + # + # Cipher and serializer are pinned, not inherited: the gem supports + # ActiveSupport >= 6.1, where both defaults are Rails-configuration-dependent + # (`:marshal`, and 7.1+'s `:json_allow_marshal`). The payload is already a JSON + # String, so NullSerializer costs nothing and leaves JSON.parse the only parser + # this code hands the plaintext to. + # + # It does NOT buy immunity from a writable cache: `Cache::Store` marshals the + # entry itself, so a store that can be written to is a code-execution problem + # before anything here is reached. This is determinism across the supported + # range, not a mitigation. + def mcp_oauth_encryptor(code) + key = OpenSSL::HMAC.digest("SHA256", mcp_oauth_signing_secret, "#{CODE_CACHE_PREFIX}key:#{code}") + ActiveSupport::MessageEncryptor.new( + key, cipher: "aes-256-gcm", serializer: ActiveSupport::MessageEncryptor::NullSerializer + ) + end + + def mcp_oauth_signing_secret + mcp_oauth_config.oauth_signing_secret.tap do |secret| + raise McpToolkit::Errors::ConfigurationError, "oauth_signing_secret is not configured" if secret.to_s.empty? + end + end + + def mcp_oauth_exchange_valid?(payload) + payload[:redirect_uri] == params[:redirect_uri].to_s && + mcp_oauth_pkce_valid?(params[:code_verifier].to_s, payload[:code_challenge].to_s) + end + + # S256: base64url(sha256(verifier)), unpadded. Packed rather than via base64 so + # the gem needs no extra dependency. + def mcp_oauth_pkce_valid?(verifier, challenge) + return false if verifier.empty? || challenge.empty? + + digest = [Digest::SHA256.digest(verifier)].pack("m0").tr("+/", "-_").delete("=") + ActiveSupport::SecurityUtils.secure_compare(digest, challenge) + end + + # ---- token verification --------------------------------------------------- + + # The same authenticator the transport authenticates every MCP request with — + # this bridge introduces no second notion of a valid token. + def mcp_oauth_authenticate(access_token) + return nil if access_token.empty? + + McpToolkit::Auth::Authority.authenticate(access_token, config: mcp_oauth_config) + end + + # ---- urls ----------------------------------------------------------------- + + # Deliberately path-ful: a client path-INSERTS this to find the metadata, which + # is what keeps the bridge off the origin-global bare path (see + # Configuration#oauth_protected_resource_path). Issuer path == resource path, so + # a client derives the same URL from either. + def mcp_oauth_issuer + mcp_oauth_resource_url + end + + def mcp_oauth_resource_url + "#{request.base_url}#{mcp_oauth_config.oauth_resource_path_component}" + end + + def mcp_oauth_endpoint_url(action) + "#{mcp_oauth_resource_url}/oauth/#{action}" + end + + # Sets `code` (and echoes `state`) on the client's redirect_uri, preserving any + # other query it already carries. + # + # SETS, not appends: a loopback redirect_uri is not an exact-matched string, so + # a caller can put `?code=…` in it themselves. Appending would emit + # `?code=theirs&code=ours` and leave which one wins to the client's parser. + # Dropping any inbound `code`/`state` keeps the response OAuth-shaped whatever + # was passed in. + # + # The base is taken as the checked string rather than round-tripped through + # `URI`, so the host part of what is emitted is byte-identical to what the + # policy approved. The query IS re-encoded (`?a=1?b=2` normalises to + # `?a=1%3Fb%3D2`), which is the point — that is where `code` gets stripped. + def mcp_oauth_callback_url(code) + redirect_uri = params[:redirect_uri].to_s + base, _, existing = redirect_uri.partition("?") + pairs = mcp_oauth_preserved_query_pairs(existing) + pairs << ["code", code] + pairs << ["state", params[:state].to_s] if params[:state].present? + "#{base}?#{URI.encode_www_form(pairs)}" + end + + # A client's own query survives; the two parameters this response owns do not, + # whoever put them there. The rescue is a backstop, not a designed path: both + # sanctioned redirect_uris are `URI.parse`d before they reach here, which + # already refuses what `decode_www_form` would raise on. + def mcp_oauth_preserved_query_pairs(query) + return [] if query.empty? + + URI.decode_www_form(query).reject { |pair| RESPONSE_OWNED_QUERY_KEYS.include?(pair.first) } + rescue ArgumentError + [] + end + + # ---- responses ------------------------------------------------------------ + + # 422 as an integer, not a symbol: the gemspec pins no Rack floor, and neither + # symbol spans the range it allows — `:unprocessable_content` raises below Rack + # 3.1, `:unprocessable_entity` is deprecated above it. A symbol that raises here + # would turn a mistyped paste into an unauthenticated 500 on the one page whose + # job is to say "that token isn't valid". + def mcp_oauth_reject_paste + @mcp_oauth_error = "That access token is not valid, or it has expired or been revoked." + render :authorize, layout: false, formats: [:html], status: 422 + end + + def mcp_oauth_render_bad_request(message) + render plain: message, status: :bad_request + end + + # Applied to the token response, where RFC 6749 §5.1 makes both headers a MUST + # for anything carrying a token, and to both metadata documents, where the + # reason is subtler: they name the `authorization_endpoint` an operator will be + # sent to and are built from the live request origin (`request.base_url`, which + # honours `X-Forwarded-Host`), so a shared cache that stored one keyed only by + # path could serve every client an origin an attacker chose — with the document + # itself vouching for it. A host MUST pin `config.hosts` — Rails' + # HostAuthorization then rejects a forged header before it reaches here, but + # Rails does NOT do that for you: `config.hosts` is populated in development and + # left EMPTY in production, where an empty list means no checking at all. This + # header is the half that does not depend on the host getting that right. + def mcp_oauth_forbid_caching + response.headers["Cache-Control"] = "no-store" + response.headers["Pragma"] = "no-cache" + end + + def mcp_oauth_render_token_error(code) + render json: { error: code }, status: :bad_request + end +end diff --git a/lib/mcp_toolkit/version.rb b/lib/mcp_toolkit/version.rb index b0c7d2f..edfcda2 100644 --- a/lib/mcp_toolkit/version.rb +++ b/lib/mcp_toolkit/version.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true module McpToolkit - VERSION = "0.5.0" + VERSION = "0.6.0" end diff --git a/spec/mcp_toolkit/authority/controller_methods_spec.rb b/spec/mcp_toolkit/authority/controller_methods_spec.rb index 59a1523..019f2c2 100644 --- a/spec/mcp_toolkit/authority/controller_methods_spec.rb +++ b/spec/mcp_toolkit/authority/controller_methods_spec.rb @@ -30,7 +30,8 @@ def initialize end def request - @request ||= Struct.new(:headers, :body).new(request_headers, StringIO.new(request_body.to_s)) + @request ||= Struct.new(:headers, :body, :base_url) + .new(request_headers, StringIO.new(request_body.to_s), "https://mcp.example.test") end def response @@ -251,6 +252,47 @@ def rpc(method, params = {}, id: 1) expect(controller.send(:mcp_principal)).to be(token) expect(token).to have_received(:touch_last_used!) end + + # A hosted MCP client will not start an authorization flow off a bare 401; this + # header is what tells it where to look. See McpToolkit::Oauth::ControllerMethods. + context "when the OAuth bridge is configured" do + before do + McpToolkit.config.auth_role = :authority + McpToolkit.config.oauth_allowed_redirect_uris = ["https://client.example/callback"] + # Rejects every token — the caller in these examples is unauthenticated — + # but its PRESENCE is part of what makes the bridge configured at all. + McpToolkit.config.token_authenticator = ->(_plaintext) { nil } + # Likewise: no Rails here, so the secret_key_base default cannot resolve. + McpToolkit.config.oauth_signing_secret = "spec-oauth-signing-secret-at-least-32-bytes-long" + end + + # Path-scoped: stating the location outright is also what keeps a client from + # probing the origin's bare, globally-meaningful well-known path. + it "challenges an unauthenticated caller with the path-scoped protected-resource metadata url" do + controller.send(:mcp_authenticate!) + + expect(controller.response.headers["WWW-Authenticate"]).to eq( + %(Bearer resource_metadata="https://mcp.example.test/.well-known/oauth-protected-resource/mcp") + ) + end + + # `base_url` honours X-Forwarded-Host, so it is caller-influenced: a quote + # in it would close the quoted-string and let a caller append auth-params of + # their own. A URL cannot legitimately contain one, so the challenge is + # withheld rather than emitted broken. + it "emits no challenge at all when the request origin contains a quote" do + controller.request.base_url = 'https://evil.example"; x="y' + controller.send(:mcp_authenticate!) + + expect(controller.response.headers).not_to have_key("WWW-Authenticate") + end + end + + it "leaves the 401 unchallenged when the bridge is not configured (an opted-out host is unaffected)" do + controller.send(:mcp_authenticate!) + + expect(controller.response.headers).not_to have_key("WWW-Authenticate") + end end # ---- session lifecycle ---------------------------------------------------- diff --git a/spec/mcp_toolkit/engine_spec.rb b/spec/mcp_toolkit/engine_spec.rb index 5d8c610..a1aa099 100644 --- a/spec/mcp_toolkit/engine_spec.rb +++ b/spec/mcp_toolkit/engine_spec.rb @@ -62,6 +62,15 @@ def self.after_action(*); end expect(McpToolkit::ServerController.include?(McpToolkit::Transport::ControllerMethods)).to be(false) end end + + # The bridge's parent (default ActionController::Base) must not be + # constantized on a host that never enables it: that would pull view + # machinery into an API-only app and break a non-Rails host outright. Its + # absence here is what proves the build is skipped — this whole suite runs + # without Rails, so a constantize would raise NameError. + it "does not build the OAuth controller when the bridge is off" do + expect(McpToolkit.const_defined?(:OauthController, false)).to be(false) + end end describe "McpToolkit::Engine routes" do @@ -81,8 +90,18 @@ def draw(&block) instance_eval(&block) end + # `format:` is recorded separately (see `drawn_formats`): the bridge's + # routes must disable Rails' optional format segment, and a recorder that + # swallowed the option would let that regress unseen. %i[post get delete].each do |verb| - define_method(verb) { |path, to:| @drawn << [verb, path, to] } + define_method(verb) do |path, to:, **options| + @drawn << [verb, path, to] + (@formats ||= {})[[verb, path]] = options[:format] + end + end + + def drawn_formats + @formats ||= {} end end.new end @@ -91,9 +110,23 @@ def draw(&block) # Default to :authority so the full endpoint set is asserted; the satellite # context below covers the gated-off case. let(:auth_role) { :authority } + # The OAuth bridge is off until a host names who may receive an authorization + # code, so it stays out of the default endpoint set. + let(:oauth_redirect_uris) { [] } + let(:oauth_allow_native) { false } + # The bridge also needs the authenticator it verifies a pasted token with; an + # authority always has one. Its own gate is asserted below. + let(:token_authenticator) { ->(_plaintext) { nil } } + # No Rails in this suite, so the secret_key_base default cannot resolve; the + # gate's own requirement for it is asserted separately below. + let(:oauth_signing_secret) { "spec-oauth-signing-secret-at-least-32-bytes-long" } before do McpToolkit.config.auth_role = auth_role + McpToolkit.config.oauth_allowed_redirect_uris = oauth_redirect_uris + McpToolkit.config.oauth_allow_loopback_redirects = oauth_allow_native + McpToolkit.config.token_authenticator = token_authenticator + McpToolkit.config.oauth_signing_secret = oauth_signing_secret recorder = route_recorder stub_const("Rails", Module.new) # The engine class body now also calls `config.to_prepare { ... }` (lazy @@ -102,6 +135,10 @@ def draw(&block) config_double = Class.new { def to_prepare(*); end }.new engine_base = Class.new do define_singleton_method(:isolate_namespace) { |_mod| } + # The engine also registers an initializer (filter_parameters for the + # bridge's token-bearing params); recorded, not run — this unit boots no app. + define_singleton_method(:initializer) { |name, &block| (@initializers ||= {})[name] = block } + define_singleton_method(:initializers) { @initializers ||= {} } end engine_base.define_singleton_method(:config) { config_double } stub_const("Rails::Engine", engine_base) @@ -138,6 +175,83 @@ def draw(&block) end end + oauth_routes = [ + [:get, "oauth/authorize", "oauth#authorize"], + [:post, "oauth/authorize", "oauth#approve"], + [:post, "oauth/token", "oauth#token"], + [:post, "oauth/register", "oauth#register"] + ] + + it "does NOT draw the OAuth bridge for an authority that has not opted in" do + expect(route_recorder.drawn).not_to include(*oauth_routes) + end + + context "when the OAuth bridge is configured on an authority" do + let(:oauth_redirect_uris) { ["https://client.example/callback"] } + + it "draws the bridge's endpoints" do + expect(route_recorder.drawn).to include(*oauth_routes) + end + + # Without this, Rails' optional `(.:format)` matches and + # `/mcp/oauth/authorize.json` reaches the action, finds no JSON template and + # 500s — an unauthenticated error for a format the bridge never speaks. + it "disables the format segment on every bridge endpoint" do + formats = oauth_routes.map { |verb, path, _| route_recorder.drawn_formats[[verb, path]] } + + expect(formats).to all(be(false)) + end + end + + # Allowing native clients names who may receive a code just as an allowlist + # entry does ("anything on my operators' machines"), so it is an opt-in in its + # own right — a host serving only desktop MCP clients needs no allowlist. + context "when an authority allows native clients but names no allowlist" do + let(:oauth_allow_native) { true } + + it "draws the bridge's endpoints" do + expect(route_recorder.drawn).to include(*oauth_routes) + end + end + + # The bridge verifies the pasted token through the authenticator on both legs, + # so without one it cannot work. Drawing no route at all beats an authorization + # page that takes an operator's token and then errors. + context "when an authority named a redirect target but configured no token_authenticator" do + let(:oauth_redirect_uris) { ["https://client.example/callback"] } + let(:token_authenticator) { nil } + + # Still an authority, so its introspection route is unaffected — it is only + # the bridge that goes away. + it "still draws no OAuth bridge" do + expect(route_recorder.drawn).not_to include(*oauth_routes) + end + end + + # Without a server-held secret the bridge cannot seal a code's payload, and + # must not fall back to sealing it with something weaker. A Rails host never + # sees this — it inherits secret_key_base. + context "when an authority has no signing secret to resolve" do + let(:oauth_redirect_uris) { ["https://client.example/callback"] } + let(:oauth_signing_secret) { nil } + + it "still draws no OAuth bridge" do + expect(route_recorder.drawn).not_to include(*oauth_routes) + end + end + + # The flow hands back a token this app authenticates itself; a satellite's + # tokens belong to its central app, so there is nothing for it to authorize + # against — an allowlist alone must not switch the bridge on. + context "when a satellite configures a redirect allowlist" do + let(:auth_role) { :satellite } + let(:oauth_redirect_uris) { ["https://client.example/callback"] } + + it "still draws no OAuth bridge" do + expect(route_recorder.drawn).to contain_exactly(*transport_routes) + end + end + it "subclasses ::Rails::Engine" do expect(McpToolkit::Engine.superclass).to eq(Rails::Engine) end diff --git a/spec/mcp_toolkit/oauth/bridge_end_to_end_spec.rb b/spec/mcp_toolkit/oauth/bridge_end_to_end_spec.rb new file mode 100644 index 0000000..82307b9 --- /dev/null +++ b/spec/mcp_toolkit/oauth/bridge_end_to_end_spec.rb @@ -0,0 +1,544 @@ +# frozen_string_literal: true + +require "spec_helper" + +# End-to-end spec for the OAuth authorization bridge, against a REAL Rails app. +# +# The sibling controller_methods_spec.rb drives the concern against a fake +# controller, which is fast and pins the logic — but a fake controller cannot +# prove the parts that only Rails does: that the routes are drawn where a client +# looks for them, that the authorization page actually RENDERS (the view is +# resolved from the engine's app/views through a dynamically built controller), +# that `redirect_to ..., allow_other_host: true` is permitted, and that the token +# handed back at the end genuinely authenticates a subsequent MCP call. A fake +# would only confirm the fake. +# +# Following engine_route_reload_spec.rb: booting a real Rails::Application +# in-process would irreversibly mutate global state (Rails.application, the +# Zeitwerk loader set eager_load_spec inspects, the engine's lazy route set) and +# contaminate this otherwise Rails-absent suite under random ordering. So the boot +# runs in an ISOLATED CHILD process that walks the whole flow exactly as a client +# would and prints the result as JSON for this process to assert on. +# +# Rails-only: railties/actionpack are a TEST dependency, so the group is skipped +# when Rails is unavailable. +rails_available = + begin + require "rails/version" + true + rescue LoadError + false + end + +RSpec.describe "OAuth bridge end to end", if: rails_available do + # The driver: boots a minimal app configured as an authority with the bridge on, + # then performs the client's exact sequence — discover, register, authorize, + # paste, exchange, and finally call the MCP endpoint with what came back. + # + # Named distinctly (not `DRIVER`): a constant assigned inside an RSpec block + # lands on Object, so a bare name would collide with the sibling + # engine_route_reload_spec's driver and silently hand one spec the other's boot. + OAUTH_BRIDGE_DRIVER = <<~'RUBY' + require "bundler/setup" + require "json" + require "tmpdir" + require "logger" + require "mcp_toolkit" + require "rails" + require "action_controller/railtie" + require "rack/test" + + REDIRECT_URI = "https://client.example/callback" + VALID_TOKEN = "tok_pasted_by_the_operator" + + # A duck-typed principal: what the authority path reads off a token. + Principal = Struct.new(:id) do + def authorized_for_scope?(_scope) = true + def default_account = Struct.new(:id).new(99) + def authorize_account(_candidate) = default_account + def superuser? = false + end + + McpToolkit.configure do |c| + # Mirror a real authority: the MCP transport is a JSON-only endpoint, so its + # parent is ActionController::API — which CANNOT render an HTML view. The + # bridge's page is one, and it must render anyway (it has its own parent). + c.parent_controller = "ActionController::API" + c.auth_role = :authority + c.server_name = "example-mcp" + c.oauth_allowed_redirect_uris = [REDIRECT_URI] + c.token_authenticator = ->(plaintext) { plaintext == VALID_TOKEN ? Principal.new(1) : nil } + end + + unless McpToolkit.const_defined?(:Engine, false) + load File.expand_path("lib/mcp_toolkit/engine.rb", Dir.pwd) + end + + app = Class.new(Rails::Application) do + config.eager_load = false + config.consider_all_requests_local = true + config.secret_key_base = "oauth-bridge-spec-secret-key-base" + config.logger = Logger.new(IO::NULL) + config.root = Dir.mktmpdir("mcp_toolkit_oauth_spec") + config.hosts.clear # Rack::Test requests arrive as example.org + end + + # A host that ALREADY runs its own OAuth provider at the conventional + # top-level /oauth/* paths (the shape Doorkeeper draws). The bridge must not + # touch any of it. + class HostOauthController < ActionController::Base + def authorize = render(plain: "HOST_AUTHORIZE") + def token = render(plain: "HOST_TOKEN") + def token_info = render(plain: "HOST_TOKEN_INFO") + end + + app.initialize! + app.routes.draw do + # Drawn BEFORE the bridge, as in a host whose use_doorkeeper call sits at the + # top of its route set — first match wins, so this is the order that would + # expose a collision. + get "/oauth/authorize", to: "host_oauth#authorize" + post "/oauth/token", to: "host_oauth#token" + get "/oauth/token/info", to: "host_oauth#token_info" + + McpToolkit.draw_oauth_metadata_routes(self) + mount McpToolkit::Engine => "/mcp" + end + + session = Rack::Test::Session.new(Rack::MockSession.new(app)) + result = {} + + # 0. The host's existing OAuth provider must answer exactly as before. + session.get("/oauth/authorize") + result["host_authorize"] = session.last_response.body + session.post("/oauth/token") + result["host_token"] = session.last_response.body + session.get("/oauth/token/info") + result["host_token_info"] = session.last_response.body + + # 0b. Every path the bridge claims OUTSIDE its own engine mount. Anything here + # beyond the two metadata documents would be a land-grab on the host's routes. + result["host_level_paths"] = app.routes.routes.filter_map { |r| + path = r.path.spec.to_s.sub(/\(\.:format\)\z/, "") + next unless r.defaults[:controller].to_s.start_with?("mcp_toolkit/") + path + } + # And the engine's own set, which must stay under the mount. + result["engine_paths"] = McpToolkit::Engine.routes.routes.map { |r| + r.path.spec.to_s.sub(/\(\.:format\)\z/, "") + } + result["oauth_controller_parent"] = McpToolkit::OauthController.superclass.name + result["server_controller_parent"] = McpToolkit::ServerController.superclass.name + + # Nothing above configured a signing secret: on a Rails host it must resolve + # to secret_key_base by itself, or every host would have to wire one. + result["signing_secret_resolves_from_rails"] = + McpToolkit.config.oauth_signing_secret == app.secret_key_base + result["bridge_enabled"] = McpToolkit.config.oauth_bridge? + + # 1. An unauthenticated MCP call must point the client at the metadata. + session.post("/mcp", JSON.generate({ jsonrpc: "2.0", id: 1, method: "initialize", params: {} }), + "CONTENT_TYPE" => "application/json") + result["unauthenticated_status"] = session.last_response.status + result["www_authenticate"] = session.last_response.headers["WWW-Authenticate"] + + # 2 + 3. Discovery, at the PATH-SCOPED locations (RFC 9728 §3.1 / RFC 8414 §3.1). + session.get("/.well-known/oauth-protected-resource/mcp") + result["prm_status"] = session.last_response.status + result["prm"] = JSON.parse(session.last_response.body) + result["prm_cache_control"] = session.last_response.headers["Cache-Control"] + + session.get("/.well-known/oauth-authorization-server/mcp") + result["as_status"] = session.last_response.status + result["as"] = JSON.parse(session.last_response.body) + result["as_cache_control"] = session.last_response.headers["Cache-Control"] + + # The bare, ORIGIN-GLOBAL well-known paths must stay untouched — they describe + # the whole origin's authorization server, which on a host already running one + # is that provider's to claim. Nothing here may answer them. + session.get("/.well-known/oauth-protected-resource") + result["bare_prm_status"] = session.last_response.status + session.get("/.well-known/oauth-authorization-server") + result["bare_as_status"] = session.last_response.status + + # 4. Client registration. + session.post("/mcp/oauth/register", JSON.generate({ redirect_uris: [REDIRECT_URI] }), + "CONTENT_TYPE" => "application/json") + result["register_status"] = session.last_response.status + result["register"] = JSON.parse(session.last_response.body) + + # 5. The authorization page — does the view actually render? + verifier = "e2e-high-entropy-code-verifier-value-of-legal-length" # RFC 7636 §4.1: 43-128 chars + challenge = [Digest::SHA256.digest(verifier)].pack("m0").tr("+/", "-_").delete("=") + authorize_query = { + response_type: "code", client_id: result["register"]["client_id"], redirect_uri: REDIRECT_URI, + state: "opaque-state", code_challenge: challenge, code_challenge_method: "S256" + } + session.get("/mcp/oauth/authorize", authorize_query) + result["authorize_status"] = session.last_response.status + authorize_body = session.last_response.body + result["authorize_renders_form"] = authorize_body.include?("access_token") && authorize_body.include?(" "application/json", + "HTTP_AUTHORIZATION" => "Bearer #{token_body["access_token"]}") + result["mcp_with_issued_token_status"] = session.last_response.status + result["mcp_with_issued_token_body"] = JSON.parse(session.last_response.body) + + # 9. And the code must be spent. + session.post("/mcp/oauth/token", { + grant_type: "authorization_code", code: code, redirect_uri: REDIRECT_URI, code_verifier: verifier + }) + result["replayed_code_status"] = session.last_response.status + result["replayed_code_body"] = JSON.parse(session.last_response.body) + + # 10. A LOOPBACK client (RFC 8252 §7.3), whose ephemeral port no host could + # have allowlisted ahead of time. Off until the host opts in, so prove both + # states through the real stack rather than trusting the unit's fake host. + loopback_uri = "http://127.0.0.1:54321/cb" + loopback_query = authorize_query.merge(redirect_uri: loopback_uri) + session.get("/mcp/oauth/authorize", loopback_query) + result["loopback_authorize_status_when_off"] = session.last_response.status + + McpToolkit.config.oauth_allow_loopback_redirects = true + session.get("/mcp/oauth/authorize", loopback_query) + result["loopback_authorize_status_when_on"] = session.last_response.status + + session.post("/mcp/oauth/authorize", loopback_query.merge(access_token: VALID_TOKEN)) + result["approve_redirect_status"] = session.last_response.status + loopback_location = session.last_response.headers["Location"] + result["loopback_location_prefix"] = loopback_location && loopback_location.split("?").first + loopback_code = loopback_location && Rack::Utils.parse_query(URI.parse(loopback_location).query)["code"] + + session.post("/mcp/oauth/token", { + grant_type: "authorization_code", code: loopback_code, redirect_uri: loopback_uri, code_verifier: verifier + }) + result["loopback_token_is_the_pasted_token"] = JSON.parse(session.last_response.body)["access_token"] == VALID_TOKEN + result["token_cache_control"] = session.last_response.headers["Cache-Control"] + result["token_pragma"] = session.last_response.headers["Pragma"] + + # Allowing loopback must widen NOTHING else. A registered network scheme names + # a REMOTE host and would carry the code off the device; a private-use scheme + # is a fixed string that belongs in the allowlist; and the remote allowlist is + # the phishing target and stays named-only. + { + "ssh_status" => "ssh://attacker.example/cb", + "gopher_status" => "gopher://attacker.example/cb", + "ldap_status" => "ldap://attacker.example/cb", + "private_scheme_status" => "cursor://anysphere.cursor-retrieval/oauth/callback", + "remote_unregistered_status" => "https://attacker.example/x" + }.each do |key, uri| + session.get("/mcp/oauth/authorize", authorize_query.merge(redirect_uri: uri)) + result[key] = session.last_response.status + end + + # 11. A format must not reach the action by EITHER route — a `.json` suffix or + # an `Accept` header. There is no JSON template, so both would raise + # unauthenticated. The suffix was fixed first; the header is the same hole. + session.get("/mcp/oauth/authorize.json", authorize_query) + result["authorize_json_suffix_status"] = session.last_response.status + + session.get("/mcp/oauth/authorize", authorize_query, "HTTP_ACCEPT" => "application/json") + result["authorize_json_accept_status"] = session.last_response.status + + # 11b. And the bad-paste page must be a real 422, not a 500. `:unprocessable_content` + # raises below Rack 3.1 and the gemspec declares no floor. + session.post("/mcp/oauth/authorize", authorize_query.merge(access_token: "nope")) + result["bad_paste_status"] = session.last_response.status + session.post("/mcp/oauth/authorize", authorize_query.merge(access_token: "nope"), + "HTTP_ACCEPT" => "application/json") + result["bad_paste_json_accept_status"] = session.last_response.status + + # 12. A loopback client controls its own query, so it can pass `?code=`. The + # response owns that parameter — ours must be the only one. + polluted = "http://127.0.0.1:54321/cb?code=ATTACKER&tenant=acme" + session.post("/mcp/oauth/authorize", + authorize_query.merge(redirect_uri: polluted, access_token: VALID_TOKEN)) + polluted_location = session.last_response.headers["Location"] + polluted_query = Rack::Utils.parse_query(URI.parse(polluted_location.to_s).query) + result["polluted_code_values"] = Array(polluted_query["code"]) + result["polluted_keeps_client_query"] = polluted_query["tenant"] + + puts JSON.generate(result) + RUBY + + before(:all) do + require "json" + require "open3" + + # This spec sits a level deeper than its sibling under spec/mcp_toolkit/oauth. + gem_root = File.expand_path("../../..", __dir__) + stdout, stderr, status = Open3.capture3(Gem.ruby, "-e", OAUTH_BRIDGE_DRIVER, chdir: gem_root) + raise "oauth bridge driver failed (#{status.exitstatus}):\n#{stderr}" unless status.success? + + @result = JSON.parse(stdout.lines.last) + end + + # The bridge is additive to a host that already runs an OAuth provider (as an + # app with Doorkeeper for its own API does). It must not shadow one route of it. + describe "coexistence with a host's existing OAuth provider" do + it "leaves the host's /oauth/authorize, /oauth/token and /oauth/token/info untouched" do + expect(@result.fetch("host_authorize")).to eq("HOST_AUTHORIZE") + expect(@result.fetch("host_token")).to eq("HOST_TOKEN") + expect(@result.fetch("host_token_info")).to eq("HOST_TOKEN_INFO") + end + + it "claims nothing at host level beyond the two path-scoped metadata documents" do + expect(@result.fetch("host_level_paths")).to contain_exactly( + "/.well-known/oauth-protected-resource/mcp", + "/.well-known/oauth-authorization-server/mcp" + ) + end + + it "keeps every one of its own endpoints under the engine mount" do + expect(@result.fetch("engine_paths")).to contain_exactly( + "/", "/", "/", "/health", "/tokens/introspect", + "/oauth/authorize", "/oauth/authorize", "/oauth/token", "/oauth/register" + ) + end + end + + # Both documents name the authorization_endpoint an operator is sent to, and + # both are built from the caller-influenced request origin — so a shared cache + # holding one on another client's behalf is a token-theft primitive. Asserted + # through real Rails because Rails writes its own Cache-Control on commit, and + # a fake controller could only ever confirm the fake. + describe "metadata cacheability" do + it "forbids a shared cache from storing either document" do + expect(@result.fetch("prm_cache_control")).to eq("no-store") + expect(@result.fetch("as_cache_control")).to eq("no-store") + end + end + + # Loopback (RFC 8252 §7.3) is the one target that cannot be named ahead of time + # — the client picks an ephemeral port at runtime — and the one that need not be, + # since the code goes to the operator's own machine. + describe "loopback (RFC 8252) clients" do + it "refuses loopback until the host opts in" do + expect(@result.fetch("loopback_authorize_status_when_off")).to eq(400) + end + + it "runs a loopback client through the whole flow once opted in" do + expect(@result.fetch("loopback_authorize_status_when_on")).to eq(200) + expect(@result.fetch("loopback_location_prefix")).to eq("http://127.0.0.1:54321/cb") + expect(@result.fetch("loopback_token_is_the_pasted_token")).to be(true) + end + + # Allowing loopback widens loopback and nothing else. A scheme judged local + # merely by its absence from a denylist is how `ssh://attacker.example` would + # carry the code straight off the device. + it "refuses registered network schemes, which name a remote host" do + expect(@result.fetch("ssh_status")).to eq(400) + expect(@result.fetch("gopher_status")).to eq(400) + expect(@result.fetch("ldap_status")).to eq(400) + end + + it "refuses an unnamed private-use scheme, which is a fixed string to allowlist" do + expect(@result.fetch("private_scheme_status")).to eq(400) + end + + it "keeps a remote callback allowlist-only even with loopback allowed" do + expect(@result.fetch("remote_unregistered_status")).to eq(400) + end + end + + # A Rails host must not have to wire a signing secret: secret_key_base is + # already the "server-held, in ENV, never logged" value the sealing wants. + describe "the signing secret" do + it "resolves from the Rails app's secret_key_base with no configuration" do + expect(@result.fetch("signing_secret_resolves_from_rails")).to be(true) + expect(@result.fetch("bridge_enabled")).to be(true) + end + end + + describe "request shapes that must not reach an action" do + it "does not serve a format suffix the bridge never speaks" do + expect(@result.fetch("authorize_json_suffix_status")).to eq(404) + end + + # The `.json` suffix and an `Accept` header pick the format equally; closing + # only the one a reviewer named would leave its twin open. + it "renders the page regardless of what the caller says it accepts" do + expect(@result.fetch("authorize_json_accept_status")).to eq(200) + end + end + + # A mistyped paste is the one thing this page exists to handle gracefully, and + # it must not depend on a Rack version the gemspec never pinned. + describe "the bad-paste page" do + it "is a real 422 on any Rack, whatever the caller accepts" do + expect(@result.fetch("bad_paste_status")).to eq(422) + expect(@result.fetch("bad_paste_json_accept_status")).to eq(422) + end + end + + # A loopback redirect_uri is not exact-matched, so its query is the caller's to + # choose. The parameters this response owns are not. + describe "a loopback redirect_uri carrying its own code" do + it "emits exactly one code, ours, and keeps the client's own query" do + codes = @result.fetch("polluted_code_values") + + expect(codes.size).to eq(1) + expect(codes.first).not_to eq("ATTACKER") + expect(@result.fetch("polluted_keeps_client_query")).to eq("acme") + end + end + + # RFC 6749 §5.1 (both headers on a token response) and RFC 9700 §4.12 (303 after + # a POST that carried a credential), asserted through the real stack. + describe "token-bearing responses" do + it "forbids caching the token response" do + expect(@result.fetch("token_cache_control")).to eq("no-store") + expect(@result.fetch("token_pragma")).to eq("no-cache") + end + + it "redirects with 303 after the paste, so the token body is not resent" do + expect(@result.fetch("approve_redirect_status")).to eq(303) + end + end + + # The transport is JSON-only and its parent is ActionController::API, which + # cannot render HTML. Enabling the bridge must not force a host to change that. + describe "controller parents" do + it "leaves the transport on the host's ActionController::API parent" do + expect(@result.fetch("server_controller_parent")).to eq("ActionController::API") + end + + it "builds the bridge from its own parent, so it can render a page" do + expect(@result.fetch("oauth_controller_parent")).to eq("ActionController::Base") + end + end + + describe "the 401 that starts the flow" do + it "challenges an unauthenticated caller with a resource_metadata pointer" do + expect(@result.fetch("unauthenticated_status")).to eq(401) + expect(@result.fetch("www_authenticate")).to eq( + %(Bearer resource_metadata="http://example.org/.well-known/oauth-protected-resource/mcp") + ) + end + end + + describe "discovery" do + it "answers protected-resource metadata path-scoped under the mount" do + expect(@result.fetch("prm_status")).to eq(200) + expect(@result.fetch("prm")).to include( + "resource" => "http://example.org/mcp", + "authorization_servers" => ["http://example.org/mcp"] + ) + end + + it "answers authorization-server metadata at the path-INSERTED location" do + expect(@result.fetch("as_status")).to eq(200) + expect(@result.fetch("as")).to include( + "issuer" => "http://example.org/mcp", + "authorization_endpoint" => "http://example.org/mcp/oauth/authorize", + "token_endpoint" => "http://example.org/mcp/oauth/token", + "code_challenge_methods_supported" => ["S256"] + ) + end + + # The load-bearing one for a host that already runs its own OAuth: the bare + # well-known paths are origin-global and must remain that provider's to claim. + it "leaves BOTH origin-global well-known paths unclaimed" do + expect(@result.fetch("bare_prm_status")).to eq(404) + expect(@result.fetch("bare_as_status")).to eq(404) + end + end + + describe "registration" do + it "accepts a JSON registration and returns a client_id" do + expect(@result.fetch("register_status")).to eq(201) + expect(@result.fetch("register")["client_id"]).to be_a(String).and be_present + end + end + + describe "the authorization page" do + it "renders the paste form" do + expect(@result.fetch("authorize_status")).to eq(200) + expect(@result.fetch("authorize_renders_form")).to be(true) + end + + it "carries the PKCE challenge through to the second leg" do + expect(@result.fetch("authorize_echoes_challenge")).to be(true) + end + + it "masks the pasted token" do + expect(@result.fetch("authorize_masks_input")).to be(true) + end + + it "refuses to render for an unregistered redirect_uri" do + expect(@result.fetch("authorize_unregistered_status")).to eq(400) + end + end + + describe "the paste" do + # 303, not 302: this POST carried the token in its body, and only 303 tells the + # browser unambiguously to GET the callback without resending it. + it "redirects back to the client with the echoed state" do + expect(@result.fetch("approve_status")).to eq(303) + expect(@result.fetch("approve_location_host")).to eq("https://client.example/callback") + expect(@result.fetch("approve_state")).to eq("opaque-state") + end + + it "does not redirect (or issue a code) for a token that does not authenticate" do + expect(@result.fetch("approve_bad_token_status")).to eq(422) + expect(@result.fetch("approve_bad_token_redirected")).to be(false) + end + end + + describe "the exchange" do + it "accepts a form-encoded request and returns JSON" do + expect(@result.fetch("token_status")).to eq(200) + expect(@result.fetch("token_content_type")).to include("application/json") + end + + it "hands back the operator's own token, as a bearer" do + expect(@result.fetch("token_is_the_pasted_token")).to be(true) + expect(@result.fetch("token_body")).to include("token_type" => "Bearer") + end + + it "spends the code (a replay is refused)" do + expect(@result.fetch("replayed_code_status")).to eq(400) + expect(@result.fetch("replayed_code_body")).to eq("error" => "invalid_grant") + end + end + + # The whole point of the bridge: a client that walks this flow ends up holding + # something that works against the MCP endpoint it was trying to reach. + describe "the issued token against the MCP endpoint" do + it "authenticates a real MCP call" do + expect(@result.fetch("mcp_with_issued_token_status")).to eq(200) + expect(@result.fetch("mcp_with_issued_token_body")).to include("result") + end + end +end diff --git a/spec/mcp_toolkit/oauth/controller_methods_spec.rb b/spec/mcp_toolkit/oauth/controller_methods_spec.rb new file mode 100644 index 0000000..d78a3b4 --- /dev/null +++ b/spec/mcp_toolkit/oauth/controller_methods_spec.rb @@ -0,0 +1,716 @@ +# frozen_string_literal: true + +require "spec_helper" + +# The OAuth bridge concern runs WITHOUT Rails in the gem's suite, so this drives it +# against a minimal host class providing only the surface it touches: params, +# request origin, render, redirect_to — and a before_action chain, since the two +# browser legs are guarded by one and a fake that silently dropped it would let +# every example here pass while the guard was gone. Drive an action via +# `dispatch`, never by calling it directly, or the filters are skipped. +RSpec.describe McpToolkit::Oauth::ControllerMethods do + let(:controller_class) do + Class.new do + # A minimal before_action chain, because the concern now guards the two + # browser legs with one and a callback that never runs would let every + # example below pass for the wrong reason. Records the registrations, then + # `dispatch` runs them and halts on a render — the two properties of the + # real chain these specs depend on. (Real Rails still adjudicates it: the + # end-to-end spec drives an actual filter chain.) + def self.before_action(name, only: nil) + registered_filters << [name, Array(only).map(&:to_sym)] + end + + def self.registered_filters + @registered_filters ||= [] + end + + include McpToolkit::Oauth::ControllerMethods + + attr_accessor :params + attr_reader :rendered, :redirected_to + + def initialize + @params = {} + end + + # The action, with its filters — what a request actually does. + def dispatch(action) + self.class.registered_filters.each do |name, only| + next unless only.empty? || only.include?(action) + + send(name) + return nil if @rendered || @redirected_to # a filter halted the chain + end + send(action) + end + + def request + @request ||= Struct.new(:base_url).new("https://mcp.example.test") + end + + def response + @response ||= Struct.new(:headers).new({}) + end + + def render(*args, **options) + @rendered = { template: args.first, options: } + end + + def redirect_to(url, **options) + @redirected_to = { url:, options: } + end + end + end + + let(:controller) { controller_class.new } + let(:redirect_uri) { "https://client.example/callback" } + let(:principal) { FakePrincipal.new(id: 7) } + let(:valid_token) { "tok_live" } + + # A verifier and its real S256 challenge, so the PKCE check is exercised against + # genuine values rather than a stubbed comparison. RFC 7636 §4.1 shape (43–128 + # unreserved chars) — the old 35-char value predated the format check and would + # now, correctly, be refused. + let(:code_verifier) { "a-high-entropy-code-verifier-string-of-legal-length" } + let(:code_challenge) do + [Digest::SHA256.digest(code_verifier)].pack("m0").tr("+/", "-_").delete("=") + end + + # What a client posts to the token endpoint, minus the code it just received. + let(:exchange_params) do + { grant_type: "authorization_code", redirect_uri:, code_verifier: } + end + + before do + McpToolkit.configure do |c| + c.auth_role = :authority + c.server_name = "example-mcp" + c.oauth_allowed_redirect_uris = [redirect_uri] + c.token_authenticator = ->(plaintext) { plaintext == valid_token ? principal : nil } + # Set explicitly because this suite runs without Rails, so the default + # (`Rails.application.secret_key_base`) has nothing to resolve against — + # exactly what a non-Rails host faces. + c.oauth_signing_secret = "spec-oauth-signing-secret-at-least-32-bytes-long" + end + end + + def authorize_params(overrides = {}) + { + response_type: "code", + redirect_uri:, + state: "opaque-state", + code_challenge:, + code_challenge_method: "S256" + }.merge(overrides) + end + + # Runs the authorize + approve legs and returns the issued code. + def issue_code(overrides = {}) + controller.params = authorize_params(overrides).merge(access_token: valid_token) + controller.dispatch(:approve) + URI.decode_www_form(URI.parse(controller.redirected_to[:url]).query).to_h["code"] + end + + describe "#protected_resource" do + it "identifies the MCP endpoint from the live origin and points at it as the authorization server" do + controller.protected_resource + + expect(controller.rendered[:options][:json]).to eq( + resource: "https://mcp.example.test/mcp", + authorization_servers: ["https://mcp.example.test/mcp"], + bearer_methods_supported: ["header"] + ) + end + + it "tracks a host that mounted the engine somewhere other than /mcp" do + McpToolkit.config.oauth_resource_path = "/agent/mcp" + controller.protected_resource + + expect(controller.rendered[:options][:json][:resource]).to eq("https://mcp.example.test/agent/mcp") + end + end + + # The bare well-known paths are ORIGIN-GLOBAL: they describe the authorization + # server of the whole host, which on an origin already running an unrelated + # OAuth provider is that provider's to claim. Path-scoping (RFC 8414 §3.1 / + # RFC 9728 §3.1) is what lets both live on one host. + describe "metadata locations" do + it "scopes both documents under the engine's mount, claiming nothing at the origin root" do + expect(McpToolkit.config.oauth_protected_resource_path).to eq("/.well-known/oauth-protected-resource/mcp") + expect(McpToolkit.config.oauth_authorization_server_path).to eq("/.well-known/oauth-authorization-server/mcp") + end + + it "issues a path-ful issuer, so a client path-INSERTS and never probes the origin root" do + controller.authorization_server + issuer = controller.rendered[:options][:json][:issuer] + + expect(issuer).to eq("https://mcp.example.test/mcp") + # The document's issuer must equal the identifier used to construct its URL. + expect(URI.parse(issuer).path).to eq(McpToolkit.config.oauth_resource_path_component) + end + + it "falls back to the bare paths only for an endpoint mounted AT the origin root" do + McpToolkit.config.oauth_resource_path = "/" + + expect(McpToolkit.config.oauth_protected_resource_path).to eq("/.well-known/oauth-protected-resource") + expect(McpToolkit.config.oauth_authorization_server_path).to eq("/.well-known/oauth-authorization-server") + end + + it "drops a trailing slash rather than emitting a doubled one" do + McpToolkit.config.oauth_resource_path = "/mcp/" + + expect(McpToolkit.config.oauth_protected_resource_path).to eq("/.well-known/oauth-protected-resource/mcp") + expect(McpToolkit.config.oauth_resource_path_component).to eq("/mcp") + end + end + + describe "#authorization_server" do + subject(:metadata) do + controller.authorization_server + controller.rendered[:options][:json] + end + + it "advertises the path-ful MCP endpoint as issuer, so an RFC 8414 lookup path-inserts" do + expect(metadata[:issuer]).to eq("https://mcp.example.test/mcp") + end + + it "advertises S256, which clients send a challenge for regardless" do + expect(metadata[:code_challenge_methods_supported]).to eq(["S256"]) + end + + it "advertises public clients (no token-endpoint authentication)" do + expect(metadata[:token_endpoint_auth_methods_supported]).to eq(["none"]) + end + + it "advertises only the authorization_code grant (no refresh — the pasted token's own expiry is the lifetime)" do + expect(metadata[:grant_types_supported]).to eq(["authorization_code"]) + end + + it "points at the bridge's endpoints under the engine mount" do + expect(metadata).to include( + authorization_endpoint: "https://mcp.example.test/mcp/oauth/authorize", + token_endpoint: "https://mcp.example.test/mcp/oauth/token", + registration_endpoint: "https://mcp.example.test/mcp/oauth/register" + ) + end + end + + describe "#register" do + it "hands back a client identifier" do + controller.register + + expect(controller.rendered[:options][:json][:client_id]).to be_a(String).and be_present + end + + it "issues a fresh identifier per call and stores neither (nothing downstream reads one)" do + controller.register + first = controller.rendered[:options][:json][:client_id] + controller.register + + expect(controller.rendered[:options][:json][:client_id]).not_to eq(first) + end + end + + describe "#authorize" do + it "renders the paste page for a well-formed request" do + controller.params = authorize_params + controller.dispatch(:authorize) + + expect(controller.rendered).to include(template: :authorize) + expect(controller.rendered[:options]).to include(layout: false) + end + + it "refuses an unregistered redirect_uri rather than redirecting to it" do + controller.params = authorize_params(redirect_uri: "https://attacker.example/steal") + controller.dispatch(:authorize) + + expect(controller.rendered[:options]).to include(status: :bad_request) + expect(controller.redirected_to).to be_nil + end + + # An exact-match allowlist makes "the client's callback has a trailing slash" + # indistinguishable from an attack, so the offered value has to be recoverable + # from the logs or an operator is left guessing it. + it "logs the offered redirect_uri and the allowlist, so a misconfiguration names itself" do + logger = instance_double(Logger, warn: nil) + McpToolkit.config.logger = logger + controller.params = authorize_params(redirect_uri: "https://client.example/callback/") + controller.dispatch(:authorize) + + expect(logger).to have_received(:warn).with( + a_string_including("https://client.example/callback/").and(a_string_including(redirect_uri)) + ) + end + + it "refuses a request with no PKCE challenge" do + controller.params = authorize_params(code_challenge: nil) + controller.dispatch(:authorize) + + expect(controller.rendered[:options]).to include(status: :bad_request) + end + + it "refuses a downgraded (plain) PKCE method" do + controller.params = authorize_params(code_challenge_method: "plain") + controller.dispatch(:authorize) + + expect(controller.rendered[:options]).to include(status: :bad_request) + end + end + + # Loopback (RFC 8252 §7.3) is the ONE target accepted without being named, + # because it is the one that cannot be named: the client picks an ephemeral port + # at runtime. Everything else — including a private-use scheme, which also keeps + # the code on the device — is a fixed string and goes in the allowlist. + describe "loopback redirect targets" do + def authorize_with(uri) + controller.params = authorize_params(redirect_uri: uri) + controller.dispatch(:authorize) + controller.rendered + end + + context "when the host has not opted in" do + it "refuses loopback like any other unnamed target" do + expect(authorize_with("http://127.0.0.1:54321/cb")[:options]).to include(status: :bad_request) + end + end + + context "when the host allows loopback" do + before { McpToolkit.config.oauth_allow_loopback_redirects = true } + + # The port is ephemeral, so it cannot be registered ahead of time — which is + # the entire reason RFC 8252 §7.3 exists. + it "accepts loopback on an arbitrary port" do + expect(authorize_with("http://127.0.0.1:54321/cb")[:template]).to eq(:authorize) + end + + it "accepts the loopback name and the IPv6 literal clients really send" do + expect(authorize_with("http://localhost:3000/cb")[:template]).to eq(:authorize) + expect(authorize_with("http://[::1]:8080/cb")[:template]).to eq(:authorize) + end + + # The switch says "loopback", never "any scheme that looks local". A + # REGISTERED NETWORK scheme names a remote host and would carry the code + # straight off the device, so no scheme is judged local by its absence from + # a list of ones we thought of — a private-use scheme is a fixed string and + # belongs in the allowlist like anything else. + it "refuses registered network schemes, which name a remote host" do + ["ssh://attacker.example/cb", "gopher://attacker.example/cb", "ldap://attacker.example/cb", + "telnet://attacker.example/cb", "smb://attacker.example/cb", "nfs://attacker.example/cb"] + .each { |uri| expect(authorize_with(uri)[:options]).to include(status: :bad_request) } + end + + it "refuses an unnamed private-use scheme (it is a fixed string — allowlist it)" do + expect(authorize_with("cursor://anysphere.cursor-retrieval/oauth/callback")[:options]) + .to include(status: :bad_request) + expect(authorize_with("com.example.app:/oauth2redirect")[:options]).to include(status: :bad_request) + end + + it "still refuses a remote https callback that was never named" do + expect(authorize_with("https://attacker.example/steal")[:options]).to include(status: :bad_request) + end + + # Everything below is remote wearing a loopback costume. Each is checked + # against the PARSED host, which is what a browser actually resolves. + it "refuses a host that merely embeds a loopback address" do + # userinfo: the host is evil.example + expect(authorize_with("http://127.0.0.1@evil.example/cb")[:options]).to include(status: :bad_request) + # a subdomain of the attacker's domain + expect(authorize_with("http://127.0.0.1.evil.example/cb")[:options]).to include(status: :bad_request) + # loopback hidden in a fragment + expect(authorize_with("http://evil.example/cb#@127.0.0.1/")[:options]).to include(status: :bad_request) + end + + it "refuses a scheme-relative uri, which names no scheme to judge" do + expect(authorize_with("//evil.example/cb")[:options]).to include(status: :bad_request) + end + + it "refuses pseudo-schemes a browser may treat as script or local content" do + ["javascript:alert(1)", "data:text/html,x", "file:///etc/passwd", "blob:https://evil.example/x"] + .each { |uri| expect(authorize_with(uri)[:options]).to include(status: :bad_request) } + end + + it "refuses a redirect_uri carrying a fragment, which OAuth forbids" do + expect(authorize_with("http://127.0.0.1:5/cb#frag")[:options]).to include(status: :bad_request) + end + + it "hands a code to a loopback client through the full flow" do + loopback_uri = "http://127.0.0.1:54321/cb" + controller.params = authorize_params(redirect_uri: loopback_uri).merge(access_token: valid_token) + controller.dispatch(:approve) + + expect(controller.redirected_to[:url]).to start_with("#{loopback_uri}?code=") + end + + # An allowlisted private-use scheme must still WORK — the rule is "name it", + # not "no desktop clients". + it "hands a code to an allowlisted private-use scheme client" do + scheme_uri = "cursor://anysphere.cursor-retrieval/oauth/callback" + McpToolkit.config.oauth_allowed_redirect_uris = [scheme_uri] + controller.params = authorize_params(redirect_uri: scheme_uri).merge(access_token: valid_token) + controller.dispatch(:approve) + + expect(controller.redirected_to[:url]).to start_with("#{scheme_uri}?code=") + end + end + end + + # A bad entry here would otherwise surface at the worst possible moment: the + # page renders, the operator pastes a live token, a code is cached — and only + # then does building the callback URL raise. Same reasoning (and same shape) as + # `filter_operator_overrides=`. + describe "allowlist validation at config time" do + def assign(uri) + McpToolkit.config.oauth_allowed_redirect_uris = [uri] + end + + it "accepts the shapes a real client registers" do + ["https://client.example/callback", "cursor://anysphere.cursor-retrieval/oauth/callback", + "com.example.app:/oauth2redirect", "http://127.0.0.1:1/cb?tenant=acme"].each do |uri| + expect { assign(uri) }.not_to raise_error + end + end + + # The sharp one: legal URI syntax, invited by the docs, and it cannot carry a + # query — so no code could ever reach it. + it "rejects an opaque uri, naming the fix" do + expect { assign("com.example.app:oauth2redirect") } + .to raise_error(ArgumentError, /opaque.*com\.example\.app:\/oauth2redirect/m) + end + + it "rejects entries the bridge could never redirect to" do + expect { assign("https://client.example/cb#frag") }.to raise_error(ArgumentError, /fragment/) + expect { assign("/just/a/path") }.to raise_error(ArgumentError, /names no scheme/) + expect { assign("ht tp://nonsense") }.to raise_error(ArgumentError, /not a valid URI/) + end + + # Cleartext to a remote host puts the code on the wire. Cleartext to loopback + # never leaves the machine (RFC 8252 §7.3), so that one is allowed. + it "rejects cleartext http to a remote host but allows it to loopback" do + expect { assign("http://client.example/cb") }.to raise_error(ArgumentError, /cleartext http.*use https/m) + expect { assign("http://127.0.0.1:54321/cb") }.not_to raise_error + expect { assign("http://localhost:3000/cb") }.not_to raise_error + end + + # Naming bad schemes is sound HERE and nowhere else: this list is what the + # HOST wrote, so there is no unlisted scheme for an attacker to slip through. + it "rejects schemes a browser treats as script or local content" do + ["javascript:/alert(1)", "data:/text/html,x", "file:///tmp/cb", "vbscript:/x"].each do |uri| + expect { assign(uri) }.to raise_error(ArgumentError, /script or local content/) + end + end + + # Validation is a gate, not a suggestion: the reader used to hand back a + # mutable array, so `<<` slipped an unchecked entry straight past the setter. + # A `#frag` entry is not a harmless typo — the emitted `…/cb#frag?code=…` puts + # the code in the browser's hash, where the client's server never sees it. + it "cannot be bypassed by mutating the list in place" do + assign("https://client.example/callback") + + expect(McpToolkit.config.oauth_allowed_redirect_uris).to be_frozen + expect { McpToolkit.config.oauth_allowed_redirect_uris << "https://late.example/cb#frag" } + .to raise_error(FrozenError) + end + end + + # The sibling of the allowlist setter, and it skipped the same lesson: a + # non-String passed `oauth_bridge?`, drew the routes, and raised a TypeError out + # of OpenSSL::HMAC at request time — after the operator had pasted a live token. + describe "signing-secret validation at config time" do + it "rejects a non-String before it can reach a request" do + expect { McpToolkit.config.oauth_signing_secret = 12_345 } + .to raise_error(ArgumentError, /must be a String, got Integer/) + end + + it "rejects a secret too short to be a real one" do + expect { McpToolkit.config.oauth_signing_secret = "short" } + .to raise_error(ArgumentError, /bytes; use at least/) + end + + it "accepts a realistic secret, and nil (which falls back to secret_key_base)" do + expect { McpToolkit.config.oauth_signing_secret = "x" * 128 }.not_to raise_error + expect { McpToolkit.config.oauth_signing_secret = nil }.not_to raise_error + end + end + + # RFC 6749 §5.1 makes both headers a MUST on any response carrying a token, and + # RFC 9700 §4.12 wants a 303 after a POST that carried a credential. + describe "the token-bearing responses" do + it "forbids caching the token response" do + code = issue_code + controller.params = exchange_params.merge(code:) + controller.token + + expect(controller.response.headers["Cache-Control"]).to eq("no-store") + expect(controller.response.headers["Pragma"]).to eq("no-cache") + end + + # The approve POST carried the operator's token in its body; only 303 tells the + # browser unambiguously to GET the callback without resending it. + it "redirects with 303 after the credential POST, not 302" do + controller.params = authorize_params.merge(access_token: valid_token) + controller.dispatch(:approve) + + expect(controller.redirected_to[:options]).to include(status: :see_other) + end + end + + describe "#approve" do + it "redirects to the client with a code and the echoed state" do + controller.params = authorize_params.merge(access_token: valid_token) + controller.dispatch(:approve) + + redirect = URI.parse(controller.redirected_to[:url]) + query = URI.decode_www_form(redirect.query).to_h + + expect("#{redirect.scheme}://#{redirect.host}#{redirect.path}").to eq(redirect_uri) + expect(query["code"]).to be_present + expect(query["state"]).to eq("opaque-state") + end + + it "preserves a query the client's redirect_uri already carried" do + with_query = "#{redirect_uri}?tenant=acme" + McpToolkit.config.oauth_allowed_redirect_uris = [with_query] + controller.params = authorize_params(redirect_uri: with_query).merge(access_token: valid_token) + controller.dispatch(:approve) + + query = URI.decode_www_form(URI.parse(controller.redirected_to[:url]).query).to_h + expect(query["tenant"]).to eq("acme") + expect(query["code"]).to be_present + end + + it "re-renders the page with an error for an unknown token, without issuing a code" do + controller.params = authorize_params.merge(access_token: "not-a-real-token") + controller.dispatch(:approve) + + expect(controller.redirected_to).to be_nil + expect(controller.rendered).to include(template: :authorize) + expect(controller.instance_variable_get(:@mcp_oauth_error)).to be_present + end + + # 422 as an integer: no Rack floor is declared, and neither symbol spans the + # supported range (`:unprocessable_content` raises below Rack 3.1, + # `:unprocessable_entity` is deprecated above it). + it "re-renders the page when no token was pasted" do + controller.params = authorize_params.merge(access_token: "") + controller.dispatch(:approve) + + expect(controller.redirected_to).to be_nil + expect(controller.rendered[:options]).to include(status: 422) + end + + # The hidden fields are attacker-tamperable, so the allowlist is re-checked on + # this leg too — the check on #authorize alone would not bind the POST. + it "refuses a redirect_uri swapped between the two legs" do + controller.params = authorize_params(redirect_uri: "https://attacker.example/steal") + .merge(access_token: valid_token) + controller.dispatch(:approve) + + expect(controller.redirected_to).to be_nil + expect(controller.rendered[:options]).to include(status: :bad_request) + end + end + + describe "#token" do + it "hands back the pasted token itself" do + code = issue_code + controller.params = exchange_params.merge(code:) + controller.token + + expect(controller.rendered[:options][:json]).to eq(access_token: valid_token, token_type: "Bearer") + end + + it "rejects a grant type other than authorization_code" do + controller.params = exchange_params.merge(code: issue_code, grant_type: "client_credentials") + controller.token + + expect(controller.rendered[:options]).to include(json: { error: "unsupported_grant_type" }, status: :bad_request) + end + + it "rejects an unknown code" do + controller.params = exchange_params.merge(code: "never-issued") + controller.token + + expect(controller.rendered[:options][:json]).to eq(error: "invalid_grant") + end + + it "burns the code on first use" do + code = issue_code + controller.params = exchange_params.merge(code:) + controller.token + controller.params = exchange_params.merge(code:) + controller.token + + expect(controller.rendered[:options][:json]).to eq(error: "invalid_grant") + end + + # Well-formed but WRONG: reaches the comparison, fails it, and burns the code. + # That burn is the only thing stopping someone who intercepted a code from + # retrying verifiers against it, so it is asserted, not incidental. + it "rejects a code redeemed with the wrong PKCE verifier, and burns it" do + code = issue_code + wrong = "a-wrong-but-perfectly-well-formed-code-verifier" + controller.params = exchange_params.merge(code:, code_verifier: wrong) + controller.token + + expect(controller.rendered[:options][:json]).to eq(error: "invalid_grant") + + # Spent, even though the exchange failed. + controller.params = exchange_params.merge(code:) + controller.token + expect(controller.rendered[:options][:json]).to eq(error: "invalid_grant") + end + + # MALFORMED is refused before the code is touched — a request that could never + # verify must not cost a legitimate client its code. + it "refuses a malformed verifier without burning the code" do + code = issue_code + controller.params = exchange_params.merge(code:, code_verifier: "too-short") + controller.token + + expect(controller.rendered[:options][:json]).to eq(error: "invalid_request") + + # Still redeemable with the real verifier. + controller.params = exchange_params.merge(code:) + controller.token + expect(controller.rendered[:options][:json]).to eq(access_token: valid_token, token_type: "Bearer") + end + + it "rejects a code redeemed with no PKCE verifier at all" do + controller.params = exchange_params.merge(code: issue_code, code_verifier: nil) + controller.token + + expect(controller.rendered[:options][:json]).to eq(error: "invalid_request") + end + + it "rejects a code redeemed against a different redirect_uri than it was issued for" do + code = issue_code + McpToolkit.config.oauth_allowed_redirect_uris = [redirect_uri, "https://client.example/other"] + controller.params = exchange_params.merge(code:, redirect_uri: "https://client.example/other") + controller.token + + expect(controller.rendered[:options][:json]).to eq(error: "invalid_grant") + end + + # The bridge is an envelope, not a second source of truth: a token revoked + # between the paste and the exchange must not come back out of it. + it "rejects a token revoked between authorization and exchange" do + code = issue_code + McpToolkit.config.token_authenticator = ->(_plaintext) { nil } + controller.params = exchange_params.merge(code:) + controller.token + + expect(controller.rendered[:options][:json]).to eq(error: "invalid_grant") + end + + # Expiry is simulated by emptying the store rather than deleting a + # hand-built key: the key derivation is the bridge's own business, and a spec + # that restates it passes just as happily when it is wrong. + it "lets an expired code lapse" do + code = issue_code + McpToolkit.config.cache_store.clear + controller.params = exchange_params.merge(code:) + controller.token + + expect(controller.rendered[:options][:json]).to eq(error: "invalid_grant") + end + end + + # `cache_store` is documented to be the host's shared Rails.cache, and what is + # parked in it is the operator's own long-lived token — so what a dump of that + # store would yield is a property worth pinning, not an implementation detail. + describe "what the code leaves in the cache" do + # Reaches into the store deliberately: the claim is about the bytes a snapshot + # of it would contain, so it has to be asserted against the stored entries + # themselves — `Entry#value` and not `Entry#to_s`, which shows an object id and + # would let this pass while the token sat there in the clear. + def cached_keys + McpToolkit.config.cache_store.instance_variable_get(:@data).keys + end + + def cached_values + McpToolkit.config.cache_store.instance_variable_get(:@data).values.map { |entry| entry.value.to_s } + end + + # Guards the guard: if the payload were NOT sealed, this is the assertion that + # has to fail, so prove the plaintext would be visible to it. + it "would see a plaintext token in the store (so the assertion below means something)" do + McpToolkit.config.cache_store.write("probe", { access_token: valid_token }.to_json, expires_in: 60) + + expect(cached_values.join).to include(valid_token) + end + + it "parks neither the token nor the code itself" do + code = issue_code + + expect(cached_keys.join).not_to include(code) + expect(cached_values.join).not_to include(valid_token) + end + + it "still round-trips the token to the client holding the code" do + code = issue_code + controller.params = exchange_params.merge(code:) + controller.token + + expect(controller.rendered[:options][:json]).to eq(access_token: valid_token, token_type: "Bearer") + end + + # Sealed as well as hidden: a payload swapped in the store does not decrypt, + # so a writable cache cannot be used to inject a token of the attacker's + # choosing into the exchange. + it "refuses a payload that was tampered with in the store" do + code = issue_code + McpToolkit.config.cache_store.write(cached_keys.first, "tampered-ciphertext", expires_in: 60) + controller.params = exchange_params.merge(code:) + controller.token + + expect(controller.rendered[:options][:json]).to eq(error: "invalid_grant") + end + + # The code is NOT the key. It cannot be: Rails logs an authorization code + # twice per flow at INFO, so a key derived from it alone would be sitting in + # the log — the one artifact more widely read and longer kept than the 60s + # entry it protects. Knowing the code must not be enough. + it "does not open the payload to someone who has the code but not the secret" do + code = issue_code + blob = cached_values.first + + derived_from_code_alone = ActiveSupport::MessageEncryptor.new( + Digest::SHA256.digest("#{described_class::CODE_CACHE_PREFIX}key:#{code}"), cipher: "aes-256-gcm" + ) + expect { derived_from_code_alone.decrypt_and_verify(blob) } + .to raise_error(ActiveSupport::MessageEncryptor::InvalidMessage) + end + + it "refuses to issue a code at all once the signing secret is gone" do + McpToolkit.config.oauth_signing_secret = nil + + expect { issue_code }.to raise_error(McpToolkit::Errors::ConfigurationError, /oauth_signing_secret/) + end + + # Every serializer ActiveSupport defaults to across the supported range + # (:marshal, and 7.1+'s :json_allow_marshal) reaches Marshal.load, so a host + # with cache-write access forging one blob would get code execution. Pinning + # NullSerializer means JSON.parse is the only parser that ever sees it. + it "never hands the stored payload to Marshal" do + issue_code + encryptor = controller.send(:mcp_oauth_encryptor, "any-code") + + expect(encryptor.instance_variable_get(:@serializer)) + .to be(ActiveSupport::MessageEncryptor::NullSerializer) + end + end + + # A metadata document names the authorization_endpoint an operator will be sent + # to, and it is built from the caller-influenced request origin — so it is + # precisely the thing a shared cache must not hold on another client's behalf. + describe "metadata caching" do + it "forbids storing either document" do + controller.protected_resource + expect(controller.response.headers["Cache-Control"]).to eq("no-store") + + controller.authorization_server + expect(controller.response.headers["Cache-Control"]).to eq("no-store") + end + end +end