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

fix(auth): hold the snapshot lock across the main lock's release (#2082)

Browse source 
Copilot review round 8 — two findings, both upheld. The first is a gap in
round 7's own fix.

**The snapshot was unprotected between two awaits.** Returning from
`withSecretFileLock(filePath, ...)` releases the main lock *before*
`handOffStagedSecrets` acquired the snapshot's, so a second startup could
take the main lock in that window, see the staged file unlocked, and adopt
and re-claim it — leaving this process reading a path that no longer exists.
Narrower than the race round 7 closed, but the same one.

`openSecretFileLock` is now split out of `withSecretFileLock`: it takes the
lock and returns its release, so a caller can hold one *across* another's
release. The claim acquires the snapshot's lock before returning — inside the
main lock — and the hand-off then runs outside the main lock, still holding
it. `withSecretFileLock` is a thin wrapper over the same function, so which
failures refuse and which degrade is decided in one place and the two entry
points cannot drift.

Locking the snapshot degrades rather than refusing (`null` → hand off
anyway): the exposure is one uniquely-named snapshot, which is a better trade
than refusing to migrate at all on a box that cannot lock.

**Both migration scans matched the lock directories themselves.**
`secrets.json.migrating-<pid>-<uuid>.lock` passes the plain prefix test, and
treating it as a snapshot is self-sustaining damage: the liveness probe asks
about a nonexistent `<name>.lock.lock` and so answers "not held", recovery
tries to hard-link a *directory* onto the secrets path, fails, and prints the
orphan warning. Because a liveness *check* never clears a stale lock
directory — only a would-be acquirer does — that false migration repeats on
every startup forever, including long after the real snapshot was recovered.
`isSnapshotName` now excludes `.lock` for both `anythingToMigrate` and
`recoverOrphanedSnapshots`.

New test 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 12:26:52 -04:00
commit edddc6c9cc
3 changed files with 112 additions and 49 deletions
Download patch file Download diff file Expand all files Collapse all files

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

