From 688f535bbf1abc9b4b69704ccd76ba1e3bd06a65 Mon Sep 17 00:00:00 2001 From: Tin Chi Lo Date: Sat, 11 Jul 2026 13:34:23 -0700 Subject: [PATCH] fix(mcp): run the route gate on bridge admission so allowed_routes are enforced The envelope arm reloaded the identity and ran _run_centralized_common_checks but skipped RouteChecks.should_call_route, which the standard pipeline runs between the builder and common_checks. Because the centralized checks treat MCP as an inference route and never re-check allowed_routes, a key barred from MCP routes could mint an envelope at the token endpoint (not itself an MCP route) and replay it against MCP. Run the route gate before admitting, and clear the request-scoped budget_reservation, matching the wrapper's sequence; a disallowed route now surfaces the gate's own 403. --- .../mcp_server/auth/user_api_key_auth_mcp.py | 36 +++++++++++-------- .../auth/test_user_api_key_auth_mcp.py | 25 +++++++++++++ 2 files changed, 47 insertions(+), 14 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 e785b6b8da4..f63819b1822 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 @@ -636,24 +636,32 @@ class MCPRequestHandler: @staticmethod async def _enforce_admitted_live_policy(admitted: UserAPIKeyAuth, request: Request, route: str) -> None: - """Run the standard pipeline's single authorization point over the admitted identity. + """Run the standard pipeline's authorization checks over the admitted identity. - ``_run_centralized_common_checks`` is the same gate ``user_api_key_auth`` applies - after every builder path, so the envelope identity gets team-block, project-block, - org, and budget enforcement identical to the same key presented directly, and any - policy dimension added to the standard pipeline applies here without this arm - mirroring it. + Mirrors the ``user_api_key_auth`` wrapper between the builder and its return: clear the + request-scoped ``budget_reservation`` on the reloaded identity, run the route gate + (``RouteChecks.should_call_route``) to enforce the identity's ``allowed_routes`` and any + disabled/admin-only route, then run ``_run_centralized_common_checks`` (the same gate every + builder path funnels through) for team-block, project-block, org, and budget. The route gate + closes a bypass: a key barred from MCP routes could otherwise mint an envelope at the token + endpoint (not itself an MCP route) and replay it against MCP, because the centralized checks + treat MCP as an inference route and never re-check ``allowed_routes``. Failures surface with the status the standard pipeline would give them, mirroring - ``UserAPIKeyAuthExceptionHandler``: an over-budget identity is a 429, a sub-check that - raised its own ``HTTPException``/``ProxyException`` keeps that status, a transient - database outage is a retryable 503, and only a genuinely unresolvable failure (a - blocked team/project raises a bare ``Exception``, same as the standard pipeline's - fallback) becomes the fail-closed 401. Collapsing every failure to 401 was misleading: - it told an over-budget but validly-authenticated caller their credential was invalid, - which on a DCR client reads as broken auth and can trigger a pointless re-authorize - loop that cannot fix a budget problem, and it masked a DB outage as an auth error.""" + ``UserAPIKeyAuthExceptionHandler``: a disallowed route is the route gate's own 403, an + over-budget identity is a 429, a sub-check that raised its own ``HTTPException``/ + ``ProxyException`` keeps that status, a transient database outage is a retryable 503, and + only a genuinely unresolvable failure (a blocked team/project raises a bare ``Exception``, + same as the standard pipeline's fallback) becomes the fail-closed 401. Collapsing every + failure to 401 was misleading: it told an over-budget but validly-authenticated caller their + credential was invalid, which on a DCR client reads as broken auth and can trigger a + pointless re-authorize loop that cannot fix a budget problem, and it masked a DB outage as an + auth error.""" + from litellm.proxy.auth.route_checks import RouteChecks + + admitted.budget_reservation = None try: + RouteChecks.should_call_route(route=route, valid_token=admitted, request=request) await _run_centralized_common_checks( user_api_key_auth_obj=admitted, request=request, 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 da3e3250c59..eefaaf1dc99 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 @@ -5297,6 +5297,31 @@ class TestMCPDcrBridgeDelegateAdmission: assert exc_info.value.status_code == 503 + async def test_envelope_for_key_barred_from_mcp_routes_is_rejected_403(self): + """A key whose allowed_routes exclude MCP must not reach tools via an envelope: the arm runs + RouteChecks.should_call_route before admitting, exactly as the standard pipeline does between + the builder and common_checks. A route-restricted key can mint an envelope at the token + endpoint (not itself an MCP route) and would otherwise replay it against MCP, because the + centralized checks treat MCP as an inference route and never re-check allowed_routes; the + route gate rejects it with its own 403.""" + envelope = self._mint_bridge_envelope(key_hash=self._KEY_HASH) + 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_key_reload(return_value=self._reloaded_key(allowed_routes=["/chat/completions"])), + ): + 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 == 403 + async def test_blocked_state_bare_exception_stays_401(self): """A blocked team/project raises a bare Exception (no status) in common_checks, which the standard pipeline renders as 401; the arm keeps failing those closed as 401, never a 500."""