From af3d940e7883a27fc8f31cadb1d045b54a066979 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 22 Aug 2026 23:30:17 -0400 Subject: [PATCH] fix(auth): stop a config-only PUT deleting stored secrets (#2084) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 24 produced no inline comments again. Of the three suppressed items, one is a pre-existing data-loss bug that predates #1950 entirely. **Editing a server's config deletes its stored OAuth client secret — from the keychain, not only from the stores this PR adds.** Filed as #2084 and fixed here, since the review surfaced it and these lines were already changing. `useServers.updateServer` deliberately sends `{ id, config }` with no `settings`, so a config-only save cannot wipe persisted settings. The route reads that as `preserve` and re-derives settings from the *on-disk* entry — which by #1356's design no longer holds the secrets. The submitted secret set is therefore empty, `expectedSecretFields` always lists the OAuth slot, and the reconcile deleted a value the user never touched. The rename branch was given `mergeRenameKeychainSecrets` for exactly this reason; the in-place branch never got the equivalent. The first fix was too broad — skip obsolete deletion whenever the intent is `preserve` — and an existing test caught it immediately: stdio `env` is part of `config`, so a config-only PUT that drops a key really has retired that secret. Obsolete detection is per-field now: - **`env:` fields** are implied by the entry's shape, so the new entry no longer expecting one is proof it was retired. - **The OAuth slot** is implied by nothing, so absence counts as "cleared" only when the caller spoke about settings at all. Both directions are tested, and the pre-existing env-key reconcile test still passes. Also: - `refreshingPersist` moves to `lib/`. The combinator computes nothing, but what it exists to do is sequence side effects, which is the `lib` half of the repo's split; I judged it the other way on the grounds that the function is a pure higher-order one, and the rule is about what a module is *for*. - `SecretStorageInfo.plaintext` is documented as file-only and *omitted* elsewhere rather than "always false", which is what the producer does and what `usableSecretStorage` enforces — the old wording invited a consumer to build the exact shape the web client discards. Full `npm run ci` green, staged tree verified first. Signed-off-by: cliffhall --- clients/web/src/App.tsx | 2 +- .../{utils => lib}/refreshingPersist.test.ts | 0 .../src/{utils => lib}/refreshingPersist.ts | 7 ++ .../mcp/remote/servers-route.test.ts | 118 ++++++++++++++++++ core/auth/secret-storage-info.ts | 8 +- core/mcp/remote/node/server.ts | 52 ++++++-- 6 files changed, 171 insertions(+), 16 deletions(-) rename clients/web/src/{utils => lib}/refreshingPersist.test.ts (100%) rename clients/web/src/{utils => lib}/refreshingPersist.ts (83%) diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index bfbc1dd3..2a9f1eab 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -111,7 +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 { refreshingPersist } from "./lib/refreshingPersist"; import { usePendingClientRequests } from "@inspector/core/react/usePendingClientRequests.js"; import { InspectorView } from "./components/views/InspectorView/InspectorView"; import type { diff --git a/clients/web/src/utils/refreshingPersist.test.ts b/clients/web/src/lib/refreshingPersist.test.ts similarity index 100% rename from clients/web/src/utils/refreshingPersist.test.ts rename to clients/web/src/lib/refreshingPersist.test.ts diff --git a/clients/web/src/utils/refreshingPersist.ts b/clients/web/src/lib/refreshingPersist.ts similarity index 83% rename from clients/web/src/utils/refreshingPersist.ts rename to clients/web/src/lib/refreshingPersist.ts index e7a46123..0b9aaa34 100644 --- a/clients/web/src/utils/refreshingPersist.ts +++ b/clients/web/src/lib/refreshingPersist.ts @@ -7,6 +7,13 @@ * settings-modal footer has to be re-fetched afterwards, or the footer keeps * reporting the state it had at page load. * + * Lives in `lib/` rather than `utils/`: the combinator itself computes + * nothing, but what it exists to do is *sequence side effects* — a persist + * that writes to disk and a refetch that hits the network — which is the + * `lib` half of the repo's split. Judged the other way at first on the + * grounds that the function is a pure higher-order one; the rule is about + * what a module is for, not about whether its own body touches I/O. + * * It exists as a named unit rather than two inline `await …; refresh()` pairs * in `App.tsx` because of where the coverage gate can reach. `App.tsx` is * deliberately outside the coverage `include` (a ~4.5k-line composition root), 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 d0c4668a..9862f55d 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 @@ -2625,6 +2625,124 @@ describe("/api/servers read-only sessions (#1481/#1483)", () => { }); }); +describe("a config-only PUT must not delete stored secrets (#2083)", () => { + // `useServers.updateServer` sends `{ id, config }` with no `settings` — + // the Add/Edit modal's save. The route re-derives settings from the *disk* + // entry, which by #1356's design no longer holds the secrets, so they were + // absent from the submitted set; `expectedSecretFields` always lists the + // OAuth slot, and the reconcile deleted a value the user never touched. + let tempDir: string; + let configPath: string; + + beforeEach(() => { + tempDir = mkdtempSync(join(tmpdir(), "inspector-config-put-")); + configPath = join(tempDir, "mcp.json"); + writeFileSync(configPath, JSON.stringify({ mcpServers: {} })); + }); + + afterEach(() => { + rmSync(tempDir, { recursive: true, force: true }); + }); + + const seed = async ( + app: ReturnType["app"], + extra: Record = {}, + ) => + app.request( + new Request("http://test/api/servers", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + id: "srv", + config: { type: "streamable-http", url: "https://a.test/mcp" }, + settings: { + headers: [], + env: [], + metadata: [], + connectionTimeout: 30000, + requestTimeout: 60000, + taskTtl: 60000, + maxFetchRequests: 1000, + roots: [], + oauthClientId: "cid", + oauthClientSecret: "keep-me", + ...extra, + }, + }), + }), + ); + + it("keeps the secret in a durable store when only the config changes", async () => { + // Not a session-store quirk — this loses the value from the keychain too, + // which is what makes it a bug in shipped behaviour rather than in the + // fallback added by #1950. + const store = new InMemorySecretStore(); + const { app } = createRemoteApp({ + dangerouslyOmitAuth: true, + mcpConfigPath: configPath, + initialConfig: { defaultEnvironment: {} }, + secretStore: store, + }); + await seed(app); + expect(await store.get("srv", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( + "keep-me", + ); + + const res = await app.request( + new Request("http://test/api/servers/srv", { + method: "PUT", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + id: "srv", + config: { type: "streamable-http", url: "https://b.test/mcp" }, + }), + }), + ); + expect(res.status).toBe(200); + expect(await store.get("srv", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( + "keep-me", + ); + expect(readFileSync(configPath, "utf-8")).toContain("b.test"); + }); + + it("still clears a secret the user actually cleared", async () => { + // The reconcile has to keep working when the caller *did* say something: + // an explicit settings apply with the field emptied must delete it. + const store = new InMemorySecretStore(); + const { app } = createRemoteApp({ + dangerouslyOmitAuth: true, + mcpConfigPath: configPath, + initialConfig: { defaultEnvironment: {} }, + secretStore: store, + }); + await seed(app); + + const res = await app.request( + new Request("http://test/api/servers/srv", { + method: "PUT", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + id: "srv", + settings: { + headers: [], + env: [], + metadata: [], + connectionTimeout: 30000, + requestTimeout: 60000, + taskTtl: 60000, + maxFetchRequests: 1000, + roots: [], + oauthClientId: "cid", + oauthClientSecret: "", + }, + }), + }), + ); + expect(res.status).toBe(200); + expect(await store.get("srv", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe(null); + }); +}); + describe("plaintext migration against a session-scoped store (#1950)", () => { // The container fallback selects an in-memory store in production. The // migration must not take that as licence to delete the durable copy: it diff --git a/core/auth/secret-storage-info.ts b/core/auth/secret-storage-info.ts index ce438dfc..c63220e2 100644 --- a/core/auth/secret-storage-info.ts +++ b/core/auth/secret-storage-info.ts @@ -50,8 +50,12 @@ export interface SecretStorageInfo { * readable until then. Reporting the intent would tell that user their * secrets were encrypted while they were not. * - * Always false for the other kinds — a keychain is opaque to us and RAM - * is not "at rest" in the sense this flag is about. + * **File-only, and omitted entirely for the other kinds** — not "false". + * A keychain is opaque to us and RAM is not "at rest" in the sense this + * flag is about, so the producer leaves it off and + * `useInitialConfig.usableSecretStorage` rejects a descriptor that carries + * it on a non-file kind. Documenting it as "always false elsewhere" invited + * an API consumer to construct exactly the shape the web client discards. */ plaintext?: boolean; /** diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index 1f3fec0d..6f778c66 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -79,7 +79,10 @@ import { writeClientConfigStore, } from "../../../client/node-persistence.js"; import { formatClientConfigLoadError } from "../../../client/config-parse.js"; -import { envSecretField } from "../../../auth/secret-fields.js"; +import { + envSecretField, + SECRET_FIELD_OAUTH_CLIENT_SECRET, +} from "../../../auth/secret-fields.js"; import type { SecretStorageInfo } from "../../../auth/secret-storage-info.js"; import { ZodError } from "zod"; @@ -2055,17 +2058,6 @@ export function createRemoteApp( // to perform the destructive operation (we want it after the disk // write so a failed disk write doesn't leave the user with their // old config on disk but missing keychain entries). - const computeObsoleteFields = ( - previousFields: Set, - nextSecrets: Record, - ): string[] => { - const nextFieldSet = new Set(Object.keys(nextSecrets)); - const obsolete: string[] = []; - for (const field of previousFields) { - if (!nextFieldSet.has(field)) obsolete.push(field); - } - return obsolete; - }; /** * Merge keychain secrets for a server-id rename. PUT payload values win @@ -2599,8 +2591,42 @@ export function createRemoteApp( // In-place update: same id, possibly different fields. Set // the new values first, then write disk, then drop obsolete // fields (env keys the user removed, OAuth secret cleared). + // + // **Only when the caller said something about settings.** On a + // `preserve` intent the body carried no `settings` at all — the + // config-only PUT that `useServers.updateServer` sends for the + // Add/Edit modal — so the settings are re-derived from the *disk* + // entry, which by #1356's design no longer holds the secrets. They + // were therefore absent from `secrets`, `expectedSecretFields` + // always lists the OAuth slot, and the reconcile deleted a value + // the user never touched: editing a server's URL destroyed its + // stored OAuth client secret, on the keychain as much as on a + // session store. Saying nothing about settings has to mean + // "leave the secrets alone", not "the user cleared them". + // Two kinds of field, retired on different evidence — a single + // "absent from the submitted secrets" test cannot serve both. + // + // A stdio `env:` field is implied by the entry's *shape*: `env` is + // part of `config`, so a config-only PUT that drops a key really + // has retired that secret, and the new entry no longer expecting it + // is the proof. + // + // The OAuth slot is not implied by anything — + // `expectedSecretFields` always lists it — so its absence from the + // submitted set means "the caller cleared it" only when the caller + // spoke about settings at all. On a `preserve` intent (the + // config-only PUT the Add/Edit modal sends) the settings were + // re-derived from the disk entry, which by #1356's design no longer + // holds the secret; treating that absence as a clear deleted a + // value the user never touched, on the keychain as much as on a + // session store. const previousFields = new Set(expectedSecretFields(existing)); - const obsolete = computeObsoleteFields(previousFields, secrets); + const stillExpected = new Set(expectedSecretFields(built)); + const obsolete = [...previousFields].filter((field) => + field === SECRET_FIELD_OAUTH_CLIENT_SECRET + ? settingsIntent.kind !== "preserve" && !(field in secrets) + : !stillExpected.has(field), + ); await writeKeychainEntriesFor(newId, secrets); await writeMcpAndTrackMtime(serializeStore(next)); await deleteKeychainFields(newId, obsolete);