Skip to content

fix(purchases): TOCTOU race in cancel paths — concurrent approve+cancel can overwrite "approved" with "cancelled" mid-flight (P1) #671

Description

@cristim

Symptom

Both cancel paths can silently flip an already-approved (and potentially already-executing) purchase back to cancelled, terminating a purchase that the AWS / Azure / GCP API may have already accepted. This creates a state divergence: DB says cancelled, cloud says purchased. Surfaced by the concurrency-audit pass on #667 / #632 / #669.

The misleading comment on internal/api/handler_purchases.go:524 explicitly claims an "optimistic-locking guard" exists inside the transaction. It does not.

Root cause

Two cancel paths — both vulnerable:

  1. Session-authenticated dashboard cancelinternal/api/handler_purchases.go:509-543 cancelPurchaseViaSession
  2. Token-authenticated email cancelinternal/purchase/approvals.go:136-165 CancelExecution

Both follow the same broken pattern:

// 1. Out-of-transaction status read
exec := loadExecution(...)
if !exec.IsCancelable() { return 409 }

// 2. Unconditional upsert with no status guard
SavePurchaseExecutionTx(tx, exec)  // ON CONFLICT DO UPDATE — overwrites whatever's in the row

Meanwhile, ApproveAndExecute correctly uses atomic CAS via TransitionExecutionStatus (UPDATE ... WHERE execution_id=$1 AND status = ANY(...) RETURNING ...). So the approve side defends against the race; the cancel side doesn't.

Race window:

T=0   User A clicks Approve → ApproveAndExecute starts
T=10  User B clicks Cancel  → cancelPurchaseViaSession loads exec (status=pending)
T=20  ApproveAndExecute's CAS flips status to "approved"; purchase Lambda starts
T=30  cancelPurchaseViaSession.SavePurchaseExecutionTx upserts (status="cancelled")
       → row now says "cancelled" even though the AWS purchase is already running

Fix

Replace the unconditional SavePurchaseExecutionTx in BOTH cancel paths with a conditional UPDATE:

result, err := tx.Exec(ctx,
    `UPDATE purchase_executions
        SET status = 'cancelled',
            cancelled_at = NOW(),
            ...
      WHERE execution_id = $1
        AND status IN ('pending', 'notified')`,
    execID,
)
if rowsAffected, _ := result.RowsAffected(); rowsAffected == 0 {
    return 409 // race lost — status was already approved/running/completed/cancelled
}

Surface the 409 to the user with a meaningful body explaining the race. Also fix the misleading comment on handler_purchases.go:524 so the next reader doesn't trust a non-existent guard.

Acceptance criteria

  • cancelPurchaseViaSession uses a conditional UPDATE with status IN ('pending','notified') in the WHERE clause
  • CancelExecution (token path) uses the same conditional UPDATE
  • Zero-rows-affected → 409 response with body explaining "execution status transitioned to by a concurrent operation"
  • The misleading "optimistic-locking guard inside the tx" comment is removed or updated to reflect reality
  • Regression test: two concurrent goroutines (one Approve, one Cancel) against the same execution_id; assert that EXACTLY ONE wins, and the loser surfaces a clean error rather than silently overwriting

Severity

P1 / high. Real exploit window is small (sync Approve handler races with sync Cancel handler — milliseconds), but with the Lambda runtime serving multiple users concurrently and the async/email-link cancel path, the race is reachable in practice. The consequences are catastrophic: a purchase that AWS has accepted is marked cancelled in our DB, the user gets a "purchase cancelled" toast, but the commitment is real and will appear on their AWS bill.

Cross-references

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions