feat(mcp): support MCP access group names in URL-based namespacing - #27726
Merged
mateo-berri merged 9 commits intoMay 14, 2026
Merged
Conversation
Contributor
Greptile SummaryThis PR extends the
Confidence Score: 5/5Safe to merge; the new routing logic is well-tested and correctly bounded. The resolution logic is correct and all existing edge cases are handled and tested. The two flagged items are documentation and minor configurability concerns, not behavioural defects. No files require special attention beyond the minor comment fix in proxy_server.py and the optional env-var support in constants.py.
|
| Filename | Overview |
|---|---|
| litellm/proxy/proxy_server.py | Refactors dynamic_mcp_route to support MCP access-group tags and CSV namespaces; resolution order rearranged correctly but docstring has inaccurate DB-call claim for CSV path. |
| litellm/constants.py | Adds DEFAULT_MCP_ACCESS_GROUP_NEGATIVE_CACHE_TTL (hardcoded 10s) and DEFAULT_MCP_NAMESPACE_CSV_MAX_TOKENS (16); negative TTL not env-configurable unlike analogous constants. |
| tests/test_litellm/proxy/test_dynamic_mcp_route.py | New unit tests covering all resolution paths with mocks only; no real network calls. |
Reviews (6): Last reviewed commit: "fix(mcp): exact-match CSV token dedupe t..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
Contributor
MCP access-group URL routing addedThis PR extends dynamic MCP URL namespacing to recognize access-group tags and comma-separated targets, with cached existence checks and a cap on per-request token fan-out. I checked the route ordering, pre-auth lookup behavior, downstream MCP authentication, and server filtering paths and did not find a new security issue in the changed code. Status: 2 open |
Contributor
Author
|
@greptile re review |
Contributor
Author
|
bugbot run |
Contributor
Author
|
@veria-ai re review |
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Negative access group results cached after transient failures
- Changed MCP access-group existence caching to cache only positive lookups so empty results from transient lookup failures are not retained.
Preview (285d63d622)
diff --git a/litellm/proxy/proxy_server.py b/litellm/proxy/proxy_server.py
--- a/litellm/proxy/proxy_server.py
+++ b/litellm/proxy/proxy_server.py
@@ -15106,97 +15106,121 @@
raise HTTPException(status_code=500, detail=f"Internal server error: {str(e)}")
+async def _mcp_forward_as_path(path_segment: str, request: Request):
+ """Rewrite path to /mcp/{path_segment} and buffer the response."""
+ from litellm.proxy._experimental.mcp_server.server import (
+ handle_streamable_http_mcp,
+ )
+ from starlette.responses import Response
+
+ scope = dict(request.scope)
+ scope["path"] = f"/mcp/{path_segment}"
+
+ response_body = b""
+ response_status = 200
+ response_headers: list = []
+
+ async def custom_send(message):
+ nonlocal response_body, response_status, response_headers
+ if message["type"] == "http.response.start":
+ response_status = message["status"]
+ response_headers = message.get("headers", [])
+ elif message["type"] == "http.response.body":
+ response_body += message.get("body", b"")
+
+ await handle_streamable_http_mcp(scope, receive=request.receive, send=custom_send)
+ headers_dict = {k.decode(): v.decode() for k, v in response_headers}
+ return Response(
+ content=response_body,
+ status_code=response_status,
+ headers=headers_dict,
+ media_type=headers_dict.get("content-type", "application/json"),
+ )
+
+
+async def _is_mcp_access_group_cached(name: str) -> bool:
+ """Return True if *name* is a known MCP access group tag, caching positive results."""
+ from litellm.constants import DEFAULT_MANAGEMENT_OBJECT_IN_MEMORY_CACHE_TTL
+ from litellm.proxy._experimental.mcp_server.auth.user_api_key_auth_mcp import (
+ MCPRequestHandler,
+ )
+
+ cache_key = f"mcp_access_group_exists:{name}"
+ cached = await user_api_key_cache.async_get_cache(key=cache_key)
+ if cached is not None:
+ return bool(cached)
+ result = bool(await MCPRequestHandler._get_mcp_servers_from_access_groups([name]))
+ if result:
+ await user_api_key_cache.async_set_cache(
+ key=cache_key,
+ value=result,
+ ttl=DEFAULT_MANAGEMENT_OBJECT_IN_MEMORY_CACHE_TTL,
+ )
+ return result
+
+
# Dynamic MCP server routes - handle /{mcp_server_name}/mcp
@app.api_route(
"/{mcp_server_name}/mcp",
methods=["GET", "POST", "PUT", "DELETE", "PATCH", "OPTIONS", "HEAD"],
)
async def dynamic_mcp_route(mcp_server_name: str, request: Request):
- """Handle dynamic MCP server routes like /github_mcp/mcp and toolset routes like /devtooling-prod/mcp"""
+ """Handle /{name}/mcp for MCP server aliases, toolsets, MCP access group tags, and comma-separated lists.
+
+ Resolution order:
+ 1. Registered MCP server alias / name
+ 2. Comma-separated list (short-circuits before any DB call)
+ 3. Toolset name (DB lookup, cached)
+ 4. MCP access group tag (DB lookup, cached)
+ """
try:
- # Validate that the MCP server exists
from litellm.proxy._experimental.mcp_server.mcp_server_manager import (
global_mcp_server_manager,
)
from litellm.proxy.auth.ip_address_utils import IPAddressUtils
- from litellm.types.mcp import MCPAuth
client_ip = IPAddressUtils.get_mcp_client_ip(request)
- mcp_server = global_mcp_server_manager.get_mcp_server_by_name(
+
+ # 1. Registered MCP server alias
+ if global_mcp_server_manager.get_mcp_server_by_name(
mcp_server_name, client_ip=client_ip
- )
- if mcp_server is None:
- # Check if this is a toolset name — toolsets are accessible at /{name}/mcp
- # the same way individual servers are, no separate /toolset/ prefix needed.
- if prisma_client is not None:
- from litellm.proxy._experimental.mcp_server.mcp_server_manager import (
- global_mcp_server_manager,
- )
- from litellm.proxy._experimental.mcp_server.server import (
- _mcp_active_toolset_id,
- handle_streamable_http_mcp,
- )
+ ):
+ return await _mcp_forward_as_path(mcp_server_name, request)
- toolset = await global_mcp_server_manager.get_toolset_by_name_cached(
- prisma_client, mcp_server_name
- )
- if toolset is not None:
- scope = dict(request.scope)
- scope["path"] = "/mcp"
+ # 2. Comma-separated list — forward directly before any DB call.
+ if "," in mcp_server_name:
+ return await _mcp_forward_as_path(mcp_server_name, request)
- token = _mcp_active_toolset_id.set(toolset.toolset_id)
- try:
- return await _stream_mcp_asgi_response(
- handle_streamable_http_mcp, scope, request.receive
- )
- finally:
- _mcp_active_toolset_id.reset(token)
+ # 3. Toolset name (cached)
+ if prisma_client is not None:
+ from litellm.proxy._experimental.mcp_server.server import (
+ _mcp_active_toolset_id,
+ handle_streamable_http_mcp,
+ )
- raise HTTPException(
- status_code=404, detail=f"MCP server '{mcp_server_name}' not found"
+ toolset = await global_mcp_server_manager.get_toolset_by_name_cached(
+ prisma_client, mcp_server_name
)
+ if toolset is not None:
+ scope = dict(request.scope)
+ scope["path"] = "/mcp"
+ token = _mcp_active_toolset_id.set(toolset.toolset_id)
+ try:
+ return await _stream_mcp_asgi_response(
+ handle_streamable_http_mcp, scope, request.receive
+ )
+ finally:
+ _mcp_active_toolset_id.reset(token)
- # Create a new scope with the correct path format that the MCP handler expects
- # Transform /{mcp_server_name}/mcp to /mcp/{mcp_server_name}
- scope = dict(request.scope)
- scope["path"] = f"/mcp/{mcp_server_name}"
+ # 4. MCP access group tag (cached)
+ if await _is_mcp_access_group_cached(mcp_server_name):
+ return await _mcp_forward_as_path(mcp_server_name, request)
- # Import the MCP handler
- from litellm.proxy._experimental.mcp_server.server import (
- handle_streamable_http_mcp,
+ raise HTTPException(
+ status_code=404,
+ detail=f"MCP server, toolset, or access group '{mcp_server_name}' not found",
)
- # Create a custom send function to capture the response
- response_started = False
- response_body = b""
- response_status = 200
- response_headers = []
-
- async def custom_send(message):
- nonlocal response_started, response_body, response_status, response_headers
- if message["type"] == "http.response.start":
- response_started = True
- response_status = message["status"]
- response_headers = message.get("headers", [])
- elif message["type"] == "http.response.body":
- response_body += message.get("body", b"")
-
- # Call the existing MCP handler
- await handle_streamable_http_mcp(
- scope, receive=request.receive, send=custom_send
- )
-
- # Return the response
- from starlette.responses import Response
-
- headers_dict = {k.decode(): v.decode() for k, v in response_headers}
- return Response(
- content=response_body,
- status_code=response_status,
- headers=headers_dict,
- media_type=headers_dict.get("content-type", "application/json"),
- )
-
except HTTPException as e:
raise e
except Exception as e:
diff --git a/tests/test_litellm/proxy/test_dynamic_mcp_route.py b/tests/test_litellm/proxy/test_dynamic_mcp_route.py
new file mode 100644
--- /dev/null
+++ b/tests/test_litellm/proxy/test_dynamic_mcp_route.py
@@ -1,0 +1,339 @@
+"""
+Tests for the dynamic_mcp_route handler in proxy_server.py.
+
+Covers the resolution order:
+ 1. Registered MCP server alias → forwards to /mcp/{name}
+ 2. Comma-separated list → short-circuits before any DB call;
+ forwarded to /mcp/{segment}
+ 3. Toolset name (cached) → sets toolset scope, forwards to /mcp
+ 4. MCP access group tag (cached) → forwards to /mcp/{name} when the group
+ resolves to at least one server
+ 5. Unknown name → 404
+
+Patch targets are at the source modules because dynamic_mcp_route
+uses lazy local imports inside the function body.
+"""
+
+from unittest.mock import ANY, AsyncMock, MagicMock, patch
+
+import pytest
+from fastapi import HTTPException
+
+
+# ---------------------------------------------------------------------------
+# helpers
+# ---------------------------------------------------------------------------
+
+_MCP_MANAGER = "litellm.proxy._experimental.mcp_server.mcp_server_manager.global_mcp_server_manager"
+_HANDLE_HTTP = (
+ "litellm.proxy._experimental.mcp_server.server.handle_streamable_http_mcp"
+)
+_STREAM_ASGI = "litellm.proxy.proxy_server._stream_mcp_asgi_response"
+_PRISMA = "litellm.proxy.proxy_server.prisma_client"
+_IS_ACCESS_GROUP = "litellm.proxy.proxy_server._is_mcp_access_group_cached"
+_USER_API_KEY_CACHE = "litellm.proxy.proxy_server.user_api_key_cache"
+_GET_ACCESS_GROUP_SERVERS = (
+ "litellm.proxy._experimental.mcp_server.auth.user_api_key_auth_mcp."
+ "MCPRequestHandler._get_mcp_servers_from_access_groups"
+)
+_FORWARD = "litellm.proxy.proxy_server._mcp_forward_as_path"
+
+
+def _make_request(path: str = "/test/mcp"):
+ """Minimal fake Starlette Request."""
+ from starlette.requests import Request
+
+ scope = {
+ "type": "http",
+ "method": "POST",
+ "path": path,
+ "headers": [],
+ "query_string": b"",
+ "server": ("localhost", 4000),
+ "scheme": "http",
+ }
+
+ async def receive():
+ return {"type": "http.request", "body": b"{}"}
+
+ return Request(scope=scope, receive=receive)
+
+
+def _fake_server(name: str = "my_server", server_id: str = "server-id-1"):
+ s = MagicMock()
+ s.name = name
+ s.server_id = server_id
+ return s
+
+
+def _fake_toolset(name: str = "my_toolset", toolset_id: str = "ts-1"):
+ t = MagicMock()
+ t.toolset_id = toolset_id
+ t.name = name
+ return t
+
+
+async def _ok_mcp_handle(scope, receive, send):
+ """Stub MCP handler that returns HTTP 200."""
+ await send({"type": "http.response.start", "status": 200, "headers": []})
+ await send({"type": "http.response.body", "body": b"{}"})
+
+
+# ---------------------------------------------------------------------------
+# 1. Registered MCP server alias
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.asyncio
+async def test_dynamic_mcp_route_resolves_registered_server():
+ """When the segment matches a known server alias the request is forwarded
+ to /mcp/{name} and the handler returns 200."""
+ from starlette.responses import Response
+
+ from litellm.proxy.proxy_server import dynamic_mcp_route
+
+ request = _make_request("/my_server/mcp")
+ fake_mgr = MagicMock()
+ fake_mgr.get_mcp_server_by_name = MagicMock(return_value=_fake_server("my_server"))
+
+ fake_forward = AsyncMock(return_value=Response(content=b"{}", status_code=200))
+
+ with (
+ patch(_MCP_MANAGER, fake_mgr),
+ patch(_FORWARD, new=fake_forward),
+ ):
+ response = await dynamic_mcp_route("my_server", request)
+
+ assert response.status_code == 200
+ fake_forward.assert_awaited_once_with("my_server", request)
+
+
+# ---------------------------------------------------------------------------
+# 2. Comma-separated list (short-circuits before toolset DB call)
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.asyncio
+async def test_dynamic_mcp_route_comma_list_forwarded():
+ """A comma-separated segment is forwarded directly without hitting
+ the toolset or access-group DB lookups."""
+ from starlette.responses import Response
+
+ from litellm.proxy.proxy_server import dynamic_mcp_route
+
+ segment = "github_mcp,zapier"
+ request = _make_request(f"/{segment}/mcp")
+
+ fake_mgr = MagicMock()
+ fake_mgr.get_mcp_server_by_name = MagicMock(return_value=None)
+
+ fake_forward = AsyncMock(return_value=Response(content=b"{}", status_code=200))
+
+ with (
+ patch(_MCP_MANAGER, fake_mgr),
+ patch(_FORWARD, new=fake_forward),
+ ):
+ response = await dynamic_mcp_route(segment, request)
+
+ assert response.status_code == 200
+ fake_forward.assert_awaited_once_with(segment, request)
+ # Toolset lookup must NOT be called for comma names
+ fake_mgr.get_toolset_by_name_cached.assert_not_called()
+
+
+# ---------------------------------------------------------------------------
+# 3. Toolset name
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.asyncio
+async def test_dynamic_mcp_route_resolves_toolset():
+ """When the segment is a toolset name the toolset context var is set
+ and the request is forwarded to /mcp (not /mcp/{name})."""
+ from litellm.proxy.proxy_server import dynamic_mcp_route
+
+ request = _make_request("/my_toolset/mcp")
+ fake_toolset = _fake_toolset("my_toolset", "ts-42")
+
+ fake_mgr = MagicMock()
+ fake_mgr.get_mcp_server_by_name = MagicMock(return_value=None)
+ fake_mgr.get_toolset_by_name_cached = AsyncMock(return_value=fake_toolset)
+
+ captured_toolset_id = None
+ captured_scope = {}
+
+ async def fake_stream(fn, scope, receive):
+ nonlocal captured_toolset_id
+ from litellm.proxy._experimental.mcp_server.server import (
+ _mcp_active_toolset_id,
+ )
+
+ captured_toolset_id = _mcp_active_toolset_id.get()
+ captured_scope.update(scope)
+
+ with (
+ patch(_MCP_MANAGER, fake_mgr),
+ patch(_PRISMA, new=MagicMock()),
+ patch(_STREAM_ASGI, new=AsyncMock(side_effect=fake_stream)),
+ ):
+ await dynamic_mcp_route("my_toolset", request)
+
+ assert captured_toolset_id == "ts-42"
+ assert captured_scope.get("path") == "/mcp"
+
+
+# ---------------------------------------------------------------------------
+# 4. MCP access group tag (cached)
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.asyncio
+async def test_dynamic_mcp_route_resolves_access_group():
+ """When the segment is an MCP access group the request is forwarded (not 404)."""
+ from starlette.responses import Response
+
+ from litellm.proxy.proxy_server import dynamic_mcp_route
+
+ request = _make_request("/dev_group/mcp")
+
+ fake_mgr = MagicMock()
+ fake_mgr.get_mcp_server_by_name = MagicMock(return_value=None)
+ fake_mgr.get_toolset_by_name_cached = AsyncMock(return_value=None)
+
+ fake_forward = AsyncMock(return_value=Response(content=b"{}", status_code=200))
+
+ with (
+ patch(_MCP_MANAGER, fake_mgr),
+ patch(_PRISMA, new=MagicMock()),
+ patch(_IS_ACCESS_GROUP, new=AsyncMock(return_value=True)),
+ patch(_FORWARD, new=fake_forward),
+ ):
+ response = await dynamic_mcp_route("dev_group", request)
+
+ assert response.status_code == 200
+ fake_forward.assert_awaited_once_with("dev_group", request)
+
+
+@pytest.mark.asyncio
+async def test_dynamic_mcp_route_access_group_called_with_correct_name():
+ """The access group lookup receives exactly the segment from the URL."""
+ from starlette.responses import Response
+
+ from litellm.proxy.proxy_server import dynamic_mcp_route
+
+ request = _make_request("/qa_tools/mcp")
+
+ fake_mgr = MagicMock()
+ fake_mgr.get_mcp_server_by_name = MagicMock(return_value=None)
+ fake_mgr.get_toolset_by_name_cached = AsyncMock(return_value=None)
+
+ is_group = AsyncMock(return_value=True)
+
+ with (
+ patch(_MCP_MANAGER, fake_mgr),
+ patch(_PRISMA, new=MagicMock()),
+ patch(_IS_ACCESS_GROUP, new=is_group),
+ patch(
+ _FORWARD,
+ new=AsyncMock(return_value=Response(content=b"{}", status_code=200)),
+ ),
+ ):
+ await dynamic_mcp_route("qa_tools", request)
+
+ is_group.assert_awaited_once_with("qa_tools")
+
+
+@pytest.mark.asyncio
+async def test_is_mcp_access_group_cached_caches_positive_result():
+ """Known access groups are cached after resolving to one or more servers."""
+ from litellm.proxy.proxy_server import _is_mcp_access_group_cached
+
+ fake_cache = MagicMock()
+ fake_cache.async_get_cache = AsyncMock(return_value=None)
+ fake_cache.async_set_cache = AsyncMock()
+ get_access_group_servers = AsyncMock(return_value=["server-id"])
+
+ with (
+ patch(_USER_API_KEY_CACHE, new=fake_cache),
+ patch(_GET_ACCESS_GROUP_SERVERS, new=get_access_group_servers),
+ ):
+ result = await _is_mcp_access_group_cached("dev_group")
+
+ assert result is True
+ get_access_group_servers.assert_awaited_once_with(["dev_group"])
+ fake_cache.async_set_cache.assert_awaited_once_with(
+ key="mcp_access_group_exists:dev_group",
+ value=True,
+ ttl=ANY,
+ )
+
+
+@pytest.mark.asyncio
+async def test_is_mcp_access_group_cached_does_not_cache_negative_result():
+ """Empty access-group lookups are not cached because DB errors also return empty."""
+ from litellm.proxy.proxy_server import _is_mcp_access_group_cached
+
+ fake_cache = MagicMock()
+ fake_cache.async_get_cache = AsyncMock(return_value=None)
+ fake_cache.async_set_cache = AsyncMock()
+ get_access_group_servers = AsyncMock(return_value=[])
+
+ with (
+ patch(_USER_API_KEY_CACHE, new=fake_cache),
+ patch(_GET_ACCESS_GROUP_SERVERS, new=get_access_group_servers),
+ ):
+ result = await _is_mcp_access_group_cached("dev_group")
+
+ assert result is False
+ get_access_group_servers.assert_awaited_once_with(["dev_group"])
+ fake_cache.async_set_cache.assert_not_awaited()
+
+
+# ---------------------------------------------------------------------------
+# 5. Unknown name → 404
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.asyncio
+async def test_dynamic_mcp_route_unknown_name_returns_404():
+ """A segment that is not a server, toolset, or access group → 404."""
+ from litellm.proxy.proxy_server import dynamic_mcp_route
+
+ request = _make_request("/does_not_exist/mcp")
+
+ fake_mgr = MagicMock()
+ fake_mgr.get_mcp_server_by_name = MagicMock(return_value=None)
+ fake_mgr.get_toolset_by_name_cached = AsyncMock(return_value=None)
+
+ with (
+ patch(_MCP_MANAGER, fake_mgr),
+ patch(_PRISMA, new=MagicMock()),
+ patch(_IS_ACCESS_GROUP, new=AsyncMock(return_value=False)),
+ ):
+ with pytest.raises(HTTPException) as exc_info:
+ await dynamic_mcp_route("does_not_exist", request)
+
+ assert exc_info.value.status_code == 404
+ assert "does_not_exist" in str(exc_info.value.detail)
+
+
+@pytest.mark.asyncio
+async def test_dynamic_mcp_route_empty_access_group_returns_404():
+ """An access group tag that resolves to zero servers still returns 404."""
+ from litellm.proxy.proxy_server import dynamic_mcp_route
+
+ request = _make_request("/empty_group/mcp")
+
+ fake_mgr = MagicMock()
+ fake_mgr.get_mcp_server_by_name = MagicMock(return_value=None)
+ fake_mgr.get_toolset_by_name_cached = AsyncMock(return_value=None)
+
+ with (
+ patch(_MCP_MANAGER, fake_mgr),
+ patch(_PRISMA, new=MagicMock()),
+ patch(_IS_ACCESS_GROUP, new=AsyncMock(return_value=False)),
+ ):
+ with pytest.raises(HTTPException) as exc_info:
+ await dynamic_mcp_route("empty_group", request)
+
+ assert exc_info.value.status_code == 404You can send follow-ups to the cloud agent here.
|
|
Contributor
Author
|
@greptile re review |
Contributor
Author
|
bugbot run |
Contributor
Author
|
@veria-ai re review |
Contributor
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 6c4f8e9. Configure here.
| # Return the response | ||
| from starlette.responses import Response | ||
| # 4. MCP access group tag (cached) | ||
| if await _is_mcp_access_group_cached(mcp_server_name): |
Contributor
There was a problem hiding this comment.
Medium: Unauthenticated access-group lookup
_is_mcp_access_group_cached() runs before the request reaches the MCP handler's API-key authentication, and negative results are not cached. An unauthenticated caller can repeatedly request /random-name/mcp and force a fresh access-group database lookup on every request; move this resolution behind MCP auth or cache confirmed negative results separately from lookup errors.
Contributor
There was a problem hiding this comment.
Addressed in a577aaf — negative results from _is_mcp_access_group_cached are now cached for a short 10s DEFAULT_MCP_ACCESS_GROUP_NEGATIVE_CACHE_TTL window, so unauthenticated callers can no longer force a fresh DB lookup per request for unknown names. Bounding the negative TTL keeps a transient DB error (which surfaces as an empty list) from hiding a real group for long.
Contributor
|
@veria-ai re review |
Contributor
|
@greptileai re-review please |
|
|
||
| scope = dict(request.scope) | ||
| scope["path"] = f"/mcp/{path_segment}" | ||
| return await _stream_mcp_asgi_response( |
Contributor
There was a problem hiding this comment.
Medium: Dynamic MCP auth failures can hold requests open
An unauthenticated caller can hit a valid dynamic MCP namespace without an API key and keep each request open until the 30-second timeout, because auth failures raised by handle_streamable_http_mcp happen in the background task before any headers are sent. Propagate pre-header task exceptions in _stream_mcp_asgi_response or keep the direct ASGI call behavior for auth failures so invalid requests fail immediately.
Contributor
There was a problem hiding this comment.
_stream_mcp_asgi_response already does this — _ensure_eof is a add_done_callback on the handler task that calls headers_ready.set_exception(task_exception) whenever the task ends with an exception before headers were sent (proxy_server.py L15036-L15040). The downstream await asyncio.wait_for(asyncio.shield(headers_ready), ...) then re-raises immediately rather than waiting for the 30s timeout. So a pre-header auth exception from handle_streamable_http_mcp propagates synchronously to the caller, not held open. (For the normal MCP auth path the handler returns a 401 response rather than raising, which sends headers immediately and bypasses the timeout entirely.)
Extends dynamic_mcp_route to resolve /{name}/mcp requests where {name}
is an MCP access group tag or a comma-separated list of servers/groups,
matching what the documentation promised but the handler did not implement.
Resolution order: registered server alias → toolset → comma-separated
list → single access group tag (404 if none match).
Adds unit tests covering all four resolution paths plus 404 cases.
Co-authored-by: Cursor <cursoragent@cursor.com>
- Move comma-separated check before toolset DB lookup so comma names short-circuit without hitting the database - Cache access-group DB lookups via user_api_key_cache to avoid a raw find_many on every request (matches toolset caching pattern) - Remove unused response_started variable from _forward_as_mcp_path - Update tests to assert comma list skips toolset call and to mock cache Co-authored-by: Cursor <cursoragent@cursor.com>
…dynamic_mcp_route Extract _mcp_forward_as_path and _is_mcp_access_group_cached as module-level helpers so dynamic_mcp_route stays under the 50-statement limit. Update tests to patch the new module-level symbols directly. Co-authored-by: Cursor <cursoragent@cursor.com>
…of buffering _mcp_forward_as_path previously accumulated the full response body in memory before sending it. Replace the buffering custom_send pattern with _stream_mcp_asgi_response, which uses an asyncio.Queue bridge so chunks are yielded to the client as they arrive, preventing unbounded memory growth on large or long-lived MCP responses. Co-authored-by: Cursor <cursoragent@cursor.com>
An unauthenticated caller could repeatedly request /<unknown>/mcp and force a fresh DB lookup for the access-group existence check on every request (only positive results were cached). Cache negative results for a short DEFAULT_MCP_ACCESS_GROUP_NEGATIVE_CACHE_TTL window (10s by default) so the DB is shielded from flooding while a transient DB error (which surfaces as an empty list) cannot hide a real group for long. https://claude.ai/code/session_01SjyPmwfmrq8fveFgw9iHW9
Drop the os.getenv wrapper around DEFAULT_MCP_ACCESS_GROUP_NEGATIVE_CACHE_TTL to avoid the documentation_test_env_keys check failing on the new variable. The negative-cache window is a small internal tuning constant, not a user-facing knob, so a plain integer is clearer than an env override. https://claude.ai/code/session_01SjyPmwfmrq8fveFgw9iHW9
For /{name1,name2,...}/mcp, validate every token resolves to a known
server alias or access group, dedupe case-insensitively, and cap at
DEFAULT_MCP_NAMESPACE_CSV_MAX_TOKENS=16 before forwarding.
- Bounds the per-request DB / cache fan-out an authenticated caller can
trigger by stuffing the path with tokens (raised by veria-ai).
- Returns 404 instead of forwarding when no token resolves, so the
downstream server filter cannot silently fall back to the full
allowed_mcp_servers list (raised by Cursor agentic security review).
- Forwards only the resolved subset, so unknown tokens cannot ride along
into the downstream filter.
Co-authored-by: Mateo Wang <mateo-berri@users.noreply.github.com>
cursor
Bot
force-pushed
the
litellm_mcp_access_group_url_routing
branch
from
May 13, 2026 21:51
70410a9 to
d98f5ee
Compare
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is ON, but it could not run because the spend limit has been reached. To enable Bugbot Autofix, have a team admin raise the spend limit in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d98f5ee. Configure here.
Contributor
|
@greptile re review |
…tinct tokens Bugbot flagged that case-insensitive dedup on `MyGroup,mygroup` could collapse to whichever case appeared first and silently drop the matching casing if the downstream resolver is case-sensitive. Switch to exact-match dedup so distinct casings survive; whitespace-only differences still collapse via the .strip() before comparison. Co-authored-by: Mateo Wang <mateo-berri@users.noreply.github.com>
Contributor
Contributor
Author
|
@mateo-berri IS this good to merge?? |
Contributor
|
Yes, sorry for the delay |
fzowl
pushed a commit
to fzowl/litellm
that referenced
this pull request
Jun 24, 2026
…erriAI#27726) * feat(mcp): support MCP access group names in URL-based namespacing Extends dynamic_mcp_route to resolve /{name}/mcp requests where {name} is an MCP access group tag or a comma-separated list of servers/groups, matching what the documentation promised but the handler did not implement. Resolution order: registered server alias → toolset → comma-separated list → single access group tag (404 if none match). Adds unit tests covering all four resolution paths plus 404 cases. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(mcp): address Greptile review comments on dynamic_mcp_route - Move comma-separated check before toolset DB lookup so comma names short-circuit without hitting the database - Cache access-group DB lookups via user_api_key_cache to avoid a raw find_many on every request (matches toolset caching pattern) - Remove unused response_started variable from _forward_as_mcp_path - Update tests to assert comma list skips toolset call and to mock cache Co-authored-by: Cursor <cursoragent@cursor.com> * refactor(mcp): extract helpers to fix PLR0915 too-many-statements in dynamic_mcp_route Extract _mcp_forward_as_path and _is_mcp_access_group_cached as module-level helpers so dynamic_mcp_route stays under the 50-statement limit. Update tests to patch the new module-level symbols directly. Co-authored-by: Cursor <cursoragent@cursor.com> * Avoid caching missing MCP access groups * fix(mcp): stream MCP responses via _stream_mcp_asgi_response instead of buffering _mcp_forward_as_path previously accumulated the full response body in memory before sending it. Replace the buffering custom_send pattern with _stream_mcp_asgi_response, which uses an asyncio.Queue bridge so chunks are yielded to the client as they arrive, preventing unbounded memory growth on large or long-lived MCP responses. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(mcp): short-TTL negative cache for access-group existence lookup An unauthenticated caller could repeatedly request /<unknown>/mcp and force a fresh DB lookup for the access-group existence check on every request (only positive results were cached). Cache negative results for a short DEFAULT_MCP_ACCESS_GROUP_NEGATIVE_CACHE_TTL window (10s by default) so the DB is shielded from flooding while a transient DB error (which surfaces as an empty list) cannot hide a real group for long. https://claude.ai/code/session_01SjyPmwfmrq8fveFgw9iHW9 * fix(mcp): use plain int for access-group negative cache TTL Drop the os.getenv wrapper around DEFAULT_MCP_ACCESS_GROUP_NEGATIVE_CACHE_TTL to avoid the documentation_test_env_keys check failing on the new variable. The negative-cache window is a small internal tuning constant, not a user-facing knob, so a plain integer is clearer than an env override. https://claude.ai/code/session_01SjyPmwfmrq8fveFgw9iHW9 * fix(mcp): validate, dedupe, and cap CSV tokens in dynamic MCP route For /{name1,name2,...}/mcp, validate every token resolves to a known server alias or access group, dedupe case-insensitively, and cap at DEFAULT_MCP_NAMESPACE_CSV_MAX_TOKENS=16 before forwarding. - Bounds the per-request DB / cache fan-out an authenticated caller can trigger by stuffing the path with tokens (raised by veria-ai). - Returns 404 instead of forwarding when no token resolves, so the downstream server filter cannot silently fall back to the full allowed_mcp_servers list (raised by Cursor agentic security review). - Forwards only the resolved subset, so unknown tokens cannot ride along into the downstream filter. Co-authored-by: Mateo Wang <mateo-berri@users.noreply.github.com> * fix(mcp): exact-match CSV token dedupe to preserve case-sensitive distinct tokens Bugbot flagged that case-insensitive dedup on `MyGroup,mygroup` could collapse to whichever case appeared first and silently drop the matching casing if the downstream resolver is case-sensitive. Switch to exact-match dedup so distinct casings survive; whitespace-only differences still collapse via the .strip() before comparison. Co-authored-by: Mateo Wang <mateo-berri@users.noreply.github.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: mateo-berri <mateo@berri.ai> Co-authored-by: Mateo Wang <mateo-berri@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/{name}/mcpnow correctly resolves when{name}is an MCP access group tag, closing the gap between the documented behavior and the actual implementation./github_mcp,zapier/mcpby forwarding them directly to the MCP handler which already knows how to resolve each token.Resolution order in
dynamic_mcp_routeTests
Added
tests/test_litellm/proxy/test_dynamic_mcp_route.pywith 7 unit tests covering:Fixes LIT-2984
"test" is a mcp access group
Before
After
Note
Medium Risk
Changes request routing for
/{name}/mcp, including new validation/caching behavior that can alter which MCP servers are reachable and when requests 404, so mis-resolution could break existing integrations or unintentionally change access scope.Overview
Updates the dynamic MCP endpoint
/{name}/mcpto also resolve MCP access-group tags and to support comma-separated namespaces (e.g./{a,b}/mcp) by validating/deduping tokens, capping fan-out, and returning 404 when nothing resolves (preventing downstream fallback to an unintended broader server set).Adds lightweight caching for access-group existence checks (including a short negative-cache TTL) and introduces new constants to bound negative-cache duration and maximum CSV tokens. Includes a new unit test suite covering server alias, CSV, toolset, access-group, and 404 behaviors.
Reviewed by Cursor Bugbot for commit cf3b893. Bugbot is set up for automated code reviews on this repo. Configure here.