fix(web): correct the Custom Headers OAuth hint, name key/value rows for a11y (#2040) by cliffhall · Pull Request #2038 · modelcontextprotocol/inspector

cliffhall · 2026-08-17T00:57:41Z

Closes#2040

Two defects in ServerSettingsForm's key/value editors. No behavior change beyond the copy and the accessible names.

Note on this PR's history. It started as #1915 — custom headers on the manual add form — before it was pointed out that Server Settings already has a Custom Headers editor, making that a second place to edit the same field. The reporter had also conditionally withdrawn #1915 ("editing that information from the server settings dialog is sufficient" once #1848 closed, which it has). The feature commits are reverted; these two fixes are what was genuinely worth keeping from the work, so the PR now closes#2040 instead. #1915 is left for a separate decision.

1. The Custom Headers hint stated the opposite of what happens

The hint told users a custom Authorization header is ignored once OAuth is configured. It is not — it overrides the OAuth access token. Both transports in the installed @modelcontextprotocol/client@2.x build their headers the same way:

if(token)headers["Authorization"]=`Bearer ${token}`;
...
constextraHeaders=normalizeHeaders(this._requestInit?.headers);returnnewHeaders({ ...headers, ...extraHeaders});// custom wins

So a user who set a bearer by hand before configuring OAuth was told the stale value was harmless, while it silently suppressed the token the flow had just obtained — a 401 with no visible cause.

Before	After

2. Key/value rows were indistinguishable to a screen reader

KeyValueRows renders Custom Headers, Request Metadata and Environment Variables, and nothing tied a control to its row: the inputs carried only a placeholder (not a label), every remove button announced as "X", and every clear button as "Clear". Three headers plus two metadata entries meant ten identically-named text boxes and five identical "X" buttons.

Each control now names its entity and row — header value, Cookie, row 1, Remove metadata entry, userId, row 1, Clear header name, Cookie, row 1. The row number is always part of the name rather than a fallback for a blank key, because two rows can share a key (mid-edit, or a duplicate a server persisted), which would otherwise leave them indistinguishable again.

Testing

The affected assertions in ServerSettingsForm.test.tsx and ServerSettingsModal.test.tsx now select rows by accessible name instead of by index — several previously did getAllByRole("button", { name: "X" }) and clicked [0] or [length - 1], which is exactly the ambiguity this fixes. clearButtonFor matches on a /^Clear/ prefix so standalone fields keep the bare label.

npm run ci green from the repo root.

Adding a server by hand offered no way to set custom HTTP headers, so the only route to a cookie or a routing header was importing a config file. The manual "Add server" form now carries a Custom Headers key/value editor for the sse / streamable-http transports, pre-populated when editing or cloning and submitted alongside the config. Headers are not part of `MCPServerConfig` — they live on the entry's `settings` — so they travel as `onSubmit`'s third argument rather than folded into the config, and `addServer` / `updateServer` gained an optional `settings` parameter to carry them to the backend (`POST /api/servers` already accepted one). `updateServer`'s omission semantics are unchanged: with no settings passed, the route still preserves the node on disk. `KeyValueRows` moves out of ServerSettingsForm into a shared element so the two editors cannot drift, and each row's controls gain a row-scoped `aria-label` ("header value, Cookie", "Remove header, Cookie") — previously every remove button announced only "X", indistinguishable across rows. Signed-off-by: cliffhall <cliff@futurescale.com>

Copilot

Pull request overview

Adds custom HTTP headers to the web client’s manual server configuration flow and persists them through shared server settings.

Changes:

Adds header editing for SSE and streamable HTTP servers.
Extracts a reusable, accessible KeyValueRows component.
Expands unit and Storybook coverage for editing and persistence.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
`core/react/useServers.ts`	Adds optional settings to server create/update calls.
`clients/web/src/test/core/react/useServers.test.tsx`	Tests settings persistence.
`clients/web/src/App.tsx`	Connects modal headers to server settings.
`clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.tsx`	Adds the custom-header editor.
`clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.test.tsx`	Tests header form behavior.
`clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.stories.tsx`	Adds header-focused stories.
`clients/web/src/components/elements/KeyValueRows/KeyValueRows.tsx`	Introduces shared key/value row controls.
`clients/web/src/components/elements/KeyValueRows/KeyValueRows.test.tsx`	Tests shared row interactions and labels.
`clients/web/src/components/elements/KeyValueRows/KeyValueRows.stories.tsx`	Documents shared row states.
`clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx`	Reuses the extracted row component.
`clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx`	Updates accessible control queries.
`clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.test.tsx`	Updates row-removal queries.

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

Review follow-ups on #2038. `configModalTarget` is the *source* server in clone mode, so spreading its settings copied that server's OAuth client secret, metadata, roots and behavior flags onto a new entry the user had only given a URL and headers. Only an edit carries the other fields forward now — it has to, since `settings` replaces the node wholesale — while add and clone build from the empty settings shape. Also drops an unnecessary `as unknown as` in the new useServers test (`mcpServers` already holds `StoredMCPServer`, which types `headers`), and removes a `settings: null` instruction the parameter's type does not allow. Signed-off-by: cliffhall <cliff@futurescale.com>

cliffhall · 2026-08-17T01:15:02Z

Round 1 — all three Copilot comments addressed in 82ce02b (mirrored here since inline replies get hidden once the threads go outdated):

Clone copied the source server's settings — real bug, fixed. configModalTarget is the source entry in clone mode, so a source with headers dragged its OAuth client secret, metadata, roots and behavior flags onto the new server. Only an edit carries the other fields forward now (it must — settings replaces the node wholesale); add and clone build from EMPTY_SETTINGS plus the submitted headers. Headers themselves still clone, which is intended and visible in the form before submit.
Unjustified as unknown as in the new test — dropped; mcpServers already holds StoredMCPServer, which types headers.
settings: null documented but not typed — instruction removed. Nothing clears settings through this API today, so widening to | null would type a capability with no caller.

npm run ci green from the repo root after the change.

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Suppressed comments (3)

clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.tsx:509

The header rows remain editable while submitting is true, unlike the rest of this modal. If the request is slow, a user can change or remove a header after Save captured headers; the UI shows that edit, but the earlier payload is persisted and the modal then closes, silently discarding it. Add a disabled state to KeyValueRows and pass submitting here so its inputs, clear controls, and remove controls are locked during submission.

 <KeyValueRows
items={form.headers}
entityLabel="header"
onChange={changeHeader}
onRemove={removeHeader}
/>

clients/web/src/App.tsx:3948

The new App-level settings merge is not covered by the added tests: the modal tests stop at the three-argument callback, while the hook tests begin after a settings object has already been built. This leaves the security-sensitive seam that distinguishes edit from clone unverified—the same seam that previously copied OAuth credentials into clones. Add App tests proving edits preserve non-header settings, clones submit only visible headers with fresh defaults, and removing the final header clears it without losing other settings.

 const settingsChanged =
headers.length > 0 || (existing?.headers.length ?? 0) > 0;
const settings = settingsChanged
? { ...(existing ?? EMPTY_SETTINGS), headers }
: undefined;

clients/web/src/components/elements/KeyValueRows/KeyValueRows.tsx:65

The new row-scoped names do not cover the clear controls: every key/value clear button still has the fixed accessible name Clear, so a screen-reader user sees multiple indistinguishable buttons across these rows. Override each ClearButton label with the entity, field, and rowLabel (for example, Clear header name, Cookie and Clear header value, Cookie).

 <ClearButton
onClick={() => onChange(index, "", item.value)}
/>

…#1915) 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 <cliff@futurescale.com>

cliffhall · 2026-08-17T01:40:32Z

Round 2 — the review generated no new inline comments, but its suppressed block held three worth acting on. All addressed in ed99617:

Header rows stayed editable while submitting — real inconsistency with the rest of the modal, and it could silently discard an edit made after Save captured the payload. KeyValueRows now takes a disabled prop; the modal passes submitting, so the inputs, clear controls and remove controls all lock during submission. Covered by a new test.
Clear buttons still named the bare "Clear" — right, the row-scoped naming stopped short of them, leaving six indistinguishable buttons across three rows. Each now names the field it empties (Clear header name, Cookie). The existing clearButtonFor helper matches on a /^Clear/ prefix so standalone fields keep the bare label.
The App-level merge was untested — fair, and the gap is structural: App.tsx is deliberately outside the coverage include, so nothing inline there can be covered. Rather than start an App test harness, I extracted the seam to utils/serverSettingsPatch.ts (buildHeaderSettingsPatch) with six tests covering exactly the cases named: an edit preserves metadata / OAuth secret / timeouts / roots, a clone builds from the empty shape and copies none of them, clearing the last header still sends, and no headers on either side sends nothing.

npm run ci green. One note on the run: the Storybook step first died on Port 63315 is already in use — that was contention with other suites running concurrently on this machine, not this branch; re-run on its own it passes 482/482.

Copilot

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

clients/web/src/components/elements/KeyValueRows/KeyValueRows.tsx:59

Using only the key as rowLabel does not actually make names row-scoped when two rows have the same key: both key/value inputs and both remove buttons receive identical accessible names. Duplicate rows can exist while editing, and metadata duplicates can also persist, so assistive-technology users still cannot distinguish those controls. Include the row number even when the key is nonblank (for example, Cookie, row 2) and update the affected accessible-name assertions.

 const rowLabel = item.key.trim() || `row ${index + 1}`;

…sition (#1915) Round-3 review follow-ups on #2038. `buildHeaderSettingsPatch` treated the mere presence of stored headers as a change, so saving an id or URL edit re-sent the modal's settings snapshot — taken when it opened — and could overwrite a metadata or OAuth change made in the settings form since. It now compares the submitted headers against the stored ones and omits `settings` when they match, which is what makes the PUT route preserve the node. Order counts as a difference: it is what the form round-trips and what the user sees. Row labels carried the key alone, so two rows sharing a key — mid-edit, or a duplicate a server persisted — gave both rows' controls identical accessible names, the thing the labelling exists to prevent. The row number is now always part of the label rather than a blank-key fallback. Signed-off-by: cliffhall <cliff@futurescale.com>

cliffhall · 2026-08-17T01:57:28Z

Round 3 — one inline comment plus one suppressed, both addressed in d60a5c7:

Presence of stored headers counted as a change (inline) — correct, and the failure mode is a lost write, not just a redundant one: the modal's settings snapshot is taken when it opens, so saving an id/URL edit pushed that snapshot back over any metadata or OAuth change made in the settings form since. buildHeaderSettingsPatch now compares submitted vs. stored headers and omits settings when they match, which is what makes the PUT route preserve the node. A reorder counts as a change — it is what the form round-trips and what the user sees.
Row labels used the key alone (suppressed) — right, two rows sharing a key (mid-edit, or a duplicate a server persisted) produced identical accessible names, defeating the point. The row number is now always part of the label rather than a blank-key fallback: header value, Set-Cookie, row 2. Assertions across the five affected suites updated, plus a new test that two same-key rows are distinguishable.

npm run ci green.

Copilot

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

Suppressed comments (2)

clients/web/src/utils/serverSettingsPatch.ts:3

This pure utility imports both of its shared types from UI components, reversing the repository’s intended dependency direction. KeyValuePair is the persisted key/value domain shape, and ServerConfigModalMode is now shared with non-UI logic; AGENTS.md:728,735 requires shared domain types to live in utils and consumers to depend on that layer, not the reverse. Move/derive these types in the utility layer (and have the components import or re-export them) so utils remains independent of components.

import type { ServerConfigModalMode } from "../components/groups/ServerConfigModal/ServerConfigModal";
import type { KeyValuePair } from "../components/elements/KeyValueRows/KeyValueRows";

clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.tsx:503

This guidance is incorrect for the installed @modelcontextprotocol/client@2.0.0: SSEClientTransport._commonHeaders() adds the OAuth bearer token first and then spreads requestInit.headers, so a custom Authorization value overrides OAuth rather than being ignored. Users can therefore break OAuth while this hint tells them the opposite; warn them to remove the custom header (or change the transport merge behavior) instead.

 <HeadersHint>
Sent with every HTTP request to this server — cookies
included. If OAuth is configured later, the `Authorization`
header is owned by the OAuth flow and any value set here is
ignored.

…1915) Round-4 review follow-ups on #2038. The Custom Headers hint claimed a custom `Authorization` value is "ignored" once OAuth is configured. Verified against the installed @modelcontextprotocol/client: both SSE and streamable-HTTP `_commonHeaders()` set the bearer first and then spread `requestInit.headers`, so a custom value *overrides* the OAuth token. The hint told users the opposite of what would happen, and could have them silently break their own auth. Corrected in both places — the modal's new hint and the pre-existing one in ServerSettingsForm, which carried the same wrong claim. `utils/serverSettingsPatch.ts` imported both its shared types from components, reversing the one-way `components -> utils` direction AGENTS.md sets. `KeyValuePair` moves to `utils/keyValuePairs.ts` (KeyValueRows re-exports it), and the mode is spelled as a literal union — still checked, since `ServerConfigModalMode` must stay assignable to it at the call site. Signed-off-by: cliffhall <cliff@futurescale.com>

cliffhall · 2026-08-17T02:14:56Z

Round 4 — no new inline comments; both suppressed ones were right and are fixed in 51d35d8:

The OAuth hint was factually backwards. Verified against the installed @modelcontextprotocol/client: in both SSEClientTransport._commonHeaders() and the streamable-HTTP one, the bearer is set first and requestInit.headers is spread after it, so a custom Authorizationoverrides the OAuth token rather than being ignored. The hint was telling users the opposite of what happens, which could have them silently break their own auth. Corrected in the modal, and in the pre-existing ServerSettingsForm hint that carried the same wrong claim — leaving it there would have kept the falsehood in the place users are most likely to read it.
utils importing from components — correct, that reverses the one-way direction AGENTS.md sets. KeyValuePair now lives in utils/keyValuePairs.ts and KeyValueRows re-exports it for existing call sites. For the mode I spelled a literal union in the helper rather than moving ServerConfigModalMode out of the modal, since the mode is genuinely a UI concept; it stays honest because ServerConfigModalMode has to remain assignable to it at the call site, so adding a fourth mode is a compile error rather than a silent divergence.

npm run ci green.

Copilot

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Server Settings already has a Custom Headers editor, so adding one to the manual add form created a second place to edit the same field. The reporter had also conditionally withdrawn #1915 — "editing that information from the server settings dialog is sufficient" once #1848 closed, which it has. Reverts the add-form editor, the shared KeyValueRows element, the optional `settings` argument on addServer / updateServer, and the App-level merge helper. What remains are two defects found while working in ServerSettingsForm, which this PR now closes instead (#2040): the Custom Headers hint misstating OAuth precedence, and the key/value rows' missing accessible names. Signed-off-by: cliffhall <cliff@futurescale.com>

cliffhall · 2026-08-17T02:58:16Z

Scope change — this PR no longer adds custom headers to the manual add form.

Server Settings already has a Custom Headers editor, so the add-form version was a second place to edit the same field. (My own comment in ServerConfigModal said as much — "custom headers live in ServerSettingsForm now" — a deliberate earlier decision I reversed without noticing.) The reporter had also conditionally withdrawn #1915: "editing that information from the server settings dialog is sufficient" once #1848 closed, which it has.

Reverted in 4dd8ed4: the add-form editor, the shared KeyValueRows element, the optional settings argument on addServer / updateServer, and the App-level merge helper. KeyValueRows is back to a local component in ServerSettingsForm, since with one consumer the extraction had no drift to prevent.

What remains are the two defects found along the way, which now stand on their own as #2040 — the OAuth precedence hint being backwards, and the key/value rows having no per-row accessible names. Diff is three files. Title and description rewritten; earlier review rounds on this PR mostly concerned code that is no longer here.

#1915 is left open for a separate decision.

cliffhall · 2026-08-17T03:08:01Z

Correction to my scope-change comment above. I implied #1848 was what made Custom Headers reachable. That is wrong, and the real timeline matters for whoever reads #1915 later:

2026-03-17, 39ef184b — the Custom Headers section shipped in the firstServerSettingsForm, in the initial Mantine component build-out.
2026-04-21, 771a67ee — InspectorServerSettings.headers declared as the pair-array during the Phase-2 SDK-types refactor.
2026-05-24, 85a4b4c0 (Persist per-server settings (headers, metadata, timeouts, OAuth credentials) to mcp.json #1352 / feat(servers): persist per-server settings to mcp.json (#1352) #1353) — headers became functional end to end: persisted to mcp.json and fed to the transport via headersFromSettings.

So the feature has worked since late May, roughly ten weeks before #1915 was filed. #1848 is the KeyringSecretStore constructor bug that 500-ed GET /api/servers — the cause of the #1914 / #1918 ghost-entry symptom, which is what kept the reporter from reaching the settings dialog. Their comment was about regaining access to an existing feature, not about the feature arriving.

cliffhall added the v2 Issues and PRs for v2 label Aug 17, 2026

cliffhall requested a balanced review from Copilot August 17, 2026 00:58

Copilotstarted reviewing on behalf of cliffhall August 17, 2026 00:58 View session