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

Browse source 
Four findings (two inline, two suppressed), all taken. Three are the lock,
which is where the remaining sharp edges were.

- **A lock could be created and then never stamped.** If `mkdir` succeeded
  but writing the owner token failed, the catch treated it as "cannot lock
  here", returned the no-op release, and left the directory behind. The
  mutation then ran unlocked *and* every later writer queued behind an
  orphan nobody could deliberately release, until it aged out. The
  acquisition now tracks whether it created the directory, removes it on a
  failed stamp, and refuses rather than entering the section unlocked.

- **An old mtime was being treated as proof the owner was dead.** It is
  not: a holder that is SIGSTOPed, starved by a blocked event loop, or on a
  machine that suspended is alive and not running, and expiring it on
  elapsed time alone takes the lock out from under a live critical section
  — the lost update the lock exists to prevent, arrived at through the
  machinery meant to prevent it. The heartbeat narrowed the window; it
  could not close it, because the holder that most needs protecting is
  precisely the one not running its timers.

  The owner stamp now carries `pid` and `host`, and staleness only *opens*
  the question — on the same host a waiter asks the OS whether that pid
  still exists (`kill(pid, 0)`, which sends no signal; `EPERM` counts as
  alive, so a holder running as another user is never stolen from). Only a
  genuinely absent process is stolen from. Where the stamp cannot be
  trusted — another machine over a shared filesystem, or a missing/corrupt
  owner file — the mtime remains the answer, because the alternative is a
  lock nothing can ever break, turning one dead process into a permanent
  outage.

- **Podman was classified as a host.** `/.dockerenv` is Docker's marker and
  rootless Podman has no equivalent of it, while its cgroup paths say
  `libpod-<id>.scope` rather than the literal "podman" the pattern looked
  for. Such a box fell back to a file in the ephemeral writable layer and
  reported it as durable — the wrong way round to be wrong.
  `/run/.containerenv` and `libpod` are both recognized now.

- **Two tests floated a second `acquireLock`**, so the test could finish and
  `afterEach` could remove the temp directory while the acquisition was
  still running. Both hold and settle it now.

Separately, a defect of my own found while re-reading round 3:
`describeFileStore` called `tightenSecretFilePermissions`, so the function
whose job is to *describe* the store issued a `chmod` on every
`GET /api/config` — once per page load, on a file another process may be
mid-write on. Split out a read-only `readSecretFilePermissions`; the repair
stays at startup, where it happens once and its outcome is announced.

Tests: a stale lock whose owner is alive is not stolen (and survives the
attempt), one whose pid is gone is, one stamped by another host is; a
failed ownership stamp reports and leaves nothing behind; Podman via both
signals; and the read-only permission check leaves a 0644 file at 0644.
Full `npm run ci` green — 5,711 tests under the coverage gate.

Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-22 13:40:32 -04:00
commit 050556e1a7
4 changed files with 324 additions and 28 deletions
Download patch file Download diff file Expand all files Collapse all files

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

