From de7795ca54419120339f15a4f017e1621576cb69 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 22 Aug 2026 12:51:14 -0400 Subject: [PATCH] fix(auth): address Copilot review round 2 on the secret-store PR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seven findings, all taken. - **The descriptor went stale after the upgrading write.** Round 1 made it read the file instead of the policy, but it was still computed once and cached beside the selection — so the first `set` under a newly-set passphrase encrypted the file while `/api/config` kept serving "still unencrypted" for the rest of the session, describing bytes this very process had just changed. The *selection* stays cached (the banner, the payload, and the store doing the writing must agree on which store it is); the file descriptor is re-derived per call, which costs one read of a small file per config fetch. - **The mount check asked the wrong question.** Comparing a directory's `st_dev` against its parent's detects a mount *at* that path, but a volume mounted at `/home/node` leaves `/home/node/.mcp-inspector` an ordinary subdirectory sharing its parent's device — so a durable container setup was judged unmounted and demoted to session-only `memory`. It now reads `/proc/self/mountinfo` and asks whether the directory is *under* any mount other than `/`, which also catches the same-device bind mount no `st_dev` test can see. Where mountinfo is unreadable (macOS, Windows, a `/proc`-less container) it falls back to the device comparison, now against `/` rather than the parent. - **A decrypted payload was cast, not checked.** GCM proves the bytes are authentic, not that they parse to the right shape. An array passes `typeof === "object"`, takes the named assignment, and then `JSON.stringify` drops every named property of an array — so `set` would resolve having written a file without the secret it was handed. Silent loss on the one operation whose entire contract is that it does not lose things. Both branches now validate a non-array object of string values, and a shape refusal is deliberately not rethrown as a key mismatch, which would send the user to fix a working passphrase. - **The read-modify-write cycle was only serialized in-process.** A durable file is shared state and a second Inspector on the same box (a CLI run beside a web session) could read the same map and overwrite the first one's save, both reporting success. Mutations now take a `mkdir`-based lock. A lock left by a killed process is stolen after 10s — one `docker kill` must not brick secret writes permanently, which would be a worse failure than the race. If the lock cannot be created at all, the write proceeds unlocked rather than denying a real user a real secret to prevent a race with a process that may not exist. Reads do not lock: `writeStoreFile` is atomic, so a reader sees the old file or the new one, never a torn one. - **The permission repair was fire-and-forget while the docs promised 0600.** A file owned by another user, or on a read-only mount, stayed exposed while the footer described it as protected. It now verifies the resulting mode and warns, naming the actual mode, when it could not be tightened. - **`Group`'s theme carried a domain-named `secretStorageFooter` variant**, which AGENTS.md reserves for app-wide primitive styling. Renamed to the structural `stickyModalFooter`; the secret-storage meaning stays in the element that owns it. - The PR description still claimed the salt made a short passphrase safe, contradicting the corrected comment and README. Updated. Tests: both mount layouts that motivated the change (mount-at and mount-above) plus the sibling-prefix, space-escaped, first-run, and no-mountinfo cases; the array and non-string refusals on both the plaintext and the authentic-ciphertext paths (the latter through a hand-built fixture, since the store's own writer cannot produce that state); lock exclusion, stale-lock stealing, the un-lockable fallback, and two independent store instances racing; the permission report in all three states; and the descriptor no longer going stale within one process. Full `npm run ci` green — 5,688 tests under the coverage gate. Signed-off-by: cliffhall --- README.md | 4 + .../SecretStorageFooter.tsx | 2 +- .../auth/node/file-secret-store.test.ts | 226 +++++++++++++++- .../auth/node/secret-store-selection.test.ts | 136 ++++++++++ clients/web/src/theme/Group.ts | 8 +- core/auth/node/file-secret-store.ts | 241 +++++++++++++++--- core/auth/node/secret-store-selection.ts | 195 ++++++++++---- 7 files changed, 716 insertions(+), 96 deletions(-) diff --git a/README.md b/README.md index eb0624d7..73ffcd10 100644 --- a/README.md +++ b/README.md @@ -480,6 +480,10 @@ docker run --rm -p 127.0.0.1:6274:6274 \ Setting the passphrase later is safe — the next write upgrades an existing plaintext file in place. Until that write happens the existing values really are still readable, and the banner and footer keep saying so rather than reporting the file as encrypted the moment the variable appears. **Changing or losing the passphrase is not safe**: a file that can no longer be decrypted is read as empty and _refuses to be written_, rather than being silently replaced with a new one holding only your latest secret. Restore the original passphrase, or delete `secrets.json` and re-enter the values. +The Inspector writes the file `0600` and re-tightens it at startup if something loosened it. If it _cannot_ — the file belongs to another user, or the mount is read-only — it says so in the log rather than continuing to describe the file as protected, since on that box the mode claim above is not true. + +**One durable file, one writer.** Mutations take a lock (`secrets.json.lock`) so a second Inspector on the same box — a CLI run beside a web session — cannot read the same state and overwrite the first one's save. A lock left behind by a killed process is treated as abandoned after 10 seconds and stolen, because a lock that only a clean exit can release would turn one `docker kill` into permanently broken secret writes. + Three env vars affect where the file lands. `MCP_INSPECTOR_SECRET_STORE=keyring|file|memory` picks the store outright, bypassing the probe. `MCP_INSPECTOR_SECRET_FILE` names the file. Failing both, the file follows `MCP_STORAGE_DIR` — the same variable that relocates OAuth tokens and `client.json` — so mounting a volume at your configured storage directory is enough to make secrets durable there. **Upgrading from an image before this fix?** Earlier images did not create `/home/node/.mcp-inspector`, so Docker created the volume's mount point as `root` and the non-root `node` user couldn't write to it. An **empty** volume repairs itself on the first run of a current image (Docker applies the image directory's ownership to an empty volume), but one that already has files in it keeps its old `root` ownership and still fails with `EACCES`. Fix it once: diff --git a/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.tsx b/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.tsx index 008787f8..8bd445ba 100644 --- a/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.tsx +++ b/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.tsx @@ -32,7 +32,7 @@ export interface SecretStorageFooterProps { // header, body and footer as one. `nowrap` keeps it to one line at any modal // width; the truncating detail text on the right absorbs a narrow modal. const FooterBand = Group.withProps({ - variant: "secretStorageFooter", + variant: "stickyModalFooter", h: SECRET_STORAGE_FOOTER_HEIGHT, px: "md", gap: 6, diff --git a/clients/web/src/test/integration/auth/node/file-secret-store.test.ts b/clients/web/src/test/integration/auth/node/file-secret-store.test.ts index 369546fa..26e433a3 100644 --- a/clients/web/src/test/integration/auth/node/file-secret-store.test.ts +++ b/clients/web/src/test/integration/auth/node/file-secret-store.test.ts @@ -7,11 +7,13 @@ * not contain the plaintext — and a mocked filesystem can be made to * agree with any of those without them being true. */ -import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +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 { + acquireLock, FileSecretStore, SecretFileKeyMismatchError, tightenSecretFilePermissions, @@ -447,10 +449,52 @@ describe("tightenSecretFilePermissions", () => { expect((await fs.stat(filePath())).mode & 0o777).toBe(0o600); }); - it("is a no-op for a file that doesn't exist yet (the first-run case)", async () => { + 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. await expect( tightenSecretFilePermissions(path.join(tmpDir, "nope.json")), - ).resolves.toBeUndefined(); + ).resolves.toEqual({ state: "absent" }); + }); + + it("reports a mode it could not fix, instead of claiming 0600", async () => { + // The case the caller has to hear about: the docs and the footer both say + // the file is 0600, so a file we failed to tighten must be surfaced rather + // than silently described as protected. + // + // Driven through a fresh module whose `chmod` rejects — a test cannot + // readily own a file it lacks permission to chmod, and vitest cannot spy + // on a `node:fs/promises` export because the ESM namespace is not + // configurable. + const store = new FileSecretStore({ filePath: filePath() }); + await store.set("alpha", "env:A", "1"); + await fs.chmod(filePath(), 0o644); + + vi.resetModules(); + vi.doMock("node:fs/promises", async () => { + const actual = + await vi.importActual( + "node:fs/promises", + ); + return { + ...actual, + default: actual, + chmod: vi.fn(() => + Promise.reject(new Error("EPERM: operation not permitted")), + ), + }; + }); + try { + const mod = + await import("@inspector/core/auth/node/file-secret-store.js"); + expect(await mod.tightenSecretFilePermissions(filePath())).toEqual({ + state: "loose", + mode: 0o644, + }); + } finally { + vi.doUnmock("node:fs/promises"); + vi.resetModules(); + } }); }); @@ -498,3 +542,179 @@ describe("readOnDiskEncryption", () => { expect(await keyless.readOnDiskEncryption()).toBe("aes-256-gcm"); }); }); + +/** + * Write a genuinely-encrypted secrets file whose decrypted payload is + * `payload` — including payloads the store would never produce. + * + * Hand-rolled against the documented format rather than driven through the + * store, because the whole point is to reach a state the store's own writer + * cannot create: an authentic ciphertext (right passphrase, valid GCM tag) + * carrying the wrong shape. Anything less would test the key check instead + * of the shape check. + */ +async function writeEncryptedFixtureWithPayload( + payload: unknown, + passphrase: string, +): Promise { + const salt = crypto.randomBytes(16); + const kdf = { + algorithm: "scrypt", + salt: salt.toString("base64"), + N: 16384, + r: 8, + p: 1, + }; + const key: Buffer = await new Promise((resolve, reject) => { + crypto.scrypt( + passphrase, + salt, + 32, + { N: kdf.N, r: kdf.r, p: kdf.p }, + (err, k) => (err ? reject(err) : resolve(k)), + ); + }); + const iv = crypto.randomBytes(12); + const cipher = crypto.createCipheriv("aes-256-gcm", key, iv); + const body = Buffer.concat([ + cipher.update(JSON.stringify(payload), "utf-8"), + cipher.final(), + ]); + const data = [iv, cipher.getAuthTag(), body] + .map((b) => b.toString("base64")) + .join("."); + await fs.writeFile( + filePath(), + JSON.stringify({ version: 1, encryption: "aes-256-gcm", kdf, data }), + "utf-8", + ); +} + +describe("payload shape validation", () => { + // GCM proves who wrote the bytes, not what shape they parse to, and the + // plaintext branch proves nothing at all — so neither may be cast. + it("refuses a plaintext file whose secrets field is an array", async () => { + // The array is the case a cast cannot survive: it passes `typeof + // "object"`, takes the named assignment, and then `JSON.stringify` drops + // every named property — so `set` would resolve having written a file + // without the secret it was handed. + await fs.writeFile( + filePath(), + JSON.stringify({ version: 1, encryption: "none", secrets: [] }), + "utf-8", + ); + const store = new FileSecretStore({ filePath: filePath() }); + await expect(store.set("alpha", "env:A", "1")).rejects.toThrow(/an array/); + expect(await store.get("alpha", "env:A")).toBe(null); + }); + + it("refuses a plaintext file holding a non-string value", async () => { + await fs.writeFile( + filePath(), + JSON.stringify({ + version: 1, + encryption: "none", + secrets: { "alpha:env:A": { nested: true } }, + }), + "utf-8", + ); + const store = new FileSecretStore({ filePath: filePath() }); + await expect(store.set("alpha", "env:B", "1")).rejects.toThrow( + /non-string value for "alpha:env:A"/, + ); + }); + + it("refuses an authentic ciphertext that decodes to the wrong shape", async () => { + // Encrypted with the right passphrase, so it decrypts cleanly and the + // refusal can only come from the shape check. It must NOT surface as a key + // mismatch, which would send the user to fix a passphrase that is working. + await writeEncryptedFixtureWithPayload([], "hunter2"); + const store = new FileSecretStore({ + filePath: filePath(), + passphrase: "hunter2", + }); + expect(await store.get("alpha", "env:A")).toBe(null); + const err = await store + .set("alpha", "env:A", "1") + .then(() => null) + .catch((e: unknown) => e); + expect(err).toBeInstanceOf(SecretStoreUnavailableError); + expect(err).not.toBeInstanceOf(SecretFileKeyMismatchError); + expect((err as Error).message).toMatch(/an array/); + }); +}); + +describe("acquireLock", () => { + it("excludes a second holder while the first is live", async () => { + const target = path.join(tmpDir, "locked.json"); + const release = await acquireLock(target); + let secondTaken = false; + const second = acquireLock(target).then((r) => { + secondTaken = true; + return r; + }); + // Still held: the waiter must not have been let through. + await new Promise((r) => setTimeout(r, 60)); + expect(secondTaken).toBe(false); + await release(); + await ( + await second + )(); + expect(secondTaken).toBe(true); + }); + + it("steals a lock left behind by a killed process", async () => { + // A container killed with SIGKILL leaves the directory. A lock only a + // clean exit can release would turn that into permanently broken writes — + // strictly worse than the race it prevents. + const target = path.join(tmpDir, "stale.json"); + const lockDir = `${target}.lock`; + await fs.mkdir(lockDir); + const old = new Date(Date.now() - 60_000); + await fs.utimes(lockDir, old, old); + const release = await acquireLock(target); + await release(); + }); + + it("proceeds unlocked rather than failing when the lock cannot be created", async () => { + // A path whose parent does not exist: mkdir fails with ENOENT, not + // EEXIST. Refusing to write here would deny a real user a real secret to + // prevent a race with a second process that may not exist. + const release = await acquireLock( + path.join(tmpDir, "no-such-dir", "secrets.json"), + ); + await release(); + }); + + 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 + // write drops the first one's entry. + const target = path.join(tmpDir, "shared.json"); + const a = new FileSecretStore({ filePath: target }); + const b = new FileSecretStore({ filePath: target }); + await Promise.all([ + a.set("alpha", "env:A", "1"), + b.set("alpha", "env:B", "2"), + ]); + expect(await a.get("alpha", "env:A")).toBe("1"); + expect(await a.get("alpha", "env:B")).toBe("2"); + }); +}); + +describe("tightenSecretFilePermissions reports what it could not fix", () => { + it("says absent when there is no file yet", async () => { + expect(await tightenSecretFilePermissions(filePath())).toEqual({ + state: "absent", + }); + }); + + it("says ok once the file is 0600", async () => { + const store = new FileSecretStore({ filePath: filePath() }); + await store.set("alpha", "env:A", "1"); + await fs.chmod(filePath(), 0o644); + expect(await tightenSecretFilePermissions(filePath())).toEqual({ + state: "ok", + }); + }); +}); 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 b77dec18..ffc0f24f 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 @@ -221,6 +221,105 @@ describe("isOnMountPoint", () => { }); }); +describe("isOnMountPoint via /proc/self/mountinfo", () => { + /** + * Load the module with a synthetic mount table, so the container layouts + * that matter can be exercised on a developer's laptop. `existsSync` is + * stubbed alongside it because the paths in these tables do not exist here. + */ + async function loadWithMounts(mountinfo: string | null, existing: string[]) { + vi.resetModules(); + vi.doMock("node:fs", async () => { + const actual = await vi.importActual("node:fs"); + return { + ...actual, + existsSync: (p: string) => existing.includes(p), + readFileSync: (p: string, enc?: unknown) => { + if (p === "/proc/self/mountinfo") { + if (mountinfo === null) throw new Error("ENOENT"); + return mountinfo; + } + return actual.readFileSync(p, enc as never); + }, + }; + }); + return import("@inspector/core/auth/node/secret-store-selection.js"); + } + + afterEach(() => { + vi.doUnmock("node:fs"); + vi.resetModules(); + }); + + // Field 5 is the mount point; the rest is real mountinfo shape. + const line = (point: string) => + `36 35 0:32 / ${point} rw,relatime - overlay overlay rw`; + + it("sees a directory that IS the mount point", async () => { + const mod = await loadWithMounts( + [line("/"), line("/home/node/.mcp-inspector")].join("\n"), + ["/home/node/.mcp-inspector"], + ); + expect(mod.isOnMountPoint("/home/node/.mcp-inspector")).toBe(true); + }); + + it("sees a directory INSIDE a mounted volume", async () => { + // The case a device-boundary test gets wrong: the volume is at + // /home/node, so .mcp-inspector is an ordinary subdirectory sharing its + // parent's device. Judging it unmounted demotes a durable setup to + // session-only memory. + const mod = await loadWithMounts( + [line("/"), line("/home/node")].join("\n"), + ["/home/node/.mcp-inspector"], + ); + expect(mod.isOnMountPoint("/home/node/.mcp-inspector")).toBe(true); + }); + + it("is false on the container's writable layer", async () => { + // Only `/` is mounted, and `/` is deliberately excluded — it is the + // writable layer this question exists to distinguish against. + const mod = await loadWithMounts(line("/"), ["/home/node/.mcp-inspector"]); + expect(mod.isOnMountPoint("/home/node/.mcp-inspector")).toBe(false); + }); + + it("does not mistake a sibling prefix for a parent mount", async () => { + // `/home/node-2` starts with `/home/node` as a string but is not under + // it, which is why the check appends the separator. + const mod = await loadWithMounts( + [line("/"), line("/home/node")].join("\n"), + ["/home/node-2/.mcp-inspector"], + ); + expect(mod.isOnMountPoint("/home/node-2/.mcp-inspector")).toBe(false); + }); + + it("decodes a space-escaped mount point", async () => { + const mod = await loadWithMounts( + [line("/"), line("/mnt/my\\040volume")].join("\n"), + ["/mnt/my volume/secrets"], + ); + expect(mod.isOnMountPoint("/mnt/my volume/secrets")).toBe(true); + }); + + it("answers from the nearest existing ancestor, not the missing leaf", async () => { + // First run: the secrets directory has not been created inside the + // mounted volume yet. + const mod = await loadWithMounts( + [line("/"), line("/home/node")].join("\n"), + ["/home/node"], + ); + expect(mod.isOnMountPoint("/home/node/.mcp-inspector")).toBe(true); + }); + + it("falls back to the device comparison where mountinfo is unreadable", async () => { + // macOS, Windows, or a /proc-less container. The fallback compares + // against `/` rather than the immediate parent, so a subdirectory of a + // mount is still recognized — but a real path is needed for `stat`, so + // this only asserts it does not throw and answers the ordinary case. + const mod = await loadWithMounts(null, [tmpDir]); + expect(mod.isOnMountPoint(tmpDir)).toBe(false); + }); +}); + describe("warnAboutSecretStorage", () => { const base: SecretStorageInfo = { kind: "keyring", @@ -452,6 +551,43 @@ describe("the descriptor reports the file, not the write policy", () => { expect(info.pendingEncryption).toBeUndefined(); }); + it("stops saying unencrypted once this process performs the upgrading write", async () => { + // The staleness trap: the selection is cached for the process, so a + // descriptor cached alongside it would keep serving "still unencrypted" + // from `/api/config` for the rest of the session — describing bytes this + // very process had just changed. + const filePath = path.join(tmpDir, "secrets.json"); + process.env.MCP_INSPECTOR_SECRET_FILE = filePath; + process.env.MCP_INSPECTOR_SECRET_STORE = "file"; + vi.spyOn(console, "warn").mockImplementation(() => {}); + + const plain = await loadWithProbe(false); + await plain.defaultSecretStore().set("alpha", "env:A", "1"); + + process.env.MCP_INSPECTOR_SECRET_KEY = "hunter2"; + const mod = await loadWithProbe(false); + + // Same module instance throughout — this is one running process. + expect((await mod.getSecretStorageInfo()).plaintext).toBe(true); + await mod.defaultSecretStore().set("alpha", "env:B", "2"); + const after = await mod.getSecretStorageInfo(); + expect(after.plaintext).toBe(false); + expect(after.pendingEncryption).toBeUndefined(); + }); + + it("keeps returning the same store while re-deriving the descriptor", async () => { + // The selection itself must stay cached — the banner, /api/config, and + // the store doing the writing have to agree on *which* store it is. + process.env.MCP_INSPECTOR_SECRET_FILE = path.join(tmpDir, "secrets.json"); + process.env.MCP_INSPECTOR_SECRET_STORE = "file"; + vi.spyOn(console, "warn").mockImplementation(() => {}); + const mod = await loadWithProbe(false); + const first = await mod.resolveSecretStore(); + const second = await mod.resolveSecretStore(); + expect(second.store).toBe(first.store); + expect((await mod.getSecretStorageInfo()).kind).toBe("file"); + }); + it("falls back to the write policy when there is no file yet", async () => { // Nothing on disk to describe, and the policy is then accurate: the first // write creates the file in exactly that mode. diff --git a/clients/web/src/theme/Group.ts b/clients/web/src/theme/Group.ts index d6e5b0df..9f16e716 100644 --- a/clients/web/src/theme/Group.ts +++ b/clients/web/src/theme/Group.ts @@ -14,7 +14,7 @@ export const ThemeGroup = Group.extend({ return {}; }, styles: (_theme, props) => { - // `secretStorageFooter` is the permanent band at the bottom of the + // `stickyModalFooter` is the permanent band at the bottom of the // Client/Server Settings modals naming where secrets are stored (#1950). // Flat properties, so they belong here rather than in App.css. // @@ -27,7 +27,11 @@ export const ThemeGroup = Group.extend({ // this is supposed to be, so it sticks to the bottom edge instead, with // the same opaque body background and z-index the sticky header uses so // content passes underneath rather than through it. - if (props.variant === "secretStorageFooter") { + // Structural, not domain: this says "a band pinned to the bottom of a + // modal", which is all the theme layer should know. What the band *means* + // — that it names where your secrets go — belongs to `SecretStorageFooter`, + // the element that owns those semantics. + if (props.variant === "stickyModalFooter") { return { root: { position: "sticky", diff --git a/core/auth/node/file-secret-store.ts b/core/auth/node/file-secret-store.ts index 7c5e419d..5ab95096 100644 --- a/core/auth/node/file-secret-store.ts +++ b/core/auth/node/file-secret-store.ts @@ -122,6 +122,37 @@ interface SecretFile { const buildAccount = (serverId: string, field: string): string => `${serverId}:${field}`; +/** + * Assert that a decoded payload really is a `Record`. + * + * Neither branch of {@link FileSecretStore.readMap} can trust its input: the + * plaintext `secrets` field is whatever the file says, and a decrypted + * payload only proves the *bytes* were authentic — GCM verifies who wrote + * them, not what shape they parse to. + * + * An array is the case that shows why a cast is not enough. `[]` passes + * `typeof === "object"`, so `map[account] = value` assigns a named property + * to it, and `JSON.stringify` then drops every named property of an array — + * so `set` resolves successfully having written a file that does not contain + * the secret it was given. Silent loss on the one operation whose entire + * contract is that it does not lose things. + */ +function asSecretMap(value: unknown, filePath: string): Record { + if (typeof value !== "object" || value === null || Array.isArray(value)) { + throw new SecretStoreUnavailableError( + `The secrets file at ${filePath} does not hold a secret map (found ${Array.isArray(value) ? "an array" : typeof value}). Refusing to write, which would replace it. Move the file aside to start over.`, + ); + } + for (const [key, entry] of Object.entries(value)) { + if (typeof entry !== "string") { + throw new SecretStoreUnavailableError( + `The secrets file at ${filePath} holds a non-string value for "${key}". Refusing to write, which would replace it. Move the file aside to start over.`, + ); + } + } + return { ...(value as Record) }; +} + /** * Thrown when a file was written encrypted and the current passphrase * cannot open it. A distinct type so `set` can refuse with advice that @@ -161,6 +192,83 @@ function decodeParts( }; } +/** + * How long a lock may be held before another process treats it as + * abandoned. Generous next to the work it guards (a read, one scrypt + * derivation, an atomic write — single-digit milliseconds), because the + * cost of being wrong is asymmetric: waiting a second too long is + * invisible, while stealing a live lock reintroduces the very race the + * 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; +const LOCK_POLL_MS = 25; + +const sleep = (ms: number): Promise => + new Promise((resolve) => setTimeout(resolve, ms)); + +/** + * Take an exclusive, cross-process lock on `filePath`. + * + * `mkdir` is the primitive rather than a lock *file*: it is atomic and + * 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. + * + * 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. + */ +export async function acquireLock( + filePath: string, +): Promise<() => Promise> { + const lockDir = `${filePath}.lock`; + const noop = async (): Promise => {}; + const deadline = Date.now() + LOCK_TIMEOUT_MS; + + for (;;) { + try { + await fs.mkdir(lockDir, { recursive: false }); + return async () => { + try { + await fs.rmdir(lockDir); + } catch { + // Already gone — another process judged us stale and stole it. + // Nothing to undo, 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; + } + try { + const held = (await fs.stat(lockDir)).mtimeMs; + if (Date.now() - held > LOCK_STALE_MS) { + await fs.rmdir(lockDir); + continue; + } + } catch { + // The holder released it between our mkdir and our stat. Retry. + } + if (Date.now() >= deadline) return noop; + await sleep(LOCK_POLL_MS); + } + } +} + export interface FileSecretStoreOptions { /** Absolute path of the secrets file. */ filePath: string; @@ -285,7 +393,7 @@ export class FileSecretStore implements SecretStore { // lands the file really is still readable, which is why // `readOnDiskEncryption` exists — the descriptor must report the file, // not the intent. - return { ...(parsed.secrets ?? {}) }; + return asSecretMap(parsed.secrets ?? {}, this.filePath); } if (parsed.encryption !== CIPHER || !parsed.kdf || !parsed.data) { @@ -306,8 +414,12 @@ export class FileSecretStore implements SecretStore { decipher.update(parts.body), decipher.final(), ]).toString("utf-8"); - return JSON.parse(plain) as Record; - } catch { + return asSecretMap(JSON.parse(plain), this.filePath); + } catch (err) { + // A shape refusal is about the *contents*, not the key — it decrypted + // fine. Rethrowing it as a key mismatch would send the user to fix a + // passphrase that is working. + if (err instanceof SecretStoreUnavailableError) throw err; // GCM authentication failure is what a wrong passphrase looks like, and // it is indistinguishable from a tampered/corrupt file — both mean "this // key does not open this file", which is what the message says. @@ -351,6 +463,31 @@ export class FileSecretStore implements SecretStore { await writeStoreFile(this.filePath, `${JSON.stringify(file, null, 2)}\n`); } + /** + * Run `fn` with the file to itself across *processes* as well. + * + * The in-process queue is necessary and not sufficient: a durable secrets + * file is shared state, and a second Inspector on the same box — a CLI run + * beside a web session is the ordinary case, not a contrived one — reads + * the same "before" map and then atomically replaces the file, dropping + * whatever the first process had just added. Both writes report success; + * one secret is simply gone. + * + * Only mutations take the lock. Reads deliberately do not: `writeStoreFile` + * is atomic (write-temp-then-rename), so a concurrent reader sees either the + * old file or the new one and never a torn one, and making every `get` + * contend on a lock would put a filesystem round-trip in front of the + * request path for no correctness gain. + */ + private async withFileLock(fn: () => Promise): Promise { + const release = await acquireLock(this.filePath); + try { + return await fn(); + } finally { + await release(); + } + } + /** Run `fn` with the file to itself, so concurrent callers can't lose writes. */ private serialize(fn: () => Promise): Promise { const run = this.queue.then(fn); @@ -380,27 +517,29 @@ export class FileSecretStore implements SecretStore { } async set(serverId: string, field: string, value: string): Promise { - await this.serialize(async () => { - let map: Record; - try { - map = (await this.readMap()) ?? {}; - } catch (err) { - // Already a typed refusal (unreadable file / key mismatch) — rethrow - // rather than re-wrapping, so the specific advice survives. - if (err instanceof SecretStoreUnavailableError) throw err; - throw new SecretStoreUnavailableError( - `Could not read the secrets file at ${this.filePath}: ${err instanceof Error ? err.message : String(err)}`, - ); - } - map[buildAccount(serverId, field)] = value; - try { - await this.writeMap(map); - } catch (err) { - throw new SecretStoreUnavailableError( - `Could not write the secrets file at ${this.filePath}: ${err instanceof Error ? err.message : String(err)}`, - ); - } - }); + await this.serialize(() => + this.withFileLock(async () => { + let map: Record; + try { + map = (await this.readMap()) ?? {}; + } catch (err) { + // Already a typed refusal (unreadable file / key mismatch) — + // rethrow rather than re-wrapping, so the specific advice survives. + if (err instanceof SecretStoreUnavailableError) throw err; + throw new SecretStoreUnavailableError( + `Could not read the secrets file at ${this.filePath}: ${err instanceof Error ? err.message : String(err)}`, + ); + } + map[buildAccount(serverId, field)] = value; + try { + await this.writeMap(map); + } catch (err) { + throw new SecretStoreUnavailableError( + `Could not write the secrets file at ${this.filePath}: ${err instanceof Error ? err.message : String(err)}`, + ); + } + }), + ); } async delete(serverId: string, field: string): Promise { @@ -419,17 +558,20 @@ export class FileSecretStore implements SecretStore { */ private async deleteWhere(match: (key: string) => boolean): Promise { try { - await this.serialize(async () => { - const map = await this.readMap(); - if (!map) return; - const keys = Object.keys(map).filter(match); - if (keys.length === 0) return; - for (const key of keys) delete map[key]; - // An emptied map keeps the file rather than unlinking it: its presence - // is what records which encryption mode this install settled on, so a - // later write can't silently switch modes on the user. - await this.writeMap(map); - }); + await this.serialize(() => + this.withFileLock(async () => { + const map = await this.readMap(); + if (!map) return; + const keys = Object.keys(map).filter(match); + if (keys.length === 0) return; + for (const key of keys) delete map[key]; + // An emptied map keeps the file rather than unlinking it: its + // presence is what records which encryption mode this install + // settled on, so a later write can't silently switch modes on the + // user. + await this.writeMap(map); + }), + ); } catch { // Intentionally silent — see the doc comment above. } @@ -450,11 +592,36 @@ export class FileSecretStore implements SecretStore { */ export async function tightenSecretFilePermissions( filePath: string, -): Promise { +): Promise { try { await fs.chmod(filePath, 0o600); } catch { // Non-existent (the common case — nothing written yet), read-only mount, - // or a platform without POSIX modes. Nothing actionable here. + // or a platform without POSIX modes. The verification below decides + // whether that mattered. + } + // 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. + try { + const mode = (await fs.stat(filePath)).mode & 0o777; + if (mode === 0o600) return { state: "ok" }; + return { state: "loose", mode }; + } catch { + // No file yet — the common first run. `writeStoreFile` creates it 0600, + // so there is nothing to warn about. + return { state: "absent" }; } } + +/** + * Outcome of {@link tightenSecretFilePermissions}: 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. + */ +export type SecretFilePermissionState = + | { state: "ok" } + | { state: "absent" } + | { state: "loose"; mode: number }; diff --git a/core/auth/node/secret-store-selection.ts b/core/auth/node/secret-store-selection.ts index a135d3d1..f447b0e3 100644 --- a/core/auth/node/secret-store-selection.ts +++ b/core/auth/node/secret-store-selection.ts @@ -145,44 +145,92 @@ export function isContainer(): boolean { } /** - * Is `dir` — or the nearest existing ancestor of it — a mount point? + * Nearest ancestor of `dir` that actually exists. * - * The test is `st_dev`: a mount is exactly the case where a directory sits - * on a different device from its parent. That is what makes this work - * regardless of *what* was mounted (named volume, bind mount, tmpfs) and - * without parsing `/proc/mounts`, whose format and container-namespace - * visibility vary. + * The directory usually does not exist on a first run — `~/.mcp-inspector` + * is created on demand — so every check below has to ask about the closest + * thing that does. A volume mounted *at* the target path exists by + * definition, so the common positive case needs no walk at all. + */ +function nearestExisting(dir: string): string | null { + let current = path.resolve(dir); + while (!fsSync.existsSync(current)) { + const parent = path.dirname(current); + /* v8 ignore next -- @preserve: the walk can only run out of ancestors if + the filesystem root itself does not exist, which would mean the process + has no cwd to have resolved against. Kept as the loop's termination + guard rather than trusting that invariant. */ + if (parent === current) return null; + current = parent; + } + return current; +} + +/** + * Mount points visible to this process, from `/proc/self/mountinfo`, or + * `null` where that file cannot be read (macOS, Windows, a `/proc`-less + * container). * - * It walks to the nearest existing ancestor because the directory usually - * does not exist yet on a first run — `~/.mcp-inspector` is created on - * demand. A volume mounted *at* that path exists by definition, so the - * common positive case is found immediately; the walk is what keeps the - * common negative case from being answered by a missing-file error. + * Field 5 of each line is the mount point, with spaces escaped as `\040`. + * `/` is dropped: in a container that is the writable layer, which is + * exactly what the durability question is asking *against*. + */ +function readMountPoints(): string[] | null { + try { + return fsSync + .readFileSync("/proc/self/mountinfo", "utf-8") + .split("\n") + .map((line) => line.split(" ")[4]) + .filter((point): point is string => Boolean(point)) + .map((point) => point.replace(/\\040/g, " ")) + .filter((point) => point !== "/"); + } catch { + return null; + } +} + +/** + * Does `dir` live on something mounted over the root filesystem? * - * `/` is its own parent and is trivially a "mount", so the loop stops - * there and reports false: the container's root filesystem is the writable - * layer this whole check exists to distinguish *against*. + * Two mechanisms, because either one alone gets a realistic case wrong. + * + * **`/proc/self/mountinfo` first**, where it exists. It answers the question + * actually being asked — "is this path *under* a mount" — rather than the + * narrower "is this path *itself* a mount". The difference is a real + * container layout: mounting a volume at `/home/node` leaves + * `/home/node/.mcp-inspector` an ordinary subdirectory sharing its parent's + * device, so a device-boundary test answers "not mounted" and demotes a + * durable setup to session-only `memory`. Reading the mount table also sees + * a bind mount that happens to come from the same device, which no `st_dev` + * comparison can. + * + * **The `st_dev` comparison as the fallback**, and against `/` rather than + * against the immediate parent — so a subdirectory of a mount is still + * recognized. Outside a container this barely matters (the fallback there is + * `file` either way); it matters in a `/proc`-less container, where being + * wrong costs durability. + * + * Errors answer `false`, keeping the fallback conservative — `memory` in a + * container — which never promises to keep a secret it then loses. */ export function isOnMountPoint(dir: string): boolean { try { - let current = path.resolve(dir); - // Walk up to the first path that exists. - while (!fsSync.existsSync(current)) { - const parent = path.dirname(current); - /* v8 ignore next -- @preserve: the walk can only run out of ancestors - if the filesystem root itself does not exist, which would mean the - process has no cwd to have resolved against. Kept as the loop's - termination guard rather than trusting that invariant. */ - if (parent === current) return false; - current = parent; + const current = nearestExisting(dir); + if (current === null) return false; + + const mounts = readMountPoints(); + if (mounts !== null) { + return mounts.some( + (point) => current === point || current.startsWith(`${point}/`), + ); } + const parent = path.dirname(current); - if (parent === current) return false; // reached `/` - return fsSync.statSync(current).dev !== fsSync.statSync(parent).dev; + if (parent === current) return false; // `dir` resolved to the root itself + return fsSync.statSync(current).dev !== fsSync.statSync("/").dev; } catch { // Permission denied, a racing unmount, or a platform where `dev` is not - // meaningful. "Not a mount" keeps the fallback on the conservative side - // (memory in a container), which never loses a secret it promised to keep. + // meaningful. return false; } } @@ -207,32 +255,54 @@ async function buildStore( } const filePath = defaultSecretFilePath(); const store = new FileSecretStore({ filePath }); - // Repair a loosened mode on a pre-existing file before anything reads or - // writes it. Fire-and-forget: it is a best-effort tightening (it swallows - // every failure), and blocking selection on a chmod would make an - // unwritable file a startup problem rather than a `set`-time one. - void tightenSecretFilePermissions(filePath); - // Report the FILE, not the intent. `store.encrypted` is what the next write - // will do; a file already on disk in the clear stays readable until that - // write happens, and saying "File (encrypted)" in the meantime is the one - // false reassurance this footer must never give. With no file yet (or one we - // cannot parse) the policy is all there is to report, and it is then - // accurate — the first write creates the file in that mode. + // Repair a loosened mode on a pre-existing file, and — unlike before — + // *verify* the result. The UI and the README both state the file is 0600, + // so a file we could not tighten (owned by another user, on a read-only + // mount) has to be reported rather than quietly described as protected. + const perms = await tightenSecretFilePermissions(filePath); + if (perms.state === "loose") { + console.warn( + `[mcp-inspector] The secrets file at ${filePath} is mode ${perms.mode.toString(8).padStart(4, "0")}, not 0600, and could not be tightened. Anyone who can read it can read the secrets in it.`, + ); + } + return { + store, + info: await describeFileStore(store, filePath, kind, reason, detail), + }; +} + +/** + * Build the descriptor for a file store by *looking at the file*. + * + * `store.encrypted` is what the next write will do; a file already on disk in + * the clear stays readable until that write happens, and saying "File + * (encrypted)" in the meantime is the one false reassurance this whole + * subsystem exists not to give. With no file yet (or one we cannot parse) the + * policy is all there is to report, and it is then accurate — the first write + * creates the file in that mode. + * + * Split out from `buildStore` so it can be re-run: this is a property of a + * file that changes underneath us, not a property of the selection. + */ +async function describeFileStore( + store: FileSecretStore, + filePath: string, + kind: SecretStoreKind, + reason: SecretStorageInfo["reason"], + detail?: string, +): Promise { const onDisk = await store.readOnDiskEncryption(); const plaintext = onDisk === null ? !store.encrypted : onDisk === "none"; return { - store, - info: { - kind, - reason, - path: filePath, - plaintext, - // Only meaningful while the two disagree; omitted otherwise so the - // payload does not carry a flag that says nothing. - ...(plaintext && store.encrypted ? { pendingEncryption: true } : {}), - durable: true, - detail, - }, + kind, + reason, + path: filePath, + plaintext, + // Only meaningful while the two disagree; omitted otherwise so the + // payload does not carry a flag that says nothing. + ...(plaintext && store.encrypted ? { pendingEncryption: true } : {}), + durable: true, + detail, }; } @@ -305,9 +375,28 @@ export function resolveSecretStore(): Promise { return resolved; } -/** The active store's descriptor — for the banner and `GET /api/config`. */ +/** + * The active store's descriptor — for the banner and `GET /api/config`. + * + * The *selection* is cached, deliberately (see {@link resolveSecretStore}). + * The descriptor is not, for the file store: `plaintext` and + * `pendingEncryption` describe bytes on disk that this very process + * changes. Cache them and the first `set` under a newly-set passphrase + * encrypts the file while every subsequent `/api/config` keeps serving + * "still unencrypted" until a restart — stale in the safe direction, but + * stale about the one fact this surface exists to state. Re-deriving costs + * one read of a small file per config fetch, which is once per page load. + */ export async function getSecretStorageInfo(): Promise { - return (await resolveSecretStore()).info; + const { store, info } = await resolveSecretStore(); + if (info.kind !== "file" || !(store instanceof FileSecretStore)) return info; + return describeFileStore( + store, + store.filePath, + "file", + info.reason, + info.detail, + ); } /**