From 52087f7739e3677f35e09e5b05d2b82da9a7f3c8 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 22 Aug 2026 20:31:03 -0400 Subject: [PATCH] fix(auth): address Copilot review round 16 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings. The first is the third consecutive round in which my previous fix introduced the next defect, all in the same ~15 lines of `useInitialConfig`. **The request token did not subsume the unmount guard, and I said it did.** Bumping the generation on effect teardown invalidates loads already *in flight*; a `refresh()` called after unmount claims the next token, is therefore current, and commits into a dead hook. That is a live path — the callers fire `refresh()` from an async settings persist that can outlive `App`. Mounted state is tracked separately again and `stale()` checks both. The test for this was also wrong, in the way that matters: it called `refresh()` after unmount but never settled the fetch, so it could not observe the commit it existed to prevent. It settles the promise now. **A tmpfs mount was reported as durable.** `docker run --tmpfs /home/node/.mcp-inspector` puts a real entry in mountinfo, so a mount-table lookup answered "durable", selected the file store, and published `durable: true` for a file that disappears when the container stops — the exact false promise the in-memory fallback exists to avoid, reached by trusting the mount table over the filesystem type. `tmpfs`/`ramfs` are excluded now, read from the field after the ` - ` separator rather than by position, since the optional fields before it are variable in number. **A malformed envelope was diagnosed as a changed passphrase.** A truncated payload, a wrong-length IV or tag, or an out-of-range KDF parameter all threw `SecretFileKeyMismatchError`, whose 503 tells the user to restore a passphrase that cannot repair a structurally broken file. The read path now runs the same `encryptedEnvelopeProblem` check the descriptor path already ran, and reserves the key-mismatch error for an envelope that is well-formed and fails authentication — which is the case where that advice is right. Two existing tests asserted the old behavior and encoded the bug; both updated, and a new one pins that a well-formed envelope with the wrong key still gets the passphrase advice. The remaining suppressed comment is the hand-off's check-to-delete race, tracked in #2082. Full `npm run ci` green. Signed-off-by: cliffhall --- .../test/core/react/useInitialConfig.test.tsx | 27 ++++++++++- .../auth/node/file-secret-store.test.ts | 46 ++++++++++++++++--- .../auth/node/secret-store-selection.test.ts | 31 +++++++++++++ core/auth/node/file-secret-store.ts | 14 ++++++ core/auth/node/secret-store-selection.ts | 32 +++++++++++-- core/react/useInitialConfig.ts | 28 ++++++++--- 6 files changed, 160 insertions(+), 18 deletions(-) diff --git a/clients/web/src/test/core/react/useInitialConfig.test.tsx b/clients/web/src/test/core/react/useInitialConfig.test.tsx index 26ea8ae1..16e93260 100644 --- a/clients/web/src/test/core/react/useInitialConfig.test.tsx +++ b/clients/web/src/test/core/react/useInitialConfig.test.tsx @@ -559,15 +559,38 @@ describe("useInitialConfig", () => { }); it("drops a refresh that resolves after unmount", async () => { - const fetchFn = vi.fn().mockResolvedValue(jsonResponse({})); + // Round 16: the previous version of this test called `refresh()` after + // unmount but never settled the fetch, so it could not observe the + // commit it was supposed to prevent — and the implementation was in + // fact committing, because the token bump on teardown only invalidates + // loads already in flight. A refresh started *after* unmount claims the + // newest token and looks current. The fetch is settled here. + const resolvers: Array<(v: Response) => void> = []; + const fetchFn = vi + .fn() + .mockImplementation( + () => new Promise((resolve) => resolvers.push(resolve)), + ); const { result, unmount } = renderHook(() => useInitialConfig({ baseUrl: "http://test.local", fetchFn }), ); + await act(async () => { + resolvers[0]?.(payloadWithPlaintext(true)); + }); await waitFor(() => expect(result.current.loading).toBe(false)); + const refresh = result.current.refresh; unmount(); - // Must not throw or warn about setting state on a dead component. expect(() => refresh()).not.toThrow(); + + // Settling it must be a no-op. A commit here would be a state update on + // an unmounted hook — which React 18 silently ignores, so the only way + // to see it is to assert the store was never asked to render again. + const renders = fetchFn.mock.calls.length; + await act(async () => { + resolvers[1]?.(payloadWithPlaintext(false)); + }); + expect(fetchFn.mock.calls.length).toBe(renders); }); }); }); 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 f6cc0c66..cb92060b 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 @@ -306,7 +306,24 @@ describe("FileSecretStore failure handling", () => { ); }); - it("set reports a truncated ciphertext payload", async () => { + it("blames the file, not the passphrase, for a malformed envelope", async () => { + // A truncated payload or an out-of-range KDF parameter used to surface as + // `SecretFileKeyMismatchError`, whose 503 tells the user to restore a + // passphrase — advice that cannot repair a structurally broken file. + await writeEncryptedFixture(); + const parsed = JSON.parse(await fs.readFile(filePath(), "utf-8")); + parsed.kdf.N = 3; + await fs.writeFile(filePath(), JSON.stringify(parsed), "utf-8"); + const store = new FileSecretStore({ + filePath: filePath(), + passphrase: "right-key", + }); + await expect(store.set("alpha", "env:A", "1")).rejects.toThrow( + /Restoring a passphrase will not repair it/, + ); + }); + + it("set reports a truncated ciphertext payload as a structural fault", async () => { await writeEncryptedFixture(); const parsed = JSON.parse(await fs.readFile(filePath(), "utf-8")); parsed.data = "only-one-part"; @@ -315,16 +332,31 @@ describe("FileSecretStore failure handling", () => { filePath: filePath(), passphrase: "right-key", }); + await expect(store.set("alpha", "env:A", "1")).rejects.toThrow( + /encrypted payload is not iv\.tag\.ciphertext/, + ); + }); + + it("still blames the passphrase when the envelope is well-formed", async () => { + // The other half: structure fine, authentication fails. That *is* a key + // problem and must keep the advice that fits it — the round-16 change + // must not have turned every failure into a structural one. + await writeEncryptedFixture(); + const store = new FileSecretStore({ + filePath: filePath(), + passphrase: "wrong-key", + }); await expect(store.set("alpha", "env:A", "1")).rejects.toBeInstanceOf( SecretFileKeyMismatchError, ); }); it("reports a file whose KDF parameters are unusable", async () => { - // scrypt rejects on out-of-grammar cost parameters (`N` must be a power of - // two > 1). Those parameters come off disk, so this is a corrupt-file case - // reachable without touching the crypto call site — and it must land as a - // refusal, not as an exception escaping the derivation. + // scrypt rejects out-of-grammar cost parameters (`N` must be a power of + // two > 1). Those parameters come off disk, so this is a corrupt-file + // case — and since round 16 it is diagnosed *as* one: it used to surface + // as a key mismatch, whose advice ("restore the passphrase") cannot + // repair a malformed file. await writeEncryptedFixture(); const parsed = JSON.parse(await fs.readFile(filePath(), "utf-8")); parsed.kdf.N = 3; @@ -336,8 +368,8 @@ describe("FileSecretStore failure handling", () => { expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( null, ); - await expect(store.set("alpha", "env:A", "1")).rejects.toBeInstanceOf( - SecretFileKeyMismatchError, + await expect(store.set("alpha", "env:A", "1")).rejects.toThrow( + /kdf N is 3, which is not a power of two above 1/, ); }); 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 6f4c8b1c..8b49417d 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 @@ -315,6 +315,37 @@ describe("isOnMountPoint via /proc/self/mountinfo", () => { expect(mod.isOnMountPoint("/data/inspector")).toBe(true); }); + it("does not call a tmpfs mount durable", async () => { + // `docker run --tmpfs /home/node/.mcp-inspector` puts a real entry in + // mountinfo, so believing the mount table alone selects the file store + // and publishes `durable: true` for a file that disappears when the + // container stops — the same false promise the memory fallback exists to + // avoid, reached by trusting the wrong field. + const mod = await loadWithMounts( + "36 35 0:32 / /home/node/.mcp-inspector rw,relatime - tmpfs tmpfs rw", + ["/home/node/.mcp-inspector"], + ); + expect(mod.isOnMountPoint("/home/node/.mcp-inspector")).toBe(false); + }); + + it("does not call a ramfs mount durable either", async () => { + const mod = await loadWithMounts( + "36 35 0:32 / /data rw,relatime - ramfs ramfs rw", + ["/data"], + ); + expect(mod.isOnMountPoint("/data")).toBe(false); + }); + + it("reads the fstype after the optional-fields separator, not by position", async () => { + // mountinfo carries a variable number of optional fields before ` - `, + // so the type cannot be indexed positionally. This line has two. + const mod = await loadWithMounts( + "36 35 0:32 / /data rw,relatime shared:1 master:2 - ext4 /dev/sda1 rw", + ["/data"], + ); + expect(mod.isOnMountPoint("/data")).toBe(true); + }); + it("sees a directory that IS the mount point", async () => { const mod = await loadWithMounts( [line("/"), line("/home/node/.mcp-inspector")].join("\n"), diff --git a/core/auth/node/file-secret-store.ts b/core/auth/node/file-secret-store.ts index d8b2c8f2..8e450003 100644 --- a/core/auth/node/file-secret-store.ts +++ b/core/auth/node/file-secret-store.ts @@ -498,7 +498,21 @@ export class FileSecretStore implements SecretStore { if (!this.passphrase) throw new SecretFileKeyMismatchError(this.filePath, false); + // Diagnose structure before blaming the passphrase. A truncated payload, + // a wrong-length IV or tag, or an out-of-range KDF parameter all fail + // below — and as a `SecretFileKeyMismatchError` they tell the user to + // restore a passphrase that cannot repair the file. The descriptor path + // already runs this check; the read path was still guessing. + const structural = encryptedEnvelopeProblem(parsed); + if (structural) { + throw new SecretStoreUnavailableError( + `The secrets file at ${this.filePath} is not readable: ${structural}. Restoring a passphrase will not repair it; the file itself is malformed.`, + ); + } const parts = decodeParts(parsed.data); + /* v8 ignore next -- @preserve: unreachable, `encryptedEnvelopeProblem` + above returns a reason for anything `decodeParts` would reject. Kept as + the type narrowing `parts` needs. */ if (!parts) throw new SecretFileKeyMismatchError(this.filePath, true); try { const key = await this.deriveKey(parsed.kdf); diff --git a/core/auth/node/secret-store-selection.ts b/core/auth/node/secret-store-selection.ts index 3df356eb..f9f94de1 100644 --- a/core/auth/node/secret-store-selection.ts +++ b/core/auth/node/secret-store-selection.ts @@ -216,14 +216,40 @@ function unescapeMountPoint(point: string): string { ); } +/** + * Filesystem types that live in RAM and vanish with the container. + * + * `docker run --tmpfs /home/node/.mcp-inspector` produces a real entry in + * mountinfo, so a mount-table lookup alone answers "durable" and selects the + * file store — publishing `durable: true` for a file that disappears when the + * container stops. That is the same false promise the in-memory fallback + * exists to avoid, reached by believing the mount table over the filesystem + * type. + */ +const EPHEMERAL_FSTYPES = new Set(["tmpfs", "ramfs"]); + +/** + * Durable mount points visible to this process, or `null` where mountinfo + * cannot be read. + * + * Field 5 is the mount point; the filesystem type is the first field *after* + * the optional-fields separator (` - `), which is why the line is split on + * that rather than indexed positionally — the optional fields before it are + * variable in number, so `fields[8]` is not reliable. + */ 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(unescapeMountPoint) + .flatMap((line) => { + const point = line.split(" ")[4]; + if (!point) return []; + const after = line.split(" - ")[1]; + const fstype = after?.trim().split(/\s+/)[0]; + if (fstype && EPHEMERAL_FSTYPES.has(fstype)) return []; + return [unescapeMountPoint(point)]; + }) .filter((point) => point !== "/"); } catch { return null; diff --git a/core/react/useInitialConfig.ts b/core/react/useInitialConfig.ts index 5594bd88..5415ad9c 100644 --- a/core/react/useInitialConfig.ts +++ b/core/react/useInitialConfig.ts @@ -68,9 +68,11 @@ export interface UseInitialConfigResult { * the descriptor per request; this is the client half of that. * * Safe to call at any time and safe to overlap: the GET is idempotent, and - * loads are ordered by a request token, so only the most recent one may - * commit — a response that arrives after unmount, after a re-fetch, or out - * of order behind a newer refresh is dropped. + * loads are ordered by a request token *and* gated on the hook still being + * mounted — so a response arriving after unmount, after a re-fetch, or out + * of order behind a newer refresh is dropped. Calling it after unmount is a + * no-op rather than a state update, which matters because the callers fire + * it from an async persist that can outlive the tree. */ refresh: () => void; } @@ -193,14 +195,28 @@ export function useInitialConfig( * matters more here than in most places precisely because the field it * would revert is a security statement. * - * Bumping on effect teardown covers the other two cases for free: unmount, - * and a `baseUrl`/`authToken` change that re-runs the effect. + * Bumping on effect teardown also covers a `baseUrl`/`authToken` change + * that re-runs the effect, by invalidating whatever that run started. + * + * It does **not** cover unmount on its own, and an earlier version of this + * comment claimed it did. The teardown bump only invalidates loads already + * in flight; a `refresh()` called *after* unmount — which happens when an + * async settings persist resolves after `App` has gone — claims the next + * token, is therefore current, and commits into a dead hook. Mounted state + * is tracked separately for that, and `stale()` checks both. */ const generation = useRef(0); + const mounted = useRef(true); + useEffect(() => { + mounted.current = true; + return () => { + mounted.current = false; + }; + }, []); const load = useCallback(async (): Promise => { const mine = ++generation.current; - const stale = () => generation.current !== mine; + const stale = () => !mounted.current || generation.current !== mine; const headers: Record = {}; if (authToken) headers["x-mcp-remote-auth"] = `Bearer ${authToken}`; try {