diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 9e8e80332..e6a22c98c 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 a63b8d0d8..67cd15964 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -68,7 +68,28 @@ 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. Read its header before +│ │ │ # citing it: it makes two LIVE Inspectors exclusive, and +│ │ │ # 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 (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 +│ │ │ # 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, │ │ │ # else fall back LOUDLY — to memory in a container with @@ -228,6 +249,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 e88002727..49528454a 100644 --- a/README.md +++ b/README.md @@ -504,9 +504,17 @@ 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. +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: 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. + +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. 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 03cf74319..c37897cae 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 2c0197a9a..109011ba8 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/server/vite-base-config.ts b/clients/web/server/vite-base-config.ts index c351f4732..3d9c1a902 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 new file mode 100644 index 000000000..bd4d59292 --- /dev/null +++ b/clients/web/src/test/integration/auth/node/file-lock.test.ts @@ -0,0 +1,514 @@ +/** + * `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 { existsSync } from "node:fs"; +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"; +import { SecretStoreUnavailableError } from "@inspector/core/auth/node/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, + /** + * 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(); + 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("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. + // + // 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 }); + 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); + + 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; + + 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_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 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 — 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 () => { + 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("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("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 + // 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`; + 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:AFTER_CRASH", "1"); + + const reader = new FileSecretStore({ filePath: target }); + 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("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: 10_000, + }); + await expect(store.delete("srv", "env:A")).resolves.toBeUndefined(); + await release(); + + // …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", () => { + it("does not delete the winner's lock after being taken over", async () => { + // 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(); + const lockPath = `${target}.lock`; + + let winner: { ino: number; birthtimeMs: number } | undefined; + const result = await withSecretFileLock(target, async () => { + 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"); + // 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("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"); + 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"); + // 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 () => { + // `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 removal guard. + 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 }, + ); + }); + + // 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 7fb03da4b..9cc2b065d 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,197 @@ 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 + // 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("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; + 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(); + }); + + // 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("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("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 + // 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/src/test/integration/server/vite-base-config.test.ts b/clients/web/src/test/integration/server/vite-base-config.test.ts index fc3052233..90c39abd9 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/clients/web/tsup.runner.config.ts b/clients/web/tsup.runner.config.ts index 863c0e296..a6512a48e 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 000000000..aaf566d69 --- /dev/null +++ b/core/auth/node/file-lock.ts @@ -0,0 +1,527 @@ +/** + * 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 — 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)`). + * + * 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 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. + * + * **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 + * 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 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 +// 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"; +import { SecretStoreUnavailableError } from "./secret-store.js"; + +/** + * 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 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. + * + * 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: 20, + factor: 2, + minTimeout: 20, + 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(); + +/** + * 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(); +} + +/** + * 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? + * + * 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); + +/** 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; +} + +/** + * Identify a lock directory, or `null` if it cannot be read. + * + * `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 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. + */ +function identifySync(lockPath: string): LockIdentity | null { + try { + const stat = statSync(lockPath); + return { ino: stat.ino, birthtimeMs: stat.birthtimeMs }; + } catch { + return null; + } +} + +/** + * 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. + * + * **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. + * + * `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. + */ +function guardedFs( + lockPath: string, + 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, + // **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. + 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(); + }, + }; +} + +/** + * 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 + * 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. + */ +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 + // 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.`, + ), + }); +} + +/** + * 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`. + * + * 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. + */ +/** + * 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, +): 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* + // 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(() => {}); + // 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; + try { + 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 + // 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 (!isStuckOrHeld(err, lockPathOf(target))) throw err; + return acquire(target, RETRY, fsShim); + }); + } 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 (isStuckOrHeld(err, lockPathOf(target))) { + throw new SecretStoreUnavailableError( + `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 + // 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( + `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 null; + } + return async () => { + 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. + // + // **`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 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( + // **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.`, + ); + } + }; +} + +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/file-secret-store.ts b/core/auth/node/file-secret-store.ts index edff72c19..4d8127518 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 cb468af93..3a55ec1f5 100644 --- a/core/auth/node/secret-store-selection.ts +++ b/core/auth/node/secret-store-selection.ts @@ -54,6 +54,11 @@ import { readSecretFilePermissions, tightenSecretFilePermissions, } from "./file-secret-store.js"; +import { + isFileLockHeld, + openSecretFileLock, + withSecretFileLock, +} from "./file-lock.js"; import { KeyringSecretStore, parseAccount, @@ -532,52 +537,130 @@ 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. + // 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 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 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.`, - ); + // 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. + // + 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; + // **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 + // 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; + // 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?.(); } +} +/** + * 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 { 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. @@ -625,6 +708,52 @@ export async function absorbFileSecretsIntoKeyring( } } +/** + * 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? + * + * 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 || isSnapshotName(name, base), + ); + } 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"; + } +} + /** * Adopt a snapshot left behind by a process that died mid-migration. * @@ -635,15 +764,23 @@ export async function absorbFileSecretsIntoKeyring( */ 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. } 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 }); diff --git a/package-lock.json b/package-lock.json index d934e96ec..d29509885 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 4545060a0..991138524 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 53a7a0a2c..b9b09f0a3 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" — 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. - - **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. @@ -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. diff --git a/vitest.shared.mts b/vitest.shared.mts index 1ebbc96ae..8ad914b50 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 = {