From fe0d10ab35900d82bcbd19ebb5acc5f6c9dcd9e6 Mon Sep 17 00:00:00 2001 From: Olivier Bex-Chauvet Date: Mon, 21 Sep 2026 17:23:26 +0200 Subject: [PATCH 1/4] feat(audit-trail): filter the per-record history by operation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `GET /forest/_audit-trail/{collection}/{recordId}` accepts an `operation` query param: comma-separated, from the closed set the agent stores (`create`, `update`, `delete`, `action`, `action_failed`). An unrecognized value returns 400 rather than being dropped. A silently ignored filter returns unfiltered rows into a list the caller believes is filtered — the failure mode this param exists to avoid. Pushed into the store query rather than applied to the fetched page, so `meta.count` and `meta.availableUsers` agree with the rows and paging stays correct. Co-Authored-By: Claude Opus 5 (1M context) --- packages/agent/src/audit-trail/README.md | 4 + packages/agent/src/audit-trail/sql-store.ts | 2 + packages/agent/src/audit-trail/types.ts | 2 + .../agent/src/routes/access/audit-trail.ts | 37 +++++++- .../agent/test/audit-trail/in-memory-store.ts | 2 + .../agent/test/audit-trail/sql-store.test.ts | 25 +++++ .../test/routes/access/audit-trail.test.ts | 92 +++++++++++++++++++ 7 files changed, 162 insertions(+), 2 deletions(-) diff --git a/packages/agent/src/audit-trail/README.md b/packages/agent/src/audit-trail/README.md index badd18c4f8..7ab5155326 100644 --- a/packages/agent/src/audit-trail/README.md +++ b/packages/agent/src/audit-trail/README.md @@ -256,6 +256,7 @@ Optional filters (all combine with `AND`; omitting them keeps the full history): | query param | format | effect | | ----------- | ------------------------------------------ | -------------------------------------------------- | | `userIds` | comma-separated integers `12,45` | keep only entries whose `userId` is in the list | +| `operation` | comma-separated operations `create,delete` | keep only entries recorded under one of them | | `startDate` | `YYYY-MM-DD` or datetime (incl.) | keep entries from this lower bound onward | | `endDate` | `YYYY-MM-DD` or datetime (incl.) | keep entries up to this upper bound | | `fields` | comma-separated column names `city,street` | keep entries whose change touched at least one | @@ -299,6 +300,9 @@ Defensive parsing: - `userIds`: non-numeric tokens are dropped (`12,abc,45` → `12,45`); if nothing numeric remains the filter is ignored. +- `operation`: the set is closed (`create`, `update`, `delete`, `action`, `action_failed`), and an + unrecognized value returns **HTTP 400** rather than being dropped — a silently ignored filter + returns unfiltered rows into a list the caller believes is filtered. - `startDate` / `endDate`: a value matching none of the accepted formats returns **HTTP 400**. **Pagination** follows JSON:API: `page[number]` is 1-based (default `1`) and `page[size]` defaults diff --git a/packages/agent/src/audit-trail/sql-store.ts b/packages/agent/src/audit-trail/sql-store.ts index d2f4453549..fe588cf8a3 100644 --- a/packages/agent/src/audit-trail/sql-store.ts +++ b/packages/agent/src/audit-trail/sql-store.ts @@ -194,6 +194,7 @@ function buildHistoryWhereClause( collection, recordId, userIds, + operations, startTimestamp, endTimestamp, fields, @@ -204,6 +205,7 @@ function buildHistoryWhereClause( const where: Record = { collection, recordId }; if (userIds) where.userId = { [Op.in]: userIds }; + if (operations?.length) where.operation = { [Op.in]: operations }; const timestampRange: Record = {}; if (startTimestamp) timestampRange[Op.gte] = new Date(startTimestamp); diff --git a/packages/agent/src/audit-trail/types.ts b/packages/agent/src/audit-trail/types.ts index c82ca7a90e..2850fd2272 100644 --- a/packages/agent/src/audit-trail/types.ts +++ b/packages/agent/src/audit-trail/types.ts @@ -44,6 +44,8 @@ export type AuditHistoryQuery = { skip?: number; limit?: number; userIds?: number[]; + /** Keep only entries recorded under one of these operations. */ + operations?: AuditOperation[]; /** Inclusive lower bound on `timestamp` as a UTC ISO instant. */ startTimestamp?: string; /** Inclusive upper bound on `timestamp` as a UTC ISO instant. */ diff --git a/packages/agent/src/routes/access/audit-trail.ts b/packages/agent/src/routes/access/audit-trail.ts index 6742019fcf..75772fde59 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 { AuditOperation, AuditRecord } from '../../audit-trail'; import type { CollectionSchema, ConditionTree } from '@forestadmin/datasource-toolkit'; import type Router from '@koa/router'; import type { Context } from 'koa'; @@ -34,8 +34,17 @@ const ISO_INSTANT = /[Zz]$|[+-]\d{2}:?\d{2}$/; const DEFAULT_PAGE_SIZE = 20; const MAX_PAGE_SIZE = 100; +const AUDIT_OPERATIONS: readonly AuditOperation[] = [ + 'create', + 'update', + 'delete', + 'action', + 'action_failed', +]; + type AuditHistoryFilters = { userIds?: number[]; + operations?: AuditOperation[]; startTimestamp?: string; endTimestamp?: string; fields?: string[]; @@ -68,13 +77,14 @@ export default class AuditTrailRoute extends CollectionRoute { const { store } = this.options.auditTrail; const { skip, limit } = AuditTrailRoute.parsePagination(context); const order = AuditTrailRoute.parseSort(context); - const { userIds, startTimestamp, endTimestamp, fields, search } = + const { userIds, operations, startTimestamp, endTimestamp, fields, search } = AuditTrailRoute.parseFilters(context); const filters = { collection: this.collection.name, recordId: context.params.id, ...(userIds && { userIds }), + ...(operations && { operations }), ...(startTimestamp && { startTimestamp }), ...(endTimestamp && { endTimestamp }), ...(fields && { fields }), @@ -341,6 +351,7 @@ export default class AuditTrailRoute extends CollectionRoute { return { userIds: AuditTrailRoute.parseUserIds(query.userIds?.toString()), + operations: AuditTrailRoute.parseOperations(query.operation?.toString()), startTimestamp: AuditTrailRoute.parseDateBoundary( query.startDate?.toString(), timezone, @@ -365,6 +376,28 @@ export default class AuditTrailRoute extends CollectionRoute { return ids.length > 0 ? ids : undefined; } + // Comma-separated, from a closed set. An unrecognized value is rejected rather than dropped: a + // silently ignored filter returns unfiltered rows into a list the caller believes is filtered, + // which is worse than an error. + private static parseOperations(raw?: string): AuditOperation[] | undefined { + if (!raw) return undefined; + + const tokens = raw + .split(',') + .map(token => token.trim()) + .filter(token => token.length > 0); + + const unknown = tokens.find(token => !AUDIT_OPERATIONS.includes(token as AuditOperation)); + + if (unknown) { + throw new ValidationError( + `Invalid operation: "${unknown}" (expected one of ${AUDIT_OPERATIONS.join(', ')})`, + ); + } + + return tokens.length > 0 ? (tokens as AuditOperation[]) : undefined; + } + private static parseFields(raw?: string): string[] | undefined { if (!raw) return undefined; diff --git a/packages/agent/test/audit-trail/in-memory-store.ts b/packages/agent/test/audit-trail/in-memory-store.ts index b6c16477d5..43b943d007 100644 --- a/packages/agent/test/audit-trail/in-memory-store.ts +++ b/packages/agent/test/audit-trail/in-memory-store.ts @@ -111,6 +111,7 @@ export default class InMemoryAuditStore implements AuditStore { collection, recordId, userIds, + operations, startTimestamp, endTimestamp, order = 'asc', @@ -122,6 +123,7 @@ export default class InMemoryAuditStore implements AuditStore { 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)); diff --git a/packages/agent/test/audit-trail/sql-store.test.ts b/packages/agent/test/audit-trail/sql-store.test.ts index 4e2e744dc5..1d2adda1ec 100644 --- a/packages/agent/test/audit-trail/sql-store.test.ts +++ b/packages/agent/test/audit-trail/sql-store.test.ts @@ -254,6 +254,31 @@ describe('createSqlAuditStore (sqlite round-trip)', () => { await close(); }); + it('filters by operation at the query level', async () => { + const { store, close } = createSqlAuditStore({ connectionString: 'sqlite::memory:' }); + + await seed(store, record({ recordId: '1', operation: 'create' })); + await seed(store, record({ recordId: '1', operation: 'update' })); + await seed(store, record({ recordId: '1', operation: 'delete' })); + + const history = await store.listByRecord({ + collection: 'accounts', + recordId: '1', + operations: ['create', 'delete'], + }); + + expect(history.map(r => r.operation)).toEqual(['create', 'delete']); + expect( + await store.countByRecord({ + collection: 'accounts', + recordId: '1', + operations: ['create', 'delete'], + }), + ).toBe(2); + + await close(); + }); + it('filters by the inclusive startTimestamp/endTimestamp range at the query level', 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..3e6d56bac5 100644 --- a/packages/agent/test/routes/access/audit-trail.test.ts +++ b/packages/agent/test/routes/access/audit-trail.test.ts @@ -285,6 +285,98 @@ describe('AuditTrailRoute', () => { ); }); + test('forwards a single operation filter to the store', async () => { + const { services, dataSource, options, store } = setup(); + const route = new AuditTrailRoute(services, options, dataSource, 'books'); + const context = createMockContext({ + state: { user: { email: 'john.doe@domain.com' } }, + customProperties: { + query: { timezone: 'Europe/Paris', operation: 'delete' }, + params: { id: '2' }, + }, + }); + + await route.handleHistory(context); + + expect(store.listByRecord).toHaveBeenCalledWith( + expect.objectContaining({ operations: ['delete'] }), + ); + }); + + test('forwards a comma-separated operation filter as a list', async () => { + const { services, dataSource, options, store } = setup(); + const route = new AuditTrailRoute(services, options, dataSource, 'books'); + const context = createMockContext({ + state: { user: { email: 'john.doe@domain.com' } }, + customProperties: { + query: { timezone: 'Europe/Paris', operation: 'create, action_failed' }, + params: { id: '2' }, + }, + }); + + await route.handleHistory(context); + + expect(store.listByRecord).toHaveBeenCalledWith( + expect.objectContaining({ operations: ['create', 'action_failed'] }), + ); + }); + + test('applies the operation filter to the count and the author list too', async () => { + const { services, dataSource, options, store } = setup(); + const route = new AuditTrailRoute(services, options, dataSource, 'books'); + const context = createMockContext({ + state: { user: { email: 'john.doe@domain.com' } }, + customProperties: { + query: { timezone: 'Europe/Paris', operation: 'update' }, + params: { id: '2' }, + }, + }); + + await route.handleHistory(context); + + expect(store.countByRecord).toHaveBeenCalledWith( + expect.objectContaining({ operations: ['update'] }), + ); + expect(store.listDistinctUsers).toHaveBeenCalledWith( + expect.objectContaining({ operations: ['update'] }), + ); + }); + + test('rejects an unrecognized operation rather than ignoring the filter', async () => { + const { services, dataSource, options, store } = setup(); + const route = new AuditTrailRoute(services, options, dataSource, 'books'); + const context = createMockContext({ + state: { user: { email: 'john.doe@domain.com' } }, + customProperties: { + query: { timezone: 'Europe/Paris', operation: 'create,destroy' }, + params: { id: '2' }, + }, + }); + + await expect(route.handleHistory(context)).rejects.toThrow( + 'Invalid operation: "destroy" (expected one of create, update, delete, action, action_failed)', + ); + expect(store.listByRecord).not.toHaveBeenCalled(); + }); + + test('omits the operation filter when the parameter is empty', async () => { + const { services, dataSource, options, store } = setup(); + const route = new AuditTrailRoute(services, options, dataSource, 'books'); + const context = createMockContext({ + state: { user: { email: 'john.doe@domain.com' } }, + customProperties: { + query: { timezone: 'Europe/Paris', operation: '' }, + params: { id: '2' }, + }, + }); + + await route.handleHistory(context); + + expect(store.listByRecord).toHaveBeenCalledWith( + expect.not.objectContaining({ operations: expect.anything() }), + ); + }); + test('converts startDate/endDate to inclusive UTC instants in the request timezone', async () => { const { services, dataSource, options, store } = setup(); const route = new AuditTrailRoute(services, options, dataSource, 'books'); From 85f7a702ee434805829856162fcc51969c9f63de Mon Sep 17 00:00:00 2001 From: Olivier Bex-Chauvet Date: Fri, 25 Sep 2026 15:34:26 +0200 Subject: [PATCH 2/4] fix(audit-trail): match search and fields against served values on a gone scoped record Matched in SQL, `search` and `fields` test the values as captured, so the rows, `meta.count` and `availableUsers` would reveal whether a withheld value holds the term. For a record gone under a permission scope, the history is read without those two filters and matched and paged in memory against the values served. Co-Authored-By: Claude Opus 5.5 --- packages/agent/src/audit-trail/README.md | 6 ++ .../agent/src/routes/access/audit-trail.ts | 82 ++++++++++++++-- .../test/routes/access/audit-trail.test.ts | 96 +++++++++++++++++++ 3 files changed, 175 insertions(+), 9 deletions(-) diff --git a/packages/agent/src/audit-trail/README.md b/packages/agent/src/audit-trail/README.md index 7ab5155326..b59e3ff039 100644 --- a/packages/agent/src/audit-trail/README.md +++ b/packages/agent/src/audit-trail/README.md @@ -289,6 +289,12 @@ 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 and matched and paged in +memory, which only a gone record's history pays for. + 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/routes/access/audit-trail.ts b/packages/agent/src/routes/access/audit-trail.ts index 75772fde59..6b429d3a56 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 { AuditOperation, AuditRecord } from '../../audit-trail'; +import type { AuditOperation, AuditRecord, AuditUserSummary } from '../../audit-trail'; import type { CollectionSchema, ConditionTree } from '@forestadmin/datasource-toolkit'; import type Router from '@koa/router'; import type { Context } from 'koa'; @@ -80,16 +80,15 @@ export default class AuditTrailRoute extends CollectionRoute { const { userIds, operations, startTimestamp, endTimestamp, fields, search } = AuditTrailRoute.parseFilters(context); - const filters = { + const rowFilters = { collection: this.collection.name, recordId: context.params.id, ...(userIds && { userIds }), ...(operations && { operations }), ...(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 @@ -97,12 +96,20 @@ export default class AuditTrailRoute extends CollectionRoute { const isFirstFetch = (context.request.query as Record)['page[number]'] === undefined; + // 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 below hides — one probe per + // character. For a record gone at the check they are matched against the values served instead, + // which means paging the whole history here. One in scope then was the caller's to read whole. + const matchServedValues = Boolean(permissionScope && goneEntirely && (fields || search)); + // `count` reflects the active filters and is independent of the page. - const [rawData, count, availableUsers] = await Promise.all([ - store.listByRecord({ ...filters, skip, limit, order }), - store.countByRecord(filters), - isFirstFetch ? store.listDistinctUsers(filters) : undefined, - ]); + const [rawData, count, availableUsers] = matchServedValues + ? [await store.listByRecord({ ...rowFilters, order }), undefined, undefined] + : await Promise.all([ + store.listByRecord({ ...filters, skip, limit, order }), + store.countByRecord(filters), + isFirstFetch ? store.listDistinctUsers(filters) : undefined, + ]); // The record can be deleted — or moved out of the caller's permission scope — between the check above and // the audit read: the audit trail lives in its own database, often its own engine, so no single @@ -133,12 +140,69 @@ export default class AuditTrailRoute extends CollectionRoute { const data = permissionScope && gone ? this.withhold(rawData, permissionScope, context) : rawData; + if (matchServedValues) { + const matched = data.filter(entry => + AuditTrailRoute.matchesServedValues(entry, fields, search), + ); + + context.response.body = { + data: matched.slice(skip, skip + limit), + meta: { + count: matched.length, + ...(isFirstFetch && { availableUsers: AuditTrailRoute.authorsOf(matched) }), + }, + }; + + return; + } + context.response.body = { data, meta: { count, ...(availableUsers && { availableUsers }) }, }; } + // 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 static authorsOf(entries: AuditRecord[]): AuditUserSummary[] { + const byUser = new Map(); + + entries.forEach(entry => { + if (byUser.has(entry.userId)) return; + + byUser.set(entry.userId, { + id: entry.userId, + firstName: entry.userFirstName, + lastName: entry.userLastName, + email: entry.userEmail, + }); + }); + + return [...byUser.values()]; + } + private withhold( entries: AuditRecord[], 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 3e6d56bac5..1cdb5b696f 100644 --- a/packages/agent/test/routes/access/audit-trail.test.ts +++ b/packages/agent/test/routes/access/audit-trail.test.ts @@ -840,6 +840,102 @@ 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 = (userEmail = 'jane@acme.io') => ({ + operation: 'delete', + recordId: '2', + userId: 7, + userFirstName: null, + userLastName: null, + userEmail, + actionName: null, + previousValues: { ownerId: 2, title: 'Secret' }, + newValues: {}, + }); + + 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 kept = { ...secretDelete(), previousValues: { ownerId: 1, title: 'Mine' } }; + + 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('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 and pages what matched', async () => { + const kept = { ...secretDelete(), previousValues: { ownerId: 1, title: 'Mine' } }; + + const { body, store } = await searched([kept, { ...kept, operation: 'update' }], { + search: 'mine', + userIds: '12', + 'page[size]': '1', + 'page[number]': '2', + }); + + expect(store.listByRecord).toHaveBeenCalledWith({ + collection: 'books', + recordId: '2', + userIds: [12], + order: 'desc', + }); + expect(store.countByRecord).not.toHaveBeenCalled(); + expect(body).toEqual({ data: [{ ...kept, operation: 'update' }], meta: { count: 2 } }); + }); + }); + test("keeps a genuinely-deleted delete row's previousValues when they match the caller's scope", async () => { const history = [ { From 1196c0d504570192d150d4e4f3237ae66651ce0d Mon Sep 17 00:00:00 2001 From: Olivier Bex-Chauvet Date: Fri, 25 Sep 2026 15:43:28 +0200 Subject: [PATCH 3/4] fix(audit-trail): bound the served-value search and apply it to a record deleted mid-request A record visible at the check but gone by the recheck had its rows, count and authors matched in SQL on captured values; that answer is now discarded and re-read against the served values. The served-value read runs in batches of 500, keeping only the requested page and the authors in memory. Co-Authored-By: Claude Opus 5.5 --- packages/agent/src/audit-trail/README.md | 5 +- .../agent/src/routes/access/audit-trail.ts | 136 ++++++++++++------ .../test/routes/access/audit-trail.test.ts | 69 +++++++++ 3 files changed, 166 insertions(+), 44 deletions(-) diff --git a/packages/agent/src/audit-trail/README.md b/packages/agent/src/audit-trail/README.md index b59e3ff039..d91bb0427f 100644 --- a/packages/agent/src/audit-trail/README.md +++ b/packages/agent/src/audit-trail/README.md @@ -292,8 +292,9 @@ 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 and matched and paged in -memory, which only a gone record's history pays for. +withheld value holds the term. The rows are read without those two filters, in batches of 500, and +matched and paged in memory, keeping only the requested page and the authors. That holds too for a +record deleted while the request was in flight, whose SQL-matched answer is discarded and re-read. 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 6b429d3a56..57d1630e96 100644 --- a/packages/agent/src/routes/access/audit-trail.ts +++ b/packages/agent/src/routes/access/audit-trail.ts @@ -1,4 +1,9 @@ -import type { AuditOperation, AuditRecord, AuditUserSummary } from '../../audit-trail'; +import type { + AuditHistoryQuery, + AuditOperation, + 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 +38,7 @@ const ISO_INSTANT = /[Zz]$|[+-]\d{2}:?\d{2}$/; const DEFAULT_PAGE_SIZE = 20; const MAX_PAGE_SIZE = 100; +const SERVED_MATCH_BATCH_SIZE = 500; const AUDIT_OPERATIONS: readonly AuditOperation[] = [ 'create', @@ -51,6 +57,14 @@ type AuditHistoryFilters = { search?: string; }; +type ServedMatchQuery = Pick & { + rowFilters: AuditHistoryQuery; + order: 'asc' | 'desc'; + skip: number; + limit: number; + isFirstFetch: boolean; +}; + export default class AuditTrailRoute extends CollectionRoute { setupRoutes(router: Router): void { router.get(`/_audit-trail/${this.collectionUrlSlug}/:id`, this.handleHistory.bind(this)); @@ -98,18 +112,26 @@ 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 below hides — one probe per - // character. For a record gone at the check they are matched against the values served instead, - // which means paging the whole history here. One in scope then was the caller's to read whole. - const matchServedValues = Boolean(permissionScope && goneEntirely && (fields || search)); + // character. For a gone record they are matched against the values served instead. + const filtersOnValues = Boolean(fields || search); + const servedMatchQuery = { rowFilters, order, fields, search, skip, limit, isFirstFetch }; + + if (permissionScope && filtersOnValues && goneEntirely) { + context.response.body = await this.listServedMatches( + servedMatchQuery, + permissionScope, + context, + ); + + return; + } // `count` reflects the active filters and is independent of the page. - const [rawData, count, availableUsers] = matchServedValues - ? [await store.listByRecord({ ...rowFilters, order }), undefined, undefined] - : await Promise.all([ - store.listByRecord({ ...filters, skip, limit, order }), - store.countByRecord(filters), - isFirstFetch ? store.listDistinctUsers(filters) : undefined, - ]); + const [rawData, count, availableUsers] = await Promise.all([ + store.listByRecord({ ...filters, skip, limit, order }), + store.countByRecord(filters), + isFirstFetch ? store.listDistinctUsers(filters) : undefined, + ]); // The record can be deleted — or moved out of the caller's permission scope — between the check above and // the audit read: the audit trail lives in its own database, often its own engine, so no single @@ -132,6 +154,17 @@ export default class AuditTrailRoute extends CollectionRoute { const gone = after ? after.goneEntirely : goneEntirely; + // Gone between the check and the read: the rows, count and authors above were matched in SQL. + if (permissionScope && filtersOnValues && gone) { + context.response.body = await this.listServedMatches( + servedMatchQuery, + permissionScope, + context, + ); + + 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, @@ -140,25 +173,61 @@ export default class AuditTrailRoute extends CollectionRoute { const data = permissionScope && gone ? this.withhold(rawData, permissionScope, context) : rawData; - if (matchServedValues) { - const matched = data.filter(entry => + context.response.body = { + data, + meta: { count, ...(availableUsers && { availableUsers }) }, + }; + } + + // Read in batches so a gone record's history never sits in memory whole: only the requested + // page and the distinct authors are kept while the count runs over every match. + private async listServedMatches( + { rowFilters, order, fields, search, skip, limit, isFirstFetch }: ServedMatchQuery, + permissionScope: ConditionTree, + context: Context, + ): Promise<{ + data: AuditRecord[]; + meta: { count: number; availableUsers?: AuditUserSummary[] }; + }> { + const { store } = this.options.auditTrail; + const page: AuditRecord[] = []; + const authors = new Map(); + let count = 0; + + for (let offset = 0; ; offset += SERVED_MATCH_BATCH_SIZE) { + // Sequential on purpose: batches are held one at a time. + // eslint-disable-next-line no-await-in-loop + const batch = await store.listByRecord({ + ...rowFilters, + order, + skip: offset, + limit: SERVED_MATCH_BATCH_SIZE, + }); + + const matched = this.withhold(batch, permissionScope, context).filter(entry => AuditTrailRoute.matchesServedValues(entry, fields, search), ); + const pageStart = Math.max(0, skip - count); + + page.push(...matched.slice(pageStart, pageStart + limit - page.length)); + matched.forEach(entry => { + if (authors.has(entry.userId)) return; + + authors.set(entry.userId, { + id: entry.userId, + firstName: entry.userFirstName, + lastName: entry.userLastName, + email: entry.userEmail, + }); + }); + count += matched.length; - context.response.body = { - data: matched.slice(skip, skip + limit), - meta: { - count: matched.length, - ...(isFirstFetch && { availableUsers: AuditTrailRoute.authorsOf(matched) }), - }, - }; - - return; + if (batch.length < SERVED_MATCH_BATCH_SIZE) break; } - context.response.body = { - data, - meta: { count, ...(availableUsers && { availableUsers }) }, + return { + data: page, + meta: { count, ...(isFirstFetch && { availableUsers: [...authors.values()] }) }, }; } @@ -186,23 +255,6 @@ export default class AuditTrailRoute extends CollectionRoute { return touchesField && (!term || texts.some(text => text?.toLowerCase().includes(term))); } - private static authorsOf(entries: AuditRecord[]): AuditUserSummary[] { - const byUser = new Map(); - - entries.forEach(entry => { - if (byUser.has(entry.userId)) return; - - byUser.set(entry.userId, { - id: entry.userId, - firstName: entry.userFirstName, - lastName: entry.userLastName, - email: entry.userEmail, - }); - }); - - return [...byUser.values()]; - } - private withhold( entries: AuditRecord[], 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 1cdb5b696f..27dc7e52d3 100644 --- a/packages/agent/test/routes/access/audit-trail.test.ts +++ b/packages/agent/test/routes/access/audit-trail.test.ts @@ -930,10 +930,79 @@ describe('AuditTrailRoute', () => { recordId: '2', userIds: [12], order: 'desc', + skip: 0, + limit: 500, }); expect(store.countByRecord).not.toHaveBeenCalled(); expect(body).toEqual({ data: [{ ...kept, operation: 'update' }], meta: { count: 2 } }); }); + + test('reads the history in batches and counts every match across them', async () => { + const kept = { ...secretDelete(), previousValues: { ownerId: 1, title: 'Mine' } }; + const { services, dataSource, options, store } = setup(); + store.listByRecord + .mockResolvedValueOnce(Array.from({ length: 500 }, () => kept)) + .mockResolvedValueOnce([kept]); + (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', search: 'mine', 'page[size]': '1' }, + params: { id: '2' }, + }, + }); + + await route.handleHistory(context); + + expect(store.listByRecord).toHaveBeenNthCalledWith( + 2, + expect.objectContaining({ skip: 500, limit: 500 }), + ); + expect(context.response.body).toEqual({ + data: [kept], + meta: { + count: 501, + availableUsers: [{ id: 7, firstName: null, lastName: null, email: 'jane@acme.io' }], + }, + }); + }); + + 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', + skip: 0, + limit: 500, + }); + expect(context.response.body).toEqual({ + data: [], + meta: { count: 0, availableUsers: [] }, + }); + }); }); test("keeps a genuinely-deleted delete row's previousValues when they match the caller's scope", async () => { From d53344718e57ab095863d063091f8308ad746cf2 Mon Sep 17 00:00:00 2001 From: Olivier Bex-Chauvet Date: Wed, 30 Sep 2026 15:29:37 +0200 Subject: [PATCH 4/4] refactor(audit-trail): name the history filter `operations`, as it takes a list The parameter accepts a comma-separated set and is read into `operations` everywhere behind the route, so the singular on the wire was the odd one out and read as though one value were all it would take. Nothing consumes it yet outside the front's own branch, so the rename costs no compatibility. Co-Authored-By: Claude Opus 5 (1M context) --- packages/agent/src/audit-trail/README.md | 2 +- packages/agent/src/routes/access/audit-trail.ts | 2 +- packages/agent/test/routes/access/audit-trail.test.ts | 10 +++++----- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/agent/src/audit-trail/README.md b/packages/agent/src/audit-trail/README.md index d91bb0427f..b9edc60f90 100644 --- a/packages/agent/src/audit-trail/README.md +++ b/packages/agent/src/audit-trail/README.md @@ -256,7 +256,7 @@ Optional filters (all combine with `AND`; omitting them keeps the full history): | query param | format | effect | | ----------- | ------------------------------------------ | -------------------------------------------------- | | `userIds` | comma-separated integers `12,45` | keep only entries whose `userId` is in the list | -| `operation` | comma-separated operations `create,delete` | keep only entries recorded under one of them | +| `operations` | comma-separated operations `create,delete` | keep only entries recorded under one of them | | `startDate` | `YYYY-MM-DD` or datetime (incl.) | keep entries from this lower bound onward | | `endDate` | `YYYY-MM-DD` or datetime (incl.) | keep entries up to this upper bound | | `fields` | comma-separated column names `city,street` | keep entries whose change touched at least one | diff --git a/packages/agent/src/routes/access/audit-trail.ts b/packages/agent/src/routes/access/audit-trail.ts index 57d1630e96..5fddecb640 100644 --- a/packages/agent/src/routes/access/audit-trail.ts +++ b/packages/agent/src/routes/access/audit-trail.ts @@ -467,7 +467,7 @@ export default class AuditTrailRoute extends CollectionRoute { return { userIds: AuditTrailRoute.parseUserIds(query.userIds?.toString()), - operations: AuditTrailRoute.parseOperations(query.operation?.toString()), + operations: AuditTrailRoute.parseOperations(query.operations?.toString()), startTimestamp: AuditTrailRoute.parseDateBoundary( query.startDate?.toString(), timezone, diff --git a/packages/agent/test/routes/access/audit-trail.test.ts b/packages/agent/test/routes/access/audit-trail.test.ts index 27dc7e52d3..57b44951d4 100644 --- a/packages/agent/test/routes/access/audit-trail.test.ts +++ b/packages/agent/test/routes/access/audit-trail.test.ts @@ -291,7 +291,7 @@ describe('AuditTrailRoute', () => { const context = createMockContext({ state: { user: { email: 'john.doe@domain.com' } }, customProperties: { - query: { timezone: 'Europe/Paris', operation: 'delete' }, + query: { timezone: 'Europe/Paris', operations: 'delete' }, params: { id: '2' }, }, }); @@ -309,7 +309,7 @@ describe('AuditTrailRoute', () => { const context = createMockContext({ state: { user: { email: 'john.doe@domain.com' } }, customProperties: { - query: { timezone: 'Europe/Paris', operation: 'create, action_failed' }, + query: { timezone: 'Europe/Paris', operations: 'create, action_failed' }, params: { id: '2' }, }, }); @@ -327,7 +327,7 @@ describe('AuditTrailRoute', () => { const context = createMockContext({ state: { user: { email: 'john.doe@domain.com' } }, customProperties: { - query: { timezone: 'Europe/Paris', operation: 'update' }, + query: { timezone: 'Europe/Paris', operations: 'update' }, params: { id: '2' }, }, }); @@ -348,7 +348,7 @@ describe('AuditTrailRoute', () => { const context = createMockContext({ state: { user: { email: 'john.doe@domain.com' } }, customProperties: { - query: { timezone: 'Europe/Paris', operation: 'create,destroy' }, + query: { timezone: 'Europe/Paris', operations: 'create,destroy' }, params: { id: '2' }, }, }); @@ -365,7 +365,7 @@ describe('AuditTrailRoute', () => { const context = createMockContext({ state: { user: { email: 'john.doe@domain.com' } }, customProperties: { - query: { timezone: 'Europe/Paris', operation: '' }, + query: { timezone: 'Europe/Paris', operations: '' }, params: { id: '2' }, }, });