mirror of
https://github.com/BerriAI/litellm.git
synced 2026-08-28 05:25:59 +00:00
refactor(mcp): answer every tool-name permission question through one matcher
The allow list, the deny list, allowed_params and the discovery filter all ask the same question, "which configured entry names this tool on this server", and each answered it in its own idiom: any() over a spelling tuple, all() over the same tuple negated, a next() that pulled a value out of a dict, and a lowercased set membership. Two review findings on this PR were symptoms of that duplication. Deriving the operands differently at one site produced the over-strip; needing a value rather than a boolean at another produced a truthiness test that read an explicitly empty allowed_params list as "nothing configured" and allowed every parameter. match_known_tool_name returns the matching entry or None, and all four sites read it, so no site can test a container's values to decide membership and the empty-list fail-open is no longer representable. Matching is case-insensitive everywhere, which closes the last divergence between discovery and dispatch: a case-variant disallowed_tools entry used to hide a tool from tools/list while tools/call still executed it. Executable lines over the merge-base drop from +9 to +4, all of it the new owner; mcp_server_manager.py loses 12 lines and the discovery filter loses 17.
This commit is contained in:
parent
33a92bd48f
commit
d200e4a8ea
5 changed files with 60 additions and 62 deletions
|
|
@ -118,6 +118,7 @@ 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,
|
||||
merge_mcp_headers,
|
||||
normalize_server_name,
|
||||
|
|
@ -4262,26 +4263,19 @@ class MCPServerManager:
|
|||
"""
|
||||
Check if the tool is allowed or banned for the given server.
|
||||
|
||||
``tool_name`` is bare: every caller resolves the boundary against the
|
||||
server's registered prefixes before dispatch (``server.py``'s
|
||||
``original_tool_name``, the Responses handler's ``sanitized_tool_name``).
|
||||
Stored entries are matched by deriving the spellings routing accepts
|
||||
rather than by stripping the entries, which would cut a second boundary
|
||||
out of a native name that itself opens with the server prefix.
|
||||
``tool_name`` is bare: every caller resolves the boundary against the server's
|
||||
registered prefixes before dispatch (``server.py``'s ``original_tool_name``, the
|
||||
Responses handler's ``sanitized_tool_name``). Configured entries are matched by
|
||||
deriving the spellings routing accepts, never by stripping the entry, which would
|
||||
cut a second boundary out of a native name that opens with the server prefix.
|
||||
"""
|
||||
from litellm.proxy._experimental.mcp_server.utils import (
|
||||
server_applies_tool_allowlist,
|
||||
)
|
||||
|
||||
spellings = tuple(iter_known_tool_name_spellings(tool_name, server))
|
||||
|
||||
if server_applies_tool_allowlist(server):
|
||||
if not server.allowed_tools:
|
||||
return False
|
||||
return any(spelling in server.allowed_tools for spelling in spellings)
|
||||
if server.disallowed_tools:
|
||||
return all(spelling not in server.disallowed_tools for spelling in spellings)
|
||||
return True
|
||||
return match_known_tool_name(tool_name, server, server.allowed_tools or ()) is not None
|
||||
return match_known_tool_name(tool_name, server, server.disallowed_tools or ()) is None
|
||||
|
||||
def validate_allowed_params(self, tool_name: str, arguments: dict[str, Any], server: MCPServer) -> None:
|
||||
"""
|
||||
|
|
@ -4299,18 +4293,12 @@ class MCPServerManager:
|
|||
Raises:
|
||||
HTTPException: If allowed_params is configured for this tool but arguments contain disallowed params
|
||||
"""
|
||||
# If no allowed_params configured, return all arguments
|
||||
if not server.allowed_params:
|
||||
allowed_params = server.allowed_params or {}
|
||||
matched = match_known_tool_name(tool_name, server, allowed_params)
|
||||
if matched is None:
|
||||
return
|
||||
|
||||
spellings = iter_known_tool_name_spellings(tool_name, server)
|
||||
allowed_params_list = next(
|
||||
(server.allowed_params[name] for name in spellings if name in server.allowed_params), None
|
||||
)
|
||||
|
||||
# If this tool doesn't have allowed_params specified, allow all params
|
||||
if allowed_params_list is None:
|
||||
return None
|
||||
allowed_params_list = allowed_params[matched]
|
||||
|
||||
# Filter arguments to only include allowed parameters
|
||||
disallowed_params = [param for param in arguments.keys() if param not in allowed_params_list]
|
||||
|
|
|
|||
|
|
@ -65,7 +65,7 @@ from litellm.proxy._experimental.mcp_server.utils import (
|
|||
extract_mcp_tool_result_error_message,
|
||||
get_server_prefix,
|
||||
iter_known_server_prefixes,
|
||||
iter_known_tool_name_spellings,
|
||||
match_known_tool_name,
|
||||
)
|
||||
from litellm.proxy._types import (
|
||||
ProxyException,
|
||||
|
|
@ -1421,28 +1421,13 @@ if MCP_AVAILABLE:
|
|||
"""
|
||||
Check if a tool name matches any name in the filter list.
|
||||
|
||||
Matches via the same ``iter_known_tool_name_spellings`` the server-level
|
||||
permission checks use, so discovery hides exactly what dispatch refuses;
|
||||
covering fewer spellings here leaves a blocked tool advertised in
|
||||
``tools/list``. Comparison is case-insensitive to handle OpenAPI
|
||||
operationIds that may be in camelCase.
|
||||
|
||||
Args:
|
||||
tool_name: The tool name to check (may be prefixed like "server-tool_name")
|
||||
filter_list: List of tool names to match against
|
||||
mcp_server: The server the tool belongs to, whose registered prefixes
|
||||
locate the boundary exactly. Required: guessing the boundary at
|
||||
the first separator silently mismatches every tool on a server
|
||||
whose prefix contains the separator.
|
||||
|
||||
Returns:
|
||||
True if any spelling of the tool name is in the filter list
|
||||
Reads the same owner the server-level permission checks use, so discovery hides
|
||||
exactly what dispatch refuses. ``mcp_server`` is required: guessing the boundary
|
||||
at the first separator mismatches every tool on a server whose prefix contains
|
||||
the separator.
|
||||
"""
|
||||
filter_list_lower = {f.lower() for f in filter_list}
|
||||
bare_name = strip_known_server_prefix(tool_name, mcp_server)
|
||||
spellings = (tool_name, *iter_known_tool_name_spellings(bare_name, mcp_server))
|
||||
|
||||
return any(spelling.lower() in filter_list_lower for spelling in spellings)
|
||||
return match_known_tool_name(bare_name, mcp_server, filter_list) is not None
|
||||
|
||||
def filter_tools_by_allowed_tools(
|
||||
tools: list[MCPTool],
|
||||
|
|
|
|||
|
|
@ -23,6 +23,8 @@ import importlib
|
|||
import os
|
||||
from urllib.parse import quote
|
||||
|
||||
from litellm.types.mcp_server.mcp_server_manager import MCPServer
|
||||
|
||||
# Constants
|
||||
#
|
||||
# NOTE: The environment-backed values below are read once, when this module is
|
||||
|
|
@ -326,24 +328,31 @@ def iter_known_server_prefixes(server: Any) -> Iterator[str]:
|
|||
yield from _emit(server_id)
|
||||
|
||||
|
||||
def iter_known_tool_name_spellings(tool_name: str, server: Any) -> Iterator[str]:
|
||||
"""Yield every name that denotes the bare ``tool_name`` on ``server``.
|
||||
|
||||
The bare name, then its wire spelling under each prefix from
|
||||
``iter_known_server_prefixes``. Routing resolves an inbound name against that
|
||||
whole set, so anything keyed by tool name (the routing map, the allow/deny
|
||||
lists, ``allowed_params``) must cover it too or it answers for fewer names
|
||||
than are reachable, which fails open on ``disallowed_tools``.
|
||||
``get_server_prefix`` alone covers only the published spelling, and that moves
|
||||
with the alias and with ``LITELLM_USE_SHORT_MCP_TOOL_PREFIX``. These are
|
||||
spellings of one tool on one server, so honoring all of them normalizes the
|
||||
entry rather than widening a grant.
|
||||
def iter_known_tool_name_spellings(tool_name: str, server: MCPServer) -> Iterator[str]:
|
||||
"""Yield every name that denotes the bare ``tool_name`` on ``server``: the bare name,
|
||||
then its wire spelling under each prefix ``iter_known_server_prefixes`` accepts.
|
||||
``get_server_prefix`` covers only the currently published one, and that moves with the
|
||||
alias and with ``LITELLM_USE_SHORT_MCP_TOOL_PREFIX``.
|
||||
"""
|
||||
yield tool_name
|
||||
for prefix in iter_known_server_prefixes(server):
|
||||
yield add_server_prefix_to_name(tool_name, prefix)
|
||||
|
||||
|
||||
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. 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".
|
||||
"""
|
||||
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)
|
||||
|
||||
|
||||
def split_server_prefix_from_name(prefixed_name: str) -> Tuple[str, str]:
|
||||
"""Return the unprefixed name plus the server name used as prefix.
|
||||
|
||||
|
|
|
|||
|
|
@ -8146,10 +8146,11 @@ class TestListFiltersHonorThePrefixBoundary:
|
|||
|
||||
published = MCPTool(name="eiG-read_wiki_contents", description="", inputSchema={"type": "object"})
|
||||
for spelling in registered:
|
||||
server = _server(disallowed_tools=[spelling])
|
||||
for entry in (spelling, spelling.upper()):
|
||||
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) == []
|
||||
refused = not manager.check_allowed_or_banned_tools("read_wiki_contents", server)
|
||||
hidden = filter_tools_by_allowed_tools([published], server) == []
|
||||
|
||||
assert refused, spelling
|
||||
assert hidden, spelling
|
||||
assert refused, entry
|
||||
assert hidden, entry
|
||||
|
|
|
|||
|
|
@ -9511,6 +9511,21 @@ class TestServerToolListsHonorThePrefixBoundary:
|
|||
assert exc_info.value.status_code == 403
|
||||
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"])
|
||||
|
||||
with pytest.raises(HTTPException) as exc_info:
|
||||
await self._run_check(server, "deletepet")
|
||||
|
||||
assert exc_info.value.status_code == 403
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_a_case_variant_allowlist_entry_grants_the_tool(self):
|
||||
server = self._aliased_server(allowed_tools=["PetStore-GetPetById"])
|
||||
|
||||
await self._run_check(server, "getpetbyid")
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_an_explicitly_empty_allowed_params_list_refuses_every_parameter(self):
|
||||
server = self._alias_less_server(allowed_params={"read_wiki_contents": []})
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue