mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-10 22:41:41 +00:00
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.
This commit is contained in:
parent
ceff2d1f3c
commit
2f349f6cd1
2 changed files with 95 additions and 4 deletions
|
|
@ -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")),
|
||||
)
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -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."""
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue