diff --git a/clients/web/src/test/core/mcp/node/servers.test.ts b/clients/web/src/test/core/mcp/node/servers.test.ts index d0e3751d..05373af5 100644 --- a/clients/web/src/test/core/mcp/node/servers.test.ts +++ b/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); + }); +}); diff --git a/clients/web/src/test/integration/auth/node/file-secret-store.test.ts b/clients/web/src/test/integration/auth/node/file-secret-store.test.ts index 0df11689..e9081852 100644 --- a/clients/web/src/test/integration/auth/node/file-secret-store.test.ts +++ b/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("node:crypto"); + return { + ...actual, + default: actual, + scrypt: (...args: Parameters) => { + 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: {} }); }); }); diff --git a/clients/web/src/test/integration/auth/node/secret-store-selection.test.ts b/clients/web/src/test/integration/auth/node/secret-store-selection.test.ts index 56c901e8..6f4c8b1c 100644 --- a/clients/web/src/test/integration/auth/node/secret-store-selection.test.ts +++ b/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 = {}, + ) { vi.resetModules(); vi.doMock("node:fs", async () => { const actual = await vi.importActual("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" } }); }); }); diff --git a/core/auth/node/file-secret-store.ts b/core/auth/node/file-secret-store.ts index dbfadae0..95994b72 100644 --- a/core/auth/node/file-secret-store.ts +++ b/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> { + requests: SecretBulkRequest[], + ): Promise>> { let map: Record | null; try { map = await this.serialize(() => this.readMap()); } catch { - return {}; + map = null; } - if (!map) return {}; - const out: Record = {}; - for (const field of fields) { - const value = map[buildAccount(serverId, field)]; - if (value !== undefined) out[field] = value; + const out: Record> = {}; + for (const { serverId, fields } of requests) { + const found: Record = {}; + for (const field of fields) { + const value = map?.[buildAccount(serverId, field)]; + if (value !== undefined) found[field] = value; + } + out[serverId] = found; } return out; } diff --git a/core/auth/node/secret-store-selection.ts b/core/auth/node/secret-store-selection.ts index 3a4ab124..3df356eb 100644 --- a/core/auth/node/secret-store-selection.ts +++ b/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> { - return secretStoreGetMany(await this.target(), serverId, fields); + requests: SecretBulkRequest[], + ): Promise>> { + return secretStoreGetMany(await this.target(), requests); } async get(serverId: string, field: string): Promise { return (await this.target()).get(serverId, field); diff --git a/core/auth/node/secret-store.ts b/core/auth/node/secret-store.ts index 88fff072..611418dd 100644 --- a/core/auth/node/secret-store.ts +++ b/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; /** - * 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>; + getMany?( + requests: SecretBulkRequest[], + ): Promise>>; set(serverId: string, field: string, value: string): Promise; /** No-op if no entry exists. */ delete(serverId: string, field: string): Promise; @@ -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> { - 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>> { + if (store.getMany) return store.getMany(requests); + const out: Record> = {}; + 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 = {}; + for (const [field, value] of entries) { + if (value !== null) found[field] = value; + } + out[serverId] = found; + }), ); - const out: Record = {}; - for (const [field, value] of entries) { - if (value !== null) out[field] = value; - } return out; } diff --git a/core/mcp/node/server-secrets.ts b/core/mcp/node/server-secrets.ts index a35e558c..0658aa52 100644 --- a/core/mcp/node/server-secrets.ts +++ b/core/mcp/node/server-secrets.ts @@ -14,16 +14,22 @@ export async function rehydrateMcpConfigFromKeychain( config: MCPConfig, secretStore: SecretStore, ): Promise { - 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; } diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index 45453f98..10403aa4 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -1907,11 +1907,13 @@ export function createRemoteApp( id: string, fields: string[], ): Promise> => { - // 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 => { + 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; };