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(