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): address Copilot review round 3 on the secret-store PR

Browse source 
Nine findings (six inline, three suppressed), all taken. Two were serious.

**The memory fallback made the migrations destructive.** Both migrations —
mcp.json in the remote server, client.json in `node-persistence` — lift a
plaintext secret off disk, write it to the secret store, and then delete it
from the file. That trade is safe only while the store is at least as
durable as the disk, which held by construction while `InMemorySecretStore`
was a test double. Making it a production fallback broke it: on an
unmounted container a plain `GET /api/servers` would have moved a user's
existing secrets into RAM and lost them at exit — destroying data on a
read.

`SecretStore` gains an optional `isDurable()`, absent meaning durable, and
both migrations withhold the disk delete when it answers false (the value
is still loaded, so the session behaves normally). The production fallback
is now a named `SessionSecretStore` rather than the bare
`InMemorySecretStore`, so the suite's many doubles keep standing in for a
*working keychain* — which is what they are there to be — and only code
that deliberately chooses RAM in production says so.

**Round 2's per-call descriptor never reached anyone.** Both web servers
resolved it once at startup and baked it into `initialConfig`, which
`/api/config` serves unchanged — so the freshness fix was inert for its
only consumer. The route now takes a `secretStorageResolver` and calls it
per request.

The lock needed three more fixes, and they compound:

- **Release removed whatever directory was at the path.** A holder whose
  work outran the stale threshold would delete the *next* owner's lock,
  letting a third writer into a section two processes believed they held.
  Ownership is now stamped with a random token and release only removes a
  lock still carrying it.
- **The timeout fell through to an unlocked write** — under contention,
  the one moment another writer is provably active. It now throws
  `SecretFileLockTimeoutError`, which `set` is allowed to do.
- **The first write of every process bypassed the lock**, because the
  secrets directory does not exist yet and `mkdir` failed ENOENT. The
  parent is created first. First writes are exactly when two processes are
  most likely to start together.

Fixing the timeout exposed a fourth: with stale (10s) below timeout, a
waiter stole from a holder that was merely *slow* rather than dead —
reinstating the lost update. A holder now heartbeats its lock, so
"stale" means the holder is gone, and the timeout is what a live-but-slow
holder produces. Lock timings are injectable so the tests can exercise
contention, staleness, and timeout without spending the real intervals.

Also: the loose-permission state is carried into `SecretStorageInfo` as
`looseMode` and rendered as the caveat (it outranks the encryption caveat —
a file others can read is a live exposure, while "unencrypted" is a
property of a file only its owner can open), rather than living in a
startup log the browser cannot see; and `usableSecretStorage` validates
`pendingEncryption` and `looseMode` as the optional booleans/numbers they
are (`pendingEncryption: "false"` is truthy and would have printed advice
that is the opposite of the truth) and rejects file-only fields on a
keychain or memory descriptor.

Tests: both migrations against a session store with a durable control
alongside, so the guard is visibly the difference; `/api/config` serving a
changing resolver and falling back without one; lock ownership after a
steal, the timeout under a live holder, no-steal from a slow holder, and a
locked first write; `looseMode` through the descriptor, the caveat
ordering, and the tone; and five more rejected payload shapes. Full
`npm run ci` green — 5,704 tests under the coverage gate.

Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-22 13:19:56 -04:00
commit b143dd6cbd
3 changed files with 674 additions and 36 deletions
Download patch file Download diff file Expand all files Collapse all files

148
core/auth/node/file-secret-store.ts
View file

