diff --git a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py index 0b81bd7aff7..53dc6e512c5 100644 --- a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py +++ b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py @@ -38,6 +38,7 @@ from litellm.proxy._experimental.mcp_server.auth.user_api_key_auth_mcp import ( MCPRequestHandler, ) from litellm.proxy._experimental.mcp_server.utils import ( + MCP_TOOL_PREFIX_SEPARATOR, add_server_prefix_to_name, get_server_prefix, is_tool_name_prefixed, @@ -61,6 +62,45 @@ from litellm.types.mcp_server.mcp_server_manager import ( MCPOAuthMetadata, MCPServer, ) +from mcp.shared.tool_name_validation import SEP_986_URL, validate_tool_name + + +# Probe includes characters on both sides of the separator to mimic real prefixed tool names. +_separator_probe_tool_name = f"litellm{MCP_TOOL_PREFIX_SEPARATOR}probe" +_separator_probe = validate_tool_name(_separator_probe_tool_name) +if not _separator_probe.is_valid: + verbose_logger.warning( + "MCP tool prefix separator '%s' violates SEP-986. See %s", + MCP_TOOL_PREFIX_SEPARATOR, + SEP_986_URL, + ) + + +def _warn_on_server_name_fields( + *, + server_id: str, + alias: Optional[str], + server_name: Optional[str], +): + def _warn(field_name: str, value: Optional[str]) -> None: + if not value: + return + result = validate_tool_name(value) + if result.is_valid: + return + + warning_text = "; ".join(result.warnings) if result.warnings else "Validation failed" + verbose_logger.warning( + "MCP server '%s' has invalid %s '%s': %s", + server_id, + field_name, + value, + warning_text, + ) + + _warn("alias", alias) + _warn("server_name", server_name) + def _deserialize_json_dict(data: Any) -> Optional[Dict[str, str]]: @@ -209,6 +249,12 @@ class MCPServerManager: alias=alias, ) + _warn_on_server_name_fields( + server_id=server_id, + alias=alias, + server_name=server_name, + ) + auth_type = server_config.get("auth_type", None) if server_url and auth_type is not None and auth_type == MCPAuth.oauth2: mcp_oauth_metadata = await self._descovery_metadata( @@ -2099,6 +2145,11 @@ class MCPServerManager: new_registry[server.server_id] = existing_server continue + _warn_on_server_name_fields( + server_id=server.server_id, + alias=getattr(server, "alias", None), + server_name=getattr(server, "server_name", None), + ) verbose_logger.debug( f"Building server from DB: {server.server_id} ({server.server_name})" ) diff --git a/litellm/proxy/_experimental/mcp_server/server.py b/litellm/proxy/_experimental/mcp_server/server.py index f22040a7dd9..76a28344856 100644 --- a/litellm/proxy/_experimental/mcp_server/server.py +++ b/litellm/proxy/_experimental/mcp_server/server.py @@ -786,7 +786,6 @@ if MCP_AVAILABLE: add_prefix=add_prefix, raw_headers=raw_headers, ) - filtered_tools = filter_tools_by_allowed_tools(tools, server) filtered_tools = await filter_tools_by_key_team_permissions( diff --git a/litellm/proxy/management_endpoints/mcp_management_endpoints.py b/litellm/proxy/management_endpoints/mcp_management_endpoints.py index d8816df010a..a15f47d13bd 100644 --- a/litellm/proxy/management_endpoints/mcp_management_endpoints.py +++ b/litellm/proxy/management_endpoints/mcp_management_endpoints.py @@ -37,7 +37,7 @@ from litellm._uuid import uuid from litellm.constants import LITELLM_PROXY_ADMIN_NAME from litellm.proxy._experimental.mcp_server.utils import ( get_server_prefix, - validate_and_normalize_mcp_server_payload, + validate_and_normalize_mcp_server_payload as _base_validate_and_normalize_mcp_server_payload, ) router = APIRouter(prefix="/v1/mcp", tags=["mcp"]) @@ -56,6 +56,7 @@ except ImportError as e: MCP_AVAILABLE = False if MCP_AVAILABLE: + from mcp.shared.tool_name_validation import validate_tool_name from litellm.proxy._experimental.mcp_server.db import ( create_mcp_server, delete_mcp_server, @@ -97,6 +98,43 @@ if MCP_AVAILABLE: server: MCPServer expires_at: datetime + def _validate_mcp_server_name_fields(payload: Any) -> None: + candidates: List[tuple[str, Optional[str]]] = [] + + server_name = getattr(payload, "server_name", None) + alias = getattr(payload, "alias", None) + + if server_name: + candidates.append(("server_name", server_name)) + if alias: + candidates.append(("alias", alias)) + + for field_name, value in candidates: + if not value: + continue + + validation_result = validate_tool_name(value) + if validation_result.is_valid: + continue + + error_messages_text = ( + f"Invalid MCP tool prefix '{value}' provided via {field_name}" + ) + if validation_result.warnings: + error_messages_text = ( + error_messages_text + + "\n" + + "\n".join(validation_result.warnings) + ) + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail={"error": error_messages_text}, + ) + + def validate_and_normalize_mcp_server_payload(payload: Any) -> None: + _base_validate_and_normalize_mcp_server_payload(payload) + _validate_mcp_server_name_fields(payload) + def _is_public_registry_enabled() -> bool: from litellm.proxy.proxy_server import ( general_settings as proxy_general_settings, 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 9ddfbf2059c..86abec31012 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 @@ -1,3 +1,6 @@ +import importlib +import logging +import os import sys from datetime import datetime from unittest.mock import AsyncMock, MagicMock, patch @@ -29,6 +32,15 @@ from litellm.types.mcp import MCPAuth from litellm.types.mcp_server.mcp_server_manager import MCPOAuthMetadata, MCPServer +def _reload_mcp_manager_module(): + utils_module = sys.modules["litellm.proxy._experimental.mcp_server.utils"] + manager_module = sys.modules[ + "litellm.proxy._experimental.mcp_server.mcp_server_manager" + ] + importlib.reload(utils_module) + return importlib.reload(manager_module) + + class TestMCPServerManager: """Test MCP Server Manager stdio functionality""" @@ -148,6 +160,90 @@ class TestMCPServerManager: # When the header isn't provided, the key is omitted entirely assert env == {} + @pytest.mark.asyncio + async def test_load_servers_from_config_warns_on_invalid_alias(self, caplog): + """Invalid aliases from config should emit warnings during load.""" + + manager = MCPServerManager() + config = { + "validserver": { + "alias": "bad/name", + "url": "https://example.com", + "transport": MCPTransport.http, + } + } + + with caplog.at_level(logging.WARNING, logger="LiteLLM"): + await manager.load_servers_from_config(config) + + assert any( + "invalid alias 'bad/name'" in message for message in caplog.messages + ) + + @pytest.mark.asyncio + async def test_load_servers_from_config_accepts_valid_alias(self, caplog): + """Valid aliases should be accepted and populate the registry.""" + + manager = MCPServerManager() + config = { + "validserver": { + "alias": "friendly_alias", + "url": "https://example.com", + "transport": MCPTransport.http, + } + } + + with caplog.at_level(logging.WARNING, logger="LiteLLM"): + await manager.load_servers_from_config(config) + + # No warnings logged for the valid alias + assert all("invalid alias" not in message for message in caplog.messages) + + server = next(iter(manager.config_mcp_servers.values())) + assert server.alias == "friendly_alias" + assert server.server_name == "validserver" + + def test_warns_when_custom_separator_invalid(self, monkeypatch, caplog): + """Invalid MCP_TOOL_PREFIX_SEPARATOR values should log a warning.""" + + original_value = os.environ.get("MCP_TOOL_PREFIX_SEPARATOR") + monkeypatch.setenv("MCP_TOOL_PREFIX_SEPARATOR", "/") + + with caplog.at_level(logging.WARNING, logger="LiteLLM"): + _reload_mcp_manager_module() + + assert any("violates SEP-986" in message for message in caplog.messages) + + # Restore original setting and ensure warning disappears + if original_value is None: + monkeypatch.delenv("MCP_TOOL_PREFIX_SEPARATOR", raising=False) + else: + monkeypatch.setenv("MCP_TOOL_PREFIX_SEPARATOR", original_value) + + caplog.clear() + with caplog.at_level(logging.WARNING, logger="LiteLLM"): + _reload_mcp_manager_module() + + assert all("violates SEP-986" not in message for message in caplog.messages) + + def test_accepts_valid_custom_separator(self, monkeypatch, caplog): + """Valid separators should not emit warnings during module import.""" + + original_value = os.environ.get("MCP_TOOL_PREFIX_SEPARATOR") + monkeypatch.setenv("MCP_TOOL_PREFIX_SEPARATOR", "_") + + with caplog.at_level(logging.WARNING, logger="LiteLLM"): + _reload_mcp_manager_module() + + assert all("violates SEP-986" not in message for message in caplog.messages) + + if original_value is None: + monkeypatch.delenv("MCP_TOOL_PREFIX_SEPARATOR", raising=False) + else: + monkeypatch.setenv("MCP_TOOL_PREFIX_SEPARATOR", original_value) + + _reload_mcp_manager_module() + @pytest.mark.asyncio async def test_list_tools_with_server_specific_auth_headers(self): """Test list_tools method with server-specific auth headers""" diff --git a/tests/test_litellm/proxy/management_endpoints/test_mcp_management_endpoints.py b/tests/test_litellm/proxy/management_endpoints/test_mcp_management_endpoints.py index bc223d15d5f..f7e7fcebaef 100644 --- a/tests/test_litellm/proxy/management_endpoints/test_mcp_management_endpoints.py +++ b/tests/test_litellm/proxy/management_endpoints/test_mcp_management_endpoints.py @@ -2,14 +2,18 @@ import json import os import sys import types +from types import SimpleNamespace from datetime import datetime, timedelta from typing import List, Optional from unittest.mock import AsyncMock, MagicMock, patch import pytest -from fastapi import FastAPI +from fastapi import FastAPI, HTTPException from fastapi.testclient import TestClient from litellm._uuid import uuid +from litellm.proxy.management_endpoints import ( + mcp_management_endpoints as mgmt_endpoints, +) sys.path.insert( 0, os.path.abspath("../../../..") @@ -726,8 +730,6 @@ class TestTemporaryMCPSessionEndpoints: "litellm.proxy.management_endpoints.mcp_management_endpoints.get_cached_temporary_mcp_server", return_value=None, ): - from fastapi import HTTPException - with pytest.raises(HTTPException) as exc_info: _get_cached_temporary_mcp_server_or_404("missing") @@ -1195,6 +1197,25 @@ class TestMCPRegistryEndpoint: assert result[0]["server_id"] == "server-1" assert result[0]["status"] == "healthy" + +class TestManagementPayloadValidation: + def test_rejects_invalid_alias(self): + payload = SimpleNamespace(server_name="valid_server", alias="bad/name") + + with pytest.raises(HTTPException) as exc_info: + mgmt_endpoints.validate_and_normalize_mcp_server_payload(payload) + + assert exc_info.value.status_code == 400 + error_message = exc_info.value.detail["error"] + assert "bad/name" in error_message + + def test_accepts_valid_names(self): + payload = SimpleNamespace(server_name="valid_server", alias=None) + + mgmt_endpoints.validate_and_normalize_mcp_server_payload(payload) + + assert payload.alias == "valid_server" + @pytest.mark.asyncio async def test_health_check_view_all_mode(self): """view_all mode should return health info for all MCP servers.""" diff --git a/ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.tsx b/ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.tsx index 82932a320e6..a3e27fb68fa 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.tsx @@ -425,7 +425,7 @@ const CreateMCPServer: React.FC = ({ label={ MCP Server Name - +