From 82ce02b516aee46a3b75173934aaef0b39b85297 Mon Sep 17 00:00:00 2001
From: cliffhall <cliff@futurescale.com>
Date: Sun, 16 Aug 2026 21:14:33 -0400
Subject: [PATCH] fix(web): don't copy a cloned server's other settings (#1915)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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>
---
 clients/web/src/App.tsx                       | 19 ++++++++++++++-----
 .../src/test/core/react/useServers.test.tsx   |  8 +++++---
 core/react/useServers.ts                      |  4 ++--
 3 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx
index e3d2557a..4c6598bf 100644
--- a/clients/web/src/App.tsx
+++ b/clients/web/src/App.tsx
@@ -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
diff --git a/clients/web/src/test/core/react/useServers.test.tsx b/clients/web/src/test/core/react/useServers.test.tsx
index 49b423f2..d66beb4c 100644
--- a/clients/web/src/test/core/react/useServers.test.tsx
+++ b/clients/web/src/test/core/react/useServers.test.tsx
@@ -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 () => {
diff --git a/core/react/useServers.ts b/core/react/useServers.ts
index 69935dd9..f206a0d5 100644
--- a/core/react/useServers.ts
+++ b/core/react/useServers.ts
@@ -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)}`,
         {