From 6efc65d2afd68e6f49b25cdc50592c86270da0ee Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 22 Aug 2026 14:57:55 -0400 Subject: [PATCH] fix(auth): address Copilot review round 7 (five of six findings) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five taken; one rejected with evidence, below. - **The keychain hand-off ran outside the file lock.** Read → copy → delete has to be one transaction: another Inspector completing a `set` between the read and the delete had its brand-new secret removed without ever being copied — a write that reported success and then vanished, which is the loss the lock exists to prevent, reached through the migration meant to preserve things. The whole sequence now runs under the same cross-process lock every mutation takes. - **An unmigratable entry was skipped and the file deleted anyway.** A key that is not `serverId:field` cannot be addressed through the store API, so it cannot be copied — which makes the hand-off incomplete by definition, and the complete-success rule then forbids removing the source. It was discarding that value permanently in order to tidy up a migration that had failed. The copyable entries still go across; the file stays, and says why. - **`readOnDiskEncryption` accepted any format version.** A version-2 file was reported as a usable plaintext or encrypted store even though `readMap` refuses it and every save fails. Checked before the mode now, matching `readMap`. - **The loose-mode warning at selection was encryption-blind**, claiming a reader could read the secrets of a file that is ciphertext, and it duplicated the caveat printed moments later by `warnAboutSecretStorage`. Selection now only repairs; the descriptor path does the reporting, because it is the one that knows whether the file is encrypted. - **The footer's tooltip offered to copy the path without showing it.** The band stopped printing it and the accessible name carries it for screen-reader users, which left a sighted user unable to see where a custom destination points without pasting it somewhere else. **Not taken: "`writeStoreFile` is not atomic."** It is. The finding reads `store-io.ts` line 84 as a direct write, but `writeFile` there is imported from the `atomically` package, not `node:fs`. Verified rather than argued: writing over an existing file changes its inode (so a rename happened) and leaves mode `0600`. I had started implementing the suggested temp+rename before checking the import, which would have replaced a tested library with a hand-rolled duplicate. That check corrected something of mine, though: `tightenSecretFilePermissions` justified itself with "a rename over a 0644 file keeps the loose mode forever", which is false — the temp carries `mode: 0o600` and the rename replaces the inode, so every *write* re-establishes it. The function is still needed for the case it actually addresses: a file that exists and is never written. Comment corrected to say so. Full `npm run ci` green — 5,753 tests under the coverage gate. Signed-off-by: cliffhall --- .../SecretStorageFooter.test.tsx | 24 ++++++++ .../SecretStorageFooter.tsx | 7 ++- .../auth/node/file-secret-store.test.ts | 16 +++++ .../auth/node/secret-store-selection.test.ts | 50 ++++++++++++++-- core/auth/node/file-secret-store.ts | 21 +++++-- core/auth/node/secret-store-selection.ts | 59 ++++++++++++++++--- 6 files changed, 160 insertions(+), 17 deletions(-) diff --git a/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.test.tsx b/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.test.tsx index a639d91f..74cf979f 100644 --- a/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.test.tsx +++ b/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.test.tsx @@ -173,4 +173,28 @@ describe("SecretStorageFooter", () => { expect(band).toHaveTextContent("Mode 0644 — not owner-only."); expect(band).toHaveAttribute("data-tone", "warn"); }); + + it("puts the path in the tooltip, not just an offer to copy it", async () => { + // The band no longer prints the path and the accessible name carries it + // for screen-reader users, which left a sighted user unable to see where + // a custom destination points without copying it out somewhere else. + renderWithMantine( + , + ); + // Hover the control the tooltip is attached to, not the band around it. + await userEvent.hover( + screen.getByRole("button", { name: /Copy secrets file path/ }), + ); + const tip = await screen.findByRole("tooltip"); + expect(tip).toHaveTextContent("/custom/place/secrets.json"); + expect(tip).toHaveTextContent("Click to copy."); + }); }); diff --git a/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.tsx b/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.tsx index 8df1234e..e60679be 100644 --- a/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.tsx +++ b/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.tsx @@ -206,7 +206,12 @@ function footerTooltip(info: SecretStorageInfo): string | undefined { `The file's permissions could not be read (${info.permissionsUnknown}), so it is not known whether others can read it.`, ); } - parts.push("Click to copy file path"); + // The path itself, not just an offer to copy it. The band no longer prints + // it, and the accessible name carries it for screen-reader users — which + // left a sighted user unable to see *where* a custom destination points + // without copying it out to somewhere else first. + parts.push(info.path); + parts.push("Click to copy."); return parts.join(" "); } 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 69f6df4b..39e481c4 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 @@ -623,6 +623,22 @@ describe("readOnDiskEncryption", () => { ).toMatch(/unsupported envelope/); }); + it("calls a newer format version unreadable rather than usable", async () => { + // `readMap` refuses such a file, so describing it as a working plaintext + // or encrypted store would present it as fine while every save fails. + await fs.writeFile( + filePath(), + JSON.stringify({ version: 2, encryption: "none", secrets: {} }), + "utf-8", + ); + const store = new FileSecretStore({ filePath: filePath() }); + const state = await store.readOnDiskEncryption(); + expect(state.state).toBe("unreadable"); + expect(state.state === "unreadable" ? state.detail : "").toMatch( + /format version 2/, + ); + }); + it("needs no passphrase — it reads the envelope, never the payload", async () => { // A store whose key is wrong still has to be describable in the UI. const writer = new FileSecretStore({ 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 a18a1a8f..35906b53 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 @@ -704,22 +704,64 @@ describe("absorbFileSecretsIntoKeyring", () => { expect(warn).not.toHaveBeenCalled(); }); - it("skips an entry whose account key is malformed", async () => { + it("copies what it can but keeps the file when an entry is unmigratable", async () => { // A hand-edited file can carry a key that is not `serverId:field`. It - // cannot be addressed through the store API, so it is passed over rather - // than guessed at — but it must not abort the entries around it. + // cannot be addressed through the store API, so it cannot be copied — + // which makes the hand-off incomplete, and an incomplete hand-off must + // not delete its source. Removing the file anyway would discard that + // value permanently in order to tidy up a migration that had failed. const filePath = await seedFile({ "no-separator": "orphan", "srv:oauthClientSecret": "from-file", }); process.env.MCP_INSPECTOR_SECRET_FILE = filePath; - vi.spyOn(console, "warn").mockImplementation(() => {}); + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); const mod = await loadWithProbe(true); const keyring = new InMemorySecretStore(); await mod.absorbFileSecretsIntoKeyring(keyring); + // The migratable entry is across... expect(await keyring.get("srv", "oauthClientSecret")).toBe("from-file"); + // ...and the orphan still exists on disk rather than being dropped. + expect(existsSync(filePath)).toBe(true); + expect(JSON.parse(await fs.readFile(filePath, "utf-8"))).toMatchObject({ + secrets: { "no-separator": "orphan" }, + }); + expect(warn).toHaveBeenCalledWith( + expect.stringContaining("left in place so nothing is lost"), + ); + }); + + it("holds the file lock for the whole hand-off", async () => { + // Read → copy → delete must be one transaction. Another Inspector + // completing a `set` between the read and the delete would have its + // brand-new secret removed without ever being copied — a write that + // reported success and then vanished. + const filePath = await seedFile({ "srv:oauthClientSecret": "from-file" }); + process.env.MCP_INSPECTOR_SECRET_FILE = filePath; + vi.spyOn(console, "warn").mockImplementation(() => {}); + const mod = await loadWithProbe(true); + + const { acquireLock } = + await import("@inspector/core/auth/node/file-secret-store.js"); + // Hold the lock as the "other process" would. + const release = await acquireLock(filePath, { heartbeatMs: 20 }); + let finished = false; + const handOff = mod + .absorbFileSecretsIntoKeyring(new InMemorySecretStore()) + .then(() => { + finished = true; + }); + + await new Promise((r) => setTimeout(r, 120)); + // Blocked on us, not steaming ahead and deleting the file. + expect(finished).toBe(false); + expect(existsSync(filePath)).toBe(true); + + await release(); + await handOff; + expect(finished).toBe(true); }); it("runs the hand-off for an explicitly configured keyring too", async () => { diff --git a/core/auth/node/file-secret-store.ts b/core/auth/node/file-secret-store.ts index 35bf92bc..a18f5b26 100644 --- a/core/auth/node/file-secret-store.ts +++ b/core/auth/node/file-secret-store.ts @@ -589,6 +589,16 @@ export class FileSecretStore implements SecretStore { if (raw === null) return { state: "absent" }; try { const parsed = JSON.parse(raw) as SecretFile; + // Version before mode, matching `readMap`. A version this build does + // not understand is refused there, so presenting such a file as a + // usable "plaintext" or "encrypted" store would describe it as working + // while every save fails. + if (parsed.version !== FORMAT_VERSION) { + return { + state: "unreadable", + detail: `format version ${String(parsed.version)}, expected ${FORMAT_VERSION}`, + }; + } if (parsed.encryption === "none") return { state: "plaintext" }; if (parsed.encryption === CIPHER) return { state: "encrypted" }; return { @@ -860,11 +870,12 @@ export class FileSecretStore implements SecretStore { /** * Best-effort permission repair for a pre-existing secrets file. * - * `writeStoreFile` passes `mode: 0o600`, but a mode argument only applies - * when the file is *created* — an atomic rename over a file someone made - * `0644` by hand (or restored from a backup, or copied out of an image) - * keeps the loose mode forever. Called once at store selection so the - * guarantee the docs make is the guarantee on disk. + * `writeStoreFile` writes through `atomically`, which creates a temp file + * with `mode: 0o600` and renames it over the destination — so every *write* + * re-establishes the mode regardless of what the old file had. What it + * cannot fix is a file that is never written: one made `0644` by hand, + * restored from a backup, or copied out of an image is read at that mode + * indefinitely. Called once at store selection to close exactly that gap. * * Failures are swallowed: a file we cannot chmod is one we probably cannot * write either, and `set` is where that gets reported with context. diff --git a/core/auth/node/secret-store-selection.ts b/core/auth/node/secret-store-selection.ts index 536e0761..6802a89b 100644 --- a/core/auth/node/secret-store-selection.ts +++ b/core/auth/node/secret-store-selection.ts @@ -49,6 +49,7 @@ import { secretStorageSummary, } from "../secret-storage-info.js"; import { + acquireLock, FileSecretStore, readSecretFilePermissions, tightenSecretFilePermissions, @@ -302,12 +303,12 @@ async function buildStore( // *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.`, - ); - } + // Repair only — the *reporting* belongs to the descriptor path, which + // knows whether the file is encrypted and so what the consequence + // actually is. This used to warn here too, in wording that claimed a + // reader could read the secrets even when the file was ciphertext, and + // duplicated the caveat `warnAboutSecretStorage` prints moments later. + await tightenSecretFilePermissions(filePath); return { store, info: await describeFileStore(store, filePath, kind, reason, detail), @@ -479,6 +480,34 @@ export async function absorbFileSecretsIntoKeyring( if (!fsSync.existsSync(filePath)) return; const file = new FileSecretStore({ filePath }); + // The whole read → copy → delete sequence runs under the same cross-process + // lock every mutation takes. Without it, another Inspector completing a + // `set` between our read and our delete has its brand-new secret removed + // and never copied — a write that reported success and then vanished, + // which is precisely the loss the lock exists to prevent, arrived at + // through the migration meant to preserve things. + let release: (() => Promise) | undefined; + try { + release = await acquireLock(filePath); + } catch (err) { + console.warn( + `\n[mcp-inspector] Could not lock the secrets file at ${filePath} to move it into the OS keychain, so it has been left in place: ${err instanceof Error ? err.message : String(err)}`, + ); + return; + } + try { + await handOffUnderLock(file, filePath, keyring); + } finally { + await release(); + } +} + +/** The hand-off body. Split out so the lock's release is unmissable. */ +async function handOffUnderLock( + file: FileSecretStore, + filePath: string, + keyring: SecretStore, +): Promise { let entries: Record | null; try { entries = await file.readAll(); @@ -490,10 +519,19 @@ export async function absorbFileSecretsIntoKeyring( } if (!entries || Object.keys(entries).length === 0) return; + // A key that is not `serverId:field` cannot be addressed through the store + // API, so it cannot be copied — which makes the hand-off incomplete by + // definition, and an incomplete hand-off must not delete its source. It + // used to be skipped and the file removed anyway, discarding the value + // permanently to tidy up a file we had failed to fully migrate. + let complete = true; try { for (const [account, value] of Object.entries(entries)) { const parsed = parseAccount(account); - if (!parsed) continue; + if (!parsed) { + complete = false; + continue; + } // Strict: `get` answers `null` for an unreadable keychain as well as // for a missing entry, and this branch *writes* on `null`. A transient // read failure would therefore overwrite a newer keychain value with @@ -516,6 +554,13 @@ export async function absorbFileSecretsIntoKeyring( return; } + if (!complete) { + console.warn( + `\n[mcp-inspector] Secrets from ${filePath} have been copied into the OS keychain, but at least one entry could not be read (its key is not in \`serverId:field\` form). The file has been left in place so nothing is lost.`, + ); + return; + } + try { await fs.rm(filePath, { force: true }); console.warn(