diff --git a/README.md b/README.md index b986c262..0a05d0e3 100644 --- a/README.md +++ b/README.md @@ -2118,7 +2118,9 @@ Required keyword arguments to `Provider.new`: an explicit value always wins. - `redirect_uri`: String. Must use HTTPS or be a loopback URL (`localhost`, `127.0.0.0/8`, `::1`); other values raise `Provider::InsecureRedirectURIError`. - `redirect_handler`: Callable invoked with the fully-built authorization `URI`. Typically opens the user's browser. -- `callback_handler`: Callable that returns `[code, state]` after the user is redirected back to `redirect_uri`. +- `callback_handler`: Callable that returns `[code, state]` or `[code, state, iss]` after the user is redirected back to `redirect_uri`. Returning the 3-element form + (with `iss` set to the RFC 9207 `iss` parameter from the redirect, or `nil` when absent) opts into SEP-2468 issuer validation: a present `iss` must match + the authorization server's issuer, and a missing one is rejected when the server advertises `authorization_response_iss_parameter_supported`. Optional keyword arguments: diff --git a/conformance/client.rb b/conformance/client.rb index c9f3ec5f..73565e55 100644 --- a/conformance/client.rb +++ b/conformance/client.rb @@ -95,7 +95,9 @@ def build_oauth_provider(context, scenario:) callback_handler = -> do query = URI.decode_www_form(callback_holder.fetch(:url).query).to_h - [query["code"], query["state"]] + # The 3-element form opts into SEP-2468 / RFC 9207 `iss` validation; + # `query["iss"]` is nil when the authorization response carried no `iss`. + [query["code"], query["state"], query["iss"]] end MCP::Client::OAuth::Provider.new( diff --git a/lib/mcp/client/oauth/flow.rb b/lib/mcp/client/oauth/flow.rb index 7236f712..5a5c6001 100644 --- a/lib/mcp/client/oauth/flow.rb +++ b/lib/mcp/client/oauth/flow.rb @@ -74,13 +74,20 @@ def run!(server_url:, resource_metadata_url: nil, scope: nil) ) @provider.redirect_handler.call(authorization_url) - code, returned_state = Array(@provider.callback_handler.call) + callback_result = Array(@provider.callback_handler.call) + code, returned_state, returned_iss = callback_result raise AuthorizationError, "Authorization callback did not return an authorization code." unless code unless states_match?(returned_state, state) raise AuthorizationError, "OAuth state mismatch (CSRF protection)." end + validate_authorization_response_issuer!( + as_metadata: as_metadata, + iss: returned_iss, + iss_provided: callback_result.length >= 3, + ) + tokens = exchange_authorization_code( as_metadata: as_metadata, client_info: client_info, @@ -534,6 +541,43 @@ def safe_canonicalize_url(url, label:) raise AuthorizationError, "#{label} #{url.inspect} is not a valid URI: #{e.message}." end + # Validates the RFC 9207 `iss` authorization response parameter against the issuer of the authorization server + # the flow is talking to, per SEP-2468 (mix-up attack mitigation in multi-IdP setups): + # + # - When the callback supplied a non-empty `iss`, it MUST equal the AS metadata `issuer` exactly (simple string comparison, + # no normalization, per RFC 9207 Section 2.4); on mismatch the flow aborts before the authorization code is ever sent to + # a token endpoint. + # - When the callback returned a 3-element `[code, state, iss]` whose `iss` is nil (asserting it inspected the authorization response + # and found no `iss`) and the AS metadata advertises `authorization_response_iss_parameter_supported: true`, the missing parameter + # is treated as an attack per RFC 9207 Section 2.4 and the flow aborts. + # - A legacy 2-element `[code, state]` callback skips the check: the caller never looked for `iss`, so its absence carries no signal. + # + # `as_metadata["issuer"]` has already been byte-compared against the discovery URL by `ensure_issuer_matches!`, + # so it is the RFC 9207 anchor value. `iss` is not a secret; a plain `==` suffices. + # + # - https://github.com/modelcontextprotocol/modelcontextprotocol/pull/2468 + # - https://www.rfc-editor.org/rfc/rfc9207 + def validate_authorization_response_issuer!(as_metadata:, iss:, iss_provided:) + expected = as_metadata["issuer"] + + if iss && !iss.to_s.empty? + return if iss.to_s == expected.to_s + + raise AuthorizationError, + "Authorization response `iss` does not match the authorization server issuer " \ + "(expected #{expected.inspect}, got #{iss.inspect}) (RFC 9207); " \ + "refusing to exchange the authorization code." + end + + return unless iss_provided + return unless as_metadata["authorization_response_iss_parameter_supported"] == true + + raise AuthorizationError, + "Authorization server advertises `authorization_response_iss_parameter_supported` but " \ + "the authorization response carried no `iss` parameter (RFC 9207 Section 2.4); " \ + "refusing to exchange the authorization code." + end + # Constant-time comparison for the OAuth `state` parameter to prevent timing-based discovery # of the expected value. # `OpenSSL.fixed_length_secure_compare` would be ideal, but it is not available on Ruby 2.7 diff --git a/lib/mcp/client/oauth/provider.rb b/lib/mcp/client/oauth/provider.rb index 4864ae40..c8fe55cb 100644 --- a/lib/mcp/client/oauth/provider.rb +++ b/lib/mcp/client/oauth/provider.rb @@ -21,8 +21,13 @@ module OAuth # - `redirect_handler` - Callable invoked with the fully-built authorization # URL (a `URI`). Implementations typically open the user's browser. # - `callback_handler` - Callable invoked after `redirect_handler`. Returns - # `[code, state]` where `code` is the authorization code and `state` is - # the `state` parameter received on the redirect URI. + # `[code, state]` or `[code, state, iss]`, where `code` is the authorization code, + # `state` is the `state` parameter received on the redirect URI, and `iss` is + # the RFC 9207 `iss` parameter (or nil when the authorization response carried none). + # Returning the 3-element form opts into SEP-2468 issuer validation: a present `iss` + # must match the authorization server's issuer, and a nil `iss` is + # rejected when the AS advertises `authorization_response_iss_parameter_supported`. + # The 2-element form skips the check for backward compatibility. # # Optional keyword arguments: # - `scope` - String of space-separated scopes to request when the server's diff --git a/test/mcp/client/oauth/flow_test.rb b/test/mcp/client/oauth/flow_test.rb index b92b9f95..0891fd29 100644 --- a/test/mcp/client/oauth/flow_test.rb +++ b/test/mcp/client/oauth/flow_test.rb @@ -985,6 +985,111 @@ def test_run_raises_when_dcr_response_lacks_client_id assert_match(/client_id/i, error.message) end + # Builds a provider whose callback returns the given `iss` shape: + # `:omitted` keeps the legacy 2-element `[code, state]` form, any other value (including nil) returns + # the 3-element `[code, state, iss]` form that opts into SEP-2468 / RFC 9207 issuer validation. + def build_iss_validation_provider(callback_iss: :omitted) + state_holder = {} + callback_handler = if callback_iss == :omitted + -> { ["test-auth-code", state_holder[:state]] } + else + -> { ["test-auth-code", state_holder[:state], callback_iss] } + end + + 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: callback_handler, + ) + end + + # Re-stubs the AS metadata with `authorization_response_iss_parameter_supported: true`. + def stub_as_metadata_with_iss_support + 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", + token_endpoint: "#{@auth_base}/token", + registration_endpoint: "#{@auth_base}/register", + response_types_supported: ["code"], + grant_types_supported: ["authorization_code"], + code_challenge_methods_supported: ["S256"], + token_endpoint_auth_methods_supported: ["none"], + authorization_response_iss_parameter_supported: true, + ), + ) + end + + def test_run_completes_when_authorization_response_iss_matches_issuer + provider = build_iss_validation_provider(callback_iss: @auth_base) + + result = Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + + assert_equal(:authorized, result) + assert_equal("test-token-from-flow", provider.access_token) + end + + def test_run_aborts_before_code_exchange_when_authorization_response_iss_mismatches + provider = build_iss_validation_provider(callback_iss: "https://evil.example.com") + + error = assert_raises(Flow::AuthorizationError) do + Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + end + + # Per RFC 9207, the client MUST NOT send the authorization code to a token endpoint after an `iss` mismatch. + assert_match(/`iss` does not match/, error.message) + assert_not_requested(:post, "#{@auth_base}/token") + end + + def test_run_aborts_when_iss_is_missing_but_as_advertises_iss_support + stub_as_metadata_with_iss_support + provider = build_iss_validation_provider(callback_iss: nil) + + error = assert_raises(Flow::AuthorizationError) do + Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + end + + assert_match(/carried no `iss`/, error.message) + assert_not_requested(:post, "#{@auth_base}/token") + end + + def test_run_completes_with_legacy_two_element_callback_even_when_as_advertises_iss_support + stub_as_metadata_with_iss_support + provider = build_iss_validation_provider + + result = Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + + assert_equal(:authorized, result) + end + + def test_run_completes_when_iss_is_missing_and_as_does_not_advertise_iss_support + provider = build_iss_validation_provider(callback_iss: nil) + + result = Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + + assert_equal(:authorized, result) + end + + def test_run_validates_present_iss_even_when_as_does_not_advertise_iss_support + provider = build_iss_validation_provider(callback_iss: "https://evil.example.com") + + assert_raises(Flow::AuthorizationError) do + Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + end + + assert_not_requested(:post, "#{@auth_base}/token") + end + def test_run_skips_dcr_when_client_information_already_stored stub_request(:post, "#{@auth_base}/register").to_raise(StandardError.new("DCR should not be called."))