fix(platform): governance and audit-integrity dead ends - #3179
Draft
larryro wants to merge 8 commits into
Draft
Conversation
The audit writer hashed the caller's in-memory strings while the verifier rebuilds the record from the stored row, so anything Postgres alters on the way in made an untouched row read as TAMPERED, or failed the write outright. A lone UTF-16 surrogate (a slice through an emoji) is stored as U+FFFD but was hashed as the escape sequence JSON.stringify emits for it; a NUL or a lone surrogate inside a jsonb field made the INSERT throw and took the user's transaction with it; an undefined array item, a sparse hole, or a Date in a jsonb payload diverged silently. Normalize every text field, text[] element, and jsonb key and string into the form the column hands back (toWellFormed, NUL to U+FFFD, jsonb through one JSON round-trip) before hashing AND inserting, so the writer signs exactly what a later read rebuilds. The normalization is the identity on storable content: rows already written keep recomputing to their own hash. Rows a lone surrogate had already broken stay broken (their hash covers content that was never stored) - no history is silently re-signed. Parity test models the text/jsonb round trip; the integration check writes a row with a lone surrogate, NUL, quotes, control characters, a Date, an array hole, and a 200 KB payload, and verifies the chain clean.
Two ways the scheduled audit-integrity walk misreported the chain. A resume anchor the retention sweep had reaped (a job outage longer than the window, or a backlog outpacing the daily page) made the first surviving row's previous_hash mismatch the stored resume hash: a false "chain broken" verdict, a critical bell, and a walk that never advanced past it - clearable only by hand in the database. The walk now takes the org's effective audit-retention cutoff (the same clamped, cooldown- overlaid policy the sweep enforces); an anchor that is gone AND older than that cutoff re-anchors on the first surviving row exactly as a fresh walk would. An anchor missing INSIDE the window is not excused - nothing legitimately deletes an audit row there - so the linkage check still reports it. A real break stamped the alert fingerprint BEFORE writing the bell; when the bell write failed, every later run read the stamp as "already alerted" and never retried - one transient failure and the admins were never told. The bell is now re-asserted on every broken run, idempotent through its dedupe key, and the run reports whether it landed. Unit tests drive both walks over a fake connection; the integration check blocks the bell table with a CHECK constraint and sees the bell land once the block lifts, and points the progress row at a reaped anchor (older than the cutoff: re-anchors to head) and at a vanished in-window one (still a break).
Four hand-rolled copies paired retention categories with their policy fields, and they had drifted: the sweep's clamp and the policy save's bounds check omitted agentRuns (a value below the operator's floor saved fine and the sweep deleted by it), the bounds banner's impact preview included it (promising a clamp that never happened), and the shortening detector missed agentRuns and notifications (shortening either skipped the 7-day cooldown) while still listing a field the schema no longer has. One exported map, RETENTION_POLICY_FIELD_BY_CATEGORY, exhaustive over RETENTION_CATEGORIES by type, now drives all four. The clamp also skips a category the org's applied snapshot does not bound yet instead of crashing the sweep on it - the banner proposes the new category. Tests pin the map's completeness, the agentRuns clamp, preview/clamp parity over every category, and the detector; the integration check saves agentRuns below the floor and is refused with RETENTION_BELOW_FLOOR.
Restoring an expired document or chat thread from the admin Trash flipped only its lifecycle column. The sweep ages documents by created_at_ms and chat threads by threads.updated_at_ms, neither of which a restore touches, so the very next nightly pass re-expired the row the admin had just brought back - and with a zero grace window hard-deleted it outright, silently. The restore already stamps status_changed_at_ms; the document and chat sweeps now age a live row by GREATEST(its age column, that stamp), so a restore restarts the retention window from the moment of the restore. No schema change: the stamp is the clock. Documented on the Trash page in en/de/fr. The integration check restores an expired document and an expired chat thread through the Trash API, sweeps with a 30-day window, and sees both still live while an untouched old control document expires.
A staged DSAR-policy loosening wrote only its pending row; the file flip happened lazily inside the policy page's read once effective_at passed. The erasure lane's enforcement read went straight to the file and never applied a matured change, so the loosening the UI announced as "effective at <date>" stayed inert until an admin happened to reopen the editor - the owner waited out the 24h grace and filing still enforced the old cooling-off, dual-approval, or daily limit. The apply is now one seam, applyMaturedDsarPolicyChange (file write, then DELETE ... RETURNING as the claim, then a system audit row policy.dsar_governance_loosening_applied), and every reader that must see the EFFECTIVE policy runs it first: the erasure lane's readEffectiveDsarPolicy (filing + approval), the editor's read, and a new 5-minute schedule, governance.apply_dsar_policy_changes, that keeps the promise even when nobody opens the page or files a request. Idempotent and race-safe; works inside a caller's transaction or opens its own. The integration check ages a staged loosening past its grace and sees the sweep apply it (row gone, audit row written) before any page read, then stages another and sees the enforcement read apply it on its own.
The externalConversations sweep (#3143, landed on main while this branch was in flight) ages a live conversation by last_message_at_ms alone, so a conversation an admin restored from the Trash was re-expired by the next pass exactly like the document and chat thread the previous commit fixed. The restore's status_changed_at_ms stamp restarts the clock here as well - written as a second comparison rather than GREATEST, which ignores NULLs and would have turned a never-messaged conversation (deliberately not a candidate) into one. The Trash page's sentence and the integration probe now cover all three lifecycle-bearing categories.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Six verified medium-severity governance / audit-integrity defects, each checked against base
e616480a8(post-#3135/#3162) before touching it. All six were live; all six are fixed here, one seam per commit. The branch is rebased onto899fcc08a; #3143's new conversation sweep (landed mid-task) carried finding 4's shape too and is covered by the last commit.Per-finding outcome
audit_logs/service.ts)toStoredAuditRecordnormalizes every text field,text[]element, and jsonb key/string into the form Postgres hands back (toWellFormed, NUL→U+FFFD, jsonb through one JSON round-trip) before hashing and inserting. Parity test inhash-input.test.ts; integration probe writes a row with a lone surrogate, NUL, quotes, control chars, a Date, an array hole, and a 200 KB payload and verifies the chain clean.verify.ts)dedupeKey), never gated on the stamped fingerprint; the run reportsalerted. Unit test drives a failing bell then a retry; integration probe blocks the bell table with aCHECKconstraint, sees the break stamped with no bell, lifts the block, sees the bell land.verify.ts)verifyAuditChaintakesreapedBefore(the org's effective audit-retention cutoff — the same clamped, cooldown-overlaid policy the sweep enforces, viaauditLogRetentionCutoff); an anchor that is gone and older than the cutoff re-anchors on the first surviving row. An anchor missing inside the window is not excused. Unit tests for both; integration probe points the progress row at a reaped anchor (walks to head) and at an in-window vanished one (still a break).governance/trash.ts)status_changed_at_ms; the restore's existing lifecycle stamp restarts the retention clock. No migration. Docs (trash.md, en/de/fr) say so. Integration probe restores an expired document, chat thread, and conversation through the Trash API and sweeps: all three stay live, an untouched control document expires.settings-tail.ts)applyMaturedDsarPolicyChange(file write →DELETE … RETURNINGclaim → system audit rowpolicy.dsar_governance_loosening_applied), run by the erasure lane'sreadEffectiveDsarPolicy(filing + approval), the editor's read, and a new 5-minute schedulegovernance.apply_dsar_policy_changes. Integration probe applies a matured change through the sweep before any page read, then another through the enforcement read.retention_floors.ts)RETENTION_POLICY_FIELD_BY_CATEGORY(exhaustive overRETENTION_CATEGORIESby type) drives the clamp, the save's bounds check, the impact preview, and the shortening detector (which had also missednotifications). The clamp skips a category the applied snapshot does not bound instead of crashing the sweep. Tests pin completeness, the agentRuns clamp, preview↔clamp parity, and the detector; integration probe savesagentRunsRetentionDays: 0and is refused withRETENTION_BELOW_FLOOR.The audit-hash round trip (finding 1)
The writer hashed the caller's in-memory strings; the verifier rebuilds the record from the stored row. Anything Postgres alters on the way in therefore made an untouched row read as tampered, or failed the write:
.slice()through an emoji —truncateRunDetailproduces exactly this) is stored as U+FFFD but was hashed as the escape sequenceJSON.stringifyemits for it → false tamper verdict, forever;undefinedarray items, sparse holes,Date/toJSONobjects in jsonb → silent divergence.The fix hashes the stored form: every text field,
text[]element, and jsonb key/string is normalized the way the column hands it back, and the INSERT writes exactly that. The verifier is untouched (stored data is already in that form, so normalization is the identity there).Chain boundary — none, and nothing silently re-signed. The normalization is the identity on storable content, so every row that recomputes to its own hash today keeps doing so. Rows a lone surrogate had already broken stay broken: their hash covers content that was never stored and cannot be rebuilt; this change prevents new ones. No migration needed (0071 stays free).
Tests
backend/domains/audit_logs/hash-input.test.ts(new, 9) — text/jsonb round-trip model, the defect demonstration, stored-form parity, identity on plain rows.backend/domains/audit_logs/verify.test.ts(new, 7) — re-anchor past a reaped anchor, in-window gap still a break, no excuse without retention, bell survives a failed write and is re-asserted, cutoff asked only with an anchor.backend/core/governance/retention_floors.test.ts(+3),retention_bounds_proposal.test.ts(+1),backend/domains/governance/settings-tail.test.ts(new, 2).backend/integration-check.ts— 6 new probe records (tricky text; bell durability; reaped/in-window anchor; restored document + thread + conversation survive the sweep; DSAR sweep + enforcement read; agentRuns floor).Red on base (the same test files copied onto an export of
e616480a8):hash-input.test.ts8/9 fail (normalizeStoredText is not a function; the passing one demonstrates the defect),verify.test.ts4/7 fail (re-anchor, bell stamp/retry, cutoff), floors/proposal/settings-tail 5 fail (the new ones). The branch harness run against the base tree recorded the finding-4 failure (doc=expired (want null) … thread=expired (want active)) and then aborted at the finding-5 probe (applyMaturedDsarPolicyChanges is not a function— the seam does not exist on base), so probes 1/2/3/6 were not reached there; their red-on-base proof is the unit layer above.Verification
bunx tsc --noEmit(platform): 0 errors.bunx oxlint --type-aware(platform): clean. Docs suite (@tale/docs test): 194/194. Full platform unit suite: 72,742 tests pass; the only 3 red files (app/routes/dashboard/…route tests) fail to load identically on the base export — Vite denies the@fontsource/inter … .woff2?urlimport in theserverproject (environmental, unrelated).backend:integrationon a fresh throwaway tale-db + MinIO per run (SANDBOX_LLM_GATEWAY_ADMIN_PASSWORDset): basee616480a8(its own harness) 375/375 · base899fcc08a(its own harness) 379/379 (its first attempt aborted on the pre-existing40001-in-lockChainHead→ 500 flake noted below; the re-run was clean) · branch (21bb37f55) 385/385 — all six new probe records green.Cross-class discoveries (not fixed here)
selfCheckPriorRowonly logs). Pre-existing; the manual verify still catches it.sweepTempFilesand themessageFeedback/automationRunsweeps delete by age regardless oflifecycle_status, so the Trash stop for those types is cosmetic — same class as finding 4, different tables.clampConfigToBoundsused to crash the whole org sweep on an applied snapshot that predates a category (boundsByCategory[category]undefined); now skipped, but only the banner surfaces the unapplied category.899fcc08a, its own harness): a40001serialization failure insidelockChainHead(the audit chain-head lock) escaped as a 500 from the document upload route, and the harness'sJSON.parseof "Internal Server Error" aborted the run — atransactSerializableretry seam is missing somewhere on that path.