Merge pull request #2088 from modelcontextprotocol/v2/feat/2082-file-secret-store-lock
feat(auth): give the secrets file a real cross-process lock
This commit is contained in:
Cliff Hall
GitHub
commit
fd1fa06cf5
17 changed files with 1591 additions and 91 deletions
1
.github/copilot-instructions.md
vendored
1
.github/copilot-instructions.md
vendored
|
|
@ -80,6 +80,7 @@ Both exist and do different jobs. Theme files (`src/theme/<Component>.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 `<client>/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
|
||||
|
||||
|
|
|
|||
25
AGENTS.md
25
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.
|
||||
|
|
|
|||
12
README.md
12
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.
|
||||
|
||||
|
|
|
|||
|
|
@ -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",
|
||||
|
|
|
|||
|
|
@ -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) {
|
||||
|
|
|
|||
|
|
@ -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() {
|
||||
|
|
|
|||
514
clients/web/src/test/integration/auth/node/file-lock.test.ts
Normal file
514
clients/web/src/test/integration/auth/node/file-lock.test.ts
Normal file
|
|
@ -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<typeof console.warn>;
|
||||
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<string, string>,
|
||||
): Promise<{ ready: Promise<void>; done: Promise<void> }> {
|
||||
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<void>((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<void>>;
|
||||
};
|
||||
// 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<void>>;
|
||||
};
|
||||
// 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);
|
||||
});
|
||||
|
|
@ -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-<pid>-<uuid>.lock` matches the plain prefix
|
||||
// test, and treating it as a snapshot is self-sustaining damage: the
|
||||
// liveness probe asks about a nonexistent `<name>.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;
|
||||
|
|
|
|||
|
|
@ -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",
|
||||
]),
|
||||
);
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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",
|
||||
],
|
||||
|
|
|
|||
527
core/auth/node/file-lock.ts
Normal file
527
core/auth/node/file-lock.ts
Normal file
|
|
@ -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<string>();
|
||||
|
||||
/**
|
||||
* 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<void>> {
|
||||
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<boolean> {
|
||||
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 `<filePath>.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<void>) | 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<void>;
|
||||
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<T>(
|
||||
filePath: string,
|
||||
fn: () => Promise<T>,
|
||||
): Promise<T> {
|
||||
const release = await openSecretFileLock(filePath);
|
||||
if (release === null) return fn();
|
||||
try {
|
||||
return await fn();
|
||||
} finally {
|
||||
await release();
|
||||
}
|
||||
}
|
||||
|
|
@ -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<string, string>) => Record<string, string> | null,
|
||||
): Promise<void> {
|
||||
await withSecretFileLock(this.filePath, () => this.mutateLocked(apply));
|
||||
}
|
||||
|
||||
/** {@link mutate}'s body, with the lock already held. */
|
||||
private async mutateLocked(
|
||||
apply: (map: Record<string, string>) => Record<string, string> | null,
|
||||
): Promise<void> {
|
||||
for (let attempt = 0; attempt < MAX_WRITE_ATTEMPTS; attempt++) {
|
||||
let current: Record<string, string>;
|
||||
|
|
@ -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<T>(fn: () => Promise<T>): Promise<T> {
|
||||
const key = path.resolve(this.filePath);
|
||||
|
|
|
|||
|
|
@ -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<void> {
|
||||
const filePath = defaultSecretFilePath();
|
||||
|
||||
// A crash between the claim and the delete leaves only
|
||||
// `secrets.json.migrating-<pid>`. 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-<pid>`. 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<void> {
|
||||
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-<pid>-<uuid>.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 `<name>.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<boolean> {
|
||||
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<void> {
|
||||
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 });
|
||||
|
|
|
|||
45
package-lock.json
generated
45
package-lock.json
generated
|
|
@ -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",
|
||||
|
|
|
|||
|
|
@ -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",
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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 = {
|
||||
|
|
|
|||
Loading…
Reference in a new issue