From 67527c1f694f2570f2aeb03983d31d76fbb5d5bf Mon Sep 17 00:00:00 2001 From: Jeremiah Lowin <153965+jlowin@users.noreply.github.com> Date: Sun, 5 Jul 2026 14:16:10 -0700 Subject: [PATCH] Block unsafe OAuth redirect schemes (#4419) --- docs/servers/auth/oauth-proxy.mdx | 5 +- docs/servers/auth/oidc-proxy.mdx | 2 +- docs/servers/auth/remote-oauth.mdx | 6 +- .../server/auth/oauth_proxy/consent.py | 33 ++++++++++ .../fastmcp/server/auth/oauth_proxy/models.py | 25 ++++---- .../fastmcp/server/auth/oauth_proxy/proxy.py | 47 +++++++++++++- .../fastmcp/server/auth/oidc_proxy.py | 2 +- .../server/auth/redirect_validation.py | 31 ++++++++- .../oauth_proxy/test_client_registration.py | 63 ++++++++++++++++++- .../auth/oauth_proxy/test_oauth_proxy.py | 34 ++++++++++ tests/server/auth/oauth_proxy/test_tokens.py | 45 +++++++++++++ tests/server/auth/test_oauth_consent_page.py | 33 ++++++++++ .../test_oauth_proxy_redirect_validation.py | 11 ++++ tests/server/auth/test_redirect_validation.py | 52 ++++++++++++--- 14 files changed, 354 insertions(+), 35 deletions(-) diff --git a/docs/servers/auth/oauth-proxy.mdx b/docs/servers/auth/oauth-proxy.mdx index 1f28c5de6..e35f244eb 100644 --- a/docs/servers/auth/oauth-proxy.mdx +++ b/docs/servers/auth/oauth-proxy.mdx @@ -194,7 +194,8 @@ mcp = FastMCP(name="My Server", auth=auth) List of allowed redirect URI patterns for MCP clients. Patterns support wildcards (e.g., `"http://localhost:*"`, `"https://*.example.com/*"`). - `None` (default): DCR clients use registered redirect URIs, with loopback - ports allowed to vary for MCP compatibility + ports allowed to vary for MCP compatibility. Unsafe browser schemes such as + `javascript:`, `data:`, `file:`, and `vbscript:` are rejected. - Empty list `[]`: No redirect URIs allowed - Custom list: Only matching patterns allowed @@ -559,7 +560,7 @@ auth = OAuthProxy( ### Redirect URI Validation -By default, the OAuth proxy validates DCR clients against their registered redirect URIs while allowing loopback ports to vary for MCP compatibility. You can restrict which clients can connect at the server level by specifying allowed patterns: +By default, the OAuth proxy validates DCR clients against their registered redirect URIs while allowing loopback ports to vary for MCP compatibility. Unsafe browser schemes such as `javascript:` are always rejected. You can restrict which clients can connect at the server level by specifying allowed patterns: ```python # Allow only localhost clients (common for development) diff --git a/docs/servers/auth/oidc-proxy.mdx b/docs/servers/auth/oidc-proxy.mdx index 08f349246..fde747e2b 100644 --- a/docs/servers/auth/oidc-proxy.mdx +++ b/docs/servers/auth/oidc-proxy.mdx @@ -124,7 +124,7 @@ mcp = FastMCP(name="My Server", auth=auth) List of allowed redirect URI patterns for MCP clients. Patterns support wildcards (e.g., `"http://localhost:*"`, `"https://*.example.com/*"`). - - `None` (default): DCR clients use registered redirect URIs, with loopback ports allowed to vary for MCP compatibility + - `None` (default): DCR clients use registered redirect URIs, with loopback ports allowed to vary for MCP compatibility. Unsafe browser schemes such as `javascript:`, `data:`, `file:`, and `vbscript:` are rejected. - Empty list `[]`: No redirect URIs allowed - Custom list: Only matching patterns allowed diff --git a/docs/servers/auth/remote-oauth.mdx b/docs/servers/auth/remote-oauth.mdx index 7c94a937f..c2256b052 100644 --- a/docs/servers/auth/remote-oauth.mdx +++ b/docs/servers/auth/remote-oauth.mdx @@ -116,7 +116,7 @@ auth = RemoteAuthProvider( token_verifier=token_verifier, authorization_servers=[AnyHttpUrl("https://auth.yourcompany.com")], base_url="https://api.yourcompany.com", # Your server base URL - # Optional: restrict allowed client redirect URIs (defaults to all for DCR compatibility) + # Optional: restrict allowed client redirect URIs allowed_client_redirect_uris=["http://localhost:*", "http://127.0.0.1:*"] ) @@ -218,7 +218,7 @@ WorkOS's support for Dynamic Client Registration makes it particularly well-suit `RemoteAuthProvider` also supports the `allowed_client_redirect_uris` parameter for controlling which redirect URIs are accepted from MCP clients during DCR: -- `None` (default): All redirect URIs allowed (for DCR compatibility) +- `None` (default): Broad DCR-compatible redirect support, while rejecting unsafe browser schemes such as `javascript:`, `data:`, `file:`, and `vbscript:` - Custom list: Specify allowed patterns with wildcard support - Empty list `[]`: No redirect URIs allowed @@ -237,4 +237,4 @@ Remote OAuth integration requires careful attention to several technical details **Scope Management**: Map token scopes to your application's permission model consistently. Consider how scope changes affect existing tokens and plan for smooth permission updates. -The complexity of these considerations reinforces why external identity providers are recommended over custom OAuth implementations. Established providers handle these technical details with extensive testing and operational experience. \ No newline at end of file +The complexity of these considerations reinforces why external identity providers are recommended over custom OAuth implementations. Established providers handle these technical details with extensive testing and operational experience. diff --git a/fastmcp_slim/fastmcp/server/auth/oauth_proxy/consent.py b/fastmcp_slim/fastmcp/server/auth/oauth_proxy/consent.py index 68551f7cc..c25194936 100644 --- a/fastmcp_slim/fastmcp/server/auth/oauth_proxy/consent.py +++ b/fastmcp_slim/fastmcp/server/auth/oauth_proxy/consent.py @@ -23,6 +23,7 @@ from starlette.responses import HTMLResponse, RedirectResponse from fastmcp.server.auth.oauth_proxy.models import ProxyDCRClient from fastmcp.server.auth.oauth_proxy.ui import create_consent_html +from fastmcp.server.auth.redirect_validation import validate_redirect_uri from fastmcp.utilities.logging import get_logger from fastmcp.utilities.ui import create_secure_html_response @@ -60,6 +61,16 @@ class ConsentMixin: normalized = self._normalize_uri(str(redirect_uri)) return f"{client_id}:{normalized}" + def _validate_client_redirect_uri( + self: OAuthProxy, + redirect_uri: str, + ) -> bool: + """Validate a stored transaction redirect URI before sending a browser to it.""" + return validate_redirect_uri( + redirect_uri=redirect_uri, + allowed_patterns=self._allowed_client_redirect_uris, + ) + def _cookie_name(self: OAuthProxy, base_name: str) -> str: """Return secure cookie name for HTTPS, fallback for HTTP development.""" if self._is_https: @@ -361,6 +372,18 @@ class ConsentMixin: return response if client_key in denied: + if not self._validate_client_redirect_uri( + txn["client_redirect_uri"] + ): + logger.warning( + "Blocked consent denial redirect to disallowed URI for transaction %s", + txn_id, + ) + return create_secure_html_response( + "

Error

Invalid redirect URI

", + status_code=400, + ) + callback_params = { "error": "access_denied", "state": txn.get("client_state") or "", @@ -526,6 +549,16 @@ class ConsentMixin: return response elif action == "deny": + if not self._validate_client_redirect_uri(txn["client_redirect_uri"]): + logger.warning( + "Blocked consent denial redirect to disallowed URI for transaction %s", + txn_id, + ) + return create_secure_html_response( + "

Error

Invalid redirect URI

", + status_code=400, + ) + callback_params = { "error": "access_denied", "state": txn.get("client_state") or "", diff --git a/fastmcp_slim/fastmcp/server/auth/oauth_proxy/models.py b/fastmcp_slim/fastmcp/server/auth/oauth_proxy/models.py index 01a5969e5..7668d98fd 100644 --- a/fastmcp_slim/fastmcp/server/auth/oauth_proxy/models.py +++ b/fastmcp_slim/fastmcp/server/auth/oauth_proxy/models.py @@ -243,14 +243,9 @@ class ProxyDCRClient(OAuthClientInformationFull): f"Invalid CIMD redirect_uri: {e}" ) from e - # Respect proxy-level redirect URI restrictions even when the - # client omits redirect_uri and we fall back to CIMD defaults. - if ( - self.allowed_redirect_uri_patterns is not None - and not validate_redirect_uri( - redirect_uri=resolved, - allowed_patterns=self.allowed_redirect_uri_patterns, - ) + if not validate_redirect_uri( + redirect_uri=resolved, + allowed_patterns=self.allowed_redirect_uri_patterns, ): raise InvalidRedirectUriError( f"Redirect URI '{resolved}' does not match allowed patterns." @@ -263,6 +258,11 @@ class ProxyDCRClient(OAuthClientInformationFull): ) if redirect_uri is not None: + if not validate_redirect_uri(redirect_uri, None): + raise InvalidRedirectUriError( + f"Redirect URI '{redirect_uri}' uses an unsafe scheme." + ) + cimd_redirect_uris = ( self.cimd_document.redirect_uris if self.cimd_document else None ) @@ -312,9 +312,8 @@ class ProxyDCRClient(OAuthClientInformationFull): # (handles the single-registered-URI shortcut for DCR clients), then validate # the resolved URI against patterns so [] and other restrictions are enforced. resolved = super().validate_redirect_uri(redirect_uri) - if self.allowed_redirect_uri_patterns is not None: - if not validate_redirect_uri(resolved, self.allowed_redirect_uri_patterns): - raise InvalidRedirectUriError( - f"Redirect URI '{resolved}' does not match allowed patterns." - ) + if not validate_redirect_uri(resolved, self.allowed_redirect_uri_patterns): + raise InvalidRedirectUriError( + f"Redirect URI '{resolved}' does not match allowed patterns." + ) return resolved diff --git a/fastmcp_slim/fastmcp/server/auth/oauth_proxy/proxy.py b/fastmcp_slim/fastmcp/server/auth/oauth_proxy/proxy.py index a6296a0c3..33eea92cb 100644 --- a/fastmcp_slim/fastmcp/server/auth/oauth_proxy/proxy.py +++ b/fastmcp_slim/fastmcp/server/auth/oauth_proxy/proxy.py @@ -48,6 +48,7 @@ from mcp.server.auth.provider import ( AuthorizationParams, AuthorizeError, RefreshToken, + RegistrationError, TokenError, ) from mcp.server.auth.routes import build_metadata, cors_middleware @@ -91,6 +92,7 @@ from fastmcp.server.auth.oauth_proxy.models import ( _hash_token, ) from fastmcp.server.auth.oauth_proxy.ui import create_error_html +from fastmcp.server.auth.redirect_validation import validate_redirect_uri from fastmcp.utilities.auth import parse_scopes from fastmcp.utilities.logging import get_logger @@ -301,7 +303,7 @@ class OAuthProxy(OAuthProvider, ConsentMixin): allowed_client_redirect_uris: List of allowed redirect URI patterns for MCP clients. Patterns support wildcards (e.g., "http://localhost:*", "https://*.example.com/*"). If None (default), DCR clients use registered redirect URIs, with loopback - ports allowed to vary for MCP compatibility. + ports allowed to vary for MCP compatibility. Unsafe browser schemes are rejected. If empty list, no redirect URIs are allowed. These are for MCP clients performing loopback redirects, NOT for the upstream OAuth app. valid_scopes: List of all the possible valid scopes for a client. @@ -839,6 +841,19 @@ class OAuthProxy(OAuthProvider, ConsentMixin): # Create a ProxyDCRClient with configured redirect URI validation if client_info.client_id is None: raise ValueError("client_id is required for client registration") + if client_info.redirect_uris: + for redirect_uri in client_info.redirect_uris: + if not validate_redirect_uri( + redirect_uri=redirect_uri, + allowed_patterns=self._allowed_client_redirect_uris, + ): + raise RegistrationError( + "invalid_redirect_uri", + f"Redirect URI '{redirect_uri}' is not allowed.", + ) + + redirect_uris = client_info.redirect_uris or [AnyUrl("http://localhost")] + # We use token_endpoint_auth_method="none" because the proxy handles # all upstream authentication. The client_secret must also be None # because the SDK requires secrets to be provided if they're set, @@ -846,7 +861,7 @@ class OAuthProxy(OAuthProvider, ConsentMixin): proxy_client: ProxyDCRClient = ProxyDCRClient( client_id=client_info.client_id, client_secret=None, - redirect_uris=client_info.redirect_uris or [AnyUrl("http://localhost")], + redirect_uris=redirect_uris, grant_types=client_info.grant_types or ["authorization_code", "refresh_token"], scope=client_info.scope or self._default_scope_str, @@ -2160,13 +2175,25 @@ class OAuthProxy(OAuthProvider, ConsentMixin): ) if transaction_model: # Forward the error to the client's redirect_uri (RFC 6749 §4.1.2.1) + client_redirect_uri = transaction_model.client_redirect_uri + if not self._validate_client_redirect_uri(client_redirect_uri): + logger.warning( + "Blocked IdP callback error redirect to disallowed URI " + "for transaction %s", + txn_id, + ) + html_content = create_error_html( + error_title="OAuth Error", + error_message="Invalid redirect URI", + ) + return HTMLResponse(content=html_content, status_code=400) + error_params: dict[str, str] = { "error": error, "state": transaction_model.client_state, } if error_description: error_params["error_description"] = error_description - client_redirect_uri = transaction_model.client_redirect_uri separator = "&" if "?" in client_redirect_uri else "?" return RedirectResponse( url=f"{client_redirect_uri}{separator}{urlencode(error_params)}", @@ -2186,6 +2213,20 @@ class OAuthProxy(OAuthProvider, ConsentMixin): error_message="Invalid or expired authorization transaction. Please try authenticating again.", ) return HTMLResponse(content=html_content, status_code=400) + + if not self._validate_client_redirect_uri( + transaction_model.client_redirect_uri + ): + logger.warning( + "Blocked IdP callback redirect to disallowed URI for transaction %s", + txn_id, + ) + html_content = create_error_html( + error_title="OAuth Error", + error_message="Invalid redirect URI", + ) + return HTMLResponse(content=html_content, status_code=400) + # Verify consent binding cookie to prevent confused deputy attacks. # When consent is enabled, the browser that approved consent receives # a signed cookie. A different browser (e.g., a victim lured to the diff --git a/fastmcp_slim/fastmcp/server/auth/oidc_proxy.py b/fastmcp_slim/fastmcp/server/auth/oidc_proxy.py index 7757dc546..ac7bfaed5 100644 --- a/fastmcp_slim/fastmcp/server/auth/oidc_proxy.py +++ b/fastmcp_slim/fastmcp/server/auth/oidc_proxy.py @@ -278,7 +278,7 @@ class OIDCProxy(OAuthProxy): allowed_client_redirect_uris: List of allowed redirect URI patterns for MCP clients. Patterns support wildcards (e.g., "http://localhost:*", "https://*.example.com/*"). If None (default), DCR clients use registered redirect URIs, with loopback - ports allowed to vary for MCP compatibility. + ports allowed to vary for MCP compatibility. Unsafe browser schemes are rejected. If empty list, no redirect URIs are allowed. These are for MCP clients performing loopback redirects, NOT for the upstream OAuth app. client_storage: Storage backend for OAuth state (client registrations, encrypted tokens). diff --git a/fastmcp_slim/fastmcp/server/auth/redirect_validation.py b/fastmcp_slim/fastmcp/server/auth/redirect_validation.py index 678125765..cdc62cc4f 100644 --- a/fastmcp_slim/fastmcp/server/auth/redirect_validation.py +++ b/fastmcp_slim/fastmcp/server/auth/redirect_validation.py @@ -9,6 +9,15 @@ from urllib.parse import unquote, urlparse from pydantic import AnyUrl +UNSAFE_REDIRECT_URI_SCHEMES = frozenset( + { + "javascript", + "data", + "file", + "vbscript", + } +) + def _parse_host_port(netloc: str) -> tuple[str | None, str | None]: """Parse host and port from netloc, handling wildcards. @@ -144,6 +153,15 @@ def _match_path(uri_path: str, pattern_path: str) -> bool: return fnmatch.fnmatch(uri_path, pattern_path) +def _is_unsafe_redirect_uri(uri: str) -> bool: + try: + parsed = urlparse(uri) + except ValueError: + return True + + return parsed.scheme.lower() in UNSAFE_REDIRECT_URI_SCHEMES + + def matches_allowed_pattern(uri: str, pattern: str) -> bool: """Securely check if a URI matches an allowed pattern with wildcard support. @@ -172,6 +190,9 @@ def matches_allowed_pattern(uri: str, pattern: str) -> bool: except ValueError: return False + if uri_parsed.scheme.lower() in UNSAFE_REDIRECT_URI_SCHEMES: + return False + # SECURITY: Reject URIs with userinfo (user:pass@host) # This prevents bypass attacks like http://localhost@evil.com/callback # which would match http://localhost:* with naive fnmatch @@ -216,7 +237,8 @@ def validate_redirect_uri( Args: redirect_uri: The redirect URI to validate - allowed_patterns: List of allowed patterns. If None, all URIs are allowed (for DCR compatibility). + allowed_patterns: List of allowed patterns. If None, ordinary URIs are allowed + for DCR compatibility, while unsafe browser schemes are rejected. If empty list, no URIs are allowed. To restrict to localhost only, explicitly pass DEFAULT_LOCALHOST_PATTERNS. @@ -228,8 +250,11 @@ def validate_redirect_uri( uri_str = str(redirect_uri) - # If no patterns specified, allow all for DCR compatibility - # (clients need to dynamically register with their own redirect URIs) + if _is_unsafe_redirect_uri(uri_str): + return False + + # If no patterns specified, preserve broad DCR compatibility after the + # unsafe browser-scheme check above. if allowed_patterns is None: return True diff --git a/tests/server/auth/oauth_proxy/test_client_registration.py b/tests/server/auth/oauth_proxy/test_client_registration.py index c1409b50a..a074c75f0 100644 --- a/tests/server/auth/oauth_proxy/test_client_registration.py +++ b/tests/server/auth/oauth_proxy/test_client_registration.py @@ -2,6 +2,7 @@ import httpx import pytest +from mcp.server.auth.provider import RegistrationError from mcp.shared.auth import OAuthClientInformationFull from pydantic import AnyUrl from starlette.applications import Starlette @@ -29,6 +30,59 @@ class TestOAuthProxyClientRegistration: # Proxy uses token_endpoint_auth_method="none", so client_secret is not stored assert stored.client_secret is None + async def test_register_client_allows_external_https_by_default(self, oauth_proxy): + """Default DCR registration accepts ordinary external HTTPS callbacks.""" + client_info = OAuthClientInformationFull( + client_id="https-client", + client_secret="original-secret", + redirect_uris=[AnyUrl("https://client.example.com/callback")], + ) + + await oauth_proxy.register_client(client_info) + + stored = await oauth_proxy.get_client("https-client") + assert stored is not None + assert stored.redirect_uris == [AnyUrl("https://client.example.com/callback")] + + async def test_register_client_rejects_unsafe_redirect_scheme_by_default( + self, oauth_proxy + ): + """Default DCR registration rejects active browser redirect schemes.""" + client_info = OAuthClientInformationFull( + client_id="javascript-client", + client_secret="original-secret", + redirect_uris=[AnyUrl("javascript:alert(document.cookie)//")], + ) + + with pytest.raises(RegistrationError, match="invalid_redirect_uri"): + await oauth_proxy.register_client(client_info) + + async def test_register_client_without_redirect_uris_defers_allowlist_validation( + self, oauth_proxy + ): + """DCR clients may omit redirect_uris until the authorization request.""" + oauth_proxy._allowed_client_redirect_uris = ["https://client.example/*"] + client_info = OAuthClientInformationFull( + client_id="deferred-client", + client_secret="original-secret", + redirect_uris=None, + ) + + await oauth_proxy.register_client(client_info) + + stored = await oauth_proxy.get_client("deferred-client") + assert stored is not None + assert stored.redirect_uris is not None + assert str(stored.redirect_uris[0]).rstrip("/") == "http://localhost" + + redirect_uri = stored.validate_redirect_uri( + AnyUrl("https://client.example/callback") + ) + assert str(redirect_uri) == "https://client.example/callback" + + with pytest.raises(InvalidRedirectUriError): + stored.validate_redirect_uri(None) + async def test_get_registered_client(self, oauth_proxy): """Test retrieving a registered client.""" client_info = OAuthClientInformationFull( @@ -169,12 +223,19 @@ class TestUpstreamClientIdFallback: assert client is None async def test_redirect_uri_allowed_when_no_pattern_restriction(self, oauth_proxy): - """Any redirect URI is accepted when allowed_client_redirect_uris is None.""" + """Ordinary redirect URIs are accepted when allowed_client_redirect_uris is None.""" assert oauth_proxy._allowed_client_redirect_uris is None client = await oauth_proxy.get_client("test-client-id") assert client is not None uri = client.validate_redirect_uri(AnyUrl("https://claude.ai/oauth/callback")) assert str(uri) == "https://claude.ai/oauth/callback" + uri = client.validate_redirect_uri( + AnyUrl("cursor://anysphere.cursor-mcp/oauth/callback") + ) + assert str(uri) == "cursor://anysphere.cursor-mcp/oauth/callback" + + with pytest.raises(InvalidRedirectUriError): + client.validate_redirect_uri(AnyUrl("javascript:alert(document.cookie)//")) async def test_redirect_uri_validated_against_patterns(self, oauth_proxy): """Redirect URI validation honours allowed_client_redirect_uris when set.""" diff --git a/tests/server/auth/oauth_proxy/test_oauth_proxy.py b/tests/server/auth/oauth_proxy/test_oauth_proxy.py index 8d42f8da7..b221f3ccc 100644 --- a/tests/server/auth/oauth_proxy/test_oauth_proxy.py +++ b/tests/server/auth/oauth_proxy/test_oauth_proxy.py @@ -361,6 +361,40 @@ class TestIdpCallbackErrorForwarding: assert params["error_description"] == ["User denied access"] assert params["state"] == [client_state] + async def test_error_with_unsafe_transaction_redirect_returns_html_error( + self, oauth_proxy + ): + """IdP errors must not redirect to unsafe stored callback URIs.""" + txn_id = "test-txn-unsafe" + + transaction = OAuthTransaction( + txn_id=txn_id, + client_id="test-client", + client_redirect_uri="javascript:alert(document.cookie)//", + client_state="client-state-abc", + code_challenge=None, + code_challenge_method="S256", + scopes=["read"], + created_at=time.time(), + ) + await oauth_proxy._transaction_store.put(key=txn_id, value=transaction) + + app = Starlette(routes=oauth_proxy.get_routes()) + transport = httpx.ASGITransport(app=app) + + async with httpx.AsyncClient( + transport=transport, + base_url="https://myserver.com", + follow_redirects=False, + ) as client: + response = await client.get( + f"/auth/callback?error=access_denied&state={txn_id}" + ) + + assert response.status_code == 400 + assert "location" not in response.headers + assert "Invalid redirect URI" in response.text + async def test_error_with_missing_transaction_returns_html_error(self, oauth_proxy): """When the IdP returns an error but the transaction is missing or expired, the proxy must return a local HTML error page — there is no diff --git a/tests/server/auth/oauth_proxy/test_tokens.py b/tests/server/auth/oauth_proxy/test_tokens.py index 414278cc2..0ecb42afe 100644 --- a/tests/server/auth/oauth_proxy/test_tokens.py +++ b/tests/server/auth/oauth_proxy/test_tokens.py @@ -249,6 +249,51 @@ class TestOAuthProxyTokenEndpointAuth: mock_client.fetch_token.assert_awaited_once() mock_client.aclose.assert_awaited_once() + async def test_callback_rejects_unsafe_transaction_redirect(self, jwt_verifier): + proxy = OAuthProxy( + upstream_authorization_endpoint="https://oauth.example.com/authorize", + upstream_token_endpoint="https://oauth.example.com/token", + upstream_client_id="client-id", + upstream_client_secret="client-secret", + token_verifier=jwt_verifier, + base_url="https://proxy.example.com", + require_authorization_consent=False, + jwt_signing_key="test-secret", + client_storage=MemoryStore(), + ) + + await proxy._transaction_store.put( + key="txn-id", + value=OAuthTransaction( + txn_id="txn-id", + client_id="test-client", + client_redirect_uri="javascript:alert(document.cookie)//", + client_state="client-state", + code_challenge="", + code_challenge_method="S256", + scopes=["read"], + created_at=time.time(), + ), + ) + + mock_request = Mock() + mock_request.query_params = {"code": "idp-code", "state": "txn-id"} + mock_request.cookies = {} + + mock_client = AsyncMock() + mock_client.fetch_token = AsyncMock() + + with patch.object( + proxy, "_create_upstream_oauth_client", return_value=mock_client + ) as create_upstream_oauth_client: + response = await proxy._handle_idp_callback(mock_request) + + assert response.status_code == 400 + assert "location" not in response.headers + assert "Invalid redirect URI" in bytes(response.body).decode() + create_upstream_oauth_client.assert_not_called() + mock_client.fetch_token.assert_not_called() + class TestTokenHandlerErrorTransformation: """Tests for TokenHandler's OAuth 2.1 compliant error transformation.""" diff --git a/tests/server/auth/test_oauth_consent_page.py b/tests/server/auth/test_oauth_consent_page.py index a0a4ea050..b408b7cb9 100644 --- a/tests/server/auth/test_oauth_consent_page.py +++ b/tests/server/auth/test_oauth_consent_page.py @@ -471,6 +471,39 @@ class TestConsentBindingCookie: and send it to a victim whose browser completes the flow. """ + async def test_deny_does_not_redirect_to_unsafe_stored_uri(self, oauth_proxy_https): + """Consent denial must not redirect to an unsafe stored callback URI.""" + txn_id = "test-deny-javascript" + transaction = OAuthTransaction( + txn_id=txn_id, + client_id="test-client", + client_redirect_uri="javascript:alert(document.cookie)//", + client_state="client-state", + code_challenge="challenge", + code_challenge_method="S256", + scopes=["read"], + created_at=time.time(), + ) + await oauth_proxy_https._transaction_store.put(key=txn_id, value=transaction) + + app = Starlette(routes=oauth_proxy_https.get_routes()) + with TestClient(app) as c: + consent = c.get(f"/consent?txn_id={txn_id}") + csrf = _extract_csrf(consent.text) + assert csrf + for k, v in consent.cookies.items(): + c.cookies.set(k, v) + + response = c.post( + "/consent", + data={"action": "deny", "txn_id": txn_id, "csrf_token": csrf}, + follow_redirects=False, + ) + + assert response.status_code == 400 + assert "location" not in response.headers + assert "Invalid redirect URI" in response.text + async def test_approve_sets_consent_binding_cookie(self, oauth_proxy_https): """Approving consent must set a signed consent binding cookie.""" txn_id, _ = await _start_flow( diff --git a/tests/server/auth/test_oauth_proxy_redirect_validation.py b/tests/server/auth/test_oauth_proxy_redirect_validation.py index 9d1b9295e..15f2f8d41 100644 --- a/tests/server/auth/test_oauth_proxy_redirect_validation.py +++ b/tests/server/auth/test_oauth_proxy_redirect_validation.py @@ -89,6 +89,17 @@ class TestProxyDCRClient: AnyUrl("https://claude.ai/api/mcp/auth_callback") ) == AnyUrl("https://claude.ai/api/mcp/auth_callback") + def test_default_rejects_unsafe_registered_redirect_scheme(self): + """Stored DCR metadata cannot preserve unsafe browser schemes.""" + client = ProxyDCRClient( + client_id="test", + client_secret="secret", + redirect_uris=[AnyUrl("javascript:alert(document.cookie)//")], + ) + + with pytest.raises(InvalidRedirectUriError): + client.validate_redirect_uri(AnyUrl("javascript:alert(document.cookie)//")) + def test_custom_patterns(self): """Test custom redirect URI patterns.""" client = ProxyDCRClient( diff --git a/tests/server/auth/test_redirect_validation.py b/tests/server/auth/test_redirect_validation.py index ffe3a4d36..58bb29568 100644 --- a/tests/server/auth/test_redirect_validation.py +++ b/tests/server/auth/test_redirect_validation.py @@ -1,5 +1,6 @@ """Tests for redirect URI validation in OAuth flows.""" +import pytest from pydantic import AnyUrl from fastmcp.server.auth.redirect_validation import ( @@ -66,14 +67,49 @@ class TestValidateRedirectUri: assert validate_redirect_uri(None, []) assert validate_redirect_uri(None, ["http://localhost:*"]) - def test_default_allows_all(self): - """Test that None (default) allows all URIs for DCR compatibility.""" - # All URIs should be allowed when None is provided (DCR compatibility) - assert validate_redirect_uri("http://localhost:3000", None) - assert validate_redirect_uri("http://127.0.0.1:8080", None) - assert validate_redirect_uri("http://example.com", None) - assert validate_redirect_uri("https://app.example.com", None) - assert validate_redirect_uri("https://claude.ai/api/mcp/auth_callback", None) + @pytest.mark.parametrize( + "uri", + [ + "http://localhost:3000", + "http://127.0.0.1:8080", + "http://example.com", + "https://app.example.com", + "https://claude.ai/api/mcp/auth_callback", + "cursor://anysphere.cursor-mcp/oauth/callback", + ], + ) + def test_default_allows_dcr_compatible_redirects(self, uri: str): + """None preserves broad DCR compatibility for ordinary redirect URIs.""" + assert validate_redirect_uri(uri, None) + + @pytest.mark.parametrize( + "uri", + [ + "javascript:alert(document.cookie)//", + "JaVaScRiPt:alert(document.cookie)//", + "data:text/html,", + "file:///tmp/callback", + "vbscript:msgbox(1)", + ], + ) + def test_default_rejects_unsafe_browser_schemes(self, uri: str): + """Default DCR compatibility must not allow active browser schemes.""" + assert not validate_redirect_uri(uri, None) + + @pytest.mark.parametrize( + "uri,pattern", + [ + ("javascript:alert(document.cookie)//", "javascript:*"), + ("data:text/html,", "data:*"), + ("file:///tmp/callback", "file:///*"), + ("vbscript:msgbox(1)", "vbscript:*"), + ], + ) + def test_custom_patterns_cannot_allow_unsafe_browser_schemes( + self, uri: str, pattern: str + ): + """Unsafe browser schemes stay blocked even if a pattern names them.""" + assert not validate_redirect_uri(uri, [pattern]) def test_empty_list_allows_none(self): """Test that empty list allows no redirect URIs."""