From 2f349f6cd18194a67b7c5985e989de5009bec4c9 Mon Sep 17 00:00:00 2001 From: Tin Chi Lo Date: Sat, 11 Jul 2026 16:16:52 -0700 Subject: [PATCH] fix(mcp): coerce numeric expires_in and make the active-key check total Two correctness gaps in the bridge mint. _bridge_grant_from_token_response only accepted an int expires_in, dropping a float (3600.0) or numeric-string ('3600') lifetime to None so the envelope fell back to its 1h cap and could outlive a shorter-lived upstream token; coerce it to a positive int (bool excluded). And _key_is_active called datetime.fromisoformat on the str|datetime expires outside the resolver's try, so a malformed stored expiry raised an unhandled 500 instead of the fail-closed invalid_request; it now fails closed (inactive) on an unparseable expiry. Regression tests cover int/float/string/bool coercion, the short-float TTL, and the malformed-expiry fail-closed path. --- .../mcp_server/discoverable_endpoints.py | 39 ++++++++++-- .../mcp_server/test_discoverable_endpoints.py | 60 +++++++++++++++++++ 2 files changed, 95 insertions(+), 4 deletions(-) diff --git a/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py b/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py index c5d7c3b40fd..00586a6afb3 100644 --- a/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py +++ b/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py @@ -342,12 +342,23 @@ def _key_is_active(key_obj: "UserAPIKeyAuth") -> bool: team-scoped or service-account key has no ``user_id`` yet is a legitimate credential, so gating on ``user_id`` presence would wrongly reject it. Callers that need the user (the per-user token store) derive it separately via :func:`_active_key_user_id`. + + Total by design: ``expires`` is typed ``str | datetime``, and an unparseable string would make + ``datetime.fromisoformat`` raise. Since the callers run this outside their key-resolution + ``try``, an uncaught parse error would surface as a 500 instead of the endpoint's fail-closed + behavior, so a malformed expiry is treated as inactive (return ``False``) rather than raising. """ if key_obj.blocked is True: return False expires = key_obj.expires if expires is not None: - expiry = expires if isinstance(expires, datetime) else datetime.fromisoformat(expires) + if isinstance(expires, datetime): + expiry = expires + else: + try: + expiry = datetime.fromisoformat(expires) + except (ValueError, TypeError): + return False if expiry.tzinfo is None or expiry.tzinfo.utcoffset(expiry) is None: expiry = expiry.replace(tzinfo=timezone.utc) if expiry < datetime.now(timezone.utc): @@ -698,10 +709,31 @@ async def authorize_with_server( return response +def _coerce_positive_expires_in(value: object) -> int | None: + """Coerce an upstream ``expires_in`` to a positive int, or ``None`` when it is absent or not a + 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): + 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 + + def _bridge_grant_from_token_response(token_response: object) -> Optional["UpstreamTokenGrant"]: """Validate an upstream OAuth token response into a typed grant, or None when it lacks a usable access token. Each field is isinstance-checked so nothing untyped from ``response.json()`` flows - into the grant.""" + into the grant; ``expires_in`` is numerically coerced so a float/string lifetime is honored + rather than dropped to the envelope's default cap.""" from litellm.proxy._experimental.mcp_server.outbound_credentials.envelope import ( # noqa: PLC0415 # inline import avoids a module-load circular import UpstreamTokenGrant, ) @@ -714,13 +746,12 @@ def _bridge_grant_from_token_response(token_response: object) -> Optional["Upstr token_type = token_response.get("token_type") refresh = token_response.get("refresh_token") scope = token_response.get("scope") - expires_in = token_response.get("expires_in") 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, scope=scope if isinstance(scope, str) and scope else None, - expires_in=expires_in if isinstance(expires_in, int) and expires_in > 0 else None, + expires_in=_coerce_positive_expires_in(token_response.get("expires_in")), ) 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 2bf5e49ec79..0dc8d8116df 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 @@ -4449,6 +4449,43 @@ async def test_oauth_delegate_bridge_token_exchange_fails_closed_without_litellm assert exc.value.detail["error"] == "invalid_request" +def test_bridge_grant_coerces_numeric_expires_in(): + """expires_in from an IdP may be an int, a float (3600.0), or a numeric string ("3600"); coerce + it to a positive int so the envelope TTL honors the real lifetime instead of dropping a non-int + value and defaulting to the 1h cap (which can outlive a shorter-lived upstream token). bool and + non-numeric values become None.""" + from litellm.proxy._experimental.mcp_server.discoverable_endpoints import ( + _bridge_grant_from_token_response, + ) + + def ei(v): + return _bridge_grant_from_token_response({"access_token": "x", "expires_in": v}).expires_in + + assert ei(300) == 300 + assert ei(300.0) == 300 + assert ei("300") == 300 + assert ei(" 300 ") == 300 + assert ei(True) is None + assert ei("nope") is None + assert ei(0) is None + assert ei(-5) is None + assert ei(None) is None + + +@pytest.mark.asyncio +async def test_bridge_token_exchange_honors_short_float_expires_in_ttl(): + """A short float expires_in from the upstream caps the envelope TTL, so the client-held envelope + does not outlive the upstream token. Before coercion a float was dropped and the envelope + defaulted to the 1h cap (3600), which would forward a stale bearer after the upstream token + expired.""" + from litellm.types.mcp import MCPAuth + + server = _bridge_server(auth_type=MCPAuth.oauth_delegate) + upstream = {"access_token": "UP", "token_type": "Bearer", "expires_in": 120.0} + response = await _exchange_for_bridge_server(server, upstream, key_hash="hashed-litellm-key-77") + assert json.loads(response.body)["expires_in"] <= 120 + + @pytest.mark.asyncio async def test_oauth_delegate_bridge_token_exchange_missing_access_token_is_502_not_keyerror(): """When the upstream token response has no access_token, a dcr_bridge oauth_delegate exchange @@ -4915,6 +4952,29 @@ async def test_extract_active_key_hash_rejects_blocked_key(proxy_globals): assert await _extract_active_key_hash_from_request(request) is None +@pytest.mark.asyncio +async def test_extract_active_key_hash_fails_closed_on_malformed_expiry(proxy_globals): + """A key whose stored expires string does not parse must fail closed to no-hash (the mint then + returns invalid_request), not surface an unhandled 500. The active-state check runs outside the + resolver's try, so it must be total over a bad expires rather than letting datetime.fromisoformat + raise. Before the fix this raised a ValueError instead of returning None.""" + from litellm.proxy._experimental.mcp_server.discoverable_endpoints import ( + _extract_active_key_hash_from_request, + ) + from litellm.proxy._types import UserAPIKeyAuth + from litellm.proxy.common_utils.user_api_key_cache import UserApiKeyCache + + class _FakePrisma: + async def get_data(self, token, table_name, parent_otel_span=None, proxy_logging_obj=None): + return UserAPIKeyAuth(token=token, user_id="u", expires="not-a-parseable-date") + + proxy_globals.user_api_key_cache = UserApiKeyCache() + proxy_globals.prisma_client = _FakePrisma() + + request = _token_request({"x-litellm-api-key": "sk-bad-expiry-key"}) + assert await _extract_active_key_hash_from_request(request) is None + + @pytest.mark.asyncio async def test_extract_active_key_hash_none_without_litellm_key(proxy_globals): """No LiteLLM key on the request yields no hash without consulting the resolver."""