Audit log is mutable and deletable through ordinary ORM access

claude-bot commented

2026-07-28 05:57:13 +00:00

Severity: MEDIUM

The bug

AuditEvent is an ordinary SQLAlchemy model on an ordinary table. Its docstring says
"Append-only audit log. Never mutated after write," but that is enforced only by convention —
AuditRepository happens to expose just emit(). There are no database triggers preventing
UPDATE/DELETE, no append-only constraint, no hash chaining, and no external sink.

Any future code, any migration, or anyone with access to the database file can rewrite history
silently.

Why it matters

docs/circa-spec.md §5 designates the ledger "the canonical audit history," and §6.4 promises
deletions are "recoverable by an admin" — a promise that depends entirely on the ledger being
trustworthy. An attacker who reaches RCE (see the ingest issue) can modify audit_event rows to
erase evidence of what they altered, then adjust photo records to match, and nothing detects the
inconsistency.

This is primarily a post-compromise and future-regression concern rather than a directly
reachable one — the app is the sole writer today.

Fix

SQLite triggers raising on UPDATE and DELETE for audit_event.
Consider hash-chaining: each row stores a hash over the previous row plus its own content, so
tampering is detectable even with file access.
Longer term, the optional append-only external sink already contemplated in #49.

Done when

UPDATE and DELETE on audit_event fail at the database level
A test asserts the triggers fire
If chaining is adopted, a verification command reports tampering

References

backend/app/models/models.py:437-462
backend/app/repositories/audit.py
docs/circa-spec.md §5, §6.4

Related: #49 (append-only audit export sink).

## Severity: MEDIUM ## The bug `AuditEvent` is an ordinary SQLAlchemy model on an ordinary table. Its docstring says "Append-only audit log. Never mutated after write," but that is enforced only by convention — `AuditRepository` happens to expose just `emit()`. There are no database triggers preventing `UPDATE`/`DELETE`, no append-only constraint, no hash chaining, and no external sink. Any future code, any migration, or anyone with access to the database file can rewrite history silently. ## Why it matters `docs/circa-spec.md` §5 designates the ledger "the canonical audit history," and §6.4 promises deletions are "recoverable by an admin" — a promise that depends entirely on the ledger being trustworthy. An attacker who reaches RCE (see the ingest issue) can modify `audit_event` rows to erase evidence of what they altered, then adjust photo records to match, and nothing detects the inconsistency. This is primarily a post-compromise and future-regression concern rather than a directly reachable one — the app is the sole writer today. ## Fix - SQLite triggers raising on `UPDATE` and `DELETE` for `audit_event`. - Consider hash-chaining: each row stores a hash over the previous row plus its own content, so tampering is detectable even with file access. - Longer term, the optional append-only external sink already contemplated in #49. ## Done when - [ ] `UPDATE` and `DELETE` on `audit_event` fail at the database level - [ ] A test asserts the triggers fire - [ ] If chaining is adopted, a verification command reports tampering ## References - `backend/app/models/models.py:437-462` - `backend/app/repositories/audit.py` - `docs/circa-spec.md` §5, §6.4 Related: #49 (append-only audit export sink).

claude-bot added this to the v0.1.1 milestone

2026-07-28 05:57:13 +00:00

claude-bot added the

backend

security

labels

2026-07-28 05:57:13 +00:00

rbrooks referenced this issue from a commit

2026-07-30 20:41:33 +00:00

Prove the audit ledger's append-only triggers actually fire

rbrooks referenced this issue from a commit

2026-07-30 20:41:33 +00:00

Give notes a history instead of overwriting them

claude-bot commented

2026-07-30 20:41:46 +00:00

Author

Done in 093e01b.

