Validates registered redirect_uris for DCR are a secure schema with no fragments by Varshith-Kali · Pull Request #3206 · modelcontextprotocol/python-sdk

Varshith-Kali · 2026-07-28T16:42:29Z

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

Added validate_redirect_uri() to routes.py, mirroring the existing validate_issuer_url() pattern: only https:// allowed, with http:// loopback exception for localhost/127.0.0.1/[::1]
Rejects URIs with explicit or empty fragments
Called in RegistrationHandler.handle() after model_validate_json, with 400 invalid_redirect_uri response on failure
9 unit tests covering allowed HTTP loopback URIs and rejected unsafe schemes and fragments

cubic-dev-ai

All reported issues were addressed across 3 files

_{Reply with feedback, questions, or to request a fix.

Re-trigger cubic}

…o fragments

… 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)

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.

cubic-dev-ai

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}

cubic-dev-ai · 2026-07-29T18:40:46Z

+        validate_redirect_uri(AnyHttpUrl("file:///etc/passwd"))
+
+
+def test_validate_redirect_uri_http_non_loopback_allowed():


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.

cubic-dev-ai

All reported issues were addressed across 2 files (changes from recent commits).

_{Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic}

Varshith-Kali · 2026-07-31T16:36:23Z

Thanks for the review @cubic-dev-ai! Both issues found are now addressed:

P1: test_validate_redirect_uri_javascript_scheme_rejected — Fixed by switching from AnyHttpUrl to AnyUrl. The AnyHttpUrl type was rejecting non-HTTP schemes at construction time before our validation could run. Now using AnyUrl which accepts any scheme, letting us properly reject javascript:, ile:, etc. inside the validator.
P1: HTTP non-loopback accepted — This aligns with the SDK's existing test est_a_non_loopback_http_redirect_uri_is_accepted_at_registration which explicitly expects http://evil.example/callback to get 201. Our validator now rejects only truly unsafe schemes and fragments per the SDK's existing security policy.

cubic-dev-ai · 2026-07-31T16:36:54Z

Thanks for the review @cubic-dev-ai! Both issues found are now addressed:

P1: test_validate_redirect_uri_javascript_scheme_rejected — Fixed by switching from AnyHttpUrl to AnyUrl. The AnyHttpUrl type was rejecting non-HTTP schemes at construction time before our validation could run. Now using AnyUrl which accepts any scheme, letting us properly reject javascript:, ile:, etc. inside the validator.

P1: HTTP non-loopback accepted — This aligns with the SDK's existing test est_a_non_loopback_http_redirect_uri_is_accepted_at_registration which explicitly expects http://evil.example/callback to get 201. Our validator now rejects only truly unsafe schemes and fragments per the SDK's existing security policy.

@Varshith-Kali Looks good — both fixes are correctly reflected in the code I'm seeing.

1. AnyUrl instead of AnyHttpUrl — Spot on. AnyHttpUrl validates the scheme at construction time via Pydantic's internal type coercion, so AnyHttpUrl("javascript:alert(1)") raises a ValidationError before validate_redirect_uri ever runs. Switching to AnyUrl defers that to your validator, which is exactly what you want here. The test confirms it with AnyUrl("javascript:alert(1)") and the expected ValueError match.

2. Non-loopback HTTP accepted — Also consistent with what's landed. The validator's scope is now narrow: reject only non-HTTP schemes and fragments. http://evil.com/cb passes cleanly through validate_redirect_uri, matching the SDK's existing registration test. That's the right call — if the SDK already accepts those at registration, your validator shouldn't be more restrictive than the rest of the pipeline.

No new issues stand out in the current state.

_{Tip: get faster answers by chatting with cubic’s review copilot in the review UI}

cubic-dev-ai

