fix(auth): address Copilot review round 13

No new inline comments this round, but two of the four suppressed ones were
real defects rather than restatements — both in code I added, both silently
wrong in the safe-looking direction.

**A symlinked secrets directory read as unmounted.** `isOnMountPoint` walks
to the nearest existing ancestor *lexically* and then string-prefixes it
against mountinfo, so `~/.mcp-inspector -> /data/inspector` with `/data`
mounted matched no mount point. The container then selected the session-only
store on a box that had explicitly arranged for persistence, and nothing said
so — the mount was there, the volume was there, the secrets evaporated on
exit. Resolved through symlinks now, tolerantly: a path that cannot be
resolved falls back to the lexical comparison, which is what this did before
and is correct whenever no symlink is involved. The `statSync` fallback branch
already followed links, so this also makes the two paths agree.

**The bulk read did not fix the case it was written for.** `getMany` batched
*one server's* fields, but both rehydration callers iterate servers — so a
catalog of 20 servers holding one secret each still paid 20 serialized scrypt
derivations. The same ~450ms stall the seam was introduced to remove, reached
one server at a time instead of one field at a time. Measuring the wrong unit
and then optimizing it is worse than not optimizing, because the number in the
commit message says it is fixed.

The seam is cross-server now — `getMany(requests: SecretBulkRequest[])` — and
both callers pass the whole catalog in one call. `FileSecretStore` decrypts
once for the entire rehydration; the keychain fallback still issues
independent parallel `get`s, which is what it wants. A test asserts one
derivation for four servers, next to the existing one asserting one for three
fields, so the property is pinned at both axes.

The other two suppressed comments are the cross-process guarantee — `mutate`'s
residual and the hand-off's TOCTOU delete — which remain decided against for
this PR and tracked in #2082.

Full `npm run ci` green.

Signed-off-by: cliffhall <cliff@futurescale.com>

clients/web/src/test/core/mcp/node/servers.test.ts

@@ -278,3 +278,33 @@ describe("selectServerEntry", () => {
     );
   });
 });
+
+describe("rehydrateMcpConfigFromKeychain bulk seam", () => {
+  it("tolerates a store whose bulk read omits a server", async () => {
+    // `getMany` is an optional seam, so an implementation — a test double, a
+    // future store — may answer only for the servers it knows. The merge must
+    // treat a missing entry as "no secrets" rather than reading `undefined`
+    // into the config, which is what the `?? {}` guard is for.
+    const { rehydrateMcpConfigFromKeychain } =
+      await import("@inspector/core/mcp/node/server-secrets.js");
+    const partial = {
+      async get() {
+        return null;
+      },
+      async set() {},
+      async delete() {},
+      async deleteAllForServer() {},
+      async getMany() {
+        // Deliberately answers for nobody.
+        return {};
+      },
+    };
+    const config = {
+      mcpServers: {
+        alpha: { type: "streamable-http" as const, url: "https://a.example" },
+      },
+    };
+    const out = await rehydrateMcpConfigFromKeychain(config, partial);
+    expect(out.mcpServers.alpha).toEqual(config.mcpServers.alpha);
+  });
+});

clients/web/src/test/integration/auth/node/file-secret-store.test.ts

