From db88b389f4cc0f8df582d44a0eb87aa2724d5802 Mon Sep 17 00:00:00 2001 From: Sameer Kankute Date: Thu, 26 Mar 2026 16:05:25 +0530 Subject: [PATCH] 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 --- litellm/constants.py | 2 ++ .../_experimental/mcp_server/mcp_server_manager.py | 14 +++++++++----- litellm/proxy/_types.py | 9 +++------ .../mcp_server/test_rest_endpoints.py | 12 +++++++++++- 4 files changed, 25 insertions(+), 12 deletions(-) diff --git a/litellm/constants.py b/litellm/constants.py index 9661e585d99..a7e6be9de0a 100644 --- a/litellm/constants.py +++ b/litellm/constants.py @@ -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( diff --git a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py index 76a05f2b1e2..d68492a5a5b 100644 --- a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py +++ b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py @@ -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 diff --git a/litellm/proxy/_types.py b/litellm/proxy/_types.py index 16b72958209..fa0eb5ec80a 100644 --- a/litellm/proxy/_types.py +++ b/litellm/proxy/_types.py @@ -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 " diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/test_rest_endpoints.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_rest_endpoints.py index 25786d982c3..ed543c7df50 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/test_rest_endpoints.py +++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_rest_endpoints.py @@ -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."""