From f369ce4b69c14ce25e9559da5e99c197e7d44742 Mon Sep 17 00:00:00 2001 From: israel Date: Thu, 3 Sep 2026 13:53:49 +0100 Subject: [PATCH] fix(platform): re-check legal holds per erasure pass and fix the receipt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two ways an erasure receipt could claim more than happened. The legal hold was read once, before the cascade, and never again. The passes are not one transaction, and two of them fan out per-thread lineage purges and per-document blob and corpus deletes, so a hold placed mid-cascade was ignored for everything after it. 0.4 re-read holds inside all 19 of its arms and its helper names the reason: FRCP 37(e) spoliation. The re-check now runs before every pass, and an unreadable hold table skips the pass rather than running it — a table you cannot read is not evidence that nothing is held. A held-off pass now lands the receipt 'partial' and says which, tracked separately from a failed one so an operator can tell a hold from a fault. Before, only a thrown pass did, so a hold that stopped the cascade halfway still reported 'done' — and that receipt is the subject's Art 19 confirmation. The Full breakdown panel also rendered blank for every recent request: a pass records a plain count, and the renderer skipped anything that was not an object, so every entry was dropped before the empty-category branch. It reads both shapes now, because receipts written earlier still carry the older one. Both rules are extracted so they are testable at all — the status decision out of the cascade, the entry fold out of the drawer. Refs #3142. --- .../breakdown-entries.test.ts | 65 +++++++++++++++++++ .../breakdown-entries.ts | 58 +++++++++++++++++ .../request-detail-drawer.tsx | 29 +-------- .../domains/erasure/receipt-status.test.ts | 37 +++++++++++ .../backend/domains/erasure/service.ts | 55 +++++++++++++++- 5 files changed, 215 insertions(+), 29 deletions(-) create mode 100644 services/platform/app/features/settings/governance/data-subject-requests/breakdown-entries.test.ts create mode 100644 services/platform/app/features/settings/governance/data-subject-requests/breakdown-entries.ts create mode 100644 services/platform/backend/domains/erasure/receipt-status.test.ts diff --git a/services/platform/app/features/settings/governance/data-subject-requests/breakdown-entries.test.ts b/services/platform/app/features/settings/governance/data-subject-requests/breakdown-entries.test.ts new file mode 100644 index 0000000000..449d4f6a88 --- /dev/null +++ b/services/platform/app/features/settings/governance/data-subject-requests/breakdown-entries.test.ts @@ -0,0 +1,65 @@ +import { describe, expect, test } from 'vitest'; + +import { foldBreakdownEntries } from './breakdown-entries.ts'; + +describe('foldBreakdownEntries', () => { + test('reads the plain-count shape a pass records', () => { + // The whole panel rendered blank on this shape before: every entry was + // skipped, so neither a row nor the empty-category line appeared. + expect( + foldBreakdownEntries({ uploads: 3, cloudGrants: 2, memories: 0 }), + ).toEqual({ + visible: [ + { key: 'uploads', rows: 3, skippedByHold: 0 }, + { key: 'cloudGrants', rows: 2, skippedByHold: 0 }, + ], + zeroCount: 1, + }); + }); + + test('still reads the older object shape on existing receipts', () => { + expect( + foldBreakdownEntries({ + documents: { rows: 4, skippedByHold: 0 }, + threads: { rows: 0, skippedByHold: 2 }, + feedback: { rows: 0, skippedByHold: 0 }, + }), + ).toEqual({ + visible: [ + { key: 'documents', rows: 4, skippedByHold: 0 }, + { key: 'threads', rows: 0, skippedByHold: 2 }, + ], + zeroCount: 1, + }); + }); + + test('sums the login-attempts pair into one row count', () => { + expect( + foldBreakdownEntries({ + loginAttempts: { attempts: 2, blockCounters: 1 }, + }), + ).toEqual({ + visible: [{ key: 'loginAttempts', rows: 3, skippedByHold: 0 }], + zeroCount: 0, + }); + }); + + test('counts a held-off category as visible, not as empty', () => { + const { visible, zeroCount } = foldBreakdownEntries({ + uploads: { rows: 0, skippedByHold: 5 }, + }); + expect(zeroCount).toBe(0); + expect(visible).toEqual([{ key: 'uploads', rows: 0, skippedByHold: 5 }]); + }); + + test('ignores a value that is neither a count nor an entry', () => { + expect(foldBreakdownEntries({ odd: 'nope', missing: null })).toEqual({ + visible: [], + zeroCount: 0, + }); + }); + + test('an empty snapshot yields nothing to show', () => { + expect(foldBreakdownEntries({})).toEqual({ visible: [], zeroCount: 0 }); + }); +}); diff --git a/services/platform/app/features/settings/governance/data-subject-requests/breakdown-entries.ts b/services/platform/app/features/settings/governance/data-subject-requests/breakdown-entries.ts new file mode 100644 index 0000000000..70bfbfe96b --- /dev/null +++ b/services/platform/app/features/settings/governance/data-subject-requests/breakdown-entries.ts @@ -0,0 +1,58 @@ +/** + * Folds an erasure receipt's per-category snapshot into the rows the + * breakdown panel renders, plus a count of the categories that came back + * empty. + * + * Two shapes reach this. A pass records a plain count. The older shape was an + * object per category, and receipts written before that change still carry + * it, so both are read. Reading only the object form is what rendered the + * panel blank for every recent request: every entry was skipped before the + * empty-category branch, so the panel showed neither rows nor the "no data in + * N other categories" line. + */ + +export interface BreakdownRow { + key: string; + rows: number; + skippedByHold: number; +} + +interface PerCategoryEntry { + rows?: number; + skippedByHold?: number; + blobs?: number; + attempts?: number; + blockCounters?: number; +} + +export function foldBreakdownEntries(snapshot: Record): { + visible: BreakdownRow[]; + zeroCount: number; +} { + const visible: BreakdownRow[] = []; + let zeroCount = 0; + for (const [key, value] of Object.entries(snapshot)) { + let rows: number; + let skippedByHold = 0; + if (typeof value === 'number') { + rows = value; + } else if (typeof value === 'object' && value !== null) { + const entry = value as PerCategoryEntry; + // `loginAttempts` reported {attempts, blockCounters} rather than {rows}; + // sum them into the same "rows" view. + rows = + typeof entry.rows === 'number' + ? entry.rows + : (entry.attempts ?? 0) + (entry.blockCounters ?? 0); + skippedByHold = entry.skippedByHold ?? 0; + } else { + continue; + } + if (rows === 0 && skippedByHold === 0) { + zeroCount++; + } else { + visible.push({ key, rows, skippedByHold }); + } + } + return { visible, zeroCount }; +} diff --git a/services/platform/app/features/settings/governance/data-subject-requests/request-detail-drawer.tsx b/services/platform/app/features/settings/governance/data-subject-requests/request-detail-drawer.tsx index 56f3840105..083212bbce 100644 --- a/services/platform/app/features/settings/governance/data-subject-requests/request-detail-drawer.tsx +++ b/services/platform/app/features/settings/governance/data-subject-requests/request-detail-drawer.tsx @@ -14,6 +14,7 @@ import { TableDateCell } from '@/app/components/ui/data-display/table-date-cell' import { Sheet } from '@/app/components/ui/overlays/sheet'; import { useT } from '@/lib/i18n/client'; +import { foldBreakdownEntries } from './breakdown-entries.ts'; import { CancelDialog } from './cancel-dialog'; import { ExtendDeadlineDialog } from './extend-deadline-dialog'; import { useGetErasureRequest } from './hooks/queries'; @@ -511,38 +512,12 @@ function CancelledBlock({ ); } -interface PerCategoryEntry { - rows?: number; - skippedByHold?: number; - blobs?: number; - attempts?: number; - blockCounters?: number; -} - function FullBreakdown({ snapshot }: { snapshot: Record }) { const { t } = useT('governance'); // Sort entries: non-zero first, zero-value categories collapsed to a // count at the bottom. Each known field uses an i18n label; unknown // category names fall back to the raw key. - const entries = Object.entries(snapshot); - const visible: { key: string; rows: number; skippedByHold: number }[] = []; - let zeroCount = 0; - for (const [key, value] of entries) { - if (typeof value !== 'object' || value === null) continue; - const e = value as PerCategoryEntry; - // `loginAttempts` uses {attempts, blockCounters} instead of {rows}; - // sum them as the "rows" view for the breakdown. - const rows = - typeof e.rows === 'number' - ? e.rows - : (e.attempts ?? 0) + (e.blockCounters ?? 0); - const skippedByHold = e.skippedByHold ?? 0; - if (rows === 0 && skippedByHold === 0) { - zeroCount++; - } else { - visible.push({ key, rows, skippedByHold }); - } - } + const { visible, zeroCount } = foldBreakdownEntries(snapshot); return (
diff --git a/services/platform/backend/domains/erasure/receipt-status.test.ts b/services/platform/backend/domains/erasure/receipt-status.test.ts new file mode 100644 index 0000000000..7a59eb1549 --- /dev/null +++ b/services/platform/backend/domains/erasure/receipt-status.test.ts @@ -0,0 +1,37 @@ +import { describe, expect, test } from 'vitest'; + +import { erasureReceiptError, erasureReceiptStatus } from './service.ts'; + +describe('erasureReceiptStatus', () => { + test('a clean cascade is done', () => { + expect(erasureReceiptStatus([], [])).toBe('done'); + }); + + test('a failed pass makes the receipt partial', () => { + expect(erasureReceiptStatus(['uploads'], [])).toBe('partial'); + }); + + test('a pass held off by a legal hold makes it partial too', () => { + // Without this the receipt would claim `done` for a cascade a hold + // stopped halfway, which is the Art 19 confirmation the subject reads. + expect(erasureReceiptStatus([], ['documents'])).toBe('partial'); + }); +}); + +describe('erasureReceiptError', () => { + test('says nothing when nothing went wrong', () => { + expect(erasureReceiptError([], [])).toBeNull(); + }); + + test('names the failed passes', () => { + expect(erasureReceiptError(['uploads', 'threads'], [])).toBe( + 'failed passes: uploads, threads', + ); + }); + + test('names the held-off passes separately from the failed ones', () => { + expect(erasureReceiptError(['uploads'], ['documents'])).toBe( + 'failed passes: uploads; held off by a legal hold: documents', + ); + }); +}); diff --git a/services/platform/backend/domains/erasure/service.ts b/services/platform/backend/domains/erasure/service.ts index 3a9f170475..91b70ea7c2 100644 --- a/services/platform/backend/domains/erasure/service.ts +++ b/services/platform/backend/domains/erasure/service.ts @@ -559,6 +559,38 @@ async function scrubSubjectAuditLogs( return scrubbed.length; } +/** + * `done` is a claim that every category was reached. A pass that threw, or + * that a legal hold held off, makes the claim false — so the receipt reads + * `partial` and names which, rather than reporting a clean erasure. Under Art + * 19 this receipt is the subject's confirmation, so the distinction between + * "found nothing" and "never looked" has to survive onto it. + */ +export function erasureReceiptStatus( + failedPasses: readonly string[], + heldOffPasses: readonly string[], +): 'done' | 'partial' { + return failedPasses.length === 0 && heldOffPasses.length === 0 + ? 'done' + : 'partial'; +} + +/** One line naming what stopped a `partial` receipt from being `done`, or + * `null` when nothing did. */ +export function erasureReceiptError( + failedPasses: readonly string[], + heldOffPasses: readonly string[], +): string | null { + const parts: string[] = []; + if (failedPasses.length > 0) { + parts.push(`failed passes: ${failedPasses.join(', ')}`); + } + if (heldOffPasses.length > 0) { + parts.push(`held off by a legal hold: ${heldOffPasses.join(', ')}`); + } + return parts.length > 0 ? parts.join('; ') : null; +} + /** * Does the subject still belong to another organization that has not * disabled them? @@ -623,10 +655,29 @@ export async function processErasure( const counts: Record = {}; const failures: string[] = []; + const heldOff: string[] = []; const pass = async ( name: string, run: () => Promise, ): Promise => { + // The hold is re-read before EVERY pass, not once before the cascade. + // The passes are not one transaction, and two of them fan out per-thread + // and per-document deletes, so a hold placed mid-cascade would otherwise + // be ignored for everything after it. 0.4 re-read holds inside all 19 of + // its arms and named the reason: FRCP 37(e) spoliation. + try { + const current = await loadActiveHolds(sql, organizationId); + if (current.orgHeld || current.userMembershipIds.has(targetUserId)) { + heldOff.push(name); + return; + } + } catch (error) { + // An unreadable hold table is not evidence that nothing is held, so the + // pass is skipped rather than run. + console.error(`[erasure] hold re-check before ${name} failed:`, error); + heldOff.push(name); + return; + } try { counts[name] = await run(); } catch (error) { @@ -970,13 +1021,13 @@ export async function processErasure( scrubSubjectAuditLogs(sql, organizationId, targetUserId), ); - const status = failures.length === 0 ? 'done' : 'partial'; + const status = erasureReceiptStatus(failures, heldOff); await sql.begin(async (tx) => { await tx` UPDATE app.gdpr_erasure_requests SET status = ${status}, finished_at_ms = ${Date.now()}, counts = ${tx.json(toJson(counts))}, - error = ${failures.length > 0 ? `failed passes: ${failures.join(', ')}` : null} + error = ${erasureReceiptError(failures, heldOff)} WHERE id = ${requestId} `; await createAuditLog(tx, {