From 08cf939e1f576e2325613f3d0dd3b33c46a1a563 Mon Sep 17 00:00:00 2001 From: Danijel Martinek Date: Fri, 10 Jul 2026 18:09:56 +0200 Subject: [PATCH] fix(core-consent): merge per-category on persist instead of replacing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PayloadConsent.persist() wrote the WHOLE consentState array from a per-request cache, so two interleaved grant/withdraw requests dropped one another's categories (audit finding A7). Persist now re-reads the freshest stored state immediately before writing, overlays ONLY the mutated categories, and adopts the merged view locally. The residual same-window race is documented in the method doc — Payload json fields have no targeted array patch, so read-merge-write is the trade-off. Co-Authored-By: Claude Fable 5 --- .../core-consent/src/payload-consent.test.ts | 103 ++++++++++++++++++ packages/core-consent/src/payload-consent.ts | 53 ++++++++- 2 files changed, 152 insertions(+), 4 deletions(-) diff --git a/packages/core-consent/src/payload-consent.test.ts b/packages/core-consent/src/payload-consent.test.ts index dfb2ea9..2d56c0f 100644 --- a/packages/core-consent/src/payload-consent.test.ts +++ b/packages/core-consent/src/payload-consent.test.ts @@ -259,3 +259,106 @@ describe("PayloadConsent.load — deserializeEntry branches", () => { expect(cats[0]!.withdrawnAt).toBeInstanceOf(Date); }); }); + +describe("PayloadConsent.persist — read-merge-write (A7)", () => { + async function makeTwoConsents() { + const mock = makePayloadMock(); + const a = new PayloadConsent( + "user_1", + {} as never, + new RecordingAuditLog(), + mock.getPayload, + ); + const b = new PayloadConsent( + "user_1", + {} as never, + new RecordingAuditLog(), + mock.getPayload, + ); + // Both instances hydrate from the SAME empty snapshot — the per-request + // cache staleness that caused the lost update. + await a.load(); + await b.load(); + return { a, b, ...mock }; + } + + function storedCategories(db: Record): string[] { + return (db["user_1"] ?? []) + .map((e) => (e as { category: string }).category) + .sort(); + } + + it("two interleaved grants from stale caches both survive", async () => { + const { a, b, db } = await makeTwoConsents(); + + await a.grant("analytics"); + await b.grant("marketing"); // pre-fix: whole-array write dropped "analytics" + + expect(storedCategories(db)).toEqual(["analytics", "marketing"]); + }); + + it("a grant and a withdraw on different categories both survive", async () => { + const { a, b, db } = await makeTwoConsents(); + + await a.grant("analytics"); + await b.grant("marketing"); + await a.withdraw("analytics"); + + expect(storedCategories(db)).toEqual(["analytics", "marketing"]); + const analytics = ( + db["user_1"] as Array<{ category: string; state: string }> + ).find((e) => e.category === "analytics"); + expect(analytics?.state).toBe("denied"); + }); + + it("adopts concurrent writers' entries into the local cache after persist", async () => { + const { a, b } = await makeTwoConsents(); + + await a.grant("analytics"); + await b.grant("marketing"); + + // b re-read the freshest state during persist, so it now sees a's grant. + expect(b.isGranted("analytics")).toBe(true); + expect(b.isGranted("marketing")).toBe(true); + }); + + it("truly concurrent grants both survive when the second read lands after the first write", async () => { + const mock = makePayloadMock(); + const a = new PayloadConsent( + "user_1", + {} as never, + new RecordingAuditLog(), + mock.getPayload, + ); + const b = new PayloadConsent( + "user_1", + {} as never, + new RecordingAuditLog(), + mock.getPayload, + ); + await a.load(); + await b.load(); + + // Gate b's persist-read until a's write has committed — the ordering the + // read-merge-write strategy is designed for. (A same-window overlap is + // the documented residual race.) + let releaseB: () => void = () => {}; + const bGate = new Promise((resolve) => { + releaseB = resolve; + }); + const originalFindByID = mock.findByID.getMockImplementation()!; + let firstPersistRead = true; + // a loads+persists first; instrument findByID so b's persist read waits. + mock.findByID.mockImplementation(async (args: { id: string }) => { + if (!firstPersistRead) await bGate; + firstPersistRead = false; + return originalFindByID(args); + }); + + const aDone = a.grant("analytics").then(() => releaseB()); + const bDone = b.grant("marketing"); + await Promise.all([aDone, bDone]); + + expect(storedCategories(mock.db)).toEqual(["analytics", "marketing"]); + }); +}); diff --git a/packages/core-consent/src/payload-consent.ts b/packages/core-consent/src/payload-consent.ts index 7394061..4c544c9 100644 --- a/packages/core-consent/src/payload-consent.ts +++ b/packages/core-consent/src/payload-consent.ts @@ -87,7 +87,7 @@ export class PayloadConsent implements IConsent { method: meta?.method, }; this.cache.set(category, entry); - await this.persist(); + await this.persist([category]); await this.auditLog.record({ actorId: this.userId, actorType: "user", @@ -116,7 +116,7 @@ export class PayloadConsent implements IConsent { withdrawnAt: now, }; this.cache.set(category, entry); - await this.persist(); + await this.persist([category]); await this.auditLog.record({ actorId: this.userId, actorType: "user", @@ -139,9 +139,54 @@ export class PayloadConsent implements IConsent { return Array.from(this.cache.values()); } - private async persist(): Promise { + /** + * Read-merge-write persistence (audit finding A7 — lost-update race). + * + * Payload's `update` on a json field replaces the WHOLE value; there is no + * targeted array-element patch. Writing this instance's per-request cache + * verbatim would drop any category another request persisted since our + * `load()`. Instead we re-read the freshest stored state immediately + * before writing and overlay ONLY the categories this call mutated, so + * two interleaved writers touching different categories both survive. + * + * Residual window (documented, accepted): between this read and the write, + * a concurrent writer to the SAME category is last-writer-wins, and a + * concurrent writer to a different category that lands inside the window + * can still be overwritten. Closing it fully needs a DB-level transaction + * or JSON-patch support in Payload; for consent state (idempotent, + * per-subject, low frequency) read-merge-write is the accepted trade-off. + */ + private async persist(mutated: ConsentCategory[]): Promise { const payload = await this.getPayloadFn({ config: this.config }); - const state = Array.from(this.cache.values()).map((entry) => ({ + + // Freshest stored state, immediately before the write. + const doc = await payload.findByID({ + collection: "users", + id: this.userId, + overrideAccess: true, + }); + const merged = new Map(); + const rawState = doc["consentState"]; + if (Array.isArray(rawState)) { + for (const raw of rawState) { + if (raw && typeof raw === "object") { + const entry = deserializeEntry(raw as Record); + merged.set(entry.category, entry); + } + } + } + + // Overlay only what this call changed. + for (const category of mutated) { + const entry = this.cache.get(category); + if (entry) merged.set(category, entry); + } + + // Adopt the merged view locally so isGranted/getCategories reflect + // concurrent writers' entries too. + this.cache = merged; + + const state = Array.from(merged.values()).map((entry) => ({ category: entry.category, state: entry.state, grantedAt: entry.grantedAt?.toISOString() ?? null,