fix(auth): stop a config-only PUT deleting stored secrets (#2084)

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 <cliff@futurescale.com>

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 {

clients/web/src/lib/refreshingPersist.test.ts

@@ -0,0 +1,64 @@
+import { describe, it, expect, vi } from "vitest";
+import { refreshingPersist } from "./refreshingPersist";
+
+describe("refreshingPersist", () => {
+  it("refreshes after the persist resolves, not before", async () => {
+    // Ordering is the point: refreshing first would re-fetch the descriptor
+    // that the pending write is about to invalidate, which is the stale read
+    // this wrapper exists to prevent, one step earlier.
+    const order: string[] = [];
+    const persist = vi.fn(async () => {
+      order.push("persist");
+    });
+    const refresh = vi.fn(() => {
+      order.push("refresh");
+    });
+
+    await refreshingPersist(persist, refresh)();
+
+    expect(order).toEqual(["persist", "refresh"]);
+  });
+
+  it("passes every argument through unchanged", async () => {
+    // The server-settings caller is `(id, settings)`; the wrapper must be
+    // transparent or it silently drops the entry it was meant to save.
+    // Typed rather than given placeholder parameters: this scope's eslint
+    // does not honour the `_` prefix, and the signature is what matters.
+    const persist = vi
+      .fn<(id: string, settings: unknown) => Promise<void>>()
+      .mockResolvedValue(undefined);
+    const settings = { headers: [] };
+    await refreshingPersist(persist, vi.fn())("srv-1", settings);
+    expect(persist).toHaveBeenCalledWith("srv-1", settings);
+  });
+
+  it("refreshes even when the persist fails, and still propagates the error", async () => {
+    // This asserted the opposite until round 22, on reasoning that sounded
+    // right and was wrong for this write order: both persistence paths write
+    // the secret store *before* the file, so a rejected disk write can follow
+    // a `set` that already upgraded `secrets.json` from plaintext to
+    // encrypted. Skipping the refresh there left the footer describing a file
+    // that no longer exists in that form.
+    //
+    // The asymmetry decides it: a needless refresh costs one idempotent GET;
+    // a missed one leaves a security statement wrong until reload.
+    const boom = new Error("keychain unavailable");
+    const persist = vi.fn().mockRejectedValue(boom);
+    const refresh = vi.fn();
+
+    await expect(refreshingPersist(persist, refresh)()).rejects.toBe(boom);
+    expect(refresh).toHaveBeenCalledTimes(1);
+  });
+
+  it("returns a function usable as a persist callback more than once", async () => {
+    const persist = vi.fn(async () => {});
+    const refresh = vi.fn();
+    const wrapped = refreshingPersist(persist, refresh);
+
+    await wrapped();
+    await wrapped();
+
+    expect(persist).toHaveBeenCalledTimes(2);
+    expect(refresh).toHaveBeenCalledTimes(2);
+  });
+});

clients/web/src/lib/refreshingPersist.ts

@@ -0,0 +1,51 @@
+/**
+ * Wrap a persist callback so it refreshes derived state after it succeeds.
+ *
+ * Written for one case (#1950): saving a secret can change what the secrets
+ * file *is* — the first save under a newly-set passphrase re-encrypts a
+ * pre-existing plaintext file — so the `secretStorage` descriptor behind the
+ * 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),
+ * so wiring written there is not exercised by anything: the hook's own tests
+ * and the modal's own tests can both pass while the callback that connects
+ * them is broken, leaving the security footer stale after exactly the write
+ * that changed it. Review round 17 of #1950 caught that, after three earlier
+ * rounds had each found a defect in this same path.
+ *
+ * `refresh` runs in a `finally`, **not only on success**, and the first
+ * version of this got that wrong on plausible-sounding reasoning: "a persist
+ * that threw did not write, so there is nothing new to describe". That is
+ * false for this write order. Both persistence paths write the secret store
+ * *before* the file — so a rejected disk write can follow a `set` that has
+ * already upgraded `secrets.json` from plaintext to encrypted. Skipping the
+ * refresh there leaves the footer describing a file that no longer exists in
+ * that form, which is the stale-descriptor bug this wrapper exists to
+ * prevent, reached through its own error path.
+ *
+ * The asymmetry is what settles it: refreshing after a failure that changed
+ * nothing costs one idempotent GET, while not refreshing after a failure that
+ * changed something leaves a security statement wrong until reload.
+ */
+export function refreshingPersist<Args extends unknown[]>(
+  persist: (...args: Args) => Promise<void>,
+  refresh: () => void,
+): (...args: Args) => Promise<void> {
+  return async (...args: Args) => {
+    try {
+      await persist(...args);
+    } finally {
+      refresh();
+    }
+  };
+}

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<typeof createRemoteApp>["app"],
+    extra: Record<string, unknown> = {},
+  ) =>
+    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

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;
   /**

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<string>,
-    nextSecrets: Record<string, string>,
-  ): 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);