fix(servers): pass-4 review — strip smuggled settings out of config

Pass-4 review at #1353 (comment).

Main finding: `normalizeServerType` spreads unknown keys, so a body
that nested `settings` inside `config` would smuggle a settings field
onto the stored entry without ever passing through `validateSettings`.
Strip the settings key off the incoming config in `buildStoredEntry`
so `validateSettings` remains the single write path for the settings
node — the "one source of truth" invariant pass 2 set up holds again.

Two new tests pin the strip on both routes:
  - POST with `config.settings = { bogus }` lands an entry with no
    settings node on disk.
  - PUT with `config.settings = { bogus }` + `settings: null` clears
    the real settings *and* doesn't re-attach the bogus payload via
    the spread.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: cliffhall

clients/web/src/test/integration/mcp/remote/servers-route.test.ts

@@ -605,6 +605,69 @@ describe("/api/servers routes", () => {
       expect(res.status).toBe(400);
     });
 
+    it("strips smuggled config.settings on POST (validateSettings is the only write path)", async () => {
+      // `normalizeServerType` spreads unknown keys verbatim. Without the
+      // strip in buildStoredEntry, a body that nests `settings` inside
+      // `config` would land it on the stored entry without ever passing
+      // through `validateSettings`. Pin the strip.
+      const res = await fetch(`${h.baseUrl}/api/servers`, {
+        method: "POST",
+        headers: { "Content-Type": "application/json" },
+        body: JSON.stringify({
+          id: "smuggle-post",
+          config: {
+            type: "stdio",
+            command: "node",
+            settings: { bogus: true },
+          },
+        }),
+      });
+      expect(res.status).toBe(200);
+      const stored = readConfig(h.configPath).mcpServers["smuggle-post"] as {
+        settings?: unknown;
+      };
+      expect(stored.settings).toBeUndefined();
+    });
+
+    it("strips smuggled config.settings on PUT even when settings:null clears the real node", async () => {
+      writeFileSync(
+        h.configPath,
+        JSON.stringify({
+          mcpServers: {
+            smuggle: {
+              type: "stdio",
+              command: "node",
+              settings: {
+                headers: [{ key: "X-Real", value: "yes" }],
+                metadata: [],
+                connectionTimeout: 0,
+                requestTimeout: 0,
+              },
+            },
+          },
+        }),
+      );
+      // settings: null clears the real settings node; the bogus key nested
+      // under config must not re-attach via the spread inside normalizeServerType.
+      const res = await fetch(`${h.baseUrl}/api/servers/smuggle`, {
+        method: "PUT",
+        headers: { "Content-Type": "application/json" },
+        body: JSON.stringify({
+          config: {
+            type: "stdio",
+            command: "node",
+            settings: { bogus: true },
+          },
+          settings: null,
+        }),
+      });
+      expect(res.status).toBe(200);
+      const stored = readConfig(h.configPath).mcpServers.smuggle as {
+        settings?: unknown;
+      };
+      expect(stored.settings).toBeUndefined();
+    });
+
     it("rejects a settings array (not an object) with 400", async () => {
       const res = await fetch(`${h.baseUrl}/api/servers`, {
         method: "POST",

core/mcp/remote/node/server.ts

@@ -662,12 +662,20 @@ export function createRemoteApp(
 
   // Build a single on-disk entry from `{ config, settings }`, normalizing the
   // type discriminator and attaching settings only when defined.
+  //
+  // `normalizeServerType` spreads unknown keys from the incoming config
+  // through verbatim, so a caller that included `config.settings` on the
+  // wire would smuggle a `settings` field straight onto the stored entry —
+  // bypassing `validateSettings`. Strip it here so `validateSettings`
+  // remains the single write path for the settings node.
   const buildStoredEntry = (
     config: unknown,
     settings: InspectorServerSettings | undefined,
   ): StoredMCPServer => {
+    const { settings: _smuggled, ...configOnly } =
+      (config as Record<string, unknown>) ?? {};
     const normalized = normalizeServerType(
-      config as Record<string, unknown> & { type?: string },
+      configOnly as Record<string, unknown> & { type?: string },
     ) as StoredMCPServer;
     if (settings !== undefined) {
       normalized.settings = settings;