Merge pull request #556 from koic/validate_the_issuer_of_legacy_authorization_server_metadata

Validate the issuer of legacy authorization server metadata

docs/_client/authorization.md

@@ -37,7 +37,7 @@ pass an `MCP::Client::OAuth::Provider` to the transport instead of a static `Aut
 - On a `401 Unauthorized`, parse the `WWW-Authenticate` header, discover the authorization server (Protected Resource Metadata + RFC 8414 Authorization Server Metadata),
   perform Dynamic Client Registration if needed, run the OAuth 2.1 Authorization Code flow with PKCE (S256), and retry the failed request with the acquired token.
 - Fall back to the legacy 2025-03-26 discovery when the server publishes no Protected Resource Metadata, matching the TypeScript and Python SDKs: the MCP server's origin acts
-  as the authorization base URL, its metadata is fetched from `<origin>/.well-known/oauth-authorization-server` without the RFC 8414 issuer byte-match (which the legacy spec predates),
+  as the authorization base URL, its metadata is fetched from `<origin>/.well-known/oauth-authorization-server` and must name that origin as its `issuer` (RFC 8414 Section 3.3),
   and when even that is absent the spec's default endpoints `/authorize`, `/token`, and `/register` at the origin are used with PKCE S256 assumed.
 - On subsequent 401s with a saved `refresh_token`, exchange it at the token endpoint before falling back to the full interactive flow (RFC 6749 Section 6).
 - On a `403 Forbidden` whose `WWW-Authenticate` header carries `error="insufficient_scope"` (OAuth 2.0 step-up, RFC 6750 Section 3.1 and the MCP scope-selection-strategy),

lib/mcp/client/oauth/flow.rb

@@ -405,16 +405,26 @@ module MCP
 
         # Fetches and validates the authorization server's RFC 8414 metadata.
         #
-        # On the modern path the metadata `issuer` must be byte-identical to the discovery URL (RFC 8414 Section 3.3).
+        # The metadata `issuer` must be byte-identical to the discovery URL (RFC 8414 Section 3.3) on both paths.
-        # On the legacy 2025-03-26 path that validation is skipped: the legacy spec predates the requirement,
+        # On the legacy 2025-03-26 path the discovery URL is the MCP server's origin, which that spec names as
-        # and a pre-PRM server may host its OAuth endpoints under a path prefix whose `issuer` legitimately differs from
+        # the authorization base URL and which a document may render with a trailing slash; the TypeScript and Python SDKs
-        # the origin the metadata was discovered at (neither the TypeScript nor the Python SDK validates the issuer on this path).
+        # accept the same slash-only difference. A document naming any other issuer is refused: an unverified `issuer`
+        # would otherwise become the identity tokens and client information are bound to, assertions are minted for,
+        # and the validator is shown, so a server could claim another authorization server and unlock the credentials
+        # bound to it.
         # When even the metadata document is absent, the legacy spec's default endpoints are used.
         def authorization_server_metadata(authorization_server:, legacy:, server_url:)
           metadata = if legacy
-            begin
+            fetched = begin
               fetch_authorization_server_metadata(issuer_url: authorization_server)
             rescue AuthorizationError
+              nil
+            end
+
+            if fetched
+              ensure_legacy_issuer_matches!(expected: authorization_server, returned: fetched["issuer"])
+              fetched
+            else
               default_legacy_metadata(authorization_server)
             end
           else
@@ -459,6 +469,14 @@ module MCP
           fetch_metadata_json(urls, label: "authorization server metadata")
         end
 
+        # The legacy authorization base is an origin, which a document may render as `https://host/`;
+        # both name the same server, and nothing else does.
+        def ensure_legacy_issuer_matches!(expected:, returned:)
+          return if returned == "#{expected}/"
+
+          ensure_issuer_matches!(expected: expected, returned: returned)
+        end
+
         # Reads `authorization_servers` from a PRM document and returns
         # the first entry, raising `AuthorizationError` for any of the malformed
         # shapes a non-compliant server could emit (missing field, non-Array

test/mcp/client/oauth/flow_test.rb

@@ -1142,22 +1142,22 @@ module MCP
           stub_request(:get, "https://srv.example.com/.well-known/oauth-protected-resource").to_return(status: 404)
         end
 
