From 179ebdb86bfefea9cde3d9e9028b7e62927af1eb Mon Sep 17 00:00:00 2001 From: Tin Date: Mon, 27 Jul 2026 17:34:20 -0700 Subject: [PATCH] fix(mcp): make the operationId to tool-name map a single owner Greptile found that the OpenAPI fallback added a commit ago collapsed operation IDs that registration keeps apart: foo/bar and foo.bar register as two tools but sanitize_openapi_tool_name rewrites both to foo_bar, so a policy naming either also decided the other. The cause was two owners for one map, and picking the wrong one. Registration names an operationId inline at _register_openapi_tools with operation_id.replace(" ", "_").lower(), which keeps / and . ; the separate sanitize_openapi_tool_name replaces every character outside [a-zA-Z0-9_-] and belongs to register_tools_from_openapi, which has no production caller. Nothing made the matcher use the one that actually registers, so it used the lookalike. That inline expression is now openapi_tool_name in utils, and both registration and the matcher call it. Replaying the registering function is the whole safety argument, and it is structural rather than a claim: two operationIds that register as two tools normalize to two names here by construction, because this is the map that registered them. A coarser lookalike cannot be substituted without a test failing. The matcher also loses its exact-then-fallback split. The transform is identity on native servers and idempotent on already-registered names, so normalizing both sides is exact matching where no OpenAPI spec is involved. Executable lines drop by three this round; the branch is +5 over the merge-base for four shared owners that removed duplication at six call sites. --- .../mcp_server/mcp_server_manager.py | 5 ++- .../proxy/_experimental/mcp_server/utils.py | 45 ++++++++++--------- .../mcp_server/test_mcp_server_manager.py | 10 +++++ 3 files changed, 36 insertions(+), 24 deletions(-) diff --git a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py index 0462e76c2ad..89c83459524 100644 --- a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py +++ b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py @@ -118,10 +118,11 @@ from litellm.proxy._experimental.mcp_server.utils import ( is_short_mcp_tool_prefix_enabled, iter_known_server_prefixes, iter_known_tool_name_spellings, - match_known_tool_name, match_known_server_prefix, + match_known_tool_name, merge_mcp_headers, normalize_server_name, + openapi_tool_name, parse_admin_env_vars, strip_known_server_prefix, validate_mcp_server_name, @@ -1785,7 +1786,7 @@ class MCPServerManager: # Generate tool name (without prefix initially) operation_id = operation.get("operationId", f"{method}_{path.replace('/', '_')}") - base_tool_name = operation_id.replace(" ", "_").lower() + base_tool_name = openapi_tool_name(operation_id) # Add server prefix to tool name prefixed_tool_name = add_server_prefix_to_name(base_tool_name, server_prefix) diff --git a/litellm/proxy/_experimental/mcp_server/utils.py b/litellm/proxy/_experimental/mcp_server/utils.py index 4c0690082d5..698df147247 100644 --- a/litellm/proxy/_experimental/mcp_server/utils.py +++ b/litellm/proxy/_experimental/mcp_server/utils.py @@ -2,7 +2,10 @@ MCP Server Utilities """ +import hashlib +import importlib import json +import os import re from collections.abc import MutableMapping, MutableSequence from typing import ( @@ -17,10 +20,6 @@ from typing import ( Tuple, Union, ) - -import hashlib -import importlib -import os from urllib.parse import quote from litellm.types.mcp_server.mcp_server_manager import MCPServer @@ -339,34 +338,36 @@ def iter_known_tool_name_spellings(tool_name: str, server: MCPServer) -> Iterato yield add_server_prefix_to_name(tool_name, prefix) +def openapi_tool_name(operation_id: str) -> str: + """Return the tool name ``_register_openapi_tools`` registers ``operation_id`` under. + + The single transform between a spec's operationId and the name the gateway serves. + Policy recovers the link by replaying this exact function, which is what keeps it from + deciding for a tool it does not name: two operationIds that register as two tools + necessarily normalize to two names here, because this is the map that registered them. + """ + return operation_id.replace(" ", "_").lower() + + def match_known_tool_name(tool_name: str, server: MCPServer, names: Iterable[str]) -> str | None: """Return the entry of ``names`` that denotes ``tool_name`` on ``server``, else ``None``. The single question every tool-name-keyed site asks: the allow list, the deny list, ``allowed_params`` and the discovery filter, so discovery hides exactly what dispatch - refuses. It spans every spelling routing accepts and no more. A tool's identity is its - exact name, because routing dispatches two names differing only in case as two tools, - and folding case here would let one policy decide both. + refuses. It spans every spelling routing accepts and no more, because a tool's identity + is the exact name routing dispatches; anything looser lets one policy decide two tools. - OpenAPI servers are the exception, and not a fuzzy one. ``_register_openapi_tools`` - rewrites every operationId through ``sanitize_openapi_tool_name``, so configuration - holding the spec's own spelling never equals the registered name; replaying that exact - rewrite on the entry recovers the link. It cannot merge identities, because every name - it produces is lowercased, so no two tools on such a server differ only in case. + On an OpenAPI server the configured entry holds the spec's operationId while routing + holds :func:`openapi_tool_name` of it, so both sides go through that map first. Doing it + with the registering function rather than a lookalike is the whole safety argument: a + coarser one collapses operationIds that registration keeps apart. Callers read the returned entry rather than testing a container's values, which is what stops an explicitly empty ``allowed_params`` list from reading as "nothing configured". """ - from litellm.proxy._experimental.mcp_server.openapi_to_mcp_generator import ( - sanitize_openapi_tool_name, - ) - - spellings = set(iter_known_tool_name_spellings(tool_name, server)) - exact = next((name for name in names if name in spellings), None) - if exact is not None or not getattr(server, "spec_path", None): - return exact - sanitized = {sanitize_openapi_tool_name(spelling) for spelling in spellings} - return next((name for name in names if sanitize_openapi_tool_name(name) in sanitized), None) + normalize = openapi_tool_name if getattr(server, "spec_path", None) else str + spellings = {normalize(spelling) for spelling in iter_known_tool_name_spellings(tool_name, server)} + return next((name for name in names if normalize(name) in spellings), None) def split_server_prefix_from_name(prefixed_name: str) -> Tuple[str, str]: diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py index cdef50ad6b5..db5b64ef131 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py +++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py @@ -9511,6 +9511,16 @@ class TestServerToolListsHonorThePrefixBoundary: assert exc_info.value.status_code == 403 assert "include_internal" in exc_info.value.detail["error"] + @pytest.mark.asyncio + async def test_an_entry_does_not_decide_an_operation_id_registration_keeps_separate(self): + server = self._aliased_server(disallowed_tools=["foo/bar"], spec_path="/specs/petstore.yaml") + + with pytest.raises(HTTPException) as exc_info: + await self._run_check(server, "foo/bar") + + assert exc_info.value.status_code == 403 + await self._run_check(server, "foo.bar") + @pytest.mark.asyncio async def test_a_blocklist_entry_does_not_reach_a_case_variant_sibling_tool(self): server = self._aliased_server(disallowed_tools=["petstore-getPet"])