fix(servers): pass-5 review — absorb late connect rejection, tighten validateSettings, log smuggle warning

Pass-5 review at #1353 (comment).

Main finding: when the connect-time `Promise.race` lost to the timeout,
the original `this.client.connect(this.transport)` promise was left
pending. After `disconnect()` tore the transport down, real transports
(SSE / streamable-http) would reject that promise later — with no
handler attached, producing a Node `unhandledRejection` (and a vitest
suite warning). Attach a no-op `.catch` right after creating the
connect promise so Node sees the late rejection as handled.

Nit (a): `validateSettings` was casting the raw object through to
`InspectorServerSettings`, which let unknown keys ride along onto disk.
Replaced the cast with an explicit pick-and-build over the known
fields. Unknown stowaways now silently drop on the way through. New
route test pins the behavior.

Nit (b): `buildStoredEntry` now logs a `warn` via the route's pino
logger when an incoming `config` carries a `settings` key. The strip
is still defensive (it isn't a documented contract) but the warning
lets a misconfigured client find out about the wasted field instead
of debugging a silent drop.

Nit (c) — the defensive `?? {}` fallback in `buildStoredEntry` —
acknowledged in the review reply; left in place as belt-and-braces.

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,37 @@ describe("/api/servers routes", () => {
       expect(res.status).toBe(400);
     });
 
+    it("validateSettings drops unknown keys (explicit pick-and-build, not spread)", async () => {
+      const res = await fetch(`${h.baseUrl}/api/servers`, {
+        method: "POST",
+        headers: { "Content-Type": "application/json" },
+        body: JSON.stringify({
+          id: "unknown-keys",
+          config: { type: "stdio", command: "node" },
+          settings: {
+            headers: [{ key: "X-A", value: "1" }],
+            metadata: [],
+            connectionTimeout: 0,
+            requestTimeout: 0,
+            // Unknown stowaway — must not survive the validator.
+            stowaway: { keep: "me" },
+          },
+        }),
+      });
+      expect(res.status).toBe(200);
+      const stored = readConfig(h.configPath).mcpServers["unknown-keys"] as {
+        settings?: Record<string, unknown>;
+      };
+      expect(stored.settings).toBeDefined();
+      expect(stored.settings).not.toHaveProperty("stowaway");
+      expect(stored.settings).toEqual({
+        headers: [{ key: "X-A", value: "1" }],
+        metadata: [],
+        connectionTimeout: 0,
+        requestTimeout: 0,
+      });
+    });
+
     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

core/mcp/inspectorClient.ts

@@ -603,6 +603,13 @@ export class InspectorClient extends InspectorClientEventTarget {
       // connect() starts clean and the upstream socket isn't left hanging.
       const connectTimeoutMs = this.serverSettings?.connectionTimeout ?? 0;
       const connectPromise = this.client.connect(this.transport);
+      // Absorb any late rejection from `connectPromise` — when the timeout
+      // wins the race and `disconnect()` tears the transport down, real
+      // transports (SSE / streamable-http) reject the in-flight handshake
+      // *after* Promise.race has already settled. Without a handler here
+      // Node emits an unhandledRejection warning (and in test runs vitest
+      // can surface it as a suite failure).
+      connectPromise.catch(() => {});
       if (connectTimeoutMs > 0) {
         let timer: ReturnType<typeof setTimeout> | undefined;
         const timeoutPromise = new Promise<never>((_, reject) => {

core/mcp/remote/node/server.ts

@@ -667,13 +667,21 @@ export function createRemoteApp(
   // 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.
+  // remains the single write path for the settings node. Log a warning if
+  // we observe this — it indicates a client bug (settings should travel
+  // through the body's top-level `settings` field, not nested in config).
   const buildStoredEntry = (
     config: unknown,
     settings: InspectorServerSettings | undefined,
   ): StoredMCPServer => {
-    const { settings: _smuggled, ...configOnly } =
-      (config as Record<string, unknown>) ?? {};
+    const configObj = (config as Record<string, unknown>) ?? {};
+    if ("settings" in configObj) {
+      fileLogger?.warn(
+        { route: "/api/servers", smuggledKey: "settings" },
+        "Stripping `settings` key from request body's `config` — settings must travel through the top-level `settings` field, not nested inside `config`.",
+      );
+    }
+    const { settings: _smuggled, ...configOnly } = configObj;
     const normalized = normalizeServerType(
       configOnly as Record<string, unknown> & { type?: string },
     ) as StoredMCPServer;
@@ -743,7 +751,26 @@ export function createRemoteApp(
         return { ok: false, error: `settings.${optional} must be a string` };
       }
     }
-    return { ok: true, value: obj as unknown as InspectorServerSettings };
+    // Build the validated value from explicitly named fields rather than
+    // casting the raw object through. Unknown keys silently drop so a
+    // misconfigured client can't smuggle stowaways onto disk, and consumers
+    // can rely on the validated shape being exactly InspectorServerSettings.
+    const value: InspectorServerSettings = {
+      headers: obj.headers as { key: string; value: string }[],
+      metadata: obj.metadata as { key: string; value: string }[],
+      connectionTimeout: obj.connectionTimeout as number,
+      requestTimeout: obj.requestTimeout as number,
+    };
+    if (typeof obj.oauthClientId === "string") {
+      value.oauthClientId = obj.oauthClientId;
+    }
+    if (typeof obj.oauthClientSecret === "string") {
+      value.oauthClientSecret = obj.oauthClientSecret;
+    }
+    if (typeof obj.oauthScopes === "string") {
+      value.oauthScopes = obj.oauthScopes;
+    }
+    return { ok: true, value };
   };
 
   // In-process serialization for the read-modify-write flow on the