Merge commit from fork
Restrict OAuth Discovery URL Destinations
This commit is contained in:
Koichi ITO
GitHub
commit
84eac104f0
3 changed files with 601 additions and 27 deletions
|
|
@ -437,6 +437,121 @@ module MCP
|
|||
refute(Discovery.secure_url?("not a url"))
|
||||
end
|
||||
|
||||
def test_same_origin_accepts_matching_scheme_host_and_port
|
||||
assert(Discovery.same_origin?("https://srv.example.com/.well-known/x", "https://srv.example.com/mcp"))
|
||||
assert(Discovery.same_origin?("https://srv.example.com:8443/a", "https://srv.example.com:8443/b"))
|
||||
assert(Discovery.same_origin?("http://localhost:9292/a", "http://localhost:9292/mcp"))
|
||||
assert(Discovery.same_origin?("https://srv.example.com/a?q=1#f", "https://srv.example.com/mcp"))
|
||||
end
|
||||
|
||||
def test_same_origin_ignores_case_in_scheme_and_host
|
||||
assert(Discovery.same_origin?("HTTPS://SRV.Example.COM/a", "https://srv.example.com/mcp"))
|
||||
end
|
||||
|
||||
def test_same_origin_treats_an_explicit_default_port_as_the_implicit_one
|
||||
assert(Discovery.same_origin?("https://srv.example.com:443/a", "https://srv.example.com/mcp"))
|
||||
assert(Discovery.same_origin?("http://srv.example.com:80/a", "http://srv.example.com/mcp"))
|
||||
end
|
||||
|
||||
def test_same_origin_rejects_differing_components
|
||||
refute(Discovery.same_origin?("https://attacker.example.com/a", "https://srv.example.com/mcp"))
|
||||
refute(Discovery.same_origin?("https://169.254.169.254/latest/", "https://srv.example.com/mcp"))
|
||||
refute(Discovery.same_origin?("https://srv.example.com:8443/a", "https://srv.example.com/mcp"))
|
||||
refute(Discovery.same_origin?("http://srv.example.com/a", "https://srv.example.com/mcp"))
|
||||
# A subdomain is a different origin, so a hijacked challenge cannot walk
|
||||
# sideways into a sibling host that shares the registrable domain.
|
||||
refute(Discovery.same_origin?("https://evil.srv.example.com/a", "https://srv.example.com/mcp"))
|
||||
end
|
||||
|
||||
def test_same_origin_rejects_malformed_or_hostless_urls
|
||||
refute(Discovery.same_origin?("not a url", "https://srv.example.com/mcp"))
|
||||
refute(Discovery.same_origin?("https://srv.example.com/a", "not a url"))
|
||||
refute(Discovery.same_origin?("https://", "https://srv.example.com/mcp"))
|
||||
refute(Discovery.same_origin?(nil, "https://srv.example.com/mcp"))
|
||||
refute(Discovery.same_origin?("/relative/path", "https://srv.example.com/mcp"))
|
||||
end
|
||||
|
||||
def test_private_network_host_recognizes_reserved_ipv4_ranges
|
||||
assert(Discovery.private_network_host?("0.0.0.0"))
|
||||
assert(Discovery.private_network_host?("10.0.0.5"))
|
||||
assert(Discovery.private_network_host?("100.64.0.1"))
|
||||
assert(Discovery.private_network_host?("127.0.0.1"))
|
||||
# The cloud metadata address this check exists for.
|
||||
assert(Discovery.private_network_host?("169.254.169.254"))
|
||||
assert(Discovery.private_network_host?("172.16.0.1"))
|
||||
assert(Discovery.private_network_host?("172.31.255.255"))
|
||||
assert(Discovery.private_network_host?("192.168.1.1"))
|
||||
end
|
||||
|
||||
def test_private_network_host_recognizes_reserved_ipv6_ranges
|
||||
assert(Discovery.private_network_host?("::"))
|
||||
assert(Discovery.private_network_host?("::1"))
|
||||
assert(Discovery.private_network_host?("[::1]"))
|
||||
assert(Discovery.private_network_host?("fd00::1"))
|
||||
assert(Discovery.private_network_host?("fe80::1"))
|
||||
assert(Discovery.private_network_host?("[fe80::1]"))
|
||||
end
|
||||
|
||||
def test_private_network_host_unwraps_ipv4_mapped_ipv6
|
||||
# `::ffff:169.254.169.254` reaches the same interface as the IPv4 spelling,
|
||||
# so the mapped form must not be a way around the ranges above.
|
||||
assert(Discovery.private_network_host?("::ffff:169.254.169.254"))
|
||||
assert(Discovery.private_network_host?("[::ffff:10.0.0.5]"))
|
||||
assert(Discovery.private_network_host?("::ffff:127.0.0.1"))
|
||||
refute(Discovery.private_network_host?("::ffff:93.184.216.34"))
|
||||
end
|
||||
|
||||
def test_private_network_host_refuses_non_canonical_ipv4_spellings
|
||||
# `inet_aton` accepts forms `IPAddr` rejects, and the resolver reaches the same
|
||||
# interfaces through them: `2130706433` and `127.1` are 127.0.0.1, and
|
||||
# `0xa9fea9fe` is the cloud metadata address. Checking only the dotted-decimal
|
||||
# spelling would refuse `169.254.169.254` and admit every alias of it.
|
||||
assert(Discovery.private_network_host?("2130706433"))
|
||||
assert(Discovery.private_network_host?("0x7f000001"))
|
||||
assert(Discovery.private_network_host?("0xa9fea9fe"))
|
||||
assert(Discovery.private_network_host?("127.1"))
|
||||
assert(Discovery.private_network_host?("127.0.1"))
|
||||
assert(Discovery.private_network_host?("0x7f.0.0.1"))
|
||||
assert(Discovery.private_network_host?("0251.0376.0251.0376"))
|
||||
end
|
||||
|
||||
def test_private_network_host_refuses_numeric_spellings_whose_value_is_resolver_dependent
|
||||
# `010.0.0.1` is 8.0.0.1 read as octal and 10.0.0.1 read as decimal, and platforms
|
||||
# disagree on which it is. Both readings are refused rather than one being chosen,
|
||||
# since the reading not taken would otherwise be a way through.
|
||||
assert(Discovery.private_network_host?("010.0.0.1"))
|
||||
assert(Discovery.private_network_host?("0177.0.0.1"))
|
||||
|
||||
# The same applies to numeric spellings that land outside the blocked ranges, or
|
||||
# nowhere at all: nothing is addressed this way on purpose.
|
||||
assert(Discovery.private_network_host?("0x8080808"))
|
||||
assert(Discovery.private_network_host?("256.1.1.1"))
|
||||
end
|
||||
|
||||
def test_private_network_host_recognizes_localhost_by_name_only
|
||||
assert(Discovery.private_network_host?("localhost"))
|
||||
assert(Discovery.private_network_host?("LOCALHOST"))
|
||||
refute(Discovery.private_network_host?("foo.localhost"))
|
||||
refute(Discovery.private_network_host?("localhost.attacker.com"))
|
||||
end
|
||||
|
||||
def test_private_network_host_rejects_public_addresses_and_names
|
||||
refute(Discovery.private_network_host?("93.184.216.34"))
|
||||
refute(Discovery.private_network_host?("8.8.8.8"))
|
||||
refute(Discovery.private_network_host?("172.32.0.1"))
|
||||
refute(Discovery.private_network_host?("2606:4700::1111"))
|
||||
refute(Discovery.private_network_host?("srv.example.com"))
|
||||
refute(Discovery.private_network_host?(nil))
|
||||
refute(Discovery.private_network_host?(""))
|
||||
end
|
||||
|
||||
def test_private_network_host_does_not_resolve_names
|
||||
# Documents the deliberate limit of the predicate: an internal host that is
|
||||
# named rather than addressed reads as public here. `same_origin?` is what
|
||||
# keeps the `resource_metadata` URL from reaching one.
|
||||
refute(Discovery.private_network_host?("vault.corp.internal"))
|
||||
end
|
||||
|
||||
def test_client_id_metadata_document_url_accepts_https_with_path
|
||||
assert(Discovery.client_id_metadata_document_url?("https://app.example.com/client-metadata.json"))
|
||||
assert(Discovery.client_id_metadata_document_url?("https://app.example.com/path/to/cm"))
|
||||
|
|
|
|||
|
|
@ -68,6 +68,23 @@ module MCP
|
|||
WebMock.reset!
|
||||
end
|
||||
|
||||
# A plain authorization-code provider for the destination-check tests.
|
||||
# They all expect to fail before any redirect happens, so the handlers stay inert;
|
||||
# a test that reached them would be asserting the wrong thing.
|
||||
def ssrf_test_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) {},
|
||||
callback_handler: -> { ["code", "state"] },
|
||||
)
|
||||
end
|
||||
|
||||
def client_credentials_provider(token_endpoint_auth_method: "client_secret_basic")
|
||||
ClientCredentialsProvider.new(
|
||||
client_id: "cc-client",
|
||||
|
|
@ -1744,10 +1761,212 @@ module MCP
|
|||
assert_match(/resource_metadata|HTTPS|Communication Security/i, error.message)
|
||||
end
|
||||
|
||||
def test_run_does_not_fetch_a_resource_metadata_url_off_the_server_origin
|
||||
# A server that answers 401 chooses the `resource_metadata` URL, and discovery puts it first
|
||||
# in the candidate list. Left unchecked, a single response header aims the client's first OAuth request
|
||||
# at any host it can route to, including the cloud metadata address. The assertion that matters is not
|
||||
# the raise but `assert_not_requested`: nothing downstream can un-send a request.
|
||||
metadata_url = "https://169.254.169.254/latest/meta-data/iam/"
|
||||
stub_request(:get, metadata_url).to_return(status: 200, body: "{}")
|
||||
|
||||
error = assert_raises(Flow::AuthorizationError) do
|
||||
Flow.new(provider: ssrf_test_provider).run!(
|
||||
server_url: @server_url,
|
||||
resource_metadata_url: metadata_url,
|
||||
)
|
||||
end
|
||||
|
||||
assert_not_requested(:get, metadata_url)
|
||||
assert_match(/resource_metadata/i, error.message)
|
||||
assert_match(/origin/i, error.message)
|
||||
end
|
||||
|
||||
def test_run_does_not_fetch_an_https_resource_metadata_url_on_a_foreign_public_origin
|
||||
# The scheme check alone passes this one: it is HTTPS, just not the server's.
|
||||
metadata_url = "https://attacker.example.com/.well-known/oauth-protected-resource"
|
||||
stub_request(:get, metadata_url).to_return(status: 200, body: "{}")
|
||||
|
||||
assert_raises(Flow::AuthorizationError) do
|
||||
Flow.new(provider: ssrf_test_provider).run!(
|
||||
server_url: @server_url,
|
||||
resource_metadata_url: metadata_url,
|
||||
)
|
||||
end
|
||||
|
||||
assert_not_requested(:get, metadata_url)
|
||||
end
|
||||
|
||||
def test_refresh_does_not_fetch_a_resource_metadata_url_off_the_server_origin
|
||||
# `refresh!` reads the same header on a 401 and must gate it identically;
|
||||
# otherwise the token-refresh path stays reachable after `run!` is closed.
|
||||
metadata_url = "https://169.254.169.254/latest/meta-data/iam/"
|
||||
stub_request(:get, metadata_url).to_return(status: 200, body: "{}")
|
||||
|
||||
provider = ssrf_test_provider
|
||||
provider.save_client_information("client_id" => "test-client")
|
||||
provider.save_tokens("access_token" => "stale-at", "refresh_token" => "saved-rt")
|
||||
|
||||
assert_raises(Flow::AuthorizationError) do
|
||||
Flow.new(provider: provider).refresh!(
|
||||
server_url: @server_url,
|
||||
resource_metadata_url: metadata_url,
|
||||
)
|
||||
end
|
||||
|
||||
assert_not_requested(:get, metadata_url)
|
||||
end
|
||||
|
||||
def test_run_does_not_contact_an_authorization_server_on_a_private_address
|
||||
# One hop deeper than the `resource_metadata` URL: the server serves its own PRM from its own origin,
|
||||
# then names a private address as its authorization server. Same-origin cannot see this one,
|
||||
# so the range check has to.
|
||||
stub_request(:get, @prm_url).to_return(
|
||||
status: 200,
|
||||
headers: { "Content-Type" => "application/json" },
|
||||
body: JSON.generate(
|
||||
resource: @server_url,
|
||||
authorization_servers: ["https://169.254.169.254"],
|
||||
),
|
||||
)
|
||||
stub_request(:get, %r{\Ahttps://169\.254\.169\.254/}).to_return(status: 200, body: "{}")
|
||||
|
||||
error = assert_raises(Flow::AuthorizationError) do
|
||||
Flow.new(provider: ssrf_test_provider).run!(server_url: @server_url, resource_metadata_url: @prm_url)
|
||||
end
|
||||
|
||||
assert_not_requested(:get, %r{\Ahttps://169\.254\.169\.254/})
|
||||
assert_match(/private network/i, error.message)
|
||||
end
|
||||
|
||||
def test_run_does_not_contact_an_authorization_server_spelled_as_a_packed_integer
|
||||
# `0xa9fea9fe` is 169.254.169.254 written for `inet_aton`. The connection resolves it to
|
||||
# the same address, so the previous test's protection has to survive the rewrite.
|
||||
stub_request(:get, @prm_url).to_return(
|
||||
status: 200,
|
||||
headers: { "Content-Type" => "application/json" },
|
||||
body: JSON.generate(
|
||||
resource: @server_url,
|
||||
authorization_servers: ["https://0xa9fea9fe"],
|
||||
),
|
||||
)
|
||||
stub_request(:get, %r{\Ahttps://0xa9fea9fe/}).to_return(status: 200, body: "{}")
|
||||
|
||||
error = assert_raises(Flow::AuthorizationError) do
|
||||
Flow.new(provider: ssrf_test_provider).run!(server_url: @server_url, resource_metadata_url: @prm_url)
|
||||
end
|
||||
|
||||
assert_not_requested(:get, %r{\Ahttps://0xa9fea9fe/})
|
||||
assert_match(/private network/i, error.message)
|
||||
end
|
||||
|
||||
def test_run_does_not_post_to_a_token_endpoint_on_a_private_address
|
||||
# The token endpoint receives the client credentials and the authorization code,
|
||||
# so an AS document that relocates it inside the client's network is a credential disclosure,
|
||||
# not just a probe.
|
||||
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: "https://10.0.0.5/token",
|
||||
registration_endpoint: "#{@auth_base}/register",
|
||||
code_challenge_methods_supported: ["S256"],
|
||||
token_endpoint_auth_methods_supported: ["none"],
|
||||
),
|
||||
)
|
||||
stub_request(:post, "https://10.0.0.5/token").to_return(status: 200, body: "{}")
|
||||
|
||||
error = assert_raises(Flow::AuthorizationError) do
|
||||
Flow.new(provider: ssrf_test_provider).run!(server_url: @server_url, resource_metadata_url: @prm_url)
|
||||
end
|
||||
|
||||
assert_not_requested(:post, "https://10.0.0.5/token")
|
||||
assert_match(/private network/i, error.message)
|
||||
end
|
||||
|
||||
def test_run_does_not_post_to_a_registration_endpoint_on_a_private_address
|
||||
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: "https://192.168.1.1/register",
|
||||
code_challenge_methods_supported: ["S256"],
|
||||
token_endpoint_auth_methods_supported: ["none"],
|
||||
),
|
||||
)
|
||||
stub_request(:post, "https://192.168.1.1/register").to_return(status: 201, body: "{}")
|
||||
|
||||
assert_raises(Flow::AuthorizationError) do
|
||||
Flow.new(provider: ssrf_test_provider).run!(server_url: @server_url, resource_metadata_url: @prm_url)
|
||||
end
|
||||
|
||||
assert_not_requested(:post, "https://192.168.1.1/register")
|
||||
end
|
||||
|
||||
def test_run_allows_a_private_authorization_server_when_the_mcp_server_is_also_private
|
||||
# The shape the conformance harness runs and the shape local development takes:
|
||||
# the MCP server and the authorization server sit on two loopback ports.
|
||||
# Once the caller has pointed the client at a private address, the authorization server
|
||||
# living there too is not an escalation, and refusing it would break every local setup.
|
||||
server_url = "http://localhost:9292/mcp"
|
||||
prm_url = "http://localhost:9292/.well-known/oauth-protected-resource/mcp"
|
||||
auth_base = "http://localhost:9393"
|
||||
|
||||
stub_request(:get, prm_url).to_return(
|
||||
status: 200,
|
||||
headers: { "Content-Type" => "application/json" },
|
||||
body: JSON.generate(resource: server_url, authorization_servers: [auth_base]),
|
||||
)
|
||||
stub_request(:get, "#{auth_base}/.well-known/oauth-authorization-server").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",
|
||||
code_challenge_methods_supported: ["S256"],
|
||||
token_endpoint_auth_methods_supported: ["none"],
|
||||
),
|
||||
)
|
||||
stub_request(:post, "#{auth_base}/register").to_return(
|
||||
status: 201,
|
||||
headers: { "Content-Type" => "application/json" },
|
||||
body: JSON.generate(client_id: "local-client"),
|
||||
)
|
||||
stub_request(:post, "#{auth_base}/token").to_return(
|
||||
status: 200,
|
||||
headers: { "Content-Type" => "application/json" },
|
||||
body: JSON.generate(access_token: "local-token", token_type: "Bearer", expires_in: 3600),
|
||||
)
|
||||
|
||||
state_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) { state_holder[:state] = URI.decode_www_form(url.query).to_h.fetch("state") },
|
||||
callback_handler: -> { ["test-auth-code", state_holder[:state]] },
|
||||
)
|
||||
|
||||
result = Flow.new(provider: provider).run!(server_url: server_url, resource_metadata_url: prm_url)
|
||||
|
||||
assert_equal(:authorized, result)
|
||||
assert_equal("local-token", provider.access_token)
|
||||
end
|
||||
|
||||
def test_run_rejects_when_as_metadata_issuer_does_not_match
|
||||
# RFC 8414 Section 3.3: the AS metadata's `issuer` value MUST equal
|
||||
# the discovery URL. A metadata document advertising a different issuer
|
||||
# than the one PRM pointed at is treated as a confused-deputy attempt.
|
||||
# RFC 8414 Section 3.3: the AS metadata's `issuer` value MUST equal the discovery URL.
|
||||
# A metadata document advertising a different issuer than the one PRM pointed at is treated
|
||||
# as a confused-deputy attempt.
|
||||
stub_request(:get, @as_metadata_url).to_return(
|
||||
status: 200,
|
||||
headers: { "Content-Type" => "application/json" },
|
||||
|
|
@ -1781,9 +2000,8 @@ module MCP
|
|||
end
|
||||
|
||||
def test_run_rejects_when_as_metadata_issuer_differs_by_trailing_slash
|
||||
# RFC 8414 Section 3.3 requires the issuer to be identical, not "equivalent
|
||||
# after normalization". `https://auth.example.com` and
|
||||
# `https://auth.example.com/` are different strings and MUST NOT match.
|
||||
# RFC 8414 Section 3.3 requires the issuer to be identical, not "equivalent after normalization".
|
||||
# `https://auth.example.com` and `https://auth.example.com/` are different strings and MUST NOT match.
|
||||
stub_request(:get, @as_metadata_url).to_return(
|
||||
status: 200,
|
||||
headers: { "Content-Type" => "application/json" },
|
||||
|
|
|
|||
|
|
@ -101,6 +101,43 @@ module MCP
|
|||
assert_equal("test-token-after-flow", provider.access_token)
|
||||
end
|
||||
|
||||
def test_send_request_does_not_follow_a_resource_metadata_challenge_off_the_server_origin
|
||||
# End to end over the transport, which is where the header is actually parsed:
|
||||
# a server that answers 401 must not be able to name an unrelated host in
|
||||
# `resource_metadata` and have the client fetch it. The request never leaving
|
||||
# is the point; the 401 surfacing to the caller is secondary.
|
||||
metadata_url = "https://169.254.169.254/latest/meta-data/iam/"
|
||||
|
||||
stub_request(:post, @mcp_url).to_return(
|
||||
status: 401,
|
||||
headers: {
|
||||
"WWW-Authenticate" => %(Bearer error="invalid_token", resource_metadata="#{metadata_url}"),
|
||||
},
|
||||
body: "",
|
||||
)
|
||||
stub_request(:get, metadata_url).to_return(status: 200, body: "{}")
|
||||
|
||||
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) {},
|
||||
callback_handler: -> { ["code", "state"] },
|
||||
)
|
||||
|
||||
transport = HTTP.new(url: @mcp_url, oauth: provider)
|
||||
|
||||
assert_raises(StandardError) do
|
||||
transport.send_request(request: { jsonrpc: "2.0", id: "1", method: "tools/list" })
|
||||
end
|
||||
|
||||
assert_not_requested(:get, metadata_url)
|
||||
end
|
||||
|
||||
def test_send_request_runs_step_up_flow_on_403_insufficient_scope
|
||||
# First call with the initial token: 403 carrying an `insufficient_scope`
|
||||
# Bearer challenge with the escalated scope. The transport must re-run
|
||||
|
|
|
|||
Loading…
Reference in a new issue