diff --git a/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.stories.tsx b/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.stories.tsx index 6b0ce287..38d7a245 100644 --- a/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.stories.tsx +++ b/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.stories.tsx @@ -80,10 +80,10 @@ export const InMemory: Story = { play: async ({ canvasElement }) => { const canvas = within(canvasElement); const band = await canvas.findByTestId("secret-storage-footer"); - await expect(band).toHaveTextContent("Secrets: Memory (this session only)"); - await expect( - canvas.getByText(/lost when the Inspector exits/), - ).toBeInTheDocument(); + await expect(band).toHaveTextContent( + "Secrets: Memory (this session only):", + ); + await expect(canvas.getByText(/lost on exit/)).toBeInTheDocument(); }, }; diff --git a/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.test.tsx b/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.test.tsx index 90548f38..a18a7d0b 100644 --- a/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.test.tsx +++ b/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.test.tsx @@ -62,10 +62,8 @@ describe("SecretStorageFooter", () => { it("warns that an in-memory store loses secrets on exit", () => { renderWithMantine(); const band = screen.getByTestId("secret-storage-footer"); - expect(band).toHaveTextContent("Secrets: Memory (this session only)"); - expect( - screen.getByText(/lost when the Inspector exits/), - ).toBeInTheDocument(); + expect(band).toHaveTextContent("Secrets: Memory (this session only):"); + expect(screen.getByText(/lost on exit/)).toBeInTheDocument(); expect(band).toHaveAttribute("data-tone", "warn"); }); diff --git a/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.tsx b/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.tsx index 802e530a..74e81039 100644 --- a/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.tsx +++ b/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.tsx @@ -144,9 +144,14 @@ export function SecretStorageFooter({ info }: SecretStorageFooterProps) { `.withProps()` rule, and it takes `color` as a prop rather than a style. */} + {/* The colon lives here rather than in `secretStorageLabel`, because the + label is also used on its own — by `secretStorageSummary` for the + startup banner, where a trailing colon before " at " would be + wrong. It appears only when something follows it. */} Secrets:{" "} {secretStorageLabel(info)} + {detail ? ":" : ""} {detail && {detail}} diff --git a/clients/web/src/test/core/auth/secret-storage-info.test.ts b/clients/web/src/test/core/auth/secret-storage-info.test.ts index c21afe43..5bfcd48b 100644 --- a/clients/web/src/test/core/auth/secret-storage-info.test.ts +++ b/clients/web/src/test/core/auth/secret-storage-info.test.ts @@ -77,7 +77,7 @@ describe("secretStorageCaveat", () => { it("names the loss for memory", () => { expect(secretStorageCaveat(memory)).toContain( - "lost when the Inspector exits", + "lost on exit", ); }); @@ -127,7 +127,7 @@ describe("secretStorageCaveat", () => { // Defensive ordering: `plaintext` is meaningless for memory, and the // durability loss is the more consequential of the two statements. expect(secretStorageCaveat({ ...memory, plaintext: true })).toContain( - "lost when the Inspector exits", + "lost on exit", ); }); }); 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 d9d9616e..9670ac44 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 @@ -18,6 +18,7 @@ * which is what makes them worth exporting. */ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import { existsSync } from "node:fs"; import * as fs from "node:fs/promises"; import * as os from "node:os"; import * as path from "node:path"; @@ -27,6 +28,10 @@ import { parseSecretStoreEnv, warnAboutSecretStorage, } from "@inspector/core/auth/node/secret-store-selection.js"; +import { + InMemorySecretStore, + type SecretStore, +} from "@inspector/core/auth/node/secret-store.js"; import type { SecretStorageInfo } from "@inspector/core/auth/secret-storage-info.js"; const ENV_KEYS = [ @@ -384,7 +389,7 @@ describe("warnAboutSecretStorage", () => { expect(output).toContain("MCP_INSPECTOR_SECRET_STORE"); // The lossy-store caveat rides along — this is the "loud" half of // "automatic fallback with a loud banner". - expect(output).toContain("lost when the Inspector exits"); + expect(output).toContain("lost on exit"); }); it("warns about an unencrypted file even when it was explicitly chosen", () => { @@ -540,6 +545,107 @@ describe("defaultSecretFilePath", () => { }); }); +describe("absorbFileSecretsIntoKeyring", () => { + // The transition: a box without libsecret falls back to a file, the user + // saves secrets there, then installs libsecret. Without a hand-off the next + // run selects the keychain and every saved value silently disappears — + // still on disk, but read by nothing. + async function seedFile(secrets: Record): Promise { + const filePath = path.join(tmpDir, "secrets.json"); + await fs.writeFile( + filePath, + JSON.stringify({ version: 1, encryption: "none", secrets }), + "utf-8", + ); + return filePath; + } + + it("moves the file's secrets into the keychain and removes the file", async () => { + 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 keyring = new InMemorySecretStore(); + + await mod.absorbFileSecretsIntoKeyring(keyring); + + expect(await keyring.get("srv", "oauthClientSecret")).toBe("from-file"); + expect(existsSync(filePath)).toBe(false); + }); + + it("lets the keychain win on conflict", async () => { + // The file's copy is the older one by construction — it was written while + // the keychain was unreachable. + 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 keyring = new InMemorySecretStore(); + await keyring.set("srv", "oauthClientSecret", "already-here"); + + await mod.absorbFileSecretsIntoKeyring(keyring); + + expect(await keyring.get("srv", "oauthClientSecret")).toBe("already-here"); + }); + + it("leaves the file alone when the keychain write fails", async () => { + // A partial hand-off must not delete the source: the next run retries and + // the keychain-wins rule absorbs whatever already made it across. + const filePath = await seedFile({ "srv:oauthClientSecret": "from-file" }); + process.env.MCP_INSPECTOR_SECRET_FILE = filePath; + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const mod = await loadWithProbe(true); + const failing: SecretStore = { + get: async () => null, + set: async () => { + throw new Error("keychain exploded"); + }, + delete: async () => {}, + deleteAllForServer: async () => {}, + }; + + await mod.absorbFileSecretsIntoKeyring(failing); + + expect(existsSync(filePath)).toBe(true); + expect(warn).toHaveBeenCalledWith(expect.stringContaining("left in place")); + }); + + it("does not delete a file it cannot read", async () => { + // An undecryptable file is not an empty one. Deleting it here would lose + // exactly the values this whole hand-off exists to preserve. + const filePath = path.join(tmpDir, "secrets.json"); + process.env.MCP_INSPECTOR_SECRET_FILE = filePath; + process.env.MCP_INSPECTOR_SECRET_KEY = "right-key"; + const seeded = await loadWithProbe(false); + vi.spyOn(console, "warn").mockImplementation(() => {}); + await new ( + await import("@inspector/core/auth/node/file-secret-store.js") + ).FileSecretStore({ filePath, passphrase: "right-key" }).set( + "srv", + "oauthClientSecret", + "encrypted", + ); + void seeded; + + delete process.env.MCP_INSPECTOR_SECRET_KEY; + const mod = await loadWithProbe(true); + const keyring = new InMemorySecretStore(); + + await mod.absorbFileSecretsIntoKeyring(keyring); + + expect(existsSync(filePath)).toBe(true); + expect(await keyring.get("srv", "oauthClientSecret")).toBe(null); + }); + + it("is a no-op when there is no file", async () => { + process.env.MCP_INSPECTOR_SECRET_FILE = path.join(tmpDir, "absent.json"); + const mod = await loadWithProbe(true); + const keyring = new InMemorySecretStore(); + await mod.absorbFileSecretsIntoKeyring(keyring); + expect(existsSync(path.join(tmpDir, "absent.json"))).toBe(false); + }); +}); + describe("the descriptor reports a mode it could not tighten", () => { it("carries looseMode into the descriptor and warns once at selection", async () => { // The warning alone was not enough: the caveat under the secret field diff --git a/core/auth/secret-storage-info.ts b/core/auth/secret-storage-info.ts index d1b42ab6..5435222c 100644 --- a/core/auth/secret-storage-info.ts +++ b/core/auth/secret-storage-info.ts @@ -121,14 +121,14 @@ export function secretStorageTone(info: SecretStorageInfo): SecretStorageTone { /** * One sentence of consequence, or undefined when there is none worth - * saying. Consequence, not mechanism: "will be lost when the Inspector - * exits" is what a user can act on; "uses an in-memory Map" is not. + * saying. Consequence, not mechanism: "lost on exit" is what a user can + * act on; "uses an in-memory Map" is not. */ export function secretStorageCaveat( info: SecretStorageInfo, ): string | undefined { if (info.kind === "memory") { - return "Secrets are not written anywhere and are lost when the Inspector exits."; + return "Secrets are not written anywhere and are lost on exit."; } // The permission problem outranks the encryption one when both hold: a // file other users can read is a live exposure, while "unencrypted" is a