From cbf540e83819ecf129501f17987f79cae5f33a20 Mon Sep 17 00:00:00 2001 From: Olivier Bex-Chauvet Date: Wed, 7 Oct 2026 16:07:44 +0200 Subject: [PATCH 1/9] 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 2db24c713c..02888254c1 100644 --- a/packages/agent/src/audit-trail/README.md +++ b/packages/agent/src/audit-trail/README.md @@ -211,6 +211,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 @@ -310,11 +315,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 b80a40d894..2a184cf54e 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'; @@ -161,12 +162,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 @@ -174,21 +183,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 456ee5e331..e275e03a27 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, @@ -246,15 +246,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 7f2643e961..7f975ab2ab 100644 --- a/packages/agent/test/audit-trail/sql-store.test.ts +++ b/packages/agent/test/audit-trail/sql-store.test.ts @@ -901,7 +901,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 @@ -920,7 +920,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(); }); @@ -1102,8 +1135,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 0b3e4b61f7..cdb6fad5f6 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: {} }, + ]); }); }); @@ -2100,7 +2128,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: { @@ -2163,7 +2191,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: { @@ -2219,7 +2247,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: { @@ -2244,7 +2272,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 087b4e513fc944b1340337ac70a75e8e8cc4df16 Mon Sep 17 00:00:00 2001 From: Olivier Bex-Chauvet Date: Wed, 7 Oct 2026 16:12:26 +0200 Subject: [PATCH 2/9] 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 02888254c1..8a2ca0e6d6 100644 --- a/packages/agent/src/audit-trail/README.md +++ b/packages/agent/src/audit-trail/README.md @@ -216,6 +216,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 e275e03a27..93bd29f8d0 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, @@ -114,8 +115,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; } @@ -148,10 +164,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; } @@ -160,9 +193,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, @@ -178,7 +210,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, @@ -208,7 +240,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), ); @@ -328,6 +360,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). @@ -366,7 +406,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 cdb6fad5f6..ddfdf0d08d 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. @@ -2114,6 +2194,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 21f7cb993a6819f9d70d134041fd40206c4f340c Mon Sep 17 00:00:00 2001 From: Olivier Bex-Chauvet Date: Wed, 7 Oct 2026 16:18:33 +0200 Subject: [PATCH 3/9] 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 8a2ca0e6d6..b08e400b8e 100644 --- a/packages/agent/src/audit-trail/README.md +++ b/packages/agent/src/audit-trail/README.md @@ -221,7 +221,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 93bd29f8d0..3e2a741620 100644 --- a/packages/agent/src/routes/access/audit-trail.ts +++ b/packages/agent/src/routes/access/audit-trail.ts @@ -360,13 +360,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 ddfdf0d08d..831fd1f171 100644 --- a/packages/agent/test/routes/access/audit-trail.test.ts +++ b/packages/agent/test/routes/access/audit-trail.test.ts @@ -2212,7 +2212,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 ({ @@ -2222,15 +2226,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' }]); @@ -2255,6 +2257,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', () => { From 08fce754d9949766d5f061d8ce79201ee79089b0 Mon Sep 17 00:00:00 2001 From: Olivier Bex-Chauvet Date: Mon, 12 Oct 2026 12:11:24 +0200 Subject: [PATCH 4/9] fix(audit-trail): count the id's latest create as a life boundary [PRD-1269] A primary-key move or an unconfirmed delete frees an id without a confirmed delete, so the latest confirmed create also marks where the current record's life starts. The gone-or-earlier-life decision now lives once in earlier-life.ts, and the SQL and served search matchers share one column list in value-filters.ts, split out of sql-store.ts. Co-Authored-By: Claude Opus 5.5 --- packages/agent/src/audit-trail/README.md | 18 ++- .../agent/src/audit-trail/earlier-life.ts | 59 +++++-- packages/agent/src/audit-trail/index.ts | 9 +- packages/agent/src/audit-trail/sql-store.ts | 113 +------------ .../agent/src/audit-trail/value-filters.ts | 150 ++++++++++++++++++ .../routes/access/audit-trail-correlation.ts | 33 ++-- .../agent/src/routes/access/audit-trail.ts | 69 ++------ .../test/routes/access/audit-trail.test.ts | 38 ++++- 8 files changed, 277 insertions(+), 212 deletions(-) create mode 100644 packages/agent/src/audit-trail/value-filters.ts diff --git a/packages/agent/src/audit-trail/README.md b/packages/agent/src/audit-trail/README.md index b08e400b8e..1d61263122 100644 --- a/packages/agent/src/audit-trail/README.md +++ b/packages/agent/src/audit-trail/README.md @@ -216,14 +216,16 @@ 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 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. +**An id freed and taken since has two lives.** A live record in the caller's scope says nothing +about the rows filed under its id before it took the id: they are an earlier record's. The boundary +is the id's latest confirmed `delete` (the earlier record's last row) or `create` (the current +record's first), whichever is later; the `create` covers an id freed with no confirmed `delete`, by a +primary-key move or a delete that was never confirmed. 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 an instant before the boundary is tested against the scope +(strictly before: at the boundary's own instant the state already includes a replacement `create` +sharing it). A `pending` row marks no boundary, 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, diff --git a/packages/agent/src/audit-trail/earlier-life.ts b/packages/agent/src/audit-trail/earlier-life.ts index bccdf039d1..afc259597e 100644 --- a/packages/agent/src/audit-trail/earlier-life.ts +++ b/packages/agent/src/audit-trail/earlier-life.ts @@ -1,33 +1,66 @@ 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 +// nothing about the rows filed under that id before it took the id: they are another record's, and // its values go through the same withholding as a record gone for good. +// +// A confirmed `create` marks the boundary too: a primary-key move or an unconfirmed delete frees an +// id without leaving a confirmed `delete`, and the record that takes it next starts with its create. -/** The id's latest confirmed `delete`, or null when the id has never been freed. */ -export async function lastDeleteOf( +/** The id's latest confirmed `delete` or `create`, or null when no record life ever ended or began. */ +export async function lifeBoundaryOf( store: AuditStore, collection: string, recordId: string, ): Promise { - const deletes = await store.listByRecord({ + const boundaries = await store.listByRecord({ collection, recordId, - operations: ['delete'], + operations: ['delete', 'create'], 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; + // A pending row may never have landed, so it marks nothing. Filtered here too, not just in the + // query: a store that ignores `operations` must not turn any row into the boundary. + return ( + boundaries.find( + row => (row.operation === 'delete' || row.operation === 'create') && 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; +/** + * Whether a row belongs to a record that held the id before the boundary, in the trail's + * (timestamp, id) order. A boundary `delete` is the earlier record's last row, a boundary `create` + * the current record's first. + */ +export function belongsToEarlierLife(row: AuditRecord, boundary: AuditRecord | null): boolean { + if (!boundary) return false; + if (row.id === boundary.id) return boundary.operation === 'delete'; return ( - row.timestamp < lastDelete.timestamp || - (row.timestamp === lastDelete.timestamp && row.id <= lastDelete.id) + row.timestamp < boundary.timestamp || + (row.timestamp === boundary.timestamp && row.id < boundary.id) ); } + +/** The rows as served: all withheld for a gone record, only the earlier lives' for a live one. */ +export function withholdEarlierLives( + rows: AuditRecord[], + { + gone, + boundary, + withhold, + }: { + gone: boolean; + boundary: AuditRecord | null; + withhold: (rows: AuditRecord[]) => AuditRecord[]; + }, +): AuditRecord[] { + if (gone) return withhold(rows); + if (!boundary) return rows; + + const withheld = withhold(rows); + + return rows.map((row, index) => (belongsToEarlierLife(row, boundary) ? withheld[index] : row)); +} diff --git a/packages/agent/src/audit-trail/index.ts b/packages/agent/src/audit-trail/index.ts index 12ae8d2a1c..fdba3055a3 100644 --- a/packages/agent/src/audit-trail/index.ts +++ b/packages/agent/src/audit-trail/index.ts @@ -10,14 +10,17 @@ export { createSqlAuditStore, ensureAuditStorage, defineAuditLogModel, - fieldsChangedCondition, - jsonEscaped, - searchCondition, toRow, fromRow, DEFAULT_SCHEMA, DEFAULT_TABLE, } from './sql-store'; export { runAuditMigrations } from './migrations'; +export { + fieldsChangedCondition, + jsonEscaped, + matchesServedValues, + searchCondition, +} from './value-filters'; export { default as revertRecord } from './revert'; export * from './types'; diff --git a/packages/agent/src/audit-trail/sql-store.ts b/packages/agent/src/audit-trail/sql-store.ts index 2a184cf54e..9b454241c4 100644 --- a/packages/agent/src/audit-trail/sql-store.ts +++ b/packages/agent/src/audit-trail/sql-store.ts @@ -12,8 +12,8 @@ import type { Model, ModelStatic } from 'sequelize'; import { DataTypes, Op, Sequelize } from 'sequelize'; -import { REDACTED } from './instrument'; import { runAuditMigrations } from './migrations'; +import { fieldsChangedCondition, searchCondition } from './value-filters'; export const DEFAULT_SCHEMA = 'forest'; export const DEFAULT_TABLE = 'audit_logs'; @@ -93,117 +93,6 @@ export function toRow( }; } -// A bare `$.${field}` path treats a dot in `field` as a nested-member separator instead of a -// literal character (so `profile.name` would look up `profile` → `name` instead of the top-level -// key `"profile.name"` the audit row actually stores under). Quoting the segment makes it literal. -function jsonPath(field: string): string { - return `$."${field.replace(/"/g, '\\"')}"`; -} - -export function fieldsChangedCondition(sequelize: Sequelize, fields: string[]) { - const dialect = sequelize.getDialect(); - - if (dialect === 'postgres') { - const keys = fields.map(field => sequelize.escape(field)).join(','); - - return Sequelize.literal( - `(jsonb_exists_any("previous_values"::jsonb, ARRAY[${keys}]) ` + - `OR jsonb_exists_any("new_values"::jsonb, ARRAY[${keys}]))`, - ); - } - - if (dialect === 'sqlite') { - const tests = fields.flatMap(field => { - const path = sequelize.escape(jsonPath(field)); - - return [ - `json_type(previous_values, ${path}) IS NOT NULL`, - `json_type(new_values, ${path}) IS NOT NULL`, - ]; - }); - - return Sequelize.literal(`(${tests.join(' OR ')})`); - } - - if (dialect === 'mysql' || dialect === 'mariadb') { - const paths = fields.map(field => sequelize.escape(jsonPath(field))).join(','); - - return Sequelize.literal( - `(JSON_CONTAINS_PATH(previous_values, 'one', ${paths}) ` + - `OR JSON_CONTAINS_PATH(new_values, 'one', ${paths}))`, - ); - } - - if (dialect === 'mssql') { - const tests = fields.flatMap(field => { - const path = sequelize.escape(jsonPath(field)); - - return [ - `JSON_PATH_EXISTS(previous_values, ${path}) = 1`, - `JSON_PATH_EXISTS(new_values, ${path}) = 1`, - ]; - }); - - // JSON_PATH_EXISTS requires SQL Server 2022+. - return Sequelize.literal(`(${tests.join(' OR ')})`); - } - - throw new Error(`Audit-trail "fields" filter is not supported on dialect "${dialect}"`); -} - -// JSON columns are already stored as plain text on dialects with no native JSON type (sqlite, -// mssql); only postgres/mysql/mariadb need an explicit cast before a text match can run on them. -function jsonColumnAsText(sequelize: Sequelize, column: string): string { - const dialect = sequelize.getDialect(); - - if (dialect === 'postgres') return `${column}::text`; - if (dialect === 'mysql' || dialect === 'mariadb') return `CAST(${column} AS CHAR)`; - - 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. -// -// 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 - // itself malformed there — the backslash escapes the closing quote instead of terminating the - // 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 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(`(${clauses.join(' OR ')})`); -} - function buildHistoryWhereClause( { collection, diff --git a/packages/agent/src/audit-trail/value-filters.ts b/packages/agent/src/audit-trail/value-filters.ts new file mode 100644 index 0000000000..01f16436b3 --- /dev/null +++ b/packages/agent/src/audit-trail/value-filters.ts @@ -0,0 +1,150 @@ +import type { AuditRecord } from './types'; + +import { Sequelize } from 'sequelize'; + +import { REDACTED } from './instrument'; + +// The `fields` and `search` filters, matched in SQL by the store and on the values as served by a +// route that withholds some of them. Both read the columns below, so they cannot drift apart. +const IDENTITY_COLUMNS = [ + ['actionName', 'action_name'], + ['userFirstName', 'user_first_name'], + ['userLastName', 'user_last_name'], + ['userEmail', 'user_email'], +] as const; + +const VALUE_COLUMNS = [ + ['previousValues', 'previous_values'], + ['newValues', 'new_values'], +] as const; + +// A bare `$.${field}` path treats a dot in `field` as a nested-member separator instead of a +// literal character (so `profile.name` would look up `profile` → `name` instead of the top-level +// key `"profile.name"` the audit row actually stores under). Quoting the segment makes it literal. +function jsonPath(field: string): string { + return `$."${field.replace(/"/g, '\\"')}"`; +} + +export function fieldsChangedCondition(sequelize: Sequelize, fields: string[]) { + const dialect = sequelize.getDialect(); + + if (dialect === 'postgres') { + const keys = fields.map(field => sequelize.escape(field)).join(','); + + return Sequelize.literal( + `(jsonb_exists_any("previous_values"::jsonb, ARRAY[${keys}]) ` + + `OR jsonb_exists_any("new_values"::jsonb, ARRAY[${keys}]))`, + ); + } + + if (dialect === 'sqlite') { + const tests = fields.flatMap(field => { + const path = sequelize.escape(jsonPath(field)); + + return [ + `json_type(previous_values, ${path}) IS NOT NULL`, + `json_type(new_values, ${path}) IS NOT NULL`, + ]; + }); + + return Sequelize.literal(`(${tests.join(' OR ')})`); + } + + if (dialect === 'mysql' || dialect === 'mariadb') { + const paths = fields.map(field => sequelize.escape(jsonPath(field))).join(','); + + return Sequelize.literal( + `(JSON_CONTAINS_PATH(previous_values, 'one', ${paths}) ` + + `OR JSON_CONTAINS_PATH(new_values, 'one', ${paths}))`, + ); + } + + if (dialect === 'mssql') { + const tests = fields.flatMap(field => { + const path = sequelize.escape(jsonPath(field)); + + return [ + `JSON_PATH_EXISTS(previous_values, ${path}) = 1`, + `JSON_PATH_EXISTS(new_values, ${path}) = 1`, + ]; + }); + + // JSON_PATH_EXISTS requires SQL Server 2022+. + return Sequelize.literal(`(${tests.join(' OR ')})`); + } + + throw new Error(`Audit-trail "fields" filter is not supported on dialect "${dialect}"`); +} + +// JSON columns are already stored as plain text on dialects with no native JSON type (sqlite, +// mssql); only postgres/mysql/mariadb need an explicit cast before a text match can run on them. +function jsonColumnAsText(sequelize: Sequelize, column: string): string { + const dialect = sequelize.getDialect(); + + if (dialect === 'postgres') return `${column}::text`; + if (dialect === 'mysql' || dialect === 'mariadb') return `CAST(${column} AS CHAR)`; + + 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. +// +// 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 + // itself malformed there — the backslash escapes the closing quote instead of terminating the + // 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 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 = [ + ...IDENTITY_COLUMNS.map(([, column]) => like(column, term)), + ...VALUE_COLUMNS.map(([, column]) => like(valuesAsText(column), jsonEscaped(term))), + ]; + + return Sequelize.literal(`(${clauses.join(' OR ')})`); +} + +/** `fieldsChangedCondition` and `searchCondition`, run on the values as served. */ +export function matchesServedValues( + entry: AuditRecord, + { fields, search }: { fields?: string[]; search?: string }, +): boolean { + const sides = VALUE_COLUMNS.map(([attribute]) => entry[attribute] ?? {}); + const touchesField = + !fields || + sides.some(values => fields.some(field => Object.prototype.hasOwnProperty.call(values, field))); + + if (!touchesField) return false; + if (!search) return true; + + const term = search.toLowerCase(); + const valueTerm = jsonEscaped(term); + + return ( + IDENTITY_COLUMNS.some(([attribute]) => entry[attribute]?.toLowerCase().includes(term)) || + sides.some(values => + JSON.stringify(values).split(REDACTED).join('').toLowerCase().includes(valueTerm), + ) + ); +} diff --git a/packages/agent/src/routes/access/audit-trail-correlation.ts b/packages/agent/src/routes/access/audit-trail-correlation.ts index d9b3b4d3ea..78ccff0001 100644 --- a/packages/agent/src/routes/access/audit-trail-correlation.ts +++ b/packages/agent/src/routes/access/audit-trail-correlation.ts @@ -7,7 +7,7 @@ import type { Context } from 'koa'; import { ValidationError } from '@forestadmin/datasource-toolkit'; -import { belongsToEarlierLife, lastDeleteOf } from '../../audit-trail/earlier-life'; +import { lifeBoundaryOf, withholdEarlierLives } from '../../audit-trail/earlier-life'; import checkRecordVisibility, { recheckRecordVisibility, } from '../../audit-trail/record-visibility'; @@ -113,25 +113,22 @@ export default class AuditTrailCorrelationRoute extends BaseRoute { if (!target.permissionScope) return entries; - // Live now, but the rows up to the id's last delete may be an earlier record's. - const lastDelete = gone + // Live now, but the rows before the current record took the id may be an earlier record's. + const boundary = 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, + : await lifeBoundaryOf(this.options.auditTrail.store, target.collection, target.recordId); + + return withholdEarlierLives(entries, { + gone, + boundary, + withhold: rows => + withholdOutsidePermissionScope(rows, { + 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 3e2a741620..565de0d072 100644 --- a/packages/agent/src/routes/access/audit-trail.ts +++ b/packages/agent/src/routes/access/audit-trail.ts @@ -16,8 +16,8 @@ import { ValidationError, } from '@forestadmin/datasource-toolkit'; -import { REDACTED, jsonEscaped, revertRecord } from '../../audit-trail'; -import { belongsToEarlierLife, lastDeleteOf } from '../../audit-trail/earlier-life'; +import { matchesServedValues, revertRecord } from '../../audit-trail'; +import { lifeBoundaryOf, withholdEarlierLives } from '../../audit-trail/earlier-life'; import { parseDateBoundary, parseFields, @@ -164,26 +164,19 @@ export default class AuditTrailRoute extends CollectionRoute { const gone = after ? after.goneEntirely : goneEntirely; - // 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 = + // Live now, but the id may have been freed and taken since: the rows before the current record + // took it are an earlier record's, which the current one's scope says nothing about. + const boundary = permissionScope && !gone - ? await lastDeleteOf(store, this.collection.name, context.params.id) + ? await lifeBoundaryOf(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, - ); - }; + const withholdServed = (rows: AuditRecord[]) => + withholdEarlierLives(rows, { gone, boundary, withhold: withholdAll }); // 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) { + if (permissionScope && (gone || boundary) && filtersOnValues) { await serveMatchedValues(withholdServed); return; @@ -194,7 +187,7 @@ export default class AuditTrailRoute extends CollectionRoute { // 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. The same holds for an earlier record that held this id. - const data = permissionScope && (gone || lastDelete) ? withholdServed(rawData) : rawData; + const data = permissionScope && (gone || boundary) ? withholdServed(rawData) : rawData; context.response.body = { data, @@ -241,7 +234,7 @@ export default class AuditTrailRoute extends CollectionRoute { }); const matched = withholdServed(rows).filter(entry => - AuditTrailRoute.matchesServedValues(entry, fields, search), + matchesServedValues(entry, { fields, search }), ); for (const entry of matched) { @@ -265,34 +258,6 @@ export default class AuditTrailRoute extends CollectionRoute { return { page, count, authors }; } - // The SQL store's `fieldsChangedCondition` and `searchCondition`, run on the values as served. - private static matchesServedValues( - entry: AuditRecord, - fields?: string[], - search?: string, - ): boolean { - const sides = [entry.previousValues ?? {}, entry.newValues ?? {}]; - const term = search?.toLowerCase(); - const touchesField = - !fields || - sides.some(values => - fields.some(field => Object.prototype.hasOwnProperty.call(values, field)), - ); - - 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( entries: AuditRecord[], permissionScope: ConditionTree, @@ -360,15 +325,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 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 + // A live record whose id was freed and taken since: a state before that boundary is the + // earlier record's, which the current one's scope says nothing about. Strictly before: at the + // boundary's own instant the state already reflects every row there, including a replacement // `create` sharing it. - const lastDelete = + const boundary = permissionScope && !goneNow - ? await lastDeleteOf(store, this.collection.name, context.params.id) + ? await lifeBoundaryOf(store, this.collection.name, context.params.id) : null; - const ofEarlierLife = Boolean(lastDelete && at < lastDelete.timestamp); + const ofEarlierLife = Boolean(boundary && at < boundary.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 831fd1f171..65818ab7f6 100644 --- a/packages/agent/test/routes/access/audit-trail.test.ts +++ b/packages/agent/test/routes/access/audit-trail.test.ts @@ -1249,7 +1249,7 @@ describe('AuditTrailRoute', () => { // The audit trail lives in its own database — often its own engine — so no single snapshot // spans the visibility check and the audit read. The record can go away in between. describe('the record changes between the visibility check and the audit read', () => { - const raceWith = async (...listAnswers: unknown[][]) => { + const raceWithQuery = async (query: Record, ...listAnswers: unknown[][]) => { const history = [ { operation: 'delete', @@ -1267,7 +1267,7 @@ describe('AuditTrailRoute', () => { const route = new AuditTrailRoute(services, options, dataSource, 'books'); const context = createMockContext({ state: { user: { email: 'john.doe@domain.com' } }, - customProperties: { query: { timezone: 'Europe/Paris' }, params: { id: '2' } }, + customProperties: { query: { timezone: 'Europe/Paris', ...query }, params: { id: '2' } }, }); await route.handleHistory(context); @@ -1275,6 +1275,8 @@ describe('AuditTrailRoute', () => { return { context, list, services }; }; + const raceWith = (...listAnswers: unknown[][]) => raceWithQuery({}, ...listAnswers); + test('withholds the values when the record was deleted in between', async () => { // present and in scope at the check, gone by the time the rows are in hand const { context } = await raceWith([{ id: 2 }], [], []); @@ -1308,6 +1310,12 @@ describe('AuditTrailRoute', () => { expect(context.throw).toHaveBeenCalledWith(404, 'Record does not exists'); }); + test('refuses a search when an id gone at the first check now belongs to a record the caller cannot read', async () => { + const { context } = await raceWithQuery({ search: 'secret' }, [], [], [], [{ 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 }]); @@ -1384,18 +1392,36 @@ describe('AuditTrailRoute', () => { 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 () => { + test('withholds the rows before the current create when the delete was never confirmed', async () => { const pending = { ...freed, status: 'pending' }; const body = await readHistory([current, pending, earlier]); - expect(body.data).toEqual([current, pending, earlier]); + expect(body.data).toEqual([ + current, + { ...pending, previousValues: {} }, + { ...earlier, previousValues: {}, newValues: {} }, + ]); }); - test('serves the whole history of an id that was never freed', async () => { + test('withholds the rows of a record whose primary key moved off the id', async () => { const body = await readHistory([current, earlier]); - expect(body.data).toEqual([current, earlier]); + expect(body.data).toEqual([current, { ...earlier, previousValues: {}, newValues: {} }]); + }); + + test('serves the whole history of an id that was never freed', async () => { + const later = row({ + id: 4, + timestamp: '2026-01-04T00:00:00.000Z', + operation: 'update', + previousValues: { ownerId: 1, title: 'Mine' }, + newValues: { ownerId: 1, title: 'Still mine' }, + }); + + const body = await readHistory([later, current]); + + expect(body.data).toEqual([later, current]); }); }); From 8788ef0fbd9902aed4bf04c061799dc74d4541b3 Mon Sep 17 00:00:00 2001 From: Olivier Bex-Chauvet Date: Wed, 14 Oct 2026 17:23:17 +0200 Subject: [PATCH 5/9] fix(audit-trail): keep a lone create from forcing the served-value scan [PRD-1269] A record created through Forest has its own create as the latest boundary, so every scoped search or fields request on it scanned the whole history to match on served values. A create with nothing filed before it began the id's only life, and no longer counts as a boundary. Same fix as agent-ruby #410. Co-Authored-By: Claude Opus 5.5 --- .../agent/src/audit-trail/earlier-life.ts | 45 ++++++++++++------- .../test/routes/access/audit-trail.test.ts | 38 ++++++++++++++++ 2 files changed, 66 insertions(+), 17 deletions(-) diff --git a/packages/agent/src/audit-trail/earlier-life.ts b/packages/agent/src/audit-trail/earlier-life.ts index afc259597e..b414233502 100644 --- a/packages/agent/src/audit-trail/earlier-life.ts +++ b/packages/agent/src/audit-trail/earlier-life.ts @@ -7,7 +7,25 @@ import type { AuditRecord, AuditStore } from './types'; // A confirmed `create` marks the boundary too: a primary-key move or an unconfirmed delete frees an // id without leaving a confirmed `delete`, and the record that takes it next starts with its create. -/** The id's latest confirmed `delete` or `create`, or null when no record life ever ended or began. */ +/** + * Whether a row belongs to a record that held the id before the boundary, in the trail's + * (timestamp, id) order. A boundary `delete` is the earlier record's last row, a boundary `create` + * the current record's first. + */ +export function belongsToEarlierLife(row: AuditRecord, boundary: AuditRecord | null): boolean { + if (!boundary) return false; + if (row.id === boundary.id) return boundary.operation === 'delete'; + + return ( + row.timestamp < boundary.timestamp || + (row.timestamp === boundary.timestamp && row.id < boundary.id) + ); +} + +/** + * The id's latest confirmed `delete` or `create`, or null when the id has had one life only: no + * boundary at all, or a `create` with nothing filed before it. + */ export async function lifeBoundaryOf( store: AuditStore, collection: string, @@ -22,26 +40,19 @@ export async function lifeBoundaryOf( // A pending row may never have landed, so it marks nothing. Filtered here too, not just in the // query: a store that ignores `operations` must not turn any row into the boundary. - return ( + const boundary = boundaries.find( row => (row.operation === 'delete' || row.operation === 'create') && row.status !== 'pending', - ) ?? null - ); -} + ) ?? null; -/** - * Whether a row belongs to a record that held the id before the boundary, in the trail's - * (timestamp, id) order. A boundary `delete` is the earlier record's last row, a boundary `create` - * the current record's first. - */ -export function belongsToEarlierLife(row: AuditRecord, boundary: AuditRecord | null): boolean { - if (!boundary) return false; - if (row.id === boundary.id) return boundary.operation === 'delete'; + if (boundary?.operation !== 'create') return boundary; - return ( - row.timestamp < boundary.timestamp || - (row.timestamp === boundary.timestamp && row.id < boundary.id) - ); + // A create with nothing filed before it began the id's only life. A record created through Forest + // has its own create here, and counting it would send every scoped search on it down the + // served-value scan. Tested on whatever comes back, should a store ignore the order or the limit. + const oldest = await store.listByRecord({ collection, recordId, order: 'asc', limit: 1 }); + + return oldest.some(row => belongsToEarlierLife(row, boundary)) ? boundary : null; } /** The rows as served: all withheld for a gone record, only the earlier lives' for a live one. */ diff --git a/packages/agent/test/routes/access/audit-trail.test.ts b/packages/agent/test/routes/access/audit-trail.test.ts index 65818ab7f6..880e840e2d 100644 --- a/packages/agent/test/routes/access/audit-trail.test.ts +++ b/packages/agent/test/routes/access/audit-trail.test.ts @@ -1410,6 +1410,44 @@ describe('AuditTrailRoute', () => { expect(body.data).toEqual([current, { ...earlier, previousValues: {}, newValues: {} }]); }); + // The record's own create is the latest boundary of any record created through Forest. With + // nothing before it there is no earlier life, so a search stays a single matched query rather + // than the served-value scan. + test('keeps a search on a record created through Forest in the store query', async () => { + const later = row({ + id: 4, + timestamp: '2026-01-04T00:00:00.000Z', + operation: 'update', + previousValues: { ownerId: 1, title: 'Mine' }, + newValues: { ownerId: 1, title: 'Still mine' }, + }); + const { services, dataSource, options, store } = setup([later, current]); + store.listByRecord.mockImplementation(async ({ order, limit }: Record) => { + const rows = order === 'asc' ? [current, later] : [later, current]; + + return limit ? rows.slice(0, limit as number) : rows; + }); + (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: { search: 'mine' }, params: { id: '2' } }, + }); + + await route.handleHistory(context); + + const scans = (store.listByRecord as jest.Mock).mock.calls.filter( + ([query]) => query.limit === 500, + ); + expect(scans).toEqual([]); + expect(store.countByRecord).toHaveBeenCalledWith( + expect.objectContaining({ search: 'mine' }), + ); + }); + test('serves the whole history of an id that was never freed', async () => { const later = row({ id: 4, From 838bd75ca1296359baebac1c995e445b263e8c8d Mon Sep 17 00:00:00 2001 From: Olivier Bex-Chauvet Date: Thu, 15 Oct 2026 13:31:58 +0200 Subject: [PATCH 6/9] feat(agent): make sequelize and umzug optional peer dependencies of the audit trail [PRD-1269] The audit trail store is loaded only when `auditTrail` is configured, so an agent without it no longer installs nor loads sequelize and umzug at boot. An agent that enables it without them gets an error naming the packages to install: `npm install sequelize umzug`. Co-Authored-By: Claude Opus 5.5 --- packages/agent/package.json | 16 +++++++--- packages/agent/src/audit-trail/README.md | 7 +++-- packages/agent/src/audit-trail/index.ts | 10 ------- .../agent/src/audit-trail/value-filters.ts | 13 ++++---- packages/agent/src/utils/options-validator.ts | 19 ++++++++++-- .../agent/test/audit-trail/migrations.test.ts | 2 +- .../agent/test/audit-trail/sql-store.test.ts | 9 ++---- .../test/utils/optional-sequelize.test.ts | 30 +++++++++++++++++++ 8 files changed, 72 insertions(+), 34 deletions(-) create mode 100644 packages/agent/test/utils/optional-sequelize.test.ts diff --git a/packages/agent/package.json b/packages/agent/package.json index 33387d53c8..59af631ce6 100644 --- a/packages/agent/package.json +++ b/packages/agent/package.json @@ -31,9 +31,7 @@ "light-my-request": "^5.14.0", "luxon": "^3.2.1", "object-hash": "^3.0.0", - "sequelize": "^6.37.8", "superagent": "^10.3.0", - "umzug": "^3.8.2", "uuid": "11.1.1" }, "files": [ @@ -70,13 +68,17 @@ "openid-client": "^5.7.1", "reflect-metadata": "^0.1.13", "rxjs": "^7.8.0", + "sequelize": "^6.37.8", "sqlite3": "^5.1.7", - "supertest": "^7.1.3" + "supertest": "^7.1.3", + "umzug": "^3.8.2" }, "peerDependencies": { "@fastify/express": "^1.1.0 || ^2.0.0 || ^3.0.0 || ^4.0.0", "@forestadmin/agent-bff": "1.42.2", - "@forestadmin/workflow-executor": "1.34.0" + "@forestadmin/workflow-executor": "1.34.0", + "sequelize": "^6.37.8", + "umzug": "^3.8.2" }, "peerDependenciesMeta": { "@fastify/express": { @@ -87,6 +89,12 @@ }, "@forestadmin/workflow-executor": { "optional": true + }, + "sequelize": { + "optional": true + }, + "umzug": { + "optional": true } } } diff --git a/packages/agent/src/audit-trail/README.md b/packages/agent/src/audit-trail/README.md index 1d61263122..df08d08dae 100644 --- a/packages/agent/src/audit-trail/README.md +++ b/packages/agent/src/audit-trail/README.md @@ -16,8 +16,11 @@ The audit trail has two decoupled parts, both handled by the agent: Pass an `auditTrail` option to `createAgent`. The agent constructs the SQL store, registers the CRUD hooks on every collection, and mounts the record-history routes only when this option is set. -Sequelize does not bundle database drivers, so install the one matching `connectionString`'s -dialect — e.g. `pg` and `pg-hstore` for PostgreSQL — even in an agent whose main datasource isn't SQL. +The SQL store runs on `sequelize` and `umzug`, which the agent lists as optional peer dependencies: +install them alongside the agent (`npm install sequelize umzug`), or it refuses to start with the +audit trail on. An agent without the audit trail never loads them. Sequelize does not bundle +database drivers either, so also install the one matching `connectionString`'s dialect — e.g. `pg` +and `pg-hstore` for PostgreSQL — even in an agent whose main datasource isn't SQL. ```ts import { createAgent } from '@forestadmin/agent'; diff --git a/packages/agent/src/audit-trail/index.ts b/packages/agent/src/audit-trail/index.ts index fdba3055a3..0d3f1d18af 100644 --- a/packages/agent/src/audit-trail/index.ts +++ b/packages/agent/src/audit-trail/index.ts @@ -6,16 +6,6 @@ export { } from './instrument'; export type { Recorder } from './instrument'; export { captureActionPending, captureActionConfirm } from './action-capture'; -export { - createSqlAuditStore, - ensureAuditStorage, - defineAuditLogModel, - toRow, - fromRow, - DEFAULT_SCHEMA, - DEFAULT_TABLE, -} from './sql-store'; -export { runAuditMigrations } from './migrations'; export { fieldsChangedCondition, jsonEscaped, diff --git a/packages/agent/src/audit-trail/value-filters.ts b/packages/agent/src/audit-trail/value-filters.ts index 01f16436b3..d4721feb37 100644 --- a/packages/agent/src/audit-trail/value-filters.ts +++ b/packages/agent/src/audit-trail/value-filters.ts @@ -1,6 +1,5 @@ import type { AuditRecord } from './types'; - -import { Sequelize } from 'sequelize'; +import type { Sequelize } from 'sequelize'; import { REDACTED } from './instrument'; @@ -31,7 +30,7 @@ export function fieldsChangedCondition(sequelize: Sequelize, fields: string[]) { if (dialect === 'postgres') { const keys = fields.map(field => sequelize.escape(field)).join(','); - return Sequelize.literal( + return sequelize.literal( `(jsonb_exists_any("previous_values"::jsonb, ARRAY[${keys}]) ` + `OR jsonb_exists_any("new_values"::jsonb, ARRAY[${keys}]))`, ); @@ -47,13 +46,13 @@ export function fieldsChangedCondition(sequelize: Sequelize, fields: string[]) { ]; }); - return Sequelize.literal(`(${tests.join(' OR ')})`); + return sequelize.literal(`(${tests.join(' OR ')})`); } if (dialect === 'mysql' || dialect === 'mariadb') { const paths = fields.map(field => sequelize.escape(jsonPath(field))).join(','); - return Sequelize.literal( + return sequelize.literal( `(JSON_CONTAINS_PATH(previous_values, 'one', ${paths}) ` + `OR JSON_CONTAINS_PATH(new_values, 'one', ${paths}))`, ); @@ -70,7 +69,7 @@ export function fieldsChangedCondition(sequelize: Sequelize, fields: string[]) { }); // JSON_PATH_EXISTS requires SQL Server 2022+. - return Sequelize.literal(`(${tests.join(' OR ')})`); + return sequelize.literal(`(${tests.join(' OR ')})`); } throw new Error(`Audit-trail "fields" filter is not supported on dialect "${dialect}"`); @@ -122,7 +121,7 @@ export function searchCondition(sequelize: Sequelize, term: string) { ...VALUE_COLUMNS.map(([, column]) => like(valuesAsText(column), jsonEscaped(term))), ]; - return Sequelize.literal(`(${clauses.join(' OR ')})`); + return sequelize.literal(`(${clauses.join(' OR ')})`); } /** `fieldsChangedCondition` and `searchCondition`, run on the values as served. */ diff --git a/packages/agent/src/utils/options-validator.ts b/packages/agent/src/utils/options-validator.ts index db3c5ccb1f..327e528029 100644 --- a/packages/agent/src/utils/options-validator.ts +++ b/packages/agent/src/utils/options-validator.ts @@ -1,11 +1,10 @@ +import type * as SqlAuditStore from '../audit-trail/sql-store'; import type { AgentOptions, AgentOptionsWithDefaults, AuditTrailRuntime } from '../types'; import createForestAdminClient from '@forestadmin/forestadmin-client'; import { existsSync } from 'fs'; import path from 'path'; -import { createSqlAuditStore } from '../audit-trail'; - const DEFAULT_MINIMUM_CACHE_DURATION = 60; // One year cache duration when using events const DEFAULT_CACHE_DURATION_WITH_EVENTS = 31560000; @@ -104,7 +103,7 @@ export default class OptionsValidator { const auditTrail: AuditTrailRuntime | null = copyOptions.auditTrail ? (() => { - const { store, close } = createSqlAuditStore({ + const { store, close } = OptionsValidator.loadSqlAuditStore().createSqlAuditStore({ connectionString: copyOptions.auditTrail.connectionString, schema: copyOptions.auditTrail.schema, tableName: copyOptions.auditTrail.tableName, @@ -126,6 +125,20 @@ export default class OptionsValidator { } as AgentOptionsWithDefaults; } + // `sequelize` and `umzug` are optional peers: only an agent with the audit trail on loads them. + private static loadSqlAuditStore(): typeof SqlAuditStore { + try { + return require('../audit-trail/sql-store'); // eslint-disable-line @typescript-eslint/no-var-requires,global-require + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'MODULE_NOT_FOUND') throw error; + + throw new Error( + 'The audit trail requires the `sequelize` and `umzug` packages. ' + + 'Install them with `npm install sequelize umzug`.', + ); + } + } + static validate(options: AgentOptions): AgentOptionsWithDefaults { OptionsValidator.checkForestServerOptions(options); OptionsValidator.checkAuthOptions(options); diff --git a/packages/agent/test/audit-trail/migrations.test.ts b/packages/agent/test/audit-trail/migrations.test.ts index 10051ec769..ca2852aabb 100644 --- a/packages/agent/test/audit-trail/migrations.test.ts +++ b/packages/agent/test/audit-trail/migrations.test.ts @@ -2,13 +2,13 @@ import type { AuditStatus, PendingAuditRecord } from '../../src/audit-trail'; import { Sequelize } from 'sequelize'; -import { ensureAuditStorage, toRow } from '../../src/audit-trail'; import { buildUmzug, ensureSchema, isSchemaAlreadyExistsError, runAuditMigrations, } from '../../src/audit-trail/migrations'; +import { ensureAuditStorage, toRow } from '../../src/audit-trail/sql-store'; type StoredRecord = PendingAuditRecord & { status: AuditStatus }; diff --git a/packages/agent/test/audit-trail/sql-store.test.ts b/packages/agent/test/audit-trail/sql-store.test.ts index 7f975ab2ab..df2986b7c5 100644 --- a/packages/agent/test/audit-trail/sql-store.test.ts +++ b/packages/agent/test/audit-trail/sql-store.test.ts @@ -3,14 +3,9 @@ import type * as MigrationsModule from '../../src/audit-trail/migrations'; import { Sequelize } from 'sequelize'; -import { - createSqlAuditStore, - ensureAuditStorage, - fieldsChangedCondition, - searchCondition, - toRow, -} from '../../src/audit-trail'; +import { fieldsChangedCondition, searchCondition } from '../../src/audit-trail'; import { runAuditMigrations } from '../../src/audit-trail/migrations'; +import { createSqlAuditStore, ensureAuditStorage, toRow } from '../../src/audit-trail/sql-store'; jest.mock('../../src/audit-trail/migrations', () => ({ runAuditMigrations: jest.fn().mockResolvedValue(undefined), diff --git a/packages/agent/test/utils/optional-sequelize.test.ts b/packages/agent/test/utils/optional-sequelize.test.ts new file mode 100644 index 0000000000..403475e3e4 --- /dev/null +++ b/packages/agent/test/utils/optional-sequelize.test.ts @@ -0,0 +1,30 @@ +import OptionsValidator from '../../src/utils/options-validator'; + +jest.mock('sequelize', () => { + throw Object.assign(new Error("Cannot find module 'sequelize'"), { code: 'MODULE_NOT_FOUND' }); +}); + +describe('without the optional sequelize peer installed', () => { + const mandatoryOptions = { + authSecret: '2536a2f786fc98a1ee62f9e9f405ff2521181cd01e15adcc', + envSecret: '61a31971206f285c3e8eb8f3ee420175eb004bfa9fa24846dde6d5dd438e3991', + isProduction: false, + }; + + test('loads the agent when the audit trail is off', () => { + // eslint-disable-next-line global-require, @typescript-eslint/no-var-requires + expect(require('../../src').createAgent).toEqual(expect.any(Function)); + }); + + test('names the packages to install when the audit trail is on', () => { + expect(() => + OptionsValidator.withDefaults({ + ...mandatoryOptions, + auditTrail: { connectionString: 'sqlite::memory:' }, + }), + ).toThrow( + 'The audit trail requires the `sequelize` and `umzug` packages. ' + + 'Install them with `npm install sequelize umzug`.', + ); + }); +}); From 3b256b542819ebc1cee72be508ea5e0e41468293 Mon Sep 17 00:00:00 2001 From: Olivier Bex-Chauvet Date: Fri, 16 Oct 2026 14:09:49 +0200 Subject: [PATCH 7/9] feat(agent): ship the audit trail's dependencies with the agent [PRD-1269] Reverts the optional peer dependencies of 144864d65: sequelize and umzug are regular dependencies again, and the PostgreSQL driver (pg, pg-hstore) joins them, so the auditTrail option alone turns the audit trail on, whatever the main datasource. Co-Authored-By: Claude Opus 5.5 --- packages/agent/package.json | 18 ++++------- packages/agent/src/audit-trail/README.md | 7 ++--- packages/agent/src/audit-trail/index.ts | 10 +++++++ .../agent/src/audit-trail/value-filters.ts | 13 ++++---- packages/agent/src/utils/options-validator.ts | 19 ++---------- .../agent/test/audit-trail/migrations.test.ts | 2 +- .../agent/test/audit-trail/sql-store.test.ts | 9 ++++-- .../test/utils/optional-sequelize.test.ts | 30 ------------------- yarn.lock | 12 ++++++++ 9 files changed, 48 insertions(+), 72 deletions(-) delete mode 100644 packages/agent/test/utils/optional-sequelize.test.ts diff --git a/packages/agent/package.json b/packages/agent/package.json index 59af631ce6..b4e7af49b3 100644 --- a/packages/agent/package.json +++ b/packages/agent/package.json @@ -31,7 +31,11 @@ "light-my-request": "^5.14.0", "luxon": "^3.2.1", "object-hash": "^3.0.0", + "pg": "^8.8.0", + "pg-hstore": "^2.3.4", + "sequelize": "^6.37.8", "superagent": "^10.3.0", + "umzug": "^3.8.2", "uuid": "11.1.1" }, "files": [ @@ -68,17 +72,13 @@ "openid-client": "^5.7.1", "reflect-metadata": "^0.1.13", "rxjs": "^7.8.0", - "sequelize": "^6.37.8", "sqlite3": "^5.1.7", - "supertest": "^7.1.3", - "umzug": "^3.8.2" + "supertest": "^7.1.3" }, "peerDependencies": { "@fastify/express": "^1.1.0 || ^2.0.0 || ^3.0.0 || ^4.0.0", "@forestadmin/agent-bff": "1.42.2", - "@forestadmin/workflow-executor": "1.34.0", - "sequelize": "^6.37.8", - "umzug": "^3.8.2" + "@forestadmin/workflow-executor": "1.34.0" }, "peerDependenciesMeta": { "@fastify/express": { @@ -89,12 +89,6 @@ }, "@forestadmin/workflow-executor": { "optional": true - }, - "sequelize": { - "optional": true - }, - "umzug": { - "optional": true } } } diff --git a/packages/agent/src/audit-trail/README.md b/packages/agent/src/audit-trail/README.md index df08d08dae..85bb3e4b07 100644 --- a/packages/agent/src/audit-trail/README.md +++ b/packages/agent/src/audit-trail/README.md @@ -16,11 +16,8 @@ The audit trail has two decoupled parts, both handled by the agent: Pass an `auditTrail` option to `createAgent`. The agent constructs the SQL store, registers the CRUD hooks on every collection, and mounts the record-history routes only when this option is set. -The SQL store runs on `sequelize` and `umzug`, which the agent lists as optional peer dependencies: -install them alongside the agent (`npm install sequelize umzug`), or it refuses to start with the -audit trail on. An agent without the audit trail never loads them. Sequelize does not bundle -database drivers either, so also install the one matching `connectionString`'s dialect — e.g. `pg` -and `pg-hstore` for PostgreSQL — even in an agent whose main datasource isn't SQL. +The agent ships `sequelize`, `umzug` and the PostgreSQL driver (`pg`, `pg-hstore`), so the option is all +it takes, even in an agent whose main datasource isn't SQL. ```ts import { createAgent } from '@forestadmin/agent'; diff --git a/packages/agent/src/audit-trail/index.ts b/packages/agent/src/audit-trail/index.ts index 0d3f1d18af..fdba3055a3 100644 --- a/packages/agent/src/audit-trail/index.ts +++ b/packages/agent/src/audit-trail/index.ts @@ -6,6 +6,16 @@ export { } from './instrument'; export type { Recorder } from './instrument'; export { captureActionPending, captureActionConfirm } from './action-capture'; +export { + createSqlAuditStore, + ensureAuditStorage, + defineAuditLogModel, + toRow, + fromRow, + DEFAULT_SCHEMA, + DEFAULT_TABLE, +} from './sql-store'; +export { runAuditMigrations } from './migrations'; export { fieldsChangedCondition, jsonEscaped, diff --git a/packages/agent/src/audit-trail/value-filters.ts b/packages/agent/src/audit-trail/value-filters.ts index d4721feb37..01f16436b3 100644 --- a/packages/agent/src/audit-trail/value-filters.ts +++ b/packages/agent/src/audit-trail/value-filters.ts @@ -1,5 +1,6 @@ import type { AuditRecord } from './types'; -import type { Sequelize } from 'sequelize'; + +import { Sequelize } from 'sequelize'; import { REDACTED } from './instrument'; @@ -30,7 +31,7 @@ export function fieldsChangedCondition(sequelize: Sequelize, fields: string[]) { if (dialect === 'postgres') { const keys = fields.map(field => sequelize.escape(field)).join(','); - return sequelize.literal( + return Sequelize.literal( `(jsonb_exists_any("previous_values"::jsonb, ARRAY[${keys}]) ` + `OR jsonb_exists_any("new_values"::jsonb, ARRAY[${keys}]))`, ); @@ -46,13 +47,13 @@ export function fieldsChangedCondition(sequelize: Sequelize, fields: string[]) { ]; }); - return sequelize.literal(`(${tests.join(' OR ')})`); + return Sequelize.literal(`(${tests.join(' OR ')})`); } if (dialect === 'mysql' || dialect === 'mariadb') { const paths = fields.map(field => sequelize.escape(jsonPath(field))).join(','); - return sequelize.literal( + return Sequelize.literal( `(JSON_CONTAINS_PATH(previous_values, 'one', ${paths}) ` + `OR JSON_CONTAINS_PATH(new_values, 'one', ${paths}))`, ); @@ -69,7 +70,7 @@ export function fieldsChangedCondition(sequelize: Sequelize, fields: string[]) { }); // JSON_PATH_EXISTS requires SQL Server 2022+. - return sequelize.literal(`(${tests.join(' OR ')})`); + return Sequelize.literal(`(${tests.join(' OR ')})`); } throw new Error(`Audit-trail "fields" filter is not supported on dialect "${dialect}"`); @@ -121,7 +122,7 @@ export function searchCondition(sequelize: Sequelize, term: string) { ...VALUE_COLUMNS.map(([, column]) => like(valuesAsText(column), jsonEscaped(term))), ]; - return sequelize.literal(`(${clauses.join(' OR ')})`); + return Sequelize.literal(`(${clauses.join(' OR ')})`); } /** `fieldsChangedCondition` and `searchCondition`, run on the values as served. */ diff --git a/packages/agent/src/utils/options-validator.ts b/packages/agent/src/utils/options-validator.ts index 327e528029..db3c5ccb1f 100644 --- a/packages/agent/src/utils/options-validator.ts +++ b/packages/agent/src/utils/options-validator.ts @@ -1,10 +1,11 @@ -import type * as SqlAuditStore from '../audit-trail/sql-store'; import type { AgentOptions, AgentOptionsWithDefaults, AuditTrailRuntime } from '../types'; import createForestAdminClient from '@forestadmin/forestadmin-client'; import { existsSync } from 'fs'; import path from 'path'; +import { createSqlAuditStore } from '../audit-trail'; + const DEFAULT_MINIMUM_CACHE_DURATION = 60; // One year cache duration when using events const DEFAULT_CACHE_DURATION_WITH_EVENTS = 31560000; @@ -103,7 +104,7 @@ export default class OptionsValidator { const auditTrail: AuditTrailRuntime | null = copyOptions.auditTrail ? (() => { - const { store, close } = OptionsValidator.loadSqlAuditStore().createSqlAuditStore({ + const { store, close } = createSqlAuditStore({ connectionString: copyOptions.auditTrail.connectionString, schema: copyOptions.auditTrail.schema, tableName: copyOptions.auditTrail.tableName, @@ -125,20 +126,6 @@ export default class OptionsValidator { } as AgentOptionsWithDefaults; } - // `sequelize` and `umzug` are optional peers: only an agent with the audit trail on loads them. - private static loadSqlAuditStore(): typeof SqlAuditStore { - try { - return require('../audit-trail/sql-store'); // eslint-disable-line @typescript-eslint/no-var-requires,global-require - } catch (error) { - if ((error as NodeJS.ErrnoException).code !== 'MODULE_NOT_FOUND') throw error; - - throw new Error( - 'The audit trail requires the `sequelize` and `umzug` packages. ' + - 'Install them with `npm install sequelize umzug`.', - ); - } - } - static validate(options: AgentOptions): AgentOptionsWithDefaults { OptionsValidator.checkForestServerOptions(options); OptionsValidator.checkAuthOptions(options); diff --git a/packages/agent/test/audit-trail/migrations.test.ts b/packages/agent/test/audit-trail/migrations.test.ts index ca2852aabb..10051ec769 100644 --- a/packages/agent/test/audit-trail/migrations.test.ts +++ b/packages/agent/test/audit-trail/migrations.test.ts @@ -2,13 +2,13 @@ import type { AuditStatus, PendingAuditRecord } from '../../src/audit-trail'; import { Sequelize } from 'sequelize'; +import { ensureAuditStorage, toRow } from '../../src/audit-trail'; import { buildUmzug, ensureSchema, isSchemaAlreadyExistsError, runAuditMigrations, } from '../../src/audit-trail/migrations'; -import { ensureAuditStorage, toRow } from '../../src/audit-trail/sql-store'; type StoredRecord = PendingAuditRecord & { status: AuditStatus }; diff --git a/packages/agent/test/audit-trail/sql-store.test.ts b/packages/agent/test/audit-trail/sql-store.test.ts index df2986b7c5..7f975ab2ab 100644 --- a/packages/agent/test/audit-trail/sql-store.test.ts +++ b/packages/agent/test/audit-trail/sql-store.test.ts @@ -3,9 +3,14 @@ import type * as MigrationsModule from '../../src/audit-trail/migrations'; import { Sequelize } from 'sequelize'; -import { fieldsChangedCondition, searchCondition } from '../../src/audit-trail'; +import { + createSqlAuditStore, + ensureAuditStorage, + fieldsChangedCondition, + searchCondition, + toRow, +} from '../../src/audit-trail'; import { runAuditMigrations } from '../../src/audit-trail/migrations'; -import { createSqlAuditStore, ensureAuditStorage, toRow } from '../../src/audit-trail/sql-store'; jest.mock('../../src/audit-trail/migrations', () => ({ runAuditMigrations: jest.fn().mockResolvedValue(undefined), diff --git a/packages/agent/test/utils/optional-sequelize.test.ts b/packages/agent/test/utils/optional-sequelize.test.ts deleted file mode 100644 index 403475e3e4..0000000000 --- a/packages/agent/test/utils/optional-sequelize.test.ts +++ /dev/null @@ -1,30 +0,0 @@ -import OptionsValidator from '../../src/utils/options-validator'; - -jest.mock('sequelize', () => { - throw Object.assign(new Error("Cannot find module 'sequelize'"), { code: 'MODULE_NOT_FOUND' }); -}); - -describe('without the optional sequelize peer installed', () => { - const mandatoryOptions = { - authSecret: '2536a2f786fc98a1ee62f9e9f405ff2521181cd01e15adcc', - envSecret: '61a31971206f285c3e8eb8f3ee420175eb004bfa9fa24846dde6d5dd438e3991', - isProduction: false, - }; - - test('loads the agent when the audit trail is off', () => { - // eslint-disable-next-line global-require, @typescript-eslint/no-var-requires - expect(require('../../src').createAgent).toEqual(expect.any(Function)); - }); - - test('names the packages to install when the audit trail is on', () => { - expect(() => - OptionsValidator.withDefaults({ - ...mandatoryOptions, - auditTrail: { connectionString: 'sqlite::memory:' }, - }), - ).toThrow( - 'The audit trail requires the `sequelize` and `umzug` packages. ' + - 'Install them with `npm install sequelize umzug`.', - ); - }); -}); diff --git a/yarn.lock b/yarn.lock index e1872393e1..bd89f9f346 100644 --- a/yarn.lock +++ b/yarn.lock @@ -15112,6 +15112,13 @@ pg-connection-string@^2.7.0: resolved "https://registry.yarnpkg.com/pg-connection-string/-/pg-connection-string-2.7.0.tgz#f1d3489e427c62ece022dba98d5262efcb168b37" integrity sha512-PI2W9mv53rXJQEOb8xNR8lH7Hr+EKa6oJa38zsK0S/ky2er16ios1wLKhZyxzD7jUReiWokc9WK5nxSnC7W1TA== +pg-hstore@^2.3.4: + version "2.3.4" + resolved "https://registry.yarnpkg.com/pg-hstore/-/pg-hstore-2.3.4.tgz#4425e3e2a3e15d2a334c35581186c27cf2e9b8dd" + integrity sha512-N3SGs/Rf+xA1M2/n0JBiXFDVMzdekwLZLAO0g7mpDY9ouX+fDI7jS6kTq3JujmYbtNSJ53TJ0q4G98KVZSM4EA== + dependencies: + underscore "^1.13.1" + pg-int8@1.0.1: version "1.0.1" resolved "https://registry.yarnpkg.com/pg-int8/-/pg-int8-1.0.1.tgz#943bd463bf5b71b4170115f80f8efc9a0c0eb78c" @@ -18366,6 +18373,11 @@ undefsafe@^2.0.5: resolved "https://registry.yarnpkg.com/undefsafe/-/undefsafe-2.0.5.tgz#38733b9327bdcd226db889fb723a6efd162e6e2c" integrity sha512-WxONCrssBM8TSPRqN5EmsjVrsv4A8X12J4ArBiiayv3DyyG3ZlIg6yysuuSYdZsVz3TKcTg2fd//Ujd4CHV1iA== +underscore@^1.13.1: + version "1.13.8" + resolved "https://registry.yarnpkg.com/underscore/-/underscore-1.13.8.tgz#a93a21186c049dbf0e847496dba72b7bd8c1e92b" + integrity sha512-DXtD3ZtEQzc7M8m4cXotyHR+FAS18C64asBYY5vqZexfYryNNnDc02W4hKg3rdQuqOYas1jkseX0+nZXjTXnvQ== + undici-types@~5.26.4: version "5.26.5" resolved "https://registry.yarnpkg.com/undici-types/-/undici-types-5.26.5.tgz#bcd539893d00b56e964fd2657a4866b221a65617" From df8fb170c33ee3461f391597c2770c3b2e864a4e Mon Sep 17 00:00:00 2001 From: Olivier Bex-Chauvet Date: Mon, 19 Oct 2026 14:21:47 +0200 Subject: [PATCH 8/9] docs(audit-trail): say a scoped collection has no audit trail on the project timeline [PRD-1323] Co-Authored-By: Claude Opus 5.5 --- packages/agent/src/audit-trail/README.md | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/packages/agent/src/audit-trail/README.md b/packages/agent/src/audit-trail/README.md index 85bb3e4b07..d389c1434b 100644 --- a/packages/agent/src/audit-trail/README.md +++ b/packages/agent/src/audit-trail/README.md @@ -368,8 +368,9 @@ The cursor follows the direction — `before`, an inclusive upper bound, newest inclusive lower bound, oldest first — and `meta.cursor` hands back the one the walk uses. The bound for the other direction is ignored. -**Detail values follow the same rule as the per-record routes:** a caller who can read a collection -gets its rows with `previousValues` and `newValues`, whatever their permission level. Someone who can +**Detail values follow the same rule as the per-record routes:** a caller who can read a collection, +with no scope on it, gets its rows with `previousValues` and `newValues`, whatever their permission +level. Someone who can read the collection can already read each record's history one at a time, so the feed serves the same values in aggregate. Who can open the project's Activity Logs at all is decided per team by Forest (the `activityLogs` feature a team can be deactivated for), not by this route. The capabilities @@ -377,11 +378,13 @@ payload advertises the route as `canUseAuditTrailTimeline`, true only when the s `listTimeline`, and `canSortAuditTrailTimeline` alongside it: an agent without that flag serves the feed newest first whatever `sort` asks. A custom `listTimeline` must honour `order` and `after`. -**Permissions.** Only the collections the caller can read are queried. A collection the caller sees -only through a **record-level scope is left out entirely**: a scope can't be evaluated across a whole -timeline without fetching every record it mentions, and the row alone would already reveal that a -record the caller cannot read exists and was touched. A caller scoped on every collection therefore -gets an empty timeline. +**Permissions.** Only the collections the caller can read are queried. A collection the caller's +team has a **record-level scope on is left out entirely**, whatever the scope's condition, even one +every record passes: a scope can't be evaluated across a whole timeline without fetching every record +it mentions. A caller scoped on every collection therefore gets an empty timeline. On the project's +Activity Logs page, such a collection still shows its Forest activity logs, without diffs, since +Forest never filtered those by roles or scopes; each record's own history route still serves its +audit trail. **Filters** are the per-record route's, minus `fields` (a column name means nothing across collections): `userIds`, `operations`, `startDate` / `endDate` / `timezone`, `search`, parsed exactly From 3147db05c71ffb523cf66f9168c4c1bb42ee987f Mon Sep 17 00:00:00 2001 From: Olivier Bex-Chauvet Date: Wed, 21 Oct 2026 11:35:11 +0200 Subject: [PATCH 9/9] perf(audit-trail): index a record's history and its life boundaries [PRD-1269] Migration 003 adds (collection, record_id, timestamp, id), and the same columns over create and delete rows only. Without them, lifeBoundaryOf's oldest-row lookup walked the whole table's (timestamp, id) index: 534 ms on a 2.1M-row table for a recently created record, 0.04 ms with them. MySQL and MariaDB have no partial index and get the first one only. Co-Authored-By: Claude Opus 5.5 --- packages/agent/src/audit-trail/migrations.ts | 98 ++++++++++---- .../agent/test/audit-trail/migrations.test.ts | 124 +++++++++++++++++- 2 files changed, 198 insertions(+), 24 deletions(-) diff --git a/packages/agent/src/audit-trail/migrations.ts b/packages/agent/src/audit-trail/migrations.ts index da82101bc6..3d57577264 100644 --- a/packages/agent/src/audit-trail/migrations.ts +++ b/packages/agent/src/audit-trail/migrations.ts @@ -1,4 +1,4 @@ -import type { QueryInterface, Sequelize, Transaction } from 'sequelize'; +import type { QueryInterface, Sequelize, Transaction, WhereOptions } from 'sequelize'; import { DataTypes } from 'sequelize'; import { SequelizeStorage, Umzug } from 'umzug'; @@ -35,6 +35,14 @@ function qualifiedMigrationName002(schema: string | undefined, tableName: string : `${tableName}:002-index-timestamp-id`; } +function qualifiedMigrationName003(schema: string | undefined, tableName: string): string { + return schema + ? `${schema}.${tableName}:003-index-record-history` + : `${tableName}:003-index-record-history`; +} + +const DIALECTS_WITHOUT_PARTIAL_INDEXES = new Set(['mysql', 'mariadb']); + // Every table name claimed by an audit-trail store configured in this process, keyed by // `schema\0name` — both its own data table and the migration table Umzug derives from it. // A second store's data table can otherwise land on the exact name a first store's migration @@ -74,6 +82,31 @@ async function indexNames( return new Set(indexes.map(index => index.name)); } +async function addIndexOnce( + context: MigrationContext, + name: string, + options: { fields: Array; where?: WhereOptions }, +): Promise { + const table = { tableName: context.tableName, schema: context.schema }; + const hasIndex = () => + indexNames(context.queryInterface, table, context.transaction).then(names => names.has(name)); + + // Idempotent for the same reason 001 is: a process losing a concurrent-boot race retries. + if (await hasIndex()) return; + + try { + await context.queryInterface.addIndex(table, { + ...options, + name, + transaction: context.transaction, + }); + } catch (error) { + // Without Postgres's advisory lock, another agent booting at the same moment can add the + // index between the check above and this statement. Anything else is a real failure. + if (!(await hasIndex().catch(() => false))) throw error; + } +} + async function columnNames( queryInterface: QueryInterface, table: { tableName: string; schema?: string }, @@ -210,27 +243,9 @@ function buildMigrations(schema: string | undefined, tableName: string) { // record, ordered and paged by (timestamp, id): without this, each page sorts the whole table. name: qualifiedMigrationName002(schema, tableName), up: async ({ context }: { context: MigrationContext }) => { - const table = { tableName: context.tableName, schema: context.schema }; - const name = `${context.tableName}_timestamp_id`; - const hasIndex = () => - indexNames(context.queryInterface, table, context.transaction).then(names => - names.has(name), - ); - - // Idempotent for the same reason 001 is: a process losing a concurrent-boot race retries. - if (await hasIndex()) return; - - try { - await context.queryInterface.addIndex(table, { - fields: ['timestamp', 'id'], - name, - transaction: context.transaction, - }); - } catch (error) { - // Without Postgres's advisory lock, another agent booting at the same moment can add the - // index between the check above and this statement. Anything else is a real failure. - if (!(await hasIndex().catch(() => false))) throw error; - } + await addIndexOnce(context, `${context.tableName}_timestamp_id`, { + fields: ['timestamp', 'id'], + }); }, down: async ({ context }: { context: MigrationContext }) => { await context.queryInterface.removeIndex( @@ -240,6 +255,45 @@ function buildMigrations(schema: string | undefined, tableName: string) { ); }, }, + { + // A record's history is read by (collection, record_id) in (timestamp, id) order. With only + // `record_id` and `(timestamp, id)` to choose from, Postgres either reads every row the id + // holds across all collections, or walks the whole table's (timestamp, id) index under a + // LIMIT: finding a recent record's oldest row discarded 2.1M rows (534 ms) on a 2.1M-row table. + name: qualifiedMigrationName003(schema, tableName), + up: async ({ context }: { context: MigrationContext }) => { + await addIndexOnce(context, `${context.tableName}_record_history`, { + fields: ['collection', { name: 'record_id', length: 191 }, 'timestamp', 'id'], + }); + + if (DIALECTS_WITHOUT_PARTIAL_INDEXES.has(context.queryInterface.sequelize.getDialect())) { + return; + } + + // The life boundary lookup keeps a record's `create` and `delete` rows only: without this, + // a record updated 100k times is read whole to find its one `create`. + await addIndexOnce(context, `${context.tableName}_life_boundaries`, { + fields: ['collection', { name: 'record_id', length: 191 }, 'timestamp', 'id'], + where: { operation: ['create', 'delete'] }, + }); + }, + down: async ({ context }: { context: MigrationContext }) => { + const table = { tableName: context.tableName, schema: context.schema }; + const existing = await indexNames(context.queryInterface, table, context.transaction); + + for (const name of [ + `${context.tableName}_life_boundaries`, + `${context.tableName}_record_history`, + ]) { + if (existing.has(name)) { + // eslint-disable-next-line no-await-in-loop + await context.queryInterface.removeIndex(table, name, { + transaction: context.transaction, + }); + } + } + }, + }, ]; } diff --git a/packages/agent/test/audit-trail/migrations.test.ts b/packages/agent/test/audit-trail/migrations.test.ts index 10051ec769..89e05e1ce6 100644 --- a/packages/agent/test/audit-trail/migrations.test.ts +++ b/packages/agent/test/audit-trail/migrations.test.ts @@ -71,6 +71,7 @@ describe('runAuditMigrations (sqlite)', () => { expect(applied).toEqual([ { name: 'forest.audit_logs:001-create-audit-logs' }, { name: 'forest.audit_logs:002-index-timestamp-id' }, + { name: 'forest.audit_logs:003-index-record-history' }, ]); await sequelize.close(); @@ -85,6 +86,7 @@ describe('runAuditMigrations (sqlite)', () => { expect(applied).toEqual([ { name: 'audit_logs:001-create-audit-logs' }, { name: 'audit_logs:002-index-timestamp-id' }, + { name: 'audit_logs:003-index-record-history' }, ]); await sequelize.close(); @@ -169,7 +171,7 @@ describe('runAuditMigrations (sqlite)', () => { ).resolves.toBeUndefined(); const [applied] = await sequelize.query('SELECT name FROM "audit_logs_migration"'); - expect(applied).toHaveLength(2); + expect(applied).toHaveLength(3); await sequelize.close(); }); @@ -255,7 +257,7 @@ describe('runAuditMigrations (sqlite)', () => { const umzug = buildUmzug(sequelize, { tableName: 'audit_logs' }); await umzug.up(); - await umzug.down(); + await umzug.down({ step: 2 }); expect(await indexOf(sequelize)).toBeUndefined(); @@ -263,6 +265,124 @@ describe('runAuditMigrations (sqlite)', () => { }); }); + describe('migration 003: the record history indexes', () => { + const MIGRATION_003 = 'audit_logs:003-index-record-history'; + + const indexesOf = async (sequelize: Sequelize) => { + const [rows] = await sequelize.query( + "SELECT name, sql FROM sqlite_master WHERE type = 'index' AND tbl_name = 'audit_logs'", + ); + + return new Map((rows as Array<{ name: string; sql: string }>).map(r => [r.name, r.sql])); + }; + + const shippedBefore003 = async () => { + const sequelize = new Sequelize('sqlite::memory:', { logging: false }); + await runAuditMigrations(sequelize, { tableName: 'audit_logs' }); + await sequelize.query('DROP INDEX "audit_logs_record_history"'); + await sequelize.query('DROP INDEX "audit_logs_life_boundaries"'); + await sequelize.query(`DELETE FROM "audit_logs_migration" WHERE name = '${MIGRATION_003}'`); + + return sequelize; + }; + + it('indexes a record history by collection, record_id, then timestamp and id', async () => { + const sequelize = new Sequelize('sqlite::memory:', { logging: false }); + + await runAuditMigrations(sequelize, { tableName: 'audit_logs' }); + + expect((await indexesOf(sequelize)).get('audit_logs_record_history')).toMatch( + /\(`collection`, `record_id`, `timestamp`, `id`\)$/, + ); + + await sequelize.close(); + }); + + it('indexes the same columns over create and delete rows only, for the life boundary', async () => { + const sequelize = new Sequelize('sqlite::memory:', { logging: false }); + + await runAuditMigrations(sequelize, { tableName: 'audit_logs' }); + + expect((await indexesOf(sequelize)).get('audit_logs_life_boundaries')).toMatch( + /\(`collection`, `record_id`, `timestamp`, `id`\) WHERE `operation` IN \('create', 'delete'\)$/, + ); + + await sequelize.close(); + }); + + it('adds both indexes to a table that already applied 002', async () => { + const sequelize = await shippedBefore003(); + + await runAuditMigrations(sequelize, { tableName: 'audit_logs' }); + + const indexes = await indexesOf(sequelize); + expect([...indexes.keys()]).toEqual( + expect.arrayContaining(['audit_logs_record_history', 'audit_logs_life_boundaries']), + ); + + await sequelize.close(); + }); + + it('skips the partial index on a dialect that cannot filter one', async () => { + const sequelize = await shippedBefore003(); + jest.spyOn(sequelize, 'getDialect').mockReturnValue('mysql'); + + await buildUmzug(sequelize, { tableName: 'audit_logs' }).up(); + + const indexes = await indexesOf(sequelize); + expect(indexes.has('audit_logs_record_history')).toBe(true); + expect(indexes.has('audit_logs_life_boundaries')).toBe(false); + + await sequelize.close(); + }); + + it('treats an index as added when a concurrent boot added it after the check', async () => { + const sequelize = await shippedBefore003(); + const queryInterface = sequelize.getQueryInterface(); + const addIndex = queryInterface.addIndex.bind(queryInterface); + jest.spyOn(queryInterface, 'addIndex').mockImplementationOnce(async (...args) => { + await (addIndex as (...a: unknown[]) => Promise)(...args); + throw new Error('index "audit_logs_record_history" already exists'); + }); + + await expect( + runAuditMigrations(sequelize, { tableName: 'audit_logs' }), + ).resolves.toBeUndefined(); + + const [applied] = await sequelize.query('SELECT name FROM "audit_logs_migration"'); + expect(applied).toContainEqual({ name: MIGRATION_003 }); + + await sequelize.close(); + }); + + it('still fails when adding an index fails and the index is missing', async () => { + const sequelize = await shippedBefore003(); + jest + .spyOn(sequelize.getQueryInterface(), 'addIndex') + .mockRejectedValueOnce(new Error('disk full')); + + await expect(runAuditMigrations(sequelize, { tableName: 'audit_logs' })).rejects.toThrow( + 'disk full', + ); + + await sequelize.close(); + }); + + it('drops both indexes on down', async () => { + const sequelize = new Sequelize('sqlite::memory:', { logging: false }); + const umzug = buildUmzug(sequelize, { tableName: 'audit_logs' }); + await umzug.up(); + + await umzug.down(); + + const indexes = await indexesOf(sequelize); + expect(indexes.has('audit_logs_record_history')).toBe(false); + expect(indexes.has('audit_logs_life_boundaries')).toBe(false); + + await sequelize.close(); + }); + }); + it('rejects a pre-existing table sharing the name that is missing audit-trail columns', async () => { const sequelize = new Sequelize('sqlite::memory:', { logging: false }); await sequelize.getQueryInterface().createTable('audit_logs', {