diff --git a/litellm/proxy/auth/route_checks.py b/litellm/proxy/auth/route_checks.py index 3144c5cd25b4..a9519aa6cc5f 100644 --- a/litellm/proxy/auth/route_checks.py +++ b/litellm/proxy/auth/route_checks.py @@ -62,7 +62,11 @@ class RouteChecks: @staticmethod - def should_call_route(route: str, valid_token: UserAPIKeyAuth): + def should_call_route( + route: str, + valid_token: UserAPIKeyAuth, + request: Optional[Request] = None, + ): """ Check if management route is disabled and raise exception """ @@ -77,13 +81,15 @@ def should_call_route(route: str, valid_token: UserAPIKeyAuth): # Check if Virtual Key is allowed to call the route - Applies to all Roles RouteChecks.is_virtual_key_allowed_to_call_route( - route=route, valid_token=valid_token + route=route, valid_token=valid_token, request=request ) return True @staticmethod def is_virtual_key_allowed_to_call_route( - route: str, valid_token: UserAPIKeyAuth + route: str, + valid_token: UserAPIKeyAuth, + request: Optional[Request] = None, ) -> bool: """ Raises Exception if Virtual Key is not allowed to call the route @@ -130,6 +136,21 @@ def is_virtual_key_allowed_to_call_route( ): return True + # Method-aware carve-out: allow GET on the two + # read-only MCP-server discovery endpoints + # (`/v1/mcp/server` and `/v1/mcp/server/{server_id}`) + # so virtual keys with allowed_routes=["llm_api_routes"] + # can list/inspect MCP servers. The GET handlers in + # mcp_management_endpoints.py sanitize the response + # for restricted virtual keys (stripping url, + # headers, env, credentials). POST/PUT/DELETE on + # these paths are admin-only management writes and + # are intentionally not covered. + if RouteChecks._is_get_mcp_server_discovery_route( + route=route, request=request + ): + return True + # check if wildcard pattern is allowed for allowed_route in valid_token.allowed_routes: if RouteChecks._route_matches_wildcard_pattern( @@ -401,6 +422,31 @@ def is_llm_api_route(route: str) -> bool: return True return False + @staticmethod + def _is_get_mcp_server_discovery_route( + route: str, request: Optional[Request] + ) -> bool: + """ + Returns True if `request` is a GET against one of the two read-only + MCP-server discovery paths: + + - GET `/v1/mcp/server` (list) + - GET `/v1/mcp/server/{server_id}` (single server, single segment) + + Multi-segment paths (`/v1/mcp/server/{id}/approve`, etc.) and any + non-GET method return False, so admin-only management writes on the + same path prefix are not reachable through this carve-out. + """ + if request is None or request.method.upper() != "GET": + return False + if route == "/v1/mcp/server": + return True + prefix = "/v1/mcp/server/" + if not route.startswith(prefix): + return False + remainder = route[len(prefix) :] + return bool(remainder) and "/" not in remainder + @staticmethod def is_management_route(route: str) -> bool: """ diff --git a/litellm/proxy/auth/user_api_key_auth.py b/litellm/proxy/auth/user_api_key_auth.py index 032786339280..813b9826b370 100644 --- a/litellm/proxy/auth/user_api_key_auth.py +++ b/litellm/proxy/auth/user_api_key_auth.py @@ -2200,7 +2200,9 @@ async def user_api_key_auth( user_api_key_auth_obj.budget_reservation = None ## ENSURE DISABLE ROUTE WORKS ACROSS ALL USER AUTH FLOWS ## - RouteChecks.should_call_route(route=route, valid_token=user_api_key_auth_obj) + RouteChecks.should_call_route( + route=route, valid_token=user_api_key_auth_obj, request=request + ) # Single authorization point. Builder paths MUST NOT call common_checks. # Route through the same exception handler the builder uses so diff --git a/tests/test_litellm/proxy/auth/test_route_checks.py b/tests/test_litellm/proxy/auth/test_route_checks.py index ad00c55a8382..b308a665062b 100644 --- a/tests/test_litellm/proxy/auth/test_route_checks.py +++ b/tests/test_litellm/proxy/auth/test_route_checks.py @@ -262,12 +262,125 @@ def test_virtual_key_mcp_routes_allows_v1_mcp_server_subpaths(route): ) def test_mcp_management_routes_classified_as_management_not_llm_api(route): """MCP server CRUD must be management routes, not llm_api routes, so - DISABLE_LLM_API_ENDPOINTS on admin nodes does not block the Admin UI.""" + DISABLE_LLM_API_ENDPOINTS on admin nodes does not block the Admin UI. + + Note: virtual keys with allowed_routes=["llm_api_routes"] can still call + *GET* `/v1/mcp/server` and *GET* `/v1/mcp/server/{server_id}` — that + carve-out is enforced method-aware inside + `is_virtual_key_allowed_to_call_route`, not by adding the paths to + `llm_api_routes`. So `is_llm_api_route()` still returns False here and + `DISABLE_LLM_API_ENDPOINTS` still does not block these paths. + """ assert RouteChecks.is_llm_api_route(route=route) is False assert RouteChecks.is_management_route(route=route) is True +def _mock_request(method: str) -> Request: + request = MagicMock(spec=Request) + request.method = method + return request + + +@pytest.mark.parametrize( + "route", + [ + "/v1/mcp/server", + "/v1/mcp/server/abc-123", + ], +) +def test_virtual_key_llm_api_routes_allows_get_mcp_server_discovery(route): + """ + Regression test: virtual keys with allowed_routes=["llm_api_routes"] must + be able to list/inspect MCP servers via GET /v1/mcp/server[/{server_id}]. + + The handlers strip credential-bearing fields via + `_sanitize_mcp_server_list_for_virtual_key` when the caller is a + restricted virtual key, so GET is safe to expose. The carve-out is + method-aware (see below) — non-GET requests to the same paths are + rejected at this layer, so admin-only writes remain gated. + """ + + valid_token = UserAPIKeyAuth( + user_id="test_user", + allowed_routes=["llm_api_routes"], + ) + + result = RouteChecks.is_virtual_key_allowed_to_call_route( + route=route, + valid_token=valid_token, + request=_mock_request("GET"), + ) + + assert result is True + + +@pytest.mark.parametrize( + "route", + [ + "/v1/mcp/server", + "/v1/mcp/server/abc-123", + ], +) +@pytest.mark.parametrize("method", ["POST", "PUT", "PATCH", "DELETE"]) +def test_virtual_key_llm_api_routes_rejects_non_get_mcp_server_discovery(route, method): + """Method-aware: the MCP server discovery carve-out is GET-only. + + POST/PUT/PATCH/DELETE on `/v1/mcp/server[/{server_id}]` are admin-only + management writes and must not be reachable via llm_api_routes. + """ + + valid_token = UserAPIKeyAuth( + user_id="test_user", + allowed_routes=["llm_api_routes"], + ) + + with pytest.raises(HTTPException) as exc_info: + RouteChecks.is_virtual_key_allowed_to_call_route( + route=route, + valid_token=valid_token, + request=_mock_request(method), + ) + + assert exc_info.value.status_code == 403 + + +@pytest.mark.parametrize( + "route", + [ + # Multi-segment admin-only sub-paths must NOT be reachable via + # llm_api_routes, even on GET. + "/v1/mcp/server/abc-123/approve", + "/v1/mcp/server/abc-123/reject", + "/v1/mcp/server/oauth/session", + "/v1/mcp/server/abc-123/user-credential", + ], +) +def test_virtual_key_llm_api_routes_rejects_mcp_multi_segment_admin_subpaths( + route, +): + """Multi-segment admin-only MCP sub-paths are not reachable via llm_api_routes. + + The discovery carve-out only matches `/v1/mcp/server` and + `/v1/mcp/server/{server_id}` (single segment after `/server/`), so any + path with additional segments is rejected even when the request is GET. + """ + + valid_token = UserAPIKeyAuth( + user_id="test_user", + allowed_routes=["llm_api_routes"], + ) + + with pytest.raises(HTTPException) as exc_info: + RouteChecks.is_virtual_key_allowed_to_call_route( + route=route, + valid_token=valid_token, + request=_mock_request("GET"), + ) + + assert exc_info.value.status_code == 403 + + def test_spend_logs_v2_classified_as_management_not_llm_api(): """Paginated spend logs are a management/spend read route, not an LLM API."""