Skip to content

fix(agent): stop the audit store's queries from adopting a host's CLS transaction - #1842

Open
bexchauveto wants to merge 1 commit into
mainfrom
fix/audit-trail-cls-transaction-hijack
Open

fix(agent): stop the audit store's queries from adopting a host's CLS transaction#1842
bexchauveto wants to merge 1 commit into
mainfrom
fix/audit-trail-cls-transaction-hijack

Conversation

@bexchauveto

@bexchauveto bexchauveto commented Aug 21, 2026

Copy link
Copy Markdown
Member

Summary

  • Found while enabling auditTrail on a CLS-using host: Sequelize.useCLS(ns) sets Sequelize._cls, a class-level static shared by every Sequelize instance in the same loaded copy of the package — including the audit store's own, otherwise-unrelated connection. Any query issued without an explicit transaction reads that static (options.transaction === void 0 && Sequelize._cls) to decide whether to adopt an ambient transaction. A host using CLS for its own request-scoped transactions therefore had every audit-store query silently run against the host's connection, inside the host's transaction, instead of the audit store's own pool.
  • Verified directly against Sequelize's own source (Sequelize.prototype.query()) and reproduced with a fake CLS namespace causing a live insertPending/confirm call to reach for a bogus hijacked connection instead of its own.
  • Consequences: if the audit and host databases differ, the write 404s inside the host's transaction and aborts it (an audited business write breaks even though critical: false); if they're the same database, a pending row rolls back with the host's transaction, defeating the "a pending row is evidence a write was attempted" guarantee the write protocol is built on.
  • Fix: every query the store issues now passes transaction: null explicitly, opting out of the CLS lookup unconditionally — insertPending, insertPendingBatch, confirm, listByRecord, countByRecord, listDistinctUsers, listByCorrelation(s), and authenticate. Also threads a migration's own transaction through assertOwnsTable's describeTable probe, which previously passed no transaction option at all (same class of gap, narrower window).
  • A second finding from the same report (pending rows storing previousValues unredacted) was investigated and does not reproduce: buildRecord in instrument.ts already applies redactValues unconditionally before returning, including for the Before-Update/Before-Delete pending inserts — verified against both current source and the published 1.96.0 dist bundle.

Test plan

  • yarn workspace @forestadmin/agent test — 1439 passing (was 1438; one new regression test added)
  • yarn workspace @forestadmin/agent lint — 0 errors
  • New test (sql-store.test.ts) sets a fake Sequelize._cls namespace with a transaction whose .connection getter is spied, then confirms a pending audit row and asserts the getter was never called — verified this test fails without the fix and passes with it.
  • Manually configure an agent with auditTrail alongside a host that calls Sequelize.useCLS(...) for its own transactions, and confirm an audited write inside a host transaction no longer touches the host's connection.

Note

Stop audit store queries from adopting host CLS transactions

  • Pass { transaction: null } to every Sequelize model operation in sql-store.ts (create, bulkCreate, update, findAll, count, and authenticate) so the store never adopts an ambient CLS transaction set by the host process.
  • Forward context.transaction through assertOwnsTablecolumnNames in migrations.ts so the migration ownership/idempotency check runs inside the migration's own transaction instead of outside it.
  • Add a regression test in sql-store.test.ts that injects a fake CLS namespace via Reflect.set(Sequelize, "_cls", ...) and asserts store.confirm does not touch the hijacked transaction's connection getter.
  • Behavioral Change: audit store queries that previously ran inside a host CLS transaction now always execute on their own connection with no transaction. Migration ownership checks now run transactionally when context.transaction is provided.

Macroscope summarized 845b909.

… transaction

Sequelize.useCLS(ns) sets Sequelize._cls, a class-level static shared
by every Sequelize instance in the same loaded copy of the package —
including the audit store's own, otherwise-unrelated connection. Any
query issued without an explicit `transaction` reads that static to
decide whether to adopt an ambient transaction, so a host that uses
CLS for its own request-scoped transactions would have every audit
query silently run against the host's connection and inside the
host's transaction instead of the audit store's own pool: if the two
databases differ, the write 404s inside the host's transaction and
aborts it; if they're the same database, a pending row rolls back
with the host transaction, defeating the "pending row is evidence a
write was attempted" guarantee the write protocol is built on.

Every query the store issues now passes `transaction: null` explicitly,
opting out of the CLS lookup unconditionally. Also threads a migration's
own transaction through `assertOwnsTable`'s `describeTable` probe,
which omitted any transaction at all.

Found while enabling auditTrail on a CLS-using host.
@qltysh

qltysh Bot commented Aug 21, 2026

Copy link
Copy Markdown

1 new issue

Tool Category Rule Count
qlty Structure Function with many returns (count = 4): createSqlAuditStore 1

@qltysh

qltysh Bot commented Aug 21, 2026

Copy link
Copy Markdown

Qlty


Coverage Impact

This PR will not change total coverage.

Modified Files with Diff Coverage (2)

RatingFile% DiffUncovered Line #s
Coverage rating: A Coverage rating: A
packages/agent/src/audit-trail/migrations.ts100.0%
Coverage rating: A Coverage rating: A
packages/agent/src/audit-trail/sql-store.ts100.0%
Total100.0%
🚦 See full report on Qlty Cloud »

🛟 Help
  • Diff Coverage: Coverage for added or modified lines of code (excludes deleted files). Learn more.

  • Total Coverage: Coverage for the whole repository, calculated as the sum of all File Coverage. Learn more.

  • File Coverage: Covered Lines divided by Covered Lines plus Missed Lines. (Excludes non-executable lines including blank lines and comments.)

    • Indirect Changes: Changes to File Coverage for files that were not modified in this PR. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant