Surface unreachable Protected Resource Metadata instead of falling back to legacy discovery
## Motivation and Context `Flow#locate_authorization_server` treated every failure to fetch Protected Resource Metadata as "the server publishes none" and took the legacy 2025-03-26 path, where the MCP server's origin becomes the authorization base URL. A request that never reached the server, or a `5xx` or `429`, says nothing about what the server publishes, so a transient failure moved the flow to a different authorization server, discarded client information and tokens bound to the real one on the way, and surfaced as an unrelated error, such as a failed registration at the origin, rather than as the fetch failure itself. The comment claimed both reference SDKs behave that way, which is not so: outside browsers the TypeScript SDK propagates network errors from discovery, and the Python SDK propagates them and refuses the legacy path after a `5xx` or `429` from any candidate. Discovery now keeps trying candidate URLs until one serves a JSON object and classifies the outcome once they are exhausted. Like the Python SDK, it remembers a `5xx` or `429` from any candidate before deciding on the legacy path; moving on to the next candidate after a network error is what the loop already did. `MetadataNotPublishedError` (a `4xx` other than `429`, or a body that is not a JSON object) is the only failure that selects the legacy path; `MetadataUnreachableError` (a network error, or a `5xx` or `429`, from any candidate) is raised instead. Both subclass `AuthorizationError`, so callers rescuing that class keep working, and `MCP::Client::HTTP` still falls back from a failed refresh to the full flow, which repeats discovery and raises the same way unless the server has recovered. A body over the response cap is refused outright with a plain `AuthorizationError`, as it already was on the modern path. The message lists every candidate's failure, with the URL stripped of userinfo, query and fragment and cut to a fixed length, but otherwise spelled as requested, and with transport and parser messages bounded, as token endpoint diagnostics already are. The default legacy endpoints for an origin whose own metadata document is absent are unchanged. ## How Has This Been Tested? New tests in `test/mcp/client/oauth/flow_test.rb` and `test/mcp/client/oauth/discovery_test.rb`. Against the library before this change, discovery in the scenarios these tests set up took the legacy path instead of raising. ## Breaking Changes When no Protected Resource Metadata candidate serves a JSON object, a request that failed to reach the server, or returned a `5xx` or `429`, now raises instead of attempting the legacy 2025-03-26 discovery at the MCP server's origin, and a Protected Resource Metadata body over the response cap is refused instead of triggering that fallback.
This commit is contained in:
Koichi ITO
parent
2c33ad061a
commit
5081cc5626
2 changed files with 330 additions and 41 deletions
|
|
@ -259,6 +259,19 @@ module MCP
|
|||
)
|
||||
end
|
||||
|
||||
def test_redact_url_drops_credentials_query_and_fragment_but_keeps_the_spelling
|
||||
# Reported next to a failed request, the URL must still match the server's access log, so the host
|
||||
# and path are left alone: no lowercasing of the host, no dot-segment resolution, no decoding.
|
||||
assert_equal(
|
||||
"https://Srv.Example.COM:8443/tenant/%2e%2e/prm%2Ejson",
|
||||
Discovery.redact_url("https://user:pass@Srv.Example.COM:8443/tenant/%2e%2e/prm%2Ejson?token=abc#frag"),
|
||||
)
|
||||
end
|
||||
|
||||
def test_redact_url_does_not_echo_a_url_it_cannot_parse
|
||||
assert_equal("[unparseable URL]", Discovery.redact_url("https://user:pass@srv.example.com/pr m.json?token=abc"))
|
||||
end
|
||||
|
||||
def test_canonicalize_url_normalizes_query_to_match_faraday
|
||||
# Faraday rewrites `env.url` before sending a request: it sorts
|
||||
# parameters by name, uppercases percent-encoded hex, and drops
|
||||
|
|
|
|||
|
|
@ -1084,9 +1084,9 @@ module MCP
|
|||
end
|
||||
|
||||
def test_run_falls_back_to_legacy_discovery_when_prm_is_not_a_json_object
|
||||
# Valid JSON but the wrong shape. Any PRM discovery failure selects the legacy 2025-03-26 path
|
||||
# (matching the TypeScript and Python SDKs); here the legacy path also dead-ends, surfacing
|
||||
# a domain error rather than a raw `TypeError` from indexing the array.
|
||||
# Valid JSON but the wrong shape counts as nothing usable being published, so it selects the legacy 2025-03-26 path
|
||||
# (matching the TypeScript and Python SDKs); here the legacy path also dead-ends, surfacing a domain error rather than
|
||||
# a raw `TypeError` from indexing the array.
|
||||
stub_request(:get, @prm_url).to_return(
|
||||
status: 200,
|
||||
headers: { "Content-Type" => "application/json" },
|
||||
|
|
@ -1118,6 +1118,211 @@ module MCP
|
|||
assert_requested(:get, "https://srv.example.com/.well-known/oauth-authorization-server")
|
||||
end
|
||||
|
||||
def test_run_surfaces_a_network_error_during_prm_discovery_instead_of_falling_back
|
||||
# A request that never reached the server says nothing about whether it publishes PRM, so once
|
||||
# the other candidates answer 404 the flow stops instead of moving to the legacy authorization base;
|
||||
# the TypeScript and Python SDKs propagate the error the same way.
|
||||
challenge_url = "https://srv.example.com/prm.json"
|
||||
stub_request(:get, challenge_url).to_raise(Faraday::ConnectionFailed.new("connection refused"))
|
||||
stub_prm_not_found
|
||||
|
||||
error = assert_raises(Flow::MetadataUnreachableError) do
|
||||
Flow.new(provider: build_legacy_discovery_provider({})).run!(server_url: @server_url, resource_metadata_url: challenge_url)
|
||||
end
|
||||
|
||||
assert_match(/Faraday::ConnectionFailed/, error.message)
|
||||
assert_not_requested(:get, "https://srv.example.com/.well-known/oauth-authorization-server")
|
||||
assert_not_requested(:post, "https://srv.example.com/register")
|
||||
end
|
||||
|
||||
def test_run_reports_a_prm_candidate_url_without_its_query_in_the_discovery_error
|
||||
# The candidate URL comes from the server's `WWW-Authenticate` challenge and lands in every log line
|
||||
# the error reaches, so the query is dropped, while the path keeps the spelling that was requested
|
||||
# (`%2E` is not decoded) so the line can be matched against the server's access log.
|
||||
challenge_url = "https://srv.example.com/prm%2Ejson?token=abc"
|
||||
stub_request(:get, challenge_url).to_raise(Faraday::ConnectionFailed.new("connection refused"))
|
||||
stub_prm_not_found
|
||||
|
||||
error = assert_raises(Flow::MetadataUnreachableError) do
|
||||
Flow.new(provider: build_legacy_discovery_provider({})).run!(server_url: @server_url, resource_metadata_url: challenge_url)
|
||||
end
|
||||
|
||||
assert_includes(error.message, "GET https://srv.example.com/prm%2Ejson raised Faraday::ConnectionFailed: connection refused; GET")
|
||||
refute_includes(error.message, "token=abc")
|
||||
end
|
||||
|
||||
def test_run_cuts_an_overlong_prm_candidate_url_in_the_discovery_error
|
||||
# The challenge URL is the server's to choose, so its length is bounded before it lands in the message.
|
||||
challenge_url = "https://srv.example.com/#{"a" * 5000}/prm.json"
|
||||
stub_request(:get, challenge_url).to_raise(Faraday::ConnectionFailed.new("connection refused"))
|
||||
stub_prm_not_found
|
||||
|
||||
error = assert_raises(Flow::MetadataUnreachableError) do
|
||||
Flow.new(provider: build_legacy_discovery_provider({})).run!(server_url: @server_url, resource_metadata_url: challenge_url)
|
||||
end
|
||||
|
||||
assert_includes(error.message, "GET #{challenge_url[0, Flow::METADATA_URL_MAX_LENGTH - 3]}... raised Faraday::ConnectionFailed")
|
||||
refute_includes(error.message, "a" * (Flow::METADATA_URL_MAX_LENGTH - 3))
|
||||
end
|
||||
|
||||
def test_run_bounds_the_transport_error_message_in_the_discovery_error
|
||||
# A caller-supplied client can raise with server-chosen text in the message (Faraday's `raise_error` middleware repeats
|
||||
# the status line and the request URL), so it is bounded like the parser's message.
|
||||
stub_request(:get, @prm_url).to_raise(Faraday::ConnectionFailed.new("x" * 5000))
|
||||
stub_request(:get, "https://srv.example.com/.well-known/oauth-protected-resource").to_return(status: 404)
|
||||
|
||||
error = assert_raises(Flow::MetadataUnreachableError) do
|
||||
Flow.new(provider: build_legacy_discovery_provider({})).run!(server_url: @server_url, resource_metadata_url: @prm_url)
|
||||
end
|
||||
|
||||
assert_includes(error.message, "raised Faraday::ConnectionFailed: #{"x" * 125}...; GET")
|
||||
refute_includes(error.message, "x" * 126)
|
||||
end
|
||||
|
||||
def test_run_bounds_the_parser_message_in_the_discovery_error
|
||||
# json before 2.10 repeats the remaining source in `JSON::ParserError#message`, so a malformed body
|
||||
# could otherwise put up to the response cap into the message once per candidate.
|
||||
stub_request(:get, @prm_url).to_return(status: 200, headers: { "Content-Type" => "application/json" }, body: "{")
|
||||
stub_request(:get, "https://srv.example.com/.well-known/oauth-protected-resource").to_return(status: 503)
|
||||
JSON.stubs(:parse).raises(JSON::ParserError, "unexpected token at '#{"{" * 5000}'")
|
||||
|
||||
error = assert_raises(Flow::MetadataUnreachableError) do
|
||||
Flow.new(provider: build_legacy_discovery_provider({})).run!(server_url: @server_url, resource_metadata_url: @prm_url)
|
||||
end
|
||||
|
||||
# `METADATA_DIAGNOSTIC_MAX_LENGTH` is 128: the first 125 characters survive, then three dots.
|
||||
assert_includes(error.message, <<~MESSAGE.chomp)
|
||||
returned a body that is not JSON: unexpected token at '#{"{" * 104}...; \
|
||||
GET https://srv.example.com/.well-known/oauth-protected-resource returned 503.
|
||||
MESSAGE
|
||||
|
||||
refute_includes(error.message, "{" * 105)
|
||||
end
|
||||
|
||||
def test_run_surfaces_a_server_error_during_prm_discovery_instead_of_falling_back
|
||||
# A `5xx` or `429` says nothing about what the server publishes either, even when a later candidate answers 404;
|
||||
# the Python SDK refuses the legacy path the same way.
|
||||
[503, 429].each do |status|
|
||||
stub_request(:get, @prm_url).to_return(status: status)
|
||||
stub_request(:get, "https://srv.example.com/.well-known/oauth-protected-resource").to_return(status: 404)
|
||||
|
||||
error = assert_raises(Flow::MetadataUnreachableError, "status #{status}") do
|
||||
Flow.new(provider: build_legacy_discovery_provider({})).run!(server_url: @server_url, resource_metadata_url: @prm_url)
|
||||
end
|
||||
|
||||
assert_match(/returned #{status}/, error.message)
|
||||
end
|
||||
|
||||
assert_not_requested(:get, "https://srv.example.com/.well-known/oauth-authorization-server")
|
||||
assert_not_requested(:post, "https://srv.example.com/register")
|
||||
end
|
||||
|
||||
def test_run_keeps_a_server_error_in_mind_when_a_later_candidate_is_unusable
|
||||
# An unusable document from a later candidate must not turn an earlier `503` into "nothing published".
|
||||
challenge_url = "https://srv.example.com/prm.json"
|
||||
stub_request(:get, challenge_url).to_return(status: 503)
|
||||
stub_request(:get, @prm_url).to_return(status: 200, headers: { "Content-Type" => "application/json" }, body: "[]")
|
||||
stub_request(:get, "https://srv.example.com/.well-known/oauth-protected-resource").to_return(status: 404)
|
||||
|
||||
error = assert_raises(Flow::MetadataUnreachableError) do
|
||||
Flow.new(provider: build_legacy_discovery_provider({})).run!(server_url: @server_url, resource_metadata_url: challenge_url)
|
||||
end
|
||||
|
||||
assert_match(/returned 503/, error.message)
|
||||
assert_not_requested(:get, "https://srv.example.com/.well-known/oauth-authorization-server")
|
||||
end
|
||||
|
||||
def test_run_uses_a_later_prm_candidate_when_the_challenge_url_serves_an_unusable_document
|
||||
# The setup stubs a valid document at the well-known path; a broken one at the challenge URL is skipped.
|
||||
challenge_url = "https://srv.example.com/prm.json"
|
||||
stub_request(:get, challenge_url).to_return(status: 200, headers: { "Content-Type" => "application/json" }, body: "[]")
|
||||
|
||||
result = Flow.new(provider: build_legacy_discovery_provider({})).run!(server_url: @server_url, resource_metadata_url: challenge_url)
|
||||
|
||||
assert_equal(:authorized, result)
|
||||
assert_requested(:get, @prm_url)
|
||||
end
|
||||
|
||||
def test_run_recovers_authorization_server_metadata_from_a_later_candidate_after_a_network_error
|
||||
# The OAuth document being unreachable must not stop the OpenID one from being tried.
|
||||
stub_request(:get, @as_metadata_url).to_raise(Faraday::TimeoutError.new("execution expired"))
|
||||
stub_request(:get, "#{@auth_base}/.well-known/openid-configuration").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"],
|
||||
code_challenge_methods_supported: ["S256"],
|
||||
token_endpoint_auth_methods_supported: ["none"],
|
||||
),
|
||||
)
|
||||
|
||||
result = run_authorization_flow
|
||||
|
||||
assert_equal(:authorized, result)
|
||||
assert_requested(:get, "#{@auth_base}/.well-known/openid-configuration")
|
||||
end
|
||||
|
||||
def test_run_recovers_authorization_server_metadata_from_a_later_candidate_after_an_unusable_document
|
||||
# An OAuth document that is not JSON, or not a JSON object, must not stop the OpenID one from being tried.
|
||||
stub_request(:get, "#{@auth_base}/.well-known/openid-configuration").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"],
|
||||
code_challenge_methods_supported: ["S256"],
|
||||
token_endpoint_auth_methods_supported: ["none"],
|
||||
),
|
||||
)
|
||||
|
||||
["{", "[]"].each do |body|
|
||||
stub_request(:get, @as_metadata_url).to_return(status: 200, headers: { "Content-Type" => "application/json" }, body: body)
|
||||
|
||||
assert_equal(:authorized, run_authorization_flow, "body #{body.inspect}")
|
||||
end
|
||||
|
||||
assert_requested(:get, "#{@auth_base}/.well-known/openid-configuration", times: 2)
|
||||
end
|
||||
|
||||
def test_refresh_surfaces_a_network_error_during_prm_discovery_instead_of_falling_back
|
||||
stub_request(:get, @prm_url).to_raise(Faraday::TimeoutError.new("execution expired"))
|
||||
stub_request(:get, "https://srv.example.com/.well-known/oauth-protected-resource").to_return(status: 404)
|
||||
provider = build_legacy_discovery_provider({})
|
||||
provider.save_client_information("client_id" => "test-client")
|
||||
provider.save_tokens("access_token" => "stale-at", "refresh_token" => "saved-rt")
|
||||
|
||||
assert_raises(Flow::MetadataUnreachableError) do
|
||||
Flow.new(provider: provider).refresh!(server_url: @server_url, resource_metadata_url: @prm_url)
|
||||
end
|
||||
|
||||
assert_not_requested(:get, "https://srv.example.com/.well-known/oauth-authorization-server")
|
||||
assert_not_requested(:post, "https://srv.example.com/token")
|
||||
assert_equal("saved-rt", provider.tokens["refresh_token"])
|
||||
end
|
||||
|
||||
def test_refresh_surfaces_a_server_error_during_prm_discovery_instead_of_falling_back
|
||||
stub_request(:get, @prm_url).to_return(status: 503)
|
||||
stub_request(:get, "https://srv.example.com/.well-known/oauth-protected-resource").to_return(status: 404)
|
||||
provider = build_legacy_discovery_provider({})
|
||||
provider.save_client_information("client_id" => "test-client")
|
||||
provider.save_tokens("access_token" => "stale-at", "refresh_token" => "saved-rt")
|
||||
|
||||
error = assert_raises(Flow::MetadataUnreachableError) do
|
||||
Flow.new(provider: provider).refresh!(server_url: @server_url, resource_metadata_url: @prm_url)
|
||||
end
|
||||
|
||||
assert_match(/returned 503/, error.message)
|
||||
assert_not_requested(:post, "https://srv.example.com/token")
|
||||
assert_equal("saved-rt", provider.tokens["refresh_token"])
|
||||
end
|
||||
|
||||
# Builds a provider for the legacy-discovery tests, capturing the authorization URL so tests can assert
|
||||
# which endpoint was used.
|
||||
def build_legacy_discovery_provider(holder)
|
||||
|
|
@ -1511,10 +1716,10 @@ module MCP
|
|||
end
|
||||
|
||||
def test_run_refuses_a_protected_resource_metadata_body_over_the_cap
|
||||
# PRM discovery failures select the legacy path by design, so the refusal shows up as
|
||||
# the fallback rather than as a raise. The padded document is valid JSON naming
|
||||
# an authorization server, so contacting that server is exactly what would happen
|
||||
# if the body had been read: the assertion below fails if the cap stops working.
|
||||
# An oversized document is refused outright rather than treated as unpublished, so the flow stops
|
||||
# without the legacy fallback. The padded document is valid JSON naming an authorization server,
|
||||
# so contacting that server is exactly what would happen if the body had been read:
|
||||
# the assertion below fails if the cap stops working.
|
||||
stub_request(:any, %r{\Ahttps://srv\.example\.com/}).to_return(status: 404)
|
||||
stub_request(:get, @prm_url).to_return(
|
||||
status: 200,
|
||||
|
|
@ -1526,9 +1731,11 @@ module MCP
|
|||
),
|
||||
)
|
||||
|
||||
assert_raises(Flow::AuthorizationError) { run_authorization_flow }
|
||||
error = assert_raises(Flow::AuthorizationError) { run_authorization_flow }
|
||||
|
||||
assert_match(/exceeds \d+ bytes/, error.message)
|
||||
assert_not_requested(:get, @as_metadata_url)
|
||||
assert_not_requested(:get, "https://srv.example.com/.well-known/oauth-authorization-server")
|
||||
end
|
||||
|
||||
def test_run_refuses_a_dynamic_client_registration_body_over_the_cap
|
||||
|
|
@ -1632,6 +1839,7 @@ module MCP
|
|||
headers: { "Content-Type" => "application/json" },
|
||||
body: "null",
|
||||
)
|
||||
stub_request(:get, "#{@auth_base}/.well-known/openid-configuration").to_return(status: 404)
|
||||
|
||||
provider = Provider.new(
|
||||
client_metadata: {
|
||||
|
|
@ -3283,7 +3491,7 @@ module MCP
|
|||
end
|
||||
|
||||
def test_token_endpoint_errors_fall_back_when_diagnostic_extraction_raises
|
||||
Flow.any_instance.stubs(:token_endpoint_diagnostic).raises(ArgumentError, "sensitive provider text")
|
||||
Flow.any_instance.stubs(:bounded_diagnostic).raises(ArgumentError, "sensitive provider text")
|
||||
|
||||
{ "invalid_grant" => Flow::InvalidGrantError, "invalid_client" => Flow::AuthorizationError }.each do |code, klass|
|
||||
error = refresh_token_endpoint_error(JSON.generate(error: code, error_description: "details"))
|
||||
|
|
|
|||
Loading…
Reference in a new issue