Restrict OAuth Discovery URL Destinations
## Motivation and Context The OAuth client checked the scheme of every discovery URL but never checked where it pointed. `Discovery.secure_url?` returns true for any `https` URL regardless of host, and the `resource_metadata` URL from a `WWW-Authenticate` challenge is placed first in the candidate list, so a server answering `401 Bearer resource_metadata="https://169.254.169.254/..."` had the client issue that request during discovery, before the user approves anything. The same gap applied one hop deeper: a server that serves its own Protected Resource Metadata can name any host in `authorization_servers`, and the endpoints in the document fetched from there receive the dynamic client registration and the token request that carries the client credentials. Two rules now apply to those URLs. Both run before the request is sent, and neither is configurable. The `resource_metadata` URL must be on the MCP server's own origin. Protected Resource Metadata describes that server, so a real deployment publishes it there, and requiring it means a challenge cannot aim the first request of the flow at an unrelated host. RFC 9728 does not itself require this, so the SDK is deliberately stricter than the specification: the range check below cannot recognize an internal host that is named rather than addressed, and this is the one URL a server supplies directly. 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, which is the precaution RFC 9728 Section 7.7 asks clients to take. That check is skipped when the configured MCP server URL is itself on such an address. Pointing a client at a private network is a deliberate act and its authorization server usually lives on the same network, so `http://localhost` development and deployments that never leave a corporate network keep working. Only a server reachable on the public internet is barred from steering the client inward. A range check is only as good as its reading of the address, so the second rule also refuses hosts 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 reading only the dotted-decimal spelling would refuse `169.254.169.254` while admitting every alias of it. Those hosts are refused outright rather than decoded and range-checked, because their 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`. Choosing a reading here would leave whichever one was not taken as a way through, and nothing is legitimately addressed in this notation. Hostnames are not resolved. A lookup here would be a second network request driven by the same untrusted input, and the address it returned need not be the one the HTTP client connects to a moment later, so the check would read as a guarantee it cannot make. `MCP::Client::OAuth::Flow` also documents why its Faraday connection has no redirect-following middleware: every check runs against the URL as written, so a connection that follows a `3xx` on its own would reach hosts these rules just refused. ## How Has This Been Tested? - New unit tests in `test/mcp/client/oauth/discovery_test.rb` cover `same_origin?` (case-insensitive scheme and host, explicit default ports, differing port/host/scheme, subdomains, malformed and hostless input) and `private_network_host?` (every blocked range, IPv4-mapped IPv6 spellings, bracketed IPv6, `localhost` matched exactly, and public addresses and names). One test documents that a named internal host reads as public, which is the deliberate limit of the predicate. - Further unit tests cover the numeric spellings: packed integers, hexadecimal, and short forms that reach a blocked range, and the ambiguous leading-zero forms whose value depends on the resolver. Removing the rule leaves them addressable, which is what those tests fail on. - New tests in `test/mcp/client/oauth/flow_test.rb` and `test/mcp/client/oauth/http_oauth_test.rb` drive both rules through `run!`, `refresh!`, and the transport. They assert with `assert_not_requested` that no request was issued rather than only that an error was raised, since nothing downstream can un-send one. One of them names the authorization server `https://0xa9fea9fe`, which without the rule above is fetched. A further test runs a complete flow with the MCP server and the authorization server on two loopback ports, which is what the carve-out exists for. - `rake conformance` passes with all 22 auth scenarios green. Its harness runs the MCP server and the authorization server on separate loopback ports and publishes PRM on the server's own origin, so it exercises exactly the shapes these rules must not break. ## Breaking Changes A client whose MCP server is reachable on the public internet no longer follows a `resource_metadata` URL on a different origin, and no longer contacts an authorization server or OAuth endpoint at a private IP literal or at a host written in the numeric IPv4 notation above. All were previously allowed. Local development, private-network deployments, and the conformance suite are unaffected.
This commit is contained in:
Koichi ITO
parent
b195a21410
commit
aaf0c71995
1 changed files with 601 additions and 27 deletions
|
|
@ -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.
|
||||
|
|
|
|||
Loading…
Reference in a new issue