mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-07 08:26:10 +00:00
fix(mcp): address Greptile review feedback
- Defense-in-depth: warn instead of hard-fail for legacy servers - Move os import to module level in _types.py - Document args residual risk in allowlist comment - Add UpdateMCPServerRequest allowlist test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
f5a8f1e699
commit
db88b389f4
4 changed files with 25 additions and 12 deletions
|
|
@ -143,6 +143,8 @@ MCP_HEALTH_CHECK_TIMEOUT = float(os.getenv("LITELLM_MCP_HEALTH_CHECK_TIMEOUT", "
|
|||
|
||||
# Allowlist of commands permitted for MCP stdio transport.
|
||||
# Prevents arbitrary command execution via /mcp-rest/test/* endpoints or server creation.
|
||||
# Note: allowlisted runtimes can still execute code via args (e.g. python -c "...").
|
||||
# This is an accepted residual risk since these endpoints require PROXY_ADMIN.
|
||||
# Extend via LITELLM_MCP_STDIO_EXTRA_COMMANDS env var (comma-separated).
|
||||
_MCP_STDIO_EXTRA_COMMANDS = os.getenv("LITELLM_MCP_STDIO_EXTRA_COMMANDS", "")
|
||||
MCP_STDIO_ALLOWED_COMMANDS: frozenset = frozenset(
|
||||
|
|
|
|||
|
|
@ -981,8 +981,9 @@ class MCPServerManager:
|
|||
from litellm.constants import MCP_NPM_CACHE_DIR
|
||||
|
||||
resolved_env["NPM_CONFIG_CACHE"] = MCP_NPM_CACHE_DIR
|
||||
# Defense-in-depth: validate command even if Pydantic validation was bypassed
|
||||
# (e.g. MCPServer built from config/DB records predating the allowlist)
|
||||
# Defense-in-depth: warn for commands not in the allowlist.
|
||||
# The Pydantic validator blocks new servers; this catches legacy
|
||||
# config/DB records predating the allowlist.
|
||||
if server.command:
|
||||
import os as _os
|
||||
|
||||
|
|
@ -990,9 +991,12 @@ class MCPServerManager:
|
|||
|
||||
base_command = _os.path.basename(server.command)
|
||||
if base_command not in MCP_STDIO_ALLOWED_COMMANDS:
|
||||
raise ValueError(
|
||||
f"Command '{server.command}' is not in the allowed commands list "
|
||||
f"for stdio transport. Allowed commands: {sorted(MCP_STDIO_ALLOWED_COMMANDS)}"
|
||||
verbose_logger.warning(
|
||||
"MCP stdio command '%s' is not in the allowlist (%s). "
|
||||
"Add it to LITELLM_MCP_STDIO_EXTRA_COMMANDS to suppress this warning. "
|
||||
"A future release may block non-allowlisted commands.",
|
||||
server.command,
|
||||
sorted(MCP_STDIO_ALLOWED_COMMANDS),
|
||||
)
|
||||
|
||||
stdio_config: Optional[MCPStdioConfig] = None
|
||||
|
|
|
|||
|
|
@ -1,5 +1,6 @@
|
|||
import enum
|
||||
import json
|
||||
import os
|
||||
from datetime import datetime
|
||||
from typing import TYPE_CHECKING, Any, Callable, Dict, List, Literal, Optional, Union
|
||||
|
||||
|
|
@ -1156,11 +1157,9 @@ class NewMCPServerRequest(LiteLLMPydanticObjectBase):
|
|||
if not values.get("args"):
|
||||
raise ValueError("args is required for stdio transport")
|
||||
# Validate command against allowlist to prevent arbitrary execution
|
||||
import os as _os
|
||||
|
||||
from litellm.constants import MCP_STDIO_ALLOWED_COMMANDS
|
||||
|
||||
base_command = _os.path.basename(values["command"])
|
||||
base_command = os.path.basename(values["command"])
|
||||
if base_command not in MCP_STDIO_ALLOWED_COMMANDS:
|
||||
raise ValueError(
|
||||
f"Command '{values['command']}' is not in the allowed commands list "
|
||||
|
|
@ -1227,11 +1226,9 @@ class UpdateMCPServerRequest(LiteLLMPydanticObjectBase):
|
|||
if not values.get("args"):
|
||||
raise ValueError("args is required for stdio transport")
|
||||
# Validate command against allowlist to prevent arbitrary execution
|
||||
import os as _os
|
||||
|
||||
from litellm.constants import MCP_STDIO_ALLOWED_COMMANDS
|
||||
|
||||
base_command = _os.path.basename(values["command"])
|
||||
base_command = os.path.basename(values["command"])
|
||||
if base_command not in MCP_STDIO_ALLOWED_COMMANDS:
|
||||
raise ValueError(
|
||||
f"Command '{values['command']}' is not in the allowed commands list "
|
||||
|
|
|
|||
|
|
@ -9,7 +9,7 @@ from litellm.proxy._experimental.mcp_server import rest_endpoints
|
|||
from litellm.proxy._experimental.mcp_server.auth import (
|
||||
user_api_key_auth_mcp as auth_mcp,
|
||||
)
|
||||
from litellm.proxy._types import NewMCPServerRequest, UserAPIKeyAuth
|
||||
from litellm.proxy._types import NewMCPServerRequest, UpdateMCPServerRequest, UserAPIKeyAuth
|
||||
from litellm.proxy.auth.user_api_key_auth import user_api_key_auth
|
||||
from litellm.types.mcp import MCPAuth
|
||||
|
||||
|
|
@ -1268,6 +1268,16 @@ class TestStdioCommandAllowlist:
|
|||
)
|
||||
assert req.command == "node"
|
||||
|
||||
def test_update_request_disallowed_command_raises(self):
|
||||
"""UpdateMCPServerRequest should also block non-allowlisted commands."""
|
||||
with pytest.raises(ValueError, match="not in the allowed commands list"):
|
||||
UpdateMCPServerRequest(
|
||||
server_id="some-id",
|
||||
transport="stdio",
|
||||
command="bash",
|
||||
args=["-c", "echo pwned"],
|
||||
)
|
||||
|
||||
|
||||
class TestEndpointRoleChecks:
|
||||
"""Tests for PROXY_ADMIN role checks on MCP test endpoints."""
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue