From 5daf0168a8da6622e69dfe2adaddccd3c3f2517b Mon Sep 17 00:00:00 2001 From: user <70670632+stuxf@users.noreply.github.com> Date: Fri, 1 May 2026 22:02:47 +0000 Subject: [PATCH 1/2] fix(mcp): run pre_call_tool_check on OpenAPI/local-registry path (VERIA-7) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `execute_mcp_tool` dispatches in two ways: managed MCP servers go through `_handle_managed_mcp_tool`, which calls `MCPServerManager.pre_call_tool_check` to enforce allowed/banned tool lists, key/team `object_permission` tool grants, and parameter validation. OpenAPI-backed tools, however, were resolved via `global_mcp_tool_registry` and dispatched directly to `_handle_local_mcp_tool` — entirely skipping `pre_call_tool_check`. A caller could invoke any registered OpenAPI tool regardless of their key/team permissions, including administrative or destructive operations on the upstream API. Run `pre_call_tool_check` before the local-registry dispatch whenever the resolved server is set (the same condition used to surface server context to the managed path). Honor any guardrail-modified arguments the hook returns. Errors raised by the hook propagate up before `_handle_local_mcp_tool` runs. Tests cover both directions: the pre-call hook fires when the local tool resolves alongside a server, and a hook-raised HTTPException prevents the local handler from being invoked. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../proxy/_experimental/mcp_server/server.py | 21 +++ .../mcp_server/test_openapi_tool_auth.py | 154 ++++++++++++++++++ 2 files changed, 175 insertions(+) create mode 100644 tests/test_litellm/proxy/_experimental/mcp_server/test_openapi_tool_auth.py diff --git a/litellm/proxy/_experimental/mcp_server/server.py b/litellm/proxy/_experimental/mcp_server/server.py index ae6055217b..62250798ab 100644 --- a/litellm/proxy/_experimental/mcp_server/server.py +++ b/litellm/proxy/_experimental/mcp_server/server.py @@ -2104,6 +2104,27 @@ 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 whenever we have a + # resolved server, so OpenAPI tools enforce the same allowlist + # the proxy applies to managed MCP tools. + if mcp_server is not None: + 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=kwargs.get("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. diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/test_openapi_tool_auth.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_openapi_tool_auth.py new file mode 100644 index 0000000000..f1e612bafa --- /dev/null +++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_openapi_tool_auth.py @@ -0,0 +1,154 @@ +""" +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 + + +@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() From 8ee599aa7d1d030707704d92ff70f720a1dd9169 Mon Sep 17 00:00:00 2001 From: user <70670632+stuxf@users.noreply.github.com> Date: Fri, 1 May 2026 22:28:46 +0000 Subject: [PATCH 2/2] fix(mcp): use canonical proxy_logging_obj, deny when MCP server is unresolvable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Greptile flagged two follow-ups on the OpenAPI/local-registry pre-call check: 1. **P1 runtime crash via None proxy_logging_obj.** `kwargs.get("proxy_logging_obj")` is `None` on the MCP entry path, and `pre_call_tool_check` calls `proxy_logging_obj._create_mcp_request_object_from_kwargs` unconditionally after the security checks, which would have crashed every legitimate call with `AttributeError`. Source the logging object from `litellm.proxy.proxy_server` the same way `_handle_managed_mcp_tool` already does. 2. **P2 authorization-bypass window when mcp_server is None.** Previously the new check was guarded by `if mcp_server is not None`, so any local tool whose registry entry had no resolvable server (a startup-race window before `_initialize_tool_name_to_mcp_server_name_mapping` completes, or an orphaned registry entry) ran without the security check. Tools registered via openapi_to_mcp_generator are always tied to a server, so a missing one is a configuration/timing fault — fail the call with 503 instead of dispatching unguarded. Tests: existing two pass with an added assertion that `proxy_logging_obj` is non-None at the call site, plus a new test that covers the 503 deny branch when the tool→server mapping is missing. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../proxy/_experimental/mcp_server/server.py | 52 ++++++++++----- .../mcp_server/test_openapi_tool_auth.py | 66 +++++++++++++++++++ 2 files changed, 102 insertions(+), 16 deletions(-) diff --git a/litellm/proxy/_experimental/mcp_server/server.py b/litellm/proxy/_experimental/mcp_server/server.py index 62250798ab..26e508f1d5 100644 --- a/litellm/proxy/_experimental/mcp_server/server.py +++ b/litellm/proxy/_experimental/mcp_server/server.py @@ -2107,23 +2107,43 @@ if MCP_AVAILABLE: # 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 whenever we have a - # resolved server, so OpenAPI tools enforce the same allowlist - # the proxy applies to managed MCP tools. - if mcp_server is not None: - 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=kwargs.get("proxy_logging_obj"), - server=mcp_server, - raw_headers=raw_headers, + # 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` 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"] + + # `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 diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/test_openapi_tool_auth.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_openapi_tool_auth.py index f1e612bafa..3ad01e9c3e 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/test_openapi_tool_auth.py +++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_openapi_tool_auth.py @@ -83,6 +83,11 @@ async def test_openapi_local_tool_runs_pre_call_tool_check(): 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 @@ -152,3 +157,64 @@ async def test_openapi_local_tool_blocked_when_pre_call_check_raises(): 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()