Skip to content

Commit 999eec1

Browse files
committed
feat(promo-codes): finish TOCTOU fix — remove post-facto check, add tests
Step 3 of 3. The per-member QuantityPerAccount check now lives solely in PreProcessReservationTask::reserveMemberQuotas (added in ad113d5, leak-guarded in 77c3059), where it runs atomically under the PESSIMISTIC_WRITE row lock on the parent promo code, BEFORE ReserveTicketsTask and ReserveOrderTask commit any tickets. Changes: - Remove the belt-and-suspenders post-facto check from ApplyPromoCodeTask::run. A comment now points at the pre-reservation location and links to smarcet's TOCTOU reproduction for context. The old check counted committed tickets and could not distinguish concurrent orders' rows — see the race narrative in PR #525. - Delete tests/Unit/Services/ApplyPromoCodeTaskConcurrencyTest.php. smarcet added this in the cherry-picked 2e0ef84 to prove the TOCTOU bug against the old check surface (static getTicketCountByMemberAndPromoCode). That surface is no longer in the write path, so the test targets removed code. The narrative is preserved — and extended — in the new file below. - Delete tests/Unit/Services/ApplyPromoCodeTaskQuantityPerAccountTest.php. All six cases validated the exact post-facto check that was just removed. - Add tests/Unit/Services/PreProcessReservationTaskConcurrencyTest.php with 6 cases exercising the new surface via reflection on reserveMemberQuotas: 1. First reserve succeeds when no prior row exists (repo `add` called with qty_used=1). 2. Second reserve rejects when the prior row's QtyUsed already sits at the limit (the serialized-second-request flow that replaces smarcet's TOCTOU reproduction). 3. Within-limit reserve increments the existing row. 4. Limit = 0 bypasses reservation entirely (unlimited per account). 5. Non-IDomainAuthorizedPromoCode codes are skipped (no reservation repo calls). 6. undo() decrements each reserved counter exactly once and is idempotent via the $undone guard. Verified: docker exec summit-api composer dump-autoload # pick up new entity docker exec summit-api vendor/bin/phpunit --filter "PreProcessReservationTask|SagaCompensationTest|ApplyPromoCodeTask" → 13/13 pass (3 PHPUnit deprecations match repo baseline). Outstanding from smarcet's PR #525 review: #7 (discoverPromoCodes N+1) is the only remaining item.
1 parent 2e0ef84 commit 999eec1

4 files changed

Lines changed: 337 additions & 677 deletions

File tree

app/Services/Model/Imp/SummitOrderService.php

Lines changed: 7 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -815,43 +815,13 @@ public function run(array $formerState): array
815815
}
816816
}
817817

818-
// QuantityPerAccount enforcement for domain-authorized promo codes.
819-
//
820-
// IMPORTANT — the condition below is `>`, NOT `>=`, and we do NOT add $qty.
821-
// That is because $existingCount ALREADY INCLUDES the current order's
822-
// tickets. The saga runs ReserveOrderTask before ApplyPromoCodeTask;
823-
// ReserveOrderTask calls $promo_code->applyTo($ticket), which sets
824-
// PromoCodeID on each new ticket, and its transaction commits before
825-
// this task's transaction opens. getTicketCountByMemberAndPromoCode
826-
// is a raw SQL count over SummitAttendeeTicket joined to SummitOrder
827-
// with status IN ('Reserved','Paid','Confirmed'), so the in-flight
828-
// order's freshly-reserved tickets are already counted.
829-
//
830-
// Examples (limit = 2, prior tickets = 0):
831-
// buying 2 -> existingCount=2 -> 2 > 2 false -> allowed (exactly at cap)
832-
// buying 3 -> existingCount=3 -> 3 > 2 true -> rejected
833-
// Examples (limit = 2, prior tickets = 2):
834-
// buying 1 -> existingCount=3 -> 3 > 2 true -> rejected
835-
//
836-
// If the saga order changes, or PromoCodeID assignment moves out of
837-
// ReserveOrderTask, the semantics here break — revisit this check.
838-
if ($promo_code instanceof IDomainAuthorizedPromoCode
839-
&& !is_null($this->owner)
840-
) {
841-
$quantityPerAccount = $promo_code->getQuantityPerAccount();
842-
if ($quantityPerAccount > 0) {
843-
$existingCount = $this->promo_code_repository->getTicketCountByMemberAndPromoCode($this->owner, $promo_code);
844-
if ($existingCount > $quantityPerAccount) {
845-
throw new ValidationException(
846-
sprintf(
847-
"Promo code %s has reached the maximum of %s tickets per account.",
848-
$promo_code_value,
849-
$quantityPerAccount
850-
)
851-
);
852-
}
853-
}
854-
}
818+
// QuantityPerAccount enforcement lives in PreProcessReservationTask.
819+
// That's where the check-and-increment runs atomically under the
820+
// PESSIMISTIC_WRITE row lock on the promo code, BEFORE ReserveOrderTask
821+
// commits tickets. A post-facto count here (as this task did prior to
822+
// the TOCTOU fix) cannot distinguish the current order's freshly-
823+
// committed tickets from a concurrent order's — see smarcet's
824+
// reproduction in tests/Unit/Services/ApplyPromoCodeTaskConcurrencyTest.
855825

856826
Log::debug(sprintf("adding %s usage to promo code %s", $qty, $promo_code->getId()));
857827

tests/Unit/Services/ApplyPromoCodeTaskConcurrencyTest.php

Lines changed: 0 additions & 328 deletions
This file was deleted.

0 commit comments

Comments
 (0)