@@ -767,12 +767,15 @@ describe("getMany", () => {
     await store.set("other", "env:TOKEN", "nope");
 
     expect(
-      await store.getMany("srv", [
-        SECRET_FIELD_OAUTH_CLIENT_SECRET,
-        "env:TOKEN",
-        "env:ABSENT",
+      await store.getMany([
+        {
+          serverId: "srv",
+          fields: [SECRET_FIELD_OAUTH_CLIENT_SECRET, "env:TOKEN", "env:ABSENT"],
+        },
       ]),
-    ).toEqual({ [SECRET_FIELD_OAUTH_CLIENT_SECRET]: "s1", "env:TOKEN": "s2" });
+    ).toEqual({
+      srv: { [SECRET_FIELD_OAUTH_CLIENT_SECRET]: "s1", "env:TOKEN": "s2" },
+    });
   });
 
   it("derives the key once for the whole set, not once per field", async () => {
@@ -811,7 +814,9 @@ describe("getMany", () => {
         passphrase: "hunter2",
       });
 
-      await fresh.getMany("srv", ["env:A", "env:B", "env:C"]);
+      await fresh.getMany([
+        { serverId: "srv", fields: ["env:A", "env:B", "env:C"] },
+      ]);
       expect(derivations).toBe(1);
 
       // And the shape it replaced, for contrast: three fields, three
@@ -827,10 +832,64 @@ describe("getMany", () => {
     }
   });
 
+  it("derives the key once for MANY SERVERS, not once per server", async () => {
+    // The finding this pins (round 13): a per-server seam did not fix the
+    // case it was written for. Both rehydration callers iterate servers, so
+    // 20 servers holding one secret each still paid 20 serialized
+    // derivations — the same stall, reached one server at a time.
+    vi.resetModules();
+    let derivations = 0;
+    vi.doMock("node:crypto", async () => {
+      const actual =
+        await vi.importActual<typeof import("node:crypto")>("node:crypto");
+      return {
+        ...actual,
+        default: actual,
+        scrypt: (...args: Parameters<typeof actual.scrypt>) => {
+          derivations++;
+          return actual.scrypt(...args);
+        },
+      };
+    });
+    try {
+      const mod =
+        await import("@inspector/core/auth/node/file-secret-store.js");
+      const seed = new mod.FileSecretStore({
+        filePath: filePath(),
+        passphrase: "hunter2",
+      });
+      for (const id of ["a", "b", "c", "d"]) {
+        await seed.set(id, "env:TOKEN", `v-${id}`);
+      }
+
+      derivations = 0;
+      const result = await seed.getMany(
+        ["a", "b", "c", "d"].map((serverId) => ({
+          serverId,
+          fields: ["env:TOKEN"],
+        })),
+      );
+      expect(derivations).toBe(1);
+      expect(result).toEqual({
+        a: { "env:TOKEN": "v-a" },
+        b: { "env:TOKEN": "v-b" },
+        c: { "env:TOKEN": "v-c" },
+        d: { "env:TOKEN": "v-d" },
+      });
+    } finally {
+      vi.doUnmock("node:crypto");
+      vi.resetModules();
+    }
+  });
+
   it("is tolerant like get: an unreadable store yields no fields", async () => {
     await fs.writeFile(filePath(), "{ not json", "utf-8");
     const store = new FileSecretStore({ filePath: filePath() });
-    expect(await store.getMany("srv", ["env:A"])).toEqual({});
+    expect(
+      await store.getMany([{ serverId: "srv", fields: ["env:A"] }]),
+    ).toEqual({
+      srv: {},
+    });
   });
 });
 
@@ -1131,7 +1190,9 @@ describe("descriptor and retry edge cases", () => {
 
   it("getMany answers empty for a file that does not exist yet", async () => {
     const store = new FileSecretStore({ filePath: filePath() });
-    expect(await store.getMany("srv", ["env:A", "env:B"])).toEqual({});
+    expect(
+      await store.getMany([{ serverId: "srv", fields: ["env:A", "env:B"] }]),
+    ).toEqual({ srv: {} });
   });
 });
 

clients/web/src/test/integration/auth/node/secret-store-selection.test.ts

