From 09e1564672c35483cc50afdaf91745d8ea145d2a Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 11:31:37 -0400 Subject: [PATCH] fix(auth): refuse on a stuck lock, capture identity at the mkdir seam (#2082) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review round 6 — five findings (two inline, three suppressed), all valid. **A stale lock that cannot be cleared caused unlocked writes.** `acquireLock` does not only *create* directories: on finding a stale one it removes it and retries, and that removal can fail — `ENOTEMPTY` for a lock with anything inside it, `EACCES`/`EROFS` for one we may not touch. Those surface as ordinary non-`ELOCKED` errors, which the probe read as "locks do not work here" and degraded on. So every Inspector on the box quietly bypassed the *same* stuck lock and raced its writes — while the release-failure message was telling the operator saves would keep failing until they cleared it. `isStuckOrHeld` now discriminates on **whether the lock directory exists** rather than on an errno taxonomy: if it is there, something holds it and we refuse; if it is not, we genuinely could not create one and degrading is the documented trade (#1848, #1905). Reading the state the decision is about avoids enumerating error codes per platform — and an enumeration is exactly what let `ENOTEMPTY` through. **Identity was captured after the acquire promise settled.** That span includes the library's own `utimes`/`stat` probe, so a waiter replacing our directory inside it would have us record *the winner's* identity as our own, after which the removal guard would accept and delete their lock. It is now recorded in the `mkdir` callback — the moment the directory becomes ours, and synchronous with respect to this process, so there is no window. **The expiry advice was still wrong for `EACCES`/`EPERM`/`EROFS`.** Since the guard `stat`s before removing, everything reaching the release catch is an `rmdir` that was refused — and stale takeover reclaims through that same `rmdir`. The `ENOENT` arm was unreachable for the same reason, so the branch was dead code: removed, and the advice is now unconditional. **`proper-lockfile` was missing from `NODE_ONLY_OPTIMIZE_DEPS_EXCLUDE`.** A fourth externalization surface — the tsup lists configure the production bundles, not `vite dev`, whose dep scanner would otherwise walk its CJS/`graceful-fs`/`signal-exit` graph. Added, and `vite-base-config.test.ts` now asserts it along with `chokidar` and `@napi-rs/keyring`, which were also absent from that assertion. **A test was named for something it did not exercise.** "does not treat an unlistable directory as an empty one" was driving `ENOTDIR`, which is deliberately treated *as* the fresh-install case, so the actual new behaviour was untested. Split in two: one for `ENOTDIR`, and a real `EACCES` one using a `--x` directory — `readdir` denied, a known name still reachable, which is the asymmetry that makes "unlistable" different from "empty". Driven with a real mode rather than a stub, since `vi.spyOn` cannot redefine an ESM namespace export; skipped as root and on Windows, where the mode would not bite and the test would assert nothing. Both new tests verified by mutation. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H Signed-off-by: cliffhall --- clients/web/server/vite-base-config.ts | 8 ++ .../integration/auth/node/file-lock.test.ts | 31 +++++++ .../auth/node/secret-store-selection.test.ts | 48 +++++++++-- .../server/vite-base-config.test.ts | 6 ++ core/auth/node/file-lock.ts | 83 ++++++++++++------- 5 files changed, 140 insertions(+), 36 deletions(-) diff --git a/clients/web/server/vite-base-config.ts b/clients/web/server/vite-base-config.ts index c351f473..3d9c1a90 100644 --- a/clients/web/server/vite-base-config.ts +++ b/clients/web/server/vite-base-config.ts @@ -34,6 +34,14 @@ const NODE_ONLY_OPTIMIZE_DEPS_EXCLUDE = [ // excluding it keeps Vite's dep scanner from chasing into the // platform-specific binaries during dev startup. "@napi-rs/keyring", + // `proper-lockfile` is reached only through `core/auth/node/file-lock.ts` + // — the secrets file's cross-process lock (#2082) — which the Hono + // `/api/servers` handlers pull in via `core/auth/node/file-secret-store.ts`. + // Same node-only import chain as `atomically` above, and the same reason: + // it is CJS with a `graceful-fs`/`signal-exit`/`retry` graph that Vite's + // dev scanner has no business walking. Note the tsup `external` lists do + // **not** cover this — they configure the production bundles, not `vite dev`. + "proper-lockfile", ] as const; export function getViteBaseConfig() { diff --git a/clients/web/src/test/integration/auth/node/file-lock.test.ts b/clients/web/src/test/integration/auth/node/file-lock.test.ts index c42dbd3d..0f10bc5b 100644 --- a/clients/web/src/test/integration/auth/node/file-lock.test.ts +++ b/clients/web/src/test/integration/auth/node/file-lock.test.ts @@ -305,6 +305,37 @@ describe("withSecretFileLock degrades rather than failing", () => { expect(existsSync(target)).toBe(false); }, 90_000); + it("refuses rather than degrading when a stale lock cannot be cleared", async () => { + // `acquireLock` does not only *create* directories — on finding a stale + // one it removes it and retries, and that removal can fail. A stale lock + // with anything inside it fails `ENOTEMPTY`, which is not `ELOCKED`, and + // treating every non-`ELOCKED` error as "locks do not work here" meant + // every Inspector on the box quietly bypassed the *same* stuck lock and + // raced its writes — while the release-failure message was telling the + // operator saves would keep failing until they cleared it. + const target = filePath(); + const lockPath = `${target}.lock`; + await fs.mkdir(path.dirname(target), { recursive: true }); + await fs.mkdir(lockPath); + await fs.writeFile(`${lockPath}/stray`, "", "utf-8"); + // Backdated so it reads as stale — which is what sends `acquireLock` down + // the remove-and-retry path rather than straight to `ELOCKED`. + const longDead = new Date(Date.now() - 60_000); + await fs.utimes(lockPath, longDead, longDead); + + const store = new FileSecretStore({ filePath: target }); + const err = await store + .set("srv", "env:MINE", "1") + .catch((e: unknown) => e); + + expect(err).toBeInstanceOf(SecretStoreUnavailableError); + expect((err as Error).message).toMatch(/was not saved/); + // Nothing written behind the stuck lock, and no "unprotected" warning: + // this is a refusal, not a degrade. + expect(existsSync(target)).toBe(false); + expect(warnings()).not.toContain("not protected"); + }, 90_000); + it("takes over the lock of a holder that died, rather than failing the save", async () => { // The invariant behind refusing on `ELOCKED`: refusing is only defensible // because a *crashed* holder resolves on its own first. `RETRY` therefore diff --git a/clients/web/src/test/integration/auth/node/secret-store-selection.test.ts b/clients/web/src/test/integration/auth/node/secret-store-selection.test.ts index e9cb8f7e..21a6ea3d 100644 --- a/clients/web/src/test/integration/auth/node/secret-store-selection.test.ts +++ b/clients/web/src/test/integration/auth/node/secret-store-selection.test.ts @@ -735,13 +735,11 @@ describe("absorbFileSecretsIntoKeyring", () => { expect(existsSync(`${filePath}.lock`)).toBe(false); }); - it("does not treat an unlistable directory as an empty one (#2082)", async () => { - // Only "there is no directory" proves the fresh-install case. A path - // whose parent is a *file* answers `readdir` with ENOTDIR — the other - // shape of "nothing there" — and must be treated the same, whereas an - // EACCES directory that denies listing while still permitting access to - // the known `secrets.json` must not be, or the keychain is selected and - // those secrets go invisible with nothing said. + it("treats ENOTDIR as the fresh-install case, like ENOENT (#2082)", async () => { + // A path whose parent is a *file* answers `readdir` with ENOTDIR — the + // other shape of "there is no directory here" — so it takes the same + // silent path as a missing one. The *unlistable* case is different and is + // covered by the test below. await fs.writeFile(path.join(tmpDir, "not-a-dir"), "", "utf-8"); const filePath = path.join(tmpDir, "not-a-dir", "secrets.json"); process.env.MCP_INSPECTOR_SECRET_FILE = filePath; @@ -754,6 +752,42 @@ describe("absorbFileSecretsIntoKeyring", () => { expect(warn).not.toHaveBeenCalled(); }); + // Root bypasses POSIX permission checks, and Windows does not model them + // the same way — in either case the directory below stays listable and the + // test would assert nothing. + const canDenyListing = + process.platform !== "win32" && process.getuid?.() !== 0; + + it.skipIf(!canDenyListing)( + "does not treat an unlistable directory as an empty one (#2082)", + async () => { + // The case ENOTDIR does *not* cover, and the reason the catch narrowed + // to ENOENT/ENOTDIR rather than swallowing everything. A directory with + // `--x` permission denies `readdir` with EACCES while still permitting + // access to a *known* name inside it — so reading EACCES as "nothing to + // migrate" selects the keychain and leaves those secrets invisible with + // nothing said. That asymmetry is real POSIX behaviour, which is why + // this drives it with a real mode rather than a stubbed `readdir`. + const filePath = await seedFile({ "srv:oauthClientSecret": "from-file" }); + process.env.MCP_INSPECTOR_SECRET_FILE = filePath; + vi.spyOn(console, "warn").mockImplementation(() => {}); + // Write + execute, no read: `readdir` fails, `stat`/`open`/`rename` of a + // known name still work — which is exactly what the migration needs. + await fs.chmod(tmpDir, 0o300); + try { + const mod = await loadWithProbe(true); + const keyring = new InMemorySecretStore(); + await mod.absorbFileSecretsIntoKeyring(keyring); + + // It went on and migrated, rather than silently skipping. + expect(await keyring.get("srv", "oauthClientSecret")).toBe("from-file"); + } finally { + // Restore before `afterEach`, which needs to list it to remove it. + await fs.chmod(tmpDir, 0o700); + } + }, + ); + it("still adopts an orphan when the live file is absent (#2082)", async () => { // The fast path above must not be a `stat` of `secrets.json`: an // interrupted migration leaves *only* the snapshot, which is precisely diff --git a/clients/web/src/test/integration/server/vite-base-config.test.ts b/clients/web/src/test/integration/server/vite-base-config.test.ts index fc305223..90c39abd 100644 --- a/clients/web/src/test/integration/server/vite-base-config.test.ts +++ b/clients/web/src/test/integration/server/vite-base-config.test.ts @@ -21,8 +21,14 @@ describe("getViteBaseConfig", () => { expect.arrayContaining([ "@modelcontextprotocol/client/stdio", "atomically", + "chokidar", "cross-spawn", "which", + "@napi-rs/keyring", + // #2082 — reached through `core/auth/node/file-lock.ts`. The tsup + // `external` lists cover the production bundles, not `vite dev`, so + // a node-only dependency has to be named in both places. + "proper-lockfile", ]), ); }); diff --git a/core/auth/node/file-lock.ts b/core/auth/node/file-lock.ts index 11db6a38..f9f8f5f5 100644 --- a/core/auth/node/file-lock.ts +++ b/core/auth/node/file-lock.ts @@ -162,6 +162,32 @@ export function resetFileLockWarnings(): void { warned.clear(); } +/** + * Is this failure "the lock is there and we could not have it", as opposed to + * "locks do not work here"? + * + * `ELOCKED` is the obvious member, but not the only one, and the difference + * decides whether a save **refuses** or **degrades to an unlocked write** — + * so getting it wrong is not cosmetic. `acquireLock` does not only *create* + * directories: on finding a stale one it removes it and retries, and that + * removal can fail — `ENOTEMPTY` for a lock with anything inside it, `EACCES` + * or `EROFS` for one we may not touch. Those surface as ordinary non-`ELOCKED` + * errors, and treating them as infrastructure failures meant every Inspector + * on the box quietly bypassed the *same* stuck lock and raced its writes — + * while the release-failure message was telling the operator saves would keep + * failing until they cleared it. + * + * The discriminator is the lock directory itself rather than an errno + * taxonomy: **if it exists, something holds it and we must not proceed**; if + * it does not, we genuinely could not create one and degrading is the + * documented trade (#1848, #1905). That reads the state the decision is + * actually about, instead of enumerating error codes per platform and + * filesystem — which is the enumeration that let `ENOTEMPTY` through. + */ +function isStuckOrHeld(err: unknown, lockPath: string): boolean { + return isHeldElsewhere(err) || identifySync(lockPath) !== null; +} + /** * Did `proper-lockfile` decline because someone else holds the lock? * @@ -266,6 +292,23 @@ function guardedFs( }; return { ...nodeFs, + // **Identity is captured here, not after `lock()` resolves.** `mkdir` is + // the moment the directory becomes ours, and it is synchronous with + // respect to this process: recording it in the callback, before yielding, + // leaves no window. Capturing after the acquire promise settled — as this + // did originally — spans the library's own `utimes`/`stat` probe, and a + // waiter replacing our directory inside that span would have us record + // *the winner's* identity as our own, after which the guard below would + // cheerfully delete their lock. + mkdir: ( + p: string, + cb: (err: NodeJS.ErrnoException | null) => void, + ): void => { + nodeFs.mkdir(p, (err) => { + if (!err) owned.id = identifySync(lockPath); + cb(err); + }); + }, // Reported as success when refused: the library's bookkeeping should // forget this lock either way. Leaving it registered would hand the // `signal-exit` handler a record pointing at the winner's directory. @@ -286,26 +329,6 @@ function guardedFs( }; } -/** - * What to tell an operator about a lock we could not remove. - * - * The blanket "it expires on its own" is **false for `ENOTEMPTY`**, and that - * is the reachable case rather than a hypothetical: stale takeover reclaims a - * lock through the same `rmdir`, which also cannot remove a non-empty - * directory. So a lock directory with anything inside it is not cleaned up by - * the staleness mechanism, by us, or by the next writer — it stays until - * somebody deletes it, and every later save fails `ELOCKED` against it. - * - * Any other failure leaves a directory the refresher has stopped touching, so - * the stale path really does reclaim it after {@link STALE_MS}. - */ -function releaseAdvice(err: unknown, target: string): string { - const code = (err as NodeJS.ErrnoException).code; - return code === "ENOTEMPTY" - ? `It has something inside it, which stale takeover cannot clear either, so saves will keep failing until you remove ${lockPathOf(target)} by hand.` - : `It expires on its own after ${STALE_MS / 1000}s.`; -} - /** * One `proper-lockfile` acquire against `target`, with the given retry policy. * @@ -390,7 +413,7 @@ export async function withSecretFileLock( // time, on every save, before degrading. Probing once with no retries // separates the two answers at the cost of one syscall: `ELOCKED` is // worth waiting out, an infrastructure failure is not. - if (!isHeldElsewhere(err)) throw err; + if (!isStuckOrHeld(err, lockPathOf(target))) throw err; return acquire(target, RETRY, fsShim); }); } catch (err) { @@ -402,9 +425,9 @@ export async function withSecretFileLock( // {@link RETRY}), a holder still there is not one that crashed; it is one // that is stuck. Refusing loses nothing — `set` reports it and the user // retries — whereas proceeding can lose a secret while reporting success. - if (isHeldElsewhere(err)) { + if (isStuckOrHeld(err, lockPathOf(target))) { throw new SecretStoreUnavailableError( - `Could not save to the secrets file at ${target}: another process has held the lock on it for the ${Math.round(RETRY_BUDGET_MS / 1000)} seconds this save waited. Its secrets are intact; the value you just entered was not saved. If no other Inspector is running, remove ${target}.lock and try again.`, + `Could not save to the secrets file at ${target}: its lock (${lockPathOf(target)}) was still held after the ${Math.round(RETRY_BUDGET_MS / 1000)} seconds this save waited. Its secrets are intact; the value you just entered was not saved. If no other Inspector is running, remove that lock and try again.`, ); } // Everything else is the lock being *unavailable* rather than held — a @@ -417,9 +440,6 @@ export async function withSecretFileLock( ); return fn(); } - // Now that we hold it, record which directory is ours so the guard above - // can refuse to delete anyone else's. - owned.id = identifySync(lockPathOf(target)); try { return await fn(); } finally { @@ -428,10 +448,15 @@ export async function withSecretFileLock( } catch (err) { // The body already ran and its result is being returned; turning a // completed save into a failure at teardown would be the wrong trade. - // But a lock left behind is worth explaining, and the two reasons need - // different advice — see `releaseAdvice`. + // But a lock left behind is worth explaining. warnOnce( - `Could not release the lock on the secrets file at ${target} (${describeError(err)}). ${releaseAdvice(err, target)}`, + // No branch on the error code, deliberately. Our own guard `stat`s + // before removing, so anything reaching here is an `rmdir` that was + // refused — and **stale takeover reclaims through that same `rmdir`**, + // so whatever blocked ours blocks that too. `ENOTEMPTY` is the + // reachable one; `EACCES`, `EPERM` and `EROFS` behave identically. + // Promising expiry for "everything else" was wrong for all of them. + `Could not release the lock on the secrets file at ${target} (${describeError(err)}). Stale takeover reclaims a lock through the same directory removal, so whatever blocked this blocks that too: saves will keep failing until you remove ${lockPathOf(target)} by hand.`, ); } }