Merge pull request #27016 from stuxf/fix/mcp-openapi-tool-auth-bypass
fix(mcp): run pre_call_tool_check on OpenAPI/local-registry path (VERIA-7)
This commit is contained in:
yuneng-jiang
GitHub
commit
c154b0df24
@ -2138,6 +2138,47 @@ if MCP_AVAILABLE:
|
||||
#########################################################
|
||||
local_tool = global_mcp_tool_registry.get_tool(name)
|
||||
if local_tool:
|
||||
# OpenAPI-backed tools used to bypass `pre_call_tool_check` —
|
||||
# only the managed path ran allowed/banned-tool checks, key/team
|
||||
# tool permissions, and parameter validation. Run the same checks
|
||||
# before dispatching to the local registry. Refuse the call if
|
||||
# we cannot resolve a server: tools registered via
|
||||
# openapi_to_mcp_generator are always tied to a server, so a
|
||||
# missing mcp_server here means the tool->server mapping has
|
||||
# not finished initializing or the registry entry is orphaned.
|
||||
# Skipping the check would re-open the same authorization gap.
|
||||
if mcp_server is None:
|
||||
raise HTTPException(
|
||||
status_code=503,
|
||||
detail=(
|
||||
f"MCP server for tool '{name}' is not available; "
|
||||
"refusing to dispatch without authorization checks. "
|
||||
"Retry once the server is registered."
|
||||
),
|
||||
)
|
||||
|
||||
# `pre_call_tool_check` calls into `proxy_logging_obj` for the
|
||||
# pre-call guardrail hooks, so source it from the canonical
|
||||
# `proxy_server` module the same way `_handle_managed_mcp_tool`
|
||||
# does. `kwargs.get("proxy_logging_obj")` is None on the MCP
|
||||
# entry path and would crash with AttributeError after the
|
||||
# security checks pass.
|
||||
from litellm.proxy.proxy_server import proxy_logging_obj
|
||||
|
||||
hook_result = await global_mcp_server_manager.pre_call_tool_check(
|
||||
name=original_tool_name,
|
||||
arguments=arguments or {},
|
||||
server_name=server_name or mcp_server.name,
|
||||
user_api_key_auth=user_api_key_auth,
|
||||
proxy_logging_obj=proxy_logging_obj,
|
||||
server=mcp_server,
|
||||
raw_headers=raw_headers,
|
||||
)
|
||||
# `pre_call_tool_check` may return guardrail-modified
|
||||
# arguments; honor them on the local path too.
|
||||
if isinstance(hook_result, dict) and "arguments" in hook_result:
|
||||
arguments = hook_result["arguments"]
|
||||
|
||||
verbose_logger.debug(f"Executing local registry tool: {name}")
|
||||
# For BYOK servers the credential must be injected via a ContextVar
|
||||
# because the tool function has headers baked into its closure.
|
||||
|
||||
@ -0,0 +1,220 @@
|
||||
"""
|
||||
VERIA-7 regression: OpenAPI-backed (local-registry) MCP tools must run
|
||||
through `pre_call_tool_check` before dispatch, the same as managed
|
||||
MCP server tools.
|
||||
"""
|
||||
|
||||
from datetime import datetime, timezone
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
import pytest
|
||||
|
||||
from litellm.proxy._types import LitellmUserRoles, UserAPIKeyAuth
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_openapi_local_tool_runs_pre_call_tool_check():
|
||||
"""When `execute_mcp_tool` resolves a local-registry (OpenAPI) tool
|
||||
AND a server, the pre-call hook must fire before the local handler
|
||||
runs. Pre-fix this path skipped the hook entirely."""
|
||||
from litellm.proxy._experimental.mcp_server import server as mcp_module
|
||||
|
||||
user = UserAPIKeyAuth(
|
||||
api_key="sk-user",
|
||||
user_id="alice",
|
||||
user_role=LitellmUserRoles.INTERNAL_USER.value,
|
||||
)
|
||||
|
||||
fake_server = MagicMock()
|
||||
fake_server.name = "openapi-petstore"
|
||||
fake_server.is_byok = False
|
||||
fake_server.auth_type = None
|
||||
fake_server.mcp_info = None
|
||||
fake_server.server_id = "srv-1"
|
||||
fake_server.server_name = "openapi-petstore"
|
||||
|
||||
fake_tool = MagicMock()
|
||||
fake_tool.name = "list_pets"
|
||||
|
||||
pre_call = AsyncMock(return_value={})
|
||||
handle_local = AsyncMock(return_value=[])
|
||||
|
||||
with (
|
||||
patch.object(
|
||||
mcp_module.global_mcp_server_manager,
|
||||
"_get_mcp_server_from_tool_name",
|
||||
return_value=fake_server,
|
||||
),
|
||||
patch.object(
|
||||
mcp_module.global_mcp_server_manager,
|
||||
"pre_call_tool_check",
|
||||
new=pre_call,
|
||||
),
|
||||
patch.object(
|
||||
mcp_module.global_mcp_tool_registry,
|
||||
"get_tool",
|
||||
return_value=fake_tool,
|
||||
),
|
||||
patch(
|
||||
"litellm.proxy._experimental.mcp_server.server._handle_local_mcp_tool",
|
||||
new=handle_local,
|
||||
),
|
||||
patch(
|
||||
"litellm.proxy._experimental.mcp_server.server.MCPRequestHandler.is_tool_allowed",
|
||||
return_value=True,
|
||||
),
|
||||
):
|
||||
await mcp_module.execute_mcp_tool(
|
||||
name="list_pets",
|
||||
arguments={"limit": 10},
|
||||
allowed_mcp_servers=[fake_server],
|
||||
start_time=datetime.now(timezone.utc),
|
||||
user_api_key_auth=user,
|
||||
)
|
||||
|
||||
pre_call.assert_awaited_once()
|
||||
handle_local.assert_awaited_once()
|
||||
|
||||
# The pre-call hook must run before _handle_local_mcp_tool so an
|
||||
# unauthorized tool is blocked before any work runs. AsyncMock
|
||||
# records call order indirectly — we already asserted both were
|
||||
# called; the relative ordering is enforced by the source change.
|
||||
pre_call_kwargs = pre_call.await_args.kwargs
|
||||
assert pre_call_kwargs["name"] == "list_pets"
|
||||
assert pre_call_kwargs["server"] is fake_server
|
||||
assert pre_call_kwargs["user_api_key_auth"] is user
|
||||
# `proxy_logging_obj` must be sourced from the canonical proxy_server
|
||||
# module (same as the managed path) — passing None would crash the
|
||||
# downstream `_create_mcp_request_object_from_kwargs` call with
|
||||
# AttributeError after the security checks succeed.
|
||||
assert pre_call_kwargs["proxy_logging_obj"] is not None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_openapi_local_tool_blocked_when_pre_call_check_raises():
|
||||
"""If the pre-call check raises (caller not authorized for this
|
||||
tool), the local handler must NOT be invoked."""
|
||||
from fastapi import HTTPException
|
||||
|
||||
from litellm.proxy._experimental.mcp_server import server as mcp_module
|
||||
|
||||
user = UserAPIKeyAuth(
|
||||
api_key="sk-user",
|
||||
user_id="alice",
|
||||
user_role=LitellmUserRoles.INTERNAL_USER.value,
|
||||
)
|
||||
|
||||
fake_server = MagicMock()
|
||||
fake_server.name = "openapi-petstore"
|
||||
fake_server.is_byok = False
|
||||
fake_server.auth_type = None
|
||||
fake_server.mcp_info = None
|
||||
fake_server.server_id = "srv-1"
|
||||
fake_server.server_name = "openapi-petstore"
|
||||
|
||||
fake_tool = MagicMock()
|
||||
fake_tool.name = "delete_pet"
|
||||
|
||||
pre_call = AsyncMock(
|
||||
side_effect=HTTPException(status_code=403, detail="not allowed")
|
||||
)
|
||||
handle_local = AsyncMock(return_value=[])
|
||||
|
||||
with (
|
||||
patch.object(
|
||||
mcp_module.global_mcp_server_manager,
|
||||
"_get_mcp_server_from_tool_name",
|
||||
return_value=fake_server,
|
||||
),
|
||||
patch.object(
|
||||
mcp_module.global_mcp_server_manager,
|
||||
"pre_call_tool_check",
|
||||
new=pre_call,
|
||||
),
|
||||
patch.object(
|
||||
mcp_module.global_mcp_tool_registry,
|
||||
"get_tool",
|
||||
return_value=fake_tool,
|
||||
),
|
||||
patch(
|
||||
"litellm.proxy._experimental.mcp_server.server._handle_local_mcp_tool",
|
||||
new=handle_local,
|
||||
),
|
||||
patch(
|
||||
"litellm.proxy._experimental.mcp_server.server.MCPRequestHandler.is_tool_allowed",
|
||||
return_value=True,
|
||||
),
|
||||
):
|
||||
with pytest.raises(HTTPException) as exc:
|
||||
await mcp_module.execute_mcp_tool(
|
||||
name="delete_pet",
|
||||
arguments={},
|
||||
allowed_mcp_servers=[fake_server],
|
||||
start_time=datetime.now(timezone.utc),
|
||||
user_api_key_auth=user,
|
||||
)
|
||||
|
||||
assert exc.value.status_code == 403
|
||||
pre_call.assert_awaited_once()
|
||||
handle_local.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_openapi_local_tool_denied_when_server_not_resolvable():
|
||||
"""If the local-registry tool is found but no MCP server resolves
|
||||
(startup race or orphaned registry entry), the call must be rejected
|
||||
rather than dispatched without `pre_call_tool_check`."""
|
||||
from fastapi import HTTPException
|
||||
|
||||
from litellm.proxy._experimental.mcp_server import server as mcp_module
|
||||
|
||||
user = UserAPIKeyAuth(
|
||||
api_key="sk-user",
|
||||
user_id="alice",
|
||||
user_role=LitellmUserRoles.INTERNAL_USER.value,
|
||||
)
|
||||
|
||||
fake_tool = MagicMock()
|
||||
fake_tool.name = "list_pets"
|
||||
|
||||
pre_call = AsyncMock(return_value={})
|
||||
handle_local = AsyncMock(return_value=[])
|
||||
|
||||
# `_get_mcp_server_from_tool_name` returns None — no server context.
|
||||
with (
|
||||
patch.object(
|
||||
mcp_module.global_mcp_server_manager,
|
||||
"_get_mcp_server_from_tool_name",
|
||||
return_value=None,
|
||||
),
|
||||
patch.object(
|
||||
mcp_module.global_mcp_server_manager,
|
||||
"pre_call_tool_check",
|
||||
new=pre_call,
|
||||
),
|
||||
patch.object(
|
||||
mcp_module.global_mcp_tool_registry,
|
||||
"get_tool",
|
||||
return_value=fake_tool,
|
||||
),
|
||||
patch(
|
||||
"litellm.proxy._experimental.mcp_server.server._handle_local_mcp_tool",
|
||||
new=handle_local,
|
||||
),
|
||||
patch(
|
||||
"litellm.proxy._experimental.mcp_server.server.MCPRequestHandler.is_tool_allowed",
|
||||
return_value=True,
|
||||
),
|
||||
):
|
||||
with pytest.raises(HTTPException) as exc:
|
||||
await mcp_module.execute_mcp_tool(
|
||||
name="list_pets",
|
||||
arguments={},
|
||||
allowed_mcp_servers=[],
|
||||
start_time=datetime.now(timezone.utc),
|
||||
user_api_key_auth=user,
|
||||
)
|
||||
|
||||
assert exc.value.status_code == 503
|
||||
pre_call.assert_not_awaited()
|
||||
handle_local.assert_not_awaited()
|
||||
Loading…
Reference in New Issue
Block a user