@ -60,6 +60,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 {
SecretStoreUnavailableError,
@ -201,13 +202,50 @@ function decodeParts(
* lock exists to close.
*/
const LOCK_STALE_MS = 10_000;
/** Total time to wait for a lock before giving up and reporting it. */
const LOCK_TIMEOUT_MS = 5_000;
/**
* How long to wait for a live lock. Deliberately longer than
* {@link LOCK_STALE_MS}, so a waiter always outlives the point at which the
* holder becomes stealable — with the shorter value it used to have, a
* waiter gave up before it could ever steal an abandoned lock, which is the
* one case the stealing exists for.
*/
const LOCK_TIMEOUT_MS = 15_000;
const LOCK_POLL_MS = 25;
/**
* How often a holder touches its lock to prove it is alive.
*
* Comfortably under {@link LOCK_STALE_MS} so a live holder is never judged
* stale, which is what makes the two mechanisms coherent: staleness then
* means *dead*, not merely *slow*. Without the heartbeat, a holder on a
* slow filesystem — or one whose scrypt derivation ran long — has its lock
* stolen while it is still inside the critical section, reinstating exactly
* the lost update the lock exists to prevent.
*/
const LOCK_HEARTBEAT_MS = 3_000;
const sleep = (ms: number): Promise<void> =>
new Promise((resolve) => setTimeout(resolve, ms));
/**
* Thrown when a lock is held by someone else for longer than
* {@link LOCK_TIMEOUT_MS}.
*
* A timeout must **fail the mutation**, never fall through and write
* anyway: proceeding unlocked past a live holder is precisely the
* lost-update the lock exists to prevent, and doing it under contention —
* the one moment another writer is provably active — would be the worst
* possible time. `set` is allowed to hard-fail by the store contract, so
* this surfaces as the documented 503 and the user retries.
*/
export class SecretFileLockTimeoutError extends SecretStoreUnavailableError {
constructor(filePath: string) {
super(
`Timed out waiting for another process to finish writing the secrets file at ${filePath}. Another Inspector may be busy; try again.`,
);
this.name = "SecretFileLockTimeoutError";
}
}
/**
* Take an exclusive, cross-process lock on `filePath`.
*
@ -215,56 +253,112 @@ const sleep = (ms: number): Promise<void> =>
* fails with `EEXIST` on every POSIX filesystem and on Windows, and it
* needs no `O_EXCL` open-flag juggling. Returns the release function.
*
* **Stale locks are stolen, and that is not optional.** A container killed
* with SIGKILL leaves the directory behind, and a lock that can only be
* released by a cooperative exit would make one `docker kill` brick secret
* writes permanently — turning a rare lost-write race into a permanent
* outage, which is a strictly worse failure. Staleness is judged by the
* directory's mtime.
* **Ownership is stamped, not assumed.** The holder writes a random token
* inside the lock directory and release only removes a lock still carrying
* that token. Without it, a holder whose work outran the stale threshold
* would delete the *next* owner's lock on its way out, letting a third
* writer into a critical section two processes believed they held — a
* rarer and much more confusing corruption than the one being prevented.
*
* Never throws for lock-infrastructure reasons alone. If the lock cannot
* be created at all (a read-only mount, a filesystem without mkdir
* semantics we can rely on), the caller proceeds unlocked: the in-process
* queue still protects the common single-process case, and refusing to
* write would deny a real user a real secret in order to prevent a race
* with a second process that may not exist.
* **Stale locks are stolen, and that is not optional.** A container killed
* with SIGKILL leaves the directory behind, and a lock that only a
* cooperative exit can release would make one `docker kill` brick secret
* writes permanently — turning a rare lost write into a permanent outage,
* which is strictly worse. Staleness is judged by the directory's mtime,
* which a live holder refreshes on a heartbeat — so "stale" means the
* holder is *dead*, never merely slow. That distinction is what keeps the
* stealing from reinstating the very race the lock prevents.
*
* Throws {@link SecretFileLockTimeoutError} when another holder keeps it
* past the timeout. The *only* path that proceeds unlocked is a filesystem
* that cannot create the lock at all (a read-only mount, an exotic
* filesystem) — there, refusing would deny every write on a box where the
* single-process case is still perfectly safe, and a genuinely unwritable
* location fails at the write itself with a better message than a lock
* error would give.
*/
/**
* Timing policy, overridable so the tests can exercise contention,
* staleness, and timeout without spending the real intervals. Production
* callers pass nothing and get the constants above.
*/
export interface LockTimings {
staleMs?: number;
timeoutMs?: number;
heartbeatMs?: number;
pollMs?: number;
}
export async function acquireLock(
filePath: string,
timings: LockTimings = {},
): Promise<() => Promise<void>> {
const staleMs = timings.staleMs ?? LOCK_STALE_MS;
const timeoutMs = timings.timeoutMs ?? LOCK_TIMEOUT_MS;
const heartbeatMs = timings.heartbeatMs ?? LOCK_HEARTBEAT_MS;
const pollMs = timings.pollMs ?? LOCK_POLL_MS;
const lockDir = `${filePath}.lock`;
const ownerFile = `${lockDir}/owner`;
const token = crypto.randomBytes(16).toString("hex");
const noop = async (): Promise<void> => {};
const deadline = Date.now() + LOCK_TIMEOUT_MS;
const deadline = Date.now() + timeoutMs;
// The parent must exist before the lock can. On a first run it does not —
// `writeStoreFile` creates it later — so without this the very first write
// of every process failed with ENOENT and proceeded unlocked, meaning two
// first writes could both bypass the lock, read an empty map, and overwrite
// each other. The first write is exactly when two processes are most likely
// to start together.
try {
await fs.mkdir(path.dirname(filePath), { recursive: true });
} catch {
// Unwritable parent. The mkdir below will fail too, and the write after
// it will fail with a message about the actual problem.
}
for (;;) {
try {
await fs.mkdir(lockDir, { recursive: false });
await fs.writeFile(ownerFile, token, "utf-8");
// Prove liveness for as long as we hold it. `unref` so a held lock
// never keeps the process alive on its own — the interval is a
// side-channel, not work anyone is waiting for.
const heartbeat = setInterval(() => {
const now = new Date();
void fs.utimes(lockDir, now, now).catch(() => {
// Lock already released or stolen; the release below copes.
});
}, heartbeatMs);
heartbeat.unref?.();
return async () => {
clearInterval(heartbeat);
try {
await fs.rmdir(lockDir);
// Release only what we still own. A lock judged stale and stolen
// now belongs to someone else, and removing it would drop them
// into a race with whoever comes next.
const held = await fs.readFile(ownerFile, "utf-8");
if (held !== token) return;
await fs.rm(lockDir, { recursive: true, force: true });
} catch {
// Already gone — another process judged us stale and stole it.
// Nothing to undo, and nothing the caller can act on.
// Already gone, or unreadable. Either way there is nothing to
// release and nothing the caller can act on.
}
};
} catch (err) {
const code = (err as NodeJS.ErrnoException).code;
if (code !== "EEXIST") {
// Not "someone else holds it" — the lock itself is unavailable.
// Proceed unlocked rather than failing the write; see above.
return noop;
}
if (code !== "EEXIST") return noop; // cannot lock here at all — see above
try {
const held = (await fs.stat(lockDir)).mtimeMs;
if (Date.now() - held > LOCK_STALE_MS) {
await fs.rmdir(lockDir);
if (Date.now() - held > staleMs) {
await fs.rm(lockDir, { recursive: true, force: true });
continue;
}
} catch {
// The holder released it between our mkdir and our stat. Retry.
}
if (Date.now() >= deadline) return noop;
await sleep(LOCK_POLL_MS);
if (Date.now() >= deadline)
throw new SecretFileLockTimeoutError(filePath);
await sleep(pollMs);
}
}
}

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

@ -52,9 +52,10 @@ import {
tightenSecretFilePermissions,
} from "./file-secret-store.js";
import {
InMemorySecretStore,
KeyringSecretStore,
probeKeyringAvailable,
secretStoreIsDurable,
SessionSecretStore,
type SecretStore,
} from "./secret-store.js";
@ -248,8 +249,12 @@ async function buildStore(
};
}
if (kind === "memory") {
// `SessionSecretStore`, not the bare `InMemorySecretStore` the tests
// use: it reports `isDurable() === false`, which is what stops the
// mcp.json / client.json migrations from stripping a plaintext secret
// off disk in exchange for a copy that dies with this process.
return {
store: new InMemorySecretStore(),
store: new SessionSecretStore(),
info: { kind, reason, durable: false, detail },
};
}
@ -293,11 +298,16 @@ async function describeFileStore(
): Promise<SecretStorageInfo> {
const onDisk = await store.readOnDiskEncryption();
const plaintext = onDisk === null ? !store.encrypted : onDisk === "none";
// Re-checked here rather than captured at selection, for the same reason
// the encryption state is: the mode can change under a running process,
// and this descriptor is what the browser renders as fact.
const perms = await tightenSecretFilePermissions(filePath);
return {
kind,
reason,
path: filePath,
plaintext,
...(perms.state === "loose" ? { looseMode: perms.mode } : {}),
// Only meaningful while the two disagree; omitted otherwise so the
// payload does not carry a flag that says nothing.
...(plaintext && store.encrypted ? { pendingEncryption: true } : {}),
@ -414,6 +424,9 @@ class DeferredSecretStore implements SecretStore {
private async target(): Promise<SecretStore> {
return (await resolveSecretStore()).store;
}
async isDurable(): Promise<boolean> {
return secretStoreIsDurable(await this.target());
}
async get(serverId: string, field: string): Promise<string | null> {
return (await this.target()).get(serverId, field);
}

56
core/auth/node/secret-store.ts
View file

@ -301,6 +301,24 @@ const PROBE_ACCOUNT = "__inspector:probe";
* be lost.
*/
export interface SecretStore {
/**
* Do values written here outlive the process?
*
* Optional, and **absent means durable** — every store that predates
* #1950 is, and a custom one is far more likely to be backed by
* something than by RAM.
*
* It exists because the *migrations* need it. Both of them (mcp.json in
* `server.ts`, client.json in `client/node-persistence.ts`) lift a
* plaintext secret off disk, write it to this store, and then strip it
* from the file — a safe trade only while "written to the store" is at
* least as durable as "left on disk". `InMemorySecretStore` used to be
* a test double only, so that held by construction; making it a
* production fallback broke it, and a plain `GET /api/servers` on an
* unmounted container would have moved a user's existing secrets into
* RAM and lost them at exit.
*/
isDurable?(): Promise<boolean>;
get(serverId: string, field: string): Promise<string | null>;
set(serverId: string, field: string, value: string): Promise<void>;
/** No-op if no entry exists. */
@ -439,6 +457,21 @@ export class KeyringSecretStore implements SecretStore {
* server factory. Mirrors the keyring contract exactly so swapping it
* in/out doesn't change behavior beyond persistence.
*/
/**
* Is `store` durable? Absent `isDurable` means yes — see the interface.
*
* A free function rather than a required method so the existing test
* doubles (and any third-party implementation) keep compiling, while the
* one store that is *not* durable has to say so explicitly. Defaulting the
* other way would make every double silently non-durable and quietly
* disable the migrations they exist to exercise.
*/
export async function secretStoreIsDurable(
store: SecretStore,
): Promise<boolean> {
return store.isDurable ? store.isDurable() : true;
}
export class InMemorySecretStore implements SecretStore {
private readonly map = new Map<string, string>();
@ -461,3 +494,26 @@ export class InMemorySecretStore implements SecretStore {
}
}
}
/**
* The in-memory store as a **production** choice — the fallback for a
* container with no keychain and nothing durable mounted (#1950).
*
* Behaviorally identical to {@link InMemorySecretStore}; it exists to
* answer `isDurable()` with `false`, and that one bit is load-bearing.
* Both migrations (mcp.json, client.json) lift a plaintext secret off
* disk, write it to the store, and then delete it from the file — safe
* only while the store outlives the process. Against a session-scoped
* store that trade destroys the secret, and it runs on an ordinary read,
* so merely opening the app would do it.
*
* Kept as a separate class rather than a flag on the base so the test
* suite's many `new InMemorySecretStore()` doubles keep standing in for a
* *working keychain*, which is what they are there to be. Only the code
* that deliberately chooses RAM in production says so.
*/
export class SessionSecretStore extends InMemorySecretStore {
async isDurable(): Promise<boolean> {
return false;
}
}