Resolve dot-segments when deriving and matching OAuth resource URLs - #3343
Draft
maxisbey wants to merge 1 commit into
Draft
Resolve dot-segments when deriving and matching OAuth resource URLs#3343maxisbey wants to merge 1 commit into
maxisbey wants to merge 1 commit into
Conversation
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolve dot-segments in
resource_url_from_server_url()andcheck_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, andcheck_resource_allowed()compared the raw paths withstartswith. httpx resolves./..before sending, so for aserver_urllikehttps://host/a/mcp/../../b/mcpthe request goes to/b/mcpwhile the derived resource identifier still reads/a/mcp/../../b/mcp, and a PRMresourceofhttps://host/a/mcpprefix-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, wherenew URL()does this normalisation implicitly.Changes:
_remove_dot_segments(): RFC 3986 §5.2.4, additionally treating%2E,.%2E,%2E.and%2E%2Eas dot-segments (the WHATWG rule, which is what pydantic applies to the PRM side).%2F, empty segments and;are left as written.resource_url_from_server_url()applies it to the path.check_resource_allowed()applies it to both paths, and parses withurlsplitinstead ofurlparseso;paramsin the last segment stay part of the path rather than being dropped before the comparison.How Has This Been Tested?
%2F////;preserved, resolution applied to both sides, plus a property test that the resolver agrees with pydantic's WHATWG parser over every combination of literal./../ empty / plain segments up to depth 4.server_url=".../victim/mcp/../../m/mcp"rejects a PRMresourceof.../victim/mcp, andget_resource_url()returns.../m/mcp.OAuthClientProviderthrough a realhttpx2.AsyncClientagainst an in-process host whose/mtenant advertisesresource=https://shared.example/victim/mcp. Before: the flow reached/authorizewithresource=https://shared.example/victim/mcp. After: it stops at PRM validation withOAuthFlowError: Protected resource https://shared.example/victim/mcp does not match expected https://shared.example/m/mcp. Origin-root and exact-match PRMs behave as before.Breaking Changes
None for ordinary URLs.
resource_url_from_server_url()output changes for server URLs that contain./..segments (they are now resolved), andcheck_resource_allowed()no longer ignores;paramsin 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