diff --git a/src/backend/base/langflow/api/v2/mcp.py b/src/backend/base/langflow/api/v2/mcp.py index 2dc79af3e1..91ee1f892b 100644 --- a/src/backend/base/langflow/api/v2/mcp.py +++ b/src/backend/base/langflow/api/v2/mcp.py @@ -40,6 +40,46 @@ def is_mcp_servers_locked(settings: object) -> bool: return getattr(settings, "mcp_servers_locked", False) is True +def _enforce_immutable_server_name(server_name: str, server_config: dict) -> dict: + """Enforce that the server name is owned by the URL path, not the request body. + + ``server_name`` is the immutable identifier for an MCP server: it is both the + storage key and the POST/PATCH URL path segment. Because :class:`MCPServerConfig` + permits extra fields (``extra="allow"``), a ``name`` in the request body would + otherwise be silently persisted as stray config data and — on PATCH — echoed back + in the 200 response, falsely implying that a rename succeeded. + + A body ``name`` that disagrees with the URL is rejected with 422 so the constraint + is explicit. A redundant but matching ``name`` is dropped so it never pollutes the + stored config. + + Args: + server_name: The server name from the URL path (the canonical identifier). + server_config: The request body dumped to a dict. + + Returns: + A config dict with any ``name`` key removed. + + Raises: + HTTPException: 422 when ``name`` is present and differs from ``server_name``. + """ + if "name" not in server_config: + return server_config + + body_name = server_config["name"] + if body_name != server_name: + raise HTTPException( + status_code=422, + detail=( + f"Server name is immutable and is determined by the URL path " + f"('{server_name}'); it cannot be set or changed via the request body " + f"(got '{body_name}'). To rename a server, delete it and create a new one." + ), + ) + # Matching name is redundant — drop it so it isn't persisted as stray config. + return {key: value for key, value in server_config.items() if key != "name"} + + async def upload_server_config( server_config: dict, current_user: CurrentActiveUser, @@ -353,7 +393,7 @@ async def add_server( return await update_server( server_name, - server_config.model_dump(exclude_unset=True), + _enforce_immutable_server_name(server_name, server_config.model_dump(exclude_unset=True)), current_user, session, storage_service, @@ -380,7 +420,7 @@ async def update_server_endpoint( return await update_server( server_name, - server_config.model_dump(exclude_unset=True), + _enforce_immutable_server_name(server_name, server_config.model_dump(exclude_unset=True)), current_user, session, storage_service, diff --git a/src/backend/tests/unit/api/v2/test_mcp_servers_file.py b/src/backend/tests/unit/api/v2/test_mcp_servers_file.py index 80fb98fef1..a95ea0bdeb 100644 --- a/src/backend/tests/unit/api/v2/test_mcp_servers_file.py +++ b/src/backend/tests/unit/api/v2/test_mcp_servers_file.py @@ -4,7 +4,7 @@ from types import SimpleNamespace from typing import TYPE_CHECKING import pytest -from fastapi import UploadFile +from fastapi import HTTPException, UploadFile # Module under test from langflow.api.v2.files import upload_user_file @@ -239,3 +239,122 @@ async def test_concurrent_update_server_should_not_lose_servers( assert "server_a" in config_state["mcpServers"], "server_a was lost due to concurrent update_server race condition" assert "server_b" in config_state["mcpServers"], "server_b was lost due to concurrent update_server race condition" assert "server_c" in config_state["mcpServers"], "server_c was lost due to concurrent update_server race condition" + + +def test_enforce_immutable_server_name_rejects_mismatch(): + """A body name that differs from the URL name is an explicit 422, not a silent no-op.""" + from langflow.api.v2.mcp import _enforce_immutable_server_name + + with pytest.raises(HTTPException) as exc_info: + _enforce_immutable_server_name("old-name", {"name": "new-name", "url": "http://localhost:9000"}) + + assert exc_info.value.status_code == 422 + # The message names both the URL identifier and the rejected body value. + assert "old-name" in exc_info.value.detail + assert "new-name" in exc_info.value.detail + + +def test_enforce_immutable_server_name_strips_matching_name(): + """A redundant matching name is dropped so it never pollutes the stored config.""" + from langflow.api.v2.mcp import _enforce_immutable_server_name + + cleaned = _enforce_immutable_server_name("srv", {"name": "srv", "command": "npx", "args": ["-y", "x"]}) + + assert cleaned == {"command": "npx", "args": ["-y", "x"]} + assert "name" not in cleaned + + +def test_enforce_immutable_server_name_passthrough_without_name(): + """Configs without a name field are returned unchanged.""" + from langflow.api.v2.mcp import _enforce_immutable_server_name + + config = {"command": "npx", "args": ["-y", "x"]} + + assert _enforce_immutable_server_name("srv", config) == config + + +@pytest.mark.asyncio +async def test_patch_server_rejects_name_change(session, storage_service, settings_service, current_user): + """PATCH with a body name different from the URL must 422 instead of silently no-op'ing. + + Regression: MCPServerConfig allows extra fields, so a ``name`` in the PATCH body was + persisted as stray config and echoed in the 200 response, falsely implying a rename + succeeded while the server stayed keyed under the original (URL) name. + """ + from unittest.mock import AsyncMock, patch + + from langflow.api.v2.mcp import update_server_endpoint + from langflow.api.v2.schemas import MCPServerConfig + + body = MCPServerConfig(name="new-name", url="http://localhost:9000") + + with ( + patch("langflow.api.v2.mcp.update_server", new=AsyncMock()) as mock_update, + pytest.raises(HTTPException) as exc_info, + ): + await update_server_endpoint( + server_name="old-name", + server_config=body, + current_user=current_user, + session=session, + storage_service=storage_service, + settings_service=settings_service, + ) + + assert exc_info.value.status_code == 422 + mock_update.assert_not_called() # guard fires before any write/upsert + + +@pytest.mark.asyncio +async def test_patch_server_strips_matching_name_before_persist( + session, storage_service, settings_service, current_user +): + """A PATCH body that echoes the URL name must not persist ``name`` as stray config.""" + from unittest.mock import AsyncMock, patch + + from langflow.api.v2.mcp import update_server_endpoint + from langflow.api.v2.schemas import MCPServerConfig + + body = MCPServerConfig(name="srv", url="http://localhost:9000") + + with patch("langflow.api.v2.mcp.update_server", new=AsyncMock(return_value={"ok": True})) as mock_update: + await update_server_endpoint( + server_name="srv", + server_config=body, + current_user=current_user, + session=session, + storage_service=storage_service, + settings_service=settings_service, + ) + + # update_server is called positionally as (server_name, server_config_dict, ...). + persisted_config = mock_update.call_args.args[1] + assert persisted_config == {"url": "http://localhost:9000"} + assert "name" not in persisted_config + + +@pytest.mark.asyncio +async def test_post_server_rejects_name_mismatch(session, storage_service, settings_service, current_user): + """POST with a body name different from the URL must 422 (same guard as PATCH).""" + from unittest.mock import AsyncMock, patch + + from langflow.api.v2.mcp import add_server + from langflow.api.v2.schemas import MCPServerConfig + + body = MCPServerConfig(name="different", url="http://localhost:9000") + + with ( + patch("langflow.api.v2.mcp.update_server", new=AsyncMock()) as mock_update, + pytest.raises(HTTPException) as exc_info, + ): + await add_server( + server_name="intended-name", + server_config=body, + current_user=current_user, + session=session, + storage_service=storage_service, + settings_service=settings_service, + ) + + assert exc_info.value.status_code == 422 + mock_update.assert_not_called() diff --git a/src/frontend/src/modals/addMcpServerModal/__tests__/addMcpServerModal.test.tsx b/src/frontend/src/modals/addMcpServerModal/__tests__/addMcpServerModal.test.tsx index 2adb39ec28..f12f4d9083 100644 --- a/src/frontend/src/modals/addMcpServerModal/__tests__/addMcpServerModal.test.tsx +++ b/src/frontend/src/modals/addMcpServerModal/__tests__/addMcpServerModal.test.tsx @@ -5,6 +5,7 @@ import type { MCPServerType } from "@/types/mcp"; import AddMcpServerModal from ".."; const mockPatchMCPServer = jest.fn(); +const mockAddMCPServer = jest.fn(); const mockSetQueryData = jest.fn(); jest.mock("nanoid", () => ({ @@ -23,7 +24,7 @@ jest.mock("@tanstack/react-query", () => ({ jest.mock("@/controllers/API/queries/mcp/use-add-mcp-server", () => ({ useAddMCPServer: () => ({ - mutateAsync: jest.fn(), + mutateAsync: mockAddMCPServer, isPending: false, }), })); @@ -66,16 +67,19 @@ jest.mock("@/components/ui/input", () => ({ onChange, "data-testid": dataTestId, placeholder, + disabled, }: { value?: string; onChange?: (event: { target: { value: string } }) => void; "data-testid"?: string; placeholder?: string; + disabled?: boolean; }) => ( onChange?.({ target: { value: event.target.value } }) } @@ -203,6 +207,7 @@ describe("AddMcpServerModal", () => { beforeEach(() => { jest.clearAllMocks(); mockPatchMCPServer.mockResolvedValue(undefined); + mockAddMCPServer.mockResolvedValue(undefined); }); it("sends empty headers and env when deleting the last remaining HTTP entries", async () => { @@ -233,4 +238,68 @@ describe("AddMcpServerModal", () => { env: {}, }); }); + + it("locks the HTTP server name field when editing an existing server", () => { + const initialData: MCPServerType = { + name: "my-server", + url: "http://host/sse", + }; + + render( + , + ); + + // The name is the server's immutable identifier, so it cannot be edited. + expect(screen.getByTestId("http-name-input")).toBeDisabled(); + }); + + it("locks the STDIO server name field when editing an existing server", () => { + const initialData: MCPServerType = { + name: "my-server", + command: "uvx", + args: ["mcp-server"], + }; + + render( + , + ); + + expect(screen.getByTestId("stdio-name-input")).toBeDisabled(); + }); + + it("patches the original server name when editing, instead of creating a duplicate", async () => { + const user = userEvent.setup(); + const initialData: MCPServerType = { + name: "my-server", + command: "uvx", + args: ["mcp-server"], + }; + + render( + , + ); + + // The name field is locked, so the update targets the original record. + await user.click(screen.getByTestId("add-mcp-server-button")); + + expect(mockPatchMCPServer).toHaveBeenCalledTimes(1); + expect(mockPatchMCPServer).toHaveBeenCalledWith( + expect.objectContaining({ name: "my-server" }), + ); + // The create (add) flow must never fire during an edit — that is what + // produced the duplicate server. + expect(mockAddMCPServer).not.toHaveBeenCalled(); + }); }); diff --git a/src/frontend/src/modals/addMcpServerModal/index.tsx b/src/frontend/src/modals/addMcpServerModal/index.tsx index b7907c5e27..d19e8e689a 100644 --- a/src/frontend/src/modals/addMcpServerModal/index.tsx +++ b/src/frontend/src/modals/addMcpServerModal/index.tsx @@ -181,11 +181,18 @@ export default function AddMcpServerModal({ setError(t("mcp.modal.errorDuplicateEnvKeys")); return; } - const name = parseString(stdioName, [ - "mcp_name_case", - "no_blank", - "lowercase", - ]).slice(0, MAX_MCP_SERVER_NAME_LENGTH); + // The server name is the immutable identifier: it is the storage key and + // the URL path PATCH targets. When editing, always reuse the original + // name so the update hits the existing record. Re-deriving it from the + // input would let a name change retarget the request and create a + // duplicate server instead of updating the original. + const name = initialData + ? initialData.name + : parseString(stdioName, [ + "mcp_name_case", + "no_blank", + "lowercase", + ]).slice(0, MAX_MCP_SERVER_NAME_LENGTH); const argsPayload = buildArgsPayload(stdioArgs, initialData?.args); const envPayload = buildKeyPairPayload(stdioEnv, initialData?.env); try { @@ -233,11 +240,18 @@ export default function AddMcpServerModal({ setError(t("mcp.modal.errorDuplicateHeaders")); return; } - const name = parseString(httpName, [ - "mcp_name_case", - "no_blank", - "lowercase", - ]).slice(0, MAX_MCP_SERVER_NAME_LENGTH); + // The server name is the immutable identifier: it is the storage key and + // the URL path PATCH targets. When editing, always reuse the original + // name so the update hits the existing record. Re-deriving it from the + // input would let a name change retarget the request and create a + // duplicate server instead of updating the original. + const name = initialData + ? initialData.name + : parseString(httpName, [ + "mcp_name_case", + "no_blank", + "lowercase", + ]).slice(0, MAX_MCP_SERVER_NAME_LENGTH); const envPayload = buildKeyPairPayload(httpEnv, initialData?.env); const headersPayload = buildKeyPairPayload( httpHeaders, @@ -432,7 +446,7 @@ export default function AddMcpServerModal({ onChange={(e) => setStdioName(e.target.value)} placeholder={t("mcp.modal.placeholderServerName")} data-testid="stdio-name-input" - disabled={isPending} + disabled={isPending || !!initialData} />
@@ -493,7 +507,7 @@ export default function AddMcpServerModal({ onChange={(e) => setHttpName(e.target.value)} placeholder={t("mcp.modal.placeholderHttpName")} data-testid="http-name-input" - disabled={isPending} + disabled={isPending || !!initialData} />