diff --git a/litellm/proxy/_experimental/mcp_server/db.py b/litellm/proxy/_experimental/mcp_server/db.py index 18723a76b2b..70c6e6f4bf3 100644 --- a/litellm/proxy/_experimental/mcp_server/db.py +++ b/litellm/proxy/_experimental/mcp_server/db.py @@ -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 diff --git a/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py b/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py index 64bab0a7832..ade829a1b67 100644 --- a/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py +++ b/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py @@ -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): diff --git a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py index 20c114a2f3e..0c520142fb3 100644 --- a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py +++ b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py @@ -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 diff --git a/litellm/proxy/management_endpoints/mcp_management_endpoints.py b/litellm/proxy/management_endpoints/mcp_management_endpoints.py index c22081ebb3a..aa218f42023 100644 --- a/litellm/proxy/management_endpoints/mcp_management_endpoints.py +++ b/litellm/proxy/management_endpoints/mcp_management_endpoints.py @@ -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, diff --git a/tests/integration/mcp/test_mcp_management.py b/tests/integration/mcp/test_mcp_management.py index 917acb9a1dc..bde18840d7d 100644 --- a/tests/integration/mcp/test_mcp_management.py +++ b/tests/integration/mcp/test_mcp_management.py @@ -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: diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_partial_update.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_partial_update.py index 669e094fee4..bd37a976286 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_partial_update.py +++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_partial_update.py @@ -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 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 7725aca1948..cd5dae1269a 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 @@ -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] 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 53645e62034..557e753a76f 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 @@ -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 == () diff --git a/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/CreateMCPServer.tsx b/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/CreateMCPServer.tsx index ebca780c766..f656bd2fc60 100644 --- a/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/CreateMCPServer.tsx +++ b/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/CreateMCPServer.tsx @@ -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 = ({ isModalVisible, setModalVisible, availableAccessGroups, + existingServers, prefillData, onBackToDiscovery, }) => { @@ -418,6 +421,16 @@ const CreateMCPServer: React.FC = ({ }; const handleCreate = async (values: Record) => { + 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 = ({ 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); diff --git a/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/duplicateServerCheck.test.ts b/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/duplicateServerCheck.test.ts new file mode 100644 index 00000000000..8590e40229c --- /dev/null +++ b/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/duplicateServerCheck.test.ts @@ -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"); + }); +}); diff --git a/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/duplicateServerCheck.ts b/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/duplicateServerCheck.ts new file mode 100644 index 00000000000..0e24e13e7f1 --- /dev/null +++ b/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/duplicateServerCheck.ts @@ -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[] | undefined, + serverName: string | null | undefined, + alias: string | null | undefined, + excludeServerId?: string, +): McpIdentifierDuplicate | null { + const candidates: ReadonlyArray = [ + ["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); +}; diff --git a/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/mcp_server_edit.tsx b/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/mcp_server_edit.tsx index 2a37029a2c4..d45909445e9 100644 --- a/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/mcp_server_edit.tsx +++ b/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/mcp_server_edit.tsx @@ -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 = ({ onCancel, onSuccess, availableAccessGroups, + existingServers, }) => { const initialStaticHeaders = React.useMemo(() => { if (!mcpServer.static_headers) { @@ -724,6 +727,17 @@ const MCPServerEdit: React.FC = ({ 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 = ({ 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}` : "")); } }; diff --git a/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/mcp_server_view.tsx b/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/mcp_server_view.tsx index 6045be4607d..a7ff34301a0 100644 --- a/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/mcp_server_view.tsx +++ b/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/mcp_server_view.tsx @@ -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 = ({ 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 = ({ onCancel={() => setEditing(false)} onSuccess={handleSuccess} availableAccessGroups={availableAccessGroups} + existingServers={existingServers} /> ) : (
diff --git a/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/mcp_servers.tsx b/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/mcp_servers.tsx index 738409e28c2..b4b7ab6b3c8 100644 --- a/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/mcp_servers.tsx +++ b/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/mcp_servers.tsx @@ -497,6 +497,7 @@ const MCPServers: React.FC = ({ 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 = ({ accessToken, userRole, userID, i userRole={userRole} isViewOnly={isViewOnly} availableAccessGroups={uniqueMcpAccessGroups} + existingServers={mcpServers} initialTabIndex={selectedServerId === toolsTabServerId ? 1 : 0} /> ) : (