mirror of
https://github.com/BerriAI/litellm.git
synced 2026-08-28 05:25:59 +00:00
fix(mcp): keep tool identity exact, fold case only where registration does
Greptile flagged that match_known_tool_name case-folded both the configured entry and the derived spellings. Routing keeps two tools whose names differ only in case as two tools, so folding merged identities the dispatcher separates: on a server exposing getPet and getpet, an allowlist naming getPet also granted getpet, and a blocklist naming getPet also denied getpet. That is unauthorized execution on one arm and the wrong tool denied on the other. Matching is now exact, which is what identity means here. The case leniency it replaces was never typo tolerance; _register_openapi_tools rewrites every operationId through sanitize_openapi_tool_name, so an allowed_tools entry holding the spec's own spelling never equals the registered name. That link is recovered by replaying the same rewrite, and only on servers that carry a spec_path, which is how the rest of the manager already recognizes an OpenAPI server. Every name that rewrite produces is lowercased, so no two tools on such a server can differ only in case and the fold cannot merge anything. Native servers get no folding at all. test_case_folding_applies_to_openapi_ servers_and_not_to_native_ones pins both halves, and two tests pin that a policy naming one tool leaves its case-variant sibling alone. Dropping the spec_path guard, dropping the fold, and forcing the fold path are all killed. The two pre-existing case-insensitivity tests describe OpenAPI servers in their own docstrings but built fixtures without a spec_path, a shape production never produces for one; they now set it.
This commit is contained in:
parent
b2d4dde464
commit
7962407be0
3 changed files with 54 additions and 19 deletions
|
|
@ -343,14 +343,30 @@ def match_known_tool_name(tool_name: str, server: MCPServer, names: Iterable[str
|
|||
"""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. Matching spans every spelling routing
|
||||
accepts and ignores case, so discovery hides exactly what dispatch refuses. 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".
|
||||
``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.
|
||||
|
||||
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.
|
||||
|
||||
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".
|
||||
"""
|
||||
entries = {name.casefold(): name for name in names}
|
||||
spellings = map(str.casefold, iter_known_tool_name_spellings(tool_name, server))
|
||||
return next((entries[spelling] for spelling in spellings if spelling in entries), None)
|
||||
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)
|
||||
|
||||
|
||||
def split_server_prefix_from_name(prefixed_name: str) -> Tuple[str, str]:
|
||||
|
|
|
|||
|
|
@ -4519,6 +4519,7 @@ def test_tool_name_matches_case_insensitive():
|
|||
server_name="per_store",
|
||||
url="http://127.0.0.1:5115/mcp",
|
||||
transport=MCPTransport.http,
|
||||
spec_path="/specs/petstore.yaml",
|
||||
)
|
||||
|
||||
# Test case 1: Unprefixed tool name with camelCase in filter list
|
||||
|
|
@ -4597,6 +4598,7 @@ def test_filter_tools_by_allowed_tools_case_insensitive():
|
|||
name="per_store",
|
||||
server_name="per_store",
|
||||
transport=MCPTransport.http,
|
||||
spec_path="/specs/petstore.yaml",
|
||||
allowed_tools=["addPet", "updatePet", "findPetsByStatus"],
|
||||
)
|
||||
|
||||
|
|
@ -8080,12 +8082,19 @@ class TestListFiltersHonorThePrefixBoundary:
|
|||
|
||||
assert not _tool_name_matches(f"{self.SERVER_ID}-read_wiki_contents", ["read_wiki_structure"], server)
|
||||
|
||||
def test_match_is_still_case_insensitive(self):
|
||||
def test_case_folding_applies_to_openapi_servers_and_not_to_native_ones(self):
|
||||
# Registration rewrites operationIds through sanitize_openapi_tool_name, so
|
||||
# folding recovers a spec-spelled entry on an OpenAPI server. A native server
|
||||
# gets none of it: routing dispatches two names differing only in case as two
|
||||
# tools, so one policy must not decide both.
|
||||
from litellm.proxy._experimental.mcp_server.server import _tool_name_matches
|
||||
|
||||
server = self._alias_less_server()
|
||||
native = self._alias_less_server()
|
||||
openapi = self._alias_less_server(spec_path="/specs/petstore.yaml")
|
||||
|
||||
assert _tool_name_matches(f"{self.SERVER_ID}-findPetsByStatus", ["findpetsbystatus"], server)
|
||||
assert _tool_name_matches(f"{self.SERVER_ID}-findPetsByStatus", ["findpetsbystatus"], openapi)
|
||||
assert not _tool_name_matches(f"{self.SERVER_ID}-findPetsByStatus", ["findpetsbystatus"], native)
|
||||
assert _tool_name_matches(f"{self.SERVER_ID}-findpetsbystatus", ["findpetsbystatus"], native)
|
||||
|
||||
def test_alias_form_entry_matches_a_tool_published_under_the_short_prefix(self, monkeypatch):
|
||||
# Routing accepts the alias form, so an entry stored before short
|
||||
|
|
@ -8111,6 +8120,10 @@ class TestListFiltersHonorThePrefixBoundary:
|
|||
|
||||
A spelling the blocklist enforces but the filter misses leaves a blocked
|
||||
tool advertised; the reverse hides a tool that would have been callable.
|
||||
Every spelling routing registers bans, and its upper-cased form bans nothing,
|
||||
because a tool's identity is its exact name; asserting the verdict and not only
|
||||
the agreement is what keeps this from passing on a matcher that answers wrongly
|
||||
but consistently.
|
||||
"""
|
||||
from mcp.types import Tool as MCPTool
|
||||
|
||||
|
|
@ -8146,14 +8159,14 @@ class TestListFiltersHonorThePrefixBoundary:
|
|||
|
||||
published = MCPTool(name="eiG-read_wiki_contents", description="", inputSchema={"type": "object"})
|
||||
for spelling in registered:
|
||||
for entry in (spelling, spelling.upper()):
|
||||
for entry, expected in ((spelling, True), (spelling.upper(), False)):
|
||||
server = _server(disallowed_tools=[entry])
|
||||
|
||||
refused = not manager.check_allowed_or_banned_tools("read_wiki_contents", server)
|
||||
hidden = filter_tools_by_allowed_tools([published], server) == []
|
||||
|
||||
assert refused, entry
|
||||
assert hidden, entry
|
||||
assert refused == hidden, entry
|
||||
assert refused is expected, entry
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize(
|
||||
|
|
|
|||
|
|
@ -9512,19 +9512,25 @@ class TestServerToolListsHonorThePrefixBoundary:
|
|||
assert "include_internal" in exc_info.value.detail["error"]
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_a_case_variant_blocklist_entry_still_blocks(self):
|
||||
server = self._aliased_server(disallowed_tools=["PetStore-DeletePet"])
|
||||
async def test_a_blocklist_entry_does_not_reach_a_case_variant_sibling_tool(self):
|
||||
server = self._aliased_server(disallowed_tools=["petstore-getPet"])
|
||||
|
||||
with pytest.raises(HTTPException) as exc_info:
|
||||
await self._run_check(server, "deletepet")
|
||||
await self._run_check(server, "getPet")
|
||||
|
||||
assert exc_info.value.status_code == 403
|
||||
await self._run_check(server, "getpet")
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_a_case_variant_allowlist_entry_grants_the_tool(self):
|
||||
server = self._aliased_server(allowed_tools=["PetStore-GetPetById"])
|
||||
async def test_an_allowlist_entry_does_not_grant_a_case_variant_sibling_tool(self):
|
||||
server = self._aliased_server(allowed_tools=["petstore-getPet"])
|
||||
|
||||
await self._run_check(server, "getpetbyid")
|
||||
await self._run_check(server, "getPet")
|
||||
|
||||
with pytest.raises(HTTPException) as exc_info:
|
||||
await self._run_check(server, "getpet")
|
||||
|
||||
assert exc_info.value.status_code == 403
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_an_explicitly_empty_allowed_params_list_refuses_every_parameter(self):
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue