From 469e59a7c47668b0f70774fb27cbf69df3d60cf9 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 22 Aug 2026 21:06:47 -0400 Subject: [PATCH] fix(auth): commit the utility round 17 wired in, and drop a prohibited cast MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Round 17 pushed a branch that does not compile, and I reported it green.** `refreshingPersist.ts` and its test were new files; `git commit -s -a` stages modified *tracked* files and not untracked ones, so `App.tsx` went to origin importing a module that was never committed. `npm run ci` passed against my working tree, where both files exist — so the claim was true of what I had and false of what I pushed, which is the worst shape a green result can take. That is the second staging mistake on this branch. The first was `git commit -o `, which commits whole files rather than my hunks and swept in a collaborator's in-flight tests. Both come from trusting a flag's shorthand instead of reading what was actually staged, so from here the commit goes through an explicit `git add -A` plus a look at `git status --short` before the gate runs — the check is cheap and it is the only one that describes the pushed state rather than the local one. Also replaces the `as unknown as` on the test logger. The rules prohibit it, and it was doing real damage beyond style: forcing a four-method object through `pino.Logger` erased the fact that the stand-in was not one, so a change in how the server logs — a child logger, bindings, a different level — would have gone unnoticed. It is a real pino logger writing to a capturing destination now, which needs no cast because the destination interface is satisfied structurally. Both remaining suppressed comments are the cross-process guarantee, tracked in #2082. Full `npm run ci` green, run against a fully staged tree this time. Signed-off-by: cliffhall --- .../mcp/remote/servers-route.test.ts | 35 ++++++----- .../web/src/utils/refreshingPersist.test.ts | 58 +++++++++++++++++++ clients/web/src/utils/refreshingPersist.ts | 31 ++++++++++ 3 files changed, 110 insertions(+), 14 deletions(-) create mode 100644 clients/web/src/utils/refreshingPersist.test.ts create mode 100644 clients/web/src/utils/refreshingPersist.ts 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 a47b5710..cef80f28 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,8 @@ * exercises the routes via real HTTP. */ -import { describe, it, expect, afterEach, beforeEach, vi } from "vitest"; +import { describe, it, expect, afterEach, beforeEach } from "vitest"; +import pino from "pino"; import { mkdtempSync, readFileSync, @@ -2680,14 +2681,23 @@ describe("plaintext migration against a session-scoped store (#1950)", () => { // 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"]; + // A real pino logger writing to a capturing destination, rather than a + // four-method object forced through the API type with a double cast: the + // cast is prohibited here, and it also erased the fact that the stand-in + // was not a `pino.Logger` — so a change to how the server logs (child + // loggers, bindings, a different level) would have gone unnoticed. + const lines: string[] = []; + const logger = pino( + { level: "warn" }, + { + write(chunk: string) { + lines.push(chunk); + }, + }, + ); const sessionMessage = /kept in memory for this session/; + const saidSessionWarning = () => + lines.filter((l) => sessionMessage.test(l)); // A catalog with no plaintext secrets: nothing is being preserved. const emptyPath = join(tempDir, "empty.json"); @@ -2701,10 +2711,10 @@ describe("plaintext migration against a session-scoped store (#1950)", () => { }); 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(); + expect(saidSessionWarning()).toHaveLength(0); // And with a plaintext secret: said once, not once per read. - warn.mockClear(); + lines.length = 0; const withSecret = createRemoteApp({ dangerouslyOmitAuth: true, mcpConfigPath: configPath, @@ -2714,10 +2724,7 @@ describe("plaintext migration against a session-scoped store (#1950)", () => { }); 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); + expect(saidSessionWarning()).toHaveLength(1); }); it("aborts the migration when the keychain read fails, preserving mcp.json", async () => { diff --git a/clients/web/src/utils/refreshingPersist.test.ts b/clients/web/src/utils/refreshingPersist.test.ts new file mode 100644 index 00000000..ef3a8865 --- /dev/null +++ b/clients/web/src/utils/refreshingPersist.test.ts @@ -0,0 +1,58 @@ +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>() + .mockResolvedValue(undefined); + const settings = { headers: [] }; + await refreshingPersist(persist, vi.fn())("srv-1", settings); + 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. + 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(); + }); + + 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); + }); +}); diff --git a/clients/web/src/utils/refreshingPersist.ts b/clients/web/src/utils/refreshingPersist.ts new file mode 100644 index 00000000..fbf04411 --- /dev/null +++ b/clients/web/src/utils/refreshingPersist.ts @@ -0,0 +1,31 @@ +/** + * 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. + * + * 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 **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. + */ +export function refreshingPersist( + persist: (...args: Args) => Promise, + refresh: () => void, +): (...args: Args) => Promise { + return async (...args: Args) => { + await persist(...args); + refresh(); + }; +}