From ed99617c53619a8d62e2b0b506b826bf1feaac21 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 21:40:07 -0400 Subject: [PATCH] fix(web): lock header rows while submitting, name their clear buttons (#1915) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 review follow-ups on #2038, all three from the suppressed set. The header rows stayed editable while a save was in flight, unlike the rest of the modal — a slow request let a user edit a row whose value had already been captured, and the modal then closed having persisted the earlier one. `KeyValueRows` takes a `disabled` prop and the modal passes `submitting`. Each row's clear buttons were still named the bare "Clear", so a screen reader saw six indistinguishable buttons across three rows. They now name the field they empty ("Clear header name, Cookie"). The App-level headers-into-settings merge — the seam that distinguishes an edit from a clone, and where the credential copy lived — moves into `utils/serverSettingsPatch.ts` with its own tests. App.tsx is outside the coverage gate, so inline it could not be covered at all. Signed-off-by: cliffhall --- clients/web/src/App.tsx | 29 ++---- .../KeyValueRows/KeyValueRows.test.tsx | 34 +++++-- .../elements/KeyValueRows/KeyValueRows.tsx | 21 ++++- .../ServerConfigModal/ServerConfigModal.tsx | 1 + .../ServerSettingsForm.test.tsx | 15 +-- .../web/src/utils/serverSettingsPatch.test.ts | 93 +++++++++++++++++++ clients/web/src/utils/serverSettingsPatch.ts | 42 +++++++++ 7 files changed, 202 insertions(+), 33 deletions(-) create mode 100644 clients/web/src/utils/serverSettingsPatch.test.ts create mode 100644 clients/web/src/utils/serverSettingsPatch.ts 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 }; +}