From 339aea37296d09a284c9b4cab99d70ce2a433f43 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 22 Aug 2026 18:05:26 -0400 Subject: [PATCH] fix(auth): replace the secrets-file lock with optimistic verify-and-retry, and address review round 9 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two changes, interleaved in the same files because both are about the same read-modify-write path. ## The lock is gone (maintainer decision on the round-5/7/8 finding) Three consecutive review rounds found a real race in the `mkdir` lock, and the last one is not patchable with what Node exposes: claiming a stale lock needs compare-and-swap on a directory entry (`renameat2(RENAME_EXCHANGE)`). Without it, `canSteal` inspects one lock but `rename` acts on whatever is at that path *later*, so a waiter that lost the first race can move the winner's fresh lock aside and enter alongside it. Rather than patch it a fourth time, writers now collide and the loser notices. `mutate` reads `M0`, applies the mutation to get `M1`, writes it, then reads back `M2`; if they differ, someone wrote in between, so it re-applies onto what they left and retries, bounded. The comparison is over the **whole map**, and that is the load-bearing detail. Checking only the entry we touched passes in exactly the case that loses data — ours is present, and it is the *other* writer's that is gone. With A and B from the same `M0`: A writes `MA`, B writes `MB` and clobbers A; B verifies and correctly sees `MB`; A verifies, sees `M2 !== MA`, re-applies onto `MB`, writes again. Both survive, and the writer that was clobbered is the one that repairs it. Two things this deliberately does not claim: - It is **not mutual exclusion**. A process that crashes between its write and its verifying read cannot notice it was clobbered, and nothing else will. That window is strictly narrower than the lock's — which lost updates with every participant alive — and needs no primitive Node lacks. Stated in the code rather than left to be rediscovered in review. - Non-convergence **fails loudly**. After five lost rounds `set` throws rather than returning, because returning would reintroduce the silent loss with extra steps. `delete` stays silent, per the interface contract. `serialize` is now keyed on the resolved path **process-wide** rather than per instance. Two `FileSecretStore`s on one file are ordinary — the resolved store holds one and `absorbFileSecretsIntoKeyring` builds another — and a per-instance queue does not order them against each other at all. The new convergence test caught this on its first run: two instances racing `set` lost an entry. In-process is now correct by construction; the verify covers what it cannot see, a second Inspector process. The hand-off's read → copy → delete was the lock's other user. Its delete is now conditional: the file is re-read immediately before removal and compared against what was copied, and it declines to delete when they differ. Leaving the file costs nothing (keychain-wins makes the next run idempotent) while deleting on a stale read costs a secret. ## Round 9 (two inline, two suppressed) - **`KeyringSecretStore.getStrict` let raw binding errors escape.** Both plaintext migrations keep their source file only when they catch `SecretStoreUnavailableError`, so a transient keychain failure 500'd `GET /api/servers` instead of abandoning the migration. Wrapped, as `set` already does. - **`FileSecretStore` had no strict read at all**, so `secretStoreGetStrict` fell back to the tolerant `get` and mapped every read failure to `null` — leaving the exact inversion the seam was introduced to prevent open on the store it was introduced for. - **`readOnDiskEncryption` reported "encrypted" for an envelope it cannot open.** Naming the cipher is not the same as being openable: a file with no `kdf`/`data`, or a `data` that is not `iv.tag.ciphertext`, produced the quiet reassuring footer for a file whose next save was guaranteed to fail. - **`/api/config` kept a stale descriptor when the resolver returned `undefined`**, because the conditional spread left the startup value standing. Assigned unconditionally now; `c.json` omits the key. Full `npm run ci` green. Signed-off-by: cliffhall --- .../auth/node/file-secret-store.test.ts | 517 ++++++----------- .../auth/node/secret-store-selection.test.ts | 69 ++- .../auth/node/secret-store.test.ts | 37 ++ .../mcp/remote/server-config.test.ts | 27 + core/auth/node/file-secret-store.ts | 534 ++++++------------ core/auth/node/secret-store-selection.ts | 81 ++- core/auth/node/secret-store.ts | 26 +- core/mcp/remote/node/server.ts | 8 +- 8 files changed, 538 insertions(+), 761 deletions(-) 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 8922e4cc..891218c7 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 @@ -8,16 +8,13 @@ * agree with any of those without them being true. */ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; -import { existsSync } from "node:fs"; import * as crypto from "node:crypto"; import * as fs from "node:fs/promises"; import * as os from "node:os"; import * as path from "node:path"; import { - acquireLock, FileSecretStore, readSecretFilePermissions, - SecretFileLockTimeoutError, SecretFileKeyMismatchError, tightenSecretFilePermissions, } from "@inspector/core/auth/node/file-secret-store.js"; @@ -876,380 +873,188 @@ describe("payload shape validation", () => { }); }); -describe("acquireLock", () => { - it("excludes a second holder while the first is live", async () => { - const target = path.join(tmpDir, "locked.json"); - const release = await acquireLock(target); - let secondTaken = false; - const second = acquireLock(target).then((r) => { - secondTaken = true; - return r; +describe("getStrict (round 9)", () => { + it("returns the value like get does when the file is readable", async () => { + const store = new FileSecretStore({ filePath: filePath() }); + await store.set("srv", "env:A", "1"); + expect(await store.getStrict("srv", "env:A")).toBe("1"); + expect(await store.getStrict("srv", "env:MISSING")).toBe(null); + }); + + it("throws where get would answer null, so a migration cannot write over a newer value", async () => { + // The whole point of the seam: `get` maps an unreadable store to `null`, + // and both plaintext migrations *write* on `null` and then delete the + // disk copy. Falling back to `get` here left that inversion open on the + // very store the seam was added for. + const writer = new FileSecretStore({ + filePath: filePath(), + passphrase: "right-key", }); - // Still held: the waiter must not have been let through. - await new Promise((r) => setTimeout(r, 60)); - expect(secondTaken).toBe(false); - await release(); - await ( - await second - )(); - expect(secondTaken).toBe(true); - }); + await writer.set("srv", "env:A", "1"); - it("steals a lock left behind by a killed process", async () => { - // A container killed with SIGKILL leaves the directory. A lock only a - // clean exit can release would turn that into permanently broken writes — - // strictly worse than the race it prevents. - const target = path.join(tmpDir, "stale.json"); - const lockDir = `${target}.lock`; - await fs.mkdir(lockDir); - const old = new Date(Date.now() - 60_000); - await fs.utimes(lockDir, old, old); - const release = await acquireLock(target); - await release(); - }); - - it("creates the parent directory so a first write is locked too", async () => { - // The first run has no secrets directory yet — `writeStoreFile` creates - // it later. Without creating it here, mkdir failed ENOENT and the lock - // was skipped, so two processes' *first* writes could both proceed - // unlocked and overwrite each other. First writes are exactly when two - // processes are most likely to start together. - const target = path.join(tmpDir, "no-such-dir", "secrets.json"); - const release = await acquireLock(target, { pollMs: 10 }); - // A real lock was taken: a second acquisition must not be handed out. - let secondTaken = false; - // Held, not floated: an acquisition still running when the test ends - // would race `afterEach` removing the temp directory. - const second = acquireLock(target, { pollMs: 10 }).then((r) => { - secondTaken = true; - return r; + const wrongKey = new FileSecretStore({ + filePath: filePath(), + passphrase: "wrong-key", }); - await new Promise((r) => setTimeout(r, 60)); - expect(secondTaken).toBe(false); - await release(); - await ( - await second - )(); - }); - - it("releases only a lock it still owns", async () => { - // A holder whose work outran the stale threshold must not delete the - // next owner's lock on its way out — that would drop a third writer into - // a section two processes believed they held. - const target = path.join(tmpDir, "owned.json"); - const lockDir = `${target}.lock`; - const release = await acquireLock(target); - // Simulate a steal: someone judged us stale, removed our lock, and took - // their own. - await fs.rm(lockDir, { recursive: true, force: true }); - await fs.mkdir(lockDir); - await fs.writeFile(path.join(lockDir, "owner"), "someone-else", "utf-8"); - await release(); - // Their lock is untouched. - expect(await fs.readFile(path.join(lockDir, "owner"), "utf-8")).toBe( - "someone-else", + expect(await wrongKey.get("srv", "env:A")).toBe(null); + await expect(wrongKey.getStrict("srv", "env:A")).rejects.toBeInstanceOf( + SecretFileKeyMismatchError, ); - await fs.rm(lockDir, { recursive: true, force: true }); }); - it("fails the mutation rather than writing past a live holder", async () => { - // Falling through to an unlocked write under contention would be the - // worst possible moment for it: another writer is provably active. `set` - // is allowed to hard-fail, so this surfaces as the documented 503. - // - // The holder heartbeats faster than it goes stale, so the waiter can - // never mistake it for dead and must time out instead. - const target = path.join(tmpDir, "contended.json"); - const timings = { - staleMs: 400, - timeoutMs: 250, - heartbeatMs: 50, - pollMs: 10, - }; - const release = await acquireLock(target, timings); - try { - await expect(acquireLock(target, timings)).rejects.toBeInstanceOf( - SecretFileLockTimeoutError, - ); - } finally { - await release(); - } - }); - - it("does not steal from a holder that is merely slow", async () => { - // The heartbeat is what separates "dead" from "slow". Without it a - // holder on a slow filesystem loses its lock mid-section and the lost - // update comes straight back — so the waiter here must time out rather - // than acquire, even though it waits well past the stale threshold. - const target = path.join(tmpDir, "slow.json"); - const release = await acquireLock(target, { - staleMs: 120, - timeoutMs: 60_000, - heartbeatMs: 20, - pollMs: 10, + it("wraps a filesystem failure as SecretStoreUnavailableError", async () => { + // Not a typed refusal — a raw ENOTDIR — which must still arrive as + // something the migrations' catch recognises. + const blocker = path.join(tmpDir, "blocker"); + await fs.writeFile(blocker, "x", "utf-8"); + const store = new FileSecretStore({ + filePath: path.join(blocker, "secrets.json"), }); - try { - let stolen = false; - // Held and settled below rather than floated, so the waiter cannot - // still be running when `afterEach` removes the temp directory. - const waiter = acquireLock(target, { - staleMs: 120, - timeoutMs: 400, - pollMs: 10, - }) - .then(async (r) => { - stolen = true; - await r(); - }) - .catch(() => { - // Timed out, which is the expected outcome. - }); - await waiter; - expect(stolen).toBe(false); - } finally { - await release(); - } - }); - - it("does not steal a stale lock whose owner process is still alive", async () => { - // An old mtime means the holder has not heartbeated lately, and there are - // two very different reasons: it died, or it is alive but not running — - // SIGSTOPed, event-loop-blocked, or on a machine that suspended. Taking - // the lock on elapsed time alone would pull it out from under a live - // critical section, which is the lost update the lock exists to prevent. - const target = path.join(tmpDir, "suspended.json"); - const lockDir = `${target}.lock`; - await fs.mkdir(lockDir, { recursive: true }); - // Stamp it with *this* process, which is definitionally alive, and age it - // well past stale — the shape of a suspended holder. - await fs.writeFile( - path.join(lockDir, "owner"), - JSON.stringify({ - token: "someone", - pid: process.pid, - host: os.hostname(), - }), - "utf-8", + await expect(store.getStrict("srv", "env:A")).rejects.toBeInstanceOf( + SecretStoreUnavailableError, ); - const old = new Date(Date.now() - 60_000); - await fs.utimes(lockDir, old, old); - - await expect( - acquireLock(target, { staleMs: 50, timeoutMs: 150, pollMs: 10 }), - ).rejects.toBeInstanceOf(SecretFileLockTimeoutError); - // Their lock survives the attempt. - expect(existsSync(lockDir)).toBe(true); - await fs.rm(lockDir, { recursive: true, force: true }); - }); - - it("steals a stale lock whose owner process is gone", async () => { - // The case the stealing exists for. A pid that no longer resolves is a - // dead holder, and refusing to break its lock would turn one killed - // container into permanently broken secret writes. - const target = path.join(tmpDir, "dead.json"); - const lockDir = `${target}.lock`; - await fs.mkdir(lockDir, { recursive: true }); - await fs.writeFile( - path.join(lockDir, "owner"), - // A pid that cannot be running: `kill(0)` answers ESRCH. - JSON.stringify({ token: "gone", pid: 2 ** 31 - 1, host: os.hostname() }), - "utf-8", - ); - const old = new Date(Date.now() - 60_000); - await fs.utimes(lockDir, old, old); - - const release = await acquireLock(target, { - staleMs: 50, - timeoutMs: 2_000, - pollMs: 10, - }); - await release(); }); +}); +describe("readOnDiskEncryption rejects an envelope it could not open", () => { + // Naming the cipher is not the same as being openable, and reporting + // "encrypted" for a file whose next save is guaranteed to fail is the + // quiet, reassuring footer arriving for the worst case. it.each([ - ["not JSON at all", "{ not json"], - ["JSON that is not an object", '"just-a-string"'], - ["an object with no token", JSON.stringify({ pid: 1, host: "h" })], - ])("steals a stale lock whose owner stamp is %s", async (_label, stamp) => { - // An unusable stamp cannot answer the liveness question, so the mtime is - // all there is — and stealing on it is right, because the alternative is - // a lock nothing can ever break. - const target = path.join(tmpDir, `stamp-${_label.length}.json`); - const lockDir = `${target}.lock`; - await fs.mkdir(lockDir, { recursive: true }); - await fs.writeFile(path.join(lockDir, "owner"), stamp, "utf-8"); - const old = new Date(Date.now() - 60_000); - await fs.utimes(lockDir, old, old); - - const release = await acquireLock(target, { - staleMs: 50, - timeoutMs: 2_000, - pollMs: 10, + [ + "no kdf or data", + { version: 1, encryption: "aes-256-gcm" }, + "encrypted envelope is missing its kdf or data", + ], + [ + "data that is not iv.tag.ciphertext", + { + version: 1, + encryption: "aes-256-gcm", + kdf: { algorithm: "scrypt", salt: "AAAA", N: 16384, r: 8, p: 1 }, + data: "only-one-part", + }, + "encrypted payload is not iv.tag.ciphertext", + ], + ])("reports unreadable for %s", async (_label, envelope, detail) => { + await fs.writeFile(filePath(), JSON.stringify(envelope), "utf-8"); + const store = new FileSecretStore({ filePath: filePath() }); + expect(await store.readOnDiskEncryption()).toEqual({ + state: "unreadable", + detail, }); - await release(); + }); +}); + +describe("concurrent writers (no lock, optimistic verify-and-retry)", () => { + // The lock these replace was removed after three review rounds each found + // a real race in it; the last needed a compare-and-swap on a directory + // entry that Node does not expose. The store now lets writers collide and + // makes the loser notice — so these assert convergence, which is the + // property that actually matters, rather than mutual exclusion, which it + // no longer claims. + + it("two stores racing set on one file keep both entries", async () => { + // The classic lost update: both read the same map, both write. Whichever + // lands second clobbers the other's entry — and the clobbered writer's + // verifying read is what catches it and re-applies. + const a = new FileSecretStore({ filePath: filePath() }); + const b = new FileSecretStore({ filePath: filePath() }); + await Promise.all([a.set("srv", "env:A", "1"), b.set("srv", "env:B", "2")]); + + const reader = new FileSecretStore({ filePath: filePath() }); + expect(await reader.get("srv", "env:A")).toBe("1"); + expect(await reader.get("srv", "env:B")).toBe("2"); }); - it("steals a stale lock whose stamp carries no usable pid", async () => { - // A stamp with a non-numeric pid names no process to ask about, so it - // cannot vouch for the holder being alive. - const target = path.join(tmpDir, "nopid.json"); - const lockDir = `${target}.lock`; - await fs.mkdir(lockDir, { recursive: true }); - await fs.writeFile( - path.join(lockDir, "owner"), - JSON.stringify({ token: "t", pid: "not-a-number", host: os.hostname() }), - "utf-8", + it("keeps every entry when many independent stores write at once", async () => { + // Independent instances, so the in-process queue does not serialize them + // for us — each is its own writer with its own view of the file. + const stores = Array.from( + { length: 8 }, + () => new FileSecretStore({ filePath: filePath() }), ); - const old = new Date(Date.now() - 60_000); - await fs.utimes(lockDir, old, old); + await Promise.all(stores.map((s, i) => s.set("srv", `env:K${i}`, `v${i}`))); - const release = await acquireLock(target, { - staleMs: 50, - timeoutMs: 2_000, - pollMs: 10, - }); - await release(); - }); - - it("steals a stale lock stamped by another machine", async () => { - // A shared filesystem: we cannot ask this host about that host's pid, so - // the mtime is all there is. Refusing would make a lock nothing can ever - // break. - const target = path.join(tmpDir, "remote.json"); - const lockDir = `${target}.lock`; - await fs.mkdir(lockDir, { recursive: true }); - await fs.writeFile( - path.join(lockDir, "owner"), - JSON.stringify({ - token: "elsewhere", - pid: process.pid, - host: "some-other-host", - }), - "utf-8", - ); - const old = new Date(Date.now() - 60_000); - await fs.utimes(lockDir, old, old); - - const release = await acquireLock(target, { - staleMs: 50, - timeoutMs: 2_000, - pollMs: 10, - }); - await release(); - }); - - it("stops heartbeating a lock that is no longer ours", async () => { - // After a steal, the directory belongs to someone else. Continuing to - // touch it would keep *their* lock looking alive after they died, which - // turns a stale lock into an unbreakable one. - const target = path.join(tmpDir, "stolen.json"); - const lockDir = `${target}.lock`; - const release = await acquireLock(target, { heartbeatMs: 10 }); - // Someone judges us dead and takes it. - await fs.writeFile( - path.join(lockDir, "owner"), - JSON.stringify({ token: "thief", pid: process.pid, host: os.hostname() }), - "utf-8", - ); - const afterSteal = new Date(Date.now() - 30_000); - await fs.utimes(lockDir, afterSteal, afterSteal); - // Give the heartbeat several chances to (wrongly) refresh it. - await new Promise((r) => setTimeout(r, 80)); - const mtime = (await fs.stat(lockDir)).mtimeMs; - expect(Date.now() - mtime).toBeGreaterThan(1_000); - // And our release leaves their lock alone, since the token no longer - // matches. - await release(); - expect(existsSync(lockDir)).toBe(true); - await fs.rm(lockDir, { recursive: true, force: true }); - }); - - it("elects a single winner when two waiters find the same stale lock", async () => { - // `rm`-then-`mkdir` is not an election: both waiters judge the lock - // stale, the first removes it and takes a fresh one, and the second then - // removes *that* — putting both inside the section at once, which is the - // lost update the takeover exists to prevent. The rename is atomic, so - // exactly one waiter can claim a given stale lock. - const target = path.join(tmpDir, "elected.json"); - const lockDir = `${target}.lock`; - await fs.mkdir(lockDir, { recursive: true }); - await fs.writeFile( - path.join(lockDir, "owner"), - // A pid that cannot be running, so both waiters agree it is stale. - JSON.stringify({ token: "dead", pid: 2 ** 31 - 1, host: os.hostname() }), - "utf-8", - ); - const old = new Date(Date.now() - 60_000); - await fs.utimes(lockDir, old, old); - - let inside = 0; - let maxInside = 0; - const timings = { staleMs: 50, timeoutMs: 10_000, pollMs: 5 }; - const run = async () => { - const release = await acquireLock(target, timings); - inside += 1; - maxInside = Math.max(maxInside, inside); - await new Promise((r) => setTimeout(r, 40)); - inside -= 1; - await release(); - }; - await Promise.all([run(), run()]); - // Never two holders at once. - expect(maxInside).toBe(1); - }); - - it("cleans up and reports when it cannot stamp ownership", async () => { - // mkdir succeeded, the owner write did not. Returning a no-op release - // here would enter the critical section unlocked *and* leave the - // directory behind, so every later writer queues behind an orphan nobody - // can deliberately release. - const target = path.join(tmpDir, "unstampable.json"); - vi.resetModules(); - vi.doMock("node:fs/promises", async () => { - const actual = - await vi.importActual( - "node:fs/promises", - ); - return { - ...actual, - default: actual, - writeFile: vi.fn((p: string, ...rest: unknown[]) => - String(p).endsWith(`${path.sep}owner`) - ? Promise.reject(new Error("ENOSPC: no space left on device")) - : (actual.writeFile as (...a: unknown[]) => Promise)( - p, - ...rest, - ), - ), - }; - }); - try { - const mod = - await import("@inspector/core/auth/node/file-secret-store.js"); - await expect(mod.acquireLock(target)).rejects.toThrow( - /Could not initialize the lock/, - ); - // And nothing is left obstructing the next writer. - expect(existsSync(`${target}.lock`)).toBe(false); - } finally { - vi.doUnmock("node:fs/promises"); - vi.resetModules(); + const reader = new FileSecretStore({ filePath: filePath() }); + for (let i = 0; i < stores.length; i++) { + expect(await reader.get("srv", `env:K${i}`)).toBe(`v${i}`); } }); - it("serializes concurrent writers through two independent store instances", async () => { - // Two `FileSecretStore` objects have separate in-process queues, so this - // is the cross-process race in miniature: without the file lock the second - // write drops the first one's entry. - const target = path.join(tmpDir, "shared.json"); - const a = new FileSecretStore({ filePath: target }); - const b = new FileSecretStore({ filePath: target }); + it("converges when the racing writers are encrypted", async () => { + // Same property with a scrypt derivation inside every attempt, which is + // where the window is widest. + const opts = { filePath: filePath(), passphrase: "hunter2" }; + const a = new FileSecretStore(opts); + const b = new FileSecretStore(opts); + await Promise.all([a.set("srv", "env:A", "1"), b.set("srv", "env:B", "2")]); + + const reader = new FileSecretStore(opts); + expect(await reader.get("srv", "env:A")).toBe("1"); + expect(await reader.get("srv", "env:B")).toBe("2"); + }); + + it("a racing delete and set both land", async () => { + const store = new FileSecretStore({ filePath: filePath() }); + await store.set("srv", "env:OLD", "x"); + + const a = new FileSecretStore({ filePath: filePath() }); + const b = new FileSecretStore({ filePath: filePath() }); await Promise.all([ - a.set("alpha", "env:A", "1"), - b.set("alpha", "env:B", "2"), + a.delete("srv", "env:OLD"), + b.set("srv", "env:NEW", "y"), ]); - expect(await a.get("alpha", "env:A")).toBe("1"); - expect(await a.get("alpha", "env:B")).toBe("2"); + + const reader = new FileSecretStore({ filePath: filePath() }); + expect(await reader.get("srv", "env:OLD")).toBe(null); + expect(await reader.get("srv", "env:NEW")).toBe("y"); + }); + + // A writer that never wins. Simulated by replacing the file *behind* the + // store immediately after each write — with a raw `fs` write rather than a + // second store, because every store on this path now shares one in-process + // queue and writing through one from inside a queued section would + // deadlock on it. + const clobberAfterEveryWrite = (store: FileSecretStore): void => { + const inner = store as unknown as { + writeMap: (m: Record) => Promise; + }; + const realWrite = inner.writeMap.bind(store); + let n = 0; + inner.writeMap = async (map: Record) => { + await realWrite(map); + await fs.writeFile( + filePath(), + JSON.stringify({ + version: 1, + encryption: "none", + secrets: { "srv:env:INTRUDER": `v${n++}` }, + }), + "utf-8", + ); + }; + }; + + it("reports non-convergence instead of silently dropping the value", async () => { + // Returning after N attempts would reintroduce the silent loss with + // extra steps, so this must reject — and say which value was not saved. + const store = new FileSecretStore({ filePath: filePath() }); + clobberAfterEveryWrite(store); + + await expect(store.set("srv", "env:MINE", "1")).rejects.toThrow( + /kept overwriting it/, + ); + }); + + it("a non-convergent delete stays silent, per the interface contract", async () => { + // `delete` reports nothing by contract — only `set` hard-fails — so a + // delete that cannot converge must still resolve rather than throw. + const store = new FileSecretStore({ filePath: filePath() }); + await store.set("srv", "env:A", "1"); + clobberAfterEveryWrite(store); + + await expect(store.delete("srv", "env:A")).resolves.toBeUndefined(); }); }); 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 4767aab3..56c901e8 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 @@ -772,35 +772,58 @@ describe("absorbFileSecretsIntoKeyring", () => { ); }); - it("holds the file lock for the whole hand-off", async () => { - // Read → copy → delete must be one transaction. Another Inspector - // completing a `set` between the read and the delete would have its - // brand-new secret removed without ever being copied — a write that - // reported success and then vanished. + 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. + const filePath = await seedFile({ "srv:oauthClientSecret": "from-file" }); + process.env.MCP_INSPECTOR_SECRET_FILE = filePath; + const warn = 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"); + const original = keyring.set.bind(keyring); + let intruded = false; + keyring.set = async (id: string, field: string, value: string) => { + await original(id, field, value); + if (!intruded) { + intruded = true; + await new FileSecretStore({ filePath }).set("other", "env:LATE", "z"); + } + }; + + await mod.absorbFileSecretsIntoKeyring(keyring); + + // The file survives, and so does the entry that arrived late. + 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/, + ); + }); + + it("deletes the file when nothing changed under it", async () => { + // The ordinary path: no concurrent writer, so the comparison matches and + // the now-redundant file goes away. const filePath = await seedFile({ "srv:oauthClientSecret": "from-file" }); process.env.MCP_INSPECTOR_SECRET_FILE = filePath; vi.spyOn(console, "warn").mockImplementation(() => {}); const mod = await loadWithProbe(true); - const { acquireLock } = - await import("@inspector/core/auth/node/file-secret-store.js"); - // Hold the lock as the "other process" would. - const release = await acquireLock(filePath, { heartbeatMs: 20 }); - let finished = false; - const handOff = mod - .absorbFileSecretsIntoKeyring(new InMemorySecretStore()) - .then(() => { - finished = true; - }); + const keyring = new InMemorySecretStore(); + await mod.absorbFileSecretsIntoKeyring(keyring); - await new Promise((r) => setTimeout(r, 120)); - // Blocked on us, not steaming ahead and deleting the file. - expect(finished).toBe(false); - expect(existsSync(filePath)).toBe(true); - - await release(); - await handOff; - expect(finished).toBe(true); + expect(existsSync(filePath)).toBe(false); + expect(await keyring.get("srv", "oauthClientSecret")).toBe("from-file"); }); it("runs the hand-off for an explicitly configured keyring too", async () => { diff --git a/clients/web/src/test/integration/auth/node/secret-store.test.ts b/clients/web/src/test/integration/auth/node/secret-store.test.ts index a2e079ed..10cc8aa7 100644 --- a/clients/web/src/test/integration/auth/node/secret-store.test.ts +++ b/clients/web/src/test/integration/auth/node/secret-store.test.ts @@ -378,6 +378,43 @@ describe("KeyringSecretStore (mocked native bindings)", () => { }); }); + describe("getStrict wraps its failures (round 9)", () => { + // "Strict" must mean the caller *learns* the read failed, in a form it + // can act on. Both plaintext migrations keep their source file only when + // they catch `SecretStoreUnavailableError`; a raw binding error escaping + // here sails past that and 500s `GET /api/servers` instead of abandoning + // the migration. + it("returns the value when the keychain answers", async () => { + await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "shh"); + expect( + await store.getStrict("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET), + ).toBe("shh"); + }); + + it("throws the typed error when the read itself fails", async () => { + keyringMocks.failures.getThrows = true; + await expect( + store.getStrict("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET), + ).rejects.toBeInstanceOf(KeychainUnavailableError); + }); + + it("throws the typed error when AsyncEntry construction fails", async () => { + // #1848's shape: the platform store is unreachable, and it is the + // constructor that says so. + keyringMocks.failures.constructorThrows = true; + await expect( + store.getStrict("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET), + ).rejects.toBeInstanceOf(KeychainUnavailableError); + }); + + it("still answers null for a genuinely absent entry", async () => { + // Strictness is about the *store* failing, not about absence. + expect( + await store.getStrict("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET), + ).toBe(null); + }); + }); + describe("probeKeyringAvailable (#1950)", () => { // The input to the automatic-fallback policy. It has to answer "is the // keychain *usable*", not "did the package import" — the container that diff --git a/clients/web/src/test/integration/mcp/remote/server-config.test.ts b/clients/web/src/test/integration/mcp/remote/server-config.test.ts index 122ebf07..26e138b1 100644 --- a/clients/web/src/test/integration/mcp/remote/server-config.test.ts +++ b/clients/web/src/test/integration/mcp/remote/server-config.test.ts @@ -122,6 +122,33 @@ describe("GET /api/config re-resolves secretStorage per request (#1950)", () => expect(second.secretStorage?.plaintext).toBe(false); }); + it("clears the field when the resolver says it is not known (round 9)", async () => { + // The resolver's contract is that `undefined` means "not known right + // now". Spreading it in conditionally left the *startup* descriptor + // standing in its place, so a store that became undescribable kept + // serving a stale, confident answer — the one thing this surface must + // never do. + const { app } = createRemoteApp({ + dangerouslyOmitAuth: true, + initialConfig: { + defaultEnvironment: {}, + secretStorage: { + kind: "file", + reason: "fallback", + durable: true, + plaintext: true, + path: "/tmp/secrets.json", + }, + }, + secretStorageResolver: async () => undefined, + }); + + const body = (await ( + await app.request(new Request("http://test/api/config")) + ).json()) as { secretStorage?: unknown }; + expect(body.secretStorage).toBeUndefined(); + }); + it("falls back to the startup payload when no resolver is supplied", async () => { // Embedders and the test suite pass no resolver; they must keep the // previous behaviour rather than losing the field. diff --git a/core/auth/node/file-secret-store.ts b/core/auth/node/file-secret-store.ts index c4262bff..fef8d424 100644 --- a/core/auth/node/file-secret-store.ts +++ b/core/auth/node/file-secret-store.ts @@ -60,7 +60,6 @@ import * as crypto from "node:crypto"; import * as fs from "node:fs/promises"; -import * as os from "node:os"; import * as path from "node:path"; import { readStoreFile, writeStoreFile } from "../../storage/store-io.js"; import { @@ -195,311 +194,34 @@ function decodeParts( } /** - * How long a lock may be held before another process treats it as - * abandoned. Generous next to the work it guards (a read, one scrypt - * derivation, an atomic write — single-digit milliseconds), because the - * cost of being wrong is asymmetric: waiting a second too long is - * invisible, while stealing a live lock reintroduces the very race the - * lock exists to close. - */ -const LOCK_STALE_MS = 10_000; -/** - * How long to wait for a live lock. Deliberately longer than - * {@link LOCK_STALE_MS}, so a waiter always outlives the point at which the - * holder becomes stealable — with the shorter value it used to have, a - * waiter gave up before it could ever steal an abandoned lock, which is the - * one case the stealing exists for. - */ -const LOCK_TIMEOUT_MS = 15_000; -const LOCK_POLL_MS = 25; -/** - * How often a holder touches its lock to prove it is alive. + * How many times a mutation will re-apply itself before giving up. * - * Comfortably under {@link LOCK_STALE_MS} so a live holder is never judged - * stale, which is what makes the two mechanisms coherent: staleness then - * means *dead*, not merely *slow*. Without the heartbeat, a holder on a - * slow filesystem — or one whose scrypt derivation ran long — has its lock - * stolen while it is still inside the critical section, reinstating exactly - * the lost update the lock exists to prevent. + * Each attempt is a full read-modify-write plus a verifying read, so the + * bound is on *rounds lost to another writer*, not on retries of a failing + * operation. Two Inspector processes writing the same file at the same + * instant is already rare; losing five consecutive rounds to one means + * something other than ordinary contention is happening, and reporting that + * is more honest than looping. */ -const LOCK_HEARTBEAT_MS = 3_000; - -const sleep = (ms: number): Promise => - new Promise((resolve) => setTimeout(resolve, ms)); +const MAX_WRITE_ATTEMPTS = 5; /** - * Thrown when a lock is held by someone else for longer than - * {@link LOCK_TIMEOUT_MS}. + * In-process mutation queues, one per resolved secrets-file path. * - * A timeout must **fail the mutation**, never fall through and write - * anyway: proceeding unlocked past a live holder is precisely the - * lost-update the lock exists to prevent, and doing it under contention — - * the one moment another writer is provably active — would be the worst - * possible time. `set` is allowed to hard-fail by the store contract, so - * this surfaces as the documented 503 and the user retries. + * Process-wide rather than per-store: see `FileSecretStore.serialize`. */ -export class SecretFileLockTimeoutError extends SecretStoreUnavailableError { - constructor(filePath: string) { - super( - `Timed out waiting for another process to finish writing the secrets file at ${filePath}. Another Inspector may be busy; try again.`, - ); - this.name = "SecretFileLockTimeoutError"; - } -} +const fileQueues = new Map>(); -/** - * Take an exclusive, cross-process lock on `filePath`. - * - * `mkdir` is the primitive rather than a lock *file*: it is atomic and - * fails with `EEXIST` on every POSIX filesystem and on Windows, and it - * needs no `O_EXCL` open-flag juggling. Returns the release function. - * - * **Ownership is stamped, not assumed.** The holder writes a random token - * inside the lock directory and release only removes a lock still carrying - * that token. Without it, a holder whose work outran the stale threshold - * would delete the *next* owner's lock on its way out, letting a third - * writer into a critical section two processes believed they held — a - * rarer and much more confusing corruption than the one being prevented. - * - * **Stale locks are stolen, and that is not optional.** A container killed - * with SIGKILL leaves the directory behind, and a lock that only a - * cooperative exit can release would make one `docker kill` brick secret - * writes permanently — turning a rare lost write into a permanent outage, - * which is strictly worse. Staleness is judged by the directory's mtime, - * which a live holder refreshes on a heartbeat — so "stale" means the - * holder is *dead*, never merely slow. That distinction is what keeps the - * stealing from reinstating the very race the lock prevents. - * - * Throws {@link SecretFileLockTimeoutError} when another holder keeps it - * past the timeout. The *only* path that proceeds unlocked is a filesystem - * that cannot create the lock at all (a read-only mount, an exotic - * filesystem) — there, refusing would deny every write on a box where the - * single-process case is still perfectly safe, and a genuinely unwritable - * location fails at the write itself with a better message than a lock - * error would give. - */ -/** - * Timing policy, overridable so the tests can exercise contention, - * staleness, and timeout without spending the real intervals. Production - * callers pass nothing and get the constants above. - */ -export interface LockTimings { - staleMs?: number; - timeoutMs?: number; - heartbeatMs?: number; - pollMs?: number; -} - -export async function acquireLock( - filePath: string, - timings: LockTimings = {}, -): Promise<() => Promise> { - const staleMs = timings.staleMs ?? LOCK_STALE_MS; - const timeoutMs = timings.timeoutMs ?? LOCK_TIMEOUT_MS; - const heartbeatMs = timings.heartbeatMs ?? LOCK_HEARTBEAT_MS; - const pollMs = timings.pollMs ?? LOCK_POLL_MS; - const lockDir = `${filePath}.lock`; - const ownerFile = `${lockDir}/owner`; - const token = crypto.randomBytes(16).toString("hex"); - // The stamp a would-be thief consults before taking this lock: the token - // proves ownership on release, the pid and host let a waiter ask the OS - // whether we are still alive rather than inferring it from elapsed time. - const owner: LockOwner = { token, pid: process.pid, host: os.hostname() }; - const noop = async (): Promise => {}; - const deadline = Date.now() + timeoutMs; - - // The parent must exist before the lock can. On a first run it does not — - // `writeStoreFile` creates it later — so without this the very first write - // of every process failed with ENOENT and proceeded unlocked, meaning two - // first writes could both bypass the lock, read an empty map, and overwrite - // each other. The first write is exactly when two processes are most likely - // to start together. - try { - await fs.mkdir(path.dirname(filePath), { recursive: true }); - } catch { - // Unwritable parent. The mkdir below will fail too, and the write after - // it will fail with a message about the actual problem. - } - - for (;;) { - let created = false; - try { - await fs.mkdir(lockDir, { recursive: false }); - created = true; - // Stamp ownership before entering the section. If this fails the lock - // exists but says nothing about who holds it, so nobody could ever - // release it deliberately and every later writer would queue behind an - // orphan until it aged out. Clean up and report, rather than proceeding - // unlocked *and* leaving the obstruction behind. - await fs.writeFile(ownerFile, JSON.stringify(owner), "utf-8"); - // Prove liveness for as long as we hold it. `unref` so a held lock - // never keeps the process alive on its own — the interval is a - // side-channel, not work anyone is waiting for. - const heartbeat = setInterval(() => { - // Confirm the lock is still ours before refreshing it. If it was - // stolen (only possible when we looked dead, or across hosts where - // the pid check cannot apply), the directory now belongs to someone - // else — and touching it would keep *their* lock looking alive after - // they died, which is how a stale lock becomes an unbreakable one. - void (async () => { - try { - const held = parseOwner(await fs.readFile(ownerFile, "utf-8")); - if (held?.token !== token) { - clearInterval(heartbeat); - return; - } - const now = new Date(); - await fs.utimes(lockDir, now, now); - } catch { - // Released, stolen, or unreadable. The release below copes, and - // there is nothing useful to do from a timer. - } - })(); - }, heartbeatMs); - heartbeat.unref?.(); - return async () => { - clearInterval(heartbeat); - try { - // Release only what we still own. A lock judged stale and stolen - // now belongs to someone else, and removing it would drop them - // into a race with whoever comes next. - const held = parseOwner(await fs.readFile(ownerFile, "utf-8")); - if (held?.token !== token) return; - await fs.rm(lockDir, { recursive: true, force: true }); - } catch { - // Already gone, or unreadable. Either way there is nothing to - // release and nothing the caller can act on. - } - }; - } catch (err) { - const code = (err as NodeJS.ErrnoException).code; - if (created) { - // We made the directory and then failed to stamp it. Ours to remove. - await fs.rm(lockDir, { recursive: true, force: true }).catch(() => {}); - throw new SecretStoreUnavailableError( - `Could not initialize the lock for the secrets file at ${filePath}: ${err instanceof Error ? err.message : String(err)}`, - ); - } - if (code !== "EEXIST") return noop; // cannot lock here at all — see above - if (await canSteal(lockDir, ownerFile, staleMs)) { - // Claim the stale lock by *renaming* it, not by removing it. - // - // `rm` is not an election: two waiters can both judge the same lock - // stale, and by the time the second one runs, the first may already - // have removed it and created a fresh lock of its own — which the - // second then deletes, and both enter the section together. That is - // the lost update, reached through the takeover meant to prevent it. - // - // `rename` is atomic and single-winner: exactly one waiter moves the - // directory aside, and the losers get ENOENT and fall through to - // retry against whatever the winner does next. The unique - // destination (our token) also means two winners of *different* - // stale locks cannot collide. - const claimed = `${lockDir}.stale-${token}`; - try { - await fs.rename(lockDir, claimed); - await fs.rm(claimed, { recursive: true, force: true }); - continue; - } catch { - // Lost the election, or the rename failed outright. Either way, - // fall through to the deadline check rather than looping — a - // takeover that can never succeed must time out, not spin. - } - } - if (Date.now() >= deadline) - throw new SecretFileLockTimeoutError(filePath); - await sleep(pollMs); - } - } -} - -/** - * May this waiter take a lock it found already held? - * - * Elapsed wall time alone does not answer that. An old mtime means the - * holder has not run its heartbeat lately, and there are two very different - * reasons for that: it died, or it is *alive but not running* — SIGSTOPed, - * starved by a blocked event loop, or on a machine that suspended. Expiring - * on time alone treats those identically and takes the lock out from under a - * live critical section, which is the lost update the lock exists to - * prevent, now arrived at through the machinery meant to prevent it. - * - * So the mtime only opens the question, and the owner stamp answers it: on - * the same host, ask the operating system whether that pid is still there. - * `kill(pid, 0)` sends no signal — it is the standard existence probe. - * `EPERM` counts as alive (the process exists; it just isn't ours), so a - * holder running as another user is never stolen from. - * - * The stamp is not always usable — a lock written by a different machine - * over a shared filesystem, or one whose owner file is missing or corrupt. - * There the mtime is all there is, and stealing on it is still right: the - * alternative is a lock nothing can ever break, which turns one dead process - * into a permanent outage. - */ -async function canSteal( - lockDir: string, - ownerFile: string, - staleMs: number, -): Promise { - let held: number; - try { - held = (await fs.stat(lockDir)).mtimeMs; - } catch { - // Released between our mkdir and our stat — retry rather than steal. - return false; - } - if (Date.now() - held <= staleMs) return false; - - let owner: LockOwner | null = null; - try { - owner = parseOwner(await fs.readFile(ownerFile, "utf-8")); - } catch { - // No stamp to consult; fall through to the time-only answer. - } - if (!owner || owner.host !== os.hostname() || owner.pid === undefined) { - return true; - } - return !processIsAlive(owner.pid); -} - -/** Does this pid exist? `kill(pid, 0)` is the probe; it sends no signal. */ -function processIsAlive(pid: number): boolean { - try { - process.kill(pid, 0); - return true; - } catch (err) { - // EPERM means it exists and belongs to someone else — still alive. - return (err as NodeJS.ErrnoException).code === "EPERM"; - } -} - -interface LockOwner { - token: string; - /** Absent when the stamp carried no usable (positive) pid. */ - pid?: number; - host: string; -} - -/** Parse an owner stamp, tolerating anything that isn't one. */ -function parseOwner(raw: string): LockOwner | null { - try { - const parsed: unknown = JSON.parse(raw); - if (typeof parsed !== "object" || parsed === null) return null; - const { token, pid, host } = parsed as Partial; - if (typeof token !== "string") return null; - return { - // Only a *positive* pid is usable. `-1` and `0` are wildcards to - // `process.kill` — `kill(-1, 0)` asks about every process this user - // can signal and answers "alive" for almost any host, which would make - // a garbled stamp permanently unstealable. Anything else is recorded - // as absent so the caller falls back to the mtime. - pid: typeof pid === "number" && pid > 0 ? pid : undefined, - token, - host: String(host), - }; - } catch { - return null; - } +/** Do two secret maps hold exactly the same entries? */ +function sameMap( + 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], + ); } /** @@ -527,16 +249,6 @@ export interface FileSecretStoreOptions { export class FileSecretStore implements SecretStore { readonly filePath: string; private readonly passphrase: string | undefined; - /** - * Serializes read-modify-write cycles within this process. `writeStoreFile` - * already makes each individual write atomic and ordered, but that is not - * enough here: two concurrent `set`s would each read the same "before" map - * and the second write would drop the first one's entry. The route handlers - * write a server's fields with `Promise.all`, so this is the ordinary path, - * not a race that needs contriving. - */ - private queue: Promise = Promise.resolve(); - constructor(options: FileSecretStoreOptions) { this.filePath = options.filePath; const raw = options.passphrase ?? process.env[SECRET_KEY_ENV]; @@ -600,7 +312,26 @@ export class FileSecretStore implements SecretStore { }; } if (parsed.encryption === "none") return { state: "plaintext" }; - if (parsed.encryption === CIPHER) return { state: "encrypted" }; + if (parsed.encryption === CIPHER) { + // Naming the cipher is not the same as being openable. An envelope + // with no `kdf`/`data` — or a `data` that is not the three-part + // payload — is refused by `readMap`, so reporting it as "encrypted" + // gives the quiet, reassuring footer for a file whose very next save + // is guaranteed to fail. Check the shape the decryptor requires. + if (!parsed.kdf || typeof parsed.data !== "string") { + return { + state: "unreadable", + detail: "encrypted envelope is missing its kdf or data", + }; + } + if (!decodeParts(parsed.data)) { + return { + state: "unreadable", + detail: "encrypted payload is not iv.tag.ciphertext", + }; + } + return { state: "encrypted" }; + } return { state: "unreadable", detail: `unsupported envelope (encryption="${String(parsed.encryption)}", version=${String(parsed.version)})`, @@ -762,33 +493,126 @@ export class FileSecretStore implements SecretStore { * whatever the first process had just added. Both writes report success; * one secret is simply gone. * - * Only mutations take the lock. Reads deliberately do not: `writeStoreFile` - * is atomic (write-temp-then-rename), so a concurrent reader sees either the - * old file or the new one and never a torn one, and making every `get` - * contend on a lock would put a filesystem round-trip in front of the - * request path for no correctness gain. + * Apply a mutation to the file, and confirm it survived. + * + * **There is no cross-process lock.** There was one — a `mkdir` election + * with an owner stamp, a heartbeat and a stale-takeover — and three + * consecutive review rounds found a real race in it. The last one is + * unfixable with what Node exposes: claiming a stale lock needs + * compare-and-swap on a directory entry (`renameat2(RENAME_EXCHANGE)`), + * and without it a waiter that loses the race can still move the winner's + * *fresh* lock aside and enter alongside it. + * + * So this does the opposite: it lets writers collide and makes the loser + * notice. Read `M0`, apply the mutation to get `M1`, write it, then read + * back `M2`. If `M2` equals `M1` nothing interleaved. If it does not, + * someone wrote between our write and our read — so re-apply onto what + * they left and try again. + * + * The comparison is over the **whole map**, not just the entry we touched, + * and that is the entire reason this works. Checking only our own key + * passes in precisely the case that loses data: our entry is present, and + * it is the *other* writer's that is gone. Trace two writers A and B from + * the same `M0` — A writes `MA`, B writes `MB` and clobbers A's entry. B + * verifies and sees `M2 === MB`, correctly: B did nothing wrong. A + * verifies, sees `M2 !== MA`, re-applies onto `MB`, and writes again. Both + * entries survive, and the writer that was clobbered is the one that + * repairs it. + * + * **The residual window, stated plainly rather than left to be + * rediscovered:** a process that crashes between its write and its + * verifying read cannot notice it was clobbered, and nothing else will. + * That is strictly narrower than the lock's failure — which lost updates + * with every participant alive and well — and unlike the lock it needs no + * primitive Node lacks. It is not mutual exclusion and this file does not + * claim to provide it. + * + * Reads deliberately do not participate: `writeStoreFile` is atomic + * (write-temp-then-rename), so a reader sees either the old file or the + * new one, never a torn one. */ - private async withFileLock(fn: () => Promise): Promise { - const release = await acquireLock(this.filePath); - try { - return await fn(); - } finally { - await release(); + private async mutate( + apply: (map: Record) => Record | null, + ): Promise { + for (let attempt = 0; attempt < MAX_WRITE_ATTEMPTS; attempt++) { + let current: Record; + try { + current = (await this.readMap()) ?? {}; + } catch (err) { + // Already a typed refusal (unreadable file / key mismatch) — + // rethrow rather than re-wrapping, so the specific advice survives. + if (err instanceof SecretStoreUnavailableError) throw err; + throw new SecretStoreUnavailableError( + `Could not read the secrets file at ${this.filePath}: ${err instanceof Error ? err.message : String(err)}`, + ); + } + const next = apply(current); + // `null` means "nothing to do" — a delete that matched no entry. No + // write, so nothing to verify. + if (next === null) return; + try { + await this.writeMap(next); + } catch (err) { + throw new SecretStoreUnavailableError( + `Could not write the secrets file at ${this.filePath}: ${err instanceof Error ? err.message : String(err)}`, + ); + } + let observed: Record; + try { + observed = (await this.readMap()) ?? {}; + } catch { + // We wrote successfully and cannot read it back — most likely + // another writer replaced it with something this passphrase cannot + // open. Retrying would not help and claiming success would be a + // guess, so fall through to the non-convergence error below. + continue; + } + if (sameMap(next, observed)) return; + // Someone wrote between our write and our read. Loop: `apply` runs + // again against what they left. } + throw new SecretStoreUnavailableError( + `Could not write the secrets file at ${this.filePath}: another process kept overwriting it (gave up after ${MAX_WRITE_ATTEMPTS} attempts). Its secrets are intact; the value you just entered was not saved.`, + ); } - /** Run `fn` with the file to itself, so concurrent callers can't lose writes. */ + /** + * Run `fn` with the file to itself, so concurrent callers can't lose + * writes. + * + * Keyed on the **resolved path, process-wide** rather than on this + * instance. Two `FileSecretStore`s pointing at one file are ordinary — + * the resolved store holds one and `absorbFileSecretsIntoKeyring` + * constructs another — and a per-instance queue does not order them + * against each other at all. They then race in-process, which the + * optimistic verify is only *supposed* to catch for writers in different + * processes: both read the same map, both write, and whichever verifies + * before the other's write lands sees its own value and returns happy. + * Measured, not theorised — two instances racing `set` lost an entry on + * the first run of the convergence test below. + * + * So the in-process case is made correct by construction here, and the + * verify-and-retry in {@link mutate} covers what this cannot see: a + * second Inspector process. + */ private serialize(fn: () => Promise): Promise { - const run = this.queue.then(fn); + const key = path.resolve(this.filePath); + const run = (fileQueues.get(key) ?? Promise.resolve()).then(fn); // Swallow on the chain only — the returned promise still rejects, so a // failed `set` surfaces to its caller while a later `set` is not poisoned // by it. This assignment is also why `this.queue` needs no `.catch()` of // its own above: it is only ever `Promise.resolve()` or an // already-swallowed promise, so it cannot reject. - this.queue = run.then( + const settled = run.then( () => {}, () => {}, ); + fileQueues.set(key, settled); + // Drop the entry once it is the tail, so a long-lived process does not + // accumulate one resolved promise per file it has ever touched. + void settled.then(() => { + if (fileQueues.get(key) === settled) fileQueues.delete(key); + }); return run; } @@ -820,6 +644,32 @@ export class FileSecretStore implements SecretStore { return out; } + /** + * The intolerant read — same lookup as {@link get}, minus the catch. + * + * Without this, `secretStoreGetStrict` fell back to `get` for the file + * store and turned every read failure into `null`. Both plaintext + * migrations *write* on `null` and then strip the disk copy, so a + * transient read error that cleared before the write let an older + * on-disk value replace a newer stored one — the same inversion the + * strict seam was introduced to prevent, still open on the store the + * seam was introduced for. + */ + async getStrict(serverId: string, field: string): Promise { + try { + const map = await this.serialize(() => this.readMap()); + return map?.[buildAccount(serverId, field)] ?? null; + } catch (err) { + // Typed refusals (key mismatch, unsupported envelope) already carry + // the advice that fits; anything else is a filesystem failure that + // must still reach the caller as something it knows to catch. + if (err instanceof SecretStoreUnavailableError) throw err; + throw new SecretStoreUnavailableError( + `Could not read the secrets file at ${this.filePath}: ${err instanceof Error ? err.message : String(err)}`, + ); + } + } + async get(serverId: string, field: string): Promise { try { const map = await this.serialize(() => this.readMap()); @@ -835,27 +685,10 @@ export class FileSecretStore implements SecretStore { async set(serverId: string, field: string, value: string): Promise { await this.serialize(() => - this.withFileLock(async () => { - let map: Record; - try { - map = (await this.readMap()) ?? {}; - } catch (err) { - // Already a typed refusal (unreadable file / key mismatch) — - // rethrow rather than re-wrapping, so the specific advice survives. - if (err instanceof SecretStoreUnavailableError) throw err; - throw new SecretStoreUnavailableError( - `Could not read the secrets file at ${this.filePath}: ${err instanceof Error ? err.message : String(err)}`, - ); - } - map[buildAccount(serverId, field)] = value; - try { - await this.writeMap(map); - } catch (err) { - throw new SecretStoreUnavailableError( - `Could not write the secrets file at ${this.filePath}: ${err instanceof Error ? err.message : String(err)}`, - ); - } - }), + this.mutate((map) => ({ + ...map, + [buildAccount(serverId, field)]: value, + })), ); } @@ -876,17 +709,16 @@ export class FileSecretStore implements SecretStore { private async deleteWhere(match: (key: string) => boolean): Promise { try { await this.serialize(() => - this.withFileLock(async () => { - const map = await this.readMap(); - if (!map) return; + this.mutate((map) => { const keys = Object.keys(map).filter(match); - if (keys.length === 0) return; - for (const key of keys) delete map[key]; + if (keys.length === 0) return null; + const next = { ...map }; + for (const key of keys) delete next[key]; // An emptied map keeps the file rather than unlinking it: its // presence is what records which encryption mode this install // settled on, so a later write can't silently switch modes on the // user. - await this.writeMap(map); + return next; }), ); } catch { diff --git a/core/auth/node/secret-store-selection.ts b/core/auth/node/secret-store-selection.ts index ed05cf3b..3a4ab124 100644 --- a/core/auth/node/secret-store-selection.ts +++ b/core/auth/node/secret-store-selection.ts @@ -49,7 +49,6 @@ import { secretStorageSummary, } from "../secret-storage-info.js"; import { - acquireLock, FileSecretStore, readSecretFilePermissions, tightenSecretFilePermissions, @@ -481,30 +480,27 @@ export async function absorbFileSecretsIntoKeyring( if (!fsSync.existsSync(filePath)) return; const file = new FileSecretStore({ filePath }); - // The whole read → copy → delete sequence runs under the same cross-process - // lock every mutation takes. Without it, another Inspector completing a - // `set` between our read and our delete has its brand-new secret removed - // and never copied — a write that reported success and then vanished, - // which is precisely the loss the lock exists to prevent, arrived at - // through the migration meant to preserve things. - let release: (() => Promise) | undefined; - try { - release = await acquireLock(filePath); - } catch (err) { - console.warn( - `\n[mcp-inspector] Could not lock the secrets file at ${filePath} to move it into the OS keychain, so it has been left in place: ${err instanceof Error ? err.message : String(err)}`, - ); - return; - } - try { - await handOffUnderLock(file, filePath, keyring); - } finally { - await release(); - } + await handOff(file, filePath, keyring); } -/** The hand-off body. Split out so the lock's release is unmissable. */ -async function handOffUnderLock( +/** + * The hand-off body: read the file, copy every entry into the keychain, + * then delete the file. + * + * **The delete is conditional, because there is no lock.** The store's + * mutations are optimistic (see `FileSecretStore.mutate`), so another + * Inspector can complete a `set` between our read and our delete — and + * removing the file then discards a secret whose write reported success, + * which is the exact loss this migration exists to prevent. So the + * contents are re-read immediately before the delete and compared against + * what was copied; if they differ, the new entries are copied too and the + * comparison is made again. + * + * Bounded, and it declines to delete rather than looping forever: leaving + * the file in place costs nothing (the keychain-wins rule makes the next + * run idempotent) while deleting it on a stale comparison costs a secret. + */ +async function handOff( file: FileSecretStore, filePath: string, keyring: SecretStore, @@ -562,6 +558,33 @@ async function handOffUnderLock( return; } + // 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( @@ -574,6 +597,18 @@ async function handOffUnderLock( } } +/** 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], + ); +} + /** * The active store's descriptor — for the banner and `GET /api/config`. * diff --git a/core/auth/node/secret-store.ts b/core/auth/node/secret-store.ts index 3ccf1c73..88fff072 100644 --- a/core/auth/node/secret-store.ts +++ b/core/auth/node/secret-store.ts @@ -407,13 +407,25 @@ export class KeyringSecretStore implements SecretStore { * caller that is about to write based on the answer. */ async getStrict(serverId: string, field: string): Promise { - const keyring = await loadKeyring(); - if (!keyring.ok) throw new KeychainUnavailableError(keyring.err); - const entry = new keyring.mod.AsyncEntry( - SERVICE_NAME, - buildAccount(serverId, field), - ); - return (await entry.getPassword()) ?? null; + try { + const keyring = await loadKeyring(); + if (!keyring.ok) throw new KeychainUnavailableError(keyring.err); + const entry = new keyring.mod.AsyncEntry( + SERVICE_NAME, + buildAccount(serverId, field), + ); + return (await entry.getPassword()) ?? null; + } catch (err) { + // "Strict" means the caller learns the read failed — not that it + // learns in a form it cannot handle. Both plaintext migrations keep + // their source file only when they catch `SecretStoreUnavailableError`; + // a raw binding error escaping here sails past that and turns + // `GET /api/servers` (or client-config loading) into a 500 instead of + // an abandoned migration. Same wrapping `set` already does, for the + // same reason. + if (err instanceof KeychainUnavailableError) throw err; + throw new KeychainUnavailableError(err); + } } async get(serverId: string, field: string): Promise { diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index cf2f58cf..45453f98 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -689,7 +689,13 @@ export function createRemoteApp( const payload = { ...options.initialConfig, writable, - ...(secretStorage ? { secretStorage } : {}), + // Assigned unconditionally, not spread-when-truthy. The resolver's + // contract is that `undefined` means "not known right now", and a + // conditional spread leaves the *startup* descriptor from + // `initialConfig` standing in its place — so a store that became + // undescribable would keep serving a stale, confident answer. Setting + // it to `undefined` is what clears it; `c.json` omits the key. + secretStorage, ...(options.sandboxUrl ? { sandboxUrl: options.sandboxUrl } : {}), }; return c.json(payload);