mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-26 01:12:21 +00:00
fix(mcp): reject duplicate MCP server names and aliases (#42791)
* fix(mcp): reject duplicate MCP server names and aliases MCP server_name and alias were unchecked at write time, so two servers could share one tool prefix and tool routing resolved to an arbitrary winner. Writes now run inside an advisory-locked transaction that rejects a collision on either column case-insensitively with a 400 naming the colliding identifier, covering create, edit, connector import and restricted-admin submission. Server reload logs one warning per identifier already shared in the database. Co-Authored-By: bot_apk <apk@cognition.ai> * fix(ui): block duplicate MCP server names and aliases before submit The create and edit forms now check the normalized name/alias against the loaded server list (case-insensitive, spaces to underscores, own row excluded on edit) and show a field error instead of submitting. Structured proxy error bodies are unwrapped so a 400 no longer renders as 'Error: [object Object]'. Co-Authored-By: bot_apk <apk@cognition.ai> * fix(mcp): check identifier conflicts when an alias is cleared Clearing an alias drops the tool prefix to the stored server_name, so that name must go through the conflict check too; an explicit alias:null is now treated as an identifier write. Also narrows the new db tests to behavioral assertions instead of pinning prisma where shapes. Co-Authored-By: bot_apk <apk@cognition.ai> * fix(mcp): treat an empty alias as a clear in conflict checks An empty-string alias was written unchecked even though the prefix falls back to server_name; the update path now treats any falsy alias like a clear. The edit form likewise compares a cleared alias as empty instead of re-checking the alias being removed. Co-Authored-By: bot_apk <apk@cognition.ai> * test(mcp): cover clearing an alias to an empty string Co-Authored-By: bot_apk <apk@cognition.ai> --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: bot_apk <apk@cognition.ai>
This commit is contained in:
devin-ai-integration[bot]
GitHub
parent
1175559c39
commit
ce582affaa
14 changed files with 938 additions and 29 deletions
|
|
@ -3,6 +3,7 @@ import binascii
|
|||
import hashlib
|
||||
import json
|
||||
from collections.abc import Awaitable, Callable, Iterable, Mapping, Sequence
|
||||
from dataclasses import dataclass
|
||||
from datetime import datetime, timedelta, timezone
|
||||
from typing import TYPE_CHECKING, Any, Final, Literal, Protocol, TypedDict, cast
|
||||
|
||||
|
|
@ -64,6 +65,7 @@ if TYPE_CHECKING:
|
|||
|
||||
class _UserEnvVarsTransactionClient(Protocol):
|
||||
litellm_mcpuserenvvars: "TableActions[prisma_db_models.LiteLLM_MCPUserEnvVars]"
|
||||
litellm_mcpservertable: "TableActions[prisma_db_models.LiteLLM_MCPServerTable]"
|
||||
|
||||
async def execute_raw(self, query: str, *args: object) -> int: ...
|
||||
|
||||
|
|
@ -74,6 +76,19 @@ class _UserEnvVarsTransaction(Protocol):
|
|||
async def __aexit__(self, exc_type: object, exc_value: object, traceback: object) -> bool | None: ...
|
||||
|
||||
|
||||
@dataclass(frozen=True, slots=True)
|
||||
class McpIdentifierConflict:
|
||||
"""An incoming ``server_name``/``alias`` already belongs to another MCP server row.
|
||||
|
||||
``field`` is the incoming identifier that collided, ``value`` the submitted
|
||||
string, and ``server_id`` the existing row that owns it.
|
||||
"""
|
||||
|
||||
field: Literal["server_name", "alias"]
|
||||
value: str
|
||||
server_id: str
|
||||
|
||||
|
||||
_AUTH_FLOW_SCOPED_FIELDS: Final["frozenset[str]"] = frozenset(
|
||||
{
|
||||
"issuer",
|
||||
|
|
@ -500,6 +515,121 @@ def _db_transaction_manager(prisma_client: PrismaClient) -> _UserEnvVarsTransact
|
|||
return manager
|
||||
|
||||
|
||||
def _identifier_where(value: str, exclude_server_id: str | None) -> "prisma_db_types.LiteLLM_MCPServerTableWhereInput":
|
||||
own_row_guard: Final = (
|
||||
({"NOT": [{"server_id": exclude_server_id}]},) # mutable-ok: prisma where-inputs must be plain dicts
|
||||
if exclude_server_id is not None
|
||||
else ()
|
||||
)
|
||||
where: Final[prisma_db_types.LiteLLM_MCPServerTableWhereInput] = {
|
||||
"AND": [ # mutable-ok: prisma where-inputs must be plain dicts
|
||||
{
|
||||
"OR": [ # mutable-ok: prisma where-inputs must be plain dicts
|
||||
{"server_name": {"equals": value, "mode": "insensitive"}},
|
||||
{"alias": {"equals": value, "mode": "insensitive"}},
|
||||
]
|
||||
},
|
||||
{
|
||||
"OR": [{"approval_status": None}, {"approval_status": {"not": MCPApprovalStatus.draft}}]
|
||||
}, # mutable-ok: prisma where-inputs must be plain dicts
|
||||
*own_row_guard,
|
||||
]
|
||||
}
|
||||
return where
|
||||
|
||||
|
||||
def _identifier_field(data_dict: "Mapping[str, object]", field: str) -> str | None:
|
||||
value: Final = data_dict.get(field)
|
||||
return value if isinstance(value, str) else None
|
||||
|
||||
|
||||
async def _find_mcp_server_identifier_conflict(
|
||||
table: "TableActions[prisma_db_models.LiteLLM_MCPServerTable]",
|
||||
*,
|
||||
server_name: str | None,
|
||||
alias: str | None,
|
||||
exclude_server_id: str | None,
|
||||
) -> McpIdentifierConflict | None:
|
||||
"""Return the collision between an incoming identifier and a stored row, else None.
|
||||
|
||||
Each non-empty incoming identifier is compared case-insensitively against
|
||||
BOTH the ``server_name`` and ``alias`` columns, because a value that matches
|
||||
either column would still share the tool prefix another server answers to.
|
||||
``alias`` is checked first so the reported field is deterministic. Draft
|
||||
rows back the transient OAuth session flow and never reach the registry, so
|
||||
they cannot collide. NULL ``approval_status`` predates the approval
|
||||
workflow and is kept via the inner OR, matching ``get_all_mcp_servers``.
|
||||
"""
|
||||
candidates: Final[tuple[tuple[Literal["alias", "server_name"], str | None], ...]] = (
|
||||
("alias", alias),
|
||||
("server_name", server_name),
|
||||
)
|
||||
for field_name, value in candidates:
|
||||
if not value:
|
||||
continue
|
||||
if (row := await table.find_first(where=_identifier_where(value, exclude_server_id))) is not None:
|
||||
return McpIdentifierConflict(field=field_name, value=value, server_id=row.server_id)
|
||||
return None
|
||||
|
||||
|
||||
async def find_mcp_server_identifier_conflict(
|
||||
prisma_client: PrismaClient,
|
||||
*,
|
||||
server_name: str | None,
|
||||
alias: str | None,
|
||||
exclude_server_id: str | None,
|
||||
) -> McpIdentifierConflict | None:
|
||||
"""Unlocked identifier-collision check, for callers outside a write path."""
|
||||
return await _find_mcp_server_identifier_conflict(
|
||||
_mcp_server_table_actions(prisma_client),
|
||||
server_name=server_name,
|
||||
alias=alias,
|
||||
exclude_server_id=exclude_server_id,
|
||||
)
|
||||
|
||||
|
||||
def _mcp_identifier_lock_keys(*identifiers: str | None) -> tuple[int, ...]:
|
||||
"""Deterministic advisory-lock keys for the lowercased identifiers, sorted
|
||||
so concurrent requests for the same pair always lock in the same order."""
|
||||
return tuple(
|
||||
int.from_bytes(
|
||||
hashlib.blake2b(f"mcp_identifier:{normalized}".encode(), digest_size=8).digest(),
|
||||
"big",
|
||||
signed=True,
|
||||
)
|
||||
for normalized in sorted(frozenset(value.lower() for value in identifiers if value))
|
||||
)
|
||||
|
||||
|
||||
async def _mcp_server_write_if_identifier_free(
|
||||
prisma_client: PrismaClient,
|
||||
*,
|
||||
server_name: str | None,
|
||||
alias: str | None,
|
||||
exclude_server_id: str | None,
|
||||
write: "Callable[[TableActions[prisma_db_models.LiteLLM_MCPServerTable]], Awaitable[prisma_db_models.LiteLLM_MCPServerTable | None]]",
|
||||
) -> "prisma_db_models.LiteLLM_MCPServerTable | McpIdentifierConflict | None":
|
||||
"""Run ``write`` only when no other live row owns ``server_name``/``alias``.
|
||||
|
||||
The conflict check and the write share a transaction guarded by per-identifier
|
||||
advisory locks, so two concurrent requests for the same name cannot both
|
||||
pass the check and both insert.
|
||||
"""
|
||||
lock_keys: Final = _mcp_identifier_lock_keys(server_name, alias)
|
||||
async with _db_transaction_manager(prisma_client) as tx:
|
||||
for lock_key in lock_keys:
|
||||
await tx.execute_raw("SELECT pg_advisory_xact_lock($1::bigint)", lock_key)
|
||||
conflict: Final = await _find_mcp_server_identifier_conflict(
|
||||
tx.litellm_mcpservertable,
|
||||
server_name=server_name,
|
||||
alias=alias,
|
||||
exclude_server_id=exclude_server_id,
|
||||
)
|
||||
if conflict is not None:
|
||||
return conflict
|
||||
return await write(tx.litellm_mcpservertable)
|
||||
|
||||
|
||||
async def _db_find_mcp_server_rows(
|
||||
prisma_client: PrismaClient,
|
||||
where: "prisma_db_types.LiteLLM_MCPServerTableWhereInput | None" = None,
|
||||
|
|
@ -880,6 +1010,43 @@ async def create_mcp_server(
|
|||
return LiteLLM_MCPServerTable.model_validate(new_mcp_server.model_dump())
|
||||
|
||||
|
||||
async def create_mcp_server_if_identifier_free(
|
||||
prisma_client: PrismaClient, data: NewMCPServerRequest, touched_by: str
|
||||
) -> LiteLLM_MCPServerTable | McpIdentifierConflict:
|
||||
"""Create the row only when no other live server owns ``server_name``/``alias``.
|
||||
|
||||
Returns the McpIdentifierConflict instead of inserting when the collision
|
||||
check finds an existing row; the advisory-lock transaction keeps two
|
||||
concurrent creates of the same identifier from both passing.
|
||||
"""
|
||||
if data.server_id is None:
|
||||
data.server_id = str(uuid.uuid4())
|
||||
|
||||
data_dict: Final = _prepare_mcp_server_data(data)
|
||||
data_dict["created_by"] = touched_by
|
||||
data_dict["updated_by"] = touched_by
|
||||
|
||||
async def _create(
|
||||
table: "TableActions[prisma_db_models.LiteLLM_MCPServerTable]",
|
||||
) -> "prisma_db_models.LiteLLM_MCPServerTable | None":
|
||||
return await table.create(data=data_dict)
|
||||
|
||||
written: Final = await _mcp_server_write_if_identifier_free(
|
||||
prisma_client,
|
||||
server_name=_identifier_field(data_dict, "server_name"),
|
||||
alias=_identifier_field(data_dict, "alias"),
|
||||
exclude_server_id=None,
|
||||
write=_create,
|
||||
)
|
||||
if isinstance(written, McpIdentifierConflict):
|
||||
return written
|
||||
if written is None:
|
||||
raise RuntimeError("inserted MCP server row missing")
|
||||
|
||||
_decrypt_env_vars_on_returned_row(written)
|
||||
return LiteLLM_MCPServerTable.model_validate(written.model_dump())
|
||||
|
||||
|
||||
async def create_draft_mcp_server(
|
||||
prisma_client: PrismaClient,
|
||||
data: NewMCPServerRequest,
|
||||
|
|
@ -970,14 +1137,57 @@ async def get_draft_mcp_server(
|
|||
return table
|
||||
|
||||
|
||||
async def _update_mcp_server_row(
|
||||
prisma_client: PrismaClient,
|
||||
*,
|
||||
server_id: str,
|
||||
data_dict: Mapping[str, object],
|
||||
) -> "prisma_db_models.LiteLLM_MCPServerTable | McpIdentifierConflict | None":
|
||||
identifier_write: Final = any(field in data_dict for field in ("server_name", "alias"))
|
||||
|
||||
async def _update(
|
||||
table: "TableActions[prisma_db_models.LiteLLM_MCPServerTable]",
|
||||
) -> "prisma_db_models.LiteLLM_MCPServerTable | None":
|
||||
return await table.update(
|
||||
where={"server_id": server_id}, # mutable-ok: prisma where-inputs must be plain dicts
|
||||
data=data_dict,
|
||||
)
|
||||
|
||||
if not identifier_write:
|
||||
return await _update(_mcp_server_table_actions(prisma_client))
|
||||
if "alias" in data_dict and not data_dict["alias"] and "server_name" not in data_dict:
|
||||
# Clearing the alias drops the prefix to the stored server_name, which
|
||||
# may already belong to another row, so that name needs the check too.
|
||||
existing: Final = await _db_find_mcp_server_row(prisma_client, server_id)
|
||||
if existing is None:
|
||||
return await _update(_mcp_server_table_actions(prisma_client))
|
||||
return await _mcp_server_write_if_identifier_free(
|
||||
prisma_client,
|
||||
server_name=existing.server_name,
|
||||
alias=None,
|
||||
exclude_server_id=server_id,
|
||||
write=_update,
|
||||
)
|
||||
return await _mcp_server_write_if_identifier_free(
|
||||
prisma_client,
|
||||
server_name=_identifier_field(data_dict, "server_name"),
|
||||
alias=_identifier_field(data_dict, "alias"),
|
||||
exclude_server_id=server_id,
|
||||
write=_update,
|
||||
)
|
||||
|
||||
|
||||
async def update_mcp_server(
|
||||
prisma_client: PrismaClient,
|
||||
data: UpdateMCPServerRequest,
|
||||
touched_by: str,
|
||||
fields_set: set[str] | None = None,
|
||||
) -> LiteLLM_MCPServerTable | None:
|
||||
) -> LiteLLM_MCPServerTable | McpIdentifierConflict | None:
|
||||
"""
|
||||
Update a new mcp server record in the db
|
||||
|
||||
Returns McpIdentifierConflict instead of writing when the update would put
|
||||
``server_name``/``alias`` onto identifiers another live row already owns.
|
||||
"""
|
||||
from litellm.litellm_core_utils.safe_json_dumps import safe_dumps
|
||||
|
||||
|
|
@ -1086,11 +1296,14 @@ async def update_mcp_server(
|
|||
|
||||
data_dict["credentials"] = Json(None)
|
||||
|
||||
updated_mcp_server: Final = await MCPServerRepository(prisma_client).table.update(
|
||||
where={"server_id": data.server_id},
|
||||
data=data_dict,
|
||||
updated_mcp_server: Final = await _update_mcp_server_row(
|
||||
prisma_client,
|
||||
server_id=data.server_id,
|
||||
data_dict=data_dict,
|
||||
)
|
||||
|
||||
if isinstance(updated_mcp_server, McpIdentifierConflict):
|
||||
return updated_mcp_server
|
||||
_decrypt_env_vars_on_returned_row(updated_mcp_server)
|
||||
return LiteLLM_MCPServerTable.model_validate(updated_mcp_server.model_dump()) if updated_mcp_server else None
|
||||
|
||||
|
|
|
|||
|
|
@ -1570,6 +1570,7 @@ async def _persist_dcr_client_registration(
|
|||
}
|
||||
|
||||
from litellm.proxy._experimental.mcp_server.db import ( # noqa: PLC0415 # avoids circular import
|
||||
McpIdentifierConflict,
|
||||
update_mcp_server,
|
||||
upsert_mcp_server_oauth_client_credentials,
|
||||
)
|
||||
|
|
@ -1601,7 +1602,7 @@ async def _persist_dcr_client_registration(
|
|||
),
|
||||
touched_by="mcp_oauth_dcr",
|
||||
)
|
||||
if updated_row is not None:
|
||||
if updated_row is not None and not isinstance(updated_row, McpIdentifierConflict):
|
||||
await global_mcp_server_manager.update_server(updated_row)
|
||||
return "persisted"
|
||||
if global_mcp_server_manager.is_config_declared_server(mcp_server.server_id):
|
||||
|
|
|
|||
|
|
@ -1453,6 +1453,35 @@ def _warn_on_server_name_fields(
|
|||
_warn("server_name", server_name)
|
||||
|
||||
|
||||
def _warn_on_shared_identifier_prefixes(servers: Iterable[MCPServer]) -> None:
|
||||
"""Warn once per identifier that several servers share.
|
||||
|
||||
``get_server_prefix`` resolves alias first, so two servers sharing a
|
||||
lowercased ``alias or server_name`` publish the same tool prefix and calls
|
||||
routed by that prefix are ambiguous. A write-time uniqueness check keeps
|
||||
new collisions out; this surfaces the ones already stored.
|
||||
"""
|
||||
pairs: Final = tuple(
|
||||
((server.alias or server.server_name or "").lower(), server.server_id)
|
||||
for server in servers
|
||||
if server.alias or server.server_name
|
||||
)
|
||||
groups: Final = MappingProxyType(
|
||||
{
|
||||
identifier: tuple(sorted(server_id for key, server_id in pairs if key == identifier))
|
||||
for identifier in frozenset(key for key, _server_id in pairs)
|
||||
}
|
||||
)
|
||||
for identifier, server_ids in groups.items():
|
||||
if len(server_ids) > 1:
|
||||
verbose_logger.warning(
|
||||
"MCP servers %s share the identifier '%s'; tool routing for that prefix is ambiguous. "
|
||||
"Rename or delete all but one.",
|
||||
sorted(server_ids),
|
||||
identifier,
|
||||
)
|
||||
|
||||
|
||||
def _warn_legacy_delegate_auth_if_applicable(server: MCPServer, *, source: str) -> None:
|
||||
"""Direct legacy delegated OAuth configurations to the admitted replacement."""
|
||||
if server.auth_type != MCPAuth.oauth2:
|
||||
|
|
@ -6613,6 +6642,7 @@ class MCPServerManager:
|
|||
if previous_registry.get(server_id) != registered_registry.get(server_id):
|
||||
self._invalidate_discovery_lists(server_id)
|
||||
self.registry = registered_registry
|
||||
_warn_on_shared_identifier_prefixes(registered_registry.values())
|
||||
# A discovery task may have published into ``previous_registry`` while
|
||||
# this replacement was being staged. Reconcile every published entry
|
||||
# synchronously after the swap so a lost publication cannot also leave
|
||||
|
|
|
|||
|
|
@ -27,6 +27,7 @@ from typing import (
|
|||
Annotated,
|
||||
Final,
|
||||
Literal,
|
||||
NoReturn,
|
||||
Protocol,
|
||||
cast, # noqa: TID251 # validated JSON values need explicit narrowing
|
||||
)
|
||||
|
|
@ -137,9 +138,10 @@ if MCP_AVAILABLE:
|
|||
return _ToolNameValidationResult()
|
||||
|
||||
from litellm.proxy._experimental.mcp_server.db import (
|
||||
McpIdentifierConflict,
|
||||
approve_mcp_server,
|
||||
create_draft_mcp_server,
|
||||
create_mcp_server,
|
||||
create_mcp_server_if_identifier_free,
|
||||
delete_mcp_server,
|
||||
delete_user_credential,
|
||||
delete_user_env_vars,
|
||||
|
|
@ -288,6 +290,21 @@ if MCP_AVAILABLE:
|
|||
_validate_mcp_server_name_fields(payload)
|
||||
_validate_upstream_token_header(payload)
|
||||
|
||||
def mcp_identifier_conflict_message(conflict: McpIdentifierConflict) -> str:
|
||||
return (
|
||||
f"An MCP server with {conflict.field} '{conflict.value}' already exists "
|
||||
f"(server_id={conflict.server_id}). "
|
||||
"MCP server names and aliases must be unique, case-insensitive."
|
||||
)
|
||||
|
||||
def raise_mcp_identifier_conflict(conflict: McpIdentifierConflict) -> NoReturn:
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_400_BAD_REQUEST,
|
||||
detail={ # mutable-ok: FastAPI HTTPException detail requires a plain dict
|
||||
"error": mcp_identifier_conflict_message(conflict)
|
||||
},
|
||||
)
|
||||
|
||||
def warn_if_id_jag_server_outruns_sso(server_id: str | None, auth_type: MCPAuth | str | None) -> None:
|
||||
"""Registering an ``oauth2_id_jag`` server under an SSO provider that captures no IdP
|
||||
identity assertion is a dead configuration: nothing here fails, and then every ID-JAG call
|
||||
|
|
@ -1388,7 +1405,7 @@ if MCP_AVAILABLE:
|
|||
payload.submitted_at = datetime.now(timezone.utc)
|
||||
|
||||
try:
|
||||
new_mcp_server: Final = await create_mcp_server(
|
||||
new_mcp_server: Final = await create_mcp_server_if_identifier_free(
|
||||
prisma_client,
|
||||
payload,
|
||||
touched_by=user_api_key_dict.user_id or user_api_key_dict.team_id,
|
||||
|
|
@ -1399,6 +1416,8 @@ if MCP_AVAILABLE:
|
|||
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
|
||||
detail={"error": f"Error registering mcp server: {e}"},
|
||||
)
|
||||
if isinstance(new_mcp_server, McpIdentifierConflict):
|
||||
raise_mcp_identifier_conflict(new_mcp_server)
|
||||
# Do NOT add to runtime registry — pending servers are not active
|
||||
return _redact_mcp_credentials(new_mcp_server)
|
||||
|
||||
|
|
@ -1749,7 +1768,7 @@ if MCP_AVAILABLE:
|
|||
# The database write is the commit point: if it fails nothing was
|
||||
# persisted and the request is a genuine failure.
|
||||
try:
|
||||
new_mcp_server: Final = await create_mcp_server(
|
||||
new_mcp_server: Final = await create_mcp_server_if_identifier_free(
|
||||
prisma_client,
|
||||
payload,
|
||||
touched_by=user_api_key_dict.user_id or LITELLM_PROXY_ADMIN_NAME,
|
||||
|
|
@ -1760,6 +1779,8 @@ if MCP_AVAILABLE:
|
|||
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
|
||||
detail={"error": f"Error creating mcp server: {e}"},
|
||||
)
|
||||
if isinstance(new_mcp_server, McpIdentifierConflict):
|
||||
raise_mcp_identifier_conflict(new_mcp_server)
|
||||
|
||||
warn_if_id_jag_server_outruns_sso(new_mcp_server.server_id, new_mcp_server.auth_type)
|
||||
|
||||
|
|
@ -1808,7 +1829,7 @@ if MCP_AVAILABLE:
|
|||
conversions: Final = convert_connector_entries(payload)
|
||||
existing_servers: Final = await get_all_mcp_servers(prisma_client)
|
||||
existing_names: Final = frozenset(
|
||||
name for server in existing_servers for name in (server.alias, server.server_name) if name
|
||||
name.lower() for server in existing_servers for name in (server.alias, server.server_name) if name
|
||||
)
|
||||
|
||||
def _classify(
|
||||
|
|
@ -1817,16 +1838,16 @@ if MCP_AVAILABLE:
|
|||
if isinstance(conversion, ConnectorConversionError):
|
||||
return conversion
|
||||
alias: Final = conversion.request.alias or ""
|
||||
if alias in existing_names:
|
||||
if alias.lower() in existing_names:
|
||||
return MCPConnectorImportSkipped(
|
||||
name=conversion.name, reason=f"An MCP server named '{alias}' already exists."
|
||||
)
|
||||
earlier_aliases: Final = frozenset(
|
||||
earlier.request.alias or ""
|
||||
(earlier.request.alias or "").lower()
|
||||
for earlier in conversions[:index]
|
||||
if isinstance(earlier, ConvertedConnector)
|
||||
)
|
||||
if alias in earlier_aliases:
|
||||
if alias.lower() in earlier_aliases:
|
||||
return MCPConnectorImportSkipped(
|
||||
name=conversion.name, reason=f"Duplicate connector name '{alias}' in the import payload."
|
||||
)
|
||||
|
|
@ -1834,7 +1855,7 @@ if MCP_AVAILABLE:
|
|||
|
||||
async def _create(
|
||||
conversion: ConvertedConnector,
|
||||
) -> MCPConnectorImportResult | MCPConnectorImportFailure:
|
||||
) -> MCPConnectorImportResult | MCPConnectorImportFailure | MCPConnectorImportSkipped:
|
||||
try:
|
||||
validate_and_normalize_mcp_server_payload(conversion.request)
|
||||
except HTTPException as e:
|
||||
|
|
@ -1843,7 +1864,7 @@ if MCP_AVAILABLE:
|
|||
)
|
||||
return MCPConnectorImportFailure(name=conversion.name, error=error_text)
|
||||
try:
|
||||
created: Final = await create_mcp_server(
|
||||
created: Final = await create_mcp_server_if_identifier_free(
|
||||
prisma_client,
|
||||
conversion.request,
|
||||
touched_by=user_api_key_dict.user_id or LITELLM_PROXY_ADMIN_NAME,
|
||||
|
|
@ -1851,6 +1872,8 @@ if MCP_AVAILABLE:
|
|||
except Exception as e: # noqa: BLE001 # any create failure must become a per-entry error, not a 500
|
||||
verbose_proxy_logger.exception("Error importing mcp server %s: %s", conversion.name, e)
|
||||
return MCPConnectorImportFailure(name=conversion.name, error=str(e))
|
||||
if isinstance(created, McpIdentifierConflict):
|
||||
return MCPConnectorImportSkipped(name=conversion.name, reason=mcp_identifier_conflict_message(created))
|
||||
try:
|
||||
await global_mcp_server_manager.add_server(created)
|
||||
except Exception as e: # noqa: BLE001 # the row is committed; the reload after the loop retries registration
|
||||
|
|
@ -2927,6 +2950,9 @@ if MCP_AVAILABLE:
|
|||
fields_set=payload_fields_set,
|
||||
)
|
||||
|
||||
if isinstance(mcp_server_record_updated, McpIdentifierConflict):
|
||||
raise_mcp_identifier_conflict(mcp_server_record_updated)
|
||||
|
||||
if mcp_server_record_updated is None:
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_404_NOT_FOUND,
|
||||
|
|
|
|||
|
|
@ -2,7 +2,6 @@ import uuid
|
|||
from pathlib import Path
|
||||
from typing import Final
|
||||
|
||||
import pytest
|
||||
import yaml
|
||||
from integration._support.client import Gateway, eventually
|
||||
from integration._support.mcp import (
|
||||
|
|
@ -118,16 +117,67 @@ def test_delete_removes_listing_calls_and_database_row(gateway: Gateway) -> None
|
|||
|
||||
|
||||
def test_duplicate_alias_is_rejected_so_tool_prefixes_cannot_collide(gateway: Gateway) -> None:
|
||||
import concurrent.futures
|
||||
|
||||
with mcp_peer() as peer, gateway.scenario() as scenario:
|
||||
alias: Final = "mgmt" + uuid.uuid4().hex[:8]
|
||||
register_mcp(scenario, peer, alias)
|
||||
identity: Final = register_mcp(scenario, peer, alias)
|
||||
key: Final = scenario.key(object_permission={"mcp_servers": [identity]})
|
||||
|
||||
duplicate: Final = gateway.request(
|
||||
"POST", "/v1/mcp/server", {"server_name": alias, "alias": alias, **peer.registration()}
|
||||
)
|
||||
if duplicate.status_code == 201:
|
||||
scenario.cleanups.callback(forget_mcp, gateway, duplicate.json()["server_id"])
|
||||
pytest.skip("BUG: POST /v1/mcp/server accepts a duplicate alias, so two servers share one tool prefix")
|
||||
assert duplicate.status_code == 400, duplicate.text
|
||||
assert alias in duplicate.json()["detail"]["error"], duplicate.text
|
||||
|
||||
same_alias: Final = gateway.request(
|
||||
"POST", "/v1/mcp/server", {"server_name": alias + "other", "alias": alias, **peer.registration()}
|
||||
)
|
||||
assert same_alias.status_code == 400, same_alias.text
|
||||
assert alias in same_alias.json()["detail"]["error"], same_alias.text
|
||||
|
||||
case_variant: Final = gateway.request(
|
||||
"POST", "/v1/mcp/server", {"server_name": alias.upper(), "alias": alias.upper(), **peer.registration()}
|
||||
)
|
||||
assert case_variant.status_code == 400, case_variant.text
|
||||
|
||||
same_name_no_alias: Final = gateway.request(
|
||||
"POST", "/v1/mcp/server", {"server_name": alias, **peer.registration()}
|
||||
)
|
||||
assert same_name_no_alias.status_code == 400, same_name_no_alias.text
|
||||
|
||||
second_alias: Final = alias + "2"
|
||||
second_identity: Final = register_mcp(scenario, peer, second_alias)
|
||||
colliding_rename: Final = gateway.request(
|
||||
"PUT", "/v1/mcp/server", {"server_id": second_identity, "alias": alias}
|
||||
)
|
||||
assert colliding_rename.status_code == 400, colliding_rename.text
|
||||
|
||||
cleared_alias: Final = gateway.request("PUT", "/v1/mcp/server", {"server_id": second_identity, "alias": None})
|
||||
assert cleared_alias.status_code == 202, cleared_alias.text
|
||||
|
||||
name: Final = tool_names(gateway, key, identity)["add"]
|
||||
response: Final = call_tool(gateway, key, identity, name, ADD)
|
||||
assert response.status_code == 200, response.text
|
||||
assert response.json()["content"][0]["text"] == "9", response.text
|
||||
|
||||
racing_alias: Final = "race" + uuid.uuid4().hex[:8]
|
||||
|
||||
def try_register() -> int:
|
||||
response: Final = gateway.request(
|
||||
"POST", "/v1/mcp/server", {"server_name": racing_alias, "alias": racing_alias, **peer.registration()}
|
||||
)
|
||||
return response.status_code
|
||||
|
||||
with concurrent.futures.ThreadPoolExecutor(max_workers=8) as pool:
|
||||
statuses: Final = tuple(pool.map(lambda _i: try_register(), range(8)))
|
||||
|
||||
assert statuses.count(201) == 1, statuses
|
||||
assert statuses.count(400) == 7, statuses
|
||||
winner: Final = next(
|
||||
server["server_id"] for server in _servers(gateway).values() if server["alias"] == racing_alias
|
||||
)
|
||||
scenario.cleanups.callback(forget_mcp, gateway, winner)
|
||||
|
||||
|
||||
def test_invalid_registrations_are_rejected(gateway: Gateway) -> None:
|
||||
|
|
|
|||
|
|
@ -29,11 +29,18 @@ def _credentials_cleared(value) -> bool:
|
|||
def _mock_prisma():
|
||||
mock_prisma = MagicMock()
|
||||
mock_prisma.db.litellm_mcpservertable = AsyncMock()
|
||||
row = models.LiteLLM_MCPServerTable.model_construct(
|
||||
server_id="test-server", transport="http", env={}, env_vars=[]
|
||||
)
|
||||
row = models.LiteLLM_MCPServerTable.model_construct(server_id="test-server", transport="http", env={}, env_vars=[])
|
||||
mock_prisma.db.litellm_mcpservertable.update = AsyncMock(return_value=row)
|
||||
mock_prisma.db.litellm_mcpservertable.create = AsyncMock(return_value=row)
|
||||
mock_prisma.db.litellm_mcpservertable.find_first = AsyncMock(return_value=None)
|
||||
mock_prisma.db.litellm_mcpservertable.find_unique = AsyncMock(return_value=None)
|
||||
tx_client = MagicMock()
|
||||
tx_client.execute_raw = AsyncMock()
|
||||
tx_client.litellm_mcpservertable = mock_prisma.db.litellm_mcpservertable
|
||||
tx = MagicMock()
|
||||
tx.__aenter__ = AsyncMock(return_value=tx_client)
|
||||
tx.__aexit__ = AsyncMock(return_value=False)
|
||||
mock_prisma.db.tx = MagicMock(return_value=tx)
|
||||
return mock_prisma
|
||||
|
||||
|
||||
|
|
@ -917,3 +924,170 @@ async def test_toolset_partial_update_ignores_a_null_name():
|
|||
assert await _run_toolset_update({"toolset_id": "ts-1", "toolset_name": None, "description": "kept"}) == {
|
||||
"description": "kept"
|
||||
}
|
||||
|
||||
|
||||
def _conflict_row(server_id: str = "other-server"):
|
||||
return models.LiteLLM_MCPServerTable.model_construct(
|
||||
server_id=server_id, server_name="taken", alias="taken", transport="http", env={}, env_vars=[]
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_find_identifier_conflict_reports_alias_hit():
|
||||
"""A stored row matching the incoming alias yields a conflict naming it.
|
||||
|
||||
Case-insensitive and cross-field matching is exercised end to end against
|
||||
real Postgres by test_duplicate_alias_is_rejected_so_tool_prefixes_cannot_collide.
|
||||
"""
|
||||
from litellm.proxy._experimental.mcp_server.db import (
|
||||
find_mcp_server_identifier_conflict,
|
||||
)
|
||||
|
||||
mock_prisma = _mock_prisma()
|
||||
mock_prisma.db.litellm_mcpservertable.find_first = AsyncMock(return_value=_conflict_row())
|
||||
|
||||
conflict = await find_mcp_server_identifier_conflict(
|
||||
mock_prisma, server_name="new-name", alias="taken", exclude_server_id="my-server"
|
||||
)
|
||||
|
||||
assert conflict is not None
|
||||
assert conflict.field == "alias"
|
||||
assert conflict.value == "taken"
|
||||
assert conflict.server_id == "other-server"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_find_identifier_conflict_reports_server_name_when_alias_is_free():
|
||||
"""alias is checked first so the reported field is deterministic; a clean
|
||||
alias does not mask a colliding server_name."""
|
||||
from litellm.proxy._experimental.mcp_server.db import (
|
||||
find_mcp_server_identifier_conflict,
|
||||
)
|
||||
|
||||
mock_prisma = _mock_prisma()
|
||||
mock_prisma.db.litellm_mcpservertable.find_first = AsyncMock(side_effect=[None, _conflict_row()])
|
||||
|
||||
conflict = await find_mcp_server_identifier_conflict(
|
||||
mock_prisma, server_name="taken", alias="free", exclude_server_id=None
|
||||
)
|
||||
|
||||
assert conflict is not None
|
||||
assert conflict.field == "server_name"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_find_identifier_conflict_returns_none_when_free():
|
||||
from litellm.proxy._experimental.mcp_server.db import (
|
||||
find_mcp_server_identifier_conflict,
|
||||
)
|
||||
|
||||
conflict = await find_mcp_server_identifier_conflict(
|
||||
_mock_prisma(), server_name="fresh", alias="fresh", exclude_server_id=None
|
||||
)
|
||||
|
||||
assert conflict is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_update_writing_alias_returns_conflict_instead_of_row():
|
||||
from litellm.proxy._experimental.mcp_server.db import McpIdentifierConflict
|
||||
|
||||
mock_prisma = _mock_prisma()
|
||||
mock_prisma.db.litellm_mcpservertable.find_first = AsyncMock(return_value=_conflict_row())
|
||||
|
||||
result = await update_mcp_server(
|
||||
mock_prisma,
|
||||
UpdateMCPServerRequest(server_id="my-test-server", alias="taken"),
|
||||
"test-user",
|
||||
)
|
||||
|
||||
assert isinstance(result, McpIdentifierConflict)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_update_without_identifier_fields_returns_the_row():
|
||||
mock_prisma = _mock_prisma()
|
||||
|
||||
result = await update_mcp_server(
|
||||
mock_prisma,
|
||||
UpdateMCPServerRequest(server_id="my-test-server", allowed_tools=["foo"]),
|
||||
"test-user",
|
||||
)
|
||||
|
||||
assert result is not None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_update_writing_free_alias_returns_the_row():
|
||||
mock_prisma = _mock_prisma()
|
||||
|
||||
result = await update_mcp_server(
|
||||
mock_prisma,
|
||||
UpdateMCPServerRequest(server_id="my-test-server", alias="fresh-alias"),
|
||||
"test-user",
|
||||
)
|
||||
|
||||
assert result is not None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_clearing_alias_conflicts_on_the_fallback_server_name():
|
||||
"""alias: null drops the tool prefix to the stored server_name, which may
|
||||
already belong to another row, so that name goes through the conflict check."""
|
||||
from litellm.proxy._experimental.mcp_server.db import McpIdentifierConflict
|
||||
|
||||
mock_prisma = _mock_prisma()
|
||||
existing = MagicMock()
|
||||
existing.server_name = "taken"
|
||||
mock_prisma.db.litellm_mcpservertable.find_unique = AsyncMock(return_value=existing)
|
||||
mock_prisma.db.litellm_mcpservertable.find_first = AsyncMock(return_value=_conflict_row())
|
||||
|
||||
result = await update_mcp_server(
|
||||
mock_prisma,
|
||||
UpdateMCPServerRequest(server_id="my-test-server", alias=None),
|
||||
"test-user",
|
||||
fields_set={"server_id", "alias"},
|
||||
)
|
||||
|
||||
assert isinstance(result, McpIdentifierConflict)
|
||||
assert result.field == "server_name"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_clearing_alias_to_empty_string_conflicts_on_the_fallback_server_name():
|
||||
"""alias: "" publishes the stored server_name as the tool prefix, just like
|
||||
alias: null, so the fallback name must go through the conflict check too."""
|
||||
from litellm.proxy._experimental.mcp_server.db import McpIdentifierConflict
|
||||
|
||||
mock_prisma = _mock_prisma()
|
||||
existing = MagicMock()
|
||||
existing.server_name = "taken"
|
||||
mock_prisma.db.litellm_mcpservertable.find_unique = AsyncMock(return_value=existing)
|
||||
mock_prisma.db.litellm_mcpservertable.find_first = AsyncMock(return_value=_conflict_row())
|
||||
|
||||
result = await update_mcp_server(
|
||||
mock_prisma,
|
||||
UpdateMCPServerRequest(server_id="my-test-server", alias=""),
|
||||
"test-user",
|
||||
fields_set={"server_id", "alias"},
|
||||
)
|
||||
|
||||
assert isinstance(result, McpIdentifierConflict)
|
||||
assert result.field == "server_name"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_clearing_alias_with_free_server_name_returns_the_row():
|
||||
mock_prisma = _mock_prisma()
|
||||
existing = MagicMock()
|
||||
existing.server_name = "free-name"
|
||||
mock_prisma.db.litellm_mcpservertable.find_unique = AsyncMock(return_value=existing)
|
||||
|
||||
result = await update_mcp_server(
|
||||
mock_prisma,
|
||||
UpdateMCPServerRequest(server_id="my-test-server", alias=None),
|
||||
"test-user",
|
||||
fields_set={"server_id", "alias"},
|
||||
)
|
||||
|
||||
assert result is not None
|
||||
|
|
|
|||
|
|
@ -14516,3 +14516,62 @@ async def test_client_sampling_does_not_fill_explicit_context_from_another_ambie
|
|||
assert captured["client_ip"] is None
|
||||
finally:
|
||||
auth_context_var.reset(token)
|
||||
|
||||
|
||||
class TestSharedIdentifierPrefixWarning:
|
||||
"""Two stored rows sharing lowercased alias-or-server_name publish one tool
|
||||
prefix; reload must surface them once so the ambiguity is visible."""
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_reload_warns_once_per_shared_identifier(self, caplog):
|
||||
manager = MCPServerManager()
|
||||
rows = [
|
||||
LiteLLM_MCPServerTable(
|
||||
server_id="srv-a", server_name="alpha", alias="shared", url="https://a.example.com/mcp",
|
||||
transport=MCPTransport.http, updated_at=datetime.now(),
|
||||
),
|
||||
LiteLLM_MCPServerTable(
|
||||
server_id="srv-b", server_name="beta", alias="Shared", url="https://b.example.com/mcp",
|
||||
transport=MCPTransport.http, updated_at=datetime.now(),
|
||||
),
|
||||
LiteLLM_MCPServerTable(
|
||||
server_id="srv-c", server_name="gamma", alias="lonely", url="https://c.example.com/mcp",
|
||||
transport=MCPTransport.http, updated_at=datetime.now(),
|
||||
),
|
||||
]
|
||||
raw_rows = [MagicMock(model_dump=lambda row=row: row.model_dump()) for row in rows]
|
||||
repository = MagicMock()
|
||||
repository.table.find_many = AsyncMock(return_value=raw_rows)
|
||||
|
||||
async def build_from_table(table, **_kwargs):
|
||||
return MCPServer(
|
||||
server_id=table.server_id,
|
||||
name=table.alias or table.server_name,
|
||||
alias=table.alias,
|
||||
server_name=table.server_name,
|
||||
url=table.url,
|
||||
transport=table.transport,
|
||||
)
|
||||
|
||||
with (
|
||||
patch(
|
||||
"litellm.proxy._experimental.mcp_server.mcp_server_manager.MCPServerRepository",
|
||||
return_value=repository,
|
||||
),
|
||||
patch(
|
||||
"litellm.proxy.management_endpoints.mcp_management_endpoints.get_prisma_client_or_throw",
|
||||
return_value=MagicMock(),
|
||||
),
|
||||
patch.object(manager, "build_mcp_server_from_table", new=build_from_table),
|
||||
patch.object(manager, "_maybe_register_openapi_tools", new=AsyncMock()),
|
||||
patch.object(manager, "_prime_oauth_metadata_discovery_for_servers"),
|
||||
caplog.at_level(logging.WARNING, logger="LiteLLM"),
|
||||
):
|
||||
await manager.reload_servers_from_database()
|
||||
|
||||
shared_warnings = [m for m in caplog.messages if "share the identifier" in m]
|
||||
assert len(shared_warnings) == 1
|
||||
assert "srv-a" in shared_warnings[0]
|
||||
assert "srv-b" in shared_warnings[0]
|
||||
assert "srv-c" not in shared_warnings[0]
|
||||
assert "'shared'" in shared_warnings[0]
|
||||
|
|
|
|||
|
|
@ -3936,7 +3936,7 @@ class TestAddMCPServerAtomicity:
|
|||
MagicMock(),
|
||||
),
|
||||
patch(
|
||||
"litellm.proxy.management_endpoints.mcp_management_endpoints.create_mcp_server",
|
||||
"litellm.proxy.management_endpoints.mcp_management_endpoints.create_mcp_server_if_identifier_free",
|
||||
AsyncMock(return_value=created_server),
|
||||
) as create_mock,
|
||||
patch(
|
||||
|
|
@ -3977,7 +3977,7 @@ class TestAddMCPServerAtomicity:
|
|||
MagicMock(),
|
||||
),
|
||||
patch(
|
||||
"litellm.proxy.management_endpoints.mcp_management_endpoints.create_mcp_server",
|
||||
"litellm.proxy.management_endpoints.mcp_management_endpoints.create_mcp_server_if_identifier_free",
|
||||
AsyncMock(side_effect=Exception("db down")),
|
||||
),
|
||||
patch(
|
||||
|
|
@ -4043,7 +4043,7 @@ class TestIdJagRegistrationWarnsAboutTheSSOGap:
|
|||
return_value=MagicMock(),
|
||||
),
|
||||
patch( # test-quality-ok: endpoint test stubs MCP server creation
|
||||
"litellm.proxy.management_endpoints.mcp_management_endpoints.create_mcp_server",
|
||||
"litellm.proxy.management_endpoints.mcp_management_endpoints.create_mcp_server_if_identifier_free",
|
||||
AsyncMock(return_value=self._server_record(auth_type)),
|
||||
),
|
||||
patch( # test-quality-ok: endpoint reads the global MCP manager
|
||||
|
|
@ -4592,7 +4592,7 @@ class TestMCPApprovalWorkflow:
|
|||
MagicMock(),
|
||||
),
|
||||
patch(
|
||||
"litellm.proxy.management_endpoints.mcp_management_endpoints.create_mcp_server",
|
||||
"litellm.proxy.management_endpoints.mcp_management_endpoints.create_mcp_server_if_identifier_free",
|
||||
AsyncMock(return_value=created_record),
|
||||
) as mock_create,
|
||||
):
|
||||
|
|
@ -7532,7 +7532,7 @@ class TestImportMCPServers:
|
|||
AsyncMock(return_value=existing_servers),
|
||||
),
|
||||
patch( # test-quality-ok: endpoint takes collaborators from module scope, matching the suite's pattern
|
||||
"litellm.proxy.management_endpoints.mcp_management_endpoints.create_mcp_server",
|
||||
"litellm.proxy.management_endpoints.mcp_management_endpoints.create_mcp_server_if_identifier_free",
|
||||
create_mock,
|
||||
),
|
||||
patch( # test-quality-ok: endpoint takes collaborators from module scope, matching the suite's pattern
|
||||
|
|
@ -7940,3 +7940,229 @@ async def test_config_server_edit_preserves_api_contract_without_creating_rows(r
|
|||
prisma.tx.assert_not_called()
|
||||
assert server.model_dump() == original
|
||||
assert manager.registry == {}
|
||||
|
||||
|
||||
class TestDuplicateIdentifierRejection:
|
||||
"""server_name/alias must be unique across live servers, case-insensitive.
|
||||
|
||||
The DB layer returns McpIdentifierConflict instead of writing; every write
|
||||
path maps it to a 400 naming the colliding identifier, so a second server
|
||||
can never share another server's tool prefix.
|
||||
"""
|
||||
|
||||
@staticmethod
|
||||
def _conflict(field: str, value: str, server_id: str = "existing-1"):
|
||||
from litellm.proxy._experimental.mcp_server.db import McpIdentifierConflict
|
||||
|
||||
return McpIdentifierConflict(field=field, value=value, server_id=server_id)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_conflict_returns_400_naming_the_alias(self):
|
||||
from litellm.proxy.management_endpoints.mcp_management_endpoints import (
|
||||
add_mcp_server,
|
||||
)
|
||||
|
||||
payload = NewMCPServerRequest(
|
||||
alias="echo",
|
||||
url="https://echo.example.com/mcp",
|
||||
transport=MCPTransport.http,
|
||||
)
|
||||
admin = generate_mock_user_api_key_auth(user_role=LitellmUserRoles.PROXY_ADMIN, user_id="admin-user")
|
||||
|
||||
with (
|
||||
patch(
|
||||
"litellm.proxy.management_endpoints.mcp_management_endpoints.get_prisma_client_or_throw",
|
||||
return_value=MagicMock(),
|
||||
),
|
||||
patch(
|
||||
"litellm.proxy.management_endpoints.mcp_management_endpoints.validate_and_normalize_mcp_server_payload",
|
||||
MagicMock(),
|
||||
),
|
||||
patch(
|
||||
"litellm.proxy.management_endpoints.mcp_management_endpoints.create_mcp_server_if_identifier_free",
|
||||
AsyncMock(return_value=self._conflict("alias", "echo")),
|
||||
),
|
||||
patch(
|
||||
"litellm.proxy.management_endpoints.mcp_management_endpoints.global_mcp_server_manager",
|
||||
MagicMock(),
|
||||
),
|
||||
):
|
||||
with pytest.raises(HTTPException) as exc_info:
|
||||
await add_mcp_server(payload=payload, user_api_key_dict=admin)
|
||||
|
||||
assert exc_info.value.status_code == 400
|
||||
assert "echo" in exc_info.value.detail["error"]
|
||||
assert "existing-1" in exc_info.value.detail["error"]
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_submission_conflict_returns_400(self):
|
||||
from litellm.proxy.management_endpoints.mcp_management_endpoints import (
|
||||
register_mcp_server,
|
||||
)
|
||||
|
||||
payload = NewMCPServerRequest(
|
||||
alias="echo",
|
||||
url="https://echo.example.com/mcp",
|
||||
transport=MCPTransport.http,
|
||||
)
|
||||
team_member = generate_mock_user_api_key_auth(
|
||||
user_role=LitellmUserRoles.INTERNAL_USER, user_id="member", team_id="team-1"
|
||||
)
|
||||
|
||||
with (
|
||||
patch(
|
||||
"litellm.proxy.management_endpoints.mcp_management_endpoints.get_prisma_client_or_throw",
|
||||
return_value=MagicMock(),
|
||||
),
|
||||
patch(
|
||||
"litellm.proxy.management_endpoints.mcp_management_endpoints.validate_and_normalize_mcp_server_payload",
|
||||
MagicMock(),
|
||||
),
|
||||
patch(
|
||||
"litellm.proxy.management_endpoints.mcp_management_endpoints.create_mcp_server_if_identifier_free",
|
||||
AsyncMock(return_value=self._conflict("server_name", "echo")),
|
||||
),
|
||||
):
|
||||
with pytest.raises(HTTPException) as exc_info:
|
||||
await register_mcp_server(payload=payload, user_api_key_dict=team_member)
|
||||
|
||||
assert exc_info.value.status_code == 400
|
||||
assert "echo" in exc_info.value.detail["error"]
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_edit_conflict_returns_400(self):
|
||||
from litellm.proxy.management_endpoints.mcp_management_endpoints import (
|
||||
edit_mcp_server,
|
||||
)
|
||||
|
||||
admin = generate_mock_user_api_key_auth(user_role=LitellmUserRoles.PROXY_ADMIN, user_id="admin-user")
|
||||
existing = generate_mock_mcp_server_db_record(server_id="edit-1", alias="first")
|
||||
|
||||
mock_manager = MagicMock()
|
||||
mock_manager.update_server = AsyncMock()
|
||||
mock_manager.reload_servers_from_database = AsyncMock()
|
||||
|
||||
with (
|
||||
patch(
|
||||
"litellm.proxy.management_endpoints.mcp_management_endpoints.get_prisma_client_or_throw",
|
||||
return_value=MagicMock(),
|
||||
),
|
||||
patch(
|
||||
"litellm.proxy.management_endpoints.mcp_management_endpoints.validate_and_normalize_mcp_server_payload",
|
||||
MagicMock(),
|
||||
),
|
||||
patch(
|
||||
"litellm.proxy.management_endpoints.mcp_management_endpoints.get_mcp_server",
|
||||
AsyncMock(return_value=existing),
|
||||
),
|
||||
patch(
|
||||
"litellm.proxy.management_endpoints.mcp_management_endpoints.update_mcp_server",
|
||||
AsyncMock(return_value=self._conflict("alias", "taken", server_id="other-1")),
|
||||
),
|
||||
patch(
|
||||
"litellm.proxy.management_endpoints.mcp_management_endpoints.global_mcp_server_manager",
|
||||
mock_manager,
|
||||
),
|
||||
):
|
||||
with pytest.raises(HTTPException) as exc_info:
|
||||
await edit_mcp_server(
|
||||
payload=UpdateMCPServerRequest(server_id="edit-1", alias="taken"),
|
||||
user_api_key_dict=admin,
|
||||
)
|
||||
|
||||
assert exc_info.value.status_code == 400
|
||||
assert "taken" in exc_info.value.detail["error"]
|
||||
assert "other-1" in exc_info.value.detail["error"]
|
||||
mock_manager.update_server.assert_not_awaited()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_edit_rename_to_free_alias_succeeds(self):
|
||||
from litellm.proxy.management_endpoints.mcp_management_endpoints import (
|
||||
edit_mcp_server,
|
||||
)
|
||||
|
||||
admin = generate_mock_user_api_key_auth(user_role=LitellmUserRoles.PROXY_ADMIN, user_id="admin-user")
|
||||
existing = generate_mock_mcp_server_db_record(server_id="edit-1", alias="first")
|
||||
updated = generate_mock_mcp_server_db_record(server_id="edit-1", alias="renamed")
|
||||
|
||||
mock_manager = MagicMock()
|
||||
mock_manager.update_server = AsyncMock()
|
||||
mock_manager.reload_servers_from_database = AsyncMock()
|
||||
|
||||
with (
|
||||
patch(
|
||||
"litellm.proxy.management_endpoints.mcp_management_endpoints.get_prisma_client_or_throw",
|
||||
return_value=MagicMock(),
|
||||
),
|
||||
patch(
|
||||
"litellm.proxy.management_endpoints.mcp_management_endpoints.validate_and_normalize_mcp_server_payload",
|
||||
MagicMock(),
|
||||
),
|
||||
patch(
|
||||
"litellm.proxy.management_endpoints.mcp_management_endpoints.get_mcp_server",
|
||||
AsyncMock(return_value=existing),
|
||||
),
|
||||
patch(
|
||||
"litellm.proxy.management_endpoints.mcp_management_endpoints.update_mcp_server",
|
||||
AsyncMock(return_value=updated),
|
||||
),
|
||||
patch(
|
||||
"litellm.proxy.management_endpoints.mcp_management_endpoints.global_mcp_server_manager",
|
||||
mock_manager,
|
||||
),
|
||||
):
|
||||
result = await edit_mcp_server(
|
||||
payload=UpdateMCPServerRequest(server_id="edit-1", alias="renamed"),
|
||||
user_api_key_dict=admin,
|
||||
)
|
||||
|
||||
assert result.alias == "renamed"
|
||||
mock_manager.update_server.assert_awaited_once_with(updated)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_import_skips_case_variant_duplicate(self):
|
||||
from litellm.proxy.management_endpoints.mcp_management_endpoints import (
|
||||
MCPConnectorImportRequest,
|
||||
import_mcp_servers,
|
||||
)
|
||||
|
||||
payload = MCPConnectorImportRequest.model_validate(
|
||||
{"mcpServers": {"EXISTING": {"url": "https://dup.example/mcp"}}}
|
||||
)
|
||||
admin = generate_mock_user_api_key_auth(user_role=LitellmUserRoles.PROXY_ADMIN, user_id="admin-user")
|
||||
existing = generate_mock_mcp_server_db_record(server_id="existing-1", alias="existing")
|
||||
create_mock = AsyncMock()
|
||||
mock_manager = MagicMock()
|
||||
|
||||
with ExitStack() as stack:
|
||||
for p in TestImportMCPServers._import_patches([existing], create_mock, mock_manager):
|
||||
stack.enter_context(p)
|
||||
result = await import_mcp_servers(payload=payload, user_api_key_dict=admin)
|
||||
|
||||
assert [entry.name for entry in result.skipped] == ["EXISTING"]
|
||||
assert "already exists" in result.skipped[0].reason
|
||||
create_mock.assert_not_awaited()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_import_skips_db_reported_identifier_conflict(self):
|
||||
from litellm.proxy.management_endpoints.mcp_management_endpoints import (
|
||||
MCPConnectorImportRequest,
|
||||
import_mcp_servers,
|
||||
)
|
||||
|
||||
payload = MCPConnectorImportRequest.model_validate(
|
||||
{"mcpServers": {"fresh": {"url": "https://dup.example/mcp"}}}
|
||||
)
|
||||
admin = generate_mock_user_api_key_auth(user_role=LitellmUserRoles.PROXY_ADMIN, user_id="admin-user")
|
||||
existing = generate_mock_mcp_server_db_record(server_id="existing-1", alias="existing")
|
||||
create_mock = AsyncMock(return_value=self._conflict("alias", "fresh", server_id="other-9"))
|
||||
mock_manager = MagicMock()
|
||||
|
||||
with ExitStack() as stack:
|
||||
for p in TestImportMCPServers._import_patches([existing], create_mock, mock_manager):
|
||||
stack.enter_context(p)
|
||||
result = await import_mcp_servers(payload=payload, user_api_key_dict=admin)
|
||||
|
||||
assert [entry.name for entry in result.skipped] == ["fresh"]
|
||||
assert "fresh" in result.skipped[0].reason
|
||||
assert result.imported == ()
|
||||
|
|
|
|||
|
|
@ -35,6 +35,7 @@ import {
|
|||
buildCreateServerPayload,
|
||||
reduceStaticHeaders,
|
||||
} from "./createServerPayload";
|
||||
import { DUPLICATE_IDENTIFIER_MESSAGE, findDuplicateMcpServer, mcpSubmitErrorReason } from "./duplicateServerCheck";
|
||||
import { readCreateUiSnapshot, writeCreateUiSnapshot } from "./createOAuthUiState";
|
||||
import AwsSigV4Fields from "./AwsSigV4Fields";
|
||||
import OpenApiByokFields from "./OpenApiByokFields";
|
||||
|
|
@ -78,6 +79,7 @@ interface CreateMCPServerProps {
|
|||
isModalVisible: boolean;
|
||||
setModalVisible: (visible: boolean) => void;
|
||||
availableAccessGroups: string[];
|
||||
existingServers?: MCPServer[];
|
||||
prefillData?: DiscoverableMCPServer | null;
|
||||
onBackToDiscovery?: () => void;
|
||||
}
|
||||
|
|
@ -108,6 +110,7 @@ const CreateMCPServer: React.FC<CreateMCPServerProps> = ({
|
|||
isModalVisible,
|
||||
setModalVisible,
|
||||
availableAccessGroups,
|
||||
existingServers,
|
||||
prefillData,
|
||||
onBackToDiscovery,
|
||||
}) => {
|
||||
|
|
@ -418,6 +421,16 @@ const CreateMCPServer: React.FC<CreateMCPServerProps> = ({
|
|||
};
|
||||
|
||||
const handleCreate = async (values: Record<string, unknown>) => {
|
||||
const duplicate = findDuplicateMcpServer(
|
||||
existingServers,
|
||||
typeof values.server_name === "string" ? values.server_name : undefined,
|
||||
typeof values.alias === "string" ? values.alias : undefined,
|
||||
);
|
||||
if (duplicate) {
|
||||
form.setError(duplicate.field, { type: "duplicate", message: DUPLICATE_IDENTIFIER_MESSAGE });
|
||||
toast.fromError(DUPLICATE_IDENTIFIER_MESSAGE);
|
||||
return;
|
||||
}
|
||||
const built = buildCreateServerPayload(values, {
|
||||
transportType,
|
||||
costConfig,
|
||||
|
|
@ -488,7 +501,7 @@ const CreateMCPServer: React.FC<CreateMCPServerProps> = ({
|
|||
onCreateSuccess(response);
|
||||
}
|
||||
} catch (error) {
|
||||
const reason = error instanceof Error ? error.message : String(error);
|
||||
const reason = mcpSubmitErrorReason(error);
|
||||
toast.fromError(isAdmin ? `Error creating MCP Server: ${reason}` : `Error submitting MCP Server: ${reason}`);
|
||||
} finally {
|
||||
setIsLoading(false);
|
||||
|
|
|
|||
|
|
@ -0,0 +1,49 @@
|
|||
import { describe, expect, it } from "vitest";
|
||||
import { ApiError } from "@/lib/http/client";
|
||||
import { findDuplicateMcpServer, mcpSubmitErrorReason } from "./duplicateServerCheck";
|
||||
|
||||
const servers = [
|
||||
{ server_id: "s1", server_name: "GitHub_MCP", alias: "github" },
|
||||
{ server_id: "s2", server_name: "Email Service", alias: "email_service" },
|
||||
];
|
||||
|
||||
describe("findDuplicateMcpServer", () => {
|
||||
it("flags an incoming server_name that matches an existing alias", () => {
|
||||
expect(findDuplicateMcpServer(servers, "github", "other")?.field).toBe("server_name");
|
||||
});
|
||||
|
||||
it("flags an incoming alias that matches an existing server_name", () => {
|
||||
expect(findDuplicateMcpServer(servers, "new", "GitHub_MCP")?.serverId).toBe("s1");
|
||||
});
|
||||
|
||||
it("matches case-insensitively", () => {
|
||||
expect(findDuplicateMcpServer(servers, "GITHUB", "new")?.serverId).toBe("s1");
|
||||
});
|
||||
|
||||
it("normalizes spaces to underscores like the backend does", () => {
|
||||
expect(findDuplicateMcpServer(servers, "new", "email service")?.serverId).toBe("s2");
|
||||
});
|
||||
|
||||
it("does not flag the server's own identifiers while editing", () => {
|
||||
expect(findDuplicateMcpServer(servers, "GitHub_MCP", "github", "s1")).toBeNull();
|
||||
});
|
||||
|
||||
it("flags the same alias on a different server while editing", () => {
|
||||
expect(findDuplicateMcpServer(servers, "other", "github", "s2")?.serverId).toBe("s1");
|
||||
});
|
||||
|
||||
it("returns null when nothing matches", () => {
|
||||
expect(findDuplicateMcpServer(servers, "brand_new", "brand_new")).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
describe("mcpSubmitErrorReason", () => {
|
||||
it("unwraps the FastAPI detail.error envelope into readable toast text", () => {
|
||||
const error = new ApiError("boom", 400, { detail: { error: "An MCP server with alias 'x' already exists" } });
|
||||
expect(mcpSubmitErrorReason(error)).toContain("An MCP server with alias 'x' already exists");
|
||||
});
|
||||
|
||||
it("never produces [object Object] for a non-Error rejection", () => {
|
||||
expect(mcpSubmitErrorReason({ detail: { error: "structured 400" } })).toBe("structured 400");
|
||||
});
|
||||
});
|
||||
|
|
@ -0,0 +1,48 @@
|
|||
import { MCPServer } from "@/components/mcp_tools/types";
|
||||
import { ApiError, deriveErrorMessage, unwrapProxyErrorMessage } from "@/lib/http/client";
|
||||
|
||||
export type McpIdentifierField = "server_name" | "alias";
|
||||
|
||||
export interface McpIdentifierDuplicate {
|
||||
field: McpIdentifierField;
|
||||
serverId: string;
|
||||
}
|
||||
|
||||
export const normalizeMcpIdentifier = (value: string | null | undefined): string =>
|
||||
(value ?? "").trim().replace(/\s+/g, "_").toLowerCase();
|
||||
|
||||
export function findDuplicateMcpServer(
|
||||
servers: readonly Pick<MCPServer, "server_id" | "server_name" | "alias">[] | undefined,
|
||||
serverName: string | null | undefined,
|
||||
alias: string | null | undefined,
|
||||
excludeServerId?: string,
|
||||
): McpIdentifierDuplicate | null {
|
||||
const candidates: ReadonlyArray<readonly [McpIdentifierField, string | null | undefined]> = [
|
||||
["alias", alias],
|
||||
["server_name", serverName],
|
||||
];
|
||||
for (const [field, value] of candidates) {
|
||||
const normalized = normalizeMcpIdentifier(value);
|
||||
if (!normalized) {
|
||||
continue;
|
||||
}
|
||||
const hit = (servers ?? []).find(
|
||||
(server) =>
|
||||
server.server_id !== excludeServerId &&
|
||||
[server.server_name, server.alias].some((existing) => normalizeMcpIdentifier(existing) === normalized),
|
||||
);
|
||||
if (hit) {
|
||||
return { field, serverId: hit.server_id };
|
||||
}
|
||||
}
|
||||
return null;
|
||||
}
|
||||
|
||||
export const DUPLICATE_IDENTIFIER_MESSAGE = "An MCP server with this name/alias already exists.";
|
||||
|
||||
export const mcpSubmitErrorReason = (error: unknown): string => {
|
||||
if (error instanceof ApiError) {
|
||||
return deriveErrorMessage(error.body);
|
||||
}
|
||||
return error instanceof Error ? unwrapProxyErrorMessage(error.message) : deriveErrorMessage(error);
|
||||
};
|
||||
|
|
@ -51,6 +51,7 @@ import MCPLogoSelector from "./MCPLogoSelector";
|
|||
import EnvVarsSection from "./EnvVarsSection";
|
||||
import { validateMCPServerUrl, validateMCPServerName, normalizeToolOverrideMap } from "./utils";
|
||||
import { EditServerFormValues, buildEditServerPayload, editPayloadErrorMessage } from "./editServerPayload";
|
||||
import { DUPLICATE_IDENTIFIER_MESSAGE, findDuplicateMcpServer, mcpSubmitErrorReason } from "./duplicateServerCheck";
|
||||
import { toast } from "@/lib/toast";
|
||||
import { getEditToolPreview } from "./editToolPreview";
|
||||
import { useMcpOAuthFlow } from "@/hooks/useMcpOAuthFlow";
|
||||
|
|
@ -88,6 +89,7 @@ interface MCPServerEditProps {
|
|||
onCancel: () => void;
|
||||
onSuccess: (server: MCPServer) => void;
|
||||
availableAccessGroups: string[];
|
||||
existingServers?: MCPServer[];
|
||||
}
|
||||
|
||||
const AUTH_TYPES_REQUIRING_AUTH_VALUE = [AUTH_TYPE.API_KEY, AUTH_TYPE.BEARER_TOKEN, AUTH_TYPE.TOKEN, AUTH_TYPE.BASIC];
|
||||
|
|
@ -100,6 +102,7 @@ const MCPServerEdit: React.FC<MCPServerEditProps> = ({
|
|||
onCancel,
|
||||
onSuccess,
|
||||
availableAccessGroups,
|
||||
existingServers,
|
||||
}) => {
|
||||
const initialStaticHeaders = React.useMemo(() => {
|
||||
if (!mcpServer.static_headers) {
|
||||
|
|
@ -724,6 +727,17 @@ const MCPServerEdit: React.FC<MCPServerEditProps> = ({
|
|||
|
||||
const handleSave = async (values: EditServerFormValues) => {
|
||||
if (!accessToken) return;
|
||||
const duplicate = findDuplicateMcpServer(
|
||||
existingServers,
|
||||
values.server_name || mcpServer.server_name,
|
||||
(values.alias ?? mcpServer.alias) || null,
|
||||
mcpServer.server_id,
|
||||
);
|
||||
if (duplicate) {
|
||||
form.setError(duplicate.field, { type: "duplicate", message: DUPLICATE_IDENTIFIER_MESSAGE });
|
||||
toast.fromError(DUPLICATE_IDENTIFIER_MESSAGE);
|
||||
return;
|
||||
}
|
||||
try {
|
||||
const built = buildEditServerPayload(values, {
|
||||
mcpServer,
|
||||
|
|
@ -783,7 +797,8 @@ const MCPServerEdit: React.FC<MCPServerEditProps> = ({
|
|||
setAppMayNotMatchUpstream(false);
|
||||
onSuccess(updated);
|
||||
} catch (error: any) {
|
||||
toast.fromError("Failed to update MCP Server" + (error?.message ? `: ${error.message}` : ""));
|
||||
const reason = mcpSubmitErrorReason(error);
|
||||
toast.fromError("Failed to update MCP Server" + (reason ? `: ${reason}` : ""));
|
||||
}
|
||||
};
|
||||
|
||||
|
|
|
|||
|
|
@ -27,6 +27,7 @@ interface MCPServerViewProps {
|
|||
userID: string | null;
|
||||
isViewOnly?: boolean;
|
||||
availableAccessGroups: string[];
|
||||
existingServers?: MCPServer[];
|
||||
initialTabIndex?: number;
|
||||
}
|
||||
|
||||
|
|
@ -58,6 +59,7 @@ export const MCPServerView: React.FC<MCPServerViewProps> = ({
|
|||
userID,
|
||||
isViewOnly = false,
|
||||
availableAccessGroups,
|
||||
existingServers,
|
||||
initialTabIndex = 0,
|
||||
}) => {
|
||||
// Open the editing Settings tab on first render when returning from the edit OAuth
|
||||
|
|
@ -244,6 +246,7 @@ export const MCPServerView: React.FC<MCPServerViewProps> = ({
|
|||
onCancel={() => setEditing(false)}
|
||||
onSuccess={handleSuccess}
|
||||
availableAccessGroups={availableAccessGroups}
|
||||
existingServers={existingServers}
|
||||
/>
|
||||
) : (
|
||||
<div className="divide-y divide-border">
|
||||
|
|
|
|||
|
|
@ -497,6 +497,7 @@ const MCPServers: React.FC<MCPServerProps> = ({ accessToken, userRole, userID, i
|
|||
isModalVisible={isModalVisible}
|
||||
setModalVisible={setModalVisible}
|
||||
availableAccessGroups={uniqueMcpAccessGroups}
|
||||
existingServers={mcpServers}
|
||||
prefillData={prefillData}
|
||||
onBackToDiscovery={() => {
|
||||
setModalVisible(false);
|
||||
|
|
@ -610,6 +611,7 @@ const MCPServers: React.FC<MCPServerProps> = ({ accessToken, userRole, userID, i
|
|||
userRole={userRole}
|
||||
isViewOnly={isViewOnly}
|
||||
availableAccessGroups={uniqueMcpAccessGroups}
|
||||
existingServers={mcpServers}
|
||||
initialTabIndex={selectedServerId === toolsTabServerId ? 1 : 0}
|
||||
/>
|
||||
) : (
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue