From 4ce0643ac1464a18f73bbd55dc35fde4addc7305 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 22 Aug 2026 22:46:42 -0400 Subject: [PATCH] fix(auth): refresh in finally, bound the KDF parameters, cover the last two writers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- clients/web/src/App.tsx | 14 +++++++++-- .../auth/node/file-secret-store.test.ts | 16 +++++++++++++ .../web/src/utils/refreshingPersist.test.ts | 16 +++++++++---- clients/web/src/utils/refreshingPersist.ts | 23 +++++++++++++++---- core/auth/node/file-secret-store.ts | 9 ++++++++ 5 files changed, 66 insertions(+), 12 deletions(-) diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index e1685093..bfbc1dd3 100644 --- a/clients/web/src/App.tsx +++ b/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, diff --git a/clients/web/src/test/integration/auth/node/file-secret-store.test.ts b/clients/web/src/test/integration/auth/node/file-secret-store.test.ts index 4c67e8a8..8becc56e 100644 --- a/clients/web/src/test/integration/auth/node/file-secret-store.test.ts +++ b/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", { diff --git a/clients/web/src/utils/refreshingPersist.test.ts b/clients/web/src/utils/refreshingPersist.test.ts index ef3a8865..68c647f9 100644 --- a/clients/web/src/utils/refreshingPersist.test.ts +++ b/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 () => { - // A save that threw did not write, so there is nothing new to describe — - // and the caller is mid-error-handling, which is the wrong moment to - // repaint state that did not change. + 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).not.toHaveBeenCalled(); + expect(refresh).toHaveBeenCalledTimes(1); }); it("returns a function usable as a persist callback more than once", async () => { diff --git a/clients/web/src/utils/refreshingPersist.ts b/clients/web/src/utils/refreshingPersist.ts index fbf04411..e7a46123 100644 --- a/clients/web/src/utils/refreshingPersist.ts +++ b/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 - * there is nothing new to describe, and re-fetching would just repaint the - * state already on screen while the caller is handling the failure. + * `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( persist: (...args: Args) => Promise, refresh: () => void, ): (...args: Args) => Promise { return async (...args: Args) => { - await persist(...args); - refresh(); + try { + await persist(...args); + } finally { + refresh(); + } }; } diff --git a/core/auth/node/file-secret-store.ts b/core/auth/node/file-secret-store.ts index ecc04a6d..a3bf9cb5 100644 --- a/core/auth/node/file-secret-store.ts +++ b/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; }