fix(core-consent): merge per-category on persist instead of replacing
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 <noreply@anthropic.com>
This commit is contained in:
@@ -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<void> {
|
||||
/**
|
||||
* 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<void> {
|
||||
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<ConsentCategory, UserConsentState>();
|
||||
const rawState = doc["consentState"];
|
||||
if (Array.isArray(rawState)) {
|
||||
for (const raw of rawState) {
|
||||
if (raw && typeof raw === "object") {
|
||||
const entry = deserializeEntry(raw as Record<string, unknown>);
|
||||
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,
|
||||
|
||||
Reference in New Issue
Block a user