mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-23 00:41:40 +00:00
fix(mcp): read the whole initialize body under allowlist enforcement and surface a stored empty allowlist in the UI
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
This commit is contained in:
parent
c085619f29
commit
14639bbb5a
4 changed files with 143 additions and 18 deletions
|
|
@ -3826,14 +3826,18 @@ if MCP_AVAILABLE:
|
|||
|
||||
return allowed_mcp_clients_from_general_settings(general_settings)
|
||||
|
||||
def _routing_peek_limit(allowed_clients: frozenset[str] | None) -> int | None:
|
||||
return _MCP_ROUTING_PEEK_MAX_BYTES if allowed_clients is None else None
|
||||
|
||||
async def _reject_initialize_from_disallowed_client(
|
||||
scope: Scope,
|
||||
receive: Receive,
|
||||
send: Send,
|
||||
body: bytes,
|
||||
client_ip: str | None,
|
||||
allowed_clients: frozenset[str] | None,
|
||||
) -> bool:
|
||||
rejection: Final = check_mcp_client_allowed(body, _load_allowed_mcp_clients())
|
||||
rejection: Final = check_mcp_client_allowed(body, allowed_clients)
|
||||
if rejection is None:
|
||||
return False
|
||||
verbose_logger.warning(
|
||||
|
|
@ -3860,19 +3864,24 @@ if MCP_AVAILABLE:
|
|||
|
||||
async def _read_request_body_for_routing(
|
||||
receive: Receive,
|
||||
peek_max_bytes: int | None = _MCP_ROUTING_PEEK_MAX_BYTES,
|
||||
) -> tuple[list[Message], bytes]:
|
||||
"""
|
||||
Read just enough of the request body to decide whether this is a
|
||||
JSON-RPC ``initialize`` call. Returns the consumed ASGI messages so
|
||||
the caller can replay them faithfully to the downstream handler, and
|
||||
the peeked body bytes (capped at ``_MCP_ROUTING_PEEK_MAX_BYTES``).
|
||||
the peeked body bytes (capped at ``peek_max_bytes``).
|
||||
|
||||
Stops reading from the wire as soon as either (a) we have peeked
|
||||
``_MCP_ROUTING_PEEK_MAX_BYTES`` of body, or (b) the body is complete.
|
||||
``peek_max_bytes`` of body, or (b) the body is complete.
|
||||
The remainder of an oversized body is streamed lazily through
|
||||
``wrapped_receive`` in the caller — so an authenticated client cannot
|
||||
force the proxy to buffer an arbitrarily large payload just to make a
|
||||
routing decision.
|
||||
|
||||
``peek_max_bytes=None`` reads the whole body. The client allowlist
|
||||
needs that: a truncated initialize body parses as non-initialize and
|
||||
would otherwise skip the allowlist check entirely.
|
||||
"""
|
||||
consumed_messages: Final[list[Message]] = []
|
||||
body_chunks: Final[list[bytes]] = []
|
||||
|
|
@ -3893,7 +3902,7 @@ if MCP_AVAILABLE:
|
|||
# handler via ``consumed_messages``, but ``body_chunks`` is
|
||||
# purely for the JSON-RPC method check — there is no reason
|
||||
# to copy a large body frame into a second buffer.
|
||||
remaining = _MCP_ROUTING_PEEK_MAX_BYTES - peeked_bytes
|
||||
remaining = len(body) if peek_max_bytes is None else peek_max_bytes - peeked_bytes
|
||||
if remaining > 0:
|
||||
body_chunks.append(body[:remaining])
|
||||
peeked_bytes += min(len(body), remaining)
|
||||
|
|
@ -3901,7 +3910,7 @@ if MCP_AVAILABLE:
|
|||
if not message.get("more_body", False):
|
||||
break
|
||||
|
||||
if peeked_bytes >= _MCP_ROUTING_PEEK_MAX_BYTES:
|
||||
if peek_max_bytes is not None and peeked_bytes >= peek_max_bytes:
|
||||
# Stop draining; downstream replay will pull remaining chunks
|
||||
# directly from the original `receive` via wrapped_receive.
|
||||
break
|
||||
|
|
@ -4556,10 +4565,13 @@ if MCP_AVAILABLE:
|
|||
|
||||
body = b""
|
||||
if scope.get("method") == "POST":
|
||||
consumed_messages, body = await _read_request_body_for_routing(receive)
|
||||
allowed_clients: Final = _load_allowed_mcp_clients()
|
||||
consumed_messages, body = await _read_request_body_for_routing(
|
||||
receive, _routing_peek_limit(allowed_clients)
|
||||
)
|
||||
is_initialize = _is_initialize_request(body)
|
||||
if is_initialize and await _reject_initialize_from_disallowed_client(
|
||||
scope, receive, send, body, _client_ip
|
||||
scope, receive, send, body, _client_ip, allowed_clients
|
||||
):
|
||||
return
|
||||
|
||||
|
|
@ -4830,11 +4842,14 @@ if MCP_AVAILABLE:
|
|||
await initialize_session_managers()
|
||||
await asyncio.sleep(0.1)
|
||||
|
||||
sse_allowed_clients: Final = _load_allowed_mcp_clients()
|
||||
sse_consumed_messages, sse_body = (
|
||||
await _read_request_body_for_routing(receive) if scope.get("method") == "POST" else ((), b"")
|
||||
await _read_request_body_for_routing(receive, _routing_peek_limit(sse_allowed_clients))
|
||||
if scope.get("method") == "POST"
|
||||
else ((), b"")
|
||||
)
|
||||
if _is_initialize_request(sse_body) and await _reject_initialize_from_disallowed_client(
|
||||
scope, receive, send, sse_body, _sse_client_ip
|
||||
scope, receive, send, sse_body, _sse_client_ip, sse_allowed_clients
|
||||
):
|
||||
return
|
||||
sse_receive: Final = (
|
||||
|
|
|
|||
|
|
@ -2242,6 +2242,78 @@ async def test_streamable_http_allowlist_only_inspects_initialize_requests() ->
|
|||
send.assert_not_awaited()
|
||||
|
||||
|
||||
def _oversized_initialize(client_name: str, peek_cap: int) -> bytes:
|
||||
import json as _json
|
||||
|
||||
return _json.dumps(
|
||||
{
|
||||
"jsonrpc": "2.0",
|
||||
"id": 0,
|
||||
"method": "initialize",
|
||||
"params": {
|
||||
"protocolVersion": "2025-06-18",
|
||||
"capabilities": {"experimental": {"padding": "x" * (peek_cap * 2)}},
|
||||
"clientInfo": {"name": client_name, "version": "1.0.0"},
|
||||
},
|
||||
}
|
||||
).encode()
|
||||
|
||||
|
||||
def _chunked_receive(body: bytes, chunk_size: int) -> AsyncMock:
|
||||
chunks: Final = [body[i : i + chunk_size] for i in range(0, len(body), chunk_size)]
|
||||
return AsyncMock(
|
||||
side_effect=[
|
||||
{"type": "http.request", "body": chunk, "more_body": i + 1 < len(chunks)} for i, chunk in enumerate(chunks)
|
||||
]
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize(("client_name", "admitted"), (("antigravity-cli", True), ("claude-code", False)))
|
||||
async def test_streamable_http_allowlist_reads_past_the_routing_peek_cap_for_initialize(
|
||||
client_name: str, admitted: bool
|
||||
) -> None:
|
||||
from starlette.types import Receive, Scope, Send
|
||||
|
||||
from litellm.proxy._experimental.mcp_server import server as mcp_module
|
||||
|
||||
request_body: Final = _oversized_initialize(client_name, mcp_module._MCP_ROUTING_PEEK_MAX_BYTES)
|
||||
assert len(request_body) > mcp_module._MCP_ROUTING_PEEK_MAX_BYTES
|
||||
scope: Final[Scope] = {"type": "http", "method": "POST", "path": "/mcp", "headers": []}
|
||||
receive: Final = _chunked_receive(request_body, 1024)
|
||||
send: Final = AsyncMock()
|
||||
downstream_bodies: Final[list[bytes]] = []
|
||||
|
||||
async def handle_request(_: Scope, downstream_receive: Receive, __: Send) -> None:
|
||||
downstream_bodies.append(await _drain_body(downstream_receive))
|
||||
|
||||
stateful_handle: Final = AsyncMock(side_effect=handle_request)
|
||||
stateless_handle: Final = AsyncMock(side_effect=handle_request)
|
||||
|
||||
with (
|
||||
_client_allowlist_patches(["antigravity-cli"]),
|
||||
patch( # test-quality-ok: session managers are module singletons; the downstream call is the observable
|
||||
"litellm.proxy._experimental.mcp_server.server.session_manager_stateful",
|
||||
SimpleNamespace(handle_request=stateful_handle),
|
||||
),
|
||||
patch( # test-quality-ok: session managers are module singletons; the downstream call is the observable
|
||||
"litellm.proxy._experimental.mcp_server.server.session_manager_stateless",
|
||||
SimpleNamespace(handle_request=stateless_handle),
|
||||
),
|
||||
):
|
||||
await mcp_module.handle_streamable_http_mcp(scope, receive, send)
|
||||
|
||||
if admitted:
|
||||
assert downstream_bodies == [request_body]
|
||||
stateless_handle.assert_not_awaited()
|
||||
send.assert_not_awaited()
|
||||
return
|
||||
assert downstream_bodies == []
|
||||
status, body = _forbidden_client_response(send)
|
||||
assert status == 403
|
||||
assert body["details"] == "MCP client 'claude-code' is not listed in this gateway's mcp_allowed_clients."
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize(
|
||||
("request_body", "admitted"),
|
||||
|
|
|
|||
|
|
@ -176,6 +176,29 @@ describe("MCPNetworkSettings", () => {
|
|||
expect(updateConfigFieldSetting).not.toHaveBeenCalledWith("tok", "mcp_allowed_clients", expect.anything());
|
||||
});
|
||||
|
||||
it("warns that a stored empty allowlist denies every client and lets Save remove it", async () => {
|
||||
vi.mocked(getGeneralSettingsCall).mockResolvedValue([{ field_name: "mcp_allowed_clients", field_value: [] }]);
|
||||
|
||||
renderSettings();
|
||||
|
||||
expect(await screen.findByText(/An empty allowlist is currently stored, so every client is denied/)).toBeVisible();
|
||||
|
||||
await userEvent.click(screen.getByRole("button", { name: /Save/ }));
|
||||
|
||||
await waitFor(() => expect(deleteConfigFieldSetting).toHaveBeenCalledWith("tok", "mcp_allowed_clients"));
|
||||
expect(updateConfigFieldSetting).not.toHaveBeenCalled();
|
||||
await waitFor(() => expect(screen.queryByText(/An empty allowlist is currently stored/)).not.toBeInTheDocument());
|
||||
});
|
||||
|
||||
it("does not show the deny-all warning when no allowlist is stored", async () => {
|
||||
vi.mocked(getGeneralSettingsCall).mockResolvedValue([{ field_name: "mcp_allowed_clients", field_value: null }]);
|
||||
|
||||
renderSettings();
|
||||
|
||||
await screen.findByText("Allowed Client Applications");
|
||||
expect(screen.queryByText(/every client is denied/)).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("keeps the private ranges and the allowed clients as independent settings on save", async () => {
|
||||
vi.mocked(getGeneralSettingsCall).mockResolvedValue([
|
||||
{ field_name: "mcp_internal_ip_ranges", field_value: ["10.0.0.0/8"] },
|
||||
|
|
|
|||
|
|
@ -29,13 +29,16 @@ function ipToSlash24(ip: string): string {
|
|||
|
||||
const sameList = (a: string[], b: string[]) => a.length === b.length && a.every((value, i) => value === b[i]);
|
||||
|
||||
const unchangedSinceLoad = (value: string[], stored: string[] | null) =>
|
||||
stored === null ? value.length === 0 : value.length > 0 && sameList(value, stored);
|
||||
|
||||
const MCPNetworkSettings: React.FC<MCPNetworkSettingsProps> = ({ accessToken }) => {
|
||||
const [loading, setLoading] = useState(true);
|
||||
const [saving, setSaving] = useState(false);
|
||||
const [privateRanges, setPrivateRanges] = useState<string[]>([]);
|
||||
const [allowedClients, setAllowedClients] = useState<string[]>([]);
|
||||
const [storedRanges, setStoredRanges] = useState<string[]>([]);
|
||||
const [storedClients, setStoredClients] = useState<string[]>([]);
|
||||
const [storedRanges, setStoredRanges] = useState<string[] | null>(null);
|
||||
const [storedClients, setStoredClients] = useState<string[] | null>(null);
|
||||
const [currentIp, setCurrentIp] = useState<string | null>(null);
|
||||
const [rangeDraft, setRangeDraft] = useState("");
|
||||
const [clientDraft, setClientDraft] = useState("");
|
||||
|
|
@ -51,11 +54,11 @@ const MCPNetworkSettings: React.FC<MCPNetworkSettingsProps> = ({ accessToken })
|
|||
try {
|
||||
const settings = await getGeneralSettingsCall(accessToken);
|
||||
for (const field of settings) {
|
||||
if (field.field_name === "mcp_internal_ip_ranges" && field.field_value) {
|
||||
if (field.field_name === "mcp_internal_ip_ranges" && Array.isArray(field.field_value)) {
|
||||
setPrivateRanges(field.field_value);
|
||||
setStoredRanges(field.field_value);
|
||||
}
|
||||
if (field.field_name === "mcp_allowed_clients" && field.field_value) {
|
||||
if (field.field_name === "mcp_allowed_clients" && Array.isArray(field.field_value)) {
|
||||
setAllowedClients(field.field_value);
|
||||
setStoredClients(field.field_value);
|
||||
}
|
||||
|
|
@ -78,15 +81,20 @@ const MCPNetworkSettings: React.FC<MCPNetworkSettingsProps> = ({ accessToken })
|
|||
const persistList = async (
|
||||
token: string,
|
||||
fieldName: "mcp_internal_ip_ranges" | "mcp_allowed_clients",
|
||||
{ value, stored, setStored }: { value: string[]; stored: string[]; setStored: (value: string[]) => void },
|
||||
{
|
||||
value,
|
||||
stored,
|
||||
setStored,
|
||||
}: { value: string[]; stored: string[] | null; setStored: (value: string[] | null) => void },
|
||||
) => {
|
||||
if (sameList(value, stored)) return;
|
||||
if (unchangedSinceLoad(value, stored)) return;
|
||||
if (value.length > 0) {
|
||||
await updateConfigFieldSetting(token, fieldName, value);
|
||||
} else {
|
||||
await deleteConfigFieldSetting(token, fieldName);
|
||||
setStored(value);
|
||||
return;
|
||||
}
|
||||
setStored(value);
|
||||
await deleteConfigFieldSetting(token, fieldName);
|
||||
setStored(null);
|
||||
};
|
||||
|
||||
const handleSave = async () => {
|
||||
|
|
@ -155,6 +163,7 @@ const MCPNetworkSettings: React.FC<MCPNetworkSettingsProps> = ({ accessToken })
|
|||
}
|
||||
|
||||
const suggestedRange = currentIp ? ipToSlash24(currentIp) : null;
|
||||
const storedAllowlistDeniesEveryone = storedClients !== null && storedClients.length === 0;
|
||||
|
||||
return (
|
||||
<div className="space-y-6 p-4">
|
||||
|
|
@ -241,6 +250,12 @@ const MCPNetworkSettings: React.FC<MCPNetworkSettingsProps> = ({ accessToken })
|
|||
<div className="mb-2 flex items-center">
|
||||
<p className="text-sm font-medium">Allowed Client Names</p>
|
||||
</div>
|
||||
{storedAllowlistDeniesEveryone && (
|
||||
<p className="mb-2 text-sm text-destructive">
|
||||
An empty allowlist is currently stored, so every client is denied. Save with the list empty to remove it and
|
||||
allow every client again.
|
||||
</p>
|
||||
)}
|
||||
{allowedClients.length > 0 && (
|
||||
<div className="mb-2 flex flex-wrap gap-1.5">
|
||||
{allowedClients.map((client) => (
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue