diff --git a/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.test.tsx b/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.test.tsx index 2f6f4e40..7ce8c291 100644 --- a/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.test.tsx +++ b/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.test.tsx @@ -471,3 +471,42 @@ describe("ServerConfigModal", () => { expect(props.onClose).toHaveBeenCalledOnce(); }); }); + +describe("secret-storage footer (#1950 review r20)", () => { + it("discloses where stdio env values will be kept", () => { + // This dialog takes stdio `env` values, which are extracted into the + // secret store exactly as Server Settings' are — so it is a secret-entry + // surface. It had no disclosure at all, which made it the one place a + // user could type a secret without being told where it goes. + renderWithMantine( + , + ); + expect(screen.getByTestId("secret-storage-footer")).toHaveTextContent( + "Secrets: Memory (this session only)", + ); + }); + + it("renders no footer when the backend didn't report a store", () => { + renderWithMantine( + , + ); + expect(screen.queryByTestId("secret-storage-footer")).toBeNull(); + }); +}); diff --git a/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.tsx b/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.tsx index e1e823a6..c89c2e06 100644 --- a/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.tsx +++ b/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.tsx @@ -10,6 +10,8 @@ import { Textarea, } from "@mantine/core"; import { ClearButton } from "../../elements/ClearButton/ClearButton"; +import { SecretStorageFooter } from "../../elements/SecretStorageFooter/SecretStorageFooter"; +import type { SecretStorageInfo } from "@inspector/core/auth/secret-storage-info.js"; import { useValueChange } from "../../../hooks/useValueChange"; import type { MCPServerConfig, @@ -33,6 +35,14 @@ export interface ServerConfigModalProps { existingIds: string[]; onClose: () => void; onSubmit: (id: string, config: MCPServerConfig) => Promise | void; + /** + * Where secrets typed here end up (#1950 review r20). This dialog takes + * stdio `env` values, which are extracted into the secret store exactly as + * the ones in Server Settings are — so it is a secret-entry surface and + * needs the same permanent disclosure. It had none, which meant a user + * could type a value into the one dialog that never said where it goes. + */ + secretStorage?: SecretStorageInfo; } type TransportChoice = "stdio" | "sse" | "streamable-http"; @@ -188,6 +198,7 @@ export function ServerConfigModal({ existingIds, onClose, onSubmit, + secretStorage, }: ServerConfigModalProps) { const initial = useMemo( () => configToFormState(initialId, initialConfig, mode), @@ -446,6 +457,7 @@ export function ServerConfigModal({ + ); } diff --git a/clients/web/src/test/core/client/node-persistence.test.ts b/clients/web/src/test/core/client/node-persistence.test.ts index 00ed8461..238e0fc2 100644 --- a/clients/web/src/test/core/client/node-persistence.test.ts +++ b/clients/web/src/test/core/client/node-persistence.test.ts @@ -235,6 +235,22 @@ describe("session-scoped store keeps client.json durable (#1950 review r19)", () const dir = await fs.mkdtemp(path.join(os.tmpdir(), "client-durable-")); const file = path.join(dir, "client.json"); try { + // Legacy state: the secret is *already* on disk in plaintext, which is + // what the durability guard exists to preserve. + await fs.writeFile( + file, + JSON.stringify({ + enterpriseManagedAuth: { + enabled: true, + idp: { + issuer: "https://idp.example/", + clientId: "cid", + clientSecret: "must-survive", + }, + }, + }), + "utf-8", + ); await writeClientConfigStore( file, { @@ -255,6 +271,79 @@ describe("session-scoped store keeps client.json durable (#1950 review r19)", () } }); + it("does not write a newly entered IdP secret to disk on a session store", async () => { + // Same overshoot as the server path: preserving *legacy* plaintext is + // right, treating every submitted value as legacy is not — it would put a + // freshly typed secret in `client.json` while the footer says a session + // store writes secrets nowhere. + const dir = await fs.mkdtemp(path.join(os.tmpdir(), "client-fresh-")); + const file = path.join(dir, "client.json"); + try { + await writeClientConfigStore( + file, + { + enterpriseManagedAuth: { + enabled: true, + idp: { + issuer: "https://idp.example/", + clientId: "cid", + clientSecret: "never-typed-before", + }, + }, + }, + new SessionSecretStore(), + ); + expect(await fs.readFile(file, "utf-8")).not.toContain( + "never-typed-before", + ); + } finally { + await fs.rm(dir, { recursive: true, force: true }); + } + }); + + it("keeps a new secret off disk when the prior file is unreadable", async () => { + // Provenance cannot be established from a file that will not parse, and + // the conservative direction is to treat the value as new — writing a + // secret to disk on a guess is the failure that matters. + const dir = await fs.mkdtemp(path.join(os.tmpdir(), "client-bad-")); + const file = path.join(dir, "client.json"); + try { + await fs.writeFile(file, "{ not json", "utf-8"); + await writeClientConfigStore( + file, + { + enterpriseManagedAuth: { + enabled: true, + idp: { + issuer: "https://idp.example/", + clientId: "cid", + clientSecret: "unprovenanced", + }, + }, + }, + new SessionSecretStore(), + ); + expect(await fs.readFile(file, "utf-8")).not.toContain("unprovenanced"); + } finally { + await fs.rm(dir, { recursive: true, force: true }); + } + }); + + it("writes nothing extra when a session store has no IdP secret at all", async () => { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), "client-none-")); + const file = path.join(dir, "client.json"); + try { + await writeClientConfigStore( + file, + { cimd: { enabled: true, clientMetadataUrl: "https://x.test/cimd" } }, + new SessionSecretStore(), + ); + expect(await fs.readFile(file, "utf-8")).toContain("cimd"); + } finally { + await fs.rm(dir, { recursive: true, force: true }); + } + }); + it("still strips against a durable store, so the guard is the difference", async () => { const dir = await fs.mkdtemp(path.join(os.tmpdir(), "client-durable-")); const file = path.join(dir, "client.json"); 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 f93b064d..7fb03da4 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 @@ -857,6 +857,106 @@ describe("absorbFileSecretsIntoKeyring", () => { ); }); + it("reports a claim that fails for a reason other than a lost race", async () => { + // ENOENT means someone else claimed it. Anything else — EACCES, EROFS — + // leaves the file where it is, and returning quietly would select the + // keychain while file-backed secrets sit there invisible to everything, + // with nothing said. + const filePath = await seedFile({ "srv:oauthClientSecret": "stuck" }); + process.env.MCP_INSPECTOR_SECRET_FILE = filePath; + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + + // Mocked before the module loads: it captures its `fs` binding at import, + // so spying on the namespace afterwards does not reach it. + vi.resetModules(); + vi.doMock("node:fs/promises", async () => { + const actual = + await vi.importActual( + "node:fs/promises", + ); + return { + ...actual, + default: actual, + rename: async (from: string, to: string) => { + if (String(to).includes(".migrating-")) { + throw Object.assign(new Error("read-only file system"), { + code: "EROFS", + }); + } + return actual.rename(from, to); + }, + }; + }); + try { + const mod = await loadWithProbe(true); + await mod.absorbFileSecretsIntoKeyring(new InMemorySecretStore()); + expect(existsSync(filePath)).toBe(true); + expect(warn.mock.calls.flat().join("\n")).toMatch( + /Could not claim the secrets file .*EROFS/, + ); + } finally { + vi.doUnmock("node:fs/promises"); + vi.resetModules(); + } + }); + + it("deferred store forwards isDurable and setMany to the resolved store", async () => { + // Both are optional seams, and an unforwarded one degrades silently — the + // exact defect that made `getStrict` inert in production for two rounds. + process.env.MCP_INSPECTOR_SECRET_STORE = "memory"; + vi.spyOn(console, "warn").mockImplementation(() => {}); + const mod = await loadWithProbe(true); + const store = mod.defaultSecretStore(); + + // The memory store reports itself session-scoped; a store that dropped + // the call would answer the `true` default instead. + expect(await store.isDurable?.()).toBe(false); + + await store.setMany?.("srv", { "env:A": "1", "env:B": "2" }); + expect(await store.get("srv", "env:A")).toBe("1"); + expect(await store.get("srv", "env:B")).toBe("2"); + }); + + it("recovers a snapshot left by a process that died mid-migration", async () => { + // The claim protects the delete, and introduced a way to lose everything: + // a crash between the rename and the copy leaves only + // `secrets.json.migrating-`, and checking the canonical path alone + // then reports "nothing to migrate" while every stored credential + // silently disappears. + const filePath = await seedFile({ "srv:oauthClientSecret": "orphaned" }); + const orphan = `${filePath}.migrating-999999`; + await fs.rename(filePath, orphan); + 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); + + // Adopted and migrated, not abandoned. + expect(await keyring.get("srv", "oauthClientSecret")).toBe("orphaned"); + expect(existsSync(orphan)).toBe(false); + }); + + it("leaves an orphan alone when a live secrets file already exists", async () => { + // The live file is newer than any orphan and must not be replaced. The + // orphan may hold values it does not, so it is reported rather than + // removed — deciding a merge is not this function's job. + const filePath = await seedFile({ "srv:oauthClientSecret": "current" }); + const orphan = `${filePath}.migrating-999999`; + await fs.copyFile(filePath, orphan); + process.env.MCP_INSPECTOR_SECRET_FILE = filePath; + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const mod = await loadWithProbe(true); + + await mod.absorbFileSecretsIntoKeyring(new InMemorySecretStore()); + + expect(existsSync(orphan)).toBe(true); + expect(warn.mock.calls.flat().join("\n")).toMatch( + /interrupted migration at .*migrating-999999/, + ); + }); + it("claims the file atomically, so a later write is not deleted", async () => { // Round 19: the pre-delete comparison was a check-to-delete race. A // writer completing a `set` *after* the comparison and before the `rm` diff --git a/clients/web/src/test/integration/mcp/remote/servers-route.test.ts b/clients/web/src/test/integration/mcp/remote/servers-route.test.ts index 8d5dc9d6..d0c4668a 100644 --- a/clients/web/src/test/integration/mcp/remote/servers-route.test.ts +++ b/clients/web/src/test/integration/mcp/remote/servers-route.test.ts @@ -2784,6 +2784,49 @@ describe("plaintext migration against a session-scoped store (#1950)", () => { expect(readFileSync(configPath, "utf-8")).toContain("45000"); }); + it("does not write a NEWLY entered secret to disk on a session store", async () => { + // The overshoot in the previous round's fix. Preserving legacy plaintext + // is right; using the whole submitted entry as a proxy for "legacy" is + // not — it turned `MCP_INSPECTOR_SECRET_STORE=memory` into "write every + // new secret to mcp.json in the clear" while the footer said secrets are + // written nowhere. + const freshPath = join(tempDir, "fresh.json"); + writeFileSync(freshPath, JSON.stringify({ mcpServers: {} })); + const { app } = createRemoteApp({ + dangerouslyOmitAuth: true, + mcpConfigPath: freshPath, + initialConfig: { defaultEnvironment: {} }, + secretStore: new SessionSecretStore(), + }); + + const res = await app.request( + new Request("http://test/api/servers", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + id: "newsrv", + config: { type: "streamable-http", url: "https://y.test/mcp" }, + settings: { + headers: [], + env: [], + metadata: [], + connectionTimeout: 30000, + requestTimeout: 60000, + taskTtl: 60000, + maxFetchRequests: 1000, + roots: [], + oauthClientId: "cid", + oauthClientSecret: "never-typed-before", + }, + }), + }), + ); + expect(res.status).toBe(200); + expect(readFileSync(freshPath, "utf-8")).not.toContain( + "never-typed-before", + ); + }); + it("aborts the migration when the keychain read fails, preserving mcp.json", async () => { // The keychain-wins lookup used the tolerant `get`, so an unreadable // keychain read as "nothing there" — and the write that followed would diff --git a/core/auth/node/secret-store-selection.ts b/core/auth/node/secret-store-selection.ts index bda19a36..8e06ffb1 100644 --- a/core/auth/node/secret-store-selection.ts +++ b/core/auth/node/secret-store-selection.ts @@ -530,6 +530,14 @@ export async function absorbFileSecretsIntoKeyring( keyring: SecretStore, ): Promise { const filePath = defaultSecretFilePath(); + + // A crash between the claim and the delete leaves only + // `secrets.json.migrating-`. Checking the canonical path alone then + // reports "nothing to migrate", the keychain is selected, and every stored + // credential silently disappears — the claim protecting the delete having + // introduced a way to lose everything. Adopt any orphan first. + await recoverOrphanedSnapshots(filePath); + if (!fsSync.existsSync(filePath)) return; // **Claim the file atomically before reading it.** Comparing its contents @@ -550,9 +558,17 @@ export async function absorbFileSecretsIntoKeyring( const staged = `${filePath}.migrating-${process.pid}`; try { await fs.rename(filePath, staged); - } catch { - // Someone else claimed it first, or it vanished. Either way there is - // nothing here to migrate and no file of ours to clean up. + } catch (err) { + const code = (err as NodeJS.ErrnoException).code; + if (code !== "ENOENT") { + // Only ENOENT means someone else claimed it. Anything else — EACCES on + // the directory, EROFS — leaves the file exactly where it is, and + // returning quietly would select the keychain while file-backed + // secrets sit there unreadable by anything, with nothing said. + console.warn( + `\n[mcp-inspector] Could not claim the secrets file at ${filePath} for migration into the OS keychain (${code ?? "unknown error"}), so it has been left in place. Its secrets are not visible to this session.`, + ); + } return; } @@ -567,10 +583,14 @@ export async function absorbFileSecretsIntoKeyring( console.warn( `\n[mcp-inspector] The OS keychain is available again. Secrets kept in ${filePath} while it was not have been moved into the keychain, and the file has been removed.`, ); - } catch { - // Every value is in the keychain; only the now-redundant snapshot - // remains. Not worth failing startup over, and keychain-wins makes the - // next attempt harmless. + } catch (err) { + // Every value is in the keychain, so this is not worth failing startup + // over — but the snapshot is a *plaintext-capable copy of the secrets* + // and the app is about to report the keychain as the only store. A + // silent catch leaves a file the user cannot know to delete. + console.warn( + `\n[mcp-inspector] Secrets were moved into the OS keychain, but the temporary copy at ${staged} could not be removed (${err instanceof Error ? err.message : String(err)}). Delete it once you have confirmed the keychain has your secrets.`, + ); } return; } @@ -580,18 +600,58 @@ export async function absorbFileSecretsIntoKeyring( // *failed* migration lose the file, which is what the claim exists to // prevent one line further down. try { - await fs.rename(staged, filePath); + // `link` and not `rename`: POSIX `rename` **replaces** its destination + // silently, so restoring with it would overwrite a `secrets.json` that a + // concurrent writer recreated while we held the snapshot — destroying a + // later successful write, which is the exact loss the claim exists to + // prevent. `link` fails with EEXIST instead, which is the no-clobber + // primitive this needs; the snapshot is unlinked only once the live path + // holds the same inode. + await fs.link(staged, filePath); + await fs.rm(staged, { force: true }); } catch { - // The live path is occupied — a writer recreated it while we held the - // snapshot — so restoring would overwrite a newer file. Leave the - // snapshot and say exactly where it is: an operator-visible orphan is - // recoverable, and silently clobbering someone's write is not. + // The live path is occupied, or hard links are unavailable here. Leave + // the snapshot and say exactly where it is: an operator-visible orphan + // is recoverable, and clobbering someone's write is not. console.warn( - `\n[mcp-inspector] Could not restore ${staged} to ${filePath} because a new secrets file was written while the migration ran. Both files exist; the snapshot holds the secrets that were not migrated.`, + `\n[mcp-inspector] Could not restore ${staged} to ${filePath}; both files exist and the snapshot holds the secrets that were not migrated. Merge it by hand, or delete it once you have confirmed nothing is missing.`, ); } } +/** + * Adopt a snapshot left behind by a process that died mid-migration. + * + * Renamed back only when the canonical path is free — a live `secrets.json` + * is newer than any orphan and must not be replaced. Otherwise the orphan is + * reported rather than removed: it may hold values the live file does not, + * and this is not the place to decide a merge. + */ +async function recoverOrphanedSnapshots(filePath: string): Promise { + const dir = path.dirname(filePath); + const prefix = `${path.basename(filePath)}.migrating-`; + let names: string[]; + try { + names = (await fs.readdir(dir)).filter((n) => n.startsWith(prefix)); + } catch { + return; // No directory yet, or unreadable — nothing to recover. + } + for (const name of names) { + const orphan = path.join(dir, name); + try { + await fs.link(orphan, filePath); + await fs.rm(orphan, { force: true }); + console.warn( + `\n[mcp-inspector] Recovered secrets left behind by an interrupted migration (${orphan} → ${filePath}).`, + ); + } catch { + console.warn( + `\n[mcp-inspector] Found secrets from an interrupted migration at ${orphan}, and a current ${filePath} already exists, so it was left alone. It may hold values the current file does not.`, + ); + } + } +} + /** * The hand-off body: read the file, copy every entry into the keychain, * then delete the file. diff --git a/core/client/node-persistence.ts b/core/client/node-persistence.ts index 859f620b..86887dd6 100644 --- a/core/client/node-persistence.ts +++ b/core/client/node-persistence.ts @@ -131,10 +131,54 @@ export async function writeClientConfigStore( const durable = await secretStoreIsDurable(secretStore); await writeStoreFile( filePath, - serializeStore(durable ? stripped : validated), + serializeStore( + durable + ? stripped + : await preserveLegacyPlaintext( + filePath, + validated, + stripped, + idpSecret, + ), + ), ); } +/** + * For a session-scoped store: keep the IdP secret on disk only when it was + * **already there, unchanged**. + * + * Stripping legacy plaintext would move the only durable copy into RAM (the + * read-path migration withholds its strip for exactly this reason, and the + * write path has to agree). But writing a *newly entered* secret to disk + * would contradict the footer, which promises a session store writes secrets + * nowhere — so provenance decides, not the presence of a value in the + * submitted body. + */ +async function preserveLegacyPlaintext( + filePath: string, + validated: ClientConfig, + stripped: ClientConfig, + idpSecret: string | undefined, +): Promise { + if (!idpSecret) return stripped; + try { + const raw = await readStoreFile(filePath); + if (raw === null) return stripped; + const prior = parseClientConfig(parseStore(raw)); + const priorSecret = + extractSecretsFromClientConfig(prior).secrets[ + SECRET_FIELD_IDP_CLIENT_SECRET + ]; + return priorSecret === idpSecret ? validated : stripped; + } catch { + // Unreadable or unparseable prior file: treat the value as new, which is + // the conservative direction — it keeps the secret off disk rather than + // writing it there on a guess. + return stripped; + } +} + /** Remove client.json and the install-level IdP secret from the keychain. */ export async function deleteClientConfigStore( filePath: string, diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index b6205180..1f3fec0d 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -2116,8 +2116,30 @@ export function createRemoteApp( const entryForDisk = async ( built: StoredMCPServer, stripped: StoredMCPServer, - ): Promise => - (await secretStoreIsDurable(secretStore)) ? stripped : built; + previous?: StoredMCPServer, + ): Promise => { + if (await secretStoreIsDurable(secretStore)) return stripped; + // Non-durable, and the distinction is the whole of it: **legacy plaintext + // that was already on disk** is preserved, because stripping it would + // move the only durable copy into RAM. A **newly entered** secret is not, + // because the footer promises that a session store writes secrets + // nowhere — and the first version of this used the whole submitted entry + // as a proxy for provenance, which turned `MCP_INSPECTOR_SECRET_STORE= + // memory` into "write every new secret to mcp.json in the clear" while + // the UI said the opposite. + // + // A changed value is new: the old plaintext goes, the new value lives in + // the session store, and the footer's promise holds for it. + const prior = previous ? extractSecretsFromStored(previous).secrets : {}; + const submitted = extractSecretsFromStored(built).secrets; + const carry: Record = {}; + for (const [field, value] of Object.entries(submitted)) { + if (prior[field] === value) carry[field] = value; + } + return Object.keys(carry).length > 0 + ? mergeSecretsIntoStored(stripped, carry) + : stripped; + }; const keychainErrorResponse = ( c: Context, @@ -2534,7 +2556,7 @@ export function createRemoteApp( } } const { stripped, secrets } = extractSecretsFromStored(built); - const onDisk = await entryForDisk(built, stripped); + const onDisk = await entryForDisk(built, stripped, existing); const next: MCPConfig = { mcpServers: {} }; for (const [key, val] of Object.entries(current.mcpServers)) { if (key === originalId) {