Validates registered redirect_uris for DCR are a secure schema with no fragments - #3206
Conversation
There was a problem hiding this comment.
3 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/handlers/register.py">
<violation number="1" location="src/mcp/server/auth/handlers/register.py:12">
P1: Importing the auth routes now fails with a circular import: `routes` imports `RegistrationHandler` before it defines `validate_redirect_uri`, while this new module-level import asks for that symbol from the partially initialized `routes` module. Move the shared validator to a module that neither side imports (or otherwise break the top-level cycle) so route construction and the route tests can load.</violation>
</file>
<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>
<file name="src/mcp/server/auth/routes.py">
<violation number="1" location="src/mcp/server/auth/routes.py:37">
P1: Non-HTTPS schemes on a loopback hostname are still accepted. For example, `ftp://localhost/callback` makes both parts of this `and` false and passes validation, even though the intended exception is specifically `http` loopback. Restrict the exception to `url.scheme == "http"` as well as a loopback host.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| from starlette.responses import Response | ||
|
|
||
| from mcp.server.auth.errors import stringify_pydantic_error | ||
| from mcp.server.auth.routes import validate_redirect_uri |
There was a problem hiding this comment.
P1: Importing the auth routes now fails with a circular import: routes imports RegistrationHandler before it defines validate_redirect_uri, while this new module-level import asks for that symbol from the partially initialized routes module. Move the shared validator to a module that neither side imports (or otherwise break the top-level cycle) so route construction and the route tests can load.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp/server/auth/handlers/register.py, line 12:
<comment>Importing the auth routes now fails with a circular import: `routes` imports `RegistrationHandler` before it defines `validate_redirect_uri`, while this new module-level import asks for that symbol from the partially initialized `routes` module. Move the shared validator to a module that neither side imports (or otherwise break the top-level cycle) so route construction and the route tests can load.</comment>
<file context>
@@ -9,6 +9,7 @@
from starlette.responses import Response
from mcp.server.auth.errors import stringify_pydantic_error
+from mcp.server.auth.routes import validate_redirect_uri
from mcp.server.auth.json_response import PydanticJSONResponse
from mcp.server.auth.provider import OAuthAuthorizationServerProvider, RegistrationError, RegistrationErrorCode
</file context>
|
|
||
|
|
||
| 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>
| ValueError: If the redirect URI uses an unsafe scheme or contains | ||
| a fragment. | ||
| """ | ||
| if url.scheme != "https" and url.host not in ( |
There was a problem hiding this comment.
P1: Non-HTTPS schemes on a loopback hostname are still accepted. For example, ftp://localhost/callback makes both parts of this and false and passes validation, even though the intended exception is specifically http loopback. Restrict the exception to url.scheme == "http" as well as a loopback host.
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 37:
<comment>Non-HTTPS schemes on a loopback hostname are still accepted. For example, `ftp://localhost/callback` makes both parts of this `and` false and passes validation, even though the intended exception is specifically `http` loopback. Restrict the exception to `url.scheme == "http"` as well as a loopback host.</comment>
<file context>
@@ -21,6 +21,32 @@
+ ValueError: If the redirect URI uses an unsafe scheme or contains
+ a fragment.
+ """
+ if url.scheme != "https" and url.host not in (
+ "localhost",
+ "127.0.0.1",
</file context>
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