mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-11 22:51:28 +00:00
fix(mcp): reject initialize with 403 when the key grants no MCP servers (#40616)
* fix(mcp): reject initialize with 403 when the key grants no MCP servers Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * test(mcp): e2e expects 403 initialize for a key with no MCP servers Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * fix(mcp): mention IP filtering in the no-servers initialize denial and keep zero-grant tool coverage Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --------- Co-authored-by: mateo <mateo@berri.ai> Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
This commit is contained in:
parent
bc77aa05d2
commit
c59fc6dc28
4 changed files with 195 additions and 41 deletions
|
|
@ -2048,18 +2048,44 @@ if MCP_AVAILABLE:
|
|||
return texts[0][1]
|
||||
return "\n\n---\n\n".join(f"[{lbl}]\n{txt}" for lbl, txt in texts)
|
||||
|
||||
async def _raise_if_initialize_grants_no_mcp_servers(
|
||||
allowed: Sequence[MCPServer],
|
||||
user_api_key_auth: UserAPIKeyAuth | None,
|
||||
mcp_servers: Sequence[str] | None,
|
||||
client_ip: str | None,
|
||||
) -> None:
|
||||
if allowed or user_api_key_auth is None or not user_api_key_auth.api_key:
|
||||
return
|
||||
if mcp_servers:
|
||||
await raise_denied_scoped_mcp_access(
|
||||
requested_names=mcp_servers,
|
||||
user_api_key_auth=user_api_key_auth,
|
||||
client_ip=client_ip,
|
||||
)
|
||||
no_servers_denial: Final[_McpDeniedDetail] = {
|
||||
"error": (
|
||||
"The key has no MCP servers granted, or none of its granted servers is loaded and allowed for "
|
||||
"this client IP. Grant servers or access groups to the key, its team, or its organization "
|
||||
"(object_permission.mcp_servers), check the server's allowed IPs, and reconnect."
|
||||
)
|
||||
}
|
||||
raise HTTPException(status_code=403, detail=no_servers_denial)
|
||||
|
||||
@contextlib.asynccontextmanager
|
||||
async def _gateway_initialize_instructions_request_scope(
|
||||
user_api_key_auth: UserAPIKeyAuth | None,
|
||||
mcp_servers: list[str] | None,
|
||||
client_ip: str | None,
|
||||
scoped_server_endpoint: bool = False,
|
||||
is_initialize: bool = False,
|
||||
) -> AsyncIterator[None]:
|
||||
allowed: Final = await _get_allowed_mcp_servers(
|
||||
user_api_key_auth=user_api_key_auth,
|
||||
mcp_servers=mcp_servers,
|
||||
client_ip=client_ip,
|
||||
)
|
||||
if is_initialize:
|
||||
await _raise_if_initialize_grants_no_mcp_servers(allowed, user_api_key_auth, mcp_servers, client_ip)
|
||||
if allowed:
|
||||
# return_exceptions=True: a per-server probe failure (incl. CancelledError
|
||||
# bubbled from anyio task group teardown on connection refused) must not
|
||||
|
|
@ -4683,6 +4709,7 @@ if MCP_AVAILABLE:
|
|||
mcp_servers,
|
||||
_client_ip,
|
||||
scoped_server_endpoint=scoped_server_endpoint,
|
||||
is_initialize=is_initialize,
|
||||
):
|
||||
await target_manager.handle_request(scope, receive, local_send)
|
||||
if use_stateful and session_id and scope.get("method") == "DELETE":
|
||||
|
|
@ -4819,6 +4846,7 @@ if MCP_AVAILABLE:
|
|||
mcp_servers,
|
||||
_sse_client_ip,
|
||||
scoped_server_endpoint=scoped_server_endpoint,
|
||||
is_initialize=scope.get("method") == "GET",
|
||||
):
|
||||
await sse_session_manager.handle_request(scope, receive, send)
|
||||
except MCPUpstreamAuthError as e:
|
||||
|
|
|
|||
|
|
@ -510,6 +510,37 @@ async def _call(session: ClientSession, tool_id: str, a: int = 3, b: int = 4) ->
|
|||
return await session.call_tool("call_tool", arguments={"tool_id": tool_id, "arguments": {"a": a, "b": b}})
|
||||
|
||||
|
||||
async def _raw_rpc(
|
||||
proxy_server_url: str, key: str | None, method: str, params: dict[str, object], **headers: str
|
||||
) -> httpx.Response:
|
||||
async with httpx.AsyncClient() as client:
|
||||
return await client.post(
|
||||
f"{proxy_server_url}/mcp/proxy",
|
||||
headers={
|
||||
"Accept": "application/json, text/event-stream",
|
||||
**({"Authorization": f"Bearer {key}"} if key else {}),
|
||||
**headers,
|
||||
},
|
||||
json={"jsonrpc": "2.0", "id": 1, "method": method, "params": params},
|
||||
)
|
||||
|
||||
|
||||
async def _raw_initialize(proxy_server_url: str, key: str | None) -> httpx.Response:
|
||||
return await _raw_rpc(
|
||||
proxy_server_url,
|
||||
key,
|
||||
"initialize",
|
||||
{"protocolVersion": "2025-03-26", "capabilities": {}, "clientInfo": {"name": "auth-test", "version": "1"}},
|
||||
)
|
||||
|
||||
|
||||
def _rpc_result(response: httpx.Response) -> dict[str, typing.Any]:
|
||||
if response.headers["content-type"].startswith("text/event-stream"):
|
||||
data_line = next(line for line in response.text.splitlines() if line.startswith("data:"))
|
||||
return json.loads(data_line.removeprefix("data:"))["result"]
|
||||
return response.json()["result"]
|
||||
|
||||
|
||||
def _assert_unauthorized(result: CallToolResult) -> None:
|
||||
assert result.isError is True
|
||||
assert result.content[0].text == "Unknown or unauthorized tool_id"
|
||||
|
|
@ -527,13 +558,31 @@ class TestProxyMcpAuthorizationScope:
|
|||
_assert_unauthorized(await _call(ungranted, restricted_id))
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_no_mcp_servers_sentinel_hides_every_tool(self, proxy_server_url: str) -> None:
|
||||
async def test_no_mcp_servers_sentinel_rejects_initialize_and_hides_every_tool(self, proxy_server_url: str) -> None:
|
||||
async with _scoped_session(proxy_server_url) as granted:
|
||||
tool_id = (await _search(granted, "add"))["math_stdio-add"]
|
||||
async with _scoped_session(proxy_server_url, "sk-none") as session:
|
||||
assert await _search(session, "add") == {}
|
||||
_assert_unauthorized(await session.call_tool("get_tool_schema", {"tool_id": tool_id}))
|
||||
_assert_unauthorized(await _call(session, tool_id))
|
||||
response = await _raw_initialize(proxy_server_url, "sk-none")
|
||||
assert response.status_code == 403, response.text
|
||||
assert "no MCP servers granted" in response.json()["detail"]["error"]
|
||||
|
||||
async def raw_call(name: str, arguments: dict[str, object]) -> dict[str, typing.Any]:
|
||||
call = await _raw_rpc(proxy_server_url, "sk-none", "tools/call", {"name": name, "arguments": arguments})
|
||||
assert call.status_code == 200, call.text
|
||||
return _rpc_result(call)
|
||||
|
||||
listed = await _raw_rpc(proxy_server_url, "sk-none", "tools/list", {})
|
||||
assert listed.status_code == 200, listed.text
|
||||
assert {tool["name"] for tool in _rpc_result(listed)["tools"]} == {"search_tools", "get_tool_schema", "call_tool"}
|
||||
search = await raw_call("search_tools", {"query": "add"})
|
||||
assert search["isError"] is False, search
|
||||
assert json.loads(search["content"][0]["text"]) == []
|
||||
for name, arguments in (
|
||||
("get_tool_schema", {"tool_id": tool_id}),
|
||||
("call_tool", {"tool_id": tool_id, "arguments": {"a": 3, "b": 4}}),
|
||||
):
|
||||
denied = await raw_call(name, arguments)
|
||||
assert denied["isError"] is True, denied
|
||||
assert denied["content"][0]["text"] == "Unknown or unauthorized tool_id"
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_tool_grant_hides_ungranted_tools_and_blocks_their_ids(self, proxy_server_url: str) -> None:
|
||||
|
|
@ -581,24 +630,7 @@ class TestProxyMcpAuthorizationScope:
|
|||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize("key", [None, "sk-invalid"])
|
||||
async def test_missing_or_invalid_key_cannot_initialize(self, proxy_server_url: str, key: str | None) -> None:
|
||||
async with httpx.AsyncClient() as client:
|
||||
response = await client.post(
|
||||
f"{proxy_server_url}/mcp/proxy",
|
||||
headers={
|
||||
"Accept": "application/json, text/event-stream",
|
||||
**({"Authorization": f"Bearer {key}"} if key else {}),
|
||||
},
|
||||
json={
|
||||
"jsonrpc": "2.0",
|
||||
"id": 1,
|
||||
"method": "initialize",
|
||||
"params": {
|
||||
"protocolVersion": "2025-03-26",
|
||||
"capabilities": {},
|
||||
"clientInfo": {"name": "auth-test", "version": "1"},
|
||||
},
|
||||
},
|
||||
)
|
||||
response = await _raw_initialize(proxy_server_url, key)
|
||||
assert response.status_code == 401, response.text
|
||||
|
||||
@pytest.mark.asyncio
|
||||
|
|
@ -646,25 +678,28 @@ class TestProxyMcpAuthorizationScope:
|
|||
|
||||
@pytest.mark.asyncio
|
||||
async def test_proxy_scope_exception_returns_iserror_and_emits_failure_log(self, proxy_server_url: str) -> None:
|
||||
async with _scoped_session(
|
||||
response = await _raw_rpc(
|
||||
proxy_server_url,
|
||||
"sk-none",
|
||||
"tools/call",
|
||||
{"name": "call_tool", "arguments": {"tool_id": "denied-scope", "arguments": {}}},
|
||||
**{"x-mcp-servers": "math_restricted", "x-litellm-call-id": "proxy-scope-denial"},
|
||||
) as session:
|
||||
result = await session.call_tool("call_tool", {"tool_id": "denied-scope", "arguments": {}})
|
||||
assert result.isError is True
|
||||
assert result.content[0].text == (
|
||||
"Error: The key is not allowed to access the requested MCP servers: math_restricted"
|
||||
)
|
||||
async with asyncio.timeout(10):
|
||||
while True:
|
||||
payload = json.loads(await asyncio.to_thread(proxy_call_recorder.failures.get, True, 5))
|
||||
if payload["id"] == "proxy-scope-denial":
|
||||
break
|
||||
assert payload["call_type"] == "call_mcp_tool"
|
||||
assert payload["status"] == "failure"
|
||||
assert payload["response_cost"] == 0
|
||||
assert "math_restricted" in payload["error_str"]
|
||||
)
|
||||
assert response.status_code == 200, response.text
|
||||
result = _rpc_result(response)
|
||||
assert result["isError"] is True
|
||||
assert result["content"][0]["text"] == (
|
||||
"Error: The key is not allowed to access the requested MCP servers: math_restricted"
|
||||
)
|
||||
async with asyncio.timeout(10):
|
||||
while True:
|
||||
payload = json.loads(await asyncio.to_thread(proxy_call_recorder.failures.get, True, 5))
|
||||
if payload["id"] == "proxy-scope-denial":
|
||||
break
|
||||
assert payload["call_type"] == "call_mcp_tool"
|
||||
assert payload["status"] == "failure"
|
||||
assert payload["response_cost"] == 0
|
||||
assert "math_restricted" in payload["error_str"]
|
||||
|
||||
@pytest.mark.parametrize("arguments", ["wrong", False, None, [], 0])
|
||||
def test_handler_rejects_non_object_arguments(
|
||||
|
|
|
|||
|
|
@ -2003,6 +2003,11 @@ async def test_mcp_routing_chunked_initialize_to_stateful():
|
|||
patch(
|
||||
"litellm.proxy._experimental.mcp_server.server.set_auth_context",
|
||||
),
|
||||
patch( # test-quality-ok: registry is empty in unit tests; key owns one server
|
||||
"litellm.proxy._experimental.mcp_server.server._get_allowed_mcp_servers",
|
||||
new_callable=AsyncMock,
|
||||
return_value=[MagicMock()],
|
||||
),
|
||||
patch(
|
||||
"litellm.proxy._experimental.mcp_server.server._SESSION_MANAGERS_INITIALIZED",
|
||||
True,
|
||||
|
|
@ -2488,6 +2493,11 @@ async def test_initialize_request_tracks_active_session_after_response_header():
|
|||
new_callable=AsyncMock,
|
||||
return_value=(owner_auth, None, None, None, None, None),
|
||||
),
|
||||
patch( # test-quality-ok: registry is empty in unit tests; key owns one server
|
||||
"litellm.proxy._experimental.mcp_server.server._get_allowed_mcp_servers",
|
||||
new_callable=AsyncMock,
|
||||
return_value=[MagicMock()],
|
||||
),
|
||||
patch(
|
||||
"litellm.proxy._experimental.mcp_server.server._SESSION_MANAGERS_INITIALIZED",
|
||||
True,
|
||||
|
|
@ -2600,6 +2610,11 @@ async def test_initialize_request_with_existing_session_tracks_new_session():
|
|||
{"x-new-header": "new"},
|
||||
),
|
||||
),
|
||||
patch( # test-quality-ok: registry is empty in unit tests; key owns one server
|
||||
"litellm.proxy._experimental.mcp_server.server._get_allowed_mcp_servers",
|
||||
new_callable=AsyncMock,
|
||||
return_value=[MagicMock()],
|
||||
),
|
||||
patch(
|
||||
"litellm.proxy._experimental.mcp_server.server._SESSION_MANAGERS_INITIALIZED",
|
||||
True,
|
||||
|
|
@ -5614,6 +5629,78 @@ class TestGatewayCreateInitializationOptions:
|
|||
|
||||
assert server.create_initialization_options().server_name == "litellm-mcp-server"
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_initialize_with_no_granted_servers_returns_403(self):
|
||||
from fastapi import HTTPException
|
||||
|
||||
from litellm.proxy._experimental.mcp_server.server import (
|
||||
_gateway_initialize_instructions_request_scope,
|
||||
)
|
||||
from litellm.proxy._types import UserAPIKeyAuth
|
||||
|
||||
with patch( # test-quality-ok: grant resolution is the input under test
|
||||
"litellm.proxy._experimental.mcp_server.server._get_allowed_mcp_servers",
|
||||
new_callable=AsyncMock,
|
||||
return_value=[],
|
||||
):
|
||||
with pytest.raises(HTTPException) as exc_info:
|
||||
async with _gateway_initialize_instructions_request_scope(
|
||||
user_api_key_auth=UserAPIKeyAuth(api_key="sk-no-mcp"),
|
||||
mcp_servers=None,
|
||||
client_ip=None,
|
||||
is_initialize=True,
|
||||
):
|
||||
pytest.fail("initialize must not proceed when the key grants no MCP servers")
|
||||
|
||||
assert exc_info.value.status_code == 403
|
||||
assert "no MCP servers granted" in exc_info.value.detail["error"]
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_initialize_with_no_granted_scoped_servers_returns_scoped_denial(self):
|
||||
from fastapi import HTTPException
|
||||
|
||||
from litellm.proxy._experimental.mcp_server.server import (
|
||||
_gateway_initialize_instructions_request_scope,
|
||||
)
|
||||
from litellm.proxy._types import UserAPIKeyAuth
|
||||
|
||||
with patch( # test-quality-ok: grant resolution is the input under test
|
||||
"litellm.proxy._experimental.mcp_server.server._get_allowed_mcp_servers",
|
||||
new_callable=AsyncMock,
|
||||
return_value=[],
|
||||
):
|
||||
with pytest.raises(HTTPException) as exc_info:
|
||||
async with _gateway_initialize_instructions_request_scope(
|
||||
user_api_key_auth=UserAPIKeyAuth(api_key="sk-no-mcp"),
|
||||
mcp_servers=["grafana"],
|
||||
client_ip=None,
|
||||
is_initialize=True,
|
||||
):
|
||||
pytest.fail("scoped initialize must not proceed when nothing resolves")
|
||||
|
||||
assert exc_info.value.status_code == 403
|
||||
assert "grafana" in exc_info.value.detail["error"]
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_non_initialize_request_with_no_granted_servers_is_not_rejected_here(self):
|
||||
from litellm.proxy._experimental.mcp_server.server import (
|
||||
_gateway_initialize_instructions_request_scope,
|
||||
_mcp_gateway_initialize_instructions,
|
||||
)
|
||||
from litellm.proxy._types import UserAPIKeyAuth
|
||||
|
||||
with patch( # test-quality-ok: grant resolution is the input under test
|
||||
"litellm.proxy._experimental.mcp_server.server._get_allowed_mcp_servers",
|
||||
new_callable=AsyncMock,
|
||||
return_value=[],
|
||||
):
|
||||
async with _gateway_initialize_instructions_request_scope(
|
||||
user_api_key_auth=UserAPIKeyAuth(api_key="sk-no-mcp"),
|
||||
mcp_servers=None,
|
||||
client_ip=None,
|
||||
):
|
||||
assert _mcp_gateway_initialize_instructions.get() is None
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_sse_handler_scopes_server_name_from_single_server_path(self):
|
||||
try:
|
||||
|
|
|
|||
|
|
@ -750,8 +750,7 @@ async def test_admitted_subject_missing_stored_token_challenged_with_resource_me
|
|||
challenge = exc_info.value.headers["www-authenticate"]
|
||||
assert "authorization_uri=" not in challenge
|
||||
assert challenge == (
|
||||
'Bearer resource_metadata="http://localhost:8000'
|
||||
'/.well-known/oauth-protected-resource/mcp/repro_oauth_server"'
|
||||
'Bearer resource_metadata="http://localhost:8000/.well-known/oauth-protected-resource/mcp/repro_oauth_server"'
|
||||
)
|
||||
|
||||
|
||||
|
|
@ -938,6 +937,11 @@ async def test_handle_streamable_http_mcp_delegated_server_surfaces_upstream_cha
|
|||
"litellm.proxy._experimental.mcp_server.server.global_mcp_server_manager.get_mcp_server_by_name",
|
||||
return_value=delegated_server,
|
||||
),
|
||||
patch( # test-quality-ok: registry is empty in unit tests; key owns the delegated server
|
||||
"litellm.proxy._experimental.mcp_server.server._get_allowed_mcp_servers",
|
||||
new_callable=AsyncMock,
|
||||
return_value=[delegated_server],
|
||||
),
|
||||
patch.object(
|
||||
session_manager_stateful,
|
||||
"handle_request",
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue