mirror of
https://github.com/PrefectHQ/fastmcp.git
synced 2026-08-20 04:24:17 +02:00
Forward IdP auth errors to MCP client instead of showing HTML error page (#4293)
Co-authored-by: Jeremiah Lowin <153965+jlowin@users.noreply.github.com>
This commit is contained in:
parent
ea63d06241
commit
7f2d034f4d
2 changed files with 99 additions and 14 deletions
|
|
@ -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",
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue