From 7a63e516252a5fc78a6150da4b3fc6cd03bab1c6 Mon Sep 17 00:00:00 2001 From: Tin Chi Lo Date: Sat, 11 Jul 2026 16:36:08 -0700 Subject: [PATCH] fix(mcp): harden the bridge token mint (multi-lens review pass) Findings from a full adversarial review of the mint path across security, correctness, error-handling, concurrency, and OAuth-protocol dimensions. - expires_in coercion is now total: int(float(...)) can raise OverflowError on Infinity / a giant numeric string, which escaped the ValueError/TypeError catch and 500'd the token endpoint. Unified to catch OverflowError too. - Resolve the litellm identity BEFORE exchanging the single-use upstream code, so a missing or transiently-unresolvable identity fails closed with invalid_request without burning the code (the mint re-resolves via a cache hit). - The no-identity failure is now an RFC 6749 5.2-shaped invalid_request (JSONResponse, top-level error, no-store) instead of a detail-wrapped HTTPException, matching the BYOK OAuth endpoint. - EnvelopeTooLarge (upstream token too big to seal) surfaces a 502, not a 500. - The upstream refresh_token is no longer sealed into the envelope: the edge never consumes it, so it was dead weight embedding a long-lived upstream credential in the client bearer and enlarging the envelope; refresh is a follow-up (a dedicated refresh-envelope). Security review found no exploitable defect (forgery, cross-server/user replay, leakage, confused-deputy all closed). Regression tests cover the OverflowError, the code-not-burned path, the RFC-shaped error, the 502, and the dropped refresh. --- .../mcp_server/discoverable_endpoints.py | 76 ++++++++++++------- .../mcp_server/test_discoverable_endpoints.py | 71 +++++++++++++++-- 2 files changed, 116 insertions(+), 31 deletions(-) diff --git a/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py b/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py index 00586a6afb3..4f72c2cb8c2 100644 --- a/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py +++ b/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py @@ -373,7 +373,7 @@ def _active_key_user_id(key_obj: "UserAPIKeyAuth") -> str | None: return key_obj.user_id if _key_is_active(key_obj) else None -async def _resolve_active_litellm_key(request: Request) -> Tuple[str, "UserAPIKeyAuth"] | None: +async def _resolve_active_litellm_key(request: Request) -> tuple[str, "UserAPIKeyAuth"] | None: """Resolve the presented litellm key to ``(its hash, the live active key record)``, or ``None`` when the key is absent, unresolvable, or blocked/expired. @@ -714,19 +714,17 @@ def _coerce_positive_expires_in(value: object) -> int | None: usable number. IdPs return it as an int, a float (``3600.0``), or a numeric string (``"3600"``); accepting only ``int`` would drop the float/string cases to ``None`` and fall back to the envelope's 1h cap, which can outlive a shorter-lived upstream token and forward a stale bearer. - ``bool`` is excluded (it is an ``int`` subclass but never a real lifetime).""" - if isinstance(value, bool): + ``bool`` is excluded (it is an ``int`` subclass but never a real lifetime). Total over hostile + input: a non-numeric string, ``NaN``, ``Infinity``, or an over-large value all resolve to + ``None`` rather than raising (``int(float(...))`` can raise ``ValueError`` or ``OverflowError``), + so a malformed upstream ``expires_in`` never surfaces as a 500 from the token endpoint.""" + if isinstance(value, bool) or not isinstance(value, (int, float, str)): return None - if isinstance(value, (int, float)): - seconds = int(value) - return seconds if seconds > 0 else None - if isinstance(value, str): - try: - seconds = int(float(value.strip())) - except (ValueError, TypeError): - return None - return seconds if seconds > 0 else None - return None + try: + seconds = int(float(value)) + except (ValueError, TypeError, OverflowError): + return None + return seconds if seconds > 0 else None def _bridge_grant_from_token_response(token_response: object) -> Optional["UpstreamTokenGrant"]: @@ -744,17 +742,38 @@ def _bridge_grant_from_token_response(token_response: object) -> Optional["Upstr if not isinstance(access, str) or not access: return None token_type = token_response.get("token_type") - refresh = token_response.get("refresh_token") scope = token_response.get("scope") return UpstreamTokenGrant( access_token=SecretStr(access), token_type=token_type if isinstance(token_type, str) and token_type else "Bearer", - refresh_token=SecretStr(refresh) if isinstance(refresh, str) and refresh else None, + # The upstream refresh_token is deliberately NOT sealed: the edge never consumes it (it forwards + # only token_type + access_token), so it would be dead weight embedding a long-lived upstream + # credential in the client-held bearer, and it enlarges the envelope. Refresh support is a + # follow-up (a dedicated refresh-envelope); the client re-runs authorization_code at the cap. + refresh_token=None, scope=scope if isinstance(scope, str) and scope else None, expires_in=_coerce_positive_expires_in(token_response.get("expires_in")), ) +def _bridge_invalid_request_response() -> JSONResponse: + """RFC 6749 §5.2-shaped ``invalid_request`` for a bridge token exchange that carries no resolvable + litellm identity. Returned (not raised) so the OAuth error members sit at the top level rather than + wrapped in FastAPI's ``detail``, with the no-store token-endpoint headers, matching the BYOK OAuth + endpoint and what a strict DCR client parses per RFC 6749 §5.2.""" + return JSONResponse( + status_code=400, + content={ + "error": "invalid_request", + "error_description": ( + "this server issues a gateway-bound credential; send a litellm credential " + "(x-litellm-api-key or Authorization) on the token request" + ), + }, + headers=TOKEN_NO_CACHE_HEADERS, + ) + + async def _mint_bridge_delegate_token_response( request: Request, mcp_server: MCPServer, token_response: object ) -> JSONResponse: @@ -784,16 +803,7 @@ async def _mint_bridge_delegate_token_response( key_hash = await _extract_active_key_hash_from_request(request) if not key_hash: - raise HTTPException( - status_code=400, - detail={ - "error": "invalid_request", - "error_description": ( - "this server issues a gateway-bound credential; send a litellm credential " - "(x-litellm-api-key or Authorization) on the token request" - ), - }, - ) + return _bridge_invalid_request_response() grant = _bridge_grant_from_token_response(token_response) if grant is None: @@ -804,7 +814,11 @@ async def _mint_bridge_delegate_token_response( identity = EnvelopeIdentity(server_id=mcp_server.server_id, key_hash=key_hash) sealed = build_bridge_token_response(identity, grant, keys, now) if not isinstance(sealed, SealedEnvelope): - raise HTTPException(status_code=500, detail="Failed to mint the gateway-bound credential") + # build_bridge_token_response returns EnvelopeTooLarge as a value when the upstream token is + # too large to seal; that is an upstream-payload condition, so surface a 502, not a 500. + raise HTTPException( + status_code=502, detail="Upstream token is too large to seal into a gateway-bound credential" + ) expires_in = max(1, int((sealed.expires_at - now).total_seconds())) body = {"access_token": sealed.token.get_secret_value(), "token_type": "Bearer", "expires_in": expires_in} @@ -884,6 +898,16 @@ async def exchange_token_with_server( if code_verifier: token_data["code_verifier"] = code_verifier + # For a bridge oauth_delegate mint, resolve the litellm identity BEFORE exchanging the + # single-use upstream code. A missing or transiently-unresolvable identity then fails closed + # with invalid_request without consuming the code, so the client can retry the same code + # instead of being forced back through the full interactive authorize. The mint below + # re-resolves authoritatively; get_key_object is cache-first, so that second call is a cache + # hit and this adds no extra database round-trip. + if mcp_server.is_oauth_delegate and mcp_server.is_dcr_bridge: + if not await _extract_active_key_hash_from_request(request): + return _bridge_invalid_request_response() + async_client = get_async_httpx_client(llm_provider=httpxSpecialProvider.Oauth2Check) response = await async_client.post( mcp_server.token_url, 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 0dc8d8116df..8aad8b24e88 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 @@ -4365,7 +4365,7 @@ async def test_register_bridge_relay_never_persists(): _BRIDGE_MASTER_KEY = "sk-bridge-producer-master-key-0123456789abcdef" -async def _exchange_for_bridge_server(server, upstream_body, key_hash): +async def _exchange_for_bridge_server(server, upstream_body, key_hash, fake_client_out=None): from litellm.proxy._experimental.mcp_server.discoverable_endpoints import ( exchange_token_with_server, ) @@ -4375,6 +4375,8 @@ async def _exchange_for_bridge_server(server, upstream_body, key_hash): fake_http_response.raise_for_status = MagicMock() fake_http_client = MagicMock() fake_http_client.post = AsyncMock(return_value=fake_http_response) + if fake_client_out is not None: + fake_client_out["client"] = fake_http_client with ( patch( @@ -4436,17 +4438,69 @@ async def test_oauth_delegate_bridge_token_exchange_mints_envelope_not_raw_token @pytest.mark.asyncio async def test_oauth_delegate_bridge_token_exchange_fails_closed_without_litellm_identity(): """Without a resolvable litellm identity on the token request, the exchange must not mint an - identity-less envelope; it returns an OAuth invalid_request so the client sends a credential.""" + identity-less envelope. It returns an RFC 6749 §5.2-shaped invalid_request (error at the top + level, not wrapped in detail) BEFORE exchanging the upstream code, so the single-use code is not + burned and the client can retry.""" from litellm.types.mcp import MCPAuth server = _bridge_server(auth_type=MCPAuth.oauth_delegate) upstream = {"access_token": "UPSTREAM-SECRET-TOKEN", "token_type": "Bearer", "expires_in": 3600} + captured: dict = {} + response = await _exchange_for_bridge_server(server, upstream, key_hash=None, fake_client_out=captured) + assert response.status_code == 400 + assert json.loads(response.body)["error"] == "invalid_request" + # identity resolution failed first, so the upstream single-use code was never exchanged (not burned) + captured["client"].post.assert_not_called() + + +@pytest.mark.asyncio +async def test_bridge_envelope_too_large_upstream_token_is_502(): + """An upstream token too large to seal into the envelope is an upstream-payload condition, so the + mint surfaces a 502 rather than a 500 (build_bridge_token_response returns EnvelopeTooLarge as a + value, and the caller maps it to a truthful status).""" + from litellm.types.mcp import MCPAuth + + server = _bridge_server(auth_type=MCPAuth.oauth_delegate) + upstream = {"access_token": "x" * 40000, "token_type": "Bearer", "expires_in": 3600} with pytest.raises(HTTPException) as exc: - await _exchange_for_bridge_server(server, upstream, key_hash=None) + await _exchange_for_bridge_server(server, upstream, key_hash="hashed-litellm-key-77") + assert exc.value.status_code == 502 - assert exc.value.status_code == 400 - assert exc.value.detail["error"] == "invalid_request" + +@pytest.mark.asyncio +async def test_bridge_envelope_does_not_seal_upstream_refresh_token(): + """The upstream refresh_token is never sealed into the client-held envelope: the edge never + consumes it and a long-lived upstream credential should not live in the client bearer. The opened + envelope's grant carries no refresh token even when the upstream returned one, and neither does + the response body.""" + from datetime import datetime, timezone + + from litellm.proxy._experimental.mcp_server.outbound_credentials.bridge_credentials import ( + envelope_keys_from_master_key, + ) + from litellm.proxy._experimental.mcp_server.outbound_credentials.envelope import ( + OpenedEnvelope, + open_envelope, + ) + from litellm.types.mcp import MCPAuth + + server = _bridge_server(auth_type=MCPAuth.oauth_delegate) + upstream = { + "access_token": "UP", + "token_type": "Bearer", + "expires_in": 3600, + "refresh_token": "UPSTREAM-REFRESH", + } + response = await _exchange_for_bridge_server(server, upstream, key_hash="hashed-litellm-key-77") + + body = json.loads(response.body) + assert "refresh_token" not in body + assert "UPSTREAM-REFRESH" not in body["access_token"] + keys = envelope_keys_from_master_key(_BRIDGE_MASTER_KEY) + opened = open_envelope(body["access_token"], keys, datetime.now(timezone.utc)) + assert isinstance(opened, OpenedEnvelope) + assert opened.grant.refresh_token is None def test_bridge_grant_coerces_numeric_expires_in(): @@ -4470,6 +4524,13 @@ def test_bridge_grant_coerces_numeric_expires_in(): assert ei(0) is None assert ei(-5) is None assert ei(None) is None + # hostile numerics must not raise (int(float(...)) can OverflowError) -> None + assert ei("inf") is None + assert ei("1e999") is None + assert ei("-inf") is None + assert ei("nan") is None + assert ei(float("inf")) is None + assert ei(10**400) is None @pytest.mark.asyncio