From 4036fa178fb265a2af3b9f497019545104cda987 Mon Sep 17 00:00:00 2001 From: Jeremiah Lowin <153965+jlowin@users.noreply.github.com> Date: Sat, 27 Jun 2026 12:47:04 -0400 Subject: [PATCH] Harden OAuth proxy redirect defaults --- docs/servers/auth/oauth-proxy.mdx | 11 ++-- docs/servers/auth/oidc-proxy.mdx | 4 +- .../server/auth/oauth_proxy/consent.py | 31 ++++++++++ .../fastmcp/server/auth/oauth_proxy/models.py | 2 +- .../fastmcp/server/auth/oauth_proxy/proxy.py | 27 ++++++-- .../fastmcp/server/auth/oidc_proxy.py | 4 +- .../fastmcp/server/auth/providers/auth0.py | 2 +- .../fastmcp/server/auth/providers/aws.py | 2 +- .../fastmcp/server/auth/providers/azure.py | 2 +- .../fastmcp/server/auth/providers/clerk.py | 2 +- .../fastmcp/server/auth/providers/discord.py | 2 +- .../fastmcp/server/auth/providers/github.py | 2 +- .../fastmcp/server/auth/providers/google.py | 2 +- .../fastmcp/server/auth/providers/workos.py | 2 +- .../server/auth/redirect_validation.py | 10 +-- .../oauth_proxy/test_client_registration.py | 62 +++++++++++++++++-- tests/server/auth/test_oauth_consent_page.py | 33 ++++++++++ .../test_oauth_proxy_redirect_validation.py | 40 ++++++------ tests/server/auth/test_oauth_proxy_storage.py | 6 +- tests/server/auth/test_redirect_validation.py | 15 +++-- 20 files changed, 200 insertions(+), 61 deletions(-) diff --git a/docs/servers/auth/oauth-proxy.mdx b/docs/servers/auth/oauth-proxy.mdx index 54ca7b764..2f4ec7156 100644 --- a/docs/servers/auth/oauth-proxy.mdx +++ b/docs/servers/auth/oauth-proxy.mdx @@ -193,10 +193,11 @@ 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): All redirect URIs allowed (for MCP/DCR compatibility) - - Empty list `[]`: No redirect URIs allowed - Custom list: Only matching - patterns allowed These patterns apply to MCP client loopback redirects, NOT - the upstream OAuth app redirect URI. + `None` (default): Loopback redirect URIs allowed (`"http://localhost:*"`, + `"http://127.0.0.1:*"`, and `"http://[::1]:*"`) - Empty list `[]`: No + redirect URIs allowed - Custom list: Only matching patterns allowed These + patterns apply to MCP client redirects, NOT the upstream OAuth app redirect + URI. @@ -556,7 +557,7 @@ auth = OAuthProxy( ### Redirect URI Validation -While the OAuth proxy accepts all redirect URIs by default (for DCR compatibility), you can restrict which clients can connect by specifying allowed patterns: +The OAuth proxy accepts loopback redirect URIs by default. Specify allowed patterns when supporting known clients with fixed external callbacks: ```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 d33cd611a..0041e5971 100644 --- a/docs/servers/auth/oidc-proxy.mdx +++ b/docs/servers/auth/oidc-proxy.mdx @@ -124,11 +124,11 @@ 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): All redirect URIs allowed (for MCP/DCR compatibility) + - `None` (default): Loopback redirect URIs allowed (`"http://localhost:*"`, `"http://127.0.0.1:*"`, and `"http://[::1]:*"`) - Empty list `[]`: No redirect URIs allowed - Custom list: Only matching patterns allowed -These patterns apply to MCP client loopback redirects, NOT the upstream OAuth app redirect URI. +These patterns apply to MCP client redirects, NOT the upstream OAuth app redirect URI. diff --git a/fastmcp_slim/fastmcp/server/auth/oauth_proxy/consent.py b/fastmcp_slim/fastmcp/server/auth/oauth_proxy/consent.py index 68551f7cc..67cb14404 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,17 @@ 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 +548,15 @@ 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 5d2366a76..c9a5a7c8d 100644 --- a/fastmcp_slim/fastmcp/server/auth/oauth_proxy/models.py +++ b/fastmcp_slim/fastmcp/server/auth/oauth_proxy/models.py @@ -243,7 +243,7 @@ class ProxyDCRClient(OAuthClientInformationFull): if pattern_matches: return redirect_uri - # Patterns configured but didn't match (None means "allow all"; [] means "block all") + # Patterns configured but didn't match ([] means "block all") if self.allowed_redirect_uri_patterns is not None: raise InvalidRedirectUriError( f"Redirect URI '{redirect_uri}' does not match allowed patterns." diff --git a/fastmcp_slim/fastmcp/server/auth/oauth_proxy/proxy.py b/fastmcp_slim/fastmcp/server/auth/oauth_proxy/proxy.py index 227f6dc92..7794c702d 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,10 @@ 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 ( + DEFAULT_LOCALHOST_PATTERNS, + validate_redirect_uri, +) from fastmcp.utilities.auth import parse_scopes from fastmcp.utilities.logging import get_logger @@ -300,7 +305,7 @@ class OAuthProxy(OAuthProvider, ConsentMixin): service_documentation_url: Optional service documentation URL 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), all redirect URIs are allowed (for DCR compatibility). + If None (default), localhost and loopback redirect URIs are allowed. 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. @@ -428,8 +433,10 @@ class OAuthProxy(OAuthProvider, ConsentMixin): "allowed_client_redirect_uris is empty list; no redirect URIs will be accepted. " "This will block all OAuth clients." ) - self._allowed_client_redirect_uris: list[str] | None = ( - allowed_client_redirect_uris + self._allowed_client_redirect_uris: list[str] = ( + list(DEFAULT_LOCALHOST_PATTERNS) + if allowed_client_redirect_uris is None + else allowed_client_redirect_uris ) # PKCE configuration @@ -837,6 +844,18 @@ 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") + + redirect_uris = client_info.redirect_uris or [AnyUrl("http://localhost")] + for redirect_uri in 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}' does not match allowed patterns.", + ) + # 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, @@ -844,7 +863,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, diff --git a/fastmcp_slim/fastmcp/server/auth/oidc_proxy.py b/fastmcp_slim/fastmcp/server/auth/oidc_proxy.py index 855688048..ca63a0435 100644 --- a/fastmcp_slim/fastmcp/server/auth/oidc_proxy.py +++ b/fastmcp_slim/fastmcp/server/auth/oidc_proxy.py @@ -277,9 +277,9 @@ class OIDCProxy(OAuthProxy): redirect_path: Redirect path configured in upstream OAuth app (defaults to "/auth/callback") 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), all redirect URIs are allowed (for DCR compatibility). + If None (default), localhost and loopback redirect URIs are allowed. If empty list, no redirect URIs are allowed. - These are for MCP clients performing loopback redirects, NOT for the upstream OAuth app. + These are for MCP client redirects, NOT for the upstream OAuth app. client_storage: Storage backend for OAuth state (client registrations, encrypted tokens). If None, an encrypted file store will be created in the data directory (derived from `platformdirs`). diff --git a/fastmcp_slim/fastmcp/server/auth/providers/auth0.py b/fastmcp_slim/fastmcp/server/auth/providers/auth0.py index 16ad68452..d6f6c5068 100644 --- a/fastmcp_slim/fastmcp/server/auth/providers/auth0.py +++ b/fastmcp_slim/fastmcp/server/auth/providers/auth0.py @@ -102,7 +102,7 @@ class Auth0Provider(OIDCProxy): required_scopes: Required Auth0 scopes (defaults to ["openid"]) redirect_path: Redirect path configured in Auth0 application allowed_client_redirect_uris: List of allowed redirect URI patterns for MCP clients. - If None (default), all URIs are allowed. If empty list, no URIs are allowed. + If None (default), localhost and loopback redirect URIs are allowed. If empty list, no URIs are allowed. client_storage: Storage backend for OAuth state (client registrations, encrypted tokens). If None, an encrypted file store will be created in the data directory (derived from `platformdirs`). diff --git a/fastmcp_slim/fastmcp/server/auth/providers/aws.py b/fastmcp_slim/fastmcp/server/auth/providers/aws.py index ff6add398..bb592ea82 100644 --- a/fastmcp_slim/fastmcp/server/auth/providers/aws.py +++ b/fastmcp_slim/fastmcp/server/auth/providers/aws.py @@ -163,7 +163,7 @@ class AWSCognitoProvider(OIDCProxy): redirect_path: Redirect path configured in Cognito app (defaults to "/auth/callback") required_scopes: Required Cognito scopes (defaults to ["openid"]) allowed_client_redirect_uris: List of allowed redirect URI patterns for MCP clients. - If None (default), all URIs are allowed. If empty list, no URIs are allowed. + If None (default), localhost and loopback redirect URIs are allowed. If empty list, no URIs are allowed. client_storage: Storage backend for OAuth state (client registrations, encrypted tokens). If None, an encrypted file store will be created in the data directory (derived from `platformdirs`). diff --git a/fastmcp_slim/fastmcp/server/auth/providers/azure.py b/fastmcp_slim/fastmcp/server/auth/providers/azure.py index c1b094f9c..8534f6242 100644 --- a/fastmcp_slim/fastmcp/server/auth/providers/azure.py +++ b/fastmcp_slim/fastmcp/server/auth/providers/azure.py @@ -163,7 +163,7 @@ class AzureProvider(OAuthProxy): upstream Azure token, but MCP clients are unaware of them. Note: "offline_access" is automatically included to obtain refresh tokens. allowed_client_redirect_uris: List of allowed redirect URI patterns for MCP clients. - If None (default), all URIs are allowed. If empty list, no URIs are allowed. + If None (default), localhost and loopback redirect URIs are allowed. If empty list, no URIs are allowed. client_storage: Storage backend for OAuth state (client registrations, encrypted tokens). If None, an encrypted file store will be created in the data directory (derived from `platformdirs`). diff --git a/fastmcp_slim/fastmcp/server/auth/providers/clerk.py b/fastmcp_slim/fastmcp/server/auth/providers/clerk.py index 21d7c3b48..7772959c9 100644 --- a/fastmcp_slim/fastmcp/server/auth/providers/clerk.py +++ b/fastmcp_slim/fastmcp/server/auth/providers/clerk.py @@ -317,7 +317,7 @@ class ClerkProvider(OAuthProxy): well-known endpoints. Defaults to required_scopes if not provided. timeout_seconds: HTTP request timeout for Clerk API calls (defaults to 10) allowed_client_redirect_uris: List of allowed redirect URI patterns for MCP clients. - If None (default), all URIs are allowed. If empty list, no URIs are allowed. + If None (default), localhost and loopback redirect URIs are allowed. If empty list, no URIs are allowed. client_storage: Storage backend for OAuth state (client registrations, encrypted tokens). If None, an encrypted file store will be created in the data directory (derived from ``platformdirs``). diff --git a/fastmcp_slim/fastmcp/server/auth/providers/discord.py b/fastmcp_slim/fastmcp/server/auth/providers/discord.py index da13a074f..a5005e378 100644 --- a/fastmcp_slim/fastmcp/server/auth/providers/discord.py +++ b/fastmcp_slim/fastmcp/server/auth/providers/discord.py @@ -230,7 +230,7 @@ class DiscordProvider(OAuthProxy): - "guilds" for server membership info timeout_seconds: HTTP request timeout for Discord API calls (defaults to 10) allowed_client_redirect_uris: List of allowed redirect URI patterns for MCP clients. - If None (default), all URIs are allowed. If empty list, no URIs are allowed. + If None (default), localhost and loopback redirect URIs are allowed. If empty list, no URIs are allowed. client_storage: Storage backend for OAuth state (client registrations, encrypted tokens). If None, an encrypted file store will be created in the data directory (derived from `platformdirs`). diff --git a/fastmcp_slim/fastmcp/server/auth/providers/github.py b/fastmcp_slim/fastmcp/server/auth/providers/github.py index d14aeb08f..786d67dcf 100644 --- a/fastmcp_slim/fastmcp/server/auth/providers/github.py +++ b/fastmcp_slim/fastmcp/server/auth/providers/github.py @@ -246,7 +246,7 @@ class GitHubProvider(OAuthProxy): enable (e.g., 300 for 5 minutes). max_cache_size: Maximum number of tokens to cache. Default: 10 000. allowed_client_redirect_uris: List of allowed redirect URI patterns for MCP clients. - If None (default), all URIs are allowed. If empty list, no URIs are allowed. + If None (default), localhost and loopback redirect URIs are allowed. If empty list, no URIs are allowed. client_storage: Storage backend for OAuth state (client registrations, encrypted tokens). If None, an encrypted file store will be created in the data directory (derived from `platformdirs`). diff --git a/fastmcp_slim/fastmcp/server/auth/providers/google.py b/fastmcp_slim/fastmcp/server/auth/providers/google.py index 07f757001..9fcfaba69 100644 --- a/fastmcp_slim/fastmcp/server/auth/providers/google.py +++ b/fastmcp_slim/fastmcp/server/auth/providers/google.py @@ -279,7 +279,7 @@ class GoogleProvider(OAuthProxy): required minimum. Shorthands are normalized to full URI forms. timeout_seconds: HTTP request timeout for Google API calls (defaults to 10) allowed_client_redirect_uris: List of allowed redirect URI patterns for MCP clients. - If None (default), all URIs are allowed. If empty list, no URIs are allowed. + If None (default), localhost and loopback redirect URIs are allowed. If empty list, no URIs are allowed. client_storage: Storage backend for OAuth state (client registrations, encrypted tokens). If None, an encrypted file store will be created in the data directory (derived from `platformdirs`). diff --git a/fastmcp_slim/fastmcp/server/auth/providers/workos.py b/fastmcp_slim/fastmcp/server/auth/providers/workos.py index 0a3f556b1..05206c1a0 100644 --- a/fastmcp_slim/fastmcp/server/auth/providers/workos.py +++ b/fastmcp_slim/fastmcp/server/auth/providers/workos.py @@ -202,7 +202,7 @@ class WorkOSProvider(OAuthProxy): required minimum. timeout_seconds: HTTP request timeout for WorkOS API calls (defaults to 10) allowed_client_redirect_uris: List of allowed redirect URI patterns for MCP clients. - If None (default), all URIs are allowed. If empty list, no URIs are allowed. + If None (default), localhost and loopback redirect URIs are allowed. If empty list, no URIs are allowed. client_storage: Storage backend for OAuth state (client registrations, encrypted tokens). If None, an encrypted file store will be created in the data directory (derived from `platformdirs`). diff --git a/fastmcp_slim/fastmcp/server/auth/redirect_validation.py b/fastmcp_slim/fastmcp/server/auth/redirect_validation.py index 678125765..78389871e 100644 --- a/fastmcp_slim/fastmcp/server/auth/redirect_validation.py +++ b/fastmcp_slim/fastmcp/server/auth/redirect_validation.py @@ -216,9 +216,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, DEFAULT_LOCALHOST_PATTERNS is used. If empty list, no URIs are allowed. - To restrict to localhost only, explicitly pass DEFAULT_LOCALHOST_PATTERNS. Returns: True if the redirect URI is allowed @@ -228,10 +227,10 @@ 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 no patterns are specified, default to loopback redirect URIs. OAuth + # clients that need fixed external callbacks can configure explicit patterns. if allowed_patterns is None: - return True + allowed_patterns = DEFAULT_LOCALHOST_PATTERNS # Check if URI matches any allowed pattern for pattern in allowed_patterns: @@ -245,4 +244,5 @@ def validate_redirect_uri( DEFAULT_LOCALHOST_PATTERNS = [ "http://localhost:*", "http://127.0.0.1:*", + "http://[::1]:*", ] diff --git a/tests/server/auth/oauth_proxy/test_client_registration.py b/tests/server/auth/oauth_proxy/test_client_registration.py index 946690b26..8ab3c9343 100644 --- a/tests/server/auth/oauth_proxy/test_client_registration.py +++ b/tests/server/auth/oauth_proxy/test_client_registration.py @@ -7,6 +7,7 @@ from pydantic import AnyUrl from starlette.applications import Starlette from fastmcp.server.auth.oauth_proxy.models import InvalidRedirectUriError +from fastmcp.server.auth.redirect_validation import DEFAULT_LOCALHOST_PATTERNS class TestOAuthProxyClientRegistration: @@ -87,7 +88,7 @@ class TestOAuthProxyClientRegistration: response = await client.post( "/register", json={ - "redirect_uris": ["https://client.example.com/callback"], + "redirect_uris": ["http://localhost:43210/callback"], "client_name": "Test Client", }, ) @@ -100,6 +101,29 @@ class TestOAuthProxyClientRegistration: assert registered_client is not None assert registered_client.scope == "read write calendar" + async def test_register_client_rejects_external_redirect_by_default( + self, oauth_proxy + ): + """DCR defaults to loopback redirects rather than arbitrary HTTPS targets.""" + app = Starlette(routes=oauth_proxy.get_routes()) + transport = httpx.ASGITransport(app=app) + + async with httpx.AsyncClient( + transport=transport, + base_url="https://myserver.com", + ) as client: + response = await client.post( + "/register", + json={ + "redirect_uris": ["https://attacker.example/callback"], + "client_name": "Test Client", + }, + ) + + assert response.status_code == 400 + body = response.json() + assert body["error"] == "invalid_redirect_uri" + class TestUpstreamClientIdFallback: """Tests for clients that skip DCR and use the upstream client_id directly.""" @@ -126,12 +150,40 @@ 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.""" - assert oauth_proxy._allowed_client_redirect_uris is None + """Default redirect URI validation accepts loopback redirects.""" + assert oauth_proxy._allowed_client_redirect_uris == DEFAULT_LOCALHOST_PATTERNS 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("http://localhost:12345/callback")) + assert str(uri) == "http://localhost:12345/callback" + + with pytest.raises(InvalidRedirectUriError): + client.validate_redirect_uri(AnyUrl("https://attacker.example/callback")) + + async def test_authorize_does_not_redirect_to_external_uri_by_default( + self, oauth_proxy + ): + """Synthetic upstream clients cannot turn /authorize into an open redirect.""" + 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( + "/authorize", + params={ + "response_type": "code", + "client_id": "test-client-id", + "redirect_uri": "https://attacker.example/callback", + "state": "abc123", + }, + ) + + assert response.status_code == 400 + assert "location" not in response.headers 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/test_oauth_consent_page.py b/tests/server/auth/test_oauth_consent_page.py index a0a4ea050..da512d28c 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_disallowed_uri(self, oauth_proxy_https): + """Consent denial must not redirect to a disallowed stored callback URI.""" + txn_id = "test-deny-external" + transaction = OAuthTransaction( + txn_id=txn_id, + client_id="test-client", + client_redirect_uri="https://attacker.example/callback", + 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 d7638454f..674bbcc74 100644 --- a/tests/server/auth/test_oauth_proxy_redirect_validation.py +++ b/tests/server/auth/test_oauth_proxy_redirect_validation.py @@ -11,6 +11,7 @@ from fastmcp.server.auth.auth import TokenVerifier from fastmcp.server.auth.cimd import CIMDDocument from fastmcp.server.auth.oauth_proxy import OAuthProxy from fastmcp.server.auth.oauth_proxy.models import ProxyDCRClient +from fastmcp.server.auth.redirect_validation import DEFAULT_LOCALHOST_PATTERNS # Standard public IP used for DNS mocking in tests TEST_PUBLIC_IP = "93.184.216.34" @@ -29,30 +30,26 @@ class MockTokenVerifier(TokenVerifier): class TestProxyDCRClient: """Test ProxyDCRClient redirect URI validation.""" - def test_default_allows_all(self): - """Test that default configuration allows all URIs for DCR compatibility.""" + def test_default_allows_loopback_redirects(self): + """Default redirect validation accepts loopback callbacks.""" client = ProxyDCRClient( client_id="test", client_secret="secret", redirect_uris=[AnyUrl("http://localhost:3000")], + allowed_redirect_uri_patterns=DEFAULT_LOCALHOST_PATTERNS, ) - # All URIs should be allowed by default for DCR compatibility - assert client.validate_redirect_uri(AnyUrl("http://localhost:3000")) == AnyUrl( - "http://localhost:3000" - ) - assert client.validate_redirect_uri(AnyUrl("http://localhost:8080")) == AnyUrl( - "http://localhost:8080" - ) - assert client.validate_redirect_uri(AnyUrl("http://127.0.0.1:3000")) == AnyUrl( - "http://127.0.0.1:3000" - ) - assert client.validate_redirect_uri(AnyUrl("http://example.com")) == AnyUrl( - "http://example.com" - ) - assert client.validate_redirect_uri( - AnyUrl("https://claude.ai/api/mcp/auth_callback") - ) == AnyUrl("https://claude.ai/api/mcp/auth_callback") + assert client.validate_redirect_uri(AnyUrl("http://localhost:3000")) + assert client.validate_redirect_uri(AnyUrl("http://localhost:8080")) + assert client.validate_redirect_uri(AnyUrl("http://127.0.0.1:3000")) + assert client.validate_redirect_uri(AnyUrl("http://[::1]:3000")) + + with pytest.raises(InvalidRedirectUriError): + client.validate_redirect_uri(AnyUrl("http://example.com")) + with pytest.raises(InvalidRedirectUriError): + client.validate_redirect_uri( + AnyUrl("https://claude.ai/api/mcp/auth_callback") + ) def test_custom_patterns(self): """Test custom redirect URI patterns.""" @@ -256,8 +253,8 @@ class TestProxyDCRClient: class TestOAuthProxyRedirectValidation: """Test OAuth proxy with redirect URI validation.""" - def test_proxy_default_allows_all(self): - """Test that OAuth proxy defaults to allowing all URIs for DCR compatibility.""" + def test_proxy_default_allows_loopback_redirects(self): + """Test that OAuth proxy defaults to loopback redirect URIs.""" proxy = OAuthProxy( upstream_authorization_endpoint="https://auth.example.com/authorize", upstream_token_endpoint="https://auth.example.com/token", @@ -269,8 +266,7 @@ class TestOAuthProxyRedirectValidation: client_storage=MemoryStore(), ) - # The proxy should store None for default (allow all) - assert proxy._allowed_client_redirect_uris is None + assert proxy._allowed_client_redirect_uris == DEFAULT_LOCALHOST_PATTERNS def test_proxy_custom_patterns(self): """Test OAuth proxy with custom redirect patterns.""" diff --git a/tests/server/auth/test_oauth_proxy_storage.py b/tests/server/auth/test_oauth_proxy_storage.py index 0273a368c..5224c381e 100644 --- a/tests/server/auth/test_oauth_proxy_storage.py +++ b/tests/server/auth/test_oauth_proxy_storage.py @@ -205,7 +205,11 @@ class TestOAuthProxyStorage: "client_secret": None, "client_id_issued_at": None, "client_secret_expires_at": None, - "allowed_redirect_uri_patterns": None, + "allowed_redirect_uri_patterns": [ + "http://localhost:*", + "http://127.0.0.1:*", + "http://[::1]:*", + ], "cimd_document": None, "cimd_fetched_at": None, } diff --git a/tests/server/auth/test_redirect_validation.py b/tests/server/auth/test_redirect_validation.py index ffe3a4d36..33a53116b 100644 --- a/tests/server/auth/test_redirect_validation.py +++ b/tests/server/auth/test_redirect_validation.py @@ -66,14 +66,16 @@ 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) + def test_default_allows_loopback_redirects(self): + """Test that None defaults to loopback redirect URI patterns.""" 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) + assert validate_redirect_uri("http://[::1]:8080", None) + assert not validate_redirect_uri("http://example.com", None) + assert not validate_redirect_uri("https://app.example.com", None) + assert not validate_redirect_uri( + "https://claude.ai/api/mcp/auth_callback", None + ) def test_empty_list_allows_none(self): """Test that empty list allows no redirect URIs.""" @@ -298,6 +300,7 @@ class TestDefaultPatterns: """Test that default patterns include localhost variations.""" assert "http://localhost:*" in DEFAULT_LOCALHOST_PATTERNS assert "http://127.0.0.1:*" in DEFAULT_LOCALHOST_PATTERNS + assert "http://[::1]:*" in DEFAULT_LOCALHOST_PATTERNS def test_explicit_localhost_patterns(self): """Test that explicitly passing DEFAULT_LOCALHOST_PATTERNS restricts to localhost."""