diff --git a/packages/agent/package.json b/packages/agent/package.json index 33387d53c8..b4e7af49b3 100644 --- a/packages/agent/package.json +++ b/packages/agent/package.json @@ -31,6 +31,8 @@ "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", diff --git a/packages/agent/src/audit-trail/README.md b/packages/agent/src/audit-trail/README.md index 2db24c713c..d389c1434b 100644 --- a/packages/agent/src/audit-trail/README.md +++ b/packages/agent/src/audit-trail/README.md @@ -16,8 +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. -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 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'; @@ -211,6 +211,22 @@ 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). + +**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, a field stored redacted — has no honest answer in the snapshot and the values are withheld rather @@ -310,11 +326,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: @@ -351,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 @@ -360,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 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..b414233502 --- /dev/null +++ b/packages/agent/src/audit-trail/earlier-life.ts @@ -0,0 +1,77 @@ +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 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. + +/** + * 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, + recordId: string, +): Promise { + const boundaries = await store.listByRecord({ + collection, + recordId, + operations: ['delete', 'create'], + order: 'desc', + }); + + // 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. + const boundary = + boundaries.find( + row => (row.operation === 'delete' || row.operation === 'create') && row.status !== 'pending', + ) ?? null; + + if (boundary?.operation !== 'create') return boundary; + + // 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. */ +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 f5e590afe2..fdba3055a3 100644 --- a/packages/agent/src/audit-trail/index.ts +++ b/packages/agent/src/audit-trail/index.ts @@ -10,13 +10,17 @@ export { createSqlAuditStore, ensureAuditStorage, defineAuditLogModel, - fieldsChangedCondition, - 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/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/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..9b454241c4 100644 --- a/packages/agent/src/audit-trail/sql-store.ts +++ b/packages/agent/src/audit-trail/sql-store.ts @@ -13,6 +13,7 @@ import type { Model, ModelStatic } from 'sequelize'; import { DataTypes, Op, Sequelize } from 'sequelize'; import { runAuditMigrations } from './migrations'; +import { fieldsChangedCondition, searchCondition } from './value-filters'; export const DEFAULT_SCHEMA = 'forest'; export const DEFAULT_TABLE = 'audit_logs'; @@ -92,105 +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; -} - -// 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. -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 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'), - ]; - - return Sequelize.literal( - `(${columns.map(column => `LOWER(${column}) LIKE LOWER(${pattern}) ESCAPE '~'`).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 17aef48285..78ccff0001 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 { lifeBoundaryOf, withholdEarlierLives } from '../../audit-trail/earlier-life'; import checkRecordVisibility, { recheckRecordVisibility, } from '../../audit-trail/record-visibility'; @@ -110,13 +111,23 @@ export default class AuditTrailCorrelationRoute extends BaseRoute { const gone = after ? after.goneEntirely : target.goneEntirely; - if (!target.permissionScope || !gone) return entries; - - return withholdOutsidePermissionScope(entries, { - collection: target.collectionObject, - permissionScope: target.permissionScope, - timezone: QueryStringParser.parseCaller(context, { defaultTimezone: 'UTC' }).timezone, - logger: this.options.logger, + if (!target.permissionScope) return entries; + + // Live now, but the rows before the current record took the id may be an earlier record's. + const boundary = gone + ? null + : 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, + }), }); } diff --git a/packages/agent/src/routes/access/audit-trail.ts b/packages/agent/src/routes/access/audit-trail.ts index 456ee5e331..565de0d072 100644 --- a/packages/agent/src/routes/access/audit-trail.ts +++ b/packages/agent/src/routes/access/audit-trail.ts @@ -16,7 +16,8 @@ import { ValidationError, } from '@forestadmin/datasource-toolkit'; -import { revertRecord } from '../../audit-trail'; +import { matchesServedValues, revertRecord } from '../../audit-trail'; +import { lifeBoundaryOf, withholdEarlierLives } 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,20 @@ 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 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 lifeBoundaryOf(store, this.collection.name, context.params.id) + : null; + + 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 || boundary) && filtersOnValues) { + await serveMatchedValues(withholdServed); return; } @@ -160,9 +186,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 || boundary) ? withholdServed(rawData) : rawData; context.response.body = { data, @@ -178,7 +203,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,8 +233,8 @@ export default class AuditTrailRoute extends CollectionRoute { ...(after && { after: { timestamp: after.timestamp, id: after.id } }), }); - const matched = this.withhold(rows, permissionScope, context).filter(entry => - AuditTrailRoute.matchesServedValues(entry, fields, search), + const matched = withholdServed(rows).filter(entry => + matchesServedValues(entry, { fields, search }), ); for (const entry of matched) { @@ -233,30 +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)), - ); - 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, @@ -324,6 +325,16 @@ export default class AuditTrailRoute extends CollectionRoute { const goneNow = after ? after.goneEntirely : goneEntirely; + // 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 boundary = + permissionScope && !goneNow + ? await lifeBoundaryOf(store, this.collection.name, context.params.id) + : null; + 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 // be kept rather than reverted (which would wrongly return the state just *before* it). @@ -362,7 +373,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/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', { 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..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), @@ -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', @@ -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); @@ -304,7 +330,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', @@ -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 0b3e4b61f7..880e840e2d 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' } }, @@ -1232,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', @@ -1250,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); @@ -1258,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 }], [], []); @@ -1284,10 +1303,163 @@ 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('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 }]); + + expect(context.throw).not.toHaveBeenCalled(); + expect((context.response.body as { data: unknown[] }).data).toEqual([ + { operation: 'delete', recordId: '2', previousValues: {}, newValues: {} }, + ]); + }); + }); + + 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('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, previousValues: {} }, + { ...earlier, previousValues: {}, newValues: {} }, + ]); + }); + + 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, 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, + 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(list).toHaveBeenCalledTimes(2); + expect(body.data).toEqual([later, current]); }); }); @@ -2086,6 +2258,84 @@ 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, + entries = history, + scope = new ConditionTreeLeaf('status', 'Equal', 'mine'), + ) => { + const { services, dataSource, store, route } = setupBooks(); + store.listByRecord.mockImplementation( + async ({ + startTimestamp, + operations, + }: { + startTimestamp?: string; + operations?: string[]; + }) => + entries.filter( + entry => + (!startTimestamp || entry.timestamp >= startTimestamp) && + (!operations || operations.includes(entry.operation)), + ), + ); + (services.authorization.getScope as jest.Mock).mockResolvedValue(scope); + 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' } }); + }); + + // 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', () => { const reconstructFor = async (scope: ConditionTreeLeaf, status: string | null = 'closed') => { const history = [ @@ -2100,7 +2350,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 +2413,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 +2469,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 +2494,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: { 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"