Skip to content

Commit 759c9be

Browse files
committed
fix(auth): reject non-spec redirect_uris in DCR handler
Extracts validate_registered_redirect_uri to mcp.server.auth._redirect_uri to avoid a circular import with handlers.register; re-exports from routes to preserve the public test import path.
1 parent 35df944 commit 759c9be

4 files changed

Lines changed: 152 additions & 30 deletions

File tree

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
"""Internal helpers for validating registered OAuth redirect URIs.
2+
3+
Lives outside :mod:`mcp.server.auth.routes` to avoid a circular import:
4+
``routes`` imports :class:`~mcp.server.auth.handlers.register.RegistrationHandler`,
5+
and the handler in turn needs the redirect-URI validator, so the validator
6+
must sit in a module that neither side has to depend on transitively.
7+
:mod:`mcp.server.auth.routes` re-exports :func:`validate_registered_redirect_uri`
8+
so callers (including tests) keep the public import path.
9+
"""
10+
11+
from pydantic import AnyUrl
12+
13+
from mcp.shared.auth import InvalidRedirectUriError
14+
15+
16+
def validate_registered_redirect_uri(url: AnyUrl) -> None:
17+
"""Validate that a registered redirect_uri meets OAuth 2.0 + RFC 7591 requirements.
18+
19+
Mirrors the policy that :func:`mcp.server.auth.routes.validate_issuer_url`
20+
applies to issuer URLs: redirect URIs must use ``https``, except that
21+
``http`` is permitted for loopback hosts (``localhost``, ``127.0.0.1``,
22+
``[::1]``) per RFC 8252 §7.3, and they MUST NOT carry a fragment component
23+
per RFC 7591 §2.
24+
25+
Args:
26+
url: A registered redirect_uri value from
27+
:class:`mcp.shared.auth.OAuthClientMetadata`.
28+
29+
Raises:
30+
InvalidRedirectUriError: If the URI uses a scheme other than ``https``
31+
or loopback ``http``, or if it contains a fragment.
32+
"""
33+
# RFC 9700 §4.1.1 (OAuth 2.0 Security BCP): https-only, with the RFC 8252
34+
# native-app loopback exception.
35+
if url.scheme not in ("https", "http"):
36+
raise InvalidRedirectUriError(f"redirect_uri must use https (or http for loopback); got scheme {url.scheme!r}")
37+
if url.scheme == "http" and url.host not in ("localhost", "127.0.0.1", "[::1]"):
38+
raise InvalidRedirectUriError(f"redirect_uri must use https for non-loopback hosts; got {str(url)!r}")
39+
# RFC 7591 §2: redirect_uri MUST NOT contain a fragment component.
40+
if url.fragment is not None:
41+
raise InvalidRedirectUriError(f"redirect_uri must not have a fragment; got {str(url)!r}")

src/mcp/server/auth/handlers/register.py

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,12 @@
88
from starlette.requests import Request
99
from starlette.responses import Response
1010

11+
from mcp.server.auth._redirect_uri import validate_registered_redirect_uri
1112
from mcp.server.auth.errors import stringify_pydantic_error
1213
from mcp.server.auth.json_response import PydanticJSONResponse
1314
from mcp.server.auth.provider import OAuthAuthorizationServerProvider, RegistrationError, RegistrationErrorCode
1415
from mcp.server.auth.settings import ClientRegistrationOptions
15-
from mcp.shared.auth import OAuthClientInformationFull, OAuthClientMetadata
16+
from mcp.shared.auth import InvalidRedirectUriError, OAuthClientInformationFull, OAuthClientMetadata
1617

1718
# this alias is a no-op; it's just to separate out the types exposed to the
1819
# provider from what we use in the HTTP handler
@@ -45,6 +46,21 @@ async def handle(self, request: Request) -> Response:
4546
status_code=400,
4647
)
4748

