diff --git a/litellm/proxy/_experimental/mcp_server/byok_oauth_endpoints.py b/litellm/proxy/_experimental/mcp_server/byok_oauth_endpoints.py index 40cb3edc857..ab753bae4fb 100644 --- a/litellm/proxy/_experimental/mcp_server/byok_oauth_endpoints.py +++ b/litellm/proxy/_experimental/mcp_server/byok_oauth_endpoints.py @@ -31,6 +31,7 @@ from litellm.proxy._experimental.mcp_server.discoverable_endpoints import ( get_request_base_url, ) from litellm.proxy._experimental.mcp_server.oauth_utils import ( + TOKEN_NO_CACHE_HEADERS, validate_loopback_redirect_uri, ) from litellm.proxy._types import UserAPIKeyAuth @@ -73,18 +74,13 @@ def _purge_expired_codes() -> None: del _byok_auth_codes[k] -# RFC 6749 §5.1 / OAuth 2.1 draft-15 §4.1.3: token-endpoint responses must -# not be cached (both success and error bodies may reveal secrets). -_TOKEN_NO_CACHE_HEADERS = {"Cache-Control": "no-store", "Pragma": "no-cache"} - - def _oauth_token_error(code: str, status: int = 400) -> JSONResponse: """RFC 6749 §5.2 token-endpoint error body: ``{"error": ""}``. FastAPI's default ``HTTPException`` renders ``{"detail": ...}`` which spec-compliant OAuth clients parsing the ``error`` field won't recognize. """ return JSONResponse( - status_code=status, content={"error": code}, headers=_TOKEN_NO_CACHE_HEADERS + status_code=status, content={"error": code}, headers=TOKEN_NO_CACHE_HEADERS ) @@ -876,5 +872,5 @@ async def byok_token( "token_type": "bearer", "expires_in": 3600, }, - headers=_TOKEN_NO_CACHE_HEADERS, + headers=TOKEN_NO_CACHE_HEADERS, ) diff --git a/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py b/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py index 80127866d32..13b19aa2d65 100644 --- a/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py +++ b/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py @@ -11,6 +11,7 @@ from litellm.llms.custom_httpx.http_handler import ( httpxSpecialProvider, ) from litellm.proxy._experimental.mcp_server.oauth_utils import ( + TOKEN_NO_CACHE_HEADERS, validate_loopback_redirect_uri, ) from litellm.proxy.auth.ip_address_utils import IPAddressUtils @@ -481,7 +482,8 @@ async def exchange_token_with_server( if "scope" in token_response and token_response["scope"]: result["scope"] = token_response["scope"] - return JSONResponse(result) + # RFC 6749 §5.1: token responses must not be cached. + return JSONResponse(result, headers=TOKEN_NO_CACHE_HEADERS) async def register_client_with_server( @@ -648,20 +650,26 @@ async def token_endpoint( @router.get("/callback") async def callback(code: str, state: str): try: - # Decode the state hash to get base_url, original state, and PKCE params state_data = decode_state_hash(state) base_url = state_data["base_url"] original_state = state_data["original_state"] - # Forward code and original state back to client - params = {"code": code, "state": original_state} + # Re-validate loopback at the sink. /authorize rejects non-loopback + # redirect_uri before encoding into state, but encrypted states + # minted before that check was added have no expiry and remain + # valid indefinitely. Validating here blocks the open-redirect + + # code-theft primitive even for pre-fix states. + validate_loopback_redirect_uri(base_url) - # Forward to client's callback endpoint + params = {"code": code, "state": original_state} complete_returned_url = f"{base_url}?{urlencode(params)}" return RedirectResponse(url=complete_returned_url, status_code=302) + except HTTPException: + # Re-raise so a non-loopback base_url surfaces as 400 instead of + # a generic "authentication incomplete" redirect. + raise except Exception: - # fallback if state hash not found return HTMLResponse( "Authentication incomplete. You can close this window." ) diff --git a/litellm/proxy/_experimental/mcp_server/oauth_utils.py b/litellm/proxy/_experimental/mcp_server/oauth_utils.py index 3d17bbf2586..ad1b97b1ac0 100644 --- a/litellm/proxy/_experimental/mcp_server/oauth_utils.py +++ b/litellm/proxy/_experimental/mcp_server/oauth_utils.py @@ -6,6 +6,10 @@ from urllib.parse import urlparse from fastapi import HTTPException +# RFC 6749 §5.1 / OAuth 2.1 draft-15 §4.1.3: token-endpoint responses +# must not be cached — both success and error bodies may reveal secrets. +TOKEN_NO_CACHE_HEADERS = {"Cache-Control": "no-store", "Pragma": "no-cache"} + def validate_loopback_redirect_uri(redirect_uri: str) -> None: """Require a loopback ``redirect_uri`` (OAuth 2.1 §4.1.2.1 + RFC 8252 diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/test_discoverable_endpoints.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_discoverable_endpoints.py index 5957c45d6a3..ae3c16ce8b0 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/test_discoverable_endpoints.py +++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_discoverable_endpoints.py @@ -1920,3 +1920,88 @@ async def test_authorize_endpoint_accepts_ipv4_loopback_range_and_ipv6_full_form state="s", ) assert response.status_code == 307, f"{uri} should be accepted" + + +@pytest.mark.asyncio +async def test_callback_revalidates_loopback_on_decoded_base_url(): + """VERIA-57 root cause B defense-in-depth: an encrypted state minted + before the /authorize validation was added has no expiry and stays + valid. /callback must re-validate the decoded base_url so those + stale states can't be used as an open-redirect + code-theft + primitive.""" + from litellm.proxy._experimental.mcp_server.discoverable_endpoints import ( + callback, + ) + + with patch( + "litellm.proxy._experimental.mcp_server.discoverable_endpoints.decode_state_hash" + ) as mock_decode: + mock_decode.return_value = { + "base_url": "https://attacker.example.com/cb", + "original_state": "s", + "code_challenge": None, + "code_challenge_method": None, + "client_redirect_uri": "https://attacker.example.com/cb", + } + with pytest.raises(HTTPException) as exc_info: + await callback(code="stolen_code", state="encrypted_stale_state") + assert exc_info.value.status_code == 400 + + +@pytest.mark.asyncio +async def test_token_endpoint_sets_no_store_cache_control(): + """RFC 6749 §5.1 / OAuth 2.1 draft-15 §4.1.3: the token response + contains an access token (and possibly a refresh token) — it MUST + NOT be cached by intermediaries or the client.""" + from fastapi import Request + + from litellm.proxy._experimental.mcp_server.discoverable_endpoints import ( + exchange_token_with_server, + ) + from litellm.proxy._types import MCPTransport + from litellm.types.mcp import MCPAuth + from litellm.types.mcp_server.mcp_server_manager import MCPServer + + server = MCPServer( + server_id="t", + name="t", + server_name="t", + alias="t", + transport=MCPTransport.http, + auth_type=MCPAuth.oauth2, + client_id="cid", + client_secret="cs", + authorization_url="https://provider.com/oauth/authorize", + token_url="https://provider.com/oauth/token", + ) + mock_request = MagicMock(spec=Request) + mock_request.base_url = "https://litellm.example.com/" + mock_request.headers = {} + + fake_http_response = MagicMock() + fake_http_response.json.return_value = { + "access_token": "tok", + "token_type": "Bearer", + "expires_in": 3600, + } + fake_http_response.raise_for_status = MagicMock() + fake_http_client = MagicMock() + fake_http_client.post = AsyncMock(return_value=fake_http_response) + + with patch( + "litellm.proxy._experimental.mcp_server.discoverable_endpoints.get_async_httpx_client", + return_value=fake_http_client, + ): + response = await exchange_token_with_server( + request=mock_request, + mcp_server=server, + grant_type="authorization_code", + code="c", + redirect_uri="http://127.0.0.1:3000/cb", + client_id="cid", + client_secret=None, + code_verifier=None, + ) + + assert response.headers["cache-control"] == "no-store" + assert response.headers["pragma"] == "no-cache"