Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Expand Down
4 changes: 3 additions & 1 deletion conformance/client.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
46 changes: 45 additions & 1 deletion lib/mcp/client/oauth/flow.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down
9 changes: 7 additions & 2 deletions lib/mcp/client/oauth/provider.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
105 changes: 105 additions & 0 deletions test/mcp/client/oauth/flow_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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."))

Expand Down