From 300bd3d7c9546ccc9ddcfa15e47ee96d399b753a Mon Sep 17 00:00:00 2001 From: Olivier Bex-Chauvet Date: Tue, 29 Sep 2026 14:08:14 +0200 Subject: [PATCH 1/6] fix(audit-trail): match search and fields against served values, not withheld ones [PRD-1295] Matched in SQL on a gone record's history, a search still answered what the withholding hides: whether a row came back, the count and the authors each said whether a withheld value held the term. Under a scope on a record gone at the check, the rows are read without those two filters, withheld, then matched and paged as they go, in batches that continue past the last row read and are bounded at the instant the scan starts. Co-Authored-By: Claude Opus 5.5 --- packages/agent/src/audit-trail/README.md | 9 ++ packages/agent/src/audit-trail/sql-store.ts | 12 ++ packages/agent/src/audit-trail/types.ts | 5 + .../agent/src/routes/access/audit-trail.ts | 115 ++++++++++++++- .../agent/test/audit-trail/sql-store.test.ts | 22 +++ .../test/routes/access/audit-trail.test.ts | 139 ++++++++++++++++++ 6 files changed, 298 insertions(+), 4 deletions(-) diff --git a/packages/agent/src/audit-trail/README.md b/packages/agent/src/audit-trail/README.md index badd18c4f8..14b957eb91 100644 --- a/packages/agent/src/audit-trail/README.md +++ b/packages/agent/src/audit-trail/README.md @@ -288,6 +288,15 @@ rows, so it composes with pagination and `meta.count` the same way every other f redacted value can never match a search for the real value: `redact` replaces it before the row is ever written, so the real value was never in the database to find. +Nor can a search confirm a value the scope withholding hides. On a record gone for good under a +caller's permission scope, `search` and `fields` are matched against the values as served, never as +captured — in SQL, which rows come back, `meta.count` and `availableUsers` would each say whether a +withheld value holds the term. The rows are read without those two filters, in batches of 500 that +each continue past the last row read, never at an offset, and bounded at the instant the scan +starts. They are matched and paged as they go, keeping only the page asked for. Only a gone +record's history pays that scan, and it is one record's history — the same rows the SQL search +would have scanned without an index. + 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 diff --git a/packages/agent/src/audit-trail/sql-store.ts b/packages/agent/src/audit-trail/sql-store.ts index d2f4453549..21e7ef29d1 100644 --- a/packages/agent/src/audit-trail/sql-store.ts +++ b/packages/agent/src/audit-trail/sql-store.ts @@ -198,6 +198,8 @@ function buildHistoryWhereClause( endTimestamp, fields, search, + order, + after, }: AuditHistoryQuery, sequelize: Sequelize, ): Record { @@ -213,6 +215,16 @@ function buildHistoryWhereClause( const andConditions = []; if (fields?.length) andConditions.push(fieldsChangedCondition(sequelize, fields)); if (search) andConditions.push(searchCondition(sequelize, search)); + + if (after) { + const past = order === 'desc' ? Op.lt : Op.gt; + const at = new Date(after.timestamp); + + andConditions.push({ + [Op.or]: [{ timestamp: { [past]: at } }, { timestamp: at, id: { [past]: after.id } }], + }); + } + if (andConditions.length) where[Op.and] = andConditions; return where; diff --git a/packages/agent/src/audit-trail/types.ts b/packages/agent/src/audit-trail/types.ts index c82ca7a90e..2b4897e9ae 100644 --- a/packages/agent/src/audit-trail/types.ts +++ b/packages/agent/src/audit-trail/types.ts @@ -69,6 +69,11 @@ export type AuditHistoryQuery = { search?: string; /** Sort direction on `timestamp` (ties broken by insertion order). Defaults to `'asc'`. */ order?: 'asc' | 'desc'; + /** + * Keep only the rows strictly past this one in `order`, so a scan keyed on the last row it read + * neither repeats nor skips one when entries are written between its reads, as `skip` would. + */ + after?: Pick; }; export type AuditCorrelationQuery = { diff --git a/packages/agent/src/routes/access/audit-trail.ts b/packages/agent/src/routes/access/audit-trail.ts index 6742019fcf..d00bbe3eaf 100644 --- a/packages/agent/src/routes/access/audit-trail.ts +++ b/packages/agent/src/routes/access/audit-trail.ts @@ -1,4 +1,4 @@ -import type { AuditRecord } from '../../audit-trail'; +import type { AuditHistoryQuery, AuditRecord, AuditUserSummary } from '../../audit-trail'; import type { CollectionSchema, ConditionTree } from '@forestadmin/datasource-toolkit'; import type Router from '@koa/router'; import type { Context } from 'koa'; @@ -33,6 +33,7 @@ const ISO_INSTANT = /[Zz]$|[+-]\d{2}:?\d{2}$/; const DEFAULT_PAGE_SIZE = 20; const MAX_PAGE_SIZE = 100; +const SCAN_BATCH_SIZE = 500; type AuditHistoryFilters = { userIds?: number[]; @@ -71,15 +72,14 @@ export default class AuditTrailRoute extends CollectionRoute { const { userIds, startTimestamp, endTimestamp, fields, search } = AuditTrailRoute.parseFilters(context); - const filters = { + const rowFilters = { collection: this.collection.name, recordId: context.params.id, ...(userIds && { userIds }), ...(startTimestamp && { startTimestamp }), ...(endTimestamp && { endTimestamp }), - ...(fields && { fields }), - ...(search && { search }), }; + const filters = { ...rowFilters, ...(fields && { fields }), ...(search && { search }) }; // Distinct authors are scoped to the active filters but independent of the page — returned // only on the first fetch (no explicit page[number]) so the front keeps the list it already @@ -87,6 +87,26 @@ export default class AuditTrailRoute extends CollectionRoute { const isFirstFetch = (context.request.query as Record)['page[number]'] === undefined; + if (permissionScope && goneEntirely && (fields || search)) { + const matched = await this.scanServedValues(context, permissionScope, rowFilters, { + fields, + search, + order, + skip, + limit, + }); + + context.response.body = { + data: matched.page, + meta: { + count: matched.count, + ...(isFirstFetch && { availableUsers: [...matched.authors.values()] }), + }, + }; + + return; + } + // `count` reflects the active filters and is independent of the page. const [rawData, count, availableUsers] = await Promise.all([ store.listByRecord({ ...filters, skip, limit, order }), @@ -129,6 +149,93 @@ export default class AuditTrailRoute extends CollectionRoute { }; } + // Matched in SQL, `search` and `fields` test the values as captured, so which rows come back, the + // count and the authors would still say what the withholding hides — one probe per character. For + // a record gone at the check they are matched against the values served instead, which means + // scanning the whole history: in batches, keeping only the page asked for, each continuing past + // the last row read rather than at an offset that entries written in between would shift, and + // bounded at the instant the scan starts so an id taken since cannot keep it chasing new rows. A + // record in scope at the check was the caller's to read whole. + private async scanServedValues( + context: Context, + permissionScope: ConditionTree, + rowFilters: Omit, + { + fields, + search, + order, + skip, + limit, + }: AuditHistoryFilters & { order: 'asc' | 'desc'; skip: number; limit: number }, + ): Promise<{ page: AuditRecord[]; count: number; authors: Map }> { + const { store } = this.options.auditTrail; + const now = new Date().toISOString(); + const endTimestamp = + rowFilters.endTimestamp && rowFilters.endTimestamp < now ? rowFilters.endTimestamp : now; + const page: AuditRecord[] = []; + const authors = new Map(); + let count = 0; + let after: AuditRecord | undefined; + let rows: AuditRecord[]; + + do { + // eslint-disable-next-line no-await-in-loop + rows = await store.listByRecord({ + ...rowFilters, + endTimestamp, + order, + limit: SCAN_BATCH_SIZE, + ...(after && { after: { timestamp: after.timestamp, id: after.id } }), + }); + + const matched = this.withhold(rows, permissionScope, context).filter(entry => + AuditTrailRoute.matchesServedValues(entry, fields, search), + ); + + for (const entry of matched) { + if (count >= skip && page.length < limit) page.push(entry); + count += 1; + + if (!authors.has(entry.userId)) { + authors.set(entry.userId, { + id: entry.userId, + firstName: entry.userFirstName, + lastName: entry.userLastName, + email: entry.userEmail, + }); + } + } + + after = rows[rows.length - 1]; + } while (rows.length === SCAN_BATCH_SIZE); + + 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)), + ); + 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))); + } + private withhold( entries: AuditRecord[], permissionScope: ConditionTree, diff --git a/packages/agent/test/audit-trail/sql-store.test.ts b/packages/agent/test/audit-trail/sql-store.test.ts index 4e2e744dc5..5f1280f2c0 100644 --- a/packages/agent/test/audit-trail/sql-store.test.ts +++ b/packages/agent/test/audit-trail/sql-store.test.ts @@ -496,6 +496,28 @@ describe('createSqlAuditStore (sqlite round-trip)', () => { await close(); }); + // A cursor survives writes between reads that would shift an offset: nothing repeats, nothing is + // skipped. + it('continues past a given row in either order, whatever was written since', async () => { + const { store, close } = createSqlAuditStore({ connectionString: 'sqlite::memory:' }); + const query = { collection: 'accounts', recordId: '1' }; + + await seed(store, record({ timestamp: '2026-01-01T00:00:00.000Z', correlationKey: 'a' })); + await seed(store, record({ timestamp: '2026-01-02T00:00:00.000Z', correlationKey: 'b' })); + await seed(store, record({ timestamp: '2026-01-01T00:00:00.000Z', correlationKey: 'a2' })); + + const newest = await store.listByRecord({ ...query, order: 'desc', limit: 2 }); + await seed(store, record({ timestamp: '2026-01-03T00:00:00.000Z', correlationKey: 'late' })); + const rest = await store.listByRecord({ ...query, order: 'desc', after: newest[1] }); + const oldest = await store.listByRecord({ ...query, limit: 1 }); + const later = await store.listByRecord({ ...query, after: oldest[0] }); + + expect([...newest, ...rest].map(r => r.correlationKey)).toEqual(['b', 'a2', 'a']); + expect([...oldest, ...later].map(r => r.correlationKey)).toEqual(['a', 'a2', 'b', 'late']); + + await close(); + }); + it('returns rows recorded under a correlationKey for a record, scoped and oldest first', async () => { const { store, close } = createSqlAuditStore({ connectionString: 'sqlite::memory:' }); diff --git a/packages/agent/test/routes/access/audit-trail.test.ts b/packages/agent/test/routes/access/audit-trail.test.ts index da3d8f3886..366233c7f2 100644 --- a/packages/agent/test/routes/access/audit-trail.test.ts +++ b/packages/agent/test/routes/access/audit-trail.test.ts @@ -748,6 +748,145 @@ describe('AuditTrailRoute', () => { }); }); + // Matched in SQL, a search would still answer what the withholding hides: whether the row comes + // back, and the count, say whether the withheld value holds the term. + describe('a genuinely gone, scoped record searched or filtered by field', () => { + const secretDelete = (over: Record = {}) => ({ + id: 1, + timestamp: '2026-01-01T00:00:00.000Z', + operation: 'delete', + recordId: '2', + userId: 7, + userFirstName: null, + userLastName: null, + userEmail: 'jane@acme.io', + actionName: null, + previousValues: { ownerId: 2, title: 'Secret' }, + newValues: {}, + ...over, + }); + const kept = (over: Record = {}) => + secretDelete({ previousValues: { ownerId: 1, title: 'Mine' }, ...over }); + + const searched = async (history: unknown[], query: Record) => { + const { services, dataSource, options, store } = setup(history); + (services.authorization.getScope as jest.Mock).mockResolvedValue( + new ConditionTreeLeaf('ownerId', 'Equal', 1), + ); + jest.spyOn(dataSource.getCollection('books'), 'list').mockResolvedValue([]); + 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 { body: context.response.body as { data: unknown[]; meta: unknown }, store }; + }; + + test('finds nothing in a withheld value, and counts nothing', async () => { + const { body } = await searched([secretDelete()], { search: 'secret' }); + + expect(body).toEqual({ data: [], meta: { count: 0, availableUsers: [] } }); + }); + + test('matches the values it serves', async () => { + const { body } = await searched([kept()], { search: 'MINE' }); + + expect(body.data).toEqual([kept()]); + }); + + test('still matches what stays visible on a withheld row, such as its author', async () => { + const { body } = await searched([secretDelete()], { search: 'acme' }); + + expect(body).toEqual({ + data: [secretDelete({ previousValues: {} })], + meta: { + count: 1, + availableUsers: [{ id: 7, firstName: null, lastName: null, email: 'jane@acme.io' }], + }, + }); + }); + + test('lists an author once even when their rows carry different identities', async () => { + const { body } = await searched( + [secretDelete(), secretDelete({ id: 2, userEmail: 'jane@acme.com' })], + { search: 'acme' }, + ); + + expect(body.meta).toEqual({ + count: 2, + availableUsers: [{ id: 7, firstName: null, lastName: null, email: 'jane@acme.io' }], + }); + }); + + test('does not match a field only a withheld side touched', async () => { + const { body } = await searched([secretDelete()], { fields: 'title' }); + + expect(body.data).toEqual([]); + }); + + test('reads the rows without the value filters, bounded at the instant the scan starts', async () => { + const before = new Date().toISOString(); + + const { store } = await searched([kept()], { search: 'mine', userIds: '12' }); + + const [query] = (store.listByRecord as jest.Mock).mock.calls[0]; + expect(query).toEqual({ + collection: 'books', + recordId: '2', + userIds: [12], + order: 'desc', + limit: 500, + endTimestamp: expect.any(String), + }); + expect(query.endTimestamp >= before).toBe(true); + expect(store.countByRecord).not.toHaveBeenCalled(); + }); + + test('keeps an end date the caller asked for when it is earlier', async () => { + const { store } = await searched([], { search: 'mine', endDate: '2020-01-01' }); + + expect(store.listByRecord).toHaveBeenCalledWith( + expect.objectContaining({ endTimestamp: expect.stringMatching(/^2020-01-01T/) }), + ); + }); + + // The page cap bounds what is served, not what is scanned: the history is read in batches, + // each continuing past the last row read, so a long one is never held in memory whole. + test('scans in batches past the last row read, counting across them and keeping only the page', async () => { + const rows = Array.from({ length: 1001 }, (_, index) => kept({ id: index + 1 })); + const { services, dataSource, options, store } = setup(); + store.listByRecord.mockImplementation( + async ({ after, limit }: { after?: { id: number }; limit: number }) => + rows.filter(row => !after || row.id > after.id).slice(0, limit), + ); + (services.authorization.getScope as jest.Mock).mockResolvedValue( + new ConditionTreeLeaf('ownerId', 'Equal', 1), + ); + jest.spyOn(dataSource.getCollection('books'), 'list').mockResolvedValue([]); + const route = new AuditTrailRoute(services, options, dataSource, 'books'); + const context = createMockContext({ + state: { user: { email: 'john.doe@domain.com' } }, + customProperties: { + query: { search: 'mine', 'page[size]': '2', 'page[number]': '251' }, + params: { id: '2' }, + }, + }); + + await route.handleHistory(context); + + const calls = (store.listByRecord as jest.Mock).mock.calls.map(([query]) => query); + expect(calls.map(query => query.after?.id)).toEqual([undefined, 500, 1000]); + expect(calls.every(query => query.skip === undefined)).toBe(true); + expect(context.response.body).toEqual({ + data: [rows[500], rows[501]], + meta: { count: 1001 }, + }); + }); + }); + test("keeps a genuinely-deleted delete row's previousValues when they match the caller's scope", async () => { const history = [ { From 29a2dd1f731962ad176c608a4d4157b653342848 Mon Sep 17 00:00:00 2001 From: Olivier Bex-Chauvet Date: Tue, 29 Sep 2026 14:21:32 +0200 Subject: [PATCH 2/6] fix(audit-trail): list a searched gone record's author as their latest identity [PRD-1295] Co-Authored-By: Claude Opus 5.5 --- packages/agent/src/routes/access/audit-trail.ts | 3 ++- .../agent/test/routes/access/audit-trail.test.ts | 12 ++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/packages/agent/src/routes/access/audit-trail.ts b/packages/agent/src/routes/access/audit-trail.ts index d00bbe3eaf..988ef626be 100644 --- a/packages/agent/src/routes/access/audit-trail.ts +++ b/packages/agent/src/routes/access/audit-trail.ts @@ -196,7 +196,8 @@ export default class AuditTrailRoute extends CollectionRoute { if (count >= skip && page.length < limit) page.push(entry); count += 1; - if (!authors.has(entry.userId)) { + // An author reads as their latest identity whichever way the history is sorted. + if (order === 'asc' || !authors.has(entry.userId)) { authors.set(entry.userId, { id: entry.userId, firstName: entry.userFirstName, diff --git a/packages/agent/test/routes/access/audit-trail.test.ts b/packages/agent/test/routes/access/audit-trail.test.ts index 366233c7f2..e8229e4734 100644 --- a/packages/agent/test/routes/access/audit-trail.test.ts +++ b/packages/agent/test/routes/access/audit-trail.test.ts @@ -821,6 +821,18 @@ describe('AuditTrailRoute', () => { }); }); + test('lists an author as their latest identity when sorted oldest first', async () => { + const { body } = await searched( + [secretDelete(), secretDelete({ id: 2, userEmail: 'jane@acme.com' })], + { search: 'acme', sort: 'timestamp' }, + ); + + expect(body.meta).toEqual({ + count: 2, + availableUsers: [{ id: 7, firstName: null, lastName: null, email: 'jane@acme.com' }], + }); + }); + test('does not match a field only a withheld side touched', async () => { const { body } = await searched([secretDelete()], { fields: 'title' }); From 272be0eede5ff84e1667b03903f4be3f67ceff5e Mon Sep 17 00:00:00 2001 From: Olivier Bex-Chauvet Date: Wed, 30 Sep 2026 18:10:49 +0200 Subject: [PATCH 3/6] fix(audit-trail): match served values for a record deleted mid-request [PRD-1295] The second read of the record decides the withholding, so the count, authors and page follow it too instead of the SQL match. Co-Authored-By: Claude Opus 5.5 --- packages/agent/src/audit-trail/README.md | 4 ++- .../agent/src/routes/access/audit-trail.ts | 23 ++++++++++--- .../test/routes/access/audit-trail.test.ts | 33 +++++++++++++++++++ 3 files changed, 54 insertions(+), 6 deletions(-) diff --git a/packages/agent/src/audit-trail/README.md b/packages/agent/src/audit-trail/README.md index 1ce00daf4a..86feb1a812 100644 --- a/packages/agent/src/audit-trail/README.md +++ b/packages/agent/src/audit-trail/README.md @@ -296,7 +296,9 @@ withheld value holds the term. The rows are read without those two filters, in b each continue past the last row read, never at an offset, and bounded at the instant the scan starts. They are matched and paged as they go, keeping only the page asked for. Only a gone record's history pays that scan, and it is one record's history — the same rows the SQL search -would have scanned without an index. +would have scanned without an index. That holds too for a record deleted while the request was in flight: +the second read of the record decides the withholding, so the SQL-matched answer is discarded and +the 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 (`,`, diff --git a/packages/agent/src/routes/access/audit-trail.ts b/packages/agent/src/routes/access/audit-trail.ts index 30762016f1..a2b206c168 100644 --- a/packages/agent/src/routes/access/audit-trail.ts +++ b/packages/agent/src/routes/access/audit-trail.ts @@ -102,8 +102,10 @@ export default class AuditTrailRoute extends CollectionRoute { const isFirstFetch = (context.request.query as Record)['page[number]'] === undefined; - if (permissionScope && goneEntirely && (fields || search)) { - const matched = await this.scanServedValues(context, permissionScope, rowFilters, { + const filtersOnValues = Boolean(fields || search); + + const serveMatchedValues = async (scope: ConditionTree) => { + const matched = await this.scanServedValues(context, scope, rowFilters, { fields, search, order, @@ -118,6 +120,10 @@ export default class AuditTrailRoute extends CollectionRoute { ...(isFirstFetch && { availableUsers: [...matched.authors.values()] }), }, }; + }; + + if (permissionScope && goneEntirely && filtersOnValues) { + await serveMatchedValues(permissionScope); return; } @@ -150,6 +156,14 @@ 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); + + return; + } + // A genuinely deleted record bypasses the permission-scope check above — there's nothing left to check // 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, @@ -166,11 +180,10 @@ export default class AuditTrailRoute extends CollectionRoute { // Matched in SQL, `search` and `fields` test the values as captured, so which rows come back, the // count and the authors would still say what the withholding hides — one probe per character. For - // a record gone at the check they are matched against the values served instead, which means + // a gone record they are matched against the values served instead, which means // scanning the whole history: in batches, keeping only the page asked for, each continuing past // the last row read rather than at an offset that entries written in between would shift, and - // bounded at the instant the scan starts so an id taken since cannot keep it chasing new rows. A - // record in scope at the check was the caller's to read whole. + // 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, diff --git a/packages/agent/test/routes/access/audit-trail.test.ts b/packages/agent/test/routes/access/audit-trail.test.ts index 5c8a47f179..7955066963 100644 --- a/packages/agent/test/routes/access/audit-trail.test.ts +++ b/packages/agent/test/routes/access/audit-trail.test.ts @@ -957,6 +957,39 @@ describe('AuditTrailRoute', () => { ); }); + test('matches served values for a record deleted while the audit read was in flight', async () => { + const { services, dataSource, options, store } = setup([secretDelete()]); + (services.authorization.getScope as jest.Mock).mockResolvedValue( + new ConditionTreeLeaf('ownerId', 'Equal', 1), + ); + jest + .spyOn(dataSource.getCollection('books'), 'list') + .mockResolvedValueOnce([{ id: 2, ownerId: 1 }]) // present and in scope + .mockResolvedValue([]); // re-read, scoped and bare: genuinely gone + const route = new AuditTrailRoute(services, options, dataSource, 'books'); + const context = createMockContext({ + state: { user: { email: 'john.doe@domain.com' } }, + customProperties: { + query: { timezone: 'Europe/Paris', search: 'secret' }, + params: { id: '2' }, + }, + }); + + await route.handleHistory(context); + + expect(store.listByRecord).toHaveBeenLastCalledWith({ + collection: 'books', + recordId: '2', + order: 'desc', + limit: 500, + endTimestamp: expect.any(String), + }); + expect(context.response.body).toEqual({ + data: [], + meta: { count: 0, availableUsers: [] }, + }); + }); + // The page cap bounds what is served, not what is scanned: the history is read in batches, // each continuing past the last row read, so a long one is never held in memory whole. test('scans in batches past the last row read, counting across them and keeping only the page', async () => { From f32d022ee562a131774fe954a7d2d80263cef336 Mon Sep 17 00:00:00 2001 From: Olivier Bex-Chauvet Date: Wed, 30 Sep 2026 18:18:34 +0200 Subject: [PATCH 4/6] test(audit-trail): honor the after cursor in the in-memory store [PRD-1295] Breaks timestamp ties by id in the sort direction, as the SQL store does, so a scan over the fake pages like one over the real store. Co-Authored-By: Claude Opus 5.5 --- .../test/audit-trail/in-memory-store.test.ts | 26 +++++++++++++++++++ .../agent/test/audit-trail/in-memory-store.ts | 8 +++--- 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/packages/agent/test/audit-trail/in-memory-store.test.ts b/packages/agent/test/audit-trail/in-memory-store.test.ts index 112c1f37a8..286dd4f60f 100644 --- a/packages/agent/test/audit-trail/in-memory-store.test.ts +++ b/packages/agent/test/audit-trail/in-memory-store.test.ts @@ -153,6 +153,32 @@ describe('InMemoryAuditStore', () => { expect(page2.map(r => r.newValues)).toEqual([{ n: 3 }]); }); + describe('listByRecord after a cursor', () => { + const history = { collection: 'accounts', recordId: '1' }; + const seedTied = (store: InMemoryAuditStore) => + [1, 2, 3].map(id => + store.seed(record({ id, ...(id === 3 && { timestamp: '2026-01-02T00:00:00.000Z' }) })), + ); + + test('continues past the cursor oldest first, breaking a timestamp tie by id', () => { + const store = new InMemoryAuditStore(); + const [first] = seedTied(store); + + const rows = store.listByRecord({ ...history, order: 'asc', after: first }); + + expect(rows.map(row => row.id)).toEqual([2, 3]); + }); + + test('continues past the cursor newest first, breaking a timestamp tie by id', () => { + const store = new InMemoryAuditStore(); + const [, second] = seedTied(store); + + const rows = store.listByRecord({ ...history, order: 'desc', after: second }); + + expect(rows.map(row => row.id)).toEqual([1]); + }); + }); + describe('countByRecord', () => { it('counts all matching entries, ignoring skip and limit', () => { const store = new InMemoryAuditStore(); diff --git a/packages/agent/test/audit-trail/in-memory-store.ts b/packages/agent/test/audit-trail/in-memory-store.ts index 43b943d007..a271d07ddb 100644 --- a/packages/agent/test/audit-trail/in-memory-store.ts +++ b/packages/agent/test/audit-trail/in-memory-store.ts @@ -115,17 +115,19 @@ export default class InMemoryAuditStore implements AuditStore { startTimestamp, endTimestamp, order = 'asc', + after, }: AuditHistoryQuery): AuditRecord[] { const direction = order === 'desc' ? -1 : 1; + const compare = (a: Pick, b: typeof a) => + direction * (a.timestamp.localeCompare(b.timestamp) || a.id - b.id); - // Ties on equal timestamps fall back to insertion order (the stable sort keeps it), which is - // the in-memory equivalent of the SQL store's auto-increment id — deterministic and chronological. return this.records .filter(record => record.collection === collection && record.recordId === recordId) .filter(record => !userIds || userIds.includes(record.userId)) .filter(record => !operations?.length || operations.includes(record.operation)) .filter(record => !startTimestamp || record.timestamp >= startTimestamp) .filter(record => !endTimestamp || record.timestamp <= endTimestamp) - .sort((a, b) => direction * a.timestamp.localeCompare(b.timestamp)); + .filter(record => !after || compare(record, after) > 0) + .sort(compare); } } From e9204833c2648b0b92d322ca95de9275c063747e Mon Sep 17 00:00:00 2001 From: Olivier Bex-Chauvet Date: Thu, 1 Oct 2026 09:55:21 +0200 Subject: [PATCH 5/6] fix(audit-trail): a captured null answers only a scope asking for it [PRD-1295] In memory `status != 'private'` holds for a null status and `null < 5` coerces to `0 < 5`, while the scoped read that guarded the live record left that NULL out: a record the caller could never read alive became readable once deleted. A null now matches only Blank, Missing, Equal null or an In list holding null, leaf by leaf, on every route that withholds. Co-Authored-By: Claude Opus 5.5 --- packages/agent/src/audit-trail/README.md | 7 ++ packages/agent/src/audit-trail/withhold.ts | 54 +++++++++- .../test/routes/access/audit-trail.test.ts | 98 ++++++++++++++++++- 3 files changed, 154 insertions(+), 5 deletions(-) diff --git a/packages/agent/src/audit-trail/README.md b/packages/agent/src/audit-trail/README.md index 86feb1a812..e2ff2e19a0 100644 --- a/packages/agent/src/audit-trail/README.md +++ b/packages/agent/src/audit-trail/README.md @@ -214,6 +214,13 @@ That test only runs when the snapshot can actually answer it. The capture keeps a field stored redacted — has no honest answer in the snapshot and the values are withheld rather than matched against a missing key: absent is not the same as passing. +A captured `null` is tested the way the database tests a `NULL`: it answers only a condition asking +for the null itself (`Blank`, `Missing`, `Equal` null, an `In` list holding null), never a negated or +ordered one. In memory `status != 'private'` holds for a null status and `null < 5` coerces to +`0 < 5`, while the scoped read that guarded the live record left that record out — without this, a +record the caller could never read alive would become readable once deleted. On a datasource whose +own `!=` keeps a NULL (Mongo's `$ne`), this is stricter than the live read, never looser. + Primary keys are the exception, read back from the row's own id — but only for the side that id speaks for. A row is filed under the identity the record ended up with, so its id answers for a `create`, a `delete` and the new side of an `update`, never for what an update moved away from. A diff --git a/packages/agent/src/audit-trail/withhold.ts b/packages/agent/src/audit-trail/withhold.ts index 60ac08d489..1bea817644 100644 --- a/packages/agent/src/audit-trail/withhold.ts +++ b/packages/agent/src/audit-trail/withhold.ts @@ -1,5 +1,11 @@ import type { AuditRecord } from './types'; -import type { Collection, ConditionTree, Logger } from '@forestadmin/datasource-toolkit'; +import type { + Collection, + ConditionTree, + ConditionTreeBranch, + ConditionTreeLeaf, + Logger, +} from '@forestadmin/datasource-toolkit'; import { SchemaUtils } from '@forestadmin/datasource-toolkit'; @@ -23,6 +29,50 @@ export type Withholding = { */ type IdAnswersForSide = boolean; +function asksForNull({ operator, value }: ConditionTreeLeaf): boolean { + switch (operator) { + case 'Blank': + case 'Missing': + return true; + case 'Equal': + return value === null || value === undefined; + case 'In': + return Array.isArray(value) && value.some(item => item === null || item === undefined); + default: + return false; + } +} + +// `ConditionTree.match`, except that a null answers only a condition asking for the null itself, as +// the database tests it. In memory `status != 'private'` holds for a null status and `null < 5` +// coerces to `0 < 5`, while the scoped read that guarded the live record left that NULL out: a record +// the caller could never read alive would become readable once deleted. Stricter than a datasource +// that matches NULL there (Mongo's `$ne`), never looser. +function matchesAsStored( + tree: ConditionTree, + values: Record, + collection: Collection, + timezone: string, +): boolean { + // By shape, not `instanceof`: a scope can be built by another copy of the toolkit. + if ('aggregator' in tree) { + const branch = tree as ConditionTreeBranch; + const evaluate = (condition: ConditionTree) => + matchesAsStored(condition, values, collection, timezone); + + return branch.aggregator === 'And' + ? branch.conditions.every(evaluate) + : branch.conditions.some(evaluate); + } + + const leaf = tree as ConditionTreeLeaf; + const value = values[leaf.field]; + + return value === null || value === undefined + ? asksForNull(leaf) + : leaf.match(values, collection, timezone); +} + // Only a snapshot that answers every field the permission scope asks about is worth matching. The // capture keeps the writable columns, so a permission scope reaching for anything else — a // read-only column, a relation — reads `undefined` there and would answer for a value the row never @@ -43,7 +93,7 @@ export function permissionScopeAccepts( Object.prototype.hasOwnProperty.call(values, field), ); - return answered && permissionScope.match(values, collection, timezone); + return answered && matchesAsStored(permissionScope, values, collection, timezone); } /** diff --git a/packages/agent/test/routes/access/audit-trail.test.ts b/packages/agent/test/routes/access/audit-trail.test.ts index 7955066963..80156dbba5 100644 --- a/packages/agent/test/routes/access/audit-trail.test.ts +++ b/packages/agent/test/routes/access/audit-trail.test.ts @@ -1,4 +1,4 @@ -import { ConditionTreeLeaf } from '@forestadmin/datasource-toolkit'; +import { ConditionTreeBranch, ConditionTreeLeaf } from '@forestadmin/datasource-toolkit'; import { createMockContext } from '@shopify/jest-koa-mocks'; import { REDACTED } from '../../../src/audit-trail'; @@ -1359,6 +1359,89 @@ describe('AuditTrailRoute', () => { return (context.response.body as { data: unknown[] }).data; }; + // In memory `!=` holds for a null and `null < 5` coerces to `0 < 5`, while the database leaves a + // NULL out of both: a record the caller could never read alive must not become readable once + // deleted. + describe('a captured null', () => { + const nullSecret = [ + { + operation: 'delete', + recordId: '2', + previousValues: { ownerId: null, secret: null }, + newValues: {}, + }, + ]; + const withheld = [ + { operation: 'delete', recordId: '2', previousValues: {}, newValues: {} }, + ]; + + test('withholds the values under a negated scope, as the database would have left the record out', async () => { + const notEqual = await historyUnder( + new ConditionTreeLeaf('secret', 'NotEqual', 'private'), + nullSecret, + ); + const notIn = await historyUnder( + new ConditionTreeLeaf('secret', 'NotIn', ['private']), + nullSecret, + ); + + expect([notEqual, notIn]).toEqual([withheld, withheld]); + }); + + test('withholds the values under an ordered scope a null would coerce through', async () => { + const data = await historyUnder( + new ConditionTreeLeaf('ownerId', 'LessThan', 5), + nullSecret, + ); + + expect(data).toEqual(withheld); + }); + + test('withholds when the negation sits inside a branch', async () => { + const scope = new ConditionTreeBranch('And', [ + new ConditionTreeLeaf('ownerId', 'Equal', 1), + new ConditionTreeLeaf('secret', 'NotEqual', 'private'), + ]); + + const data = await historyUnder(scope as unknown as ConditionTreeLeaf, [ + { operation: 'delete', recordId: '2', previousValues: { ownerId: 1, secret: null } }, + ]); + + expect(data).toEqual(withheld); + }); + + test('keeps the values a scope asking for the null itself covers', async () => { + const missing = await historyUnder( + new ConditionTreeLeaf('secret', 'Missing'), + nullSecret, + ); + const equalNull = await historyUnder( + new ConditionTreeLeaf('secret', 'Equal', null), + nullSecret, + ); + + expect([missing, equalNull]).toEqual([nullSecret, nullSecret]); + }); + + test('keeps a non-null value the negation covers', async () => { + const history = [ + { + operation: 'delete', + recordId: '2', + previousValues: { ownerId: 1, secret: 'open' }, + newValues: {}, + }, + ]; + + const data = await historyUnder( + new ConditionTreeLeaf('secret', 'NotEqual', 'private'), + history, + ); + + expect(data).toEqual(history); + }); + }); + test('withholds a delete row when the scope reads a column the snapshot never captured', async () => { const data = await historyUnder(new ConditionTreeLeaf('status', 'NotEqual', 'private'), [ { operation: 'delete', recordId: '2', previousValues: { ownerId: 1, secret: 'shh' } }, @@ -1991,11 +2074,11 @@ describe('AuditTrailRoute', () => { // 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('a genuinely gone record, read by a scoped caller', () => { - const reconstructFor = async (scope: ConditionTreeLeaf) => { + const reconstructFor = async (scope: ConditionTreeLeaf, status: string | null = 'closed') => { const history = [ { operation: 'delete', - previousValues: { id: 2, status: 'closed', name: 'Acme' }, + previousValues: { id: 2, status, name: 'Acme' }, newValues: {}, }, ]; @@ -2025,6 +2108,15 @@ describe('AuditTrailRoute', () => { expect(context.response.body).toEqual({ data: null }); }); + test('withholds a reconstruction whose NULL the scope negates, as the database would have refused the record', async () => { + const context = await reconstructFor( + new ConditionTreeLeaf('status', 'NotEqual', 'private'), + null, + ); + + expect(context.response.body).toEqual({ data: null }); + }); + test('serves the reconstruction when it matches the scope', async () => { const context = await reconstructFor(new ConditionTreeLeaf('status', 'Equal', 'closed')); From 62e02d469cae39feb47e16fa505f775f410b7261 Mon Sep 17 00:00:00 2001 From: Olivier Bex-Chauvet Date: Thu, 1 Oct 2026 10:08:47 +0200 Subject: [PATCH 6/6] test(audit-trail): cover a null under an In scope, with and without null in the list [PRD-1295] Co-Authored-By: Claude Opus 5.5 --- .../agent/test/routes/access/audit-trail.test.ts | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/packages/agent/test/routes/access/audit-trail.test.ts b/packages/agent/test/routes/access/audit-trail.test.ts index 80156dbba5..0b3e4b61f7 100644 --- a/packages/agent/test/routes/access/audit-trail.test.ts +++ b/packages/agent/test/routes/access/audit-trail.test.ts @@ -1419,8 +1419,21 @@ describe('AuditTrailRoute', () => { new ConditionTreeLeaf('secret', 'Equal', null), nullSecret, ); + const inWithNull = await historyUnder( + new ConditionTreeLeaf('secret', 'In', ['open', null]), + nullSecret, + ); - expect([missing, equalNull]).toEqual([nullSecret, nullSecret]); + expect([missing, equalNull, inWithNull]).toEqual([nullSecret, nullSecret, nullSecret]); + }); + + test('withholds the values under an In list that does not hold null', async () => { + const data = await historyUnder( + new ConditionTreeLeaf('secret', 'In', ['open']), + nullSecret, + ); + + expect(data).toEqual(withheld); }); test('keeps a non-null value the negation covers', async () => {