feat(auth): give the secrets file a real cross-process lock (#2082)
#1950 shipped FileSecretStore without cross-process mutual exclusion, by decision: an earlier revision hand-rolled a `mkdir` election with an owner stamp, a heartbeat and a stale-takeover, and three consecutive review rounds found a real race in it. The last one is not closable with what Node exposes — claiming a stale lock atomically needs compare-and-swap on a directory entry (`renameat2(RENAME_EXCHANGE)`) — so it was replaced with optimistic verify-and-retry and the residual documented. #2082 settles that as "borrow, don't hand-roll". `proper-lockfile` is what npm itself locks with, and stale-takeover is precisely the problem it has already solved: it re-stats the lock directory after claiming it and gives the lock up when the mtime is not the one it wrote, so the loser of a takeover race releases instead of proceeding. - core/auth/node/file-lock.ts: `withSecretFileLock`. `realpath: false` so a file can be locked into existence (the very first `set` has no `secrets.json`, and the library's default resolves through `fs.realpath`); a warning in place of the library's `onCompromised`, which throws from a timer and would take the session down; and a degrade-with-one-warning path rather than a throw when no lock can be taken — this store exists for boxes missing the usual mechanism (#1848, #1905) and must not gain a new way to be unavailable. - FileSecretStore.mutate holds it across the whole read-modify-write. The optimistic verify stays underneath and is not redundant: a lock is advisory between the processes that take it, so the verify covers a writer outside this codebase and covers the degrade path. The in-process queue stays too, and gains a second job — proper-lockfile is not reentrant, so serializing per path keeps ELOCKED meaning "another process". - absorbFileSecretsIntoKeyring takes the same lock around orphan adoption and the atomic claim, behind a lock-free `readdir` fast path so the common startup (keychain available, no file ever written) neither creates a lock directory nor warns about one it could not create. proper-lockfile is a root `dependency` per the placement rule, and is named in all three bundler `external` lists: tsup externalizes what the *client's* manifest declares, so a root-only CJS package was being inlined into the ESM bundles, leaving esbuild's `Dynamic require of "path" is not supported` shim that killed `--cli` at import time. That rule was undocumented; it is now in AGENTS.md beside the placement rule that creates it, and mirrored into .github/copilot-instructions.md. Tests: 7 new in file-lock.test.ts driving a real second process (the existing suite structurally cannot — `serialize` orders in-process callers before the lock sees them), plus two for the migration fast path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
parent
7701c68028
commit
04dbc36544
14 changed files with 718 additions and 90 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
|
||||
|
||||
|
|
|
|||
12
AGENTS.md
12
AGENTS.md
|
|
@ -68,7 +68,15 @@ v2/main/
|
|||
│ │ │ # throws and the routes turn into a 503, and the keychain
|
||||
│ │ │ # probe), file-secret-store.ts (0600 JSON, AES-256-GCM when
|
||||
│ │ │ # MCP_INSPECTOR_SECRET_KEY is set — refuses to overwrite a
|
||||
│ │ │ # file it cannot decrypt rather than destroying it), and
|
||||
│ │ │ # file it cannot decrypt rather than destroying it),
|
||||
│ │ │ # file-lock.ts (withSecretFileLock: the cross-process
|
||||
│ │ │ # mutual exclusion #2082 settled on — proper-lockfile,
|
||||
│ │ │ # borrowed rather than hand-rolled, because stale-takeover
|
||||
│ │ │ # is what three review rounds of a mkdir election failed to
|
||||
│ │ │ # get right; DEGRADES rather than throws when no lock can
|
||||
│ │ │ # be taken, since this store exists for boxes missing the
|
||||
│ │ │ # usual mechanism and must not gain a new way to fail),
|
||||
│ │ │ # and
|
||||
│ │ │ # secret-store-selection.ts (the POLICY: explicit
|
||||
│ │ │ # MCP_INSPECTOR_SECRET_STORE wins, else probe the keychain,
|
||||
│ │ │ # else fall back LOUDLY — to memory in a container with
|
||||
|
|
@ -222,6 +230,8 @@ The same **placement** rule covers anything reached only through **root-owned co
|
|||
- A package only the tests, the test servers, or the build tooling need belongs in **`devDependencies`** — `express`, added there by #1970.
|
||||
- `yaml` is in `dependencies` today even though its only importer is `test-servers/src/load-config.ts`. Left as-is deliberately (moving it changes what ships, which is not a docs change); if you touch it, confirm no published path reads YAML first.
|
||||
|
||||
**A root-declared package that `core/` imports at runtime must also be named in each client's bundler `external` list.** tsup and Vite externalize what the *client's* `package.json` declares, and a root-only dependency is in none of them — so it gets **bundled**, silently, and the placement rule above is what guarantees every such package is root-only. For a CJS package inlined into an ESM bundle that is fatal rather than merely wasteful: esbuild leaves a `Dynamic require of "path" is not supported` shim that throws at *import* time, so the binary dies before it parses a flag. `proper-lockfile` hit exactly that in #2082; `@napi-rs/keyring` is listed in all three for the same reason. The three lists are `clients/cli/tsup.config.ts`, `clients/tui/tsup.config.ts`, and `clients/web/tsup.runner.config.ts` — add a new package to **all** of them, since which client reaches it is a function of what `core/` imports, not of what the client's own code names.
|
||||
|
||||
**A dependency that renders React components must be bundled into the client that uses it, and is then not a root dependency.** An externalized package resolves its own `react` from wherever npm placed **it** in the consumer's tree, and npm places a package beside a React satisfying *that package's* peer range — looser than ours in every case here, which is all it takes to split React. `ink-form` and `ink-scroll-view` declare `">=18"`, satisfied by a consumer's React 18 while our React 19 nests underneath: the bundle renders through one React, those packages call hooks on another, and the TUI crashes on the first hook (#1952). Both are inlined by `clients/tui/tsup.config.ts` (`noExternal`) and declared only in `clients/tui/package.json`, where the build resolves them — declaring an inlined package at the root would just make consumers install a second, unused copy.
|
||||
|
||||
**`ink` is the single exemption, and it is justified by cost, not by safety.** Bundling it works but adds ~1.4MB (`react-reconciler` + `yoga-layout`, plus a `createRequire` banner, since inlined CJS calls `require` at runtime and esbuild's ESM interop rejects that without a real `require` in scope). **Never justify an exemption by a peer range** — `ink` briefly carried "its `">=19"` peer keeps npm honest", which is false: a consumer pinning React 19.0 satisfies `">=19"` while a narrower range of ours nests underneath. What actually makes the exemption safe is a *different* lever: the **root `react` range stays open to the whole major (`^19.0.0`)**, so npm can dedupe our React with whatever React 19 a consumer pins and an external `ink` lands on the same copy the bundle uses. Narrowing it (e.g. back to `^19.2.4`) silently reopens the crash for the renderer itself, which breaks TUI *startup*, not just its forms. `clients/tui/__tests__/tsupConfig.test.ts` enforces all of it: React-rendering deps inlined, each exempt package both external and root-declared, and the root range pinned to `ink`'s peer floor.
|
||||
|
|
|
|||
|
|
@ -502,9 +502,11 @@ Setting the passphrase later is safe — the next write upgrades an existing pla
|
|||
|
||||
The Inspector writes the file `0600` and re-tightens it at startup if something loosened it. If it _cannot_ — the file belongs to another user, or the mount is read-only — it says so in the log rather than continuing to describe the file as protected, since on that box the mode claim above is not true.
|
||||
|
||||
**Two Inspectors, one file.** Within a process, mutations are serialized per file path, so a web session's own concurrent saves cannot lose each other. Across processes — a CLI run beside a web session — there is deliberately **no lock**: writers are allowed to collide and the loser is made to notice. Each mutation reads the file, applies its change, writes, then reads back and compares the whole map; if another process wrote in between, it re-applies onto what they left and retries, and after five lost rounds it fails loudly rather than returning as though the value were saved.
|
||||
**Two Inspectors, one file.** Within a process, mutations are serialized per file path, so a web session's own concurrent saves cannot lose each other. Across processes — a CLI run beside a web session — each mutation takes an exclusive lock on `secrets.json.lock` for the whole read-modify-write, using [`proper-lockfile`](https://github.com/moxystudio/node-proper-lockfile) (the same library npm itself locks with). The lock expires 10 seconds after its holder stops refreshing it, so an Inspector that is killed mid-save does not leave the file unwritable.
|
||||
|
||||
This is **not mutual exclusion**, and the residual case is worth stating: the verify only catches a clobber that has already landed, so if one Inspector reads back *before* the other's write arrives, both report success and one value is gone. That needs two Inspectors writing the same file within the gap between one's write and its read-back — narrow, but not only a crash. An earlier build did take a lock (`secrets.json.lock`); it was removed because making a `mkdir` lock single-winner on a stale takeover needs a compare-and-swap on a directory entry that Node does not expose, so it had the same class of failure with several hundred more lines and no way to close it.
|
||||
Underneath the lock, each mutation still reads the file, applies its change, writes, then reads back and compares the whole map; if something wrote in between it re-applies onto what was left and retries, failing loudly after five lost rounds rather than returning as though the value were saved. That check is not redundant with the lock — a lock only orders the writers that *take* it, so it covers an editor, a restored backup, or an Inspector older than this release.
|
||||
|
||||
It is also what covers the lock being unavailable. This store exists for boxes where the usual mechanism isn't there, so a directory that can't hold a lock file — a read-only `$HOME`, a mount owned by another uid — makes the save proceed unlocked with a warning, rather than turning every `set` into a failure on exactly the deployments the store was written for.
|
||||
|
||||
Three env vars affect where the file lands. `MCP_INSPECTOR_SECRET_STORE=keyring|file|memory` picks the store outright, bypassing the probe. `MCP_INSPECTOR_SECRET_FILE` names the file. Failing both, the file follows `MCP_STORAGE_DIR` — the same variable that relocates OAuth tokens and `client.json` — so mounting a volume at your configured storage directory is enough to make secrets durable there.
|
||||
|
||||
|
|
|
|||
|
|
@ -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) {
|
||||
|
|
|
|||
266
clients/web/src/test/integration/auth/node/file-lock.test.ts
Normal file
266
clients/web/src/test/integration/auth/node/file-lock.test.ts
Normal file
|
|
@ -0,0 +1,266 @@
|
|||
/**
|
||||
* `withSecretFileLock` against a real filesystem and a real second process
|
||||
* (#2082).
|
||||
*
|
||||
* The property under test is cross-process mutual exclusion, and the
|
||||
* existing suite structurally cannot reach it: `FileSecretStore.serialize`
|
||||
* is one process-wide queue per path, so two in-process callers are ordered
|
||||
* before the lock ever sees them. A child process is not scaffolding here —
|
||||
* it is the only participant that can produce the interleaving.
|
||||
*/
|
||||
import {
|
||||
describe,
|
||||
it,
|
||||
expect,
|
||||
beforeEach,
|
||||
afterEach,
|
||||
vi,
|
||||
type MockInstance,
|
||||
} from "vitest";
|
||||
import { execFile } from "node:child_process";
|
||||
import { createRequire } from "node:module";
|
||||
import * as fs from "node:fs/promises";
|
||||
import * as os from "node:os";
|
||||
import * as path from "node:path";
|
||||
import { promisify } from "node:util";
|
||||
import {
|
||||
withSecretFileLock,
|
||||
resetFileLockWarnings,
|
||||
} from "@inspector/core/auth/node/file-lock.js";
|
||||
import { FileSecretStore } from "@inspector/core/auth/node/file-secret-store.js";
|
||||
|
||||
const run = promisify(execFile);
|
||||
const require_ = createRequire(import.meta.url);
|
||||
/**
|
||||
* Resolved in the parent and handed to the child. The child's cwd is not
|
||||
* this repo, and `proper-lockfile` lives in the *root* install rather than
|
||||
* `clients/web`'s, so a bare `require("proper-lockfile")` there resolves
|
||||
* against whatever happens to be above the temp directory — usually nothing.
|
||||
*/
|
||||
const LOCKFILE_MODULE = require_.resolve("proper-lockfile");
|
||||
|
||||
let tmpDir: string;
|
||||
let warn: MockInstance<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,
|
||||
): Promise<{ ready: Promise<void>; done: Promise<void> }> {
|
||||
const script = `
|
||||
const lockfile = require(${JSON.stringify(LOCKFILE_MODULE)});
|
||||
lockfile
|
||||
.lock(${JSON.stringify(target)}, { realpath: false, stale: 10000 })
|
||||
.then(async (release) => {
|
||||
process.stdout.write("acquired\\n");
|
||||
await new Promise((r) => setTimeout(r, ${holdMs}));
|
||||
await release();
|
||||
process.stdout.write("released\\n");
|
||||
})
|
||||
.catch((err) => {
|
||||
process.stdout.write("failed:" + err.code + "\\n");
|
||||
process.exitCode = 1;
|
||||
});
|
||||
`;
|
||||
const child = run(process.execPath, ["-e", script]);
|
||||
let seenReady = false;
|
||||
const done = child.then(({ stdout }) => {
|
||||
expect(stdout).toContain("acquired");
|
||||
expect(stdout).toContain("released");
|
||||
});
|
||||
// `execFile` buffers, so the "acquired" line is only readable off the
|
||||
// stream. Subscribe before awaiting anything, or the line is missed.
|
||||
const ready = new Promise<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("serializes two FileSecretStores in different processes", async () => {
|
||||
// The end-to-end shape from the issue: a CLI run beside a web session.
|
||||
// The child holds the lock while the parent's `set` is in flight, so the
|
||||
// parent's whole read-modify-write happens after the child is gone.
|
||||
const target = filePath();
|
||||
const store = new FileSecretStore({ filePath: target });
|
||||
await store.set("srv", "env:FIRST", "1");
|
||||
|
||||
const { ready, done } = await holdLockInChildProcess(target, 300);
|
||||
await ready;
|
||||
await store.set("srv", "env:SECOND", "2");
|
||||
await done;
|
||||
|
||||
const reader = new FileSecretStore({ filePath: target });
|
||||
expect(await reader.get("srv", "env:FIRST")).toBe("1");
|
||||
expect(await reader.get("srv", "env:SECOND")).toBe("2");
|
||||
}, 20_000);
|
||||
});
|
||||
|
||||
describe("withSecretFileLock degrades rather than failing", () => {
|
||||
it("runs the body anyway when the lock cannot be created, and says so once", async () => {
|
||||
// A directory that does not exist stands in for every real variant —
|
||||
// read-only `$HOME`, a mount owned by another uid, a filesystem without
|
||||
// `mkdir` semantics. This store exists for boxes where the usual
|
||||
// mechanism is missing, so it must not gain a new way to be unavailable.
|
||||
const target = path.join(tmpDir, "no-such-dir", "secrets.json");
|
||||
|
||||
let ran = 0;
|
||||
await withSecretFileLock(target, async () => {
|
||||
ran += 1;
|
||||
});
|
||||
await withSecretFileLock(target, async () => {
|
||||
ran += 1;
|
||||
});
|
||||
|
||||
expect(ran).toBe(2);
|
||||
expect(warnings()).toContain("Could not take a lock on the secrets file");
|
||||
// Once per reason per process — a warning on every save would be noise
|
||||
// on precisely the deployment that cannot act on it.
|
||||
expect(warn).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("still saves the secret when no lock can be taken", async () => {
|
||||
// The degrade has to be end-to-end, not just in the helper: `set` on a
|
||||
// lock-hostile directory must persist, falling back to the #1950
|
||||
// optimistic behaviour.
|
||||
const target = filePath();
|
||||
const lockPath = `${target}.lock`;
|
||||
// Occupy the lock's own path with a *file*, so `mkdir` fails EEXIST
|
||||
// forever and no takeover can succeed — a permanent, non-ELOCKED
|
||||
// failure rather than contention.
|
||||
await fs.writeFile(lockPath, "not a lock directory", "utf-8");
|
||||
|
||||
const store = new FileSecretStore({ filePath: target });
|
||||
await store.set("srv", "env:MINE", "1");
|
||||
|
||||
const reader = new FileSecretStore({ filePath: target });
|
||||
expect(await reader.get("srv", "env:MINE")).toBe("1");
|
||||
expect(warnings()).toMatch(
|
||||
/Could not take a lock|has held the secrets file/,
|
||||
);
|
||||
}, 20_000);
|
||||
|
||||
it("gives up waiting on a holder that never releases, and proceeds", async () => {
|
||||
// Holding it from *this* process: `proper-lockfile` is not reentrant, so
|
||||
// a second `lock()` on the same path fails `ELOCKED` exactly as a remote
|
||||
// holder's would — the difference the in-process queue exists to keep
|
||||
// out of the lock's way.
|
||||
const target = filePath();
|
||||
const lockfile = require_(LOCKFILE_MODULE) as {
|
||||
lock: (f: string, o: object) => Promise<() => Promise<void>>;
|
||||
};
|
||||
const release = await lockfile.lock(target, {
|
||||
realpath: false,
|
||||
stale: 60_000,
|
||||
});
|
||||
|
||||
let ran = false;
|
||||
await withSecretFileLock(target, async () => {
|
||||
ran = true;
|
||||
});
|
||||
await release();
|
||||
|
||||
// Proceeding is the lesser evil: refusing would lose the value outright,
|
||||
// whereas proceeding falls back to the residual #1950 behaviour — and it
|
||||
// says which of the two happened.
|
||||
expect(ran).toBe(true);
|
||||
expect(warnings()).toContain("without the lock");
|
||||
}, 20_000);
|
||||
});
|
||||
|
||||
describe("withSecretFileLock reports what it cannot clean up", () => {
|
||||
it("warns rather than crashing the process when the lock is taken over", async () => {
|
||||
// `proper-lockfile`'s default `onCompromised` *throws*, from a timer
|
||||
// with no caller on the stack — an uncaught exception that takes an
|
||||
// Inspector session down. Driven for real: the lock directory is removed
|
||||
// while held, which is what an operator "clearing a stuck lock" does,
|
||||
// and the library's own refresh tick (`stale / 2`) notices.
|
||||
const target = filePath();
|
||||
const result = await withSecretFileLock(target, async () => {
|
||||
await fs.rm(`${target}.lock`, { recursive: true, force: true });
|
||||
await vi.waitFor(
|
||||
() => expect(warnings()).toContain("was taken over by another process"),
|
||||
{ timeout: 20_000, interval: 250 },
|
||||
);
|
||||
return "saved";
|
||||
});
|
||||
|
||||
// The body's result is returned regardless. A compromised lock means the
|
||||
// guarantee was lost, not that the work did not happen, and the release
|
||||
// that then fails (`ELOCKNOTHELD`, since the library has already given
|
||||
// the lock up) must not turn a completed save into a thrown error.
|
||||
expect(result).toBe("saved");
|
||||
expect(warnings()).toContain("Could not release the lock");
|
||||
}, 30_000);
|
||||
});
|
||||
|
|
@ -684,6 +684,49 @@ describe("absorbFileSecretsIntoKeyring", () => {
|
|||
return filePath;
|
||||
}
|
||||
|
||||
it("takes no lock when there is nothing to migrate (#2082)", async () => {
|
||||
// The overwhelmingly common startup: a keychain is available and no file
|
||||
// was ever written. Locking first would create and remove a lock
|
||||
// directory on every run — and on a box whose storage directory does not
|
||||
// exist yet the lock cannot be created at all, so the degrade path would
|
||||
// warn about unprotected writes on every single run, with nothing to
|
||||
// protect.
|
||||
const filePath = path.join(tmpDir, "no-such-dir", "secrets.json");
|
||||
process.env.MCP_INSPECTOR_SECRET_FILE = filePath;
|
||||
const warn = vi.spyOn(console, "warn").mockImplementation(() => {});
|
||||
const mod = await loadWithProbe(true);
|
||||
|
||||
await mod.absorbFileSecretsIntoKeyring(new InMemorySecretStore());
|
||||
|
||||
expect(warn).not.toHaveBeenCalled();
|
||||
expect(existsSync(`${filePath}.lock`)).toBe(false);
|
||||
});
|
||||
|
||||
it("still adopts an orphan when the live file is absent (#2082)", async () => {
|
||||
// The fast path above must not be a `stat` of `secrets.json`: an
|
||||
// interrupted migration leaves *only* the snapshot, which is precisely
|
||||
// the case where there is everything to migrate and no live file.
|
||||
const orphan = path.join(tmpDir, "secrets.json.migrating-123-abc");
|
||||
await fs.writeFile(
|
||||
orphan,
|
||||
JSON.stringify({
|
||||
version: 1,
|
||||
encryption: "none",
|
||||
secrets: { "srv:oauthClientSecret": "from-orphan" },
|
||||
}),
|
||||
"utf-8",
|
||||
);
|
||||
process.env.MCP_INSPECTOR_SECRET_FILE = path.join(tmpDir, "secrets.json");
|
||||
vi.spyOn(console, "warn").mockImplementation(() => {});
|
||||
const mod = await loadWithProbe(true);
|
||||
const keyring = new InMemorySecretStore();
|
||||
|
||||
await mod.absorbFileSecretsIntoKeyring(keyring);
|
||||
|
||||
expect(await keyring.get("srv", "oauthClientSecret")).toBe("from-orphan");
|
||||
expect(existsSync(orphan)).toBe(false);
|
||||
});
|
||||
|
||||
it("moves the file's secrets into the keychain and removes the file", async () => {
|
||||
const filePath = await seedFile({ "srv:oauthClientSecret": "from-file" });
|
||||
process.env.MCP_INSPECTOR_SECRET_FILE = filePath;
|
||||
|
|
|
|||
|
|
@ -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",
|
||||
],
|
||||
|
|
|
|||
183
core/auth/node/file-lock.ts
Normal file
183
core/auth/node/file-lock.ts
Normal file
|
|
@ -0,0 +1,183 @@
|
|||
/**
|
||||
* Cross-process mutual exclusion for the secrets file (#2082).
|
||||
*
|
||||
* **Why a library and not a hand-rolled election.** #1950 shipped without a
|
||||
* lock on purpose. An earlier revision of it *did* take one — a `mkdir`
|
||||
* election with an owner stamp, a heartbeat and a stale-takeover — and three
|
||||
* consecutive review rounds found a real race in it. The last one is not
|
||||
* closable with what Node exposes: claiming a stale lock atomically needs
|
||||
* compare-and-swap on a directory entry (`renameat2(RENAME_EXCHANGE)`), and
|
||||
* without it a waiter that loses the race can move the winner's *fresh* lock
|
||||
* aside and enter alongside it.
|
||||
*
|
||||
* So the choice #2082 settles is not "lock versus no lock" but "hand-rolled
|
||||
* versus borrowed". `proper-lockfile` is the borrowed one: it is what npm
|
||||
* itself uses, and stale-takeover is precisely the problem it has already
|
||||
* solved — it re-`stat`s the lock directory after claiming it and gives the
|
||||
* lock up if the mtime is not the one it wrote, so a loser of the takeover
|
||||
* race releases rather than proceeding.
|
||||
*
|
||||
* **What this does not remove.** The optimistic verify in
|
||||
* {@link FileSecretStore.mutate} stays. A lock is an advisory convention
|
||||
* between participants that take it, so it orders Inspector against
|
||||
* Inspector and says nothing about an editor, a backup restore, or an
|
||||
* Inspector old enough to predate this file. The verify is also what covers
|
||||
* {@link withSecretFileLock} *declining* — see below.
|
||||
*
|
||||
* **Degrading rather than failing is deliberate.** The store's whole reason
|
||||
* for existing is a box where the usual mechanism is unavailable (no
|
||||
* keychain, #1848/#1905), so it must not acquire a *new* way to be
|
||||
* unavailable. A directory that cannot hold a lock file — a read-only
|
||||
* `$HOME`, a filesystem without `mkdir` semantics, a container mount owned
|
||||
* by another uid — would otherwise turn every `set` into a hard failure on
|
||||
* exactly the deployments this store was written for. So a lock that cannot
|
||||
* be taken runs the body anyway, with the #1950 optimistic behaviour
|
||||
* underneath it, and says so once.
|
||||
*/
|
||||
|
||||
import * as path from "node:path";
|
||||
// CJS-only package. A default import is the shape that survives every
|
||||
// bundler this repo runs core/ through (tsup for cli/tui, vite's SSR/node
|
||||
// graph for the web runner); named imports off a CJS module depend on
|
||||
// lexer detection that esbuild and rollup disagree about.
|
||||
import properLockfile from "proper-lockfile";
|
||||
|
||||
/**
|
||||
* How long a lock may go untouched before another process may claim it.
|
||||
*
|
||||
* `proper-lockfile` refreshes the lock's mtime at `stale / 2` for as long as
|
||||
* the holder is alive, so this is not "how long a mutation may take" — it is
|
||||
* how long after a holder *dies* the file stays unwritable. 10s is the
|
||||
* library's own default and the value npm ships with; a mutation is a read,
|
||||
* an scrypt derivation and an atomic write, so the margin is enormous.
|
||||
*/
|
||||
const STALE_MS = 10_000;
|
||||
|
||||
/**
|
||||
* How long a waiter will keep trying before giving up.
|
||||
*
|
||||
* The retry schedule below tops out around 3s of waiting. That is long
|
||||
* enough for any real mutation to finish (see above) and short enough that a
|
||||
* pathological case surfaces as a slow save rather than a hung one.
|
||||
*/
|
||||
const RETRY = {
|
||||
retries: 8,
|
||||
factor: 2,
|
||||
minTimeout: 20,
|
||||
maxTimeout: 1_000,
|
||||
} as const;
|
||||
|
||||
/** Emitted once per process, not once per call — see {@link warnOnce}. */
|
||||
const warned = new Set<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();
|
||||
}
|
||||
|
||||
/**
|
||||
* Did `proper-lockfile` decline because someone else holds the lock?
|
||||
*
|
||||
* A bare cast rather than a guarded narrowing: the only caller is the `catch`
|
||||
* around `properLockfile.lock`, and the library rejects with a real `Error`
|
||||
* carrying a `code` on every path. Guarding would add branches nothing can
|
||||
* exercise, which is a worse trade than an assertion whose one caller is two
|
||||
* lines away.
|
||||
*/
|
||||
const isHeldElsewhere = (err: unknown): boolean =>
|
||||
(err as NodeJS.ErrnoException).code === "ELOCKED";
|
||||
|
||||
/**
|
||||
* The message from whatever was thrown.
|
||||
*
|
||||
* Same reasoning as above for the non-`Error` arm — `proper-lockfile` does not
|
||||
* reject with one — except that this is used where a thrown non-`Error` would
|
||||
* otherwise be reported as `[object Object]`, so the fallback earns its place
|
||||
* even though nothing can provoke it.
|
||||
*/
|
||||
/* v8 ignore next 2 -- @preserve: the non-Error arm is unreachable via
|
||||
proper-lockfile, which rejects only with Errors. */
|
||||
const describeError = (err: unknown): string =>
|
||||
err instanceof Error ? err.message : String(err);
|
||||
|
||||
/**
|
||||
* Run `fn` holding an exclusive cross-process lock on `filePath`.
|
||||
*
|
||||
* The lock is `<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.
|
||||
*
|
||||
* `realpath: false` is load-bearing: by default the library resolves the
|
||||
* target through `fs.realpath`, which fails `ENOENT` on a secrets file that
|
||||
* does not exist yet — i.e. on the very first `set`, the one call that has
|
||||
* nothing to fall back on. Resolving the path lexically instead lets a file
|
||||
* be locked into existence. The cost is that two paths reaching one file
|
||||
* through different symlinks take different locks; the store resolves its
|
||||
* path once at construction and every caller goes through it, so that is a
|
||||
* shape this codebase does not produce.
|
||||
*
|
||||
* Returns whatever `fn` returns. `fn` runs exactly once either way — the
|
||||
* lock's absence changes the guarantee, never whether the work happens.
|
||||
*/
|
||||
export async function withSecretFileLock<T>(
|
||||
filePath: string,
|
||||
fn: () => Promise<T>,
|
||||
): Promise<T> {
|
||||
const target = path.resolve(filePath);
|
||||
let release: (() => Promise<void>) | undefined;
|
||||
try {
|
||||
release = await properLockfile.lock(target, {
|
||||
realpath: false,
|
||||
stale: STALE_MS,
|
||||
retries: RETRY,
|
||||
// The library's default `onCompromised` *throws* — from a timer, with
|
||||
// no caller on the stack, so it lands as an uncaught exception and
|
||||
// takes the process down. A compromised lock means someone declared
|
||||
// ours stale and took it; the write in flight is at risk, which is
|
||||
// worth saying and is not worth killing an Inspector session over.
|
||||
onCompromised: (err) =>
|
||||
warnOnce(
|
||||
`The lock on the secrets file at ${target} was taken over by another process while a write was in progress (${err.message}). If a secret you just saved is missing, save it again.`,
|
||||
),
|
||||
});
|
||||
} catch (err) {
|
||||
warnOnce(
|
||||
isHeldElsewhere(err)
|
||||
? `Another process has held the secrets file at ${target} for longer than this write was willing to wait, so the write went ahead without the lock. If two Inspectors are saving secrets at once, one of them may not be saved.`
|
||||
: `Could not take a lock on the secrets file at ${target} (${describeError(err)}), so writes to it are not protected against another process writing at the same moment.`,
|
||||
);
|
||||
return fn();
|
||||
}
|
||||
try {
|
||||
return await fn();
|
||||
} finally {
|
||||
try {
|
||||
await release();
|
||||
} catch (err) {
|
||||
// The body already ran and its result is being returned; a release
|
||||
// that failed means the lock was taken from us (declared stale while
|
||||
// we held it) or the directory went away. Neither is worth turning a
|
||||
// successful save into a failure, but a silent catch would leave a
|
||||
// lock nobody can explain, so say it.
|
||||
warnOnce(
|
||||
`Could not release the lock on the secrets file at ${target} (${describeError(err)}). It expires on its own after ${STALE_MS / 1000}s.`,
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -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,7 @@ import {
|
|||
readSecretFilePermissions,
|
||||
tightenSecretFilePermissions,
|
||||
} from "./file-secret-store.js";
|
||||
import { withSecretFileLock } from "./file-lock.js";
|
||||
import {
|
||||
KeyringSecretStore,
|
||||
parseAccount,
|
||||
|
|
@ -532,51 +533,76 @@ export async function absorbFileSecretsIntoKeyring(
|
|||
): Promise<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.
|
||||
//
|
||||
// The staged name carries the pid so two Inspectors starting together
|
||||
// cannot claim the same destination; whichever wins the rename does the
|
||||
// migration and the loser sees ENOENT and returns.
|
||||
// A per-attempt nonce, not just the pid. `recoverOrphanedSnapshots`
|
||||
// deliberately leaves an orphan in place when a live `secrets.json` also
|
||||
// exists — and a pid-only name is reusable across restarts (pid 1 on every
|
||||
// container start), so the next claim would `rename` straight over that
|
||||
// orphan and permanently discard secrets it may uniquely hold.
|
||||
const staged = `${filePath}.migrating-${process.pid}-${randomUUID()}`;
|
||||
try {
|
||||
await fs.rename(filePath, staged);
|
||||
} catch (err) {
|
||||
const code = (err as NodeJS.ErrnoException).code;
|
||||
if (code !== "ENOENT") {
|
||||
// Only ENOENT means someone else claimed it. Anything else — EACCES on
|
||||
// the directory, EROFS — leaves the file exactly where it is, and
|
||||
// returning quietly would select the keychain while file-backed
|
||||
// secrets sit there unreadable by anything, with nothing said.
|
||||
console.warn(
|
||||
`\n[mcp-inspector] Could not claim the secrets file at ${filePath} for migration into the OS keychain (${code ?? "unknown error"}), so it has been left in place. Its secrets are not visible to this session.`,
|
||||
);
|
||||
// Racy by construction, and that is fine: it can only be wrong by saying
|
||||
// "nothing here" about a file created a moment later, which is a file the
|
||||
// next run migrates — the same outcome as a writer that recreates the path
|
||||
// after the claim below. Everything that *acts* re-checks under the lock.
|
||||
if (!(await anythingToMigrate(filePath))) return;
|
||||
|
||||
// Orphan adoption and the claim below both move the live path around, so
|
||||
// they run under the same cross-process lock a `set` takes (#2082) —
|
||||
// otherwise a concurrent writer's atomic rename can land between the two
|
||||
// and be adopted, claimed, or clobbered depending on the interleaving.
|
||||
// The lock is released before the hand-off: that part reads a snapshot
|
||||
// nobody else can reach, and holding it across a keychain round-trip per
|
||||
// secret would block every writer for the duration of a migration.
|
||||
const claimed = await withSecretFileLock(filePath, async () => {
|
||||
// A crash between the claim and the delete leaves only
|
||||
// `secrets.json.migrating-<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;
|
||||
}
|
||||
return staged;
|
||||
});
|
||||
if (claimed === null) return;
|
||||
const staged = claimed;
|
||||
|
||||
const file = new FileSecretStore({ filePath: staged });
|
||||
// True only when every value reached the keychain *and* there was
|
||||
|
|
@ -625,6 +651,28 @@ export async function absorbFileSecretsIntoKeyring(
|
|||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Is there a `secrets.json`, or a snapshot orphaned by an interrupted
|
||||
* migration, worth taking the lock for?
|
||||
*
|
||||
* One `readdir` rather than a `stat` of the canonical path: an orphan is the
|
||||
* case where the live file is *absent* and there is still everything to
|
||||
* migrate, so checking only `secrets.json` would skip the recovery that
|
||||
* exists because skipping it loses every stored credential.
|
||||
*/
|
||||
async function anythingToMigrate(filePath: string): Promise<boolean> {
|
||||
const base = path.basename(filePath);
|
||||
try {
|
||||
return (await fs.readdir(path.dirname(filePath))).some(
|
||||
(name) => name === base || name.startsWith(`${base}.migrating-`),
|
||||
);
|
||||
} catch {
|
||||
// No storage directory yet — the first run on a fresh install, and the
|
||||
// path this check exists to keep quiet.
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Adopt a snapshot left behind by a process that died mid-migration.
|
||||
*
|
||||
|
|
|
|||
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": stale-takeover is exactly the problem the library has already solved, re-`stat`ing the lock after claiming it so the loser of a takeover race releases instead of proceeding. **The optimistic verify stays underneath it**, and is not redundant: read `M0`, apply, write `M1`, read back `M2`, re-apply onto whatever a concurrent writer left if they differ, bounded, with `set` throwing on non-convergence rather than returning as though the value were saved. The comparison is over the **whole map** — checking only your own entry passes in exactly the case that loses data, because yours is present and the other writer's is gone. A lock is advisory between the processes that take it, so the verify is what covers a writer outside this codebase (an editor, a restored backup, an Inspector predating #2082) and what covers the lock being *unavailable*: `withSecretFileLock` runs the body anyway, warning once, on a directory that cannot hold a lock file — this store exists for boxes where the usual mechanism is missing (#1848, #1905) and must not acquire a new way to be unavailable. Reads take no lock; `writeStoreFile` is atomic, so a reader sees the old file or the new one, never a torn one.
|
||||
- **Strict reads (`getStrict`)**: `get` is tolerant by contract, and that tolerance is wrong for exactly one caller — a migration that treats `null` as proof of absence and then _writes_. A transient read failure would look like "nothing there", and the write would replace a newer stored value with an older on-disk copy, inverting the keychain-wins rule the migration is built on. `getStrict` throws instead, and both plaintext migrations plus the keychain hand-off use it. Two traps this hit on the way in, both worth remembering: the seam must be forwarded by `DeferredSecretStore` (the store production actually uses, so without forwarding the strictness existed only in tests that injected a concrete store), and it must be implemented by _every_ store rather than falling back to `get` for the one it was introduced for.
|
||||
- **Bulk reads (`getMany`)**: rehydration asked field by field, and `FileSecretStore.get` reads and decrypts the entire file per call. scrypt at `N=16384` measures ~23ms, so an encrypted catalog spent ~450ms of pure key derivation on every `GET /api/servers` — a visible stall rather than a micro-optimization. The seam takes a **list of `{ serverId, fields }` across servers**, and both rehydration callers pass the whole catalog in one request, so `FileSecretStore` decrypts once per rehydration; stores for which per-field reads are already cheap (the keychain) fall back to parallel `get`. The cross-server shape is the point: a per-server version shipped first and left a 20-server catalog paying 20 serialized derivations, because both callers iterate servers — the same stall reached one server at a time instead of one field at a time.
|
||||
- **Durability gate on migration**: the plaintext-stripping migrations only delete the disk copy once the value is somewhere that outlives the process (`isDurable`). Against a session-scoped store, stripping would trade a secret that survives restarts for one that dies with the process — and it runs on an ordinary `GET`, so merely opening the app would destroy it.
|
||||
- **Hand-off when a keychain appears**: install libsecret after storing secrets in a file, and the next run selects the keychain and stops seeing them — still on disk, read by nothing, nothing visibly broken. `absorbFileSecretsIntoKeyring` copies them over on that run, under the same keychain-wins rule, and deletes the file **only** on complete success. Since the store takes no lock, the source is **claimed atomically** first: the live `secrets.json` is renamed to a unique snapshot (pid plus a per-attempt nonce, so a staging path can never be reused — pid 1 recurs on every container start), the migration reads only that snapshot, and a writer that recreates the live path is untouched and migrated on the next run. The snapshot is deleted only on a complete hand-off; otherwise it is restored with `link` + `unlink` rather than `rename`, since POSIX `rename` silently replaces its destination and would overwrite a newer live file. A snapshot left behind by a process that died mid-migration is adopted at startup — checking only the canonical path would otherwise report "nothing to migrate" while every stored credential quietly disappeared.
|
||||
- **Hand-off when a keychain appears**: install libsecret after storing secrets in a file, and the next run selects the keychain and stops seeing them — still on disk, read by nothing, nothing visibly broken. `absorbFileSecretsIntoKeyring` copies them over on that run, under the same keychain-wins rule, and deletes the file **only** on complete success. The claim runs under the same cross-process lock a `set` takes, and the source is **claimed atomically** within it: the live `secrets.json` is renamed to a unique snapshot (pid plus a per-attempt nonce, so a staging path can never be reused — pid 1 recurs on every container start), the migration reads only that snapshot, and a writer that recreates the live path is untouched and migrated on the next run. The snapshot is deleted only on a complete hand-off; otherwise it is restored with `link` + `unlink` rather than `rename`, since POSIX `rename` silently replaces its destination and would overwrite a newer live file. A snapshot left behind by a process that died mid-migration is adopted at startup — checking only the canonical path would otherwise report "nothing to migrate" while every stored credential quietly disappeared.
|
||||
- **Surfacing it**: the active store rides `GET /api/config` as a `secretStorage` descriptor and is stated in a permanent footer at the bottom of every dialog that accepts a secret: Client Settings (the enterprise IdP client secret), Server Settings (the per-server OAuth client secret and stdio `env:` values), and Server Config (stdio `env:` values). That third one was missed at first, which made it the one dialog taking secrets with no disclosure at all — so the count here is load-bearing rather than descriptive. A startup banner is seen once by whoever started the process; a toast is seen once; a dismissible banner is by design the thing a user dismisses before doing the work it describes. The descriptor is re-derived per request rather than cached, because `plaintext`/`pendingEncryption` describe bytes this very process changes.
|
||||
|
||||
- **Hard-cutover legacy behavior (per #1358 decision 4)**: files written by the one pre-#1358 build of v2/main have a nested `settings` block. `normalizeMcpServers` drops the node on read and logs a one-line warn including the server id; the persisted headers / metadata / timeouts / OAuth credentials are intentionally lost on first read. Users re-enter them via the settings form (or hand-edit the file into the flat shape). v2 has not shipped a stable release with the nested shape, so the blast radius is the small set of v2/main dogfooders who edited per-server settings between #1353 merging and this change.
|
||||
|
|
|
|||
Loading…
Reference in a new issue