fix(audit-trail): close the remaining scope oracles and parity gaps with Ruby [PRD-1269]

packages/agent/src/audit-trail/README.md

@@ -212,6 +212,20 @@ 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 by a delete and taken since has two lives.** A live record in the caller's scope says
+nothing about the rows filed under its id before the id's last confirmed `delete`: they are an
+earlier record's. Those rows go through the same withholding as a record gone for good — on the
+history route (with `search` and `fields` matched against the served values, so the count and the
+authors cannot leak them either), on the correlation lookups, and on `/state`, whose reconstruction
+at an instant before that `delete` is tested against the scope (strictly before: at the delete's own
+instant the state already includes a replacement `create` sharing it). A `pending` delete frees nothing, since it may
+never have landed.
+
 That test only runs when the snapshot can actually answer it. The capture keeps the writable columns
 (plus the packed record id), so a scope reaching for anything else — a read-only column, a relation,
 a field stored redacted — has no honest answer in the snapshot and the values are withheld rather
@@ -311,11 +325,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:
 

packages/agent/src/audit-trail/earlier-life.ts

@@ -0,0 +1,33 @@
+import type { AuditRecord, AuditStore } from './types';
+
+// A delete frees the id, and a later record can take it. A live record under the caller's scope says
+// nothing about the rows filed under that id before its last delete: they are another record's, and
+// its values go through the same withholding as a record gone for good.
+
+/** The id's latest confirmed `delete`, or null when the id has never been freed. */
+export async function lastDeleteOf(
+  store: AuditStore,
+  collection: string,
+  recordId: string,
+): Promise<AuditRecord | null> {
+  const deletes = await store.listByRecord({
+    collection,
+    recordId,
+    operations: ['delete'],
+    order: 'desc',
+  });
+
+  // A pending delete may never have landed, so it frees nothing. Filtered here too, not just in
+  // the query: a store that ignores `operations` must not turn any row into the boundary.
+  return deletes.find(row => row.operation === 'delete' && row.status !== 'pending') ?? null;
+}
+
+/** Whether a row was filed at or before the boundary delete, in the trail's (timestamp, id) order. */
+export function belongsToEarlierLife(row: AuditRecord, lastDelete: AuditRecord | null): boolean {
+  if (!lastDelete) return false;
+
+  return (
+    row.timestamp < lastDelete.timestamp ||
+    (row.timestamp === lastDelete.timestamp && row.id <= lastDelete.id)
+  );
+}

packages/agent/src/audit-trail/index.ts

@@ -11,6 +11,7 @@ export {
   ensureAuditStorage,
   defineAuditLogModel,
   fieldsChangedCondition,
+  jsonEscaped,
   searchCondition,
   toRow,
   fromRow,

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<RecordVisibility | null> {
-  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 };
 }

packages/agent/src/audit-trail/sql-store.ts

@@ -12,6 +12,7 @@ import type { Model, ModelStatic } from 'sequelize';
 
 import { DataTypes, Op, Sequelize } from 'sequelize';
 
+import { REDACTED } from './instrument';
 import { runAuditMigrations } from './migrations';
 
 export const DEFAULT_SCHEMA = 'forest';
@@ -167,34 +168,46 @@ function jsonColumnAsText(sequelize: Sequelize, column: string): string {
   return column;
 }
 
+// What serializing the term would have produced, minus its own surrounding quotes.
+export function jsonEscaped(term: string): string {
+  return JSON.stringify(term).slice(1, -1);
+}
+
 // A free-text search against the *serialized* JSON text naturally covers "keys and scalar values
 // at any depth" without walking the structure by hand: both a key and a value appear as a quoted
 // literal substring of that text. This runs as a SQL WHERE clause (not an in-memory scan of