-        def test_run_falls_back_to_server_origin_metadata_without_prm
+        # Legacy 2025-03-26 shape: no PRM, AS metadata served from the MCP server origin with the given `issuer`
-          # Legacy 2025-03-26 shape: no PRM, AS metadata served from the MCP server origin,
+        # and the OAuth endpoints under a path prefix.
-          # OAuth endpoints under a path prefix whose `issuer` differs from the discovery origin.
+        def stub_legacy_metadata_with_prefixed_endpoints(issuer:, iss_supported: false)
-          # The legacy path must not apply the RFC 8414 issuer byte-match (the legacy spec predates it).
           stub_prm_not_found
           stub_request(:get, "https://srv.example.com/.well-known/oauth-authorization-server").to_return(
             status: 200,
             headers: { "Content-Type" => "application/json" },
             body: JSON.generate(
-              issuer: "https://srv.example.com/oauth",
+              issuer: issuer,
               authorization_endpoint: "https://srv.example.com/oauth/authorize",
               token_endpoint: "https://srv.example.com/oauth/token",
               registration_endpoint: "https://srv.example.com/oauth/register",
               response_types_supported: ["code"],
               code_challenge_methods_supported: ["S256"],
               token_endpoint_auth_methods_supported: ["none"],
+              authorization_response_iss_parameter_supported: iss_supported,
             ),
           )
           stub_request(:post, "https://srv.example.com/oauth/register").to_return(
@@ -1170,6 +1170,11 @@ module MCP
             headers: { "Content-Type" => "application/json" },
             body: JSON.generate(access_token: "legacy-token", token_type: "Bearer", expires_in: 3600),
           )
+        end
+
+        def test_run_falls_back_to_server_origin_metadata_without_prm
+          # The document names the origin, so the RFC 8414 Section 3.3 check passes and its prefixed endpoints are used.
+          stub_legacy_metadata_with_prefixed_endpoints(issuer: "https://srv.example.com")
 
           holder = {}
           provider = build_legacy_discovery_provider(holder)
@@ -1178,11 +1183,54 @@ module MCP
 
           assert_equal(:authorized, result)
           assert_equal("legacy-token", provider.access_token)
+          assert_equal("https://srv.example.com", provider.tokens["issuer"])
           assert_equal("/oauth/authorize", holder[:authorization_url].path)
           assert_requested(:post, "https://srv.example.com/oauth/register")
           assert_requested(:post, "https://srv.example.com/oauth/token")
         end
 
+        def test_run_accepts_legacy_metadata_naming_the_origin_with_a_trailing_slash
+          # A root issuer rendered as `https://host/` names the same server; the TypeScript and Python SDKs accept it too,
+          # and the document's spelling is what the RFC 9207 `iss` and the recorded issuer carry.
+          stub_legacy_metadata_with_prefixed_endpoints(issuer: "https://srv.example.com/", iss_supported: true)
+
+          holder = {}
+          provider = Provider.new(
+            client_metadata: {
+              redirect_uris: ["http://localhost:0/callback"],
+              grant_types: ["authorization_code"],
+              response_types: ["code"],
+              token_endpoint_auth_method: "none",
+            },
+            redirect_uri: "http://localhost:0/callback",
+            redirect_handler: ->(url) { holder[:state] = URI.decode_www_form(url.query).to_h.fetch("state") },
+            callback_handler: -> { ["test-auth-code", holder[:state], "https://srv.example.com/"] },
+          )
+
+          result = Flow.new(provider: provider).run!(server_url: @server_url)
+
+          assert_equal(:authorized, result)
+          assert_equal("https://srv.example.com/", provider.tokens["issuer"])
+        end
+
+        def test_run_refuses_legacy_metadata_whose_issuer_is_not_the_origin
+          # RFC 8414 Section 3.3 applies on the legacy path as well: the 2025-03-26 spec places the metadata
+          # at the origin, so an issuer under a path prefix is a mismatch, as it is for the TypeScript and Python SDKs.
+          stub_legacy_metadata_with_prefixed_endpoints(issuer: "https://srv.example.com/oauth")
+
+          holder = {}
+          provider = build_legacy_discovery_provider(holder)
+
+          error = assert_raises(Flow::AuthorizationError) do
+            Flow.new(provider: provider).run!(server_url: @server_url)
+          end
+
+          assert_match(/`issuer` does not match/, error.message)
+          assert_nil(holder[:authorization_url])
+          assert_not_requested(:post, "https://srv.example.com/oauth/register")
+          assert_not_requested(:post, "https://srv.example.com/oauth/token")
+        end
+
         def test_run_falls_back_to_default_endpoints_without_any_metadata
           # Legacy 2025-03-26 "Fallbacks for Servers without Metadata Discovery": with no PRM and no AS metadata,
           # the client MUST use /authorize, /token, and /register at the authorization base URL, still sending PKCE S256.
