fix(auth): normalize explicit default ports in resource_url_from_server_url (RFC 3986) - #3310
Open
teddiesloco wants to merge 2 commits into
Open
Conversation
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
- _canonical_netloc now catches ValueError from parsed.port for malformed explicit ports (e.g. out-of-range), falling back to the original netloc instead of letting check_resource_allowed() crash - add test coverage for the userinfo-in-netloc and IPv6-literal branches that were previously untested (CI requires 100% coverage) - add regression test for the malformed-port fallback
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
1 participant
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.
Summary
Fixes #3297.
resource_url_from_server_url()lowercases the scheme/authority and strips URL fragments, but previously preserved explicitly specified default ports (:80for HTTP,:443for HTTPS).Per RFC 3986 §6.2.3 (Scheme-Based Normalization), an explicit default port is equivalent to an omitted port. Furthermore, Pydantic's
HttpUrlautomatically normalizes metadata URLs by dropping default ports. Becauseresource_url_from_server_url()preserved them,check_resource_allowed()would reject an otherwise identical resource (e.g. comparinghttps://example.com:443/mcpagainsthttps://example.com/mcp), leading to false-positive authorization failures.Changes
_canonical_netloc()Helper: Normalizes netloc by stripping default port80forhttpand443forhttps, while preserving non-default ports, userinfo, and IPv6 literals.check_resource_allowed(): Canonicalizes bothrequested_resourceandconfigured_resourcebefore parsing to guarantee consistent comparison across equivalent URL representations.80and443are stripped from canonical resource URLs while non-default ports like8443or8080are preserved.check_resource_allowed()correctly evaluates matching resources regardless of explicit default port notation in either parameter.Verification
uv run pytest tests/shared/test_auth_utils.py tests/client/test_auth.py: 155 passed, 1 xfailed (pre-existing).