Keep the audit trail from being truncated away - #139
Conversation
`audit_events_append_only` is BEFORE UPDATE OR DELETE ... FOR EACH ROW, and a row-level trigger cannot fire on TRUNCATE. So `TRUNCATE audit_events` removes every row and raises nothing. Confirmed against Postgres before writing the fix: five rows in, TRUNCATE, zero rows out, while UPDATE and DELETE were refused in the same session. That is the case the schema comment has in mind. It says the guarantee is enforced in the database "because the application is not the only thing that can reach this table", and TRUNCATE is the other thing reaching it. The obvious fix is worse than the gap, so this does not use it. Pointing a BEFORE TRUNCATE trigger at the existing function looks right and fails open: a statement trigger has no OLD record, `OLD.created_at >= now() - ...` is NULL, the IF does not fire, and control reaches RETURN OLD. Measured both ways -- with the retention setting unset it refused, with it set to '30' it truncated the table. It refuses only in the state a happy-path test would check, and permits the truncate exactly while a retention sweep is in flight. So the TRUNCATE branch is answered before the setting is read. Retention still works: a row older than the window is still deleted, a recent one is still refused, and the sweep's own setting is untouched. Two tests alongside it, both proven to fail without the fix: `the trail cannot be truncated away` drives TRUNCATE with the setting unset and again with it set to '30', which is the case that separates this fix from the naive one. Dropping the new trigger reddens it; so does keeping the trigger and removing the TG_OP branch, which is the naive fix in full. `the database refuses a malformed window even when the caller does not` covers the trigger's own zero/negative/empty/non-numeric branch. The existing test named for this loops [0, -1, NaN] through sweepAuditTrail, which returns before it opens a connection, so it proves the TypeScript guard and never reaches the plpgsql one. That test is left as it is, since the caller's guard is worth holding; this one sets the value the sweep would set and deletes directly. audit.test.ts's mirror assertion is repaired rather than extended. It read 0000_schema.sql and asserted three substrings, but 0007 replaced the function and this migration replaces it again, so it described a definition no database runs. It also passed with the trigger dropped outright, which I checked by dropping it: the old assertion returned 1 pass, 0 fail. It now reads the whole chain and asserts the guard is installed and not later dropped.
A deployment behaves differently afterwards: a statement that used to empty the audit trail now raises. The entry says so in the operator's terms, and states the limit rather than overselling the fix, since the shipped compose file connects as the database owner and an owner can still drop the trigger.
|
Thank you for this, and an apology for how it played out: I built the same fix in #148 without checking the open PRs first, and it merged. That was my miss, not a judgement on this. The fix that landed is the same one you wrote — the statement-level This PR also covered ground #148 did not, and I have taken both pieces in #154 with credit to you:
Your changelog entry is in too, including the caveat about the application connecting as the database owner, which was the right thing to state plainly rather than let the entry overclaim. Closing this as superseded. Please do keep them coming. |
* Test the append-only guard the database actually runs The immutability test read `0000_schema.sql` and asserted on the function defined there. 0007 replaced that function and 0012 replaced it again and added the truncate trigger, so the assertions described a definition no database runs, and would have gone on passing if the live one were edited out from under them. A mirror test pinned to one file is a test of that file. It now reads the whole migration chain, requires both triggers, and fails if a later migration drops either. Also covers the trigger's own branch for a malformed retention window. `sweepAuditTrail` returns before it opens a connection, so the existing test proves the caller's guard and never reaches the trigger, which is the branch anything not going through the sweep would meet. Both reported by @beardthelion in #138 and written in their PR #139, which arrived before the fix that merged and covered ground it did not. Catches the changelog up with tonight's fixes, including the truncate hole with the caveat about the owner role that the report was careful to state. * Say who routed a conversation in the third person The mention row's reason read "you chose them yourself". True in the conversation and false on the audit page, where every row belongs to somebody else and an administrator is the one reading it. The person is already on the row as actorUserId; the reason only has to say what kind of decision it was. @zopeVaibhav had this right in #134.
|
Superseded by #148, which ships the same guard: the operation answered before the retention setting is read, so the missing OLD record on a statement-level trigger cannot fall through. Verified against a database built from the current migration chain, with rows present: UPDATE and an unscoped DELETE refused, TRUNCATE refused both with the retention window unset and with it set, and a sweep inside its window still removing only the rows older than it. Closing in favour of it. One thing #148 does not carry, which I will send separately: |
|
Ignore the follow-up I promised above, our comments crossed. #154 had already landed both pieces, so there is nothing separate coming from me. No apology needed on the duplicate. Appreciated all the same. |
2 participants
What this changes
TRUNCATE audit_eventserased the whole trail without raising, becauseaudit_events_append_onlyisFOR EACH ROWand a row-level trigger cannot fire on truncate. A statement-level trigger now refuses it.The
TG_OP = 'TRUNCATE'branch is answered before the retention setting is read, and that ordering is the fix rather than an implementation detail. Reusing the existing function unchanged fails open: a statement trigger has noOLD, soOLD.created_at >= now() - ...isNULL, theIFdoes not fire, and control reachesRETURN OLD. Measured both ways: with the setting unset it refused, with it set to'30'it truncated the table.Closes #138.
Where it runs
psql.Boundary and audit
Changelog
Unreleased->Fixed, including the limitation that the shipped compose file connects as the database owner, so an owner can still drop the trigger.Proof
Full migration chain applied on
pgvector/pgvector:pg17, the image CI uses.TRUNCATE, setting unsetTRUNCATE, setting'30'TRUNCATE,'99999'/'garbage'/CASCADEUPDATEDELETErecent row,'30'DELETEold row,'30'INSERTRetention still works, which is what a careless guard would break.
Both new tests fail without the fix. Dropping the new trigger reddens them, and so does keeping the trigger while removing the
TG_OPbranch, which is the naive fix in full.audit.test.ts's mirror assertion is repaired rather than extended. It read0000_schema.sqland asserted three substrings, but0007replaced the function and this migration replaces it again, so it described a definition no database runs. It also passed with the trigger dropped outright, which I checked by dropping it. It now reads the whole chain and asserts the guard is installed and not later dropped.This adds two tests and repairs one. The rest of the suite behaves the same on this branch as on clean
main: I ran both against the same database, and the only failures are identical on either side, theagent-botstartup tests and a missing generated module, neither touched by this change.