diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx
index 4c6598bf..a32ecb90 100644
--- a/clients/web/src/App.tsx
+++ b/clients/web/src/App.tsx
@@ -139,6 +139,7 @@ import {
type ServerConfigModalMode,
} from "./components/groups/ServerConfigModal/ServerConfigModal";
import type { KeyValuePair } from "./components/elements/KeyValueRows/KeyValueRows";
+import { buildHeaderSettingsPatch } from "./utils/serverSettingsPatch";
import { ServerSettingsModal } from "./components/groups/ServerSettingsModal/ServerSettingsModal";
import { ClientSettingsModal } from "./components/groups/ClientSettingsModal/ClientSettingsModal";
import {
@@ -3927,25 +3928,15 @@ function App() {
const onConfigSubmit = useCallback(
async (id: string, config: MCPServerConfig, headers: KeyValuePair[]) => {
// Headers live on the entry's `settings`, not on the transport config,
- // so they're folded back in here (#1915).
- //
- // Only an EDIT carries the target's other settings forward. `settings`
- // replaces the whole node, so an edit must re-send the fields this modal
- // doesn't expose (metadata, timeouts, OAuth credentials, roots) or they
- // would be dropped. A CLONE must not: `configModalTarget` there is the
- // *source* server, and spreading it would copy that server's OAuth
- // client secret and every behavior flag onto a new entry the user only
- // gave a URL and some headers.
- const isEdit = configModal?.mode === "edit" && !!configModal.targetId;
- const existing = isEdit ? configModalTarget?.settings : undefined;
- // Send `settings` only when it would change something: with no headers
- // on either side there is nothing to write, and omitting the key is what
- // makes the backend preserve the node it already has.
- const settingsChanged =
- headers.length > 0 || (existing?.headers.length ?? 0) > 0;
- const settings = settingsChanged
- ? { ...(existing ?? EMPTY_SETTINGS), headers }
- : undefined;
+ // so they're folded back in here (#1915). The edit-vs-clone rules live
+ // in the helper, which is unit-tested — this file is outside the
+ // coverage gate and that distinction is where credentials can leak.
+ const settings = buildHeaderSettingsPatch(
+ configModal?.mode ?? "add",
+ configModalTarget?.settings,
+ headers,
+ EMPTY_SETTINGS,
+ );
if (configModal?.mode === "edit" && configModal.targetId) {
const originalId = configModal.targetId;
await updateServer(originalId, id, config, settings);
diff --git a/clients/web/src/components/elements/KeyValueRows/KeyValueRows.test.tsx b/clients/web/src/components/elements/KeyValueRows/KeyValueRows.test.tsx
index ef96bb8d..09fceca7 100644
--- a/clients/web/src/components/elements/KeyValueRows/KeyValueRows.test.tsx
+++ b/clients/web/src/components/elements/KeyValueRows/KeyValueRows.test.tsx
@@ -4,13 +4,14 @@ import { renderWithMantine, screen } from "../../../test/renderWithMantine";
import { KeyValueRows } from "./KeyValueRows";
describe("KeyValueRows", () => {
- function setup(items: { key: string; value: string }[]) {
+ function setup(items: { key: string; value: string }[], disabled?: boolean) {
const onChange = vi.fn();
const onRemove = vi.fn();
renderWithMantine(
,
@@ -57,21 +58,42 @@ describe("KeyValueRows", () => {
expect(onChange).toHaveBeenLastCalledWith(0, "X", "12");
});
- it("clears a key or a value through its Clear button", async () => {
+ it("clears a key or a value through its own named Clear button", async () => {
const user = userEvent.setup({ delay: null });
const { onChange } = setup([{ key: "X", value: "1" }]);
- const clearButtons = screen.getAllByRole("button", { name: "Clear" });
- await user.click(clearButtons[0]!);
+ // A bare "Clear" repeated per field would be indistinguishable across
+ // rows, so each clear button names the field it empties.
+ await user.click(
+ screen.getByRole("button", { name: "Clear header name, X" }),
+ );
expect(onChange).toHaveBeenLastCalledWith(0, "", "1");
- await user.click(clearButtons[1]!);
+ await user.click(
+ screen.getByRole("button", { name: "Clear header value, X" }),
+ );
expect(onChange).toHaveBeenLastCalledWith(0, "X", "");
});
it("omits the Clear button for an empty key or value", () => {
setup([{ key: "", value: "" }]);
- expect(screen.queryAllByRole("button", { name: "Clear" })).toHaveLength(0);
+ expect(screen.queryAllByRole("button", { name: /^Clear/ })).toHaveLength(0);
+ });
+
+ it("locks every control when disabled", () => {
+ setup([{ key: "X", value: "1" }], true);
+ expect(
+ screen.getByRole("textbox", { name: "header name, X" }),
+ ).toBeDisabled();
+ expect(
+ screen.getByRole("textbox", { name: "header value, X" }),
+ ).toBeDisabled();
+ expect(
+ screen.getByRole("button", { name: "Clear header name, X" }),
+ ).toBeDisabled();
+ expect(
+ screen.getByRole("button", { name: "Remove header, X" }),
+ ).toBeDisabled();
});
it("removes the clicked row", async () => {
diff --git a/clients/web/src/components/elements/KeyValueRows/KeyValueRows.tsx b/clients/web/src/components/elements/KeyValueRows/KeyValueRows.tsx
index c41bd9c3..cf03e8e1 100644
--- a/clients/web/src/components/elements/KeyValueRows/KeyValueRows.tsx
+++ b/clients/web/src/components/elements/KeyValueRows/KeyValueRows.tsx
@@ -13,9 +13,16 @@ export interface KeyValueRowsProps {
* to build each control's `aria-label`: the section heading and the "Key" /
* "Value" placeholders are not programmatically associated with the inputs,
* so without it an assistive technology cannot tell one list's key box from
- * another's, and the remove button announces only "X".
+ * another's, and the remove button announces only "X". The clear buttons are
+ * named the same way — a bare "Clear" repeated six times across three rows
+ * tells a screen-reader user nothing about which field it empties.
*/
entityLabel: string;
+ /**
+ * Lock every control in the list — used while a submit is in flight, so the
+ * rows can't drift out of sync with the payload the caller already captured.
+ */
+ disabled?: boolean;
onChange: (index: number, key: string, value: string) => void;
onRemove: (index: number) => void;
}
@@ -42,6 +49,7 @@ const RemoveIcon = ActionIcon.withProps({
export function KeyValueRows({
items,
entityLabel,
+ disabled,
onChange,
onRemove,
}: KeyValueRowsProps) {
@@ -55,12 +63,15 @@ export function KeyValueRows({
placeholder="Key"
aria-label={`${entityLabel} name, ${rowLabel}`}
value={item.key}
+ disabled={disabled}
onChange={(e) =>
onChange(index, e.currentTarget.value, item.value)
}
rightSection={
item.key ? (
onChange(index, "", item.value)}
/>
) : null
@@ -70,15 +81,21 @@ export function KeyValueRows({
placeholder="Value"
aria-label={`${entityLabel} value, ${rowLabel}`}
value={item.value}
+ disabled={disabled}
onChange={(e) => onChange(index, item.key, e.currentTarget.value)}
rightSection={
item.value ? (
- onChange(index, item.key, "")} />
+ onChange(index, item.key, "")}
+ />
) : null
}
/>
onRemove(index)}
>
X
diff --git a/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.tsx b/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.tsx
index 24089ff5..0361aa32 100644
--- a/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.tsx
+++ b/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.tsx
@@ -504,6 +504,7 @@ export function ServerConfigModal({
diff --git a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx
index daf31021..8bc001c7 100644
--- a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx
+++ b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx
@@ -8,12 +8,15 @@ import {
type ServerSettingsSection,
} from "./ServerSettingsForm";
-/** Find the "Clear" button living in the rightSection of `input`'s field. */
+/** Find the clear button living in the rightSection of `input`'s field. The
+ * name is a prefix match because a KeyValueRows field names its clear button
+ * for the row it belongs to ("Clear header name, Cookie"), while a standalone
+ * field keeps the bare "Clear". */
function clearButtonFor(input: HTMLElement): HTMLElement {
const root =
input.closest('[class*="mantine-TextInput-root"]') ??
input.closest('[class*="Input-wrapper"]');
- return within(root as HTMLElement).getByRole("button", { name: "Clear" });
+ return within(root as HTMLElement).getByRole("button", { name: /^Clear/ });
}
const emptySettings: InspectorServerSettings = {
@@ -1391,12 +1394,12 @@ describe("ServerSettingsForm", () => {
expect(
within(
keyInput.closest('[class*="Input-wrapper"]') as HTMLElement,
- ).queryByRole("button", { name: "Clear" }),
+ ).queryByRole("button", { name: /^Clear/ }),
).toBeNull();
expect(
within(
valueInput.closest('[class*="Input-wrapper"]') as HTMLElement,
- ).queryByRole("button", { name: "Clear" }),
+ ).queryByRole("button", { name: /^Clear/ }),
).toBeNull();
});
@@ -1419,12 +1422,12 @@ describe("ServerSettingsForm", () => {
expect(
within(
uriInput.closest('[class*="Input-wrapper"]') as HTMLElement,
- ).queryByRole("button", { name: "Clear" }),
+ ).queryByRole("button", { name: /^Clear/ }),
).toBeNull();
expect(
within(
nameInput.closest('[class*="Input-wrapper"]') as HTMLElement,
- ).queryByRole("button", { name: "Clear" }),
+ ).queryByRole("button", { name: /^Clear/ }),
).toBeNull();
// Typing into the URI threads the empty name through (`root.name ?? ""`).
await user.type(uriInput, "f");
diff --git a/clients/web/src/utils/serverSettingsPatch.test.ts b/clients/web/src/utils/serverSettingsPatch.test.ts
new file mode 100644
index 00000000..12d7c081
--- /dev/null
+++ b/clients/web/src/utils/serverSettingsPatch.test.ts
@@ -0,0 +1,93 @@
+import { describe, it, expect } from "vitest";
+import type { InspectorServerSettings } from "@inspector/core/mcp/types.js";
+import { buildHeaderSettingsPatch } from "./serverSettingsPatch";
+
+const EMPTY: InspectorServerSettings = {
+ headers: [],
+ env: [],
+ metadata: [],
+ connectionTimeout: 0,
+ requestTimeout: 0,
+ taskTtl: 60000,
+ autoRefreshOnListChanged: false,
+ paginatedLists: false,
+ maxFetchRequests: 1000,
+ roots: [],
+};
+
+// A server carrying settings the modal never shows — the fields an edit must
+// preserve and a clone must not copy.
+const POPULATED: InspectorServerSettings = {
+ ...EMPTY,
+ headers: [{ key: "X-Old", value: "1" }],
+ metadata: [{ key: "trace", value: "abc" }],
+ connectionTimeout: 5000,
+ oauthClientId: "cid",
+ oauthClientSecret: "shhh",
+ roots: [{ uri: "file:///project", name: "Project" }],
+};
+
+const NEW_HEADERS = [{ key: "Cookie", value: "branch=x" }];
+
+describe("buildHeaderSettingsPatch", () => {
+ it("sends nothing when there are no headers on either side", () => {
+ expect(
+ buildHeaderSettingsPatch("add", undefined, [], EMPTY),
+ ).toBeUndefined();
+ expect(buildHeaderSettingsPatch("edit", EMPTY, [], EMPTY)).toBeUndefined();
+ });
+
+ it("carries the target's other settings forward on an edit", () => {
+ const patch = buildHeaderSettingsPatch(
+ "edit",
+ POPULATED,
+ NEW_HEADERS,
+ EMPTY,
+ );
+ expect(patch).toEqual({ ...POPULATED, headers: NEW_HEADERS });
+ // The node is replaced wholesale, so the fields the modal doesn't show
+ // have to travel with it.
+ expect(patch?.metadata).toEqual([{ key: "trace", value: "abc" }]);
+ expect(patch?.oauthClientSecret).toBe("shhh");
+ expect(patch?.connectionTimeout).toBe(5000);
+ });
+
+ it("still sends on an edit that clears the last header", () => {
+ expect(buildHeaderSettingsPatch("edit", POPULATED, [], EMPTY)).toEqual({
+ ...POPULATED,
+ headers: [],
+ });
+ });
+
+ it("does not copy the source server's settings on a clone", () => {
+ // The regression this guards: `configModalTarget` in clone mode is the
+ // SOURCE entry, so spreading it put that server's OAuth client secret on a
+ // brand-new one.
+ const patch = buildHeaderSettingsPatch(
+ "clone",
+ POPULATED,
+ NEW_HEADERS,
+ EMPTY,
+ );
+ expect(patch).toEqual({ ...EMPTY, headers: NEW_HEADERS });
+ expect(patch?.oauthClientSecret).toBeUndefined();
+ expect(patch?.oauthClientId).toBeUndefined();
+ expect(patch?.metadata).toEqual([]);
+ expect(patch?.roots).toEqual([]);
+ expect(patch?.connectionTimeout).toBe(0);
+ });
+
+ it("builds from the empty shape on an add", () => {
+ expect(
+ buildHeaderSettingsPatch("add", undefined, NEW_HEADERS, EMPTY),
+ ).toEqual({ ...EMPTY, headers: NEW_HEADERS });
+ });
+
+ it("ignores a stale target on a clone with no headers", () => {
+ // A clone of a server that HAS headers, submitted with none: nothing about
+ // the new entry needs a settings node.
+ expect(
+ buildHeaderSettingsPatch("clone", POPULATED, [], EMPTY),
+ ).toBeUndefined();
+ });
+});
diff --git a/clients/web/src/utils/serverSettingsPatch.ts b/clients/web/src/utils/serverSettingsPatch.ts
new file mode 100644
index 00000000..b2b51a81
--- /dev/null
+++ b/clients/web/src/utils/serverSettingsPatch.ts
@@ -0,0 +1,42 @@
+import type { InspectorServerSettings } from "@inspector/core/mcp/types.js";
+import type { ServerConfigModalMode } from "../components/groups/ServerConfigModal/ServerConfigModal";
+import type { KeyValuePair } from "../components/elements/KeyValueRows/KeyValueRows";
+
+/**
+ * Decide what `settings` node ServerConfigModal's submit should send for the
+ * custom headers it just collected (#1915).
+ *
+ * Two rules, both load-bearing:
+ *
+ * 1. **Only an edit carries the target's other settings forward.** The
+ * backend replaces the whole node when one is sent, so an edit must
+ * re-send the fields the modal doesn't expose (metadata, timeouts, OAuth
+ * credentials, roots) or they'd be dropped. An **add or clone must not**:
+ * the modal's target in clone mode is the *source* server, so spreading it
+ * would copy that server's OAuth client secret and behavior flags onto a
+ * new entry the user only gave a URL and some headers.
+ * 2. **`undefined` means "don't send the key at all."** Omitting `settings`
+ * is what makes the backend preserve the node it already has, so with no
+ * headers on either side there is nothing to write and nothing to clear.
+ * Clearing the last header still sends — `existing` had headers, so the
+ * node must be rewritten without them.
+ *
+ * Lives here rather than inline in App.tsx so this seam is unit-testable:
+ * App.tsx is outside the coverage gate, and the edit-vs-clone distinction is
+ * exactly where credentials leaked before.
+ *
+ * @param emptySettings the app's blank settings shape, used as the base for an
+ * add or clone.
+ */
+export function buildHeaderSettingsPatch(
+ mode: ServerConfigModalMode,
+ existingSettings: InspectorServerSettings | undefined,
+ headers: KeyValuePair[],
+ emptySettings: InspectorServerSettings,
+): InspectorServerSettings | undefined {
+ const existing = mode === "edit" ? existingSettings : undefined;
+ const changesSomething =
+ headers.length > 0 || (existing?.headers.length ?? 0) > 0;
+ if (!changesSomething) return undefined;
+ return { ...(existing ?? emptySettings), headers };
+}