From f96899ae2b793f049a01a91db648980cd85021d2 Mon Sep 17 00:00:00 2001 From: Tin Chi Lo Date: Mon, 13 Jul 2026 11:08:08 -0700 Subject: [PATCH] fix(mcp): classify the user-subject reload's errors like the key path (503 outage, 401 missing) _reload_admitted_user mirrored only part of _reload_admitted_key's error contract: it caught ProxyException and HTTPException but had no arm for anything else, so a transient DB outage surfaced as an opaque 500 instead of the retryable 503 the key path guarantees, and a missing user surfaced as a 500 too. The missing-user case is the subtle one: get_user_object raises a bare Exception for a deleted user (not a ProxyException like get_key_object does for a missing key), so the ProxyException/HTTPException clause never caught it and the user_object-is-None branch it was supposed to hit is unreachable on the production path. Add the same except-Exception arm the key path uses, with the one deliberate difference the differing get_user_object contract requires: a database-service-unavailable error still raises the retryable 503, while a missing user or any other non-outage resolution failure fails closed as a 401 rather than propagating as a 500. The regression tests now drive the real behavior (get_user_object raising) rather than a None return that never happens in production, and cover both the 503 outage and the 401 missing-user paths. --- .../mcp_server/auth/user_api_key_auth_mcp.py | 13 +++++-- .../auth/test_user_api_key_auth_mcp.py | 34 +++++++++++++++---- 2 files changed, 39 insertions(+), 8 deletions(-) diff --git a/litellm/proxy/_experimental/mcp_server/auth/user_api_key_auth_mcp.py b/litellm/proxy/_experimental/mcp_server/auth/user_api_key_auth_mcp.py index faec35db41a..01861abbbc0 100644 --- a/litellm/proxy/_experimental/mcp_server/auth/user_api_key_auth_mcp.py +++ b/litellm/proxy/_experimental/mcp_server/auth/user_api_key_auth_mcp.py @@ -605,8 +605,14 @@ class MCPRequestHandler: caller's centralized policy gate then enforces the user's live budget and org state, and a SCIM-deactivated owner fails closed here exactly as the key path enforces it. No team is bound; a user may belong to many teams or none, so the envelope grants the - user's own access rather than silently selecting one team's scope. A missing user - fails closed with a 401 rather than admitting an unresolved identity.""" + user's own access rather than silently selecting one team's scope. + + Error handling mirrors the key path's retryable-503 contract, with one deliberate + difference: ``get_key_object`` raises a ``ProxyException`` for a missing key, but + ``get_user_object`` raises a bare ``Exception`` for a missing user (it does not surface as a + ``ProxyException``/``HTTPException``). So a transient DB outage still surfaces as a retryable + 503 via ``_raise_503_if_db_unavailable``, while a missing user, or any other non-outage + resolution failure, fails closed as a 401 rather than propagating as an opaque 500.""" from litellm.proxy.auth.auth_checks import get_user_object from litellm.proxy.proxy_server import prisma_client, user_api_key_cache @@ -621,6 +627,9 @@ class MCPRequestHandler: ) except (ProxyException, HTTPException): raise HTTPException(status_code=401, detail="Invalid or expired credential") from None + except Exception as e: # noqa: BLE001 # DB outage -> retryable 503; a missing user (bare Exception) or any other resolution failure -> fail closed 401, never an opaque 500 + MCPRequestHandler._raise_503_if_db_unavailable(e) + raise HTTPException(status_code=401, detail="Invalid or expired credential") from None if user_object is None: raise HTTPException(status_code=401, detail="Invalid or expired credential") if isinstance(user_object.metadata, dict) and user_object.metadata.get("scim_active") is False: diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/auth/test_user_api_key_auth_mcp.py b/tests/test_litellm/proxy/_experimental/mcp_server/auth/test_user_api_key_auth_mcp.py index a6affe5496c..a441e6a154b 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/auth/test_user_api_key_auth_mcp.py +++ b/tests/test_litellm/proxy/_experimental/mcp_server/auth/test_user_api_key_auth_mcp.py @@ -5117,8 +5117,10 @@ class TestMCPDcrBridgeDelegateAdmission: } async def test_user_subject_envelope_missing_user_fails_closed_401(self): - """A user_id envelope whose user has since been deleted must fail closed: get_user_object - resolves None, so admission 401s instead of admitting an unresolved identity.""" + """A user_id envelope whose user has since been deleted must fail closed with a 401, not a 500. + get_user_object raises a bare Exception for a missing user (it does not return None on the + production path), so the reload must catch it and fail closed rather than let it propagate as an + opaque 500. Regression for the missing-user path surfacing as a 500.""" envelope = self._mint_bridge_envelope(user_id="ghost-user") scope = { "type": "http", @@ -5129,7 +5131,7 @@ class TestMCPDcrBridgeDelegateAdmission: with ( patch("litellm.proxy._experimental.mcp_server.mcp_server_manager.global_mcp_server_manager") as mock_mgr, patch("litellm.proxy.proxy_server.master_key", self._MASTER_KEY), - self._patch_user_reload(return_value=None), + self._patch_user_reload(side_effect=Exception("user not found")), ): mock_mgr.get_mcp_server_by_name.return_value = self._bridge_delegate_server() with pytest.raises(HTTPException) as exc_info: @@ -5137,6 +5139,28 @@ class TestMCPDcrBridgeDelegateAdmission: assert exc_info.value.status_code == 401 + async def test_user_subject_envelope_db_outage_is_retryable_503(self): + """A transient database outage while reloading the envelope's user is a retryable 503, not an + opaque 500, matching the key path's contract so an interactive DCR client retries instead of + treating a live identity as invalid. Regression for the user reload dropping the 503 arm.""" + envelope = self._mint_bridge_envelope(user_id="sso-user-7") + scope = { + "type": "http", + "method": "POST", + "path": "/mcp/bridge_delegate_server", + "headers": [(b"authorization", f"Bearer {envelope}".encode("latin-1"))], + } + with ( + patch("litellm.proxy._experimental.mcp_server.mcp_server_manager.global_mcp_server_manager") as mock_mgr, + patch("litellm.proxy.proxy_server.master_key", self._MASTER_KEY), + self._patch_user_reload(side_effect=ConnectionError("auth database unreachable")), + ): + mock_mgr.get_mcp_server_by_name.return_value = self._bridge_delegate_server() + with pytest.raises(HTTPException) as exc_info: + await MCPRequestHandler.process_mcp_request(scope) + + assert exc_info.value.status_code == 503 + async def test_user_subject_envelope_scim_deactivated_user_fails_closed_401(self): """SCIM-deactivating the envelope's user revokes it immediately: the reloaded user carries scim_active False, so admission 401s rather than letting an offboarded user keep tool access @@ -5151,9 +5175,7 @@ class TestMCPDcrBridgeDelegateAdmission: with ( patch("litellm.proxy._experimental.mcp_server.mcp_server_manager.global_mcp_server_manager") as mock_mgr, patch("litellm.proxy.proxy_server.master_key", self._MASTER_KEY), - self._patch_user_reload( - return_value=MagicMock(user_id="offboarded-user", metadata={"scim_active": False}) - ), + self._patch_user_reload(return_value=MagicMock(user_id="offboarded-user", metadata={"scim_active": False})), ): mock_mgr.get_mcp_server_by_name.return_value = self._bridge_delegate_server() with pytest.raises(HTTPException) as exc_info: