From 28298b0ca8e8ac8e68e67aa84c8a0f4537e06b91 Mon Sep 17 00:00:00 2001 From: Olivier Bex-Chauvet Date: Wed, 7 Oct 2026 16:07:44 +0200 Subject: [PATCH 1/3] fix(audit-trail): escape the search term as JSON, ignore the redaction mask, and re-read a gone record [PRD-1269] Parity with agent-ruby on two gaps. Search: the values are matched as serialized JSON, where a quote or a backslash sits escaped, so the term is now escaped the same way. That finds '15" monitor' and keeps a bare quote off the document's structure. The [redacted] mask is removed before matching, so search=redacted no longer confirms which rows hold a masked value. Same change on the served-value path. Re-read: the record is read again after the audit read even when it was already gone at the first check, and gone at either read means gone. An id taken since by a record the caller cannot read answers 404, and one taken by an in-scope record does not release the earlier life's values. Co-Authored-By: Claude Opus 5.5 --- packages/agent/src/audit-trail/README.md | 16 ++++-- packages/agent/src/audit-trail/index.ts | 1 + .../src/audit-trail/record-visibility.ts | 16 ++++-- packages/agent/src/audit-trail/sql-store.ts | 45 ++++++++++------ .../agent/src/routes/access/audit-trail.ts | 24 +++++---- .../agent/test/audit-trail/sql-store.test.ts | 41 +++++++++++++-- .../access/audit-trail-correlation.test.ts | 4 +- .../test/routes/access/audit-trail.test.ts | 52 ++++++++++++++----- 8 files changed, 145 insertions(+), 54 deletions(-) diff --git a/packages/agent/src/audit-trail/README.md b/packages/agent/src/audit-trail/README.md index 5c6c64ad1f..5768c13c45 100644 --- a/packages/agent/src/audit-trail/README.md +++ b/packages/agent/src/audit-trail/README.md @@ -212,6 +212,11 @@ audit read; a record deleted in between is treated as a request starting a momen treated it, and one moved out of the caller's scope in between is refused. The extra read only happens for a scoped caller — with no scope there is nothing to withhold. +Both reads count, and neither can clear the other, as in agent-ruby. A record gone at either read is +treated as gone: an id freed by a delete and taken by another record since answers for itself, never +for the rows of the record that held it before. And the re-read runs even for a record already gone +at the first check, so an id since taken by a record the caller cannot read is refused (404). + That test only runs when the snapshot can actually answer it. The capture keeps the writable columns (plus the packed record id), so a scope reaching for anything else — a read-only column, a relation, a field stored redacted — has no honest answer in the snapshot and the values are withheld rather @@ -311,11 +316,12 @@ the second read of the record decides the withholding, so the SQL-matched answer the history scanned the same way. Matching the *serialized* text rather than a structural walk of the parsed value is cheap and still -correct for "keys and scalar values" — but two things follow from it. A punctuation-only term (`,`, -`:`, `{`) matches almost any row whose diff has more than one key, since those characters are JSON -structure rather than content. And a value containing a double quote or a backslash can't be found -by searching for it literally — `5"` is stored in the JSON text as `5\"`, so searching `5"` never -matches the row that holds it. Neither is severe, and both are inherent to the approach. +correct for "keys and scalar values". The term is escaped the way JSON escaped the values, so `5"` +finds the row whose JSON text holds `5\"`, and a bare quote can't match the document's own +structure. The redaction mask is removed before matching, so `search=redacted` confirms nothing. +Both hold on the served-value path too, and both mirror agent-ruby. One thing remains inherent to +the approach: a punctuation-only term (`,`, `:`, `{`) matches almost any row whose diff has more +than one key, since those characters are JSON structure rather than content. Defensive parsing: diff --git a/packages/agent/src/audit-trail/index.ts b/packages/agent/src/audit-trail/index.ts index f5e590afe2..12ae8d2a1c 100644 --- a/packages/agent/src/audit-trail/index.ts +++ b/packages/agent/src/audit-trail/index.ts @@ -11,6 +11,7 @@ export { ensureAuditStorage, defineAuditLogModel, fieldsChangedCondition, + jsonEscaped, searchCondition, toRow, fromRow, diff --git a/packages/agent/src/audit-trail/record-visibility.ts b/packages/agent/src/audit-trail/record-visibility.ts index e759e6ff28..de7f154de9 100644 --- a/packages/agent/src/audit-trail/record-visibility.ts +++ b/packages/agent/src/audit-trail/record-visibility.ts @@ -77,9 +77,13 @@ export default async function checkRecordVisibility( * The record can be deleted — or moved out of the caller's permission scope — between the check * that authorized the request and the audit read that answers it: the audit trail lives in its own * database, often its own engine, so no single snapshot spans both. Re-reads once the rows are in - * hand, and returns null when there is nothing to re-read: a caller with no scope has nothing to - * withhold, and a record already gone at the first check has already had the withholding applied — - * an id recreated in the meantime would only widen what is served, never what is hidden. + * hand, and returns null only for a caller with no scope, who has nothing to withhold. + * + * Both reads count, and neither can clear the other, as in agent-ruby. The first is the only one that + * saw the record whose rows these are, so an id freed by a delete and taken by another record since + * answers for itself, never for the earlier life: gone at either read means gone. The second is the + * only one that sees a record deleted since, and the one that answers 404 when the id now belongs to + * a record this caller cannot read — even if it was already gone at the first. */ export async function recheckRecordVisibility( collection: Collection, @@ -88,7 +92,9 @@ export async function recheckRecordVisibility( permissionScope: ConditionTree | null, wasGoneEntirely: boolean, ): Promise { - if (!permissionScope || wasGoneEntirely) return null; + if (!permissionScope) return null; - return checkRecordVisibility(collection, packedId, context, permissionScope); + const now = await checkRecordVisibility(collection, packedId, context, permissionScope); + + return { visible: now.visible, goneEntirely: wasGoneEntirely || now.goneEntirely }; } diff --git a/packages/agent/src/audit-trail/sql-store.ts b/packages/agent/src/audit-trail/sql-store.ts index 9459be3d8f..77baf29a4f 100644 --- a/packages/agent/src/audit-trail/sql-store.ts +++ b/packages/agent/src/audit-trail/sql-store.ts @@ -12,6 +12,7 @@ import type { Model, ModelStatic } from 'sequelize'; import { DataTypes, Op, Sequelize } from 'sequelize'; +import { REDACTED } from './instrument'; import { runAuditMigrations } from './migrations'; export const DEFAULT_SCHEMA = 'forest'; @@ -167,12 +168,20 @@ function jsonColumnAsText(sequelize: Sequelize, column: string): string { return column; } +// What serializing the term would have produced, minus its own surrounding quotes. +export function jsonEscaped(term: string): string { + return JSON.stringify(term).slice(1, -1); +} + // A free-text search against the *serialized* JSON text naturally covers "keys and scalar values // at any depth" without walking the structure by hand: both a key and a value appear as a quoted // literal substring of that text. This runs as a SQL WHERE clause (not an in-memory scan of -// fetched rows), so it composes with pagination and COUNT the same way every other filter does. A -// redacted value can't match a search for the real value — the real value already isn't in this -// text, `redactValues` replaced it before the row was ever written. +// fetched rows), so it composes with pagination and COUNT the same way every other filter does. +// +// Two things keep it honest about what the text holds. The term is escaped the way JSON wrote the +// values, so `15" monitor` (stored as `15\" monitor`) is found, and a bare quote can't match the +// document's own structure. And the redaction mask is removed before matching, so `search=redacted` +// can't confirm which rows hold a masked value. Both mirror agent-ruby's `Sql::TextSearch`. export function searchCondition(sequelize: Sequelize, term: string) { // `~` rather than the standard `\`: MySQL/MariaDB treat backslash as a string-literal escape // character under the default sql_mode (no NO_BACKSLASH_ESCAPES), so a bare `ESCAPE '\'` is @@ -180,21 +189,25 @@ export function searchCondition(sequelize: Sequelize, term: string) { // literal. `~` has no special meaning to any of the four supported dialects' string literals, so // it sidesteps that interaction entirely. Escapes the two LIKE wildcards plus itself, so a term // containing `%`/`_`/`~` is matched literally instead of behaving like a pattern. - const escaped = term.replace(/[~%_]/g, char => `~${char}`); - const pattern = sequelize.escape(`%${escaped}%`); - - const columns = [ - 'action_name', - 'user_first_name', - 'user_last_name', - 'user_email', - jsonColumnAsText(sequelize, 'previous_values'), - jsonColumnAsText(sequelize, 'new_values'), + const like = (expression: string, text: string) => { + const escaped = text.replace(/[~%_]/g, char => `~${char}`); + + return `LOWER(${expression}) LIKE LOWER(${sequelize.escape(`%${escaped}%`)}) ESCAPE '~'`; + }; + + const valuesAsText = (column: string) => + `REPLACE(${jsonColumnAsText(sequelize, column)}, ${sequelize.escape(REDACTED)}, '')`; + + const clauses = [ + ...['action_name', 'user_first_name', 'user_last_name', 'user_email'].map(column => + like(column, term), + ), + ...['previous_values', 'new_values'].map(column => + like(valuesAsText(column), jsonEscaped(term)), + ), ]; - return Sequelize.literal( - `(${columns.map(column => `LOWER(${column}) LIKE LOWER(${pattern}) ESCAPE '~'`).join(' OR ')})`, - ); + return Sequelize.literal(`(${clauses.join(' OR ')})`); } function buildHistoryWhereClause( diff --git a/packages/agent/src/routes/access/audit-trail.ts b/packages/agent/src/routes/access/audit-trail.ts index 94edab757a..397622358f 100644 --- a/packages/agent/src/routes/access/audit-trail.ts +++ b/packages/agent/src/routes/access/audit-trail.ts @@ -16,7 +16,7 @@ import { ValidationError, } from '@forestadmin/datasource-toolkit'; -import { revertRecord } from '../../audit-trail'; +import { REDACTED, jsonEscaped, revertRecord } from '../../audit-trail'; import { parseDateBoundary, parseFields, @@ -248,15 +248,19 @@ export default class AuditTrailRoute extends CollectionRoute { sides.some(values => fields.some(field => Object.prototype.hasOwnProperty.call(values, field)), ); - const texts = [ - entry.actionName, - entry.userFirstName, - entry.userLastName, - entry.userEmail, - ...sides.map(values => JSON.stringify(values)), - ]; - - return touchesField && (!term || texts.some(text => text?.toLowerCase().includes(term))); + + if (!touchesField) return false; + if (!term) return true; + + const identity = [entry.actionName, entry.userFirstName, entry.userLastName, entry.userEmail]; + const valueTerm = jsonEscaped(term); + + return ( + identity.some(text => text?.toLowerCase().includes(term)) || + sides.some(values => + JSON.stringify(values).split(REDACTED).join('').toLowerCase().includes(valueTerm), + ) + ); } private withhold( diff --git a/packages/agent/test/audit-trail/sql-store.test.ts b/packages/agent/test/audit-trail/sql-store.test.ts index c13d2b0fab..d6206849c0 100644 --- a/packages/agent/test/audit-trail/sql-store.test.ts +++ b/packages/agent/test/audit-trail/sql-store.test.ts @@ -869,7 +869,7 @@ describe('createSqlAuditStore (sqlite round-trip)', () => { await close(); }); - it('search does not match a value that was redacted before it was ever stored', async () => { + it('search confirms neither a redacted value nor the mask that replaced it', async () => { const { store, close } = createSqlAuditStore({ connectionString: 'sqlite::memory:' }); // Simulates what instrument.ts's redactValues does before a redacted row ever reaches the @@ -888,7 +888,40 @@ describe('createSqlAuditStore (sqlite round-trip)', () => { ).resolves.toEqual([]); await expect( store.listByRecord({ collection: 'accounts', recordId: '1', search: 'redacted' }), - ).resolves.toHaveLength(1); + ).resolves.toEqual([]); + + await close(); + }); + + it('search finds a value holding a quote or a backslash, as JSON serialized it', async () => { + const { store, close } = createSqlAuditStore({ connectionString: 'sqlite::memory:' }); + await seed(store, record({ recordId: '1', newValues: { name: '15" monitor' } })); + await seed(store, record({ recordId: '1', newValues: { path: 'C:\\temp' } })); + + const byQuote = await store.listByRecord({ + collection: 'accounts', + recordId: '1', + search: '15" monitor', + }); + const byBackslash = await store.listByRecord({ + collection: 'accounts', + recordId: '1', + search: 'C:\\temp', + }); + + expect(byQuote.map(entry => entry.newValues)).toEqual([{ name: '15" monitor' }]); + expect(byBackslash.map(entry => entry.newValues)).toEqual([{ path: 'C:\\temp' }]); + + await close(); + }); + + it('search does not let a bare quote match the JSON structure', async () => { + const { store, close } = createSqlAuditStore({ connectionString: 'sqlite::memory:' }); + await seed(store, record({ recordId: '1', newValues: { status: 'open' } })); + + await expect( + store.listByRecord({ collection: 'accounts', recordId: '1', search: '"status"' }), + ).resolves.toEqual([]); await close(); }); @@ -1070,8 +1103,8 @@ describe('searchCondition', () => { const sqlite = new Sequelize('sqlite::memory:', { logging: false }); const mssql = new Sequelize('mssql://user:pwd@localhost/db', { logging: false }); - expect(searchCondition(sqlite, 'Lyon').val).toMatch(/LOWER\(previous_values\)/); - expect(searchCondition(mssql, 'Lyon').val).toMatch(/LOWER\(previous_values\)/); + expect(searchCondition(sqlite, 'Lyon').val).toMatch(/LOWER\(REPLACE\(previous_values, /); + expect(searchCondition(mssql, 'Lyon').val).toMatch(/LOWER\(REPLACE\(previous_values, /); }); it('matches against the identity columns as well as the JSON columns', () => { diff --git a/packages/agent/test/routes/access/audit-trail-correlation.test.ts b/packages/agent/test/routes/access/audit-trail-correlation.test.ts index 32b5554e7a..1323de916d 100644 --- a/packages/agent/test/routes/access/audit-trail-correlation.test.ts +++ b/packages/agent/test/routes/access/audit-trail-correlation.test.ts @@ -200,7 +200,7 @@ describe('AuditTrailCorrelationRoute', () => { jest .spyOn(dataSource.getCollection('books'), 'list') .mockResolvedValueOnce([]) // scoped check: not found - .mockResolvedValueOnce([]); // bare check: genuinely gone + .mockResolvedValue([]); // bare check: genuinely gone, and at the re-read const route = new AuditTrailCorrelationRoute(services, options, dataSource); const context = contextWith({ timezone: 'Europe/Paris', @@ -304,7 +304,7 @@ describe('AuditTrailCorrelationRoute', () => { jest .spyOn(dataSource.getCollection('books'), 'list') .mockResolvedValueOnce([]) // scoped check: not found - .mockResolvedValueOnce([]); // bare check: genuinely gone + .mockResolvedValue([]); // bare check: genuinely gone, and at the re-read const route = new AuditTrailCorrelationRoute(services, options, dataSource); const context = contextWith({ timezone: 'Europe/Paris', diff --git a/packages/agent/test/routes/access/audit-trail.test.ts b/packages/agent/test/routes/access/audit-trail.test.ts index bd737cdf4e..9257269d05 100644 --- a/packages/agent/test/routes/access/audit-trail.test.ts +++ b/packages/agent/test/routes/access/audit-trail.test.ts @@ -823,7 +823,7 @@ describe('AuditTrailRoute', () => { jest .spyOn(dataSource.getCollection('books'), 'list') .mockResolvedValueOnce([]) // scoped check: not found - .mockResolvedValueOnce([]); // bare check: genuinely gone, not just out of scope + .mockResolvedValue([]); // bare check: genuinely gone, not just out of scope, and at the re-read const route = new AuditTrailRoute(services, options, dataSource, 'books'); const context = createMockContext({ state: { user: { email: 'john.doe@domain.com' } }, @@ -925,6 +925,23 @@ describe('AuditTrailRoute', () => { }); }); + test('does not confirm a masked value by searching the mask', async () => { + const { body } = await searched( + [kept({ previousValues: { ownerId: 1, ssn: '[redacted]' } })], + { search: 'redacted' }, + ); + + expect(body.data).toEqual([]); + }); + + test('finds a served value holding a quote, as JSON serialized it', async () => { + const quoted = kept({ previousValues: { ownerId: 1, title: '15" monitor' } }); + + const { body } = await searched([quoted], { search: '15" monitor' }); + + expect(body.data).toEqual([quoted]); + }); + test('does not match a field only a withheld side touched', async () => { const { body } = await searched([secretDelete()], { fields: 'title' }); @@ -1040,7 +1057,7 @@ describe('AuditTrailRoute', () => { jest .spyOn(dataSource.getCollection('books'), 'list') .mockResolvedValueOnce([]) // scoped check: not found - .mockResolvedValueOnce([]); // bare check: genuinely gone, not just out of scope + .mockResolvedValue([]); // bare check: genuinely gone, not just out of scope, and at the re-read const route = new AuditTrailRoute(services, options, dataSource, 'books'); const context = createMockContext({ state: { user: { email: 'john.doe@domain.com' } }, @@ -1070,7 +1087,7 @@ describe('AuditTrailRoute', () => { jest .spyOn(dataSource.getCollection('books'), 'list') .mockResolvedValueOnce([]) // scoped check: not found - .mockResolvedValueOnce([]); // bare check: genuinely gone + .mockResolvedValue([]); // bare check: genuinely gone, and at the re-read const route = new AuditTrailRoute(services, options, dataSource, 'books'); const context = createMockContext({ state: { user: { email: 'john.doe@domain.com' } }, @@ -1135,7 +1152,7 @@ describe('AuditTrailRoute', () => { jest .spyOn(dataSource.getCollection('books'), 'list') .mockResolvedValueOnce([]) // scoped check: not found - .mockResolvedValueOnce([]); // bare check: genuinely gone, not just out of scope + .mockResolvedValue([]); // bare check: genuinely gone, not just out of scope, and at the re-read const route = new AuditTrailRoute(services, options, dataSource, 'books'); const context = createMockContext({ state: { user: { email: 'john.doe@domain.com' } }, @@ -1166,7 +1183,7 @@ describe('AuditTrailRoute', () => { jest .spyOn(dataSource.getCollection('books'), 'list') .mockResolvedValueOnce([]) // scoped check: not found - .mockResolvedValueOnce([]); // bare check: genuinely gone, not just out of scope + .mockResolvedValue([]); // bare check: genuinely gone, not just out of scope, and at the re-read const route = new AuditTrailRoute(services, options, dataSource, 'books'); const context = createMockContext({ state: { user: { email: 'john.doe@domain.com' } }, @@ -1284,10 +1301,21 @@ describe('AuditTrailRoute', () => { expect(services.authorization.getScope).toHaveBeenCalledTimes(1); }); - test('does not ask again for a record that was already gone at the first check', async () => { - const { list } = await raceWith([], []); + test('refuses when an id gone at the first check now belongs to a record the caller cannot read', async () => { + // gone at the check (scoped and bare), then the id was taken by someone else's record + const { context } = await raceWith([], [], [], [{ id: 2 }]); + + expect(context.throw).toHaveBeenCalledWith(404, 'Record does not exists'); + }); + + test('keeps withholding when an in-scope record took the id after the first check', async () => { + // gone at the check; the replacement answers for itself, not for the rows of the earlier life + const { context } = await raceWith([], [], [{ id: 2 }]); - expect(list).toHaveBeenCalledTimes(2); + expect(context.throw).not.toHaveBeenCalled(); + expect((context.response.body as { data: unknown[] }).data).toEqual([ + { operation: 'delete', recordId: '2', previousValues: {}, newValues: {} }, + ]); }); }); @@ -2159,7 +2187,7 @@ describe('AuditTrailRoute', () => { jest .spyOn(dataSource.getCollection('books'), 'list') .mockResolvedValueOnce([]) // scoped fetch: not found - .mockResolvedValueOnce([]); // bare check: genuinely gone, not just out of scope + .mockResolvedValue([]); // bare check: genuinely gone, not just out of scope, and at the re-read const context = createMockContext({ state: { user: { email: 'john.doe@domain.com' } }, customProperties: { @@ -2222,7 +2250,7 @@ describe('AuditTrailRoute', () => { .spyOn(dataSource.getCollection('books'), 'list') .mockResolvedValueOnce([{ id: 2, status: 'closed', name: 'Acme' }]) // present and in scope .mockResolvedValueOnce([]) // re-read, scoped: gone - .mockResolvedValueOnce([]); // re-read, bare: genuinely gone + .mockResolvedValue([]); // re-read, bare: genuinely gone, and at the re-read const context = createMockContext({ state: { user: { email: 'john.doe@domain.com' } }, customProperties: { @@ -2278,7 +2306,7 @@ describe('AuditTrailRoute', () => { jest .spyOn(dataSource.getCollection('books'), 'list') .mockResolvedValueOnce([]) // scoped fetch: not found - .mockResolvedValueOnce([]); // bare check: genuinely gone + .mockResolvedValue([]); // bare check: genuinely gone, and at the re-read const context = createMockContext({ state: { user: { email: 'john.doe@domain.com' } }, customProperties: { @@ -2303,7 +2331,7 @@ describe('AuditTrailRoute', () => { jest .spyOn(dataSource.getCollection('books'), 'list') .mockResolvedValueOnce([]) // scoped fetch: not found - .mockResolvedValueOnce([]); // bare check: genuinely gone + .mockResolvedValue([]); // bare check: genuinely gone, and at the re-read const context = createMockContext({ state: { user: { email: 'john.doe@domain.com' } }, customProperties: { From 92335a445cc5b6816cd5e8cf053d17b47f5dbb67 Mon Sep 17 00:00:00 2001 From: Olivier Bex-Chauvet Date: Wed, 7 Oct 2026 16:12:26 +0200 Subject: [PATCH 2/3] fix(audit-trail): withhold the rows of an earlier record that held the id [PRD-1269] A delete frees the id and a later record can take it. Both reads then see a live record in scope, so the route served the whole history as the current record's, including values the caller's scope would have withheld on the record that held the id before. Rows filed at or before the id's last confirmed delete now go through the gone-record withholding: on the history route (search and fields matched against the served values, so count and authors cannot leak them), on the correlation lookups, and on /state for a reconstruction at or before that delete. A pending delete frees nothing. The history route's early path for a record gone at the first check also re-reads it now, so an id since taken by a record the caller cannot read answers 404 there too. Co-Authored-By: Claude Opus 5.5 --- packages/agent/src/audit-trail/README.md | 8 + .../agent/src/audit-trail/earlier-life.ts | 33 ++++ .../routes/access/audit-trail-correlation.ts | 18 ++- .../agent/src/routes/access/audit-trail.ts | 66 ++++++-- .../access/audit-trail-correlation.test.ts | 30 +++- .../test/routes/access/audit-trail.test.ts | 143 ++++++++++++++++++ 6 files changed, 281 insertions(+), 17 deletions(-) create mode 100644 packages/agent/src/audit-trail/earlier-life.ts diff --git a/packages/agent/src/audit-trail/README.md b/packages/agent/src/audit-trail/README.md index 5768c13c45..899f58a78e 100644 --- a/packages/agent/src/audit-trail/README.md +++ b/packages/agent/src/audit-trail/README.md @@ -217,6 +217,14 @@ treated as gone: an id freed by a delete and taken by another record since answe for the rows of the record that held it before. And the re-read runs even for a record already gone at the first check, so an id since taken by a record the caller cannot read is refused (404). +**An id freed by a delete and taken since has two lives.** A live record in the caller's scope says +nothing about the rows filed under its id before the id's last confirmed `delete`: they are an +earlier record's. Those rows go through the same withholding as a record gone for good — on the +history route (with `search` and `fields` matched against the served values, so the count and the +authors cannot leak them either), on the correlation lookups, and on `/state`, whose reconstruction +at or before that `delete` is tested against the scope. A `pending` delete frees nothing, since it may +never have landed. + That test only runs when the snapshot can actually answer it. The capture keeps the writable columns (plus the packed record id), so a scope reaching for anything else — a read-only column, a relation, a field stored redacted — has no honest answer in the snapshot and the values are withheld rather diff --git a/packages/agent/src/audit-trail/earlier-life.ts b/packages/agent/src/audit-trail/earlier-life.ts new file mode 100644 index 0000000000..bccdf039d1 --- /dev/null +++ b/packages/agent/src/audit-trail/earlier-life.ts @@ -0,0 +1,33 @@ +import type { AuditRecord, AuditStore } from './types'; + +// A delete frees the id, and a later record can take it. A live record under the caller's scope says +// nothing about the rows filed under that id before its last delete: they are another record's, and +// its values go through the same withholding as a record gone for good. + +/** The id's latest confirmed `delete`, or null when the id has never been freed. */ +export async function lastDeleteOf( + store: AuditStore, + collection: string, + recordId: string, +): Promise { + const deletes = await store.listByRecord({ + collection, + recordId, + operations: ['delete'], + order: 'desc', + }); + + // A pending delete may never have landed, so it frees nothing. Filtered here too, not just in + // the query: a store that ignores `operations` must not turn any row into the boundary. + return deletes.find(row => row.operation === 'delete' && row.status !== 'pending') ?? null; +} + +/** Whether a row was filed at or before the boundary delete, in the trail's (timestamp, id) order. */ +export function belongsToEarlierLife(row: AuditRecord, lastDelete: AuditRecord | null): boolean { + if (!lastDelete) return false; + + return ( + row.timestamp < lastDelete.timestamp || + (row.timestamp === lastDelete.timestamp && row.id <= lastDelete.id) + ); +} diff --git a/packages/agent/src/routes/access/audit-trail-correlation.ts b/packages/agent/src/routes/access/audit-trail-correlation.ts index 17aef48285..d9b3b4d3ea 100644 --- a/packages/agent/src/routes/access/audit-trail-correlation.ts +++ b/packages/agent/src/routes/access/audit-trail-correlation.ts @@ -7,6 +7,7 @@ import type { Context } from 'koa'; import { ValidationError } from '@forestadmin/datasource-toolkit'; +import { belongsToEarlierLife, lastDeleteOf } from '../../audit-trail/earlier-life'; import checkRecordVisibility, { recheckRecordVisibility, } from '../../audit-trail/record-visibility'; @@ -110,14 +111,27 @@ export default class AuditTrailCorrelationRoute extends BaseRoute { const gone = after ? after.goneEntirely : target.goneEntirely; - if (!target.permissionScope || !gone) return entries; + if (!target.permissionScope) return entries; - return withholdOutsidePermissionScope(entries, { + // Live now, but the rows up to the id's last delete may be an earlier record's. + const lastDelete = gone + ? null + : await lastDeleteOf(this.options.auditTrail.store, target.collection, target.recordId); + + if (!gone && !lastDelete) return entries; + + const withheld = withholdOutsidePermissionScope(entries, { collection: target.collectionObject, permissionScope: target.permissionScope, timezone: QueryStringParser.parseCaller(context, { defaultTimezone: 'UTC' }).timezone, logger: this.options.logger, }); + + return gone + ? withheld + : entries.map((entry, index) => + belongsToEarlierLife(entry, lastDelete) ? withheld[index] : entry, + ); } // Returns null (after issuing the 404) when a configured record-level permission scope excludes the id — diff --git a/packages/agent/src/routes/access/audit-trail.ts b/packages/agent/src/routes/access/audit-trail.ts index 397622358f..d90eaa976d 100644 --- a/packages/agent/src/routes/access/audit-trail.ts +++ b/packages/agent/src/routes/access/audit-trail.ts @@ -17,6 +17,7 @@ import { } from '@forestadmin/datasource-toolkit'; import { REDACTED, jsonEscaped, revertRecord } from '../../audit-trail'; +import { belongsToEarlierLife, lastDeleteOf } from '../../audit-trail/earlier-life'; import { parseDateBoundary, parseFields, @@ -96,8 +97,8 @@ export default class AuditTrailRoute extends CollectionRoute { const filtersOnValues = Boolean(fields || search); - const serveMatchedValues = async (scope: ConditionTree) => { - const matched = await this.scanServedValues(context, scope, rowFilters, { + const serveMatchedValues = async (withholdServed: (rows: AuditRecord[]) => AuditRecord[]) => { + const matched = await this.scanServedValues(context, withholdServed, rowFilters, { fields, search, order, @@ -116,8 +117,23 @@ export default class AuditTrailRoute extends CollectionRoute { }; }; + const withholdAll = (rows: AuditRecord[]) => + permissionScope ? this.withhold(rows, permissionScope, context) : rows; + if (permissionScope && goneEntirely && filtersOnValues) { - await serveMatchedValues(permissionScope); + await serveMatchedValues(withholdAll); + + // Gone at the check, so everything stays withheld whatever the re-read finds; it is asked + // only for the 404, when the id has since been taken by a record this caller cannot read. + const recheck = await recheckRecordVisibility( + this.collection, + context.params.id, + context, + permissionScope, + goneEntirely, + ); + + if (recheck && !recheck.visible) context.throw(HttpCode.NotFound, 'Record does not exists'); return; } @@ -150,10 +166,27 @@ export default class AuditTrailRoute extends CollectionRoute { const gone = after ? after.goneEntirely : goneEntirely; - // Gone between the check and the read: this answer withholds, so the rows, count and authors - // matched in SQL above must not decide what is served either. - if (permissionScope && gone && filtersOnValues) { - await serveMatchedValues(permissionScope); + // Live now, but the id may have been freed by a delete and taken since: the rows up to that + // delete are the earlier record's, which the current one's scope says nothing about. + const lastDelete = + permissionScope && !gone + ? await lastDeleteOf(store, this.collection.name, context.params.id) + : null; + + const withholdServed = gone + ? withholdAll + : (rows: AuditRecord[]) => { + const withheld = withholdAll(rows); + + return rows.map((row, index) => + belongsToEarlierLife(row, lastDelete) ? withheld[index] : row, + ); + }; + + // Gone between the check and the read, or carrying an earlier life: this answer withholds, so + // the rows, count and authors matched in SQL above must not decide what is served either. + if (permissionScope && (gone || lastDelete) && filtersOnValues) { + await serveMatchedValues(withholdServed); return; } @@ -162,9 +195,8 @@ export default class AuditTrailRoute extends CollectionRoute { // existence against — but create/update/delete rows still carry captured column values from // when the record existed. If those values themselves would have failed the caller's permission scope, // withhold them while still surfacing that the row happened, by whom and when: that part - // stays visible regardless. - const data = - permissionScope && gone ? this.withhold(rawData, permissionScope, context) : rawData; + // stays visible regardless. The same holds for an earlier record that held this id. + const data = permissionScope && (gone || lastDelete) ? withholdServed(rawData) : rawData; context.response.body = { data: data.map(({ previousRecordId, ...served }) => served), @@ -180,7 +212,7 @@ export default class AuditTrailRoute extends CollectionRoute { // bounded at the instant the scan starts so an id taken since cannot keep it chasing new rows. private async scanServedValues( context: Context, - permissionScope: ConditionTree, + withholdServed: (rows: AuditRecord[]) => AuditRecord[], rowFilters: Omit, { fields, @@ -210,7 +242,7 @@ export default class AuditTrailRoute extends CollectionRoute { ...(after && { after: { timestamp: after.timestamp, id: after.id } }), }); - const matched = this.withhold(rows, permissionScope, context).filter(entry => + const matched = withholdServed(rows).filter(entry => AuditTrailRoute.matchesServedValues(entry, fields, search), ); @@ -330,6 +362,14 @@ export default class AuditTrailRoute extends CollectionRoute { const goneNow = after ? after.goneEntirely : goneEntirely; + // A live record whose id was freed by a delete and taken since: a state at or before that + // delete is the earlier record's, which the current one's scope says nothing about. + const lastDelete = + permissionScope && !goneNow + ? await lastDeleteOf(store, this.collection.name, context.params.id) + : null; + const ofEarlierLife = Boolean(lastDelete && at <= lastDelete.timestamp); + // `startTimestamp` is an inclusive lower bound, so an entry timestamped exactly `at` comes // back too — but the record already reflects that entry's change at instant `at`, so it must // be kept rather than reverted (which would wrongly return the state just *before* it). @@ -368,7 +408,7 @@ export default class AuditTrailRoute extends CollectionRoute { // id is merged in first for the same reason it is on a row: a read-only primary key never lands // in the capture, so a scope on the id would blank the very record it names. if ( - goneNow && + (goneNow || ofEarlierLife) && !permissionScopeAccepts( // `false`: the reconstruction may sit on the far side of a primary-key move this route // cannot see, so the requested id does not answer for a key the trail redacted. diff --git a/packages/agent/test/routes/access/audit-trail-correlation.test.ts b/packages/agent/test/routes/access/audit-trail-correlation.test.ts index 1323de916d..9dad2fc5c8 100644 --- a/packages/agent/test/routes/access/audit-trail-correlation.test.ts +++ b/packages/agent/test/routes/access/audit-trail-correlation.test.ts @@ -17,7 +17,7 @@ describe('AuditTrailCorrelationRoute', () => { }), ]); const store = { - listByRecord: jest.fn(), + listByRecord: jest.fn().mockResolvedValue([]), countByRecord: jest.fn(), listByCorrelation: jest.fn().mockResolvedValue(history), listByCorrelations: jest.fn().mockResolvedValue(history), @@ -244,6 +244,32 @@ describe('AuditTrailCorrelationRoute', () => { }); }); + test('withholds an entry from before the id was freed and taken by a record in scope', async () => { + const freed = { + id: 2, + timestamp: '2026-01-02T00:00:00.000Z', + operation: 'delete', + recordId: '2', + correlationKey: 'req-1', + previousValues: { title: 'Secret' }, + newValues: {}, + }; + const { services, dataSource, options, store } = setup([freed]); + store.listByRecord.mockResolvedValue([freed]); + (services.authorization.getScope as jest.Mock).mockResolvedValue( + new ConditionTreeLeaf('id', 'Equal', 1), + ); + jest.spyOn(dataSource.getCollection('books'), 'list').mockResolvedValue([{ id: 2 }]); + const route = new AuditTrailCorrelationRoute(services, options, dataSource); + const context = contextWith({ timezone: 'Europe/Paris', collection: 'books', recordId: '2' }); + + await route.handleHistory(context); + + expect((context.response.body as { data: unknown[] }).data).toEqual([ + { ...freed, previousValues: {} }, + ]); + }); + test('allows an id inside a restrictive scope through to the store', async () => { const history = [{ operation: 'update', correlationKey: 'req-1' }]; const { services, dataSource, options, store } = setup(history); @@ -489,7 +515,7 @@ describe('AuditTrailCorrelationRoute', () => { const services = factories.forestAdminHttpDriverServices.build(); const dataSource = buildDataSource(); const store = { - listByRecord: jest.fn(), + listByRecord: jest.fn().mockResolvedValue([]), countByRecord: jest.fn(), listByCorrelation: jest.fn(), listByCorrelations: jest.fn(), diff --git a/packages/agent/test/routes/access/audit-trail.test.ts b/packages/agent/test/routes/access/audit-trail.test.ts index 9257269d05..32a4d83f81 100644 --- a/packages/agent/test/routes/access/audit-trail.test.ts +++ b/packages/agent/test/routes/access/audit-trail.test.ts @@ -1319,6 +1319,86 @@ describe('AuditTrailRoute', () => { }); }); + describe('an id freed by a delete and taken by a record in scope since', () => { + const row = (over: Record) => ({ + recordId: '2', + userId: 7, + userFirstName: null, + userLastName: null, + userEmail: 'jane@acme.io', + actionName: null, + ...over, + }); + const current = row({ + id: 3, + timestamp: '2026-01-03T00:00:00.000Z', + operation: 'create', + previousValues: {}, + newValues: { ownerId: 1, title: 'Mine' }, + }); + const freed = row({ + id: 2, + timestamp: '2026-01-02T00:00:00.000Z', + operation: 'delete', + previousValues: { ownerId: 2, title: 'Secret' }, + newValues: {}, + }); + const earlier = row({ + id: 1, + timestamp: '2026-01-01T00:00:00.000Z', + operation: 'update', + previousValues: { ownerId: 2, title: 'Old secret' }, + newValues: { ownerId: 2, title: 'Secret' }, + }); + + const readHistory = async (history: unknown[], query: Record = {}) => { + const { services, dataSource, options } = setup(history); + (services.authorization.getScope as jest.Mock).mockResolvedValue( + new ConditionTreeLeaf('ownerId', 'Equal', 1), + ); + jest.spyOn(dataSource.getCollection('books'), 'list').mockResolvedValue([{ id: 2 }]); + const route = new AuditTrailRoute(services, options, dataSource, 'books'); + const context = createMockContext({ + state: { user: { email: 'john.doe@domain.com' } }, + customProperties: { query: { timezone: 'Europe/Paris', ...query }, params: { id: '2' } }, + }); + + await route.handleHistory(context); + + return context.response.body as { data: unknown[]; meta: unknown }; + }; + + test('withholds the earlier record values and serves the current record', async () => { + const body = await readHistory([current, freed, earlier]); + + expect(body.data).toEqual([ + current, + { ...freed, previousValues: {} }, + { ...earlier, previousValues: {}, newValues: {} }, + ]); + }); + + test('matches a search on the values served, so it cannot find the earlier record values', async () => { + const body = await readHistory([current, freed, earlier], { search: 'secret' }); + + expect(body).toEqual({ data: [], meta: { count: 0, availableUsers: [] } }); + }); + + test('does not treat a pending delete as freeing the id, since it may never have landed', async () => { + const pending = { ...freed, status: 'pending' }; + + const body = await readHistory([current, pending, earlier]); + + expect(body.data).toEqual([current, pending, earlier]); + }); + + test('serves the whole history of an id that was never freed', async () => { + const body = await readHistory([current, earlier]); + + expect(body.data).toEqual([current, earlier]); + }); + }); + describe('a snapshot that cannot answer the scope', () => { // Mirrors what `instrument.ts` captures: writable columns only, so a read-only column is // absent from the snapshot and a redacted one holds the placeholder rather than the value. @@ -2173,6 +2253,69 @@ describe('AuditTrailRoute', () => { // The history route withholds a gone record's captured values from a caller whose scope they // fail; this route is those same values reassembled, so it has to answer the same way or the // withheld values are one request away. + describe('an id freed by a delete and taken by a record in scope since', () => { + const history = [ + { + id: 3, + timestamp: '2026-06-20T00:00:00.000Z', + operation: 'create', + previousValues: {}, + newValues: { id: 2, status: 'mine', name: 'New' }, + }, + { + id: 2, + timestamp: '2026-06-19T00:00:00.000Z', + operation: 'delete', + previousValues: { id: 2, status: 'secret', name: 'Old' }, + newValues: {}, + }, + ]; + + const stateAt = async (at: string) => { + const { services, dataSource, store, route } = setupBooks(); + store.listByRecord.mockImplementation( + async ({ + startTimestamp, + operations, + }: { + startTimestamp?: string; + operations?: string[]; + }) => + history.filter( + entry => + (!startTimestamp || entry.timestamp >= startTimestamp) && + (!operations || operations.includes(entry.operation)), + ), + ); + (services.authorization.getScope as jest.Mock).mockResolvedValue( + new ConditionTreeLeaf('status', 'Equal', 'mine'), + ); + jest + .spyOn(dataSource.getCollection('books'), 'list') + .mockResolvedValue([{ id: 2, status: 'mine', name: 'New' }]); + const context = createMockContext({ + state: { user: { email: 'john.doe@domain.com' } }, + customProperties: { query: { timezone: 'UTC', at }, params: { id: '2' } }, + }); + + await route.handleStateAt(context); + + return context; + }; + + test('withholds a state from before the delete when the earlier record fails the scope', async () => { + const context = await stateAt('2026-06-18'); + + expect(context.response.body).toEqual({ data: null }); + }); + + test('serves a state from after the id was taken', async () => { + const context = await stateAt('2026-06-21'); + + expect(context.response.body).toEqual({ data: { id: 2, status: 'mine', name: 'New' } }); + }); + }); + describe('a genuinely gone record, read by a scoped caller', () => { const reconstructFor = async (scope: ConditionTreeLeaf, status: string | null = 'closed') => { const history = [ From 13e339bedc53c24f9b1e80ebef8e62e0328f589c Mon Sep 17 00:00:00 2001 From: Olivier Bex-Chauvet Date: Wed, 7 Oct 2026 16:18:33 +0200 Subject: [PATCH 3/3] fix(audit-trail): treat only a state strictly before the delete as the earlier record's [PRD-1269] At the delete's own instant the state already reflects every row there, including a replacement create sharing that timestamp. Testing it against the scope as an earlier life withheld the replacement whenever the scope read a column the capture never kept, although the caller had just been shown to read it. Co-Authored-By: Claude Opus 5.5 --- packages/agent/src/audit-trail/README.md | 3 ++- .../agent/src/routes/access/audit-trail.ts | 8 +++--- .../test/routes/access/audit-trail.test.ts | 25 +++++++++++++++---- 3 files changed, 27 insertions(+), 9 deletions(-) diff --git a/packages/agent/src/audit-trail/README.md b/packages/agent/src/audit-trail/README.md index 899f58a78e..1ea194c95b 100644 --- a/packages/agent/src/audit-trail/README.md +++ b/packages/agent/src/audit-trail/README.md @@ -222,7 +222,8 @@ nothing about the rows filed under its id before the id's last confirmed `delete earlier record's. Those rows go through the same withholding as a record gone for good — on the history route (with `search` and `fields` matched against the served values, so the count and the authors cannot leak them either), on the correlation lookups, and on `/state`, whose reconstruction -at or before that `delete` is tested against the scope. A `pending` delete frees nothing, since it may +at an instant before that `delete` is tested against the scope (strictly before: at the delete's own +instant the state already includes a replacement `create` sharing it). A `pending` delete frees nothing, since it may never have landed. That test only runs when the snapshot can actually answer it. The capture keeps the writable columns diff --git a/packages/agent/src/routes/access/audit-trail.ts b/packages/agent/src/routes/access/audit-trail.ts index d90eaa976d..95365f392a 100644 --- a/packages/agent/src/routes/access/audit-trail.ts +++ b/packages/agent/src/routes/access/audit-trail.ts @@ -362,13 +362,15 @@ export default class AuditTrailRoute extends CollectionRoute { const goneNow = after ? after.goneEntirely : goneEntirely; - // A live record whose id was freed by a delete and taken since: a state at or before that - // delete is the earlier record's, which the current one's scope says nothing about. + // A live record whose id was freed by a delete and taken since: a state before that delete is + // the earlier record's, which the current one's scope says nothing about. Strictly before: at + // the delete's own instant the state already reflects every row there, including a replacement + // `create` sharing it. const lastDelete = permissionScope && !goneNow ? await lastDeleteOf(store, this.collection.name, context.params.id) : null; - const ofEarlierLife = Boolean(lastDelete && at <= lastDelete.timestamp); + const ofEarlierLife = Boolean(lastDelete && at < lastDelete.timestamp); // `startTimestamp` is an inclusive lower bound, so an entry timestamped exactly `at` comes // back too — but the record already reflects that entry's change at instant `at`, so it must diff --git a/packages/agent/test/routes/access/audit-trail.test.ts b/packages/agent/test/routes/access/audit-trail.test.ts index 32a4d83f81..7e5c5da4f8 100644 --- a/packages/agent/test/routes/access/audit-trail.test.ts +++ b/packages/agent/test/routes/access/audit-trail.test.ts @@ -2271,7 +2271,11 @@ describe('AuditTrailRoute', () => { }, ]; - const stateAt = async (at: string) => { + const stateAt = async ( + at: string, + entries = history, + scope = new ConditionTreeLeaf('status', 'Equal', 'mine'), + ) => { const { services, dataSource, store, route } = setupBooks(); store.listByRecord.mockImplementation( async ({ @@ -2281,15 +2285,13 @@ describe('AuditTrailRoute', () => { startTimestamp?: string; operations?: string[]; }) => - history.filter( + entries.filter( entry => (!startTimestamp || entry.timestamp >= startTimestamp) && (!operations || operations.includes(entry.operation)), ), ); - (services.authorization.getScope as jest.Mock).mockResolvedValue( - new ConditionTreeLeaf('status', 'Equal', 'mine'), - ); + (services.authorization.getScope as jest.Mock).mockResolvedValue(scope); jest .spyOn(dataSource.getCollection('books'), 'list') .mockResolvedValue([{ id: 2, status: 'mine', name: 'New' }]); @@ -2314,6 +2316,19 @@ describe('AuditTrailRoute', () => { expect(context.response.body).toEqual({ data: { id: 2, status: 'mine', name: 'New' } }); }); + + // A scope on a column the capture never kept cannot be answered from a reconstruction, so + // testing this state at all would withhold the replacement the caller was just shown to read. + test('serves the state at the instant of a delete that a replacement create shares', async () => { + const sameInstant = '2026-06-19T00:00:00.000Z'; + const context = await stateAt( + sameInstant, + [{ ...history[0], timestamp: sameInstant }, history[1]], + new ConditionTreeLeaf('displayName', 'Equal', 'New'), + ); + + expect(context.response.body).toEqual({ data: { id: 2, status: 'mine', name: 'New' } }); + }); }); describe('a genuinely gone record, read by a scoped caller', () => {