diff --git a/packages/agent/src/audit-trail/migrations.ts b/packages/agent/src/audit-trail/migrations.ts index d037e778c4..f84afa0225 100644 --- a/packages/agent/src/audit-trail/migrations.ts +++ b/packages/agent/src/audit-trail/migrations.ts @@ -71,8 +71,12 @@ async function indexNames( async function columnNames( queryInterface: QueryInterface, table: { tableName: string; schema?: string }, + transaction?: Transaction, ): Promise> { - const columns = await queryInterface.describeTable(table); + // `describeTable`'s options type omits `transaction`, unlike its sibling queryInterface methods, + // even though it forwards the whole options object into the underlying `sequelize.query()` call + // (verified against sequelize's own source) — a typings gap, not a runtime one. + const columns = await queryInterface.describeTable(table, { transaction } as never); return new Set(Object.keys(columns)); } @@ -100,8 +104,9 @@ const AUDIT_LOG_COLUMNS = [ async function assertOwnsTable( queryInterface: QueryInterface, table: { tableName: string; schema?: string }, + transaction?: Transaction, ): Promise { - const existing = await columnNames(queryInterface, table); + const existing = await columnNames(queryInterface, table, transaction); const missing = AUDIT_LOG_COLUMNS.filter(column => !existing.has(column)); if (missing.length > 0) { @@ -149,7 +154,7 @@ function buildMigrations(schema: string | undefined, tableName: string) { // failing on a "relation already exists" error from the winner's DDL. Guards against // reusing an unrelated table sharing the name, too — see assertOwnsTable. if (await context.queryInterface.tableExists(table, { transaction: context.transaction })) { - await assertOwnsTable(context.queryInterface, table); + await assertOwnsTable(context.queryInterface, table, context.transaction); return; } diff --git a/packages/agent/src/audit-trail/sql-store.ts b/packages/agent/src/audit-trail/sql-store.ts index 1346b4fcbd..d2f4453549 100644 --- a/packages/agent/src/audit-trail/sql-store.ts +++ b/packages/agent/src/audit-trail/sql-store.ts @@ -262,7 +262,14 @@ export function createSqlAuditStore(options: AuditStorageOptions): { sequelize = connection; bootstrap = (async () => { try { - await connection.authenticate(); + // `Sequelize.useCLS(ns)` sets a class-level static shared by every Sequelize instance in + // the same loaded copy of the package — including this one, however unrelated its own + // connection is. A host that uses CLS for its own transactions would otherwise have every + // query below silently adopt its ambient transaction (and thus its connection) the moment + // that query omits `transaction`, since Sequelize reads `options.transaction === void 0` + // to decide whether to consult CLS. Passing `transaction: null` explicitly opts every + // query in this store out, so it always uses its own dedicated connection. + await connection.authenticate({ transaction: null }); const model = await ensureAuditStorage(connection, { schema, tableName }); return { model, connection }; @@ -291,7 +298,9 @@ export function createSqlAuditStore(options: AuditStorageOptions): { }, async insertPending(record) { const { model } = await init(); - const row = await model.create(toRow({ ...record, status: 'pending' })); + const row = await model.create(toRow({ ...record, status: 'pending' }), { + transaction: null, + }); return Number(row.get('id')); }, @@ -305,13 +314,14 @@ export function createSqlAuditStore(options: AuditStorageOptions): { const { model } = await init(); const rows = await model.bulkCreate( records.map(record => toRow({ ...record, status: 'pending' })), + { transaction: null }, ); return rows.map(row => Number(row.get('id'))); }, async confirm(id, patch) { const { model } = await init(); - await model.update({ ...patch, status: 'done' }, { where: { id } }); + await model.update({ ...patch, status: 'done' }, { where: { id }, transaction: null }); }, async listByRecord(query) { const { model, connection } = await init(); @@ -326,6 +336,7 @@ export function createSqlAuditStore(options: AuditStorageOptions): { ], offset: skip, limit, + transaction: null, }); return rows.map(fromRow); @@ -333,7 +344,10 @@ export function createSqlAuditStore(options: AuditStorageOptions): { async countByRecord(query) { const { model, connection } = await init(); - return model.count({ where: buildHistoryWhereClause(query, connection) }); + return model.count({ + where: buildHistoryWhereClause(query, connection), + transaction: null, + }); }, async listDistinctUsers(query) { const { model, connection } = await init(); @@ -350,6 +364,7 @@ export function createSqlAuditStore(options: AuditStorageOptions): { ], group: ['userId'], raw: true, + transaction: null, })) as unknown as Array<{ userId: number; userFirstName: string | null; @@ -370,6 +385,7 @@ export function createSqlAuditStore(options: AuditStorageOptions): { const rows = await model.findAll({ where: { collection, recordId, correlationKey }, order: chronologicalOrder as never, + transaction: null, }); return rows.map(fromRow); @@ -382,6 +398,7 @@ export function createSqlAuditStore(options: AuditStorageOptions): { const rows = await model.findAll({ where: { collection, recordId, correlationKey: { [Op.in]: correlationKeys } }, order: chronologicalOrder as never, + transaction: null, }); return rows.map(fromRow); diff --git a/packages/agent/test/audit-trail/sql-store.test.ts b/packages/agent/test/audit-trail/sql-store.test.ts index 296fcf87e9..4e2e744dc5 100644 --- a/packages/agent/test/audit-trail/sql-store.test.ts +++ b/packages/agent/test/audit-trail/sql-store.test.ts @@ -118,6 +118,49 @@ describe('createSqlAuditStore (sqlite round-trip)', () => { await close(); }); + it('never adopts a transaction from Sequelize.useCLS, even when a host process sets one', async () => { + // `Sequelize._cls` is a class-level static, shared by every Sequelize instance in the same + // loaded copy of the package — including this store's own, unrelated connection. A query + // that omits `transaction` reads it as the ambient transaction (and thus connection) to use. + // Migrations run their own explicit `sequelize.transaction(...)`, which — unlike a plain + // query — invokes CLS's `.run()`/`.bind()` too; warming the store up before CLS is active + // keeps this fake namespace to the one method (`.get()`) the vulnerable path actually calls, + // without having to reimplement the rest of a real CLS namespace's contract. + const { store, close } = createSqlAuditStore({ connectionString: 'sqlite::memory:' }); + const id = await store.insertPending(record()); + + const connectionAccessed = jest.fn(); + const hijackedTransaction = { + get connection() { + connectionAccessed(); + + return { fakeConnection: true }; + }, + }; + // Reflect avoids both a dot-notation member expression (forbidden on a dangling-underscore + // name) and an object-literal property of the same name (same rule, same reason). + Reflect.set(Sequelize, '_cls', { + get: (key: string) => (key === 'transaction' ? hijackedTransaction : undefined), + }); + + try { + await expect( + store.confirm(id, { + operation: 'update', + recordId: '1', + previousValues: {}, + newValues: {}, + }), + ).resolves.toBeUndefined(); + + expect(connectionAccessed).not.toHaveBeenCalled(); + } finally { + Reflect.deleteProperty(Sequelize, '_cls'); + } + + await close(); + }); + it('returns rows previously appended, sorted by timestamp, scoped to a single record', async () => { const { store, close } = createSqlAuditStore({ connectionString: 'sqlite::memory:' });