diff --git a/README.md b/README.md index b986c262..30a28f92 100644 --- a/README.md +++ b/README.md @@ -2124,7 +2124,9 @@ Optional keyword arguments: - `scope`: Space-separated scopes to request when the server's `WWW-Authenticate` does not specify one. - `storage`: Object responding to `tokens`, `save_tokens(t)`, `client_information`, `save_client_information(info)`. Defaults to `MCP::Client::OAuth::InMemoryStorage`, - which keeps credentials in process memory only. + which keeps credentials in process memory only. Persisted `client_information` is stamped with an `"issuer"` member binding it to the authorization server that + issued it (SEP-2352): when the server's authorization server changes, the SDK discards the stale registration and its tokens and re-registers automatically + (portable CIMD `client_id`s are kept). Treat the hash as opaque and persist it as-is. - `client_id_metadata_document_url`: URL where you publish a Client ID Metadata Document (`draft-ietf-oauth-client-id-metadata-document` and the MCP authorization specification). When the authorization server advertises `client_id_metadata_document_supported: true`, diff --git a/lib/mcp/client/oauth/flow.rb b/lib/mcp/client/oauth/flow.rb index 7236f712..6ee42b43 100644 --- a/lib/mcp/client/oauth/flow.rb +++ b/lib/mcp/client/oauth/flow.rb @@ -101,6 +101,7 @@ def run!(server_url:, resource_metadata_url: nil, scope: nil) # https://modelcontextprotocol.io/specification/2025-11-25/basic/authorization def run_client_credentials!(as_metadata:, prm:, resource:, scope:) client_info = client_credentials_client_info + ensure_client_credentials_issuer!(client_info, as_metadata: as_metadata) form = { "grant_type" => "client_credentials" } effective_scope = resolve_scope(scope: scope, prm: prm) @@ -127,6 +128,25 @@ def client_credentials_client_info info end + # Per SEP-2352, static machine-to-machine credentials are bound to their authorization + # server the same way registered ones are: when the stored `client_information` records + # an `issuer` that differs from the current authorization server, surface an error + # instead of silently sending another server's credentials (the spec's "SHOULD surface + # an error"; the TypeScript SDK raises `AuthorizationServerMismatchError` here). + # Re-registration is not an option for the `client_credentials` grant - the credentials + # are pre-registered, not DCR results - so the operator must update the stored credentials. + # Credentials without a recorded issuer keep working unchanged. + def ensure_client_credentials_issuer!(client_info, as_metadata:) + stored_issuer = client_info_required_value(client_info, "issuer") + return if stored_issuer.nil? + return if stored_issuer == as_metadata["issuer"] + + raise AuthorizationError, + "Stored client credentials are bound to a different authorization server " \ + "(stored issuer #{stored_issuer.inspect}, current #{as_metadata["issuer"].inspect}); " \ + "refusing to send them to the current authorization server (SEP-2352)." + end + # Exchanges the saved `refresh_token` for a fresh access token (RFC 6749 Section 6). # Re-discovers PRM and AS metadata so we always pick up a moved token endpoint, and re-runs the audience / issuer / security # checks before talking to it. @@ -166,6 +186,7 @@ def refresh!(server_url:, resource_metadata_url: nil) client_info = if have_stored_client_info # Pre-registered / DCR-issued `client_information` always wins: if the user picked an explicit identity, # do not silently swap it for the CIMD URL even when the AS also advertises CIMD support. + ensure_refreshable_client_information!(stored_client_info, as_metadata: as_metadata) stored_client_info elsif as_metadata["client_id_metadata_document_supported"] == true { "client_id" => @provider.client_id_metadata_document_url } @@ -412,8 +433,8 @@ def ensure_issuer_matches!(expected:, returned:) end def ensure_client_registered(as_metadata:) - existing = @provider.client_information - return existing if existing.is_a?(Hash) && client_info_required_value(existing, "client_id") + existing = stored_client_information_for(issuer: as_metadata["issuer"]) + return existing if existing # Per the MCP authorization specification and `draft-ietf-oauth-client-id-metadata-document`, # if the authorization server advertises Client ID Metadata Document support and the provider has @@ -462,7 +483,12 @@ def ensure_client_registered(as_metadata:) "Dynamic client registration response is missing `client_id`." end - @provider.save_client_information(info) + # Per SEP-2352, persisted client credentials are keyed by the issuer identifier of + # the authorization server that minted them, so a later flow can detect an AS change and + # re-register instead of replaying another server's credentials. `issuer` is not an RFC 7591 response field; + # the SDK adds it to the opaque persisted hash. + @provider.save_client_information(info.merge("issuer" => as_metadata["issuer"])) + info end @@ -480,6 +506,60 @@ def registration_client_metadata metadata.merge("application_type" => Discovery.infer_application_type(redirect_uris)) end + # Returns the stored `client_information` when it may be used against the authorization server + # identified by `issuer`, applying SEP-2352's authorization server binding rules: + # + # - Credentials persisted with an `"issuer"` binding MUST NOT be reused against + # a different authorization server. When the AS changed, the stale registration and its tokens + # are discarded (tokens minted by the old AS are dead at the new one) and nil is returned so + # the flow re-registers. + # - Stored credentials without an `"issuer"` binding (data persisted by an older SDK version, + # or user-supplied pre-registered credentials) are bound to the current issuer on first use. + # - A CIMD `client_id` (an HTTPS URL) is portable across authorization servers, so it is reused + # and re-bound instead of discarded. + # + # https://github.com/modelcontextprotocol/modelcontextprotocol/pull/2352 + def stored_client_information_for(issuer:) + existing = @provider.client_information + return unless existing.is_a?(Hash) && client_info_required_value(existing, "client_id") + + stored_issuer = client_info_required_value(existing, "issuer") + return existing if stored_issuer == issuer + return rebind_client_information(existing, issuer: issuer) if stored_issuer.nil? + + client_id = client_info_required_value(existing, "client_id") + return rebind_client_information(existing, issuer: issuer) if Discovery.client_id_metadata_document_url?(client_id) + + @provider.save_client_information(nil) + @provider.clear_tokens! + nil + end + + def rebind_client_information(info, issuer:) + rebound = info.merge("issuer" => issuer) + @provider.save_client_information(rebound) + rebound + end + + # Per SEP-2352, stored client credentials are bound to the authorization server that issued them; + # refuse to replay another AS's credentials at this token endpoint. `HTTP#attempt_refresh` rescues + # the error and falls back to the full flow, which discards the stale registration and re-registers. + # Credentials without an `"issuer"` binding predate this check and are allowed through; + # CIMD `client_id`s are portable. + def ensure_refreshable_client_information!(client_info, as_metadata:) + stored_issuer = client_info_required_value(client_info, "issuer") + return if stored_issuer.nil? + return if stored_issuer == as_metadata["issuer"] + + client_id = client_info_required_value(client_info, "client_id") + return if Discovery.client_id_metadata_document_url?(client_id) + + raise AuthorizationError, + "Cannot refresh: stored client credentials were issued by a different authorization server " \ + "(stored issuer #{stored_issuer.inspect}, current #{as_metadata["issuer"].inspect}); " \ + "re-registration is required (SEP-2352)." + end + # Reads `key` from a `client_information` hash that may use either string or # symbol keys, so users can persist the result of `JSON.parse` *or* a hand-built # `{ client_id:, client_secret: }` and have both work. diff --git a/lib/mcp/client/oauth/in_memory_storage.rb b/lib/mcp/client/oauth/in_memory_storage.rb index 83b261bb..23edad93 100644 --- a/lib/mcp/client/oauth/in_memory_storage.rb +++ b/lib/mcp/client/oauth/in_memory_storage.rb @@ -12,7 +12,9 @@ module OAuth # - `client_information`: the hash returned by Dynamic Client Registration # or supplied as pre-registered credentials # (`client_id`, optional `client_secret`, optional - # `token_endpoint_auth_method`). + # `token_endpoint_auth_method`). The SDK additionally stamps an `"issuer"` member + # binding the credentials to the authorization server that issued them (SEP-2352); + # custom storages should treat the hash as opaque and persist it as-is. # # This class keeps everything in process memory, so the credentials live # only for the lifetime of the Ruby process. Applications that need diff --git a/lib/mcp/client/oauth/provider.rb b/lib/mcp/client/oauth/provider.rb index 4864ae40..22581989 100644 --- a/lib/mcp/client/oauth/provider.rb +++ b/lib/mcp/client/oauth/provider.rb @@ -29,7 +29,10 @@ module OAuth # `WWW-Authenticate` does not specify one. # - `storage` - Object responding to `tokens`, `save_tokens(tokens)`, # `client_information`, and `save_client_information(info)`. Defaults to - # an `InMemoryStorage`. + # an `InMemoryStorage`. Persisted `client_information` is stamped with + # an `"issuer"` member binding it to the authorization server that + # issued it (SEP-2352); when the authorization server changes, the SDK discards + # the stale registration and tokens and re-registers. # - `client_id_metadata_document_url` - URL where the client publishes its Client ID Metadata Document # (`draft-ietf-oauth-client-id-metadata-document-00` and the MCP authorization specification). # When the authorization server advertises `client_id_metadata_document_supported: true`, diff --git a/test/mcp/client/oauth/flow_test.rb b/test/mcp/client/oauth/flow_test.rb index b92b9f95..d682cc5e 100644 --- a/test/mcp/client/oauth/flow_test.rb +++ b/test/mcp/client/oauth/flow_test.rb @@ -155,6 +155,33 @@ def test_run_client_credentials_raises_clean_error_when_client_information_missi assert_not_requested(:post, "#{@auth_base}/token") end + def test_run_client_credentials_succeeds_with_matching_stored_issuer + provider = client_credentials_provider + provider.storage.save_client_information( + provider.storage.client_information.merge("issuer" => @auth_base), + ) + + result = Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + + assert_equal(:authorized, result) + end + + def test_run_client_credentials_raises_for_mismatched_stored_issuer + # Per SEP-2352, static machine-to-machine credentials bound to another authorization server must surface + # an error instead of being replayed; re-registration is not an option for pre-registered credentials. + provider = client_credentials_provider + provider.storage.save_client_information( + provider.storage.client_information.merge("issuer" => "https://old-as.example.com"), + ) + + error = assert_raises(Flow::AuthorizationError) do + Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + end + + assert_match(/bound to a different authorization server/, error.message) + assert_not_requested(:post, "#{@auth_base}/token") + end + def test_run_client_credentials_requests_scope_from_prm_scopes_supported stub_request(:get, @prm_url).to_return( status: 200, @@ -1035,6 +1062,125 @@ def test_run_accepts_symbol_keys_in_preregistered_client_information assert_not_requested(:post, "#{@auth_base}/register") end + # Builds a provider that completes the non-interactive authorization + # flow against the stubs from `setup`, for the SEP-2352 issuer-binding + # tests. + def build_issuer_binding_provider + state_holder = {} + 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) { + state_holder[:state] = URI.decode_www_form(url.query).to_h.fetch("state") + }, + callback_handler: -> { ["test-auth-code", state_holder[:state]] }, + ) + end + + def test_run_skips_dcr_when_stored_issuer_matches + stub_request(:post, "#{@auth_base}/register").to_raise(StandardError.new("DCR should not be called.")) + + provider = build_issuer_binding_provider + provider.save_client_information("client_id" => "preregistered-client", "issuer" => @auth_base) + + Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + + assert_equal("test-token-from-flow", provider.access_token) + assert_not_requested(:post, "#{@auth_base}/register") + end + + def test_run_reregisters_when_authorization_server_changes + provider = build_issuer_binding_provider + provider.save_client_information("client_id" => "old-client", "issuer" => "https://other-as.example.com") + provider.save_tokens("access_token" => "old-as-token") + + Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + + assert_requested(:post, "#{@auth_base}/register") + info = provider.client_information + assert_equal("test-client", info["client_id"]) + assert_equal(@auth_base, info["issuer"]) + end + + def test_run_clears_stale_tokens_when_authorization_server_changes + # Abort the flow right after the discard (DCR fails) to observe that tokens minted by + # the old authorization server were wiped before any re-registration attempt. + stub_request(:post, "#{@auth_base}/register").to_return(status: 500, body: "") + + provider = build_issuer_binding_provider + provider.save_client_information("client_id" => "old-client", "issuer" => "https://other-as.example.com") + provider.save_tokens("access_token" => "old-as-token", "refresh_token" => "old-as-rt") + + assert_raises(Flow::AuthorizationError) do + Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + end + + assert_nil(provider.tokens) + assert_nil(provider.client_information) + end + + def test_run_binds_legacy_client_information_to_issuer_on_first_use + # Credentials persisted before issuer binding existed (or supplied as pre-registered credentials) + # are reused and stamped with the current issuer rather than discarded. + stub_request(:post, "#{@auth_base}/register").to_raise(StandardError.new("DCR should not be called.")) + + provider = build_issuer_binding_provider + provider.save_client_information("client_id" => "preregistered-client") + + Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + + assert_not_requested(:post, "#{@auth_base}/register") + assert_equal(@auth_base, provider.client_information["issuer"]) + end + + def test_run_binds_symbol_keyed_client_information_to_issuer_on_first_use + stub_request(:post, "#{@auth_base}/register").to_raise(StandardError.new("DCR should not be called.")) + + provider = build_issuer_binding_provider + provider.save_client_information(client_id: "preregistered-symbol") + + Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + + assert_not_requested(:post, "#{@auth_base}/register") + assert_equal("test-token-from-flow", provider.access_token) + assert_equal(@auth_base, provider.client_information["issuer"]) + end + + def test_run_reuses_portable_cimd_client_id_across_authorization_servers + # Per SEP-2352, a CIMD `client_id` (an HTTPS URL) is portable across authorization servers: + # reuse it and re-bind the issuer instead of discarding the registration. + stub_request(:post, "#{@auth_base}/register").to_raise(StandardError.new("DCR should not be called.")) + + provider = build_issuer_binding_provider + provider.save_client_information( + "client_id" => "https://app.example.com/client-metadata.json", + "issuer" => "https://other-as.example.com", + ) + + Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + + assert_not_requested(:post, "#{@auth_base}/register") + info = provider.client_information + assert_equal("https://app.example.com/client-metadata.json", info["client_id"]) + assert_equal(@auth_base, info["issuer"]) + end + + def test_run_persists_issuer_with_dcr_client_information + provider = build_issuer_binding_provider + + Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + + assert_requested(:post, "#{@auth_base}/register") + info = provider.client_information + assert_equal("test-client", info["client_id"]) + assert_equal(@auth_base, info["issuer"]) + end + def test_run_skips_dcr_when_as_supports_cimd_and_provider_has_cimd_url # When the AS advertises Client ID Metadata Document support and the provider was configured with a CIMD URL, # the SDK uses the URL as the OAuth `client_id` and does not call /register. @@ -1634,6 +1780,55 @@ def test_refresh_swaps_refresh_token_for_new_access_token assert_equal("saved-rt", provider.tokens["refresh_token"]) end + def test_refresh_succeeds_when_stored_issuer_matches + stub_request(:post, "#{@auth_base}/token").with( + body: hash_including("grant_type" => "refresh_token", "refresh_token" => "saved-rt"), + ).to_return( + status: 200, + headers: { "Content-Type" => "application/json" }, + body: JSON.generate(access_token: "fresh-at", token_type: "Bearer", expires_in: 3600), + ) + + provider = Provider.new( + client_metadata: { redirect_uris: ["http://localhost:0/callback"] }, + redirect_uri: "http://localhost:0/callback", + redirect_handler: ->(_url) {}, + callback_handler: -> { [nil, nil] }, + ) + provider.save_client_information("client_id" => "test-client", "issuer" => @auth_base) + provider.save_tokens("access_token" => "stale-at", "refresh_token" => "saved-rt") + + result = Flow.new(provider: provider).refresh!( + server_url: @server_url, + resource_metadata_url: @prm_url, + ) + + assert_equal(:refreshed, result) + assert_equal("fresh-at", provider.access_token) + end + + def test_refresh_raises_when_stored_issuer_differs + # Per SEP-2352, another authorization server's credentials must not be replayed at this token endpoint; + # the caller falls back to the full flow, which discards the stale registration and re-registers. + stub_request(:post, "#{@auth_base}/token").to_raise(StandardError.new("Token endpoint should not be called.")) + + provider = Provider.new( + client_metadata: { redirect_uris: ["http://localhost:0/callback"] }, + redirect_uri: "http://localhost:0/callback", + redirect_handler: ->(_url) {}, + callback_handler: -> { [nil, nil] }, + ) + provider.save_client_information("client_id" => "old-client", "issuer" => "https://other-as.example.com") + provider.save_tokens("access_token" => "stale-at", "refresh_token" => "saved-rt") + + error = assert_raises(Flow::AuthorizationError) do + Flow.new(provider: provider).refresh!(server_url: @server_url, resource_metadata_url: @prm_url) + end + + assert_match(/different authorization server/, error.message) + assert_not_requested(:post, "#{@auth_base}/token") + end + def test_refresh_uses_cimd_url_as_client_id_when_provider_has_cimd_and_as_supports_it # Regression: the CIMD branch in `ensure_client_registered` does not persist `client_information`, # so a refresh call that requires stored client info would always fail after a CIMD-only flow.