49+
# RFC 9700 §4.1.1 + RFC 7591 §2: Pydantic AnyUrl accepts non-OAuth
50+
# schemes (javascript:, data:, file:, etc.) and URIs with fragments;
51+
# reject those here per spec.
52+
for uri in client_metadata.redirect_uris or []:
53+
try:
54+
validate_registered_redirect_uri(uri)
55+
except InvalidRedirectUriError as e:
56+
return PydanticJSONResponse(
57+
content=RegistrationErrorResponse(
58+
error="invalid_redirect_uri",
59+
error_description=e.message,
60+
),
61+
status_code=400,
62+
)
63+
4864
client_id = str(uuid4())
4965

5066
# If auth method is None, default to client_secret_post

src/mcp/server/auth/routes.py

Lines changed: 8 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,14 @@
22
from typing import Any
33
from urllib.parse import urlparse
44

5-
from pydantic import AnyHttpUrl, AnyUrl
5+
from pydantic import AnyHttpUrl
66
from starlette.middleware.cors import CORSMiddleware
77
from starlette.requests import Request
88
from starlette.responses import Response
99
from starlette.routing import Route, request_response # type: ignore
1010
from starlette.types import ASGIApp
1111

12+
from mcp.server.auth._redirect_uri import validate_registered_redirect_uri
1213
from mcp.server.auth.handlers.authorize import AuthorizationHandler
1314
from mcp.server.auth.handlers.metadata import MetadataHandler, ProtectedResourceMetadataHandler
1415
from mcp.server.auth.handlers.register import RegistrationHandler
@@ -18,7 +19,12 @@
1819
from mcp.server.auth.provider import OAuthAuthorizationServerProvider
1920
from mcp.server.auth.settings import ClientRegistrationOptions, RevocationOptions
2021
from mcp.server.streamable_http import MCP_PROTOCOL_VERSION_HEADER
21-
from mcp.shared.auth import InvalidRedirectUriError, OAuthMetadata, ProtectedResourceMetadata
22+
from mcp.shared.auth import OAuthMetadata, ProtectedResourceMetadata
23+
24+
# Re-exported from ._redirect_uri to avoid a circular import with
25+
# .handlers.register, which also needs this validator. External callers and
26+
# tests should keep importing it from this module.
27+
__all__ = ["validate_registered_redirect_uri"]
2228

2329

2430
def validate_issuer_url(url: AnyHttpUrl):
@@ -42,33 +48,6 @@ def validate_issuer_url(url: AnyHttpUrl):
4248
raise ValueError("Issuer URL must not have a query string")
4349

4450

45-
def validate_registered_redirect_uri(url: AnyUrl) -> None:
46-
"""Validate that a registered redirect_uri meets OAuth 2.0 + RFC 7591 requirements.
47-
48-
Mirrors the policy that :func:`validate_issuer_url` applies to issuer URLs:
49-
redirect URIs must use ``https``, except that ``http`` is permitted for
50-
loopback hosts (``localhost``, ``127.0.0.1``, ``[::1]``) per RFC 8252 §7.3,
51-
and they MUST NOT carry a fragment component per RFC 7591 §2.
52-
53-
Args:
54-
url: A registered redirect_uri value from
55-
:class:`mcp.shared.auth.OAuthClientMetadata`.
56-
57-
Raises:
58-
InvalidRedirectUriError: If the URI uses a scheme other than ``https``
59-
or loopback ``http``, or if it contains a fragment.
60-
"""
61-
# RFC 9700 §4.1.1 (OAuth 2.0 Security BCP): https-only, with the RFC 8252
62-
# native-app loopback exception.
63-
if url.scheme not in ("https", "http"):
64-
raise InvalidRedirectUriError(f"redirect_uri must use https (or http for loopback); got scheme {url.scheme!r}")
65-
if url.scheme == "http" and url.host not in ("localhost", "127.0.0.1", "[::1]"):
66-
raise InvalidRedirectUriError(f"redirect_uri must use https for non-loopback hosts; got {str(url)!r}")
67-
# RFC 7591 §2: redirect_uri MUST NOT contain a fragment component.
68-
if url.fragment is not None:
69-
raise InvalidRedirectUriError(f"redirect_uri must not have a fragment; got {str(url)!r}")
70-
71-
7251
AUTHORIZATION_PATH = "/authorize"
7352
TOKEN_PATH = "/token"
7453
REGISTRATION_PATH = "/register"