@ -828,6 +828,28 @@ describe("absorbFileSecretsIntoKeyring", () => {
expect(await keyring.get("srv", "oauthClientSecret")).toBe(null);
});
it("ignores a snapshot's own lock directory (#2082)", async () => {
// `secrets.json.migrating-<pid>-<uuid>.lock` matches the plain prefix
// test, and treating it as a snapshot is self-sustaining damage: the
// liveness probe asks about a nonexistent `<name>.lock.lock` and answers
// "not held", recovery tries to hard-link a *directory* onto the secrets
// path, fails, and prints the orphan warning — every startup, forever,
// since a liveness *check* never clears a stale lock directory.
const strayLock = path.join(tmpDir, "secrets.json.migrating-999-abc.lock");
await fs.mkdir(strayLock);
process.env.MCP_INSPECTOR_SECRET_FILE = path.join(tmpDir, "secrets.json");
const warn = vi.spyOn(console, "warn").mockImplementation(() => {});
const mod = await loadWithProbe(true);
await mod.absorbFileSecretsIntoKeyring(new InMemorySecretStore());
// Not mistaken for a snapshot: nothing said, nothing linked, and the
// directory left exactly where it was.
expect(warn).not.toHaveBeenCalled();
expect(existsSync(strayLock)).toBe(true);
expect(existsSync(path.join(tmpDir, "secrets.json"))).toBe(false);
});
it("still adopts an orphan when the live file is absent (#2082)", async () => {
// The fast path above must not be a `stat` of `secrets.json`: an
// interrupted migration leaves *only* the snapshot, which is precisely

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

@ -402,10 +402,22 @@ export async function isFileLockHeld(filePath: string): Promise<boolean> {
* 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>(
/**
* Take the lock and hand back its release, or `null` when locking is
* unavailable here and the caller should proceed unprotected.
*
* Split out of {@link withSecretFileLock} so a caller that must hold a lock
* **across** another lock's release can do so — the keychain hand-off needs
* exactly that, see `absorbFileSecretsIntoKeyring`. Everything about *which*
* failures refuse and which degrade lives here, so both entry points cannot
* drift on that question.
*
* Throws {@link SecretStoreUnavailableError} when the lock is held or stuck;
* returns `null` when it could not be created at all.
*/
export async function openSecretFileLock(
filePath: string,
fn: () => Promise<T>,
): Promise<T> {
): Promise<(() => Promise<void>) | null> {
const target = path.resolve(filePath);
// Create the parent directory before locking, not after. `writeStoreFile`
// creates it on the way to writing the secrets file, but that runs *inside*
@ -427,7 +439,7 @@ export async function withSecretFileLock<T>(
`The lock on the secrets file at ${target} was taken over by another process while a write was in progress, so it was left alone rather than removed. If a secret you just saved is missing, save it again.`,
),
);
let release: (() => Promise<void>) | undefined;
let release: () => Promise<void>;
try {
release = await acquire(target, 0, fsShim).catch((err: unknown) => {
// **Retries are for contention, and only for contention.**
@ -462,11 +474,9 @@ export async function withSecretFileLock<T>(
warnOnce(
`Could not take a lock on the secrets file at ${target} (${describeError(err)}), so writes to it are not protected against another process writing at the same moment.`,
);
return fn();
return null;
}
try {
return await fn();
} finally {
return async () => {
try {
await release();
} catch (err) {
@ -476,26 +486,35 @@ export async function withSecretFileLock<T>(
//
// **`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.`,
);
}
// registry entry *before* calling `onCompromised`, so this returns
// `ERELEASED` without touching the filesystem. The directory sitting
// there is then the **winner's live lock** — and the message below
// would tell the operator to delete it, destroying the exclusion of a
// process that did nothing wrong. `onCompromised` already said what
// happened, so there is nothing to add.
if ((err as NodeJS.ErrnoException).code === "ERELEASED") return;
warnOnce(
// 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 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.`,
);
}
};
}
export async function withSecretFileLock<T>(
filePath: string,
fn: () => Promise<T>,
): Promise<T> {
const release = await openSecretFileLock(filePath);
if (release === null) return fn();
try {
return await fn();
} finally {
await release();
}
}

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

@ -54,7 +54,11 @@ import {
readSecretFilePermissions,
tightenSecretFilePermissions,
} from "./file-secret-store.js";
import { isFileLockHeld, withSecretFileLock } from "./file-lock.js";
import {
isFileLockHeld,
openSecretFileLock,
withSecretFileLock,
} from "./file-lock.js";
import {
KeyringSecretStore,
parseAccount,
@ -604,7 +608,17 @@ export async function absorbFileSecretsIntoKeyring(
}
return null;
}
return staged;
// **Acquire the snapshot's lock before returning**, i.e. before the main
// lock is released. Taking it afterwards leaves a gap in which a second
// startup can take the main lock, see the staged file unlocked, and adopt
// and re-claim it — leaving this process reading a path that no longer
// exists, which is the very race the snapshot lock exists to prevent.
//
// `null` means locking is unavailable here; the hand-off still runs. The
// exposure is one unique nonce-named snapshot, which is a far better
// trade than refusing to migrate at all on a box that cannot lock.
const releaseSnapshot = await openSecretFileLock(staged).catch(() => null);
return { staged, releaseSnapshot };
// `withSecretFileLock` rejects only with the `SecretStoreUnavailableError`
// it constructs itself (a lock it could not create degrades instead of
// throwing), so the cast is over a value produced one call away rather
@ -616,7 +630,14 @@ export async function absorbFileSecretsIntoKeyring(
return null;
});
if (claimed === null) return;
await handOffStagedSecrets(claimed, filePath, keyring);
// Outside the main lock, still holding the snapshot's: a keychain
// round-trip per secret must not block ordinary writers (#1950 guarantees a
// write completing after the claim survives).
try {
await handOffStagedSecrets(claimed.staged, filePath, keyring);
} finally {
await claimed.releaseSnapshot?.();
}
}
/**
@ -639,21 +660,6 @@ 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
@ -702,6 +708,22 @@ async function handOffStagedSecretsLocked(
}
}
/**
* Is `name` a migration snapshot, as opposed to the lock directory beside one?
*
* The `.lock` exclusion is load-bearing, not tidiness. `secrets.json.lock`
* and `secrets.json.migrating-<pid>-<uuid>.lock` both match the plain prefix
* test, and treating the latter as a snapshot is self-sustaining damage: the
* liveness probe asks about a nonexistent `<name>.lock.lock` and so answers
* "not held", recovery then tries to hard-link a *directory* onto the secrets
* path, fails, and prints the orphan warning. Because a stale lock directory
* is never removed by a liveness *check* — only a would-be acquirer clears
* one — that false migration repeats on every startup forever, including
* after the real snapshot has long since been recovered.
*/
const isSnapshotName = (name: string, base: string): boolean =>
name.startsWith(`${base}.migrating-`) && !name.endsWith(".lock");
/**
* Is there a `secrets.json`, or a snapshot orphaned by an interrupted
* migration, worth taking the lock for?
@ -715,7 +737,7 @@ 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-`),
(name) => name === base || isSnapshotName(name, base),
);
} catch (err) {
const code = (err as NodeJS.ErrnoException).code;
@ -742,10 +764,10 @@ async function anythingToMigrate(filePath: string): Promise<boolean> {
*/
async function recoverOrphanedSnapshots(filePath: string): Promise<void> {
const dir = path.dirname(filePath);
const prefix = `${path.basename(filePath)}.migrating-`;
const base = path.basename(filePath);
let names: string[];
try {
names = (await fs.readdir(dir)).filter((n) => n.startsWith(prefix));
names = (await fs.readdir(dir)).filter((n) => isSnapshotName(n, base));
} catch {
return; // No directory yet, or unreadable — nothing to recover.
}