From dc8efd971568bb3e54d3eaf19e409917ccf6e4ff Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Tue, 22 Sep 2026 01:21:43 +0900 Subject: [PATCH] Replace authorization request parameters the endpoint URL already carries ## Motivation and Context `Flow#build_authorization_url` appended the flow's parameters after whatever query the authorization server metadata's `authorization_endpoint` already carried. An endpoint URL carrying `client_id`, `redirect_uri`, `state`, `code_challenge`, or `resource` of its own therefore produced a request with each of them twice, the URL's value first. RFC 6749 Section 3.1 forbids sending a parameter more than once, and which value a server honors is its own choice; on the legacy 2025-03-26 path the endpoint URL is served by the MCP server itself, so its query could speak for the client's identity, redirect URI, PKCE challenge, and resource at the authorization server. A parameter the flow sets now replaces one of the same name in the endpoint URL, and the rest of the URL's query is kept, which is what the TypeScript SDK's `searchParams.set` does. `request` and `request_uri` are dropped from the endpoint URL's query as well, although the flow sets neither. RFC 9101 has an authorization server take the whole authorization request from the object they carry, over every parameter in the query, and both are the client's to send, so an endpoint URL holding one of them could speak for the client through that door too. A `scope` in the endpoint URL stays when the flow has none of its own, as it does in the TypeScript SDK: an authorization server may place a default scope on its own endpoint URL. ## How Has This Been Tested? A new test in `test/mcp/client/oauth/flow_test.rb` serves an `authorization_endpoint` whose query carries those parameters and one more, and checks that the authorization URL holds each parameter once with the flow's value while keeping the extra one. It fails against the previous library. Two more tests in the same file check that `request` and `request_uri` in the endpoint URL's query are dropped, and that a `scope` there stays when the flow has none; the first of them fails against the previous library. ## Breaking Changes None. --- lib/mcp/client/oauth/flow.rb | 34 +++++++++---- test/mcp/client/oauth/flow_test.rb | 78 ++++++++++++++++++++++++++++++ 2 files changed, 102 insertions(+), 10 deletions(-) diff --git a/lib/mcp/client/oauth/flow.rb b/lib/mcp/client/oauth/flow.rb index 58c234dc..07164df5 100644 --- a/lib/mcp/client/oauth/flow.rb +++ b/lib/mcp/client/oauth/flow.rb @@ -1206,16 +1206,30 @@ def build_authorization_url(as_metadata:, client_id:, scope:, state:, code_chall "Authorization server metadata `authorization_endpoint` is not a valid URI: #{e.message}." end - params = URI.decode_www_form(uri.query.to_s) - params << ["response_type", "code"] - params << ["client_id", client_id] - params << ["redirect_uri", @provider.redirect_uri] - params << ["code_challenge", code_challenge] - params << ["code_challenge_method", "S256"] - params << ["state", state] - params << ["scope", scope] if scope - params << ["resource", resource] if resource - uri.query = URI.encode_www_form(params) + # A parameter the flow sets replaces any of the same name the endpoint URL already carries. + # RFC 6749 Section 3.1 forbids sending a parameter twice, and which of two values a server would honor is + # its own choice; on the legacy path the endpoint URL is served by the MCP server, whose query must not speak + # for the client's `client_id`, `redirect_uri`, `code_challenge`, or `resource`. + # Other parameters in the URL are kept, as the TypeScript SDK's `searchParams.set` keeps them; that includes + # a `scope` when the flow has none, since an authorization server may set a default scope there. + # RFC 9101 `request` and `request_uri` are dropped as well, though the flow sets neither: a server takes + # the whole authorization request from the object they carry, over every parameter in the query, and both are + # the client's to send, never an endpoint URL's to supply. + own_params = [ + ["response_type", "code"], + ["client_id", client_id], + ["redirect_uri", @provider.redirect_uri], + ["code_challenge", code_challenge], + ["code_challenge_method", "S256"], + ["state", state], + ] + own_params << ["scope", scope] if scope + own_params << ["resource", resource] if resource + dropped_names = own_params.map(&:first) + ["request", "request_uri"] + + params = URI.decode_www_form(uri.query.to_s).reject { |name, _value| dropped_names.include?(name) } + uri.query = URI.encode_www_form(params + own_params) + uri end diff --git a/test/mcp/client/oauth/flow_test.rb b/test/mcp/client/oauth/flow_test.rb index c13b49b1..15cf8dc2 100644 --- a/test/mcp/client/oauth/flow_test.rb +++ b/test/mcp/client/oauth/flow_test.rb @@ -1572,6 +1572,52 @@ def test_run_raises_when_authorization_endpoint_is_malformed_uri assert_match(/authorization_endpoint/i, error.message) end + def test_run_replaces_authorization_request_parameters_the_endpoint_url_already_carries + # An `authorization_endpoint` may carry a query of its own. A parameter of the same name as one the flow sets is + # replaced rather than sent twice, so the URL cannot speak for the client's identity, redirect URI, or PKCE challenge; + # the rest of the query is kept. + query = authorization_url_query_for_endpoint_query( + "client_id=other&redirect_uri=https%3A%2F%2Fother.example.com%2Fcb&state=fixed&code_challenge=theirs&audience=api", + ) + + assert_equal( + ["audience", "response_type", "client_id", "redirect_uri", "code_challenge", "code_challenge_method", "state", "resource"], + query.map(&:first), + ) + assert_equal("api", query.to_h["audience"]) + assert_equal("test-client", query.to_h["client_id"]) + assert_equal("http://localhost:0/callback", query.to_h["redirect_uri"]) + refute_equal("theirs", query.to_h["code_challenge"]) + refute_equal("fixed", query.to_h["state"]) + end + + def test_run_drops_request_object_parameters_the_endpoint_url_carries + # RFC 9101 has an authorization server take the whole authorization request from `request` or `request_uri`, + # over every parameter in the query, so neither may come from the endpoint URL even though the flow sets no + # parameter of either name. + query = authorization_url_query_for_endpoint_query( + "request_uri=https%3A%2F%2Fother.example.com%2Frequest&request=opaque&audience=api", + ) + + assert_equal( + ["audience", "response_type", "client_id", "redirect_uri", "code_challenge", "code_challenge_method", "state", "resource"], + query.map(&:first), + ) + end + + def test_run_keeps_an_endpoint_scope_when_the_flow_has_none + # An authorization server may place a default `scope` on its own endpoint URL. With no scope of its own + # (none requested, none in the resource metadata, none on the provider) the flow leaves it there, + # as the TypeScript SDK does. + query = authorization_url_query_for_endpoint_query("scope=openid") + + assert_equal( + ["scope", "response_type", "client_id", "redirect_uri", "code_challenge", "code_challenge_method", "state", "resource"], + query.map(&:first), + ) + assert_equal("openid", query.to_h["scope"]) + end + def test_run_raises_when_prm_resource_is_malformed_uri stub_request(:get, @prm_url).to_return( status: 200, @@ -4436,6 +4482,38 @@ def test_run_sends_server_url_as_resource_when_prm_omits_it private + # Serves authorization server metadata whose `authorization_endpoint` carries `endpoint_query`, + # runs the authorization-code flow to completion, and returns the query of the URL the browser was + # sent to as name/value pairs in order. + def authorization_url_query_for_endpoint_query(endpoint_query) + stub_request(:get, @as_metadata_url).to_return( + status: 200, + headers: { "Content-Type" => "application/json" }, + body: JSON.generate( + issuer: @auth_base, + authorization_endpoint: "#{@auth_base}/authorize?#{endpoint_query}", + token_endpoint: "#{@auth_base}/token", + registration_endpoint: "#{@auth_base}/register", + response_types_supported: ["code"], + code_challenge_methods_supported: ["S256"], + token_endpoint_auth_methods_supported: ["none"], + ), + ) + holder = {} + provider = Provider.new( + **authorization_code_provider_arguments( + ->(url) { holder[:authorization_url] = url }, + -> { ["test-auth-code", URI.decode_www_form(holder[:authorization_url].query).to_h.fetch("state")] }, + ), + ) + + result = Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + + assert_equal(:authorized, result) + + URI.decode_www_form(holder[:authorization_url].query) + end + def refresh_only_provider provider = Provider.new( client_metadata: { redirect_uris: ["http://localhost:0/callback"] },