diff --git a/CHANGELOG.md b/CHANGELOG.md index 5dc43a3..d841fb8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,43 @@ +## [0.6.2] - 2026-08-28 + +RFC 9207 authorization server issuer identification. Additive hardening, and the +fix for hosted clients that choose their redirect URI based on whether the +authorization server supports it. + +### Added + +- **`iss` in the authorization response, and `authorization_response_iss_parameter_supported` + in the metadata** (RFC 9207). The parameter names which authorization server + produced a response, so a client registered with several cannot be induced to + redeem a code at the wrong one — the mixed-up authorization server attack. + + This also unblocks **ChatGPT connectors**, which pick their `redirect_uri` from + whether the server meets RFC 9207: when it does, ChatGPT uses the stable + `https://chatgpt.com/connector_platform_oauth_redirect`; when it does not, it + mints a per-connector `https://chatgpt.com/connector/oauth/{callback_id}` that + no exact-match allowlist can express, and the connection fails with an + unregistered `redirect_uri`. Observed in production against a real customer + before this release. + + Note for hosts: a client that picks its callback at *connector-creation* time + reads the metadata then. An existing connector created against a pre-0.6.2 + server keeps the URI it already chose — **it has to be re-created** to pick up + the change. + + `iss` is emitted on the one response this bridge redirects (`approve`); every + error path renders rather than redirecting, so there is no error response for it + to be absent from. It is byte-identical to the advertised `issuer` — clients + compare by exact string and do not normalise trailing slashes, paths, ports or + casing — and a spec pins the redirect against the discovery document rather than + against a literal. + + `iss` is now response-owned alongside `code` and `state`: a caller that seeds one + into its own `redirect_uri` has it replaced, not appended. A value the caller + chose would defeat the point of the parameter. + + **No control was relaxed.** The redirect allowlist is unchanged and still exact-match + (RFC 9700 §2.1); this release makes conforming clients ask for a URI already on it. + ## [0.6.1] - 2026-07-20 Two additive fixes for hosted MCP clients whose OAuth setup could not complete diff --git a/README.md b/README.md index c22cff9..f6458b0 100644 --- a/README.md +++ b/README.md @@ -493,6 +493,25 @@ So **every target must be named by exact string**, with exactly one exception: | 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. | +### If a client's callback looks impossible to name, check RFC 9207 first + +Some hosted clients mint a **per-connector** callback (`https://vendor.example/connector/oauth/{id}`), +which no exact-match list can express — and the obvious response, matching the +host and path by pattern, is the wrong one. A prefix turns an attacker into +someone who can **name their own destination inside it**: they create their own +connector at that vendor, put their callback in the authorize URL, and the code +goes somewhere they control. Exact matching leaves a weaker residual (a code sent +to the vendor's *legitimate* shared callback, separated by the client binding +`state` to the initiating session — RFC 6819 §4.4.1.7); a prefix removes the need +for any of that to go wrong. + +At least one such client picks the per-connector form **only when the +authorization server does not implement RFC 9207**, and uses a single stable +callback when it does. Since 0.6.2 this gem implements it, so the fix for that +class of failure is to be conforming, not to be permissive. Before relaxing the +policy for a client that "cannot be named", check whether it is asking you for a +capability instead. + 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 diff --git a/lib/mcp_toolkit/oauth/controller_methods.rb b/lib/mcp_toolkit/oauth/controller_methods.rb index f057e6f..c323129 100644 --- a/lib/mcp_toolkit/oauth/controller_methods.rb +++ b/lib/mcp_toolkit/oauth/controller_methods.rb @@ -31,7 +31,7 @@ module McpToolkit::Oauth::ControllerMethods # 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 + RESPONSE_OWNED_QUERY_KEYS = %w[code state iss].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. @@ -95,7 +95,8 @@ def authorization_server response_types_supported: SUPPORTED_RESPONSE_TYPES, grant_types_supported: SUPPORTED_GRANT_TYPES, code_challenge_methods_supported: ["S256"], - token_endpoint_auth_methods_supported: ["none"] + token_endpoint_auth_methods_supported: ["none"], + authorization_response_iss_parameter_supported: true } end @@ -408,6 +409,7 @@ def mcp_oauth_callback_url(code) pairs = mcp_oauth_preserved_query_pairs(existing) pairs << ["code", code] pairs << ["state", params[:state].to_s] if params[:state].present? + pairs << ["iss", mcp_oauth_issuer] "#{base}?#{URI.encode_www_form(pairs)}" end diff --git a/lib/mcp_toolkit/version.rb b/lib/mcp_toolkit/version.rb index 4d8be86..bcf86f9 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.6.1" + VERSION = "0.6.2" 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 8865350..ee2889b 100644 --- a/spec/mcp_toolkit/oauth/bridge_end_to_end_spec.rb +++ b/spec/mcp_toolkit/oauth/bridge_end_to_end_spec.rb @@ -215,6 +215,7 @@ def token_info = render(plain: "HOST_TOKEN_INFO") result["approve_location_host"] = location && location.split("?").first code = location && Rack::Utils.parse_query(URI.parse(location).query)["code"] result["approve_state"] = location && Rack::Utils.parse_query(URI.parse(location).query)["state"] + result["approve_iss"] = location && Rack::Utils.parse_query(URI.parse(location).query)["iss"] # 6b. A bad paste must not issue a code. session.post("/mcp/oauth/authorize", authorize_query.merge(access_token: "wrong-token")) @@ -304,12 +305,13 @@ def token_info = render(plain: "HOST_TOKEN_INFO") # 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" + polluted = "http://127.0.0.1:54321/cb?code=ATTACKER&iss=https://attacker.example&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_iss_values"] = Array(polluted_query["iss"]) result["polluted_keeps_client_query"] = polluted_query["tenant"] puts JSON.generate(result) @@ -430,7 +432,7 @@ def token_info = render(plain: "HOST_TOKEN_INFO") # 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 + describe "a loopback redirect_uri carrying its own response parameters" do it "emits exactly one code, ours, and keeps the client's own query" do codes = @result.fetch("polluted_code_values") @@ -438,6 +440,13 @@ def token_info = render(plain: "HOST_TOKEN_INFO") expect(codes.first).not_to eq("ATTACKER") expect(@result.fetch("polluted_keeps_client_query")).to eq("acme") end + + it "emits exactly one iss, ours, over a caller-seeded one" do + values = @result.fetch("polluted_iss_values") + + expect(values.size).to eq(1) + expect(values.first).to eq("http://example.org/mcp") + end end # RFC 6749 §5.1 (both headers on a token response) and RFC 9700 §4.12 (303 after @@ -489,7 +498,8 @@ def token_info = render(plain: "HOST_TOKEN_INFO") "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"] + "code_challenge_methods_supported" => ["S256"], + "authorization_response_iss_parameter_supported" => true ) end @@ -509,7 +519,8 @@ def token_info = render(plain: "HOST_TOKEN_INFO") "issuer" => "http://example.org/mcp", "authorization_endpoint" => "http://example.org/mcp/oauth/authorize", "token_endpoint" => "http://example.org/mcp/oauth/token", - "registration_endpoint" => "http://example.org/mcp/oauth/register" + "registration_endpoint" => "http://example.org/mcp/oauth/register", + "authorization_response_iss_parameter_supported" => true ) expect(@result.fetch("appended_as_cache_control")).to eq("no-store") end @@ -593,6 +604,14 @@ def token_info = render(plain: "HOST_TOKEN_INFO") expect(@result.fetch("approve_bad_token_status")).to eq(422) expect(@result.fetch("approve_bad_token_redirected")).to be(false) end + + # Compared against the document, not a literal: both derive from + # `request.base_url`, so X-Forwarded-Host divergence has to fail here. + it "carries an iss that byte-matches the advertised issuer" do + expect(@result.fetch("approve_iss")).to eq(@result.fetch("as").fetch("issuer")) + expect(@result.fetch("approve_iss")).to eq(@result.fetch("appended_as").fetch("issuer")) + expect(@result.fetch("approve_iss")).to eq(@result.fetch("prm").fetch("authorization_servers").first) + end end describe "the exchange" do