From 1543160f6a235748f9861924417d3e84cb3279b5 Mon Sep 17 00:00:00 2001 From: Stephen Adamson Date: Fri, 14 Aug 2026 21:13:48 +0100 Subject: [PATCH] =?UTF-8?q?fix:=20loop-B=20full-scope=20adversarial=20find?= =?UTF-8?q?ings=20=E2=80=94=20tip-excluded=20detail=20endpoints,=20=C2=A30?= =?UTF-8?q?-charge=20guard,=2024h=20window,=202FA=20single-use=20everywher?= =?UTF-8?q?e?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Loop B full-scope red-team (money/security/dup-mod) findings: - CRITICAL: booking detail handlers (GetBookingHandler/GetAdminBookingHandler) now exclude payment_type='tip' from amount_paid — a tip before the final balance no longer undercharges the booking (bookings.go x3 sites) - HIGH: A6 deposit clamp adds a zero-guard — when the eligible discount covers the entire deposit, the flow returns deposit_covered_by_discount instead of charging £0 at Square (real Square rejects £0; the mock accepted it); square_dev CreatePayment + CreateRefund now reject Amount <= 0 (mock/prod parity) - HIGH: replayLegitimateRetryWindow restored to 22h (== stalePendingKeyedAge) so sweep-produced duplicate charges are still auto-refunded, not rescued-and-hidden - HIGH: 2FA single-use consume-at-gate applied to ALL saved-card charge gates (booking 2263, admin saved-card 960, tip 4483, till 967, gift-card purchase 1482) with re-issue-on-failed-charge on each; pending-reuse retries keep their code - MEDIUM: 2FA re-issue now fires only when the gate actually consumed a code (fresh saved-card path) — new-card failures no longer silently burn a standing code - MEDIUM: pre_start tip-exclusion consistent across admin lists + detail handlers (bookings.go) - MEDIUM: remaining-balance counts pending refunds (service.go) — capacity consistent with GetBookingPaymentInfo - Mock CreatePayment/CreateRefund reject £0 amounts (INVALID_REQUEST_ERROR) for dev/prod parity 26/26 backend packages; 80/80 frontend tests + build; env-docs 41/41. --- backend/handlers/bookings/bookings.go | 26 +- backend/handlers/payments/giftcards.go | 22 +- backend/handlers/payments/handlers.go | 82 ++++++- .../payments/loop_b_money_fixes_test.go | 152 ++++++++++++ backend/handlers/payments/service.go | 11 +- backend/handlers/payments/sweep.go | 33 ++- backend/handlers/payments/sweep_test.go | 13 +- backend/handlers/payments/till.go | 24 +- .../payments/twofa_gate_consume_test.go | 232 ++++++++++++++++++ backend/internal/square/square_dev.go | 16 ++ backend/internal/square/square_dev_test.go | 35 +++ .../payments/UserPaymentModal.svelte | 16 +- 12 files changed, 605 insertions(+), 57 deletions(-) create mode 100644 backend/handlers/payments/loop_b_money_fixes_test.go create mode 100644 backend/handlers/payments/twofa_gate_consume_test.go diff --git a/backend/handlers/bookings/bookings.go b/backend/handlers/bookings/bookings.go index 764381b..0c6421a 100644 --- a/backend/handlers/bookings/bookings.go +++ b/backend/handlers/bookings/bookings.go @@ -681,6 +681,7 @@ func GetAllAdminBookingsHandler(w http.ResponseWriter, r *http.Request) { SELECT COALESCE(SUM(amount), 0) AS pre_start_amount_paid FROM payments WHERE booking_id = b.id AND status = 'completed' AND created_at < b.start_time + AND payment_type <> 'tip' ) pre_pay ON true ` @@ -1287,7 +1288,13 @@ func GetAdminBookingHandler(w http.ResponseWriter, r *http.Request) { p.InvoiceNumber = &num } booking.Payments = append(booking.Payments, p) - if p.Status == "completed" { + // A tip is gratuity paid beyond the booking total — it must not reduce + // the balance owed. This mirrors the list views (which filter + // payment_type <> 'tip' in SQL) and GetBookingPaymentInfo; without the + // exclusion AmountDue would be understated by the tip and the frontend + // would charge less than the true remaining balance, leaving the + // booking never completed and the merchant short. + if p.Status == "completed" && p.PaymentType != "tip" { amountPaid += p.Amount if p.CreatedAt.Before(booking.StartTime) { preStartAmountPaid += p.Amount @@ -1679,7 +1686,13 @@ func UpdateBookingServicesHandler(w http.ResponseWriter, r *http.Request) { p.InvoiceNumber = &num } booking.Payments = append(booking.Payments, p) - if p.Status == "completed" { + // A tip is gratuity paid beyond the booking total — it must not reduce + // the balance owed. This mirrors the list views (which filter + // payment_type <> 'tip' in SQL) and GetBookingPaymentInfo; without the + // exclusion AmountDue would be understated by the tip and the frontend + // would charge less than the true remaining balance, leaving the + // booking never completed and the merchant short. + if p.Status == "completed" && p.PaymentType != "tip" { amountPaid += p.Amount if p.CreatedAt.Before(booking.StartTime) { preStartAmountPaid += p.Amount @@ -1775,6 +1788,7 @@ func SearchAdminBookingsHandler(w http.ResponseWriter, r *http.Request) { SELECT COALESCE(SUM(amount), 0) AS pre_start_amount_paid FROM payments WHERE booking_id = b.id AND status = 'completed' AND created_at < b.start_time + AND payment_type <> 'tip' ) pre_pay ON true -- NOTE: ILIKE with leading wildcard prevents B-tree index usage. -- At scale, replace with pg_trgm GIN index: CREATE INDEX idx_bookings_search_trgm ON bookings USING GIN (id gin_trgm_ops, notes gin_trgm_ops); @@ -3304,7 +3318,13 @@ func GetBookingHandler(w http.ResponseWriter, r *http.Request) { p.CreatedBy = &pCreatedBy.String } - if p.Status == "completed" { + // A tip is gratuity paid beyond the booking total — it must not reduce + // the balance owed. This mirrors the list views (which filter + // payment_type <> 'tip' in SQL) and GetBookingPaymentInfo; without the + // exclusion AmountDue would be understated by the tip and the frontend + // would charge less than the true remaining balance, leaving the + // booking never completed and the merchant short. + if p.Status == "completed" && p.PaymentType != "tip" { amountPaid += p.Amount if p.CreatedAt.Before(booking.StartTime) { preStartAmountPaid += p.Amount diff --git a/backend/handlers/payments/giftcards.go b/backend/handlers/payments/giftcards.go index 5ba0176..9b16716 100644 --- a/backend/handlers/payments/giftcards.go +++ b/backend/handlers/payments/giftcards.go @@ -1469,12 +1469,17 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { // and SAVING a new card during this purchase (SaveCard), mirroring // CreateBookingPayment/CreateTipPayment. A one-off new-card (nonce) charge // that is not saved is not gated. - // consume=false (MEDIUM-2): the code is verified here but only NULLed - // inside the completed-charge transaction below (ConsumePendingCode), so a - // failed/ambiguous Square charge does NOT burn the operator-relayed code - // and a same-key retry can re-verify the SAME code. + // consume=!reuse (LOW 6a): a FRESH purchase verifies WITH consumption — + // the code is single-use at the gate, closing the TOCTOU where a + // verified-but-unconsumed code could authorize a second charge within its + // lifetime — and a failed Square charge re-issues a fresh code + // (reissueTwoFACodeAfterFailedCharge below). A pending-reuse retry + // (reusePendingID != "") verifies WITHOUT consuming: the code was re-issued + // for exactly this retry and the completed-charge transaction below + // (ConsumePendingCode) burns it on terminal success, so a retry that fails + // again keeps its code for one more attempt. if (req.CardID != nil && *req.CardID != "") || req.SaveCard { - if !requireTwoFactorForCardAccess(w, r, paymentService, userID, req.VerificationCode, false) { + if !requireTwoFactorForCardAccess(w, r, paymentService, userID, req.VerificationCode, reusePendingID == "") { return } } @@ -1691,6 +1696,13 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { paymentResult, err := SquareClient.CreatePayment(ctx, paymentReq) if err != nil { log.Printf("Failed to process gift card purchase payment: %v", err) + // The gate consumed the 2FA code for a FRESH saved-card purchase — + // re-issue so the same-key retry has a live code to verify. A + // pending-reuse retry verified without consuming at the gate, so its + // code survives for one more attempt. + if ((req.CardID != nil && *req.CardID != "") || req.SaveCard) && reusePendingID == "" { + reissueTwoFACodeAfterFailedCharge(ctx, userID) + } // Payment record intentionally left as 'pending' for manual retry. http.Error(w, "Payment failed", chargeFailureStatus(err)) return diff --git a/backend/handlers/payments/handlers.go b/backend/handlers/payments/handlers.go index 092f99b..1f19a06 100644 --- a/backend/handlers/payments/handlers.go +++ b/backend/handlers/payments/handlers.go @@ -949,13 +949,15 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { // AFTER the idempotency dedup/reuse switch above: a same-key retry of // an already-completed payment short-circuits there and returns the // existing result WITHOUT demanding a fresh code — no new money moves, - // so no new authorization is needed. Pending-reuse retries and fresh - // charges still pass through the gate. - // consume=false (MEDIUM-2): the code is verified here but only NULLed - // inside the completed-charge transaction below (ConsumePendingCode), - // so a failed/ambiguous Square charge does NOT burn the operator-relayed - // code and a same-key retry can re-verify the SAME code. - if bookingUserID.Valid && !requireTwoFactorForCardAccess(w, r, service, bookingUserID.String, req.VerificationCode, false) { + // so no new authorization is needed. consume=!reusePendingRecord + // (finding 4): a FRESH charge verifies WITH consumption — the code is + // single-use at the gate, closing the TOCTOU where a verified-but- + // unconsumed code could authorize a second charge — and a pending-reuse + // retry verifies WITHOUT consuming, so a retry that fails again keeps + // its code for one more attempt (the completed-charge transaction + // consumes it on terminal success). + reusePendingRecord := paymentID != "" + if bookingUserID.Valid && !requireTwoFactorForCardAccess(w, r, service, bookingUserID.String, req.VerificationCode, !reusePendingRecord) { return } @@ -1066,6 +1068,15 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { paymentResult, err := SquareClient.CreatePayment(r.Context(), paymentReq) if err != nil { log.Printf("Failed to process saved-card payment: %v (error_code=%q)", err, square.ErrorCode(err)) + // The gate consumed the 2FA code for a fresh saved-card charge — + // re-issue so the same-key retry has a live code to verify + // (mirrors CreateBookingPayment's post-failure re-issue, finding + // 4). Only runs when the gate actually ran (the booking's user is + // known); a pending-reuse retry verified WITHOUT consuming, so a + // fresh code never invalidates anything that still needs verifying. + if bookingUserID.Valid { + reissueTwoFACodeAfterFailedCharge(r.Context(), bookingUserID.String) + } http.Error(w, "Payment failed", chargeFailureStatus(err)) return } @@ -2212,6 +2223,30 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { } } + // A6 (money): when the eligible campaign credit covers the ENTIRE + // remaining obligation, the deposit charge clamps to £0. Charging £0 at + // Square is a provable INVALID_REQUEST_ERROR in production (the pending + // row + Square call would fail forever and block the flow), and the dev + // mock used to ACCEPT £0 and mint a completed £0 deposit that consumed + // the discount — leaving the booking unpaid and the 'full' balance + // charge to overcharge later. There is nothing to charge, so skip the + // Square call entirely and report the discount-covered deposit; the + // flow completes without moving any money. The discount rows themselves + // are applied by the next real charge or at booking completion + // (applyEligibleCampaignsAtPayment). + if chargeAmount <= 0 { + log.Printf("Deposit for booking %s fully covered by %d pence of eligible campaign credit — skipping the Square charge", bookingID, eligibleDiscountPence) + mw.RespondJSON(w, http.StatusOK, map[string]any{ + "id": "", + "booking_id": bookingID, + "payment_type": req.PaymentType, + "status": "completed", + "amount": 0, + "deposit_covered_by_discount": true, + }) + return + } + var sourceID string var savedCardID *string var savedCardCustomerID string @@ -2353,9 +2388,17 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { paymentResult, err := SquareClient.CreatePayment(r.Context(), paymentReq) if err != nil { log.Printf("Failed to create payment: %v (error_code=%q)", err, square.ErrorCode(err)) - // The gate consumed the 2FA code for a fresh charge — re-issue so the - // same-key retry has a live code to verify. - reissueTwoFACodeAfterFailedCharge(r.Context(), userID) + // The gate consumed the 2FA code for a fresh saved-card charge — + // re-issue so the same-key retry has a live code to verify. A NEW-CARD + // (cnon) charge never gated and involves no code: re-issuing here would + // overwrite the customer's standing pending code with a fresh + // undelivered one, silently burning the code the operator relayed + // (finding 5). Pending-reuse saved-card retries verified WITHOUT + // consuming, so re-issuing keeps a live code available for the retry + // (the completed-charge transaction burns it on terminal success). + if req.CardID != nil && *req.CardID != "" { + reissueTwoFACodeAfterFailedCharge(r.Context(), userID) + } http.Error(w, "Payment failed", chargeFailureStatus(err)) return } @@ -4429,10 +4472,15 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { // idempotency dedup's completed short-circuit (Loop B MEDIUM): a same-key // lost-response retry returns the already-completed payment above without // re-entering the gate, so its single-use code (already consumed by the - // original attempt) is never re-rejected as "expired". Pending-reuse and - // fresh paths still gate — a new charge may move at Square. + // original attempt) is never re-rejected as "expired". consume=!reusePendingRecord + // (finding 4): a FRESH charge verifies WITH consumption — the code is + // single-use at the gate, closing the TOCTOU where a verified-but- + // unconsumed code could authorize a second charge — and a pending-reuse + // retry verifies WITHOUT consuming, so a retry that fails again keeps its + // code for one more attempt (the completed-charge transaction consumes it + // on terminal success). if req.CardID != nil && *req.CardID != "" { - if !requireTwoFactorForCardAccess(w, r, service, userID, req.VerificationCode, false) { + if !requireTwoFactorForCardAccess(w, r, service, userID, req.VerificationCode, !reusePendingRecord) { return } } @@ -4544,6 +4592,14 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { if err != nil { log.Printf("Failed to create tip payment: %v (error_code=%q)", err, square.ErrorCode(err)) // Payment record intentionally left as 'pending' for manual retry. + // The gate consumed the 2FA code for a fresh saved-card charge — + // re-issue so the same-key retry has a live code to verify (mirrors + // CreateBookingPayment's post-failure re-issue, finding 4). A NEW-CARD + // (cnon) charge never gated and involves no code — re-issuing would + // overwrite a standing pending code with an undelivered one (finding 5). + if req.CardID != nil && *req.CardID != "" { + reissueTwoFACodeAfterFailedCharge(r.Context(), userID) + } http.Error(w, "Payment failed", chargeFailureStatus(err)) return } diff --git a/backend/handlers/payments/loop_b_money_fixes_test.go b/backend/handlers/payments/loop_b_money_fixes_test.go new file mode 100644 index 0000000..73b422a --- /dev/null +++ b/backend/handlers/payments/loop_b_money_fixes_test.go @@ -0,0 +1,152 @@ +//go:build test && dev + +package payments + +import ( + "context" + "encoding/json" + "net/http" + "testing" + "time" + + "crussell/clock" + "crussell/db" + "crussell/internal/square" + "crussell/testutils" + "crussell/testutils/fixtures" + "crussell/testutils/jwt" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// ============================================================================= +// Finding 2 — the A6 deposit clamp can produce chargeAmount=0 with no guard: +// the handler then charged £0 at Square (invalid in prod, minted a completed +// £0 deposit in the dev mock that consumed the discount). A deposit whose +// eligible campaign credit covers the ENTIRE remaining obligation must skip the +// Square call and report deposit_covered_by_discount. +// ============================================================================= + +// seedActiveCampaign inserts an active time-based campaign with the given +// discount percent and returns its id. +func seedActiveCampaign(t *testing.T, ctx context.Context, q db.Querier, percent int) string { + t.Helper() + now := clock.Now() + var id string + err := q.QueryRow(ctx, ` + INSERT INTO discount_campaigns (name, campaign_type, discount_percent, status, start_date, end_date, times_redeemed) + VALUES ($1, 'time_based', $2, 'active', $3, $4, 0) + RETURNING id + `, "Money-Fix Campaign", percent, now.Add(-24*time.Hour), now.Add(24*time.Hour)).Scan(&id) + require.NoError(t, err) + return id +} + +// failOnChargeClient fails the test if a Square charge is attempted. Proves a +// discount-covered deposit skips the Square call entirely (finding 2). +type failOnChargeClient struct { + square.SquareClient + t *testing.T +} + +func (c *failOnChargeClient) CreatePayment(ctx context.Context, req square.CreatePaymentReq) (*square.PaymentResult, error) { + c.t.Fatalf("Square CreatePayment must NOT be called for a discount-covered deposit (amount=%d)", req.Amount) + return nil, nil +} + +func TestBookingPayment_DepositFullyCoveredByDiscount_SkipsSquareCharge(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestData(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + // 100% time-based campaign = £50 discount on the £50 fixture booking, which + // fully covers the £25 deposit request (chargeAmount clamps to £0). + seedActiveCampaign(t, ctx, tx, 100) + + origClient := SquareClient + SquareClient = &failOnChargeClient{SquareClient: square.NewDevClient(), t: t} + defer func() { SquareClient = origClient }() + + cardToken := "cnon:deposit-covered" + req := CreateBookingPaymentRequest{ + Amount: 2500, // £25 deposit + PaymentType: "deposit", + NewCardToken: &cardToken, + IdempotencyKey: "deposit-covered-" + bookingID, + } + + handler := CreateBookingPayment + w := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusOK, w.Code, "a discount-covered deposit must complete without a Square charge, body: %s", w.Body.String()) + + var body map[string]any + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) + assert.Equal(t, true, body["deposit_covered_by_discount"], "the response must signal the discount-covered deposit") + + var payCount int + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM payments WHERE booking_id = $1`, bookingID).Scan(&payCount)) + assert.Zero(t, payCount, "no payment row may be recorded for a discount-covered deposit") + + // The discount rows are applied by the next real charge / at completion, + // never minted for a charge that did not happen. + var discountCount int + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM booking_discounts WHERE booking_id = $1`, bookingID).Scan(&discountCount)) + assert.Zero(t, discountCount, "no discount may be applied for a charge that never happened") +} + +// ============================================================================= +// Finding 7 — remaining-balance capacity ignored PENDING refunds: service.go +// counted only completed refunds in GetBookingRemainingBalancePence while +// GetBookingPaymentInfo counts completed + pending. An in-flight refund +// understated the remaining balance and blocked a legitimate retry. +// ============================================================================= + +func TestGetBookingRemainingBalancePence_PendingRefundsReopenCapacity(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + + serviceID, err := fixtures.CreateTestService(tx) + require.NoError(t, err) + + bookingID, err := fixtures.CreateTestBooking(tx, userID, serviceID) + require.NoError(t, err) + + var bookingTotal int64 + require.NoError(t, tx.QueryRow(ctx, `SELECT ROUND(total_amount * 100)::bigint FROM bookings WHERE id = $1`, bookingID).Scan(&bookingTotal)) + + svc := NewPaymentService() + + // Pay the full booking amount. + _, err = svc.CreatePaymentRecord(ctx, PaymentRecord{ + BookingID: bookingID, + PaymentType: "full", + PaymentMethod: "online_square", + Status: "completed", + Amount: float64(bookingTotal) / 100.0, + }, nil) + require.NoError(t, err) + + remaining, err := svc.GetBookingRemainingBalancePence(ctx, bookingID) + require.NoError(t, err) + require.Equal(t, int64(0), remaining, "a fully-paid booking must have 0 remaining") + + // A PENDING refund is money in flight that will come back — it must re-open + // capacity by its amount exactly like a completed refund. + var payRowID string + require.NoError(t, tx.QueryRow(ctx, `SELECT id FROM payments WHERE booking_id = $1 AND payment_type = 'full' ORDER BY created_at DESC LIMIT 1`, bookingID).Scan(&payRowID)) + + refundAmount := bookingTotal / 2 + _, err = tx.Exec(ctx, ` + INSERT INTO refunds (payment_id, booking_id, amount, status, reason, origin) + VALUES ($1, $2, $3, 'pending', 'in-flight test refund', 'manual') + `, payRowID, bookingID, float64(refundAmount)/100.0) + require.NoError(t, err) + + remaining, err = svc.GetBookingRemainingBalancePence(ctx, bookingID) + require.NoError(t, err) + require.Equal(t, refundAmount, remaining, "a pending refund must re-open the remaining balance by its amount") +} diff --git a/backend/handlers/payments/service.go b/backend/handlers/payments/service.go index 3d219a0..e301c65 100644 --- a/backend/handlers/payments/service.go +++ b/backend/handlers/payments/service.go @@ -524,10 +524,13 @@ func (s *PaymentService) GetBookingRemainingBalancePence(ctx context.Context, bo SELECT COALESCE(SUM(r.amount), 0) AS refunded_pounds FROM refunds r JOIN payments p ON r.payment_id = p.id - WHERE p.booking_id = $1 AND r.status = 'completed' - -- A tip refund returns gratuity, not booking money — it must not - -- re-open booking charge capacity (mirror of the paid_total tip - -- exclusion above). + -- Pending refunds count too (matching GetBookingPaymentInfo): a + -- refund in flight is money that will come back, so the remaining + -- capacity must not be understated while it settles — understating + -- it blocks a legitimate retry (finding 7). A tip refund returns + -- gratuity, not booking money — it must not re-open booking charge + -- capacity (mirror of the paid_total tip exclusion above). + WHERE p.booking_id = $1 AND r.status IN ('completed', 'pending') AND p.payment_type <> 'tip' ) -- Money-safety (M-cap): refunds return money, so they re-open booking diff --git a/backend/handlers/payments/sweep.go b/backend/handlers/payments/sweep.go index 376c706..858575c 100644 --- a/backend/handlers/payments/sweep.go +++ b/backend/handlers/payments/sweep.go @@ -665,23 +665,22 @@ func clawbackTillSaleFunding(ctx context.Context, r staleRow) bool { // creation and a replayed payment's creation for the payment to be the REAL // charge under a legitimately replayed key. A same-key retry — the documented // retry path (handlers.go:1579-1591) — creates its charge somewhere between -// the row's creation and the 24h retry-eligible window (the same-key retry -// path stays valid until Square's ~24h idempotency-key retention expires), so -// any COMPLETED payment created within [row.CreatedAt, row.CreatedAt + -// replayLegitimateRetryWindow] can be that retry charge and must be rescued. +// the row's creation and the retry-eligible window, so any COMPLETED payment +// created within [row.CreatedAt, row.CreatedAt + replayLegitimateRetryWindow] +// can be that retry charge and must be rescued. // -// The window is 24h — the full Square idempotency-key retention window. The -// sweep's own replay only runs once a row is at least 22h old -// (stalePendingKeyedAge); a legitimate same-key retry can still land up to 24h -// after the row, and a shorter window would misclassify a retry landing in the -// 22-24h zone as a NEW expired-key replay and auto-refund the customer's -// legitimate charge (B1). A payment created LATER than 24h after the row is the -// classic expired-key replay-induced charge — the sweep just created it by -// replaying the still valid saved-card source under a key Square no longer -// retains — and rescuing it would hide the duplicate charge behind the -// original row (finding A1). The boundary is inclusive: a payment created -// EXACTLY 24h after the row is still within the legitimate window. -const replayLegitimateRetryWindow = 24 * time.Hour +// The window is 22h, matching stalePendingKeyedAge: the sweep first replays a +// keyed row once it is 22h old, so a LEGITIMATE same-key retry charge can only +// have landed before that first sweep replay (a retry made after the replay is +// the sweep's OWN expired-key replay, which must NOT be rescued). A window +// larger than the sweep cutoff would hide that second charge behind the +// original row: the sweep replays the row at 22h, Square's ~24h key retention +// may have lapsed, the replay creates a NEW charge at row+22h..24h that lands +// INSIDE the window, and the sweep rescues it as the "legitimate retry" — a +// real double-charge silently hidden (finding 3). The boundary is inclusive: a +// payment created EXACTLY 22h after the row is still within the legitimate +// window. +const replayLegitimateRetryWindow = 22 * time.Hour // replayRescueLowerBoundSkew is the lower-bound tolerance for a replayed // COMPLETED payment to still be treated as the ORIGINAL charge under a retained @@ -712,7 +711,7 @@ func replayMatchesRowAmount(r staleRow, pr *square.PaymentResult) bool { // replayWithinLegitimateWindow reports whether a replayed COMPLETED payment is // the REAL charge this pending row is waiting on — the ORIGINAL charge under a // retained key (created ~at row creation) or a later SAME-KEY RETRY charge -// (created between the row's creation and the 24h retry-eligible window, F2). +// (created between the row's creation and the 22h retry-eligible window, F2). // The amount must match the row (a retry can never change it) and the payment // must have been created within replayLegitimateRetryWindow of the row. The // source is matched by construction: the replay body is rebuilt from the row's diff --git a/backend/handlers/payments/sweep_test.go b/backend/handlers/payments/sweep_test.go index 266ebeb..691a99f 100644 --- a/backend/handlers/payments/sweep_test.go +++ b/backend/handlers/payments/sweep_test.go @@ -756,8 +756,9 @@ func TestSweepStalePendingPayments_KeyedReplayNewCharge_AutoRefunded(t *testing. } // The replayed COMPLETED payment's created_at is 25h AFTER the pending row - // — beyond the 24h legitimate-retry window (replayLegitimateRetryWindow), - // so it is provably a NEW expired-key replay charge, not a same-key retry. + // — beyond the 22h legitimate-retry window (replayLegitimateRetryWindow, + // matching the sweep's 22h keyed cutoff), so it is provably a NEW expired-key + // replay charge, not a same-key retry. // The cross-check runs only in a non-dev/mock env, so the env is flipped to // production for the sweep (sequential, like the 2FA tests). The dev mock // is constructed BEFORE the flip (NewDevClient refuses production without @@ -1493,8 +1494,10 @@ func TestSweepStalePendingPayments_KeyedReplaySlightlyBeforeRow_Rescues(t *testi // row (between the old 21h window and the 22h sweep cutoff) is the REAL charge // under a legitimately replayed key and MUST be rescued. The pre-B2 21h window // refused it and stranded the row pending. The legitimate-retry boundary is now -// replayLegitimateRetryWindow (24h): only a payment created AFTER -// row.CreatedAt+24h can be the sweep's own expired-key replay. +// replayLegitimateRetryWindow (22h, matching stalePendingKeyedAge): only a +// payment created AFTER row.CreatedAt+22h can be the sweep's own expired-key +// replay — a 24h window would rescue a charge the sweep itself minted between +// 22h and 24h and hide a real double-charge (finding 3). func TestSweepStalePendingPayments_KeyedReplayRetryAt215h_Rescues(t *testing.T) { ctx, tx := testutils.SetupTestTx(t) @@ -1522,7 +1525,7 @@ func TestSweepStalePendingPayments_KeyedReplayRetryAt215h_Rescues(t *testing.T) } // The replayed COMPLETED payment is a legitimate same-key retry created - // 21.5h after the row — inside the 24h legitimate window, so it is the real + // 21.5h after the row — inside the 22h legitimate window, so it is the real // charge and must be rescued, not refused as an expired-key duplicate. var rowCreatedAt time.Time if err := tx.QueryRow(ctx, "SELECT created_at FROM payments WHERE id = $1", staleID).Scan(&rowCreatedAt); err != nil { diff --git a/backend/handlers/payments/till.go b/backend/handlers/payments/till.go index b886c8e..8d7e067 100644 --- a/backend/handlers/payments/till.go +++ b/backend/handlers/payments/till.go @@ -954,12 +954,17 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { } // 2FA gating (C5): charging a customer's saved card requires 2FA when - // the feature is enforced. consume=false (MEDIUM-2): the code is - // verified here but only NULLed once the charge reaches its terminal - // success state below (ConsumePendingCode), so a failed/ambiguous - // Square charge does NOT burn the operator-relayed code and a same-key - // retry can re-verify the SAME code. - if cardUserID.Valid && !requireTwoFactorForCardAccess(w, r, service, cardUserID.String, req.VerificationCode, false) { + // the feature is enforced. consume=!reuse (LOW 6a): a FRESH charge + // verifies WITH consumption — the code is single-use at the gate, + // closing the TOCTOU where a verified-but-unconsumed code could + // authorize a second charge within its lifetime — and a failed Square + // charge re-issues a fresh code (reissueTwoFACodeAfterFailedCharge + // below). A pending-reuse retry (existingPendingID != "") verifies + // WITHOUT consuming: the code was re-issued for exactly this retry and + // the post-charge success path below (ConsumePendingCode) burns it on + // terminal success, so a retry that fails again keeps its code for one + // more attempt. + if cardUserID.Valid && !requireTwoFactorForCardAccess(w, r, service, cardUserID.String, req.VerificationCode, existingPendingID == "") { return } @@ -1252,6 +1257,13 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { log.Printf("CRITICAL: till sale %s charge definitively failed (%v) but gift-card clawback also failed: %v — MANUAL RECONCILIATION REQUIRED: gift card %s may still be funded", tillSaleID, squareErr, revErr, giftCardID) } } + // The gate consumed the 2FA code for a FRESH saved-card charge — + // re-issue so the same-key retry has a live code to verify. A + // pending-reuse retry verified without consuming at the gate, so + // its code survives for one more attempt. + if req.PaymentMethod == "saved_card" && cardUserID.Valid && existingPendingID == "" { + reissueTwoFACodeAfterFailedCharge(ctx, cardUserID.String) + } // 402 only for definitive declines; ambiguous transport/5xx must be // 503 so the pending sale stays resumable on a same-key retry // (M3). The clawback decision above stays keyed on diff --git a/backend/handlers/payments/twofa_gate_consume_test.go b/backend/handlers/payments/twofa_gate_consume_test.go new file mode 100644 index 0000000..416b511 --- /dev/null +++ b/backend/handlers/payments/twofa_gate_consume_test.go @@ -0,0 +1,232 @@ +//go:build test && dev + +package payments + +// Tests pinning the Loop B 2FA single-use consume-at-gate fix on the two +// saved-card charge gates that were still consuming AFTER the charge landed: +// the till saved-card charge (till.go) and the gift-card purchase saved-card +// gate (giftcards.go). +// +// Semantics (mirroring CreateBookingPayment, handlers.go): +// - A FRESH charge verifies the 2FA code WITH consumption at the gate +// (consume = !reusePendingRecord). The code is single-use, closing the +// TOCTOU where a verified-but-unconsumed code could authorize a second +// concurrent charge. If Square then fails, a fresh code is re-issued +// (reissueTwoFACodeAfterFailedCharge) so the same-key retry has a live +// code to verify. +// - A PENDING-REUSE retry verifies WITHOUT consuming: the code was re-issued +// for exactly this retry, and the post-charge success path consumes it on +// terminal success, so a retry that fails again keeps its code for one +// more attempt. +// +// These tests flip REQUIRE_2FA/SQUARE_ENVIRONMENT via t.Setenv and therefore +// must stay sequential (no t.Parallel) — see the note at the top of +// twofa_test.go. They use SQUARE_ENVIRONMENT=staging (NOT production) for +// enforcement: twoFactorEnforced() is fail-closed, so any non-mock/dev value +// enforces the gate, while square.NewDevClient() — which the injected fault +// client and structuredSquareErrorWithCode construct at call time — returns +// the in-memory mock for every env except production/sandbox. The shared +// helperEnvEnforce2FA sets production, which would panic NewDevClient in a +// dev build. + +import ( + "context" + "database/sql" + "net/http" + "testing" + + "crussell/internal/square" + "crussell/internal/twofa" + "crussell/testutils" + "crussell/testutils/fixtures" + "crussell/testutils/jwt" + + "github.com/stretchr/testify/require" +) + +// TestTwoFactorEnforced_CreateTillSale_SavedCard_FreshCharge_Failure pins the +// till saved-card gate on a FRESH charge: the 2FA code is consumed AT THE GATE +// (single-use), and a failed Square charge re-issues a fresh code so the +// same-key retry can verify again. The stored hash must differ from the seeded +// one — if the gate still used consume=false the seeded hash would survive +// unchanged and there would be no re-issue. +func TestTwoFactorEnforced_CreateTillSale_SavedCard_FreshCharge_Failure_ConsumesAtGate_Reissues(t *testing.T) { + t.Setenv("REQUIRE_2FA", "true") + t.Setenv("SQUARE_ENVIRONMENT", "staging") + ctx, tx := testutils.SetupTestTx(t) + + adminID, err := fixtures.CreateTestAdminUser(tx) + require.NoError(t, err) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + adminToken := jwt.GenerateTestToken(adminID, "admin") + seedTwoFAPendingCode(t, tx, userID, "556677") + + cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "ccof:sq_test_card_id", "VISA", "1234") + require.NoError(t, err) + + origClient := SquareClient + SquareClient = &definitiveChargeClient{SquareClient: square.NewDevClient(), createErr: structuredSquareErrorWithCode(t, http.StatusPaymentRequired, "CARD_DECLINED")} + defer func() { SquareClient = origClient }() + + req := TillSaleRequest{ + ItemType: "gift_card", + Action: "create", + Amount: 50.00, + PaymentMethod: "saved_card", + UserSavedCardID: &cardID, + UserID: &userID, + IdempotencyKey: "2fa-till-fresh-decline", + VerificationCode: "556677", + } + + w := makePaymentRequest(CreateTillSale, "POST", "/api/admin/till/sale", req, adminToken, ctx) + require.Equal(t, http.StatusPaymentRequired, w.Code, w.Body.String()) + + var hash sql.NullString + require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_pending_code_hash FROM users WHERE id = $1", userID).Scan(&hash)) + require.True(t, hash.Valid, "a failed fresh saved-card charge must re-issue a live 2FA code for the same-key retry") + require.NotEqual(t, twofa.Hash("556677"), hash.String, "the gate must have consumed the seeded code at verification time (single-use)") +} + +// TestTwoFactorEnforced_CreateTillSale_SavedCard_PendingReuse_Failure pins the +// till saved-card gate on a PENDING-REUSE retry: the code is verified WITHOUT +// consumption, so a retry that fails again keeps its seeded code unchanged and +// no re-issue runs (the fresh-charge-only guard must not fire). +func TestTwoFactorEnforced_CreateTillSale_SavedCard_PendingReuse_Failure_KeepsCode(t *testing.T) { + t.Setenv("REQUIRE_2FA", "true") + t.Setenv("SQUARE_ENVIRONMENT", "staging") + ctx, tx := testutils.SetupTestTx(t) + + adminID, err := fixtures.CreateTestAdminUser(tx) + require.NoError(t, err) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + adminToken := jwt.GenerateTestToken(adminID, "admin") + seedTwoFAPendingCode(t, tx, userID, "667788") + + cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "ccof:sq_test_card_id", "VISA", "1234") + require.NoError(t, err) + + // Seed a PENDING till_sale with the same key — a prior attempt whose + // Square charge failed after the DB transaction committed (card funded). + key := "2fa-till-pending-reuse" + var giftCardID string + require.NoError(t, tx.QueryRow(ctx, ` + INSERT INTO gift_cards (total_funds_added, amount_remaining, created_by, is_inventory, voucher_type_at_purchase) + VALUES (50.00, 50.00, $1, FALSE, 'SPV') RETURNING id + `, adminID).Scan(&giftCardID)) + _, err = tx.Exec(ctx, ` + INSERT INTO till_sales (item_type, item_id, description, quantity, unit_price, total_amount, + payment_method, status, user_id, user_saved_card_id, idempotency_key, created_by, created_at, updated_at) + VALUES ('gift_card', $1, 'Gift Card create', 1, 50.00, 50.00, 'online_square', 'pending', + $2, $3, $4, $5, NOW(), NOW()) + `, giftCardID, userID, cardID, key, adminID) + require.NoError(t, err) + + origClient := SquareClient + SquareClient = &definitiveChargeClient{SquareClient: square.NewDevClient(), createErr: structuredSquareErrorWithCode(t, http.StatusPaymentRequired, "CARD_DECLINED")} + defer func() { SquareClient = origClient }() + + req := TillSaleRequest{ + ItemType: "gift_card", + Action: "create", + Amount: 50.00, + PaymentMethod: "saved_card", + UserSavedCardID: &cardID, + UserID: &userID, + IdempotencyKey: key, + VerificationCode: "667788", + } + + w := makePaymentRequest(CreateTillSale, "POST", "/api/admin/till/sale", req, adminToken, ctx) + require.Equal(t, http.StatusPaymentRequired, w.Code, w.Body.String()) + + var hash sql.NullString + require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_pending_code_hash FROM users WHERE id = $1", userID).Scan(&hash)) + require.True(t, hash.Valid, "a pending-reuse retry must not consume the code at the gate") + require.Equal(t, twofa.Hash("667788"), hash.String, "a failed pending-reuse retry must keep its seeded code unchanged (no re-issue)") +} + +// TestTwoFactorEnforced_BuyGiftCard_SavedCard_Fresh_Failure pins the gift-card +// purchase saved-card gate on a FRESH charge: consume-at-gate + re-issue on +// failure, exactly as the till gate above. +func TestTwoFactorEnforced_BuyGiftCard_SavedCard_Fresh_Failure_ConsumesAtGate_Reissues(t *testing.T) { + t.Setenv("REQUIRE_2FA", "true") + t.Setenv("SQUARE_ENVIRONMENT", "staging") + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + token := jwt.GenerateTestToken(userID, "verified_email") + seedTwoFAPendingCode(t, tx, userID, "112233") + + cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "ccof:sq_test_card_id", "VISA", "1234") + require.NoError(t, err) + + origClient := SquareClient + SquareClient = &definitiveChargeClient{SquareClient: square.NewDevClient(), createErr: structuredSquareErrorWithCode(t, http.StatusPaymentRequired, "CARD_DECLINED")} + defer func() { SquareClient = origClient }() + + req := BuyGiftCardRequest{ + Amount: 2000, + RecipientType: "self", + CardID: &cardID, + IdempotencyKey: "2fa-buy-gc-fresh-decline", + VerificationCode: "112233", + } + + w := makePaymentRequest(BuyGiftCard, "POST", "/api/user/giftcards/buy", req, token, ctx) + require.Equal(t, http.StatusPaymentRequired, w.Code, w.Body.String()) + + var hash sql.NullString + require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_pending_code_hash FROM users WHERE id = $1", userID).Scan(&hash)) + require.True(t, hash.Valid, "a failed fresh saved-card purchase must re-issue a live 2FA code for the same-key retry") + require.NotEqual(t, twofa.Hash("112233"), hash.String, "the gate must have consumed the seeded code at verification time (single-use)") +} + +// TestTwoFactorEnforced_BuyGiftCard_SavedCard_PendingReuse_Failure pins the +// gift-card purchase gate on a PENDING-REUSE retry: verify-without-consume and +// no re-issue, so the seeded code survives a second failed retry unchanged. +func TestTwoFactorEnforced_BuyGiftCard_SavedCard_PendingReuse_Failure_KeepsCode(t *testing.T) { + t.Setenv("REQUIRE_2FA", "true") + t.Setenv("SQUARE_ENVIRONMENT", "staging") + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + token := jwt.GenerateTestToken(userID, "verified_email") + seedTwoFAPendingCode(t, tx, userID, "334455") + + cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "ccof:sq_test_card_id", "VISA", "1234") + require.NoError(t, err) + + // Seed a PENDING payment record with the same key — a prior attempt whose + // Square charge failed (mirrors TestBuyGiftCard_RetryPending_ReattemptsCharge). + key := "2fa-buy-gc-pending-reuse" + _, err = tx.Exec(context.Background(), ` + INSERT INTO payments (payment_type, payment_method, status, amount, idempotency_key, created_at, updated_at, created_by) + VALUES ('full', 'online_square', 'pending', 20.00, $1, NOW(), NOW(), $2) + `, key, userID) + require.NoError(t, err) + + origClient := SquareClient + SquareClient = &definitiveChargeClient{SquareClient: square.NewDevClient(), createErr: structuredSquareErrorWithCode(t, http.StatusPaymentRequired, "CARD_DECLINED")} + defer func() { SquareClient = origClient }() + + req := BuyGiftCardRequest{ + Amount: 2000, + RecipientType: "self", + CardID: &cardID, + IdempotencyKey: key, + VerificationCode: "334455", + } + + w := makePaymentRequest(BuyGiftCard, "POST", "/api/user/giftcards/buy", req, token, ctx) + require.Equal(t, http.StatusPaymentRequired, w.Code, w.Body.String()) + + var hash sql.NullString + require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_pending_code_hash FROM users WHERE id = $1", userID).Scan(&hash)) + require.True(t, hash.Valid, "a pending-reuse retry must not consume the code at the gate") + require.Equal(t, twofa.Hash("334455"), hash.String, "a failed pending-reuse retry must keep its seeded code unchanged (no re-issue)") +} diff --git a/backend/internal/square/square_dev.go b/backend/internal/square/square_dev.go index 9ea7ce1..eecf7dd 100644 --- a/backend/internal/square/square_dev.go +++ b/backend/internal/square/square_dev.go @@ -297,6 +297,22 @@ func (m *MockClient) CreatePayment(ctx context.Context, req CreatePaymentReq) (* if !isTokenLike(req.SourceID) { return nil, fmt.Errorf("invalid source_id: %s — use a card nonce (cnon:xxx) or card ID (ccof:xxx)", tokenPrefix(req.SourceID)) } + // Square's CreatePayment requires a positive amount_money — a missing or + // zero amount is rejected (400 INVALID_REQUEST_ERROR), never treated as a + // no-op. The mock mirrors the rejection so a caller that tries to charge + // £0 (e.g. a deposit fully covered by a campaign discount) fails loudly in + // dev instead of minting a completed £0 payment that real Square would + // never accept (finding 2). + if req.Amount <= 0 { + return nil, &squareAPIError{ + Code: "INVALID_REQUEST_ERROR", + Category: "INVALID_REQUEST_ERROR", + Field: "amount_money", + Detail: "The payment amount must be greater than zero", + StatusCode: http.StatusBadRequest, + err: errors.New("square: payment amount must be positive (amount_money is required)"), + } + } // Square requires customer_id when charging a card-on-file (ccof:) token. // The mock enforces the same rule so dev parity catches the production bug // where a saved-card charge is sent without the customer's Square customer diff --git a/backend/internal/square/square_dev_test.go b/backend/internal/square/square_dev_test.go index 3e88bba..6d10896 100644 --- a/backend/internal/square/square_dev_test.go +++ b/backend/internal/square/square_dev_test.go @@ -73,6 +73,41 @@ func TestDevClient_CreatePayment_ReturnsCompleted(t *testing.T) { assert.NotEmpty(t, result.LocationID) } +func TestDevClient_CreatePayment_RejectsZeroOrNegativeAmount(t *testing.T) { + client := NewDevClient().(*MockClient) + + ctx := context.Background() + req := CreatePaymentReq{ + Amount: 0, + Currency: "GBP", + SourceID: "cnon:test-card", + IdempotencyKey: "test-key-zero-amount", + ReferenceID: "booking-0", + } + + // Real Square rejects a zero amount_money with 400 INVALID_REQUEST_ERROR — + // the mock must mirror that so a £0 charge (e.g. a deposit fully covered by + // a campaign discount) can never be masked in dev (finding 8). + result, err := client.CreatePayment(ctx, req) + require.Error(t, err, "a £0 payment must be rejected exactly like real Square") + require.Nil(t, result, "a rejected payment must not be recorded") + var apiErr *squareAPIError + require.ErrorAs(t, err, &apiErr) + assert.Equal(t, "INVALID_REQUEST_ERROR", apiErr.Category) + assert.Equal(t, http.StatusBadRequest, apiErr.StatusCode) + + // Negative amounts are equally invalid. + req.Amount = -1 + req.IdempotencyKey = "test-key-neg-amount" + _, err = client.CreatePayment(ctx, req) + require.Error(t, err, "a negative payment must be rejected") + + // The zero-amount key must NOT have been registered for dedup. + client.mu.RLock() + defer client.mu.RUnlock() + assert.NotContains(t, client.paymentByKey, "test-key-zero-amount", "a rejected payment must not be registered under its idempotency key") +} + func TestDevClient_CreateCheckout_PendingThenCompleted(t *testing.T) { client := NewDevClient().(*MockClient) diff --git a/frontend/src/lib/components/payments/UserPaymentModal.svelte b/frontend/src/lib/components/payments/UserPaymentModal.svelte index 27e38d0..877bb8c 100644 --- a/frontend/src/lib/components/payments/UserPaymentModal.svelte +++ b/frontend/src/lib/components/payments/UserPaymentModal.svelte @@ -558,14 +558,22 @@ newCardTokenizedForSaveCard = false; twoFactor.setCode(''); twoFactor.reveal = false; + // The backend skips the Square charge entirely when an eligible + // campaign discount covers the whole deposit + // (`deposit_covered_by_discount` — a £0 charge is invalid at + // Square). Report it as a completed, nothing-to-pay deposit. paymentResult = { - id: data.id, - amount: data.amount, + id: data.id ?? '', + amount: data.amount ?? 0, card_brand: data.card_brand, card_last4: data.card_last4, - payment_type: data.payment_type + payment_type: data.payment_type ?? 'deposit' }; - toast.success('Payment successful'); + toast.success( + data.deposit_covered_by_discount + ? 'Deposit covered by your discount — nothing to pay' + : 'Payment successful' + ); savedCardsStore.invalidate(); onComplete(); releaseLock();