Merge pull request #566 from koic/override_query_parameters_of_the_authorization_endpoint

Replace authorization request parameters the endpoint URL already carries

Author: koic

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
 

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