fix(audit-trail): withhold on every route that serves captured values [PRD-1295] - #396
Open
bexchauveto wants to merge 11 commits into
Open
bexchauveto wants to merge 11 commits into
bexchauveto wants to merge 11 commits into
Conversation
… [PRD-1295]
The history route withheld a gone record's captured values from a caller
whose permission scope they fail; `/state` reassembled the same values and
served them unfiltered, and the two correlation routes checked the record
but not the values. The same caller read one request away what the history
route had just withheld.
The withholding moves to `AuditTrailWithholding`, shared by all four routes,
and `/state` answers `{ "data": null }` when the reconstruction fails the
scope — the decision taken on the Node side in agent-nodejs#1909.
One difference on `/state`: a reconstruction can sit on the far side of a
primary-key move the route cannot see, so the requested id fills in only a
key that was never captured at all, never one the trail redacted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
13 new issues
|
![qltysh[bot]](https://avatars.githubusercontent.com/in/890766?s=60&v=4){kind=small}
| # The history route withholds a gone record's captured values from a caller whose scope they fail, and | ||
| # this route is nothing but those values reassembled: without the same test they come back one request | ||
| # away. A reconstruction the scope cannot answer withholds too — absent is not the same as passing. | ||
| def answerable_state(state, scope, context, packed_id) |
| # the scope doesn't apply to them. | ||
| else | ||
| entry | ||
| end |
| # matching. The capture keeps the writable columns, so a scope on anything else — a read-only column, a | ||
| # relation — reads as nil there and would answer for a value the row never held: `status != 'private'` | ||
| # would match, and an ordered operator would raise on the nil. A redacted value answers no better. | ||
| def in_scope?(values, packed_id, withholding, id_answers_for_keys: true) |
| # that may not be the one its values were true under — a state reconstruction, which can sit on the far | ||
| # side of a primary-key move it cannot see. A key never captured at all is still filled: read-only, so | ||
| # it cannot have moved. | ||
| def answerable_snapshot(values, packed_id, collection, id_answers_for_keys: true) |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
|
Coverage Impact This PR will not change total coverage. Modified Files with Diff Coverage (5) 🛟 Help
|
The first check ran before the audit rows were read, so its answer was already stale by the time anything was withheld. Taking it as a fallback — `scope ||= scope_if_gone_since(...)` — meant an id that was gone at the check and taken by another record before the rows came back served that record's history to a caller with no claim on it, where a request starting a moment later answers 404. The first check stays for the 404 it raises before the audit database is touched, and its answer is now discarded. All three routes read again and act on that, rather than only when the first check came back empty. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bexchauveto
force-pushed
the
feature/prd-1295-audit-trail-state-and-the-correlation-routes-bypass-the
branch
from
September 25, 2026 12:22
a7fafb1 to
3a0b718
Compare
…e life before it Taking the second read as the only answer let it clear the withholding the first had established: an id freed by a delete and taken, before the rows came back, by a record the caller *can* read reported "present and in scope", and the dead record's captured values went out unwithheld. Both reads count now and neither cancels the other. The first is the only one that saw the record as it was while the rows were being chosen; the second is the only one that can see a record deleted since, and the only one that can refuse an id now held by a record this caller cannot read. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| # These routes serve the same rows as the per-record history route, so a gone record's captured values | ||
| # are tested against the caller's scope here too — otherwise what that route withholds comes back | ||
| # through a correlation lookup. | ||
| def withhold_gone_record(history, context, collection, record_id, gone_at_check) |
| # taken by another record since — answers for itself, never for the life whose rows these are. The | ||
| # second is the only one that can see a record deleted since, and it is where the 404 comes from when | ||
| # that replacement is one this caller cannot read. | ||
| def withholding_scope_for(context, collection, packed_id, gone_at_check) |
bexchauveto
commented
Sep 25, 2026
bexchauveto
left a comment
bexchauveto
left a comment
Member
Author
There was a problem hiding this comment.
Spec (PRD-1295): conforms. /state takes option 2, both correlation routes withhold row by row, and the predicate is shared in AuditTrailWithholding.
…not the ones withheld [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 in memory. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
|
||
| false | ||
| end | ||
| def history_matched_in_store(context, args, filters, gone_at_check) |
… identity their rows carry Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…eeping only the page Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… instant it starts Offsets over a log still being written to shift between batches, repeating or skipping rows, and an id taken since could keep an oldest-first scan chasing new rows. Each batch now continues past the last row read, within an end bound set when the scan starts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| @@ -74,13 +74,15 @@ def discard(ids) | |||
| end | |||
|
|
|||
| def list_by_record(collection:, record_id:, skip: 0, limit: nil, user_ids: nil, start_timestamp: nil, | |||
…t identity, whatever the sort Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…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>
| # count and the authors would still say what the withholding hides — one probe per character. They are | ||
| # matched against what is served instead, which means scanning the whole history here, in batches, so | ||
| # only the page asked for is kept. Only for a gone record: one in scope was the caller's to read whole. | ||
| def history_matched_after_withholding(context, args, filters, gone_at_check, withholding = nil) |
| # Each batch continues past the last row read rather than at an offset, which entries written between | ||
| # batches would shift. Bounded at the instant the scan starts too, so an id taken by another record | ||
| # since cannot keep an oldest-first scan chasing its new rows. | ||
| def each_withheld_batch(context, args, filters, gone_at_check, withholding, &block) |
…are a timestamp [PRD-1295] The store breaks timestamp ties by id ascending in both directions, so the first row read newest first is not always the latest one. Keep the row with the greatest (timestamp, id) instead of relying on read order. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| latest[entry.user_id] = entry if newer?(entry, latest[entry.user_id]) | ||
| end | ||
|
|
||
| [page, count, -> { latest.values.map { |entry| author_of(entry) } }] |
bexchauveto
commented
Oct 1, 2026
bexchauveto
left a comment
bexchauveto
left a comment
Member
Author
There was a problem hiding this comment.
Spec (PRD-1295): conforms. History, /state and both correlation routes withhold, and /state answers null when the reconstruction fails the scope, per option 2.
…he database would not [PRD-1295] In memory `status != 'private'` holds for a nil status, while the scoped read that guarded the live record left it out: a record the caller could never read alive became readable once deleted. NOT_EQUAL, NOT_IN and NOT_CONTAINS now never match a nil, leaf by leaf, so a scope asking for the nil itself still does. Specs also cover the redaction mask on the served-value scan and assert what the mid-request re-check asks for. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| end | ||
| return false if snapshot[tree.field].nil? && NULL_EXCLUDING_OPERATORS.include?(tree.operator) | ||
|
|
||
| tree.match(snapshot, withholding.collection, withholding.timezone) |
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.
Closes PRD-1295 — the Ruby half. The Node half landed in agent-nodejs#1909.
The leak
PRD-1226 made the history route withhold a gone record's captured values from a caller whose record-level scope those values fail. The other three routes serve the same rows and did not:
What changed
The predicate moves out of the history route into
AuditTrailWithholding, included byAuditTrailRoute, so all four routes share one rule — mirroring Node'saudit-trail/withhold.ts. Nothing about the rule itself changed; the history route's behaviour is unchanged.assert_record_in_scopealready returned the scope needed to do it; the return value was being dropped./statetakes option 2 from the ticket, as Node did: the reconstruction is tested as a whole anddataisnullwhen it fails, rather than 404. A scoped caller keeps the legitimate "what did this deleted record look like" the route exists for.One difference on
/state: a reconstruction can sit on the far side of a primary-key move the route cannot see, so the requested id fills in only a key that was never captured at all (read-only, so it cannot have moved), never one the trail redacted. On a row the id is authoritative, because a row is filed under an id that was true of the side being tested —previous_record_idcarries the other one.Tests
13 new examples, 1270 green in the package. Each of the six behaviours was mutation-tested — reverted one at a time, confirming a spec fails:
/statedoes not withhold at all/statelets the requested id answer for a redacted key/stateskips the second readAUDIT_TRAIL.mdloses the "this covers the history route only" caveat.🤖 Generated with Claude Code
Note
Apply scope-based withholding to all audit-trail routes serving captured values
AuditTrailWithholdingmodule used by the history, state, and correlation routes, so capturedprevious_values/new_valuesthat fail the caller's scope are blanked while event metadata is kept (audit_trail_withholding.rb)each_withheld_batch, with matching count, page, and first-page availableUsers derived from the same pathGET /_audit-trail/:collection_name/:id/statereturnsnildata when the reconstructed state cannot satisfy the caller's scopeAuditTrailWithholding.matches_as_stored?treats captured NULLs as non-matching forNOT_EQUAL,NOT_IN, andNOT_CONTAINS, fixing a scope-check leak; scope checks also no longer treat redaction markers as valuesAuditTrail::Store.list_by_recordsupports timestamp/id cursor continuationMacroscope summarized 1ad5d56.