@ -8,6 +8,7 @@
* agree with any of those without them being true.
*/
import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";
import { existsSync } from "node:fs";
import * as crypto from "node:crypto";
import * as fs from "node:fs/promises";
import * as os from "node:os";
@ -15,6 +16,7 @@ import * as path from "node:path";
import {
acquireLock,
FileSecretStore,
readSecretFilePermissions,
SecretFileLockTimeoutError,
SecretFileKeyMismatchError,
tightenSecretFilePermissions,
@ -450,6 +452,21 @@ describe("tightenSecretFilePermissions", () => {
expect((await fs.stat(filePath())).mode & 0o777).toBe(0o600);
});
it("readSecretFilePermissions reports the mode without changing it", async () => {
// The descriptor is rebuilt on every GET /api/config, so the function it
// calls must not chmod — once per page load, on a file another process
// may be mid-write on. The repair belongs at startup.
const store = new FileSecretStore({ filePath: filePath() });
await store.set("alpha", "env:A", "1");
await fs.chmod(filePath(), 0o644);
expect(await readSecretFilePermissions(filePath())).toEqual({
state: "loose",
mode: 0o644,
});
// Still 0644 — reading did not quietly repair it.
expect((await fs.stat(filePath())).mode & 0o777).toBe(0o644);
});
it("reports absent, not a failure, for a file that doesn't exist yet", async () => {
// The first-run case. Nothing to tighten and nothing to warn about —
// `writeStoreFile` creates the file 0600.
@ -684,16 +701,21 @@ describe("acquireLock", () => {
// unlocked and overwrite each other. First writes are exactly when two
// processes are most likely to start together.
const target = path.join(tmpDir, "no-such-dir", "secrets.json");
const release = await acquireLock(target);
const release = await acquireLock(target, { pollMs: 10 });
// A real lock was taken: a second acquisition must not be handed out.
let secondTaken = false;
void acquireLock(target).then((r) => {
// Held, not floated: an acquisition still running when the test ends
// would race `afterEach` removing the temp directory.
const second = acquireLock(target, { pollMs: 10 }).then((r) => {
secondTaken = true;
return r();
return r;
});
await new Promise((r) => setTimeout(r, 60));
expect(secondTaken).toBe(false);
await release();
await (
await second
)();
});
it("releases only a lock it still owns", async () => {
@ -754,25 +776,148 @@ describe("acquireLock", () => {
});
try {
let stolen = false;
void acquireLock(target, {
// Held and settled below rather than floated, so the waiter cannot
// still be running when `afterEach` removes the temp directory.
const waiter = acquireLock(target, {
staleMs: 120,
timeoutMs: 400,
pollMs: 10,
})
.then((r) => {
.then(async (r) => {
stolen = true;
return r();
await r();
})
.catch(() => {
// Timed out, which is the expected outcome.
});
await new Promise((r) => setTimeout(r, 500));
await waiter;
expect(stolen).toBe(false);
} finally {
await release();
}
});
it("does not steal a stale lock whose owner process is still alive", async () => {
// An old mtime means the holder has not heartbeated lately, and there are
// two very different reasons: it died, or it is alive but not running —
// SIGSTOPed, event-loop-blocked, or on a machine that suspended. Taking
// the lock on elapsed time alone would pull it out from under a live
// critical section, which is the lost update the lock exists to prevent.
const target = path.join(tmpDir, "suspended.json");
const lockDir = `${target}.lock`;
await fs.mkdir(lockDir, { recursive: true });
// Stamp it with *this* process, which is definitionally alive, and age it
// well past stale — the shape of a suspended holder.
await fs.writeFile(
path.join(lockDir, "owner"),
JSON.stringify({
token: "someone",
pid: process.pid,
host: os.hostname(),
}),
"utf-8",
);
const old = new Date(Date.now() - 60_000);
await fs.utimes(lockDir, old, old);
await expect(
acquireLock(target, { staleMs: 50, timeoutMs: 150, pollMs: 10 }),
).rejects.toBeInstanceOf(SecretFileLockTimeoutError);
// Their lock survives the attempt.
expect(existsSync(lockDir)).toBe(true);
await fs.rm(lockDir, { recursive: true, force: true });
});
it("steals a stale lock whose owner process is gone", async () => {
// The case the stealing exists for. A pid that no longer resolves is a
// dead holder, and refusing to break its lock would turn one killed
// container into permanently broken secret writes.
const target = path.join(tmpDir, "dead.json");
const lockDir = `${target}.lock`;
await fs.mkdir(lockDir, { recursive: true });
await fs.writeFile(
path.join(lockDir, "owner"),
// A pid that cannot be running: `kill(0)` answers ESRCH.
JSON.stringify({ token: "gone", pid: 2 ** 31 - 1, host: os.hostname() }),
"utf-8",
);
const old = new Date(Date.now() - 60_000);
await fs.utimes(lockDir, old, old);
const release = await acquireLock(target, {
staleMs: 50,
timeoutMs: 2_000,
pollMs: 10,
});
await release();
});
it("steals a stale lock stamped by another machine", async () => {
// A shared filesystem: we cannot ask this host about that host's pid, so
// the mtime is all there is. Refusing would make a lock nothing can ever
// break.
const target = path.join(tmpDir, "remote.json");
const lockDir = `${target}.lock`;
await fs.mkdir(lockDir, { recursive: true });
await fs.writeFile(
path.join(lockDir, "owner"),
JSON.stringify({
token: "elsewhere",
pid: process.pid,
host: "some-other-host",
}),
"utf-8",
);
const old = new Date(Date.now() - 60_000);
await fs.utimes(lockDir, old, old);
const release = await acquireLock(target, {
staleMs: 50,
timeoutMs: 2_000,
pollMs: 10,
});
await release();
});
it("cleans up and reports when it cannot stamp ownership", async () => {
// mkdir succeeded, the owner write did not. Returning a no-op release
// here would enter the critical section unlocked *and* leave the
// directory behind, so every later writer queues behind an orphan nobody
// can deliberately release.
const target = path.join(tmpDir, "unstampable.json");
vi.resetModules();
vi.doMock("node:fs/promises", async () => {
const actual =
await vi.importActual<typeof import("node:fs/promises")>(
"node:fs/promises",
);
return {
...actual,
default: actual,
writeFile: vi.fn((p: string, ...rest: unknown[]) =>
String(p).endsWith(`${path.sep}owner`)
? Promise.reject(new Error("ENOSPC: no space left on device"))
: (actual.writeFile as (...a: unknown[]) => Promise<void>)(
p,
...rest,
),
),
};
});
try {
const mod =
await import("@inspector/core/auth/node/file-secret-store.js");
await expect(mod.acquireLock(target)).rejects.toThrow(
/Could not initialize the lock/,
);
// And nothing is left obstructing the next writer.
expect(existsSync(`${target}.lock`)).toBe(false);
} finally {
vi.doUnmock("node:fs/promises");
vi.resetModules();
}
});
it("serializes concurrent writers through two independent store instances", async () => {
// Two `FileSecretStore` objects have separate in-process queues, so this
// is the cross-process race in miniature: without the file lock the second

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

@ -135,6 +135,7 @@ describe("isContainer", () => {
*/
async function loadWithFs(opts: {
dockerenv?: boolean;
containerenv?: boolean;
cgroup?: string | Error;
}) {
vi.resetModules();
@ -142,8 +143,11 @@ describe("isContainer", () => {
const actual = await vi.importActual<typeof import("node:fs")>("node:fs");
return {
...actual,
existsSync: (p: string) =>
p === "/.dockerenv" ? !!opts.dockerenv : actual.existsSync(p),
existsSync: (p: string) => {
if (p === "/.dockerenv") return !!opts.dockerenv;
if (p === "/run/.containerenv") return !!opts.containerenv;
return actual.existsSync(p);
},
readFileSync: (p: string, enc?: unknown) => {
if (p === "/proc/1/cgroup") {
if (opts.cgroup instanceof Error) throw opts.cgroup;
@ -173,6 +177,23 @@ describe("isContainer", () => {
expect(mod.isContainer()).toBe(true);
});
it("is true when Podman's /run/.containerenv exists", async () => {
// Rootless Podman has no `/.dockerenv`, and its cgroup line often does
// not contain the literal "podman" either — this file is the signal that
// is reliably there. Missing it classified the container as a host, so a
// keychain-less box wrote a "durable" file into the ephemeral layer.
const mod = await loadWithFs({ containerenv: true });
expect(mod.isContainer()).toBe(true);
});
it("is true for a libpod cgroup path", async () => {
// What Podman actually writes: `/machine.slice/libpod-<id>.scope`.
const mod = await loadWithFs({
cgroup: "0::/machine.slice/libpod-3f2a.scope\n",
});
expect(mod.isContainer()).toBe(true);
});
it("is true when a container runtime appears in /proc/1/cgroup", async () => {
const mod = await loadWithFs({
cgroup: "0::/system.slice/containerd.service\n",

143
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 os from "node:os";
import * as path from "node:path";
import { readStoreFile, writeStoreFile } from "../../storage/store-io.js";
import {
@ -300,6 +301,10 @@ export async function acquireLock(
const lockDir = `${filePath}.lock`;
const ownerFile = `${lockDir}/owner`;
const token = crypto.randomBytes(16).toString("hex");
// The stamp a would-be thief consults before taking this lock: the token
// proves ownership on release, the pid and host let a waiter ask the OS
// whether we are still alive rather than inferring it from elapsed time.
const owner: LockOwner = { token, pid: process.pid, host: os.hostname() };
const noop = async (): Promise<void> => {};
const deadline = Date.now() + timeoutMs;
@ -317,9 +322,16 @@ export async function acquireLock(
}
for (;;) {
let created = false;
try {
await fs.mkdir(lockDir, { recursive: false });
await fs.writeFile(ownerFile, token, "utf-8");
created = true;
// Stamp ownership before entering the section. If this fails the lock
// exists but says nothing about who holds it, so nobody could ever
// release it deliberately and every later writer would queue behind an
// orphan until it aged out. Clean up and report, rather than proceeding
// unlocked *and* leaving the obstruction behind.
await fs.writeFile(ownerFile, JSON.stringify(owner), "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.
@ -336,8 +348,8 @@ export async function acquireLock(
// 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;
const held = parseOwner(await fs.readFile(ownerFile, "utf-8"));
if (held?.token !== token) return;
await fs.rm(lockDir, { recursive: true, force: true });
} catch {
// Already gone, or unreadable. Either way there is nothing to
@ -346,15 +358,17 @@ export async function acquireLock(
};
} catch (err) {
const code = (err as NodeJS.ErrnoException).code;
if (created) {
// We made the directory and then failed to stamp it. Ours to remove.
await fs.rm(lockDir, { recursive: true, force: true }).catch(() => {});
throw new SecretStoreUnavailableError(
`Could not initialize the lock for the secrets file at ${filePath}: ${err instanceof Error ? err.message : String(err)}`,
);
}
if (code !== "EEXIST") return noop; // cannot lock here at all — see above
try {
const held = (await fs.stat(lockDir)).mtimeMs;
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 (await canSteal(lockDir, ownerFile, staleMs)) {
await fs.rm(lockDir, { recursive: true, force: true }).catch(() => {});
continue;
}
if (Date.now() >= deadline)
throw new SecretFileLockTimeoutError(filePath);
@ -363,6 +377,89 @@ export async function acquireLock(
}
}
/**
* May this waiter take a lock it found already held?
*
* Elapsed wall time alone does not answer that. An old mtime means the
* holder has not run its heartbeat lately, and there are two very different
* reasons for that: it died, or it is *alive but not running* — SIGSTOPed,
* starved by a blocked event loop, or on a machine that suspended. Expiring
* on time alone treats those identically and takes the lock out from under a
* live critical section, which is the lost update the lock exists to
* prevent, now arrived at through the machinery meant to prevent it.
*
* So the mtime only opens the question, and the owner stamp answers it: on
* the same host, ask the operating system whether that pid is still there.
* `kill(pid, 0)` sends no signal — it is the standard existence probe.
* `EPERM` counts as alive (the process exists; it just isn't ours), so a
* holder running as another user is never stolen from.
*
* The stamp is not always usable — a lock written by a different machine
* over a shared filesystem, or one whose owner file is missing or corrupt.
* There the mtime is all there is, and stealing on it is still right: the
* alternative is a lock nothing can ever break, which turns one dead process
* into a permanent outage.
*/
async function canSteal(
lockDir: string,
ownerFile: string,
staleMs: number,
): Promise<boolean> {
let held: number;
try {
held = (await fs.stat(lockDir)).mtimeMs;
} catch {
// Released between our mkdir and our stat — retry rather than steal.
return false;
}
if (Date.now() - held <= staleMs) return false;
let owner: LockOwner | null = null;
try {
owner = parseOwner(await fs.readFile(ownerFile, "utf-8"));
} catch {
// No stamp to consult; fall through to the time-only answer.
}
if (!owner || owner.host !== os.hostname() || typeof owner.pid !== "number") {
return true;
}
return !processIsAlive(owner.pid);
}
/** Does this pid exist? `kill(pid, 0)` is the probe; it sends no signal. */
function processIsAlive(pid: number): boolean {
try {
process.kill(pid, 0);
return true;
} catch (err) {
// EPERM means it exists and belongs to someone else — still alive.
return (err as NodeJS.ErrnoException).code === "EPERM";
}
}
interface LockOwner {
token: string;
pid: number;
host: string;
}
/** Parse an owner stamp, tolerating anything that isn't one. */
function parseOwner(raw: string): LockOwner | null {
try {
const parsed: unknown = JSON.parse(raw);
if (typeof parsed !== "object" || parsed === null) return null;
const { token, pid, host } = parsed as Partial<LockOwner>;
if (typeof token !== "string") return null;
return {
token,
pid: typeof pid === "number" ? pid : -1,
host: String(host),
};
} catch {
return null;
}
}
export interface FileSecretStoreOptions {
/** Absolute path of the secrets file. */
filePath: string;
@ -696,9 +793,24 @@ export async function tightenSecretFilePermissions(
}
// Verify rather than assume. The chmod above is best-effort, but the UI and
// the README both state the file is 0600 — so a file we failed to tighten
// (owned by another user, on a read-only mount) must be *reported*, not
// quietly described as protected. This is the one place that can tell the
// difference, because it is the only one that looks at the file.
// (owned by another user, on a read-only mount) must be reported, not
// quietly described as protected.
return readSecretFilePermissions(filePath);
}
/**
* Read the file's permission state **without touching it**.
*
* The read-only half of {@link tightenSecretFilePermissions}, split out
* because the descriptor is rebuilt on every `GET /api/config` and a
* function whose job is to *describe* must not chmod as a side effect —
* once per page load, on a file another process may be mid-write on. The
* repair belongs at startup, where it happens once and its outcome is
* announced; from then on the question is only "what is the mode now".
*/
export async function readSecretFilePermissions(
filePath: string,
): Promise<SecretFilePermissionState> {
try {
const mode = (await fs.stat(filePath)).mode & 0o777;
if (mode === 0o600) return { state: "ok" };
@ -711,7 +823,8 @@ export async function tightenSecretFilePermissions(
}
/**
* Outcome of {@link tightenSecretFilePermissions}: the file is `0600`
* Outcome of {@link tightenSecretFilePermissions} and
* {@link readSecretFilePermissions}: the file is `0600`
* (`ok`), does not exist yet (`absent`), or exists with a wider mode we
* could not fix (`loose`) — the only case anyone needs to hear about.
*/

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

@ -49,6 +49,7 @@ import {
} from "../secret-storage-info.js";
import {
FileSecretStore,
readSecretFilePermissions,
tightenSecretFilePermissions,
} from "./file-secret-store.js";
import {
@ -132,13 +133,26 @@ export function defaultSecretFilePath(): string {
export function isContainer(): boolean {
if (process.env.KUBERNETES_SERVICE_HOST) return true;
try {
if (fsSync.existsSync("/.dockerenv")) return true;
// `/.dockerenv` is Docker's marker; `/run/.containerenv` is Podman's
// equivalent and is the only one a rootless Podman container reliably
// has. Missing it classified such a container as a host, which is the
// wrong way round to be wrong: the fallback then writes a file into the
// ephemeral writable layer and calls it durable.
if (
fsSync.existsSync("/.dockerenv") ||
fsSync.existsSync("/run/.containerenv")
) {
return true;
}
} catch {
// A sandboxed / restricted filesystem. Fall through to the cgroup read.
}
try {
const cgroup = fsSync.readFileSync("/proc/1/cgroup", "utf-8");
return /\b(docker|containerd|kubepods|podman|lxc)\b/.test(cgroup);
// `libpod` is what Podman actually writes into cgroup paths
// (`/machine.slice/libpod-<id>.scope`); the literal string "podman"
// often does not appear at all.
return /\b(docker|containerd|kubepods|podman|libpod|lxc)\b/.test(cgroup);
} catch {
// Not Linux, or /proc is not mounted. Not a container as far as we know.
return false;
@ -300,8 +314,11 @@ async function describeFileStore(
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);
// and this descriptor is what the browser renders as fact. The *read-only*
// half deliberately — describing must not chmod, least of all once per
// page load on a file another process may be mid-write on. The repair
// happens once, at selection.
const perms = await readSecretFilePermissions(filePath);
return {
kind,
reason,