mirror of
https://github.com/BerriAI/litellm.git
synced 2026-10-06 02:48:13 +00:00
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
This commit is contained in:
Devin AI
parent
9c59e2ae55
commit
b5c47c24ba
3 changed files with 115 additions and 172 deletions
|
|
@ -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(
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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."""
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue