From d205a9577ae145fac12a0110c3260ab7e38c80d1 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Fri, 11 Sep 2026 21:41:41 -0400 Subject: [PATCH] Fix TOCTOU race in bulk operations by row-locking tickets (#34) processBulkOperation() validated each ticket's status/priority transition against a pre-transaction snapshot (ticketsById) instead of re-reading inside the transaction, so two concurrent bulk operations touching the same ticket could both pass validation against stale data and one transition could silently clobber the other. Add lockTicketForUpdate(), which re-fetches a ticket via SELECT ... FOR UPDATE, and use it wherever the loop needs current status/priority, removing the three redundant re-reads from the stale snapshot in the bulk_close/bulk_priority/ bulk_status branches. 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. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_0117oBw2jN4kALYeS8HPq4zV --- models/BulkOperationsModel.php | 40 +++++++++++++++++++++++++++++----- 1 file changed, 35 insertions(+), 5 deletions(-) diff --git a/models/BulkOperationsModel.php b/models/BulkOperationsModel.php index a5d68ef..47dee2c 100644 --- a/models/BulkOperationsModel.php +++ b/models/BulkOperationsModel.php @@ -33,6 +33,23 @@ class BulkOperationsModel return $this->workflowModel; } + /** + * Re-fetch a ticket row inside the current transaction with a row lock + * (FOR UPDATE), so a concurrent transaction touching the same row blocks + * until this one commits or rolls back instead of both validating + * against the same stale snapshot. Must be called after + * begin_transaction() and before the row is written. + */ + private function lockTicketForUpdate(string $ticketId): ?array + { + $stmt = $this->conn->prepare("SELECT * FROM tickets WHERE ticket_id = ? FOR UPDATE"); + $stmt->bind_param('s', $ticketId); + $stmt->execute(); + $row = $stmt->get_result()->fetch_assoc(); + $stmt->close(); + return $row ?: null; + } + /** * The status a bulk operation is trying to move tickets into, or null for * operations that don't change status. @@ -183,13 +200,27 @@ class BulkOperationsModel $success = false; try { + // Re-fetch and row-lock the ticket inside the transaction for any + // operation that validates against or reads its current fields — + // the pre-transaction $ticketsById snapshot (loaded before + // begin_transaction()) can be stale by the time we get here if a + // concurrent request (a single-ticket edit, or another bulk op) + // changed the row in between. Validating a transition against a + // stale status, or writing back stale title/description/etc., + // could silently bypass Workflow Designer rules or clobber a + // concurrent edit. FOR UPDATE blocks a concurrent transaction + // from reading/writing this row until ours commits or rolls back. + $needsCurrentTicket = $targetStatus !== null || $operation['operation_type'] === 'bulk_priority'; + $currentTicket = $needsCurrentTicket + ? $this->lockTicketForUpdate($ticketId) + : ($ticketsById[$ticketId] ?? null); + // bulk_status / bulk_close enforce the same Workflow Designer // rules as the single-ticket path: a transition the designer // doesn't define is refused, and requires_comment is honoured // (checked up front, above). requires_admin is satisfied because // api/bulk_operation.php already gates the endpoint on admin. if ($targetStatus !== null) { - $currentTicket = $ticketsById[$ticketId] ?? null; if ($currentTicket && $currentTicket['status'] === $targetStatus) { // Already in the requested state — nothing to do, and // reporting a no-op as a failure would just confuse. @@ -211,8 +242,7 @@ class BulkOperationsModel switch ($operation['operation_type']) { case 'bulk_close': - // Get current ticket from pre-loaded batch - $currentTicket = $ticketsById[$ticketId] ?? null; + // $currentTicket is the fresh, row-locked read from above. if ($currentTicket) { $updateResult = $ticketModel->updateTicket([ 'ticket_id' => $ticketId, @@ -264,7 +294,7 @@ class BulkOperationsModel case 'bulk_priority': if (isset($parameters['priority'])) { - $currentTicket = $ticketsById[$ticketId] ?? null; + // $currentTicket is the fresh, row-locked read from above. if ($currentTicket) { $updateResult = $ticketModel->updateTicket([ 'ticket_id' => $ticketId, @@ -292,7 +322,7 @@ class BulkOperationsModel case 'bulk_status': if (isset($parameters['status'])) { - $currentTicket = $ticketsById[$ticketId] ?? null; + // $currentTicket is the fresh, row-locked read from above. if ($currentTicket) { $updateResult = $ticketModel->updateTicket([ 'ticket_id' => $ticketId,