diff --git a/fastmcp_slim/fastmcp/server/auth/oauth_proxy/proxy.py b/fastmcp_slim/fastmcp/server/auth/oauth_proxy/proxy.py index 01e63deed..938dfff9b 100644 --- a/fastmcp_slim/fastmcp/server/auth/oauth_proxy/proxy.py +++ b/fastmcp_slim/fastmcp/server/auth/oauth_proxy/proxy.py @@ -2130,22 +2130,7 @@ class OAuthProxy(OAuthProvider, ConsentMixin): txn_id = request.query_params.get("state") error = request.query_params.get("error") - if error: - error_description = request.query_params.get("error_description") - logger.error( - "IdP callback error: %s - %s", - error, - error_description, - ) - # Show error page to user - html_content = create_error_html( - error_title="OAuth Error", - error_message=f"Authentication failed: {error_description or 'Unknown error'}", - error_details={"Error Code": error} if error else None, - ) - return HTMLResponse(content=html_content, status_code=400) - - if not idp_code or not txn_id: + if not idp_code and not error: logger.error("IdP callback missing code or transaction ID") html_content = create_error_html( error_title="OAuth Error", @@ -2154,8 +2139,39 @@ class OAuthProxy(OAuthProvider, ConsentMixin): return HTMLResponse(content=html_content, status_code=400) # Look up transaction data - transaction_model = await self._transaction_store.get(key=txn_id) - if not transaction_model: + transaction_model = ( + await self._transaction_store.get(key=txn_id) if txn_id else None + ) + + if error: + error_description = request.query_params.get("error_description") + logger.error( + "IdP callback error: %s - %s", + error, + error_description, + ) + if transaction_model: + # Forward the error to the client's redirect_uri (RFC 6749 §4.1.2.1) + 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)}", + status_code=302, + ) + # No trusted redirect_uri available — show local error page + html_content = create_error_html( + error_title="OAuth Error", + error_message=f"Authentication failed: {error_description or 'Unknown error'}", + error_details={"Error Code": error}, + ) + return HTMLResponse(content=html_content, status_code=400) + if not txn_id or not transaction_model: logger.error("IdP callback with invalid transaction ID: %s", txn_id) html_content = create_error_html( error_title="OAuth Error", diff --git a/tests/server/auth/oauth_proxy/test_oauth_proxy.py b/tests/server/auth/oauth_proxy/test_oauth_proxy.py index 846f82eaa..dbc2ee0e6 100644 --- a/tests/server/auth/oauth_proxy/test_oauth_proxy.py +++ b/tests/server/auth/oauth_proxy/test_oauth_proxy.py @@ -1,5 +1,8 @@ """Tests for OAuth proxy initialization and configuration.""" +import time +from urllib.parse import parse_qs, urlparse + import httpx import pytest from authlib.integrations.httpx_client import AsyncOAuth2Client @@ -7,6 +10,7 @@ from key_value.aio.stores.memory import MemoryStore from starlette.applications import Starlette from fastmcp.server.auth.oauth_proxy import OAuthProxy +from fastmcp.server.auth.oauth_proxy.models import OAuthTransaction class TestOAuthProxyInitialization: @@ -303,3 +307,68 @@ class TestOptionalClientSecret: signed = proxy._sign_cookie("test-payload") assert proxy._verify_cookie(signed) == "test-payload" assert proxy._verify_cookie("tampered.payload") is None + + +class TestIdpCallbackErrorForwarding: + """Tests for error forwarding in the IdP callback.""" + + async def test_error_with_valid_transaction_redirects_to_client(self, oauth_proxy): + """When the IdP returns an error and the transaction exists, the proxy + must forward the error to the client's redirect_uri rather than showing + an HTML error page.""" + txn_id = "test-txn-123" + client_redirect_uri = "http://localhost:12345/callback" + client_state = "client-state-abc" + + transaction = OAuthTransaction( + txn_id=txn_id, + client_id="test-client", + client_redirect_uri=client_redirect_uri, + client_state=client_state, + 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&error_description=User+denied+access&state={txn_id}" + ) + + assert response.status_code == 302 + location = response.headers["location"] + parsed = urlparse(location) + assert ( + parsed.scheme + "://" + parsed.netloc + parsed.path == client_redirect_uri + ) + params = parse_qs(parsed.query) + assert params["error"] == ["access_denied"] + assert params["error_description"] == ["User denied access"] + assert params["state"] == [client_state] + + 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 + trusted client redirect_uri to forward to.""" + 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( + "/auth/callback?error=access_denied&state=nonexistent-txn" + ) + + assert response.status_code == 400