Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions src/mcp/server/auth/handlers/register.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

from mcp.server.auth.json_response import PydanticJSONResponse
from mcp.server.auth.provider import OAuthAuthorizationServerProvider, RegistrationError, RegistrationErrorCode
from mcp.server.auth.settings import ClientRegistrationOptions
Expand All @@ -35,6 +36,20 @@ async def handle(self, request: Request) -> Response:
body = await request.body()
client_metadata = OAuthClientMetadata.model_validate_json(body)

# Validate redirect_uris per RFC 7591 section 2
if client_metadata.redirect_uris:
for uri in client_metadata.redirect_uris:
try:
validate_redirect_uri(uri)
except ValueError as e:
return PydanticJSONResponse(
content=RegistrationErrorResponse(
error="invalid_redirect_uri",
error_description=str(e),
),
status_code=400,
)

# Scope validation is handled below
except ValidationError as validation_error:
return PydanticJSONResponse(
Expand Down
26 changes: 26 additions & 0 deletions src/mcp/server/auth/routes.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,32 @@
from mcp.shared.inbound import MCP_PROTOCOL_VERSION_HEADER


def validate_redirect_uri(url: AnyHttpUrl):
"""Validate a registered redirect_uri for DCR.

RFC 9700 section 4.1.1 and RFC 7591 section 2 require HTTPS for
redirect_uris, with an HTTP loopback exception for local development.

Args:
url: The redirect URI to validate.

Raises:
ValueError: If the redirect URI uses an unsafe scheme or contains
a fragment.
"""
if url.scheme != "https" and url.host not in (

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

"localhost",
"127.0.0.1",
"[::1]",
):
raise ValueError(
"Redirect URI must use HTTPS (or HTTP loopback for local development)"
)

if url.fragment is not None:
raise ValueError("Redirect URI must not contain a fragment")


def validate_issuer_url(url: AnyHttpUrl):
"""Validate that the issuer URL meets OAuth 2.0 requirements.

Expand Down
42 changes: 41 additions & 1 deletion tests/server/auth/test_routes.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import pytest
from pydantic import AnyHttpUrl

from mcp.server.auth.routes import build_metadata, validate_issuer_url
from mcp.server.auth.routes import build_metadata, validate_issuer_url, validate_redirect_uri
from mcp.server.auth.settings import AuthSettings, ClientRegistrationOptions, RevocationOptions


Expand Down Expand Up @@ -70,3 +70,43 @@ def test_build_metadata_serves_issuer_without_trailing_slash():
assert served["issuer"] == "https://as.example.com"
assert served["authorization_endpoint"] == "https://as.example.com/authorize"
assert served["token_endpoint"] == "https://as.example.com/token"

def test_validate_redirect_uri_https_allowed():
validate_redirect_uri(AnyHttpUrl('https://example.com/cb'))


def test_validate_redirect_uri_http_localhost_allowed():
validate_redirect_uri(AnyHttpUrl('http://localhost:3000/cb'))


def test_validate_redirect_uri_http_127_0_0_1_allowed():
validate_redirect_uri(AnyHttpUrl('http://127.0.0.1:8080/cb'))


def test_validate_redirect_uri_http_ipv6_loopback_allowed():
validate_redirect_uri(AnyHttpUrl('http://[::1]:9090/cb'))


def test_validate_redirect_uri_javascript_scheme_rejected():
with pytest.raises(ValueError, match='Redirect URI must use HTTPS'):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

validate_redirect_uri(AnyHttpUrl('javascript:alert(1)'))


def test_validate_redirect_uri_file_scheme_rejected():
with pytest.raises(ValueError, match='Redirect URI must use HTTPS'):
validate_redirect_uri(AnyHttpUrl('file:///etc/passwd'))


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_fragment_rejected():
with pytest.raises(ValueError, match='Redirect URI must not contain a fragment'):
validate_redirect_uri(AnyHttpUrl('https://example.com/cb#frag'))


def test_validate_redirect_uri_empty_fragment_rejected():
with pytest.raises(ValueError, match='Redirect URI must not contain a fragment'):
validate_redirect_uri(AnyHttpUrl('https://example.com/cb#'))
Loading