fix(auth): refresh in finally, bound the KDF parameters, cover the last two writers

Round 22. Three findings, all in code I wrote, and the first two are both
cases of a stated rationale being wrong rather than a line being wrong.

**`refreshingPersist` skipped the refresh on failure, and I had argued for
that explicitly.** The doc and its test both said "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. The wrapper then left the footer
describing a file that no longer exists in that form — the stale-descriptor
bug it exists to prevent, reached through its own error path.

It refreshes in a `finally` now. 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. The test asserting the old behavior is inverted, with the
reasoning recorded so it is not "corrected" back.

**The KDF parameters were validated for well-formedness, not for cost.**
scrypt's work is proportional to `N * r * p`, and all three come off disk — so
a file declaring a modest `N` with `p: 1_000_000` burns unbounded CPU on every
read *and* on merely describing the store for `/api/config`, which is a page
load. Anyone who can write that file can already read its secrets, so this is
not a privilege boundary; it is a footgun and a hang, and the fix is free
because this build writes exactly the constants it now enforces as maxima.

**Two secret-store mutations still bypassed the refresh.** The pagination
toggle resends the server's rehydrated secrets, and deletion sweeps them —
both are writes, and a write is what performs the pending upgrade. Routed
through the same wrapper as the rest.

Full `npm run ci` green, staged tree verified first.

Signed-off-by: cliffhall <cliff@futurescale.com>

clients/web/src/App.tsx

@@ -3625,7 +3625,14 @@ function App() {
           runCommandInBackground(() => refreshResources(), "ambient");
         }
       }
-      void updateServerSettings(activeServerId, next).catch((err: unknown) => {
+      // Refreshed like every other secret-store mutation: this resends the
+      // server's rehydrated secrets, so it can trigger the pending
+      // plaintext-to-encrypted upgrade even though the user only toggled
+      // pagination (#1950 review r22).
+      void refreshingPersist(updateServerSettings, refreshInitialConfig)(
+        activeServerId,
+        next,
+      ).catch((err: unknown) => {
         // Persist failed: revert the optimistic override (the effect only
         // clears it when the persisted value changes, which won't happen here)
         // and roll the live client setting back, so the UI and client reflect
@@ -3892,7 +3899,10 @@ function App() {
       setStderrLogState(null);
       setActiveServerId(undefined);
     }
-    await removeServer(id);
+    // Deleting sweeps the server's secrets from the store, which for a
+    // file-backed store is a write — and a write is what performs the pending
+    // plaintext-to-encrypted upgrade (#1950 review r22).
+    await refreshingPersist(removeServer, refreshInitialConfig)(id);
     setRemoveTarget(null);
   }, [
     removeTarget,

clients/web/src/test/integration/auth/node/file-secret-store.test.ts

@@ -1164,6 +1164,22 @@ describe("readOnDiskEncryption rejects an envelope it could not open", () => {
       },
       "kdf r/p are 0/1, which are not positive integers",
     ],
+    [
+      "a KDF cost parameter large enough to burn unbounded CPU",
+      {
+        version: 1,
+        encryption: "aes-256-gcm",
+        kdf: {
+          algorithm: "scrypt",
+          salt: "AAAA",
+          N: 16384,
+          r: 8,
+          p: 1_000_000,
+        },
+        data: `${Buffer.alloc(12).toString("base64")}.${Buffer.alloc(16).toString("base64")}.${Buffer.alloc(8).toString("base64")}`,
+      },
+      "kdf cost parameters (N=16384, r=8, p=1000000) exceed the supported maximum (N=16384, r=8, p=1)",
+    ],
     [
       "an unsupported KDF",
       {

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

@@ -32,16 +32,22 @@ describe("refreshingPersist", () => {
     expect(persist).toHaveBeenCalledWith("srv-1", settings);
   });
 
-  it("does not refresh when the persist fails, and propagates the error", async () => {
+  it("refreshes even when the persist fails, and still propagates the error", async () => {
-    // A save that threw did not write, so there is nothing new to describe —
+    // This asserted the opposite until round 22, on reasoning that sounded
-    // and the caller is mid-error-handling, which is the wrong moment to
+    // right and was wrong for this write order: both persistence paths write
-    // repaint state that did not change.
+    // 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).not.toHaveBeenCalled();
+    expect(refresh).toHaveBeenCalledTimes(1);
   });
 
   it("returns a function usable as a persist callback more than once", async () => {

clients/web/src/utils/refreshingPersist.ts

@@ -16,16 +16,29 @@
  * 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 **only on success**. A persist that threw did not write, so
+ * `refresh` runs in a `finally`, **not only on success**, and the first
- * there is nothing new to describe, and re-fetching would just repaint the
+ * version of this got that wrong on plausible-sounding reasoning: "a persist
- * state already on screen while the caller is handling the failure.
+ * 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) => {
-    await persist(...args);
+    try {
-    refresh();
+      await persist(...args);
+    } finally {
+      refresh();
+    }
   };
 }

core/auth/node/file-secret-store.ts

@@ -277,6 +277,15 @@ function encryptedEnvelopeProblem(parsed: SecretFile): string | null {
   if (!isPositiveInt(r) || !isPositiveInt(p)) {
     return `kdf r/p are ${String(r)}/${String(p)}, which are not positive integers`;
   }
+  // Bounded, not merely well-formed. scrypt's work is proportional to
+  // `N * r * p`, and every one of those comes off disk — so a file declaring
+  // a modest `N` with an enormous `p` burns unbounded CPU on any read *and*
+  // on merely describing the store for `/api/config`, which is a page load.
+  // This build writes exactly the constants above and refuses a `version` it
+  // does not know, so anything larger cannot be a file it produced.
+  if (N > SCRYPT_N || r > SCRYPT_R || p > SCRYPT_P) {
+    return `kdf cost parameters (N=${N}, r=${r}, p=${p}) exceed the supported maximum (N=${SCRYPT_N}, r=${SCRYPT_R}, p=${SCRYPT_P})`;
+  }
   return null;
 }