From 7fb037f20ca11ed4d1e51920266fd534f18bde11 Mon Sep 17 00:00:00 2001 From: Jeremiah Lowin <153965+jlowin@users.noreply.github.com> Date: Fri, 17 Apr 2026 16:39:50 -0400 Subject: [PATCH] Harden OAuth Proxy silent consent against AS-in-the-middle (#3960) --- docs/servers/auth/oauth-proxy.mdx | 24 +- docs/servers/auth/oidc-proxy.mdx | 4 +- docs/v2/servers/auth/oauth-proxy.mdx | 24 +- docs/v2/servers/auth/oidc-proxy.mdx | 4 +- .../server/auth/oauth_proxy/consent.py | 120 ++++++--- src/fastmcp/server/auth/oauth_proxy/proxy.py | 48 ++-- src/fastmcp/server/auth/oidc_proxy.py | 2 +- src/fastmcp/server/auth/providers/auth0.py | 2 +- src/fastmcp/server/auth/providers/aws.py | 2 +- src/fastmcp/server/auth/providers/azure.py | 2 +- src/fastmcp/server/auth/providers/clerk.py | 2 +- src/fastmcp/server/auth/providers/discord.py | 2 +- src/fastmcp/server/auth/providers/github.py | 2 +- src/fastmcp/server/auth/providers/google.py | 2 +- src/fastmcp/server/auth/providers/oci.py | 2 +- src/fastmcp/server/auth/providers/workos.py | 2 +- tests/server/auth/test_oauth_consent_flow.py | 235 ++++++++++++++++-- tests/server/auth/test_oauth_consent_page.py | 37 ++- 18 files changed, 411 insertions(+), 105 deletions(-) diff --git a/docs/servers/auth/oauth-proxy.mdx b/docs/servers/auth/oauth-proxy.mdx index 1a67dea65..afdf0532d 100644 --- a/docs/servers/auth/oauth-proxy.mdx +++ b/docs/servers/auth/oauth-proxy.mdx @@ -297,14 +297,20 @@ auth = OAuthProxy(..., client_storage=MemoryStore()) - - Whether to require user consent before authorizing MCP clients. When enabled (default), users see a consent screen that displays which client is requesting access, preventing [confused deputy attacks](https://modelcontextprotocol.io/specification/2025-06-18/basic/security_best_practices#confused-deputy-problem) by ensuring users explicitly approve new clients. + + Consent screen behavior for authorization requests. The consent page displays which client is requesting access, defending against [confused deputy attacks](https://modelcontextprotocol.io/specification/2025-06-18/basic/security_best_practices#confused-deputy-problem) and AS-in-the-middle attacks (GHSA-6x8h-498w-gv8c) by requiring explicit user approval. - **Default behavior (True):** - Users see a consent screen on first authorization. Consent choices are remembered via signed cookies, so users only need to approve each client once. This protects against malicious clients impersonating the user. + **`True` (default) — always prompt:** + Users see the consent screen on every authorization. Strongest protection against AS-in-the-middle attacks where a malicious MCP server redirects the victim's browser into a legitimate proxy and relies on a previously-remembered approval to silently complete the flow. - **Disabling consent (False):** - Authorization proceeds directly to the upstream provider without user confirmation. Only use this for local development or testing environments where the security trade-off is acceptable. + **`"remember"` — silent consent on return:** + Users see the consent screen on first authorization; subsequent flows from the same browser for the same `(client_id, redirect_uri)` are silently approved via a signed cookie. Cross-site navigations (detected via `Sec-Fetch-Site`) still prompt, which preserves the mitigation against AS-in-the-middle while keeping the prompt-free UX for direct client-initiated flows. Weaker than `True` — use only if the prompt friction is a problem. + + **`"external"` — delegate to upstream:** + Skip the built-in consent page; consent is collected by the upstream IdP or a custom login page referenced via `upstream_authorization_endpoint`. No security warning is logged. + + **`False` — disable entirely:** + Authorization proceeds directly to the upstream provider without any consent UI. Logs a security warning. Only for local development or testing. ```python # Development/testing only - skip consent screen @@ -312,6 +318,12 @@ auth = OAuthProxy(..., client_storage=MemoryStore()) ..., require_authorization_consent=False # ⚠️ Security warning: only for local/testing ) + + # Convenience mode - silent consent on return visits (less safe than True) + auth = OAuthProxy( + ..., + require_authorization_consent="remember", + ) ``` diff --git a/docs/servers/auth/oidc-proxy.mdx b/docs/servers/auth/oidc-proxy.mdx index d811d8583..d33cd611a 100644 --- a/docs/servers/auth/oidc-proxy.mdx +++ b/docs/servers/auth/oidc-proxy.mdx @@ -198,8 +198,8 @@ auth = OIDCProxy( - - Whether to require user consent before authorizing MCP clients. When enabled (default), users see a consent screen that displays which client is requesting access. See [OAuthProxy documentation](/servers/auth/oauth-proxy#confused-deputy-attacks) for details on confused deputy attack protection. + + Consent screen behavior for authorization requests. Accepts `True` (default; always prompt — strongest protection), `"remember"` (silent consent on return visits via signed cookie, gated by `Sec-Fetch-Site` to block AS-in-the-middle attacks), `"external"` (consent handled by upstream IdP or custom page), or `False` (disable entirely; local/testing only). See the [OAuthProxy documentation](/servers/auth/oauth-proxy) for full details on each mode and the security trade-offs. diff --git a/docs/v2/servers/auth/oauth-proxy.mdx b/docs/v2/servers/auth/oauth-proxy.mdx index c3c2e25c5..c8fb0f1f3 100644 --- a/docs/v2/servers/auth/oauth-proxy.mdx +++ b/docs/v2/servers/auth/oauth-proxy.mdx @@ -287,14 +287,20 @@ auth = OAuthProxy(..., client_storage=MemoryStore()) - - Whether to require user consent before authorizing MCP clients. When enabled (default), users see a consent screen that displays which client is requesting access, preventing [confused deputy attacks](https://modelcontextprotocol.io/specification/2025-06-18/basic/security_best_practices#confused-deputy-problem) by ensuring users explicitly approve new clients. + + Consent screen behavior for authorization requests. The consent page displays which client is requesting access, defending against [confused deputy attacks](https://modelcontextprotocol.io/specification/2025-06-18/basic/security_best_practices#confused-deputy-problem) and AS-in-the-middle attacks (GHSA-6x8h-498w-gv8c) by requiring explicit user approval. - **Default behavior (True):** - Users see a consent screen on first authorization. Consent choices are remembered via signed cookies, so users only need to approve each client once. This protects against malicious clients impersonating the user. + **`True` (default) — always prompt:** + Users see the consent screen on every authorization. Strongest protection against AS-in-the-middle attacks where a malicious MCP server redirects the victim's browser into a legitimate proxy and relies on a previously-remembered approval to silently complete the flow. - **Disabling consent (False):** - Authorization proceeds directly to the upstream provider without user confirmation. Only use this for local development or testing environments where the security trade-off is acceptable. + **`"remember"` — silent consent on return:** + Users see the consent screen on first authorization; subsequent flows from the same browser for the same `(client_id, redirect_uri)` are silently approved via a signed cookie. Cross-site navigations (detected via `Sec-Fetch-Site`) still prompt, which preserves the mitigation against AS-in-the-middle while keeping the prompt-free UX for direct client-initiated flows. Weaker than `True` — use only if the prompt friction is a problem. + + **`"external"` — delegate to upstream:** + Skip the built-in consent page; consent is collected by the upstream IdP or a custom login page referenced via `upstream_authorization_endpoint`. No security warning is logged. + + **`False` — disable entirely:** + Authorization proceeds directly to the upstream provider without any consent UI. Logs a security warning. Only for local development or testing. ```python # Development/testing only - skip consent screen @@ -302,6 +308,12 @@ auth = OAuthProxy(..., client_storage=MemoryStore()) ..., require_authorization_consent=False # ⚠️ Security warning: only for local/testing ) + + # Convenience mode - silent consent on return visits (less safe than True) + auth = OAuthProxy( + ..., + require_authorization_consent="remember", + ) ``` diff --git a/docs/v2/servers/auth/oidc-proxy.mdx b/docs/v2/servers/auth/oidc-proxy.mdx index a8a97a8de..750298298 100644 --- a/docs/v2/servers/auth/oidc-proxy.mdx +++ b/docs/v2/servers/auth/oidc-proxy.mdx @@ -198,8 +198,8 @@ auth = OIDCProxy( - - Whether to require user consent before authorizing MCP clients. When enabled (default), users see a consent screen that displays which client is requesting access. See [OAuthProxy documentation](/v2/servers/auth/oauth-proxy#confused-deputy-attacks) for details on confused deputy attack protection. + + Consent screen behavior for authorization requests. Accepts `True` (default; always prompt — strongest protection), `"remember"` (silent consent on return visits via signed cookie, gated by `Sec-Fetch-Site` to block AS-in-the-middle attacks), `"external"` (consent handled by upstream IdP or custom page), or `False` (disable entirely; local/testing only). See the [OAuthProxy documentation](/v2/servers/auth/oauth-proxy) for full details on each mode and the security trade-offs. diff --git a/src/fastmcp/server/auth/oauth_proxy/consent.py b/src/fastmcp/server/auth/oauth_proxy/consent.py index 01b28baf3..2917d10d3 100644 --- a/src/fastmcp/server/auth/oauth_proxy/consent.py +++ b/src/fastmcp/server/auth/oauth_proxy/consent.py @@ -329,28 +329,54 @@ class ConsentMixin: txn = txn_model.model_dump() client_key = self._make_client_key(txn["client_id"], txn["client_redirect_uri"]) - approved = set(self._decode_list_cookie(request, "MCP_APPROVED_CLIENTS")) - denied = set(self._decode_list_cookie(request, "MCP_DENIED_CLIENTS")) + # Silent consent only fires in "remember" mode, and only when the + # request arrived via a safe navigation context. AS-in-the-middle + # attacks (GHSA-6x8h-498w-gv8c) surface as cross-site redirects from + # a third-party origin into /authorize; forcing the HTML prompt in + # that case preserves the consent-screen mitigation without blocking + # legitimate client-initiated flows (Sec-Fetch-Site: none). + if self._require_authorization_consent == "remember": + sec_fetch_site = request.headers.get("Sec-Fetch-Site") + # Fail closed on missing header: legacy clients degrade to the + # explicit prompt rather than silent approval. + silent_eligible = sec_fetch_site in ("same-origin", "same-site", "none") - if client_key in approved: - consent_token = secrets.token_urlsafe(32) - txn_model.consent_token = consent_token - await self._transaction_store.put(key=txn_id, value=txn_model, ttl=15 * 60) - upstream_url = self._build_upstream_authorize_url(txn_id, txn) - response = RedirectResponse(url=upstream_url, status_code=302) - self._set_consent_binding_cookie(request, response, txn_id, consent_token) - return response + if silent_eligible: + approved = set( + self._decode_list_cookie(request, "MCP_APPROVED_CLIENTS") + ) + denied = set(self._decode_list_cookie(request, "MCP_DENIED_CLIENTS")) - if client_key in denied: - callback_params = { - "error": "access_denied", - "state": txn.get("client_state") or "", - } - sep = "&" if "?" in txn["client_redirect_uri"] else "?" - return RedirectResponse( - url=f"{txn['client_redirect_uri']}{sep}{urlencode(callback_params)}", - status_code=302, - ) + if client_key in approved: + consent_token = secrets.token_urlsafe(32) + txn_model.consent_token = consent_token + await self._transaction_store.put( + key=txn_id, value=txn_model, ttl=15 * 60 + ) + upstream_url = self._build_upstream_authorize_url(txn_id, txn) + response = RedirectResponse(url=upstream_url, status_code=302) + self._set_consent_binding_cookie( + request, response, txn_id, consent_token + ) + return response + + if client_key in denied: + callback_params = { + "error": "access_denied", + "state": txn.get("client_state") or "", + } + sep = "&" if "?" in txn["client_redirect_uri"] else "?" + return RedirectResponse( + url=f"{txn['client_redirect_uri']}{sep}{urlencode(callback_params)}", + status_code=302, + ) + else: + logger.info( + "Silent consent skipped for transaction %s: Sec-Fetch-Site=%r " + "(cross-site navigation; forcing explicit consent prompt)", + txn_id, + sec_fetch_site, + ) # Need consent: issue CSRF token and show HTML csrf_token = secrets.token_urlsafe(32) @@ -465,23 +491,33 @@ class ConsentMixin: client_key = self._make_client_key(txn["client_id"], txn["client_redirect_uri"]) - if action == "approve": - approved = list(self._decode_list_cookie(request, "MCP_APPROVED_CLIENTS")) - if client_key in approved: - approved.remove(client_key) - approved.append(client_key) - approved = approved[-_MAX_REMEMBERED_CLIENTS:] - approved_b64 = self._encode_list_cookie(approved) + remember_mode = self._require_authorization_consent == "remember" + if action == "approve": consent_token = secrets.token_urlsafe(32) txn_model.consent_token = consent_token await self._transaction_store.put(key=txn_id, value=txn_model, ttl=15 * 60) upstream_url = self._build_upstream_authorize_url(txn_id, txn) response = RedirectResponse(url=upstream_url, status_code=302) - self._set_list_cookie( - response, "MCP_APPROVED_CLIENTS", approved_b64, max_age=365 * 24 * 3600 - ) + + # Only persist the approval for future silent consent in "remember" + # mode; in the default "always" mode the cookie would never be read. + if remember_mode: + approved = list( + self._decode_list_cookie(request, "MCP_APPROVED_CLIENTS") + ) + if client_key in approved: + approved.remove(client_key) + approved.append(client_key) + approved = approved[-_MAX_REMEMBERED_CLIENTS:] + self._set_list_cookie( + response, + "MCP_APPROVED_CLIENTS", + self._encode_list_cookie(approved), + max_age=365 * 24 * 3600, + ) + # Clear CSRF cookie by setting empty short-lived value self._set_list_cookie( response, "MCP_CONSENT_STATE", self._encode_list_cookie([]), max_age=60 @@ -490,13 +526,6 @@ class ConsentMixin: return response elif action == "deny": - denied = list(self._decode_list_cookie(request, "MCP_DENIED_CLIENTS")) - if client_key in denied: - denied.remove(client_key) - denied.append(client_key) - denied = denied[-_MAX_REMEMBERED_CLIENTS:] - denied_b64 = self._encode_list_cookie(denied) - callback_params = { "error": "access_denied", "state": txn.get("client_state") or "", @@ -506,9 +535,20 @@ class ConsentMixin: f"{txn['client_redirect_uri']}{sep}{urlencode(callback_params)}" ) response = RedirectResponse(url=client_callback_url, status_code=302) - self._set_list_cookie( - response, "MCP_DENIED_CLIENTS", denied_b64, max_age=365 * 24 * 3600 - ) + + if remember_mode: + denied = list(self._decode_list_cookie(request, "MCP_DENIED_CLIENTS")) + if client_key in denied: + denied.remove(client_key) + denied.append(client_key) + denied = denied[-_MAX_REMEMBERED_CLIENTS:] + self._set_list_cookie( + response, + "MCP_DENIED_CLIENTS", + self._encode_list_cookie(denied), + max_age=365 * 24 * 3600, + ) + self._set_list_cookie( response, "MCP_CONSENT_STATE", self._encode_list_cookie([]), max_age=60 ) diff --git a/src/fastmcp/server/auth/oauth_proxy/proxy.py b/src/fastmcp/server/auth/oauth_proxy/proxy.py index e7cf82542..cf049f885 100644 --- a/src/fastmcp/server/auth/oauth_proxy/proxy.py +++ b/src/fastmcp/server/auth/oauth_proxy/proxy.py @@ -262,7 +262,7 @@ class OAuthProxy(OAuthProvider, ConsentMixin): # JWT signing key jwt_signing_key: str | bytes | None = None, # Consent screen configuration - require_authorization_consent: bool | Literal["external"] = True, + require_authorization_consent: bool | Literal["remember", "external"] = True, consent_csp_policy: str | None = None, # Token expiry fallback fallback_access_token_expiry_seconds: int | None = None, @@ -311,12 +311,19 @@ class OAuthProxy(OAuthProvider, ConsentMixin): If bytes are provided, they will be used as-is. If a string is provided, it will be derived into a 32-byte key using PBKDF2 (1.2M iterations). If not provided, it will be derived from the upstream client secret using HKDF. - require_authorization_consent: Whether to require user consent before authorizing clients (default True). - When True, users see a consent screen before being redirected to the upstream IdP. - When False, authorization proceeds directly without user confirmation. - When "external", the built-in consent screen is skipped but no warning is - logged, indicating that consent is handled externally (e.g. by the upstream IdP). - SECURITY WARNING: Only set to False for local development or testing environments. + require_authorization_consent: Consent screen behavior (default True). + - True: always show the consent screen before redirecting to the + upstream IdP. Strongest protection against AS-in-the-middle attacks + (GHSA-6x8h-498w-gv8c). + - "remember": show the consent screen the first time, then silently + approve subsequent authorizations for the same (client_id, + redirect_uri) in the same browser. Cross-site navigations are + still prompted to block AS-in-the-middle attacks. Lower UX + friction, but weaker protection than True. + - "external": skip the built-in consent screen; consent is handled + externally (e.g. by the upstream IdP or a custom login page). + - False: skip consent entirely. SECURITY WARNING: only set to + False for local development or testing environments. consent_csp_policy: Content Security Policy for the consent page. If None (default), uses the built-in CSP policy with appropriate directives. If empty string "", disables CSP entirely (no meta tag is rendered). @@ -397,7 +404,7 @@ class OAuthProxy(OAuthProvider, ConsentMixin): self._token_endpoint_auth_method: str | None = token_endpoint_auth_method # Consent screen configuration - self._require_authorization_consent: bool | Literal["external"] = ( + self._require_authorization_consent: bool | Literal["remember", "external"] = ( require_authorization_consent ) self._consent_csp_policy: str | None = consent_csp_policy @@ -405,6 +412,13 @@ class OAuthProxy(OAuthProvider, ConsentMixin): logger.info( "Built-in consent screen disabled; consent is handled externally." ) + elif require_authorization_consent == "remember": + logger.info( + "Consent screen in 'remember' mode: silent consent on return visits " + "for previously-approved clients (with Sec-Fetch-Site gating). " + "Set require_authorization_consent=True for strongest protection " + "against AS-in-the-middle attacks (GHSA-6x8h-498w-gv8c)." + ) elif not require_authorization_consent: logger.warning( "Authorization consent screen disabled - only use for local development or testing. " @@ -785,8 +799,10 @@ class OAuthProxy(OAuthProvider, ConsentMixin): 3. Return local /consent URL; browser visits consent first 4. Consent handler redirects to upstream IdP if approved/already approved - If consent is disabled (require_authorization_consent=False), skip the consent screen - and redirect directly to the upstream IdP. + If consent is disabled (require_authorization_consent=False or "external"), + skip the consent screen and redirect directly to the upstream IdP. In + "remember" mode, still route through /consent so the cookie lookup and + Sec-Fetch-Site gating can run. """ # Security check: validate client's requested resource matches this server # This prevents tokens intended for one server from being used on another @@ -863,8 +879,10 @@ class OAuthProxy(OAuthProvider, ConsentMixin): ttl=15 * 60, # Auto-expire after 15 minutes ) - # If consent is disabled or handled externally, skip consent screen - if self._require_authorization_consent is not True: + # If consent is disabled or handled externally, skip consent screen. + # "remember" mode still routes through /consent so cookie lookup and + # Sec-Fetch-Site gating can run. + if self._require_authorization_consent in (False, "external"): upstream_url = self._build_upstream_authorize_url( txn_id, transaction.model_dump() ) @@ -1971,8 +1989,10 @@ class OAuthProxy(OAuthProvider, ConsentMixin): # 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 - # IdP URL) won't have this cookie and will be rejected. - if self._require_authorization_consent is True: + # IdP URL) won't have this cookie and will be rejected. "remember" + # mode still issues consent tokens (on silent approval or HTML + # approval), so both code paths need binding verification. + if self._require_authorization_consent in (True, "remember"): consent_token = transaction_model.consent_token if not consent_token: logger.error("Transaction %s missing consent_token", txn_id) diff --git a/src/fastmcp/server/auth/oidc_proxy.py b/src/fastmcp/server/auth/oidc_proxy.py index 08c746f0b..549c375ad 100644 --- a/src/fastmcp/server/auth/oidc_proxy.py +++ b/src/fastmcp/server/auth/oidc_proxy.py @@ -225,7 +225,7 @@ class OIDCProxy(OAuthProxy): # Token validation configuration token_endpoint_auth_method: str | None = None, # Consent screen configuration - require_authorization_consent: bool | Literal["external"] = True, + require_authorization_consent: bool | Literal["remember", "external"] = True, consent_csp_policy: str | None = None, forward_resource: bool = True, # Extra parameters diff --git a/src/fastmcp/server/auth/providers/auth0.py b/src/fastmcp/server/auth/providers/auth0.py index 601f314ef..28c50d060 100644 --- a/src/fastmcp/server/auth/providers/auth0.py +++ b/src/fastmcp/server/auth/providers/auth0.py @@ -72,7 +72,7 @@ class Auth0Provider(OIDCProxy): allowed_client_redirect_uris: list[str] | None = None, client_storage: AsyncKeyValue | None = None, jwt_signing_key: str | bytes | None = None, - require_authorization_consent: bool | Literal["external"] = True, + require_authorization_consent: bool | Literal["remember", "external"] = True, consent_csp_policy: str | None = None, forward_resource: bool = True, ) -> None: diff --git a/src/fastmcp/server/auth/providers/aws.py b/src/fastmcp/server/auth/providers/aws.py index 6f2cfcd56..6837dc5b5 100644 --- a/src/fastmcp/server/auth/providers/aws.py +++ b/src/fastmcp/server/auth/providers/aws.py @@ -133,7 +133,7 @@ class AWSCognitoProvider(OIDCProxy): allowed_client_redirect_uris: list[str] | None = None, client_storage: AsyncKeyValue | None = None, jwt_signing_key: str | bytes | None = None, - require_authorization_consent: bool | Literal["external"] = True, + require_authorization_consent: bool | Literal["remember", "external"] = True, consent_csp_policy: str | None = None, forward_resource: bool = True, ): diff --git a/src/fastmcp/server/auth/providers/azure.py b/src/fastmcp/server/auth/providers/azure.py index d336e84ac..0f0763b97 100644 --- a/src/fastmcp/server/auth/providers/azure.py +++ b/src/fastmcp/server/auth/providers/azure.py @@ -112,7 +112,7 @@ class AzureProvider(OAuthProxy): allowed_client_redirect_uris: list[str] | None = None, client_storage: AsyncKeyValue | None = None, jwt_signing_key: str | bytes | None = None, - require_authorization_consent: bool | Literal["external"] = True, + require_authorization_consent: bool | Literal["remember", "external"] = True, consent_csp_policy: str | None = None, forward_resource: bool = True, base_authority: str = "login.microsoftonline.com", diff --git a/src/fastmcp/server/auth/providers/clerk.py b/src/fastmcp/server/auth/providers/clerk.py index b465faf39..b17261573 100644 --- a/src/fastmcp/server/auth/providers/clerk.py +++ b/src/fastmcp/server/auth/providers/clerk.py @@ -286,7 +286,7 @@ class ClerkProvider(OAuthProxy): allowed_client_redirect_uris: list[str] | None = None, client_storage: AsyncKeyValue | None = None, jwt_signing_key: str | bytes | None = None, - require_authorization_consent: bool | Literal["external"] = True, + require_authorization_consent: bool | Literal["remember", "external"] = True, consent_csp_policy: str | None = None, forward_resource: bool = True, extra_authorize_params: dict[str, str] | None = None, diff --git a/src/fastmcp/server/auth/providers/discord.py b/src/fastmcp/server/auth/providers/discord.py index 333001cfa..edf407ae1 100644 --- a/src/fastmcp/server/auth/providers/discord.py +++ b/src/fastmcp/server/auth/providers/discord.py @@ -204,7 +204,7 @@ class DiscordProvider(OAuthProxy): allowed_client_redirect_uris: list[str] | None = None, client_storage: AsyncKeyValue | None = None, jwt_signing_key: str | bytes | None = None, - require_authorization_consent: bool | Literal["external"] = True, + require_authorization_consent: bool | Literal["remember", "external"] = True, consent_csp_policy: str | None = None, forward_resource: bool = True, http_client: httpx.AsyncClient | None = None, diff --git a/src/fastmcp/server/auth/providers/github.py b/src/fastmcp/server/auth/providers/github.py index 0655a8bd5..57ee16799 100644 --- a/src/fastmcp/server/auth/providers/github.py +++ b/src/fastmcp/server/auth/providers/github.py @@ -219,7 +219,7 @@ class GitHubProvider(OAuthProxy): allowed_client_redirect_uris: list[str] | None = None, client_storage: AsyncKeyValue | None = None, jwt_signing_key: str | bytes | None = None, - require_authorization_consent: bool | Literal["external"] = True, + require_authorization_consent: bool | Literal["remember", "external"] = True, consent_csp_policy: str | None = None, forward_resource: bool = True, http_client: httpx.AsyncClient | None = None, diff --git a/src/fastmcp/server/auth/providers/google.py b/src/fastmcp/server/auth/providers/google.py index 219e6c0db..55dcad17c 100644 --- a/src/fastmcp/server/auth/providers/google.py +++ b/src/fastmcp/server/auth/providers/google.py @@ -244,7 +244,7 @@ class GoogleProvider(OAuthProxy): allowed_client_redirect_uris: list[str] | None = None, client_storage: AsyncKeyValue | None = None, jwt_signing_key: str | bytes | None = None, - require_authorization_consent: bool | Literal["external"] = True, + require_authorization_consent: bool | Literal["remember", "external"] = True, consent_csp_policy: str | None = None, forward_resource: bool = True, extra_authorize_params: dict[str, str] | None = None, diff --git a/src/fastmcp/server/auth/providers/oci.py b/src/fastmcp/server/auth/providers/oci.py index 07bad2a41..ae765299d 100644 --- a/src/fastmcp/server/auth/providers/oci.py +++ b/src/fastmcp/server/auth/providers/oci.py @@ -131,7 +131,7 @@ class OCIProvider(OIDCProxy): allowed_client_redirect_uris: list[str] | None = None, client_storage: AsyncKeyValue | None = None, jwt_signing_key: str | bytes | None = None, - require_authorization_consent: bool | Literal["external"] = True, + require_authorization_consent: bool | Literal["remember", "external"] = True, consent_csp_policy: str | None = None, forward_resource: bool = True, ) -> None: diff --git a/src/fastmcp/server/auth/providers/workos.py b/src/fastmcp/server/auth/providers/workos.py index bd6d582ad..91ab83dc1 100644 --- a/src/fastmcp/server/auth/providers/workos.py +++ b/src/fastmcp/server/auth/providers/workos.py @@ -172,7 +172,7 @@ class WorkOSProvider(OAuthProxy): allowed_client_redirect_uris: list[str] | None = None, client_storage: AsyncKeyValue | None = None, jwt_signing_key: str | bytes | None = None, - require_authorization_consent: bool | Literal["external"] = True, + require_authorization_consent: bool | Literal["remember", "external"] = True, consent_csp_policy: str | None = None, forward_resource: bool = True, http_client: httpx.AsyncClient | None = None, diff --git a/tests/server/auth/test_oauth_consent_flow.py b/tests/server/auth/test_oauth_consent_flow.py index 025eb3452..b898043b8 100644 --- a/tests/server/auth/test_oauth_consent_flow.py +++ b/tests/server/auth/test_oauth_consent_flow.py @@ -95,6 +95,22 @@ def oauth_proxy_https(): ) +@pytest.fixture +def oauth_proxy_https_remember(): + """OAuthProxy in 'remember' mode for silent-consent cookie tests.""" + return OAuthProxy( + upstream_authorization_endpoint="https://github.com/login/oauth/authorize", + upstream_token_endpoint="https://github.com/login/oauth/access_token", + upstream_client_id="client-id", + upstream_client_secret="client-secret", + token_verifier=_Verifier(), + base_url="https://myserver.example", + client_storage=MemoryStore(), + jwt_signing_key="test-secret", + require_authorization_consent="remember", + ) + + async def _start_flow( proxy: OAuthProxy, client_id: str, redirect: str ) -> tuple[str, str]: @@ -587,11 +603,15 @@ class TestConsentSecurity: assert r.status_code == 200 assert r.headers.get("X-Frame-Options") == "DENY" - async def test_deny_sets_cookie_and_redirects_with_error(self, oauth_proxy_https): - """Verify denying consent sets signed cookie and redirects with error.""" + async def test_deny_sets_cookie_and_redirects_with_error( + self, oauth_proxy_https_remember + ): + """Verify denying consent in 'remember' mode sets signed cookie and redirects with error.""" client_redirect = "http://localhost:5002/callback" - txn_id, _ = await _start_flow(oauth_proxy_https, "client-b", client_redirect) - app = Starlette(routes=oauth_proxy_https.get_routes()) + txn_id, _ = await _start_flow( + oauth_proxy_https_remember, "client-b", client_redirect + ) + app = Starlette(routes=oauth_proxy_https_remember.get_routes()) with TestClient(app) as c: consent = c.get(f"/consent?txn_id={txn_id}") csrf = _extract_csrf(consent.text) @@ -617,13 +637,13 @@ class TestConsentSecurity: ) async def test_approve_sets_cookie_and_redirects_to_upstream( - self, oauth_proxy_https + self, oauth_proxy_https_remember ): - """Verify approving consent sets signed cookie and redirects to upstream.""" + """Verify approving consent in 'remember' mode sets signed cookie and redirects to upstream.""" txn_id, _ = await _start_flow( - oauth_proxy_https, "client-c", "http://localhost:5003/callback" + oauth_proxy_https_remember, "client-c", "http://localhost:5003/callback" ) - app = Starlette(routes=oauth_proxy_https.get_routes()) + app = Starlette(routes=oauth_proxy_https_remember.get_routes()) with TestClient(app) as c: consent = c.get(f"/consent?txn_id={txn_id}") csrf = _extract_csrf(consent.text) @@ -643,29 +663,33 @@ class TestConsentSecurity: set_cookie = ";\n".join(r.headers.get("set-cookie", "").splitlines()) assert "__Host-MCP_APPROVED_CLIENTS" in set_cookie - async def test_tampered_cookie_is_ignored(self, oauth_proxy_https): + async def test_tampered_cookie_is_ignored(self, oauth_proxy_https_remember): """Verify tampered approval cookie is ignored and consent page shown.""" txn_id, _ = await _start_flow( - oauth_proxy_https, "client-d", "http://localhost:5004/callback" + oauth_proxy_https_remember, "client-d", "http://localhost:5004/callback" ) - app = Starlette(routes=oauth_proxy_https.get_routes()) + app = Starlette(routes=oauth_proxy_https_remember.get_routes()) with TestClient(app) as c: # Create a tampered cookie (invalid signature) # Value format: payload.signature; using wrong signature to force failure tampered_value = "W10=.invalidsig" c.cookies.set("__Host-MCP_APPROVED_CLIENTS", tampered_value) - r = c.get(f"/consent?txn_id={txn_id}", follow_redirects=False) + r = c.get( + f"/consent?txn_id={txn_id}", + headers={"Sec-Fetch-Site": "none"}, + follow_redirects=False, + ) # Should not auto-redirect to upstream; should show consent page assert r.status_code == 200 # httpx returns a URL object; compare path or stringify assert urlparse(str(r.request.url)).path == "/consent" - async def test_autoapprove_cookie_skips_consent(self, oauth_proxy_https): - """Verify valid approval cookie auto-approves and redirects to upstream.""" + async def test_autoapprove_cookie_skips_consent(self, oauth_proxy_https_remember): + """Verify valid approval cookie auto-approves and redirects to upstream in 'remember' mode.""" client_id = "client-e" redirect = "http://localhost:5005/callback" - txn_id, _ = await _start_flow(oauth_proxy_https, client_id, redirect) - app = Starlette(routes=oauth_proxy_https.get_routes()) + txn_id, _ = await _start_flow(oauth_proxy_https_remember, client_id, redirect) + app = Starlette(routes=oauth_proxy_https_remember.get_routes()) with TestClient(app) as c: # Approve once to set approved cookie consent = c.get(f"/consent?txn_id={txn_id}") @@ -684,15 +708,188 @@ class TestConsentSecurity: # Extract approved cookie value set_cookie = ";\n".join(r.headers.get("set-cookie", "").splitlines()) m = re.search(r"__Host-MCP_APPROVED_CLIENTS=([^;]+)", set_cookie) - assert m, "approved cookie should be set" + assert m, "approved cookie should be set in 'remember' mode" approved_cookie = m.group(1) # Start a new flow for the same client and redirect - new_txn, _ = await _start_flow(oauth_proxy_https, client_id, redirect) + new_txn, _ = await _start_flow( + oauth_proxy_https_remember, client_id, redirect + ) # Should auto-redirect to upstream when visiting consent due to cookie + # (requires a safe Sec-Fetch-Site value — "none" simulates a direct + # client-initiated navigation like an MCP host opening the browser). c.cookies.set("__Host-MCP_APPROVED_CLIENTS", approved_cookie) - r2 = c.get(f"/consent?txn_id={new_txn}", follow_redirects=False) + r2 = c.get( + f"/consent?txn_id={new_txn}", + headers={"Sec-Fetch-Site": "none"}, + follow_redirects=False, + ) assert r2.status_code in (302, 303) assert r2.headers.get("location", "").startswith( "https://github.com/login/oauth/authorize" ) + + +class TestConsentModes: + """Tests for the three consent modes: True (always), 'remember' (silent), 'external'. + + Covers the default-behavior change in GHSA-6x8h-498w-gv8c: True no longer + silently approves return visits, and 'remember' mode gates silent consent + on Sec-Fetch-Site to block AS-in-the-middle attacks. + """ + + async def test_default_mode_does_not_set_approval_cookie(self, oauth_proxy_https): + """Approving in default (True) mode must NOT set a long-lived approval cookie. + + In 'always prompt' mode there's no reason to remember approvals, and + writing the cookie would only create a no-op attack surface. + """ + txn_id, _ = await _start_flow( + oauth_proxy_https, "client-default", "http://localhost:7001/callback" + ) + 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) + r = c.post( + "/consent", + data={"action": "approve", "txn_id": txn_id, "csrf_token": csrf}, + follow_redirects=False, + ) + assert r.status_code in (302, 303) + set_cookie = ";\n".join(r.headers.get("set-cookie", "").splitlines()) + # Consent binding cookie still set for the in-flight transaction + assert "__Host-MCP_CONSENT_BINDING" in set_cookie + # But no remembered-approval cookie + assert "MCP_APPROVED_CLIENTS" not in set_cookie + + async def test_default_mode_ignores_existing_approval_cookie( + self, oauth_proxy_https_remember, oauth_proxy_https + ): + """A valid approval cookie is ignored in default (True) mode — always prompts. + + Uses the 'remember' proxy to generate a legitimately-signed cookie, then + replays it against a default-mode proxy (same jwt_signing_key) to confirm + the consent page is still shown. + """ + # Both fixtures use the same signing key, so cookies generated by one + # verify against the other. + client_id = "client-xmode" + redirect = "http://localhost:7002/callback" + txn_id, _ = await _start_flow(oauth_proxy_https_remember, client_id, redirect) + app_remember = Starlette(routes=oauth_proxy_https_remember.get_routes()) + with TestClient(app_remember) 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) + r = c.post( + "/consent", + data={"action": "approve", "txn_id": txn_id, "csrf_token": csrf}, + follow_redirects=False, + ) + set_cookie_remember = ";\n".join( + r.headers.get("set-cookie", "").splitlines() + ) + m = re.search(r"__Host-MCP_APPROVED_CLIENTS=([^;]+)", set_cookie_remember) + assert m, "remember-mode proxy should have set approval cookie" + approved_cookie = m.group(1) + + # Now replay against the default-mode proxy for the same client/redirect. + new_txn, _ = await _start_flow(oauth_proxy_https, client_id, redirect) + app_default = Starlette(routes=oauth_proxy_https.get_routes()) + with TestClient(app_default) as c: + c.cookies.set("__Host-MCP_APPROVED_CLIENTS", approved_cookie) + r = c.get( + f"/consent?txn_id={new_txn}", + headers={"Sec-Fetch-Site": "none"}, + follow_redirects=False, + ) + # Default mode: HTML consent page, no silent redirect to upstream. + assert r.status_code == 200 + assert "text/html" in r.headers.get("content-type", "") + + async def test_remember_mode_cross_site_forces_prompt( + self, oauth_proxy_https_remember + ): + """Sec-Fetch-Site=cross-site must skip silent consent even with valid cookie. + + Blocks the GHSA-6x8h-498w-gv8c AS-in-the-middle scenario: attacker + redirects the victim's browser to /authorize from a third-party origin, + which browsers mark cross-site. Legit local-host flows are Sec-Fetch-Site=none. + """ + client_id = "client-cross" + redirect = "http://localhost:7003/callback" + txn_id, _ = await _start_flow(oauth_proxy_https_remember, client_id, redirect) + app = Starlette(routes=oauth_proxy_https_remember.get_routes()) + with TestClient(app) as c: + # Seed a valid approval cookie via the normal consent flow. + 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) + r = c.post( + "/consent", + data={"action": "approve", "txn_id": txn_id, "csrf_token": csrf}, + follow_redirects=False, + ) + set_cookie = ";\n".join(r.headers.get("set-cookie", "").splitlines()) + m = re.search(r"__Host-MCP_APPROVED_CLIENTS=([^;]+)", set_cookie) + assert m + approved_cookie = m.group(1) + + # Start a new flow and simulate a cross-site navigation. + new_txn, _ = await _start_flow( + oauth_proxy_https_remember, client_id, redirect + ) + c.cookies.set("__Host-MCP_APPROVED_CLIENTS", approved_cookie) + r2 = c.get( + f"/consent?txn_id={new_txn}", + headers={"Sec-Fetch-Site": "cross-site"}, + follow_redirects=False, + ) + # Silent consent skipped; HTML prompt shown. + assert r2.status_code == 200 + assert "text/html" in r2.headers.get("content-type", "") + + async def test_remember_mode_missing_sec_fetch_site_fails_closed( + self, oauth_proxy_https_remember + ): + """Missing Sec-Fetch-Site header must fall back to the explicit prompt. + + Legacy clients that don't send the header degrade to the safer behavior + rather than granting silent consent. + """ + client_id = "client-nofetch" + redirect = "http://localhost:7004/callback" + txn_id, _ = await _start_flow(oauth_proxy_https_remember, client_id, redirect) + app = Starlette(routes=oauth_proxy_https_remember.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) + r = c.post( + "/consent", + data={"action": "approve", "txn_id": txn_id, "csrf_token": csrf}, + follow_redirects=False, + ) + set_cookie = ";\n".join(r.headers.get("set-cookie", "").splitlines()) + m = re.search(r"__Host-MCP_APPROVED_CLIENTS=([^;]+)", set_cookie) + assert m + approved_cookie = m.group(1) + + new_txn, _ = await _start_flow( + oauth_proxy_https_remember, client_id, redirect + ) + c.cookies.set("__Host-MCP_APPROVED_CLIENTS", approved_cookie) + # No Sec-Fetch-Site header at all. + r2 = c.get(f"/consent?txn_id={new_txn}", follow_redirects=False) + assert r2.status_code == 200 + assert "text/html" in r2.headers.get("content-type", "") diff --git a/tests/server/auth/test_oauth_consent_page.py b/tests/server/auth/test_oauth_consent_page.py index bd394db74..a0a4ea050 100644 --- a/tests/server/auth/test_oauth_consent_page.py +++ b/tests/server/auth/test_oauth_consent_page.py @@ -48,6 +48,22 @@ def oauth_proxy_https(): ) +@pytest.fixture +def oauth_proxy_https_remember(): + """OAuthProxy in 'remember' mode for silent-consent cookie tests.""" + return OAuthProxy( + upstream_authorization_endpoint="https://github.com/login/oauth/authorize", + upstream_token_endpoint="https://github.com/login/oauth/access_token", + upstream_client_id="client-id", + upstream_client_secret="client-secret", + token_verifier=_Verifier(), + base_url="https://myserver.example", + client_storage=MemoryStore(), + jwt_signing_key="test-secret", + require_authorization_consent="remember", + ) + + async def _start_flow( proxy: OAuthProxy, client_id: str, redirect: str ) -> tuple[str, str]: @@ -476,12 +492,14 @@ class TestConsentBindingCookie: set_cookie_header = r.headers.get("set-cookie", "") assert "__Host-MCP_CONSENT_BINDING" in set_cookie_header - async def test_auto_approve_sets_consent_binding_cookie(self, oauth_proxy_https): - """Auto-approve path (previously approved client) must also set the binding cookie.""" + async def test_auto_approve_sets_consent_binding_cookie( + self, oauth_proxy_https_remember + ): + """Auto-approve path (previously approved client in 'remember' mode) must also set the binding cookie.""" client_id = "client-autobinding" redirect = "http://localhost:6002/callback" - txn_id, _ = await _start_flow(oauth_proxy_https, client_id, redirect) - app = Starlette(routes=oauth_proxy_https.get_routes()) + txn_id, _ = await _start_flow(oauth_proxy_https_remember, client_id, redirect) + app = Starlette(routes=oauth_proxy_https_remember.get_routes()) with TestClient(app) as c: # First: approve manually to get the approved cookie consent = c.get(f"/consent?txn_id={txn_id}") @@ -503,9 +521,16 @@ class TestConsentBindingCookie: approved_cookie = m.group(1) # Second: start new flow, auto-approve should set binding cookie - new_txn, _ = await _start_flow(oauth_proxy_https, client_id, redirect) + # (Sec-Fetch-Site=none simulates a direct client-initiated navigation.) + new_txn, _ = await _start_flow( + oauth_proxy_https_remember, client_id, redirect + ) c.cookies.set("__Host-MCP_APPROVED_CLIENTS", approved_cookie) - r2 = c.get(f"/consent?txn_id={new_txn}", follow_redirects=False) + r2 = c.get( + f"/consent?txn_id={new_txn}", + headers={"Sec-Fetch-Site": "none"}, + follow_redirects=False, + ) assert r2.status_code in (302, 303) set_cookie_header = r2.headers.get("set-cookie", "") assert "__Host-MCP_CONSENT_BINDING" in set_cookie_header