Review fixes for #271: audit_events RLS/erasure/CSV-injection + tests - #1
Open
amal66 wants to merge 5 commits into
Open
Conversation
…order)
WHY THIS MATTERS
This repo's threat model explicitly includes direct PostgREST access with the
public anon key — which is why schema.sql revokes anon/authenticated on every
backend-owned table. audit_events shipped with neither a revoke nor RLS, so on a
hosted Supabase deployment its default privileges leave the whole table readable
AND writable from the browser: any visitor could dump every user's email, chat
titles and prompt excerpts, or forge/delete audit rows — which defeats the whole
point of an append-only audit trail.
WHAT IS RLS / PostgREST default access
Supabase exposes every table in schema `public` over PostgREST. Whether the
browser `anon`/`authenticated` roles can touch a table is governed by two
things: (1) SQL table GRANTs (managed Supabase's default ACLs grant these roles
broad privileges on new tables), and (2) Row-Level Security. With RLS disabled
and the default grants in place, the table is wide open. The repo's convention
is defense-in-depth: `revoke all ... from anon, authenticated` removes the
grant, and `enable row level security` (with no policies) means even if a grant
slips back the rows are invisible. service_role bypasses RLS, so the backend
path is unaffected.
HOW IT WORKS
- schema.sql: audit_events now has `revoke all ... from anon, authenticated` in
the revoke block and `enable row level security`, matching every sibling
table.
- Grant ordering (F4): `grant ... on all tables in schema public to
service_role` only covers tables that already exist when it runs. The table
was defined *after* that block, so a fresh plain-Postgres install created it
with no service_role privileges and the backend's inserts failed
permission-denied — silently, because recordAudit swallows errors. The DDL is
moved above the grant block so the blanket grant covers it.
- migration 20260728: adds the same revoke + RLS, plus an explicit
`grant select, insert, update, delete ... to service_role` so a fresh apply
works even where service_role has no default ACL for new tables. The old
comment ("no RLS policies needed, like other app tables") inverted the
convention and is corrected.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… in export
WHY THIS MATTERS
audit_events stores personal data: the user's id, email, chat/document titles
and prompt excerpts. Account deletion erased chats, projects, documents and
workflows but left the audit rows behind forever — a GDPR "right to erasure"
gap, and the rows also became orphans pointing at chats/projects that no longer
exist. Separately, GET /user/export (the user's own copy of their data) omitted
audit rows, so the export was incomplete.
WHAT IS "erasure completeness"
When a user deletes their account, every table keyed by their identity must be
swept — not just the primary feature tables. Any table carrying user_id (or
their email, titles, excerpts) is in scope. Audit trails are easy to overlook
precisely because they're written by a fire-and-forget side path, but they hold
some of the most sensitive text in the system (prompt excerpts).
HOW IT WORKS
- deleteUserAccountData: adds `audit_events.delete().eq("user_id", userId)` to
the batched deletion set, alongside workflows/projects/etc. Keyed by user_id so
it removes exactly the departing user's rows.
- buildUserAccountExport: adds an `audit_events` section (the user's own rows,
ordered by created_at) so the export mirrors what deletion removes.
- Test: the account-deletion fixture gains audit rows for two users and asserts
only the other user's row survives (a1/a2 purged, a-other kept).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…A, input bounds
WHY THIS MATTERS
The history route had four independent weaknesses: CSV export could smuggle a
formula into a victim's spreadsheet, /audit/export lacked the export limiter and
MFA gate that every other data export has, an out-of-range ?page= crashed with a
500, and a malformed from/to date crashed with a 500. Titles are attacker-
controllable across shared projects, so these are reachable by another user.
WHAT IS CSV FORMULA INJECTION
Excel/Google Sheets evaluate any cell whose text begins with =, +, -, @, a tab
or a carriage return as a *formula* when the file is opened. A chat titled
=HYPERLINK("http://evil","invoice") therefore executes on export in the
victim's spreadsheet — data exfiltration / phishing with no macro prompt. The
fix (OWASP's recommendation) prefixes a single quote to any value starting with
a trigger char, forcing the value to be treated as literal text. The quote-
trigger regex also gains \r so a leading carriage return is both escaped and
quoted.
HOW IT WORKS
- csvCell (F3): prefixes ' when the value matches /^[=+\-@\t\r]/, and the
CSV-quote test now includes \r.
- Export limiter + MFA (F5): app.ts adds app.get("/audit/export",
exportLimiter) (10/hr) and the route gains requireMfaIfEnrolled, matching
/user/export. A 2000-row export that can include other users' emails no longer
runs under only the general limiter and plain auth.
- Page clamp (F7): parseQuery clamps page into [1, 100000]. Previously
?page=99999999999999 produced a ~5e15 OFFSET that PostgREST rejected as a 500;
the clamp keeps the offset inside Postgres' integer range.
- Date validation (F8): from/to must match ^\d{4}-\d{2}-\d{2}$ (they come from
<input type="date">). parseQuery now returns a discriminated result and the
handlers reply 400 on bad input instead of building "...ZT23:59:59.999Z" and
500ing.
Tests pin csvCell escaping for every trigger char, page clamping/flooring, date
rejection/acceptance, and (with queryEvents/accessibleProjectIds exported)
visibility scoping: own-events OR accessible-project events for owned+shared
projects, own-only when none, and owned/shared id de-duplication.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…oads
WHY THIS MATTERS
The history feature under-recorded reality in two ways. Replicated documents
were attributed to the wrong file and lost their id, and project document
uploads were never recorded at all — so the audit trail silently disagreed with
what the user actually did.
WHAT IS EVENT MINING
recordChatTurn derives one audit row per artifact a chat turn produced by
walking the persisted assistant-event stream (doc_created, doc_edited,
doc_replicated, workflow_applied). Each event type stores its payload
differently, so the miner has to read each shape correctly.
HOW IT WORKS
- doc_replicated (F6): per chat/streaming.ts, this event's top-level `filename`
is the SOURCE document and there is no top-level document_id — the produced
copies live in a `copies: [{ new_filename, document_id, version_id }]` array,
and one event can produce several. The miner previously read the top-level
fields, so it logged one row titled with the source filename and a null id.
It now iterates `copies`, emitting one document.generated row per copy with
that copy's new_filename and document_id.
- Project uploads (F6): POST /projects/:id/documents calls the project-scoped
handleDocumentUpload in projects.ts, a duplicate of the instrumented one in
documents.ts, and it had no recordAudit call — so project uploads never
appeared in history. It now records a document.uploaded event
(surface: "project", the project id, the new document id) on success,
fire-and-forget like the sibling handler.
Tests exercise the miner directly: a mixed turn (created/edited/workflow) maps
to the right actions, a doc_replicated with two copies yields exactly two
document.generated rows carrying the copies' filenames/ids (and never the source
filename), and an empty-copies event yields only the chat.message row.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… History page
WHY THIS MATTERS
Two UX/correctness bugs. When the fetch failed the catch cleared the list, so a
backend outage rendered the "No history yet" empty state — telling the user they
have no history when the truth is the request failed. And rapid filter changes
raced: because responses can arrive out of order, a slow earlier request could
land after a faster later one and overwrite it with stale rows, and a "Load
more" issued mid-filter-change appended a page from the old filter (wrong rows
and duplicate React key={e.id} warnings).
WHAT IS AN OUT-OF-ORDER RESPONSE RACE
The browser does not guarantee that fetches resolve in the order they were
started. If you fire request A (filter=all) then request B (filter=chat), B may
resolve first and A second, leaving the UI showing "all" results under a "chat"
filter. The standard fix is an AbortController: each new load aborts the
previous in-flight request, and the handler ignores any response whose signal
was aborted so a superseded request can neither overwrite fresher state nor
surface a false error.
HOW IT WORKS
- getAuditHistory (mikeApi) gains an optional AbortSignal, threaded into the
underlying fetch via RequestInit.
- HistoryTable keeps the live controller in a ref. Each load() aborts the
previous controller, starts a new one, and passes its signal down. Responses
are dropped when controller.signal.aborted; the aborted-request rejection
(AbortError) is swallowed rather than treated as an error. The effect's
cleanup aborts on unmount.
- A dedicated error state renders "Couldn't load your history — Try again"
(with a retry button) instead of the misleading empty state; the genuine
empty state only shows when there was no error.
- Nit (F12): drops a leaked private-fork "(Clue custom)" comment from
HistorySkeuoIcon.
Frontend tsc is clean and the existing suite passes; the History page has no
pre-existing component test to extend.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
amal66
force-pushed
the
review-fixes/pr271-audit-events
branch
from
August 2, 2026 03:03
6c01b3d to
3cba69a
Compare
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.
Implements the review feedback on Open-Legal-Products#271, packaged as five focused commits on top of your branch so it's easy to merge or cherry-pick.
fix(db)— lock downaudit_eventslike every other backend table: revoke anon/authenticated, enable RLS, and move the DDL above the service_role grant block so fresh installs don't lose backend write access.fix(privacy)— purge a user's audit rows on account deletion and include them in the/user/exportpayload (they carry emails, titles and prompt excerpts).fix(audit)— harden the/auditroute: escape CSV formula injection, rate-limit and MFA-gate the export, and boundlimit/offset/filter inputs.fix(audit)— minedoc_replicatedcopies into history and record project document uploads.fix(history)— show an error state on the History page and cancel stale requests so out-of-order responses can't clobber newer ones.Verified on this branch:
tsc --noEmitclean for backend and frontend; backend vitest 273 passed (including the 14 new audit tests), frontend vitest 38 passed.The branch is based directly on your
audit-historybranch — rebasing onto current main is intentionally left to you.