Skip to content

fix(audit-trail): match search and fields against served values, not withheld ones [PRD-1295] - #1936

Open
bexchauveto wants to merge 7 commits into
mainfrom
fix/prd-1295-audit-trail-search-matches-served-values
Open

bexchauveto wants to merge 7 commits into
mainfrom
fix/prd-1295-audit-trail-search-matches-served-values

Conversation

@bexchauveto

@bexchauveto bexchauveto commented Sep 29, 2026 •

Copy link
Copy Markdown
Member

Closes the Node half of a leak found in review of the Ruby PR for PRD-1295, agent-ruby#396, which has the same fix.

The leak

#1909 withholds a gone record's captured values from a caller whose permission scope they fail. But on the history route, search and fields are matched in SQL against the values as captured, before the withholding runs. So a scoped caller still learns what was withheld from which rows come back, meta.count and availableUsers:

GET /_audit-trail/books/2                 -> previousValues: {}
GET /_audit-trail/books/2?search=secret   -> 1 row
GET /_audit-trail/books/2?search=zzz      -> 0 rows

Extending the term one character at a time rebuilds the value. fields leaks the same way: it confirms which columns an out-of-scope change touched.

What changed

When a scope is in effect, the record was gone at the visibility check, and search or fields is present, handleHistory:

  • reads the rows without those two filters;
  • withholds values first, then matches and pages what the caller will actually see, using the same rules as searchCondition / fieldsChangedCondition;
  • scans the history in batches of 500. Each batch continues past the last row read (a new after cursor on listByRecord, keyed on timestamp then id), never at an offset that rows written in between would shift. The scan is bounded at the instant it starts, so an id taken by another record since can't keep it chasing new rows. Only the requested page, the count and one entry per author are kept, so memory is bounded whatever the history's length;
  • builds availableUsers from the matched rows, one entry per userId.

The same holds for a record deleted while the request was in flight: the second read of the record decides the withholding, so the SQL-matched answer is discarded and the scan runs instead. Every other request takes the SQL path, unchanged.

Tests

  • 9 new route tests: a record deleted mid-request is matched on served values; withheld values find nothing and count nothing; served values still match; a withheld row still matches on its author; authors are de-duplicated; a withheld side doesn't match a field filter; the store is read without the value filters and with the snapshot bound; an earlier endDate is kept; the scan batches across 1001 rows by cursor and serves the right page.
  • 1 new SQLite round-trip test for after in both orders: nothing repeats and nothing is skipped when a row is written between reads.
  • With the gate reverted, 6 of the route tests fail.
  • tsc and eslint are clean. The audit-trail suites pass (433 tests).

README.md states the rule next to the redaction one.

Conflicts with #1910: both touch the filters block in handleHistory. operations belongs in rowFilters, since it only matches the operation name, never a captured value.

🤖 Generated with Claude Code

Note

Match audit-trail search and permission scopes against served values, not captured ones

  • Filtered audit-history results, counts, and author lists are now computed from values after withholding when a record is gone or deleted mid-request, instead of SQL matches on captured values (audit-trail.ts)
  • The new scanServedValues scanner is a bounded keyset scan: it stops at the request start time, pages by timestamp/id cursor in batches of up to 500 rows, and returns only the requested page
  • Captured null/undefined fields now follow database-like semantics: they match only explicit null predicates (Blank, Missing, null equality, In with null) and no longer satisfy in-memory inequality or ordered coercion (withhold.ts)
  • Adds an after cursor to AuditHistoryQuery with strict timestamp/id continuation in both sort directions, supported by the SQL store, in-memory store, and tests
  • Behavioral Change: permission-scope evaluation accepts snapshots only when answered fields pass the new explicit-null semantics plus the existing projection check; negated and ordered scopes now withhold captured nulls

Macroscope summarized 62e02d4.

…withheld ones [PRD-1295]

Matched in SQL on a gone record's history, a search still answered what the
withholding hides: whether a row came back, the count and the authors each
said whether a withheld value held the term. Under a scope on a record gone at
the check, the rows are read without those two filters, withheld, then matched
and paged as they go, in batches that continue past the last row read and are
bounded at the instant the scan starts.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@linear-code

linear-code Bot commented Sep 29, 2026

Copy link
Copy Markdown

PRD-1295

@qltysh

qltysh Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

7 new issues

Tool Category Rule Count
qlty Structure Function with high complexity (count = 11): buildHistoryWhereClause 2
qlty Structure Function with many returns (count = 4): asksForNull 2
qlty Structure Function with many parameters (count = 4): matchesAsStored 2
qlty Structure High total complexity (count = 55) 1

Comment thread packages/agent/src/audit-trail/sql-store.ts
Comment thread packages/agent/src/routes/access/audit-trail.ts Outdated
@qltysh

qltysh Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Qlty


Coverage Impact

This PR will not change total coverage.

Modified Files with Diff Coverage (3)

RatingFile% DiffUncovered Line #s
Coverage rating: A Coverage rating: A
packages/agent/src/audit-trail/withhold.ts100.0%
Coverage rating: A Coverage rating: A
packages/agent/src/routes/access/audit-trail.ts100.0%
Coverage rating: A Coverage rating: A
packages/agent/src/audit-trail/sql-store.ts100.0%
Total100.0%
🚦 See full report on Qlty Cloud »

🛟 Help
  • Diff Coverage: Coverage for added or modified lines of code (excludes deleted files). Learn more.

  • Total Coverage: Coverage for the whole repository, calculated as the sum of all File Coverage. Learn more.

  • File Coverage: Covered Lines divided by Covered Lines plus Missed Lines. (Excludes non-executable lines including blank lines and comments.)

    • Indirect Changes: Changes to File Coverage for files that were not modified in this PR. Learn more.

bexchauveto and others added 2 commits September 29, 2026 14:21
…t identity [PRD-1295]

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ed-values

Keeps this branch's cursor-bounded scan over the offset-batched one
#1910 carried, and adds #1910's operations filter to the row filters.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment thread packages/agent/src/routes/access/audit-trail.ts Outdated
…t [PRD-1295]

The second read of the record decides the withholding, so the count,
authors and page follow it too instead of the SQL match.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment thread packages/agent/src/routes/access/audit-trail.ts
bexchauveto and others added 3 commits September 30, 2026 18:18
…-1295]

Breaks timestamp ties by id in the sort direction, as the SQL store
does, so a scan over the fake pages like one over the real store.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…[PRD-1295]

In memory `status != 'private'` holds for a null status and `null < 5` coerces
to `0 < 5`, while the scoped read that guarded the live record left that NULL
out: a record the caller could never read alive became readable once deleted.
A null now matches only Blank, Missing, Equal null or an In list holding null,
leaf by leaf, on every route that withholds.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ull in the list [PRD-1295]

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant