Validates registered redirect_uris for DCR are a secure schema with no fragments - #3206
Open
Varshith-Kali wants to merge 5 commits into
Open
Validates registered redirect_uris for DCR are a secure schema with no fragments#3206Varshith-Kali wants to merge 5 commits into
Varshith-Kali wants to merge 5 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.
1 issue 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="tests/server/auth/test_routes.py">
<violation number="1" location="tests/server/auth/test_routes.py:91">
P1: test_validate_redirect_uri_javascript_scheme_rejected will fail: AnyHttpUrl('javascript:alert(1)') raises a pydantic ValidationError at construction time — before validate_redirect_uri is even called — with message "URL scheme should be 'http' or 'https'", which does not match the expected pattern 'Redirect URI must use HTTPS'. Either pass a mock or skip the AnyHttpUrl wrapper for non-HTTP scheme inputs.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
|
|
||
| def test_validate_redirect_uri_javascript_scheme_rejected(): | ||
| with pytest.raises(ValueError, match='Redirect URI must use HTTPS'): |
There was a problem hiding this comment.
P1: test_validate_redirect_uri_javascript_scheme_rejected will fail: AnyHttpUrl('javascript:alert(1)') raises a pydantic ValidationError at construction time — before validate_redirect_uri is even called — with message "URL scheme should be 'http' or 'https'", which does not match the expected pattern 'Redirect URI must use HTTPS'. Either pass a mock or skip the AnyHttpUrl wrapper for non-HTTP scheme inputs.
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 91:
<comment>test_validate_redirect_uri_javascript_scheme_rejected will fail: AnyHttpUrl('javascript:alert(1)') raises a pydantic ValidationError at construction time — before validate_redirect_uri is even called — with message "URL scheme should be 'http' or 'https'", which does not match the expected pattern 'Redirect URI must use HTTPS'. Either pass a mock or skip the AnyHttpUrl wrapper for non-HTTP scheme inputs.</comment>
<file context>
@@ -70,3 +70,43 @@ def test_build_metadata_serves_issuer_without_trailing_slash():
+
+
+def test_validate_redirect_uri_javascript_scheme_rejected():
+ with pytest.raises(ValueError, match='Redirect URI must use HTTPS'):
+ validate_redirect_uri(AnyHttpUrl('javascript:alert(1)'))
+
</file context>
… 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>
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