fix(mcp): error instead of silent empty tools when scoped MCP access is denied; grant agent MCP servers from the UI

This commit is contained in:
mateo-berri 2026-09-01 17:04:48 -07:00
parent b52b5d9421
commit c4982ca407
7 changed files with 398 additions and 7 deletions

View file

@ -23,6 +23,7 @@ from pydantic import AnyUrl, ConfigDict
from starlette.requests import Request as StarletteRequest
from starlette.responses import JSONResponse
from starlette.types import Message, Receive, Scope, Send
from typing_extensions import ReadOnly, TypedDict
from litellm._logging import verbose_logger
from litellm.constants import MAXIMUM_TRACEBACK_LINES_TO_LOG
@ -816,6 +817,15 @@ if MCP_AVAILABLE:
}
}
return ListToolsResult.model_validate({"tools": listing.tools, "_meta": outcome_meta})
except HTTPException as e:
from mcp.shared.exceptions import McpError
from mcp.types import INVALID_REQUEST, ErrorData
detail: Final = e.detail
message: Final = (
str(detail.get("error")) if isinstance(detail, dict) and detail.get("error") else str(detail)
)
raise McpError(ErrorData(code=INVALID_REQUEST, message=message)) from e
except Exception as e:
verbose_logger.exception("Error in list_tools endpoint: %s", e)
# Return empty list instead of failing completely
@ -1440,6 +1450,45 @@ if MCP_AVAILABLE:
return allowed_mcp_servers
class _McpDeniedDetail(TypedDict):
error: ReadOnly[str]
async def _raise_denied_scoped_mcp_access(
requested_names: Sequence[str],
user_api_key_auth: UserAPIKeyAuth | None,
client_ip: str | None = None,
) -> None:
"""A scoped request (``/mcp/<name>`` path or ``x-mcp-servers`` header) resolved to zero
allowed servers. When a requested name IS a registered server visible to this client IP,
the denial is a permission outcome and must be loud: a silent 200 with no tools reads as
a healthy server with no tools. Names matching no registered server stay fail-closed
empty so scoping cannot probe for server existence."""
known_targets: Final = tuple(
(name, server)
for name in requested_names
if (server := global_mcp_server_manager.get_mcp_server_by_name(name, client_ip=client_ip)) is not None
)
if not known_targets:
return
denied_name, denied_server = known_targets[0]
agent_id: Final = user_api_key_auth.agent_id if user_api_key_auth else None
if user_api_key_auth is not None and agent_id:
allowed_without_agent: Final = await global_mcp_server_manager.get_allowed_mcp_servers(
user_api_key_auth.model_copy(update=types.MappingProxyType({"agent_id": None}))
)
if denied_server.server_id in allowed_without_agent:
agent_denial: Final[_McpDeniedDetail] = {
"error": (
f"MCP server '{denied_name}' is not available to this key: the key is bound to "
f"agent '{agent_id}', whose MCP grants do not include this server. Add the server "
f"to the agent's object_permission.mcp_servers (edit the agent in the Admin UI or "
f"PATCH /v1/agents/{agent_id}), or use a key that is not bound to the agent."
)
}
raise HTTPException(status_code=403, detail=agent_denial)
key_denial: Final[_McpDeniedDetail] = {"error": f"The key is not allowed to access server {denied_name}"}
raise HTTPException(status_code=403, detail=key_denial)
def _tool_name_matches(tool_name: str, filter_list: list[str], mcp_server: MCPServer) -> bool:
"""
Check if a tool name matches any name in the filter list.
@ -1964,6 +2013,12 @@ if MCP_AVAILABLE:
mcp_servers=mcp_servers,
client_ip=client_ip,
)
if mcp_servers is not None and not allowed_mcp_servers:
await _raise_denied_scoped_mcp_access(
requested_names=mcp_servers,
user_api_key_auth=user_api_key_auth,
client_ip=client_ip,
)
# Pre-fetch OAuth credentials only when at least one server uses OAuth2,
# to avoid an unnecessary DB round-trip on requests with no OAuth2 MCP servers.
@ -2388,6 +2443,8 @@ if MCP_AVAILABLE:
)
verbose_logger.debug("Successfully fetched %s tools from managed MCP servers", len(listing.tools))
return listing
except HTTPException:
raise
except Exception as e:
verbose_logger.exception("Error getting tools from managed MCP servers: %s", e)
# Continue with an empty listing instead of failing completely

View file

@ -1329,6 +1329,191 @@ async def test_get_tools_from_mcp_servers_handles_all_servers_failing():
mock_logger.info.assert_any_call("Successfully fetched %s tools total from all MCP servers", 0)
def _denied_scope_manager(known_server_names_to_ids: dict[str, str], allowed_without_agent: list[str]) -> MagicMock:
"""A manager whose get_mcp_server_by_name knows the given names and whose
get_allowed_mcp_servers answers the agent-stripped permission rerun."""
servers = {name: MagicMock(server_id=server_id) for name, server_id in known_server_names_to_ids.items()}
manager = MagicMock()
manager.get_mcp_server_by_name = lambda name, client_ip=None: servers.get(name)
manager.get_allowed_mcp_servers = AsyncMock(return_value=allowed_without_agent)
return manager
@pytest.mark.asyncio
async def test_scoped_list_denied_by_agent_binding_raises_403_naming_agent():
"""A scoped tools/list that resolves to zero servers because the key's bound agent lacks the
grant must raise a 403 naming the agent, never return a silent 200 with no tools."""
try:
from litellm.proxy._experimental.mcp_server.server import _get_tools_from_mcp_servers
except ImportError:
pytest.skip("MCP server not available")
user_api_key_auth = UserAPIKeyAuth(api_key="test_key", user_id="test_user", agent_id="agent-123")
mock_manager = _denied_scope_manager({"github": "srv-github"}, allowed_without_agent=["srv-github"])
with (
patch( # test-quality-ok: the permission resolver is a module-level function; the suite's only seam
"litellm.proxy._experimental.mcp_server.server._get_allowed_mcp_servers",
AsyncMock(return_value=[]),
),
patch( # test-quality-ok: the server registry is a module-level singleton; the suite's only seam
"litellm.proxy._experimental.mcp_server.server.global_mcp_server_manager",
mock_manager,
),
):
with pytest.raises(HTTPException) as exc_info:
await _get_tools_from_mcp_servers(
user_api_key_auth=user_api_key_auth,
mcp_auth_header=None,
mcp_servers=["github"],
)
assert exc_info.value.status_code == 403
message = exc_info.value.detail["error"]
assert "github" in message
assert "agent 'agent-123'" in message
rerun_auth = mock_manager.get_allowed_mcp_servers.await_args.args[0]
assert rerun_auth.agent_id is None
assert rerun_auth.user_id == "test_user"
@pytest.mark.asyncio
async def test_scoped_list_denied_for_non_agent_key_raises_generic_403():
"""A scoped tools/list denied for a key with no agent binding raises the generic 403 and
never runs the agent-stripped permission rerun."""
try:
from litellm.proxy._experimental.mcp_server.server import _get_tools_from_mcp_servers
except ImportError:
pytest.skip("MCP server not available")
user_api_key_auth = UserAPIKeyAuth(api_key="test_key", user_id="test_user")
mock_manager = _denied_scope_manager({"github": "srv-github"}, allowed_without_agent=["srv-github"])
with (
patch( # test-quality-ok: the permission resolver is a module-level function; the suite's only seam
"litellm.proxy._experimental.mcp_server.server._get_allowed_mcp_servers",
AsyncMock(return_value=[]),
),
patch( # test-quality-ok: the server registry is a module-level singleton; the suite's only seam
"litellm.proxy._experimental.mcp_server.server.global_mcp_server_manager",
mock_manager,
),
):
with pytest.raises(HTTPException) as exc_info:
await _get_tools_from_mcp_servers(
user_api_key_auth=user_api_key_auth,
mcp_auth_header=None,
mcp_servers=["github"],
)
assert exc_info.value.status_code == 403
message = exc_info.value.detail["error"]
assert "github" in message
assert "agent" not in message
mock_manager.get_allowed_mcp_servers.assert_not_awaited()
@pytest.mark.asyncio
async def test_scoped_list_unknown_server_name_stays_silent_empty():
"""A scoped request naming no registered server stays fail-closed empty (200, no tools), so
scoping cannot probe for server existence."""
try:
from litellm.proxy._experimental.mcp_server.server import _get_tools_from_mcp_servers
except ImportError:
pytest.skip("MCP server not available")
user_api_key_auth = UserAPIKeyAuth(api_key="test_key", user_id="test_user", agent_id="agent-123")
mock_manager = _denied_scope_manager({}, allowed_without_agent=["srv-github"])
with (
patch( # test-quality-ok: the permission resolver is a module-level function; the suite's only seam
"litellm.proxy._experimental.mcp_server.server._get_allowed_mcp_servers",
AsyncMock(return_value=[]),
),
patch( # test-quality-ok: the server registry is a module-level singleton; the suite's only seam
"litellm.proxy._experimental.mcp_server.server.global_mcp_server_manager",
mock_manager,
),
):
result = await _get_tools_from_mcp_servers(
user_api_key_auth=user_api_key_auth,
mcp_auth_header=None,
mcp_servers=["doesnotexist"],
)
assert result.tools == []
assert result.outcomes == {}
mock_manager.get_allowed_mcp_servers.assert_not_awaited()
@pytest.mark.asyncio
async def test_scoped_list_agent_key_denied_by_key_grants_raises_generic_403():
"""When the agent-stripped rerun still denies the server, the denial is not the agent's doing,
so the 403 stays generic instead of blaming the agent binding."""
try:
from litellm.proxy._experimental.mcp_server.server import _get_tools_from_mcp_servers
except ImportError:
pytest.skip("MCP server not available")
user_api_key_auth = UserAPIKeyAuth(api_key="test_key", user_id="test_user", agent_id="agent-123")
mock_manager = _denied_scope_manager({"github": "srv-github"}, allowed_without_agent=[])
with (
patch( # test-quality-ok: the permission resolver is a module-level function; the suite's only seam
"litellm.proxy._experimental.mcp_server.server._get_allowed_mcp_servers",
AsyncMock(return_value=[]),
),
patch( # test-quality-ok: the server registry is a module-level singleton; the suite's only seam
"litellm.proxy._experimental.mcp_server.server.global_mcp_server_manager",
mock_manager,
),
):
with pytest.raises(HTTPException) as exc_info:
await _get_tools_from_mcp_servers(
user_api_key_auth=user_api_key_auth,
mcp_auth_header=None,
mcp_servers=["github"],
)
assert exc_info.value.status_code == 403
message = exc_info.value.detail["error"]
assert "github" in message
assert "agent" not in message
mock_manager.get_allowed_mcp_servers.assert_awaited_once()
@pytest.mark.asyncio
async def test_handle_list_tools_converts_permission_httpexception_to_mcp_error():
"""The MCP protocol handler surfaces a permission HTTPException as a clean JSON-RPC error
(McpError, INVALID_REQUEST) carrying the denial message, instead of a raw 500."""
try:
from litellm.proxy._experimental.mcp_server.server import handle_list_tools
except ImportError:
pytest.skip("MCP server not available")
from mcp.shared.exceptions import McpError
from mcp.types import INVALID_REQUEST
denial_message = "MCP server 'github' is not available to this key: the key is bound to agent 'agent-123'"
denial = HTTPException(status_code=403, detail={"error": denial_message})
with (
patch( # test-quality-ok: the protocol handler reads auth from module context; no injection seam
"litellm.proxy._experimental.mcp_server.server.get_or_extract_auth_context",
new=AsyncMock(return_value=(None, None, None, None, None, None, None)),
),
patch( # test-quality-ok: the listing helper is the handler's only collaborator; the suite's seam
"litellm.proxy._experimental.mcp_server.server._list_mcp_tools",
new=AsyncMock(side_effect=denial),
),
):
with pytest.raises(McpError) as exc_info:
await handle_list_tools()
assert exc_info.value.error.code == INVALID_REQUEST
assert exc_info.value.error.message == denial_message
@pytest.mark.asyncio
async def test_mcp_server_tool_call_body_with_none_arguments():
"""Test that proxy_server_request body handles None arguments correctly"""

View file

@ -313,6 +313,28 @@ export const buildAgentDataFromForm = (values: any, existingAgent?: any) => {
return agentData;
};
/**
* Parse MCP grants from an agent's object_permission into the shared MCP form fields
*/
export const parseMcpPermissionsForForm = (agent: any) => ({
allowed_mcp_servers_and_groups: {
servers: agent.object_permission?.mcp_servers ?? [],
accessGroups: agent.object_permission?.mcp_access_groups ?? [],
},
mcp_tool_permissions: agent.object_permission?.mcp_tool_permissions ?? {},
});
/**
* Build the object_permission payload from the shared MCP form fields.
* Always includes the MCP keys (empty when cleared) so removals persist;
* the proxy merges per key, leaving non-MCP grants untouched.
*/
export const buildMcpObjectPermission = (values: any) => ({
mcp_servers: values.allowed_mcp_servers_and_groups?.servers ?? [],
mcp_access_groups: values.allowed_mcp_servers_and_groups?.accessGroups ?? [],
mcp_tool_permissions: values.mcp_tool_permissions ?? {},
});
/**
* Parse agent data for form fields
*/
@ -356,5 +378,6 @@ export const parseAgentForForm = (agent: any) => {
: [],
// extra_headers: already an array of strings
extra_headers: agent.extra_headers ?? [],
...parseMcpPermissionsForForm(agent),
};
};

View file

@ -1,4 +1,5 @@
import React from "react";
import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
import { fireEvent, render, screen, waitFor } from "@testing-library/react";
import userEvent, { PointerEventsCheckLevel } from "@testing-library/user-event";
import { describe, it, expect, vi, beforeEach } from "vitest";
@ -10,6 +11,12 @@ vi.mock("@/components/networking", () => ({
getAgentInfo: vi.fn(),
patchAgentCall: vi.fn(),
getAgentCreateMetadata: vi.fn(),
getProxyBaseUrl: vi.fn(() => ""),
getUiConfig: vi.fn(async () => ({})),
fetchMCPServers: vi.fn(async () => []),
fetchMCPAccessGroups: vi.fn(async () => []),
fetchMCPToolsets: vi.fn(async () => []),
listMCPTools: vi.fn(async () => ({ tools: [] })),
}));
vi.mock("@/app/(dashboard)/hooks/keys/useKeys", () => ({
@ -77,7 +84,14 @@ const langgraphInfo: AgentCreateInfo = {
const setup = () => userEvent.setup({ pointerEventsCheck: PointerEventsCheckLevel.Never });
const renderView = () => render(<AgentInfoView agentId="agent-1" onClose={vi.fn()} accessToken="tok" isAdmin={true} />);
const renderView = () => {
const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } });
return render(
<QueryClientProvider client={queryClient}>
<AgentInfoView agentId="agent-1" onClose={vi.fn()} accessToken="tok" isAdmin={true} />
</QueryClientProvider>,
);
};
const openEditor = async (user: ReturnType<typeof setup>) => {
await user.click(await screen.findByRole("tab", { name: "Settings" }));
@ -127,6 +141,7 @@ describe("AgentInfoView update payload", () => {
rpm_limit: 222,
session_tpm_limit: 333,
session_rpm_limit: 444,
object_permission: { mcp_servers: [], mcp_access_groups: [], mcp_tool_permissions: {} },
});
});
@ -167,6 +182,7 @@ describe("AgentInfoView update payload", () => {
rpm_limit: 222,
session_tpm_limit: 333,
session_rpm_limit: 444,
object_permission: { mcp_servers: [], mcp_access_groups: [], mcp_tool_permissions: {} },
});
});
@ -244,6 +260,29 @@ describe("AgentInfoView update payload", () => {
api_base: "https://other.example.com",
model: "langgraph/asst_1",
},
object_permission: { mcp_servers: [], mcp_access_groups: [], mcp_tool_permissions: {} },
});
});
it("keeps the agent's existing MCP grants in the update payload", async () => {
vi.mocked(networking.getAgentInfo).mockResolvedValue({
...A2A_AGENT,
object_permission: {
mcp_servers: ["srv-1"],
mcp_access_groups: ["grp-a"],
mcp_tool_permissions: { "srv-1": ["tool_x"] },
},
} as never);
const user = setup();
renderView();
await openEditor(user);
await save(user);
expect(patchedPayload().object_permission).toEqual({
mcp_servers: ["srv-1"],
mcp_access_groups: ["grp-a"],
mcp_tool_permissions: { "srv-1": ["tool_x"] },
});
});

View file

@ -24,6 +24,18 @@ vi.mock("./agent_form_fields", () => ({
unmountedA2AFieldNames: () => [],
}));
vi.mock("@/app/(dashboard)/hooks/mcpServers/useMCPServers", () => ({
useMCPServers: () => ({ data: [{ server_id: "srv-1", server_name: "github" }] }),
}));
vi.mock("@/components/mcp_server_management/MCPServerSelector", () => ({
default: () => <div data-testid="mcp-server-selector" />,
}));
vi.mock("@/components/mcp_server_management/MCPToolPermissions", () => ({
default: () => <div data-testid="mcp-tool-permissions" />,
}));
const agent = {
agent_id: "agent-1",
agent_name: "support-agent",
@ -62,5 +74,17 @@ describe("AgentInfoView settings", () => {
expect(token).toBe("sk-test");
expect(agentId).toBe("agent-1");
expect(payload.tpm_limit).toBe(42);
expect(payload.object_permission).toEqual({ mcp_servers: [], mcp_access_groups: [], mcp_tool_permissions: {} });
});
it("shows MCP grants with server names on the overview tab", async () => {
vi.mocked(networking.getAgentInfo).mockResolvedValue({
...agent,
object_permission: { mcp_servers: ["srv-1"] },
} as unknown as Agent);
render(<AgentInfoView agentId="agent-1" onClose={vi.fn()} accessToken="sk-test" isAdmin={true} />);
expect(await screen.findByText("github (srv-1)")).toBeInTheDocument();
});
});

View file

@ -15,16 +15,27 @@ import { getAgentInfo, patchAgentCall, getAgentCreateMetadata, AgentCreateInfo }
import { Agent } from "@/components/agents/types";
import { KeyResponse } from "@/components/key_team_helpers/key_list";
import { useKeys } from "@/app/(dashboard)/hooks/keys/useKeys";
import { useMCPServers } from "@/app/(dashboard)/hooks/mcpServers/useMCPServers";
import KeyInfoView from "@/components/templates/key_info_view";
import MCPServerSelector from "@/components/mcp_server_management/MCPServerSelector";
import MCPToolPermissions from "@/components/mcp_server_management/MCPToolPermissions";
import AgentVirtualKeys from "./agent_virtual_keys";
import AgentFormFields, { unmountedA2AFieldNames } from "./agent_form_fields";
import DynamicAgentFormFields, { buildDynamicAgentData, unmountedDynamicFieldNames } from "./dynamic_agent_form_fields";
import { AGENT_FORM_CONFIG, buildAgentDataFromForm, parseAgentForForm } from "./agent_config";
import {
AGENT_FORM_CONFIG,
buildAgentDataFromForm,
buildMcpObjectPermission,
parseAgentForForm,
parseMcpPermissionsForForm,
} from "./agent_config";
import {
AgentFormField,
AgentFormValues,
AgentNumberInput,
AgentRequestPayload,
McpServerSelection,
labelWithHint,
omitFieldValues,
useCollapsiblePanels,
} from "./AgentFormKit";
@ -111,7 +122,7 @@ const AgentInfoView: React.FC<AgentInfoViewProps> = ({ agentId, onClose, accessT
} else {
const typeInfo = agentTypeMetadata.find((t) => t.agent_type === agentType);
if (typeInfo) {
form.reset(parseDynamicAgentForForm(data, typeInfo));
form.reset({ ...parseDynamicAgentForForm(data, typeInfo), ...parseMcpPermissionsForForm(data) });
} else {
form.reset(parseAgentForForm(data));
}
@ -131,7 +142,7 @@ const AgentInfoView: React.FC<AgentInfoViewProps> = ({ agentId, onClose, accessT
if (agentType !== "a2a") {
const typeInfo = agentTypeMetadata.find((t) => t.agent_type === agentType);
if (typeInfo) {
form.reset(parseDynamicAgentForForm(agent, typeInfo));
form.reset({ ...parseDynamicAgentForForm(agent, typeInfo), ...parseMcpPermissionsForForm(agent) });
}
}
}
@ -139,6 +150,14 @@ const AgentInfoView: React.FC<AgentInfoViewProps> = ({ agentId, onClose, accessT
const selectedAgentTypeInfo = agentTypeMetadata.find((t) => t.agent_type === detectedAgentType);
const watchedFormValues = useWatch({ control: form.control });
const mcpSelection = useWatch({ control: form.control, name: "allowed_mcp_servers_and_groups" });
const mcpToolPermissions = useWatch({ control: form.control, name: "mcp_tool_permissions" });
const { data: mcpServers = [] } = useMCPServers();
const mcpServerLabel = (serverId: string) => {
const server = mcpServers.find((s) => s.server_id === serverId);
return server?.server_name ? `${server.server_name} (${serverId})` : serverId;
};
const discoveryRequest = useMemo(
() => buildDiscoveryRequest(detectedAgentType, watchedFormValues || {}, selectedAgentTypeInfo),
@ -199,7 +218,10 @@ const AgentInfoView: React.FC<AgentInfoViewProps> = ({ agentId, onClose, accessT
? overlayDiscoveredCardParams(built, appliedDiscoveredSelection.selected_card)
: built;
await patchAgentCall(accessToken, agentId, updateData);
await patchAgentCall(accessToken, agentId, {
...updateData,
object_permission: buildMcpObjectPermission(values),
});
toast.success("Agent updated successfully");
setIsEditing(false);
fetchAgentInfo();
@ -343,7 +365,13 @@ const AgentInfoView: React.FC<AgentInfoViewProps> = ({ agentId, onClose, accessT
<h3 className="text-lg font-medium">MCP Tool Permissions</h3>
<DetailList className="mt-4">
{agent.object_permission.mcp_servers && agent.object_permission.mcp_servers.length > 0 && (
<DetailItem label="MCP Servers">{agent.object_permission.mcp_servers.join(", ")}</DetailItem>
<DetailItem label="MCP Servers">
<div className="space-y-1">
{agent.object_permission.mcp_servers.map((serverId) => (
<div key={serverId}>{mcpServerLabel(serverId)}</div>
))}
</div>
</DetailItem>
)}
{agent.object_permission.mcp_access_groups &&
agent.object_permission.mcp_access_groups.length > 0 && (
@ -357,7 +385,7 @@ const AgentInfoView: React.FC<AgentInfoViewProps> = ({ agentId, onClose, accessT
<div className="space-y-1">
{Object.entries(agent.object_permission.mcp_tool_permissions).map(([serverId, tools]) => (
<div key={serverId}>
<span className="font-medium">{serverId}:</span>{" "}
<span className="font-medium">{mcpServerLabel(serverId)}:</span>{" "}
{Array.isArray(tools) ? tools.join(", ") : String(tools)}
</div>
))}
@ -457,6 +485,40 @@ const AgentInfoView: React.FC<AgentInfoViewProps> = ({ agentId, onClose, accessT
{rateLimitField("session_rpm_limit", "Session RPM Limit")}
</div>
<Separator className="my-6" />
<h3 className="text-lg font-medium mb-4">MCP Servers</h3>
<FieldGroup>
<AgentFormField
name="allowed_mcp_servers_and_groups"
label={labelWithHint(
"Allowed MCP Servers",
"Select which MCP servers or access groups this agent can access. Keys bound to this agent can only reach servers granted here.",
)}
>
{({ value, onChange }) => (
<MCPServerSelector
onChange={onChange}
value={{
servers: (value as McpServerSelection | undefined)?.servers ?? [],
accessGroups: (value as McpServerSelection | undefined)?.accessGroups ?? [],
}}
accessToken={accessToken ?? ""}
placeholder="Select MCP servers or access groups (optional)"
/>
)}
</AgentFormField>
</FieldGroup>
<div className="mt-4">
<MCPToolPermissions
accessToken={accessToken ?? ""}
selectedServers={mcpSelection?.servers ?? []}
toolPermissions={mcpToolPermissions ?? {}}
onChange={(toolPerms: Record<string, string[]>) =>
form.setValue("mcp_tool_permissions", toolPerms)
}
/>
</div>
<div className="mt-6 flex justify-end gap-2">
<Button
type="button"

View file

@ -6239,6 +6239,7 @@ export const patchAgentCall = async (
agent_name?: string;
litellm_params?: Record<string, any>;
agent_card_params?: Record<string, any>;
object_permission?: Record<string, any>;
tpm_limit?: number | null;
rpm_limit?: number | null;
session_tpm_limit?: number | null;