1 issue found across 1 file (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:11">
P2: Non-HTTP(S) issuer URLs using a loopback host are now accepted. Since this signature admits `AnyUrl`, `javascript://localhost/...` and `file://localhost/...` bypass the current `scheme != "https" and host not in loopbacks` check. Keeping the loopback exception specific to the `http` scheme preserves the intended HTTPS-or-HTTP-loopback policy.</violation>
</file>

_{Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic}

cubic-dev-ai · 2026-07-31T16:40:43Z

+from pydantic import AnyUrl
+
+
+def validate_issuer_url(url: AnyUrl):


P2: Non-HTTP(S) issuer URLs using a loopback host are now accepted. Since this signature admits AnyUrl, javascript://localhost/... and file://localhost/... bypass the current scheme != "https" and host not in loopbacks check. Keeping the loopback exception specific to the http scheme preserves the intended HTTPS-or-HTTP-loopback policy.

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 11: <comment>Non-HTTP(S) issuer URLs using a loopback host are now accepted. Since this signature admits `AnyUrl`, `javascript://localhost/...` and `file://localhost/...` bypass the current `scheme != "https" and host not in loopbacks` check. Keeping the loopback exception specific to the `http` scheme preserves the intended HTTPS-or-HTTP-loopback policy.</comment> <file context> @@ -8,7 +8,7 @@ -def validate_issuer_url(url: AnyHttpUrl): +def validate_issuer_url(url: AnyUrl): """Validate that the issuer URL meets OAuth 2.0 requirements. </file context>

cubic-dev-ai Bot reviewed Jul 28, 2026

View reviewed changes

Comment thread src/mcp/server/auth/handlers/register.py Outdated

Comment thread tests/server/auth/test_routes.py Outdated

Comment thread src/mcp/server/auth/routes.py Outdated

Varshith-Kali added 2 commits July 29, 2026 20:37

Validates registered redirect_uris for DCR are a secure schema with n…

6c62757

…o fragments

Varshith-Kali force-pushed the fix/validate-redirect-uris branch from 9eb6e29 to fb893dd Compare July 29, 2026 15:07

Varshith-Kali added 3 commits July 29, 2026 20:42

Fix BOM, CRLF line endings, and escaped quotes in url_validators

9060ac0

Apply ruff formatting to test_routes.py

0561256

cubic-dev-ai Bot reviewed Jul 29, 2026

View reviewed changes

Varshith-Kali added 2 commits July 30, 2026 23:11

Fix ruff import sort in register.py

a0e41d2

cubic-dev-ai Bot reviewed Jul 30, 2026

View reviewed changes

Comment thread src/mcp/server/auth/url_validators.py

Fix validate_issuer_url type hint from AnyHttpUrl to AnyUrl

cdbc445

cubic-dev-ai Bot reviewed Jul 31, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Validates registered redirect_uris for DCR are a secure schema with no fragments - #3206

Validates registered redirect_uris for DCR are a secure schema with no fragments#3206
Varshith-Kali wants to merge 8 commits into
modelcontextprotocol:mainfrom
Varshith-Kali:fix/validate-redirect-uris

Varshith-Kali commented Jul 28, 2026

Uh oh!

cubic-dev-ai Bot left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cubic-dev-ai Bot left a comment

Uh oh!

cubic-dev-ai Bot Jul 29, 2026

Uh oh!

cubic-dev-ai Bot left a comment •

edited

Loading

Uh oh!

Uh oh!

Varshith-Kali commented Jul 31, 2026

Uh oh!

cubic-dev-ai Bot commented Jul 31, 2026

Uh oh!

cubic-dev-ai Bot left a comment

Uh oh!

cubic-dev-ai Bot Jul 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

		validate_redirect_uri(AnyHttpUrl("file:///etc/passwd"))


		def test_validate_redirect_uri_http_non_loopback_allowed():

		from pydantic import AnyUrl


		def validate_issuer_url(url: AnyUrl):

Uh oh!

Conversation

Varshith-Kali commented Jul 28, 2026

Summary

Changes

Uh oh!

cubic-dev-ai Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai Bot Jul 29, 2026

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Varshith-Kali commented Jul 31, 2026

Uh oh!

cubic-dev-ai Bot commented Jul 31, 2026

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai Bot Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

cubic-dev-ai Bot left a comment •

edited

Loading

cubic-dev-ai Bot left a comment •

edited

Loading