feat(audit): JSONB path filters and NDJSON export (#8)#10
Merged
Conversation
Closes two of the seven remaining subtasks under #8: query-side JSONB filtering on the audit_payloads sibling row, and a streaming NDJSON export that reuses the same filter surface as /audit/events. Filter syntax (audit_events query layer + /audit/events + /audit/export): - param.<dotted.path>=<value> -> request_params @> {...} - response.<dotted.path>=<value> -> response_result @> {...} - header.<name>=<value> -> request_headers @> {name:[value]} - has=<column> -> column IS NOT NULL Each path filter compiles to an EXISTS subquery against audit_payloads keyed by event_id, so it hits the existing jsonb_path_ops GIN indexes. Closed allow-lists (audit.AllowedHasKeys, AllowedJSONSources) gate column / source names; the HTTP layer drops anything else silently. Values are type-detected (true/false -> bool, int/float -> number, quoted -> forced string, else string). NDJSON export (GET /api/v1/portal/audit/export?format=jsonl): - Same filter surface as /events; format=jsonl is the only supported format today (csv etc. reserved for later, returns 400) - Streams via the new audit.StreamingLogger capability (Postgres paginates buildSelect at 1000 rows/page; MemoryLogger iterates Query results; NoopLogger is a no-op) - Hard cap at 100,000 events per request; flushes every 100 lines so long exports don't sit in a buffer - Summary rows only; payload omitted (operators fetch full envelopes via /audit/events/{id}) Other: - audit.SanitizeParameters fast-path was already in main from PR #9 - Drive-by fix to TestIntegration_WhoamiOverHTTP timing flake (AsyncLogger drain race under -race); adds a 2s poll loop. Pre-existing flake, surfaced by the integration suite I ran for this branch. Tests: - pkg/audit/jsonfilter_test.go (parse, containment, match, split, allow-list) - pkg/audit/memory_test.go (filters + Stream) - pkg/audit/postgres/store_test.go (testcontainer: every filter case vs real Postgres; 1100-event Stream pagination) - pkg/httpsrv/portal_api_export_test.go (NDJSON output, format rejection, parseQueryFilter for the new syntax) Docs: - docs/operations/audit.md: new "Two-table layout" + JSONB filters + NDJSON export sections with curl examples - docs/reference/http-api.md: new endpoint rows + JSONB path filter table with index notes make verify green; tagged integration tests green.
Closes the eight findings from the PR review plus two extras the self-review caught after. Significant: - Header name canonicalization. Operators routinely write headers in lower-case in URLs; stored JSONB carries the canonical Go form (User-Agent, X-Api-Key). parseQueryFilter now normalizes via http.CanonicalHeaderKey so ?header.user-agent matches the same rows as ?header.User-Agent. - Export cap test. Stores 10 events, hits ?limit=4, asserts exactly 4 lines. Locks the >= comparison and the early-return. - /audit/events filter wiring test. The endpoint runs the same parseQueryFilter -> Logger.Query path; this test exercises that flow end-to-end through the real handler. Minor: - MaxQueryLimit constant shared by buildSelect and Stream so the page size can't drift silently. - Stream doc explicitly calls out the OFFSET-pagination duplicate risk under concurrent inserts. ts-cursor switch is a follow-up. - /audit/export limit/offset behavior documented in audit.md. - Empty-segment paths (?param.a..b=v) dropped at parse time. - enc.SetEscapeHTML(false) on the NDJSON encoder. - ctx.Err() check at the top of MemoryLogger.Stream. - numericEq extended to float32, int32, uint, uint32, uint64. Self-review caught: - Header values must always be treated as strings on both backends; previously ?header.X-Count=42 built JSONB number 42 and missed a stored "42". Both Postgres and MemoryLogger now compare raw string values for header.* and skip ParseJSONFilterValue. - matchesJSONFilters comment said "matches the first header value" but the code iterates all values. Comment now reflects the actual array-containment behavior. Tests: - portal_api_export_test.go: empty-segment drop, header canonicalization, /events filter wiring, export ?limit cap. - jsonfilter_test.go: TestNumericEq_TypeSurface covers each numeric type the helper accepts. Docs: - http-api.md: header-values-are-strings, header-name-canonicalization, empty-segment-paths sections. - operations/audit.md: limit/offset behavior on /export. make verify green; tagged integration tests green.
Closes the four findings from the second-pass review plus two extras the new self-review checklist surfaced. Significant: - ORDER BY ts DESC + id ASC tiebreaker (Postgres + MemoryLogger). Without it, tied timestamps + OFFSET pagination can duplicate or skip rows at page boundaries; tied ts is common under any sub-ms write rate. New testcontainer test writes 1100 events at the same ts and asserts Stream visits each id exactly once. Minor: - StreamingLogger interface contract documented: f.Limit and f.Offset are ignored by truly-streaming implementations; callers stop early via fn returning a sentinel error. AsyncLogger fallback is best effort, capped at MaxQueryLimit. - MaxQueryLimit moved to pkg/audit so postgres buildSelect, postgres Stream, and AsyncLogger.Stream fallback all reference one constant. Removes the silent-cap-mismatch where a wrapper promised 10000 rows but the backend silently delivered 100. - TestPortalAPI_AuditExport_DoesNotEscapeHTMLChars writes <script> tags + & into ErrorMessage and asserts the export body contains raw bytes, no < / > / & escapes. Locks the SetEscapeHTML(false) contract. Self-review checklist (new) caught: - Doc-vs-code drift on f.Limit. The new StreamingLogger doc said "f.Limit is IGNORED" but MemoryLogger.Stream forwarded f to Query, which truncated. Fix: MemoryLogger.Stream now strips Limit/Offset before delegating, mirroring Postgres. Test TestMemoryLogger_Stream_IgnoresLimit locks the contract. - f.Offset partially honored on Postgres Stream. Same drift: doc said ignored, code preserved a non-negative caller Offset as the starting page. Cleared to 0; doc and code agree. Tests: - TestStore_Stream_TiedTimestampsNoDuplicatesOrSkips (postgres, testcontainer): 1100 events at the same ts, all visited exactly once - TestMemoryLogger_Query_TiedTimestampsAreStable: ts DESC, id ASC ordering verified against the Postgres backend - TestMemoryLogger_Stream_IgnoresLimit: contract assertion - TestPortalAPI_AuditExport_DoesNotEscapeHTMLChars: encoder regression make verify green; tagged integration tests green.
Pre-commit gate review surfaced and fixed concerns across query
semantics, backend consistency, security hardening, and the export
endpoint. All findings addressed before commit; the post-PR review
should find no new issues.
Query / filter:
- header.<name>=v rejects multi-segment paths at parse time;
X-Forwarded-For etc. (single hyphenated names) still match
- Header values are always treated as strings on both backends, no
ParseJSONFilterValue type detection in the header branch
- has=<col> excludes empty containers ('{}', '[]', 'null', '') on
Postgres so MemoryLogger and Postgres agree on len()-based
emptiness for both JSONB and TEXT columns
- IsAllowedHasKey / IsAllowedJSONSource use closed switches, not
iteration over mutable exported slices; round-trip test asserts
the slice and the function agree in both directions
- Empty path segments (?param.a..b=v) and ASCII control characters
in any segment are rejected at parse time
- numericEq covers every Go integer width (int8..int, uint8..uint,
float32, float64, json.Number)
Backend consistency:
- MemoryLogger.Query / Count / Stream share matchAll helper so all
three apply the same filter+sort semantics; only Query applies
the limit clamp. Count and Stream see all matching events
regardless of the page-size default
- ts DESC, id ASC ordering on both backends; tied timestamps no
longer cause Stream pagination duplicates or skips
- StreamingLogger contract documented: f.Limit/f.Offset ignored by
truly-streaming impls; AsyncLogger fallback enforces the cap
inside its loop, not via inner.Query (so a non-Limit-honoring
inner can't leak the cap)
- MaxQueryLimit single source of truth in pkg/audit, used by
buildSelect, postgres Stream, AsyncLogger fallback, and
MemoryLogger.Query
Export endpoint:
- All response headers (Content-Type, Content-Disposition,
Cache-Control: no-store) deferred to a writeExportHeaders closure
called only on first row or no-rows-success, so a backend error
before the first row sends a clean 500 with no misleading
attachment-download header
- Backend errors logged via slog.Warn; client receives a generic
"export stream failed (see server logs)" rather than raw pgx text
- context.Canceled / DeadlineExceeded handled silently (no log
noise, no implicit 200 from a trailing flush)
- Per-row r.Context().Err() check so a client disconnect doesn't
burn a full Postgres page before the page-level ctx check
- json.NewEncoder(w).SetEscapeHTML(false) so '<' lands raw, not
'<'
- Hard cap at 100k rows, separate from the per-page MaxQueryLimit
Tests added:
- TestMemoryLogger_CountAndStream_NotCappedAtQueryDefault locks
the matchAll-helper contract with 250 events
- TestMemoryLogger_Query_TiedTimestampsAreStable / ClampsAboveMaxQueryLimit
- TestStore_Query_LimitClamping covers every Limit branch
- TestStore_Stream_TiedTimestampsNoDuplicatesOrSkips with 1100
events at one ts; TestStore_Stream_PagesThroughCursor with
Offset=500 to lock the offset-ignored claim
- TestAsyncLogger_StreamFallback_NonStreamingInner /
ClampsToMaxQueryLimit (with a fakeLogger that ignores Limit)
- TestPortalAPI_AuditEvents_AppliesJSONFilters wires the HTTP layer
- TestPortalAPI_AuditExport_DoesNotEscapeHTMLChars locks SetEscapeHTML
- TestPortalAPI_AuditExport_HonorsLimitCap asserts the exact ids
emitted (head-of-set, ts DESC) not just the count
- TestNumericEq_TypeSurface enumerates every numeric type
- TestAllowList_FunctionAndSliceAgree round-trips the closed-switch
function against the exported slice
- TestParseQueryFilter_DropsEmptyPathSegments /
CanonicalizesHeaderName
Docs:
- docs/operations/audit.md: limit/offset behavior on /export
documented; em-dash removed
- docs/reference/http-api.md: header single-segment requirement,
empty-segment behavior, has= empty-content semantics, type
detection rule for header values
make verify green; tagged integration tests green; pre-commit
adversarial-review gate verdict: CLEAN.
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
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.
First slice of the v1.1.1 follow-up under #8. Lands the query-side improvements that operators wanted before the portal UI rewrite: JSONB-aware filters that hit the existing GIN indexes, and a streaming NDJSON export. Replay, SSE live tail, the inspection drawer, and comparison page land in subsequent PRs on top of this.
What's in this PR
Filter surface (
/audit/events,/audit/export)Operators can now narrow the audit query by values inside the
audit_payloadssibling row, not just the indexed summary columns:EXISTS (SELECT 1 FROM audit_payloads p WHERE p.event_id = audit_events.id AND p.<col> @> $N::jsonb)subquery, so it hits the existingjsonb_path_opsGIN indexes onrequest_paramsandresponse_result.request_headersis unindexed today; pairheader.*with a time range.audit.AllowedHasKeys,audit.AllowedJSONSources) gate column and source names. The HTTP layer drops anything outside the lists silently rather than 400-ing, matching how unknown plain query params are handled.true/falsebecome JSON booleans, integer / float strings become numbers, quoted forms force string. So?response.isError=truematches the JSON literaltrue, and?param.code="200"forces the string"200".tool,user,success,from,to,q, etc.).NDJSON export (
GET /api/v1/portal/audit/export?format=jsonl)/audit/events;format=jsonlis the default and only supported format today (csvetc. reserved, returns 400).audit.StreamingLoggercapability. The Postgres store paginatesbuildSelectat 1000 rows/page so memory stays bounded; the MemoryLogger iterates a Query result (cheap, but matches the contract); NoopLogger is a no-op./events.http.Flusherflushes every 100 lines so a long export shows progress to the consumer immediately.audit_payloadsenvelope is omitted from each line. Pull full payloads via/audit/events/{id}after filtering.Content-Type: application/x-ndjsonandContent-Disposition: attachment; filename="audit.jsonl"for browser-driven downloads;curl | jqconsumers ignore the disposition.New audit package surface
audit.QueryFiltergainsJSONFilters []JSONPathFilterandHasKeys []string.audit.JSONPathFilteris{Source, Path, Value}where Source is one of"param"/"response"/"header".audit.AllowedHasKeysandaudit.AllowedJSONSourcesexpose the closed sets so the HTTP layer can validate.pkg/audit/jsonfilter.gois the shared compiler used by both backends:ParseJSONFilterValue(type detection),JSONFilterToContainment(dotted path to nested map),MatchJSONPath(memory-side traversal with JSON-semantic equality),SplitJSONPath,IsAllowedHasKey,IsAllowedJSONSource.audit.StreamingLogger: optional capability, type-asserted by the export endpoint, with a graceful fallback throughAsyncLogger.Streamto a boundedQuery.Drive-by
tests/integration_test.go::TestIntegration_WhoamiOverHTTPwas a pre-existing timing flake under-race: it queried Postgres for the audit row beforeAsyncLogger's background drain had flushed. Surfaced consistently while running the integration suite for this branch. Adds a 2-second poll loop. No related code change; just makes the assertion patient enough for the race-detector slowdown.Tests
pkg/audit/jsonfilter_test.goParseJSONFilterValuetype detection (bool, int, float, forced-string, plain),JSONFilterToContainment(empty path, single, nested),MatchJSONPath(numeric widening, missing path, scalar dead-end),SplitJSONPath,IsAllowedHasKeyallow/rejectpkg/audit/memory_test.goJSONFilters+HasKeysmatching againstEvent.Payload(param path, response bool, header value, has-shortcut, AND combination);Streamreturns Query orderpkg/audit/postgres/store_test.go(testcontainer-tagged)Streamtest that proves the page-loop iterates past the 1000-row internal page boundarypkg/httpsrv/portal_api_export_test.go/audit/export?format=jsonlreturns one line per event with payload omitted; unknown format gets 400; missing format defaults to jsonl;parseQueryFilterparses the four new syntaxes (param.*,response.*,header.*,has=) and rejects unknown sources / has-keysDocs
docs/reference/http-api.md: new rows for/audit/events/{id}and/audit/export. New "JSONB path filters" section: syntax table, type-detection rule, index notes (which filters are GIN-backed, which scan).docs/operations/audit.md: new "Two-table layout" section explaining theaudit_events/audit_payloadssplit that landed in v1.1.0; updated REST endpoints table; new "JSONB filters" subsection with curl examples for the four common cases (param path, response shape, header match, has-key); new "NDJSON export" subsection with a curl-piped-through-jq example.Closes (subtasks of #8)
Does not close the umbrella issue. Remaining subtasks (replay endpoint, SSE live tail, portal UI inspection drawer, comparison page,
docs/operations/inspection.mdwalkthrough) land in subsequent PRs per the focused-PR plan.Verification
make verifygreen: gofmt, vet, race-tested unit suite, golangci-lint v2.11.4, gosec v2.25.0, govulncheck, semgrep, coverage gate (80.0%, gate is 80%).go test -tags integration -count=1 -timeout 300s ./...green: testcontainers Postgres exercises the JSONB filter compiler and the streaming pagination; the full HTTP stack tests pass.Test plan
make verifygreenmake dev, fire a fewprogressandechocalls, then:curl -H "X-API-Key: $KEY" "$BASE/api/v1/portal/audit/events?param.message=hello"returns only the matchingechocallscurl -H "X-API-Key: $KEY" "$BASE/api/v1/portal/audit/events?response.isError=true"returns only error responsescurl -H "X-API-Key: $KEY" "$BASE/api/v1/portal/audit/events?has=notifications"returns only progress-tool callscurl -H "X-API-Key: $KEY" "$BASE/api/v1/portal/audit/export?from=$(date -u -v-1H +%FT%TZ)" | wc -lreturns the count of last-hour eventscurl -H "X-API-Key: $KEY" "$BASE/api/v1/portal/audit/export?format=csv"returns 400 with a clear message