Validates registered redirect_uris for DCR are a secure schema with no fragments - #3206
Open
Varshith-Kali wants to merge 7 commits into
Open
Validates registered redirect_uris for DCR are a secure schema with no fragments#3206Varshith-Kali wants to merge 7 commits into
Varshith-Kali wants to merge 7 commits into
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
… module The original implementation put validate_redirect_uri in routes.py, which caused a circular import: register.py imports from routes, and routes imports from other modules that import back. Moving the validation functions to a dedicated url_validators.py module breaks the cycle. - validate_redirect_uri now lives in url_validators.py alongside validate_issuer_url (previously in routes.py) - register.py imports from url_validators instead of routes - test_routes.py updated to import from url_validators - Ruff format fixes applied (single-line raise, double-quote match strings)
Varshith-Kali
force-pushed
the
fix/validate-redirect-uris
branch
from
July 29, 2026 15:07
9eb6e29 to
fb893dd
Compare
The SDK intentionally accepts non-loopback HTTP redirect URIs per existing tests (test_a_non_loopback_http_redirect_uri_is_accepted). Narrow scope to only reject dangerous schemes (javascript:, data:, file:, etc.) and fragments, matching the SDK's existing policy.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
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="tests/server/auth/test_routes.py">
<violation number="1" location="tests/server/auth/test_routes.py:102">
P1: validate_redirect_uri accepts http:// for any host, not just loopback. The docstring promises RFC 9700 HTTPS enforcement with loopback exception, but only non-HTTP(S) schemes are rejected — http://evil.com/cb passes. This exposes authorization codes to interception over plain HTTP for non-loopback redirect URIs.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| validate_redirect_uri(AnyHttpUrl("file:///etc/passwd")) | ||
|
|
||
|
|
||
| def test_validate_redirect_uri_http_non_loopback_allowed(): |
There was a problem hiding this comment.
P1: validate_redirect_uri accepts http:// for any host, not just loopback. The docstring promises RFC 9700 HTTPS enforcement with loopback exception, but only non-HTTP(S) schemes are rejected — http://evil.com/cb passes. This exposes authorization codes to interception over plain HTTP for non-loopback redirect URIs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/server/auth/test_routes.py, line 102:
<comment>validate_redirect_uri accepts http:// for any host, not just loopback. The docstring promises RFC 9700 HTTPS enforcement with loopback exception, but only non-HTTP(S) schemes are rejected — http://evil.com/cb passes. This exposes authorization codes to interception over plain HTTP for non-loopback redirect URIs.</comment>
<file context>
@@ -99,9 +99,8 @@ def test_validate_redirect_uri_file_scheme_rejected():
-def test_validate_redirect_uri_http_non_loopback_rejected():
- with pytest.raises(ValueError, match="Redirect URI must use HTTPS"):
- validate_redirect_uri(AnyHttpUrl("http://evil.com/cb"))
+def test_validate_redirect_uri_http_non_loopback_allowed():
+ validate_redirect_uri(AnyHttpUrl("http://evil.com/cb"))
</file context>
AnyHttpUrl rejects non-HTTP schemes (javascript:, file:, etc.) at construction time, preventing validate_redirect_uri from ever being called. Switch to AnyUrl which accepts any scheme string, then reject unsafe schemes inside the validator. Also update error match pattern in tests from 'must use HTTPS' to 'must use an HTTP' to match the updated validator message.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
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/url_validators.py">
<violation number="1" location="src/mcp/server/auth/url_validators.py:8">
P0: Importing the auth URL validators now fails at module initialization because `validate_issuer_url` still evaluates an undefined `AnyHttpUrl` annotation. Keep `AnyHttpUrl` imported alongside `AnyUrl` so the existing issuer validator and all auth imports remain usable.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| development. | ||
| """ | ||
|
|
||
| from pydantic import AnyUrl |
There was a problem hiding this comment.
P0: Importing the auth URL validators now fails at module initialization because validate_issuer_url still evaluates an undefined AnyHttpUrl annotation. Keep AnyHttpUrl imported alongside AnyUrl so the existing issuer validator and all auth imports remain usable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp/server/auth/url_validators.py, line 8:
<comment>Importing the auth URL validators now fails at module initialization because `validate_issuer_url` still evaluates an undefined `AnyHttpUrl` annotation. Keep `AnyHttpUrl` imported alongside `AnyUrl` so the existing issuer validator and all auth imports remain usable.</comment>
<file context>
@@ -5,7 +5,7 @@
"""
-from pydantic import AnyHttpUrl
+from pydantic import AnyUrl
</file context>
Suggested change
| from pydantic import AnyUrl | |
| from pydantic import AnyHttpUrl, AnyUrl |
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.
Fixes #2629.
Summary
Dynamic Client Registration currently accepts redirect_uris with unsafe schemes (javascript:, data:, file:, ftp:) and fragments without complaint. RFC 9700 .1.1 and RFC 7591 require HTTPS for redirect_uris and prohibit fragments.
Changes