From 21455b1c4537965a313e4258d7ca2d0e142fd4e2 Mon Sep 17 00:00:00 2001 From: Karol Galanciak Date: Thu, 16 Jul 2026 18:03:07 +0200 Subject: [PATCH 01/11] Add an OAuth authorization bridge for the authority role (0.6.0) Hosted MCP clients authenticate one way only: discover an authorization server, run an authorization-code + PKCE flow in a browser, and use the access_token that comes back. The MCP authorization spec also forbids a token in the request URI, so a token query param is not a fallback for them. A server whose tokens are issued out-of-band is unreachable by those clients. The bridge is a standards-shaped envelope around the tokens a host already issues, not an identity provider. Its authorization page asks an operator to paste an access token they already hold, and the token it hands back IS that token, verified through the same token_authenticator the transport uses. Scopes, expiry, revocation and tenancy stay with the host; no new way to obtain a token exists. Deliberately absent, because none of it gates anything here: client registration returns an identifier and stores nothing (no endpoint reads a client_id); no consent step (pasting a token you hold is the grant); no refresh token (the pasted token's own expiry is the real lifetime). Deliberately NOT faked, because either would be a real vulnerability rather than a skipped ceremony: redirect_uri is matched against config.oauth_allowed_redirect_uris by exact string on both legs (an unvetted target would be an open redirect emitting authorization codes), and the PKCE code_verifier is verified constant-time against the stored S256 challenge. Authority-only and opt-in: oauth_bridge? requires the authority role AND a non-empty redirect allowlist, so the bridge cannot run without bounds on where codes may go, and a satellite (whose tokens belong to its central app) never draws it. A host that configures nothing is unchanged, including its 401. Verified end to end against a real Rails 8.1 app in an isolated child process (spec/mcp_toolkit/oauth/bridge_end_to_end_spec.rb): the whole client sequence, including that the view renders and that the issued token authenticates a subsequent MCP call. 546 specs, rubocop and brakeman clean. --- CHANGELOG.md | 57 ++++ README.md | 70 ++++ .../mcp_toolkit/oauth/authorize.html.erb | 83 +++++ config/routes.rb | 17 + lib/mcp_toolkit.rb | 8 +- .../authority/controller_methods.rb | 13 + lib/mcp_toolkit/configuration.rb | 59 ++++ lib/mcp_toolkit/engine_controllers.rb | 40 ++- lib/mcp_toolkit/oauth/controller_methods.rb | 268 +++++++++++++++ lib/mcp_toolkit/version.rb | 2 +- .../authority/controller_methods_spec.rb | 26 +- spec/mcp_toolkit/engine_spec.rb | 35 ++ .../oauth/bridge_end_to_end_spec.rb | 284 ++++++++++++++++ .../oauth/controller_methods_spec.rb | 304 ++++++++++++++++++ 14 files changed, 1260 insertions(+), 6 deletions(-) create mode 100644 app/views/mcp_toolkit/oauth/authorize.html.erb create mode 100644 lib/mcp_toolkit/oauth/controller_methods.rb create mode 100644 spec/mcp_toolkit/oauth/bridge_end_to_end_spec.rb create mode 100644 spec/mcp_toolkit/oauth/controller_methods_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 976fe2b..b06bca7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,60 @@ +## [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 matched against + `config.oauth_allowed_redirect_uris` by exact string on BOTH legs (an unvetted + redirect target would be an open redirect that emits authorization codes), and + the PKCE `code_verifier` is verified (constant-time) against the stored S256 + `code_challenge`. + + Endpoints — `GET`/`POST` `/oauth/authorize`, `POST /oauth/token`, + `POST /oauth/register`, plus the two metadata documents + (`/.well-known/oauth-protected-resource`, `/.well-known/oauth-authorization-server`). + The metadata must answer at the ORIGIN ROOT, which an engine mounted under a path + cannot draw, 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. +- `config.oauth_allowed_redirect_uris` (default `[]`), `config.oauth_resource_path` + (default `"/mcp"` — must match the engine's mount point), and + `config.oauth_authorization_code_ttl` (default `60`). +- `config.oauth_bridge?` — whether the bridge is live. Gated on the authority role + AND a non-empty redirect allowlist, so it cannot run without bounds on where codes + may go, and a satellite (whose tokens belong to its central app) never draws it. +- 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 + +- The bridge renders an HTML page, so a host enabling it must point + `config.parent_controller` at an `ActionController::Base` descendant + (`ActionController::API` cannot render one). The transport itself is unaffected. +- 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..d3739a3 100644 --- a/README.md +++ b/README.md @@ -403,6 +403,76 @@ 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) } + + # 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 +end +``` + +```ruby +# config/routes.rb — the helper call must be TOP LEVEL, since the two metadata +# documents have to answer at the origin root (an engine mounted under a path +# cannot draw them). It is 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`, +`GET /.well-known/oauth-authorization-server`, `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 matched against +`oauth_allowed_redirect_uris` by exact string on both legs — an unvetted redirect +target would be an open redirect that emits authorization codes — and the PKCE +`code_verifier` is verified against the stored S256 challenge. + +Two constraints worth knowing before you enable it. The bridge renders an HTML +page, so `config.parent_controller` must descend from `ActionController::Base` +(`ActionController::API` cannot render one); the transport itself is unaffected. +And `oauth_allowed_redirect_uris` is empty by default, which leaves +`config.oauth_bridge?` false and the routes undrawn — the bridge cannot run +without bounds on where codes may go, and a satellite never draws it at all +(its tokens belong to its central app, so there is nothing for it to authorize +against). + +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..0115573 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -30,4 +30,21 @@ # 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)`. + if McpToolkit.config.oauth_bridge? + get "oauth/authorize", to: "oauth#authorize" + post "oauth/authorize", to: "oauth#approve" + post "oauth/token", to: "oauth#token" + post "oauth/register", to: "oauth#register" + end end diff --git a/lib/mcp_toolkit.rb b/lib/mcp_toolkit.rb index 55ea3df..4462b2d 100644 --- a/lib/mcp_toolkit.rb +++ b/lib/mcp_toolkit.rb @@ -8,12 +8,14 @@ # 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) # # 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 +27,11 @@ require "digest" require "time" require "securerandom" +require "uri" require "mcp" require "active_support/concern" require "active_support/cache" +require "active_support/security_utils" # 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..b10d39c 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,18 @@ 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. Emitted only when the bridge is configured, so a host that has not + # opted in keeps its 401 byte-identical. + def mcp_set_authenticate_challenge + return unless mcp_config.oauth_bridge? + + metadata_url = "#{request.base_url}#{McpToolkit::Oauth::ControllerMethods::PROTECTED_RESOURCE_PATH}" + 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..28064df 100644 --- a/lib/mcp_toolkit/configuration.rb +++ b/lib/mcp_toolkit/configuration.rb @@ -112,6 +112,46 @@ 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 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. + # + # EMPTY BY DEFAULT, and an empty list DISABLES the bridge 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_accessor :oauth_allowed_redirect_uris + + # The path McpToolkit::Engine is mounted at, used to build the `resource` + # identifier 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. + # + # @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 + # --- caching --------------------------------------------------------------- # The cache store backing sessions and introspection results. Must satisfy the @@ -434,6 +474,10 @@ def initialize @token_authenticator = nil + @oauth_allowed_redirect_uris = [] + @oauth_resource_path = "/mcp" + @oauth_authorization_code_ttl = 60 + @cache_store = ActiveSupport::Cache::MemoryStore.new initialize_data_path_defaults @@ -556,6 +600,21 @@ def authority? auth_role.to_sym == :authority 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 BOTH conditions, each for its own reason. It is 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. And it requires a non-empty + # `oauth_allowed_redirect_uris`, so the bridge cannot be running without an + # allowlist to bound where codes may go. + # + # @return [Boolean] + def oauth_bridge? + authority? && Array(oauth_allowed_redirect_uris).any? + 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_controllers.rb b/lib/mcp_toolkit/engine_controllers.rb index a6e3d62..3c27b7c 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,7 @@ 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)) + define_controller(self, :OauthController, build_oauth_controller(parent)) define_controller(Authority, :ServerController, build_authority_server_controller(parent)) ServerController end @@ -44,7 +45,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 +97,47 @@ 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 like its siblings so + # it picks up `config.parent_controller` — which, for this one, must descend from + # ActionController::Base, since the authorization page is an HTML view. + def self.build_oauth_controller(parent) + Class.new(parent) { include McpToolkit::Oauth::ControllerMethods } + 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 at the ORIGIN ROOT, where a + # client looks for them and where an engine mounted under a path cannot reach. + # The host calls this at the TOP LEVEL of its own route set — 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 + # + # A no-op unless the bridge is configured, so the call can sit in a host's + # routes unconditionally across environments. The path-suffixed + # protected-resource route covers clients that probe + # `/.well-known/oauth-protected-resource/` before the bare path. + def self.draw_oauth_metadata_routes(mapper) + return unless config.oauth_bridge? + + mapper.get McpToolkit::Oauth::ControllerMethods::PROTECTED_RESOURCE_PATH, + to: "mcp_toolkit/oauth#protected_resource", format: false + mapper.get "#{McpToolkit::Oauth::ControllerMethods::PROTECTED_RESOURCE_PATH}/*mcp_path", + to: "mcp_toolkit/oauth#protected_resource", format: false + mapper.get McpToolkit::Oauth::ControllerMethods::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/controller_methods.rb b/lib/mcp_toolkit/oauth/controller_methods.rb new file mode 100644 index 0000000..ea8d4eb --- /dev/null +++ b/lib/mcp_toolkit/oauth/controller_methods.rb @@ -0,0 +1,268 @@ +# frozen_string_literal: true + +# The AUTHORITY-side OAuth 2.1 authorization bridge, provided as an includable +# concern. +# +# WHY THIS EXISTS. Hosted MCP clients will not send a bearer token an operator +# typed into a config file: they 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 +# query string, so `?token=<...>` is not an option for those clients either. +# +# WHAT THIS IS NOT. This is not an identity provider. It mints no credential, +# stores no client, models no consent, and issues no refresh token. It is a +# STANDARDS-SHAPED ENVELOPE around tokens the host ALREADY issues by its own +# means: the authorization page asks the operator to paste an existing access +# token, and the `access_token` this bridge hands back IS that token, verified +# through the same `config.token_authenticator` the transport uses. Every +# property that actually gates access — scopes, expiry, revocation, tenancy — +# stays exactly where the host put it. Nothing here widens who can reach what. +# +# The deliberate no-ops, so a reader does not mistake them for oversights: +# * client registration returns a fresh identifier and stores nothing; no +# endpoint ever checks a `client_id`, because a public client's identifier is +# self-asserted and gates nothing on its own; +# * there is no consent step — the operator pasting a token they 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 this flow rather than refreshing a shadow of +# it. +# +# The two things that are NOT mocked, because faking them would create a real +# vulnerability rather than skip a ceremony: +# * `redirect_uri` is matched against `config.oauth_allowed_redirect_uris` by +# exact string, on BOTH legs. An unvetted redirect target here would be an +# open redirect that hands out authorization codes. +# * the PKCE `code_verifier` is verified against the stored `code_challenge`. +# It is a few lines and it is what stops an intercepted code from being +# redeemed by anyone but its requester. +# +# Endpoints (mount path `` = wherever the host mounted McpToolkit::Engine) +# GET /.well-known/oauth-protected-resource - protected-resource metadata (RFC 9728) +# GET /.well-known/oauth-authorization-server- authorization-server metadata (RFC 8414) +# POST /oauth/register - client registration (RFC 7591), a stub +# GET /oauth/authorize - the paste-your-token page +# POST /oauth/authorize - validate the paste, issue a code +# POST /oauth/token - exchange code (+ verifier) for the token +# +# The two metadata documents must answer at the ORIGIN ROOT, which an engine +# mounted under a path cannot draw. The host draws them in one line at the top +# level of its own route set: +# +# # config/routes.rb — top level, NOT inside a locale/format scope +# McpToolkit.draw_oauth_metadata_routes(self) +# +# Rendering: the authorization page is an HTML view, so the configured +# `parent_controller` must descend from ActionController::Base (ActionController::API +# cannot render one). A host restyles the page by defining its own +# `app/views/mcp_toolkit/oauth/authorize.html.erb`, which takes precedence over +# the engine's. +module McpToolkit::Oauth::ControllerMethods + extend ActiveSupport::Concern + + CODE_CACHE_PREFIX = "mcp_toolkit:oauth:code:" + CODE_BYTES = 32 + PROTECTED_RESOURCE_PATH = "/.well-known/oauth-protected-resource" + AUTHORIZATION_SERVER_PATH = "/.well-known/oauth-authorization-server" + + included do + # The token endpoint is called server-to-server by the client with no CSRF + # token; the authorization form posts with one. Skipping forgery protection + # for the former only would need per-action config on a dynamically built + # class, so the form carries its own guarantee instead: `approve` never acts + # on ambient authority (no cookie, no session), only on a pasted token, and + # its sole side effect redirects to an allowlisted URI. + protect_from_forgery with: :null_session if respond_to?(:protect_from_forgery) + end + + # GET /.well-known/oauth-protected-resource + # Points a client at this app as its own authorization server. `resource` MUST + # equal the MCP endpoint URL as the operator typed it into the client, so it is + # derived from the live request origin rather than pinned to one host. + def protected_resource + render json: { + resource: mcp_oauth_resource_url, + authorization_servers: [mcp_oauth_issuer], + bearer_methods_supported: ["header"] + } + end + + # GET /.well-known/oauth-authorization-server + # Advertises S256 because clients send a `code_challenge` regardless; `none` + # for token-endpoint auth because clients here are public and unverified. + def authorization_server + 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 + + # POST /oauth/register + # Deliberately stateless: hand back an identifier, remember nothing. Nothing + # downstream reads it, so persisting it 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 + + # GET /oauth/authorize — renders the paste-your-token page. + def authorize + return mcp_oauth_render_bad_request(mcp_oauth_request_problem) if mcp_oauth_request_problem + + render :authorize, layout: false + end + + # POST /oauth/authorize — verify the pasted token, mint a code, hand the + # client back to its redirect_uri. The token is verified here (rather than only + # at exchange) so a typo fails on the page the operator is looking at. + def approve + return mcp_oauth_render_bad_request(mcp_oauth_request_problem) if mcp_oauth_request_problem + + access_token = params[:access_token].to_s + return mcp_oauth_reject_paste if mcp_oauth_authenticate(access_token).nil? + + redirect_to mcp_oauth_callback_url(mcp_oauth_issue_code(access_token)), allow_other_host: true + end + + # POST /oauth/token — exchange a one-time code for the pasted token. + def token + return mcp_oauth_render_token_error("unsupported_grant_type") unless params[:grant_type] == "authorization_code" + + 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? + + render json: { access_token:, token_type: "Bearer" } + end + + private + + def mcp_oauth_config + McpToolkit.config + end + + # ---- request validation --------------------------------------------------- + + # The only two things worth refusing outright. A disallowed redirect_uri must + # never be redirected TO (that is the attack), so both problems render here + # instead of bouncing an OAuth error back to the caller. + def mcp_oauth_request_problem + return "Unregistered redirect_uri." unless mcp_oauth_redirect_uri_allowed? + return "Missing or unsupported PKCE code_challenge." unless mcp_oauth_code_challenge_supported? + + nil + end + + def mcp_oauth_redirect_uri_allowed? + Array(mcp_oauth_config.oauth_allowed_redirect_uris).include?(params[:redirect_uri].to_s) + end + + def mcp_oauth_code_challenge_supported? + params[:code_challenge].present? && params[:code_challenge_method].to_s == "S256" + end + + # ---- authorization codes -------------------------------------------------- + + # The whole "authorization server" state: one cache entry, short-lived, bound + # to the challenge and redirect it was issued for. + def mcp_oauth_issue_code(access_token) + code = SecureRandom.urlsafe_base64(CODE_BYTES) + mcp_oauth_config.cache_store.write( + "#{CODE_CACHE_PREFIX}#{code}", + { access_token:, code_challenge: params[:code_challenge].to_s, redirect_uri: params[:redirect_uri].to_s }, + expires_in: mcp_oauth_config.oauth_authorization_code_ttl + ) + code + end + + # Read-and-delete: a code is single-use even if the exchange then fails. + def mcp_oauth_consume_code(code) + return nil if code.empty? + + key = "#{CODE_CACHE_PREFIX}#{code}" + payload = mcp_oauth_config.cache_store.read(key) + mcp_oauth_config.cache_store.delete(key) + payload + 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 ----------------------------------------------------------------- + + # The origin. Metadata answers at the origin root, so the issuer is the origin + # and a client's RFC 8414 lookup lands on a path we actually draw. + def mcp_oauth_issuer + request.base_url + end + + # The MCP endpoint itself — origin + the engine's mount path. + def mcp_oauth_resource_url + "#{request.base_url}#{mcp_oauth_config.oauth_resource_path}" + end + + def mcp_oauth_endpoint_url(action) + "#{mcp_oauth_resource_url}/oauth/#{action}" + end + + # Appends `code` (and echoes `state`) onto the client's redirect_uri, preserving + # any query it already carries. + def mcp_oauth_callback_url(code) + uri = URI.parse(params[:redirect_uri].to_s) + query = URI.decode_www_form(uri.query.to_s) + query << ["code", code] + query << ["state", params[:state].to_s] if params[:state].present? + uri.query = URI.encode_www_form(query) + uri.to_s + end + + # ---- responses ------------------------------------------------------------ + + 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, status: :unprocessable_content + end + + def mcp_oauth_render_bad_request(message) + render plain: message, status: :bad_request + 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..75b195c 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,29 @@ 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"] + end + + it "challenges an unauthenticated caller with the 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") + ) + 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..44da456 100644 --- a/spec/mcp_toolkit/engine_spec.rb +++ b/spec/mcp_toolkit/engine_spec.rb @@ -91,9 +91,13 @@ 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) { [] } before do McpToolkit.config.auth_role = auth_role + McpToolkit.config.oauth_allowed_redirect_uris = oauth_redirect_uris recorder = route_recorder stub_const("Rails", Module.new) # The engine class body now also calls `config.to_prepare { ... }` (lazy @@ -138,6 +142,37 @@ 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 + 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..843166e --- /dev/null +++ b/spec/mcp_toolkit/oauth/bridge_end_to_end_spec.rb @@ -0,0 +1,284 @@ +# 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| + c.parent_controller = "ActionController::Base" + 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 + + app.initialize! + app.routes.draw do + McpToolkit.draw_oauth_metadata_routes(self) + mount McpToolkit::Engine => "/mcp" + end + + session = Rack::Test::Session.new(Rack::MockSession.new(app)) + result = {} + + # 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 paths a client probes. + session.get("/.well-known/oauth-protected-resource") + result["prm_status"] = session.last_response.status + result["prm"] = JSON.parse(session.last_response.body) + + # The path-suffixed probe a client tries before the bare path. + session.get("/.well-known/oauth-protected-resource/mcp") + result["prm_suffixed_status"] = session.last_response.status + + session.get("/.well-known/oauth-authorization-server") + result["as_status"] = session.last_response.status + result["as"] = JSON.parse(session.last_response.body) + + # 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" + 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) + + 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 + + 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") + ) + end + end + + describe "discovery" do + it "answers protected-resource metadata at the origin root, identifying the MCP endpoint" 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"] + ) + end + + it "also answers the path-suffixed probe a client tries first" do + expect(@result.fetch("prm_suffixed_status")).to eq(200) + end + + it "answers authorization-server metadata at the origin root" do + expect(@result.fetch("as_status")).to eq(200) + expect(@result.fetch("as")).to include( + "issuer" => "http://example.org", + "authorization_endpoint" => "http://example.org/mcp/oauth/authorize", + "token_endpoint" => "http://example.org/mcp/oauth/token", + "code_challenge_methods_supported" => ["S256"] + ) + 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 + it "redirects back to the client with the echoed state" do + expect(@result.fetch("approve_status")).to eq(302) + 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..e7b288d --- /dev/null +++ b/spec/mcp_toolkit/oauth/controller_methods_spec.rb @@ -0,0 +1,304 @@ +# 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 and redirect_to. before_actions are not auto-run (there is +# no filter runner), which is fine — the bridge has none; each action guards itself. +RSpec.describe McpToolkit::Oauth::ControllerMethods do + let(:controller_class) do + Class.new do + include McpToolkit::Oauth::ControllerMethods + + attr_accessor :params + attr_reader :rendered, :redirected_to + + def initialize + @params = {} + end + + def request + @request ||= Struct.new(:base_url).new("https://mcp.example.test") + 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. + let(:code_verifier) { "a-high-entropy-code-verifier-string" } + let(:code_challenge) do + [Digest::SHA256.digest(code_verifier)].pack("m0").tr("+/", "-_").delete("=") + 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 } + 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.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 this app 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"], + 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 + + describe "#authorization_server" do + subject(:metadata) do + controller.authorization_server + controller.rendered[:options][:json] + end + + it "advertises the origin as issuer so an RFC 8414 lookup lands on the drawn root path" do + expect(metadata[:issuer]).to eq("https://mcp.example.test") + 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.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.authorize + + expect(controller.rendered[:options]).to include(status: :bad_request) + expect(controller.redirected_to).to be_nil + end + + it "refuses a request with no PKCE challenge" do + controller.params = authorize_params(code_challenge: nil) + controller.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.authorize + + expect(controller.rendered[:options]).to include(status: :bad_request) + 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.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.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.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 + + it "re-renders the page when no token was pasted" do + controller.params = authorize_params.merge(access_token: "") + controller.approve + + expect(controller.redirected_to).to be_nil + expect(controller.rendered[:options]).to include(status: :unprocessable_content) + 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.approve + + expect(controller.redirected_to).to be_nil + expect(controller.rendered[:options]).to include(status: :bad_request) + end + end + + describe "#token" do + let(:exchange_params) do + { grant_type: "authorization_code", redirect_uri:, code_verifier: } + end + + 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 + + it "rejects a code redeemed with the wrong PKCE verifier" do + controller.params = exchange_params.merge(code: issue_code, code_verifier: "wrong-verifier") + controller.token + + expect(controller.rendered[:options][:json]).to eq(error: "invalid_grant") + 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_grant") + 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 + + it "lets an expired code lapse" do + code = issue_code + McpToolkit.config.cache_store.delete("#{described_class::CODE_CACHE_PREFIX}#{code}") + controller.params = exchange_params.merge(code:) + controller.token + + expect(controller.rendered[:options][:json]).to eq(error: "invalid_grant") + end + end +end From aec92195613445d34bf6366b9567fa3a6c22dab1 Mon Sep 17 00:00:00 2001 From: Karol Galanciak Date: Thu, 16 Jul 2026 18:16:58 +0200 Subject: [PATCH 02/11] Give the bridge its own controller parent; build it only when enabled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects found by checking the bridge against the real adopting app rather than against assumptions about it. 1. The authority sets parent_controller = "ActionController::API" — its transport is a JSON-only endpoint, so that is correct and must stay. The bridge derived its controller from the same config, and ActionController::API cannot render an HTML view, so the authorization page would have failed on the one app that needs it. The bridge now has its own config.oauth_parent_controller (default ActionController::Base). Enabling it changes nothing about the transport; a host can point it at ApplicationController for branding. 2. Building that controller unconditionally constantized its parent on every host, which would pull view machinery into an API-only app that never enables the bridge and break a non-Rails host outright. It is now built only when config.oauth_bridge?, matching its routes. Also pins coexistence, which was the question that surfaced this: the end-to-end spec now boots a host that ALREADY serves OAuth at the conventional top-level /oauth/{authorize,token,token/info} (the shape Doorkeeper draws) with those routes drawn FIRST, and asserts they answer untouched; that the bridge claims nothing at host level beyond the two .well-known metadata documents; that every bridge endpoint stays under the engine mount; and that the transport keeps its ActionController::API parent while the page still renders. 552 specs across 3 orderings, rubocop and brakeman clean. --- CHANGELOG.md | 20 +++-- README.md | 28 +++++-- lib/mcp_toolkit/configuration.rb | 31 ++++++- lib/mcp_toolkit/engine_controllers.rb | 21 +++-- lib/mcp_toolkit/oauth/controller_methods.rb | 18 +++-- spec/mcp_toolkit/engine_spec.rb | 9 +++ .../oauth/bridge_end_to_end_spec.rb | 80 ++++++++++++++++++- 7 files changed, 178 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b06bca7..8d18a4b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,9 +35,15 @@ opt-in: a host that configures nothing behaves exactly as it did on 0.5.0. `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.** Every bridge endpoint lives 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 only host-level paths the bridge + claims are the two `.well-known` metadata documents. - `config.oauth_allowed_redirect_uris` (default `[]`), `config.oauth_resource_path` - (default `"/mcp"` — must match the engine's mount point), and - `config.oauth_authorization_code_ttl` (default `60`). + (default `"/mcp"` — must match the engine's mount point), + `config.oauth_authorization_code_ttl` (default `60`), and + `config.oauth_parent_controller` (default `"ActionController::Base"`). - `config.oauth_bridge?` — whether the bridge is live. Gated on the authority role AND a non-empty redirect allowlist, so it cannot run without bounds on where codes may go, and a satellite (whose tokens belong to its central app) never draws it. @@ -48,9 +54,13 @@ opt-in: a host that configures nothing behaves exactly as it did on 0.5.0. ### Notes -- The bridge renders an HTML page, so a host enabling it must point - `config.parent_controller` at an `ActionController::Base` descendant - (`ActionController::API` cannot render one). The transport itself is unaffected. +- 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. - A host restyles the page by defining its own `app/views/mcp_toolkit/oauth/authorize.html.erb`, which takes precedence over the engine's. diff --git a/README.md b/README.md index d3739a3..f143398 100644 --- a/README.md +++ b/README.md @@ -461,14 +461,26 @@ than a skipped ceremony: `redirect_uri` is matched against target would be an open redirect that emits authorization codes — and the PKCE `code_verifier` is verified against the stored S256 challenge. -Two constraints worth knowing before you enable it. The bridge renders an HTML -page, so `config.parent_controller` must descend from `ActionController::Base` -(`ActionController::API` cannot render one); the transport itself is unaffected. -And `oauth_allowed_redirect_uris` is empty by default, which leaves -`config.oauth_bridge?` false and the routes undrawn — the bridge cannot run -without bounds on where codes may go, and a satellite never draws it at all -(its tokens belong to its central app, so there is nothing for it to authorize -against). +**It is additive to an OAuth provider you already run.** Every bridge endpoint +lives 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 only host-level paths the bridge +claims are the two `.well-known` metadata documents, which have to answer at the +origin root. + +`oauth_allowed_redirect_uris` is empty by default, which leaves +`config.oauth_bridge?` false and the routes undrawn — the bridge cannot run without +bounds on where codes may go, and a satellite never draws it at all (its tokens +belong to its central app, so there is nothing for it to authorize against). + +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. diff --git a/lib/mcp_toolkit/configuration.rb b/lib/mcp_toolkit/configuration.rb index 28064df..c39e943 100644 --- a/lib/mcp_toolkit/configuration.rb +++ b/lib/mcp_toolkit/configuration.rb @@ -152,6 +152,24 @@ class McpToolkit::Configuration # @return [Integer] attr_accessor :oauth_authorization_code_ttl + # 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 @@ -474,9 +492,7 @@ def initialize @token_authenticator = nil - @oauth_allowed_redirect_uris = [] - @oauth_resource_path = "/mcp" - @oauth_authorization_code_ttl = 60 + initialize_oauth_bridge_defaults @cache_store = ActiveSupport::Cache::MemoryStore.new initialize_data_path_defaults @@ -498,6 +514,15 @@ def initialize @upstreams = McpToolkit::Gateway::UpstreamRegistry.new end + # OAuth bridge defaults. The empty redirect allowlist is what keeps the bridge + # OFF (`oauth_bridge?`), so a host that never configures it is unaffected. + def initialize_oauth_bridge_defaults + @oauth_allowed_redirect_uris = [] + @oauth_resource_path = "/mcp" + @oauth_authorization_code_ttl = 60 + @oauth_parent_controller = "ActionController::Base" + 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). diff --git a/lib/mcp_toolkit/engine_controllers.rb b/lib/mcp_toolkit/engine_controllers.rb index 3c27b7c..689af9a 100644 --- a/lib/mcp_toolkit/engine_controllers.rb +++ b/lib/mcp_toolkit/engine_controllers.rb @@ -37,7 +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)) - define_controller(self, :OauthController, build_oauth_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 @@ -98,11 +102,16 @@ def mcp_extract_token end # The OAuth authorization bridge the engine mounts at `/oauth/*` and the - # host draws at the two `.well-known` metadata paths. Built like its siblings so - # it picks up `config.parent_controller` — which, for this one, must descend from - # ActionController::Base, since the authorization page is an HTML view. - def self.build_oauth_controller(parent) - Class.new(parent) { include McpToolkit::Oauth::ControllerMethods } + # 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 + Class.new(config.oauth_parent_controller.constantize) { include McpToolkit::Oauth::ControllerMethods } end # The AUTHORITY base controller a host subclasses (the recommended path for a diff --git a/lib/mcp_toolkit/oauth/controller_methods.rb b/lib/mcp_toolkit/oauth/controller_methods.rb index ea8d4eb..e8f3a97 100644 --- a/lib/mcp_toolkit/oauth/controller_methods.rb +++ b/lib/mcp_toolkit/oauth/controller_methods.rb @@ -45,16 +45,22 @@ # POST /oauth/authorize - validate the paste, issue a code # POST /oauth/token - exchange code (+ verifier) for the token # -# The two metadata documents must answer at the ORIGIN ROOT, which an engine -# mounted under a path cannot draw. The host draws them in one line at the top -# level of its own route set: +# ADDITIVE TO A HOST'S OWN OAUTH. Every endpoint above lives under the engine's +# mount (`/oauth/*`), so a host already running an OAuth provider at the +# conventional top-level `/oauth/*` — as an app with Doorkeeper for its own API +# does — keeps every one of those routes. The only host-level paths this claims +# are the two `.well-known` metadata documents, which must answer at the ORIGIN +# ROOT (an engine mounted under a path cannot draw them). The host draws those in +# one line at the top level of its own route set: # # # config/routes.rb — top level, NOT inside a locale/format scope # McpToolkit.draw_oauth_metadata_routes(self) # -# Rendering: the authorization page is an HTML view, so the configured -# `parent_controller` must descend from ActionController::Base (ActionController::API -# cannot render one). A host restyles the page by defining its own +# Rendering: the authorization page is an HTML view, so this controller is built +# from `config.oauth_parent_controller` (default ActionController::Base) rather +# than the `parent_controller` the transport uses — that one is typically +# ActionController::API, which cannot render a view, and enabling the bridge must +# not force a host to weaken it. A host restyles the page by defining its own # `app/views/mcp_toolkit/oauth/authorize.html.erb`, which takes precedence over # the engine's. module McpToolkit::Oauth::ControllerMethods diff --git a/spec/mcp_toolkit/engine_spec.rb b/spec/mcp_toolkit/engine_spec.rb index 44da456..00becba 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 diff --git a/spec/mcp_toolkit/oauth/bridge_end_to_end_spec.rb b/spec/mcp_toolkit/oauth/bridge_end_to_end_spec.rb index 843166e..44b57a6 100644 --- a/spec/mcp_toolkit/oauth/bridge_end_to_end_spec.rb +++ b/spec/mcp_toolkit/oauth/bridge_end_to_end_spec.rb @@ -60,7 +60,10 @@ def superuser? = false end McpToolkit.configure do |c| - c.parent_controller = "ActionController::Base" + # 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] @@ -80,8 +83,24 @@ def superuser? = false 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 @@ -89,6 +108,28 @@ def superuser? = false 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 + # 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") @@ -184,6 +225,43 @@ def superuser? = false @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 metadata documents" do + expect(@result.fetch("host_level_paths")).to contain_exactly( + "/.well-known/oauth-protected-resource", + "/.well-known/oauth-protected-resource/*mcp_path", + "/.well-known/oauth-authorization-server" + ) + 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 + + # 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) From 67917f14c16a72e585d23fb3a36e9f94e1c0f7ad Mon Sep 17 00:00:00 2001 From: Karol Galanciak Date: Thu, 16 Jul 2026 18:26:18 +0200 Subject: [PATCH 03/11] Log the offered redirect_uri when the allowlist rejects it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An exact-match allowlist makes a legitimate client rejected over an invisible difference (a trailing slash, a changed callback path) look identical to an attack, leaving an operator to guess the right string from an opaque 400. Log the offered value and the configured allowlist so the first failed connection names what to fix. It is the client's own public callback, never a credential. Writing the test for it surfaced that mcp_oauth_request_problem ran twice per request (once in the guard, once as the render argument) — harmless while it was pure, a double log now. Bound it to a local instead. --- lib/mcp_toolkit/oauth/controller_methods.rb | 22 ++++++++++++++++--- .../oauth/controller_methods_spec.rb | 14 ++++++++++++ 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/lib/mcp_toolkit/oauth/controller_methods.rb b/lib/mcp_toolkit/oauth/controller_methods.rb index e8f3a97..a207af3 100644 --- a/lib/mcp_toolkit/oauth/controller_methods.rb +++ b/lib/mcp_toolkit/oauth/controller_methods.rb @@ -124,7 +124,8 @@ def register # GET /oauth/authorize — renders the paste-your-token page. def authorize - return mcp_oauth_render_bad_request(mcp_oauth_request_problem) if mcp_oauth_request_problem + problem = mcp_oauth_request_problem + return mcp_oauth_render_bad_request(problem) if problem render :authorize, layout: false end @@ -133,7 +134,8 @@ def authorize # client back to its redirect_uri. The token is verified here (rather than only # at exchange) so a typo fails on the page the operator is looking at. def approve - return mcp_oauth_render_bad_request(mcp_oauth_request_problem) if mcp_oauth_request_problem + problem = mcp_oauth_request_problem + return mcp_oauth_render_bad_request(problem) if problem access_token = params[:access_token].to_s return mcp_oauth_reject_paste if mcp_oauth_authenticate(access_token).nil? @@ -167,12 +169,26 @@ def mcp_oauth_config # never be redirected TO (that is the attack), so both problems render here # instead of bouncing an OAuth error back to the caller. def mcp_oauth_request_problem - return "Unregistered redirect_uri." unless mcp_oauth_redirect_uri_allowed? + 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 + # An exact-match allowlist is unforgiving by design, which makes a legitimate + # client rejected over a callback URL that differs in some invisible way (a + # trailing slash, a changed path) look identical to an attack. Log the value that + # was actually offered so the first failed connection names the string to + # allowlist, rather than leaving an operator to guess it. Safe to log: it is the + # client's own public callback, never a credential. + 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})" + ) + "Unregistered redirect_uri." + end + def mcp_oauth_redirect_uri_allowed? Array(mcp_oauth_config.oauth_allowed_redirect_uris).include?(params[:redirect_uri].to_s) end diff --git a/spec/mcp_toolkit/oauth/controller_methods_spec.rb b/spec/mcp_toolkit/oauth/controller_methods_spec.rb index e7b288d..13b5a71 100644 --- a/spec/mcp_toolkit/oauth/controller_methods_spec.rb +++ b/spec/mcp_toolkit/oauth/controller_methods_spec.rb @@ -153,6 +153,20 @@ def issue_code(overrides = {}) 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.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.authorize From 6a00bf8e86f5a598ecf7ca8335a7b9ea482f0735 Mon Sep 17 00:00:00 2001 From: Karol Galanciak Date: Thu, 16 Jul 2026 18:55:20 +0200 Subject: [PATCH 04/11] Path-scope the metadata so the bridge claims nothing origin-global MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bare well-known paths are ORIGIN-GLOBAL: /.well-known/oauth-authorization-server means "the authorization server of this whole origin". Claiming it from an MCP server bolted onto a host that already runs an unrelated OAuth provider (a REST API's, say) asserts something false about the origin, and permanently spends a namespace that is the pre-existing provider's to claim. Nothing consumed it, so this was not breaking anything — but it was the wrong thing to own, and the failure mode it invites is bad: if both ever answered, a client could bind to the wrong authorization server silently rather than 404 loudly. The RFCs already solve this and say so outright. RFC 8414 §3.1 inserts the issuer's path into the well-known URL specifically so that "Using path components enables supporting multiple issuers per host"; RFC 9728 §3.1 does the same for protected resources. The MCP authorization spec (2025-11-25) goes further and requires a client given a path-ful issuer to try ONLY the path-inserted forms — there is no root fallback — so a compliant client never touches the origin root. So the issuer is now the MCP endpoint URL itself (path-ful), and both documents hang off it: /.well-known/oauth-{protected-resource,authorization-server}/mcp. Issuer path == resource path, which also keeps a client deriving the same URL whether it builds candidates from the issuer or the resource. PRM discovery does not rely on any of this: WWW-Authenticate states the location outright and the client fetches it directly. A host mounted AT its origin root (a dedicated MCP domain) has no path to insert and still gets the bare paths, which is correct there — it really is that origin's only authorization server. Set oauth_resource_path = "/". The end-to-end spec now asserts BOTH bare paths 404 on a host running its own OAuth, alongside the existing coexistence checks. 557 specs across 3 orderings, rubocop and brakeman clean. --- CHANGELOG.md | 33 ++++++---- README.md | 35 +++++++---- .../authority/controller_methods.rb | 9 ++- lib/mcp_toolkit/configuration.rb | 60 +++++++++++++++++-- lib/mcp_toolkit/engine_controllers.rb | 24 ++++---- lib/mcp_toolkit/oauth/controller_methods.rb | 56 ++++++++++------- .../authority/controller_methods_spec.rb | 6 +- .../oauth/bridge_end_to_end_spec.rb | 46 +++++++------- .../oauth/controller_methods_spec.rb | 42 +++++++++++-- 9 files changed, 224 insertions(+), 87 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8d18a4b..84022ab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,18 +28,27 @@ opt-in: a host that configures nothing behaves exactly as it did on 0.5.0. `code_challenge`. Endpoints — `GET`/`POST` `/oauth/authorize`, `POST /oauth/token`, - `POST /oauth/register`, plus the two metadata documents - (`/.well-known/oauth-protected-resource`, `/.well-known/oauth-authorization-server`). - The metadata must answer at the ORIGIN ROOT, which an engine mounted under a path - cannot draw, 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.** Every bridge endpoint lives 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 only host-level paths the bridge - claims are the two `.well-known` metadata documents. + `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 `[]`), `config.oauth_resource_path` (default `"/mcp"` — must match the engine's mount point), `config.oauth_authorization_code_ttl` (default `60`), and diff --git a/README.md b/README.md index f143398..a0ac445 100644 --- a/README.md +++ b/README.md @@ -432,17 +432,17 @@ end ``` ```ruby -# config/routes.rb — the helper call must be TOP LEVEL, since the two metadata -# documents have to answer at the origin root (an engine mounted under a path -# cannot draw them). It is a no-op unless the bridge is configured. +# 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`, -`GET /.well-known/oauth-authorization-server`, `POST /mcp/oauth/register`, +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 @@ -461,12 +461,25 @@ than a skipped ceremony: `redirect_uri` is matched against target would be an open redirect that emits authorization codes — and the PKCE `code_verifier` is verified against the stored S256 challenge. -**It is additive to an OAuth provider you already run.** Every bridge endpoint -lives 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 only host-level paths the bridge -claims are the two `.well-known` metadata documents, which have to answer at the -origin root. +**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 by default, which leaves `config.oauth_bridge?` false and the routes undrawn — the bridge cannot run without diff --git a/lib/mcp_toolkit/authority/controller_methods.rb b/lib/mcp_toolkit/authority/controller_methods.rb index b10d39c..2a895e8 100644 --- a/lib/mcp_toolkit/authority/controller_methods.rb +++ b/lib/mcp_toolkit/authority/controller_methods.rb @@ -395,12 +395,15 @@ def mcp_render_unauthorized(message) # 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. Emitted only when the bridge is configured, so a host that has not - # opted in keeps its 401 byte-identical. + # 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. def mcp_set_authenticate_challenge return unless mcp_config.oauth_bridge? - metadata_url = "#{request.base_url}#{McpToolkit::Oauth::ControllerMethods::PROTECTED_RESOURCE_PATH}" + metadata_url = "#{request.base_url}#{mcp_config.oauth_protected_resource_path}" response.headers["WWW-Authenticate"] = %(Bearer resource_metadata="#{metadata_url}") end diff --git a/lib/mcp_toolkit/configuration.rb b/lib/mcp_toolkit/configuration.rb index c39e943..3773714 100644 --- a/lib/mcp_toolkit/configuration.rb +++ b/lib/mcp_toolkit/configuration.rb @@ -137,10 +137,14 @@ class McpToolkit::Configuration attr_accessor :oauth_allowed_redirect_uris # The path McpToolkit::Engine is mounted at, used to build the `resource` - # identifier 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. + # 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 @@ -625,6 +629,54 @@ 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. + # + # The resource path is INSERTED between the well-known prefix and nothing else + # — `/.well-known/oauth-protected-resource/mcp`, not the bare + # `/.well-known/oauth-protected-resource`. That is RFC 9728 §3.1's path-scoping + # rule, and it is deliberate: the BARE well-known paths are ORIGIN-GLOBAL. They + # describe "the authorization server / protected resource of this whole origin", + # which on a host that already runs an unrelated OAuth provider (a REST API's, + # say) is that provider's claim to make, not an MCP server's. Scoping by the + # mount path lets both coexist — RFC 8414 §3.1 says so in as many words: + # "Using path components enables supporting multiple issuers per host." + # + # A host whose MCP endpoint IS the origin root has no path to insert, so it gets + # the bare paths — correct there, because on that origin it really is the 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, and the document's `issuer` must match the + # identifier used to construct it. + # + # @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`. # diff --git a/lib/mcp_toolkit/engine_controllers.rb b/lib/mcp_toolkit/engine_controllers.rb index 689af9a..ee8454e 100644 --- a/lib/mcp_toolkit/engine_controllers.rb +++ b/lib/mcp_toolkit/engine_controllers.rb @@ -120,9 +120,9 @@ def self.build_authority_server_controller(parent) Class.new(parent) { include McpToolkit::Authority::ControllerMethods } end - # Draws the OAuth bridge's two metadata documents at the ORIGIN ROOT, where a - # client looks for them and where an engine mounted under a path cannot reach. - # The host calls this at the TOP LEVEL of its own route set — not inside a + # 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 @@ -132,18 +132,20 @@ def self.build_authority_server_controller(parent) # # ... # end # - # A no-op unless the bridge is configured, so the call can sit in a host's - # routes unconditionally across environments. The path-suffixed - # protected-resource route covers clients that probe - # `/.well-known/oauth-protected-resource/` before the bare path. + # 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 McpToolkit::Oauth::ControllerMethods::PROTECTED_RESOURCE_PATH, - to: "mcp_toolkit/oauth#protected_resource", format: false - mapper.get "#{McpToolkit::Oauth::ControllerMethods::PROTECTED_RESOURCE_PATH}/*mcp_path", + mapper.get config.oauth_protected_resource_path, to: "mcp_toolkit/oauth#protected_resource", format: false - mapper.get McpToolkit::Oauth::ControllerMethods::AUTHORIZATION_SERVER_PATH, + mapper.get config.oauth_authorization_server_path, to: "mcp_toolkit/oauth#authorization_server", format: false end diff --git a/lib/mcp_toolkit/oauth/controller_methods.rb b/lib/mcp_toolkit/oauth/controller_methods.rb index a207af3..2722021 100644 --- a/lib/mcp_toolkit/oauth/controller_methods.rb +++ b/lib/mcp_toolkit/oauth/controller_methods.rb @@ -37,21 +37,33 @@ # It is a few lines and it is what stops an intercepted code from being # redeemed by anyone but its requester. # -# Endpoints (mount path `` = wherever the host mounted McpToolkit::Engine) -# GET /.well-known/oauth-protected-resource - protected-resource metadata (RFC 9728) -# GET /.well-known/oauth-authorization-server- authorization-server metadata (RFC 8414) -# POST /oauth/register - client registration (RFC 7591), a stub -# GET /oauth/authorize - the paste-your-token page -# POST /oauth/authorize - validate the paste, issue a code -# POST /oauth/token - exchange code (+ verifier) for the token +# Endpoints, for an engine mounted at `/mcp` (every path below follows the mount) +# GET /.well-known/oauth-protected-resource/mcp - protected-resource metadata (RFC 9728) +# GET /.well-known/oauth-authorization-server/mcp - authorization-server metadata (RFC 8414) +# POST /mcp/oauth/register - client registration (RFC 7591), a stub +# GET /mcp/oauth/authorize - the paste-your-token page +# POST /mcp/oauth/authorize - validate the paste, issue a code +# POST /mcp/oauth/token - exchange code (+ verifier) for the token # -# ADDITIVE TO A HOST'S OWN OAUTH. Every endpoint above lives under the engine's -# mount (`/oauth/*`), so a host already running an OAuth provider at the -# conventional top-level `/oauth/*` — as an app with Doorkeeper for its own API -# does — keeps every one of those routes. The only host-level paths this claims -# are the two `.well-known` metadata documents, which must answer at the ORIGIN -# ROOT (an engine mounted under a path cannot draw them). The host draws those in -# one line at the top level of its own route set: +# ADDITIVE TO A HOST'S OWN OAUTH — IT CLAIMS NOTHING ORIGIN-GLOBAL. The flow +# endpoints live under the engine's mount, so a host already running an OAuth +# provider at the conventional top-level `/oauth/*` — as an app with Doorkeeper +# for its own API does — keeps every one of those routes. And the two metadata +# documents are PATH-SCOPED (`/.well-known/oauth-*/mcp`), never the bare +# `/.well-known/oauth-authorization-server`: that bare path is origin-global and +# means "the authorization server of this whole origin", which belongs to that +# pre-existing provider, not to an MCP server bolted onto the same host. RFC 8414 +# §3.1 exists for precisely 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. Scoping is therefore the correct reading, not a workaround. +# +# A host whose MCP endpoint IS its origin root (a dedicated MCP domain) has no +# path to insert and gets the bare paths — correct there, since it really is that +# origin's only authorization server. +# +# The metadata must answer at the host's own route set (an engine mounted under a +# path cannot draw a `/.well-known/*` path), so the host draws them in one line: # # # config/routes.rb — top level, NOT inside a locale/format scope # McpToolkit.draw_oauth_metadata_routes(self) @@ -68,8 +80,6 @@ module McpToolkit::Oauth::ControllerMethods CODE_CACHE_PREFIX = "mcp_toolkit:oauth:code:" CODE_BYTES = 32 - PROTECTED_RESOURCE_PATH = "/.well-known/oauth-protected-resource" - AUTHORIZATION_SERVER_PATH = "/.well-known/oauth-authorization-server" included do # The token endpoint is called server-to-server by the client with no CSRF @@ -247,15 +257,21 @@ def mcp_oauth_authenticate(access_token) # ---- urls ----------------------------------------------------------------- - # The origin. Metadata answers at the origin root, so the issuer is the origin - # and a client's RFC 8414 lookup lands on a path we actually draw. + # The issuer, which is the MCP endpoint URL itself — deliberately path-ful. + # + # A client constructs the authorization-server metadata URL from this by RFC + # 8414 §3.1 path INSERTION (`https://host/.well-known/oauth-authorization-server/mcp`), + # which is the whole reason the bridge never claims the origin-global bare path. + # It also means issuer path == resource path, the shape that keeps a client + # deriving the same URL whether it builds candidates from the issuer or from the + # resource. def mcp_oauth_issuer - request.base_url + mcp_oauth_resource_url end # The MCP endpoint itself — origin + the engine's mount path. def mcp_oauth_resource_url - "#{request.base_url}#{mcp_oauth_config.oauth_resource_path}" + "#{request.base_url}#{mcp_oauth_config.oauth_resource_path_component}" end def mcp_oauth_endpoint_url(action) diff --git a/spec/mcp_toolkit/authority/controller_methods_spec.rb b/spec/mcp_toolkit/authority/controller_methods_spec.rb index 75b195c..510fbce 100644 --- a/spec/mcp_toolkit/authority/controller_methods_spec.rb +++ b/spec/mcp_toolkit/authority/controller_methods_spec.rb @@ -261,11 +261,13 @@ def rpc(method, params = {}, id: 1) McpToolkit.config.oauth_allowed_redirect_uris = ["https://client.example/callback"] end - it "challenges an unauthenticated caller with the protected-resource metadata url" do + # 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") + %(Bearer resource_metadata="https://mcp.example.test/.well-known/oauth-protected-resource/mcp") ) end 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 index 44b57a6..9bd03ea 100644 --- a/spec/mcp_toolkit/oauth/bridge_end_to_end_spec.rb +++ b/spec/mcp_toolkit/oauth/bridge_end_to_end_spec.rb @@ -136,19 +136,23 @@ def token_info = render(plain: "HOST_TOKEN_INFO") result["unauthenticated_status"] = session.last_response.status result["www_authenticate"] = session.last_response.headers["WWW-Authenticate"] - # 2 + 3. Discovery, at the paths a client probes. - session.get("/.well-known/oauth-protected-resource") + # 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) - # The path-suffixed probe a client tries before the bare path. - session.get("/.well-known/oauth-protected-resource/mcp") - result["prm_suffixed_status"] = session.last_response.status - - session.get("/.well-known/oauth-authorization-server") + session.get("/.well-known/oauth-authorization-server/mcp") result["as_status"] = session.last_response.status result["as"] = JSON.parse(session.last_response.body) + # 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") @@ -234,11 +238,10 @@ def token_info = render(plain: "HOST_TOKEN_INFO") expect(@result.fetch("host_token_info")).to eq("HOST_TOKEN_INFO") end - it "claims nothing at host level beyond the two metadata documents" do + 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", - "/.well-known/oauth-protected-resource/*mcp_path", - "/.well-known/oauth-authorization-server" + "/.well-known/oauth-protected-resource/mcp", + "/.well-known/oauth-authorization-server/mcp" ) end @@ -266,33 +269,36 @@ def token_info = render(plain: "HOST_TOKEN_INFO") 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") + %(Bearer resource_metadata="http://example.org/.well-known/oauth-protected-resource/mcp") ) end end describe "discovery" do - it "answers protected-resource metadata at the origin root, identifying the MCP endpoint" 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"] + "authorization_servers" => ["http://example.org/mcp"] ) end - it "also answers the path-suffixed probe a client tries first" do - expect(@result.fetch("prm_suffixed_status")).to eq(200) - end - - it "answers authorization-server metadata at the origin root" do + 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", + "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 diff --git a/spec/mcp_toolkit/oauth/controller_methods_spec.rb b/spec/mcp_toolkit/oauth/controller_methods_spec.rb index 13b5a71..8e46219 100644 --- a/spec/mcp_toolkit/oauth/controller_methods_spec.rb +++ b/spec/mcp_toolkit/oauth/controller_methods_spec.rb @@ -71,12 +71,12 @@ def issue_code(overrides = {}) end describe "#protected_resource" do - it "identifies the MCP endpoint from the live origin and points at this app as the authorization server" 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"], + authorization_servers: ["https://mcp.example.test/mcp"], bearer_methods_supported: ["header"] ) end @@ -89,14 +89,48 @@ def issue_code(overrides = {}) 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 origin as issuer so an RFC 8414 lookup lands on the drawn root path" do - expect(metadata[:issuer]).to eq("https://mcp.example.test") + 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 From 19a4c198f83fdc6a168fa978fb75aab445f83237 Mon Sep 17 00:00:00 2001 From: Karol Galanciak Date: Thu, 16 Jul 2026 19:04:18 +0200 Subject: [PATCH 05/11] Trim the bridge's comments to what only the code can say MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fullstack-lint: the concern was 49% comment lines on added code. Two of them were also stale after the path-scoping change, still naming the bare well-known paths the bridge deliberately no longer serves — worse than noise, since a wrong comment outlives the reader's trust in the right ones. Most of the rest was duplication, not documentation: the endpoint list is in config/routes.rb, the README and the CHANGELOG; the path-scoping rationale now lives on Configuration#oauth_protected_resource_path, next to the code that implements it; the parent-controller reasoning lives on oauth_parent_controller. Restating all three in a 75-line header means four copies to keep true. What's kept is what a reader can't get elsewhere: that this is an envelope and not a half-built identity provider, that the stubs are deliberate, and the two invariants whose removal would be a vulnerability. 49% -> 29%, no information lost. --- lib/mcp_toolkit/configuration.rb | 26 ++-- lib/mcp_toolkit/oauth/controller_methods.rb | 151 +++++--------------- 2 files changed, 50 insertions(+), 127 deletions(-) diff --git a/lib/mcp_toolkit/configuration.rb b/lib/mcp_toolkit/configuration.rb index 3773714..a22c65f 100644 --- a/lib/mcp_toolkit/configuration.rb +++ b/lib/mcp_toolkit/configuration.rb @@ -648,19 +648,16 @@ def oauth_resource_path_component # Where the protected-resource metadata (RFC 9728) answers, and where # `WWW-Authenticate` points. # - # The resource path is INSERTED between the well-known prefix and nothing else - # — `/.well-known/oauth-protected-resource/mcp`, not the bare - # `/.well-known/oauth-protected-resource`. That is RFC 9728 §3.1's path-scoping - # rule, and it is deliberate: the BARE well-known paths are ORIGIN-GLOBAL. They - # describe "the authorization server / protected resource of this whole origin", - # which on a host that already runs an unrelated OAuth provider (a REST API's, - # say) is that provider's claim to make, not an MCP server's. Scoping by the - # mount path lets both coexist — RFC 8414 §3.1 says so in as many words: - # "Using path components enables supporting multiple issuers per host." - # - # A host whose MCP endpoint IS the origin root has no path to insert, so it gets - # the bare paths — correct there, because on that origin it really is the only - # authorization server. + # 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 @@ -669,8 +666,7 @@ def oauth_protected_resource_path # 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, and the document's `issuer` must match the - # identifier used to construct it. + # URL from the issuer it was given. # # @return [String] def oauth_authorization_server_path diff --git a/lib/mcp_toolkit/oauth/controller_methods.rb b/lib/mcp_toolkit/oauth/controller_methods.rb index 2722021..068a56f 100644 --- a/lib/mcp_toolkit/oauth/controller_methods.rb +++ b/lib/mcp_toolkit/oauth/controller_methods.rb @@ -1,80 +1,27 @@ # frozen_string_literal: true -# The AUTHORITY-side OAuth 2.1 authorization bridge, provided as an includable -# concern. +# The AUTHORITY-side OAuth 2.1 authorization bridge (routes: config/routes.rb; +# setup + rationale: README). # -# WHY THIS EXISTS. Hosted MCP clients will not send a bearer token an operator -# typed into a config file: they 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 -# query string, so `?token=<...>` is not an option for those clients either. +# 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. # -# WHAT THIS IS NOT. This is not an identity provider. It mints no credential, -# stores no client, models no consent, and issues no refresh token. It is a -# STANDARDS-SHAPED ENVELOPE around tokens the host ALREADY issues by its own -# means: the authorization page asks the operator to paste an existing access -# token, and the `access_token` this bridge hands back IS that token, verified -# through the same `config.token_authenticator` the transport uses. Every -# property that actually gates access — scopes, expiry, revocation, tenancy — -# stays exactly where the host put it. Nothing here widens who can reach what. +# 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. # -# The deliberate no-ops, so a reader does not mistake them for oversights: -# * client registration returns a fresh identifier and stores nothing; no -# endpoint ever checks a `client_id`, because a public client's identifier is -# self-asserted and gates nothing on its own; -# * there is no consent step — the operator pasting a token they 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 this flow rather than refreshing a shadow of -# it. -# -# The two things that are NOT mocked, because faking them would create a real -# vulnerability rather than skip a ceremony: -# * `redirect_uri` is matched against `config.oauth_allowed_redirect_uris` by -# exact string, on BOTH legs. An unvetted redirect target here would be an -# open redirect that hands out authorization codes. -# * the PKCE `code_verifier` is verified against the stored `code_challenge`. -# It is a few lines and it is what stops an intercepted code from being -# redeemed by anyone but its requester. -# -# Endpoints, for an engine mounted at `/mcp` (every path below follows the mount) -# GET /.well-known/oauth-protected-resource/mcp - protected-resource metadata (RFC 9728) -# GET /.well-known/oauth-authorization-server/mcp - authorization-server metadata (RFC 8414) -# POST /mcp/oauth/register - client registration (RFC 7591), a stub -# GET /mcp/oauth/authorize - the paste-your-token page -# POST /mcp/oauth/authorize - validate the paste, issue a code -# POST /mcp/oauth/token - exchange code (+ verifier) for the token -# -# ADDITIVE TO A HOST'S OWN OAUTH — IT CLAIMS NOTHING ORIGIN-GLOBAL. The flow -# endpoints live under the engine's mount, so a host already running an OAuth -# provider at the conventional top-level `/oauth/*` — as an app with Doorkeeper -# for its own API does — keeps every one of those routes. And the two metadata -# documents are PATH-SCOPED (`/.well-known/oauth-*/mcp`), never the bare -# `/.well-known/oauth-authorization-server`: that bare path is origin-global and -# means "the authorization server of this whole origin", which belongs to that -# pre-existing provider, not to an MCP server bolted onto the same host. RFC 8414 -# §3.1 exists for precisely 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. Scoping is therefore the correct reading, not a workaround. -# -# A host whose MCP endpoint IS its origin root (a dedicated MCP domain) has no -# path to insert and gets the bare paths — correct there, since it really is that -# origin's only authorization server. -# -# The metadata must answer at the host's own route set (an engine mounted under a -# path cannot draw a `/.well-known/*` path), so the host draws them in one line: -# -# # config/routes.rb — top level, NOT inside a locale/format scope -# McpToolkit.draw_oauth_metadata_routes(self) -# -# Rendering: the authorization page is an HTML view, so this controller is built -# from `config.oauth_parent_controller` (default ActionController::Base) rather -# than the `parent_controller` the transport uses — that one is typically -# ActionController::API, which cannot render a view, and enabling the bridge must -# not force a host to weaken it. A host restyles the page by defining its own -# `app/views/mcp_toolkit/oauth/authorize.html.erb`, which takes precedence over -# the engine's. +# Two things are NOT mocked, because faking them would be a vulnerability rather +# than a skipped ceremony: `redirect_uri` is exact-matched against the allowlist +# on BOTH legs (an unvetted target is an open redirect handing out authorization +# codes), and the PKCE `code_verifier` is verified. module McpToolkit::Oauth::ControllerMethods extend ActiveSupport::Concern @@ -82,19 +29,14 @@ module McpToolkit::Oauth::ControllerMethods CODE_BYTES = 32 included do - # The token endpoint is called server-to-server by the client with no CSRF - # token; the authorization form posts with one. Skipping forgery protection - # for the former only would need per-action config on a dynamically built - # class, so the form carries its own guarantee instead: `approve` never acts - # on ambient authority (no cookie, no session), only on a pasted token, and - # its sole side effect redirects to an allowlisted URI. + # Safe to disable: the token endpoint is called server-to-server without a CSRF + # token, and `approve` never acts on ambient authority — no cookie, no session, + # only a pasted token, redirecting to an allowlisted URI. protect_from_forgery with: :null_session if respond_to?(:protect_from_forgery) end - # GET /.well-known/oauth-protected-resource - # Points a client at this app as its own authorization server. `resource` MUST - # equal the MCP endpoint URL as the operator typed it into the client, so it is - # derived from the live request origin rather than pinned to one host. + # `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 render json: { resource: mcp_oauth_resource_url, @@ -103,9 +45,8 @@ def protected_resource } end - # GET /.well-known/oauth-authorization-server - # Advertises S256 because clients send a `code_challenge` regardless; `none` - # for token-endpoint auth because clients here are public and unverified. + # S256 because clients send a `code_challenge` regardless; `none` because the + # clients here are public and unverified. def authorization_server render json: { issuer: mcp_oauth_issuer, @@ -119,10 +60,8 @@ def authorization_server } end - # POST /oauth/register - # Deliberately stateless: hand back an identifier, remember nothing. Nothing - # downstream reads it, so persisting it would only grow a table of strings the - # bridge never consults. + # 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, @@ -132,7 +71,6 @@ def register }, status: :created end - # GET /oauth/authorize — renders the paste-your-token page. def authorize problem = mcp_oauth_request_problem return mcp_oauth_render_bad_request(problem) if problem @@ -140,9 +78,8 @@ def authorize render :authorize, layout: false end - # POST /oauth/authorize — verify the pasted token, mint a code, hand the - # client back to its redirect_uri. The token is verified here (rather than only - # at exchange) so a typo fails on the page the operator is looking at. + # The token is verified here, not only at exchange, so a typo fails on the page + # the operator is looking at. def approve problem = mcp_oauth_request_problem return mcp_oauth_render_bad_request(problem) if problem @@ -153,7 +90,6 @@ def approve redirect_to mcp_oauth_callback_url(mcp_oauth_issue_code(access_token)), allow_other_host: true end - # POST /oauth/token — exchange a one-time code for the pasted token. def token return mcp_oauth_render_token_error("unsupported_grant_type") unless params[:grant_type] == "authorization_code" @@ -175,9 +111,8 @@ def mcp_oauth_config # ---- request validation --------------------------------------------------- - # The only two things worth refusing outright. A disallowed redirect_uri must - # never be redirected TO (that is the attack), so both problems render here - # instead of bouncing an OAuth error back to the caller. + # 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_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? @@ -185,12 +120,9 @@ def mcp_oauth_request_problem nil end - # An exact-match allowlist is unforgiving by design, which makes a legitimate - # client rejected over a callback URL that differs in some invisible way (a - # trailing slash, a changed path) look identical to an attack. Log the value that - # was actually offered so the first failed connection names the string to - # allowlist, rather than leaving an operator to guess it. Safe to log: it is the - # client's own public callback, never a credential. + # 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 " \ @@ -257,19 +189,14 @@ def mcp_oauth_authenticate(access_token) # ---- urls ----------------------------------------------------------------- - # The issuer, which is the MCP endpoint URL itself — deliberately path-ful. - # - # A client constructs the authorization-server metadata URL from this by RFC - # 8414 §3.1 path INSERTION (`https://host/.well-known/oauth-authorization-server/mcp`), - # which is the whole reason the bridge never claims the origin-global bare path. - # It also means issuer path == resource path, the shape that keeps a client - # deriving the same URL whether it builds candidates from the issuer or from the - # resource. + # 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 - # The MCP endpoint itself — origin + the engine's mount path. def mcp_oauth_resource_url "#{request.base_url}#{mcp_oauth_config.oauth_resource_path_component}" end From 8e241154667147b70c9e292dea2136981ba79755 Mon Sep 17 00:00:00 2001 From: Karol Galanciak Date: Fri, 17 Jul 2026 10:31:12 +0200 Subject: [PATCH 06/11] Harden the bridge, and let native MCP clients connect (RFC 8252) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two things, in the same files. ## The security review's findings The two load-bearing controls held under an adversarial review (the redirect allowlist survived eight open-redirect evasion variants; PKCE resisted downgrade and bypass on both legs), so these are the periphery around them. - Serve both metadata documents `Cache-Control: no-store`. They name the `authorization_endpoint` an operator is sent to and are built from `request.base_url`, which honours `X-Forwarded-Host` — so a shared cache holding one could hand every client an origin an attacker chose, with our own document vouching for it. Rails leaves `config.hosts` empty in production by default, so a host cannot be assumed to reject the forged header; the README now says to pin it. Asserted through real Rails, since Rails writes its own Cache-Control on commit and a fake controller could only confirm the fake. - Withhold `WWW-Authenticate` when the request origin contains a quote. It is interpolated into a quoted-string parameter, and `base_url` is caller-influenced, so a quote would close the quotes and let a caller append auth-params of their own. - Seal what an authorization code parks in the cache: key the entry by the code's SHA256 and encrypt the payload under a key derived from the code, which never lands in the store. What sits 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 what is documented to be the host's shared `Rails.cache`. A dump now yields ciphertext with no key, and a payload swapped in a writable cache does not decrypt. - Require a `token_authenticator` for `oauth_bridge?`. The bridge verifies the pasted token through it on both legs, so without one an operator pasted their token and got a 500. Drawing no route beats an authorization page that cannot work; the sibling introspection endpoint already fails safe this way. - Make a code single-use by the DELETE rather than the read, so of two concurrent redemptions exactly one proceeds. The race was never exploitable — both return the same token and both need the verifier — but the comment claimed an invariant the code did not keep. Also corrects the concern's "no cookie, no session" note: the GET leg does set one (`form_tag` emits an authenticity token). Nothing reads it back, so the claim it supports still holds. ## Native clients: `config.oauth_allow_native_client_redirects` An exact-match allowlist supports exactly the clients it names, which is not "any MCP client". But it cannot simply be opened up: the page is served from our OWN origin under our own certificate and asks for a live token, so an unvetted `redirect_uri` makes it a credential-phishing page we host — an attacker sends an authorize link carrying their own `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. That attack needs the code to reach a REMOTE attacker, and that is the seam. A loopback address resolves on the operator's own machine and a private-use scheme is handed to a locally registered app, so neither can carry a code off the device — which is why RFC 8252 lets native apps skip pre-registration (§7.3 for loopback, whose ephemeral port could never be named ahead of time; §7.1 for schemes). So: loopback and private-use schemes are permitted generically by the new switch, a remote `https://` callback stays allowlist-only whatever it is set to, and every MCP client on an operator's machine connects with no config. Targets are judged on the PARSED URI, never the string — `http://127.0.0.1@evil.example/` has host `evil.example` and is remote, as is `http://127.0.0.1.evil.example/` — and a fragment, an opaque URI, or a `javascript:`/`data:`/`file:` scheme is refused. Off by default: switching it on is a decision, so it is an opt-in signal in its own right and can enable the bridge without an allowlist entry. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 63 +++++-- README.md | 64 ++++++- lib/mcp_toolkit.rb | 4 +- .../authority/controller_methods.rb | 8 + lib/mcp_toolkit/configuration.rb | 81 +++++++-- lib/mcp_toolkit/oauth/controller_methods.rb | 145 ++++++++++++++-- .../authority/controller_methods_spec.rb | 14 ++ spec/mcp_toolkit/engine_spec.rb | 31 ++++ .../oauth/bridge_end_to_end_spec.rb | 60 +++++++ .../oauth/controller_methods_spec.rb | 159 +++++++++++++++++- 10 files changed, 574 insertions(+), 55 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 84022ab..979125e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,11 +21,28 @@ opt-in: a host that configures nothing behaves exactly as it did on 0.5.0. 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 matched against - `config.oauth_allowed_redirect_uris` by exact string on BOTH legs (an unvetted - redirect target would be an open redirect that emits authorization codes), and - the PKCE `code_verifier` is verified (constant-time) against the stored S256 - `code_challenge`. + 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. That attack + needs the code to reach a REMOTE attacker, which is what splits the policy in + two: a remote callback is matched by exact string against + `config.oauth_allowed_redirect_uris` and is never opened up, while RFC 8252 + native targets — loopback on any port (§7.3, the port is ephemeral and cannot be + registered ahead of time) and private-use schemes (§7.1) — are permitted + generically by `config.oauth_allow_native_client_redirects`, because they deliver + the code to the operator's OWN machine. So every MCP client running on an + operator's machine works with no configuration, while a hosted client needs one + allowlist entry. Native targets are judged on the PARSED URI + (`http://127.0.0.1@evil.example/` has host `evil.example`, and is remote), and a + fragment, an opaque URI, or a `javascript:`/`data:`/`file:` scheme is refused. Endpoints — `GET`/`POST` `/oauth/authorize`, `POST /oauth/token`, `POST /oauth/register`, plus the two metadata documents. A `/.well-known/*` @@ -49,13 +66,24 @@ opt-in: a host that configures nothing behaves exactly as it did on 0.5.0. 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 `[]`), `config.oauth_resource_path` - (default `"/mcp"` — must match the engine's mount point), - `config.oauth_authorization_code_ttl` (default `60`), and +- `config.oauth_allowed_redirect_uris` (default `[]`), + `config.oauth_allow_native_client_redirects` (default `false`), + `config.oauth_resource_path` (default `"/mcp"` — must match the engine's mount + point), `config.oauth_authorization_code_ttl` (default `60`), and `config.oauth_parent_controller` (default `"ActionController::Base"`). -- `config.oauth_bridge?` — whether the bridge is live. Gated on the authority role - AND a non-empty redirect allowlist, so it cannot run without bounds on where codes - may go, and a satellite (whose tokens belong to its central app) never draws it. +- `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 native-client 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. +- Both metadata documents are served `Cache-Control: no-store`. 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. - 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 @@ -63,6 +91,16 @@ opt-in: a host that configures nothing behaves exactly as it did on 0.5.0. ### 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` under a + key derived from the code, which never itself lands in the store — so a dump of + it (a Redis snapshot, a FileStore on disk) yields ciphertext with no key, and a + payload swapped in a writable cache does not decrypt. 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, and `cache_store` is documented to be the host's + shared `Rails.cache`. Codes are also single-use by the DELETE rather than the + read, so of two concurrent redemptions exactly one proceeds. - 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 @@ -70,6 +108,9 @@ opt-in: a host that configures nothing behaves exactly as it did on 0.5.0. 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. +- If a host logs request parameters, add `code_verifier` to + `config.filter_parameters`; `access_token` is already covered by the stock + `token` entry Rails ships. - A host restyles the page by defining its own `app/views/mcp_toolkit/oauth/authorize.html.erb`, which takes precedence over the engine's. diff --git a/README.md b/README.md index a0ac445..731cef4 100644 --- a/README.md +++ b/README.md @@ -428,6 +428,11 @@ McpToolkit.configure do |c| # 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_native_client_redirects = true end ``` @@ -456,10 +461,50 @@ no refresh token is issued (the pasted token's own expiry is the real lifetime, 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 matched against -`oauth_allowed_redirect_uris` by exact string on both legs — an unvetted redirect -target would be an open redirect that emits authorization codes — and the PKCE -`code_verifier` is verified against the stored S256 challenge. +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. + +That threat needs the code to reach a **remote** attacker, which is what splits +the policy in two: + +| Target | Rule | Why | +|---|---|---| +| Remote (`https://client.example/cb`) | Exact string, in `oauth_allowed_redirect_uris` | The phishing vector. Never opened up. | +| Loopback (`http://127.0.0.1:*`, `localhost`, `[::1]`) | Allowed by `oauth_allow_native_client_redirects` | RFC 8252 §7.3. The code lands on the operator's own machine; the port is ephemeral and cannot be registered ahead of time. | +| Private-use scheme (`cursor://…`, `com.example.app:/cb`) | Allowed by `oauth_allow_native_client_redirects` | RFC 8252 §7.1. Handed to a locally registered app; the code never leaves the device. | + +So **every MCP client on your operators' machines works with no configuration**, +while a hosted client needs one allowlist entry. Native targets are judged on the +*parsed* URI — `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, an opaque URI, or a `javascript:`/`data:`/`file:` scheme is refused +outright. + +### Deployment note + +Every identifier the bridge publishes is derived from the live request origin +(`request.base_url`), which honours `X-Forwarded-Host`. **Pin `config.hosts`** so +Rails' `HostAuthorization` rejects a forged header before it reaches the bridge; +Rails leaves it empty in production by default. Both metadata documents are served +`Cache-Control: no-store` regardless, so no shared cache can hand one client an +origin another client chose. If you log request parameters, add `code_verifier` to +`config.filter_parameters` (`access_token` is already covered by the stock `token` +entry). **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 @@ -481,10 +526,13 @@ If your MCP endpoint IS its origin root (a dedicated MCP domain), there is no pa 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 by default, which leaves -`config.oauth_bridge?` false and the routes undrawn — the bridge cannot run without -bounds on where codes may go, and a satellite never draws it at all (its tokens -belong to its central app, so there is nothing for it to authorize against). +`oauth_allowed_redirect_uris` is empty and `oauth_allow_native_client_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 diff --git a/lib/mcp_toolkit.rb b/lib/mcp_toolkit.rb index 4462b2d..ab0f403 100644 --- a/lib/mcp_toolkit.rb +++ b/lib/mcp_toolkit.rb @@ -15,7 +15,8 @@ # 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/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. @@ -32,6 +33,7 @@ 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 2a895e8..8722e1f 100644 --- a/lib/mcp_toolkit/authority/controller_methods.rb +++ b/lib/mcp_toolkit/authority/controller_methods.rb @@ -400,10 +400,18 @@ def mcp_render_unauthorized(message) # 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 diff --git a/lib/mcp_toolkit/configuration.rb b/lib/mcp_toolkit/configuration.rb index a22c65f..7925354 100644 --- a/lib/mcp_toolkit/configuration.rb +++ b/lib/mcp_toolkit/configuration.rb @@ -123,19 +123,52 @@ class McpToolkit::Configuration # McpToolkit::Oauth::ControllerMethods. # The exact redirect URIs an authorization code may be handed to — the - # allowlist a 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. - # - # EMPTY BY DEFAULT, and an empty list DISABLES the bridge 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. + # 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. A real authorization server blocks this with a consent + # screen naming the client plus an authenticated session. This bridge mocks both + # away, and this allowlist is what compensates. + # + # EMPTY BY DEFAULT. Empty, and with `oauth_allow_native_client_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_accessor :oauth_allowed_redirect_uris + # Permits NATIVE-client targets generically, without naming each one: loopback + # on any port (RFC 8252 §7.3 — the port is ephemeral, so it could not be named + # ahead of time) and private-use schemes (§7.1), i.e. `http://127.0.0.1:54321/cb`, + # `http://localhost:*/cb`, `cursor://…`. + # + # Safe to open generically for the same reason the allowlist above cannot be: + # these deliver the code to the OPERATOR'S OWN DEVICE, and the phishing above + # needs it to reach a REMOTE attacker. (The residual risk is a malicious app + # already installed on that machine squatting the scheme — local code execution, + # a lost game regardless. RFC 8252 lets native apps skip pre-registration on + # this same reasoning.) A remote `https://` callback is never covered by this, + # whatever it is set to. + # + # OFF BY DEFAULT: switching it on says "any MCP 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_native_client_redirects = true + # + # @return [Boolean] + attr_accessor :oauth_allow_native_client_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 @@ -518,10 +551,12 @@ def initialize @upstreams = McpToolkit::Gateway::UpstreamRegistry.new end - # OAuth bridge defaults. The empty redirect allowlist is what keeps the bridge - # OFF (`oauth_bridge?`), so a host that never configures it is unaffected. + # 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_native_client_redirects = false @oauth_resource_path = "/mcp" @oauth_authorization_code_ttl = 60 @oauth_parent_controller = "ActionController::Base" @@ -676,16 +711,30 @@ def oauth_authorization_server_path # 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 BOTH conditions, each for its own reason. It is 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. And it requires a non-empty - # `oauth_allowed_redirect_uris`, so the bridge cannot be running without an - # allowlist to bound where codes may go. + # 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. + # + # And at least one redirect target must be named — an allowlist entry, or the + # native-client 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. # # @return [Boolean] def oauth_bridge? - authority? && Array(oauth_allowed_redirect_uris).any? + return false unless authority? + return false if token_authenticator.nil? + + Array(oauth_allowed_redirect_uris).any? || !!oauth_allow_native_client_redirects end # Full introspection URL the satellite POSTs to. Raises a clear error if the diff --git a/lib/mcp_toolkit/oauth/controller_methods.rb b/lib/mcp_toolkit/oauth/controller_methods.rb index 068a56f..70ad533 100644 --- a/lib/mcp_toolkit/oauth/controller_methods.rb +++ b/lib/mcp_toolkit/oauth/controller_methods.rb @@ -19,25 +19,43 @@ # shadow of it. # # Two things are NOT mocked, because faking them would be a vulnerability rather -# than a skipped ceremony: `redirect_uri` is exact-matched against the allowlist -# on BOTH legs (an unvetted target is an open redirect handing out authorization -# codes), and the PKCE `code_verifier` is verified. +# 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 + # RFC 8252 §7.3 loopback hosts. The RFC prefers the IP literals over the name + # (a name is only as trustworthy as the resolver), but real clients use all + # three, and each resolves on the operator's own machine — which is the whole + # reason these need no allowlist entry. + LOOPBACK_HOSTS = ["127.0.0.1", "::1", "localhost"].freeze + + # Schemes that are never a private-use (native app) scheme: the web schemes, + # which travel to a remote host and so must be allowlisted; and the + # pseudo-schemes a browser may treat as script or local content, which have no + # business receiving an authorization code whatever a client claims. + RESERVED_REDIRECT_SCHEMES = %w[ + http https ws wss ftp sftp file data javascript vbscript blob about view-source + ].freeze + included do # Safe to disable: the token endpoint is called server-to-server without a CSRF - # token, and `approve` never acts on ambient authority — no cookie, no session, - # only a pasted token, redirecting to an allowlisted URI. + # 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) 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], @@ -48,6 +66,7 @@ def protected_resource # 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"), @@ -126,13 +145,59 @@ def mcp_oauth_request_problem 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})" + "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_native_client_redirects " \ + "(#{mcp_oauth_config.oauth_allow_native_client_redirects ? "enabled" : "disabled"})" ) "Unregistered redirect_uri." end + # A REMOTE target must be named exactly. A native one need not be — see + # `mcp_oauth_native_redirect_uri?` for why that is a difference in kind rather + # than a laxer rule. def mcp_oauth_redirect_uri_allowed? - Array(mcp_oauth_config.oauth_allowed_redirect_uris).include?(params[:redirect_uri].to_s) + 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_native_redirect_uri?(redirect_uri) + end + + # RFC 8252 native-client targets: loopback on any port (§7.3, the port is + # ephemeral and cannot be registered ahead of time) and private-use schemes + # (§7.1). Both deliver the code to the operator's OWN device, which is what + # makes them safe to accept unnamed — the attack the allowlist exists to stop + # needs the code to reach a remote attacker. + # + # Everything is checked against the PARSED URI, never the string: `host` is + # what a browser resolves, so `http://127.0.0.1@evil.example/` (userinfo, host + # evil.example) and `http://127.0.0.1.evil.example/` are both correctly seen as + # remote. An opaque URI is refused because it cannot carry the code anyway + # (`URI#query=` raises on one), which also disposes of `javascript:alert(1)`; + # a fragment is refused because OAuth forbids one on a redirect_uri. + def mcp_oauth_native_redirect_uri?(redirect_uri) + return false unless mcp_oauth_config.oauth_allow_native_client_redirects + + uri = mcp_oauth_parse_uri(redirect_uri) + return false if uri.nil? + + scheme = uri.scheme&.downcase + return false if scheme.nil? || !uri.opaque.nil? || !uri.fragment.nil? + return mcp_oauth_loopback_host?(uri.host) if %w[http https].include?(scheme) + + !RESERVED_REDIRECT_SCHEMES.include?(scheme) + 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) + LOOPBACK_HOSTS.include?(host.to_s.downcase.delete_prefix("[").delete_suffix("]")) end def mcp_oauth_code_challenge_supported? @@ -143,24 +208,63 @@ def mcp_oauth_code_challenge_supported? # 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 encrypted under a + # key derived from the code itself, so the cache holds nothing usable on its + # own. That is worth the few lines here because the value is not the short-lived + # credential an authorization server would normally park: it is the operator's + # pre-existing, long-lived, full-scope token, and `cache_store` is documented + # to be the host's shared `Rails.cache`. A dump of that store — a Redis + # snapshot, a FileStore on disk — now yields ciphertext whose key never landed + # in it. 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( - "#{CODE_CACHE_PREFIX}#{code}", - { access_token:, code_challenge: params[:code_challenge].to_s, redirect_uri: params[:redirect_uri].to_s }, + 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 - # Read-and-delete: a code is single-use even if the exchange then fails. + # 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 = "#{CODE_CACHE_PREFIX}#{code}" - payload = mcp_oauth_config.cache_store.read(key) - mcp_oauth_config.cache_store.delete(key) - payload + 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 + + # The code is 256 bits of `SecureRandom`, so a digest is already a uniform + # 256-bit key — the password-stretching a KDF would add buys nothing here and + # would cost a PBKDF2 run per request. Cipher and serializer are pinned rather + # than inherited: the gem supports ActiveSupport >= 6.1, where the defaults for + # both are Rails-configuration-dependent. + def mcp_oauth_encryptor(code) + ActiveSupport::MessageEncryptor.new( + Digest::SHA256.digest("#{CODE_CACHE_PREFIX}key:#{code}"), cipher: "aes-256-gcm" + ) end def mcp_oauth_exchange_valid?(payload) @@ -227,6 +331,19 @@ def mcp_oauth_render_bad_request(message) render plain: message, status: :bad_request end + # Both metadata documents name the `authorization_endpoint` an operator will be + # sent to, and they are built from the live request origin (`request.base_url`, + # which honours `X-Forwarded-Host`). A shared cache that stored one keyed only + # by path could therefore serve every client an origin an attacker chose, and + # the document itself would be what vouches for it — the operator lands on the + # attacker's page and pastes a live token. Deployments are expected to pin + # `config.hosts` (Rails' HostAuthorization rejects a forged header before it + # reaches here), but a metadata document is exactly the thing that must not be + # a shared cache's to hold, whether or not that is configured. + def mcp_oauth_forbid_caching + response.headers["Cache-Control"] = "no-store" + end + def mcp_oauth_render_token_error(code) render json: { error: code }, status: :bad_request end diff --git a/spec/mcp_toolkit/authority/controller_methods_spec.rb b/spec/mcp_toolkit/authority/controller_methods_spec.rb index 510fbce..378f08c 100644 --- a/spec/mcp_toolkit/authority/controller_methods_spec.rb +++ b/spec/mcp_toolkit/authority/controller_methods_spec.rb @@ -259,6 +259,9 @@ def rpc(method, params = {}, id: 1) 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 } end # Path-scoped: stating the location outright is also what keeps a client from @@ -270,6 +273,17 @@ def rpc(method, params = {}, id: 1) %(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 diff --git a/spec/mcp_toolkit/engine_spec.rb b/spec/mcp_toolkit/engine_spec.rb index 00becba..4eab585 100644 --- a/spec/mcp_toolkit/engine_spec.rb +++ b/spec/mcp_toolkit/engine_spec.rb @@ -103,10 +103,16 @@ def draw(&block) # 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 } } before do McpToolkit.config.auth_role = auth_role McpToolkit.config.oauth_allowed_redirect_uris = oauth_redirect_uris + McpToolkit.config.oauth_allow_native_client_redirects = oauth_allow_native + McpToolkit.config.token_authenticator = token_authenticator recorder = route_recorder stub_const("Rails", Module.new) # The engine class body now also calls `config.to_prepare { ... }` (lazy @@ -170,6 +176,31 @@ def draw(&block) 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 + # 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. diff --git a/spec/mcp_toolkit/oauth/bridge_end_to_end_spec.rb b/spec/mcp_toolkit/oauth/bridge_end_to_end_spec.rb index 9bd03ea..068a35a 100644 --- a/spec/mcp_toolkit/oauth/bridge_end_to_end_spec.rb +++ b/spec/mcp_toolkit/oauth/bridge_end_to_end_spec.rb @@ -140,10 +140,12 @@ def token_info = render(plain: "HOST_TOKEN_INFO") 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 @@ -214,6 +216,33 @@ def token_info = render(plain: "HOST_TOKEN_INFO") result["replayed_code_status"] = session.last_response.status result["replayed_code_body"] = JSON.parse(session.last_response.body) + # 10. A NATIVE client (RFC 8252), whose ephemeral loopback 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" + native_query = authorize_query.merge(redirect_uri: loopback_uri) + session.get("/mcp/oauth/authorize", native_query) + result["native_authorize_status_when_off"] = session.last_response.status + + McpToolkit.config.oauth_allow_native_client_redirects = true + session.get("/mcp/oauth/authorize", native_query) + result["native_authorize_status_when_on"] = session.last_response.status + + session.post("/mcp/oauth/authorize", native_query.merge(access_token: VALID_TOKEN)) + native_location = session.last_response.headers["Location"] + result["native_location_prefix"] = native_location && native_location.split("?").first + native_code = native_location && Rack::Utils.parse_query(URI.parse(native_location).query)["code"] + + session.post("/mcp/oauth/token", { + grant_type: "authorization_code", code: native_code, redirect_uri: loopback_uri, code_verifier: verifier + }) + result["native_token_is_the_pasted_token"] = JSON.parse(session.last_response.body)["access_token"] == VALID_TOKEN + + # Opting native clients in must not turn the remote allowlist into a + # suggestion — this is the phishing target, and it stays named-only. + session.get("/mcp/oauth/authorize", authorize_query.merge(redirect_uri: "https://attacker.example/x")) + result["remote_unregistered_status_with_native_on"] = session.last_response.status + puts JSON.generate(result) RUBY @@ -253,6 +282,37 @@ def token_info = render(plain: "HOST_TOKEN_INFO") 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 + + # RFC 8252 native clients: the ephemeral loopback port cannot be registered + # ahead of time, and it does not need to be — the code goes to the operator's + # own machine, where the phishing the allowlist stops cannot reach. + describe "native (RFC 8252) clients" do + it "refuses loopback until the host opts in" do + expect(@result.fetch("native_authorize_status_when_off")).to eq(400) + end + + it "runs a loopback client through the whole flow once opted in" do + expect(@result.fetch("native_authorize_status_when_on")).to eq(200) + expect(@result.fetch("native_location_prefix")).to eq("http://127.0.0.1:54321/cb") + expect(@result.fetch("native_token_is_the_pasted_token")).to be(true) + end + + it "keeps a remote callback allowlist-only even with native clients allowed" do + expect(@result.fetch("remote_unregistered_status_with_native_on")).to eq(400) + 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 diff --git a/spec/mcp_toolkit/oauth/controller_methods_spec.rb b/spec/mcp_toolkit/oauth/controller_methods_spec.rb index 8e46219..5be2bb9 100644 --- a/spec/mcp_toolkit/oauth/controller_methods_spec.rb +++ b/spec/mcp_toolkit/oauth/controller_methods_spec.rb @@ -22,6 +22,10 @@ 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 @@ -44,6 +48,11 @@ def redirect_to(url, **options) [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 @@ -216,6 +225,82 @@ def issue_code(overrides = {}) end end + # RFC 8252 native clients. These need no allowlist entry because the code is + # delivered to the operator's OWN machine, which is what makes them a different + # kind of target rather than a laxer rule — the phishing the allowlist exists to + # stop needs the code to reach a REMOTE attacker. + describe "native-client redirect targets" do + def authorize_with(uri) + controller.params = authorize_params(redirect_uri: uri) + controller.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 native clients" do + before { McpToolkit.config.oauth_allow_native_client_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 + + it "accepts a private-use scheme (§7.1), as a desktop MCP client registers" do + expect(authorize_with("cursor://anysphere.cursor-retrieval/oauth/callback")[:template]).to eq(:authorize) + expect(authorize_with("com.example.app:/oauth2redirect")[:template]).to eq(:authorize) + end + + # The switch says "any client on my operators' MACHINES", never "any client". + # A remote callback is the phishing vector and stays allowlist-only. + 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 native client through the full flow" do + native_uri = "http://127.0.0.1:54321/cb" + controller.params = authorize_params(redirect_uri: native_uri).merge(access_token: valid_token) + controller.approve + + expect(controller.redirected_to[:url]).to start_with("#{native_uri}?code=") + end + 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) @@ -270,10 +355,6 @@ def issue_code(overrides = {}) end describe "#token" do - let(:exchange_params) do - { grant_type: "authorization_code", redirect_uri:, code_verifier: } - end - it "hands back the pasted token itself" do code = issue_code controller.params = exchange_params.merge(code:) @@ -340,13 +421,81 @@ def issue_code(overrides = {}) 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.delete("#{described_class::CODE_CACHE_PREFIX}#{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 + 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 From 013b0c9ffd68a47dc5bf0e208ccb5542e5c77647 Mon Sep 17 00:00:00 2001 From: Karol Galanciak Date: Fri, 17 Jul 2026 13:24:49 +0200 Subject: [PATCH 07/11] Accept only loopback unnamed; a scheme denylist fails open MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A second review (Codex) found the native-redirect rule unsound, and it was right. `mcp_oauth_native_redirect_uri?` treated every scheme absent from a 13-entry denylist as local, so all of these were accepted: ssh://attacker.example/cb gopher://attacker.example/cb ldap://attacker.example/cb telnet://attacker.example/cb smb://… nfs://… Every one names a REMOTE host. That falsifies the invariant the feature was justified by and documented with — "these deliver the code to the operator's own device" — and it is a denylist doing an allowlist's job: the only way to separate a private-use scheme from a registered network one is to enumerate the IANA registry, so the list is always the ones you happened to think of. The mistake underneath was conflating two cases that are not alike. **Loopback must be accepted unnamed because it cannot be named**: the client picks an ephemeral port at runtime, so no allowlist could enumerate it — that is the whole reason RFC 8252 §7.3 exists. A **private-use scheme has no such forcing reason**: `cursor://anysphere.cursor-retrieval/oauth/callback` is a fixed string, so it can just go in `oauth_allowed_redirect_uris` like any other entry. Accepting schemes generically bought nothing and cost the invariant. So the rule is now: name every target exactly, except loopback. - `oauth_allow_native_client_redirects` -> `oauth_allow_loopback_redirects`. The old name promised more than is safe to deliver; renaming is free while 0.6.0 is unreleased. - `RESERVED_REDIRECT_SCHEMES` is gone. Nothing is judged local by its absence from a list. - An allowlisted `cursor://` client still works, and is spec'd — the rule is "name it", not "no desktop clients". Rejecting the reviewer's suggested fix (require a reverse-domain scheme, per RFC 8252 §7.1): it would reject `cursor://`, `vscode://` and `zed://`, which are exactly the clients the feature exists for, while still trusting any scheme that happens to contain a period. Two more from the same review, both correct: - The token response now carries `Cache-Control: no-store` + `Pragma: no-cache`. RFC 6749 §5.1 makes both a MUST for any response carrying a token, and the metadata documents already had no-store — omitting it on the response that actually contains the token was the wrong way round. - `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). 587 examples, 0 failures across 3 random orders. The real-Rails E2E now drives `ssh://`/`gopher://`/`ldap://` and an unnamed `cursor://` (all 400), the 303, and the token response's headers — through the full stack, since a fake host could only ever confirm the fake. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 45 ++++++--- README.md | 32 +++--- lib/mcp_toolkit/configuration.rb | 49 +++++---- lib/mcp_toolkit/oauth/controller_methods.rb | 89 +++++++++-------- spec/mcp_toolkit/engine_spec.rb | 2 +- .../oauth/bridge_end_to_end_spec.rb | 99 +++++++++++++------ .../oauth/controller_methods_spec.rb | 75 +++++++++++--- 7 files changed, 254 insertions(+), 137 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 979125e..b166809 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,18 +31,28 @@ opt-in: a host that configures nothing behaves exactly as it did on 0.5.0. 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. That attack - needs the code to reach a REMOTE attacker, which is what splits the policy in - two: a remote callback is matched by exact string against - `config.oauth_allowed_redirect_uris` and is never opened up, while RFC 8252 - native targets — loopback on any port (§7.3, the port is ephemeral and cannot be - registered ahead of time) and private-use schemes (§7.1) — are permitted - generically by `config.oauth_allow_native_client_redirects`, because they deliver - the code to the operator's OWN machine. So every MCP client running on an - operator's machine works with no configuration, while a hosted client needs one - allowlist entry. Native targets are judged on the PARSED URI - (`http://127.0.0.1@evil.example/` has host `evil.example`, and is remote), and a - fragment, an opaque URI, or a `javascript:`/`data:`/`file:` scheme is refused. + 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/*` @@ -67,7 +77,7 @@ opt-in: a host that configures nothing behaves exactly as it did on 0.5.0. 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 `[]`), - `config.oauth_allow_native_client_redirects` (default `false`), + `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`), and `config.oauth_parent_controller` (default `"ActionController::Base"`). @@ -75,15 +85,20 @@ opt-in: a host that configures nothing behaves exactly as it did on 0.5.0. 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 native-client switch), so it cannot run + 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. -- Both metadata documents are served `Cache-Control: no-store`. They name the +- 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 diff --git a/README.md b/README.md index 731cef4..da9affb 100644 --- a/README.md +++ b/README.md @@ -432,7 +432,7 @@ McpToolkit.configure do |c| # 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_native_client_redirects = true + c.oauth_allow_loopback_redirects = true end ``` @@ -479,21 +479,25 @@ 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. -That threat needs the code to reach a **remote** attacker, which is what splits -the policy in two: +So **every target must be named by exact string**, with exactly one exception: | Target | Rule | Why | |---|---|---| -| Remote (`https://client.example/cb`) | Exact string, in `oauth_allowed_redirect_uris` | The phishing vector. Never opened up. | -| Loopback (`http://127.0.0.1:*`, `localhost`, `[::1]`) | Allowed by `oauth_allow_native_client_redirects` | RFC 8252 §7.3. The code lands on the operator's own machine; the port is ephemeral and cannot be registered ahead of time. | -| Private-use scheme (`cursor://…`, `com.example.app:/cb`) | Allowed by `oauth_allow_native_client_redirects` | RFC 8252 §7.1. Handed to a locally registered app; the code never leaves the device. | - -So **every MCP client on your operators' machines works with no configuration**, -while a hosted client needs one allowlist entry. Native targets are judged on the -*parsed* URI — `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, an opaque URI, or a `javascript:`/`data:`/`file:` scheme is refused -outright. +| 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. ### Deployment note @@ -526,7 +530,7 @@ If your MCP endpoint IS its origin root (a dedicated MCP domain), there is no pa 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_native_client_redirects` +`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 diff --git a/lib/mcp_toolkit/configuration.rb b/lib/mcp_toolkit/configuration.rb index 7925354..721f995 100644 --- a/lib/mcp_toolkit/configuration.rb +++ b/lib/mcp_toolkit/configuration.rb @@ -137,7 +137,7 @@ class McpToolkit::Configuration # screen naming the client plus an authenticated session. This bridge mocks both # away, and this allowlist is what compensates. # - # EMPTY BY DEFAULT. Empty, and with `oauth_allow_native_client_redirects` off, + # 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. @@ -147,27 +147,36 @@ class McpToolkit::Configuration # @return [Array] attr_accessor :oauth_allowed_redirect_uris - # Permits NATIVE-client targets generically, without naming each one: loopback - # on any port (RFC 8252 §7.3 — the port is ephemeral, so it could not be named - # ahead of time) and private-use schemes (§7.1), i.e. `http://127.0.0.1:54321/cb`, - # `http://localhost:*/cb`, `cursor://…`. - # - # Safe to open generically for the same reason the allowlist above cannot be: - # these deliver the code to the OPERATOR'S OWN DEVICE, and the phishing above - # needs it to reach a REMOTE attacker. (The residual risk is a malicious app - # already installed on that machine squatting the scheme — local code execution, - # a lost game regardless. RFC 8252 lets native apps skip pre-registration on - # this same reasoning.) A remote `https://` callback is never covered by this, - # whatever it is set to. - # - # OFF BY DEFAULT: switching it on says "any MCP client on my operators' machines - # may receive a code", which is a decision, not a default — and so is an opt-in + # 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_native_client_redirects = true + # c.oauth_allow_loopback_redirects = true # # @return [Boolean] - attr_accessor :oauth_allow_native_client_redirects + 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 @@ -556,7 +565,7 @@ def initialize # never configures it is unaffected. def initialize_oauth_bridge_defaults @oauth_allowed_redirect_uris = [] - @oauth_allow_native_client_redirects = false + @oauth_allow_loopback_redirects = false @oauth_resource_path = "/mcp" @oauth_authorization_code_ttl = 60 @oauth_parent_controller = "ActionController::Base" @@ -734,7 +743,7 @@ def oauth_bridge? return false unless authority? return false if token_authenticator.nil? - Array(oauth_allowed_redirect_uris).any? || !!oauth_allow_native_client_redirects + Array(oauth_allowed_redirect_uris).any? || !!oauth_allow_loopback_redirects end # Full introspection URL the satellite POSTs to. Raises a clear error if the diff --git a/lib/mcp_toolkit/oauth/controller_methods.rb b/lib/mcp_toolkit/oauth/controller_methods.rb index 70ad533..3a327f8 100644 --- a/lib/mcp_toolkit/oauth/controller_methods.rb +++ b/lib/mcp_toolkit/oauth/controller_methods.rb @@ -35,14 +35,6 @@ module McpToolkit::Oauth::ControllerMethods # reason these need no allowlist entry. LOOPBACK_HOSTS = ["127.0.0.1", "::1", "localhost"].freeze - # Schemes that are never a private-use (native app) scheme: the web schemes, - # which travel to a remote host and so must be allowlisted; and the - # pseudo-schemes a browser may treat as script or local content, which have no - # business receiving an authorization code whatever a client claims. - RESERVED_REDIRECT_SCHEMES = %w[ - http https ws wss ftp sftp file data javascript vbscript blob about view-source - ].freeze - 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 @@ -106,7 +98,13 @@ def approve access_token = params[:access_token].to_s return mcp_oauth_reject_paste if mcp_oauth_authenticate(access_token).nil? - redirect_to mcp_oauth_callback_url(mcp_oauth_issue_code(access_token)), allow_other_host: true + # 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 @@ -119,6 +117,7 @@ def token 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 @@ -146,46 +145,50 @@ 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_native_client_redirects " \ - "(#{mcp_oauth_config.oauth_allow_native_client_redirects ? "enabled" : "disabled"})" + "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 - # A REMOTE target must be named exactly. A native one need not be — see - # `mcp_oauth_native_redirect_uri?` for why that is a difference in kind rather - # than a laxer rule. + # 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_native_redirect_uri?(redirect_uri) + mcp_oauth_loopback_redirect_uri?(redirect_uri) end - # RFC 8252 native-client targets: loopback on any port (§7.3, the port is - # ephemeral and cannot be registered ahead of time) and private-use schemes - # (§7.1). Both deliver the code to the operator's OWN device, which is what - # makes them safe to accept unnamed — the attack the allowlist exists to stop - # needs the code to reach a remote attacker. + # RFC 8252 §7.3 loopback: the ONLY target accepted without being named, because + # it is the only one that CANNOT be named — the client listens on an ephemeral + # port chosen at runtime, so no allowlist could enumerate it. That, and not + # "native clients are trusted", is the whole justification: an allowlist entry + # is impossible here and merely inconvenient everywhere else. # - # Everything is checked against the PARSED URI, never the string: `host` is - # what a browser resolves, so `http://127.0.0.1@evil.example/` (userinfo, host - # evil.example) and `http://127.0.0.1.evil.example/` are both correctly seen as - # remote. An opaque URI is refused because it cannot carry the code anyway - # (`URI#query=` raises on one), which also disposes of `javascript:alert(1)`; - # a fragment is refused because OAuth forbids one on a redirect_uri. - def mcp_oauth_native_redirect_uri?(redirect_uri) - return false unless mcp_oauth_config.oauth_allow_native_client_redirects + # A private-use scheme (`cursor://…`, §7.1) is deliberately NOT accepted here, + # even though it also keeps the code on the device. Its redirect URI is a fixed + # string, so it can simply go in `oauth_allowed_redirect_uris` — there is no + # forcing reason to accept it unnamed, and accepting whole SCHEMES generically + # cannot be done safely: the only way to separate a private-use scheme from a + # registered network one (`ssh:`, `ldap:`, `gopher:` — each naming a REMOTE + # host) is to enumerate the IANA registry, and a denylist of the ones you + # thought of is exactly the shape that fails open. + # + # Checked against the PARSED URI, never the string: `host` is what a browser + # resolves, so `http://127.0.0.1@evil.example/` (userinfo; host evil.example) + # and `http://127.0.0.1.evil.example/` are both correctly seen as 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? - scheme = uri.scheme&.downcase - return false if scheme.nil? || !uri.opaque.nil? || !uri.fragment.nil? - return mcp_oauth_loopback_host?(uri.host) if %w[http https].include?(scheme) - - !RESERVED_REDIRECT_SCHEMES.include?(scheme) + mcp_oauth_loopback_host?(uri.host) end def mcp_oauth_parse_uri(value) @@ -331,17 +334,19 @@ def mcp_oauth_render_bad_request(message) render plain: message, status: :bad_request end - # Both metadata documents name the `authorization_endpoint` an operator will be - # sent to, and they are built from the live request origin (`request.base_url`, - # which honours `X-Forwarded-Host`). A shared cache that stored one keyed only - # by path could therefore serve every client an origin an attacker chose, and - # the document itself would be what vouches for it — the operator lands on the - # attacker's page and pastes a live token. Deployments are expected to pin - # `config.hosts` (Rails' HostAuthorization rejects a forged header before it - # reaches here), but a metadata document is exactly the thing that must not be - # a shared cache's to hold, whether or not that is configured. + # 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. Deployments are expected to pin `config.hosts` (Rails' + # HostAuthorization then rejects a forged header before it reaches here), but a + # metadata document is exactly the thing that must not be a shared cache's to + # hold, whether or not that is configured. def mcp_oauth_forbid_caching response.headers["Cache-Control"] = "no-store" + response.headers["Pragma"] = "no-cache" end def mcp_oauth_render_token_error(code) diff --git a/spec/mcp_toolkit/engine_spec.rb b/spec/mcp_toolkit/engine_spec.rb index 4eab585..d25a0c0 100644 --- a/spec/mcp_toolkit/engine_spec.rb +++ b/spec/mcp_toolkit/engine_spec.rb @@ -111,7 +111,7 @@ def draw(&block) before do McpToolkit.config.auth_role = auth_role McpToolkit.config.oauth_allowed_redirect_uris = oauth_redirect_uris - McpToolkit.config.oauth_allow_native_client_redirects = oauth_allow_native + McpToolkit.config.oauth_allow_loopback_redirects = oauth_allow_native McpToolkit.config.token_authenticator = token_authenticator recorder = route_recorder stub_const("Rails", Module.new) diff --git a/spec/mcp_toolkit/oauth/bridge_end_to_end_spec.rb b/spec/mcp_toolkit/oauth/bridge_end_to_end_spec.rb index 068a35a..b1053a5 100644 --- a/spec/mcp_toolkit/oauth/bridge_end_to_end_spec.rb +++ b/spec/mcp_toolkit/oauth/bridge_end_to_end_spec.rb @@ -216,32 +216,45 @@ def token_info = render(plain: "HOST_TOKEN_INFO") result["replayed_code_status"] = session.last_response.status result["replayed_code_body"] = JSON.parse(session.last_response.body) - # 10. A NATIVE client (RFC 8252), whose ephemeral loopback port no host could + # 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" - native_query = authorize_query.merge(redirect_uri: loopback_uri) - session.get("/mcp/oauth/authorize", native_query) - result["native_authorize_status_when_off"] = session.last_response.status + 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_native_client_redirects = true - session.get("/mcp/oauth/authorize", native_query) - result["native_authorize_status_when_on"] = 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", native_query.merge(access_token: VALID_TOKEN)) - native_location = session.last_response.headers["Location"] - result["native_location_prefix"] = native_location && native_location.split("?").first - native_code = native_location && Rack::Utils.parse_query(URI.parse(native_location).query)["code"] + 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: native_code, redirect_uri: loopback_uri, code_verifier: verifier + grant_type: "authorization_code", code: loopback_code, redirect_uri: loopback_uri, code_verifier: verifier }) - result["native_token_is_the_pasted_token"] = JSON.parse(session.last_response.body)["access_token"] == VALID_TOKEN - - # Opting native clients in must not turn the remote allowlist into a - # suggestion — this is the phishing target, and it stays named-only. - session.get("/mcp/oauth/authorize", authorize_query.merge(redirect_uri: "https://attacker.example/x")) - result["remote_unregistered_status_with_native_on"] = session.last_response.status + 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 puts JSON.generate(result) RUBY @@ -294,22 +307,48 @@ def token_info = render(plain: "HOST_TOKEN_INFO") end end - # RFC 8252 native clients: the ephemeral loopback port cannot be registered - # ahead of time, and it does not need to be — the code goes to the operator's - # own machine, where the phishing the allowlist stops cannot reach. - describe "native (RFC 8252) clients" do + # 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("native_authorize_status_when_off")).to eq(400) + 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("native_authorize_status_when_on")).to eq(200) - expect(@result.fetch("native_location_prefix")).to eq("http://127.0.0.1:54321/cb") - expect(@result.fetch("native_token_is_the_pasted_token")).to be(true) + 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 + + # 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 "keeps a remote callback allowlist-only even with native clients allowed" do - expect(@result.fetch("remote_unregistered_status_with_native_on")).to eq(400) + 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 @@ -388,8 +427,10 @@ def token_info = render(plain: "HOST_TOKEN_INFO") 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(302) + 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 diff --git a/spec/mcp_toolkit/oauth/controller_methods_spec.rb b/spec/mcp_toolkit/oauth/controller_methods_spec.rb index 5be2bb9..44d0a50 100644 --- a/spec/mcp_toolkit/oauth/controller_methods_spec.rb +++ b/spec/mcp_toolkit/oauth/controller_methods_spec.rb @@ -225,11 +225,11 @@ def issue_code(overrides = {}) end end - # RFC 8252 native clients. These need no allowlist entry because the code is - # delivered to the operator's OWN machine, which is what makes them a different - # kind of target rather than a laxer rule — the phishing the allowlist exists to - # stop needs the code to reach a REMOTE attacker. - describe "native-client redirect targets" do + # 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.authorize @@ -242,8 +242,8 @@ def authorize_with(uri) end end - context "when the host allows native clients" do - before { McpToolkit.config.oauth_allow_native_client_redirects = true } + 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. @@ -256,13 +256,23 @@ def authorize_with(uri) expect(authorize_with("http://[::1]:8080/cb")[:template]).to eq(:authorize) end - it "accepts a private-use scheme (§7.1), as a desktop MCP client registers" do - expect(authorize_with("cursor://anysphere.cursor-retrieval/oauth/callback")[:template]).to eq(:authorize) - expect(authorize_with("com.example.app:/oauth2redirect")[:template]).to eq(:authorize) + # 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 - # The switch says "any client on my operators' MACHINES", never "any client". - # A remote callback is the phishing vector and stays allowlist-only. 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 @@ -291,16 +301,49 @@ def authorize_with(uri) expect(authorize_with("http://127.0.0.1:5/cb#frag")[:options]).to include(status: :bad_request) end - it "hands a code to a native client through the full flow" do - native_uri = "http://127.0.0.1:54321/cb" - controller.params = authorize_params(redirect_uri: native_uri).merge(access_token: valid_token) + 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.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.approve - expect(controller.redirected_to[:url]).to start_with("#{native_uri}?code=") + expect(controller.redirected_to[:url]).to start_with("#{scheme_uri}?code=") end 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.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) From f907f81c1512fb2060a5bb2c3054213e85c8f27f Mon Sep 17 00:00:00 2001 From: Karol Galanciak Date: Fri, 17 Jul 2026 14:29:58 +0200 Subject: [PATCH 08/11] Stop the code being the key; fix four defects an independent review found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four independent reviewers over this branch. No exploitable vulnerability: PKCE matches the RFC 7636 vector and survived 17 bypass attempts, the loopback rule survived a differential fuzz (3,586 candidates, Ruby decides / WHATWG URL adjudicates the emitted Location — 672 accepted, 0 escapes), key derivation has no length-extension path, single-use holds across MemoryStore/Redis/MemCache. What they did find was a set of claims this branch makes that the code did not keep. Each is fixed here rather than reworded, except where the code was right and the prose was wrong. ## The code was the key, and Rails logs the code The payload was sealed under `SHA256(prefix + code)`. But 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 the key to the cache entry was sitting in the log: an artifact more widely read, longer retained and more replicated than the 60-second entry it protected. "A dump of that store yields ciphertext whose key never landed in it" was true and beside the point. Worse, it was two locally-defensible decisions that combine badly: I made the code a decryption key, then separately decided a logged `code` was harmless because it is single-use and already spent. Both true in isolation. Together they handed the key away. Now HMAC'd from `config.oauth_signing_secret` AND the code. The secret defaults to the Rails app's `secret_key_base` — already the "server-held, in ENV, never logged" value this wants — so a Rails host configures nothing and a non-Rails host must set one (`oauth_bridge?` refuses to run without it, rather than sealing with something weaker). ## The serializer was never pinned The comment claimed "cipher AND serializer are pinned rather than inherited", naming Rails-config-dependence as the hazard. Only `cipher:` was passed. Every default across the supported ActiveSupport range reaches `Marshal.load` — `:marshal`, and 7.1+'s `:json_allow_marshal` — so a host with cache-write access who knows one code (they run the flow with their own token) could forge a valid AEAD blob wrapping a Marshal gadget and get code execution. Narrow, but cache compromise is the exact threat the sealing exists for. Pinned to NullSerializer: the payload is already a JSON String, so JSON.parse is now the only parser that ever sees it, and the blob shrinks 140B -> 128B. ## My own quickstart would break a real deployment `cache_store` defaults to an in-process MemoryStore. The README's OAuth quickstart never set it, while the code asserted it "is documented to be the host's shared Rails.cache" — a `should` restated as an `is`. On a multi-worker Puma the two legs land on different workers, so the flow fails (N-1)/N of the time, intermittently, AFTER the operator has pasted a live token. That is exactly what `oauth_bridge?` gates on `token_authenticator` to prevent; I applied the principle to one dependency and skipped the one that fails silently. Warned once at boot, not gated: a MemoryStore is correct in a single process, and `Rails.cache` IS one in a stock development environment — gating would make the bridge undevelopable locally to prevent a production mistake. The quickstart now sets it. ## Three more - Allowlist entries are validated at assignment, like `filter_operator_overrides=` and for the same reason. An opaque URI (`com.example.app:cb` — legal syntax, and the docs invite private-use schemes into this list) cannot carry a query, so `URI#query=` raised — a 500 after the token was verified and a code cached. Now an ArgumentError at boot, naming the fix. - The bridge's four routes take `format: false`, as the metadata routes already did. Without it `/mcp/oauth/authorize.json` reached the action, found no JSON template and 500'd unauthenticated. - `code`/`state` are SET, not appended. A loopback redirect_uri is not exact-matched, so a caller can pass `?code=` themselves and get `?code=theirs&code=ours`, leaving the winner to the client's parser. The callback URL is also now built from the validated string rather than parse-and-reconstructed, which is what raised on opaque URIs. - The two browser legs are guarded by a `before_action`, not a check in each action body. `authorize` is a common method name; a gem defining one on ActionController::Base would drop the action from Rails' `action_methods`, and Rails would then serve authorize.html.erb by implicit render — skipping a body guard and showing an attacker's redirect_uri a paste page. ## Claims corrected rather than code changed "A real authorization server blocks this with a consent screen plus an authenticated session; this allowlist is what compensates" was wrong twice. The allowlist IS the redirect-URI registration a real AS does — it compensates for nothing extra. And consent would not block that attack anyway: the operator wants to connect the client and would click Allow. What the allowlist does not cover is now stated plainly: it binds which URL a code goes to, not whose session at that URL receives it, so a multi-tenant client's `state` handling (RFC 6819 §4.4.1.7) is load-bearing and outside this gem. A full authorization server has the identical exposure. `config.hosts` is now imperative, not "expected": Rails leaves it EMPTY in production, so the mitigation the comment leaned on is one Rails does not give you. ## Verification 598 examples, 0 failures across 3 random orders on all three CI rubies (3.2.5/3.3.11/3.4.9 — the floor matters again: OpenSSL::HMAC, NullSerializer). RuboCop 64/0, Brakeman 0. The unit spec's fake controller grew a real before_action chain — without one it would have kept passing while the guard was gone. Real-Rails E2E now also proves secret_key_base resolves with no config, the .json suffix 404s, and a polluted loopback callback emits exactly one code while keeping the client's own query. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 61 ++++++-- README.md | 26 +++- config/routes.rb | 12 +- lib/mcp_toolkit/configuration.rb | 130 +++++++++++++++++- lib/mcp_toolkit/engine_controllers.rb | 20 +++ lib/mcp_toolkit/oauth/controller_methods.rb | 126 ++++++++++++----- .../authority/controller_methods_spec.rb | 2 + spec/mcp_toolkit/engine_spec.rb | 37 ++++- .../oauth/bridge_end_to_end_spec.rb | 48 +++++++ .../oauth/controller_methods_spec.rb | 128 ++++++++++++++--- 10 files changed, 511 insertions(+), 79 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b166809..e4d78db 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -76,11 +76,14 @@ opt-in: a host that configures nothing behaves exactly as it did on 0.5.0. 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 `[]`), - `config.oauth_allow_loopback_redirects` (default `false`), +- `config.oauth_allowed_redirect_uris` (default `[]`; entries are validated at + assignment — an unparseable, scheme-less, fragment-bearing or *opaque* URI + raises, because the alternative is a 500 after the operator has already pasted + their token), `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`), and - `config.oauth_parent_controller` (default `"ActionController::Base"`). + 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 @@ -107,15 +110,31 @@ opt-in: a host that configures nothing behaves exactly as it did on 0.5.0. ### 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` under a - key derived from the code, which never itself lands in the store — so a dump of - it (a Redis snapshot, a FileStore on disk) yields ciphertext with no key, and a - payload swapped in a writable cache does not decrypt. 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, and `cache_store` is documented to be the host's - shared `Rails.cache`. Codes are also single-use by the DELETE rather than the - read, so of two concurrent redemptions exactly one proceeds. + 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 @@ -126,6 +145,22 @@ opt-in: a host that configures nothing behaves exactly as it did on 0.5.0. - If a host logs request parameters, add `code_verifier` to `config.filter_parameters`; `access_token` is already covered by the stock `token` entry Rails ships. +- **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. diff --git a/README.md b/README.md index da9affb..f839c9c 100644 --- a/README.md +++ b/README.md @@ -425,6 +425,12 @@ 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 @@ -499,12 +505,26 @@ Loopback is judged on the *parsed* URI, so `http://127.0.0.1@evil.example/` (hos `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`. **Pin `config.hosts`** so -Rails' `HostAuthorization` rejects a forged header before it reaches the bridge; -Rails leaves it empty in production by default. Both metadata documents are served +(`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. If you log request parameters, add `code_verifier` to `config.filter_parameters` (`access_token` is already covered by the stock `token` diff --git a/config/routes.rb b/config/routes.rb index 0115573..c43c8a7 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -41,10 +41,14 @@ # 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" - post "oauth/authorize", to: "oauth#approve" - post "oauth/token", to: "oauth#token" - post "oauth/register", to: "oauth#register" + 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/configuration.rb b/lib/mcp_toolkit/configuration.rb index 721f995..825d758 100644 --- a/lib/mcp_toolkit/configuration.rb +++ b/lib/mcp_toolkit/configuration.rb @@ -133,9 +133,22 @@ class McpToolkit::Configuration # 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. A real authorization server blocks this with a consent - # screen naming the client plus an authenticated session. This bridge mocks both - # away, and this allowlist is what compensates. + # 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 @@ -145,7 +158,32 @@ class McpToolkit::Configuration # c.oauth_allowed_redirect_uris = ["https://client.example/callback"] # # @return [Array] - attr_accessor :oauth_allowed_redirect_uris + attr_reader :oauth_allowed_redirect_uris + + # Assigns the allowlist, rejecting an entry the bridge could not actually + # 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 building the callback URL raise. A + # typo would cost the operator their paste and leave an orphaned code behind. + # + # Rejected: anything unparseable, anything with no scheme (a bare path is not a + # redirect target), a fragment (OAuth forbids one on a redirect_uri), and an + # OPAQUE URI — `com.example.app:cb` rather than `com.example.app:/cb`. The last + # is the sharp one, because the docs actively invite private-use schemes into + # this list and the opaque form is legal URI syntax; it simply cannot carry a + # query, so no code could ever reach it. + def oauth_allowed_redirect_uris=(uris) + @oauth_allowed_redirect_uris = Array(uris).map(&:to_s).each do |uri| + problem = oauth_redirect_uri_problem(uri) + next unless problem + + raise ArgumentError, + "oauth_allowed_redirect_uris contains #{uri.inspect}, which the bridge could never redirect to: " \ + "#{problem}" + end + 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`. @@ -198,6 +236,39 @@ class McpToolkit::Configuration # @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") + # + # Rotating it invalidates in-flight codes (a 60s window), nothing else — the + # tokens themselves are the host's and are untouched. + # + # @return [String, nil] + attr_writer :oauth_signing_secret + + # 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. # @@ -569,6 +640,7 @@ def initialize_oauth_bridge_defaults @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 @@ -733,19 +805,63 @@ def oauth_authorization_server_path # 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 - # native-client 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. + # 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 could never receive a code, or nil if it could. + def oauth_redirect_uri_problem(uri) + parsed = oauth_parse_redirect_uri(uri) + return "it is not a valid URI" if parsed.nil? + + return "it names no scheme" if parsed.scheme.nil? + return "it carries a fragment, which OAuth forbids on a redirect_uri" unless parsed.fragment.nil? + return nil if parsed.opaque.nil? + + "it is opaque (#{parsed.scheme}:#{parsed.opaque}) so it cannot carry the code — " \ + "write it with a path, e.g. #{parsed.scheme}:/#{parsed.opaque}" + 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_controllers.rb b/lib/mcp_toolkit/engine_controllers.rb index ee8454e..db0568b 100644 --- a/lib/mcp_toolkit/engine_controllers.rb +++ b/lib/mcp_toolkit/engine_controllers.rb @@ -111,9 +111,29 @@ def mcp_extract_token # 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) diff --git a/lib/mcp_toolkit/oauth/controller_methods.rb b/lib/mcp_toolkit/oauth/controller_methods.rb index 3a327f8..0fdbcfa 100644 --- a/lib/mcp_toolkit/oauth/controller_methods.rb +++ b/lib/mcp_toolkit/oauth/controller_methods.rb @@ -35,6 +35,10 @@ module McpToolkit::Oauth::ControllerMethods # reason these need no allowlist entry. LOOPBACK_HOSTS = ["127.0.0.1", "::1", "localhost"].freeze + # 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 + 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 @@ -42,6 +46,15 @@ module McpToolkit::Oauth::ControllerMethods # 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. + before_action :mcp_oauth_validate_request!, only: %i[authorize approve] if respond_to?(:before_action) end # `resource` MUST equal the MCP endpoint URL as the operator typed it into the @@ -83,18 +96,12 @@ def register end def authorize - problem = mcp_oauth_request_problem - return mcp_oauth_render_bad_request(problem) if problem - render :authorize, layout: false end # The token is verified here, not only at exchange, so a typo fails on the page # the operator is looking at. def approve - problem = mcp_oauth_request_problem - return mcp_oauth_render_bad_request(problem) if problem - access_token = params[:access_token].to_s return mcp_oauth_reject_paste if mcp_oauth_authenticate(access_token).nil? @@ -129,8 +136,14 @@ def mcp_oauth_config # ---- request validation --------------------------------------------------- - # 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. + # 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? @@ -212,14 +225,11 @@ def mcp_oauth_code_challenge_supported? # 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 encrypted under a - # key derived from the code itself, so the cache holds nothing usable on its - # own. That is worth the few lines here because the value is not the short-lived - # credential an authorization server would normally park: it is the operator's - # pre-existing, long-lived, full-scope token, and `cache_store` is documented - # to be the host's shared `Rails.cache`. A dump of that store — a Redis - # snapshot, a FileStore on disk — now yields ciphertext whose key never landed - # in it. + # 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 = { @@ -259,17 +269,41 @@ def mcp_oauth_code_key(code) "#{CODE_CACHE_PREFIX}#{Digest::SHA256.hexdigest(code)}" end - # The code is 256 bits of `SecureRandom`, so a digest is already a uniform - # 256-bit key — the password-stretching a KDF would add buys nothing here and - # would cost a PBKDF2 run per request. Cipher and serializer are pinned rather - # than inherited: the gem supports ActiveSupport >= 6.1, where the defaults for - # both are Rails-configuration-dependent. + # Keyed on a SERVER-HELD secret as well as the code, and that is the whole + # point: deriving from the code alone made the code the entire secret, and the + # code is not one. Rails logs it 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 an artifact that is more widely read, longer retained and more + # replicated than a 60-second cache entry was carrying the key to it. Mixing in + # `oauth_signing_secret` means the cache, the logs and the code together still + # open nothing without a secret that lives in ENV and is never logged. + # + # HMAC rather than a bare digest because two independent inputs are being + # combined and HMAC is what does that safely. No password-stretching: both + # inputs are already high-entropy (a 256-bit `SecureRandom` code, a real + # `secret_key_base`), so a PBKDF2 run per request would buy nothing. + # + # Cipher AND serializer are pinned rather than inherited — the gem supports + # ActiveSupport >= 6.1, where both defaults are Rails-configuration-dependent. + # The serializer especially: every default in that range (`:marshal`, and + # 7.1+'s `:json_allow_marshal`) reaches `Marshal.load`, so a host with cache + # write access forging one blob would get remote code execution. The payload is + # already a JSON String, so NullSerializer is exactly right and JSON.parse ends + # up the only parser that ever sees it. def mcp_oauth_encryptor(code) + key = OpenSSL::HMAC.digest("SHA256", mcp_oauth_signing_secret, "#{CODE_CACHE_PREFIX}key:#{code}") ActiveSupport::MessageEncryptor.new( - Digest::SHA256.digest("#{CODE_CACHE_PREFIX}key:#{code}"), cipher: "aes-256-gcm" + 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) @@ -312,15 +346,38 @@ def mcp_oauth_endpoint_url(action) "#{mcp_oauth_resource_url}/oauth/#{action}" end - # Appends `code` (and echoes `state`) onto the client's redirect_uri, preserving - # any query it already carries. + # 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. + # + # Built by string, not by `URI#query=`: the value here has already been checked + # by the redirect policy, and re-parsing it to reconstruct it only invents ways + # for the emitted URL to differ from the one that was approved — `URI#query=` + # also raises outright on an opaque URI (`com.example.app:cb`), which a host may + # legitimately allowlist. def mcp_oauth_callback_url(code) - uri = URI.parse(params[:redirect_uri].to_s) - query = URI.decode_www_form(uri.query.to_s) - query << ["code", code] - query << ["state", params[:state].to_s] if params[:state].present? - uri.query = URI.encode_www_form(query) - uri.to_s + 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. Malformed escapes are dropped rather than raised on — + # the alternative is a 500 after the operator has already pasted their token. + 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 ------------------------------------------------------------ @@ -340,10 +397,11 @@ def mcp_oauth_render_bad_request(message) # 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. Deployments are expected to pin `config.hosts` (Rails' - # HostAuthorization then rejects a forged header before it reaches here), but a - # metadata document is exactly the thing that must not be a shared cache's to - # hold, whether or not that is configured. + # 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" diff --git a/spec/mcp_toolkit/authority/controller_methods_spec.rb b/spec/mcp_toolkit/authority/controller_methods_spec.rb index 378f08c..985f9f7 100644 --- a/spec/mcp_toolkit/authority/controller_methods_spec.rb +++ b/spec/mcp_toolkit/authority/controller_methods_spec.rb @@ -262,6 +262,8 @@ def rpc(method, params = {}, id: 1) # 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" end # Path-scoped: stating the location outright is also what keeps a client from diff --git a/spec/mcp_toolkit/engine_spec.rb b/spec/mcp_toolkit/engine_spec.rb index d25a0c0..ff907e5 100644 --- a/spec/mcp_toolkit/engine_spec.rb +++ b/spec/mcp_toolkit/engine_spec.rb @@ -90,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 @@ -107,12 +117,16 @@ def draw(&block) # 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" } 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 @@ -174,6 +188,15 @@ def draw(&block) 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 @@ -201,6 +224,18 @@ def draw(&block) 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. diff --git a/spec/mcp_toolkit/oauth/bridge_end_to_end_spec.rb b/spec/mcp_toolkit/oauth/bridge_end_to_end_spec.rb index b1053a5..40174e2 100644 --- a/spec/mcp_toolkit/oauth/bridge_end_to_end_spec.rb +++ b/spec/mcp_toolkit/oauth/bridge_end_to_end_spec.rb @@ -130,6 +130,12 @@ def token_info = render(plain: "HOST_TOKEN_INFO") 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") @@ -256,6 +262,21 @@ def token_info = render(plain: "HOST_TOKEN_INFO") result[key] = session.last_response.status end + # 11. A format suffix must not reach the action: there is no JSON template, so + # it would 500 unauthenticated. + session.get("/mcp/oauth/authorize.json", authorize_query) + result["authorize_json_suffix_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 @@ -339,6 +360,33 @@ def token_info = render(plain: "HOST_TOKEN_INFO") 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 + 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 diff --git a/spec/mcp_toolkit/oauth/controller_methods_spec.rb b/spec/mcp_toolkit/oauth/controller_methods_spec.rb index 44d0a50..7d3bdde 100644 --- a/spec/mcp_toolkit/oauth/controller_methods_spec.rb +++ b/spec/mcp_toolkit/oauth/controller_methods_spec.rb @@ -4,11 +4,27 @@ # 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 and redirect_to. before_actions are not auto-run (there is -# no filter runner), which is fine — the bridge has none; each action guards itself. +# 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 @@ -18,6 +34,17 @@ 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 @@ -59,6 +86,10 @@ def redirect_to(url, **options) 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" end end @@ -75,7 +106,7 @@ def authorize_params(overrides = {}) # 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.approve + controller.dispatch(:approve) URI.decode_www_form(URI.parse(controller.redirected_to[:url]).query).to_h["code"] end @@ -182,7 +213,7 @@ def issue_code(overrides = {}) describe "#authorize" do it "renders the paste page for a well-formed request" do controller.params = authorize_params - controller.authorize + controller.dispatch(:authorize) expect(controller.rendered).to include(template: :authorize) expect(controller.rendered[:options]).to include(layout: false) @@ -190,7 +221,7 @@ def issue_code(overrides = {}) it "refuses an unregistered redirect_uri rather than redirecting to it" do controller.params = authorize_params(redirect_uri: "https://attacker.example/steal") - controller.authorize + controller.dispatch(:authorize) expect(controller.rendered[:options]).to include(status: :bad_request) expect(controller.redirected_to).to be_nil @@ -203,7 +234,7 @@ def issue_code(overrides = {}) logger = instance_double(Logger, warn: nil) McpToolkit.config.logger = logger controller.params = authorize_params(redirect_uri: "https://client.example/callback/") - controller.authorize + controller.dispatch(:authorize) expect(logger).to have_received(:warn).with( a_string_including("https://client.example/callback/").and(a_string_including(redirect_uri)) @@ -212,14 +243,14 @@ def issue_code(overrides = {}) it "refuses a request with no PKCE challenge" do controller.params = authorize_params(code_challenge: nil) - controller.authorize + 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.authorize + controller.dispatch(:authorize) expect(controller.rendered[:options]).to include(status: :bad_request) end @@ -232,7 +263,7 @@ def issue_code(overrides = {}) describe "loopback redirect targets" do def authorize_with(uri) controller.params = authorize_params(redirect_uri: uri) - controller.authorize + controller.dispatch(:authorize) controller.rendered end @@ -304,7 +335,7 @@ def authorize_with(uri) 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.approve + controller.dispatch(:approve) expect(controller.redirected_to[:url]).to start_with("#{loopback_uri}?code=") end @@ -315,13 +346,43 @@ def authorize_with(uri) 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.approve + 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 + 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 @@ -338,7 +399,7 @@ def authorize_with(uri) # 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.approve + controller.dispatch(:approve) expect(controller.redirected_to[:options]).to include(status: :see_other) end @@ -347,7 +408,7 @@ def authorize_with(uri) 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.approve + controller.dispatch(:approve) redirect = URI.parse(controller.redirected_to[:url]) query = URI.decode_www_form(redirect.query).to_h @@ -361,7 +422,7 @@ def authorize_with(uri) 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.approve + controller.dispatch(:approve) query = URI.decode_www_form(URI.parse(controller.redirected_to[:url]).query).to_h expect(query["tenant"]).to eq("acme") @@ -370,7 +431,7 @@ def authorize_with(uri) 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.approve + controller.dispatch(:approve) expect(controller.redirected_to).to be_nil expect(controller.rendered).to include(template: :authorize) @@ -379,7 +440,7 @@ def authorize_with(uri) it "re-renders the page when no token was pasted" do controller.params = authorize_params.merge(access_token: "") - controller.approve + controller.dispatch(:approve) expect(controller.redirected_to).to be_nil expect(controller.rendered[:options]).to include(status: :unprocessable_content) @@ -390,7 +451,7 @@ def authorize_with(uri) 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.approve + controller.dispatch(:approve) expect(controller.redirected_to).to be_nil expect(controller.rendered[:options]).to include(status: :bad_request) @@ -527,6 +588,39 @@ def cached_values 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 From 1d0b24877661eabfdd19ae9832504ad064aa42e8 Mon Sep 17 00:00:00 2001 From: Karol Galanciak Date: Fri, 17 Jul 2026 14:34:26 +0200 Subject: [PATCH 09/11] Trim the round's comments to what only the code can say MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fullstack-lint. CI linters were already green with their exact flags (rubocop 64/0; brakeman --force --no-progress --quiet --no-pager 0/0; rspec 598/0). This is the judgment half. The concern had drifted to 38% comments — the same file the last lint round cut to 29%, and the same cause: duplication rather than documentation. - `mcp_oauth_loopback_redirect_uri?` carried a 17-line restatement of `oauth_allow_loopback_redirects`' rationale. That argument belongs where the setting lives; here it is now a pointer plus the parsing detail, which is the only part this file alone can say. - `mcp_oauth_encryptor` had 22 comment lines over 6 of code, re-explaining why the signing secret exists — which is `Configuration#oauth_signing_secret`'s job. Kept: why HMAC, why no KDF, and why NullSerializer specifically, since that one reads like a redundant option and is the reason Marshal never sees the payload. - Dropped a comment that had already gone stale within this branch: it justified building the callback URL by string partly because "a host may legitimately allowlist an opaque URI" — untrue since the same round made that an ArgumentError at config time. Exactly the failure mode this round exists to fix, so it does not get to ship. Down to 34%, over four more methods than the file had. RSpec pass clean: no anonymous subjects, no `create(` (the suite is Rails-free), no ticket references or commented-out code in the diff. View rules N/A — the template carries no JS, and simple_form is deliberately not a dependency of a dependency-light gem. Co-Authored-By: Claude Opus 4.8 (1M context) --- lib/mcp_toolkit/oauth/controller_methods.rb | 59 ++++++--------------- 1 file changed, 17 insertions(+), 42 deletions(-) diff --git a/lib/mcp_toolkit/oauth/controller_methods.rb b/lib/mcp_toolkit/oauth/controller_methods.rb index 0fdbcfa..59cccc3 100644 --- a/lib/mcp_toolkit/oauth/controller_methods.rb +++ b/lib/mcp_toolkit/oauth/controller_methods.rb @@ -174,25 +174,14 @@ def mcp_oauth_redirect_uri_allowed? mcp_oauth_loopback_redirect_uri?(redirect_uri) end - # RFC 8252 §7.3 loopback: the ONLY target accepted without being named, because - # it is the only one that CANNOT be named — the client listens on an ephemeral - # port chosen at runtime, so no allowlist could enumerate it. That, and not - # "native clients are trusted", is the whole justification: an allowlist entry - # is impossible here and merely inconvenient everywhere else. + # 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. # - # A private-use scheme (`cursor://…`, §7.1) is deliberately NOT accepted here, - # even though it also keeps the code on the device. Its redirect URI is a fixed - # string, so it can simply go in `oauth_allowed_redirect_uris` — there is no - # forcing reason to accept it unnamed, and accepting whole SCHEMES generically - # cannot be done safely: the only way to separate a private-use scheme from a - # registered network one (`ssh:`, `ldap:`, `gopher:` — each naming a REMOTE - # host) is to enumerate the IANA registry, and a denylist of the ones you - # thought of is exactly the shape that fails open. - # - # Checked against the PARSED URI, never the string: `host` is what a browser - # resolves, so `http://127.0.0.1@evil.example/` (userinfo; host evil.example) - # and `http://127.0.0.1.evil.example/` are both correctly seen as remote. A - # fragment is refused because OAuth forbids one on a redirect_uri. + # 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 @@ -269,28 +258,16 @@ def mcp_oauth_code_key(code) "#{CODE_CACHE_PREFIX}#{Digest::SHA256.hexdigest(code)}" end - # Keyed on a SERVER-HELD secret as well as the code, and that is the whole - # point: deriving from the code alone made the code the entire secret, and the - # code is not one. Rails logs it 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 an artifact that is more widely read, longer retained and more - # replicated than a 60-second cache entry was carrying the key to it. Mixing in - # `oauth_signing_secret` means the cache, the logs and the code together still - # open nothing without a secret that lives in ENV and is never logged. - # - # HMAC rather than a bare digest because two independent inputs are being - # combined and HMAC is what does that safely. No password-stretching: both - # inputs are already high-entropy (a 256-bit `SecureRandom` code, a real - # `secret_key_base`), so a PBKDF2 run per request would buy nothing. + # 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 rather than inherited — the gem supports + # Cipher and serializer are pinned, not inherited: the gem supports # ActiveSupport >= 6.1, where both defaults are Rails-configuration-dependent. - # The serializer especially: every default in that range (`:marshal`, and + # The serializer especially — every default in that range (`:marshal`, and # 7.1+'s `:json_allow_marshal`) reaches `Marshal.load`, so a host with cache - # write access forging one blob would get remote code execution. The payload is - # already a JSON String, so NullSerializer is exactly right and JSON.parse ends - # up the only parser that ever sees it. + # write access forging one blob would get code execution. The payload is already + # a JSON String, so NullSerializer leaves JSON.parse the only parser that sees it. def mcp_oauth_encryptor(code) key = OpenSSL::HMAC.digest("SHA256", mcp_oauth_signing_secret, "#{CODE_CACHE_PREFIX}key:#{code}") ActiveSupport::MessageEncryptor.new( @@ -355,11 +332,9 @@ def mcp_oauth_endpoint_url(action) # Dropping any inbound `code`/`state` keeps the response OAuth-shaped whatever # was passed in. # - # Built by string, not by `URI#query=`: the value here has already been checked - # by the redirect policy, and re-parsing it to reconstruct it only invents ways - # for the emitted URL to differ from the one that was approved — `URI#query=` - # also raises outright on an opaque URI (`com.example.app:cb`), which a host may - # legitimately allowlist. + # Built by string, not by `URI#query=`: this value has already been checked by + # the redirect policy, so re-parsing it to reconstruct it only invents ways for + # the emitted URL to differ from the one that was approved. def mcp_oauth_callback_url(code) redirect_uri = params[:redirect_uri].to_s base, _, existing = redirect_uri.partition("?") From 43ace1ba10b9860ff00109fe7e5a622e4dae603d Mon Sep 17 00:00:00 2001 From: Karol Galanciak Date: Fri, 17 Jul 2026 15:16:22 +0200 Subject: [PATCH 10/11] Make the config surface a gate, not a suggestion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three independent reviewers plus Codex over 1d0b248. None found a token-theft primitive; the redirect policy, PKCE, single-use and the sealing all held. One reviewer built a differential fuzzer (Ruby decides, WHATWG URL adjudicates the emitted Location): 3,586 candidates, 0 escapes. The hand-rolled callback builder and the new key derivation — the two things most likely to have broken — are the two that hold up best. Every remaining defect was one class: **validation I wrote that could be skipped, bypassed, or that closed one instance of a hole and not its twin.** Fixing the class rather than the points. ## Validation that wasn't enforced - `oauth_allowed_redirect_uris` was an `attr_reader` on a mutable array, so `<<` slipped an unchecked entry past the setter entirely. Not cosmetic: a `#frag` entry emits `…/cb#frag?code=…`, which a browser reads as HASH — the code never reaches the client's server and is readable by any script on the page. The list is now frozen. - `oauth_signing_secret=` was a bare `attr_writer`. An Integer passed `oauth_bridge?`, drew the routes, and raised TypeError out of OpenSSL::HMAC at request time — after the operator pasted a live token. That is the exact failure the allowlist setter was added to prevent, in the same commit. It now validates type and a 32-byte minimum. ## Validation that stopped short - The allowlist accepted `http://attacker.example/cb` (cleartext remote — the code on the wire) and `javascript:`/`data:`/`file:` (a browser treats them as script or local content). Both now raise. Naming bad schemes is sound HERE and nowhere else, which is worth being explicit about since this branch removed a scheme denylist for failing open: that one was request-time, where the ATTACKER picks the scheme. This list is what the HOST wrote — an attacker cannot add to it — so a denylist is a config lint, not a boundary. `LOOPBACK_HOSTS` moved to McpToolkit::Oauth so the two paths cannot drift. - `format: false` closed `/mcp/oauth/authorize.json` but `Accept: application/json` picks the format just as well and still raised MissingTemplate. Renders now pin `formats: [:html]`. I closed the vector a reviewer named and left its twin. - `filter_parameters` was a README instruction. The bridge takes a live token in a POST body and Rails logs params at INFO, so this is the gem's business: an engine initializer now adds `access_token` + `code_verifier`. The stock `:token` entry covering `access_token` is a `rails new` default the gem does not own. ## Portability, found only because someone checked the floor `status: :unprocessable_content` raises below Rack 3.1, and the gemspec pins no rack/actionpack floor. On any Rails 7.x host a mistyped paste became an unauthenticated 500 on the one page whose job is to say "that token isn't valid". Neither symbol spans the supported range (`:unprocessable_entity` is deprecated above 3.1), so it is the integer. The suite could not see this: the unit spec asserts the symbol against a fake `render` that never resolves it, and the E2E pins rails ~> 8.0. ## PKCE shape (Codex) RFC 7636 §4.1 shapes are now enforced on both the challenge and the verifier. Worth being honest about what this does NOT do: the challenge is a 43-char digest whatever the verifier was, so a client that picked a one-character verifier is indistinguishable here — it has defeated its own PKCE and we cannot see it. This is enforced because a public gem should not quietly accept what the spec forbids. The gem's own test verifiers were 35 and 36 chars and are now legal. The ordering matters and is deliberate: shape is checked BEFORE the code is consumed, so a request that could never verify doesn't cost a client its code — but a well-formed WRONG verifier still burns it, which is the only thing stopping someone with an intercepted code from retrying verifiers. Both are now spec'd separately. ## Two more comments that claimed what the code doesn't do - The NullSerializer pinning was said to stop a writable cache becoming code execution. It doesn't: `Cache::Store` marshals the entry itself, so a writable cache is an RCE problem before the encryptor is reached. The pinning is right — it is determinism across AS 6.1–8.x — and the comment now says so. - The callback builder claimed the string form avoids "invent[ing] ways for the emitted URL to differ" — but it re-encodes the query anyway, deliberately, since that is where `code` gets stripped. Only the base is preserved byte-for-byte. Also: the guard now RAISES if the parent has no filter chain rather than silently skipping — same shape as the denylist this branch removed for failing open. 607 examples, 0 failures across 3 random orders on all three CI rubies. RuboCop 65/0, Brakeman 0 (CI's exact flags). Real-Rails E2E now pins the Accept header, the 422, the frozen allowlist and the scheme lint — the unit fake can resolve none of them. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 32 +++++-- README.md | 13 ++- lib/mcp_toolkit/configuration.rb | 91 ++++++++++++++---- lib/mcp_toolkit/engine.rb | 15 +++ lib/mcp_toolkit/oauth.rb | 23 +++++ lib/mcp_toolkit/oauth/controller_methods.rb | 80 +++++++++++----- .../authority/controller_methods_spec.rb | 2 +- spec/mcp_toolkit/engine_spec.rb | 6 +- .../oauth/bridge_end_to_end_spec.rb | 33 ++++++- .../oauth/controller_methods_spec.rb | 92 +++++++++++++++++-- 10 files changed, 325 insertions(+), 62 deletions(-) create mode 100644 lib/mcp_toolkit/oauth.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index e4d78db..68f8e9b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -77,9 +77,13 @@ opt-in: a host that configures nothing behaves exactly as it did on 0.5.0. 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 — an unparseable, scheme-less, fragment-bearing or *opaque* URI - raises, because the alternative is a 500 after the operator has already pasted - their token), `config.oauth_allow_loopback_redirects` (default `false`), + 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`), @@ -142,9 +146,25 @@ opt-in: a host that configures nothing behaves exactly as it did on 0.5.0. 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. -- If a host logs request parameters, add `code_verifier` to - `config.filter_parameters`; `access_token` is already covered by the stock - `token` entry Rails ships. +- 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 diff --git a/README.md b/README.md index f839c9c..3ffbc28 100644 --- a/README.md +++ b/README.md @@ -526,9 +526,16 @@ reaches the bridge — Rails does *not* do this for you: it populates `config.ho 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. If you log request parameters, add `code_verifier` to -`config.filter_parameters` (`access_token` is already covered by the stock `token` -entry). +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 diff --git a/lib/mcp_toolkit/configuration.rb b/lib/mcp_toolkit/configuration.rb index 825d758..16e14bb 100644 --- a/lib/mcp_toolkit/configuration.rb +++ b/lib/mcp_toolkit/configuration.rb @@ -160,29 +160,31 @@ class McpToolkit::Configuration # @return [Array] attr_reader :oauth_allowed_redirect_uris - # Assigns the allowlist, rejecting an entry the bridge could not actually + # 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 building the callback URL raise. A - # typo would cost the operator their paste and leave an orphaned code behind. - # - # Rejected: anything unparseable, anything with no scheme (a bare path is not a - # redirect target), a fragment (OAuth forbids one on a redirect_uri), and an - # OPAQUE URI — `com.example.app:cb` rather than `com.example.app:/cb`. The last - # is the sharp one, because the docs actively invite private-use schemes into - # this list and the opaque form is legal URI syntax; it simply cannot carry a - # query, so no code could ever reach it. + # 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) - @oauth_allowed_redirect_uris = Array(uris).map(&:to_s).each do |uri| + 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}, which the bridge could never redirect to: " \ - "#{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: @@ -252,11 +254,34 @@ def oauth_allowed_redirect_uris=(uris) # # 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] - attr_writer :oauth_signing_secret + # + # 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. + 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 @@ -830,17 +855,43 @@ def oauth_bridge? Array(oauth_allowed_redirect_uris).any? || !!oauth_allow_loopback_redirects end - # Why a given allowlist entry could never receive a code, or nil if it could. + # 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? - return "it names no scheme" if parsed.scheme.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? - return nil if parsed.opaque.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 opaque (#{parsed.scheme}:#{parsed.opaque}) so it cannot carry the code — " \ - "write it with a path, e.g. #{parsed.scheme}:/#{parsed.opaque}" + "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) 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/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 index 59cccc3..22f44e0 100644 --- a/lib/mcp_toolkit/oauth/controller_methods.rb +++ b/lib/mcp_toolkit/oauth/controller_methods.rb @@ -29,16 +29,14 @@ module McpToolkit::Oauth::ControllerMethods CODE_CACHE_PREFIX = "mcp_toolkit:oauth:code:" CODE_BYTES = 32 - # RFC 8252 §7.3 loopback hosts. The RFC prefers the IP literals over the name - # (a name is only as trustworthy as the resolver), but real clients use all - # three, and each resolves on the operator's own machine — which is the whole - # reason these need no allowlist entry. - LOOPBACK_HOSTS = ["127.0.0.1", "::1", "localhost"].freeze - # 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 @@ -54,7 +52,18 @@ module McpToolkit::Oauth::ControllerMethods # `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. - before_action :mcp_oauth_validate_request!, only: %i[authorize approve] if respond_to?(:before_action) + # 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 @@ -95,8 +104,11 @@ def register }, 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 + render :authorize, layout: false, formats: [:html] end # The token is verified here, not only at exchange, so a typo fails on the page @@ -116,6 +128,11 @@ def approve 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? @@ -202,11 +219,21 @@ def mcp_oauth_parse_uri(value) # `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) - LOOPBACK_HOSTS.include?(host.to_s.downcase.delete_prefix("[").delete_suffix("]")) + 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].present? && params[:code_challenge_method].to_s == "S256" + 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 -------------------------------------------------- @@ -263,11 +290,15 @@ def mcp_oauth_code_key(code) # 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. - # The serializer especially — every default in that 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. The payload is already - # a JSON String, so NullSerializer leaves JSON.parse the only parser that sees it. + # 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( @@ -332,9 +363,10 @@ def mcp_oauth_endpoint_url(action) # Dropping any inbound `code`/`state` keeps the response OAuth-shaped whatever # was passed in. # - # Built by string, not by `URI#query=`: this value has already been checked by - # the redirect policy, so re-parsing it to reconstruct it only invents ways for - # the emitted URL to differ from the one that was approved. + # 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("?") @@ -345,8 +377,9 @@ def mcp_oauth_callback_url(code) end # A client's own query survives; the two parameters this response owns do not, - # whoever put them there. Malformed escapes are dropped rather than raised on — - # the alternative is a 500 after the operator has already pasted their token. + # 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? @@ -357,9 +390,14 @@ def mcp_oauth_preserved_query_pairs(query) # ---- 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, status: :unprocessable_content + render :authorize, layout: false, formats: [:html], status: 422 end def mcp_oauth_render_bad_request(message) diff --git a/spec/mcp_toolkit/authority/controller_methods_spec.rb b/spec/mcp_toolkit/authority/controller_methods_spec.rb index 985f9f7..019f2c2 100644 --- a/spec/mcp_toolkit/authority/controller_methods_spec.rb +++ b/spec/mcp_toolkit/authority/controller_methods_spec.rb @@ -263,7 +263,7 @@ def rpc(method, params = {}, id: 1) # 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" + 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 diff --git a/spec/mcp_toolkit/engine_spec.rb b/spec/mcp_toolkit/engine_spec.rb index ff907e5..a1aa099 100644 --- a/spec/mcp_toolkit/engine_spec.rb +++ b/spec/mcp_toolkit/engine_spec.rb @@ -119,7 +119,7 @@ def drawn_formats 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" } + let(:oauth_signing_secret) { "spec-oauth-signing-secret-at-least-32-bytes-long" } before do McpToolkit.config.auth_role = auth_role @@ -135,6 +135,10 @@ def drawn_formats 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) diff --git a/spec/mcp_toolkit/oauth/bridge_end_to_end_spec.rb b/spec/mcp_toolkit/oauth/bridge_end_to_end_spec.rb index 40174e2..82307b9 100644 --- a/spec/mcp_toolkit/oauth/bridge_end_to_end_spec.rb +++ b/spec/mcp_toolkit/oauth/bridge_end_to_end_spec.rb @@ -168,7 +168,7 @@ def token_info = render(plain: "HOST_TOKEN_INFO") result["register"] = JSON.parse(session.last_response.body) # 5. The authorization page — does the view actually render? - verifier = "e2e-high-entropy-code-verifier-value" + 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, @@ -262,11 +262,23 @@ def token_info = render(plain: "HOST_TOKEN_INFO") result[key] = session.last_response.status end - # 11. A format suffix must not reach the action: there is no JSON template, so - # it would 500 unauthenticated. + # 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" @@ -373,6 +385,21 @@ def token_info = render(plain: "HOST_TOKEN_INFO") 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 diff --git a/spec/mcp_toolkit/oauth/controller_methods_spec.rb b/spec/mcp_toolkit/oauth/controller_methods_spec.rb index 7d3bdde..d78a3b4 100644 --- a/spec/mcp_toolkit/oauth/controller_methods_spec.rb +++ b/spec/mcp_toolkit/oauth/controller_methods_spec.rb @@ -69,8 +69,10 @@ def redirect_to(url, **options) 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. - let(:code_verifier) { "a-high-entropy-code-verifier-string" } + # 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 @@ -89,7 +91,7 @@ def redirect_to(url, **options) # 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" + c.oauth_signing_secret = "spec-oauth-signing-secret-at-least-32-bytes-long" end end @@ -381,6 +383,54 @@ def assign(uri) 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 @@ -438,12 +488,15 @@ def assign(uri) 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: :unprocessable_content) + expect(controller.rendered[:options]).to include(status: 422) end # The hidden fields are attacker-tamperable, so the allowlist is re-checked on @@ -491,18 +544,43 @@ def assign(uri) expect(controller.rendered[:options][:json]).to eq(error: "invalid_grant") end - it "rejects a code redeemed with the wrong PKCE verifier" do - controller.params = exchange_params.merge(code: issue_code, code_verifier: "wrong-verifier") + # 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_grant") + 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 From 4ed48fc2040cf7fa33192da3da9c41b0df6fda30 Mon Sep 17 00:00:00 2001 From: Karol Galanciak Date: Fri, 17 Jul 2026 15:18:00 +0200 Subject: [PATCH 11/11] Say why the signing-secret minimum spares the secret_key_base fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verified against a real app: a stock test env's secret_key_base is ~15 bytes, under the 32-byte minimum the setter enforces. The asymmetry is intended — the minimum catches a placeholder typed into THIS setting, whereas secret_key_base is Rails' own (128 chars in a real deployment) and a weak one is an app-wide problem, not this gem's to police. But an asymmetry nobody explains reads as an oversight, and this branch has spent four rounds on comments that claimed more than the code did. Says it. Co-Authored-By: Claude Opus 4.8 (1M context) --- lib/mcp_toolkit/configuration.rb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/lib/mcp_toolkit/configuration.rb b/lib/mcp_toolkit/configuration.rb index 16e14bb..a2bd5dc 100644 --- a/lib/mcp_toolkit/configuration.rb +++ b/lib/mcp_toolkit/configuration.rb @@ -281,6 +281,14 @@ def 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