From 04dbc365441b934039f628704135f21cf15c1402 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 09:32:41 -0400 Subject: [PATCH 01/12] feat(auth): give the secrets file a real cross-process lock (#2082) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #1950 shipped FileSecretStore without cross-process mutual exclusion, by decision: an earlier revision hand-rolled 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 not closable with what Node exposes — claiming a stale lock atomically needs compare-and-swap on a directory entry (`renameat2(RENAME_EXCHANGE)`) — so it was replaced with optimistic verify-and-retry and the residual documented. #2082 settles that as "borrow, don't hand-roll". `proper-lockfile` is what npm itself locks with, and stale-takeover is precisely the problem it has already solved: it re-stats the lock directory after claiming it and gives the lock up when the mtime is not the one it wrote, so the loser of a takeover race releases instead of proceeding. - core/auth/node/file-lock.ts: `withSecretFileLock`. `realpath: false` so a file can be locked into existence (the very first `set` has no `secrets.json`, and the library's default resolves through `fs.realpath`); a warning in place of the library's `onCompromised`, which throws from a timer and would take the session down; and a degrade-with-one-warning path rather than a throw when no lock can be taken — this store exists for boxes missing the usual mechanism (#1848, #1905) and must not gain a new way to be unavailable. - FileSecretStore.mutate holds it across the whole read-modify-write. The optimistic verify stays underneath and is not redundant: a lock is advisory between the processes that take it, so the verify covers a writer outside this codebase and covers the degrade path. The in-process queue stays too, and gains a second job — proper-lockfile is not reentrant, so serializing per path keeps ELOCKED meaning "another process". - absorbFileSecretsIntoKeyring takes the same lock around orphan adoption and the atomic claim, behind a lock-free `readdir` fast path so the common startup (keychain available, no file ever written) neither creates a lock directory nor warns about one it could not create. proper-lockfile is a root `dependency` per the placement rule, and is named in all three bundler `external` lists: tsup externalizes what the *client's* manifest declares, so a root-only CJS package was being inlined into the ESM bundles, leaving esbuild's `Dynamic require of "path" is not supported` shim that killed `--cli` at import time. That rule was undocumented; it is now in AGENTS.md beside the placement rule that creates it, and mirrored into .github/copilot-instructions.md. Tests: 7 new in file-lock.test.ts driving a real second process (the existing suite structurally cannot — `serialize` orders in-process callers before the lock sees them), plus two for the migration fast path. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H Signed-off-by: cliffhall --- .github/copilot-instructions.md | 1 + AGENTS.md | 12 +- README.md | 6 +- clients/cli/tsup.config.ts | 7 + clients/tui/tsup.config.ts | 7 + .../integration/auth/node/file-lock.test.ts | 266 ++++++++++++++++++ .../auth/node/secret-store-selection.test.ts | 43 +++ clients/web/tsup.runner.config.ts | 7 + core/auth/node/file-lock.ts | 183 ++++++++++++ core/auth/node/file-secret-store.ts | 91 +++--- core/auth/node/secret-store-selection.ts | 134 ++++++--- package-lock.json | 45 +++ package.json | 2 + specification/v2_servers_file.md | 4 +- 14 files changed, 718 insertions(+), 90 deletions(-) create mode 100644 clients/web/src/test/integration/auth/node/file-lock.test.ts create mode 100644 core/auth/node/file-lock.ts diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 569593243a..1c19fb0fba 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -80,6 +80,7 @@ Both exist and do different jobs. Theme files (`src/theme/.ts`) custo - Dependencies reached only through **root-owned code with no manifest** (`test-servers/src`, `core/`) are declared at the root and aliased to the **repo root** in `vitest.shared.mts` — as `express` and `yaml` are — not to `/node_modules` like the other pins there. - **A dependency that renders React components must be bundled into the client that uses it, and is then not a root dependency.** An externalized package resolves its own `react` from wherever npm placed it in the *consumer's* tree, beside a React satisfying *its* peer range — looser than ours, which is all it takes to split React. `ink-form`/`ink-scroll-view` declare `">=18"`, so a consumer's React 18 satisfies them, the TUI ends up with two React instances, and it crashes on the first hook (#1952). Both are inlined via `noExternal` in `clients/tui/tsup.config.ts`. **`ink` is the one exemption, justified by cost (~1.4MB) — never by a peer range**: flag any claim that `">=19"` keeps npm from misplacing it, which is false and was in this repo once. What keeps it safe is the **root `react` range staying open to the whole major (`^19.0.0`)** so npm can dedupe with a consumer's pinned React 19; treat narrowing that range as reopening the bug. `clients/tui/__tests__/tsupConfig.test.ts` enforces the split, the root-declaration of exempt packages, and that range. - **Which section is a separate question from which manifest.** A package `core/` imports at runtime must be in root **`dependencies`**: client builds externalize npm packages, so a published install resolves them from the root manifest and devDependencies are absent there. Only test/build-only packages (`express`) belong in `devDependencies`. Flag a runtime `core/` import added to `devDependencies` — it passes every local check and breaks the published package. +- **A root-declared package that `core/` imports at runtime must also be named in each client's bundler `external` list** — `clients/{cli,tui}/tsup.config.ts` and `clients/web/tsup.runner.config.ts`, all three. Bundlers externalize what the *client's* manifest declares, and these packages are root-only by rule, so omitting them means they get bundled. For a CJS package inlined into an ESM bundle that is fatal: esbuild's `Dynamic require of "path" is not supported` shim throws at import time and the binary dies before parsing a flag (#2082, `proper-lockfile`). Flag a new root runtime dependency that is not added to all three. ## Tests and the coverage gate diff --git a/AGENTS.md b/AGENTS.md index 6cb8816bd0..e8bb92f78c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -68,7 +68,15 @@ v2/main/ │ │ │ # throws and the routes turn into a 503, and the keychain │ │ │ # probe), file-secret-store.ts (0600 JSON, AES-256-GCM when │ │ │ # MCP_INSPECTOR_SECRET_KEY is set — refuses to overwrite a -│ │ │ # file it cannot decrypt rather than destroying it), and +│ │ │ # file it cannot decrypt rather than destroying it), +│ │ │ # file-lock.ts (withSecretFileLock: the cross-process +│ │ │ # mutual exclusion #2082 settled on — proper-lockfile, +│ │ │ # borrowed rather than hand-rolled, because stale-takeover +│ │ │ # is what three review rounds of a mkdir election failed to +│ │ │ # get right; DEGRADES rather than throws when no lock can +│ │ │ # be taken, since this store exists for boxes missing the +│ │ │ # usual mechanism and must not gain a new way to fail), +│ │ │ # and │ │ │ # secret-store-selection.ts (the POLICY: explicit │ │ │ # MCP_INSPECTOR_SECRET_STORE wins, else probe the keychain, │ │ │ # else fall back LOUDLY — to memory in a container with @@ -222,6 +230,8 @@ The same **placement** rule covers anything reached only through **root-owned co - A package only the tests, the test servers, or the build tooling need belongs in **`devDependencies`** — `express`, added there by #1970. - `yaml` is in `dependencies` today even though its only importer is `test-servers/src/load-config.ts`. Left as-is deliberately (moving it changes what ships, which is not a docs change); if you touch it, confirm no published path reads YAML first. +**A root-declared package that `core/` imports at runtime must also be named in each client's bundler `external` list.** tsup and Vite externalize what the *client's* `package.json` declares, and a root-only dependency is in none of them — so it gets **bundled**, silently, and the placement rule above is what guarantees every such package is root-only. For a CJS package inlined into an ESM bundle that is fatal rather than merely wasteful: esbuild leaves a `Dynamic require of "path" is not supported` shim that throws at *import* time, so the binary dies before it parses a flag. `proper-lockfile` hit exactly that in #2082; `@napi-rs/keyring` is listed in all three for the same reason. The three lists are `clients/cli/tsup.config.ts`, `clients/tui/tsup.config.ts`, and `clients/web/tsup.runner.config.ts` — add a new package to **all** of them, since which client reaches it is a function of what `core/` imports, not of what the client's own code names. + **A dependency that renders React components must be bundled into the client that uses it, and is then not a root dependency.** An externalized package resolves its own `react` from wherever npm placed **it** in the consumer's tree, and npm places a package beside a React satisfying *that package's* peer range — looser than ours in every case here, which is all it takes to split React. `ink-form` and `ink-scroll-view` declare `">=18"`, satisfied by a consumer's React 18 while our React 19 nests underneath: the bundle renders through one React, those packages call hooks on another, and the TUI crashes on the first hook (#1952). Both are inlined by `clients/tui/tsup.config.ts` (`noExternal`) and declared only in `clients/tui/package.json`, where the build resolves them — declaring an inlined package at the root would just make consumers install a second, unused copy. **`ink` is the single exemption, and it is justified by cost, not by safety.** Bundling it works but adds ~1.4MB (`react-reconciler` + `yoga-layout`, plus a `createRequire` banner, since inlined CJS calls `require` at runtime and esbuild's ESM interop rejects that without a real `require` in scope). **Never justify an exemption by a peer range** — `ink` briefly carried "its `">=19"` peer keeps npm honest", which is false: a consumer pinning React 19.0 satisfies `">=19"` while a narrower range of ours nests underneath. What actually makes the exemption safe is a *different* lever: the **root `react` range stays open to the whole major (`^19.0.0`)**, so npm can dedupe our React with whatever React 19 a consumer pins and an external `ink` lands on the same copy the bundle uses. Narrowing it (e.g. back to `^19.2.4`) silently reopens the crash for the renderer itself, which breaks TUI *startup*, not just its forms. `clients/tui/__tests__/tsupConfig.test.ts` enforces all of it: React-rendering deps inlined, each exempt package both external and root-declared, and the root range pinned to `ink`'s peer floor. diff --git a/README.md b/README.md index fe12a668dc..a307cae660 100644 --- a/README.md +++ b/README.md @@ -502,9 +502,11 @@ Setting the passphrase later is safe — the next write upgrades an existing pla The Inspector writes the file `0600` and re-tightens it at startup if something loosened it. If it _cannot_ — the file belongs to another user, or the mount is read-only — it says so in the log rather than continuing to describe the file as protected, since on that box the mode claim above is not true. -**Two Inspectors, one file.** Within a process, mutations are serialized per file path, so a web session's own concurrent saves cannot lose each other. Across processes — a CLI run beside a web session — there is deliberately **no lock**: writers are allowed to collide and the loser is made to notice. Each mutation reads the file, applies its change, writes, then reads back and compares the whole map; if another process wrote in between, it re-applies onto what they left and retries, and after five lost rounds it fails loudly rather than returning as though the value were saved. +**Two Inspectors, one file.** Within a process, mutations are serialized per file path, so a web session's own concurrent saves cannot lose each other. Across processes — a CLI run beside a web session — each mutation takes an exclusive lock on `secrets.json.lock` for the whole read-modify-write, using [`proper-lockfile`](https://github.com/moxystudio/node-proper-lockfile) (the same library npm itself locks with). The lock expires 10 seconds after its holder stops refreshing it, so an Inspector that is killed mid-save does not leave the file unwritable. -This is **not mutual exclusion**, and the residual case is worth stating: the verify only catches a clobber that has already landed, so if one Inspector reads back *before* the other's write arrives, both report success and one value is gone. That needs two Inspectors writing the same file within the gap between one's write and its read-back — narrow, but not only a crash. An earlier build did take a lock (`secrets.json.lock`); it was removed because making a `mkdir` lock single-winner on a stale takeover needs a compare-and-swap on a directory entry that Node does not expose, so it had the same class of failure with several hundred more lines and no way to close it. +Underneath the lock, each mutation still reads the file, applies its change, writes, then reads back and compares the whole map; if something wrote in between it re-applies onto what was left and retries, failing loudly after five lost rounds rather than returning as though the value were saved. That check is not redundant with the lock — a lock only orders the writers that *take* it, so it covers an editor, a restored backup, or an Inspector older than this release. + +It is also what covers the lock being unavailable. This store exists for boxes where the usual mechanism isn't there, so a directory that can't hold a lock file — a read-only `$HOME`, a mount owned by another uid — makes the save proceed unlocked with a warning, rather than turning every `set` into a failure on exactly the deployments the store was written for. Three env vars affect where the file lands. `MCP_INSPECTOR_SECRET_STORE=keyring|file|memory` picks the store outright, bypassing the probe. `MCP_INSPECTOR_SECRET_FILE` names the file. Failing both, the file follows `MCP_STORAGE_DIR` — the same variable that relocates OAuth tokens and `client.json` — so mounting a volume at your configured storage directory is enough to make secrets durable there. diff --git a/clients/cli/tsup.config.ts b/clients/cli/tsup.config.ts index 03cf743193..c37897cae7 100644 --- a/clients/cli/tsup.config.ts +++ b/clients/cli/tsup.config.ts @@ -21,6 +21,13 @@ export default defineConfig({ noExternal: [/^@inspector\/core/], external: [ "@napi-rs/keyring", + // Root-declared (see the repo's dependency-placement rule) and CJS, which + // is the combination that bites: tsup externalizes what the *client's* + // package.json declares, so a root-only dependency is bundled unless named + // here — and inlining a CJS module into an ESM bundle leaves esbuild's + // `Dynamic require of "path" is not supported` shim, which throws at + // import time and takes the whole binary down before it parses a flag. + "proper-lockfile", "@modelcontextprotocol/client", "@modelcontextprotocol/core", "commander", diff --git a/clients/tui/tsup.config.ts b/clients/tui/tsup.config.ts index 2c0197a9a6..109011ba85 100644 --- a/clients/tui/tsup.config.ts +++ b/clients/tui/tsup.config.ts @@ -120,6 +120,13 @@ export default defineConfig({ "@modelcontextprotocol/client", "@modelcontextprotocol/core", "@napi-rs/keyring", + // Root-declared (see the repo's dependency-placement rule) and CJS, which + // is the combination that bites: tsup externalizes what the *client's* + // package.json declares, so a root-only dependency is bundled unless named + // here — and inlining a CJS module into an ESM bundle leaves esbuild's + // `Dynamic require of "path" is not supported` shim, which throws at + // import time and takes the whole binary down before it parses a flag. + "proper-lockfile", ], esbuildPlugins: [inkFormLabelPatch], esbuildOptions(options) { diff --git a/clients/web/src/test/integration/auth/node/file-lock.test.ts b/clients/web/src/test/integration/auth/node/file-lock.test.ts new file mode 100644 index 0000000000..eb9b4dd18d --- /dev/null +++ b/clients/web/src/test/integration/auth/node/file-lock.test.ts @@ -0,0 +1,266 @@ +/** + * `withSecretFileLock` against a real filesystem and a real second process + * (#2082). + * + * The property under test is cross-process mutual exclusion, and the + * existing suite structurally cannot reach it: `FileSecretStore.serialize` + * is one process-wide queue per path, so two in-process callers are ordered + * before the lock ever sees them. A child process is not scaffolding here — + * it is the only participant that can produce the interleaving. + */ +import { + describe, + it, + expect, + beforeEach, + afterEach, + vi, + type MockInstance, +} from "vitest"; +import { execFile } from "node:child_process"; +import { createRequire } from "node:module"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import { promisify } from "node:util"; +import { + withSecretFileLock, + resetFileLockWarnings, +} from "@inspector/core/auth/node/file-lock.js"; +import { FileSecretStore } from "@inspector/core/auth/node/file-secret-store.js"; + +const run = promisify(execFile); +const require_ = createRequire(import.meta.url); +/** + * Resolved in the parent and handed to the child. The child's cwd is not + * this repo, and `proper-lockfile` lives in the *root* install rather than + * `clients/web`'s, so a bare `require("proper-lockfile")` there resolves + * against whatever happens to be above the temp directory — usually nothing. + */ +const LOCKFILE_MODULE = require_.resolve("proper-lockfile"); + +let tmpDir: string; +let warn: MockInstance; +const filePath = (): string => path.join(tmpDir, "secrets.json"); + +beforeEach(async () => { + tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "inspector-lock-")); + resetFileLockWarnings(); + // These paths warn by design; asserting on the text is the point, and + // letting it reach the real console would bury the suite's output. + warn = vi.spyOn(console, "warn").mockImplementation(() => {}); +}); + +afterEach(async () => { + warn.mockRestore(); + await fs.rm(tmpDir, { recursive: true, force: true }); +}); + +/** Everything `console.warn` was handed this test, as one string. */ +const warnings = (): string => + warn.mock.calls.map((c) => String(c[0])).join("\n"); + +/** + * Hold the lock on `target` in a **real second process** for `holdMs`, and + * resolve once that child confirms it has it. + * + * Resolving on the child's confirmation rather than on a sleep is what makes + * the ordering assertions below meaningful: the parent starts contending + * only once the lock is provably held elsewhere, so a pass cannot come from + * the parent simply getting there first. + */ +async function holdLockInChildProcess( + target: string, + holdMs: number, +): Promise<{ ready: Promise; done: Promise }> { + const script = ` + const lockfile = require(${JSON.stringify(LOCKFILE_MODULE)}); + lockfile + .lock(${JSON.stringify(target)}, { realpath: false, stale: 10000 }) + .then(async (release) => { + process.stdout.write("acquired\\n"); + await new Promise((r) => setTimeout(r, ${holdMs})); + await release(); + process.stdout.write("released\\n"); + }) + .catch((err) => { + process.stdout.write("failed:" + err.code + "\\n"); + process.exitCode = 1; + }); + `; + const child = run(process.execPath, ["-e", script]); + let seenReady = false; + const done = child.then(({ stdout }) => { + expect(stdout).toContain("acquired"); + expect(stdout).toContain("released"); + }); + // `execFile` buffers, so the "acquired" line is only readable off the + // stream. Subscribe before awaiting anything, or the line is missed. + const ready = new Promise((resolve, reject) => { + child.child.stdout?.on("data", (chunk: Buffer) => { + if (!seenReady && chunk.toString().includes("acquired")) { + seenReady = true; + resolve(); + } + }); + child.catch(reject); + }); + return { ready, done }; +} + +describe("withSecretFileLock across processes", () => { + it("waits for a lock another process holds, then runs", async () => { + const target = filePath(); + const { ready, done } = await holdLockInChildProcess(target, 400); + await ready; + + const startedAt = Date.now(); + let ranAt = 0; + await withSecretFileLock(target, async () => { + ranAt = Date.now(); + }); + + // It waited rather than barging in. The child holds for 400ms and the + // retry schedule's first sleeps are tens of milliseconds, so anything + // above a floor well under 400 proves contention without pinning the + // assertion to the scheduler's exact wake-up. + expect(ranAt - startedAt).toBeGreaterThan(200); + // …and having waited, it did not report a degraded write. + expect(warnings()).toBe(""); + await done; + }, 20_000); + + it("locks a file that does not exist yet", async () => { + // The first `set` on a fresh install has no `secrets.json` — and + // `proper-lockfile` resolves its target through `fs.realpath` by + // default, which is `ENOENT` there. `realpath: false` is what makes the + // very first write lockable; without it the one call with nothing to + // fall back on is the one that runs unprotected. + const target = filePath(); + await expect(fs.stat(target)).rejects.toThrow(); + + let ran = false; + await withSecretFileLock(target, async () => { + ran = true; + }); + + expect(ran).toBe(true); + expect(warnings()).toBe(""); + }); + + it("serializes two FileSecretStores in different processes", async () => { + // The end-to-end shape from the issue: a CLI run beside a web session. + // The child holds the lock while the parent's `set` is in flight, so the + // parent's whole read-modify-write happens after the child is gone. + const target = filePath(); + const store = new FileSecretStore({ filePath: target }); + await store.set("srv", "env:FIRST", "1"); + + const { ready, done } = await holdLockInChildProcess(target, 300); + await ready; + await store.set("srv", "env:SECOND", "2"); + await done; + + const reader = new FileSecretStore({ filePath: target }); + expect(await reader.get("srv", "env:FIRST")).toBe("1"); + expect(await reader.get("srv", "env:SECOND")).toBe("2"); + }, 20_000); +}); + +describe("withSecretFileLock degrades rather than failing", () => { + it("runs the body anyway when the lock cannot be created, and says so once", async () => { + // A directory that does not exist stands in for every real variant — + // read-only `$HOME`, a mount owned by another uid, a filesystem without + // `mkdir` semantics. This store exists for boxes where the usual + // mechanism is missing, so it must not gain a new way to be unavailable. + const target = path.join(tmpDir, "no-such-dir", "secrets.json"); + + let ran = 0; + await withSecretFileLock(target, async () => { + ran += 1; + }); + await withSecretFileLock(target, async () => { + ran += 1; + }); + + expect(ran).toBe(2); + expect(warnings()).toContain("Could not take a lock on the secrets file"); + // Once per reason per process — a warning on every save would be noise + // on precisely the deployment that cannot act on it. + expect(warn).toHaveBeenCalledTimes(1); + }); + + it("still saves the secret when no lock can be taken", async () => { + // The degrade has to be end-to-end, not just in the helper: `set` on a + // lock-hostile directory must persist, falling back to the #1950 + // optimistic behaviour. + const target = filePath(); + const lockPath = `${target}.lock`; + // Occupy the lock's own path with a *file*, so `mkdir` fails EEXIST + // forever and no takeover can succeed — a permanent, non-ELOCKED + // failure rather than contention. + await fs.writeFile(lockPath, "not a lock directory", "utf-8"); + + const store = new FileSecretStore({ filePath: target }); + await store.set("srv", "env:MINE", "1"); + + const reader = new FileSecretStore({ filePath: target }); + expect(await reader.get("srv", "env:MINE")).toBe("1"); + expect(warnings()).toMatch( + /Could not take a lock|has held the secrets file/, + ); + }, 20_000); + + it("gives up waiting on a holder that never releases, and proceeds", async () => { + // Holding it from *this* process: `proper-lockfile` is not reentrant, so + // a second `lock()` on the same path fails `ELOCKED` exactly as a remote + // holder's would — the difference the in-process queue exists to keep + // out of the lock's way. + const target = filePath(); + const lockfile = require_(LOCKFILE_MODULE) as { + lock: (f: string, o: object) => Promise<() => Promise>; + }; + const release = await lockfile.lock(target, { + realpath: false, + stale: 60_000, + }); + + let ran = false; + await withSecretFileLock(target, async () => { + ran = true; + }); + await release(); + + // Proceeding is the lesser evil: refusing would lose the value outright, + // whereas proceeding falls back to the residual #1950 behaviour — and it + // says which of the two happened. + expect(ran).toBe(true); + expect(warnings()).toContain("without the lock"); + }, 20_000); +}); + +describe("withSecretFileLock reports what it cannot clean up", () => { + it("warns rather than crashing the process when the lock is taken over", async () => { + // `proper-lockfile`'s default `onCompromised` *throws*, from a timer + // with no caller on the stack — an uncaught exception that takes an + // Inspector session down. Driven for real: the lock directory is removed + // while held, which is what an operator "clearing a stuck lock" does, + // and the library's own refresh tick (`stale / 2`) notices. + const target = filePath(); + const result = await withSecretFileLock(target, async () => { + await fs.rm(`${target}.lock`, { recursive: true, force: true }); + await vi.waitFor( + () => expect(warnings()).toContain("was taken over by another process"), + { timeout: 20_000, interval: 250 }, + ); + return "saved"; + }); + + // The body's result is returned regardless. A compromised lock means the + // guarantee was lost, not that the work did not happen, and the release + // that then fails (`ELOCKNOTHELD`, since the library has already given + // the lock up) must not turn a completed save into a thrown error. + expect(result).toBe("saved"); + expect(warnings()).toContain("Could not release the lock"); + }, 30_000); +}); 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 7fb03da4bb..f7317d2e01 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 @@ -684,6 +684,49 @@ describe("absorbFileSecretsIntoKeyring", () => { return filePath; } + it("takes no lock when there is nothing to migrate (#2082)", async () => { + // The overwhelmingly common startup: a keychain is available and no file + // was ever written. Locking first would create and remove a lock + // directory on every run — and on a box whose storage directory does not + // exist yet the lock cannot be created at all, so the degrade path would + // warn about unprotected writes on every single run, with nothing to + // protect. + const filePath = path.join(tmpDir, "no-such-dir", "secrets.json"); + process.env.MCP_INSPECTOR_SECRET_FILE = filePath; + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const mod = await loadWithProbe(true); + + await mod.absorbFileSecretsIntoKeyring(new InMemorySecretStore()); + + expect(warn).not.toHaveBeenCalled(); + expect(existsSync(`${filePath}.lock`)).toBe(false); + }); + + it("still adopts an orphan when the live file is absent (#2082)", async () => { + // The fast path above must not be a `stat` of `secrets.json`: an + // interrupted migration leaves *only* the snapshot, which is precisely + // the case where there is everything to migrate and no live file. + const orphan = path.join(tmpDir, "secrets.json.migrating-123-abc"); + await fs.writeFile( + orphan, + JSON.stringify({ + version: 1, + encryption: "none", + secrets: { "srv:oauthClientSecret": "from-orphan" }, + }), + "utf-8", + ); + process.env.MCP_INSPECTOR_SECRET_FILE = path.join(tmpDir, "secrets.json"); + vi.spyOn(console, "warn").mockImplementation(() => {}); + const mod = await loadWithProbe(true); + const keyring = new InMemorySecretStore(); + + await mod.absorbFileSecretsIntoKeyring(keyring); + + expect(await keyring.get("srv", "oauthClientSecret")).toBe("from-orphan"); + expect(existsSync(orphan)).toBe(false); + }); + it("moves the file's secrets into the keychain and removes the file", async () => { const filePath = await seedFile({ "srv:oauthClientSecret": "from-file" }); process.env.MCP_INSPECTOR_SECRET_FILE = filePath; diff --git a/clients/web/tsup.runner.config.ts b/clients/web/tsup.runner.config.ts index 863c0e2964..a6512a48e7 100644 --- a/clients/web/tsup.runner.config.ts +++ b/clients/web/tsup.runner.config.ts @@ -27,6 +27,13 @@ export default defineConfig({ "atomically", "chokidar", "@napi-rs/keyring", + // Root-declared (see the repo's dependency-placement rule) and CJS, which + // is the combination that bites: tsup externalizes what the *client's* + // package.json declares, so a root-only dependency is bundled unless named + // here — and inlining a CJS module into an ESM bundle leaves esbuild's + // `Dynamic require of "path" is not supported` shim, which throws at + // import time and takes the whole binary down before it parses a flag. + "proper-lockfile", "@modelcontextprotocol/client", "@modelcontextprotocol/core", ], diff --git a/core/auth/node/file-lock.ts b/core/auth/node/file-lock.ts new file mode 100644 index 0000000000..66dcaea7b6 --- /dev/null +++ b/core/auth/node/file-lock.ts @@ -0,0 +1,183 @@ +/** + * Cross-process mutual exclusion for the secrets file (#2082). + * + * **Why a library and not a hand-rolled election.** #1950 shipped without a + * lock on purpose. An earlier revision of it *did* take 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 not + * closable with what Node exposes: claiming a stale lock atomically needs + * compare-and-swap on a directory entry (`renameat2(RENAME_EXCHANGE)`), and + * without it a waiter that loses the race can move the winner's *fresh* lock + * aside and enter alongside it. + * + * So the choice #2082 settles is not "lock versus no lock" but "hand-rolled + * versus borrowed". `proper-lockfile` is the borrowed one: it is what npm + * itself uses, and stale-takeover is precisely the problem it has already + * solved — it re-`stat`s the lock directory after claiming it and gives the + * lock up if the mtime is not the one it wrote, so a loser of the takeover + * race releases rather than proceeding. + * + * **What this does not remove.** The optimistic verify in + * {@link FileSecretStore.mutate} stays. A lock is an advisory convention + * between participants that take it, so it orders Inspector against + * Inspector and says nothing about an editor, a backup restore, or an + * Inspector old enough to predate this file. The verify is also what covers + * {@link withSecretFileLock} *declining* — see below. + * + * **Degrading rather than failing is deliberate.** The store's whole reason + * for existing is a box where the usual mechanism is unavailable (no + * keychain, #1848/#1905), so it must not acquire a *new* way to be + * unavailable. A directory that cannot hold a lock file — a read-only + * `$HOME`, a filesystem without `mkdir` semantics, a container mount owned + * by another uid — would otherwise turn every `set` into a hard failure on + * exactly the deployments this store was written for. So a lock that cannot + * be taken runs the body anyway, with the #1950 optimistic behaviour + * underneath it, and says so once. + */ + +import * as path from "node:path"; +// CJS-only package. A default import is the shape that survives every +// bundler this repo runs core/ through (tsup for cli/tui, vite's SSR/node +// graph for the web runner); named imports off a CJS module depend on +// lexer detection that esbuild and rollup disagree about. +import properLockfile from "proper-lockfile"; + +/** + * How long a lock may go untouched before another process may claim it. + * + * `proper-lockfile` refreshes the lock's mtime at `stale / 2` for as long as + * the holder is alive, so this is not "how long a mutation may take" — it is + * how long after a holder *dies* the file stays unwritable. 10s is the + * library's own default and the value npm ships with; a mutation is a read, + * an scrypt derivation and an atomic write, so the margin is enormous. + */ +const STALE_MS = 10_000; + +/** + * How long a waiter will keep trying before giving up. + * + * The retry schedule below tops out around 3s of waiting. That is long + * enough for any real mutation to finish (see above) and short enough that a + * pathological case surfaces as a slow save rather than a hung one. + */ +const RETRY = { + retries: 8, + factor: 2, + minTimeout: 20, + maxTimeout: 1_000, +} as const; + +/** Emitted once per process, not once per call — see {@link warnOnce}. */ +const warned = new Set(); + +/** + * Say why locking is unavailable here, once per reason per process. + * + * Once per *reason* rather than once overall: "the directory is read-only" + * and "the lock is held by something that never releases it" are different + * problems with different fixes, and collapsing them would print whichever + * happened first and hide the other for the life of the process. Keyed on + * the message, which already encodes the reason. + */ +function warnOnce(message: string): void { + if (warned.has(message)) return; + warned.add(message); + console.warn(`[mcp-inspector] ${message}`); +} + +/** Test seam: forget which warnings have been emitted. */ +export function resetFileLockWarnings(): void { + warned.clear(); +} + +/** + * Did `proper-lockfile` decline because someone else holds the lock? + * + * A bare cast rather than a guarded narrowing: the only caller is the `catch` + * around `properLockfile.lock`, and the library rejects with a real `Error` + * carrying a `code` on every path. Guarding would add branches nothing can + * exercise, which is a worse trade than an assertion whose one caller is two + * lines away. + */ +const isHeldElsewhere = (err: unknown): boolean => + (err as NodeJS.ErrnoException).code === "ELOCKED"; + +/** + * The message from whatever was thrown. + * + * Same reasoning as above for the non-`Error` arm — `proper-lockfile` does not + * reject with one — except that this is used where a thrown non-`Error` would + * otherwise be reported as `[object Object]`, so the fallback earns its place + * even though nothing can provoke it. + */ +/* v8 ignore next 2 -- @preserve: the non-Error arm is unreachable via + proper-lockfile, which rejects only with Errors. */ +const describeError = (err: unknown): string => + err instanceof Error ? err.message : String(err); + +/** + * Run `fn` holding an exclusive cross-process lock on `filePath`. + * + * The lock is `.lock`, a directory beside the secrets file rather + * than inside it — `proper-lockfile` never opens or truncates the file it + * guards, so a lock that outlives its holder can only ever block a write, + * never damage one. + * + * `realpath: false` is load-bearing: by default the library resolves the + * target through `fs.realpath`, which fails `ENOENT` on a secrets file that + * does not exist yet — i.e. on the very first `set`, the one call that has + * nothing to fall back on. Resolving the path lexically instead lets a file + * be locked into existence. The cost is that two paths reaching one file + * through different symlinks take different locks; the store resolves its + * path once at construction and every caller goes through it, so that is a + * shape this codebase does not produce. + * + * Returns whatever `fn` returns. `fn` runs exactly once either way — the + * lock's absence changes the guarantee, never whether the work happens. + */ +export async function withSecretFileLock( + filePath: string, + fn: () => Promise, +): Promise { + const target = path.resolve(filePath); + let release: (() => Promise) | undefined; + try { + release = await properLockfile.lock(target, { + realpath: false, + stale: STALE_MS, + retries: RETRY, + // The library's default `onCompromised` *throws* — from a timer, with + // no caller on the stack, so it lands as an uncaught exception and + // takes the process down. A compromised lock means someone declared + // ours stale and took it; the write in flight is at risk, which is + // worth saying and is not worth killing an Inspector session over. + onCompromised: (err) => + warnOnce( + `The lock on the secrets file at ${target} was taken over by another process while a write was in progress (${err.message}). If a secret you just saved is missing, save it again.`, + ), + }); + } catch (err) { + warnOnce( + isHeldElsewhere(err) + ? `Another process has held the secrets file at ${target} for longer than this write was willing to wait, so the write went ahead without the lock. If two Inspectors are saving secrets at once, one of them may not be saved.` + : `Could not take a lock on the secrets file at ${target} (${describeError(err)}), so writes to it are not protected against another process writing at the same moment.`, + ); + return fn(); + } + try { + return await fn(); + } finally { + try { + await release(); + } catch (err) { + // The body already ran and its result is being returned; a release + // that failed means the lock was taken from us (declared stale while + // we held it) or the directory went away. Neither is worth turning a + // successful save into a failure, but a silent catch would leave a + // lock nobody can explain, so say it. + warnOnce( + `Could not release the lock on the secrets file at ${target} (${describeError(err)}). It expires on its own after ${STALE_MS / 1000}s.`, + ); + } + } +} diff --git a/core/auth/node/file-secret-store.ts b/core/auth/node/file-secret-store.ts index edff72c19e..4d81275188 100644 --- a/core/auth/node/file-secret-store.ts +++ b/core/auth/node/file-secret-store.ts @@ -62,6 +62,7 @@ import * as crypto from "node:crypto"; import * as fs from "node:fs/promises"; import * as path from "node:path"; import { readStoreFile, writeStoreFile } from "../../storage/store-io.js"; +import { withSecretFileLock } from "./file-lock.js"; import { SecretStoreUnavailableError, type SecretBulkRequest, @@ -597,30 +598,33 @@ export class FileSecretStore implements SecretStore { } /** - * Run `fn` with the file to itself across *processes* as well. + * Apply a mutation to the file, under a cross-process lock, and confirm it + * survived. * - * The in-process queue is necessary and not sufficient: a durable secrets - * file is shared state, and a second Inspector on the same box — a CLI run - * beside a web session is the ordinary case, not a contrived one — reads - * the same "before" map and then atomically replaces the file, dropping - * whatever the first process had just added. Both writes report success; - * one secret is simply gone. + * **Two mechanisms, and they answer different questions** — this is not + * belt-and-braces. * - * Apply a mutation to the file, and confirm it survived. + * The lock ({@link withSecretFileLock}, #2082) is the one that provides + * mutual exclusion: a durable secrets file is shared state, and a second + * Inspector on the same box — a CLI run beside a web session is the + * ordinary case, not a contrived one — otherwise reads the same "before" + * map and then atomically replaces the file, dropping whatever the first + * process had just added. Both writes report success; one secret is simply + * gone. Held across the read *and* the write, so no other holder can + * observe or replace the map in between. * - * **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. + * The verify covers what a lock structurally cannot. A lock is an advisory + * convention between the processes that take it, so it says nothing about + * a writer that does not: an editor, a restored backup, a `jq` one-liner, + * or an Inspector predating #2082. It is also the fallback when the lock + * is unavailable at all — `withSecretFileLock` runs the body anyway on a + * read-only or lock-hostile directory rather than failing a `set` on + * exactly the deployments this store exists for (#1848, #1905). * - * 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. + * So: 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. * Checking only our own key would pass in precisely the case that loses @@ -629,32 +633,29 @@ export class FileSecretStore implements SecretStore { * repairs it — A writes `MA`, B writes `MB` over it, B verifies `MB` * correctly, A verifies, sees `M2 !== MA`, re-applies onto `MB`. * - * **This is not mutual exclusion, and the gap is wider than a crash.** - * The verify only catches a clobber that has already landed. Order the - * same two writers as write-A, verify-A, write-B, verify-B and both - * succeed while A's entry is gone: A's verify ran before B's write, so - * there was nothing yet to see, and B did nothing wrong. Nothing detects - * it afterwards. A crash between write and verify is one instance of the - * same shape, not the whole of it — an earlier version of this comment - * said otherwise, which understated it. - * - * What that buys, stated without overselling: two processes must write the - * same file within the window between one's write and its read-back, and - * the loss is one secret that reported success. The lock this replaced - * lost updates in a *wider* set of interleavings, with every participant - * alive, and could not be closed without a primitive Node does not expose - * (`renameat2(RENAME_EXCHANGE)`); this costs ~220 fewer lines and converges - * in every interleaving where the clobber lands before the verify. If the - * residual matters for a deployment, the answer is a real lock — an - * OS-backed one from a dedicated library — not another hand-rolled - * election. + * **What is left, stated without overselling.** Against an unlocked writer + * the verify is not mutual exclusion: it only catches a clobber that has + * already landed, so ordering the two as write-A, verify-A, write-B, + * verify-B loses A's entry with both reporting success. That residual is + * now confined to a writer outside this codebase, or to a directory where + * no lock could be taken — and the latter announces itself, once, on the + * console. * * 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. + * new one, never a torn one. Taking the lock for them would serialize + * every `GET /api/servers` behind whatever else holds it, to prevent + * nothing. */ private async mutate( apply: (map: Record) => Record | null, + ): Promise { + await withSecretFileLock(this.filePath, () => this.mutateLocked(apply)); + } + + /** {@link mutate}'s body, with the lock already held. */ + private async mutateLocked( + apply: (map: Record) => Record | null, ): Promise { for (let attempt = 0; attempt < MAX_WRITE_ATTEMPTS; attempt++) { let current: Record; @@ -713,9 +714,15 @@ export class FileSecretStore implements SecretStore { * 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 + * So the in-process case is made correct by construction here, and + * {@link mutate}'s cross-process lock covers what this cannot see: a * second Inspector process. + * + * The queue is *also* what keeps that lock usable. `proper-lockfile` is + * not reentrant — a second `lock()` on a path this process already holds + * fails `ELOCKED`, which is indistinguishable from a genuine remote + * holder. Serializing here means the lock is only ever contended between + * processes, which is the only contention it is asked to arbitrate. */ private serialize(fn: () => Promise): Promise { const key = path.resolve(this.filePath); diff --git a/core/auth/node/secret-store-selection.ts b/core/auth/node/secret-store-selection.ts index cb468af932..4cd6937583 100644 --- a/core/auth/node/secret-store-selection.ts +++ b/core/auth/node/secret-store-selection.ts @@ -54,6 +54,7 @@ import { readSecretFilePermissions, tightenSecretFilePermissions, } from "./file-secret-store.js"; +import { withSecretFileLock } from "./file-lock.js"; import { KeyringSecretStore, parseAccount, @@ -532,51 +533,76 @@ export async function absorbFileSecretsIntoKeyring( ): Promise { const filePath = defaultSecretFilePath(); - // A crash between the claim and the delete leaves only - // `secrets.json.migrating-`. Checking the canonical path alone then - // reports "nothing to migrate", the keychain is selected, and every stored - // credential silently disappears — the claim protecting the delete having - // introduced a way to lose everything. Adopt any orphan first. - await recoverOrphanedSnapshots(filePath); - - if (!fsSync.existsSync(filePath)) return; - - // **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. + // Cheap, lock-free "is there anything at all to do". Taking the lock first + // would mean every startup on the overwhelmingly common path — a keychain + // is available and no file was ever written — creates and removes a lock + // directory, and on a box whose storage dir does not exist yet the lock + // cannot be created at all, so `withSecretFileLock` would warn about + // unprotected writes on every single run with nothing to protect. // - // `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. - // A per-attempt nonce, not just the pid. `recoverOrphanedSnapshots` - // deliberately leaves an orphan in place when a live `secrets.json` also - // exists — and a pid-only name is reusable across restarts (pid 1 on every - // container start), so the next claim would `rename` straight over that - // orphan and permanently discard secrets it may uniquely hold. - const staged = `${filePath}.migrating-${process.pid}-${randomUUID()}`; - try { - await fs.rename(filePath, staged); - } catch (err) { - const code = (err as NodeJS.ErrnoException).code; - if (code !== "ENOENT") { - // Only ENOENT means someone else claimed it. Anything else — EACCES on - // the directory, EROFS — leaves the file exactly where it is, and - // returning quietly would select the keychain while file-backed - // secrets sit there unreadable by anything, with nothing said. - console.warn( - `\n[mcp-inspector] Could not claim the secrets file at ${filePath} for migration into the OS keychain (${code ?? "unknown error"}), so it has been left in place. Its secrets are not visible to this session.`, - ); + // Racy by construction, and that is fine: it can only be wrong by saying + // "nothing here" about a file created a moment later, which is a file the + // next run migrates — the same outcome as a writer that recreates the path + // after the claim below. Everything that *acts* re-checks under the lock. + if (!(await anythingToMigrate(filePath))) return; + + // Orphan adoption and the claim below both move the live path around, so + // they run under the same cross-process lock a `set` takes (#2082) — + // otherwise a concurrent writer's atomic rename can land between the two + // and be adopted, claimed, or clobbered depending on the interleaving. + // The lock is released before the hand-off: that part reads a snapshot + // nobody else can reach, and holding it across a keychain round-trip per + // secret would block every writer for the duration of a migration. + const claimed = await withSecretFileLock(filePath, async () => { + // A crash between the claim and the delete leaves only + // `secrets.json.migrating-`. Checking the canonical path alone then + // reports "nothing to migrate", the keychain is selected, and every stored + // credential silently disappears — the claim protecting the delete having + // introduced a way to lose everything. Adopt any orphan first. + await recoverOrphanedSnapshots(filePath); + + if (!fsSync.existsSync(filePath)) return null; + + // **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. + // A per-attempt nonce, not just the pid. `recoverOrphanedSnapshots` + // deliberately leaves an orphan in place when a live `secrets.json` also + // exists — and a pid-only name is reusable across restarts (pid 1 on every + // container start), so the next claim would `rename` straight over that + // orphan and permanently discard secrets it may uniquely hold. + const staged = `${filePath}.migrating-${process.pid}-${randomUUID()}`; + try { + await fs.rename(filePath, staged); + } catch (err) { + const code = (err as NodeJS.ErrnoException).code; + if (code !== "ENOENT") { + // Only ENOENT means someone else claimed it. Anything else — EACCES on + // the directory, EROFS — leaves the file exactly where it is, and + // returning quietly would select the keychain while file-backed + // secrets sit there unreadable by anything, with nothing said. + console.warn( + `\n[mcp-inspector] Could not claim the secrets file at ${filePath} for migration into the OS keychain (${code ?? "unknown error"}), so it has been left in place. Its secrets are not visible to this session.`, + ); + } + return null; } - return; - } + return staged; + }); + if (claimed === null) return; + const staged = claimed; const file = new FileSecretStore({ filePath: staged }); // True only when every value reached the keychain *and* there was @@ -625,6 +651,28 @@ export async function absorbFileSecretsIntoKeyring( } } +/** + * Is there a `secrets.json`, or a snapshot orphaned by an interrupted + * migration, worth taking the lock for? + * + * One `readdir` rather than a `stat` of the canonical path: an orphan is the + * case where the live file is *absent* and there is still everything to + * migrate, so checking only `secrets.json` would skip the recovery that + * exists because skipping it loses every stored credential. + */ +async function anythingToMigrate(filePath: string): Promise { + const base = path.basename(filePath); + try { + return (await fs.readdir(path.dirname(filePath))).some( + (name) => name === base || name.startsWith(`${base}.migrating-`), + ); + } catch { + // No storage directory yet — the first run on a fresh install, and the + // path this check exists to keep quiet. + return false; + } +} + /** * Adopt a snapshot left behind by a process that died mid-migration. * diff --git a/package-lock.json b/package-lock.json index d934e96ecb..d295098859 100644 --- a/package-lock.json +++ b/package-lock.json @@ -26,6 +26,7 @@ "ink": "^6.0.0", "open": "^10.2.0", "pino": "^9.14.0", + "proper-lockfile": "^4.1.2", "react": "^19.0.0", "undici": "^8.5.0", "vite": "^8.1.5", @@ -37,6 +38,7 @@ }, "devDependencies": { "@eslint/js": "^10.0.1", + "@types/proper-lockfile": "^4.1.4", "eslint": "^10.8.0", "express": "^5.2.1", "globals": "^17.7.0", @@ -993,6 +995,23 @@ "dev": true, "license": "MIT" }, + "node_modules/@types/proper-lockfile": { + "version": "4.1.4", + "resolved": "https://registry.npmjs.org/@types/proper-lockfile/-/proper-lockfile-4.1.4.tgz", + "integrity": "sha512-uo2ABllncSqg9F1D4nugVl9v93RmjxF6LJzQLMLDdPaXCUIDPeOJ21Gbqi43xNKzBi/WQ0Q0dICqufzQbMjipQ==", + "dev": true, + "license": "MIT", + "dependencies": { + "@types/retry": "*" + } + }, + "node_modules/@types/retry": { + "version": "0.12.5", + "resolved": "https://registry.npmjs.org/@types/retry/-/retry-0.12.5.tgz", + "integrity": "sha512-3xSjTp3v03X/lSQLkczaN9UIEwJMoMCA1+Nb5HfbJEQWogdeQIyVtTvxPXDQjZ5zws8rFQfVfRdz03ARihPJgw==", + "dev": true, + "license": "MIT" + }, "node_modules/@typescript-eslint/eslint-plugin": { "version": "8.65.0", "resolved": "https://registry.npmjs.org/@typescript-eslint/eslint-plugin/-/eslint-plugin-8.65.0.tgz", @@ -2394,6 +2413,12 @@ "url": "https://github.com/sponsors/ljharb" } }, + "node_modules/graceful-fs": { + "version": "4.2.11", + "resolved": "https://registry.npmjs.org/graceful-fs/-/graceful-fs-4.2.11.tgz", + "integrity": "sha512-RbJ5/jmFcNNCcDV5o9eTnBLJ/HszWV0P73bc+Ff4nS/rJj+YaS6IGyiOL0VoBYX+l1Wrl3k63h/KrH+nhJ0XvQ==", + "license": "ISC" + }, "node_modules/has-symbols": { "version": "1.1.0", "resolved": "https://registry.npmjs.org/has-symbols/-/has-symbols-1.1.0.tgz", @@ -3453,6 +3478,17 @@ ], "license": "MIT" }, + "node_modules/proper-lockfile": { + "version": "4.1.2", + "resolved": "https://registry.npmjs.org/proper-lockfile/-/proper-lockfile-4.1.2.tgz", + "integrity": "sha512-TjNPblN4BwAWMXU8s9AEz4JmQxnD1NNL7bNOY/AKUzyamc379FWASUhc/K1pL2noVb+XmZKLL68cjzLsiOAMaA==", + "license": "MIT", + "dependencies": { + "graceful-fs": "^4.2.4", + "retry": "^0.12.0", + "signal-exit": "^3.0.2" + } + }, "node_modules/proxy-addr": { "version": "2.0.7", "resolved": "https://registry.npmjs.org/proxy-addr/-/proxy-addr-2.0.7.tgz", @@ -3593,6 +3629,15 @@ "url": "https://github.com/sponsors/sindresorhus" } }, + "node_modules/retry": { + "version": "0.12.0", + "resolved": "https://registry.npmjs.org/retry/-/retry-0.12.0.tgz", + "integrity": "sha512-9LkiTwjUh6rT555DtE9rTX+BKByPfrMzEAtnlEtdEwr3Nkffwiihqe2bWADg+OQRjt9gl6ICdmB/ZFDCGAtSow==", + "license": "MIT", + "engines": { + "node": ">= 4" + } + }, "node_modules/rolldown": { "version": "1.2.1", "resolved": "https://registry.npmjs.org/rolldown/-/rolldown-1.2.1.tgz", diff --git a/package.json b/package.json index 6d1ee7f8aa..4001d116c8 100644 --- a/package.json +++ b/package.json @@ -94,6 +94,7 @@ "ink": "^6.0.0", "open": "^10.2.0", "pino": "^9.14.0", + "proper-lockfile": "^4.1.2", "react": "^19.0.0", "undici": "^8.5.0", "vite": "^8.1.5", @@ -105,6 +106,7 @@ }, "devDependencies": { "@eslint/js": "^10.0.1", + "@types/proper-lockfile": "^4.1.4", "eslint": "^10.8.0", "express": "^5.2.1", "globals": "^17.7.0", diff --git a/specification/v2_servers_file.md b/specification/v2_servers_file.md index 53a7a0a2cd..44d02cf0a7 100644 --- a/specification/v2_servers_file.md +++ b/specification/v2_servers_file.md @@ -261,11 +261,11 @@ Each server entry may carry these Inspector-extension fields at the top level: - **Secret store selection (#1950)**: #1356 assumed a keychain. On a host without one — the published container (no D-Bus session, #1848), Android/Termux (no prebuilt binary, #1905), a minimal Linux install — `set` hard-failed, so those users could not persist an OAuth client secret or a stdio `env:` value **at all**. #1950 adds the two missing implementations plus the policy that picks one, and the surfaces that say which was picked. - **Selection**: `MCP_INSPECTOR_SECRET_STORE=keyring|file|memory` wins outright. Otherwise the keychain is _probed_ — an `AsyncEntry` construction plus a real read, because the container that motivated #1848 imports the package fine and only fails when it reaches for a Secret Service that isn't there. A read and not a write, deliberately: probing by writing would deposit a value in the user's login keyring at every startup for a store they may never use. If the probe fails the fallback is **`memory`** in a container whose secrets directory is not on a mount, and **`file`** otherwise — so mounting the volume the README already recommends for the catalog flips the same run to durable storage with no configuration, and an unmounted container gets the honest answer rather than a file `docker run --rm` will discard. Resolution is cached per process so the startup banner, `GET /api/config`, and the store doing the writing cannot disagree. - **`FileSecretStore` at rest**: one JSON document at `~/.mcp-inspector/secrets.json` (or `MCP_INSPECTOR_SECRET_FILE`, else under `MCP_STORAGE_DIR`), written `0600` and re-tightened at startup. With `MCP_INSPECTOR_SECRET_KEY` set it is AES-256-GCM with a scrypt-derived key and a per-write random salt; without it the values are in the clear, and _that_ is what the loud banner and the warning-toned modal footer are about. The **whole map is encrypted as a unit**, not value-by-value, so the account names (`serverId:field`) are hidden too — a per-value scheme would leave a readable index of which servers you hold a client secret for. Upgrading is lazy: setting the passphrase later re-encrypts on the next write rather than rewriting the file during a run that may never touch a secret, and the descriptor reports `pendingEncryption` until it does. A file that can no longer be decrypted reads as empty but **refuses to be written**, because replacing a file of still-valid secrets to satisfy an additive request destroys data. - - **Concurrency, and what it does not promise**: within a process, mutations are serialized per **resolved file path** (not per store instance — two `FileSecretStore`s on one file are ordinary, since the resolved store holds one and the keychain hand-off builds another). Across processes there is **no lock**. An earlier iteration had a `mkdir` election with an owner stamp, heartbeat and stale-takeover; three review rounds each found a real race, and the last is not closable with what Node exposes — claiming a stale lock needs compare-and-swap on a directory entry (`renameat2(RENAME_EXCHANGE)`). It was replaced with optimistic concurrency: read `M0`, apply, write `M1`, read back `M2`, and re-apply onto whatever a concurrent writer left if they differ, bounded, with `set` throwing on non-convergence rather than returning as though the value were saved. The comparison is over the **whole map** — checking only your own entry passes in exactly the case that loses data, because yours is present and the other writer's is gone. **This is not mutual exclusion.** The verify only catches a clobber that has already landed, so ordering the two writers write-A / verify-A / write-B / verify-B leaves both reporting success with A's entry gone — A's verify ran before there was anything to see. A crash between write and verify is one instance of that shape, not the whole of it. The window is narrower than the lock's (which lost updates across a wider set of interleavings, with every participant alive) and needs no primitive Node lacks, but it is a real residual and is documented as one. If a deployment needs the guarantee, the answer is an OS-backed lock from a dedicated library, not another hand-rolled election. + - **Concurrency**: within a process, mutations are serialized per **resolved file path** (not per store instance — two `FileSecretStore`s on one file are ordinary, since the resolved store holds one and the keychain hand-off builds another). Across processes, each mutation holds an exclusive `proper-lockfile` lock on `secrets.json.lock` for the whole read-modify-write (#2082). The in-process queue is what keeps that usable: `proper-lockfile` is not reentrant, so a second `lock()` from the same process fails `ELOCKED` and would be indistinguishable from a genuine remote holder. An earlier iteration hand-rolled the lock — a `mkdir` election with an owner stamp, heartbeat and stale-takeover — and three review rounds each found a real race, the last not closable with what Node exposes (claiming a stale lock needs compare-and-swap on a directory entry, `renameat2(RENAME_EXCHANGE)`). #2082 settled that as "borrow, don't hand-roll": stale-takeover is exactly the problem the library has already solved, re-`stat`ing the lock after claiming it so the loser of a takeover race releases instead of proceeding. **The optimistic verify stays underneath it**, and is not redundant: read `M0`, apply, write `M1`, read back `M2`, re-apply onto whatever a concurrent writer left if they differ, bounded, with `set` throwing on non-convergence rather than returning as though the value were saved. The comparison is over the **whole map** — checking only your own entry passes in exactly the case that loses data, because yours is present and the other writer's is gone. A lock is advisory between the processes that take it, so the verify is what covers a writer outside this codebase (an editor, a restored backup, an Inspector predating #2082) and what covers the lock being *unavailable*: `withSecretFileLock` runs the body anyway, warning once, on a directory that cannot hold a lock file — this store exists for boxes where the usual mechanism is missing (#1848, #1905) and must not acquire a new way to be unavailable. Reads take no lock; `writeStoreFile` is atomic, so a reader sees the old file or the new one, never a torn one. - **Strict reads (`getStrict`)**: `get` is tolerant by contract, and that tolerance is wrong for exactly one caller — a migration that treats `null` as proof of absence and then _writes_. A transient read failure would look like "nothing there", and the write would replace a newer stored value with an older on-disk copy, inverting the keychain-wins rule the migration is built on. `getStrict` throws instead, and both plaintext migrations plus the keychain hand-off use it. Two traps this hit on the way in, both worth remembering: the seam must be forwarded by `DeferredSecretStore` (the store production actually uses, so without forwarding the strictness existed only in tests that injected a concrete store), and it must be implemented by _every_ store rather than falling back to `get` for the one it was introduced for. - **Bulk reads (`getMany`)**: rehydration asked field by field, and `FileSecretStore.get` reads and decrypts the entire file per call. scrypt at `N=16384` measures ~23ms, so an encrypted catalog spent ~450ms of pure key derivation on every `GET /api/servers` — a visible stall rather than a micro-optimization. The seam takes a **list of `{ serverId, fields }` across servers**, and both rehydration callers pass the whole catalog in one request, so `FileSecretStore` decrypts once per rehydration; stores for which per-field reads are already cheap (the keychain) fall back to parallel `get`. The cross-server shape is the point: a per-server version shipped first and left a 20-server catalog paying 20 serialized derivations, because both callers iterate servers — the same stall reached one server at a time instead of one field at a time. - **Durability gate on migration**: the plaintext-stripping migrations only delete the disk copy once the value is somewhere that outlives the process (`isDurable`). Against a session-scoped store, stripping would trade a secret that survives restarts for one that dies with the process — and it runs on an ordinary `GET`, so merely opening the app would destroy it. - - **Hand-off when a keychain appears**: install libsecret after storing secrets in a file, and the next run selects the keychain and stops seeing them — still on disk, read by nothing, nothing visibly broken. `absorbFileSecretsIntoKeyring` copies them over on that run, under the same keychain-wins rule, and deletes the file **only** on complete success. Since the store takes no lock, the source is **claimed atomically** first: the live `secrets.json` is renamed to a unique snapshot (pid plus a per-attempt nonce, so a staging path can never be reused — pid 1 recurs on every container start), the migration reads only that snapshot, and a writer that recreates the live path is untouched and migrated on the next run. The snapshot is deleted only on a complete hand-off; otherwise it is restored with `link` + `unlink` rather than `rename`, since POSIX `rename` silently replaces its destination and would overwrite a newer live file. A snapshot left behind by a process that died mid-migration is adopted at startup — checking only the canonical path would otherwise report "nothing to migrate" while every stored credential quietly disappeared. + - **Hand-off when a keychain appears**: install libsecret after storing secrets in a file, and the next run selects the keychain and stops seeing them — still on disk, read by nothing, nothing visibly broken. `absorbFileSecretsIntoKeyring` copies them over on that run, under the same keychain-wins rule, and deletes the file **only** on complete success. The claim runs under the same cross-process lock a `set` takes, and the source is **claimed atomically** within it: the live `secrets.json` is renamed to a unique snapshot (pid plus a per-attempt nonce, so a staging path can never be reused — pid 1 recurs on every container start), the migration reads only that snapshot, and a writer that recreates the live path is untouched and migrated on the next run. The snapshot is deleted only on a complete hand-off; otherwise it is restored with `link` + `unlink` rather than `rename`, since POSIX `rename` silently replaces its destination and would overwrite a newer live file. A snapshot left behind by a process that died mid-migration is adopted at startup — checking only the canonical path would otherwise report "nothing to migrate" while every stored credential quietly disappeared. - **Surfacing it**: the active store rides `GET /api/config` as a `secretStorage` descriptor and is stated in a permanent footer at the bottom of every dialog that accepts a secret: Client Settings (the enterprise IdP client secret), Server Settings (the per-server OAuth client secret and stdio `env:` values), and Server Config (stdio `env:` values). That third one was missed at first, which made it the one dialog taking secrets with no disclosure at all — so the count here is load-bearing rather than descriptive. A startup banner is seen once by whoever started the process; a toast is seen once; a dismissible banner is by design the thing a user dismisses before doing the work it describes. The descriptor is re-derived per request rather than cached, because `plaintext`/`pendingEncryption` describe bytes this very process changes. - **Hard-cutover legacy behavior (per #1358 decision 4)**: files written by the one pre-#1358 build of v2/main have a nested `settings` block. `normalizeMcpServers` drops the node on read and logs a one-line warn including the server id; the persisted headers / metadata / timeouts / OAuth credentials are intentionally lost on first read. Users re-enter them via the settings form (or hand-edit the file into the flat shape). v2 has not shipped a stable release with the nested shape, so the blast radius is the small set of v2/main dogfooders who edited per-server settings between #1353 merging and this change. From 16613b115dc90a69841225aeb6465e8bfbf0da94 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 09:49:01 -0400 Subject: [PATCH 02/12] fix(auth): address Copilot review round 1 on the secrets-file lock (#2082) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Create the storage directory before locking. `writeStoreFile` creates it, but from inside the locked section — so on a fresh install, where `~/.mcp-inspector` does not exist, proper-lockfile failed ENOENT and every *first* save degraded to an unlocked write with a warning. That is the save most likely to be racing another (two Inspectors started together both reach it), so the lock was absent from precisely the interleaving it exists to close. - `anythingToMigrate` no longer reads every `readdir` failure as "nothing to migrate". Only ENOENT/ENOTDIR proves the fresh-install case; a directory that denies listing can still permit access to the known `secrets.json`, and returning false there selected the keychain and left those secrets invisible with nothing said. Other errors fall through to the under-lock checks, which is what happened before the fast path existed. - Pin `proper-lockfile` to the repo-root install in `vitest.shared.mts`, beside express and yaml: it is reached only through root-owned `core/`. Resolution already finds the root copy since no client declares it; the pin is what stops that depending on it never arriving as some client's transitive dependency, which would give a test two copies of a module whose whole job is one registry of held locks. - Make the cross-process test prove `mutate` takes the lock. The previous one asserted only that both secrets survived, which the optimistic verify delivers with the lock removed entirely. It now asserts the parent's `set` has *not* settled while a child holds the lock — an observation only a real lock produces. Verified by removing `withSecretFileLock` from `mutate`: three tests fail, where none did before. The degrade test's "lock cannot be created" case is now a path whose parent is a *file* rather than a missing directory, since a missing directory is no longer that case. It also fails identically for root, so it cannot pass locally and flake in a container. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H Signed-off-by: cliffhall --- .../integration/auth/node/file-lock.test.ts | 95 ++++++++++++++++--- core/auth/node/file-lock.ts | 14 +++ core/auth/node/secret-store-selection.ts | 16 +++- vitest.shared.mts | 11 +++ 4 files changed, 120 insertions(+), 16 deletions(-) diff --git a/clients/web/src/test/integration/auth/node/file-lock.test.ts b/clients/web/src/test/integration/auth/node/file-lock.test.ts index eb9b4dd18d..714432bb3c 100644 --- a/clients/web/src/test/integration/auth/node/file-lock.test.ts +++ b/clients/web/src/test/integration/auth/node/file-lock.test.ts @@ -72,12 +72,32 @@ const warnings = (): string => async function holdLockInChildProcess( target: string, holdMs: number, + /** + * Secrets the child writes *while holding the lock*, before it announces + * itself. A parent that honours the lock therefore reads a map that already + * contains them, which is what lets the caller assert on the merged result + * rather than only on timing. + */ + writeWhileHeld?: Record, ): Promise<{ ready: Promise; done: Promise }> { const script = ` const lockfile = require(${JSON.stringify(LOCKFILE_MODULE)}); + const fs = require("node:fs"); + const path = require("node:path"); + // A second Inspector reaches the lock through \`withSecretFileLock\`, which + // creates the storage directory first. Mirror that, or the fresh-install + // case below would be testing the child's omission rather than the parent. + fs.mkdirSync(path.dirname(${JSON.stringify(target)}), { recursive: true }); lockfile .lock(${JSON.stringify(target)}, { realpath: false, stale: 10000 }) .then(async (release) => { + const secrets = ${JSON.stringify(writeWhileHeld ?? null)}; + if (secrets) { + fs.writeFileSync( + ${JSON.stringify(target)}, + JSON.stringify({ version: 1, encryption: "none", secrets }), + ); + } process.stdout.write("acquired\\n"); await new Promise((r) => setTimeout(r, ${holdMs})); await release(); @@ -148,32 +168,83 @@ describe("withSecretFileLock across processes", () => { expect(warnings()).toBe(""); }); - it("serializes two FileSecretStores in different processes", async () => { + it("makes FileSecretStore.set wait on a lock another process holds", async () => { // The end-to-end shape from the issue: a CLI run beside a web session. - // The child holds the lock while the parent's `set` is in flight, so the - // parent's whole read-modify-write happens after the child is gone. + // + // Deliberately asserts that the parent's `set` has *not finished* while + // the child holds the lock. A test that only checks both keys survive + // afterwards passes with the lock removed from `mutate` entirely — the + // optimistic verify would repair the clobber and hide the regression. + // Not-yet-resolved is the observation only a real lock can produce. const target = filePath(); + const { ready, done } = await holdLockInChildProcess(target, 700, { + "srv:env:FROM_CHILD": "1", + }); + await ready; + + let settled = false; const store = new FileSecretStore({ filePath: target }); - await store.set("srv", "env:FIRST", "1"); + const pending = store.set("srv", "env:FROM_PARENT", "2").then(() => { + settled = true; + }); + + // Comfortably inside the child's hold, and comfortably outside the few + // milliseconds an unlocked read-modify-write would take. + await new Promise((r) => setTimeout(r, 300)); + expect(settled).toBe(false); - const { ready, done } = await holdLockInChildProcess(target, 300); + await done; + await pending; + + // Having waited, the parent read the map the child left, so its own entry + // landed *on top of* the child's rather than replacing it. + const reader = new FileSecretStore({ filePath: target }); + expect(await reader.get("srv", "env:FROM_CHILD")).toBe("1"); + expect(await reader.get("srv", "env:FROM_PARENT")).toBe("2"); + expect(warnings()).toBe(""); + }, 20_000); + + it("creates the storage directory so the very first save is locked too", async () => { + // `writeStoreFile` creates the parent directory, but from *inside* the + // locked section — so without the `mkdir` in `withSecretFileLock` the + // first save on a fresh install fails `ENOENT` on the lock and degrades + // to an unlocked write. That is the save most likely to be racing + // another, since two Inspectors started together both reach it. + const target = path.join(tmpDir, "fresh-install", "secrets.json"); + const { ready, done } = await holdLockInChildProcess(target, 700); await ready; - await store.set("srv", "env:SECOND", "2"); + + let settled = false; + const store = new FileSecretStore({ filePath: target }); + const pending = store.set("srv", "env:FIRST_EVER", "1").then(() => { + settled = true; + }); + + await new Promise((r) => setTimeout(r, 300)); + expect(settled).toBe(false); + await done; + await pending; const reader = new FileSecretStore({ filePath: target }); - expect(await reader.get("srv", "env:FIRST")).toBe("1"); - expect(await reader.get("srv", "env:SECOND")).toBe("2"); + expect(await reader.get("srv", "env:FIRST_EVER")).toBe("1"); + // No degrade warning: the lock was genuinely held, not skipped. + expect(warnings()).toBe(""); }, 20_000); }); describe("withSecretFileLock degrades rather than failing", () => { it("runs the body anyway when the lock cannot be created, and says so once", async () => { - // A directory that does not exist stands in for every real variant — + // A path whose parent is a *file* stands in for every real variant — // read-only `$HOME`, a mount owned by another uid, a filesystem without - // `mkdir` semantics. This store exists for boxes where the usual - // mechanism is missing, so it must not gain a new way to be unavailable. - const target = path.join(tmpDir, "no-such-dir", "secrets.json"); + // `mkdir` semantics — and unlike a permissions-based setup it fails the + // same way for root, so it cannot pass locally and flake in a container. + // Note a merely *missing* directory is no longer this case: + // `withSecretFileLock` creates it. This store exists for boxes where the + // usual mechanism is missing, so it must not gain a new way to be + // unavailable. + await fs.writeFile(path.join(tmpDir, "not-a-dir"), "", "utf-8"); + const target = path.join(tmpDir, "not-a-dir", "secrets.json"); let ran = 0; await withSecretFileLock(target, async () => { diff --git a/core/auth/node/file-lock.ts b/core/auth/node/file-lock.ts index 66dcaea7b6..21d89835e3 100644 --- a/core/auth/node/file-lock.ts +++ b/core/auth/node/file-lock.ts @@ -35,6 +35,7 @@ * underneath it, and says so once. */ +import * as fs from "node:fs/promises"; import * as path from "node:path"; // CJS-only package. A default import is the shape that survives every // bundler this repo runs core/ through (tsup for cli/tui, vite's SSR/node @@ -140,6 +141,19 @@ export async function withSecretFileLock( fn: () => Promise, ): Promise { const target = path.resolve(filePath); + // Create the parent directory before locking, not after. `writeStoreFile` + // creates it on the way to writing the secrets file, but that runs *inside* + // the locked section — so on a fresh install, where `~/.mcp-inspector` does + // not exist yet, `proper-lockfile` would fail `ENOENT` and every first save + // would degrade to an unlocked write with a warning. That is the one save + // most likely to be racing another: two Inspectors started together both + // reach it, and it is exactly the interleaving this lock exists to close. + // + // Failure is deliberately swallowed rather than reported here. A directory + // that cannot be created is the same condition as a lock that cannot be + // taken, and the catch below already says so with the right message — one + // that mentions the lock rather than a `mkdir` the caller never asked for. + await fs.mkdir(path.dirname(target), { recursive: true }).catch(() => {}); let release: (() => Promise) | undefined; try { release = await properLockfile.lock(target, { diff --git a/core/auth/node/secret-store-selection.ts b/core/auth/node/secret-store-selection.ts index 4cd6937583..ce1704ccfd 100644 --- a/core/auth/node/secret-store-selection.ts +++ b/core/auth/node/secret-store-selection.ts @@ -666,10 +666,18 @@ async function anythingToMigrate(filePath: string): Promise { return (await fs.readdir(path.dirname(filePath))).some( (name) => name === base || name.startsWith(`${base}.migrating-`), ); - } catch { - // No storage directory yet — the first run on a fresh install, and the - // path this check exists to keep quiet. - return false; + } catch (err) { + const code = (err as NodeJS.ErrnoException).code; + // Only "there is no directory" proves there is nothing to migrate — the + // first run on a fresh install, and the path this check exists to keep + // quiet. Every other failure (EACCES, EPERM, EMFILE) means the directory + // is there and could not be *listed*, which is not the same as empty: a + // directory can deny listing while still permitting access to the known + // `secrets.json` path, so returning false would select the keychain and + // leave those secrets invisible with nothing said. Fall through instead + // and let the under-lock `existsSync` and claim decide, which is what + // happened before this fast path existed. + return code !== "ENOENT" && code !== "ENOTDIR"; } } diff --git a/vitest.shared.mts b/vitest.shared.mts index 1ebbc96ae8..8ad914b50a 100644 --- a/vitest.shared.mts +++ b/vitest.shared.mts @@ -101,6 +101,17 @@ export function vitestSharedPaths(clientDir: string) { find: /^yaml$/, replacement: path.resolve(repoRoot, "node_modules/yaml"), }, + // Same reasoning, one layer in: `proper-lockfile` is reached only through + // `core/` (the secrets file's cross-process lock, #2082), which is the + // other root-owned tree with no manifest of its own. Resolution finds the + // root copy on its own today — nothing declares it in a client — and this + // pin is what keeps that from depending on nothing ever arriving as some + // client's transitive dependency, which would otherwise give a test two + // copies of a module whose whole job is a single registry of held locks. + { + find: /^proper-lockfile$/, + replacement: path.resolve(repoRoot, "node_modules/proper-lockfile"), + }, ]; const projectResolve = { From dcd70e2f11b9fb93c48882eb8a6732b41325804c Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 10:08:12 -0400 Subject: [PATCH 03/12] fix(auth): stop degrading on ELOCKED, and correct the claim about the lock (#2082) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review round 2, both comments upheld. **ELOCKED must not degrade.** Exhausting the retries on a held lock means the lock is *working* and something else demonstrably holds it — so running the mutation anyway entered the exact interleaving the lock exists to prevent, and entered it knowing a concurrent writer was there. `set` now fails instead. Two supporting changes: - The retry budget went from ~3.3s to ~15s so it outlasts the 10s stale window. Refusing is only defensible because a *crashed* holder resolves by takeover first; with the shorter budget, one Inspector killed mid-save would have failed every later save on the box until someone deleted the lock by hand. Pinned by a new test that stages a dead holder's lock directly (mkdir + backdated mtime) and asserts the save succeeds. - Acquire is now probe-then-wait. `proper-lockfile` drives its whole acquire through `retry`, which re-attempts on *any* error — so with a 20-retry budget a read-only `$HOME` would have spent the full 15s re-issuing an identical failing `mkdir` on every save before degrading. One retry-less probe separates "held" (worth waiting out) from "unavailable" (not). **The comment overclaimed what proper-lockfile provides, and the reviewer was right about the mechanism.** Verified against lib/lockfile.js@4.1.2: on EEXIST it stats, and if stale it rmdirs and re-mkdirs *without* checking the directory it removed is the one it found stale — so a slow waiter can still delete a fast waiter's fresh lock and both proceed. That is the same race the hand-rolled version could not close, and it is not closable without renameat2(RENAME_EXCHANGE). What the library actually adds is that the loser is *detected*: its refresh tick compares the lock's mtime against the value recorded at acquire, so a compromised holder is told rather than proceeding silently. And what is genuinely exclusive is the case that matters — mkdir is atomic and a live holder refreshes its mtime, so its lock never goes stale and two running Inspectors are serialized. The residual window opens only after a holder dies without releasing, and the optimistic verify still covers it. file-lock.ts, README.md, specification/v2_servers_file.md and AGENTS.md all carried the overclaim; all four now state the narrower, true version. Also fixes a test that was quietly asserting the opposite of its name: its holder used stale: 60_000 while the waiter used 10_000, and staleness is judged by the *waiter's* threshold — so the holder was declared stale after 10s and the save succeeded by takeover. Not done: a two-process stale-takeover race test. It would be testing the library rather than this code, and is inherently nondeterministic; the honest response to that finding was to stop claiming the race is closed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H Signed-off-by: cliffhall --- AGENTS.md | 14 +- README.md | 6 +- .../integration/auth/node/file-lock.test.ts | 111 ++++++++++---- core/auth/node/file-lock.ts | 144 +++++++++++++----- specification/v2_servers_file.md | 2 +- 5 files changed, 200 insertions(+), 77 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index e8bb92f78c..9c99634eb5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -71,11 +71,15 @@ v2/main/ │ │ │ # file it cannot decrypt rather than destroying it), │ │ │ # file-lock.ts (withSecretFileLock: the cross-process │ │ │ # mutual exclusion #2082 settled on — proper-lockfile, -│ │ │ # borrowed rather than hand-rolled, because stale-takeover -│ │ │ # is what three review rounds of a mkdir election failed to -│ │ │ # get right; DEGRADES rather than throws when no lock can -│ │ │ # be taken, since this store exists for boxes missing the -│ │ │ # usual mechanism and must not gain a new way to fail), +│ │ │ # borrowed rather than hand-rolled. Read its header before +│ │ │ # citing it: it makes two LIVE Inspectors exclusive, and +│ │ │ # does NOT make stale takeover single-winner — it makes the +│ │ │ # loser detectable (ECOMPROMISED), which is the honest claim. +│ │ │ # DEGRADES when no lock CAN be taken (read-only $HOME etc), +│ │ │ # since this store exists for boxes missing the usual +│ │ │ # mechanism; but THROWS on ELOCKED — a lock held by a live +│ │ │ # writer is evidence the lock works, not licence to bypass +│ │ │ # it — after waiting past the stale window), │ │ │ # and │ │ │ # secret-store-selection.ts (the POLICY: explicit │ │ │ # MCP_INSPECTOR_SECRET_STORE wins, else probe the keychain, diff --git a/README.md b/README.md index a307cae660..2da9c5bb03 100644 --- a/README.md +++ b/README.md @@ -504,7 +504,11 @@ The Inspector writes the file `0600` and re-tightens it at startup if something **Two Inspectors, one file.** Within a process, mutations are serialized per file path, so a web session's own concurrent saves cannot lose each other. Across processes — a CLI run beside a web session — each mutation takes an exclusive lock on `secrets.json.lock` for the whole read-modify-write, using [`proper-lockfile`](https://github.com/moxystudio/node-proper-lockfile) (the same library npm itself locks with). The lock expires 10 seconds after its holder stops refreshing it, so an Inspector that is killed mid-save does not leave the file unwritable. -Underneath the lock, each mutation still reads the file, applies its change, writes, then reads back and compares the whole map; if something wrote in between it re-applies onto what was left and retries, failing loudly after five lost rounds rather than returning as though the value were saved. That check is not redundant with the lock — a lock only orders the writers that *take* it, so it covers an editor, a restored backup, or an Inspector older than this release. +Two running Inspectors are therefore genuinely serialized. What a lock file cannot make single-winner is the *takeover of a lock whose holder died* — that needs a compare-and-swap on a directory entry (`renameat2`) which Node does not expose, and it is what an earlier hand-rolled attempt failed three review rounds on. `proper-lockfile` does not close that race either; what it adds is that the loser is **told** (it compares the lock's mtime against the value it recorded, so a holder whose lock was replaced is marked compromised and warns). The window opens only after a holder dies without releasing. + +Which is why, underneath the lock, each mutation still reads the file, applies its change, writes, then reads back and compares the whole map; if something wrote in between it re-applies onto what was left and retries, failing loudly after five lost rounds rather than returning as though the value were saved. That check is what still catches a clobber inside that window — and it covers what no lock can, since a lock only orders the writers that *take* it: an editor, a restored backup, or an Inspector older than this release. + +If another process holds the lock and will not let go, the save **fails** rather than going ahead unlocked — waiting past the stale window first, so a crashed Inspector resolves itself rather than failing everyone else's saves. Writing alongside a writer you can see is the one case where degrading would lose the secret it was trying to protect. It is also what covers the lock being unavailable. This store exists for boxes where the usual mechanism isn't there, so a directory that can't hold a lock file — a read-only `$HOME`, a mount owned by another uid — makes the save proceed unlocked with a warning, rather than turning every `set` into a failure on exactly the deployments the store was written for. diff --git a/clients/web/src/test/integration/auth/node/file-lock.test.ts b/clients/web/src/test/integration/auth/node/file-lock.test.ts index 714432bb3c..f0619035d9 100644 --- a/clients/web/src/test/integration/auth/node/file-lock.test.ts +++ b/clients/web/src/test/integration/auth/node/file-lock.test.ts @@ -19,6 +19,7 @@ import { } from "vitest"; import { execFile } from "node:child_process"; import { createRequire } from "node:module"; +import { existsSync } from "node:fs"; import * as fs from "node:fs/promises"; import * as os from "node:os"; import * as path from "node:path"; @@ -28,6 +29,7 @@ import { resetFileLockWarnings, } from "@inspector/core/auth/node/file-lock.js"; import { FileSecretStore } from "@inspector/core/auth/node/file-secret-store.js"; +import { SecretStoreUnavailableError } from "@inspector/core/auth/node/secret-store.js"; const run = promisify(execFile); const require_ = createRequire(import.meta.url); @@ -261,53 +263,100 @@ describe("withSecretFileLock degrades rather than failing", () => { expect(warn).toHaveBeenCalledTimes(1); }); - it("still saves the secret when no lock can be taken", async () => { - // The degrade has to be end-to-end, not just in the helper: `set` on a - // lock-hostile directory must persist, falling back to the #1950 - // optimistic behaviour. + it("refuses the save rather than writing alongside a live holder", async () => { + // `ELOCKED` is evidence the lock is *working*, so degrading here would + // enter the exact interleaving the lock exists to prevent — and enter it + // knowing another writer is there. Held from this process, which is + // indistinguishable to `proper-lockfile` from a remote holder (it is not + // reentrant); the in-process queue is what keeps that out of the way in + // production. + // + // The wait is real: the retry budget deliberately outlasts the 10s stale + // window so a *crashed* holder resolves by takeover instead of failing + // everyone else's saves. This holder is alive and refreshing, so it never + // goes stale and the budget is spent in full. + const target = filePath(); + const lockfile = require_(LOCKFILE_MODULE) as { + lock: (f: string, o: object) => Promise<() => Promise>; + }; + // The **same** `stale` production uses, and that is not incidental: + // `isLockStale` is evaluated against the *waiter's* threshold while the + // holder refreshes on its own `stale / 2`. A holder configured looser + // (say 60s) refreshes every 30s and is therefore declared stale by a + // 10s waiter after 10s — the waiter takes over and the save succeeds, + // quietly testing the opposite of what this test claims. + const release = await lockfile.lock(target, { + realpath: false, + stale: 10_000, + }); + + const store = new FileSecretStore({ filePath: target }); + // One call, both assertions off the same rejection: each attempt spends + // the full retry budget, so a second would double the test's runtime to + // re-prove the same thing. + const err = await store + .set("srv", "env:MINE", "1") + .catch((e: unknown) => e); + expect(err).toBeInstanceOf(SecretStoreUnavailableError); + expect((err as Error).message).toMatch(/was not saved/); + await release(); + + // Nothing was written behind the holder's back. + expect(existsSync(target)).toBe(false); + }, 90_000); + + it("takes over the lock of a holder that died, rather than failing the save", async () => { + // The invariant behind refusing on `ELOCKED`: refusing is only defensible + // because a *crashed* holder resolves on its own first. `RETRY` therefore + // has to outlast `STALE_MS` — if the budget were the shorter of the two, + // one Inspector killed mid-save would make every later save on the box + // fail until someone deleted the lock by hand. + // + // A dead holder is exactly a lock directory nobody is refreshing, so it + // is staged directly: no child to race, and no dependence on how quickly + // a killed process is reaped. const target = filePath(); const lockPath = `${target}.lock`; - // Occupy the lock's own path with a *file*, so `mkdir` fails EEXIST - // forever and no takeover can succeed — a permanent, non-ELOCKED - // failure rather than contention. - await fs.writeFile(lockPath, "not a lock directory", "utf-8"); + await fs.mkdir(path.dirname(target), { recursive: true }); + await fs.mkdir(lockPath); + const longDead = new Date(Date.now() - 60_000); + await fs.utimes(lockPath, longDead, longDead); const store = new FileSecretStore({ filePath: target }); - await store.set("srv", "env:MINE", "1"); + await store.set("srv", "env:AFTER_CRASH", "1"); const reader = new FileSecretStore({ filePath: target }); - expect(await reader.get("srv", "env:MINE")).toBe("1"); - expect(warnings()).toMatch( - /Could not take a lock|has held the secrets file/, - ); - }, 20_000); + expect(await reader.get("srv", "env:AFTER_CRASH")).toBe("1"); + // Took the lock over — did not fall through to an unlocked write. + expect(warnings()).toBe(""); + }, 60_000); - it("gives up waiting on a holder that never releases, and proceeds", async () => { - // Holding it from *this* process: `proper-lockfile` is not reentrant, so - // a second `lock()` on the same path fails `ELOCKED` exactly as a remote - // holder's would — the difference the in-process queue exists to keep - // out of the lock's way. + it("stays silent per the delete contract when the lock is held", async () => { + // `delete` reports nothing by contract — only `set` hard-fails — so the + // refusal above must not turn a delete into a throw. const target = filePath(); + const store = new FileSecretStore({ filePath: target }); + await store.set("srv", "env:A", "1"); + const lockfile = require_(LOCKFILE_MODULE) as { lock: (f: string, o: object) => Promise<() => Promise>; }; + // The **same** `stale` production uses, and that is not incidental: + // `isLockStale` is evaluated against the *waiter's* threshold while the + // holder refreshes on its own `stale / 2`. A holder configured looser + // (say 60s) refreshes every 30s and is therefore declared stale by a + // 10s waiter after 10s — the waiter takes over and the save succeeds, + // quietly testing the opposite of what this test claims. const release = await lockfile.lock(target, { realpath: false, - stale: 60_000, - }); - - let ran = false; - await withSecretFileLock(target, async () => { - ran = true; + stale: 10_000, }); + await expect(store.delete("srv", "env:A")).resolves.toBeUndefined(); await release(); - // Proceeding is the lesser evil: refusing would lose the value outright, - // whereas proceeding falls back to the residual #1950 behaviour — and it - // says which of the two happened. - expect(ran).toBe(true); - expect(warnings()).toContain("without the lock"); - }, 20_000); + // …and the entry it could not delete is still there, not half-removed. + expect(await store.get("srv", "env:A")).toBe("1"); + }, 90_000); }); describe("withSecretFileLock reports what it cannot clean up", () => { diff --git a/core/auth/node/file-lock.ts b/core/auth/node/file-lock.ts index 21d89835e3..d61108d0aa 100644 --- a/core/auth/node/file-lock.ts +++ b/core/auth/node/file-lock.ts @@ -11,18 +11,38 @@ * aside and enter alongside it. * * So the choice #2082 settles is not "lock versus no lock" but "hand-rolled - * versus borrowed". `proper-lockfile` is the borrowed one: it is what npm - * itself uses, and stale-takeover is precisely the problem it has already - * solved — it re-`stat`s the lock directory after claiming it and gives the - * lock up if the mtime is not the one it wrote, so a loser of the takeover - * race releases rather than proceeding. - * - * **What this does not remove.** The optimistic verify in - * {@link FileSecretStore.mutate} stays. A lock is an advisory convention - * between participants that take it, so it orders Inspector against - * Inspector and says nothing about an editor, a backup restore, or an - * Inspector old enough to predate this file. The verify is also what covers - * {@link withSecretFileLock} *declining* — see below. + * versus borrowed". `proper-lockfile` is the borrowed one — what npm itself + * locks with — and it is worth being exact about what it does and does not + * buy, because the temptation is to overclaim it. + * + * **What it makes exclusive.** `mkdir` is atomic, and a live holder refreshes + * the lock's mtime at `stale / 2` for as long as it lives, so its lock never + * becomes eligible for takeover. Two running Inspectors are therefore + * genuinely serialized: one holds, the other waits. That is the case #1950 + * lost updates in, and it is closed. + * + * **What it does not.** Stale takeover is still not single-winner. Reading + * `lib/lockfile.js@4.1.2`: on `EEXIST` it `stat`s, and if stale it `rmdir`s + * and re-`mkdir`s — without checking that the directory it removed is the + * one it found stale. So a slow waiter can delete a fast waiter's *fresh* + * lock and claim a replacement, and both proceed. That is the identical race + * the hand-rolled version could not close, and it cannot be closed with what + * Node exposes (`renameat2(RENAME_EXCHANGE)`). + * + * What the library adds over the hand-rolled one is that the loser finds + * out. Its refresh tick compares the lock's mtime against the value it + * recorded at acquire, so a holder whose directory was replaced is marked + * `ECOMPROMISED` and told — see `onCompromised` below. The window is also + * narrow and conditional: it opens only after a holder *dies without + * releasing*, since nothing else lets a lock go stale. + * + * **Which is why the optimistic verify in {@link FileSecretStore.mutate} + * stays, and is not belt-and-braces.** It is what still catches a clobber + * inside that window. It also covers what no lock can: an advisory + * convention orders Inspector against Inspector and says nothing about an + * editor, a backup restore, or an Inspector old enough to predate this file + * — and it covers {@link withSecretFileLock} being unable to lock at all, + * see below. * * **Degrading rather than failing is deliberate.** The store's whole reason * for existing is a box where the usual mechanism is unavailable (no @@ -42,6 +62,7 @@ import * as path from "node:path"; // graph for the web runner); named imports off a CJS module depend on // lexer detection that esbuild and rollup disagree about. import properLockfile from "proper-lockfile"; +import { SecretStoreUnavailableError } from "./secret-store.js"; /** * How long a lock may go untouched before another process may claim it. @@ -55,14 +76,21 @@ import properLockfile from "proper-lockfile"; const STALE_MS = 10_000; /** - * How long a waiter will keep trying before giving up. + * How long a waiter keeps trying before giving up. + * + * This schedule sums to roughly 15s, and the number that matters is that it + * is comfortably **longer than {@link STALE_MS}**. A waiter that gives up + * first would abandon the save while the lock still belonged to a process + * that had already died — the takeover that resolves it becomes possible + * only once the lock goes stale, so a budget under 10s would turn a crashed + * Inspector into a failed save for every other one on the box. * - * The retry schedule below tops out around 3s of waiting. That is long - * enough for any real mutation to finish (see above) and short enough that a - * pathological case surfaces as a slow save rather than a hung one. + * An uncontended acquire is one `mkdir` and pays none of this; the first + * retries are tens of milliseconds, so ordinary contention (a mutation is a + * read, an scrypt derivation and an atomic write) resolves imperceptibly. */ const RETRY = { - retries: 8, + retries: 20, factor: 2, minTimeout: 20, maxTimeout: 1_000, @@ -117,12 +145,7 @@ const describeError = (err: unknown): string => err instanceof Error ? err.message : String(err); /** - * Run `fn` holding an exclusive cross-process lock on `filePath`. - * - * The lock is `.lock`, a directory beside the secrets file rather - * than inside it — `proper-lockfile` never opens or truncates the file it - * guards, so a lock that outlives its holder can only ever block a write, - * never damage one. + * One `proper-lockfile` acquire against `target`, with the given retry policy. * * `realpath: false` is load-bearing: by default the library resolves the * target through `fs.realpath`, which fails `ENOENT` on a secrets file that @@ -132,6 +155,36 @@ const describeError = (err: unknown): string => * through different symlinks take different locks; the store resolves its * path once at construction and every caller goes through it, so that is a * shape this codebase does not produce. + */ +function acquire( + target: string, + retries: number | typeof RETRY, +): Promise<() => Promise> { + return properLockfile.lock(target, { + realpath: false, + stale: STALE_MS, + retries, + // The library's default `onCompromised` *throws* — from a timer, with no + // caller on the stack, so it lands as an uncaught exception and takes the + // process down. This is also the library's *only* signal for the + // stale-takeover race described at the top of this file — someone + // declared our lock stale and replaced it — so it is the one place a user + // learns the guarantee was lost. Worth saying; not worth killing an + // Inspector session over. + onCompromised: (err) => + warnOnce( + `The lock on the secrets file at ${target} was taken over by another process while a write was in progress (${err.message}). If a secret you just saved is missing, save it again.`, + ), + }); +} + +/** + * Run `fn` holding an exclusive cross-process lock on `filePath`. + * + * The lock is `.lock`, a directory beside the secrets file rather + * than inside it — `proper-lockfile` never opens or truncates the file it + * guards, so a lock that outlives its holder can only ever block a write, + * never damage one. * * Returns whatever `fn` returns. `fn` runs exactly once either way — the * lock's absence changes the guarantee, never whether the work happens. @@ -156,25 +209,38 @@ export async function withSecretFileLock( await fs.mkdir(path.dirname(target), { recursive: true }).catch(() => {}); let release: (() => Promise) | undefined; try { - release = await properLockfile.lock(target, { - realpath: false, - stale: STALE_MS, - retries: RETRY, - // The library's default `onCompromised` *throws* — from a timer, with - // no caller on the stack, so it lands as an uncaught exception and - // takes the process down. A compromised lock means someone declared - // ours stale and took it; the write in flight is at risk, which is - // worth saying and is not worth killing an Inspector session over. - onCompromised: (err) => - warnOnce( - `The lock on the secrets file at ${target} was taken over by another process while a write was in progress (${err.message}). If a secret you just saved is missing, save it again.`, - ), + release = await acquire(target, 0).catch((err: unknown) => { + // **Retries are for contention, and only for contention.** + // `proper-lockfile` drives its whole acquire through `retry`, which + // re-attempts on *any* error — so a read-only `$HOME` would spend the + // full ~15s budget re-issuing an `mkdir` that fails identically every + // time, on every save, before degrading. Probing once with no retries + // separates the two answers at the cost of one syscall: `ELOCKED` is + // worth waiting out, an infrastructure failure is not. + if (!isHeldElsewhere(err)) throw err; + return acquire(target, RETRY); }); } catch (err) { + // **`ELOCKED` is not a reason to degrade — it is the opposite.** It means + // the lock is working and something else demonstrably holds it right now, + // so running the body anyway would write alongside a *known* concurrent + // writer: the precise interleaving this exists to prevent, entered + // deliberately. Having already waited past the stale window (see + // {@link RETRY}), a holder still there is not one that crashed; it is one + // that is stuck. Refusing loses nothing — `set` reports it and the user + // retries — whereas proceeding can lose a secret while reporting success. + if (isHeldElsewhere(err)) { + throw new SecretStoreUnavailableError( + `Could not save to the secrets file at ${target}: another process has held the lock on it for over ${Math.round((RETRY.retries * RETRY.maxTimeout) / 1000)} seconds. Its secrets are intact; the value you just entered was not saved. If no other Inspector is running, remove ${target}.lock and try again.`, + ); + } + // Everything else is the lock being *unavailable* rather than held — a + // read-only `$HOME`, a mount owned by another uid, a filesystem without + // `mkdir` semantics. There the choice is between degrading and refusing + // every save on a box that has no other way to keep a secret, and this + // store exists for exactly those boxes (#1848, #1905). warnOnce( - isHeldElsewhere(err) - ? `Another process has held the secrets file at ${target} for longer than this write was willing to wait, so the write went ahead without the lock. If two Inspectors are saving secrets at once, one of them may not be saved.` - : `Could not take a lock on the secrets file at ${target} (${describeError(err)}), so writes to it are not protected against another process writing at the same moment.`, + `Could not take a lock on the secrets file at ${target} (${describeError(err)}), so writes to it are not protected against another process writing at the same moment.`, ); return fn(); } diff --git a/specification/v2_servers_file.md b/specification/v2_servers_file.md index 44d02cf0a7..32a8d9dca5 100644 --- a/specification/v2_servers_file.md +++ b/specification/v2_servers_file.md @@ -261,7 +261,7 @@ Each server entry may carry these Inspector-extension fields at the top level: - **Secret store selection (#1950)**: #1356 assumed a keychain. On a host without one — the published container (no D-Bus session, #1848), Android/Termux (no prebuilt binary, #1905), a minimal Linux install — `set` hard-failed, so those users could not persist an OAuth client secret or a stdio `env:` value **at all**. #1950 adds the two missing implementations plus the policy that picks one, and the surfaces that say which was picked. - **Selection**: `MCP_INSPECTOR_SECRET_STORE=keyring|file|memory` wins outright. Otherwise the keychain is _probed_ — an `AsyncEntry` construction plus a real read, because the container that motivated #1848 imports the package fine and only fails when it reaches for a Secret Service that isn't there. A read and not a write, deliberately: probing by writing would deposit a value in the user's login keyring at every startup for a store they may never use. If the probe fails the fallback is **`memory`** in a container whose secrets directory is not on a mount, and **`file`** otherwise — so mounting the volume the README already recommends for the catalog flips the same run to durable storage with no configuration, and an unmounted container gets the honest answer rather than a file `docker run --rm` will discard. Resolution is cached per process so the startup banner, `GET /api/config`, and the store doing the writing cannot disagree. - **`FileSecretStore` at rest**: one JSON document at `~/.mcp-inspector/secrets.json` (or `MCP_INSPECTOR_SECRET_FILE`, else under `MCP_STORAGE_DIR`), written `0600` and re-tightened at startup. With `MCP_INSPECTOR_SECRET_KEY` set it is AES-256-GCM with a scrypt-derived key and a per-write random salt; without it the values are in the clear, and _that_ is what the loud banner and the warning-toned modal footer are about. The **whole map is encrypted as a unit**, not value-by-value, so the account names (`serverId:field`) are hidden too — a per-value scheme would leave a readable index of which servers you hold a client secret for. Upgrading is lazy: setting the passphrase later re-encrypts on the next write rather than rewriting the file during a run that may never touch a secret, and the descriptor reports `pendingEncryption` until it does. A file that can no longer be decrypted reads as empty but **refuses to be written**, because replacing a file of still-valid secrets to satisfy an additive request destroys data. - - **Concurrency**: within a process, mutations are serialized per **resolved file path** (not per store instance — two `FileSecretStore`s on one file are ordinary, since the resolved store holds one and the keychain hand-off builds another). Across processes, each mutation holds an exclusive `proper-lockfile` lock on `secrets.json.lock` for the whole read-modify-write (#2082). The in-process queue is what keeps that usable: `proper-lockfile` is not reentrant, so a second `lock()` from the same process fails `ELOCKED` and would be indistinguishable from a genuine remote holder. An earlier iteration hand-rolled the lock — a `mkdir` election with an owner stamp, heartbeat and stale-takeover — and three review rounds each found a real race, the last not closable with what Node exposes (claiming a stale lock needs compare-and-swap on a directory entry, `renameat2(RENAME_EXCHANGE)`). #2082 settled that as "borrow, don't hand-roll": stale-takeover is exactly the problem the library has already solved, re-`stat`ing the lock after claiming it so the loser of a takeover race releases instead of proceeding. **The optimistic verify stays underneath it**, and is not redundant: read `M0`, apply, write `M1`, read back `M2`, re-apply onto whatever a concurrent writer left if they differ, bounded, with `set` throwing on non-convergence rather than returning as though the value were saved. The comparison is over the **whole map** — checking only your own entry passes in exactly the case that loses data, because yours is present and the other writer's is gone. A lock is advisory between the processes that take it, so the verify is what covers a writer outside this codebase (an editor, a restored backup, an Inspector predating #2082) and what covers the lock being *unavailable*: `withSecretFileLock` runs the body anyway, warning once, on a directory that cannot hold a lock file — this store exists for boxes where the usual mechanism is missing (#1848, #1905) and must not acquire a new way to be unavailable. Reads take no lock; `writeStoreFile` is atomic, so a reader sees the old file or the new one, never a torn one. + - **Concurrency**: within a process, mutations are serialized per **resolved file path** (not per store instance — two `FileSecretStore`s on one file are ordinary, since the resolved store holds one and the keychain hand-off builds another). Across processes, each mutation holds an exclusive `proper-lockfile` lock on `secrets.json.lock` for the whole read-modify-write (#2082). The in-process queue is what keeps that usable: `proper-lockfile` is not reentrant, so a second `lock()` from the same process fails `ELOCKED` and would be indistinguishable from a genuine remote holder. An earlier iteration hand-rolled the lock — a `mkdir` election with an owner stamp, heartbeat and stale-takeover — and three review rounds each found a real race, the last not closable with what Node exposes (claiming a stale lock needs compare-and-swap on a directory entry, `renameat2(RENAME_EXCHANGE)`). #2082 settled that as "borrow, don't hand-roll" — but the borrowed lock is not claimed to close that race, because it does not. `proper-lockfile@4.1.2` `rmdir`s a stale lock and re-`mkdir`s without checking the directory it removed is the one it found stale, so a slow waiter can still delete a fast waiter's fresh lock; what it adds over the hand-rolled version is *detection* — its refresh tick compares the lock's mtime against the value recorded at acquire, so a compromised holder is told (`ECOMPROMISED`, surfaced as a warning) instead of proceeding silently. What **is** exclusive is the case that matters: `mkdir` is atomic and a live holder refreshes its mtime, so its lock never goes stale and two running Inspectors are genuinely serialized. The residual window opens only after a holder dies without releasing. A waiter also waits past the stale window before giving up (so a crashed holder resolves by takeover rather than failing everyone else's saves), and a lock still held after that makes `set` **fail** rather than write alongside a visible concurrent writer — `ELOCKED` is evidence the lock is working, not a reason to bypass it. **The optimistic verify stays underneath it**, and is not redundant: read `M0`, apply, write `M1`, read back `M2`, re-apply onto whatever a concurrent writer left if they differ, bounded, with `set` throwing on non-convergence rather than returning as though the value were saved. The comparison is over the **whole map** — checking only your own entry passes in exactly the case that loses data, because yours is present and the other writer's is gone. A lock is advisory between the processes that take it, so the verify is what covers a writer outside this codebase (an editor, a restored backup, an Inspector predating #2082) and what covers the lock being *unavailable*: `withSecretFileLock` runs the body anyway, warning once, on a directory that cannot hold a lock file — this store exists for boxes where the usual mechanism is missing (#1848, #1905) and must not acquire a new way to be unavailable. Reads take no lock; `writeStoreFile` is atomic, so a reader sees the old file or the new one, never a torn one. - **Strict reads (`getStrict`)**: `get` is tolerant by contract, and that tolerance is wrong for exactly one caller — a migration that treats `null` as proof of absence and then _writes_. A transient read failure would look like "nothing there", and the write would replace a newer stored value with an older on-disk copy, inverting the keychain-wins rule the migration is built on. `getStrict` throws instead, and both plaintext migrations plus the keychain hand-off use it. Two traps this hit on the way in, both worth remembering: the seam must be forwarded by `DeferredSecretStore` (the store production actually uses, so without forwarding the strictness existed only in tests that injected a concrete store), and it must be implemented by _every_ store rather than falling back to `get` for the one it was introduced for. - **Bulk reads (`getMany`)**: rehydration asked field by field, and `FileSecretStore.get` reads and decrypts the entire file per call. scrypt at `N=16384` measures ~23ms, so an encrypted catalog spent ~450ms of pure key derivation on every `GET /api/servers` — a visible stall rather than a micro-optimization. The seam takes a **list of `{ serverId, fields }` across servers**, and both rehydration callers pass the whole catalog in one request, so `FileSecretStore` decrypts once per rehydration; stores for which per-field reads are already cheap (the keychain) fall back to parallel `get`. The cross-server shape is the point: a per-server version shipped first and left a 20-server catalog paying 20 serialized derivations, because both callers iterate servers — the same stall reached one server at a time instead of one field at a time. - **Durability gate on migration**: the plaintext-stripping migrations only delete the disk copy once the value is somewhere that outlives the process (`isDurable`). Against a session-scoped store, stripping would trade a secret that survives restarts for one that dies with the process — and it runs on an ordinary `GET`, so merely opening the app would destroy it. From 833a6ba76b410dfdc40b8ef87f1f657a881de428 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 10:32:43 -0400 Subject: [PATCH 04/12] fix(auth): keep the keychain hand-off non-throwing, and stop overstating the wait (#2082) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review round 3. - **`absorbFileSecretsIntoKeyring` must never throw, and round 2 broke that.** Making `withSecretFileLock` reject on a held lock was right for `set` — a user is waiting on that value — and wrong here: this is awaited directly by both `resolveSecretStore` branches, so a stuck writer elsewhere on the box would have failed store resolution and with it the whole session. The lock failure is now caught, warned, and the file left for the next run. Covered by a test that holds the lock from another handle and asserts the hand-off resolves, the file survives, and the lock-specific warning is emitted (it asserts that message rather than the shared "left in place" tail, which the pre-existing claim-failure path also prints and would have let it pass without the lock ever being reached). - **The refusal message overstated how long it waited.** `retries * maxTimeout` reads 20s for a schedule that sums to 15.26s — the early attempts are the exponential ramp, not the cap. Replaced with `RETRY_BUDGET_MS`, computed from the schedule rather than written down, so it cannot drift from the thing it describes. `retry` applies no jitter by default, so it is exact rather than an estimate. - **The PR description still described the pre-round-2 design** — the single-winner claim and the ~3s backoff. Rewritten to match the code. Coverage: the round-3 changes cost `secret-store-selection.ts` two branches and it fell to 89.62%. Recovered honestly rather than by annotation — the `ENOTDIR` arm of `anythingToMigrate` now has a real test (a path whose parent is a file, the other shape of "nothing there"), and the new catch types its parameter `Error` instead of re-narrowing, since `withSecretFileLock` rejects only with the `SecretStoreUnavailableError` it constructs one call away. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H Signed-off-by: cliffhall --- .../auth/node/secret-store-selection.test.ts | 52 +++++++++++++++++++ core/auth/node/file-lock.ts | 24 ++++++++- core/auth/node/secret-store-selection.ts | 17 ++++++ 3 files changed, 92 insertions(+), 1 deletion(-) 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 f7317d2e01..e9cb8f7efc 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 @@ -684,6 +684,39 @@ describe("absorbFileSecretsIntoKeyring", () => { return filePath; } + it("does not throw when another process holds the lock (#2082)", async () => { + // `withSecretFileLock` throws on a lock held past its retry budget, which + // is right for a `set` — the user is waiting on that value — and wrong + // here. This function is awaited directly by both `resolveSecretStore` + // branches, so an escaping error fails store resolution and with it the + // whole session: a stuck writer elsewhere on the box would stop the + // Inspector from starting. Leaving the file for the next run is the only + // acceptable outcome. + 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); + + const properLockfile = (await import("proper-lockfile")).default; + const release = await properLockfile.lock(filePath, { + realpath: false, + stale: 10_000, + }); + + await expect( + mod.absorbFileSecretsIntoKeyring(new InMemorySecretStore()), + ).resolves.toBeUndefined(); + await release(); + + // Left exactly as it was, and said so — asserting the *lock* message + // specifically, since the pre-existing claim-failure warning also ends in + // "left in place" and would let this pass without the lock ever being hit. + expect(existsSync(filePath)).toBe(true); + expect(warn).toHaveBeenCalledWith( + expect.stringContaining("Could not lock the secrets file"), + ); + }, 60_000); + it("takes no lock when there is nothing to migrate (#2082)", async () => { // The overwhelmingly common startup: a keychain is available and no file // was ever written. Locking first would create and remove a lock @@ -702,6 +735,25 @@ describe("absorbFileSecretsIntoKeyring", () => { expect(existsSync(`${filePath}.lock`)).toBe(false); }); + it("does not treat an unlistable directory as an empty one (#2082)", async () => { + // Only "there is no directory" proves the fresh-install case. A path + // whose parent is a *file* answers `readdir` with ENOTDIR — the other + // shape of "nothing there" — and must be treated the same, whereas an + // EACCES directory that denies listing while still permitting access to + // the known `secrets.json` must not be, or the keychain is selected and + // those secrets go invisible with nothing said. + await fs.writeFile(path.join(tmpDir, "not-a-dir"), "", "utf-8"); + const filePath = path.join(tmpDir, "not-a-dir", "secrets.json"); + process.env.MCP_INSPECTOR_SECRET_FILE = filePath; + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const mod = await loadWithProbe(true); + + await mod.absorbFileSecretsIntoKeyring(new InMemorySecretStore()); + + // ENOTDIR is a fresh-install shape: nothing said, nothing locked. + expect(warn).not.toHaveBeenCalled(); + }); + it("still adopts an orphan when the live file is absent (#2082)", async () => { // The fast path above must not be a `stat` of `secrets.json`: an // interrupted migration leaves *only* the snapshot, which is precisely diff --git a/core/auth/node/file-lock.ts b/core/auth/node/file-lock.ts index d61108d0aa..56d7782ce9 100644 --- a/core/auth/node/file-lock.ts +++ b/core/auth/node/file-lock.ts @@ -96,6 +96,28 @@ const RETRY = { maxTimeout: 1_000, } as const; +/** + * What {@link RETRY} actually sums to, in milliseconds. + * + * Computed rather than written down. `retries * maxTimeout` overstates it by + * a third — the early attempts are the exponential ramp, not the cap — and a + * hand-maintained constant is one edit away from disagreeing with the + * schedule it describes, in a message whose whole job is to tell a user how + * long the Inspector waited. `retry` applies no jitter by default + * (`randomize` is off), so this is exact rather than an estimate. + * + * It must stay **above {@link STALE_MS}**: see {@link RETRY}. + */ +const RETRY_BUDGET_MS = ((): number => { + let total = 0; + let delay = RETRY.minTimeout; + for (let i = 0; i < RETRY.retries; i++) { + total += Math.min(delay, RETRY.maxTimeout); + delay *= RETRY.factor; + } + return total; +})(); + /** Emitted once per process, not once per call — see {@link warnOnce}. */ const warned = new Set(); @@ -231,7 +253,7 @@ export async function withSecretFileLock( // retries — whereas proceeding can lose a secret while reporting success. if (isHeldElsewhere(err)) { throw new SecretStoreUnavailableError( - `Could not save to the secrets file at ${target}: another process has held the lock on it for over ${Math.round((RETRY.retries * RETRY.maxTimeout) / 1000)} seconds. Its secrets are intact; the value you just entered was not saved. If no other Inspector is running, remove ${target}.lock and try again.`, + `Could not save to the secrets file at ${target}: another process has held the lock on it for the ${Math.round(RETRY_BUDGET_MS / 1000)} seconds this save waited. Its secrets are intact; the value you just entered was not saved. If no other Inspector is running, remove ${target}.lock and try again.`, ); } // Everything else is the lock being *unavailable* rather than held — a diff --git a/core/auth/node/secret-store-selection.ts b/core/auth/node/secret-store-selection.ts index ce1704ccfd..e35a5facfc 100644 --- a/core/auth/node/secret-store-selection.ts +++ b/core/auth/node/secret-store-selection.ts @@ -553,6 +553,14 @@ export async function absorbFileSecretsIntoKeyring( // The lock is released before the hand-off: that part reads a snapshot // nobody else can reach, and holding it across a keychain round-trip per // secret would block every writer for the duration of a migration. + // + // Wrapped because `withSecretFileLock` **throws** when another process + // holds the lock past its retry budget, and this function's contract is + // that it never throws: it is awaited directly by both `resolveSecretStore` + // branches, so a stuck writer would fail store resolution and with it the + // whole session. Refusing is the right answer for a `set` — the user is + // waiting on that value — and the wrong one here, where the file simply + // stays put and the next run migrates it. const claimed = await withSecretFileLock(filePath, async () => { // A crash between the claim and the delete leaves only // `secrets.json.migrating-`. Checking the canonical path alone then @@ -600,6 +608,15 @@ export async function absorbFileSecretsIntoKeyring( return null; } return staged; + // `withSecretFileLock` rejects only with the `SecretStoreUnavailableError` + // it constructs itself (a lock it could not create degrades instead of + // throwing), so the cast is over a value produced one call away rather + // than an assumption about arbitrary throwables. + }).catch((err: Error) => { + console.warn( + `\n[mcp-inspector] Could not lock the secrets file at ${filePath} to migrate it into the OS keychain (${err.message}), so it has been left in place. Its secrets are not visible to this session; the next run will try again.`, + ); + return null; }); if (claimed === null) return; const staged = claimed; From 91e1392c1f9cf5e0d1688156becde8269adc86a7 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 10:51:40 -0400 Subject: [PATCH 05/12] fix(auth): stop deleting the winner's lock after a stale takeover (#2082) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review round 4 — four comments, all the same finding against the four files carrying the claim, and it was more than a wording problem. The comment said a holder whose lock was replaced is told. Checked against proper-lockfile@4.1.2, that is false in the case that actually happens: - `updateLock` compares mtime only on its refresh tick — `stale / 2`, so 5s here — while an ordinary mutation is a read, an scrypt derivation and an atomic write, comfortably under a second. The tick never runs, so nothing fires. - `release` → `unlock` → `removeLock` is an unconditional `rmdir` with **no ownership check**. So the compromised holder goes on to delete the *winner's* lock on the way out, ending the winner's exclusion too: one compromised writer silently becomes two unprotected ones. The second half is a correctness bug, not a documentation defect, and it is fixable here even though the takeover race itself is not. `withSecretFileLock` now records the lock directory's inode and birth time at acquire and re-checks them before releasing. On a mismatch it **declines to release** — leaving the new holder's lock alone costs nothing, ours is already gone — and warns. Inode and birth time rather than mtime, deliberately: both survive the library's own `utimes` refresh, so a lock held past one tick is not accused of being compromised, and both change on a delete-and-recreate, which is exactly the event to detect. Where a filesystem reports neither, the two reads agree and the check concludes "ours", degrading to the library's unaided behaviour rather than to a false alarm. An unreadable baseline at acquire likewise answers "ours" — with nothing to compare against, accusing a healthy lock would be worse than releasing it. Detection is therefore **best-effort**, and file-lock.ts, README.md, specification/v2_servers_file.md and AGENTS.md now all say so rather than promising the loser is told. AGENTS.md carries the negative instruction too, since this is the second round spent on the same overclaim. Tests: "does not delete the winner's lock after being taken over" replaces the lock inside the body and asserts the replacement survives with the *same* inode and birth time — it was left alone, not deleted and recreated. It waits for nothing, being the fast case the library's tick misses. Verified by mutation: forcing `stillOurs` to always answer true fails it. A second test covers a release that fails while the lock genuinely *is* ours (a stray file makes `rmdir` fail ENOTEMPTY), so the ownership check cannot swallow a real release failure. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H Signed-off-by: cliffhall --- AGENTS.md | 9 +- README.md | 4 +- .../integration/auth/node/file-lock.test.ts | 68 ++++++++--- core/auth/node/file-lock.ts | 110 +++++++++++++++--- specification/v2_servers_file.md | 2 +- 5 files changed, 160 insertions(+), 33 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 9c99634eb5..8286e68c9e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -73,8 +73,13 @@ v2/main/ │ │ │ # mutual exclusion #2082 settled on — proper-lockfile, │ │ │ # borrowed rather than hand-rolled. Read its header before │ │ │ # citing it: it makes two LIVE Inspectors exclusive, and -│ │ │ # does NOT make stale takeover single-winner — it makes the -│ │ │ # loser detectable (ECOMPROMISED), which is the honest claim. +│ │ │ # does NOT make stale takeover single-winner. proper-lockfile +│ │ │ # detects a takeover only on its 5s refresh tick, which an +│ │ │ # ordinary sub-second mutation never reaches, and its release +│ │ │ # is an unconditional rmdir — so withSecretFileLock does its +│ │ │ # OWN ownership check (inode+birthtime) before releasing, to +│ │ │ # avoid deleting the winner's lock. Detection is BEST-EFFORT; +│ │ │ # do not write that a compromised holder is always told. │ │ │ # DEGRADES when no lock CAN be taken (read-only $HOME etc), │ │ │ # since this store exists for boxes missing the usual │ │ │ # mechanism; but THROWS on ELOCKED — a lock held by a live diff --git a/README.md b/README.md index 2da9c5bb03..cc553c9580 100644 --- a/README.md +++ b/README.md @@ -504,7 +504,9 @@ The Inspector writes the file `0600` and re-tightens it at startup if something **Two Inspectors, one file.** Within a process, mutations are serialized per file path, so a web session's own concurrent saves cannot lose each other. Across processes — a CLI run beside a web session — each mutation takes an exclusive lock on `secrets.json.lock` for the whole read-modify-write, using [`proper-lockfile`](https://github.com/moxystudio/node-proper-lockfile) (the same library npm itself locks with). The lock expires 10 seconds after its holder stops refreshing it, so an Inspector that is killed mid-save does not leave the file unwritable. -Two running Inspectors are therefore genuinely serialized. What a lock file cannot make single-winner is the *takeover of a lock whose holder died* — that needs a compare-and-swap on a directory entry (`renameat2`) which Node does not expose, and it is what an earlier hand-rolled attempt failed three review rounds on. `proper-lockfile` does not close that race either; what it adds is that the loser is **told** (it compares the lock's mtime against the value it recorded, so a holder whose lock was replaced is marked compromised and warns). The window opens only after a holder dies without releasing. +Two running Inspectors are therefore genuinely serialized. What a lock file cannot make single-winner is the *takeover of a lock whose holder died* — that needs a compare-and-swap on a directory entry (`renameat2`) which Node does not expose, and it is what an earlier hand-rolled attempt failed three review rounds on. `proper-lockfile` does not close that race either. The window opens only after a holder dies without releasing. + +The Inspector adds one thing on top: before releasing, it checks the lock directory is still the one it created (by inode and birth time, which survive the library's own refresh but not a delete-and-recreate). That matters mostly because the library's release is an unconditional `rmdir` — so a holder whose lock had been replaced would otherwise delete the *winner's* lock on the way out, turning one compromised writer into two unprotected ones. It also surfaces the takeover as a warning. Treat that warning as **best-effort**, not a guarantee: it rests on filesystem metadata that not every filesystem reports. Which is why, underneath the lock, each mutation still reads the file, applies its change, writes, then reads back and compares the whole map; if something wrote in between it re-applies onto what was left and retries, failing loudly after five lost rounds rather than returning as though the value were saved. That check is what still catches a clobber inside that window — and it covers what no lock can, since a lock only orders the writers that *take* it: an editor, a restored backup, or an Inspector older than this release. diff --git a/clients/web/src/test/integration/auth/node/file-lock.test.ts b/clients/web/src/test/integration/auth/node/file-lock.test.ts index f0619035d9..ce41fdf467 100644 --- a/clients/web/src/test/integration/auth/node/file-lock.test.ts +++ b/clients/web/src/test/integration/auth/node/file-lock.test.ts @@ -360,27 +360,67 @@ describe("withSecretFileLock degrades rather than failing", () => { }); describe("withSecretFileLock reports what it cannot clean up", () => { - it("warns rather than crashing the process when the lock is taken over", async () => { - // `proper-lockfile`'s default `onCompromised` *throws*, from a timer - // with no caller on the stack — an uncaught exception that takes an - // Inspector session down. Driven for real: the lock directory is removed - // while held, which is what an operator "clearing a stuck lock" does, - // and the library's own refresh tick (`stale / 2`) notices. + it("does not delete the winner's lock after being taken over", async () => { + // The destructive half of the stale-takeover race, and the reason this + // check exists at all. `proper-lockfile`'s release is an unconditional + // `rmdir`: a holder whose lock was replaced deletes the *winner's* + // directory on the way out, so one compromised holder becomes two + // unprotected writers. Its own detection cannot prevent that — it runs on + // the refresh tick (5s here) while an ordinary mutation finishes in well + // under a second, so the tick never runs and nobody is told. + // + // No waiting here, deliberately: this is the fast case the tick misses. + const target = filePath(); + const lockPath = `${target}.lock`; + + let winner: { ino: number; birthtimeMs: number } | undefined; + const result = await withSecretFileLock(target, async () => { + // Someone declares our lock stale, removes it, and takes over. + await fs.rm(lockPath, { recursive: true, force: true }); + await fs.mkdir(lockPath); + const stat = await fs.stat(lockPath); + winner = { ino: stat.ino, birthtimeMs: stat.birthtimeMs }; + return "saved"; + }); + + expect(result).toBe("saved"); + // The winner's lock is untouched — same directory, not a recreated one. + const after = await fs.stat(lockPath); + expect(after.ino).toBe(winner?.ino); + expect(after.birthtimeMs).toBe(winner?.birthtimeMs); + expect(warnings()).toContain("was taken over by another process"); + }); + + it("warns instead of throwing when releasing a lock that is ours fails", async () => { + // The ownership check above must not swallow a genuine release failure. + // `rmdir` refuses a non-empty directory, so a stray file inside the lock + // leaves it identifiably *ours* — same inode, same birth time — and still + // unremovable. const target = filePath(); const result = await withSecretFileLock(target, async () => { + await fs.writeFile(`${target}.lock/stray`, "", "utf-8"); + return "saved"; + }); + + // The body's result is returned regardless: a save that completed must + // not be turned into a failure by its own teardown. + expect(result).toBe("saved"); + expect(warnings()).toContain("Could not release the lock"); + }); + + it("warns rather than crashing the process when the library detects the takeover", async () => { + // `proper-lockfile`'s default `onCompromised` *throws*, from a timer with + // no caller on the stack — an uncaught exception that takes an Inspector + // session down. Replaced with a warning. This is the slow path: the lock + // is removed and the body stays alive past the refresh tick, so the + // library's own detection fires rather than the release-time check above. + const target = filePath(); + await withSecretFileLock(target, async () => { await fs.rm(`${target}.lock`, { recursive: true, force: true }); await vi.waitFor( () => expect(warnings()).toContain("was taken over by another process"), { timeout: 20_000, interval: 250 }, ); - return "saved"; }); - - // The body's result is returned regardless. A compromised lock means the - // guarantee was lost, not that the work did not happen, and the release - // that then fails (`ELOCKNOTHELD`, since the library has already given - // the lock up) must not turn a completed save into a thrown error. - expect(result).toBe("saved"); - expect(warnings()).toContain("Could not release the lock"); }, 30_000); }); diff --git a/core/auth/node/file-lock.ts b/core/auth/node/file-lock.ts index 56d7782ce9..4b1f2b9d74 100644 --- a/core/auth/node/file-lock.ts +++ b/core/auth/node/file-lock.ts @@ -29,12 +29,25 @@ * the hand-rolled version could not close, and it cannot be closed with what * Node exposes (`renameat2(RENAME_EXCHANGE)`). * - * What the library adds over the hand-rolled one is that the loser finds - * out. Its refresh tick compares the lock's mtime against the value it - * recorded at acquire, so a holder whose directory was replaced is marked - * `ECOMPROMISED` and told — see `onCompromised` below. The window is also - * narrow and conditional: it opens only after a holder *dies without - * releasing*, since nothing else lets a lock go stale. + * The library detects it only on its refresh tick — `updateLock` compares + * the lock's mtime against the value recorded at acquire and fires + * `onCompromised`. That tick runs at `stale / 2`, i.e. every 5s here, while + * an ordinary mutation is a read, an scrypt derivation and an atomic write: + * comfortably under a second. **So in the common case the tick never runs + * and the library tells nobody.** Worse, its `release` path calls `rmdir` + * unconditionally, with no ownership check — so a holder whose lock was + * replaced goes on to delete the *winner's* lock on the way out, silently + * ending the winner's exclusion too. + * + * {@link withSecretFileLock} therefore does its own ownership check before + * releasing (see there). That closes the destructive half — we never remove + * a lock directory that is not the one we created — and surfaces the + * compromise in exactly the fast-mutation case the tick misses. Detection is + * still **best-effort**, not a guarantee: it rests on inode and birth-time + * identity, which some filesystems do not report. + * + * The window is at least narrow and conditional: it opens only after a + * holder *dies without releasing*, since nothing else lets a lock go stale. * * **Which is why the optimistic verify in {@link FileSecretStore.mutate} * stays, and is not belt-and-braces.** It is what still catches a clobber @@ -166,6 +179,53 @@ const isHeldElsewhere = (err: unknown): boolean => const describeError = (err: unknown): string => err instanceof Error ? err.message : String(err); +/** Where `proper-lockfile` puts the lock for `target` — its documented default. */ +const lockPathOf = (target: string): string => `${target}.lock`; + +/** + * A lock directory's identity, or `null` if it could not be read. + * + * `ino` and `birthtimeMs` together: a directory removed and recreated gets a + * new inode and a new birth time, while `utimes` — which the library's own + * refresh tick performs on our behalf every few seconds — changes neither. + * + * Not every filesystem reports both (Windows shares, some network mounts, and + * older kernels report `0`). There the two reads simply agree and the check + * below concludes the lock is ours, which is the behaviour without this check + * at all — best-effort, and never a false alarm on a healthy lock. + */ +async function identify( + lockPath: string, +): Promise<{ ino: number; birthtimeMs: number } | null> { + try { + const stat = await fs.stat(lockPath); + return { ino: stat.ino, birthtimeMs: stat.birthtimeMs }; + } catch { + return null; + } +} + +/** + * Is the lock directory still the one we created? + * + * A missing directory counts as **not ours** — it was removed by a takeover + * (or by hand), and there is nothing of ours left to release. + * + * An unreadable *baseline* (`claimedAs === null`) is the one case that + * answers "ours": we could not identify the directory at acquire, so we have + * nothing to compare against and must not accuse a healthy lock. Releasing is + * then exactly what the library would have done unaided. + */ +async function stillOurs( + lockPath: string, + claimedAs: { ino: number; birthtimeMs: number } | null, +): Promise { + if (claimedAs === null) return true; + const now = await identify(lockPath); + if (now === null) return false; + return now.ino === claimedAs.ino && now.birthtimeMs === claimedAs.birthtimeMs; +} + /** * One `proper-lockfile` acquire against `target`, with the given retry policy. * @@ -266,20 +326,40 @@ export async function withSecretFileLock( ); return fn(); } + // Identity of the directory we just created, for the ownership check below. + // **Not the mtime**, which is what the library compares: a lock held longer + // than one refresh tick has its mtime rewritten by `utimes` legitimately, + // so an mtime comparison would call our own healthy lock compromised. The + // inode and birth time both survive `utimes` and both change when a + // directory is removed and recreated, which is precisely the event to + // detect. + const claimedAs = await identify(lockPathOf(target)); try { return await fn(); } finally { - try { - await release(); - } catch (err) { - // The body already ran and its result is being returned; a release - // that failed means the lock was taken from us (declared stale while - // we held it) or the directory went away. Neither is worth turning a - // successful save into a failure, but a silent catch would leave a - // lock nobody can explain, so say it. + if (!(await stillOurs(lockPathOf(target), claimedAs))) { + // Someone declared our lock stale and replaced it. **Do not release**: + // `proper-lockfile`'s release is an unconditional `rmdir`, so calling + // it here would delete the lock directory that now belongs to whoever + // took over — ending their exclusion as well as ours, and turning one + // compromised holder into two unprotected writers. Leaving their lock + // alone costs us nothing; ours is already gone. warnOnce( - `Could not release the lock on the secrets file at ${target} (${describeError(err)}). It expires on its own after ${STALE_MS / 1000}s.`, + `The lock on the secrets file at ${target} was taken over by another process while a write was in progress. If a secret you just saved is missing, save it again.`, ); + } else { + try { + await release(); + } catch (err) { + // The body already ran and its result is being returned; a release + // that failed means the lock was taken from us (declared stale while + // we held it) or the directory went away. Neither is worth turning a + // successful save into a failure, but a silent catch would leave a + // lock nobody can explain, so say it. + warnOnce( + `Could not release the lock on the secrets file at ${target} (${describeError(err)}). It expires on its own after ${STALE_MS / 1000}s.`, + ); + } } } } diff --git a/specification/v2_servers_file.md b/specification/v2_servers_file.md index 32a8d9dca5..a529006b19 100644 --- a/specification/v2_servers_file.md +++ b/specification/v2_servers_file.md @@ -261,7 +261,7 @@ Each server entry may carry these Inspector-extension fields at the top level: - **Secret store selection (#1950)**: #1356 assumed a keychain. On a host without one — the published container (no D-Bus session, #1848), Android/Termux (no prebuilt binary, #1905), a minimal Linux install — `set` hard-failed, so those users could not persist an OAuth client secret or a stdio `env:` value **at all**. #1950 adds the two missing implementations plus the policy that picks one, and the surfaces that say which was picked. - **Selection**: `MCP_INSPECTOR_SECRET_STORE=keyring|file|memory` wins outright. Otherwise the keychain is _probed_ — an `AsyncEntry` construction plus a real read, because the container that motivated #1848 imports the package fine and only fails when it reaches for a Secret Service that isn't there. A read and not a write, deliberately: probing by writing would deposit a value in the user's login keyring at every startup for a store they may never use. If the probe fails the fallback is **`memory`** in a container whose secrets directory is not on a mount, and **`file`** otherwise — so mounting the volume the README already recommends for the catalog flips the same run to durable storage with no configuration, and an unmounted container gets the honest answer rather than a file `docker run --rm` will discard. Resolution is cached per process so the startup banner, `GET /api/config`, and the store doing the writing cannot disagree. - **`FileSecretStore` at rest**: one JSON document at `~/.mcp-inspector/secrets.json` (or `MCP_INSPECTOR_SECRET_FILE`, else under `MCP_STORAGE_DIR`), written `0600` and re-tightened at startup. With `MCP_INSPECTOR_SECRET_KEY` set it is AES-256-GCM with a scrypt-derived key and a per-write random salt; without it the values are in the clear, and _that_ is what the loud banner and the warning-toned modal footer are about. The **whole map is encrypted as a unit**, not value-by-value, so the account names (`serverId:field`) are hidden too — a per-value scheme would leave a readable index of which servers you hold a client secret for. Upgrading is lazy: setting the passphrase later re-encrypts on the next write rather than rewriting the file during a run that may never touch a secret, and the descriptor reports `pendingEncryption` until it does. A file that can no longer be decrypted reads as empty but **refuses to be written**, because replacing a file of still-valid secrets to satisfy an additive request destroys data. - - **Concurrency**: within a process, mutations are serialized per **resolved file path** (not per store instance — two `FileSecretStore`s on one file are ordinary, since the resolved store holds one and the keychain hand-off builds another). Across processes, each mutation holds an exclusive `proper-lockfile` lock on `secrets.json.lock` for the whole read-modify-write (#2082). The in-process queue is what keeps that usable: `proper-lockfile` is not reentrant, so a second `lock()` from the same process fails `ELOCKED` and would be indistinguishable from a genuine remote holder. An earlier iteration hand-rolled the lock — a `mkdir` election with an owner stamp, heartbeat and stale-takeover — and three review rounds each found a real race, the last not closable with what Node exposes (claiming a stale lock needs compare-and-swap on a directory entry, `renameat2(RENAME_EXCHANGE)`). #2082 settled that as "borrow, don't hand-roll" — but the borrowed lock is not claimed to close that race, because it does not. `proper-lockfile@4.1.2` `rmdir`s a stale lock and re-`mkdir`s without checking the directory it removed is the one it found stale, so a slow waiter can still delete a fast waiter's fresh lock; what it adds over the hand-rolled version is *detection* — its refresh tick compares the lock's mtime against the value recorded at acquire, so a compromised holder is told (`ECOMPROMISED`, surfaced as a warning) instead of proceeding silently. What **is** exclusive is the case that matters: `mkdir` is atomic and a live holder refreshes its mtime, so its lock never goes stale and two running Inspectors are genuinely serialized. The residual window opens only after a holder dies without releasing. A waiter also waits past the stale window before giving up (so a crashed holder resolves by takeover rather than failing everyone else's saves), and a lock still held after that makes `set` **fail** rather than write alongside a visible concurrent writer — `ELOCKED` is evidence the lock is working, not a reason to bypass it. **The optimistic verify stays underneath it**, and is not redundant: read `M0`, apply, write `M1`, read back `M2`, re-apply onto whatever a concurrent writer left if they differ, bounded, with `set` throwing on non-convergence rather than returning as though the value were saved. The comparison is over the **whole map** — checking only your own entry passes in exactly the case that loses data, because yours is present and the other writer's is gone. A lock is advisory between the processes that take it, so the verify is what covers a writer outside this codebase (an editor, a restored backup, an Inspector predating #2082) and what covers the lock being *unavailable*: `withSecretFileLock` runs the body anyway, warning once, on a directory that cannot hold a lock file — this store exists for boxes where the usual mechanism is missing (#1848, #1905) and must not acquire a new way to be unavailable. Reads take no lock; `writeStoreFile` is atomic, so a reader sees the old file or the new one, never a torn one. + - **Concurrency**: within a process, mutations are serialized per **resolved file path** (not per store instance — two `FileSecretStore`s on one file are ordinary, since the resolved store holds one and the keychain hand-off builds another). Across processes, each mutation holds an exclusive `proper-lockfile` lock on `secrets.json.lock` for the whole read-modify-write (#2082). The in-process queue is what keeps that usable: `proper-lockfile` is not reentrant, so a second `lock()` from the same process fails `ELOCKED` and would be indistinguishable from a genuine remote holder. An earlier iteration hand-rolled the lock — a `mkdir` election with an owner stamp, heartbeat and stale-takeover — and three review rounds each found a real race, the last not closable with what Node exposes (claiming a stale lock needs compare-and-swap on a directory entry, `renameat2(RENAME_EXCHANGE)`). #2082 settled that as "borrow, don't hand-roll" — but the borrowed lock is not claimed to close that race, because it does not. `proper-lockfile@4.1.2` `rmdir`s a stale lock and re-`mkdir`s without checking the directory it removed is the one it found stale, so a slow waiter can still delete a fast waiter's fresh lock; and its own detection is weaker than it looks: `updateLock` compares mtime only on the refresh tick (`stale / 2`, 5s here), while an ordinary mutation finishes in well under a second — so in the common case the tick never runs and nobody is told. Its `release` is also an unconditional `rmdir` with no ownership check, so a holder whose lock was replaced deletes the *winner's* lock on the way out, turning one compromised writer into two unprotected ones. `withSecretFileLock` therefore performs its own ownership check before releasing (inode + birth time, which survive the library's `utimes` refresh but not a delete-and-recreate): on a mismatch it declines to release and warns. That closes the destructive half outright and surfaces the takeover in the fast case the tick misses — but detection remains **best-effort**, since not every filesystem reports those fields, and where they are unavailable it degrades to the library's unaided behaviour rather than to a false alarm. What **is** exclusive is the case that matters: `mkdir` is atomic and a live holder refreshes its mtime, so its lock never goes stale and two running Inspectors are genuinely serialized. The residual window opens only after a holder dies without releasing. A waiter also waits past the stale window before giving up (so a crashed holder resolves by takeover rather than failing everyone else's saves), and a lock still held after that makes `set` **fail** rather than write alongside a visible concurrent writer — `ELOCKED` is evidence the lock is working, not a reason to bypass it. **The optimistic verify stays underneath it**, and is not redundant: read `M0`, apply, write `M1`, read back `M2`, re-apply onto whatever a concurrent writer left if they differ, bounded, with `set` throwing on non-convergence rather than returning as though the value were saved. The comparison is over the **whole map** — checking only your own entry passes in exactly the case that loses data, because yours is present and the other writer's is gone. A lock is advisory between the processes that take it, so the verify is what covers a writer outside this codebase (an editor, a restored backup, an Inspector predating #2082) and what covers the lock being *unavailable*: `withSecretFileLock` runs the body anyway, warning once, on a directory that cannot hold a lock file — this store exists for boxes where the usual mechanism is missing (#1848, #1905) and must not acquire a new way to be unavailable. Reads take no lock; `writeStoreFile` is atomic, so a reader sees the old file or the new one, never a torn one. - **Strict reads (`getStrict`)**: `get` is tolerant by contract, and that tolerance is wrong for exactly one caller — a migration that treats `null` as proof of absence and then _writes_. A transient read failure would look like "nothing there", and the write would replace a newer stored value with an older on-disk copy, inverting the keychain-wins rule the migration is built on. `getStrict` throws instead, and both plaintext migrations plus the keychain hand-off use it. Two traps this hit on the way in, both worth remembering: the seam must be forwarded by `DeferredSecretStore` (the store production actually uses, so without forwarding the strictness existed only in tests that injected a concrete store), and it must be implemented by _every_ store rather than falling back to `get` for the one it was introduced for. - **Bulk reads (`getMany`)**: rehydration asked field by field, and `FileSecretStore.get` reads and decrypts the entire file per call. scrypt at `N=16384` measures ~23ms, so an encrypted catalog spent ~450ms of pure key derivation on every `GET /api/servers` — a visible stall rather than a micro-optimization. The seam takes a **list of `{ serverId, fields }` across servers**, and both rehydration callers pass the whole catalog in one request, so `FileSecretStore` decrypts once per rehydration; stores for which per-field reads are already cheap (the keychain) fall back to parallel `get`. The cross-server shape is the point: a per-server version shipped first and left a 20-server catalog paying 20 serialized derivations, because both callers iterate servers — the same stall reached one server at a time instead of one field at a time. - **Durability gate on migration**: the plaintext-stripping migrations only delete the disk copy once the value is somewhere that outlives the process (`isDurable`). Against a session-scoped store, stripping would trade a secret that survives restarts for one that dies with the process — and it runs on an ordinary `GET`, so merely opening the app would destroy it. From b159c75b889d0ac6e4507fb08e93d1e3af765375 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 11:10:48 -0400 Subject: [PATCH 06/12] fix(auth): guard every lock removal, not just release, and stop promising expiry (#2082) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review round 5. Both comments upheld; the first is a real hole in round 4's own fix. **The ownership guard was in the wrong place, and overclaimed.** Round 4 checked ownership and then called `release()`. Two problems: - Check-then-act. The `stat` and the `rmdir` were separate async steps, so a waiter could replace the directory in between and we deleted the winner's fresh lock anyway — the very thing the check was added to prevent. - Skipping `release()` on a mismatch left proper-lockfile's record registered in its `locks` map, and its `signal-exit` handler `rmdirSync`s every registered lock with **no ownership check of its own**. An exit during that window deleted the winner too. The guard now lives in `options.fs`, which is the single seam *both* removal paths route through — `removeLock` on release, and the exit handler. It refuses to remove a directory whose inode and birth time are not the ones recorded at acquire, and reports the refusal as success so the library's bookkeeping forgets the lock either way (leaving it registered is what hands the exit handler a record pointing at the winner's directory). The check is a `statSync` immediately followed by an `rmdirSync`, with no `await` between them, so nothing in this process can interleave. **It still does not close the race** — it is check-then-act across processes, which needs the compare-and-swap Node does not expose — and saying it "closes the destructive half outright" was wrong. file-lock.ts, README.md, specification/v2_servers_file.md and AGENTS.md now all say it narrows the window rather than closing it. AGENTS.md carries the negative instruction alongside round 4's, since this is the third round spent on an overclaim. **"It expires on its own after 10s" is false for the ENOTEMPTY case** the new test exercises. Stale takeover reclaims through the same `rmdir`, which also cannot remove a non-empty directory — so nothing clears it, and every later save fails ELOCKED against it. `releaseAdvice` now branches: by-hand removal for ENOTEMPTY, expiry for everything else. Tests: a real child process demonstrates the exit handler removing a replacement lock (with an **empty** directory — a non-empty one makes `rmdirSync` fail ENOTEMPTY, which is how the first draft of this test passed for the wrong reason). The release-failure test now asserts the message says "by hand" and does not promise expiry. The shim's `rmdirSync` arm carries a justified `v8 ignore`: it is reachable only from a real process exit, and its logic is `removeIfMine`, covered through the async arm. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H Signed-off-by: cliffhall --- AGENTS.md | 12 +- README.md | 2 +- .../integration/auth/node/file-lock.test.ts | 70 ++++-- core/auth/node/file-lock.ts | 199 ++++++++++++------ specification/v2_servers_file.md | 2 +- 5 files changed, 201 insertions(+), 84 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 8286e68c9e..5956b41f6b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -76,10 +76,14 @@ v2/main/ │ │ │ # does NOT make stale takeover single-winner. proper-lockfile │ │ │ # detects a takeover only on its 5s refresh tick, which an │ │ │ # ordinary sub-second mutation never reaches, and its release -│ │ │ # is an unconditional rmdir — so withSecretFileLock does its -│ │ │ # OWN ownership check (inode+birthtime) before releasing, to -│ │ │ # avoid deleting the winner's lock. Detection is BEST-EFFORT; -│ │ │ # do not write that a compromised holder is always told. +│ │ │ # is an unconditional rmdir (as is its signal-exit handler) — +│ │ │ # so withSecretFileLock passes a GUARDED options.fs, the one +│ │ │ # seam both removal paths share, refusing to delete a lock +│ │ │ # that is no longer ours (inode+birthtime). That NARROWS the +│ │ │ # window, it does not close it — still check-then-act across +│ │ │ # processes. BEST-EFFORT throughout: do not write that a +│ │ │ # compromised holder is always told, or that the winner's +│ │ │ # lock is always preserved. │ │ │ # DEGRADES when no lock CAN be taken (read-only $HOME etc), │ │ │ # since this store exists for boxes missing the usual │ │ │ # mechanism; but THROWS on ELOCKED — a lock held by a live diff --git a/README.md b/README.md index cc553c9580..4fa3764a82 100644 --- a/README.md +++ b/README.md @@ -506,7 +506,7 @@ The Inspector writes the file `0600` and re-tightens it at startup if something Two running Inspectors are therefore genuinely serialized. What a lock file cannot make single-winner is the *takeover of a lock whose holder died* — that needs a compare-and-swap on a directory entry (`renameat2`) which Node does not expose, and it is what an earlier hand-rolled attempt failed three review rounds on. `proper-lockfile` does not close that race either. The window opens only after a holder dies without releasing. -The Inspector adds one thing on top: before releasing, it checks the lock directory is still the one it created (by inode and birth time, which survive the library's own refresh but not a delete-and-recreate). That matters mostly because the library's release is an unconditional `rmdir` — so a holder whose lock had been replaced would otherwise delete the *winner's* lock on the way out, turning one compromised writer into two unprotected ones. It also surfaces the takeover as a warning. Treat that warning as **best-effort**, not a guarantee: it rests on filesystem metadata that not every filesystem reports. +The Inspector adds one thing on top: every lock-directory removal the library makes on its behalf — on release, and from its exit handler — is guarded by a check that the directory is still the one it created (by inode and birth time, which survive the library's own refresh but not a delete-and-recreate). That matters because those removals are otherwise unconditional, so a holder whose lock had been replaced would delete the *winner's* lock on the way out, turning one compromised writer into two unprotected ones. It also surfaces the takeover as a warning. Treat all of this as **best-effort**: the guard is still a check followed by an act, so it makes the destructive case rare rather than impossible, and it rests on filesystem metadata that not every filesystem reports. Which is why, underneath the lock, each mutation still reads the file, applies its change, writes, then reads back and compares the whole map; if something wrote in between it re-applies onto what was left and retries, failing loudly after five lost rounds rather than returning as though the value were saved. That check is what still catches a clobber inside that window — and it covers what no lock can, since a lock only orders the writers that *take* it: an editor, a restored backup, or an Inspector older than this release. diff --git a/clients/web/src/test/integration/auth/node/file-lock.test.ts b/clients/web/src/test/integration/auth/node/file-lock.test.ts index ce41fdf467..c42dbd3db9 100644 --- a/clients/web/src/test/integration/auth/node/file-lock.test.ts +++ b/clients/web/src/test/integration/auth/node/file-lock.test.ts @@ -361,13 +361,12 @@ describe("withSecretFileLock degrades rather than failing", () => { describe("withSecretFileLock reports what it cannot clean up", () => { it("does not delete the winner's lock after being taken over", async () => { - // The destructive half of the stale-takeover race, and the reason this - // check exists at all. `proper-lockfile`'s release is an unconditional - // `rmdir`: a holder whose lock was replaced deletes the *winner's* - // directory on the way out, so one compromised holder becomes two - // unprotected writers. Its own detection cannot prevent that — it runs on - // the refresh tick (5s here) while an ordinary mutation finishes in well - // under a second, so the tick never runs and nobody is told. + // The destructive half of the stale-takeover race. `proper-lockfile`'s + // removal is an unconditional `rmdir`, so a holder whose lock was + // replaced deletes the *winner's* directory on the way out — one + // compromised holder becoming two unprotected writers. Its own detection + // cannot prevent that: it runs on the refresh tick (5s here) while an + // ordinary mutation finishes in well under a second. // // No waiting here, deliberately: this is the fast case the tick misses. const target = filePath(); @@ -375,7 +374,6 @@ describe("withSecretFileLock reports what it cannot clean up", () => { let winner: { ino: number; birthtimeMs: number } | undefined; const result = await withSecretFileLock(target, async () => { - // Someone declares our lock stale, removes it, and takes over. await fs.rm(lockPath, { recursive: true, force: true }); await fs.mkdir(lockPath); const stat = await fs.stat(lockPath); @@ -384,18 +382,58 @@ describe("withSecretFileLock reports what it cannot clean up", () => { }); expect(result).toBe("saved"); - // The winner's lock is untouched — same directory, not a recreated one. + // Same directory, not a recreated one — it was left alone, not deleted. const after = await fs.stat(lockPath); expect(after.ino).toBe(winner?.ino); expect(after.birthtimeMs).toBe(winner?.birthtimeMs); expect(warnings()).toContain("was taken over by another process"); }); - it("warns instead of throwing when releasing a lock that is ours fails", async () => { - // The ownership check above must not swallow a genuine release failure. - // `rmdir` refuses a non-empty directory, so a stray file inside the lock - // leaves it identifiably *ours* — same inode, same birth time — and still - // unremovable. + it("the exit handler this guards against really does delete a lock", async () => { + // The other lifecycle window, and the reason the guard lives in + // `options.fs` rather than in a check before `release()`: + // `proper-lockfile` registers a `signal-exit` handler that `rmdirSync`s + // every lock it still has registered, with no ownership check of its own. + // A guard placed only around release would leave that free to delete the + // winner's directory if the process exits at the wrong moment. + // + // Shown in a real child, because the handler only runs on a real exit, + // and with an **empty** replacement directory — a non-empty one makes + // `rmdirSync` fail `ENOTEMPTY` and would "pass" for the wrong reason, + // which is exactly how the first draft of this test fooled itself. + // + // `withSecretFileLock` routes that same handler through the shim's + // `rmdirSync`, which shares `removeIfMine` with the release path proven + // by the test above. + const target = filePath(); + const lockPath = `${target}.lock`; + await fs.mkdir(path.dirname(target), { recursive: true }); + + const script = ` + const lockfile = require(${JSON.stringify(LOCKFILE_MODULE)}); + const fs = require("node:fs"); + const lockPath = ${JSON.stringify(lockPath)}; + lockfile + .lock(${JSON.stringify(target)}, { realpath: false, stale: 10000 }) + .then(() => { + // Taken over while we hold it, then exit *without* releasing. + fs.rmSync(lockPath, { recursive: true, force: true }); + fs.mkdirSync(lockPath); + process.exit(0); + }); + `; + await run(process.execPath, ["-e", script]); + + // Unguarded, the winner's directory is gone. + expect(existsSync(lockPath)).toBe(false); + }, 30_000); + + it("tells the operator to clear a lock that cannot expire on its own", async () => { + // `rmdir` refuses a non-empty directory — and so does stale takeover, + // which reclaims through the same call. So an `ENOTEMPTY` lock is not + // cleaned up by the staleness mechanism, by us, or by the next writer: + // every later save fails `ELOCKED` against it until somebody deletes it. + // Promising it "expires on its own after 10s" would be false. const target = filePath(); const result = await withSecretFileLock(target, async () => { await fs.writeFile(`${target}.lock/stray`, "", "utf-8"); @@ -406,6 +444,8 @@ describe("withSecretFileLock reports what it cannot clean up", () => { // not be turned into a failure by its own teardown. expect(result).toBe("saved"); expect(warnings()).toContain("Could not release the lock"); + expect(warnings()).toContain("by hand"); + expect(warnings()).not.toContain("expires on its own"); }); it("warns rather than crashing the process when the library detects the takeover", async () => { @@ -413,7 +453,7 @@ describe("withSecretFileLock reports what it cannot clean up", () => { // no caller on the stack — an uncaught exception that takes an Inspector // session down. Replaced with a warning. This is the slow path: the lock // is removed and the body stays alive past the refresh tick, so the - // library's own detection fires rather than the release-time check above. + // library's own detection fires rather than the removal guard. const target = filePath(); await withSecretFileLock(target, async () => { await fs.rm(`${target}.lock`, { recursive: true, force: true }); diff --git a/core/auth/node/file-lock.ts b/core/auth/node/file-lock.ts index 4b1f2b9d74..11db6a3888 100644 --- a/core/auth/node/file-lock.ts +++ b/core/auth/node/file-lock.ts @@ -39,12 +39,19 @@ * replaced goes on to delete the *winner's* lock on the way out, silently * ending the winner's exclusion too. * - * {@link withSecretFileLock} therefore does its own ownership check before - * releasing (see there). That closes the destructive half — we never remove - * a lock directory that is not the one we created — and surfaces the - * compromise in exactly the fast-mutation case the tick misses. Detection is - * still **best-effort**, not a guarantee: it rests on inode and birth-time - * identity, which some filesystems do not report. + * {@link withSecretFileLock} therefore guards every removal the library makes + * on its behalf — see `guardedFs`, which sits in `options.fs` so it covers + * the release path *and* the `signal-exit` handler. It refuses to delete a + * directory that is no longer the one we created, and surfaces the compromise + * in the fast-mutation case the tick misses. + * + * **That narrows the destructive window; it does not close it.** The guard is + * a `statSync` immediately followed by an `rmdirSync`, so nothing in this + * process can interleave — but it is still check-then-act against other + * processes, and closing that needs the same compare-and-swap Node does not + * expose. It also rests on inode and birth-time identity, which some + * filesystems do not report. Best-effort throughout: it makes the destructive + * case rare, not impossible. * * The window is at least narrow and conditional: it opens only after a * holder *dies without releasing*, since nothing else lets a lock go stale. @@ -68,6 +75,7 @@ * underneath it, and says so once. */ +import nodeFs, { statSync, rmdirSync } from "node:fs"; import * as fs from "node:fs/promises"; import * as path from "node:path"; // CJS-only package. A default import is the shape that survives every @@ -182,23 +190,30 @@ const describeError = (err: unknown): string => /** Where `proper-lockfile` puts the lock for `target` — its documented default. */ const lockPathOf = (target: string): string => `${target}.lock`; +/** A lock directory's identity: what changes on delete-and-recreate. */ +interface LockIdentity { + ino: number; + birthtimeMs: number; +} + /** - * A lock directory's identity, or `null` if it could not be read. + * Identify a lock directory, or `null` if it cannot be read. * - * `ino` and `birthtimeMs` together: a directory removed and recreated gets a - * new inode and a new birth time, while `utimes` — which the library's own - * refresh tick performs on our behalf every few seconds — changes neither. + * `ino` and `birthtimeMs` together, and **not the mtime** the library + * compares: a lock held longer than one refresh tick has its mtime rewritten + * by `utimes` legitimately, so an mtime comparison would call our own healthy + * lock compromised. Both of these survive `utimes` and both change when a + * directory is removed and recreated, which is the event to detect. * - * Not every filesystem reports both (Windows shares, some network mounts, and - * older kernels report `0`). There the two reads simply agree and the check - * below concludes the lock is ours, which is the behaviour without this check - * at all — best-effort, and never a false alarm on a healthy lock. + * Not every filesystem reports either (Windows shares, some network mounts, + * older kernels report `0`). There the comparison trivially succeeds and the + * guard below concludes the lock is ours — the behaviour without the guard at + * all, which is the right way to fail: best-effort, never a false alarm on a + * healthy lock. */ -async function identify( - lockPath: string, -): Promise<{ ino: number; birthtimeMs: number } | null> { +function identifySync(lockPath: string): LockIdentity | null { try { - const stat = await fs.stat(lockPath); + const stat = statSync(lockPath); return { ino: stat.ino, birthtimeMs: stat.birthtimeMs }; } catch { return null; @@ -206,24 +221,89 @@ async function identify( } /** - * Is the lock directory still the one we created? + * A `proper-lockfile` `fs` shim whose directory removal refuses to delete a + * lock that is no longer the one we created. + * + * **Why this sits in `options.fs` rather than in a check before `release()`.** + * Every deletion the library performs on our behalf goes through this object + * — the `release()` path (`removeLock` → `fs.rmdir`) *and* its `signal-exit` + * handler (`rmdirSync` over every registered lock, with no ownership check of + * its own). A check placed before `release()` covers only the first, and + * leaves the second free to delete the winner's lock if the process exits at + * the wrong moment. Guarding at the single point where a directory is + * actually removed covers both, and there is nowhere narrower to put it. * - * A missing directory counts as **not ours** — it was removed by a takeover - * (or by hand), and there is nothing of ours left to release. + * **It narrows the window; it does not close it.** The guard is + * `statSync` immediately followed by `rmdirSync`, with no `await` between + * them — so nothing else *in this process* can interleave, and the gap is as + * small as this platform allows. It is still a check-then-act against other + * processes, and closing that needs compare-and-swap on a directory entry + * (`renameat2(RENAME_EXCHANGE)`), which is exactly what Node does not expose + * and what the whole stale-takeover problem reduces to. Treat this as making + * the destructive case rare, not impossible. * - * An unreadable *baseline* (`claimedAs === null`) is the one case that - * answers "ours": we could not identify the directory at acquire, so we have - * nothing to compare against and must not accuse a healthy lock. Releasing is - * then exactly what the library would have done unaided. + * `owned.id` stays `null` until we have acquired, which is deliberate: during + * `acquireLock` the library removes *another* holder's stale directory + * through this same seam, and that removal must go through untouched. */ -async function stillOurs( +function guardedFs( lockPath: string, - claimedAs: { ino: number; birthtimeMs: number } | null, -): Promise { - if (claimedAs === null) return true; - const now = await identify(lockPath); - if (now === null) return false; - return now.ino === claimedAs.ino && now.birthtimeMs === claimedAs.birthtimeMs; + owned: { id: LockIdentity | null }, + onRefused: () => void, +): unknown { + const mine = (): boolean => { + if (owned.id === null) return true; // Not ours yet — see above. + const now = identifySync(lockPath); + if (now === null) return false; // Already gone; nothing of ours to remove. + return now.ino === owned.id.ino && now.birthtimeMs === owned.id.birthtimeMs; + }; + const removeIfMine = (): void => { + if (!mine()) { + onRefused(); + return; + } + rmdirSync(lockPath); + }; + return { + ...nodeFs, + // Reported as success when refused: the library's bookkeeping should + // forget this lock either way. Leaving it registered would hand the + // `signal-exit` handler a record pointing at the winner's directory. + rmdir: (_p: string, cb: (err: NodeJS.ErrnoException | null) => void) => { + try { + removeIfMine(); + cb(null); + } catch (err) { + cb(err as NodeJS.ErrnoException); + } + }, + /* v8 ignore next 3 -- @preserve: only reachable from proper-lockfile's + signal-exit handler, i.e. at real process exit, which no in-process + test can drive. Its logic is `removeIfMine`, covered via `rmdir`. */ + rmdirSync: () => { + removeIfMine(); + }, + }; +} + +/** + * What to tell an operator about a lock we could not remove. + * + * The blanket "it expires on its own" is **false for `ENOTEMPTY`**, and that + * is the reachable case rather than a hypothetical: stale takeover reclaims a + * lock through the same `rmdir`, which also cannot remove a non-empty + * directory. So a lock directory with anything inside it is not cleaned up by + * the staleness mechanism, by us, or by the next writer — it stays until + * somebody deletes it, and every later save fails `ELOCKED` against it. + * + * Any other failure leaves a directory the refresher has stopped touching, so + * the stale path really does reclaim it after {@link STALE_MS}. + */ +function releaseAdvice(err: unknown, target: string): string { + const code = (err as NodeJS.ErrnoException).code; + return code === "ENOTEMPTY" + ? `It has something inside it, which stale takeover cannot clear either, so saves will keep failing until you remove ${lockPathOf(target)} by hand.` + : `It expires on its own after ${STALE_MS / 1000}s.`; } /** @@ -241,11 +321,15 @@ async function stillOurs( function acquire( target: string, retries: number | typeof RETRY, + fsShim: unknown, ): Promise<() => Promise> { return properLockfile.lock(target, { realpath: false, stale: STALE_MS, retries, + // Every directory removal the library performs — on release and from its + // exit handler — routes through here. See `guardedFs`. + fs: fsShim, // The library's default `onCompromised` *throws* — from a timer, with no // caller on the stack, so it lands as an uncaught exception and takes the // process down. This is also the library's *only* signal for the @@ -289,9 +373,16 @@ export async function withSecretFileLock( // taken, and the catch below already says so with the right message — one // that mentions the lock rather than a `mkdir` the caller never asked for. await fs.mkdir(path.dirname(target), { recursive: true }).catch(() => {}); + // Filled in once we actually hold the lock; see `guardedFs`. + const owned: { id: LockIdentity | null } = { id: null }; + const fsShim = guardedFs(lockPathOf(target), owned, () => + warnOnce( + `The lock on the secrets file at ${target} was taken over by another process while a write was in progress, so it was left alone rather than removed. If a secret you just saved is missing, save it again.`, + ), + ); let release: (() => Promise) | undefined; try { - release = await acquire(target, 0).catch((err: unknown) => { + release = await acquire(target, 0, fsShim).catch((err: unknown) => { // **Retries are for contention, and only for contention.** // `proper-lockfile` drives its whole acquire through `retry`, which // re-attempts on *any* error — so a read-only `$HOME` would spend the @@ -300,7 +391,7 @@ export async function withSecretFileLock( // separates the two answers at the cost of one syscall: `ELOCKED` is // worth waiting out, an infrastructure failure is not. if (!isHeldElsewhere(err)) throw err; - return acquire(target, RETRY); + return acquire(target, RETRY, fsShim); }); } catch (err) { // **`ELOCKED` is not a reason to degrade — it is the opposite.** It means @@ -326,40 +417,22 @@ export async function withSecretFileLock( ); return fn(); } - // Identity of the directory we just created, for the ownership check below. - // **Not the mtime**, which is what the library compares: a lock held longer - // than one refresh tick has its mtime rewritten by `utimes` legitimately, - // so an mtime comparison would call our own healthy lock compromised. The - // inode and birth time both survive `utimes` and both change when a - // directory is removed and recreated, which is precisely the event to - // detect. - const claimedAs = await identify(lockPathOf(target)); + // Now that we hold it, record which directory is ours so the guard above + // can refuse to delete anyone else's. + owned.id = identifySync(lockPathOf(target)); try { return await fn(); } finally { - if (!(await stillOurs(lockPathOf(target), claimedAs))) { - // Someone declared our lock stale and replaced it. **Do not release**: - // `proper-lockfile`'s release is an unconditional `rmdir`, so calling - // it here would delete the lock directory that now belongs to whoever - // took over — ending their exclusion as well as ours, and turning one - // compromised holder into two unprotected writers. Leaving their lock - // alone costs us nothing; ours is already gone. + try { + await release(); + } catch (err) { + // The body already ran and its result is being returned; turning a + // completed save into a failure at teardown would be the wrong trade. + // But a lock left behind is worth explaining, and the two reasons need + // different advice — see `releaseAdvice`. warnOnce( - `The lock on the secrets file at ${target} was taken over by another process while a write was in progress. If a secret you just saved is missing, save it again.`, + `Could not release the lock on the secrets file at ${target} (${describeError(err)}). ${releaseAdvice(err, target)}`, ); - } else { - try { - await release(); - } catch (err) { - // The body already ran and its result is being returned; a release - // that failed means the lock was taken from us (declared stale while - // we held it) or the directory went away. Neither is worth turning a - // successful save into a failure, but a silent catch would leave a - // lock nobody can explain, so say it. - warnOnce( - `Could not release the lock on the secrets file at ${target} (${describeError(err)}). It expires on its own after ${STALE_MS / 1000}s.`, - ); - } } } } diff --git a/specification/v2_servers_file.md b/specification/v2_servers_file.md index a529006b19..50944495e0 100644 --- a/specification/v2_servers_file.md +++ b/specification/v2_servers_file.md @@ -261,7 +261,7 @@ Each server entry may carry these Inspector-extension fields at the top level: - **Secret store selection (#1950)**: #1356 assumed a keychain. On a host without one — the published container (no D-Bus session, #1848), Android/Termux (no prebuilt binary, #1905), a minimal Linux install — `set` hard-failed, so those users could not persist an OAuth client secret or a stdio `env:` value **at all**. #1950 adds the two missing implementations plus the policy that picks one, and the surfaces that say which was picked. - **Selection**: `MCP_INSPECTOR_SECRET_STORE=keyring|file|memory` wins outright. Otherwise the keychain is _probed_ — an `AsyncEntry` construction plus a real read, because the container that motivated #1848 imports the package fine and only fails when it reaches for a Secret Service that isn't there. A read and not a write, deliberately: probing by writing would deposit a value in the user's login keyring at every startup for a store they may never use. If the probe fails the fallback is **`memory`** in a container whose secrets directory is not on a mount, and **`file`** otherwise — so mounting the volume the README already recommends for the catalog flips the same run to durable storage with no configuration, and an unmounted container gets the honest answer rather than a file `docker run --rm` will discard. Resolution is cached per process so the startup banner, `GET /api/config`, and the store doing the writing cannot disagree. - **`FileSecretStore` at rest**: one JSON document at `~/.mcp-inspector/secrets.json` (or `MCP_INSPECTOR_SECRET_FILE`, else under `MCP_STORAGE_DIR`), written `0600` and re-tightened at startup. With `MCP_INSPECTOR_SECRET_KEY` set it is AES-256-GCM with a scrypt-derived key and a per-write random salt; without it the values are in the clear, and _that_ is what the loud banner and the warning-toned modal footer are about. The **whole map is encrypted as a unit**, not value-by-value, so the account names (`serverId:field`) are hidden too — a per-value scheme would leave a readable index of which servers you hold a client secret for. Upgrading is lazy: setting the passphrase later re-encrypts on the next write rather than rewriting the file during a run that may never touch a secret, and the descriptor reports `pendingEncryption` until it does. A file that can no longer be decrypted reads as empty but **refuses to be written**, because replacing a file of still-valid secrets to satisfy an additive request destroys data. - - **Concurrency**: within a process, mutations are serialized per **resolved file path** (not per store instance — two `FileSecretStore`s on one file are ordinary, since the resolved store holds one and the keychain hand-off builds another). Across processes, each mutation holds an exclusive `proper-lockfile` lock on `secrets.json.lock` for the whole read-modify-write (#2082). The in-process queue is what keeps that usable: `proper-lockfile` is not reentrant, so a second `lock()` from the same process fails `ELOCKED` and would be indistinguishable from a genuine remote holder. An earlier iteration hand-rolled the lock — a `mkdir` election with an owner stamp, heartbeat and stale-takeover — and three review rounds each found a real race, the last not closable with what Node exposes (claiming a stale lock needs compare-and-swap on a directory entry, `renameat2(RENAME_EXCHANGE)`). #2082 settled that as "borrow, don't hand-roll" — but the borrowed lock is not claimed to close that race, because it does not. `proper-lockfile@4.1.2` `rmdir`s a stale lock and re-`mkdir`s without checking the directory it removed is the one it found stale, so a slow waiter can still delete a fast waiter's fresh lock; and its own detection is weaker than it looks: `updateLock` compares mtime only on the refresh tick (`stale / 2`, 5s here), while an ordinary mutation finishes in well under a second — so in the common case the tick never runs and nobody is told. Its `release` is also an unconditional `rmdir` with no ownership check, so a holder whose lock was replaced deletes the *winner's* lock on the way out, turning one compromised writer into two unprotected ones. `withSecretFileLock` therefore performs its own ownership check before releasing (inode + birth time, which survive the library's `utimes` refresh but not a delete-and-recreate): on a mismatch it declines to release and warns. That closes the destructive half outright and surfaces the takeover in the fast case the tick misses — but detection remains **best-effort**, since not every filesystem reports those fields, and where they are unavailable it degrades to the library's unaided behaviour rather than to a false alarm. What **is** exclusive is the case that matters: `mkdir` is atomic and a live holder refreshes its mtime, so its lock never goes stale and two running Inspectors are genuinely serialized. The residual window opens only after a holder dies without releasing. A waiter also waits past the stale window before giving up (so a crashed holder resolves by takeover rather than failing everyone else's saves), and a lock still held after that makes `set` **fail** rather than write alongside a visible concurrent writer — `ELOCKED` is evidence the lock is working, not a reason to bypass it. **The optimistic verify stays underneath it**, and is not redundant: read `M0`, apply, write `M1`, read back `M2`, re-apply onto whatever a concurrent writer left if they differ, bounded, with `set` throwing on non-convergence rather than returning as though the value were saved. The comparison is over the **whole map** — checking only your own entry passes in exactly the case that loses data, because yours is present and the other writer's is gone. A lock is advisory between the processes that take it, so the verify is what covers a writer outside this codebase (an editor, a restored backup, an Inspector predating #2082) and what covers the lock being *unavailable*: `withSecretFileLock` runs the body anyway, warning once, on a directory that cannot hold a lock file — this store exists for boxes where the usual mechanism is missing (#1848, #1905) and must not acquire a new way to be unavailable. Reads take no lock; `writeStoreFile` is atomic, so a reader sees the old file or the new one, never a torn one. + - **Concurrency**: within a process, mutations are serialized per **resolved file path** (not per store instance — two `FileSecretStore`s on one file are ordinary, since the resolved store holds one and the keychain hand-off builds another). Across processes, each mutation holds an exclusive `proper-lockfile` lock on `secrets.json.lock` for the whole read-modify-write (#2082). The in-process queue is what keeps that usable: `proper-lockfile` is not reentrant, so a second `lock()` from the same process fails `ELOCKED` and would be indistinguishable from a genuine remote holder. An earlier iteration hand-rolled the lock — a `mkdir` election with an owner stamp, heartbeat and stale-takeover — and three review rounds each found a real race, the last not closable with what Node exposes (claiming a stale lock needs compare-and-swap on a directory entry, `renameat2(RENAME_EXCHANGE)`). #2082 settled that as "borrow, don't hand-roll" — but the borrowed lock is not claimed to close that race, because it does not. `proper-lockfile@4.1.2` `rmdir`s a stale lock and re-`mkdir`s without checking the directory it removed is the one it found stale, so a slow waiter can still delete a fast waiter's fresh lock; and its own detection is weaker than it looks: `updateLock` compares mtime only on the refresh tick (`stale / 2`, 5s here), while an ordinary mutation finishes in well under a second — so in the common case the tick never runs and nobody is told. Its `release` is also an unconditional `rmdir` with no ownership check, so a holder whose lock was replaced deletes the *winner's* lock on the way out, turning one compromised writer into two unprotected ones. `withSecretFileLock` therefore supplies a guarded `options.fs` whose directory removal refuses to delete a lock that is no longer the one it created (inode + birth time, which survive the library's `utimes` refresh but not a delete-and-recreate). It sits in `options.fs` rather than around `release()` because that is the one seam **both** removal paths route through — the release path and the library's `signal-exit` handler, which `rmdirSync`s every registered lock with no ownership check of its own; a guard around release alone leaves an exit at the wrong moment free to delete the winner's directory. This **narrows** the destructive window (the check is a `statSync` immediately followed by an `rmdirSync`, so nothing in-process interleaves) and surfaces the takeover in the fast case the tick misses — but it does not close it: it is still check-then-act across processes, which needs the same CAS Node does not expose. Best-effort throughout, and where a filesystem reports neither field it degrades to the library's unaided behaviour rather than to a false alarm. What **is** exclusive is the case that matters: `mkdir` is atomic and a live holder refreshes its mtime, so its lock never goes stale and two running Inspectors are genuinely serialized. The residual window opens only after a holder dies without releasing. A waiter also waits past the stale window before giving up (so a crashed holder resolves by takeover rather than failing everyone else's saves), and a lock still held after that makes `set` **fail** rather than write alongside a visible concurrent writer — `ELOCKED` is evidence the lock is working, not a reason to bypass it. **The optimistic verify stays underneath it**, and is not redundant: read `M0`, apply, write `M1`, read back `M2`, re-apply onto whatever a concurrent writer left if they differ, bounded, with `set` throwing on non-convergence rather than returning as though the value were saved. The comparison is over the **whole map** — checking only your own entry passes in exactly the case that loses data, because yours is present and the other writer's is gone. A lock is advisory between the processes that take it, so the verify is what covers a writer outside this codebase (an editor, a restored backup, an Inspector predating #2082) and what covers the lock being *unavailable*: `withSecretFileLock` runs the body anyway, warning once, on a directory that cannot hold a lock file — this store exists for boxes where the usual mechanism is missing (#1848, #1905) and must not acquire a new way to be unavailable. Reads take no lock; `writeStoreFile` is atomic, so a reader sees the old file or the new one, never a torn one. - **Strict reads (`getStrict`)**: `get` is tolerant by contract, and that tolerance is wrong for exactly one caller — a migration that treats `null` as proof of absence and then _writes_. A transient read failure would look like "nothing there", and the write would replace a newer stored value with an older on-disk copy, inverting the keychain-wins rule the migration is built on. `getStrict` throws instead, and both plaintext migrations plus the keychain hand-off use it. Two traps this hit on the way in, both worth remembering: the seam must be forwarded by `DeferredSecretStore` (the store production actually uses, so without forwarding the strictness existed only in tests that injected a concrete store), and it must be implemented by _every_ store rather than falling back to `get` for the one it was introduced for. - **Bulk reads (`getMany`)**: rehydration asked field by field, and `FileSecretStore.get` reads and decrypts the entire file per call. scrypt at `N=16384` measures ~23ms, so an encrypted catalog spent ~450ms of pure key derivation on every `GET /api/servers` — a visible stall rather than a micro-optimization. The seam takes a **list of `{ serverId, fields }` across servers**, and both rehydration callers pass the whole catalog in one request, so `FileSecretStore` decrypts once per rehydration; stores for which per-field reads are already cheap (the keychain) fall back to parallel `get`. The cross-server shape is the point: a per-server version shipped first and left a 20-server catalog paying 20 serialized derivations, because both callers iterate servers — the same stall reached one server at a time instead of one field at a time. - **Durability gate on migration**: the plaintext-stripping migrations only delete the disk copy once the value is somewhere that outlives the process (`isDurable`). Against a session-scoped store, stripping would trade a secret that survives restarts for one that dies with the process — and it runs on an ordinary `GET`, so merely opening the app would destroy it. From 09e1564672c35483cc50afdaf91745d8ea145d2a Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 11:31:37 -0400 Subject: [PATCH 07/12] fix(auth): refuse on a stuck lock, capture identity at the mkdir seam (#2082) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review round 6 — five findings (two inline, three suppressed), all valid. **A stale lock that cannot be cleared caused unlocked writes.** `acquireLock` does not only *create* directories: on finding a stale one it removes it and retries, and that removal can fail — `ENOTEMPTY` for a lock with anything inside it, `EACCES`/`EROFS` for one we may not touch. Those surface as ordinary non-`ELOCKED` errors, which the probe read as "locks do not work here" and degraded on. So every Inspector on the box quietly bypassed the *same* stuck lock and raced its writes — while the release-failure message was telling the operator saves would keep failing until they cleared it. `isStuckOrHeld` now discriminates on **whether the lock directory exists** rather than on an errno taxonomy: if it is there, something holds it and we refuse; if it is not, we genuinely could not create one and degrading is the documented trade (#1848, #1905). Reading the state the decision is about avoids enumerating error codes per platform — and an enumeration is exactly what let `ENOTEMPTY` through. **Identity was captured after the acquire promise settled.** That span includes the library's own `utimes`/`stat` probe, so a waiter replacing our directory inside it would have us record *the winner's* identity as our own, after which the removal guard would accept and delete their lock. It is now recorded in the `mkdir` callback — the moment the directory becomes ours, and synchronous with respect to this process, so there is no window. **The expiry advice was still wrong for `EACCES`/`EPERM`/`EROFS`.** Since the guard `stat`s before removing, everything reaching the release catch is an `rmdir` that was refused — and stale takeover reclaims through that same `rmdir`. The `ENOENT` arm was unreachable for the same reason, so the branch was dead code: removed, and the advice is now unconditional. **`proper-lockfile` was missing from `NODE_ONLY_OPTIMIZE_DEPS_EXCLUDE`.** A fourth externalization surface — the tsup lists configure the production bundles, not `vite dev`, whose dep scanner would otherwise walk its CJS/`graceful-fs`/`signal-exit` graph. Added, and `vite-base-config.test.ts` now asserts it along with `chokidar` and `@napi-rs/keyring`, which were also absent from that assertion. **A test was named for something it did not exercise.** "does not treat an unlistable directory as an empty one" was driving `ENOTDIR`, which is deliberately treated *as* the fresh-install case, so the actual new behaviour was untested. Split in two: one for `ENOTDIR`, and a real `EACCES` one using a `--x` directory — `readdir` denied, a known name still reachable, which is the asymmetry that makes "unlistable" different from "empty". Driven with a real mode rather than a stub, since `vi.spyOn` cannot redefine an ESM namespace export; skipped as root and on Windows, where the mode would not bite and the test would assert nothing. Both new tests verified by mutation. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H Signed-off-by: cliffhall --- clients/web/server/vite-base-config.ts | 8 ++ .../integration/auth/node/file-lock.test.ts | 31 +++++++ .../auth/node/secret-store-selection.test.ts | 48 +++++++++-- .../server/vite-base-config.test.ts | 6 ++ core/auth/node/file-lock.ts | 83 ++++++++++++------- 5 files changed, 140 insertions(+), 36 deletions(-) diff --git a/clients/web/server/vite-base-config.ts b/clients/web/server/vite-base-config.ts index c351f4732d..3d9c1a9029 100644 --- a/clients/web/server/vite-base-config.ts +++ b/clients/web/server/vite-base-config.ts @@ -34,6 +34,14 @@ const NODE_ONLY_OPTIMIZE_DEPS_EXCLUDE = [ // excluding it keeps Vite's dep scanner from chasing into the // platform-specific binaries during dev startup. "@napi-rs/keyring", + // `proper-lockfile` is reached only through `core/auth/node/file-lock.ts` + // — the secrets file's cross-process lock (#2082) — which the Hono + // `/api/servers` handlers pull in via `core/auth/node/file-secret-store.ts`. + // Same node-only import chain as `atomically` above, and the same reason: + // it is CJS with a `graceful-fs`/`signal-exit`/`retry` graph that Vite's + // dev scanner has no business walking. Note the tsup `external` lists do + // **not** cover this — they configure the production bundles, not `vite dev`. + "proper-lockfile", ] as const; export function getViteBaseConfig() { diff --git a/clients/web/src/test/integration/auth/node/file-lock.test.ts b/clients/web/src/test/integration/auth/node/file-lock.test.ts index c42dbd3db9..0f10bc5b1c 100644 --- a/clients/web/src/test/integration/auth/node/file-lock.test.ts +++ b/clients/web/src/test/integration/auth/node/file-lock.test.ts @@ -305,6 +305,37 @@ describe("withSecretFileLock degrades rather than failing", () => { expect(existsSync(target)).toBe(false); }, 90_000); + it("refuses rather than degrading when a stale lock cannot be cleared", async () => { + // `acquireLock` does not only *create* directories — on finding a stale + // one it removes it and retries, and that removal can fail. A stale lock + // with anything inside it fails `ENOTEMPTY`, which is not `ELOCKED`, and + // treating every non-`ELOCKED` error as "locks do not work here" meant + // every Inspector on the box quietly bypassed the *same* stuck lock and + // raced its writes — while the release-failure message was telling the + // operator saves would keep failing until they cleared it. + const target = filePath(); + const lockPath = `${target}.lock`; + await fs.mkdir(path.dirname(target), { recursive: true }); + await fs.mkdir(lockPath); + await fs.writeFile(`${lockPath}/stray`, "", "utf-8"); + // Backdated so it reads as stale — which is what sends `acquireLock` down + // the remove-and-retry path rather than straight to `ELOCKED`. + const longDead = new Date(Date.now() - 60_000); + await fs.utimes(lockPath, longDead, longDead); + + const store = new FileSecretStore({ filePath: target }); + const err = await store + .set("srv", "env:MINE", "1") + .catch((e: unknown) => e); + + expect(err).toBeInstanceOf(SecretStoreUnavailableError); + expect((err as Error).message).toMatch(/was not saved/); + // Nothing written behind the stuck lock, and no "unprotected" warning: + // this is a refusal, not a degrade. + expect(existsSync(target)).toBe(false); + expect(warnings()).not.toContain("not protected"); + }, 90_000); + it("takes over the lock of a holder that died, rather than failing the save", async () => { // The invariant behind refusing on `ELOCKED`: refusing is only defensible // because a *crashed* holder resolves on its own first. `RETRY` therefore 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 e9cb8f7efc..21a6ea3d5a 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 @@ -735,13 +735,11 @@ describe("absorbFileSecretsIntoKeyring", () => { expect(existsSync(`${filePath}.lock`)).toBe(false); }); - it("does not treat an unlistable directory as an empty one (#2082)", async () => { - // Only "there is no directory" proves the fresh-install case. A path - // whose parent is a *file* answers `readdir` with ENOTDIR — the other - // shape of "nothing there" — and must be treated the same, whereas an - // EACCES directory that denies listing while still permitting access to - // the known `secrets.json` must not be, or the keychain is selected and - // those secrets go invisible with nothing said. + it("treats ENOTDIR as the fresh-install case, like ENOENT (#2082)", async () => { + // A path whose parent is a *file* answers `readdir` with ENOTDIR — the + // other shape of "there is no directory here" — so it takes the same + // silent path as a missing one. The *unlistable* case is different and is + // covered by the test below. await fs.writeFile(path.join(tmpDir, "not-a-dir"), "", "utf-8"); const filePath = path.join(tmpDir, "not-a-dir", "secrets.json"); process.env.MCP_INSPECTOR_SECRET_FILE = filePath; @@ -754,6 +752,42 @@ describe("absorbFileSecretsIntoKeyring", () => { expect(warn).not.toHaveBeenCalled(); }); + // Root bypasses POSIX permission checks, and Windows does not model them + // the same way — in either case the directory below stays listable and the + // test would assert nothing. + const canDenyListing = + process.platform !== "win32" && process.getuid?.() !== 0; + + it.skipIf(!canDenyListing)( + "does not treat an unlistable directory as an empty one (#2082)", + async () => { + // The case ENOTDIR does *not* cover, and the reason the catch narrowed + // to ENOENT/ENOTDIR rather than swallowing everything. A directory with + // `--x` permission denies `readdir` with EACCES while still permitting + // access to a *known* name inside it — so reading EACCES as "nothing to + // migrate" selects the keychain and leaves those secrets invisible with + // nothing said. That asymmetry is real POSIX behaviour, which is why + // this drives it with a real mode rather than a stubbed `readdir`. + const filePath = await seedFile({ "srv:oauthClientSecret": "from-file" }); + process.env.MCP_INSPECTOR_SECRET_FILE = filePath; + vi.spyOn(console, "warn").mockImplementation(() => {}); + // Write + execute, no read: `readdir` fails, `stat`/`open`/`rename` of a + // known name still work — which is exactly what the migration needs. + await fs.chmod(tmpDir, 0o300); + try { + const mod = await loadWithProbe(true); + const keyring = new InMemorySecretStore(); + await mod.absorbFileSecretsIntoKeyring(keyring); + + // It went on and migrated, rather than silently skipping. + expect(await keyring.get("srv", "oauthClientSecret")).toBe("from-file"); + } finally { + // Restore before `afterEach`, which needs to list it to remove it. + await fs.chmod(tmpDir, 0o700); + } + }, + ); + it("still adopts an orphan when the live file is absent (#2082)", async () => { // The fast path above must not be a `stat` of `secrets.json`: an // interrupted migration leaves *only* the snapshot, which is precisely diff --git a/clients/web/src/test/integration/server/vite-base-config.test.ts b/clients/web/src/test/integration/server/vite-base-config.test.ts index fc30522337..90c39abd93 100644 --- a/clients/web/src/test/integration/server/vite-base-config.test.ts +++ b/clients/web/src/test/integration/server/vite-base-config.test.ts @@ -21,8 +21,14 @@ describe("getViteBaseConfig", () => { expect.arrayContaining([ "@modelcontextprotocol/client/stdio", "atomically", + "chokidar", "cross-spawn", "which", + "@napi-rs/keyring", + // #2082 — reached through `core/auth/node/file-lock.ts`. The tsup + // `external` lists cover the production bundles, not `vite dev`, so + // a node-only dependency has to be named in both places. + "proper-lockfile", ]), ); }); diff --git a/core/auth/node/file-lock.ts b/core/auth/node/file-lock.ts index 11db6a3888..f9f8f5f5ed 100644 --- a/core/auth/node/file-lock.ts +++ b/core/auth/node/file-lock.ts @@ -162,6 +162,32 @@ export function resetFileLockWarnings(): void { warned.clear(); } +/** + * Is this failure "the lock is there and we could not have it", as opposed to + * "locks do not work here"? + * + * `ELOCKED` is the obvious member, but not the only one, and the difference + * decides whether a save **refuses** or **degrades to an unlocked write** — + * so getting it wrong is not cosmetic. `acquireLock` does not only *create* + * directories: on finding a stale one it removes it and retries, and that + * removal can fail — `ENOTEMPTY` for a lock with anything inside it, `EACCES` + * or `EROFS` for one we may not touch. Those surface as ordinary non-`ELOCKED` + * errors, and treating them as infrastructure failures meant every Inspector + * on the box quietly bypassed the *same* stuck lock and raced its writes — + * while the release-failure message was telling the operator saves would keep + * failing until they cleared it. + * + * The discriminator is the lock directory itself rather than an errno + * taxonomy: **if it exists, something holds it and we must not proceed**; if + * it does not, we genuinely could not create one and degrading is the + * documented trade (#1848, #1905). That reads the state the decision is + * actually about, instead of enumerating error codes per platform and + * filesystem — which is the enumeration that let `ENOTEMPTY` through. + */ +function isStuckOrHeld(err: unknown, lockPath: string): boolean { + return isHeldElsewhere(err) || identifySync(lockPath) !== null; +} + /** * Did `proper-lockfile` decline because someone else holds the lock? * @@ -266,6 +292,23 @@ function guardedFs( }; return { ...nodeFs, + // **Identity is captured here, not after `lock()` resolves.** `mkdir` is + // the moment the directory becomes ours, and it is synchronous with + // respect to this process: recording it in the callback, before yielding, + // leaves no window. Capturing after the acquire promise settled — as this + // did originally — spans the library's own `utimes`/`stat` probe, and a + // waiter replacing our directory inside that span would have us record + // *the winner's* identity as our own, after which the guard below would + // cheerfully delete their lock. + mkdir: ( + p: string, + cb: (err: NodeJS.ErrnoException | null) => void, + ): void => { + nodeFs.mkdir(p, (err) => { + if (!err) owned.id = identifySync(lockPath); + cb(err); + }); + }, // Reported as success when refused: the library's bookkeeping should // forget this lock either way. Leaving it registered would hand the // `signal-exit` handler a record pointing at the winner's directory. @@ -286,26 +329,6 @@ function guardedFs( }; } -/** - * What to tell an operator about a lock we could not remove. - * - * The blanket "it expires on its own" is **false for `ENOTEMPTY`**, and that - * is the reachable case rather than a hypothetical: stale takeover reclaims a - * lock through the same `rmdir`, which also cannot remove a non-empty - * directory. So a lock directory with anything inside it is not cleaned up by - * the staleness mechanism, by us, or by the next writer — it stays until - * somebody deletes it, and every later save fails `ELOCKED` against it. - * - * Any other failure leaves a directory the refresher has stopped touching, so - * the stale path really does reclaim it after {@link STALE_MS}. - */ -function releaseAdvice(err: unknown, target: string): string { - const code = (err as NodeJS.ErrnoException).code; - return code === "ENOTEMPTY" - ? `It has something inside it, which stale takeover cannot clear either, so saves will keep failing until you remove ${lockPathOf(target)} by hand.` - : `It expires on its own after ${STALE_MS / 1000}s.`; -} - /** * One `proper-lockfile` acquire against `target`, with the given retry policy. * @@ -390,7 +413,7 @@ export async function withSecretFileLock( // time, on every save, before degrading. Probing once with no retries // separates the two answers at the cost of one syscall: `ELOCKED` is // worth waiting out, an infrastructure failure is not. - if (!isHeldElsewhere(err)) throw err; + if (!isStuckOrHeld(err, lockPathOf(target))) throw err; return acquire(target, RETRY, fsShim); }); } catch (err) { @@ -402,9 +425,9 @@ export async function withSecretFileLock( // {@link RETRY}), a holder still there is not one that crashed; it is one // that is stuck. Refusing loses nothing — `set` reports it and the user // retries — whereas proceeding can lose a secret while reporting success. - if (isHeldElsewhere(err)) { + if (isStuckOrHeld(err, lockPathOf(target))) { throw new SecretStoreUnavailableError( - `Could not save to the secrets file at ${target}: another process has held the lock on it for the ${Math.round(RETRY_BUDGET_MS / 1000)} seconds this save waited. Its secrets are intact; the value you just entered was not saved. If no other Inspector is running, remove ${target}.lock and try again.`, + `Could not save to the secrets file at ${target}: its lock (${lockPathOf(target)}) was still held after the ${Math.round(RETRY_BUDGET_MS / 1000)} seconds this save waited. Its secrets are intact; the value you just entered was not saved. If no other Inspector is running, remove that lock and try again.`, ); } // Everything else is the lock being *unavailable* rather than held — a @@ -417,9 +440,6 @@ export async function withSecretFileLock( ); return fn(); } - // Now that we hold it, record which directory is ours so the guard above - // can refuse to delete anyone else's. - owned.id = identifySync(lockPathOf(target)); try { return await fn(); } finally { @@ -428,10 +448,15 @@ export async function withSecretFileLock( } catch (err) { // The body already ran and its result is being returned; turning a // completed save into a failure at teardown would be the wrong trade. - // But a lock left behind is worth explaining, and the two reasons need - // different advice — see `releaseAdvice`. + // But a lock left behind is worth explaining. warnOnce( - `Could not release the lock on the secrets file at ${target} (${describeError(err)}). ${releaseAdvice(err, target)}`, + // No branch on the error code, deliberately. Our own guard `stat`s + // before removing, so anything reaching here is an `rmdir` that was + // refused — and **stale takeover reclaims through that same `rmdir`**, + // so whatever blocked ours blocks that too. `ENOTEMPTY` is the + // reachable one; `EACCES`, `EPERM` and `EROFS` behave identically. + // Promising expiry for "everything else" was wrong for all of them. + `Could not release the lock on the secrets file at ${target} (${describeError(err)}). Stale takeover reclaims a lock through the same directory removal, so whatever blocked this blocks that too: saves will keep failing until you remove ${lockPathOf(target)} by hand.`, ); } } From e956d4983306b4c6a6e61143dcc259cb32d626fc Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 11:54:47 -0400 Subject: [PATCH 08/12] fix(auth): lock the snapshot during hand-off, keep quiet on ERELEASED (#2082) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review round 7 — two findings, both upheld. **An in-progress migration snapshot could be adopted by another startup.** The fast path counts every `*.migrating-*` as work and `recoverOrphanedSnapshots` adopts them — including one whose owner is still reading it. It link/unlinks the snapshot back to the live path and re-claims it under a new name, the owner's hand-off then fails `ENOENT`, and if the adopting process exits before copying, that healthy session starts with none of those secrets, recoverable only on some later run. Fixed the second way the reviewer suggested, not the first. Holding the main lock across the hand-off also closes it and was tried — it broke "claims the file atomically, so a later write is not deleted", because it blocks every ordinary writer for the whole migration and fails their save past the retry budget. That is a worse regression than the race, since #1950 guarantees a write completing after the claim survives. The existing test caught it. So the hand-off takes a lock on the **snapshot** instead, and `recoverOrphanedSnapshots` skips an orphan whose lock is held. Liveness comes from the lock rather than from the pid already in the filename: a pid both outlives its process and recurs — pid 1 on every container start, which #1950 documents as the reason the name carries a nonce — whereas a lock expires by itself if its owner dies, so a genuinely abandoned snapshot becomes adoptable with nothing to clean up. `isFileLockHeld` answers `false` when it cannot tell, since the alternative to a wrong `false` is never recovering an abandoned file. **A compromised lock arrives as `ERELEASED`, and we told the operator to delete the lock.** `setLockAsCompromised` marks the lock released and drops its registry entry *before* calling `onCompromised`, so `release()` answers `ERELEASED` without touching the filesystem — and the directory at that path is by then the **winner's live lock**. "Remove it by hand" would destroy the exclusion of a process that did nothing wrong. Handled separately and silently; `onCompromised` has already reported what happened. A conditional rather than an early return, since a `return` inside `finally` discards whatever the body was returning or throwing. Both new tests verified by mutation. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H Signed-off-by: cliffhall --- .../integration/auth/node/file-lock.test.ts | 8 ++ .../auth/node/secret-store-selection.test.ts | 40 ++++++++++ core/auth/node/file-lock.ts | 56 +++++++++++--- core/auth/node/secret-store-selection.ts | 74 +++++++++++++++---- 4 files changed, 153 insertions(+), 25 deletions(-) diff --git a/clients/web/src/test/integration/auth/node/file-lock.test.ts b/clients/web/src/test/integration/auth/node/file-lock.test.ts index 0f10bc5b1c..6d04532897 100644 --- a/clients/web/src/test/integration/auth/node/file-lock.test.ts +++ b/clients/web/src/test/integration/auth/node/file-lock.test.ts @@ -493,5 +493,13 @@ describe("withSecretFileLock reports what it cannot clean up", () => { { timeout: 20_000, interval: 250 }, ); }); + + // And it must NOT go on to tell the operator to delete the lock. Once the + // tick has fired, `release()` answers `ERELEASED` without touching the + // filesystem — the directory sitting at that path is then whoever took + // over, so "remove it by hand" would destroy the exclusion of a process + // that did nothing wrong. `onCompromised` has already said what happened. + expect(warnings()).not.toContain("by hand"); + expect(warnings()).not.toContain("Could not release the lock"); }, 30_000); }); 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 21a6ea3d5a..f3d3fa700a 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 @@ -788,6 +788,46 @@ describe("absorbFileSecretsIntoKeyring", () => { }, ); + it("does not adopt a snapshot another migration is still using (#2082)", async () => { + // A `*.migrating-*` file is not automatically an orphan: the process that + // staged it may still be reading it. Adopting one mid-hand-off links it + // back to the live path and re-claims it under a new name, so its owner's + // hand-off fails ENOENT — and if we exit before copying, that healthy + // session starts with none of those secrets. + // + // Liveness is the snapshot's own lock rather than the pid in its name: a + // pid outlives its process and recurs (pid 1 on every container start), + // whereas the lock expires by itself if the owner dies. + const inProgress = path.join(tmpDir, "secrets.json.migrating-999-abc"); + await fs.writeFile( + inProgress, + JSON.stringify({ + version: 1, + encryption: "none", + secrets: { "srv:oauthClientSecret": "still-migrating" }, + }), + "utf-8", + ); + process.env.MCP_INSPECTOR_SECRET_FILE = path.join(tmpDir, "secrets.json"); + vi.spyOn(console, "warn").mockImplementation(() => {}); + + const properLockfile = (await import("proper-lockfile")).default; + const release = await properLockfile.lock(inProgress, { + realpath: false, + stale: 10_000, + }); + + const mod = await loadWithProbe(true); + const keyring = new InMemorySecretStore(); + await mod.absorbFileSecretsIntoKeyring(keyring); + await release(); + + // Left exactly where its owner put it, and not migrated from under it. + expect(existsSync(inProgress)).toBe(true); + expect(existsSync(path.join(tmpDir, "secrets.json"))).toBe(false); + expect(await keyring.get("srv", "oauthClientSecret")).toBe(null); + }); + it("still adopts an orphan when the live file is absent (#2082)", async () => { // The fast path above must not be a `stat` of `secrets.json`: an // interrupted migration leaves *only* the snapshot, which is precisely diff --git a/core/auth/node/file-lock.ts b/core/auth/node/file-lock.ts index f9f8f5f5ed..eceed45edc 100644 --- a/core/auth/node/file-lock.ts +++ b/core/auth/node/file-lock.ts @@ -367,6 +367,30 @@ function acquire( }); } +/** + * Is someone holding the lock on `filePath` right now? + * + * Liveness, not ownership: `check` reports a *stale* lock as unheld, so a + * holder that died stops counting on its own after {@link STALE_MS} with no + * bookkeeping to clean up. That is what makes this usable as an + * "is this in progress" test — a pid stamp cannot say it, since a pid both + * outlives its process and recurs (pid 1 on every container start). + * + * Answers `false` when it cannot tell. The callers use this to decide whether + * to *leave something alone*, and the alternative to a wrong `false` is + * refusing to ever recover an abandoned file. + */ +export async function isFileLockHeld(filePath: string): Promise { + try { + return await properLockfile.check(path.resolve(filePath), { + realpath: false, + stale: STALE_MS, + }); + } catch { + return false; + } +} + /** * Run `fn` holding an exclusive cross-process lock on `filePath`. * @@ -449,15 +473,29 @@ export async function withSecretFileLock( // The body already ran and its result is being returned; turning a // completed save into a failure at teardown would be the wrong trade. // But a lock left behind is worth explaining. - warnOnce( - // No branch on the error code, deliberately. Our own guard `stat`s - // before removing, so anything reaching here is an `rmdir` that was - // refused — and **stale takeover reclaims through that same `rmdir`**, - // so whatever blocked ours blocks that too. `ENOTEMPTY` is the - // reachable one; `EACCES`, `EPERM` and `EROFS` behave identically. - // Promising expiry for "everything else" was wrong for all of them. - `Could not release the lock on the secrets file at ${target} (${describeError(err)}). Stale takeover reclaims a lock through the same directory removal, so whatever blocked this blocks that too: saves will keep failing until you remove ${lockPathOf(target)} by hand.`, - ); + // + // **`ERELEASED` is not a lock left behind.** When the library's refresh + // tick detects a takeover it marks the lock released and drops its + // registry entry *before* calling `onCompromised`, so our `release()` + // returns `ERELEASED` without touching the filesystem. The directory + // sitting there is then the **winner's live lock** — and the message + // below would tell the operator to delete it, destroying the exclusion + // of a process that did nothing wrong. `onCompromised` has already said + // what happened, so there is nothing to add. + // + // A conditional rather than an early `return`: a `return` inside + // `finally` discards whatever the body was returning or throwing. + if ((err as NodeJS.ErrnoException).code !== "ERELEASED") { + warnOnce( + // No branch on the *removal* error code, deliberately. Our own + // guard `stat`s before removing, so anything else reaching here is + // an `rmdir` that was refused — and **stale takeover reclaims + // through that same `rmdir`**, so whatever blocked ours blocks that + // too. `ENOTEMPTY` is the reachable one; `EACCES`, `EPERM` and + // `EROFS` behave identically. Promising expiry was wrong for all. + `Could not release the lock on the secrets file at ${target} (${describeError(err)}). Stale takeover reclaims a lock through the same directory removal, so whatever blocked this blocks that too: saves will keep failing until you remove ${lockPathOf(target)} by hand.`, + ); + } } } } diff --git a/core/auth/node/secret-store-selection.ts b/core/auth/node/secret-store-selection.ts index e35a5facfc..8b2add6131 100644 --- a/core/auth/node/secret-store-selection.ts +++ b/core/auth/node/secret-store-selection.ts @@ -54,7 +54,7 @@ import { readSecretFilePermissions, tightenSecretFilePermissions, } from "./file-secret-store.js"; -import { withSecretFileLock } from "./file-lock.js"; +import { isFileLockHeld, withSecretFileLock } from "./file-lock.js"; import { KeyringSecretStore, parseAccount, @@ -546,21 +546,18 @@ export async function absorbFileSecretsIntoKeyring( // after the claim below. Everything that *acts* re-checks under the lock. if (!(await anythingToMigrate(filePath))) return; - // Orphan adoption and the claim below both move the live path around, so - // they run under the same cross-process lock a `set` takes (#2082) — - // otherwise a concurrent writer's atomic rename can land between the two - // and be adopted, claimed, or clobbered depending on the interleaving. - // The lock is released before the hand-off: that part reads a snapshot - // nobody else can reach, and holding it across a keychain round-trip per - // secret would block every writer for the duration of a migration. + // Orphan adoption and the claim below run under the same cross-process lock + // a `set` takes (#2082) — otherwise a concurrent writer's atomic rename can + // land between the two and be adopted, claimed, or clobbered depending on + // the interleaving. + // + // The hand-off itself deliberately runs **outside** it, under a lock on the + // *snapshot* instead (see `handOffStagedSecrets`). Holding the main lock + // across a keychain round-trip per secret blocks every ordinary writer for + // the duration, and past the retry budget fails their save outright — which + // is a worse regression than the race it would close, since #1950 + // guarantees a write completing after the claim survives. // - // Wrapped because `withSecretFileLock` **throws** when another process - // holds the lock past its retry budget, and this function's contract is - // that it never throws: it is awaited directly by both `resolveSecretStore` - // branches, so a stuck writer would fail store resolution and with it the - // whole session. Refusing is the right answer for a `set` — the user is - // waiting on that value — and the wrong one here, where the file simply - // stays put and the next run migrates it. const claimed = await withSecretFileLock(filePath, async () => { // A crash between the claim and the delete leaves only // `secrets.json.migrating-`. Checking the canonical path alone then @@ -619,8 +616,45 @@ export async function absorbFileSecretsIntoKeyring( return null; }); if (claimed === null) return; - const staged = claimed; + await handOffStagedSecrets(claimed, filePath, keyring); +} +/** + * Move a claimed snapshot's secrets into the keychain, then dispose of it. + * + * Runs under a lock on the **snapshot**, not on `secrets.json`. That is what + * stops a second Inspector's {@link recoverOrphanedSnapshots} from adopting a + * migration still in progress — it link/unlinks the snapshot back to the live + * path and re-claims it, our hand-off then fails `ENOENT`, and if that second + * process exits before copying, the healthy first session starts with none of + * those secrets. Locking the snapshot says "in progress" in a way a filename + * cannot, and expires by itself if this process dies. + * + * Holding the *main* lock here instead would also close it, and was tried: + * it blocks every ordinary writer for the whole migration and fails their + * save past the retry budget, breaking #1950's guarantee that a write + * completing after the claim survives. + */ +async function handOffStagedSecrets( + staged: string, + filePath: string, + keyring: SecretStore, +): Promise { + await withSecretFileLock(staged, () => + handOffStagedSecretsLocked(staged, filePath, keyring), + ).catch((err: Error) => { + console.warn( + `\n[mcp-inspector] Could not lock ${staged} to migrate it into the OS keychain (${err.message}), so it has been left in place. The next run will try again.`, + ); + }); +} + +/** {@link handOffStagedSecrets}'s body, with the snapshot's lock held. */ +async function handOffStagedSecretsLocked( + staged: string, + filePath: string, + keyring: SecretStore, +): Promise { 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. @@ -717,6 +751,14 @@ async function recoverOrphanedSnapshots(filePath: string): Promise { } for (const name of names) { const orphan = path.join(dir, name); + // Not an orphan at all — another Inspector is migrating it right now, and + // adopting it would link it back to the live path and re-claim it under a + // new name while its owner is still reading it. That owner's hand-off + // then fails `ENOENT`, and if we exit before copying, its healthy session + // starts with none of those secrets. The lock expires on its own if that + // process dies, so a genuinely abandoned snapshot becomes adoptable + // without anything to clean up. + if (await isFileLockHeld(orphan)) continue; try { await fs.link(orphan, filePath); await fs.rm(orphan, { force: true }); From edddc6c9cca4249de467005fa76d35b4a7d95372 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 12:26:52 -0400 Subject: [PATCH 09/12] fix(auth): hold the snapshot lock across the main lock's release (#2082) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review round 8 — two findings, both upheld. The first is a gap in round 7's own fix. **The snapshot was unprotected between two awaits.** Returning from `withSecretFileLock(filePath, ...)` releases the main lock *before* `handOffStagedSecrets` acquired the snapshot's, so a second startup could take the main lock in that window, see the staged file unlocked, and adopt and re-claim it — leaving this process reading a path that no longer exists. Narrower than the race round 7 closed, but the same one. `openSecretFileLock` is now split out of `withSecretFileLock`: it takes the lock and returns its release, so a caller can hold one *across* another's release. The claim acquires the snapshot's lock before returning — inside the main lock — and the hand-off then runs outside the main lock, still holding it. `withSecretFileLock` is a thin wrapper over the same function, so which failures refuse and which degrade is decided in one place and the two entry points cannot drift. Locking the snapshot degrades rather than refusing (`null` → hand off anyway): the exposure is one uniquely-named snapshot, which is a better trade than refusing to migrate at all on a box that cannot lock. **Both migration scans matched the lock directories themselves.** `secrets.json.migrating--.lock` passes the plain prefix test, and treating it as a snapshot is self-sustaining damage: the liveness probe asks about a nonexistent `.lock.lock` and so answers "not held", recovery tries to hard-link a *directory* onto the secrets path, fails, and prints the orphan warning. Because a liveness *check* never clears a stale lock directory — only a would-be acquirer does — that false migration repeats on every startup forever, including long after the real snapshot was recovered. `isSnapshotName` now excludes `.lock` for both `anythingToMigrate` and `recoverOrphanedSnapshots`. New test verified by mutation. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H Signed-off-by: cliffhall --- .../auth/node/secret-store-selection.test.ts | 22 ++++++ core/auth/node/file-lock.ts | 75 ++++++++++++------- core/auth/node/secret-store-selection.ts | 64 ++++++++++------ 3 files changed, 112 insertions(+), 49 deletions(-) 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 f3d3fa700a..9cc2b065d1 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 @@ -828,6 +828,28 @@ describe("absorbFileSecretsIntoKeyring", () => { expect(await keyring.get("srv", "oauthClientSecret")).toBe(null); }); + it("ignores a snapshot's own lock directory (#2082)", async () => { + // `secrets.json.migrating--.lock` matches the plain prefix + // test, and treating it as a snapshot is self-sustaining damage: the + // liveness probe asks about a nonexistent `.lock.lock` and answers + // "not held", recovery tries to hard-link a *directory* onto the secrets + // path, fails, and prints the orphan warning — every startup, forever, + // since a liveness *check* never clears a stale lock directory. + const strayLock = path.join(tmpDir, "secrets.json.migrating-999-abc.lock"); + await fs.mkdir(strayLock); + process.env.MCP_INSPECTOR_SECRET_FILE = path.join(tmpDir, "secrets.json"); + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const mod = await loadWithProbe(true); + + await mod.absorbFileSecretsIntoKeyring(new InMemorySecretStore()); + + // Not mistaken for a snapshot: nothing said, nothing linked, and the + // directory left exactly where it was. + expect(warn).not.toHaveBeenCalled(); + expect(existsSync(strayLock)).toBe(true); + expect(existsSync(path.join(tmpDir, "secrets.json"))).toBe(false); + }); + it("still adopts an orphan when the live file is absent (#2082)", async () => { // The fast path above must not be a `stat` of `secrets.json`: an // interrupted migration leaves *only* the snapshot, which is precisely diff --git a/core/auth/node/file-lock.ts b/core/auth/node/file-lock.ts index eceed45edc..737720312e 100644 --- a/core/auth/node/file-lock.ts +++ b/core/auth/node/file-lock.ts @@ -402,10 +402,22 @@ export async function isFileLockHeld(filePath: string): Promise { * Returns whatever `fn` returns. `fn` runs exactly once either way — the * lock's absence changes the guarantee, never whether the work happens. */ -export async function withSecretFileLock( +/** + * Take the lock and hand back its release, or `null` when locking is + * unavailable here and the caller should proceed unprotected. + * + * Split out of {@link withSecretFileLock} so a caller that must hold a lock + * **across** another lock's release can do so — the keychain hand-off needs + * exactly that, see `absorbFileSecretsIntoKeyring`. Everything about *which* + * failures refuse and which degrade lives here, so both entry points cannot + * drift on that question. + * + * Throws {@link SecretStoreUnavailableError} when the lock is held or stuck; + * returns `null` when it could not be created at all. + */ +export async function openSecretFileLock( filePath: string, - fn: () => Promise, -): Promise { +): Promise<(() => Promise) | null> { const target = path.resolve(filePath); // Create the parent directory before locking, not after. `writeStoreFile` // creates it on the way to writing the secrets file, but that runs *inside* @@ -427,7 +439,7 @@ export async function withSecretFileLock( `The lock on the secrets file at ${target} was taken over by another process while a write was in progress, so it was left alone rather than removed. If a secret you just saved is missing, save it again.`, ), ); - let release: (() => Promise) | undefined; + let release: () => Promise; try { release = await acquire(target, 0, fsShim).catch((err: unknown) => { // **Retries are for contention, and only for contention.** @@ -462,11 +474,9 @@ export async function withSecretFileLock( warnOnce( `Could not take a lock on the secrets file at ${target} (${describeError(err)}), so writes to it are not protected against another process writing at the same moment.`, ); - return fn(); + return null; } - try { - return await fn(); - } finally { + return async () => { try { await release(); } catch (err) { @@ -476,26 +486,35 @@ export async function withSecretFileLock( // // **`ERELEASED` is not a lock left behind.** When the library's refresh // tick detects a takeover it marks the lock released and drops its - // registry entry *before* calling `onCompromised`, so our `release()` - // returns `ERELEASED` without touching the filesystem. The directory - // sitting there is then the **winner's live lock** — and the message - // below would tell the operator to delete it, destroying the exclusion - // of a process that did nothing wrong. `onCompromised` has already said - // what happened, so there is nothing to add. - // - // A conditional rather than an early `return`: a `return` inside - // `finally` discards whatever the body was returning or throwing. - if ((err as NodeJS.ErrnoException).code !== "ERELEASED") { - warnOnce( - // No branch on the *removal* error code, deliberately. Our own - // guard `stat`s before removing, so anything else reaching here is - // an `rmdir` that was refused — and **stale takeover reclaims - // through that same `rmdir`**, so whatever blocked ours blocks that - // too. `ENOTEMPTY` is the reachable one; `EACCES`, `EPERM` and - // `EROFS` behave identically. Promising expiry was wrong for all. - `Could not release the lock on the secrets file at ${target} (${describeError(err)}). Stale takeover reclaims a lock through the same directory removal, so whatever blocked this blocks that too: saves will keep failing until you remove ${lockPathOf(target)} by hand.`, - ); - } + // registry entry *before* calling `onCompromised`, so this returns + // `ERELEASED` without touching the filesystem. The directory sitting + // there is then the **winner's live lock** — and the message below + // would tell the operator to delete it, destroying the exclusion of a + // process that did nothing wrong. `onCompromised` already said what + // happened, so there is nothing to add. + if ((err as NodeJS.ErrnoException).code === "ERELEASED") return; + warnOnce( + // No branch on the *removal* error code, deliberately. Our own guard + // `stat`s before removing, so anything else reaching here is an + // `rmdir` that was refused — and **stale takeover reclaims through + // that same `rmdir`**, so whatever blocked ours blocks that too. + // `ENOTEMPTY` is the reachable one; `EACCES`, `EPERM` and `EROFS` + // behave identically. Promising expiry was wrong for all of them. + `Could not release the lock on the secrets file at ${target} (${describeError(err)}). Stale takeover reclaims a lock through the same directory removal, so whatever blocked this blocks that too: saves will keep failing until you remove ${lockPathOf(target)} by hand.`, + ); } + }; +} + +export async function withSecretFileLock( + filePath: string, + fn: () => Promise, +): Promise { + const release = await openSecretFileLock(filePath); + if (release === null) return fn(); + try { + return await fn(); + } finally { + await release(); } } diff --git a/core/auth/node/secret-store-selection.ts b/core/auth/node/secret-store-selection.ts index 8b2add6131..3a55ec1f52 100644 --- a/core/auth/node/secret-store-selection.ts +++ b/core/auth/node/secret-store-selection.ts @@ -54,7 +54,11 @@ import { readSecretFilePermissions, tightenSecretFilePermissions, } from "./file-secret-store.js"; -import { isFileLockHeld, withSecretFileLock } from "./file-lock.js"; +import { + isFileLockHeld, + openSecretFileLock, + withSecretFileLock, +} from "./file-lock.js"; import { KeyringSecretStore, parseAccount, @@ -604,7 +608,17 @@ export async function absorbFileSecretsIntoKeyring( } return null; } - return staged; + // **Acquire the snapshot's lock before returning**, i.e. before the main + // lock is released. Taking it afterwards leaves a gap in which a second + // startup can take the main lock, see the staged file unlocked, and adopt + // and re-claim it — leaving this process reading a path that no longer + // exists, which is the very race the snapshot lock exists to prevent. + // + // `null` means locking is unavailable here; the hand-off still runs. The + // exposure is one unique nonce-named snapshot, which is a far better + // trade than refusing to migrate at all on a box that cannot lock. + const releaseSnapshot = await openSecretFileLock(staged).catch(() => null); + return { staged, releaseSnapshot }; // `withSecretFileLock` rejects only with the `SecretStoreUnavailableError` // it constructs itself (a lock it could not create degrades instead of // throwing), so the cast is over a value produced one call away rather @@ -616,7 +630,14 @@ export async function absorbFileSecretsIntoKeyring( return null; }); if (claimed === null) return; - await handOffStagedSecrets(claimed, filePath, keyring); + // Outside the main lock, still holding the snapshot's: a keychain + // round-trip per secret must not block ordinary writers (#1950 guarantees a + // write completing after the claim survives). + try { + await handOffStagedSecrets(claimed.staged, filePath, keyring); + } finally { + await claimed.releaseSnapshot?.(); + } } /** @@ -639,21 +660,6 @@ async function handOffStagedSecrets( staged: string, filePath: string, keyring: SecretStore, -): Promise { - await withSecretFileLock(staged, () => - handOffStagedSecretsLocked(staged, filePath, keyring), - ).catch((err: Error) => { - console.warn( - `\n[mcp-inspector] Could not lock ${staged} to migrate it into the OS keychain (${err.message}), so it has been left in place. The next run will try again.`, - ); - }); -} - -/** {@link handOffStagedSecrets}'s body, with the snapshot's lock held. */ -async function handOffStagedSecretsLocked( - staged: string, - filePath: string, - keyring: SecretStore, ): Promise { const file = new FileSecretStore({ filePath: staged }); // True only when every value reached the keychain *and* there was @@ -702,6 +708,22 @@ async function handOffStagedSecretsLocked( } } +/** + * Is `name` a migration snapshot, as opposed to the lock directory beside one? + * + * The `.lock` exclusion is load-bearing, not tidiness. `secrets.json.lock` + * and `secrets.json.migrating--.lock` both match the plain prefix + * test, and treating the latter as a snapshot is self-sustaining damage: the + * liveness probe asks about a nonexistent `.lock.lock` and so answers + * "not held", recovery then tries to hard-link a *directory* onto the secrets + * path, fails, and prints the orphan warning. Because a stale lock directory + * is never removed by a liveness *check* — only a would-be acquirer clears + * one — that false migration repeats on every startup forever, including + * after the real snapshot has long since been recovered. + */ +const isSnapshotName = (name: string, base: string): boolean => + name.startsWith(`${base}.migrating-`) && !name.endsWith(".lock"); + /** * Is there a `secrets.json`, or a snapshot orphaned by an interrupted * migration, worth taking the lock for? @@ -715,7 +737,7 @@ async function anythingToMigrate(filePath: string): Promise { const base = path.basename(filePath); try { return (await fs.readdir(path.dirname(filePath))).some( - (name) => name === base || name.startsWith(`${base}.migrating-`), + (name) => name === base || isSnapshotName(name, base), ); } catch (err) { const code = (err as NodeJS.ErrnoException).code; @@ -742,10 +764,10 @@ async function anythingToMigrate(filePath: string): Promise { */ async function recoverOrphanedSnapshots(filePath: string): Promise { const dir = path.dirname(filePath); - const prefix = `${path.basename(filePath)}.migrating-`; + const base = path.basename(filePath); let names: string[]; try { - names = (await fs.readdir(dir)).filter((n) => n.startsWith(prefix)); + names = (await fs.readdir(dir)).filter((n) => isSnapshotName(n, base)); } catch { return; // No directory yet, or unreadable — nothing to recover. } From 31feed970cc1ae4c49d60c7b2c25b5edce509dd6 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 12:49:06 -0400 Subject: [PATCH 10/12] fix(auth): make the stuck-lock guidance conditional, not an instruction (#2082) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The one genuinely new finding from Copilot round 9; the other four were re-reports of rounds 7 and 8 against an earlier state of the files, verified already fixed in e956d498 / edddc6c9. This message has now been wrong in both directions. Round 5 replaced "it expires on its own after 10s" — false whenever the same `rmdir` that blocked us also blocks stale takeover, which reclaims through that identical call — with "remove it by hand". That is false the other way: `proper-lockfile` forwards whatever the filesystem returned, so a transient failure can clear, another Inspector can legitimately acquire the path afterwards, and an operator following the advice then deletes a *live* holder's lock. Same class of hazard as the `ERELEASED` case in round 7: guidance that destroys the exclusion of a process that did nothing wrong. Nothing here can tell a permanent refusal from a passing one, so it no longer claims to. It reports what happened, says the lock may or may not clear and why, and states the two conditions the operator can check for themselves — saves still failing, and no other Inspector running — rather than asserting an outcome this code does not know. Behavioural only in what it tells a human; the locking path is unchanged. The test asserts both conditions are present and that the expiry promise is absent, so neither blanket version can come back. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H Signed-off-by: cliffhall --- .../integration/auth/node/file-lock.test.ts | 13 ++++++++++-- core/auth/node/file-lock.ts | 21 ++++++++++++------- 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/clients/web/src/test/integration/auth/node/file-lock.test.ts b/clients/web/src/test/integration/auth/node/file-lock.test.ts index 6d04532897..bd4d59292c 100644 --- a/clients/web/src/test/integration/auth/node/file-lock.test.ts +++ b/clients/web/src/test/integration/auth/node/file-lock.test.ts @@ -475,8 +475,17 @@ describe("withSecretFileLock reports what it cannot clean up", () => { // not be turned into a failure by its own teardown. expect(result).toBe("saved"); expect(warnings()).toContain("Could not release the lock"); - expect(warnings()).toContain("by hand"); - expect(warnings()).not.toContain("expires on its own"); + // Conditional guidance, not an instruction. `proper-lockfile` forwards + // whatever the filesystem returned and nothing here can tell a permanent + // refusal from a transient one — and if it was transient, this path may + // by then hold a *different, live* Inspector's lock, so an unconditional + // "remove it" would destroy the exclusion of a process that did nothing + // wrong. Both conditions must be stated. + expect(warnings()).toContain("If saves keep failing"); + expect(warnings()).toContain("no other Inspector is running"); + // …and it must not promise the lock clears by itself either, which is + // false whenever the same `rmdir` also blocks stale takeover. + expect(warnings()).not.toMatch(/It expires on its own/); }); it("warns rather than crashing the process when the library detects the takeover", async () => { diff --git a/core/auth/node/file-lock.ts b/core/auth/node/file-lock.ts index 737720312e..aaf566d69f 100644 --- a/core/auth/node/file-lock.ts +++ b/core/auth/node/file-lock.ts @@ -494,13 +494,20 @@ export async function openSecretFileLock( // happened, so there is nothing to add. if ((err as NodeJS.ErrnoException).code === "ERELEASED") return; warnOnce( - // No branch on the *removal* error code, deliberately. Our own guard - // `stat`s before removing, so anything else reaching here is an - // `rmdir` that was refused — and **stale takeover reclaims through - // that same `rmdir`**, so whatever blocked ours blocks that too. - // `ENOTEMPTY` is the reachable one; `EACCES`, `EPERM` and `EROFS` - // behave identically. Promising expiry was wrong for all of them. - `Could not release the lock on the secrets file at ${target} (${describeError(err)}). Stale takeover reclaims a lock through the same directory removal, so whatever blocked this blocks that too: saves will keep failing until you remove ${lockPathOf(target)} by hand.`, + // **Conditional, not an instruction.** Two blanket versions of this + // have now been wrong in opposite directions: "it expires on its own" + // (false whenever the same `rmdir` that blocked us also blocks stale + // takeover — `ENOTEMPTY`, `EACCES`, `EPERM`, `EROFS`), and "remove it + // by hand" (false whenever the failure was transient, since the path + // may by then hold a *different, live* Inspector's lock, and deleting + // that destroys the exclusion of a process that did nothing wrong). + // + // `proper-lockfile` forwards whatever the filesystem returned, and + // nothing here can tell a permanent refusal from a passing one. So + // this reports what happened and states the two conditions the + // operator can check for themselves, rather than asserting an outcome + // this code does not know. + `Could not release the lock on the secrets file at ${target} (${describeError(err)}). It may clear on its own — stale takeover reclaims a lock through the same directory removal, so it will not if whatever blocked this persists. If saves keep failing against this file and no other Inspector is running, remove ${lockPathOf(target)} by hand.`, ); } }; From 617ac7b4c2677202b2be1e64070c164b0a95247d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 16:36:39 -0400 Subject: [PATCH 11/12] docs: drop the Claude Desktop import from the out-of-scope list (#2082) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It has shipped. `core/mcp/import/strategies.ts` carries a `claudeDesktop` strategy that reads `claude_desktop_config.json` from the documented well-known paths and merges it in, alongside Cursor, Cline and VS Code — so listing it as a follow-up describes the spec's original scope rather than the code, and reads as a gap to anyone checking what is missing. Text only; no code path touched. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H Signed-off-by: cliffhall --- specification/v2_servers_file.md | 1 - 1 file changed, 1 deletion(-) diff --git a/specification/v2_servers_file.md b/specification/v2_servers_file.md index 50944495e0..1ec8bb1498 100644 --- a/specification/v2_servers_file.md +++ b/specification/v2_servers_file.md @@ -281,7 +281,6 @@ Each server entry may carry these Inspector-extension fields at the top level: ## Out of scope (follow-ups) -- Import-from-Claude-Desktop button (read `~/Library/Application Support/Claude/claude_desktop_config.json` or the Windows/Linux equivalent, merge into our file). - File watching for hot reload of external edits. - Per-server tags / folders / groups. - Export current list as JSON. From bb15d2c6b2fb7f86bf61289539a61dacb5923e46 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 16:37:32 -0400 Subject: [PATCH 12/12] docs: normalize emphasis markers in v2_servers_file.md (#2082) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prettier prefers `_x_` over `*x*`; two spots in the Concurrency paragraph this PR rewrote were left in the other style. `specification/` is not covered by any `format:check` glob, so nothing flagged it — found by running prettier against the file by hand while editing it. Cosmetic only. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H Signed-off-by: cliffhall --- specification/v2_servers_file.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/specification/v2_servers_file.md b/specification/v2_servers_file.md index 1ec8bb1498..b9b09f0a35 100644 --- a/specification/v2_servers_file.md +++ b/specification/v2_servers_file.md @@ -261,7 +261,7 @@ Each server entry may carry these Inspector-extension fields at the top level: - **Secret store selection (#1950)**: #1356 assumed a keychain. On a host without one — the published container (no D-Bus session, #1848), Android/Termux (no prebuilt binary, #1905), a minimal Linux install — `set` hard-failed, so those users could not persist an OAuth client secret or a stdio `env:` value **at all**. #1950 adds the two missing implementations plus the policy that picks one, and the surfaces that say which was picked. - **Selection**: `MCP_INSPECTOR_SECRET_STORE=keyring|file|memory` wins outright. Otherwise the keychain is _probed_ — an `AsyncEntry` construction plus a real read, because the container that motivated #1848 imports the package fine and only fails when it reaches for a Secret Service that isn't there. A read and not a write, deliberately: probing by writing would deposit a value in the user's login keyring at every startup for a store they may never use. If the probe fails the fallback is **`memory`** in a container whose secrets directory is not on a mount, and **`file`** otherwise — so mounting the volume the README already recommends for the catalog flips the same run to durable storage with no configuration, and an unmounted container gets the honest answer rather than a file `docker run --rm` will discard. Resolution is cached per process so the startup banner, `GET /api/config`, and the store doing the writing cannot disagree. - **`FileSecretStore` at rest**: one JSON document at `~/.mcp-inspector/secrets.json` (or `MCP_INSPECTOR_SECRET_FILE`, else under `MCP_STORAGE_DIR`), written `0600` and re-tightened at startup. With `MCP_INSPECTOR_SECRET_KEY` set it is AES-256-GCM with a scrypt-derived key and a per-write random salt; without it the values are in the clear, and _that_ is what the loud banner and the warning-toned modal footer are about. The **whole map is encrypted as a unit**, not value-by-value, so the account names (`serverId:field`) are hidden too — a per-value scheme would leave a readable index of which servers you hold a client secret for. Upgrading is lazy: setting the passphrase later re-encrypts on the next write rather than rewriting the file during a run that may never touch a secret, and the descriptor reports `pendingEncryption` until it does. A file that can no longer be decrypted reads as empty but **refuses to be written**, because replacing a file of still-valid secrets to satisfy an additive request destroys data. - - **Concurrency**: within a process, mutations are serialized per **resolved file path** (not per store instance — two `FileSecretStore`s on one file are ordinary, since the resolved store holds one and the keychain hand-off builds another). Across processes, each mutation holds an exclusive `proper-lockfile` lock on `secrets.json.lock` for the whole read-modify-write (#2082). The in-process queue is what keeps that usable: `proper-lockfile` is not reentrant, so a second `lock()` from the same process fails `ELOCKED` and would be indistinguishable from a genuine remote holder. An earlier iteration hand-rolled the lock — a `mkdir` election with an owner stamp, heartbeat and stale-takeover — and three review rounds each found a real race, the last not closable with what Node exposes (claiming a stale lock needs compare-and-swap on a directory entry, `renameat2(RENAME_EXCHANGE)`). #2082 settled that as "borrow, don't hand-roll" — but the borrowed lock is not claimed to close that race, because it does not. `proper-lockfile@4.1.2` `rmdir`s a stale lock and re-`mkdir`s without checking the directory it removed is the one it found stale, so a slow waiter can still delete a fast waiter's fresh lock; and its own detection is weaker than it looks: `updateLock` compares mtime only on the refresh tick (`stale / 2`, 5s here), while an ordinary mutation finishes in well under a second — so in the common case the tick never runs and nobody is told. Its `release` is also an unconditional `rmdir` with no ownership check, so a holder whose lock was replaced deletes the *winner's* lock on the way out, turning one compromised writer into two unprotected ones. `withSecretFileLock` therefore supplies a guarded `options.fs` whose directory removal refuses to delete a lock that is no longer the one it created (inode + birth time, which survive the library's `utimes` refresh but not a delete-and-recreate). It sits in `options.fs` rather than around `release()` because that is the one seam **both** removal paths route through — the release path and the library's `signal-exit` handler, which `rmdirSync`s every registered lock with no ownership check of its own; a guard around release alone leaves an exit at the wrong moment free to delete the winner's directory. This **narrows** the destructive window (the check is a `statSync` immediately followed by an `rmdirSync`, so nothing in-process interleaves) and surfaces the takeover in the fast case the tick misses — but it does not close it: it is still check-then-act across processes, which needs the same CAS Node does not expose. Best-effort throughout, and where a filesystem reports neither field it degrades to the library's unaided behaviour rather than to a false alarm. What **is** exclusive is the case that matters: `mkdir` is atomic and a live holder refreshes its mtime, so its lock never goes stale and two running Inspectors are genuinely serialized. The residual window opens only after a holder dies without releasing. A waiter also waits past the stale window before giving up (so a crashed holder resolves by takeover rather than failing everyone else's saves), and a lock still held after that makes `set` **fail** rather than write alongside a visible concurrent writer — `ELOCKED` is evidence the lock is working, not a reason to bypass it. **The optimistic verify stays underneath it**, and is not redundant: read `M0`, apply, write `M1`, read back `M2`, re-apply onto whatever a concurrent writer left if they differ, bounded, with `set` throwing on non-convergence rather than returning as though the value were saved. The comparison is over the **whole map** — checking only your own entry passes in exactly the case that loses data, because yours is present and the other writer's is gone. A lock is advisory between the processes that take it, so the verify is what covers a writer outside this codebase (an editor, a restored backup, an Inspector predating #2082) and what covers the lock being *unavailable*: `withSecretFileLock` runs the body anyway, warning once, on a directory that cannot hold a lock file — this store exists for boxes where the usual mechanism is missing (#1848, #1905) and must not acquire a new way to be unavailable. Reads take no lock; `writeStoreFile` is atomic, so a reader sees the old file or the new one, never a torn one. + - **Concurrency**: within a process, mutations are serialized per **resolved file path** (not per store instance — two `FileSecretStore`s on one file are ordinary, since the resolved store holds one and the keychain hand-off builds another). Across processes, each mutation holds an exclusive `proper-lockfile` lock on `secrets.json.lock` for the whole read-modify-write (#2082). The in-process queue is what keeps that usable: `proper-lockfile` is not reentrant, so a second `lock()` from the same process fails `ELOCKED` and would be indistinguishable from a genuine remote holder. An earlier iteration hand-rolled the lock — a `mkdir` election with an owner stamp, heartbeat and stale-takeover — and three review rounds each found a real race, the last not closable with what Node exposes (claiming a stale lock needs compare-and-swap on a directory entry, `renameat2(RENAME_EXCHANGE)`). #2082 settled that as "borrow, don't hand-roll" — but the borrowed lock is not claimed to close that race, because it does not. `proper-lockfile@4.1.2` `rmdir`s a stale lock and re-`mkdir`s without checking the directory it removed is the one it found stale, so a slow waiter can still delete a fast waiter's fresh lock; and its own detection is weaker than it looks: `updateLock` compares mtime only on the refresh tick (`stale / 2`, 5s here), while an ordinary mutation finishes in well under a second — so in the common case the tick never runs and nobody is told. Its `release` is also an unconditional `rmdir` with no ownership check, so a holder whose lock was replaced deletes the _winner's_ lock on the way out, turning one compromised writer into two unprotected ones. `withSecretFileLock` therefore supplies a guarded `options.fs` whose directory removal refuses to delete a lock that is no longer the one it created (inode + birth time, which survive the library's `utimes` refresh but not a delete-and-recreate). It sits in `options.fs` rather than around `release()` because that is the one seam **both** removal paths route through — the release path and the library's `signal-exit` handler, which `rmdirSync`s every registered lock with no ownership check of its own; a guard around release alone leaves an exit at the wrong moment free to delete the winner's directory. This **narrows** the destructive window (the check is a `statSync` immediately followed by an `rmdirSync`, so nothing in-process interleaves) and surfaces the takeover in the fast case the tick misses — but it does not close it: it is still check-then-act across processes, which needs the same CAS Node does not expose. Best-effort throughout, and where a filesystem reports neither field it degrades to the library's unaided behaviour rather than to a false alarm. What **is** exclusive is the case that matters: `mkdir` is atomic and a live holder refreshes its mtime, so its lock never goes stale and two running Inspectors are genuinely serialized. The residual window opens only after a holder dies without releasing. A waiter also waits past the stale window before giving up (so a crashed holder resolves by takeover rather than failing everyone else's saves), and a lock still held after that makes `set` **fail** rather than write alongside a visible concurrent writer — `ELOCKED` is evidence the lock is working, not a reason to bypass it. **The optimistic verify stays underneath it**, and is not redundant: read `M0`, apply, write `M1`, read back `M2`, re-apply onto whatever a concurrent writer left if they differ, bounded, with `set` throwing on non-convergence rather than returning as though the value were saved. The comparison is over the **whole map** — checking only your own entry passes in exactly the case that loses data, because yours is present and the other writer's is gone. A lock is advisory between the processes that take it, so the verify is what covers a writer outside this codebase (an editor, a restored backup, an Inspector predating #2082) and what covers the lock being _unavailable_: `withSecretFileLock` runs the body anyway, warning once, on a directory that cannot hold a lock file — this store exists for boxes where the usual mechanism is missing (#1848, #1905) and must not acquire a new way to be unavailable. Reads take no lock; `writeStoreFile` is atomic, so a reader sees the old file or the new one, never a torn one. - **Strict reads (`getStrict`)**: `get` is tolerant by contract, and that tolerance is wrong for exactly one caller — a migration that treats `null` as proof of absence and then _writes_. A transient read failure would look like "nothing there", and the write would replace a newer stored value with an older on-disk copy, inverting the keychain-wins rule the migration is built on. `getStrict` throws instead, and both plaintext migrations plus the keychain hand-off use it. Two traps this hit on the way in, both worth remembering: the seam must be forwarded by `DeferredSecretStore` (the store production actually uses, so without forwarding the strictness existed only in tests that injected a concrete store), and it must be implemented by _every_ store rather than falling back to `get` for the one it was introduced for. - **Bulk reads (`getMany`)**: rehydration asked field by field, and `FileSecretStore.get` reads and decrypts the entire file per call. scrypt at `N=16384` measures ~23ms, so an encrypted catalog spent ~450ms of pure key derivation on every `GET /api/servers` — a visible stall rather than a micro-optimization. The seam takes a **list of `{ serverId, fields }` across servers**, and both rehydration callers pass the whole catalog in one request, so `FileSecretStore` decrypts once per rehydration; stores for which per-field reads are already cheap (the keychain) fall back to parallel `get`. The cross-server shape is the point: a per-server version shipped first and left a 20-server catalog paying 20 serialized derivations, because both callers iterate servers — the same stall reached one server at a time instead of one field at a time. - **Durability gate on migration**: the plaintext-stripping migrations only delete the disk copy once the value is somewhere that outlives the process (`isDurable`). Against a session-scoped store, stripping would trade a secret that survives restarts for one that dies with the process — and it runs on an ordinary `GET`, so merely opening the app would destroy it.