mirror of
https://github.com/langflow-ai/langflow.git
synced 2026-07-24 07:34:10 +08:00
fix: prevent MCP server edit from creating a duplicate on name change (#13464)
* fix: prevent MCP server edit from creating a duplicate on name change
When editing an MCP server, changing the name field caused a brand-new
duplicate server to be created instead of updating the existing one,
leaving the original untouched. It only happened when the name input
was changed.
Root cause: the backend PATCH /mcp/servers/{server_name} endpoint
upserts keyed by the name in the URL path, and the edit modal built the
PATCH URL from the edited name. A changed name therefore retargeted the
request to a new key, so the backend created a new server rather than
updating the original.
The server name is the immutable identifier (the storage key and the
PATCH URL path), so during edit we now:
- always reuse the original name (initialData.name) for the request
instead of re-deriving it from the input, and
- lock the name input, consistent with how the type tabs are already
disabled in edit mode.
Adds regression tests covering the locked name field (STDIO + HTTP) and
that editing patches the original name without firing the create flow.
* fix: immutable server names
This commit is contained in:
Eric Hare
@ -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,
|
||||
|
||||
@ -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()
|
||||
|
||||
@ -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;
|
||||
}) => (
|
||||
<input
|
||||
value={value}
|
||||
placeholder={placeholder}
|
||||
data-testid={dataTestId}
|
||||
disabled={disabled}
|
||||
onChange={(event) =>
|
||||
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(
|
||||
<AddMcpServerModal
|
||||
open={true}
|
||||
setOpen={jest.fn()}
|
||||
initialData={initialData}
|
||||
/>,
|
||||
);
|
||||
|
||||
// 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(
|
||||
<AddMcpServerModal
|
||||
open={true}
|
||||
setOpen={jest.fn()}
|
||||
initialData={initialData}
|
||||
/>,
|
||||
);
|
||||
|
||||
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(
|
||||
<AddMcpServerModal
|
||||
open={true}
|
||||
setOpen={jest.fn()}
|
||||
initialData={initialData}
|
||||
/>,
|
||||
);
|
||||
|
||||
// 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();
|
||||
});
|
||||
});
|
||||
|
||||
@ -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}
|
||||
/>
|
||||
</div>
|
||||
<div className="flex flex-col gap-2">
|
||||
@ -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}
|
||||
/>
|
||||
</div>
|
||||
<div className="flex flex-col gap-2">
|
||||
|
||||
Reference in New Issue
Block a user