Resolve dot-segments when deriving and matching OAuth resource URLs - #3343
Resolve dot-segments when deriving and matching OAuth resource URLs#3343maxisbey wants to merge 1 commit into
Conversation
Sorry, something went wrong.
resource_url_from_server_url() now applies RFC 3986 remove_dot_segments (including the %2E spellings WHATWG treats as dots) so the resource identifier names the location the HTTP client actually requests. check_resource_allowed() resolves both paths the same way before its prefix comparison, and parses with urlsplit so ";parameters" stay part of the last path segment instead of being dropped. Fixes #3303
There was a problem hiding this comment.
Beyond the inline findings, a few other concerns were examined and ruled out: treating the %2e/%2e%2e spellings as dot-segments does not make matching more permissive than the PRM side, since pydantic applies the same WHATWG normalization to the resource string before comparison (the exhaustive property test in tests/shared/test_auth_utils.py pins this agreement); .. is floored at the path root in _remove_dot_segments (the leading empty segment is never popped), so a request path cannot resolve above /; and %2F, empty segments, and ;params are correctly kept as segment content rather than boundaries.
Extended reasoning...This is a security-relevant change to the RFC 8707 resource-indicator helpers in src/mcp/shared/auth_utils.py, so the inline findings already signal that a human should look; this note only records what else was checked. I traced _remove_dot_segments against the new tests: the len(output) > 1 guard preserves the leading empty segment produced by an absolute path, so .. cannot climb above the root (e.g. /../admin resolves to /admin), and splitting only on / leaves %2F, //, and ; in the last segment intact, matching the documented RFC 3986 behavior. The concern that recognizing WHATWG percent-encoded dot spellings could loosen PRM validation is settled by the property test comparing the resolver against pydantic's AnyHttpUrl parsing, which is exactly what normalizes the server-supplied resource side, so both operands of check_resource_allowed agree on resolution.
Sorry, something went wrong.
| parsed._replace( | ||
| scheme=parsed.scheme.lower(), | ||
| netloc=parsed.netloc.lower(), | ||
| path=_remove_dot_segments(parsed.path), |
There was a problem hiding this comment.
🟣 Pre-existing, left behind by this partial fix: the resource URL is now derived with dot-segments floored at the path root, but build_protected_resource_metadata_discovery_urls (src/mcp/client/auth/utils.py:90) still embeds the raw unresolved path into '/.well-known/oauth-protected-resource{path}' via urljoin, whose RFC 3986 resolution has no floor at the well-known prefix — a '..' that the new _remove_dot_segments correctly discards at root instead consumes the 'oauth-protected-resource' segment, so PRM discovery queries the wrong well-known URL for exactly the dot-segmented server_urls this PR sets out to handle (same pattern at utils.py:158 for AS metadata).
Extended reasoning...A user configures OAuthClientProvider with server_url='https://host/a/../../b/mcp'. After this PR, resource_url_from_server_url correctly derives 'https://host/b/mcp' (the second '..' is floored at root by _remove_dot_segments). But path-based PRM discovery builds urljoin('https://host', '/.well-known/oauth-protected-resource/a/../../b/mcp'); CPython's urljoin pops segments with a bare resolved_path.pop(), so the second '..' removes 'oauth-protected-resource' and the client fetches 'https://host/.well-known/b/mcp' — never the RFC 9728 location 'https://host/.well-known/oauth-protected-resource/b/mcp'. On a multi-tenant server that only serves path-based PRM, discovery 404s, falls back to the root-based well-known, and _validate_resource_match then raises OAuthFlowError (or the client silently adopts the broader root PRM resource), even though the derived resource identifier is now correct. Fix: resolve dot-segments in server_url's path (e.g. reuse _remove_dot_segments / resource_url_from_server_url) before constructing the well-known discovery URLs.
Verification: pre_existing — the mechanism is real, but the base branch fails identically by the same route through untouched code. The diff (git diff 0cee624..HEAD) touches only src/mcp/shared/auth_utils.py and two test files; src/mcp/client/auth/utils.py is unchanged. At src/mcp/client/auth/utils.py:89-91, path-based PRM discovery embeds the raw, unresolved server path: `path_based_url = urljoin(base_url, f"
Sorry, something went wrong.
| configured = urlsplit(configured_resource) | ||
|
|
||
| # Compare scheme, host, and port (origin) | ||
| if requested.scheme.lower() != configured.scheme.lower() or requested.netloc.lower() != configured.netloc.lower(): |
There was a problem hiding this comment.
🟣 Pre-existing: default-port normalization asymmetry — the PRM resource side is WHATWG-normalized by pydantic (explicit :443/:80 is stripped, e.g. AnyHttpUrl("https://host:443/mcp") serializes as https://host/mcp), but resource_url_from_server_url() (netloc kept verbatim at src/mcp/shared/auth_utils.py:54) and the netloc equality check in check_resource_allowed() (line 84) keep the explicit default port from the raw server_url string. The PR's stated goal is making both operands of check_resource_allowed agree on normalization (it fixed dot-segments for exactly this reason, per the committed property test asserting agreement with pydantic's WHATWG parser), but the default-port half of WHATWG normalization is still missing, so "host:443" != "host" fails the origin…
Extended reasoning...A user configures OAuthClientProvider(server_url="https://api.example.com:443/mcp") — a spelling httpx treats as identical to the portless URL. The server's protected-resource metadata advertises resource "https://api.example.com/mcp" (or even "https://api.example.com:443/mcp" — pydantic strips the port when the client parses it either way, so str(prm.resource) is always portless). In _validate_resource_match (src/mcp/client/auth/oauth2.py:576-578), default_resource is "https://api.example.com:443/mcp" while prm_resource is "https://api.example.com/mcp"; the netloc comparison at auth_utils.py:84 returns False and the OAuth flow aborts with OAuthFlowError: Protected resource https://api.example.com/mcp does not match expected https://api.example.com:443/mcp, even though both strings name the same location. The same asymmetry makes get_resource_url() (oauth2.py:210) never adopt the PRM resource for such configs. Fix in one place: strip the scheme's default port (or otherwise WHATWG-normalize the netloc) when deriving/comparing, mirroring what was just done for dot-s
Verification: pre_existing — The asymmetry is real: check_resource_allowed compares netloc strings verbatim (src/mcp/shared/auth_utils.py:84 requested.netloc.lower() != configured.netloc.lower()) and resource_url_from_server_url keeps the netloc as written (line 54), while the PRM side is resource: AnyHttpUrl (src/mcp/shared/auth.py:243), which pydantic v2 serializes with scheme-default ports stripped. The
Sorry, something went wrong.
Reviewers
At least 1 approving review is required to merge this pull request.
Resolve dot-segments in resource_url_from_server_url() and check_resource_allowed() so the RFC 8707 resource identifier names the location the HTTP client actually contacts.
Motivation and Context
Fixes #3303.
resource_url_from_server_url() lowercased scheme/host and dropped the fragment but left the path as written, and check_resource_allowed() compared the raw paths with startswith. httpx resolves ./.. before sending, so for a server_url like https://host/a/mcp/../../b/mcp the request goes to /b/mcp while the derived resource identifier still reads /a/mcp/../../b/mcp, and a PRM resource of https://host/a/mcp prefix-matched it. The resource the client requests a token for should correspond to where it sends that token (RFC 8707 §2; RFC 9728 §3.3).
In the SDK's own call sites the server-supplied side (the PRM resource) is already normalised by pydantic, so this only changes outcomes when the client's configured URL itself contains dot-segments. It also brings the helper back in line with the TypeScript one it was ported from, where new URL() does this normalisation implicitly.
Changes:
How Has This Been Tested?
Breaking Changes
None for ordinary URLs. resource_url_from_server_url() output changes for server URLs that contain ./.. segments (they are now resolved), and check_resource_allowed() no longer ignores ;params in the configured URL's last segment.
Types of changes
Checklist
Additional context
The hierarchical prefix-matching policy itself is unchanged here. Whether the client should instead require an exact or origin-only match (as the Go and C# SDKs do, and as RFC 9728 §3.3 reads) is a separate question.
AI Disclaimer