tests/server/auth/test_error_handling.py

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -288,3 +288,89 @@ async def test_token_error_handling_refresh_token(
288288
data = refresh_response.json()
289289
assert data["error"] == "invalid_scope"
290290
assert data["error_description"] == "The requested scope is invalid"
291+
292+
293+
@pytest.mark.anyio
294+
async def test_register_rejects_javascript_scheme(client: httpx.AsyncClient):
295+
"""Registration must reject ``javascript:`` redirect URIs per RFC 9700 §4.1.1."""
296+
response = await client.post(
297+
"/register",
298+
json={
299+
"redirect_uris": ["javascript:alert(1)"],
300+
"token_endpoint_auth_method": "none",
301+
"grant_types": ["authorization_code"],
302+
"response_types": ["code"],
303+
},
304+
)
305+
assert response.status_code == 400, response.content
306+
data = response.json()
307+
assert data["error"] == "invalid_redirect_uri"
308+
assert "https" in data["error_description"]
309+
310+
311+
@pytest.mark.anyio
312+
async def test_register_rejects_cleartext_http_non_loopback(client: httpx.AsyncClient):
313+
"""Registration must reject cleartext http for non-loopback hosts per RFC 8252 §7.3."""
314+
response = await client.post(
315+
"/register",
316+
json={
317+
"redirect_uris": ["http://attacker.example/cb"],
318+
"token_endpoint_auth_method": "none",
319+
"grant_types": ["authorization_code"],
320+
"response_types": ["code"],
321+
},
322+
)
323+
assert response.status_code == 400, response.content
324+
data = response.json()
325+
assert data["error"] == "invalid_redirect_uri"
326+
assert "non-loopback" in data["error_description"]
327+
328+
329+
@pytest.mark.anyio
330+
async def test_register_rejects_fragment(client: httpx.AsyncClient):
331+
"""Registration must reject redirect URIs that include a fragment per RFC 7591 §2."""
332+
response = await client.post(
333+
"/register",
334+
json={
335+
"redirect_uris": ["https://example.com/cb#frag"],
336+
"token_endpoint_auth_method": "none",
337+
"grant_types": ["authorization_code"],
338+
"response_types": ["code"],
339+
},
340+
)
341+
assert response.status_code == 400, response.content
342+
data = response.json()
343+
assert data["error"] == "invalid_redirect_uri"
344+
assert "fragment" in data["error_description"]
345+
346+
347+
@pytest.mark.anyio
348+
async def test_register_accepts_https_redirect_uri(client: httpx.AsyncClient):
349+
"""Registration must accept https redirect URIs."""
350+
response = await client.post(
351+
"/register",
352+
json={
353+
"redirect_uris": ["https://example.com/cb"],
354+
"token_endpoint_auth_method": "none",
355+
"grant_types": ["authorization_code"],
356+
"response_types": ["code"],
357+
},
358+
)
359+
assert response.status_code == 201, response.content
360+
data = response.json()
361+
assert "client_id" in data
362+
363+
364+
@pytest.mark.anyio
365+
async def test_register_accepts_loopback_redirect_uri(client: httpx.AsyncClient):
366+
"""Registration must accept http://localhost loopback redirect URIs per RFC 8252 §7.3."""
367+
response = await client.post(
368+
"/register",
369+
json={
370+
"redirect_uris": ["http://localhost:8080/cb"],
371+
"token_endpoint_auth_method": "none",
372+
"grant_types": ["authorization_code"],
373+
"response_types": ["code"],
374+
},
375+
)
376+
assert response.status_code == 201, response.content

0 commit comments

Comments
 (0)