modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 4

fix(auth): lock the snapshot during hand-off, keep quiet on ERELEASED (#2082)

Browse source 
Copilot review round 7 — two findings, both upheld.

**An in-progress migration snapshot could be adopted by another startup.**
The fast path counts every `*.migrating-*` as work and
`recoverOrphanedSnapshots` adopts them — including one whose owner is still
reading it. It link/unlinks the snapshot back to the live path and re-claims
it under a new name, the owner's hand-off then fails `ENOENT`, and if the
adopting process exits before copying, that healthy session starts with none
of those secrets, recoverable only on some later run.

Fixed the second way the reviewer suggested, not the first. Holding the main
lock across the hand-off also closes it and was tried — it broke "claims the
file atomically, so a later write is not deleted", because it blocks every
ordinary writer for the whole migration and fails their save past the retry
budget. That is a worse regression than the race, since #1950 guarantees a
write completing after the claim survives. The existing test caught it.

So the hand-off takes a lock on the **snapshot** instead, and
`recoverOrphanedSnapshots` skips an orphan whose lock is held. Liveness comes
from the lock rather than from the pid already in the filename: a pid both
outlives its process and recurs — pid 1 on every container start, which
#1950 documents as the reason the name carries a nonce — whereas a lock
expires by itself if its owner dies, so a genuinely abandoned snapshot
becomes adoptable with nothing to clean up. `isFileLockHeld` answers `false`
when it cannot tell, since the alternative to a wrong `false` is never
recovering an abandoned file.

**A compromised lock arrives as `ERELEASED`, and we told the operator to
delete the lock.** `setLockAsCompromised` marks the lock released and drops
its registry entry *before* calling `onCompromised`, so `release()` answers
`ERELEASED` without touching the filesystem — and the directory at that path
is by then the **winner's live lock**. "Remove it by hand" would destroy the
exclusion of a process that did nothing wrong. Handled separately and
silently; `onCompromised` has already reported what happened. A conditional
rather than an early return, since a `return` inside `finally` discards
whatever the body was returning or throwing.

Both new tests verified by mutation.

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 2026-08-23 11:54:47 -04:00
commit e956d49833
4 changed files with 153 additions and 25 deletions
Download patch file Download diff file Expand all files Collapse all files

8
clients/web/src/test/integration/auth/node/file-lock.test.ts
View file

@ -493,5 +493,13 @@ describe("withSecretFileLock reports what it cannot clean up", () => {
{ timeout: 20_000, interval: 250 },
);
});
// And it must NOT go on to tell the operator to delete the lock. Once the
// tick has fired, `release()` answers `ERELEASED` without touching the
// filesystem — the directory sitting at that path is then whoever took
// over, so "remove it by hand" would destroy the exclusion of a process
// that did nothing wrong. `onCompromised` has already said what happened.
expect(warnings()).not.toContain("by hand");
expect(warnings()).not.toContain("Could not release the lock");
}, 30_000);
});

40
clients/web/src/test/integration/auth/node/secret-store-selection.test.ts
View file

