feat(proxy): allow llm_api_routes virtual keys to list MCP servers (#28442)
* feat(proxy): allow llm_api_routes virtual keys to list MCP servers
Add a new `mcp_discovery_routes` group (GET /v1/mcp/server and GET
/v1/mcp/server/{server_id}) and include it in `llm_api_routes` so that
virtual keys configured with `allowed_routes=["llm_api_routes"]` can
discover the MCP servers they have access to. Previously these calls
failed with 'Virtual key is not allowed to call this route. Only allowed
to call routes: [llm_api_routes]'.
The GET handlers already sanitize the response for restricted virtual
keys via `_sanitize_mcp_server_list_for_virtual_key`, stripping
credential-bearing fields (url, headers, env). Write methods
(POST/PUT/DELETE) on the same paths remain gated by the existing
handler-level admin role checks.
The new discovery list is intentionally kept OUT of
`mcp_inference_routes`, so `is_llm_api_route()` still returns False
for these paths — this preserves the existing contract that
DISABLE_LLM_API_ENDPOINTS must not block the Admin UI from listing MCP
servers.
Co-authored-by: ryan-crabbe-berri <ryan-crabbe-berri@users.noreply.github.com>
* refactor(proxy): make MCP discovery carve-out method-aware
Replace the `mcp_discovery_routes` group in `llm_api_routes` with a
method-aware special case inside `is_virtual_key_allowed_to_call_route`.
Virtual keys with allowed_routes=["llm_api_routes"] are now permitted
to call only GET /v1/mcp/server and GET /v1/mcp/server/{server_id} —
non-GET methods and multi-segment admin sub-paths fall through to the
existing 403. This keeps the general llm_api_routes list free of
management paths and avoids accidentally exposing POST/PUT/DELETE
writes through the route-check layer.
---------
Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: ryan-crabbe-berri <ryan-crabbe-berri@users.noreply.github.com>
This commit is contained in:
parent
c127968dfb
commit
66f10bceea
@ -62,7 +62,11 @@ _PROXY_ADMIN_VIEW_ONLY_BLOCKED_KEY_SUFFIXES = ("/regenerate", "/reset_spend")
|
||||
|
||||
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 @@ class RouteChecks:
|
||||
|
||||
# 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 @@ class RouteChecks:
|
||||
):
|
||||
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 @@ class RouteChecks:
|
||||
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:
|
||||
"""
|
||||
|
||||
@ -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
|
||||
|
||||
@ -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."""
|
||||
|
||||
|
||||
Loading…
Reference in New Issue
Block a user