diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index ba7dca8b..9e764b6b 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -111,6 +111,7 @@ import { useMessageLog } from "@inspector/core/react/useMessageLog.js"; import { useFetchRequestLog } from "@inspector/core/react/useFetchRequestLog.js"; import { useStderrLog } from "@inspector/core/react/useStderrLog.js"; import { useInitialConfig } from "@inspector/core/react/useInitialConfig.js"; +import { refreshingPersist } from "./utils/refreshingPersist"; import { usePendingClientRequests } from "@inspector/core/react/usePendingClientRequests.js"; import { InspectorView } from "./components/views/InspectorView/InspectorView"; import type { @@ -3995,16 +3996,14 @@ function App() { targetId: settingsModalTargetId, resolveInitial: (id) => servers.find((s) => s.id === id)?.settings ?? EMPTY_SETTINGS, - onPersist: async (id: string, settings: InspectorServerSettings) => { - await updateServerSettings(id, settings); - // The saved settings may include an OAuth client secret or a stdio - // `env:` value, and writing one can change what the secrets file *is* - // — the first save under a newly-set passphrase re-encrypts a - // pre-existing plaintext file. The backend re-derives the descriptor - // per request, so ask again; otherwise the footer keeps reporting the - // state from page load for the rest of the session (#1950 review r14). - refreshInitialConfig(); - }, + // The saved settings may include an OAuth client secret or a stdio `env:` + // value, and writing one can change what the secrets file *is* — the first + // save under a newly-set passphrase re-encrypts a pre-existing plaintext + // file. `refreshingPersist` re-asks the backend afterwards; it is a named + // unit rather than an inline `await …; refresh()` because this file is + // outside the coverage gate, so wiring written here is tested by nothing + // (#1950 review r17). + onPersist: refreshingPersist(updateServerSettings, refreshInitialConfig), // Surface failures via toast — the modal usually closes // immediately on user dismiss, so a silent fail-on-flush would // leave the user thinking their last edits saved when they @@ -4028,7 +4027,10 @@ function App() { } = useClientSettingsDraft({ opened: clientSettingsOpen, resolveInitial: () => clientConfigToFormValues(clientConfig), - onPersist: async (values) => { + // Wrapped for the same reason as the server-settings persist above: this + // one can carry the enterprise IdP client secret, and that write is what + // flips a pending-encryption file to encrypted. + onPersist: refreshingPersist(async (values) => { if (!canPersistClientSettingsDraft(values)) return; const next = formValuesToClientConfig(values); await saveClientConfigRemote(next, { @@ -4036,11 +4038,7 @@ function App() { authToken: getAuthToken(), }); setClientConfig(next); - // Same reason as the server-settings persist: this can carry the - // enterprise IdP client secret, and that write is what flips a - // pending-encryption file to encrypted. - refreshInitialConfig(); - }, + }, refreshInitialConfig), onError: (err) => { notifications.show({ title: "Failed to save client settings", 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 8b49417d..e4158986 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 @@ -582,6 +582,25 @@ describe("defaultSecretFilePath", () => { expect(path.isAbsolute(mod.defaultSecretFilePath())).toBe(true); }); + it("is absolute even when HOME and USERPROFILE are both unset", async () => { + // A service started with a minimal environment has neither, and the + // `"."` fallback then yields a *relative* path — while + // `FileSecretStoreOptions.filePath` and the `SecretStorageInfo.path` the + // footer offers to copy both promise an absolute one. A copied location + // that means nothing without knowing the process's cwd is not a location. + const savedHome = process.env.HOME; + const savedProfile = process.env.USERPROFILE; + delete process.env.HOME; + delete process.env.USERPROFILE; + try { + const mod = await loadWithProbe(true); + expect(path.isAbsolute(mod.defaultSecretFilePath())).toBe(true); + } finally { + if (savedHome !== undefined) process.env.HOME = savedHome; + if (savedProfile !== undefined) process.env.USERPROFILE = savedProfile; + } + }); + it("defaults to ~/.mcp-inspector/secrets.json", async () => { const mod = await loadWithProbe(true); expect(mod.defaultSecretFilePath()).toContain( 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 6062fd7d..a47b5710 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 @@ -4,7 +4,7 @@ * exercises the routes via real HTTP. */ -import { describe, it, expect, afterEach, beforeEach } from "vitest"; +import { describe, it, expect, afterEach, beforeEach, vi } from "vitest"; import { mkdtempSync, readFileSync, @@ -2674,6 +2674,52 @@ describe("plaintext migration against a session-scoped store (#1950)", () => { expect(readFileSync(configPath, "utf-8")).toBe(before); }); + it("warns only when something is actually preserved, and only once", async () => { + // The warning used to fire on every `/api/servers` read whenever the + // store was session-scoped — including for the default empty catalog, + // where "plaintext values are left on disk" is simply false, repeated on + // every list refresh. A log line that is usually untrue is one people + // learn to skip, which costs it the occasion it matters. + const warn = vi.fn(); + const logger = { + warn, + info: vi.fn(), + error: vi.fn(), + debug: vi.fn(), + } as unknown as Parameters[0]["logger"]; + const sessionMessage = /kept in memory for this session/; + + // A catalog with no plaintext secrets: nothing is being preserved. + const emptyPath = join(tempDir, "empty.json"); + writeFileSync(emptyPath, JSON.stringify({ mcpServers: {} })); + const empty = createRemoteApp({ + dangerouslyOmitAuth: true, + mcpConfigPath: emptyPath, + initialConfig: { defaultEnvironment: {} }, + secretStore: new SessionSecretStore(), + logger, + }); + await empty.app.request(new Request("http://test/api/servers")); + await empty.app.request(new Request("http://test/api/servers")); + expect(warn.mock.calls.flat().join(" ").match(sessionMessage)).toBeNull(); + + // And with a plaintext secret: said once, not once per read. + warn.mockClear(); + const withSecret = createRemoteApp({ + dangerouslyOmitAuth: true, + mcpConfigPath: configPath, + initialConfig: { defaultEnvironment: {} }, + secretStore: new SessionSecretStore(), + logger, + }); + await withSecret.app.request(new Request("http://test/api/servers")); + await withSecret.app.request(new Request("http://test/api/servers")); + const said = warn.mock.calls.filter((c: unknown[]) => + sessionMessage.test(c.map(String).join(" ")), + ); + expect(said).toHaveLength(1); + }); + 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 f9f94de1..1faaef79 100644 --- a/core/auth/node/secret-store-selection.ts +++ b/core/auth/node/secret-store-selection.ts @@ -117,7 +117,13 @@ export function defaultSecretFilePath(): string { const storageDir = process.env[STORAGE_DIR_ENV]?.trim(); if (storageDir) return path.resolve(storageDir, "secrets.json"); const homeDir = process.env.HOME || process.env.USERPROFILE || "."; - return path.join(homeDir, ".mcp-inspector", "secrets.json"); + // Resolved like the two override branches. A service started with a minimal + // environment has neither variable, and the `"."` fallback then yields + // `.mcp-inspector/secrets.json` — a *relative* path, while + // `FileSecretStoreOptions.filePath` and the `SecretStorageInfo.path` the UI + // copies both promise an absolute one. The footer would offer to copy a + // location that means nothing without knowing the process's cwd. + return path.resolve(homeDir, ".mcp-inspector", "secrets.json"); } /** diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index 10403aa4..7d5a9c9f 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -1940,17 +1940,28 @@ export function createRemoteApp( // it and abandon the migration for this GET — the on-disk file stays // as-is so the user's secret isn't lost, and the next GET retries // (e.g. after the user installs libsecret). + // Warned lazily, at most once per server instance, and only when there is + // actually something being preserved. Announcing it up front meant every + // `/api/servers` read on a session-scoped store logged that plaintext + // values were left on disk — including for the default empty catalog, + // where the statement is simply false, repeated on every list refresh. + // Hoisted out of the function so it is once per process rather than once + // per read. + let warnedSessionPreserved = false; + const migratePlaintextSecrets = async ( config: MCPConfig, ): Promise<{ migrated: MCPConfig; changed: boolean }> => { let changed = false; const next: MCPConfig = { mcpServers: {} }; const durable = await secretStoreIsDurable(secretStore); - if (!durable && fileLogger) { + const warnSessionPreserved = () => { + if (warnedSessionPreserved || durable || !fileLogger) return; + warnedSessionPreserved = true; fileLogger.warn( "Secrets are kept in memory for this session, so plaintext values in mcp.json are left on disk rather than migrated away. They would otherwise be lost when the Inspector exits.", ); - } + }; try { for (const [id, stored] of Object.entries(config.mcpServers)) { const { stripped, secrets } = extractSecretsFromStored(stored); @@ -1982,6 +1993,7 @@ export function createRemoteApp( // loaded into the store above, so this session behaves normally; // only the disk delete is withheld. if (!durable) { + warnSessionPreserved(); next.mcpServers[id] = stored; continue; }