@ -788,6 +788,46 @@ describe("absorbFileSecretsIntoKeyring", () => {
},
);
it("does not adopt a snapshot another migration is still using (#2082)", async () => {
// A `*.migrating-*` file is not automatically an orphan: the process that
// staged it may still be reading it. Adopting one mid-hand-off links it
// back to the live path and re-claims it under a new name, so its owner's
// hand-off fails ENOENT — and if we exit before copying, that healthy
// session starts with none of those secrets.
//
// Liveness is the snapshot's own lock rather than the pid in its name: a
// pid outlives its process and recurs (pid 1 on every container start),
// whereas the lock expires by itself if the owner dies.
const inProgress = path.join(tmpDir, "secrets.json.migrating-999-abc");
await fs.writeFile(
inProgress,
JSON.stringify({
version: 1,
encryption: "none",
secrets: { "srv:oauthClientSecret": "still-migrating" },
}),
"utf-8",
);
process.env.MCP_INSPECTOR_SECRET_FILE = path.join(tmpDir, "secrets.json");
vi.spyOn(console, "warn").mockImplementation(() => {});
const properLockfile = (await import("proper-lockfile")).default;
const release = await properLockfile.lock(inProgress, {
realpath: false,
stale: 10_000,
});
const mod = await loadWithProbe(true);
const keyring = new InMemorySecretStore();
await mod.absorbFileSecretsIntoKeyring(keyring);
await release();
// Left exactly where its owner put it, and not migrated from under it.
expect(existsSync(inProgress)).toBe(true);
expect(existsSync(path.join(tmpDir, "secrets.json"))).toBe(false);
expect(await keyring.get("srv", "oauthClientSecret")).toBe(null);
});
it("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

56
core/auth/node/file-lock.ts
View file

@ -367,6 +367,30 @@ function acquire(
});
}
/**
* Is someone holding the lock on `filePath` right now?
*
* Liveness, not ownership: `check` reports a *stale* lock as unheld, so a
* holder that died stops counting on its own after {@link STALE_MS} with no
* bookkeeping to clean up. That is what makes this usable as an
* "is this in progress" test — a pid stamp cannot say it, since a pid both
* outlives its process and recurs (pid 1 on every container start).
*
* Answers `false` when it cannot tell. The callers use this to decide whether
* to *leave something alone*, and the alternative to a wrong `false` is
* refusing to ever recover an abandoned file.
*/
export async function isFileLockHeld(filePath: string): Promise<boolean> {
try {
return await properLockfile.check(path.resolve(filePath), {
realpath: false,
stale: STALE_MS,
});
} catch {
return false;
}
}
/**
* Run `fn` holding an exclusive cross-process lock on `filePath`.
*
@ -449,15 +473,29 @@ export async function withSecretFileLock<T>(
// 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.
warnOnce(
// 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.`,
);
//
// **`ERELEASED` is not a lock left behind.** When the library's refresh
// tick detects a takeover it marks the lock released and drops its
// registry entry *before* calling `onCompromised`, so our `release()`
// returns `ERELEASED` without touching the filesystem. The directory
// sitting there is then the **winner's live lock** — and the message
// below would tell the operator to delete it, destroying the exclusion
// of a process that did nothing wrong. `onCompromised` has already said
// what happened, so there is nothing to add.
//
// A conditional rather than an early `return`: a `return` inside
// `finally` discards whatever the body was returning or throwing.
if ((err as NodeJS.ErrnoException).code !== "ERELEASED") {
warnOnce(
// No branch on the *removal* error code, deliberately. Our own
// guard `stat`s before removing, so anything else 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 was wrong for all.
`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.`,
);
}
}
}
}

74
core/auth/node/secret-store-selection.ts
View file

@ -54,7 +54,7 @@ import {
readSecretFilePermissions,
tightenSecretFilePermissions,
} from "./file-secret-store.js";
import { withSecretFileLock } from "./file-lock.js";
import { isFileLockHeld, withSecretFileLock } from "./file-lock.js";
import {
KeyringSecretStore,
parseAccount,
@ -546,21 +546,18 @@ export async function absorbFileSecretsIntoKeyring(
// 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.
// Orphan adoption and the claim below run under the same cross-process lock
// a `set` takes (#2082) — otherwise a concurrent writer's atomic rename can
// land between the two and be adopted, claimed, or clobbered depending on
// the interleaving.
//
// The hand-off itself deliberately runs **outside** it, under a lock on the
// *snapshot* instead (see `handOffStagedSecrets`). Holding the main lock
// across a keychain round-trip per secret blocks every ordinary writer for
// the duration, and past the retry budget fails their save outright — which
// is a worse regression than the race it would close, since #1950
// guarantees a write completing after the claim survives.
//
// Wrapped because `withSecretFileLock` **throws** when another process
// holds the lock past its retry budget, and this function's contract is
// that it never throws: it is awaited directly by both `resolveSecretStore`
// branches, so a stuck writer would fail store resolution and with it the
// whole session. Refusing is the right answer for a `set` — the user is
// waiting on that value — and the wrong one here, where the file simply
// stays put and the next run migrates it.
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
@ -619,8 +616,45 @@ export async function absorbFileSecretsIntoKeyring(
return null;
});
if (claimed === null) return;
const staged = claimed;
await handOffStagedSecrets(claimed, filePath, keyring);
}
/**
* Move a claimed snapshot's secrets into the keychain, then dispose of it.
*
* Runs under a lock on the **snapshot**, not on `secrets.json`. That is what
* stops a second Inspector's {@link recoverOrphanedSnapshots} from adopting a
* migration still in progress — it link/unlinks the snapshot back to the live
* path and re-claims it, our hand-off then fails `ENOENT`, and if that second
* process exits before copying, the healthy first session starts with none of
* those secrets. Locking the snapshot says "in progress" in a way a filename
* cannot, and expires by itself if this process dies.
*
* Holding the *main* lock here instead would also close it, and was tried:
* it blocks every ordinary writer for the whole migration and fails their
* save past the retry budget, breaking #1950's guarantee that a write
* completing after the claim survives.
*/
async function handOffStagedSecrets(
staged: string,
filePath: string,
keyring: SecretStore,
): Promise<void> {
await withSecretFileLock(staged, () =>
handOffStagedSecretsLocked(staged, filePath, keyring),
).catch((err: Error) => {
console.warn(
`\n[mcp-inspector] Could not lock ${staged} to migrate it into the OS keychain (${err.message}), so it has been left in place. The next run will try again.`,
);
});
}
/** {@link handOffStagedSecrets}'s body, with the snapshot's lock held. */
async function handOffStagedSecretsLocked(
staged: string,
filePath: string,
keyring: SecretStore,
): Promise<void> {
const file = new FileSecretStore({ filePath: staged });
// True only when every value reached the keychain *and* there was
// something to move — the one case where the snapshot is redundant.
@ -717,6 +751,14 @@ async function recoverOrphanedSnapshots(filePath: string): Promise<void> {
}
for (const name of names) {
const orphan = path.join(dir, name);
// Not an orphan at all — another Inspector is migrating it right now, and
// adopting it would link it back to the live path and re-claim it under a
// new name while its owner is still reading it. That owner's hand-off
// then fails `ENOENT`, and if we exit before copying, its healthy session
// starts with none of those secrets. The lock expires on its own if that
// process dies, so a genuinely abandoned snapshot becomes adoptable
// without anything to clean up.
if (await isFileLockHeld(orphan)) continue;
try {
await fs.link(orphan, filePath);
await fs.rm(orphan, { force: true });