From b5c47c24ba3777ae3542056d0e977ab4d6e14cdb Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Wed, 15 Jul 2026 20:13:38 +0000 Subject: [PATCH] fix(mcp): derive config MCP server_id from server_name only Editing a config MCP server's url, transport, auth_type, or alias changed its derived server_id, silently orphaning every permission grant keyed on the old id. Hash only server_name (the config key and real identity) so the id stays stable across those edits, and allow an explicit server_id in config to pin the id for migrating existing grants. Fixes #33431 --- .../mcp_server/mcp_server_manager.py | 55 +++---- tests/mcp_tests/test_mcp_server.py | 152 ++---------------- .../mcp_server/test_mcp_server.py | 80 +++++++++ 3 files changed, 115 insertions(+), 172 deletions(-) diff --git a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py index e6e265abb61..ed90308444f 100644 --- a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py +++ b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py @@ -1010,14 +1010,15 @@ class MCPServerManager: name_for_prefix = get_server_prefix(temp_server) server_url = server_config.get("url", None) or "" - # Generate stable server ID based on parameters - server_id = self._generate_stable_server_id( - server_name=server_name, - url=server_url, - transport=server_config.get("transport", MCPTransport.http), - auth_type=server_config.get("auth_type", None), - alias=alias, - ) + explicit_server_id = server_config.get("server_id", None) + if explicit_server_id is not None and ( + not isinstance(explicit_server_id, str) or not explicit_server_id.strip() + ): + raise ValueError( + f"Invalid config for MCP server '{server_name}': server_id must be a " + f"non-empty string when set (got {explicit_server_id!r})." + ) + server_id = explicit_server_id or self._generate_stable_server_id(server_name=server_name) _warn_on_server_name_fields( server_id=server_id, @@ -4835,40 +4836,26 @@ class MCPServerManager: return registry return {k: v for k, v in registry.items() if self._is_server_accessible_from_ip(v, client_ip)} - def _generate_stable_server_id( - self, - server_name: str, - url: str, - transport: str, - auth_type: Optional[str] = None, - alias: Optional[str] = None, - ) -> str: + def _generate_stable_server_id(self, server_name: str) -> str: """ - Generate a stable server ID based on server parameters using a hash function. + Generate a stable server ID for a config-defined MCP server by hashing + only its ``server_name`` (the config key, which is the server's real + identity). - This is critical to ensure the server_id is stable across server restarts. - Some users store MCPs on the config.yaml and permission management is based on server_ids. - - Eg a key might have mcp_servers = ["1234"], if the server_id changes across restarts, the key will no longer have access to the MCP. + This is critical because permission management is based on server_ids. + A key might have mcp_servers = ["1234"]; if the server_id changes, the + key silently loses access to the MCP. The id must therefore stay stable + across restarts and across edits to mutable connection fields (url, + transport, auth_type, alias). Only renaming the config key, which is a + genuinely different server, changes the id. Args: - server_name: Name of the server - url: Server URL - transport: Transport type (sse, http, etc.) - auth_type: Authentication type (optional) - alias: Server alias (optional) + server_name: Name of the server (the config key) Returns: A deterministic server ID string """ - # Create a string from all the identifying parameters - params_string = f"{server_name}|{url}|{transport}|{auth_type or ''}|{alias or ''}" - - # Generate SHA-256 hash - hash_object = hashlib.sha256(params_string.encode("utf-8")) - hash_hex = hash_object.hexdigest() - - # Take first 32 characters and format as UUID-like string + hash_hex = hashlib.sha256(server_name.encode("utf-8")).hexdigest() return hash_hex[:32] async def health_check_server( diff --git a/tests/mcp_tests/test_mcp_server.py b/tests/mcp_tests/test_mcp_server.py index 515bf1233aa..66f94fe0f53 100644 --- a/tests/mcp_tests/test_mcp_server.py +++ b/tests/mcp_tests/test_mcp_server.py @@ -581,152 +581,28 @@ def test_generate_stable_server_id(): """ Test the _generate_stable_server_id method to ensure hash stability across releases. - This test verifies that: - 1. The same inputs always produce the same hash output - 2. Different inputs produce different hash outputs - 3. The hash format is consistent (32 character hex string) - 4. Edge cases work correctly (None auth_type) + The id is derived from server_name only. server_name is the config key and + the server's real identity; mutable connection fields (url, transport, + auth_type, alias) must not affect it, otherwise editing them silently + orphans every existing permission grant keyed on the old id (issue #33431). IMPORTANT: If this test fails, it means the hashing algorithm has changed and will break backwards compatibility with existing server IDs! """ manager = MCPServerManager() - # Test Case 1: Basic functionality with known inputs - # These expected values MUST remain stable across releases - test_cases = [ - { - "params": { - "server_name": "zapier_mcp_server", - "url": "https://actions.zapier.com/mcp/sse", - "transport": "sse", - "auth_type": "api_key", - }, - "expected_hash": "8d5c9f8a12e3b7c4f6a2d8e1b5c9f2a4", - }, - { - "params": { - "server_name": "google_drive_mcp_server", - "url": "https://drive.google.com/mcp/http", - "transport": "http", - "auth_type": None, - }, - "expected_hash": "7a4b2e8f3c1d9e6b5a7c8f2d4e1b9c6a", - }, - { - "params": { - "server_name": "local_test_server", - "url": "http://localhost:8080/mcp", - "transport": "http", - "auth_type": "basic", - }, - "expected_hash": "2f1e8d7c6b5a4e3f2d1c9b8a7e6f5d4c", - }, - ] + result = manager._generate_stable_server_id(server_name="zapier_mcp_server") + assert len(result) == 32, f"Hash should be 32 characters, got {len(result)}" + assert result.isalnum(), f"Hash should be alphanumeric, got: {result}" + assert result.islower(), f"Hash should be lowercase, got: {result}" - # Test that our known inputs produce expected hash values - for test_case in test_cases: - result = manager._generate_stable_server_id(**test_case["params"]) + # Deterministic: same server_name always produces the same id + assert result == manager._generate_stable_server_id(server_name="zapier_mcp_server") - # For now, just verify the format and stability, not exact hash - # (since we need to first run to see what the actual hashes are) - assert len(result) == 32, f"Hash should be 32 characters, got {len(result)}" - assert result.isalnum(), f"Hash should be alphanumeric, got: {result}" - assert result.islower(), f"Hash should be lowercase, got: {result}" - - # Test stability - same inputs should always produce same output - result2 = manager._generate_stable_server_id(**test_case["params"]) - assert ( - result == result2 - ), f"Hash should be stable for same inputs: {result} != {result2}" - - # Test Case 2: Different inputs produce different outputs - base_params = { - "server_name": "test_server", - "url": "https://test.com/mcp", - "transport": "sse", - "auth_type": "api_key", - } - - base_hash = manager._generate_stable_server_id(**base_params) - - # Change each parameter and verify hash changes - variations = [ - {"server_name": "different_server"}, - {"url": "https://different.com/mcp"}, - {"transport": "http"}, - {"auth_type": "basic"}, - {"auth_type": None}, - ] - - for variation in variations: - modified_params = {**base_params, **variation} - modified_hash = manager._generate_stable_server_id(**modified_params) - assert ( - modified_hash != base_hash - ), f"Different params should produce different hash: {variation}" - assert ( - len(modified_hash) == 32 - ), f"Modified hash should be 32 characters: {variation}" - - # Test Case 3: Edge case with None auth_type - params_with_none = { - "server_name": "test_server", - "url": "https://test.com/mcp", - "transport": "sse", - "auth_type": None, - } - - params_with_empty = { - "server_name": "test_server", - "url": "https://test.com/mcp", - "transport": "sse", - "auth_type": "", - } - - hash_none = manager._generate_stable_server_id(**params_with_none) - hash_empty = manager._generate_stable_server_id(**params_with_empty) - - # None and empty string should produce the same hash (both become empty string) - assert ( - hash_none == hash_empty - ), "None auth_type should be equivalent to empty string" - - # Test Case 4: Real-world example hashes that must remain stable - # These are based on common configurations and MUST NOT CHANGE - zapier_sse_hash = manager._generate_stable_server_id( - server_name="zapier_mcp_server", - url="https://actions.zapier.com/mcp/sk-ak-example/sse", - transport="sse", - auth_type="api_key", - ) - - github_http_hash = manager._generate_stable_server_id( - server_name="github_mcp_server", - url="https://api.github.com/mcp/http", - transport="http", - auth_type=None, - ) - - # These should be deterministic - same call should produce same result - assert zapier_sse_hash == manager._generate_stable_server_id( - server_name="zapier_mcp_server", - url="https://actions.zapier.com/mcp/sk-ak-example/sse", - transport="sse", - auth_type="api_key", - ) - - assert github_http_hash == manager._generate_stable_server_id( - server_name="github_mcp_server", - url="https://api.github.com/mcp/http", - transport="http", - auth_type=None, - ) - - # Verify format - assert len(zapier_sse_hash) == 32 - assert len(github_http_hash) == 32 - assert zapier_sse_hash != github_http_hash + # Different server_name produces a different id + other = manager._generate_stable_server_id(server_name="github_mcp_server") + assert other != result + assert len(other) == 32 @pytest.mark.asyncio diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server.py index a983ac3ff48..df45de03b57 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server.py +++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server.py @@ -4946,6 +4946,86 @@ class TestEnsureUpstreamInitializeInstructionsCached: global_mcp_server_manager._upstream_initialize_instructions_probed_at.pop("reload-target", None) +class TestConfigServerIdStability: + """Regression tests for issue #33431 - editing a config MCP server's mutable + connection fields must not change its server_id (which would silently orphan + every existing permission grant keyed on the old id).""" + + @pytest.mark.asyncio + async def test_server_id_stable_across_auth_type_edit(self): + from litellm.proxy._experimental.mcp_server.mcp_server_manager import ( + MCPServerManager, + ) + + before = MCPServerManager() + await before.load_servers_from_config( + {"internal_docs": {"url": "http://localhost:3000/mcp", "transport": "http"}} + ) + id_before = before.get_mcp_server_by_name("internal_docs").server_id + + after = MCPServerManager() + await after.load_servers_from_config( + { + "internal_docs": { + "url": "https://docs.internal/mcp", + "transport": "sse", + "auth_type": "bearer_token", + "alias": "docs", + } + } + ) + id_after = after.get_mcp_server_by_name("internal_docs").server_id + + assert id_before == id_after + + @pytest.mark.asyncio + async def test_server_id_changes_with_server_name(self): + from litellm.proxy._experimental.mcp_server.mcp_server_manager import ( + MCPServerManager, + ) + + manager = MCPServerManager() + await manager.load_servers_from_config( + { + "server_a": {"url": "http://localhost/mcp", "transport": "http"}, + "server_b": {"url": "http://localhost/mcp", "transport": "http"}, + } + ) + id_a = manager.get_mcp_server_by_name("server_a").server_id + id_b = manager.get_mcp_server_by_name("server_b").server_id + assert id_a != id_b + + @pytest.mark.asyncio + async def test_explicit_server_id_pins_the_id(self): + from litellm.proxy._experimental.mcp_server.mcp_server_manager import ( + MCPServerManager, + ) + + manager = MCPServerManager() + await manager.load_servers_from_config( + { + "legacy": { + "server_id": "pinned-legacy-id", + "url": "http://localhost/mcp", + "transport": "http", + } + } + ) + assert manager.get_mcp_server_by_name("legacy").server_id == "pinned-legacy-id" + + @pytest.mark.asyncio + async def test_blank_explicit_server_id_rejected(self): + from litellm.proxy._experimental.mcp_server.mcp_server_manager import ( + MCPServerManager, + ) + + manager = MCPServerManager() + with pytest.raises(ValueError, match="server_id must be a non-empty string"): + await manager.load_servers_from_config( + {"legacy": {"server_id": " ", "url": "http://localhost/mcp", "transport": "http"}} + ) + + class TestGatewayCreateInitializationOptions: """Tests for the patched server.create_initialization_options via ContextVar."""