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"] },