fix(web): don't copy a cloned server's other settings (#1915)

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>
This commit is contained in:
cliffhall 2026-08-16 21:14:33 -04:00
commit 82ce02b516
3 changed files with 21 additions and 10 deletions

View file

@ -3927,11 +3927,20 @@ 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). Send `settings` only when it
// would change something: with no headers either way there is nothing to
// write, and omitting the key is what makes the backend preserve the
// rest of the settings node.
const existing = configModalTarget?.settings;
// 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

View file

@ -154,9 +154,11 @@ describe("useServers", () => {
{ key: "Cookie", value: "branch=feature-x" },
]);
});
const stored = readConfig(h.configPath).mcpServers
.remote as unknown as Record<string, unknown>;
expect(stored.headers).toEqual({ Cookie: "branch=feature-x" });
// On disk headers are the flat `Record<string, string>` form, a direct key
// on the entry (post-#1358) rather than a nested settings node.
expect(readConfig(h.configPath).mcpServers.remote?.headers).toEqual({
Cookie: "branch=feature-x",
});
});
it("updateServer replaces the settings node when one is supplied (#1915)", async () => {

View file

@ -265,8 +265,8 @@ export function useServers(opts: UseServersOptions): UseServersResult {
// settings node on disk" — so a config-only save cannot silently wipe
// persisted headers / metadata / OAuth credentials. A caller editing a
// field that lives on settings (e.g. ServerConfigModal's custom headers)
// passes the full settings object it wants written. To explicitly clear
// settings, send `settings: null`.
// passes the full settings object it wants written, which replaces the
// node wholesale — so it must carry forward the fields it isn't editing.
const res = await doFetch(
`${base}/api/servers/${encodeURIComponent(originalId)}`,
{