-// fetched rows), so it composes with pagination and COUNT the same way every other filter does. A
-// redacted value can't match a search for the real value — the real value already isn't in this
-// text, `redactValues` replaced it before the row was ever written.
+// fetched rows), so it composes with pagination and COUNT the same way every other filter does.
+//
+// Two things keep it honest about what the text holds. The term is escaped the way JSON wrote the
+// values, so `15" monitor` (stored as `15\" monitor`) is found, and a bare quote can't match the
+// document's own structure. And the redaction mask is removed before matching, so `search=redacted`
+// can't confirm which rows hold a masked value. Both mirror agent-ruby's `Sql::TextSearch`.
 export function searchCondition(sequelize: Sequelize, term: string) {
   // `~` rather than the standard `\`: MySQL/MariaDB treat backslash as a string-literal escape
   // character under the default sql_mode (no NO_BACKSLASH_ESCAPES), so a bare `ESCAPE '\'` is
   // 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'),
+  const like = (expression: string, text: string) => {
+    const escaped = text.replace(/[~%_]/g, char => `~${char}`);
+
+    return `LOWER(${expression}) LIKE LOWER(${sequelize.escape(`%${escaped}%`)}) ESCAPE '~'`;
+  };
+
+  const valuesAsText = (column: string) =>
+    `REPLACE(${jsonColumnAsText(sequelize, column)}, ${sequelize.escape(REDACTED)}, '')`;
+
+  const clauses = [
+    ...['action_name', 'user_first_name', 'user_last_name', 'user_email'].map(column =>
+      like(column, term),
+    ),
+    ...['previous_values', 'new_values'].map(column =>
+      like(valuesAsText(column), jsonEscaped(term)),
+    ),
   ];
 
-  return Sequelize.literal(
-    `(${columns.map(column => `LOWER(${column}) LIKE LOWER(${pattern}) ESCAPE '~'`).join(' OR ')})`,
-  );
+  return Sequelize.literal(`(${clauses.join(' OR ')})`);
 }
 
 function buildHistoryWhereClause(

packages/agent/src/routes/access/audit-trail-correlation.ts

@@ -7,6 +7,7 @@ import type { Context } from 'koa';
 
 import { ValidationError } from '@forestadmin/datasource-toolkit';
 
+import { belongsToEarlierLife, lastDeleteOf } from '../../audit-trail/earlier-life';
 import checkRecordVisibility, {
   recheckRecordVisibility,
 } from '../../audit-trail/record-visibility';
@@ -110,14 +111,27 @@ export default class AuditTrailCorrelationRoute extends BaseRoute {
 
     const gone = after ? after.goneEntirely : target.goneEntirely;
 
-    if (!target.permissionScope || !gone) return entries;
+    if (!target.permissionScope) return entries;
 
-    return withholdOutsidePermissionScope(entries, {
+    // Live now, but the rows up to the id's last delete may be an earlier record's.
+    const lastDelete = gone
+      ? null
+      : await lastDeleteOf(this.options.auditTrail.store, target.collection, target.recordId);
+
+    if (!gone && !lastDelete) return entries;
+
+    const withheld = withholdOutsidePermissionScope(entries, {
       collection: target.collectionObject,
       permissionScope: target.permissionScope,
       timezone: QueryStringParser.parseCaller(context, { defaultTimezone: 'UTC' }).timezone,
       logger: this.options.logger,
     });
+
+    return gone
+      ? withheld
+      : entries.map((entry, index) =>
+          belongsToEarlierLife(entry, lastDelete) ? withheld[index] : entry,
+        );
   }
 
   // Returns null (after issuing the 404) when a configured record-level permission scope excludes the id —

packages/agent/src/routes/access/audit-trail.ts

@@ -16,7 +16,8 @@ import {
   ValidationError,
 } from '@forestadmin/datasource-toolkit';
 
-import { revertRecord } from '../../audit-trail';
+import { REDACTED, jsonEscaped, revertRecord } from '../../audit-trail';
+import { belongsToEarlierLife, lastDeleteOf } from '../../audit-trail/earlier-life';
 import {
   parseDateBoundary,
   parseFields,
@@ -96,8 +97,8 @@ export default class AuditTrailRoute extends CollectionRoute {
 
     const filtersOnValues = Boolean(fields || search);
 
-    const serveMatchedValues = async (scope: ConditionTree) => {
-      const matched = await this.scanServedValues(context, scope, rowFilters, {
+    const serveMatchedValues = async (withholdServed: (rows: AuditRecord[]) => AuditRecord[]) => {
+      const matched = await this.scanServedValues(context, withholdServed, rowFilters, {
         fields,
         search,
         order,
@@ -116,8 +117,23 @@ export default class AuditTrailRoute extends CollectionRoute {
       };
     };
 
+    const withholdAll = (rows: AuditRecord[]) =>
+      permissionScope ? this.withhold(rows, permissionScope, context) : rows;
+
     if (permissionScope && goneEntirely && filtersOnValues) {
-      await serveMatchedValues(permissionScope);
+      await serveMatchedValues(withholdAll);
+
+      // Gone at the check, so everything stays withheld whatever the re-read finds; it is asked
+      // only for the 404, when the id has since been taken by a record this caller cannot read.
+      const recheck = await recheckRecordVisibility(
+        this.collection,
+        context.params.id,
+        context,
+        permissionScope,
+        goneEntirely,
+      );
+
+      if (recheck && !recheck.visible) context.throw(HttpCode.NotFound, 'Record does not exists');
 
       return;
     }
@@ -150,10 +166,27 @@ export default class AuditTrailRoute extends CollectionRoute {
 
     const gone = after ? after.goneEntirely : goneEntirely;
 
-    // Gone between the check and the read: this answer withholds, so the rows, count and authors
-    // matched in SQL above must not decide what is served either.
-    if (permissionScope && gone && filtersOnValues) {
-      await serveMatchedValues(permissionScope);
+    // Live now, but the id may have been freed by a delete and taken since: the rows up to that
+    // delete are the earlier record's, which the current one's scope says nothing about.
+    const lastDelete =
+      permissionScope && !gone
+        ? await lastDeleteOf(store, this.collection.name, context.params.id)
+        : null;
+
+    const withholdServed = gone
+      ? withholdAll
+      : (rows: AuditRecord[]) => {
+          const withheld = withholdAll(rows);
+
+          return rows.map((row, index) =>
+            belongsToEarlierLife(row, lastDelete) ? withheld[index] : row,
+          );
+        };
+
+    // Gone between the check and the read, or carrying an earlier life: this answer withholds, so
+    // the rows, count and authors matched in SQL above must not decide what is served either.
+    if (permissionScope && (gone || lastDelete) && filtersOnValues) {
+      await serveMatchedValues(withholdServed);
 
       return;
     }
@@ -162,9 +195,8 @@ export default class AuditTrailRoute extends CollectionRoute {
     // existence against — but create/update/delete rows still carry captured column values from
     // when the record existed. If those values themselves would have failed the caller's permission scope,
     // withhold them while still surfacing that the row happened, by whom and when: that part
-    // stays visible regardless.
-    const data =
-      permissionScope && gone ? this.withhold(rawData, permissionScope, context) : rawData;
+    // stays visible regardless. The same holds for an earlier record that held this id.
+    const data = permissionScope && (gone || lastDelete) ? withholdServed(rawData) : rawData;
 
     context.response.body = {
       data: data.map(({ previousRecordId, ...served }) => served),
@@ -180,7 +212,7 @@ export default class AuditTrailRoute extends CollectionRoute {
   // bounded at the instant the scan starts so an id taken since cannot keep it chasing new rows.
   private async scanServedValues(
     context: Context,
-    permissionScope: ConditionTree,
+    withholdServed: (rows: AuditRecord[]) => AuditRecord[],
     rowFilters: Omit<AuditHistoryQuery, 'fields' | 'search' | 'skip' | 'limit' | 'order' | 'after'>,
     {
       fields,
@@ -210,7 +242,7 @@ export default class AuditTrailRoute extends CollectionRoute {
         ...(after && { after: { timestamp: after.timestamp, id: after.id } }),
       });
 
-      const matched = this.withhold(rows, permissionScope, context).filter(entry =>
+      const matched = withholdServed(rows).filter(entry =>
         AuditTrailRoute.matchesServedValues(entry, fields, search),
       );
 
@@ -248,15 +280,19 @@ export default class AuditTrailRoute extends CollectionRoute {
       sides.some(values =>
         fields.some(field => Object.prototype.hasOwnProperty.call(values, field)),
       );
-    const texts = [
-      entry.actionName,
-      entry.userFirstName,
-      entry.userLastName,
-      entry.userEmail,
-      ...sides.map(values => JSON.stringify(values)),
-    ];
-
-    return touchesField && (!term || texts.some(text => text?.toLowerCase().includes(term)));
+
+    if (!touchesField) return false;
+    if (!term) return true;
+
+    const identity = [entry.actionName, entry.userFirstName, entry.userLastName, entry.userEmail];
+    const valueTerm = jsonEscaped(term);
+
+    return (
+      identity.some(text => text?.toLowerCase().includes(term)) ||
+      sides.some(values =>
+        JSON.stringify(values).split(REDACTED).join('').toLowerCase().includes(valueTerm),
+      )
+    );
   }
 
   private withhold(
@@ -326,6 +362,16 @@ export default class AuditTrailRoute extends CollectionRoute {
 
     const goneNow = after ? after.goneEntirely : goneEntirely;
 
+    // A live record whose id was freed by a delete and taken since: a state before that delete is
+    // the earlier record's, which the current one's scope says nothing about. Strictly before: at
+    // the delete's own instant the state already reflects every row there, including a replacement
+    // `create` sharing it.
+    const lastDelete =
+      permissionScope && !goneNow
+        ? await lastDeleteOf(store, this.collection.name, context.params.id)
+        : null;
+    const ofEarlierLife = Boolean(lastDelete && at < lastDelete.timestamp);
+
     // `startTimestamp` is an inclusive lower bound, so an entry timestamped exactly `at` comes
     // back too — but the record already reflects that entry's change at instant `at`, so it must
     // be kept rather than reverted (which would wrongly return the state just *before* it).
@@ -364,7 +410,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.