The triggers themselves shipped with migration 003; what was missing was any proof they fire. backend/tests/test_audit_immutability.py attacks the ledger through every route the application could plausibly reach it by — the ORM's own unit of work (attribute assignment + flush, session.delete), raw SQL, and unqualified bulk UPDATE/DELETE — rather than only through AuditRepository, since the point of database-level enforcement is to stop the paths the repository does not control. Each abort surfaces as sqlalchemy.exc.IntegrityError carrying the trigger's message, and separate tests confirm the row is unchanged after a refused write and that INSERT still works (a trigger that blocked appends would break the ledger rather than protect it).

The find worth recording. Two of the tests assert the triggers are still present in sqlite_master on the migrated schema. That is not belt-and-braces. On SQLite op.batch_alter_table is create-copy-drop-rename, and DROP TABLE takes the table's triggers with it — silently, because nothing in such a migration mentions triggers. Migration 002 already widened this same enum once, before the triggers existed; the next migration to do so would have quietly undone this issue while looking like an unrelated schema change.

That turned out to be immediate rather than hypothetical: #61 needed a new session_revoked enum value, so migration 004 widens audit_event and had to recreate both triggers afterwards. I verified the failure mode directly rather than reasoning about it — a bare batch_alter_table against a database at 003 leaves sqlite_master with zero triggers. The assertion in this suite is what will catch the omission next time.

Deliberately not done: hash chaining, and the external append-only sink. Chaining defends against someone who can drop the triggers and rewrite rows with file access; that is a real but different threat from the one here, and it needs a verification command to be worth anything (the issue says as much — "if chaining is adopted"). The sink is already #49. Neither is needed for the v0.1.1 exit criteria, and both are cheaper to add once the worker runtime exists.

Closes the "done when" items: UPDATE/DELETE fail at the database level, and a test asserts the triggers fire.

Done in 093e01b. The triggers themselves shipped with migration `003`; what was missing was any proof they fire. `backend/tests/test_audit_immutability.py` attacks the ledger through every route the application could plausibly reach it by — the ORM's own unit of work (attribute assignment + flush, `session.delete`), raw SQL, and unqualified bulk `UPDATE`/`DELETE` — rather than only through `AuditRepository`, since the point of database-level enforcement is to stop the paths the repository does not control. Each abort surfaces as `sqlalchemy.exc.IntegrityError` carrying the trigger's message, and separate tests confirm the row is unchanged after a refused write and that `INSERT` still works (a trigger that blocked appends would break the ledger rather than protect it). **The find worth recording.** Two of the tests assert the triggers are still present in `sqlite_master` on the migrated schema. That is not belt-and-braces. On SQLite `op.batch_alter_table` is create-copy-drop-rename, and `DROP TABLE` takes the table's triggers with it — silently, because nothing in such a migration mentions triggers. Migration `002` already widened this same enum once, before the triggers existed; the next migration to do so would have quietly undone this issue while looking like an unrelated schema change. That turned out to be immediate rather than hypothetical: #61 needed a new `session_revoked` enum value, so migration `004` widens `audit_event` and had to recreate both triggers afterwards. I verified the failure mode directly rather than reasoning about it — a bare `batch_alter_table` against a database at `003` leaves `sqlite_master` with zero triggers. The assertion in this suite is what will catch the omission next time. **Deliberately not done:** hash chaining, and the external append-only sink. Chaining defends against someone who can drop the triggers and rewrite rows with file access; that is a real but different threat from the one here, and it needs a verification command to be worth anything (the issue says as much — "if chaining is adopted"). The sink is already #49. Neither is needed for the v0.1.1 exit criteria, and both are cheaper to add once the worker runtime exists. Closes the "done when" items: `UPDATE`/`DELETE` fail at the database level, and a test asserts the triggers fire.

claude-bot referenced this issue

2026-07-30 20:42:03 +00:00

Logout does not invalidate the session; add server-side revocation #61

claude-bot referenced this issue

2026-07-30 20:42:19 +00:00

Notes are overwritten with no history and an empty audit record #66

claude-bot closed this issue

2026-07-30 20:42:36 +00:00

rbrooks referenced this issue from a commit

2026-08-02 05:39:43 +00:00

Prove the projection and the history cannot disagree

claude-bot referenced this issue