diff --git a/clients/web/src/test/core/client/node-persistence.test.ts b/clients/web/src/test/core/client/node-persistence.test.ts index 8d4233f8..00ed8461 100644 --- a/clients/web/src/test/core/client/node-persistence.test.ts +++ b/clients/web/src/test/core/client/node-persistence.test.ts @@ -224,3 +224,60 @@ describe("client node-persistence", () => { ).toBeNull(); }); }); + +describe("session-scoped store keeps client.json durable (#1950 review r19)", () => { + it("does not strip the IdP secret when the store cannot outlive the process", async () => { + // The read-path migration already withheld its strip for a session + // store, but the write path did not — and `readClientConfigStore` hands + // the rehydrated secret to the form, which resends the whole object when + // an unrelated field changes. Saving a CIMD URL therefore moved the only + // durable copy into RAM, to be lost at exit. + const dir = await fs.mkdtemp(path.join(os.tmpdir(), "client-durable-")); + const file = path.join(dir, "client.json"); + try { + await writeClientConfigStore( + file, + { + enterpriseManagedAuth: { + enabled: true, + idp: { + issuer: "https://idp.example/", + clientId: "cid", + clientSecret: "must-survive", + }, + }, + }, + new SessionSecretStore(), + ); + expect(await fs.readFile(file, "utf-8")).toContain("must-survive"); + } finally { + await fs.rm(dir, { recursive: true, force: true }); + } + }); + + it("still strips against a durable store, so the guard is the difference", async () => { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), "client-durable-")); + const file = path.join(dir, "client.json"); + try { + await writeClientConfigStore( + file, + { + enterpriseManagedAuth: { + enabled: true, + idp: { + issuer: "https://idp.example/", + clientId: "cid", + clientSecret: "goes-to-the-store", + }, + }, + }, + new InMemorySecretStore(), + ); + expect(await fs.readFile(file, "utf-8")).not.toContain( + "goes-to-the-store", + ); + } finally { + await fs.rm(dir, { recursive: true, force: true }); + } + }); +}); 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 cb92060b..4c67e8a8 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 @@ -10,6 +10,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import * as crypto from "node:crypto"; import * as fs from "node:fs/promises"; +import { existsSync as existsSyncFile } from "node:fs"; import * as os from "node:os"; import * as path from "node:path"; import { @@ -914,6 +915,63 @@ describe("getMany", () => { } }); + it("setMany derives the key once, not three times per field", async () => { + // `set` is a read-decrypt-encrypt-write-verify cycle per call — roughly + // three derivations each — and the settings form resends a server's full + // env map on any edit, so a 10-variable server paid ~30 to save an + // unrelated timeout change. + 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 store = new mod.FileSecretStore({ + filePath: filePath(), + passphrase: "hunter2", + }); + await store.set("srv", "env:SEED", "0"); + + derivations = 0; + await store.setMany("srv", { + "env:A": "1", + "env:B": "2", + "env:C": "3", + }); + const bulk = derivations; + + derivations = 0; + await store.set("srv", "env:D", "4"); + const single = derivations; + + // Three fields in one cycle must not cost more than one field does. + expect(bulk).toBeLessThanOrEqual(single); + expect(await store.get("srv", "env:A")).toBe("1"); + expect(await store.get("srv", "env:C")).toBe("3"); + expect(await store.get("srv", "env:SEED")).toBe("0"); + } finally { + vi.doUnmock("node:crypto"); + vi.resetModules(); + } + }); + + it("setMany with nothing to write does not touch the file", async () => { + const store = new FileSecretStore({ filePath: filePath() }); + await store.setMany("srv", {}); + expect(existsSyncFile(filePath())).toBe(false); + }); + 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() }); 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 e4158986..f93b064d 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 @@ -857,23 +857,26 @@ describe("absorbFileSecretsIntoKeyring", () => { ); }); - it("declines to delete a file that changed while the hand-off ran", async () => { - // Read → copy → delete is no longer one transaction: the store dropped - // its cross-process lock in favour of optimistic writes, so another - // Inspector can complete a `set` between the read and the delete. That - // entry has not been copied, so deleting the file would discard a write - // that reported success. The hand-off re-reads immediately before the - // delete and backs off when the contents moved. + it("claims the file atomically, so a later write is not deleted", async () => { + // Round 19: the pre-delete comparison was a check-to-delete race. A + // writer completing a `set` *after* the comparison and before the `rm` + // had its write verified, reported success, and then lost it — a later, + // already-successful write destroyed, which is worse than the + // optimistic-write residual and, unlike that one, fixable. + // + // The file is now claimed by an atomic rename before anything reads it, + // so a writer that recreates `secrets.json` afterwards is untouched: its + // file is a different one and the next run migrates it. const filePath = await seedFile({ "srv:oauthClientSecret": "from-file" }); process.env.MCP_INSPECTOR_SECRET_FILE = filePath; - const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + vi.spyOn(console, "warn").mockImplementation(() => {}); const mod = await loadWithProbe(true); - // Stand in for the other Inspector: write to the file *during* the copy, - // which is exactly the window the old lock covered. const keyring = new InMemorySecretStore(); const { FileSecretStore } = await import("@inspector/core/auth/node/file-secret-store.js"); + // Stand in for the other Inspector: write to the live path *during* the + // copy — i.e. after the claim, in the window the old comparison missed. const original = keyring.set.bind(keyring); let intruded = false; keyring.set = async (id: string, field: string, value: string) => { @@ -886,14 +889,18 @@ describe("absorbFileSecretsIntoKeyring", () => { await mod.absorbFileSecretsIntoKeyring(keyring); - // The file survives, and so does the entry that arrived late. + // The late write survives at the live path... expect(existsSync(filePath)).toBe(true); expect( await new FileSecretStore({ filePath }).get("other", "env:LATE"), ).toBe("z"); - expect(warn.mock.calls.flat().join("\n")).toMatch( - /changed while that ran, so it has been left in place/, + // ...the migrated values reached the keychain... + expect(await keyring.get("srv", "oauthClientSecret")).toBe("from-file"); + // ...and no staging file is left behind. + const leftovers = (await fs.readdir(path.dirname(filePath))).filter((f) => + f.includes(".migrating-"), ); + expect(leftovers).toEqual([]); }); it("deletes the file when nothing changed under it", async () => { diff --git a/clients/web/src/test/integration/mcp/remote/servers-route.test.ts b/clients/web/src/test/integration/mcp/remote/servers-route.test.ts index cef80f28..8d5dc9d6 100644 --- a/clients/web/src/test/integration/mcp/remote/servers-route.test.ts +++ b/clients/web/src/test/integration/mcp/remote/servers-route.test.ts @@ -2727,6 +2727,63 @@ describe("plaintext migration against a session-scoped store (#1950)", () => { expect(saidSessionWarning()).toHaveLength(1); }); + it("keeps the plaintext on disk through a later unrelated settings edit", async () => { + // The gap round 19 found. The GET migration withheld the strip for a + // session store, but the *write* paths did not — and the GET returns the + // rehydrated secret to the settings form, which resends the whole object + // on any edit. So changing a timeout wrote the stripped shape, moving the + // only durable copy into RAM to be lost at exit. The user changed a + // timeout and lost a client secret, with everything reporting success. + const { app } = createRemoteApp({ + dangerouslyOmitAuth: true, + mcpConfigPath: configPath, + initialConfig: { defaultEnvironment: {} }, + secretStore: new SessionSecretStore(), + }); + + // Read it the way the UI does — this is what fills the form. + const listed = (await ( + await app.request(new Request("http://test/api/servers")) + ).json()) as MCPConfig; + expect( + (listed.mcpServers.srv as { oauth?: { clientSecret?: string } }).oauth + ?.clientSecret, + ).toBe("must-survive"); + + // Now save an unrelated change, resending the settings verbatim. + const res = await app.request( + new Request("http://test/api/servers/srv", { + method: "PUT", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + id: "srv", + config: { type: "streamable-http", url: "https://x.test/mcp" }, + settings: { + headers: [], + env: [], + metadata: [], + connectionTimeout: 45000, + requestTimeout: 60000, + taskTtl: 60000, + maxFetchRequests: 1000, + roots: [], + oauthClientId: "cid", + oauthClientSecret: "must-survive", + }, + }), + }), + ); + expect(res.status).toBe(200); + + // The durable copy is still on disk. Exiting now loses nothing. + const onDisk = JSON.parse(readFileSync(configPath, "utf-8")) as { + mcpServers: Record; + }; + expect(onDisk.mcpServers.srv?.oauth?.clientSecret).toBe("must-survive"); + // And the unrelated edit did land. + expect(readFileSync(configPath, "utf-8")).toContain("45000"); + }); + it("aborts the migration when the keychain read fails, preserving mcp.json", async () => { // The keychain-wins lookup used the tolerant `get`, so an unreadable // keychain read as "nothing there" — and the write that followed would diff --git a/core/auth/node/file-secret-store.ts b/core/auth/node/file-secret-store.ts index 8e450003..ecc04a6d 100644 --- a/core/auth/node/file-secret-store.ts +++ b/core/auth/node/file-secret-store.ts @@ -321,9 +321,13 @@ export class FileSecretStore implements SecretStore { * existing secrets sat in the clear — the precise reassurance this whole * subsystem exists not to give. * - * Reads the envelope only. It never decrypts, so it needs no passphrase - * and answers correctly even when the key is wrong or missing — which is - * also a state the UI has to describe rather than throw on. + * Reads the envelope, and for an encrypted one **attempts to open it**. + * Structure alone is not access: a well-formed file this passphrase cannot + * decrypt answers `null` from every `get` and refuses every `set`, so + * reporting it as a healthy "encrypted" store hands the quiet footer to + * exactly the state the unreadable one exists to warn about. The + * authentication costs one scrypt derivation per call, which is once per + * `/api/config`, and the decrypted bytes are discarded. * * `absent` and `unreadable` are kept apart deliberately. Collapsing them * (as a bare `null` did) means an existing file that is corrupt, or @@ -783,6 +787,30 @@ export class FileSecretStore implements SecretStore { } } + /** + * One read-modify-write-verify cycle for a whole server's fields. + * + * `set` pays that cycle — and, encrypted, three scrypt derivations — per + * call, serialized behind this store's queue. The settings form resends a + * server's full `env` map on any edit, so a 10-variable stdio server spent + * ~30 derivations saving an unrelated timeout change. + */ + async setMany( + serverId: string, + values: Record, + ): Promise { + if (Object.keys(values).length === 0) return; + await this.serialize(() => + this.mutate((map) => { + const next = { ...map }; + for (const [field, value] of Object.entries(values)) { + next[buildAccount(serverId, field)] = value; + } + return next; + }), + ); + } + async set(serverId: string, field: string, value: string): Promise { await this.serialize(() => this.mutate((map) => ({ diff --git a/core/auth/node/secret-store-selection.ts b/core/auth/node/secret-store-selection.ts index 1faaef79..bda19a36 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, + secretStoreSetMany, type SecretBulkRequest, secretStoreGetStrict, secretStoreIsDurable, @@ -531,8 +532,64 @@ export async function absorbFileSecretsIntoKeyring( const filePath = defaultSecretFilePath(); if (!fsSync.existsSync(filePath)) return; - const file = new FileSecretStore({ filePath }); - await handOff(file, filePath, keyring); + // **Claim the file atomically before reading it.** Comparing its contents + // immediately before `rm` narrowed the window and could not close it: a + // writer that completes a `set` after the comparison and before the delete + // has its write verified, reports success, and then loses it — a *later* + // successful write destroyed, which is worse than the optimistic-write + // residual and, unlike that one, fixable here. + // + // `rename` is atomic and leaves the live path free. Everything after this + // point operates on a snapshot nobody else can reach, and a writer that + // recreates `secrets.json` in the meantime is simply untouched — its file + // is a different one, and the next run migrates it. + // + // The staged name carries the pid so two Inspectors starting together + // cannot claim the same destination; whichever wins the rename does the + // migration and the loser sees ENOENT and returns. + const staged = `${filePath}.migrating-${process.pid}`; + try { + await fs.rename(filePath, staged); + } catch { + // Someone else claimed it first, or it vanished. Either way there is + // nothing here to migrate and no file of ours to clean up. + return; + } + + const file = new FileSecretStore({ filePath: staged }); + // True only when every value reached the keychain *and* there was + // something to move — the one case where the snapshot is redundant. + const absorbed = await handOff(file, staged, keyring); + + if (absorbed) { + try { + await fs.rm(staged, { force: true }); + console.warn( + `\n[mcp-inspector] The OS keychain is available again. Secrets kept in ${filePath} while it was not have been moved into the keychain, and the file has been removed.`, + ); + } catch { + // Every value is in the keychain; only the now-redundant snapshot + // remains. Not worth failing startup over, and keychain-wins makes the + // next attempt harmless. + } + return; + } + + // Not fully migrated: put the snapshot back so the next run finds it. + // Claiming the file is what makes the delete safe; it must not also make a + // *failed* migration lose the file, which is what the claim exists to + // prevent one line further down. + try { + await fs.rename(staged, filePath); + } catch { + // The live path is occupied — a writer recreated it while we held the + // snapshot — so restoring would overwrite a newer file. Leave the + // snapshot and say exactly where it is: an operator-visible orphan is + // recoverable, and silently clobbering someone's write is not. + console.warn( + `\n[mcp-inspector] Could not restore ${staged} to ${filePath} because a new secrets file was written while the migration ran. Both files exist; the snapshot holds the secrets that were not migrated.`, + ); + } } /** @@ -556,7 +613,7 @@ async function handOff( file: FileSecretStore, filePath: string, keyring: SecretStore, -): Promise { +): Promise { let entries: Record | null; try { entries = await file.readAll(); @@ -564,9 +621,13 @@ async function handOff( console.warn( `\n[mcp-inspector] The OS keychain is available again, but the secrets file at ${filePath} could not be read, so its contents were left there: ${err instanceof Error ? err.message : String(err)}`, ); - return; + return false; } - if (!entries || Object.keys(entries).length === 0) return; + // Nothing to migrate. Report "do not delete" rather than "done": we moved + // no values out of this file, and its presence is what records which + // encryption mode this install settled on, so removing it would change + // that on the user's behalf for no gain. + if (!entries || Object.keys(entries).length === 0) return false; // A key that is not `serverId:field` cannot be addressed through the store // API, so it cannot be copied — which makes the hand-off incomplete by @@ -600,65 +661,16 @@ async function handOff( console.warn( `\n[mcp-inspector] Could not move the secrets file at ${filePath} into the OS keychain, so it has been left in place: ${err instanceof Error ? err.message : String(err)}`, ); - return; + return false; } if (!complete) { console.warn( `\n[mcp-inspector] Secrets from ${filePath} have been copied into the OS keychain, but at least one entry could not be read (its key is not in \`serverId:field\` form). The file has been left in place so nothing is lost.`, ); - return; + return false; } - - // Re-read immediately before deleting. Everything above is now in the - // keychain, but the file is written without a lock, so another Inspector - // may have added an entry since — and that entry has not been copied. - // Deleting on a stale read is how a `set` that reported success loses its - // value. - let latest: Record | null; - try { - latest = await file.readAll(); - } catch { - // Unreadable now, readable a moment ago: something changed it in a way - // this passphrase cannot open. Leave it — the copies are safe in the - // keychain and the file is the only record of whatever arrived. - console.warn( - `\n[mcp-inspector] Secrets from ${filePath} were copied into the OS keychain, but the file changed while that ran and can no longer be read, so it has been left in place.`, - ); - return; - } - if (latest && !sameSecrets(entries, latest)) { - // Leave it for the next run rather than looping here: leaving the file - // costs nothing (keychain-wins makes the retry idempotent) while - // deleting it on a stale comparison costs a secret. - console.warn( - `\n[mcp-inspector] Secrets from ${filePath} were copied into the OS keychain, but the file changed while that ran, so it has been left in place and will be moved on the next run.`, - ); - return; - } - - try { - await fs.rm(filePath, { force: true }); - console.warn( - `\n[mcp-inspector] The OS keychain is available again. Secrets kept in ${filePath} while it was not have been moved into the keychain, and the file has been removed.`, - ); - } catch { - // Every value is in the keychain; only the now-redundant file remains. - // Not worth failing startup over, and the keychain-wins rule makes the - // next attempt harmless. - } -} - -/** Do two secret maps hold exactly the same entries? */ -function sameSecrets( - a: Record, - b: Record, -): boolean { - const ak = Object.keys(a); - if (ak.length !== Object.keys(b).length) return false; - return ak.every( - (k) => Object.prototype.hasOwnProperty.call(b, k) && a[k] === b[k], - ); + return true; } /** @@ -723,6 +735,12 @@ class DeferredSecretStore implements SecretStore { async set(serverId: string, field: string, value: string): Promise { await (await this.target()).set(serverId, field, value); } + async setMany( + serverId: string, + values: Record, + ): Promise { + await secretStoreSetMany(await this.target(), serverId, values); + } async delete(serverId: string, field: string): Promise { await (await this.target()).delete(serverId, field); } diff --git a/core/auth/node/secret-store.ts b/core/auth/node/secret-store.ts index f80d2f7a..49612b49 100644 --- a/core/auth/node/secret-store.ts +++ b/core/auth/node/secret-store.ts @@ -366,6 +366,21 @@ export interface SecretStore { requests: SecretBulkRequest[], ): Promise>>; set(serverId: string, field: string, value: string): Promise; + /** + * Write several of one server's fields in a single pass. + * + * The write-side twin of {@link getMany}, and the same shape of problem: + * `FileSecretStore.set` costs a whole-file read, decrypt, encrypt, write + * and verifying read *per call*, all serialized. A settings save resends + * the server's full `env` map even when the user edited something + * unrelated, so an entry with N secret fields paid roughly `3N` scrypt + * derivations on every debounced save — a stall that grows with the + * environment rather than with the edit. + * + * Optional: {@link secretStoreSetMany} falls back to parallel `set` calls, + * which is what the keychain wants (independent native round-trips). + */ + setMany?(serverId: string, values: Record): Promise; /** No-op if no entry exists. */ delete(serverId: string, field: string): Promise; /** Remove every secret stored for this server id (called on DELETE /api/servers/:id). */ @@ -593,6 +608,26 @@ export async function secretStoreGetMany( return out; } +/** + * Write several of one server's fields, using the store's bulk path when it + * has one. Falls back to parallel `set`s. + * + * The fallback keeps `set`'s contract: `Promise.all` surfaces the first + * rejection, which is what the routes translate into a 503. + */ +export async function secretStoreSetMany( + store: SecretStore, + serverId: string, + values: Record, +): Promise { + if (store.setMany) return store.setMany(serverId, values); + await Promise.all( + Object.entries(values).map(([field, value]) => + store.set(serverId, field, value), + ), + ); +} + export async function secretStoreIsDurable( store: SecretStore, ): Promise { diff --git a/core/client/node-persistence.ts b/core/client/node-persistence.ts index 6dbaf0cc..859f620b 100644 --- a/core/client/node-persistence.ts +++ b/core/client/node-persistence.ts @@ -121,7 +121,18 @@ export async function writeClientConfigStore( SECRET_FIELD_IDP_CLIENT_SECRET, ); } - await writeStoreFile(filePath, serializeStore(stripped)); + // What actually goes to disk. The read-path migration already withholds + // the strip for a session-scoped store, but the *write* path did not — so + // saving any unrelated field (a CIMD URL, an issuer) round-tripped the + // rehydrated secret through the form and then wrote the stripped shape, + // moving the only durable copy into RAM to be lost at exit. The two paths + // have to agree: while the store cannot outlive the process, `client.json` + // stays the durable copy. + const durable = await secretStoreIsDurable(secretStore); + await writeStoreFile( + filePath, + serializeStore(durable ? stripped : validated), + ); } /** Remove client.json and the install-level IdP secret from the keychain. */ diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index 7d5a9c9f..b6205180 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -66,6 +66,7 @@ import { createRemoteAuthProvider } from "./tokenAuthProvider.js"; import { API_SERVER_ENV_VARS } from "../constants.js"; import { secretStoreGetMany, + secretStoreSetMany, secretStoreGetStrict, secretStoreIsDurable, SecretStoreUnavailableError, @@ -1886,11 +1887,11 @@ export function createRemoteApp( id: string, secrets: Record, ): Promise => { - await Promise.all( - Object.entries(secrets).map(([field, value]) => - secretStore.set(id, field, value), - ), - ); + // One pass for the entry's whole secret set. The settings form resends a + // server's full `env` map on any edit, and for the file store each `set` + // is a read-decrypt-encrypt-write-verify cycle — so this is the + // difference between one of those and one per variable. + await secretStoreSetMany(secretStore, id, secrets); }; // Fetch every keychain value an entry could need into a flat record @@ -2098,6 +2099,26 @@ export function createRemoteApp( await Promise.all(fields.map((field) => secretStore.delete(id, field))); }; + /** + * The entry that actually goes to disk. + * + * `stripped` while the store outlives the process, `built` (secrets and + * all) while it does not. The GET migration already withheld its strip for + * a session-scoped store; the POST/PUT paths did not, so saving any + * unrelated setting round-tripped the rehydrated secret through the form + * and then wrote the stripped shape — moving the only durable copy into + * RAM, where exiting loses it. The user changed a timeout and lost a + * client secret, with every operation reporting success. + * + * The two paths have to agree: while the store cannot outlive the process, + * `mcp.json` stays the durable copy. + */ + const entryForDisk = async ( + built: StoredMCPServer, + stripped: StoredMCPServer, + ): Promise => + (await secretStoreIsDurable(secretStore)) ? stripped : built; + const keychainErrorResponse = ( c: Context, err: unknown, @@ -2276,7 +2297,7 @@ export function createRemoteApp( // reusing; it's a silent no-op when the keychain is unavailable. await secretStore.deleteAllForServer(id); await writeKeychainEntriesFor(id, secrets); - current.mcpServers[id] = stripped; + current.mcpServers[id] = await entryForDisk(built, stripped); await writeMcpAndTrackMtime(serializeStore(current)); return c.json({ ok: true }); }); @@ -2513,10 +2534,11 @@ export function createRemoteApp( } } const { stripped, secrets } = extractSecretsFromStored(built); + const onDisk = await entryForDisk(built, stripped); const next: MCPConfig = { mcpServers: {} }; for (const [key, val] of Object.entries(current.mcpServers)) { if (key === originalId) { - next.mcpServers[newId] = stripped; + next.mcpServers[newId] = onDisk; } else { next.mcpServers[key] = val; }