fix(auth): strip trailing slashes from OAuth metadata URLs#3013
Open
piyushbag wants to merge 2 commits into
Open
fix(auth): strip trailing slashes from OAuth metadata URLs#3013piyushbag wants to merge 2 commits into
piyushbag wants to merge 2 commits into
Conversation
Pydantic AnyHttpUrl adds a trailing slash to bare hostnames, which breaks RFC 8414/9728 exact issuer and resource comparison during OAuth discovery. Pass canonical URL strings into metadata models so served wire JSON omits the synthetic root slash. Fixes modelcontextprotocol#1919 and modelcontextprotocol#1265.
There was a problem hiding this comment.
2 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/mcp/server/auth/routes.py">
<violation number="1" location="src/mcp/server/auth/routes.py:172">
P2: Issuer canonicalization is over-broad: `rstrip('/')` also rewrites non-root path issuers (e.g. `/tenant/` -> `/tenant`), which can break exact issuer identifier matching.</violation>
<violation number="2" location="src/mcp/server/auth/routes.py:243">
P2: Protected-resource metadata now removes trailing slashes from all configured URLs, which can silently change path-based resource or authorization-server identifiers.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| resource=cast(AnyHttpUrl, str(resource_url).rstrip("/")), | ||
| authorization_servers=cast( | ||
| list[AnyHttpUrl], | ||
| [str(server).rstrip("/") for server in authorization_servers], |
There was a problem hiding this comment.
P2: Protected-resource metadata now removes trailing slashes from all configured URLs, which can silently change path-based resource or authorization-server identifiers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp/server/auth/routes.py, line 243:
<comment>Protected-resource metadata now removes trailing slashes from all configured URLs, which can silently change path-based resource or authorization-server identifiers.</comment>
<file context>
@@ -237,8 +237,11 @@ def create_protected_resource_routes(
+ resource=cast(AnyHttpUrl, str(resource_url).rstrip("/")),
+ authorization_servers=cast(
+ list[AnyHttpUrl],
+ [str(server).rstrip("/") for server in authorization_servers],
+ ),
scopes_supported=scopes_supported,
</file context>
| # Create metadata | ||
| metadata = OAuthMetadata( | ||
| issuer=issuer_url, | ||
| issuer=cast(AnyHttpUrl, str(issuer_url).rstrip("/")), |
There was a problem hiding this comment.
P2: Issuer canonicalization is over-broad: rstrip('/') also rewrites non-root path issuers (e.g. /tenant/ -> /tenant), which can break exact issuer identifier matching.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp/server/auth/routes.py, line 172:
<comment>Issuer canonicalization is over-broad: `rstrip('/')` also rewrites non-root path issuers (e.g. `/tenant/` -> `/tenant`), which can break exact issuer identifier matching.</comment>
<file context>
@@ -169,7 +169,7 @@ def build_metadata(
# Create metadata
metadata = OAuthMetadata(
- issuer=issuer_url,
+ issuer=cast(AnyHttpUrl, str(issuer_url).rstrip("/")),
authorization_endpoint=authorization_url,
token_endpoint=token_url,
</file context>
Update docs, stories, and tests that asserted trailing slashes on root issuer and authorization_server metadata fields after the routes.py fix.
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.
Summary
issuerinbuild_metadata()before constructingOAuthMetadataresourceand eachauthorization_serversentry increate_protected_resource_routes()url_preserve_empty_pathserialize without a synthetic root slashAnyHttpUrlissuer inputs and root-level protected-resource metadata responsesFixes #1919. Also fixes #1265.
Test plan
uv run pytest tests/server/auth/test_routes.py -quv run pytest tests/server/auth/test_protected_resource.py -quv run pytest tests/server/auth/ -quv run ruff check src/mcp/server/auth/routes.py tests/server/auth/test_routes.py tests/server/auth/test_protected_resource.pyuv run pyright src/mcp/server/auth/routes.py