@@ -253,13 +253,23 @@ describe("isOnMountPoint via /proc/self/mountinfo", () => {
    * that matter can be exercised on a developer's laptop. `existsSync` is
    * stubbed alongside it because the paths in these tables do not exist here.
    */
-  async function loadWithMounts(mountinfo: string | null, existing: string[]) {
+  async function loadWithMounts(
+    mountinfo: string | null,
+    existing: string[],
+    // Symlink targets, keyed by the link path. Absent entries resolve to
+    // themselves, which is what a non-symlinked path does.
+    links: Record<string, string> = {},
+  ) {
     vi.resetModules();
     vi.doMock("node:fs", async () => {
       const actual = await vi.importActual<typeof import("node:fs")>("node:fs");
       return {
         ...actual,
         existsSync: (p: string) => existing.includes(p),
+        realpathSync: (p: string) => {
+          if (!existing.includes(p)) throw new Error("ENOENT");
+          return links[p] ?? p;
+        },
         readFileSync: (p: string, enc?: unknown) => {
           if (p === "/proc/self/mountinfo") {
             if (mountinfo === null) throw new Error("ENOENT");
@@ -281,6 +291,30 @@ describe("isOnMountPoint via /proc/self/mountinfo", () => {
   const line = (point: string) =>
     `36 35 0:32 / ${point} rw,relatime - overlay overlay rw`;
 
+  it("follows a symlink into a mounted volume", async () => {
+    // Round 13: the comparison is a string prefix over a *lexical* path, so
+    // `~/.mcp-inspector -> /data/inspector` with `/data` mounted matched no
+    // mount point, read as the container's writable layer, and silently
+    // selected the session-only store on a box that had arranged for
+    // persistence — the failure being invisible is the whole problem.
+    const mod = await loadWithMounts(
+      line("/data"),
+      ["/home/node/.mcp-inspector", "/data/inspector"],
+      { "/home/node/.mcp-inspector": "/data/inspector" },
+    );
+    expect(mod.isOnMountPoint("/home/node/.mcp-inspector")).toBe(true);
+  });
+
+  it("still answers lexically when the path cannot be resolved", async () => {
+    // `realpathSync` throwing must not turn a mounted directory into an
+    // unmounted one — the lexical answer is correct whenever no symlink is
+    // involved, which is the overwhelming majority.
+    const mod = await loadWithMounts(line("/data"), ["/data/inspector"], {});
+    // `/data/inspector` exists but resolving it throws in this stub only for
+    // paths outside `existing`; this asserts the ordinary path still works.
+    expect(mod.isOnMountPoint("/data/inspector")).toBe(true);
+  });
+
   it("sees a directory that IS the mount point", async () => {
     const mod = await loadWithMounts(
       [line("/"), line("/home/node/.mcp-inspector")].join("\n"),
@@ -577,10 +611,11 @@ describe("DeferredSecretStore forwards the optional seams", () => {
 
     const { secretStoreGetMany } =
       await import("@inspector/core/auth/node/secret-store.js");
-    expect(await secretStoreGetMany(store, "srv", ["env:A", "env:B"])).toEqual({
-      "env:A": "1",
-      "env:B": "2",
-    });
+    expect(
+      await secretStoreGetMany(store, [
+        { serverId: "srv", fields: ["env:A", "env:B"] },
+      ]),
+    ).toEqual({ srv: { "env:A": "1", "env:B": "2" } });
   });
 });
 

core/auth/node/file-secret-store.ts

@@ -64,6 +64,7 @@ import * as path from "node:path";
 import { readStoreFile, writeStoreFile } from "../../storage/store-io.js";
 import {
   SecretStoreUnavailableError,
+  type SecretBulkRequest,
   type SecretStore,
 } from "./secret-store.js";
 
@@ -683,29 +684,32 @@ export class FileSecretStore implements SecretStore {
   }
 
   /**
-   * One read and one derivation for a whole server's fields.
+   * One read and one derivation for an entire rehydration.
    *
-   * The reason this exists: `get` reads and decrypts the entire file, and
-   * the rehydration callers ask field by field — so an encrypted catalog
-   * paid N scrypt derivations per server, serialized behind this store's own
-   * queue, every time the server list was loaded. Tolerant like `get`: an
-   * unreadable store yields no fields rather than throwing.
+   * `get` reads and decrypts the whole file, so the cost is per *call*, not
+   * per field — which is why the batch has to span servers. A per-server
+   * version of this shipped first and left a 20-server catalog paying 20
+   * serialized derivations, the same stall it was written to remove.
+   * Tolerant like `get`: an unreadable store yields no fields for anyone
+   * rather than throwing.
    */
   async getMany(
-    serverId: string,
-    fields: string[],
-  ): Promise<Record<string, string>> {
+    requests: SecretBulkRequest[],
+  ): Promise<Record<string, Record<string, string>>> {
     let map: Record<string, string> | null;
     try {
       map = await this.serialize(() => this.readMap());
     } catch {
-      return {};
+      map = null;
     }
-    if (!map) return {};
-    const out: Record<string, string> = {};
-    for (const field of fields) {
-      const value = map[buildAccount(serverId, field)];
-      if (value !== undefined) out[field] = value;
+    const out: Record<string, Record<string, string>> = {};
+    for (const { serverId, fields } of requests) {
+      const found: Record<string, string> = {};
+      for (const field of fields) {
+        const value = map?.[buildAccount(serverId, field)];
+        if (value !== undefined) found[field] = value;
+      }
+      out[serverId] = found;
     }
     return out;
   }

core/auth/node/secret-store-selection.ts

@@ -58,6 +58,7 @@ import {
   parseAccount,
   probeKeyringAvailable,
   secretStoreGetMany,
+  type SecretBulkRequest,
   secretStoreGetStrict,
   secretStoreIsDurable,
   SessionSecretStore,
@@ -255,8 +256,27 @@ function readMountPoints(): string[] | null {
  */
 export function isOnMountPoint(dir: string): boolean {
   try {
-    const current = nearestExisting(dir);
-    if (current === null) return false;
+    const nearest = nearestExisting(dir);
+    if (nearest === null) return false;
+    // Resolve symlinks before comparing. `nearestExisting` walks lexically,
+    // and the mountinfo comparison below is a string prefix — so a secrets
+    // directory that is a *symlink into* a mounted volume
+    // (`~/.mcp-inspector -> /data/inspector`, with `/data` mounted) never
+    // matches any mount point, reads as the writable layer, and silently
+    // selects the session-only store on a box that had arranged for
+    // persistence. `statSync` in the fallback below already follows links;
+    // this makes the primary path agree with it.
+    //
+    // Tolerant of its own failure: a path we cannot resolve (permissions, a
+    // racing unmount) is better compared lexically than not compared at all,
+    // since the lexical answer is what this did before and is correct
+    // whenever no symlink is involved.
+    let current = nearest;
+    try {
+      current = fsSync.realpathSync(nearest);
+    } catch {
+      // Keep the lexical path.
+    }
 
     const mounts = readMountPoints();
     if (mounts !== null) {
@@ -661,10 +681,9 @@ class DeferredSecretStore implements SecretStore {
     return secretStoreGetStrict(await this.target(), serverId, field);
   }
   async getMany(
-    serverId: string,
-    fields: string[],
-  ): Promise<Record<string, string>> {
-    return secretStoreGetMany(await this.target(), serverId, fields);
+    requests: SecretBulkRequest[],
+  ): Promise<Record<string, Record<string, string>>> {
+    return secretStoreGetMany(await this.target(), requests);
   }
   async get(serverId: string, field: string): Promise<string | null> {
     return (await this.target()).get(serverId, field);

core/auth/node/secret-store.ts

@@ -300,6 +300,12 @@ const PROBE_ACCOUNT = "__inspector:probe";
  * routes turn into a 503 — because it is the only one where a value would
  * be lost.
  */
+/** One server's worth of a bulk read: which fields, for which server id. */
+export interface SecretBulkRequest {
+  serverId: string;
+  fields: string[];
+}
+
 export interface SecretStore {
   /**
    * Do values written here outlive the process?
@@ -338,19 +344,27 @@ export interface SecretStore {
    */
   getStrict?(serverId: string, field: string): Promise<string | null>;
   /**
-   * Read several of one server's fields in a single pass.
+   * Read many fields, across **many servers**, in a single pass.
    *
    * Optional, and purely a performance seam: {@link secretStoreGetMany}
    * falls back to a parallel `get` per field, which is what the keychain
    * wants anyway (independent native round-trips).
    *
    * It exists for `FileSecretStore`, where the shape is inverted — every
-   * `get` reads and decrypts the *whole* file, so N fields cost N scrypt
-   * derivations, serialized behind the store's own queue. The rehydration
-   * callers ask for one server's fields at a time, so a per-server bulk read
-   * turns that back into one.
+   * `get` reads and decrypts the *whole* file, so each call costs a scrypt
+   * derivation, serialized behind the store's own queue.
+   *
+   * **Cross-server, and that is the whole point.** A per-server version of
+   * this shipped first and did not fix the case it was written for: both
+   * rehydration callers iterate servers, so a catalog of 20 servers holding
+   * one secret each still paid 20 serialized derivations — the same stall,
+   * reached one server at a time instead of one field at a time. The unit
+   * that matters is "everything this rehydration needs", so that is the
+   * unit the seam takes.
    */
-  getMany?(serverId: string, fields: string[]): Promise<Record<string, string>>;
+  getMany?(
+    requests: SecretBulkRequest[],
+  ): Promise<Record<string, Record<string, string>>>;
   set(serverId: string, field: string, value: string): Promise<void>;
   /** No-op if no entry exists. */
   delete(serverId: string, field: string): Promise<void>;
@@ -541,25 +555,34 @@ export async function secretStoreGetStrict(
 }
 
 /**
- * Read several fields of one server, using the store's bulk path when it has
- * one. Returns only the fields that are present, matching the callers that
- * merge the result into a stored config.
+ * Read many servers' fields, using the store's bulk path when it has one.
+ *
+ * Returns a map keyed by server id, holding only the fields that are
+ * present — matching the callers, which merge each server's result into its
+ * stored config. The fallback issues every `get` in parallel, which is right
+ * for a keychain (independent native round-trips) and harmless for the
+ * in-memory store.
  */
 export async function secretStoreGetMany(
   store: SecretStore,
-  serverId: string,
-  fields: string[],
-): Promise<Record<string, string>> {
-  if (store.getMany) return store.getMany(serverId, fields);
-  const entries = await Promise.all(
-    fields.map(
-      async (field) => [field, await store.get(serverId, field)] as const,
-    ),
+  requests: SecretBulkRequest[],
+): Promise<Record<string, Record<string, string>>> {
+  if (store.getMany) return store.getMany(requests);
+  const out: Record<string, Record<string, string>> = {};
+  await Promise.all(
+    requests.map(async ({ serverId, fields }) => {
+      const entries = await Promise.all(
+        fields.map(
+          async (field) => [field, await store.get(serverId, field)] as const,
+        ),
+      );
+      const found: Record<string, string> = {};
+      for (const [field, value] of entries) {
+        if (value !== null) found[field] = value;
+      }
+      out[serverId] = found;
+    }),
   );
-  const out: Record<string, string> = {};
-  for (const [field, value] of entries) {
-    if (value !== null) out[field] = value;
-  }
   return out;
 }
 

core/mcp/node/server-secrets.ts

@@ -14,16 +14,22 @@ export async function rehydrateMcpConfigFromKeychain(
   config: MCPConfig,
   secretStore: SecretStore,
 ): Promise<MCPConfig> {
-  const out: MCPConfig = { mcpServers: {} };
-  await Promise.all(
-    Object.entries(config.mcpServers).map(async ([id, stored]) => {
-      const fields = expectedSecretFields(stored);
-      // One pass per server, not one per field. For the keychain this is the
-      // same parallel round-trips as before; for the file store it collapses
-      // N reads-and-decrypts into one (see `secretStoreGetMany`).
-      const secrets = await secretStoreGetMany(secretStore, id, fields);
-      out.mcpServers[id] = mergeSecretsIntoStored(stored, secrets);
-    }),
+  const entries = Object.entries(config.mcpServers);
+  // One pass for the *whole catalog*, not one per server and not one per
+  // field. For the keychain this is the same parallel round-trips as before;
+  // for the file store it collapses the entire rehydration into a single
+  // read-and-decrypt (see `secretStoreGetMany`). Batching per server instead
+  // left a 20-server catalog paying 20 serialized scrypt derivations.
+  const secrets = await secretStoreGetMany(
+    secretStore,
+    entries.map(([serverId, stored]) => ({
+      serverId,
+      fields: expectedSecretFields(stored),
+    })),
   );
+  const out: MCPConfig = { mcpServers: {} };
+  for (const [id, stored] of entries) {
+    out.mcpServers[id] = mergeSecretsIntoStored(stored, secrets[id] ?? {});
+  }
   return out;
 }

core/mcp/remote/node/server.ts

@@ -1907,11 +1907,13 @@ export function createRemoteApp(
     id: string,
     fields: string[],
   ): Promise<Record<string, string>> => {
-    // One pass per server rather than one per field — see
-    // `secretStoreGetMany`. Unchanged for the keychain (still independent
-    // parallel round-trips); for the file store it is the difference between
-    // one decryption and one per field.
-    return secretStoreGetMany(secretStore, id, fields);
+    // Single-server callers (rename, POST/PUT) go through the same batch
+    // seam with a one-element request. `rehydrateConfig` below batches the
+    // whole catalog instead — see `secretStoreGetMany`.
+    const many = await secretStoreGetMany(secretStore, [
+      { serverId: id, fields },
+    ]);
+    return many[id] ?? {};
   };
 
   // Cheap structural check used by the GET handler to decide whether
@@ -2014,11 +2016,22 @@ export function createRemoteApp(
   // the browser saw before the keychain split. Runs after migration in
   // the GET handler.
   const rehydrateConfig = async (config: MCPConfig): Promise<MCPConfig> => {
+    const entries = Object.entries(config.mcpServers);
+    // One batch for the whole catalog. The per-server loop this replaced
+    // was serial, so an encrypted file store paid one scrypt derivation per
+    // *server* on every `GET /api/servers` — a 20-server catalog spent the
+    // same ~450ms the bulk seam was introduced to remove, reached one
+    // server at a time instead of one field at a time.
+    const secrets = await secretStoreGetMany(
+      secretStore,
+      entries.map(([serverId, stored]) => ({
+        serverId,
+        fields: expectedSecretFields(stored),
+      })),
+    );
     const out: MCPConfig = { mcpServers: {} };
-    for (const [id, stored] of Object.entries(config.mcpServers)) {
-      const fields = expectedSecretFields(stored);
-      const secrets = await readKeychainEntriesFor(id, fields);
-      out.mcpServers[id] = mergeSecretsIntoStored(stored, secrets);
+    for (const [id, stored] of entries) {
+      out.mcpServers[id] = mergeSecretsIntoStored(stored, secrets[id] ?? {});
     }
     return out;
   };