From 05cba8362342ca7cb9dd189c064565211edc0fcd Mon Sep 17 00:00:00 2001 From: Olivier Bex-Chauvet Date: Wed, 30 Sep 2026 17:55:39 +0200 Subject: [PATCH] feat(audit-trail): record the id a primary key moved from, and judge each side by it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An update carries two states but files under one id, so a writable primary key that is also redacted left the previous side with nothing to answer a permission scope: #1909 withheld it rather than judging it by the id the record ended up with. Correct, and lossy — a caller squarely in scope saw nothing. The row now carries `previous_record_id`, written on every confirmed update, and each side is judged against the id it actually had. Written whether or not the key moved, which is where this departs from agent-ruby#394: that table had never shipped, so a null there can only mean the key held still. Here a null has to keep meaning "written before this column existed", or a row from an older agent whose key did move would be judged by the id it moved to — the leak #1909 closed. For the same reason the column arrives as its own migration rather than an edit to 001. A pending update's new side still answers with what it captured: the row is filed under the id the record had before the write, which says nothing about the state it was moving to. Co-Authored-By: Claude Opus 5 (1M context) --- packages/agent/src/audit-trail/README.md | 1 + packages/agent/src/audit-trail/instrument.ts | 4 ++ packages/agent/src/audit-trail/migrations.ts | 32 ++++++++++ packages/agent/src/audit-trail/sql-store.ts | 4 ++ packages/agent/src/audit-trail/types.ts | 18 +++++- packages/agent/src/audit-trail/withhold.ts | 38 ++++++++---- .../agent/src/routes/access/audit-trail.ts | 9 ++- .../test/audit-trail/in-memory-store.test.ts | 1 + .../agent/test/audit-trail/in-memory-store.ts | 2 +- .../agent/test/audit-trail/instrument.test.ts | 20 +++++++ .../agent/test/audit-trail/migrations.test.ts | 13 +++- .../test/routes/access/audit-trail.test.ts | 59 +++++++++++++++++++ 12 files changed, 181 insertions(+), 20 deletions(-) diff --git a/packages/agent/src/audit-trail/README.md b/packages/agent/src/audit-trail/README.md index b9edc60f90..39524acbac 100644 --- a/packages/agent/src/audit-trail/README.md +++ b/packages/agent/src/audit-trail/README.md @@ -86,6 +86,7 @@ The `forest.audit_logs` table has one row per audited change: | `operation` | `create` / `update` / `delete` / `action` / `action_failed` | | `collection` | audited collection name | | `record_id` | packed record id (primary keys joined with `\|`); `null` for a `create` row still `pending` (the record's id isn't assigned yet) | +| `previous_record_id` | the id the row was filed under before a confirmed `update`, set whether or not the key moved; `null` on every other operation and on any row written before this column existed. Internal: never served to a client | | `user_id` | id of the Forest user who made the change | | `user_first_name` | that user's first name, denormalized at write time | | `user_last_name` | that user's last name, denormalized at write time | diff --git a/packages/agent/src/audit-trail/instrument.ts b/packages/agent/src/audit-trail/instrument.ts index 79572c28f5..7921b0e69c 100644 --- a/packages/agent/src/audit-trail/instrument.ts +++ b/packages/agent/src/audit-trail/instrument.ts @@ -480,6 +480,10 @@ function instrumentCollection( return recorder.confirm(pendingId, { operation: 'update', recordId: toPackedRecordId(updated, primaryKeys), + // Written whether or not the key moved. Only a value here lets the route trust that the + // previous side's id is known: a null has to keep meaning "this row predates the column", + // or an old row whose key did move would be judged by the id it moved to. + previousRecordId: toPackedRecordId(record, primaryKeys), previousValues: redactValues(previousValues, redactedFields), newValues: redactValues(newValues, redactedFields), }); diff --git a/packages/agent/src/audit-trail/migrations.ts b/packages/agent/src/audit-trail/migrations.ts index f84afa0225..5936e729bb 100644 --- a/packages/agent/src/audit-trail/migrations.ts +++ b/packages/agent/src/audit-trail/migrations.ts @@ -29,6 +29,12 @@ function qualifiedMigrationName(schema: string | undefined, tableName: string): : `${tableName}:001-create-audit-logs`; } +function qualifiedMigrationName002(schema: string | undefined, tableName: string): string { + return schema + ? `${schema}.${tableName}:002-add-previous-record-id` + : `${tableName}:002-add-previous-record-id`; +} + // 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 @@ -198,6 +204,32 @@ function buildMigrations(schema: string | undefined, tableName: string) { ); }, }, + { + // Its own migration rather than a column added to 001: the table has shipped, so a database + // out there has already recorded 001 as applied and would never see the edit. + name: qualifiedMigrationName002(schema, tableName), + up: async ({ context }: { context: MigrationContext }) => { + const table = { tableName: context.tableName, schema: context.schema }; + const existing = await columnNames(context.queryInterface, table, context.transaction); + + // Idempotent for the same reason 001 is: a process losing a concurrent-boot race retries. + if (existing.has('previous_record_id')) return; + + await context.queryInterface.addColumn( + table, + 'previous_record_id', + { type: DataTypes.TEXT, allowNull: true }, + { transaction: context.transaction }, + ); + }, + down: async ({ context }: { context: MigrationContext }) => { + await context.queryInterface.removeColumn( + { tableName: context.tableName, schema: context.schema }, + 'previous_record_id', + { transaction: context.transaction }, + ); + }, + }, ]; } diff --git a/packages/agent/src/audit-trail/sql-store.ts b/packages/agent/src/audit-trail/sql-store.ts index fe588cf8a3..ce58bdc4f1 100644 --- a/packages/agent/src/audit-trail/sql-store.ts +++ b/packages/agent/src/audit-trail/sql-store.ts @@ -37,6 +37,9 @@ export function defineAuditLogModel( // migration creates the column as such — this must match. Nullable: a pending create's row // has no id yet, since the record doesn't exist until the write resolves. recordId: { type: DataTypes.TEXT, allowNull: true }, + // Set on every confirmed update, so a null distinguishes a row older than the column from + // one whose key held still. TEXT for the same reason as `recordId`. + previousRecordId: { type: DataTypes.TEXT, allowNull: true }, userId: { type: DataTypes.INTEGER, allowNull: true }, // Denormalised from the caller at write time — who acted then, not who holds that id today. userFirstName: { type: DataTypes.TEXT, allowNull: true }, @@ -230,6 +233,7 @@ export function fromRow(row: Model): AuditRecord { operation: plain.operation as AuditRecord['operation'], collection: plain.collection as string, recordId: (plain.recordId as string) ?? null, + previousRecordId: (plain.previousRecordId as string) ?? null, userId: plain.userId as number, userFirstName: (plain.userFirstName as string) ?? null, userLastName: (plain.userLastName as string) ?? null, diff --git a/packages/agent/src/audit-trail/types.ts b/packages/agent/src/audit-trail/types.ts index 2850fd2272..7ecf69a99b 100644 --- a/packages/agent/src/audit-trail/types.ts +++ b/packages/agent/src/audit-trail/types.ts @@ -11,6 +11,13 @@ export type AuditRecord = { collection: string; /** Null for a pending create — the record's primary key isn't assigned yet. */ recordId: string | null; + /** + * The id this record was filed under before a confirmed update, written whether or not the key + * moved. Null therefore means "written before this column existed", not "the key held still": a + * row from an earlier agent cannot claim its id answers for the previous side of an update. + * Internal — it is how the agent follows a record across a rename, never served to a client. + */ + previousRecordId: string | null; userId: number; /** Denormalised from the caller at write time: who acted then, not who holds that id today. */ userFirstName: string | null; @@ -30,13 +37,18 @@ export type AuditRecord = { }; /** The subset known before the write runs, when the pending row is first inserted. */ -export type PendingAuditRecord = Omit; +export type PendingAuditRecord = Omit & + Partial>; -/** What `confirm` updates once the write (or action) has resolved. */ +/** + * What `confirm` updates once the write (or action) has resolved. `previousRecordId` is optional + * because only an update has a previous side to file under an id of its own. + */ export type AuditRecordConfirmation = Pick< AuditRecord, 'operation' | 'recordId' | 'previousValues' | 'newValues' ->; +> & + Partial>; export type AuditHistoryQuery = { collection: string; diff --git a/packages/agent/src/audit-trail/withhold.ts b/packages/agent/src/audit-trail/withhold.ts index 60ac08d489..0fdd6f4a1b 100644 --- a/packages/agent/src/audit-trail/withhold.ts +++ b/packages/agent/src/audit-trail/withhold.ts @@ -145,22 +145,40 @@ export default function withholdOutsidePermissionScope( return entries.map(entry => { if (entry.operation === 'action' || entry.operation === 'action_failed') return entry; - // Decoded once per row rather than once per side: the two sides read the same id, and a row - // whose id no longer decodes should say so once. - const decoded = decodePrimaryKeys(entry.recordId, withholding.collection, withholding.logger); - - const side = (values: Record, idAnswersForSide: IdAnswersForSide) => + const side = ( + values: Record, + decoded: ReturnType, + idAnswersForSide: IdAnswersForSide, + ) => permissionScopeAccepts(answerableSnapshot(values, decoded, idAnswersForSide), withholding) ? values ?? {} : {}; + // An update that recorded where it came from is judged side by side: the previous state + // against the id it was filed under then, the new state against the id it ended up with. A row + // without that column is older than it, so its id still answers for the new side alone. + // `?? null` first: the column is optional on the write types, so an absent one has to read as + // unknown exactly like a null, or a row that never recorded it would answer from the id it + // ended up with. + const previousRecordId = entry.previousRecordId ?? null; + const previousIsKnown = entry.operation !== 'update' || previousRecordId !== null; + + // Decoded once per distinct id, not once per side: a row whose id no longer decodes should say + // so once, and the two sides read the same id unless the key actually moved. + const decoded = decodePrimaryKeys(entry.recordId, withholding.collection, withholding.logger); + const decodedPrevious = + previousRecordId === null + ? decoded + : decodePrimaryKeys(previousRecordId, withholding.collection, withholding.logger); + + // A pending update is filed under the id the record had before the write, which says nothing + // about the state it was moving to, so the new side answers only with what it captured. + const newIsKnown = entry.status !== 'pending'; + return { ...entry, - // The row is filed under the identity the record ended up with, so its id answers for the - // new side of an update and for a create or a delete — never for what an update moved away - // from. - previousValues: side(entry.previousValues, entry.operation !== 'update'), - newValues: side(entry.newValues, true), + previousValues: side(entry.previousValues, decodedPrevious, previousIsKnown), + newValues: side(entry.newValues, decoded, newIsKnown), }; }); } diff --git a/packages/agent/src/routes/access/audit-trail.ts b/packages/agent/src/routes/access/audit-trail.ts index 5fddecb640..92de69442e 100644 --- a/packages/agent/src/routes/access/audit-trail.ts +++ b/packages/agent/src/routes/access/audit-trail.ts @@ -174,7 +174,8 @@ export default class AuditTrailRoute extends CollectionRoute { permissionScope && gone ? this.withhold(rawData, permissionScope, context) : rawData; context.response.body = { - data, + // `previousRecordId` stays out here too: the served shape is the same whichever path built it. + data: data.map(({ previousRecordId, ...served }) => served), meta: { count, ...(availableUsers && { availableUsers }) }, }; } @@ -186,7 +187,7 @@ export default class AuditTrailRoute extends CollectionRoute { permissionScope: ConditionTree, context: Context, ): Promise<{ - data: AuditRecord[]; + data: Array>; meta: { count: number; availableUsers?: AuditUserSummary[] }; }> { const { store } = this.options.auditTrail; @@ -226,7 +227,9 @@ export default class AuditTrailRoute extends CollectionRoute { } return { - data: page, + // `previousRecordId` stays out: it is how the agent follows a record across a rename, not + // something a client reads. + data: page.map(({ previousRecordId, ...served }) => served), meta: { count, ...(isFirstFetch && { availableUsers: [...authors.values()] }) }, }; } diff --git a/packages/agent/test/audit-trail/in-memory-store.test.ts b/packages/agent/test/audit-trail/in-memory-store.test.ts index 112c1f37a8..5607235b61 100644 --- a/packages/agent/test/audit-trail/in-memory-store.test.ts +++ b/packages/agent/test/audit-trail/in-memory-store.test.ts @@ -9,6 +9,7 @@ const record = ( operation: 'update', collection: 'accounts', recordId: '1', + previousRecordId: null, userId: 1, userFirstName: null, userLastName: null, diff --git a/packages/agent/test/audit-trail/in-memory-store.ts b/packages/agent/test/audit-trail/in-memory-store.ts index 43b943d007..cf4d6e9fdc 100644 --- a/packages/agent/test/audit-trail/in-memory-store.ts +++ b/packages/agent/test/audit-trail/in-memory-store.ts @@ -30,7 +30,7 @@ export default class InMemoryAuditStore implements AuditStore { async insertPending(record: PendingAuditRecord): Promise { const id = this.nextId; this.nextId += 1; - this.records.push({ ...record, id, status: 'pending' }); + this.records.push({ previousRecordId: null, ...record, id, status: 'pending' }); return id; } diff --git a/packages/agent/test/audit-trail/instrument.test.ts b/packages/agent/test/audit-trail/instrument.test.ts index 9c7d2128c5..9f721469a4 100644 --- a/packages/agent/test/audit-trail/instrument.test.ts +++ b/packages/agent/test/audit-trail/instrument.test.ts @@ -702,12 +702,32 @@ describe('auditTrail plugin', () => { expect(sink).toHaveBeenCalledWith( expect.objectContaining({ recordId: 'new-slug', + // What the route needs to judge the previous side by the identity it actually had. + previousRecordId: 'old-slug', previousValues: { slug: 'old-slug' }, newValues: { slug: 'new-slug' }, }), ); }); + it('records where a row came from even when the key held still', async () => { + const sink = jest.fn(); + const accounts = fakeCollection('accounts', [{ id: 1, name: 'Acme', amount: 10 }]); + register([accounts], { sink }); + + await runUpdate(accounts, { + caller: makeCaller(), + patch: { name: 'Acme Inc' }, + after: [{ id: 1, name: 'Acme Inc', amount: 10 }], + }); + + // Not only on a move: a null has to keep meaning "written before this column existed", or a + // row from an older agent whose key did move would be judged by the id it moved to. + expect(sink).toHaveBeenCalledWith( + expect.objectContaining({ recordId: '1', previousRecordId: '1' }), + ); + }); + it('still records an update that changes a field the update was itself filtered on', async () => { const sink = jest.fn(); const accounts = fakeCollection('accounts', [ diff --git a/packages/agent/test/audit-trail/migrations.test.ts b/packages/agent/test/audit-trail/migrations.test.ts index 1f2779de03..dcadbfcebf 100644 --- a/packages/agent/test/audit-trail/migrations.test.ts +++ b/packages/agent/test/audit-trail/migrations.test.ts @@ -43,6 +43,7 @@ describe('runAuditMigrations (sqlite)', () => { 'id', 'new_values', 'operation', + 'previous_record_id', 'previous_values', 'record_id', 'status', @@ -68,7 +69,10 @@ describe('runAuditMigrations (sqlite)', () => { // sqlite has no real schema/catalog separation: Sequelize represents a schema-qualified table // as a single literal identifier joining schema and table name with a dot. const [applied] = await sequelize.query('SELECT name FROM "forest.audit_logs_migration"'); - expect(applied).toEqual([{ name: 'forest.audit_logs:001-create-audit-logs' }]); + expect(applied).toEqual([ + { name: 'forest.audit_logs:001-create-audit-logs' }, + { name: 'forest.audit_logs:002-add-previous-record-id' }, + ]); await sequelize.close(); }); @@ -79,7 +83,10 @@ describe('runAuditMigrations (sqlite)', () => { await runAuditMigrations(sequelize, { tableName: 'audit_logs' }); const [applied] = await sequelize.query('SELECT name FROM "audit_logs_migration"'); - expect(applied).toEqual([{ name: 'audit_logs:001-create-audit-logs' }]); + expect(applied).toEqual([ + { name: 'audit_logs:001-create-audit-logs' }, + { name: 'audit_logs:002-add-previous-record-id' }, + ]); await sequelize.close(); }); @@ -163,7 +170,7 @@ describe('runAuditMigrations (sqlite)', () => { ).resolves.toBeUndefined(); const [applied] = await sequelize.query('SELECT name FROM "audit_logs_migration"'); - expect(applied).toHaveLength(1); + expect(applied).toHaveLength(2); await sequelize.close(); }); diff --git a/packages/agent/test/routes/access/audit-trail.test.ts b/packages/agent/test/routes/access/audit-trail.test.ts index 57b44951d4..b6753d6742 100644 --- a/packages/agent/test/routes/access/audit-trail.test.ts +++ b/packages/agent/test/routes/access/audit-trail.test.ts @@ -1513,6 +1513,65 @@ describe('AuditTrailRoute', () => { ]); }); + // What PRD-1321 buys back: with the id the row was filed under before the move recorded, the + // previous side is judged by that id instead of being withheld for want of an answer. + test('judges a moved key against the id the previous side carried', async () => { + const data = await historyUnder(new ConditionTreeLeaf('id', 'Equal', 2), [ + { + operation: 'update', + recordId: '9', + previousRecordId: '2', + previousValues: { id: REDACTED, secret: 'was in scope then' }, + newValues: { id: REDACTED, secret: 'out of scope now' }, + }, + ]); + + expect(data).toEqual([ + { + operation: 'update', + recordId: '9', + previousValues: { id: REDACTED, secret: 'was in scope then' }, + newValues: {}, + }, + ]); + }); + + test("never serves the id a move came from, which is the agent's own bookkeeping", async () => { + const data = await historyUnder(new ConditionTreeLeaf('id', 'Equal', 2), [ + { + operation: 'update', + recordId: '9', + previousRecordId: '2', + previousValues: { ownerId: 2 }, + newValues: { ownerId: 2 }, + }, + ]); + + expect(data[0]).not.toHaveProperty('previousRecordId'); + }); + + test("withholds a pending update's new side, which its id cannot speak for", async () => { + const data = await historyUnder(new ConditionTreeLeaf('id', 'Equal', 2), [ + { + operation: 'update', + recordId: '2', + status: 'pending', + previousValues: { id: 2, secret: 'before' }, + newValues: { id: REDACTED, secret: 'after' }, + }, + ]); + + expect(data).toEqual([ + { + operation: 'update', + recordId: '2', + status: 'pending', + previousValues: { id: 2, secret: 'before' }, + newValues: {}, + }, + ]); + }); + test('withholds the values when the scope names an inherited property of the snapshot', async () => { const data = await historyUnder(new ConditionTreeLeaf('toString', 'NotEqual', 'private'), [ { operation: 'delete', recordId: '2', previousValues: { ownerId: 1, secret: 'shh' } },