Skip to content

Commit e45655a

Browse files
yuneng-jiangishaan-jaff
authored andcommitted
[Fix] UI MCP Tool Test Regression (#16695)
* Fix UI MCP testing tool regression * Fixed linting
1 parent 06eeb28 commit e45655a

File tree

2 files changed

+98
-19
lines changed

2 files changed

+98
-19
lines changed

litellm/proxy/_experimental/mcp_server/server.py

Lines changed: 27 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -640,24 +640,31 @@ async def call_mcp_tool(
640640

641641
allowed_mcp_servers = await _get_allowed_mcp_servers_from_mcp_server_names(
642642
mcp_servers=mcp_servers,
643-
allowed_mcp_servers=allowed_mcp_servers
643+
allowed_mcp_servers=allowed_mcp_servers,
644644
)
645645

646-
server_name: Optional[str]
647-
if len(allowed_mcp_servers) == 1:
648-
original_tool_name, server_name = name, allowed_mcp_servers[0].server_name
649-
else:
650-
# Remove prefix from tool name for logging and processing
651-
original_tool_name, server_name = get_server_name_prefix_tool_mcp(name)
646+
# Track resolved MCP server for both permission checks and dispatch
647+
mcp_server: Optional[MCPServer] = None
652648

653-
if not server_name or not MCPRequestHandler.is_tool_allowed(
654-
allowed_mcp_servers=[server.name for server in allowed_mcp_servers],
655-
server_name=server_name,
656-
):
657-
raise HTTPException(
658-
status_code=403,
659-
detail=f"User not allowed to call this tool. Allowed MCP servers: {allowed_mcp_servers}",
660-
)
649+
# Remove prefix from tool name for logging and processing
650+
original_tool_name, server_name = get_server_name_prefix_tool_mcp(name)
651+
652+
# If tool name is unprefixed, resolve its server so we can enforce permissions
653+
if not server_name:
654+
mcp_server = global_mcp_server_manager._get_mcp_server_from_tool_name(name)
655+
if mcp_server:
656+
server_name = mcp_server.name
657+
658+
# Only enforce server-level permissions when we can resolve a server
659+
if server_name:
660+
if not MCPRequestHandler.is_tool_allowed(
661+
allowed_mcp_servers=[server.name for server in allowed_mcp_servers],
662+
server_name=server_name,
663+
):
664+
raise HTTPException(
665+
status_code=403,
666+
detail=f"User not allowed to call this tool. Allowed MCP servers: {allowed_mcp_servers}",
667+
)
661668

662669
standard_logging_mcp_tool_call: StandardLoggingMCPToolCall = (
663670
_get_standard_logging_mcp_tool_call(
@@ -686,9 +693,11 @@ async def call_mcp_tool(
686693
# Primary and recommended way to use external MCP servers
687694
#########################################################
688695
else:
689-
mcp_server: Optional[
690-
MCPServer
691-
] = global_mcp_server_manager._get_mcp_server_from_tool_name(name)
696+
# If we haven't already resolved the server, do it now for dispatch
697+
if mcp_server is None:
698+
mcp_server = global_mcp_server_manager._get_mcp_server_from_tool_name(
699+
name
700+
)
692701
if mcp_server:
693702
standard_logging_mcp_tool_call["mcp_server_cost_info"] = (
694703
mcp_server.mcp_info or {}

tests/mcp_tests/test_mcp_server.py

Lines changed: 71 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2584,8 +2584,78 @@ async def test_call_mcp_tool_uses_manager_permission_lookup():
25842584

25852585
assert result == expected_response
25862586
mock_get_allowed.assert_awaited_once()
2587-
assert mock_get_server.call_count == 2
2587+
# We call `_get_mcp_server_from_tool_name` multiple times:
2588+
# - for logging/metadata
2589+
# - for resolving the server during dispatch
2590+
# - and inside `call_tool` for guardrails/hooks
2591+
# The exact count isn't important, only that it is used.
2592+
assert mock_get_server.call_count >= 2
2593+
# First call should use the prefixed tool name
25882594
assert (
25892595
mock_get_server.call_args_list[0][0][0]
25902596
== f"{mock_server.name}/gmail_send_email"
25912597
)
2598+
2599+
2600+
@pytest.mark.asyncio
2601+
async def test_call_mcp_tool_resolves_unprefixed_tool_name_and_checks_permissions():
2602+
"""
2603+
Ensure `call_mcp_tool` correctly resolves the MCP server for an unprefixed tool
2604+
name and enforces server-level permissions using that resolved server.
2605+
"""
2606+
from litellm.proxy._experimental.mcp_server.server import (
2607+
call_mcp_tool,
2608+
global_mcp_server_manager,
2609+
)
2610+
2611+
mock_server = MCPServer(
2612+
server_id="server-123",
2613+
name="test_server",
2614+
alias="test_server",
2615+
server_name="test_server",
2616+
url="https://test-server.com/mcp",
2617+
transport=MCPTransport.http,
2618+
mcp_info={"server_name": "test_server"},
2619+
)
2620+
2621+
expected_response = [TextContent(type="text", text="ok")]
2622+
2623+
with patch.object(
2624+
global_mcp_server_manager,
2625+
"get_allowed_mcp_servers",
2626+
new_callable=AsyncMock,
2627+
) as mock_get_allowed, patch.object(
2628+
global_mcp_server_manager,
2629+
"get_mcp_servers_from_ids",
2630+
return_value=[mock_server],
2631+
), patch.object(
2632+
global_mcp_server_manager,
2633+
"_get_mcp_server_from_tool_name",
2634+
return_value=mock_server,
2635+
) as mock_get_server, patch(
2636+
"litellm.proxy._experimental.mcp_server.server.global_mcp_tool_registry"
2637+
) as mock_tool_registry, patch(
2638+
"litellm.proxy._experimental.mcp_server.server._handle_managed_mcp_tool",
2639+
new_callable=AsyncMock,
2640+
) as mock_handle_managed, patch(
2641+
"litellm.proxy._experimental.mcp_server.server.MCPRequestHandler.is_tool_allowed",
2642+
return_value=True,
2643+
) as mock_is_allowed:
2644+
mock_get_allowed.return_value = [mock_server.server_id]
2645+
mock_tool_registry.get_tool.return_value = None
2646+
mock_handle_managed.return_value = expected_response
2647+
2648+
# Call with UNPREFIXED tool name; server should be resolved via mapping
2649+
result = await call_mcp_tool(
2650+
name="gmail_send_email",
2651+
arguments={"body": "hello"},
2652+
mcp_servers=["test_server"],
2653+
)
2654+
2655+
assert result == expected_response
2656+
mock_get_allowed.assert_awaited_once()
2657+
# We should resolve the server at least once using the unprefixed name
2658+
assert mock_get_server.call_count >= 1
2659+
assert mock_get_server.call_args_list[0][0][0] == "gmail_send_email"
2660+
# Permissions check should be invoked with the resolved server name
2661+
mock_is_allowed.assert_called_once()

0 commit comments

Comments
 (0)