Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions packages/agent/src/audit-trail/migrations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,12 @@ async function indexNames(
async function columnNames(
queryInterface: QueryInterface,
table: { tableName: string; schema?: string },
transaction?: Transaction,
): Promise<Set<string>> {
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));
}
Expand Down Expand Up @@ -100,8 +104,9 @@ const AUDIT_LOG_COLUMNS = [
async function assertOwnsTable(
queryInterface: QueryInterface,
table: { tableName: string; schema?: string },
transaction?: Transaction,
): Promise<void> {
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) {
Expand Down Expand Up @@ -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;
}
Expand Down
25 changes: 21 additions & 4 deletions packages/agent/src/audit-trail/sql-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
Expand Down Expand Up @@ -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'));
},
Expand All @@ -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();
Expand All @@ -326,14 +336,18 @@ export function createSqlAuditStore(options: AuditStorageOptions): {
],
offset: skip,
limit,
transaction: null,
});

return rows.map(fromRow);
},
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();
Expand All @@ -350,6 +364,7 @@ export function createSqlAuditStore(options: AuditStorageOptions): {
],
group: ['userId'],
raw: true,
transaction: null,
})) as unknown as Array<{
userId: number;
userFirstName: string | null;
Expand All @@ -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);
Expand All @@ -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);
Expand Down
43 changes: 43 additions & 0 deletions packages/agent/test/audit-trail/sql-store.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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:' });

Expand Down
Loading