Skip to content

Commit fd1fa06

Browse files
authored
Merge pull request #2088 from modelcontextprotocol/v2/feat/2082-file-secret-store-lock
feat(auth): give the secrets file a real cross-process lock
2 parents d459a08 + 89b01d0 commit fd1fa06

17 files changed

Lines changed: 1591 additions & 91 deletions

‎.github/copilot-instructions.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@ Both exist and do different jobs. Theme files (`src/theme/<Component>.ts`) custo
8080
- 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.
8181
- **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.
8282
- **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.
83+
- **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.
8384

8485
## Tests and the coverage gate
8586

‎AGENTS.md‎

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,28 @@ v2/main/
6868
│ │ │ # throws and the routes turn into a 503, and the keychain
6969
│ │ │ # probe), file-secret-store.ts (0600 JSON, AES-256-GCM when
7070
│ │ │ # MCP_INSPECTOR_SECRET_KEY is set — refuses to overwrite a
71-
│ │ │ # file it cannot decrypt rather than destroying it), and
71+
│ │ │ # file it cannot decrypt rather than destroying it),
72+
│ │ │ # file-lock.ts (withSecretFileLock: the cross-process
73+
│ │ │ # mutual exclusion #2082 settled on — proper-lockfile,
74+
│ │ │ # borrowed rather than hand-rolled. Read its header before
75+
│ │ │ # citing it: it makes two LIVE Inspectors exclusive, and
76+
│ │ │ # does NOT make stale takeover single-winner. proper-lockfile
77+
│ │ │ # detects a takeover only on its 5s refresh tick, which an
78+
│ │ │ # ordinary sub-second mutation never reaches, and its release
79+
│ │ │ # is an unconditional rmdir (as is its signal-exit handler) —
80+
│ │ │ # so withSecretFileLock passes a GUARDED options.fs, the one
81+
│ │ │ # seam both removal paths share, refusing to delete a lock
82+
│ │ │ # that is no longer ours (inode+birthtime). That NARROWS the
83+
│ │ │ # window, it does not close it — still check-then-act across
84+
│ │ │ # processes. BEST-EFFORT throughout: do not write that a
85+
│ │ │ # compromised holder is always told, or that the winner's
86+
│ │ │ # lock is always preserved.
87+
│ │ │ # DEGRADES when no lock CAN be taken (read-only $HOME etc),
88+
│ │ │ # since this store exists for boxes missing the usual
89+
│ │ │ # mechanism; but THROWS on ELOCKED — a lock held by a live
90+
│ │ │ # writer is evidence the lock works, not licence to bypass
91+
│ │ │ # it — after waiting past the stale window),
92+
│ │ │ # and
7293
│ │ │ # secret-store-selection.ts (the POLICY: explicit
7394
│ │ │ # MCP_INSPECTOR_SECRET_STORE wins, else probe the keychain,
7495
│ │ │ # 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
228249
- A package only the tests, the test servers, or the build tooling need belongs in **`devDependencies`** — `express`, added there by #1970.
229250
- `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.
230251

252+
**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.
253+
231254
**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.
232255

233256
**`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.

‎README.md‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -504,9 +504,17 @@ Setting the passphrase later is safe — the next write upgrades an existing pla
504504

505505
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.
506506

507-
**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.
507+
**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.
508508

509-
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.
509+
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.
510+
511+
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.
512+
513+
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.
514+
515+
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.
516+
517+
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.
510518

511519
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.
512520

‎clients/cli/tsup.config.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,13 @@ export default defineConfig({
2121
noExternal: [/^@inspector\/core/],
2222
external: [
2323
"@napi-rs/keyring",
24+
// Root-declared (see the repo's dependency-placement rule) and CJS, which
25+
// is the combination that bites: tsup externalizes what the *client's*
26+
// package.json declares, so a root-only dependency is bundled unless named
27+
// here — and inlining a CJS module into an ESM bundle leaves esbuild's
28+
// `Dynamic require of "path" is not supported` shim, which throws at
29+
// import time and takes the whole binary down before it parses a flag.
30+
"proper-lockfile",
2431
"@modelcontextprotocol/client",
2532
"@modelcontextprotocol/core",
2633
"commander",

‎clients/tui/tsup.config.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,13 @@ export default defineConfig({
120120
"@modelcontextprotocol/client",
121121
"@modelcontextprotocol/core",
122122
"@napi-rs/keyring",
123+
// Root-declared (see the repo's dependency-placement rule) and CJS, which
124+
// is the combination that bites: tsup externalizes what the *client's*
125+
// package.json declares, so a root-only dependency is bundled unless named
126+
// here — and inlining a CJS module into an ESM bundle leaves esbuild's
127+
// `Dynamic require of "path" is not supported` shim, which throws at
128+
// import time and takes the whole binary down before it parses a flag.
129+
"proper-lockfile",
123130
],
124131
esbuildPlugins: [inkFormLabelPatch],
125132
esbuildOptions(options) {

‎clients/web/server/vite-base-config.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,14 @@ const NODE_ONLY_OPTIMIZE_DEPS_EXCLUDE = [
3434
// excluding it keeps Vite's dep scanner from chasing into the
3535
// platform-specific binaries during dev startup.
3636
"@napi-rs/keyring",
37+
// `proper-lockfile` is reached only through `core/auth/node/file-lock.ts`
38+
// — the secrets file's cross-process lock (#2082) — which the Hono
39+
// `/api/servers` handlers pull in via `core/auth/node/file-secret-store.ts`.
40+
// Same node-only import chain as `atomically` above, and the same reason:
41+
// it is CJS with a `graceful-fs`/`signal-exit`/`retry` graph that Vite's
42+
// dev scanner has no business walking. Note the tsup `external` lists do
43+
// **not** cover this — they configure the production bundles, not `vite dev`.
44+
"proper-lockfile",
3745
] as const;
3846

3947
export function getViteBaseConfig() {

0 commit comments

Comments
 (0)