Bulk operations: TOCTOU race on ticket status used for workflow validation #34

New Issue

2026-08-31T21:29:43-04:00

jared commented

2026-08-31 21:29:43 -04:00

Severity: Medium

models/BulkOperationsModel.php (~lines 137-199): ticket state used for workflow-transition validation (ticketsById) is a snapshot read via getTicketsByIds() before begin_transaction(), and the later UPDATE (via TicketModel::updateTicket) has no WHERE status = ? guard or row lock re-checking that status hasn't changed since the snapshot.

Impact: Two concurrent admin actions on the same ticket (e.g. an overlapping single-ticket update and a bulk operation) can both validate against the same stale status and then both apply transitions — the second write overwrites without re-verifying the transition is still legal from the ticket's now-current status. This can bypass Workflow Designer rules under concurrency, similar in spirit to the bug fixed in #21 but via a race instead of a missing check.

Fix: Re-validate the transition against the current DB row inside the transaction (e.g. SELECT ... FOR UPDATE or a conditional UPDATE ... WHERE status = ? with affected-rows check) rather than trusting the pre-transaction snapshot.

**Severity:** Medium `models/BulkOperationsModel.php` (~lines 137-199): ticket state used for workflow-transition validation (`ticketsById`) is a snapshot read via `getTicketsByIds()` **before** `begin_transaction()`, and the later `UPDATE` (via `TicketModel::updateTicket`) has no `WHERE status = ?` guard or row lock re-checking that status hasn't changed since the snapshot. **Impact:** Two concurrent admin actions on the same ticket (e.g. an overlapping single-ticket update and a bulk operation) can both validate against the same stale status and then both apply transitions — the second write overwrites without re-verifying the transition is still legal from the ticket's now-current status. This can bypass Workflow Designer rules under concurrency, similar in spirit to the bug fixed in #21 but via a race instead of a missing check. **Fix:** Re-validate the transition against the current DB row inside the transaction (e.g. `SELECT ... FOR UPDATE` or a conditional `UPDATE ... WHERE status = ?` with affected-rows check) rather than trusting the pre-transaction snapshot.

jared added the concurrency priority/medium workflow labels 2026-09-08 10:15:43 -04:00

jared referenced this issue from a commit

2026-09-11 21:42:01 -04:00

Fix TOCTOU race in bulk operations by row-locking tickets (#34)

jared referenced this issue from a commit

2026-09-11 21:45:26 -04:00

Merge development into main: concurrency/atomicity fixes (#34, #35, #37)

jared commented

2026-09-11 21:50:57 -04:00

Fixed in d205a9577a (commit d205a95, merged to main in 3ab33d8).

processBulkOperation() was validating each ticket's status/priority transition against a pre-transaction snapshot ($ticketsById) taken before the loop started, rather than re-reading inside the transaction. Two concurrent bulk operations touching the same ticket could both pass validation against the same stale data and one transition would silently clobber the other.

Fix: added lockTicketForUpdate(), which re-fetches a ticket via SELECT ... FOR UPDATE, and used it wherever the loop needs current status/priority (bulk_close, bulk_priority, bulk_status), removing the redundant re-reads from the stale snapshot.

Verified against real MariaDB with two concurrent OS processes: the second process blocked ~1.2s on the first's held row lock, then correctly observed the first's committed status and rejected an otherwise-stale-data-permitted invalid transition. Also regression-tested normal bulk_close/bulk_priority operation on real tickets.

Fixed in d205a9577ae145fac12a0110c3260ab7e38c80d1 (commit d205a95, merged to main in 3ab33d8). processBulkOperation() was validating each ticket's status/priority transition against a pre-transaction snapshot ($ticketsById) taken before the loop started, rather than re-reading inside the transaction. Two concurrent bulk operations touching the same ticket could both pass validation against the same stale data and one transition would silently clobber the other. Fix: added lockTicketForUpdate(), which re-fetches a ticket via SELECT ... FOR UPDATE, and used it wherever the loop needs current status/priority (bulk_close, bulk_priority, bulk_status), removing the redundant re-reads from the stale snapshot. Verified against real MariaDB with two concurrent OS processes: the second process blocked ~1.2s on the first's held row lock, then correctly observed the first's committed status and rejected an otherwise-stale-data-permitted invalid transition. Also regression-tested normal bulk_close/bulk_priority operation on real tickets.

jared closed this issue

2026-09-11 21:50:57 -04:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: LotusGuild/tinker_tickets#34