diff --git a/README.md b/README.md index 0d7a527..a0276d8 100644 --- a/README.md +++ b/README.md @@ -2920,6 +2920,28 @@ whether the swap was triggered by `instance_variable_set(:@url, ...)`, by a Faraday customizer rewriting `url_prefix`, or by a custom middleware rewriting `env.url` (including just `env.url.query`) at request time, and whether the new URL is `http://` *or* `https://` to a different host or tenant. +##### Discovery URL Destinations + +The scheme rules above say how a URL is contacted, not where it points. Discovery URLs arrive from the network, so the SDK also constrains their destinations. +Both checks run before the request is sent, and neither is configurable. + +- The `resource_metadata` URL in a `WWW-Authenticate` challenge must be on the MCP server's own origin. Protected Resource Metadata describes that server, + so a real deployment publishes it there; requiring it means a `401` cannot aim the first request of the flow at an unrelated host. This is stricter than RFC 9728, + which does not require it. +- The PRM `authorization_servers` entry and the `authorization_endpoint`, `token_endpoint`, and `registration_endpoint` from Authorization Server metadata must not be + IP literals in a private, loopback, link-local, or unique-local range, per the SSRF precaution in [RFC 9728 Section 7.7](https://www.rfc-editor.org/rfc/rfc9728#section-7.7). + The blocked ranges are `0.0.0.0/8`, `10.0.0.0/8`, `100.64.0.0/10`, `127.0.0.0/8`, `169.254.0.0/16`, `172.16.0.0/12`, `192.168.0.0/16`, `::/96`, `fc00::/7`, + and `fe80::/10`, along with the IPv4-mapped IPv6 spellings of each and the `localhost` name. +- That range check is skipped when the MCP server URL you configured is itself on such an address. Pointing the client at a private network is a deliberate act, + and the authorization server for it usually lives on the same network, so `http://localhost` development and deployments that never leave a corporate network keep working. + +The range check compares IP literals and does not resolve hostnames, so it cannot recognize an internal service that is named rather than addressed, +such as `https://vault.corp.internal/`. Resolving names here would not close that gap either, because the address the SDK looked up need not be the one +the HTTP client connects to a moment later. The same-origin rule is what protects the `resource_metadata` URL, which is the only one of these a server supplies directly. + +If you replace the OAuth HTTP client through `MCP::Client::OAuth::Flow.new(http_client_factory:)`, do not add redirect-following middleware. Every check above runs against +the URL as written, so a connection that follows a `3xx` on its own would reach hosts these rules just refused. + #### Customizing the Faraday Connection You can pass a block to `MCP::Client::HTTP.new` to customize the underlying Faraday connection. diff --git a/lib/mcp/client/oauth/discovery.rb b/lib/mcp/client/oauth/discovery.rb index 885f1db..04030cc 100644 --- a/lib/mcp/client/oauth/discovery.rb +++ b/lib/mcp/client/oauth/discovery.rb @@ -8,7 +8,7 @@ module MCP module OAuth # Stateless helpers that map MCP-authorization spec URLs and headers into something # the `Flow` orchestrator and `MCP::Client::HTTP` transport can act on. - # The module bundles five concerns that share no state but are closely related to + # The module bundles six concerns that share no state but are closely related to # the spec's "Discovery" and "Communication Security" sections: # # - **`WWW-Authenticate` parsing** (`parse_www_authenticate`): pulls @@ -22,6 +22,10 @@ module MCP # - **Communication Security check** (`secure_url?`): enforces "HTTPS only" # for every OAuth-facing URL, with the loopback carve-out described in # `secure_url?`'s comment. + # - **Destination checks** (`same_origin?`, `private_network_host?`): + # answer *where* a URL points, which the scheme alone does not. `Flow` uses + # them to refuse server-supplied discovery URLs aimed at hosts the MCP server + # has no business steering the client toward. # - **URL canonicalization** (`canonicalize_url`): normalizes scheme, # host, port, path, percent-encoded dot segments, and fragments # so two URLs that *refer to the same resource* compare as equal, @@ -193,6 +197,51 @@ module MCP false end + # Returns true when `url` and `other` share an origin: same scheme, same host, + # and same port. Scheme and host compare case-insensitively, and an implicit default port compares + # equal to the same port written out, because `URI::HTTP#port` already resolves to the default. + # + # Anything that fails to parse or carries no host returns false, so a caller using this as + # a security gate refuses rather than admits on malformed input. + def same_origin?(url, other) + uri = URI.parse(url.to_s) + other_uri = URI.parse(other.to_s) + return false if uri.host.nil? || uri.host.empty? + return false if other_uri.host.nil? || other_uri.host.empty? + + uri.scheme&.downcase == other_uri.scheme&.downcase && + uri.host.downcase == other_uri.host.downcase && + uri.port == other_uri.port + rescue URI::InvalidURIError + false + end + + # Returns true when `host` is an IP literal that is not reachable from the public internet, + # or the `localhost` name. The ranges are the ones RFC 9728 Section 7.7 and the MCP security best practices name + # as SSRF targets, so `169.254.0.0/16` covers the `169.254.169.254` cloud metadata address that motivates the check. + # + # Hostnames are deliberately NOT resolved. A lookup here would be a second network request driven by + # the same untrusted input, and the address it returned could differ from the one the HTTP client connects to + # a moment later, so the check would read as a guarantee it cannot make. That leaves names like `vault.corp.internal` + # out of reach of this predicate; it is one layer of an SSRF defense, not the whole of one. + def private_network_host?(host) + return false if host.nil? || host.empty? + + normalized = host.downcase + return true if normalized == "localhost" + + literal = normalized.delete_prefix("[").delete_suffix("]") + address = parse_ip_address(literal) + return numeric_ipv4_spelling?(literal) unless address + + # `::ffff:169.254.169.254` addresses the same interface as `169.254.169.254`, + # so compare the IPv4 form rather than letting the mapped spelling through. + address = address.native if address.ipv6? && address.ipv4_mapped? + + ranges = address.ipv4? ? PRIVATE_IPV4_RANGES : PRIVATE_IPV6_RANGES + ranges.any? { |range| range.include?(address) } + end + # Returns true when `url` satisfies the structural requirements for # a Client ID Metadata Document URL per the MCP 2025-11-25 # authorization specification and `draft-ietf-oauth-client-id-metadata-document-00`. @@ -287,20 +336,17 @@ module MCP uri.to_s end - # Returns true when `prm` (a PRM `resource` URL) covers `server` - # (the MCP endpoint URL): same scheme/host/port, with PRM's path being - # a prefix of the server's path. When PRM also advertises a query - # string, the server's query MUST be identical to it - # (otherwise a hijacked PRM that advertises `?tenant=evil` would cover - # an MCP server at `?tenant=victim` and let the attacker mint - # a different tenant's token for the same origin + path). - # PRM with *no* query (URI#query returns `nil`) acts as a generic identifier - # over the origin + path prefix and covers any server query. + # Returns true when `prm` (a PRM `resource` URL) covers `server` (the MCP endpoint URL): + # same scheme/host/port, with PRM's path being a prefix of the server's path. When PRM + # also advertises a query string, the server's query MUST be identical to it (otherwise + # a hijacked PRM that advertises `?tenant=evil` would cover an MCP server at `?tenant=victim` + # and let the attacker mint a different tenant's token for the same origin + path). + # PRM with *no* query (URI#query returns `nil`) acts as a generic identifier over + # the origin + path prefix and covers any server query. # - # An empty query (`prm_url?` -- URI#query returns `""`) is NOT - # treated as wildcard: it represents the URI literally `<...>?`, - # which is distinct from "no query at all" and from any non-empty query, - # so it must match exactly. + # An empty query (`prm_url?` -- URI#query returns `""`) is NOT treated as wildcard: + # it represents the URI literally `<...>?`, which is distinct from "no query at all" + # and from any non-empty query, so it must match exactly. # # Both arguments must already be canonicalized. def resource_covers?(prm:, server:) @@ -340,6 +386,29 @@ module MCP IPV6_LOOPBACK = IPAddr.new("::1") private_constant :IPV4_LOOPBACK_RANGE, :IPV6_LOOPBACK + # Backing ranges for `private_network_host?`. + # + # `0.0.0.0/8` is "this network" and `100.64.0.0/10` is the carrier-grade NAT + # space; neither is a destination a legitimate authorization server publishes, + # and both are reachable enough from a client's network position to be worth + # refusing. `::/96` covers the unspecified address, IPv6 loopback, and + # the deprecated IPv4-compatible form (`::10.0.0.1`) in one range. + PRIVATE_IPV4_RANGES = [ + IPAddr.new("0.0.0.0/8"), + IPAddr.new("10.0.0.0/8"), + IPAddr.new("100.64.0.0/10"), + IPAddr.new("127.0.0.0/8"), + IPAddr.new("169.254.0.0/16"), + IPAddr.new("172.16.0.0/12"), + IPAddr.new("192.168.0.0/16"), + ].freeze + PRIVATE_IPV6_RANGES = [ + IPAddr.new("::/96"), + IPAddr.new("fc00::/7"), + IPAddr.new("fe80::/10"), + ].freeze + private_constant :PRIVATE_IPV4_RANGES, :PRIVATE_IPV6_RANGES + def loopback_host?(host) return false if host.nil? || host.empty? @@ -362,6 +431,31 @@ module MCP nil end + # Matches one part of the `inet_aton` numeric grammar: hexadecimal (`0x7f`), + # octal (`0177`), or decimal (`127`). + INET_ATON_PART = /\A(?:0x\h+|0[0-7]*|[1-9]\d*)\z/.freeze + private_constant :INET_ATON_PART + + # Returns true when `host` is written in the numeric IPv4 grammar `inet_aton` accepts + # and `IPAddr` rejects: one to four dot-separated parts, each hexadecimal, octal, or + # decimal, with the last part filling the remaining bytes. `2130706433`, `127.1`, and + # `0x7f000001` all reach 127.0.0.1, and `0xa9fea9fe` reaches the cloud metadata address, + # so a check that understood only the canonical form would refuse `169.254.169.254` + # while waving through every other spelling of it. + # + # Such a host is refused outright rather than decoded and range-checked, because its + # value is resolver-dependent: a leading zero reads as octal on one platform and decimal + # on another, which is the difference between 8.0.0.1 and 10.0.0.1 for `010.0.0.1`. + # Deciding here which one it meant would leave whichever reading was not taken as a way + # through. No authorization server is legitimately addressed in this notation, so + # refusing the grammar entirely costs nothing and leaves no spelling to miss. + def numeric_ipv4_spelling?(host) + parts = host.split(".", -1) + return false if parts.empty? || parts.length > 4 + + parts.all? { |part| INET_ATON_PART.match?(part) } + end + # A redirect URI counts as native when it uses a custom non-http(s) scheme # (e.g. `com.example.app:/callback`) or when it is an http(s) URI whose host is # a loopback address. A URI without a scheme or one that fails to parse is not native. diff --git a/lib/mcp/client/oauth/flow.rb b/lib/mcp/client/oauth/flow.rb index 32669f2..88413a3 100644 --- a/lib/mcp/client/oauth/flow.rb +++ b/lib/mcp/client/oauth/flow.rb @@ -33,9 +33,15 @@ module MCP def run!(server_url:, resource_metadata_url: nil, scope: nil) # The `resource_metadata` URL ships in `WWW-Authenticate` and is the very # first thing we contact in the OAuth flow, so it has to clear the same - # Communication Security bar as the OAuth endpoints downstream. + # Communication Security bar as the OAuth endpoints downstream, and it has to + # point back at the server that issued the challenge. if resource_metadata_url ensure_secure_url!(resource_metadata_url, label: "WWW-Authenticate resource_metadata URL") + ensure_same_origin!( + resource_metadata_url, + label: "WWW-Authenticate resource_metadata URL", + server_url: server_url, + ) end prm, authorization_server = locate_authorization_server( @@ -49,7 +55,11 @@ module MCP # being redirected to credentials minted for a different audience. resource = canonical_resource(server_url: server_url, prm_resource: prm&.dig("resource")) - as_metadata = authorization_server_metadata(authorization_server: authorization_server, legacy: prm.nil?) + as_metadata = authorization_server_metadata( + authorization_server: authorization_server, + legacy: prm.nil?, + server_url: server_url, + ) case provider_authorization_flow when :client_credentials @@ -212,6 +222,11 @@ module MCP if resource_metadata_url ensure_secure_url!(resource_metadata_url, label: "WWW-Authenticate resource_metadata URL") + ensure_same_origin!( + resource_metadata_url, + label: "WWW-Authenticate resource_metadata URL", + server_url: server_url, + ) end prm, authorization_server = locate_authorization_server( @@ -221,7 +236,11 @@ module MCP resource = canonical_resource(server_url: server_url, prm_resource: prm&.dig("resource")) - as_metadata = authorization_server_metadata(authorization_server: authorization_server, legacy: prm.nil?) + as_metadata = authorization_server_metadata( + authorization_server: authorization_server, + legacy: prm.nil?, + server_url: server_url, + ) client_info = if have_stored_client_info # Pre-registered / DCR-issued `client_information` always wins: if the user picked an explicit identity, @@ -296,6 +315,11 @@ module MCP if prm authorization_server = first_authorization_server(prm) ensure_secure_url!(authorization_server, label: "PRM `authorization_servers` entry") + ensure_routable_destination!( + authorization_server, + label: "PRM `authorization_servers` entry", + server_url: server_url, + ) [prm, authorization_server] else authorization_base = server_origin!(server_url) @@ -311,7 +335,7 @@ module MCP # and a pre-PRM server may host its OAuth endpoints under a path prefix whose `issuer` legitimately differs from # the origin the metadata was discovered at (neither the TypeScript nor the Python SDK validates the issuer on this path). # When even the metadata document is absent, the legacy spec's default endpoints are used. - def authorization_server_metadata(authorization_server:, legacy:) + def authorization_server_metadata(authorization_server:, legacy:, server_url:) metadata = if legacy begin fetch_authorization_server_metadata(issuer_url: authorization_server) @@ -324,7 +348,7 @@ module MCP end end - ensure_secure_endpoints!(metadata) + ensure_secure_endpoints!(metadata, server_url: server_url) metadata end @@ -445,13 +469,72 @@ module MCP "#{label} #{url.inspect} is not over HTTPS; refusing to use it (MCP authorization Communication Security)." end - def ensure_secure_endpoints!(as_metadata) + # Requires a URL the *server* chose to sit on the origin the *caller* chose. + # + # Protected Resource Metadata describes the MCP server itself, so on a real deployment it is + # published on that server's own origin. Without this check a `WWW-Authenticate` challenge + # can aim the first request of the flow at any host the client can route to: the URL arrives + # from the network, it is fetched before the user approves anything, and the `resource` check that + # runs afterwards cannot un-send the request. + # + # RFC 9728 does not itself require the metadata URL to be same-origin, so this is stricter than + # the specification. It is enforced unconditionally because no known deployment publishes its PRM + # anywhere else, and because the alternative (`Discovery.private_network_host?`) cannot see + # internal hosts that are named rather than addressed. + # https://www.rfc-editor.org/rfc/rfc9728#section-7.7 + def ensure_same_origin!(url, label:, server_url:) + return if Discovery.same_origin?(url, server_url) + + raise AuthorizationError, + "#{label} #{sanitized_url(url).inspect} is not on the MCP server origin " \ + "#{sanitized_url(server_url).inspect}; refusing to fetch it." + end + + # Refuses an OAuth URL that points into a private, loopback, link-local, or unique-local address, + # which is the SSRF precaution RFC 9728 Section 7.7 and the MCP security best practices ask clients to take. + # + # The carve-out matters as much as the rule: when the MCP server the caller configured is itself on such an address, + # the whole flow is already inside that network and the authorization server legitimately lives there too. + # That covers `http://localhost` development, the conformance harness (which runs the MCP server and + # the authorization server on two loopback ports), and deployments that never leave a corporate network. + # Only a server reachable on the public internet is barred from steering the client inward. + # https://modelcontextprotocol.io/specification/2025-11-25/basic/authorization + def ensure_routable_destination!(url, label:, server_url:) + return unless private_network_url?(url) + return if private_network_url?(server_url) + + raise AuthorizationError, + "#{label} #{sanitized_url(url).inspect} points into a private network range, " \ + "which the MCP server at #{sanitized_url(server_url).inspect} is not on; refusing to contact it." + end + + def ensure_secure_endpoints!(as_metadata, server_url:) ["authorization_endpoint", "token_endpoint", "registration_endpoint"].each do |key| endpoint = as_metadata[key] - ensure_secure_url!(endpoint, label: "Authorization server #{key}") if endpoint + next unless endpoint + + ensure_secure_url!(endpoint, label: "Authorization server #{key}") + ensure_routable_destination!(endpoint, label: "Authorization server #{key}", server_url: server_url) end end + # `ensure_secure_url!` runs first at every call site and already rejects a URL that fails to parse, + # so the rescue here is a backstop rather than a leniency. + def private_network_url?(url) + Discovery.private_network_host?(URI.parse(url.to_s).host) + rescue URI::InvalidURIError + false + end + + # Strips userinfo and query before a URL reaches an exception message, the same precaution `MCP::Client::HTTP` takes + # when it reports a URL: these values come off the network and can carry credentials that would otherwise land in + # every log destination the error passes through. + def sanitized_url(url) + Discovery.canonicalize_origin_and_path(url) + rescue URI::Error + url.to_s + end + # Per RFC 8414 Section 3.3, the AS metadata document's `issuer` value MUST be # identical (literal byte-for-byte equality, no normalization) to # the issuer URL the client used to discover that document. This guards @@ -962,6 +1045,11 @@ module MCP @http_client ||= @http_client_factory.call end + # Deliberately built without redirect-following middleware. Every destination check in + # this class runs against the URL as written, before the request goes out, so a connection + # that transparently followed a `3xx` would let a server reach a host the checks just refused. + # A caller passing `http_client_factory:` takes on that responsibility: add redirect following here + # and the guards above only cover the first hop. def default_http_client require "faraday" Faraday.new do |faraday| diff --git a/test/mcp/client/oauth/discovery_test.rb b/test/mcp/client/oauth/discovery_test.rb index 4b29184..c8d9fda 100644 --- a/test/mcp/client/oauth/discovery_test.rb +++ b/test/mcp/client/oauth/discovery_test.rb @@ -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")) diff --git a/test/mcp/client/oauth/flow_test.rb b/test/mcp/client/oauth/flow_test.rb index f27679b..1cc248d 100644 --- a/test/mcp/client/oauth/flow_test.rb +++ b/test/mcp/client/oauth/flow_test.rb @@ -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" }, diff --git a/test/mcp/client/oauth/http_oauth_test.rb b/test/mcp/client/oauth/http_oauth_test.rb index 26d074d..73fcf04 100644 --- a/test/mcp/client/oauth/http_oauth_test.rb +++ b/test/mcp/client/oauth/http_oauth_test.rb @@ -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