@@ -1257,6 +1305,98 @@ module MCP
           assert_match(/`issuer` does not match/, error.message)
         end
 
+        # Legacy metadata served at the MCP server origin that claims the identity of another authorization server,
+        # with every endpoint at the origin itself. The claim fails the RFC 8414 check, so none of these endpoints is reached.
+        def stub_legacy_metadata_claiming(issuer)
+          stub_prm_not_found
+          stub_request(:get, "https://srv.example.com/.well-known/oauth-authorization-server").to_return(
+            status: 200,
+            headers: { "Content-Type" => "application/json" },
+            body: JSON.generate(
+              issuer: issuer,
+              authorization_endpoint: "https://srv.example.com/authorize",
+              token_endpoint: "https://srv.example.com/token",
+              registration_endpoint: "https://srv.example.com/register",
+              code_challenge_methods_supported: ["S256"],
+            ),
+          )
+          stub_request(:post, "https://srv.example.com/register").to_return(
+            status: 201,
+            headers: { "Content-Type" => "application/json" },
+            body: JSON.generate(client_id: "legacy-client"),
+          )
+          stub_request(:post, "https://srv.example.com/token").to_return(
+            status: 200,
+            headers: { "Content-Type" => "application/json" },
+            body: JSON.generate(access_token: "legacy-token", token_type: "Bearer", expires_in: 3600),
+          )
+        end
+
+        def test_run_refuses_legacy_metadata_claiming_another_authorization_server_before_the_validator
+          stub_legacy_metadata_claiming(@auth_base)
+          recorder = []
+          provider = Provider.new(
+            client_metadata: {
+              redirect_uris: ["http://localhost:0/callback"],
+              grant_types: ["authorization_code"],
+              response_types: ["code"],
+              token_endpoint_auth_method: "none",
+            },
+            redirect_uri: "http://localhost:0/callback",
+            redirect_handler: ->(_url) { recorder << :redirected },
+            callback_handler: -> { ["test-auth-code", "state"] },
+            authorization_request_validator: ->(request) {
+              recorder << request
+              true
+            },
+          )
+
+          error = assert_raises(Flow::AuthorizationError) do
+            Flow.new(provider: provider).run!(server_url: @server_url)
+          end
+
+          assert_match(/`issuer` does not match/, error.message)
+          assert_empty(recorder)
+          assert_not_requested(:post, "https://srv.example.com/register")
+          assert_not_requested(:post, "https://srv.example.com/token")
+        end
+
+        def test_refresh_refuses_legacy_metadata_claiming_the_issuer_that_minted_the_tokens
+          # The tokens came from https://auth.example.com; a PRM-less server claiming that issuer at its own origin
+          # must not receive them, so the refresh token stays for the server that issued it.
+          stub_legacy_metadata_claiming(@auth_base)
+          provider = build_legacy_discovery_provider({})
+          provider.save_client_information("client_id" => "conf-client", "client_secret" => "conf-secret")
+          provider.save_tokens("access_token" => "stale-at", "refresh_token" => "saved-rt", "issuer" => @auth_base)
+
+          error = assert_raises(Flow::AuthorizationError) do
+            Flow.new(provider: provider).refresh!(server_url: @server_url)
+          end
+
+          assert_match(/`issuer` does not match/, error.message)
+          assert_not_requested(:post, "https://srv.example.com/token")
+          assert_equal("saved-rt", provider.tokens["refresh_token"])
+        end
+
+        def test_run_does_not_present_client_information_bound_elsewhere_when_legacy_metadata_claims_that_issuer
+          # Client information issued by https://auth.example.com is bound to it (SEP-2352); a PRM-less server claiming
+          # that issuer is refused before the credentials could be presented anywhere.
+          stub_legacy_metadata_claiming(@auth_base)
+          holder = {}
+          provider = build_legacy_discovery_provider(holder)
+          provider.save_client_information("client_id" => "conf-client", "client_secret" => "conf-secret", "issuer" => @auth_base)
+
+          assert_raises(Flow::AuthorizationError) do
+            Flow.new(provider: provider).run!(server_url: @server_url)
+          end
+
+          assert_nil(holder[:authorization_url])
+          assert_not_requested(:post, "https://srv.example.com/register")
+          assert_not_requested(:post, "https://srv.example.com/token")
+          assert_equal("conf-secret", provider.client_information["client_secret"])
+          assert_equal(@auth_base, provider.client_information["issuer"])
+        end
+
         def test_run_raises_when_prm_authorization_servers_is_not_an_array
           # `authorization_servers` MUST be an Array per RFC 9728. A misbehaving
           # PRM that returns a String would otherwise reach `.first` and raise