diff --git a/backend/auth/jwt.go b/backend/auth/jwt.go index ba17d06..13f56c3 100644 --- a/backend/auth/jwt.go +++ b/backend/auth/jwt.go @@ -217,14 +217,30 @@ func familyAliveStore(key string, alive bool) { // (refresh-token reuse kill, logout) so bound access tokens die on their next // verification instead of riding the cache TTL (HIGH 1). func InvalidateFamilyAlive(familyID string) { - if familyID == "" { + InvalidateFamilyAliveBatch([]string{familyID}) +} + +// InvalidateFamilyAliveBatch drops every cached family-alive verdict for a set +// of families so the next VerifyToken re-queries the DB. Called wherever +// rotation families are deleted — the reuse kill, logout, and the scheduled +// cleanup of expired refresh tokens (handlers/scheduling/scheduled-cleanup.go) +// — so access tokens bound to a family whose last member was deleted die on +// their next verification instead of riding the familyAliveCacheTTL (LOW 6 / +// finding 3). Empty and blank family ids are skipped. +func InvalidateFamilyAliveBatch(familyIDs []string) { + if len(familyIDs) == 0 { return } familyAliveCache.mu.Lock() defer familyAliveCache.mu.Unlock() - for k := range familyAliveCache.m { - if strings.HasPrefix(k, familyID+"|") { - delete(familyAliveCache.m, k) + for _, familyID := range familyIDs { + if familyID == "" { + continue + } + for k := range familyAliveCache.m { + if strings.HasPrefix(k, familyID+"|") { + delete(familyAliveCache.m, k) + } } } } diff --git a/backend/auth/jwt_test.go b/backend/auth/jwt_test.go index f60cc84..cb96916 100644 --- a/backend/auth/jwt_test.go +++ b/backend/auth/jwt_test.go @@ -899,3 +899,36 @@ func TestInvalidateFamilyAliveByUser(t *testing.T) { // An empty user id is a no-op, never a panic. InvalidateFamilyAliveByUser("") } + +// TestInvalidateFamilyAliveBatch pins the LOW finding-3 contract: the batch +// form drops every cached verdict for the affected families (used by the +// scheduled cleanup when expired refresh tokens kill whole families) while +// leaving unrelated families untouched. Empty and blank ids are no-ops. +func TestInvalidateFamilyAliveBatch(t *testing.T) { + t.Cleanup(func() { + familyAliveCache.mu.Lock() + familyAliveCache.m = make(map[string]familyAliveCacheEntry) + familyAliveCache.mu.Unlock() + }) + + familyAliveStore("family-a|user-1", true) + familyAliveStore("family-b|user-1", true) + familyAliveStore("family-c|user-2", true) + familyAliveStore("family-a|user-2", true) + + InvalidateFamilyAliveBatch([]string{"family-a", "family-b"}) + + _, okA1 := familyAliveLookup("family-a|user-1") + _, okB1 := familyAliveLookup("family-b|user-1") + _, okA2 := familyAliveLookup("family-a|user-2") + _, okC2 := familyAliveLookup("family-c|user-2") + require.False(t, okA1, "family-a's verdict for user-1 must be dropped") + require.False(t, okB1, "family-b's verdict for user-1 must be dropped") + require.False(t, okA2, "family-a's verdict for user-2 must be dropped") + require.True(t, okC2, "family-c's verdict must survive") + + // Empty, blank and nil inputs are no-ops, never a panic. + InvalidateFamilyAliveBatch(nil) + InvalidateFamilyAliveBatch([]string{""}) + InvalidateFamilyAlive("") +} diff --git a/backend/handlers/payments/charge_helpers.go b/backend/handlers/payments/charge_helpers.go index 98175db..75eb00a 100644 --- a/backend/handlers/payments/charge_helpers.go +++ b/backend/handlers/payments/charge_helpers.go @@ -7,6 +7,7 @@ import ( "crypto/cipher" "crypto/rand" "encoding/base64" + "encoding/json" "errors" "fmt" "io" @@ -175,6 +176,33 @@ func releaseBookingPaymentLock(pinConn *pgxpool.Conn, lockKey string) { pinConn.Release() } +// writeChargeSnapshot stores the verbatim request JSON so the sweep can replay +// the charge with an IDENTICAL body under the same key (M1). The write is +// immutability-guarded: it records the FIRST attempt's body and stays immutable +// so a nonce-changing retry can never redirect the sweep's replay away from the +// original charge. Reuse paths that legitimately refresh the snapshot (a new +// source on a pending-reuse retry) do so via the Go-reencrypt refresh +// (refreshTillSnapshotSource / the gift-card reuse refresh), not by overwriting +// the guard. table is 'payments' or 'till_sales'; label names the flow for log +// messages (e.g. "payment", "tip payment", "gift-card payment", "till sale"). +// Best-effort: failures are logged and the row stays snapshot-less — the sweep +// overrides the replay source from the live square_source_id column. +func writeChargeSnapshot(ctx context.Context, q db.Querier, table, rowID string, body any, label string) { + snap, mErr := json.Marshal(body) + if mErr != nil { + log.Printf("Failed to marshal square_request_snapshot for %s %s: %v", label, rowID, mErr) + return + } + stored, eErr := encryptSnapshot(snap) + if eErr != nil { + log.Printf("Failed to encrypt square_request_snapshot for %s %s: %v", label, rowID, eErr) + return + } + if _, sErr := q.Exec(ctx, `UPDATE `+table+` SET square_request_snapshot = $1 WHERE id = $2 AND (square_request_snapshot IS NULL OR square_request_snapshot = '')`, string(stored), rowID); sErr != nil { + log.Printf("Failed to store square_request_snapshot for %s %s: %v", label, rowID, sErr) + } +} + // recheckBookingPayable re-reads the booking status after a Square charge // succeeded (R9): a concurrent cancellation/eviction can move the booking out // of a payable state between the pre-charge status check and the charge @@ -197,6 +225,40 @@ func recheckBookingPayable(ctx context.Context, q db.Querier, bookingID string) return status, bookingStatusAllowsCompletedPayment(status), nil } +// postChargeRecheck re-reads the booking status after a Square charge +// succeeded and, when the booking is no longer payable, marks the payment row +// failed, commits the caller's transaction, writes the 409 conflict response +// and returns false — the caller must abort. The recheck and the failed mark +// run in ONE transaction so the FOR UPDATE row lock taken inside +// recheckBookingPayable persists to commit (C5). Shared by the booking, tip +// and terminal saved-card paths so the R9 recheck cannot drift between them. +// On the not-payable branch the caller's deferred rollback becomes a harmless +// no-op (the commit already closed the transaction). chargeNoun labels the +// CRITICAL log (e.g. "payment", "tip"); conflictMsg is the 409 body. +func postChargeRecheck(ctx context.Context, w http.ResponseWriter, tx pgx.Tx, bookingID, paymentID, sqStatus, sqPayID, chargeNoun, conflictMsg string) (bool, error) { + recheckStatus, payable, err := recheckBookingPayable(ctx, tx, bookingID) + if err != nil { + log.Printf("CRITICAL: Square payment %s (ID=%s) was processed but re-reading booking %s status failed: %v — manual reconciliation required", sqStatus, sqPayID, bookingID, err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return false, err + } + if !payable { + log.Printf("CRITICAL: Square payment %s (ID=%s) for booking %s was processed but booking is now %q — marking %s failed; money taken at Square MUST be refunded manually", + sqStatus, sqPayID, bookingID, recheckStatus, chargeNoun) + if _, upErr := tx.Exec(ctx, `UPDATE payments SET status = 'failed' WHERE id = $1`, paymentID); upErr != nil { + log.Printf("CRITICAL: Square payment %s (ID=%s) landed on %q booking %s but marking %s failed errored: %v — manual reconciliation required", + sqStatus, sqPayID, recheckStatus, bookingID, chargeNoun, upErr) + } + if cErr := tx.Commit(ctx); cErr != nil { + log.Printf("CRITICAL: Square payment %s (ID=%s) landed on %q booking %s and committing the failed mark errored: %v — manual reconciliation required", + sqStatus, sqPayID, recheckStatus, bookingID, cErr) + } + http.Error(w, conflictMsg, http.StatusConflict) + return false, nil + } + return true, nil +} + // snapshotEncMarker prefixes the at-rest encrypted form of a stored // square_request_snapshot (PII: buyer email + ccof tokens) so decryptSnapshot // can distinguish encrypted values from plaintext (dev/mock environments and diff --git a/backend/handlers/payments/errors.go b/backend/handlers/payments/errors.go index 1ac46b3..e47c1dc 100644 --- a/backend/handlers/payments/errors.go +++ b/backend/handlers/payments/errors.go @@ -9,6 +9,28 @@ import ( "crussell/mw" ) +// squareRefundStatusToLocal maps Square's refund status to the local refunds +// status enum, consolidating the inline PENDING/FAILED/REJECTED resolutions +// scattered across the refund handlers and sweeps. Square's PaymentRefund +// states are PENDING, APPROVED, COMPLETED, CANCELED, FAILED and REJECTED +// (developer.squareup.com/reference/square/objects/PaymentRefund). A +// synchronous COMPLETED (or any non-PENDING/FAILED/REJECTED status — APPROVED, +// CANCELED, unknown) resolves to 'completed'; PENDING stays 'pending' (money in +// flight — the sweep reconciles it later); FAILED/REJECTED is a definitive +// 'failed'. Mirrors the webhooks package's squareRefundStatusToLocal, but that +// helper returns a (status, terminal) pair for webhook semantics while this one +// returns the plain local status for the refund-handler paths. +func squareRefundStatusToLocal(status string) string { + switch status { + case "PENDING": + return "pending" + case "FAILED", "REJECTED": + return "failed" + default: + return "completed" + } +} + // verificationRequiredCodes are Square CreatePayment error codes that mean the // buyer must complete Strong Customer Authentication (3DS/SCA) before the // charge can succeed: Square is demanding a fresh verification_token from the diff --git a/backend/handlers/payments/giftcard_limits.go b/backend/handlers/payments/giftcard_limits.go index dee413a..ada4d55 100644 --- a/backend/handlers/payments/giftcard_limits.go +++ b/backend/handlers/payments/giftcard_limits.go @@ -68,14 +68,20 @@ func userGiftCardSpentToday(ctx context.Context, q db.Querier, userID string) (f } // adminGiftCardValueToday returns the total gift-card value (in pounds) the -// admin has created, topped up, or transferred today (UTC day boundary, -// created_at >= CURRENT_DATE), returned as a float64 for pence conversion. +// admin has created, topped up, transferred, or issued via the till today (UTC +// day boundary, created_at >= CURRENT_DATE), returned as a float64 for pence +// conversion. This is the SINGLE daily-cap signal shared by the admin API +// surface (CreateGiftCard / TopUpGiftCard / TransferGiftCard) AND the till +// (CreateTillSale) — an admin surface that otherwise could issue unlimited +// balance (MEDIUM-5). // -// Signal (chosen to be double-count free across the three admin operations): +// Signal (chosen to be double-count free across the admin operations): // // 1. Cards the admin created today — SUM(total_funds_added). total_funds_added // is cumulative, so a card created today already reflects any same-day -// top-up or transfer INTO it, and its creation amount. +// top-up or transfer INTO it, and its creation amount. This covers cards +// created through BOTH the admin API and the till (a till create inserts +// the card with created_by = the admin). // 2. API top-ups executed by this admin today on cards created BEFORE today // (cards created today are excluded — term 1 already includes their // funding via total_funds_added, so counting the top-up row again would @@ -83,14 +89,20 @@ func userGiftCardSpentToday(ctx context.Context, q db.Querier, userID string) (f // (reference_type='api', user_id=admin) written by CreateGiftCard // ('purchase') and TopUpGiftCard ('topup', or 'purchase' on an inventory // card's first top-up). +// 3. Till sales executed by this admin today on cards created BEFORE today — +// till_sales rows (created_by = admin, status completed/pending — a +// pending sale's card was already funded before the Square call). Cards +// created today are excluded exactly like term 2, so a till-created card +// is counted once via term 1's total_funds_added and a till top-up on an +// older card is counted once here. A till sale's gift_card_transactions +// row is attributed to the CUSTOMER (reference_type='till_sale'), so it +// never enters term 2. // // Transfers INTO pre-existing cards leave no attributable audit row // (TransferGiftCard deliberately writes no gift_card_transactions entry), so // they are not directly counted; a transfer also creates no NEW gift-card // liability, so the daily cap still measures all value this admin has newly -// issued today. Till sales (CreateTillSale) write reference_type='till_sale' -// and are attributed to the CUSTOMER (user_id), so they are excluded here — -// the admin daily cap covers the admin API surface only. +// issued today. func adminGiftCardValueToday(ctx context.Context, q db.Querier, adminID string) (float64, error) { var value float64 err := q.QueryRow(ctx, ` @@ -112,6 +124,17 @@ func adminGiftCardValueToday(ctx context.Context, q db.Querier, adminID string) WHERE gc2.created_by = $1 AND gc2.created_at >= CURRENT_DATE ) ), 0) + + COALESCE(( + SELECT SUM(ts.total_amount) + FROM till_sales ts + WHERE ts.created_by = $1 + AND ts.status IN ('completed', 'pending') + AND ts.created_at >= CURRENT_DATE + AND ts.item_id NOT IN ( + SELECT gc3.id FROM gift_cards gc3 + WHERE gc3.created_by = $1 AND gc3.created_at >= CURRENT_DATE + ) + ), 0) `, adminID).Scan(&value) if err != nil { return 0, err diff --git a/backend/handlers/payments/giftcards.go b/backend/handlers/payments/giftcards.go index 354dd38..cbb4dc1 100644 --- a/backend/handlers/payments/giftcards.go +++ b/backend/handlers/payments/giftcards.go @@ -550,6 +550,14 @@ func CreateGiftCard(w http.ResponseWriter, r *http.Request) { return } + // MEDIUM-3a coverage: an admin creating a funded gift card is an admin + // money action — record it in admin_audit_log (best-effort, own tx). + InsertAdminAuditCharge(ctx, adminID, "", "admin_gift_card_create", map[string]any{ + "gift_card_id": gc.ID, + "amount": req.Amount, + "is_inventory": req.IsInventory, + }) + w.WriteHeader(http.StatusCreated) if err := json.NewEncoder(w).Encode(gc); err != nil { log.Printf("Failed to encode JSON response: %v", err) @@ -697,6 +705,13 @@ func TopUpGiftCard(w http.ResponseWriter, r *http.Request) { return } + // MEDIUM-3a coverage: an admin topping up a gift card is an admin money + // action — record it in admin_audit_log (best-effort, own tx). + InsertAdminAuditCharge(ctx, adminID, "", "admin_gift_card_topup", map[string]any{ + "gift_card_id": cardID, + "amount": req.Amount, + }) + if err := json.NewEncoder(w).Encode(gc); err != nil { log.Printf("Failed to encode JSON response: %v", err) } @@ -1076,12 +1091,13 @@ func RedeemGiftCard(w http.ResponseWriter, r *http.Request) { var amountRemaining float64 var redeemedBy sql.NullString + var expiryDate sql.NullTime err = tx.QueryRow(ctx, ` - SELECT amount_remaining, redeemed_by + SELECT amount_remaining, redeemed_by, expiry_date FROM gift_cards WHERE id = $1 FOR UPDATE - `, code).Scan(&amountRemaining, &redeemedBy) + `, code).Scan(&amountRemaining, &redeemedBy, &expiryDate) if err != nil { if errors.Is(err, pgx.ErrNoRows) { // B16: an "invalid code" failure — count it against the per-card @@ -1104,6 +1120,17 @@ func RedeemGiftCard(w http.ResponseWriter, r *http.Request) { return } + // LOW-6: enforce expiry at redemption, not just by the nightly cleanup job. + // Between a card's expiry_date passing and the next run of + // CleanupExpiredGiftCards the card still carries amount_remaining; without + // this check the holder could redeem value that is already forfeit (the + // nightly job moves it to gift_card_expired_balances and zeroes the card). + // Legacy cards with a NULL expiry_date are treated as unexpired. + if expiryDate.Valid && expiryDate.Time.Before(clock.Now()) { + http.Error(w, "This gift card has expired", http.StatusBadRequest) + return + } + if amountRemaining <= 0 { http.Error(w, "This gift card has no remaining balance", http.StatusBadRequest) return @@ -1605,15 +1632,12 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { IdempotencyKey: &req.IdempotencyKey, Fees: float64(fees) / 100.0, UserSavedCardID: savedCardID, + SquareSourceID: &sourceID, CreatedAt: clock.Now(), UpdatedAt: clock.Now(), CreatedBy: &userID, } - err = tx.QueryRow(ctx, ` - INSERT INTO payments (payment_type, payment_method, status, amount, square_payment_id, idempotency_key, fees, user_saved_card_id, created_by, created_at, updated_at, square_source_id) - VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12) - RETURNING id - `, record.PaymentType, record.PaymentMethod, record.Status, record.Amount, record.SquarePaymentID, record.IdempotencyKey, record.Fees, record.UserSavedCardID, record.CreatedBy, record.CreatedAt, record.UpdatedAt, sourceID).Scan(&buyPaymentID) + buyPaymentID, err = paymentService.CreatePaymentRecordTx(ctx, tx, record, nil) if err != nil { log.Printf("Failed to insert payment record: %v", err) http.Error(w, "internal server error", http.StatusInternalServerError) @@ -1621,12 +1645,7 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { } // Apply VAT to the pending payment - vatCfg, vatErr := GetVATConfig(ctx, tx) - if vatErr == nil && vatAppliesToVoucher(vatCfg) { - if _, vatExecErr := tx.Exec(ctx, "SELECT apply_vat_to_payment($1, $2)", buyPaymentID, vatCfg.DefaultVATRate); vatExecErr != nil { - log.Printf("Failed to apply VAT to buy gift card payment %s: %v", buyPaymentID, vatExecErr) - } - } + applyVATToChargeRecord(ctx, tx, buyPaymentID, false) if err := tx.Commit(ctx); err != nil { log.Printf("Failed to commit buy transaction: %v", err) @@ -1679,24 +1698,13 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { // M1: store the verbatim request JSON so the sweep can replay the charge // with an IDENTICAL body under the same key — Square compares the whole // request on key reuse, and a reconstructed body returns - // IDEMPOTENCY_KEY_REUSED, leaving the row pending forever. - // - // The write is INTENTIONALLY unconditional (WHERE id = $2, no - // snapshot-is-null guard like the booking/tip/terminal flows): the reuse - // branch above (B6) already refreshed square_request_snapshot in the SAME - // transaction as the square_source_id refresh, and this post-commit write - // stores the fresh full body for THIS attempt. Both paths converge on a - // correct snapshot, so a guard would either be dead (first attempt) or - // wrongly skip this write on the reuse path when the in-tx refresh failed - // best-effort. Do NOT "fix" this into the guarded form without reworking - // the reuse-branch refresh. - if snap, mErr := json.Marshal(paymentReq); mErr != nil { - log.Printf("Failed to marshal square_request_snapshot for gift-card payment %s: %v", buyPaymentID, mErr) - } else if stored, eErr := encryptSnapshot(snap); eErr != nil { - log.Printf("Failed to encrypt square_request_snapshot for gift-card payment %s: %v", buyPaymentID, eErr) - } else if _, sErr := db.Conn.Exec(ctx, `UPDATE payments SET square_request_snapshot = $1 WHERE id = $2`, string(stored), buyPaymentID); sErr != nil { - log.Printf("Failed to store square_request_snapshot for gift-card payment %s: %v", buyPaymentID, sErr) - } + // IDEMPOTENCY_KEY_REUSED, leaving the row pending forever. The write is + // immutability-guarded (same rule as the booking/tip/terminal flows): it + // records the FIRST attempt's body only — the reuse branch above (B6) + // refreshes square_request_snapshot in the SAME transaction as the + // square_source_id refresh via the Go-reencrypt path, so a reuse never + // needs this post-commit write to overwrite the guard. + writeChargeSnapshot(ctx, db.Conn, "payments", buyPaymentID, paymentReq, "gift-card payment") paymentResult, err := SquareClient.CreatePayment(ctx, paymentReq) if err != nil { @@ -1705,8 +1713,8 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { // 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) + if (req.CardID != nil && *req.CardID != "") || req.SaveCard { + reissueTwoFACodeAfterFailedCharge(ctx, db.Conn, userID, true, twoFAFallbackUsed && reusePendingID == "", r) } // Payment record intentionally left as 'pending' for manual retry. // SCA-required failures must surface the structured verification_required @@ -1919,20 +1927,20 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { // in handlers.go). func deriveGiftCardIdempotencyKey(ctx context.Context, q db.Querier, userID string, amount int64, recipientType, cardPart string) (string, error) { baseKey := fmt.Sprintf("gc-%s-%d-%s-%s", userID, amount, recipientType, cardPart) - for seq := 0; ; seq++ { - candidate := nextIdempotencyCandidate(baseKey, seq) + return scanIdempotencySlot(ctx, baseKey, func(candidate string) (bool, error) { var occupiedID string err := q.QueryRow(ctx, ` SELECT id FROM payments WHERE created_by = $1 AND idempotency_key = $2 AND status IN ('completed', 'failed') `, userID, candidate).Scan(&occupiedID) if errors.Is(err, pgx.ErrNoRows) { - return candidate, nil + return false, nil } if err != nil { - return "", err + return false, err } - } + return true, nil + }) } // --- Helpers --- @@ -2792,16 +2800,14 @@ func cancelGiftCardForUser(ctx context.Context, w http.ResponseWriter, r *http.R switch { case sqErr == nil: - status := "completed" - if refundResult.Status == "PENDING" { + status := squareRefundStatusToLocal(refundResult.Status) + if status == "pending" { // Square accepted the refund; the money is in flight. The card is // still neutralised immediately — leaving the balance spendable // while the refund is on its way would create value from nothing. // The pending row stays for the sweep to reconcile. - status = "pending" log.Printf("Square refund %s for gift-card purchase payment %s is PENDING — refund row left pending for reconciliation", refundResult.ID, paymentID) - } else if refundResult.Status == "FAILED" || refundResult.Status == "REJECTED" { - status = "failed" + } else if status == "failed" { log.Printf("Square refund %s for gift-card purchase payment %s FAILED — refund row marked failed", refundResult.ID, paymentID) } if status == "failed" { diff --git a/backend/handlers/payments/handlers.go b/backend/handlers/payments/handlers.go index 7ef9ebd..ef65316 100644 --- a/backend/handlers/payments/handlers.go +++ b/backend/handlers/payments/handlers.go @@ -17,9 +17,7 @@ import ( "log" "log/slog" "math" - "math/big" "net/http" - "os" "strconv" "strings" "time" @@ -28,14 +26,17 @@ import ( "github.com/jackc/pgx/v5" ) -// insertAdminAuditCharge records an admin-initiated saved-card charge in +// InsertAdminAuditCharge records an admin-initiated money action in // admin_audit_log (MEDIUM-3a). Mirrors the balance_check audit in // giftcards.go:1239-1243 — same table, same columns, same best-effort // non-fatal failure handling. The insert runs in its OWN transaction (a // savepoint in the test harness) so an audit-write failure — e.g. a synthetic // admin id in tests violating the admin_id FK — rolls back only the audit // write and can never abort the caller's transaction or a completed charge. -func insertAdminAuditCharge(ctx context.Context, adminID, targetUserID, action string, details map[string]any) { +// Exported so the user package (handlers/user/twofa.go) records admin 2FA +// code mints through this SAME helper instead of keeping a byte-identical +// cross-package copy. +func InsertAdminAuditCharge(ctx context.Context, adminID, targetUserID, action string, details map[string]any) { detailsJSON, err := json.Marshal(details) if err != nil { log.Printf("Failed to marshal admin_audit_log details (non-critical): %v", err) @@ -74,7 +75,7 @@ func insertAdminAuditCharge(ctx context.Context, adminID, targetUserID, action s // charge must land an admin_audit_log row (action_type '2fa_fallback_charge', // details {sca_performed:false, fallback_reason:"verification_unavailable"}) so // the operator can distinguish SCA-authorized charges from fallback-authorized -// ones. Mirrors insertAdminAuditCharge's best-effort, own-transaction, +// ones. Mirrors InsertAdminAuditCharge's best-effort, own-transaction, // non-fatal failure handling (a failed audit write can never abort a completed // charge). For customer-initiated online charges the actor (adminID) is the // customer's own userID; for admin surfaces it is the admin from request @@ -82,7 +83,7 @@ func insertAdminAuditCharge(ctx context.Context, adminID, targetUserID, action s // by the caller at charge success (paymentResult.CardLast4, booking/till/payment // id), where they are actually known. func insertTwoFAFallbackAudit(ctx context.Context, adminID, userID, cardLast4, referenceID, notes string) { - insertAdminAuditCharge(ctx, adminID, userID, "2fa_fallback_charge", map[string]any{ + InsertAdminAuditCharge(ctx, adminID, userID, "2fa_fallback_charge", map[string]any{ "sca_performed": false, "fallback_reason": "verification_unavailable", "card_last4": cardLast4, @@ -1128,13 +1129,7 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { // body, which stays immutable so a reused pending row never redirects // the sweep's replay away from the original charge (same rule as the // booking/tip paths — see the reuse branch above). - if snap, mErr := json.Marshal(paymentReq); mErr != nil { - log.Printf("Failed to marshal square_request_snapshot for saved-card payment %s: %v", paymentID, mErr) - } else if stored, eErr := encryptSnapshot(snap); eErr != nil { - log.Printf("Failed to encrypt square_request_snapshot for saved-card payment %s: %v", paymentID, eErr) - } else if _, sErr := db.Conn.Exec(r.Context(), `UPDATE payments SET square_request_snapshot = $1 WHERE id = $2 AND (square_request_snapshot IS NULL OR square_request_snapshot = '')`, string(stored), paymentID); sErr != nil { - log.Printf("Failed to store square_request_snapshot for saved-card payment %s: %v", paymentID, sErr) - } + writeChargeSnapshot(r.Context(), db.Conn, "payments", paymentID, paymentReq, "saved-card payment") paymentResult, err := SquareClient.CreatePayment(r.Context(), paymentReq) if err != nil { @@ -1142,11 +1137,12 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { // 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. + // 4). Only runs for a FRESH charge whose gate consumed a code + // (!reusePendingRecord): a pending-reuse retry verified WITHOUT + // consuming, so its code is still live and re-issuing would + // silently invalidate the one the customer holds. if bookingUserID.Valid { - reissueTwoFACodeAfterFailedCharge(r.Context(), bookingUserID.String) + reissueTwoFACodeAfterFailedCharge(r.Context(), db.Conn, bookingUserID.String, true, twoFAFallbackUsed && !reusePendingRecord, r) } // SCA-required failures (Square demands buyer verification) must // surface the structured verification_required body so the frontend @@ -1186,25 +1182,8 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { } }() - recheckStatus, payable, err := recheckBookingPayable(r.Context(), recheckTx, bookingID) - if err != nil { - log.Printf("CRITICAL: Square payment %s was processed for booking %s but re-reading booking status failed: %v — manual reconciliation required", - paymentResult.SquarePayID, bookingID, err) - http.Error(w, "internal server error", http.StatusInternalServerError) - return - } - if !payable { - log.Printf("CRITICAL: Square payment %s was processed but booking %s is now %q — marking saved-card payment %s failed; money taken at Square MUST be refunded manually", - paymentResult.SquarePayID, bookingID, recheckStatus, paymentID) - if _, upErr := recheckTx.Exec(r.Context(), `UPDATE payments SET status = 'failed' WHERE id = $1`, paymentID); upErr != nil { - log.Printf("CRITICAL: Square payment %s landed on %q booking %s but marking payment %s failed errored: %v — manual reconciliation required", - paymentResult.SquarePayID, recheckStatus, bookingID, paymentID, upErr) - } - if cErr := recheckTx.Commit(r.Context()); cErr != nil { - log.Printf("CRITICAL: Square payment %s landed on %q booking %s and committing the failed mark errored: %v — manual reconciliation required", - paymentResult.SquarePayID, recheckStatus, bookingID, cErr) - } - http.Error(w, "This booking is no longer accepting payments", http.StatusConflict) + payable, err := postChargeRecheck(r.Context(), w, recheckTx, bookingID, paymentID, paymentResult.Status, paymentResult.SquarePayID, "saved-card payment", "This booking is no longer accepting payments") + if err != nil || !payable { return } @@ -1244,13 +1223,15 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { http.Error(w, "internal server error", http.StatusInternalServerError) return } - // MEDIUM-2: the charge reached its terminal SUCCESS state — consume the - // verified 2FA code now, INSIDE the transaction that records the - // completed charge (the gate verified without consuming so a failed - // charge would not burn the code). A failure here fails the whole - // transaction (the row stays pending and the sweep reconciles), which is - // the same known failure mode as any other post-charge tx error. - if bookingUserID.Valid { + // MEDIUM-2 / finding 1: the charge reached its terminal SUCCESS state. + // For a FRESH charge the gate already consumed the code (consume=true + // at verify time — single-use), so nothing is left to do here. A + // PENDING-REUSE retry verified WITHOUT consuming, so THIS is where its + // code is burned — inside the transaction that records the completed + // charge. A failure here fails the whole transaction (the row stays + // pending and the sweep reconciles), which is the same known failure + // mode as any other post-charge tx error. + if bookingUserID.Valid && reusePendingRecord { if consErr := twofa.ConsumePendingCode(r.Context(), recheckTx, bookingUserID.String); consErr != nil { log.Printf("CRITICAL: Square payment %s succeeded but consuming the 2FA code for user %s failed: %v — manual reconciliation required", paymentResult.SquarePayID, bookingUserID.String, consErr) http.Error(w, "internal server error", http.StatusInternalServerError) @@ -1277,7 +1258,7 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { // best-effort AFTER the money transaction commits so an audit-write // failure can never roll back a completed charge. if bookingUserID.Valid { - insertAdminAuditCharge(r.Context(), adminID, bookingUserID.String, "saved_card_charge", map[string]any{ + InsertAdminAuditCharge(r.Context(), adminID, bookingUserID.String, "saved_card_charge", map[string]any{ "booking_id": bookingID, "payment_id": paymentID, "amount": float64(amount) / 100.0, @@ -2096,6 +2077,11 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { paymentID := "" reusePendingRecord := false + // pendingStoredAmountPence is the amount the pending row's first attempt + // was charged at (the row stores chargeAmount — see the HIGH-2 note in the + // pending-reuse case). The retry's own chargeAmount (recomputed below) is + // compared against it after the A4 computation. + pendingStoredAmountPence := int64(0) switch { case err == nil && existingStatus.String == "completed": // Idempotent dedup — return the already-completed payment. First @@ -2128,17 +2114,18 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { return case err == nil && existingStatus.String == "pending": // Previous Square call failed — reuse the pending record and re-attempt. - // Guard the amount: a retry with a different amount must not mutate the - // original record or charge the new amount against the old key. Compare - // in pence via math.Round — int64(pounds*100) truncation would reject - // legitimate same-amount retries for non-exact values (see CreateTipPayment). - if int64(math.Round(existingAmount.Float64*100)) != req.Amount { - log.Printf("Payment retry amount mismatch: pending record %s has %d pence, request has %d pence", existingID.String, int64(math.Round(existingAmount.Float64*100)), req.Amount) - http.Error(w, "Amount does not match the pending payment", http.StatusBadRequest) - return - } + // The amount guard is DELAYED until chargeAmount is computed below + // (HIGH-2): the pending row stores the CHARGE amount — for a + // deposit-with-discount, chargeAmount (req.Amount minus the campaign + // credit) differs from req.Amount, so comparing req.Amount here would + // 400 every legitimate deposit-with-discount retry forever. The retry's + // chargeAmount (recomputed under the same advisory lock) is compared + // against the row's stored amount after the A4 computation, in pence + // via math.Round (int64(pounds*100) truncation would reject legitimate + // same-amount retries for non-exact values — see CreateTipPayment). paymentID = existingID.String reusePendingRecord = true + pendingStoredAmountPence = int64(math.Round(existingAmount.Float64 * 100)) case err == nil && existingStatus.String == "failed": // Swept as stale (>24h) or definitively rejected — a retry would risk a // second Square charge. Reject cleanly instead of 500-ing on the @@ -2232,11 +2219,17 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { // the idempotency dedup: a same-key retry of an already-completed payment // short-circuits above and must not hit this guard (the booking is fully // paid by then). 'tip'-type requests are excluded — tips are charged via - // CreateTipPayment (which enforces its own start-time gate). The overflow - // comparison uses the DISCOUNTED remaining (raw remaining + this payment's - // campaign credit): a payment that exceeds the raw remaining but stays - // within the discounted remaining is covered by the discount — it is NOT an - // overflow into tip territory. + // CreateTipPayment (which enforces its own start-time gate). + // The overflow comparison runs against chargeAmount — the amount that will + // actually be charged at Square and split by buildSplitRecords — NOT + // req.Amount. A pending campaign credit inflated the old comparison + // (req.Amount > remaining + discount): a full/balance payment carries its + // discount client-side (chargeAmount == req.Amount), so req.Amount beyond + // the REAL remaining would pass the inflated guard and silently mint a + // pre-start tip (HIGH-1). A deposit, by contrast, is charged net of the + // server-side campaign credit (chargeAmount = req.Amount − discount), so a + // deposit charge can never exceed the real remaining while req.Amount + // stays within it — comparing chargeAmount keeps the guard honest for both. // remainingPence is the booking's tip-excluded outstanding balance // (total - completed real payments, refunds re-open capacity). It is // computed once here — before any pending row exists — and reused by both @@ -2250,26 +2243,6 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { http.Error(w, "internal server error", http.StatusInternalServerError) return } - discountedRemainingPence := remainingPence + eligibleDiscountPence - if req.Amount > discountedRemainingPence { - // B12: an overflow that would become a tip ALWAYS requires the - // customer's explicit confirmation (confirm_overflow_tip) — both - // pre-start AND post-start. Previously only a pre-start overflow - // required the flag and a post-start overflow became a tip - // silently; the frontend's stale amount_due + discount preview - // could then mint an unintended tip. When the flag is absent the - // request is rejected with overflow_tip_confirmation_required so - // the frontend can prompt, regardless of booking state. - if !req.ConfirmOverflowTip { - log.Printf("Overflow requires confirmation: amount %d exceeds discounted remaining %d for booking %s", req.Amount, discountedRemainingPence, bookingID) - mw.RespondJSON(w, http.StatusBadRequest, map[string]string{ - "error": "The extra amount will be recorded as a tip. Confirm to continue.", - "code": "overflow_tip_confirmation_required", - }) - return - } - log.Printf("Overflow accepted as tip: amount %d exceeds discounted remaining %d for booking %s (confirmed=%v)", req.Amount, discountedRemainingPence, bookingID, req.ConfirmOverflowTip) - } } // A4: the amount actually charged at Square. The frontend's full/balance @@ -2320,6 +2293,41 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { } } + // B12: an overflow that would become a tip ALWAYS requires the customer's + // explicit confirmation (confirm_overflow_tip) — both pre-start AND + // post-start. Previously only a pre-start overflow required the flag and a + // post-start overflow became a tip silently; the frontend's stale amount_due + // + discount preview could then mint an unintended tip. When the flag is + // absent the request is rejected with overflow_tip_confirmation_required so + // the frontend can prompt, regardless of booking state. The comparison is + // chargeAmount vs the REAL remaining (see the M4/M7 note above): a deposit + // charge is already net of the campaign credit, so a pre-start deposit can + // never exceed the remaining obligation and never mints an unconfirmed tip. + if req.PaymentType != "tip" && chargeAmount > remainingPence { + if !req.ConfirmOverflowTip { + log.Printf("Overflow requires confirmation: charge %d exceeds remaining %d for booking %s (requested %d)", chargeAmount, remainingPence, bookingID, req.Amount) + mw.RespondJSON(w, http.StatusBadRequest, map[string]string{ + "error": "The extra amount will be recorded as a tip. Confirm to continue.", + "code": "overflow_tip_confirmation_required", + }) + return + } + log.Printf("Overflow accepted as tip: charge %d exceeds remaining %d for booking %s (requested %d, confirmed=%v)", chargeAmount, remainingPence, bookingID, req.Amount, req.ConfirmOverflowTip) + } + + // HIGH-2: a pending-reuse retry must match the CHARGE amount stored on the + // pending row (the discounted deposit charge, e.g. £40 — what the first + // attempt charged at Square), not req.Amount (£50 — the raw deposit the + // frontend resends). A mismatch proves the retry would charge a different + // amount than the row's first attempt under the same idempotency key + // (Square would reject the dedup anyway), so reject cleanly instead of + // 400-ing a legitimate deposit-with-discount retry forever. + if reusePendingRecord && pendingStoredAmountPence != chargeAmount { + log.Printf("Payment retry amount mismatch: pending record %s has %d pence, retry would charge %d pence (request %d pence)", paymentID, pendingStoredAmountPence, chargeAmount, req.Amount) + http.Error(w, "Amount does not match the pending payment", http.StatusBadRequest) + return + } + // 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 @@ -2474,13 +2482,7 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { // written ONLY when the row has none: it records the FIRST attempt's body, // which stays immutable so a nonce-changing retry can never redirect the // sweep's replay away from the original charge (see the reuse branch above). - if snap, mErr := json.Marshal(paymentReq); mErr != nil { - log.Printf("Failed to marshal square_request_snapshot for payment %s: %v", paymentID, mErr) - } else if stored, eErr := encryptSnapshot(snap); eErr != nil { - log.Printf("Failed to encrypt square_request_snapshot for payment %s: %v", paymentID, eErr) - } else if _, sErr := db.Conn.Exec(r.Context(), `UPDATE payments SET square_request_snapshot = $1 WHERE id = $2 AND (square_request_snapshot IS NULL OR square_request_snapshot = '')`, string(stored), paymentID); sErr != nil { - log.Printf("Failed to store square_request_snapshot for payment %s: %v", paymentID, sErr) - } + writeChargeSnapshot(r.Context(), db.Conn, "payments", paymentID, paymentReq, "payment") paymentResult, err := SquareClient.CreatePayment(r.Context(), paymentReq) if err != nil { @@ -2490,11 +2492,11 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { // (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). + // (finding 5). A pending-reuse retry verified WITHOUT consuming, so its + // code is still live and no re-issue runs (a re-issue would invalidate + // the code the customer already holds). if req.CardID != nil && *req.CardID != "" { - reissueTwoFACodeAfterFailedCharge(r.Context(), userID) + reissueTwoFACodeAfterFailedCharge(r.Context(), db.Conn, userID, true, twoFAFallbackUsed && !reusePendingRecord, r) } // SCA-required failures must surface the structured verification_required // body so the frontend triggers the 3DS challenge, not a plain decline. @@ -2536,25 +2538,8 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { // manually. The pending row is marked 'failed' in the tx below, so // idempotency dedup still blocks a second Square charge, but the row no // longer shows pending — the frontend's retry gets a 409 Conflict. - recheckStatus, payable, err := recheckBookingPayable(r.Context(), tx2, bookingID) - if err != nil { - log.Printf("CRITICAL: Square payment %s (ID=%s) was processed but re-reading booking %s status failed: %v — manual reconciliation required", - paymentResult.Status, paymentResult.SquarePayID, bookingID, err) - http.Error(w, "internal server error", http.StatusInternalServerError) - return - } - if !payable { - log.Printf("CRITICAL: Square payment %s (ID=%s) for booking %s was processed but booking is now %q — marking payment failed; money taken at Square MUST be refunded manually", - paymentResult.Status, paymentResult.SquarePayID, bookingID, recheckStatus) - if _, upErr := tx2.Exec(r.Context(), `UPDATE payments SET status = 'failed' WHERE id = $1`, paymentID); upErr != nil { - log.Printf("CRITICAL: Square payment %s (ID=%s) landed on %q booking %s but marking payment %s failed errored: %v — manual reconciliation required", - paymentResult.Status, paymentResult.SquarePayID, recheckStatus, bookingID, paymentID, upErr) - } - if cErr := tx2.Commit(r.Context()); cErr != nil { - log.Printf("CRITICAL: Square payment %s (ID=%s) landed on %q booking %s and committing the failed mark errored: %v — manual reconciliation required", - paymentResult.Status, paymentResult.SquarePayID, recheckStatus, bookingID, cErr) - } - http.Error(w, "This booking is no longer accepting payments", http.StatusConflict) + payable, err := postChargeRecheck(r.Context(), w, tx2, bookingID, paymentID, paymentResult.Status, paymentResult.SquarePayID, "payment", "This booking is no longer accepting payments") + if err != nil || !payable { return } @@ -2642,16 +2627,15 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { return } - // MEDIUM-2: a saved-card charge reached its terminal SUCCESS state — - // consume the verified 2FA code now, inside the same transaction that - // records the completed charge. For a FRESH charge the gate already - // consumed the code, so this is an idempotent no-op safety net; for a - // pending-reuse retry (gate passed consume=false — the code was re-issued - // for this retry) this is where it is burned, so a retry that fails again - // keeps its code for one more attempt. Only runs for saved-card (CardID) - // charges — the gate only ran for those, and new-card charges have no code - // to consume. - if req.CardID != nil && *req.CardID != "" { + // MEDIUM-2 / finding 1: a saved-card charge reached its terminal SUCCESS + // state. For a FRESH charge the gate already consumed the code + // (consume=true at verify time — single-use), so no write happens here. + // For a PENDING-REUSE retry (gate passed consume=false — the code was + // re-issued for this retry) this is where it is burned, so a retry that + // fails again keeps its code for one more attempt. Only runs for + // saved-card (CardID) charges — the gate only ran for those, and new-card + // charges have no code to consume. + if req.CardID != nil && *req.CardID != "" && reusePendingRecord { if consErr := twofa.ConsumePendingCode(r.Context(), tx2, userID); consErr != nil { log.Printf("CRITICAL: Square payment %s (ID=%s) was processed but consuming the 2FA code for user %s failed: %v — manual reconciliation required", paymentResult.Status, paymentResult.SquarePayID, userID, consErr) @@ -3576,12 +3560,10 @@ func RefundPayment(w http.ResponseWriter, r *http.Request) { case reissueErr == nil: // Resolve by Square's status: PENDING stays pending (sweep // reconciles), FAILED/REJECTED is definitive, COMPLETED resolves. - reissueStatus := "completed" - if reissueResult.Status == "PENDING" { - reissueStatus = "pending" + reissueStatus := squareRefundStatusToLocal(reissueResult.Status) + if reissueStatus == "pending" { log.Printf("Square reissue %s is PENDING — leaving refund %s pending for the sweep", reissueResult.ID, existingRefundID.String) - } else if reissueResult.Status == "FAILED" || reissueResult.Status == "REJECTED" { - reissueStatus = "failed" + } else if reissueStatus == "failed" { log.Printf("Square reissue %s FAILED — marking refund %s failed", reissueResult.ID, existingRefundID.String) } if _, upErr := db.Conn.Exec(r.Context(), @@ -3817,12 +3799,10 @@ func RefundPayment(w http.ResponseWriter, r *http.Request) { // would permanently block that amount in the over-refund guard. Only a // definitive COMPLETED resolves to completed; PENDING stays pending for the // sweep to reconcile; FAILED/REJECTED is a real failure. - status := "completed" - if refundResult.Status == "PENDING" { - status = "pending" + status := squareRefundStatusToLocal(refundResult.Status) + if status == "pending" { log.Printf("Square refund %s is PENDING (in flight) — leaving refund %s pending for the sweep to resolve", refundResult.ID, refundID) - } else if refundResult.Status == "FAILED" || refundResult.Status == "REJECTED" { - status = "failed" + } else if status == "failed" { log.Printf("Square refund %s FAILED — marking refund %s failed", refundResult.ID, refundID) } @@ -3910,12 +3890,10 @@ func resumeManualPendingRefund(w http.ResponseWriter, r *http.Request, paymentID // Resolve by Square's status — a PENDING resume stays pending for the // sweep (marking it completed while Square later fails it would block the // amount in the over-refund guard forever); FAILED/REJECTED is definitive. - status := "completed" - if resumeResult.Status == "PENDING" { - status = "pending" + status := squareRefundStatusToLocal(resumeResult.Status) + if status == "pending" { log.Printf("Square refund %s is PENDING — leaving refund %s pending for the sweep", resumeResult.ID, refundID) - } else if resumeResult.Status == "FAILED" || resumeResult.Status == "REJECTED" { - status = "failed" + } else if status == "failed" { log.Printf("Square refund %s FAILED — marking refund %s failed", resumeResult.ID, refundID) } @@ -4278,11 +4256,10 @@ func AdminRefundBooking(w http.ResponseWriter, r *http.Request) { }) switch { case rErr == nil: - if result.Status == "PENDING" { - status = "pending" + status = squareRefundStatusToLocal(result.Status) + if status == "pending" { log.Printf("Square refund %s is PENDING (in flight) — leaving refund %s pending for the sweep", result.ID, cf.refundID) - } else if result.Status == "FAILED" || result.Status == "REJECTED" { - status = "failed" + } else if status == "failed" { log.Printf("Square refund %s FAILED — marking refund %s failed", result.ID, cf.refundID) } if _, upErr := db.Conn.Exec(r.Context(), `UPDATE refunds SET status = $1, square_refund_id = $2 WHERE id = $3`, status, result.ID, cf.refundID); upErr != nil { @@ -4313,6 +4290,18 @@ func AdminRefundBooking(w http.ResponseWriter, r *http.Request) { totalRefunded += rf.Amount } + // MEDIUM-3a coverage: record the admin booking refund in admin_audit_log + // (best-effort, own transaction — a failed audit write can never undo the + // refund). One row per action, not per payment, so the operator sees the + // admin decision that moved money. + InsertAdminAuditCharge(r.Context(), adminID, bookingUserID, "admin_booking_refund", map[string]any{ + "booking_id": bookingID, + "amount": float64(req.Amount) / 100.0, + "reason": req.Reason, + "refund_count": len(refunds), + "refunded_payments": len(cardRefunds), + }) + if err := json.NewEncoder(w).Encode(AdminBookingRefundResponse{ RefundedAmount: totalRefunded, Refunds: refunds, @@ -4711,13 +4700,7 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { // written ONLY when the row has none: it records the FIRST attempt's body, // which stays immutable so a nonce-changing retry can never redirect the // sweep's replay away from the original charge (see the reuse branch above). - if snap, mErr := json.Marshal(paymentReq); mErr != nil { - log.Printf("Failed to marshal square_request_snapshot for tip payment %s: %v", paymentID, mErr) - } else if stored, eErr := encryptSnapshot(snap); eErr != nil { - log.Printf("Failed to encrypt square_request_snapshot for tip payment %s: %v", paymentID, eErr) - } else if _, sErr := db.Conn.Exec(r.Context(), `UPDATE payments SET square_request_snapshot = $1 WHERE id = $2 AND (square_request_snapshot IS NULL OR square_request_snapshot = '')`, string(stored), paymentID); sErr != nil { - log.Printf("Failed to store square_request_snapshot for tip payment %s: %v", paymentID, sErr) - } + writeChargeSnapshot(r.Context(), db.Conn, "payments", paymentID, paymentReq, "tip payment") paymentResult, err := SquareClient.CreatePayment(r.Context(), paymentReq) if err != nil { @@ -4727,9 +4710,11 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { // 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). + // overwrite a standing pending code with an undelivered one (finding + // 5). A pending-reuse retry verified WITHOUT consuming, so its code is + // still live and no re-issue runs. if req.CardID != nil && *req.CardID != "" { - reissueTwoFACodeAfterFailedCharge(r.Context(), userID) + reissueTwoFACodeAfterFailedCharge(r.Context(), db.Conn, userID, true, twoFAFallbackUsed && !reusePendingRecord, r) } // SCA-required failures must surface the structured verification_required // body so the frontend triggers the 3DS challenge, not a plain decline. @@ -4767,25 +4752,8 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { } }() - tipRecheckStatus, tipPayable, err := recheckBookingPayable(r.Context(), recheckTx, bookingID) - if err != nil { - log.Printf("CRITICAL: Square tip payment %s (ID=%s) was processed but re-reading booking %s status failed: %v — manual reconciliation required", - paymentResult.Status, paymentResult.SquarePayID, bookingID, err) - http.Error(w, "internal server error", http.StatusInternalServerError) - return - } - if !tipPayable { - log.Printf("CRITICAL: Square tip payment %s (ID=%s) for booking %s was processed but booking is now %q — marking tip %s failed; money taken at Square MUST be refunded manually", - paymentResult.Status, paymentResult.SquarePayID, bookingID, tipRecheckStatus, paymentID) - if _, upErr := recheckTx.Exec(r.Context(), `UPDATE payments SET status = 'failed' WHERE id = $1`, paymentID); upErr != nil { - log.Printf("CRITICAL: Square tip payment %s (ID=%s) landed on %q booking %s but marking tip %s failed errored: %v — manual reconciliation required", - paymentResult.Status, paymentResult.SquarePayID, tipRecheckStatus, bookingID, paymentID, upErr) - } - if cErr := recheckTx.Commit(r.Context()); cErr != nil { - log.Printf("CRITICAL: Square tip payment %s (ID=%s) landed on %q booking %s and committing the failed mark errored: %v — manual reconciliation required", - paymentResult.Status, paymentResult.SquarePayID, tipRecheckStatus, bookingID, cErr) - } - http.Error(w, "This booking is no longer accepting tips", http.StatusConflict) + payable, err := postChargeRecheck(r.Context(), w, recheckTx, bookingID, paymentID, paymentResult.Status, paymentResult.SquarePayID, "tip", "This booking is no longer accepting tips") + if err != nil || !payable { return } @@ -4800,11 +4768,12 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { http.Error(w, "internal server error", http.StatusInternalServerError) return } - // MEDIUM-2: a saved-card tip charge reached its terminal SUCCESS state — - // consume the verified 2FA code now, inside the transaction that records - // the completed charge (the gate verified without consuming, so a failed - // charge did not burn the code and a same-key retry could reuse it). - if req.CardID != nil && *req.CardID != "" { + // MEDIUM-2 / finding 1: a saved-card tip charge reached its terminal + // SUCCESS state. For a FRESH charge the gate already consumed the code + // (single-use at verify time); for a PENDING-REUSE retry (gate passed + // consume=false) this is where its code is burned, inside the transaction + // that records the completed charge. + if req.CardID != nil && *req.CardID != "" && reusePendingRecord { if consErr := twofa.ConsumePendingCode(r.Context(), recheckTx, userID); consErr != nil { log.Printf("CRITICAL: Square tip payment %s (ID=%s) was processed but consuming the 2FA code for user %s failed: %v — manual reconciliation required", paymentResult.Status, paymentResult.SquarePayID, userID, consErr) @@ -5196,56 +5165,3 @@ func randomHexSuffix(n int) string { } return fmt.Sprintf("%x", b) } - -// reissueTwoFACodeAfterFailedCharge mints a fresh 2FA code after a saved-card -// charge failed at Square. The charge gate consumes the verified code at gate -// time for fresh charges (single-use — closing the verify-then-consume TOCTOU -// where a verified-but-unconsumed code could authorize a second charge), so a -// failed charge leaves no live code for the same-key retry; this re-issues one -// with the same 10-minute lifetime and delivery behaviour as the user -// package's code issuance (dev/test logs the code for the operator to relay; -// production logs only with TWO_FACTOR_ALLOW_LOG_DELIVERY=true, matching the -// fail-closed delivery contract). Best-effort: a failure logs and the customer -// requests a fresh code through the normal 2FA flow. Idempotent by design — a -// pending-reuse retry whose code was NOT consumed also gets a fresh, longer- -// lived code, which never invalidates anything that still needed verifying. -func reissueTwoFACodeAfterFailedCharge(ctx context.Context, userID string) { - if userID == "" || !twoFactorEnforced() { - return - } - code, err := generatePaymentsTwoFACode() - if err != nil { - log.Printf("2FA: failed to generate a re-issued code for user %s after a failed charge: %v", userID, err) - return - } - if _, err := db.Conn.Exec(ctx, ` - UPDATE users - SET two_factor_pending_code_hash = $2, - two_factor_pending_code_expires = $3 - WHERE id = $1 - `, userID, twofa.Hash(code), clock.Now().Add(twoFAPendingCodeLifetime)); err != nil { - log.Printf("2FA: failed to store a re-issued code for user %s after a failed charge: %v", userID, err) - return - } - // Delivery mirrors the user package's build-dependent behaviour (the - // operator relays the [2FA] log line). Production logs the plaintext code - // only when explicitly opted in; dev/test always. - if IsExplicitDevOrMockEnv() || os.Getenv("TWO_FACTOR_ALLOW_LOG_DELIVERY") == "true" { - log.Printf("[2FA] code delivery requested (user=%s, purpose=re-issue after failed saved-card charge)", userID) - log.Printf("[2FA] code: %s", code) - } -} - -// generatePaymentsTwoFACode returns a random 6-digit verification code, -// mirroring the user package's generator (crypto/rand, uniform 0-999999). -func generatePaymentsTwoFACode() (string, error) { - n, err := rand.Int(rand.Reader, big.NewInt(1_000_000)) - if err != nil { - return "", err - } - return fmt.Sprintf("%06d", n.Int64()), nil -} - -// twoFAPendingCodeLifetime is how long a re-issued 2FA code stays valid, -// mirroring the user package's pending-code expiry. -const twoFAPendingCodeLifetime = 10 * time.Minute diff --git a/backend/handlers/payments/idempotency_helpers.go b/backend/handlers/payments/idempotency_helpers.go index 0d7a0f0..0506266 100644 --- a/backend/handlers/payments/idempotency_helpers.go +++ b/backend/handlers/payments/idempotency_helpers.go @@ -1,6 +1,7 @@ package payments import ( + "context" "crypto/sha256" "encoding/hex" "fmt" @@ -55,6 +56,29 @@ func nextIdempotencyCandidate(base string, seq int) string { return truncateIdempotencyKey(prefix, candidate) } +// scanIdempotencySlot iterates the candidate sequence for baseKey (seq 0, 1, +// 2, ...) until it finds a slot NOT occupied by a terminal row, returning the +// first free candidate. occupied reports whether the candidate is taken; the +// caller supplies the table-specific occupancy check. Shared by the gift-card +// purchase key derivation (deriveGiftCardIdempotencyKey) and the till-sale key +// derivation (scanTillIdempotencyKeySlot), which must agree on the +// completed/failed-occupies, pending-never-occupies rule so a lost-response +// retry reuses the same key instead of minting a second charge. Must be called +// under the caller's advisory lock so the scan-and-insert races no concurrent +// identical request. +func scanIdempotencySlot(ctx context.Context, baseKey string, occupied func(candidate string) (bool, error)) (string, error) { + for seq := 0; ; seq++ { + candidate := nextIdempotencyCandidate(baseKey, seq) + isOccupied, err := occupied(candidate) + if err != nil { + return "", err + } + if !isOccupied { + return candidate, nil + } + } +} + // IsExplicitDevOrMockEnv reports whether SQUARE_ENVIRONMENT explicitly selects // the dev/mock Square stack. Only these exact values are treated as dev; an // empty or unknown value is NOT dev (fail-closed), because in production an diff --git a/backend/handlers/payments/loop_a_money_fixes_test.go b/backend/handlers/payments/loop_a_money_fixes_test.go new file mode 100644 index 0000000..8502dcb --- /dev/null +++ b/backend/handlers/payments/loop_a_money_fixes_test.go @@ -0,0 +1,339 @@ +//go:build test && dev + +package payments + +// ============================================================================= +// LOOP A — fresh-review money findings (HIGH-1, HIGH-2, MEDIUM-3, MEDIUM-5, +// LOW-6). Each test pins the fixed behaviour and would fail on the old code. +// ============================================================================= + +import ( + "context" + "net/http" + "testing" + "time" + + "crussell/db" + "crussell/internal/square" + "crussell/testutils" + "crussell/testutils/fixtures" + "crussell/testutils/jwt" + + "github.com/jackc/pgx/v5" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// ============================================================================= +// HIGH-1 — the overflow→tip guard compares chargeAmount (what Square will +// actually charge and buildSplitRecords will split), not req.Amount against an +// inflated remaining+discount threshold. A pending campaign credit previously +// let a full payment exceed the REAL remaining and silently mint a pre-start +// tip. +// ============================================================================= + +// TestLoopA_PreStartFullWithPendingDiscount_RequiresConfirmation locks the HIGH-1 +// bypass: a full £60 payment on the £50 fixture booking with a 100% campaign +// eligible (£50 credit) would have passed the old guard (60 < 50+50) and +// silently charged £60, carving a £10 pre-start tip with no confirmation. +// chargeAmount == req.Amount for a 'full' payment, so it exceeds the real £50 +// remaining and MUST require confirmation. +func TestLoopA_PreStartFullWithPendingDiscount_RequiresConfirmation(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestData(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + seedActiveCampaign(t, ctx, tx, 100) + + cardToken := "cnon:loop-a-overflow-full" + req := CreateBookingPaymentRequest{ + Amount: 6000, // £60 on a £50 booking + PaymentType: "full", + NewCardToken: &cardToken, + IdempotencyKey: "loop-a-overflow-full-" + bookingID, + } + + handler := CreateBookingPayment + w := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusBadRequest, w.Code, "a full payment beyond the real remaining must require confirmation even with a discount pending, body: %s", w.Body.String()) + assert.Contains(t, w.Body.String(), "overflow_tip_confirmation_required") + + // No payment record may be written for the rejected overflow. + var payCount int + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM payments WHERE booking_id = $1`, bookingID).Scan(&payCount)) + assert.Zero(t, payCount, "the unconfirmed overflow must not create any payment record") +} + +// TestLoopA_PreStartDepositWithDiscount_NeverMintsUnintendedTip locks the HIGH-1 +// deposit side: a deposit charge is net of the campaign credit, so a pre-start +// deposit can never exceed the real remaining and never mints an unconfirmed +// tip. A £60 deposit on the £50 booking with a 20% campaign (£10 credit) +// charges exactly the £50 remaining — no tip record. A separate booking with a +// deposit that WOULD overflow (discounted charge > remaining) still requires +// confirmation. +func TestLoopA_PreStartDepositWithDiscount_NeverMintsUnintendedTip(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestData(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + seedActiveCampaign(t, ctx, tx, 20) + + cardToken := "cnon:loop-a-deposit-no-tip" + req := CreateBookingPaymentRequest{ + Amount: 6000, // £60 deposit; chargeAmount = £50 (remaining) + PaymentType: "deposit", + NewCardToken: &cardToken, + IdempotencyKey: "loop-a-deposit-no-tip-" + bookingID, + } + + handler := CreateBookingPayment + w := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusOK, w.Code, "a deposit whose discounted charge equals the remaining obligation must be accepted, body: %s", w.Body.String()) + + // No tip may be carved: the discounted charge (£50) is fully booking money. + var tipCount int + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND payment_type = 'tip'`, bookingID).Scan(&tipCount)) + assert.Zero(t, tipCount, "a pre-start deposit-with-discount must never mint a tip") + + // A deposit that WOULD overflow into a tip requires confirmation: £61 + // deposit → chargeAmount £51 > remaining £50 → confirmation needed. + userID2, bookingID2, _ := setupTestData(t, ctx, tx) + userToken2 := jwt.GenerateUserToken(userID2) + seedActiveCampaign(t, ctx, tx, 20) + req2 := CreateBookingPaymentRequest{ + Amount: 6100, + PaymentType: "deposit", + NewCardToken: &cardToken, + IdempotencyKey: "loop-a-deposit-overflow-" + bookingID2, + } + w2 := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID2+"/payment", req2, userToken2, ctx) + require.Equal(t, http.StatusBadRequest, w2.Code, "a deposit whose discounted charge exceeds the remaining must require confirmation, body: %s", w2.Body.String()) + assert.Contains(t, w2.Body.String(), "overflow_tip_confirmation_required") +} + +// ============================================================================= +// HIGH-2 — a deposit-with-discount pending-reuse retry must compare against the +// CHARGE amount stored on the pending row (the discounted amount), not the raw +// req.Amount the frontend resends. Previously every such retry 400'd +// "amount_mismatch" forever. +// ============================================================================= + +// TestLoopA_DepositWithDiscount_PendingReuseRetry_Succeeds seeds the pending +// row at the DISCOUNTED charge (£15 = £25 deposit − £10 campaign credit) and +// retries with the RAW £25 deposit — exactly what the frontend resends. The +// retry must be accepted (chargeAmount recomputes to £15 and matches) and the +// charge completed, not rejected with amount_mismatch. +func TestLoopA_DepositWithDiscount_PendingReuseRetry_Succeeds(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestData(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + // 20% campaign on the £50 fixture booking = £10 credit → a £25 raw deposit + // charges £15. + seedActiveCampaign(t, ctx, tx, 20) + + key := "loop-a-deposit-retry-" + bookingID + // Seed the pending row exactly as the handler's first attempt stored it: + // the CHARGE amount (£15), not the requested £25. + _, err := tx.Exec(ctx, ` + INSERT INTO payments (booking_id, payment_type, payment_method, status, amount, idempotency_key, square_source_id, created_at, updated_at, created_by) + VALUES ($1, 'deposit', 'online_square', 'pending', 15.00, $2, 'cnon:first-attempt', NOW(), NOW(), $3) + `, bookingID, key, userID) + require.NoError(t, err) + + cardToken := "cnon:loop-a-deposit-retry" + req := CreateBookingPaymentRequest{ + Amount: 2500, // raw £25 deposit — the frontend resends this + PaymentType: "deposit", + NewCardToken: &cardToken, + IdempotencyKey: key, + } + + handler := CreateBookingPayment + w := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusOK, w.Code, "a deposit-with-discount pending-reuse retry must succeed, body: %s", w.Body.String()) + + var status, sqPayID string + require.NoError(t, tx.QueryRow(ctx, `SELECT status, COALESCE(square_payment_id, '') FROM payments WHERE idempotency_key = $1`, key).Scan(&status, &sqPayID)) + assert.Equal(t, "completed", status, "the reused pending row must complete") + assert.NotEmpty(t, sqPayID, "the completed row must carry the Square payment id") + + // Exactly one row for the key — the pending row was reused, not duplicated. + var payCount int + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM payments WHERE idempotency_key = $1`, key).Scan(&payCount)) + assert.Equal(t, 1, payCount, "the retry must reuse the pending row, not mint a second one") +} + +// ============================================================================= +// MEDIUM-3 — the stale-pending sweep rescue must mirror the live-path split: an +// overflow beyond the booking's remaining obligation is carved out as a tip +// record (never mis-booked as service revenue) and the fully-paid completion +// check runs. +// ============================================================================= + +// TestLoopA_SweepRescue_CarvesTipAndCompletes rescues a keyed lost-response +// payment of £60 on the £50 fixture booking via the sweep. The rescue must: +// complete the row, split it into deposit £25 + balance £25 + a carved tip £10, +// and complete the booking (fully paid). +func TestLoopA_SweepRescue_CarvesTipAndCompletes(t *testing.T) { + 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.CreateTestBookingAtTime(tx, userID, serviceID, + time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC)) + require.NoError(t, err) + + staleID, err := fixtures.CreateTestPayment(tx, bookingID, 60.00, "online_square", "full", "pending") + require.NoError(t, err) + const key = "loop-a-sweep-rescue" + _, err = tx.Exec(ctx, "UPDATE payments SET created_at = NOW() - INTERVAL '23 hours', idempotency_key = $1, square_source_id = 'cnon:loop-a' WHERE id = $2", key, staleID) + require.NoError(t, err) + + origClient := SquareClient + mock := square.NewDevClient().(*square.MockClient) + pay, err := mock.CreatePayment(context.Background(), square.CreatePaymentReq{ + Amount: 6000, + Currency: "GBP", + SourceID: "cnon:loop-a", + IdempotencyKey: key, + }) + require.NoError(t, err, "failed to seed the completed Square payment") + SquareClient = mock + defer func() { SquareClient = origClient }() + + pgxTx := db.TxFromContext(ctx) + require.NotNil(t, pgxTx, "no transaction in context") + require.NoError(t, pgxTx.Commit(ctx), "failed to commit test tx") + + t.Cleanup(func() { + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM payments WHERE id = $1`, staleID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM bookings WHERE id = $1`, bookingID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM services WHERE id = $1`, serviceID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM users WHERE id = $1`, userID) + }) + + freshCtx := context.Background() + if _, err := SweepStalePendingPayments(freshCtx); err != nil { + t.Fatalf("sweep failed: %v", err) + } + + var status, sqPayID string + require.NoError(t, db.Conn.QueryRow(freshCtx, "SELECT status, COALESCE(square_payment_id, '') FROM payments WHERE id = $1", staleID).Scan(&status, &sqPayID)) + assert.Equal(t, "completed", status, "the rescued row must complete") + assert.Equal(t, pay.SquarePayID, sqPayID, "the rescued row must carry the replayed square_payment_id") + + // The primary row is the deposit portion (£25); the balance and tip are + // separate split rows. + var bookingPortion float64 + require.NoError(t, db.Conn.QueryRow(freshCtx, `SELECT COALESCE(SUM(amount), 0) FROM payments WHERE booking_id = $1 AND status = 'completed' AND payment_type != 'tip'`, bookingID).Scan(&bookingPortion)) + assert.InDelta(t, 50.0, bookingPortion, 0.001, "the booking portion must total the £50 obligation (no overflow mis-booked as service revenue)") + + var tipCount int + var tipAmount float64 + require.NoError(t, db.Conn.QueryRow(freshCtx, `SELECT COUNT(*), COALESCE(SUM(amount), 0) FROM payments WHERE booking_id = $1 AND status = 'completed' AND payment_type = 'tip'`, bookingID).Scan(&tipCount, &tipAmount)) + assert.Equal(t, 1, tipCount, "the £10 overflow must be carved out as a tip record") + assert.InDelta(t, 10.0, tipAmount, 0.001, "the tip must equal the £10 overflow") + + // The booking was fully paid by the rescue → completed. + var bookingStatus string + require.NoError(t, db.Conn.QueryRow(freshCtx, `SELECT status FROM bookings WHERE id = $1`, bookingID).Scan(&bookingStatus)) + assert.Equal(t, "completed", bookingStatus, "the fully-paid rescue must run the completion side-effects") +} + +// ============================================================================= +// MEDIUM-5 — the till gift-card create/top-up must go through the same £5,000/ +// day admin cap as the admin API surface. The till's own same-day value counts. +// ============================================================================= + +// TestLoopA_TillGiftCard_DailyCap_Enforced seeds a till_sales row of £4,800 +// created by the admin today and verifies a £250 till create is rejected 400 +// (would land the day on £5,050 — over the cap; £250 is at the per-transaction +// limit so the daily check is what fires) while a £200 create lands exactly on +// the £5,000 cap and succeeds — pinning the cap as inclusive and the till's own +// value as counted. +func TestLoopA_TillGiftCard_DailyCap_Enforced(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + adminID, err := fixtures.CreateTestAdminUser(tx) + require.NoError(t, err) + adminToken := jwt.GenerateTestToken(adminID, "admin") + + // A gift card created BEFORE today that the admin topped up at the till + // today for £4,800 — the seeded till_sales row is the day's issued value. + var cardID string + require.NoError(t, tx.QueryRow(ctx, ` + INSERT INTO gift_cards (total_funds_added, amount_remaining, created_by, created_at) + VALUES (4800.00, 4800.00, $1, NOW() - INTERVAL '1 day') RETURNING id + `, adminID).Scan(&cardID)) + _, err = tx.Exec(ctx, ` + INSERT INTO till_sales (item_type, item_id, description, quantity, unit_price, total_amount, + payment_method, status, idempotency_key, created_by, created_at, updated_at) + VALUES ('gift_card', $1, 'Gift Card topup', 1, 4800.00, 4800.00, 'cash', 'completed', + 'loop-a-till-seed', $2, NOW(), NOW()) + `, cardID, adminID) + require.NoError(t, err) + + // £250 (the per-transaction maximum) would land the day on £5,050 — over + // the £5,000 daily cap. + over := makeTillSaleRequest(t, TillSaleRequest{ + ItemType: "gift_card", + Action: "create", + Amount: 250.00, + PaymentMethod: "cash", + IdempotencyKey: "loop-a-till-over", + }, adminToken, ctx, tx.(pgx.Tx)) + require.Equal(t, http.StatusBadRequest, over.Code, "body: %s", over.Body.String()) + assert.Contains(t, over.Body.String(), "£5,000", "the rejection must cite the daily cap") + assert.Contains(t, over.Body.String(), "daily", "the rejection must be the daily-limit message") + + // £200 lands the day on EXACTLY £5,000 — inside the cap (inclusive). + ok := makeTillSaleRequest(t, TillSaleRequest{ + ItemType: "gift_card", + Action: "create", + Amount: 200.00, + PaymentMethod: "cash", + IdempotencyKey: "loop-a-till-ok", + }, adminToken, ctx, tx.(pgx.Tx)) + require.Equal(t, http.StatusCreated, ok.Code, "boundary body: %s", ok.Body.String()) +} + +// ============================================================================= +// LOW-6 — expiry is enforced at redemption, not just by the nightly cleanup +// job: a card whose expiry_date has passed cannot be redeemed even before the +// next CleanupExpiredGiftCards run. +// ============================================================================= + +// TestLoopA_RedeemExpiredCard_Rejected redeems a card whose expiry_date is in +// the past but whose amount_remaining is still live (the nightly job has not +// run yet). The redemption must be rejected 400 and the card left untouched. +func TestLoopA_RedeemExpiredCard_Rejected(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + token := jwt.GenerateTestToken(userID, "verified_email") + + var cardID string + require.NoError(t, tx.QueryRow(ctx, ` + INSERT INTO gift_cards (total_funds_added, amount_remaining, expiry_date) + VALUES (20.00, 20.00, NOW() - INTERVAL '1 day') RETURNING id + `).Scan(&cardID)) + + w := redeemCodeRequest(t, token, tx.(pgx.Tx), cardID) + require.Equal(t, http.StatusBadRequest, w.Code, "an expired card must not be redeemable, body: %s", w.Body.String()) + assert.Contains(t, w.Body.String(), "expired") + + // The card is untouched: balance live, not redeemed, no balance credited. + var remaining float64 + var redeemedBy interface{} + require.NoError(t, tx.QueryRow(ctx, `SELECT amount_remaining, redeemed_by FROM gift_cards WHERE id = $1`, cardID).Scan(&remaining, &redeemedBy)) + assert.Equal(t, 20.0, remaining, "the expired card's balance must be left untouched") + assert.Nil(t, redeemedBy, "the expired card must not be marked redeemed") + + var balanceCount int + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM user_giftcard_balances WHERE user_id = $1`, userID).Scan(&balanceCount)) + assert.Zero(t, balanceCount, "no balance may be credited from an expired card") +} diff --git a/backend/handlers/payments/refunds.go b/backend/handlers/payments/refunds.go index ed27b71..3a7a04e 100644 --- a/backend/handlers/payments/refunds.go +++ b/backend/handlers/payments/refunds.go @@ -1361,12 +1361,10 @@ func processChargeGroup(ctx context.Context, chargeID string, rows []pendingChar // ListPaymentRefunds); FAILED/REJECTED is a definitive failure that // must not be marked completed (that would block the amount in the // over-refund guard forever). - sqStatus := "completed" - if sqResult.Status == "PENDING" { - sqStatus = "pending" + sqStatus := squareRefundStatusToLocal(sqResult.Status) + if sqStatus == "pending" { log.Printf("Square refund %s for charge %s is PENDING — leaving refunds pending for the sweep", sqResult.ID, chargeID) - } else if sqResult.Status == "FAILED" || sqResult.Status == "REJECTED" { - sqStatus = "failed" + } else if sqStatus == "failed" { log.Printf("Square refund %s for charge %s FAILED — marking refunds failed", sqResult.ID, chargeID) } // ATOMIC — one statement for the whole group, never per-row. Keeps @@ -2265,12 +2263,10 @@ func processManualPaymentGroup(ctx context.Context, paymentID string, rows []man // resolves to completed; PENDING stays pending for the sweep to // reconcile; FAILED/REJECTED is a real failure. Mirrors // processChargeGroup and the RefundPayment handler (handlers.go). - sqStatus := "completed" - if sqResult.Status == "PENDING" { - sqStatus = "pending" + sqStatus := squareRefundStatusToLocal(sqResult.Status) + if sqStatus == "pending" { log.Printf("Square refund %s for manual refund %s is PENDING (in flight) — leaving the row pending for the sweep to resolve", sqResult.ID, pr.ID) - } else if sqResult.Status == "FAILED" || sqResult.Status == "REJECTED" { - sqStatus = "failed" + } else if sqStatus == "failed" { log.Printf("Square refund %s for manual refund %s FAILED — marking the row failed", sqResult.ID, pr.ID) } if _, upErr := db.Conn.Exec(ctx, ` diff --git a/backend/handlers/payments/service.go b/backend/handlers/payments/service.go index e301c65..00e5603 100644 --- a/backend/handlers/payments/service.go +++ b/backend/handlers/payments/service.go @@ -639,6 +639,43 @@ func (s *PaymentService) DeletePaymentMethod(ctx context.Context, cardID, userID return nil } +// upsertUserSavedCard persists a tokenized card as a saved card for the user, +// shared by CreatePaymentMethodFromToken and SaveCardForUser so the +// ON CONFLICT upsert semantics cannot drift between the two save paths. +// ON CONFLICT (user_id, square_card_id) DO UPDATE handles the two cases a +// plain INSERT cannot: a response-lost retry re-tokenizing the SAME card for +// the SAME user (CreateCardOnFile's deterministic key returns the same ccof: +// id), and a soft-deleted card still occupying the per-user UNIQUE slot (the +// DO UPDATE revives it). The conflict target is scoped per user — a card +// tokenized by user B that user A already saved is a brand-new row for B, +// never a mutation of A's row. defaultIfFirst marks the user's FIRST card as +// default (NOT EXISTS) — true for CreatePaymentMethodFromToken (the save-card +// endpoint makes the first card default), false for SaveCardForUser (a card +// saved during a charge is never made default). Returns the row id and whether +// the row is default. +func upsertUserSavedCard(ctx context.Context, q querier, userID, squareCustomerID, squareCardID, brand, last4 string, expMonth, expYear int, fingerprint string, defaultIfFirst bool) (id string, isDefault bool, err error) { + err = q.QueryRow(ctx, ` + INSERT INTO user_saved_cards (user_id, square_card_id, brand, last_4, exp_month, exp_year, fingerprint, square_customer_id, is_default) + SELECT $1, $2, $3, $4, $5, $6, $7, $8, + CASE WHEN $9 THEN NOT EXISTS(SELECT 1 FROM user_saved_cards WHERE user_id = $1 AND deleted_at IS NULL) ELSE false END + ON CONFLICT (user_id, square_card_id) DO UPDATE SET + brand = EXCLUDED.brand, + last_4 = EXCLUDED.last_4, + exp_month = EXCLUDED.exp_month, + exp_year = EXCLUDED.exp_year, + fingerprint = EXCLUDED.fingerprint, + square_customer_id = EXCLUDED.square_customer_id, + deleted_at = NULL, + retained_until = NULL + WHERE user_saved_cards.user_id = EXCLUDED.user_id + RETURNING id, is_default + `, userID, squareCardID, brand, last4, expMonth, expYear, fingerprint, squareCustomerID, defaultIfFirst).Scan(&id, &isDefault) + if err != nil { + return "", false, err + } + return id, isDefault, nil +} + func (s *PaymentService) CreatePaymentMethodFromToken(ctx context.Context, userID, cardToken string) (*SavedCard, error) { // PCI-DSS: raw PANs are never accepted. The client must supply a Square // Web Payments nonce (cnon:xxx), which the backend tokenizes via the @@ -660,29 +697,11 @@ func (s *PaymentService) CreatePaymentMethodFromToken(ctx context.Context, userI var savedCardID string var isDefault bool - // ON CONFLICT (user_id, square_card_id): a response-lost retry re-tokenizes - // the same card for the SAME user (CreateCardOnFile's deterministic key - // returns the same ccof: id), so the per-user UNIQUE constraint would - // otherwise 500 on the duplicate. Upsert instead so the retry returns the - // existing saved card (N-8). The conflict target is scoped per user — a - // card tokenized by user B that user A already saved is a brand-new row for - // B, never a mutation of A's row. - err = db.Conn.QueryRow(ctx, ` - INSERT INTO user_saved_cards (user_id, square_card_id, brand, last_4, exp_month, exp_year, fingerprint, square_customer_id, is_default) - SELECT $1, $2, $3, $4, $5, $6, $7, $8, - NOT EXISTS(SELECT 1 FROM user_saved_cards WHERE user_id = $1 AND deleted_at IS NULL) - ON CONFLICT (user_id, square_card_id) DO UPDATE SET - brand = EXCLUDED.brand, - last_4 = EXCLUDED.last_4, - exp_month = EXCLUDED.exp_month, - exp_year = EXCLUDED.exp_year, - fingerprint = EXCLUDED.fingerprint, - square_customer_id = EXCLUDED.square_customer_id, - deleted_at = NULL, - retained_until = NULL - WHERE user_saved_cards.user_id = EXCLUDED.user_id - RETURNING id, is_default - `, userID, cardOnFile.CardID, cardOnFile.Brand, cardOnFile.Last4, cardOnFile.ExpMonth, cardOnFile.ExpYear, cardOnFile.Fingerprint, squareCustomerID).Scan(&savedCardID, &isDefault) + // The shared upsert handles the ON CONFLICT cases a plain INSERT cannot + // (response-lost retry re-tokenizing the same card, and soft-deleted rows + // still occupying the per-user UNIQUE slot). The first card a user saves + // becomes default (defaultIfFirst=true). + savedCardID, isDefault, err = upsertUserSavedCard(ctx, db.Conn, userID, squareCustomerID, cardOnFile.CardID, cardOnFile.Brand, cardOnFile.Last4, cardOnFile.ExpMonth, cardOnFile.ExpYear, cardOnFile.Fingerprint, true) if err != nil { return nil, fmt.Errorf("failed to save card: %w", err) } @@ -803,34 +822,13 @@ func (s *PaymentService) EnsureSquareCustomerForSavedCard(ctx context.Context, s // method NEVER re-provisions (R7: a second ensureSquareCustomer would re-query // the DB and, on a first-save flow, re-run CreateCustomer). func (s *PaymentService) SaveCardForUser(ctx context.Context, userID, squareCustomerID, squareCardID, brand, last4 string, expMonth, expYear int, fingerprint string) (string, error) { - var id string - // ON CONFLICT (user_id, square_card_id) DO UPDATE — two cases: - // 1. Same-key retry of a save_card=true charge (resolveChargeSource runs - // CreateCardOnFile with a deterministic sha256 key, so Square returns the - // SAME ccof: id): a plain INSERT would violate the per-user UNIQUE - // constraint (N-8) and DO NOTHING would 500 via the fallback select. - // 2. A soft-deleted card (DeletePaymentMethod set deleted_at but the row - // still occupies the UNIQUE slot): the DO UPDATE revives it - // (deleted_at/retained_until = NULL), matching CreatePaymentMethodFromToken. - // The conflict target is scoped per user — a card tokenized by user B that - // user A already saved is a brand-new row for B, never a mutation of A's row. - err := db.Conn.QueryRow(ctx, ` - INSERT INTO user_saved_cards ( - user_id, square_card_id, square_customer_id, brand, last_4, exp_month, exp_year, fingerprint, is_default, created_at - ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, false, NOW()) - ON CONFLICT (user_id, square_card_id) DO UPDATE SET - square_customer_id = EXCLUDED.square_customer_id, - brand = EXCLUDED.brand, - last_4 = EXCLUDED.last_4, - exp_month = EXCLUDED.exp_month, - exp_year = EXCLUDED.exp_year, - fingerprint = EXCLUDED.fingerprint, - deleted_at = NULL, - retained_until = NULL - WHERE user_saved_cards.user_id = EXCLUDED.user_id - RETURNING id - `, userID, squareCardID, squareCustomerID, brand, last4, expMonth, expYear, fingerprint).Scan(&id) - + // The shared upsert handles the ON CONFLICT cases a plain INSERT cannot: + // a same-key retry of a save_card=true charge (resolveChargeSource runs + // CreateCardOnFile with a deterministic sha256 key, so Square returns the + // SAME ccof: id), and a soft-deleted card still occupying the per-user + // UNIQUE slot (the DO UPDATE revives it). A card saved during a charge is + // never made default (defaultIfFirst=false). + id, _, err := upsertUserSavedCard(ctx, db.Conn, userID, squareCustomerID, squareCardID, brand, last4, expMonth, expYear, fingerprint, false) if err != nil { return "", err } diff --git a/backend/handlers/payments/sweep.go b/backend/handlers/payments/sweep.go index 1154479..2ceacc4 100644 --- a/backend/handlers/payments/sweep.go +++ b/backend/handlers/payments/sweep.go @@ -19,6 +19,7 @@ import ( "crussell/internal/square" "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgconn" ) // SweepStalePendingPayments resolves pending payment records that are older @@ -255,7 +256,7 @@ func sweepStaleRows(ctx context.Context, table string, cutoff time.Time) (resolv continue } } - if rescueStaleRowCompleted(ctx, table, r.ID) { + if rescueStaleRowCompleted(ctx, table, r) { resolved++ completed++ } @@ -377,7 +378,7 @@ func sweepKeyedStaleRows(ctx context.Context, table string, cutoff time.Time) (r continue } } - if rescueKeyedStaleRowCompleted(ctx, table, r.ID, sqPayID) { + if rescueKeyedStaleRowCompleted(ctx, table, r, sqPayID) { resolved++ completed++ } @@ -520,36 +521,181 @@ func failStaleRow(ctx context.Context, table, id string) bool { } // rescueStaleRowCompleted marks one stale pending row (whose square_payment_id -// already resolves to a COMPLETED charge) 'completed'. Returns true when the -// row was updated. -func rescueStaleRowCompleted(ctx context.Context, table, id string) bool { - tag, err := db.Conn.Exec(ctx, ` - UPDATE `+pgx.Identifier{table}.Sanitize()+` SET status = 'completed', updated_at = NOW() - WHERE id = $1 AND status = 'pending' - `, id) - if err != nil { - log.Printf("Failed to rescue stale pending row %s to completed: %v", id, err) - return false - } - return int(tag.RowsAffected()) > 0 +// already resolves to a COMPLETED charge) 'completed'. For a payments row with +// a booking this mirrors the live-path recording semantics (MEDIUM-3): the +// row's charge is re-split through buildSplitRecords so any overflow beyond +// the booking's remaining obligation is carved out as its own +// payment_type='tip' record (never mis-booked as service revenue), and the +// fully-paid completion check runs so a rescue that settles the booking +// triggers the completion side-effects exactly like the live path. Returns +// true when the row was updated. +func rescueStaleRowCompleted(ctx context.Context, table string, r staleRow) bool { + return rescueStaleRowCompletedTx(ctx, table, r.ID, "", r) } // rescueKeyedStaleRowCompleted marks a keyed stale pending row 'completed' with -// the square_payment_id returned by the replay — the lost-response rescue. The -// reconcile is deliberately minimal (status + square_payment_id + updated_at -// only, no split/VAT recomputation): the row is >22h stale and this is a -// reconciliation rescue, mirroring the F3 by-id rescue's minimality. Returns -// true when the row was updated. -func rescueKeyedStaleRowCompleted(ctx context.Context, table, id, squarePaymentID string) bool { - tag, err := db.Conn.Exec(ctx, ` - UPDATE `+pgx.Identifier{table}.Sanitize()+` SET status = 'completed', square_payment_id = $1, updated_at = NOW() - WHERE id = $2 AND status = 'pending' - `, squarePaymentID, id) +// the square_payment_id returned by the replay — the lost-response rescue — and +// applies the same split/completion semantics as the by-id rescue above +// (MEDIUM-3). Returns true when the row was updated. +func rescueKeyedStaleRowCompleted(ctx context.Context, table string, r staleRow, squarePaymentID string) bool { + return rescueStaleRowCompletedTx(ctx, table, r.ID, squarePaymentID, r) +} + +// rescueStaleRowCompletedTx is the shared implementation behind both rescue +// paths. The split records are computed BEFORE the status flip (while the row +// is still 'pending', so GetBookingPaymentInfo's paid ledger excludes it — +// exactly where the live path computes buildSplitRecords), and the flip + +// split insert + completion run in ONE transaction so a crash can never leave +// the row completed but the carve missing. +func rescueStaleRowCompletedTx(ctx context.Context, table, id, squarePaymentID string, r staleRow) bool { + tx, err := db.Conn.Begin(ctx) + if err != nil { + log.Printf("Failed to begin rescue transaction for stale pending %s row %s: %v", table, id, err) + return false + } + defer func() { + if rErr := tx.Rollback(ctx); rErr != nil && !errors.Is(rErr, pgx.ErrTxClosed) { + log.Printf("Failed to rollback rescue transaction for stale pending %s row %s: %v", table, id, rErr) + } + }() + + // Pre-compute the split while the row is still 'pending' so the booking's + // paid ledger excludes it (mirroring the live post-charge path, where + // buildSplitRecords runs before the primary row is marked completed). + records := buildStaleRescueRecords(ctx, tx, r, squarePaymentID) + + var tag pgconn.CommandTag + if squarePaymentID != "" { + tag, err = tx.Exec(ctx, ` + UPDATE `+pgx.Identifier{table}.Sanitize()+` SET status = 'completed', square_payment_id = $1, updated_at = NOW() + WHERE id = $2 AND status = 'pending' + `, squarePaymentID, id) + } else { + tag, err = tx.Exec(ctx, ` + UPDATE `+pgx.Identifier{table}.Sanitize()+` SET status = 'completed', updated_at = NOW() + WHERE id = $1 AND status = 'pending' + `, id) + } if err != nil { log.Printf("Failed to rescue stale pending row %s to completed: %v", id, err) return false } - return int(tag.RowsAffected()) > 0 + if int(tag.RowsAffected()) == 0 { + // Already resolved concurrently — nothing to do. + if cErr := tx.Commit(ctx); cErr != nil { + log.Printf("Failed to commit rescue no-op for stale pending %s row %s: %v", table, id, cErr) + } + return false + } + + if len(records) > 0 { + applyStaleRescueRecords(ctx, tx, r, records) + } + + if cErr := tx.Commit(ctx); cErr != nil { + log.Printf("Failed to commit rescue of stale pending %s row %s: %v", table, id, cErr) + return false + } + return true +} + +// buildStaleRescueRecords computes the payment records a rescued stale pending +// booking payment should be split into (MEDIUM-3). It mirrors the live +// post-charge path: buildSplitRecords partitions the row's charge into the +// deposit/balance/full booking portion plus a carved payment_type='tip' record +// for any overflow beyond the booking's remaining obligation — so a rescue can +// never mis-book the overflow as service revenue. The row MUST still be +// 'pending' when this runs (the paid ledger must exclude it). Rows that need +// no split (tip-type rows, rows without a booking, till_sales) return nil. +// Any error is logged and nil is returned: the row is still rescued (status +// flip) un-split — never worse than the pre-fix minimal flip — and surfaces in +// the log for manual reconciliation. +func buildStaleRescueRecords(ctx context.Context, tx pgx.Tx, r staleRow, squarePaymentID string) []PaymentRecord { + if r.BookingID == nil { + return nil + } + var paymentType, paymentMethod string + var createdBy sql.NullString + if err := tx.QueryRow(ctx, `SELECT payment_type, payment_method, created_by FROM payments WHERE id = $1 FOR UPDATE`, r.ID).Scan(&paymentType, &paymentMethod, &createdBy); err != nil { + log.Printf("MEDIUM-3: failed to read payment row %s for rescue split (booking %s): %v — the row is completed un-split; manual reconciliation recommended", r.ID, *r.BookingID, err) + return nil + } + if paymentType == "tip" { + // A tip row is already tip-type — no carve is needed (a tip can never + // fully pay a booking, so the completion check would be a no-op too). + return nil + } + info, err := NewPaymentService().GetBookingPaymentInfo(ctx, *r.BookingID) + if err != nil || info == nil { + log.Printf("MEDIUM-3: failed to load booking info for rescue split of payment %s (booking %s): %v — the row is completed un-split; manual reconciliation recommended", r.ID, *r.BookingID, err) + return nil + } + amount := float64(r.AmountPence) / 100.0 + spID := r.SquarePaymentID + if squarePaymentID != "" { + spID = squarePaymentID + } + var createdByPtr *string + if createdBy.Valid && createdBy.String != "" { + c := createdBy.String + createdByPtr = &c + } + primary := PaymentRecord{ + BookingID: *r.BookingID, + PaymentType: paymentType, + PaymentMethod: paymentMethod, + Status: "completed", + Amount: amount, + SquarePaymentID: &spID, + CreatedBy: createdByPtr, + CreatedAt: clock.Now(), + UpdatedAt: clock.Now(), + } + if r.IdempotencyKey != "" { + k := r.IdempotencyKey + primary.IdempotencyKey = &k + } + return buildSplitRecords(primary, paymentType, info, amount) +} + +// applyStaleRescueRecords applies the pre-computed split records inside the +// rescue transaction: the primary row is aligned to records[0] and the +// remaining split records (balance/full portion + any carved tip) are inserted +// with their derived idempotency keys. The fully-paid completion check then +// runs so a rescue that settles the booking triggers the completion +// side-effects (loyalty stamps, campaigns, deposits_required) the live path +// would have run. Errors are logged with the row left as-is for manual +// reconciliation — the money was already completed at Square, so the rescue +// must never abort on a bookkeeping failure. +func applyStaleRescueRecords(ctx context.Context, tx pgx.Tx, r staleRow, records []PaymentRecord) { + if len(records) == 0 { + return + } + if _, upErr := tx.Exec(ctx, ` + UPDATE payments SET + amount = $1, + payment_type = $2, + fees = $3, + is_vat_applicable = FALSE, + vat_rate = NULL, + vat_amount = NULL, + net_amount = NULL, + updated_at = NOW() + WHERE id = $4 + `, records[0].Amount, records[0].PaymentType, records[0].Fees, r.ID); upErr != nil { + log.Printf("MEDIUM-3: failed to align rescued payment %s to its split primary (%v) — manual reconciliation recommended", r.ID, upErr) + return + } + svc := NewPaymentService() + for _, rec := range records[1:] { + if _, cErr := svc.CreatePaymentRecordTx(ctx, tx, rec, nil); cErr != nil { + log.Printf("MEDIUM-3: failed to insert rescue split record for payment %s (%v) — manual reconciliation recommended", r.ID, cErr) + return + } + } + if bookingIsFullyPaid(ctx, tx, *r.BookingID) { + completeActiveBookingFromPayment(ctx, tx, *r.BookingID) + } } // staleBookingGate is the outcome of the pre-completion booking-status recheck diff --git a/backend/handlers/payments/till.go b/backend/handlers/payments/till.go index 19bd4dd..e0e6359 100644 --- a/backend/handlers/payments/till.go +++ b/backend/handlers/payments/till.go @@ -212,20 +212,12 @@ func deriveTillIdempotencyKey(req TillSaleRequest, adminID string) string { } // Always-hashed, NEVER truncateIdempotencyKey: a pre-deploy row stores the // hashed key, so a raw-key re-derivation would miss the dedup and double- - // charge. The slot-scan candidates (tillIdempotencyKeyCandidate) keep their + // charge. The slot-scan candidates (via nextIdempotencyCandidate) keep their // own conditional truncation, matching the pre-batch code. sum := sha256.Sum256([]byte(sb.String())) return "till-" + hex.EncodeToString(sum[:16]) } -// tillIdempotencyKeyCandidate appends the slot-sequence suffix to a derived -// base key, hashing back under Square's 45-char /v2/payments limit when the -// verbatim form would overflow (the hash stays deterministic). Shared -// implementation: nextIdempotencyCandidate (idempotency_helpers.go). -func tillIdempotencyKeyCandidate(baseKey string, seq int) string { - return nextIdempotencyCandidate(baseKey, seq) -} - // scanTillIdempotencyKeySlot resolves the FINAL deterministic idempotency key // for a keyless Square-method till sale, mirroring the gift-card slot pattern // (deriveGiftCardIdempotencyKey). Must be called under the crussell:till @@ -240,23 +232,20 @@ func tillIdempotencyKeyCandidate(baseKey string, seq int) string { // pending row and adopts its STORED key, and Square dedups the charge onto the // original — ONE charge, ONE gift-card funding. func scanTillIdempotencyKeySlot(ctx context.Context, baseKey string) (string, error) { - for seq := 0; ; seq++ { - candidate := tillIdempotencyKeyCandidate(baseKey, seq) + return scanIdempotencySlot(ctx, baseKey, func(candidate string) (bool, error) { var status string err := db.Conn.QueryRow(ctx, `SELECT status FROM till_sales WHERE idempotency_key = $1`, candidate).Scan(&status) if errors.Is(err, pgx.ErrNoRows) { - return candidate, nil + return false, nil } if err != nil { - return "", fmt.Errorf("failed to scan till idempotency-key slot for %s: %w", baseKey, err) + return false, fmt.Errorf("failed to scan till idempotency-key slot for %s: %w", baseKey, err) } - if status == "completed" || status == "failed" { - continue // occupied slot — a distinct sale must diverge onto a fresh key. - } - // Pending (or any other state): the same logical sale is in flight — a - // lost-response retry must reuse it, so keep this candidate. - return candidate, nil - } + // A COMPLETED (or swept/declined FAILED) sale occupies its candidate + // slot; PENDING never occupies it — a lost-response retry reuses the + // pending row, so the candidate stays free for that reuse. + return status == "completed" || status == "failed", nil + }) } // refreshTillSnapshotSource rewrites the source_id field inside the stored @@ -321,6 +310,27 @@ func refreshTillSnapshotSource(ctx context.Context, tx pgx.Tx, tillSaleID, newSo // the sale is no longer 'pending' and the gift card must be left untouched. var errTillSaleNotPending = errors.New("till sale is not pending") +// tillSaleHasLiveRefund reports whether the till sale has a refund in a state +// meaning its money is no longer fully live: a completed refund (money +// returned) or a pending refund (money in flight). Failed refunds never moved +// money and are excluded. A till sale has no payments row of its own (refunds +// are FK'd to payments), so its refunds are matched by the sweep's +// deterministic reason string (sweepDuplicateRefundReasonFor) — the same +// linkage the B1 re-poll and in-flight guard use. Used to re-validate the till +// completed-dedup hit, mirroring paymentHasLiveRefund on the booking paths. +func tillSaleHasLiveRefund(ctx context.Context, q db.Querier, tillSaleID string) (bool, error) { + var exists bool + err := q.QueryRow(ctx, ` + SELECT EXISTS( + SELECT 1 FROM refunds WHERE reason = $1 AND status IN ('completed', 'pending') + ) + `, sweepDuplicateRefundReasonFor(tillSaleID)).Scan(&exists) + if err != nil { + return false, err + } + return exists, nil +} + // revertGiftCardFunding is the package-internal wrapper over the shared // RevertGiftCardFunding clawback helper (giftcard_clawback.go). The sweep and // the till handler both call it so every clawback path runs the single @@ -519,6 +529,20 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { http.Error(w, "Amount does not match the completed till sale", http.StatusBadRequest) return } + // RE-VALIDATE the matched sale's refund state before reporting + // it as success (same guard as the booking/tip/gift-card/ + // terminal completed-dedups): a refunded sale's money is no + // longer live, so a same-key retry must not report it as + // success. + if refunded, rErr := tillSaleHasLiveRefund(ctx, db.Conn, existingID); rErr != nil { + log.Printf("Failed to re-validate till-sale dedup hit %s against refunds: %v", existingID, rErr) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } else if refunded { + log.Printf("Till-sale retry rejected: completed sale %s (key %q) was refunded — refusing to report a refunded sale as success", existingID, req.IdempotencyKey) + http.Error(w, "This till sale has been refunded and can no longer be replayed", http.StatusConflict) + return + } if err := json.NewEncoder(w).Encode(TillSaleResponse{ ID: existingID, ItemType: existingItemType, @@ -598,6 +622,26 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { service := NewPaymentService() + // MEDIUM-5: the shared £5,000/day admin gift-card cap (giftcard_limits.go + // maxAdminGiftCardDailyPence) must cover till creates/topups too — the till + // is an admin money surface and otherwise could issue unlimited balance. + // adminGiftCardValueToday now includes the till's own same-day value + // (double-count-free), so this is the SAME daily-cap check as + // CreateGiftCard / TopUpGiftCard / TransferGiftCard. Placed AFTER the + // idempotency dedup so a same-key retry of an already-completed sale is + // returned (not blocked) even on a capped day. Runs before any gift-card + // write. + adminValueToday, dailyErr := adminGiftCardValueToday(ctx, db.Conn, adminID) + if dailyErr != nil { + log.Printf("Failed to query admin gift-card value today for %s: %v", adminID, dailyErr) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + if int64(math.Round(adminValueToday*100))+amountPence > maxAdminGiftCardDailyPence { + http.Error(w, "You have reached your £5,000 daily gift-card value limit", http.StatusBadRequest) + return + } + // squarePaymentID/squareCheckoutID are set inside the payment-method switch // below but must be declared before the deferred cancel-on-error closure // (registered at tx creation) so it can read the live checkout id. @@ -1125,12 +1169,7 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { } if req.PaymentMethod != "on_the_house" && (saleStatus == "completed" || needsSquarePayment) { - vatCfg, vatErr := GetVATConfig(ctx, tx) - if vatErr == nil && vatAppliesToVoucher(vatCfg) { - if _, vatExecErr := tx.Exec(ctx, "SELECT apply_vat_to_till_sale($1, $2)", tillSaleID, vatCfg.DefaultVATRate); vatExecErr != nil { - log.Printf("Failed to apply VAT to till sale %s: %v", tillSaleID, vatExecErr) - } - } + applyVATToChargeRecord(ctx, tx, tillSaleID, true) } } @@ -1193,17 +1232,9 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { // snapshot-is-null guard like the booking/tip/terminal flows): the // pending-reuse branch above already refreshed square_request_snapshot // in the SAME transaction as the square_source_id refresh - // (refreshTillSnapshotSource, B6), and this post-commit write stores - // the fresh full body for THIS attempt. A guard would wrongly skip - // this write on the reuse path when the in-tx refresh failed - // best-effort — do NOT "fix" it into the guarded form. - if snap, mErr := json.Marshal(paymentReq); mErr != nil { - log.Printf("Failed to marshal square_request_snapshot for till sale %s: %v", tillSaleID, mErr) - } else if stored, eErr := encryptSnapshot(snap); eErr != nil { - log.Printf("Failed to encrypt square_request_snapshot for till sale %s: %v", tillSaleID, eErr) - } else if _, sErr := db.Conn.Exec(ctx, `UPDATE till_sales SET square_request_snapshot = $1 WHERE id = $2`, string(stored), tillSaleID); sErr != nil { - log.Printf("Failed to store square_request_snapshot for till sale %s: %v", tillSaleID, sErr) - } + // (refreshTillSnapshotSource, B6), so a reuse never needs this + // post-commit write to overwrite the guard. + writeChargeSnapshot(ctx, db.Conn, "till_sales", tillSaleID, paymentReq, "till sale") paymentResult, squareErr = SquareClient.CreatePayment(ctx, paymentReq) } else if req.PaymentMethod == "online_square" { @@ -1234,22 +1265,13 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { // charge with an IDENTICAL body under the same key — Square // compares the whole request on key reuse, and a reconstructed body // returns IDEMPOTENCY_KEY_REUSED, leaving the row pending forever. - // - // The write is INTENTIONALLY unconditional (WHERE id = $2, no - // snapshot-is-null guard like the booking/tip/terminal flows): the - // pending-reuse branch above already refreshed square_request_snapshot - // in the SAME transaction as the square_source_id refresh - // (refreshTillSnapshotSource, B6), and this post-commit write stores - // the fresh full body for THIS attempt. A guard would wrongly skip - // this write on the reuse path when the in-tx refresh failed - // best-effort — do NOT "fix" it into the guarded form. - if snap, mErr := json.Marshal(paymentReq); mErr != nil { - log.Printf("Failed to marshal square_request_snapshot for till sale %s: %v", tillSaleID, mErr) - } else if stored, eErr := encryptSnapshot(snap); eErr != nil { - log.Printf("Failed to encrypt square_request_snapshot for till sale %s: %v", tillSaleID, eErr) - } else if _, sErr := db.Conn.Exec(ctx, `UPDATE till_sales SET square_request_snapshot = $1 WHERE id = $2`, string(stored), tillSaleID); sErr != nil { - log.Printf("Failed to store square_request_snapshot for till sale %s: %v", tillSaleID, sErr) - } + // The write is immutability-guarded (same rule as the + // booking/tip/terminal flows): it records the FIRST attempt's body + // only — the pending-reuse branch above refreshes + // square_request_snapshot in the SAME transaction as the + // square_source_id refresh (refreshTillSnapshotSource, B6), so a + // reuse never needs this post-commit write to overwrite the guard. + writeChargeSnapshot(ctx, db.Conn, "till_sales", tillSaleID, paymentReq, "till sale") paymentResult, squareErr = SquareClient.CreatePayment(ctx, paymentReq) } @@ -1277,8 +1299,8 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { // 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) + if req.PaymentMethod == "saved_card" && cardUserID.Valid { + reissueTwoFACodeAfterFailedCharge(ctx, db.Conn, cardUserID.String, true, twoFAFallbackUsed && existingPendingID == "", r) } // 402 only for definitive declines; ambiguous transport/5xx must be // 503 so the pending sale stays resumable on a same-key retry @@ -1332,7 +1354,7 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { } // MEDIUM-3a: record the admin-initiated saved-card till charge in // admin_audit_log (mirroring giftcards.go's balance_check audit). - insertAdminAuditCharge(ctx, adminID, cardUserID.String, "till_saved_card_charge", map[string]any{ + InsertAdminAuditCharge(ctx, adminID, cardUserID.String, "till_saved_card_charge", map[string]any{ "till_sale_id": tillSaleID, "item_type": req.ItemType, "gift_card_id": giftCardID, @@ -1376,6 +1398,22 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { saleStatus = "completed" } + // MEDIUM-3a coverage: an admin till sale that moves money (cash, + // card_machine, online_square — saved_card already writes its own audit row + // above) is an admin money action. on_the_house creates no money movement + // and is not audited. Record one admin_audit_log row per completed sale + // (best-effort, own transaction — a failed audit write never fails the + // sale). + if saleStatus == "completed" && req.PaymentMethod != "on_the_house" && req.PaymentMethod != "saved_card" { + InsertAdminAuditCharge(ctx, adminID, "", "admin_till_sale", map[string]any{ + "till_sale_id": tillSaleID, + "item_type": req.ItemType, + "gift_card_id": giftCardID, + "amount": req.Amount, + "method": req.PaymentMethod, + }) + } + w.WriteHeader(http.StatusCreated) if err := json.NewEncoder(w).Encode(TillSaleResponse{ ID: tillSaleID, diff --git a/backend/handlers/payments/twofa.go b/backend/handlers/payments/twofa.go index 84b572f..8bd0d50 100644 --- a/backend/handlers/payments/twofa.go +++ b/backend/handlers/payments/twofa.go @@ -2,12 +2,17 @@ package payments import ( "context" + "crypto/rand" "errors" + "fmt" "log" + "math/big" "net/http" "os" "strings" + "time" + "crussell/clock" "crussell/db" "crussell/internal/twofa" "crussell/mw" @@ -77,14 +82,16 @@ func (s *PaymentService) UserTwoFactorEnabled(ctx context.Context, userID string // (per-user brute-force lockout, constant-time compare, legacy pre-pepper // hash fallback, code lifetime). consume=true makes a verified code SINGLE-USE // immediately (the pending code is NULLed on success); consume=false verifies -// WITHOUT consuming (MEDIUM-2 — the saved-card CHARGE gates pass false and -// defer consumption to the completed-charge transaction via -// twofa.ConsumePendingCode, so a failed Square charge does not burn the code; -// the save-card SAVE gate passes true because saving a card is a terminal -// operation with no downstream charge to attach consumption to). It returns nil -// on a valid code, or a classified twofa.ErrIncorrect / twofa.ErrLockedOut / -// twofa.ErrMissingOrExpired (or a wrapped DB error) for the caller to map to -// the correct HTTP status. +// WITHOUT consuming. Since finding 1 the saved-card CHARGE gates pass +// consume=!reusePendingRecord: a FRESH charge consumes at the gate (one code +// authorizes exactly one charge), while a PENDING-REUSE retry passes false and +// defers consumption to the completed-charge transaction via +// twofa.ConsumePendingCode, so a retry that fails again keeps its code for one +// more attempt. The save-card SAVE gates pass true because saving a card is a +// terminal operation with no downstream charge to attach consumption to. It +// returns nil on a valid code, or a classified twofa.ErrIncorrect / +// twofa.ErrLockedOut / twofa.ErrMissingOrExpired (or a wrapped DB error) for +// the caller to map to the correct HTTP status. func verifyPendingTwoFactorCode(ctx context.Context, userID, code string, consume bool) error { return twofa.VerifyForUser(ctx, userID, code, consume) } @@ -154,10 +161,12 @@ func (s *PaymentService) TwoFactorFallbackEnabled() bool { // email-SMS channel). // // consume controls whether a verified code is NULLed immediately (consume=true -// — the save-card SAVE gate) or left intact for the caller to consume when its -// operation reaches a terminal success state (consume=false — the saved-card -// CHARGE gates; see verifyPendingTwoFactorCode / twofa.ConsumePendingCode, -// MEDIUM-2). In every case the 5-attempt lockout and the +// — a FRESH charge's single-use burn at the gate, closing the TOCTOU where a +// verified-but-unconsumed code could authorize a second charge; and the +// save-card SAVE gate, a terminal operation) or left intact for the caller to +// consume when a PENDING-REUSE retry reaches terminal success +// (consume=false — see verifyPendingTwoFactorCode / twofa.ConsumePendingCode, +// finding 1). In every case the 5-attempt lockout and the // code-destroy-on-lockout semantics are unchanged (twofa.Check). // // The code check is delegated to crussell/internal/twofa via @@ -234,3 +243,100 @@ func requireTwoFactorForCardAccess(w http.ResponseWriter, r *http.Request, servi return false, false } } + +// reissueTwoFACodeAfterFailedCharge mints a fresh 2FA code after a saved-card +// charge failed at Square — but ONLY when a code was actually consumed by a +// FRESH saved-card charge (fresh-only semantics). The charge gate consumes the +// verified code at gate time for fresh charges (single-use — closing the +// verify-then-consume TOCTOU where a verified-but-unconsumed code could +// authorize a second charge), so a failed fresh charge leaves no live code for +// the same-key retry; this re-issues one with the same 10-minute lifetime and +// delivery behaviour as the user package's code issuance (dev/test logs the +// code for the operator to relay; production logs only with +// TWO_FACTOR_ALLOW_LOG_DELIVERY=true, matching the fail-closed delivery +// contract). It is a NO-OP for every other outcome: a new-card (cnon) charge +// never gates (usedSavedCard=false), a pending-reuse retry verified WITHOUT +// consuming (fallbackUsed=false — its code survives for one more attempt and a +// re-issue would silently invalidate the one the customer holds), and an +// SCA-authorized charge never touched the 2FA gate at all. +// +// Callers pass: +// - usedSavedCard: whether this charge actually used a saved card (the 2FA +// gate applies only to saved-card ccof charges); +// - fallbackUsed: whether the 2FA fallback gate actually consumed a code on +// THIS attempt (true only for a fresh charge — the gate's +// twoFAFallbackUsed ANDed with the caller's not-a-pending-reuse test). +// +// LOW-MEDIUM (finding 2): the re-issue is routed through the same fail-closed +// issuance gate as the interactive mint paths (twoFAReissueIssueAllowed — +// mirrored from the user package's twoFAEnsureIssueAllowed via the build-tagged +// twofa_delivery_dev.go / twofa_delivery_prod.go): a production build refuses +// to re-issue when TWO_FACTOR_PEPPER is unset (an unsalted digest in the 1M +// code space would be offline-brute-forceable) or when no delivery channel is +// configured. It also respects the same per-user mint cooldown +// (twoFAMintCooldown via the shared twofa.AttemptState.LastMintAt), so a +// charge-failure loop cannot mint codes faster than the mint endpoints allow. +// Best-effort: a failure logs and the customer requests a fresh code through +// the normal 2FA flow. +func reissueTwoFACodeAfterFailedCharge(ctx context.Context, q db.Querier, userID string, usedSavedCard, fallbackUsed bool, r *http.Request) { + if userID == "" || !twoFactorEnforced() || !usedSavedCard || !fallbackUsed { + return + } + if err := twoFAReissueIssueAllowed(); err != nil { + log.Printf("2FA: refused to re-issue a code for user %s after a failed charge: %v", userID, err) + return + } + // Mint cooldown (B11a): the shared per-user mutex serializes the stamp + // read/write with the user package's mints and the gate's verify critical + // section. A successful verify (twofa.Check) clears the stamp, so a code + // verified at the gate never throttles this immediate re-issue. + st := twofa.StateFor(userID) + st.Mu.Lock() + defer st.Mu.Unlock() + if !st.LastMintAt.IsZero() && clock.Now().Sub(st.LastMintAt) < twoFAMintCooldown { + log.Printf("2FA: re-issue skipped for user %s after a failed charge (mint cooldown)", userID) + return + } + code, err := generatePaymentsTwoFACode() + if err != nil { + log.Printf("2FA: failed to generate a re-issued code for user %s after a failed charge: %v", userID, err) + return + } + if _, err := q.Exec(ctx, ` + UPDATE users + SET two_factor_pending_code_hash = $2, + two_factor_pending_code_expires = $3 + WHERE id = $1 + `, userID, twofa.Hash(code), clock.Now().Add(twoFAPendingCodeLifetime)); err != nil { + log.Printf("2FA: failed to store a re-issued code for user %s after a failed charge: %v", userID, err) + return + } + st.LastMintAt = clock.Now() + // Delivery mirrors the user package's build-dependent behaviour (the + // operator relays the [2FA] log line). Production logs the plaintext code + // only when explicitly opted in; dev/test always. + if IsExplicitDevOrMockEnv() || os.Getenv("TWO_FACTOR_ALLOW_LOG_DELIVERY") == "true" { + log.Printf("[2FA] code delivery requested (user=%s, purpose=re-issue after failed saved-card charge)", userID) + log.Printf("[2FA] code: %s", code) + } +} + +// twoFAMintCooldown bounds how often the re-issue path mints a fresh 2FA code +// for one user after a failed saved-card charge, mirroring the user package's +// mint cooldown (handlers/user/twofa.go). The shared stamp lives on the +// per-user twofa.AttemptState.LastMintAt so both mint paths cohere. +const twoFAMintCooldown = 1 * time.Minute + +// generatePaymentsTwoFACode returns a random 6-digit verification code, +// mirroring the user package's generator (crypto/rand, uniform 0-999999). +func generatePaymentsTwoFACode() (string, error) { + n, err := rand.Int(rand.Reader, big.NewInt(1_000_000)) + if err != nil { + return "", err + } + return fmt.Sprintf("%06d", n.Int64()), nil +} + +// twoFAPendingCodeLifetime is how long a re-issued 2FA code stays valid, +// mirroring the user package's pending-code expiry. +const twoFAPendingCodeLifetime = 10 * time.Minute diff --git a/backend/handlers/payments/twofa_delivery_dev.go b/backend/handlers/payments/twofa_delivery_dev.go index 2267b44..b23f2db 100644 --- a/backend/handlers/payments/twofa_delivery_dev.go +++ b/backend/handlers/payments/twofa_delivery_dev.go @@ -9,3 +9,12 @@ package payments // handlers/user/twofa_dev.go; production builds decide in // twofa_delivery_prod.go. func twoFADeliveryAvailable() bool { return true } + +// twoFAReissueIssueAllowed is the re-issue path's issuance gate +// (reissueTwoFACodeAfterFailedCharge, handlers.go), mirroring the user +// package's twoFAEnsureIssueAllowed build-tagged semantics: dev/test builds +// always allow issuance — the [2FA] log line is the delivery channel and the +// unsalted-digest fallback is the documented loose-fake stand-in (matching +// twofa_dev.go). Production builds fail closed here — no pepper, no delivery +// channel, no codes (see twofa_delivery_prod.go). +func twoFAReissueIssueAllowed() error { return nil } diff --git a/backend/handlers/payments/twofa_delivery_prod.go b/backend/handlers/payments/twofa_delivery_prod.go index e3b174b..5548a07 100644 --- a/backend/handlers/payments/twofa_delivery_prod.go +++ b/backend/handlers/payments/twofa_delivery_prod.go @@ -2,7 +2,10 @@ package payments -import "os" +import ( + "errors" + "os" +) // twoFADeliveryAvailable reports whether a 2FA code delivery channel exists in // this build. Production has no wired email/SMS transport (P6), so the ONLY @@ -15,3 +18,27 @@ import "os" func twoFADeliveryAvailable() bool { return os.Getenv("TWO_FACTOR_ALLOW_LOG_DELIVERY") == "true" } + +// errTwoFAPepperRequired is returned by twoFAReissueIssueAllowed when +// TWO_FACTOR_PEPPER is unset in a production build — the re-issue would +// otherwise persist an offline-brute-forceable unsalted SHA-256 digest in the +// 1M code space (mirrors handlers/user's errTwoFAPepperRequired). +var errTwoFAPepperRequired = errors.New("TWO_FACTOR_PEPPER is not set; refusing to issue a 2FA code (an unsalted digest would be offline-brute-forceable)") + +// twoFAReissueIssueAllowed is the re-issue path's issuance gate +// (reissueTwoFACodeAfterFailedCharge, handlers.go), mirroring the user +// package's twoFAEnsureIssueAllowed (handlers/user/twofa_prod.go) build-tagged +// semantics: production requires BOTH a delivery channel and TWO_FACTOR_PEPPER. +// Without a channel the code could never reach the customer, and without the +// pepper every stored code would be an offline-brute-forceable unsalted digest +// — either way the re-issue refuses (fail-closed), exactly like the interactive +// mint paths. Dev/test builds always allow issuance (twofa_delivery_dev.go). +func twoFAReissueIssueAllowed() error { + if os.Getenv("TWO_FACTOR_PEPPER") == "" { + return errTwoFAPepperRequired + } + if !twoFADeliveryAvailable() { + return errors.New("2FA requires an email or SMS delivery channel; contact the salon") + } + return nil +} diff --git a/backend/handlers/payments/twofa_delivery_prod_test.go b/backend/handlers/payments/twofa_delivery_prod_test.go index 2404c3f..73b5257 100644 --- a/backend/handlers/payments/twofa_delivery_prod_test.go +++ b/backend/handlers/payments/twofa_delivery_prod_test.go @@ -57,3 +57,28 @@ func TestTwoFADeliveryAvailable_ProdPredicate(t *testing.T) { } }) } + +// TestTwoFAReissueIssueAllowed_ProdPredicate pins the finding 2 re-issue +// issuance gate in a genuine production build (no dev/test tags): it fails +// closed without TWO_FACTOR_PEPPER (an unsalted digest would be +// offline-brute-forceable) or without a delivery channel, and allows issuance +// only when both are configured. In a dev/test build the marker is false and +// the test skips, because the always-allowed dev variant is compiled +// (twofa_delivery_dev.go) — same documented limitation as the delivery +// predicate above. +func TestTwoFAReissueIssueAllowed_ProdPredicate(t *testing.T) { + if !twofaDeliveryProdVariant { + t.Skip("twoFAReissueIssueAllowed() is the dev/test build's always-allowed variant (twofa_delivery_dev.go, `dev || test`); the prod fail-closed branches are unreachable under the test tag — see the file header for the documented limitation") + } + + os.Unsetenv("TWO_FACTOR_PEPPER") + os.Unsetenv("TWO_FACTOR_ALLOW_LOG_DELIVERY") + require.Error(t, twoFAReissueIssueAllowed(), "a production re-issue without the pepper must fail closed") + + os.Setenv("TWO_FACTOR_PEPPER", "test-pepper") + os.Unsetenv("TWO_FACTOR_ALLOW_LOG_DELIVERY") + require.Error(t, twoFAReissueIssueAllowed(), "a production re-issue without a delivery channel must fail closed") + + os.Setenv("TWO_FACTOR_ALLOW_LOG_DELIVERY", "true") + require.NoError(t, twoFAReissueIssueAllowed(), "a production re-issue with both the pepper and a delivery channel is allowed") +} diff --git a/backend/handlers/payments/twofa_gate_consume_test.go b/backend/handlers/payments/twofa_gate_consume_test.go index 416b511..d7f0f8a 100644 --- a/backend/handlers/payments/twofa_gate_consume_test.go +++ b/backend/handlers/payments/twofa_gate_consume_test.go @@ -34,7 +34,9 @@ import ( "database/sql" "net/http" "testing" + "time" + "crussell/db" "crussell/internal/square" "crussell/internal/twofa" "crussell/testutils" @@ -230,3 +232,36 @@ func TestTwoFactorEnforced_BuyGiftCard_SavedCard_PendingReuse_Failure_KeepsCode( 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)") } + +// TestReissueTwoFACodeAfterFailedCharge_RespectsMintCooldown pins the +// LOW-MEDIUM finding 2 contract on the re-issue path: the re-issue mints a +// live code after a FRESH charge consumed one at the gate, respects the same +// per-user mint cooldown as the interactive mint endpoints (a second re-issue +// inside the window is a no-op), and runs again once the cooldown elapses +// (simulated by clearing the shared stamp the way a successful verify does). +func TestReissueTwoFACodeAfterFailedCharge_RespectsMintCooldown(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) + + reissueTwoFACodeAfterFailedCharge(ctx, db.Conn, userID, true, true, nil) + 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 code for the same-key retry") + firstHash := hash.String + + reissueTwoFACodeAfterFailedCharge(ctx, db.Conn, userID, true, true, nil) + require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_pending_code_hash FROM users WHERE id = $1", userID).Scan(&hash)) + require.Equal(t, firstHash, hash.String, "a re-issue inside the mint cooldown must be a no-op (the stored code is untouched)") + + st := twofa.StateFor(userID) + st.Mu.Lock() + st.LastMintAt = time.Time{} + st.Mu.Unlock() + reissueTwoFACodeAfterFailedCharge(ctx, db.Conn, userID, true, true, nil) + require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_pending_code_hash FROM users WHERE id = $1", userID).Scan(&hash)) + require.NotEqual(t, firstHash, hash.String, "an out-of-window re-issue must mint a fresh code") +} diff --git a/backend/handlers/payments/vat.go b/backend/handlers/payments/vat.go index 6a0e1e2..95553d5 100644 --- a/backend/handlers/payments/vat.go +++ b/backend/handlers/payments/vat.go @@ -114,3 +114,28 @@ func ApplyVATToTillSale(ctx context.Context, q db.Querier, saleID string) { log.Printf("Failed to apply VAT to till sale %s: %v", saleID, execErr) } } + +// applyVATToChargeRecord reads VAT config and applies VAT to a gift-card +// (voucher) charge record — a payments row (online purchase) or a till_sale +// row (admin till purchase) — under the SPV voucher rule (vatAppliesToVoucher). +// Shared by BuyGiftCard and CreateTillSale so the voucher VAT decision cannot +// drift between the online and till purchase surfaces. Errors are logged — +// VAT failure should not block the purchase/sale flow. isTillSale selects the +// target function (apply_vat_to_till_sale vs apply_vat_to_payment). +func applyVATToChargeRecord(ctx context.Context, q db.Querier, rowID string, isTillSale bool) { + vatCfg, err := GetVATConfig(ctx, q) + if err != nil { + log.Printf("Failed to read VAT config for gift-card charge %s: %v", rowID, err) + return + } + if !vatAppliesToVoucher(vatCfg) { + return + } + fn := "apply_vat_to_payment" + if isTillSale { + fn = "apply_vat_to_till_sale" + } + if _, execErr := q.Exec(ctx, "SELECT "+fn+"($1, $2)", rowID, vatCfg.DefaultVATRate); execErr != nil { + log.Printf("Failed to apply VAT to gift-card charge %s: %v", rowID, execErr) + } +} diff --git a/backend/handlers/scheduling/scheduled-cleanup.go b/backend/handlers/scheduling/scheduled-cleanup.go index a91d19b..e034ad2 100644 --- a/backend/handlers/scheduling/scheduled-cleanup.go +++ b/backend/handlers/scheduling/scheduled-cleanup.go @@ -2,6 +2,7 @@ package scheduling import ( "context" + "database/sql" "encoding/json" "errors" "fmt" @@ -242,6 +243,37 @@ func CleanupExpiredRefreshTokens(ctx context.Context) (int, error) { } }() + // LOW 6 / finding 3: an expired refresh token whose family has no other + // live members is a family kill. Invalidate the family-alive cache + // (auth/jwt.go) for the affected families AFTER the commit so bound access + // tokens re-check the DB instead of riding the 30s cache TTL. The family + // ids are captured before the delete and invalidated after the commit: a + // pre-commit invalidation could race a concurrent VerifyToken that + // re-caches the still-present row as alive. + rows, err := tx.Query(ctx, ` + SELECT DISTINCT family_id FROM refresh_tokens + WHERE expires_at < NOW() + OR (revoked = TRUE AND created_at < NOW() - make_interval(days => $1)) + `, int64(auth.RefreshTokenLifetime/(24*time.Hour))) + if err != nil { + return 0, fmt.Errorf("failed to select expired refresh token families: %w", err) + } + var familyIDs []string + for rows.Next() { + var familyID sql.NullString + if err := rows.Scan(&familyID); err != nil { + rows.Close() + return 0, fmt.Errorf("failed to scan expired refresh token family: %w", err) + } + if familyID.Valid && familyID.String != "" { + familyIDs = append(familyIDs, familyID.String) + } + } + rows.Close() + if err := rows.Err(); err != nil { + return 0, fmt.Errorf("failed to iterate expired refresh token families: %w", err) + } + result, err := tx.Exec(ctx, ` DELETE FROM refresh_tokens WHERE expires_at < NOW() @@ -255,6 +287,8 @@ func CleanupExpiredRefreshTokens(ctx context.Context) (int, error) { return 0, fmt.Errorf("failed to commit: %w", err) } + auth.InvalidateFamilyAliveBatch(familyIDs) + return int(result.RowsAffected()), nil } diff --git a/backend/handlers/user/gdpr_test.go b/backend/handlers/user/gdpr_test.go index dded57a..49cb1c6 100644 --- a/backend/handlers/user/gdpr_test.go +++ b/backend/handlers/user/gdpr_test.go @@ -503,11 +503,11 @@ func TestAnonymizeUser_RetainsEditRequestNotes(t *testing.T) { // TestAnonymizeUser_ScrubsAdminAuditLog2FAFallback closes the GDPR erasure gap // for admin_audit_log: a '2fa_fallback_charge' row (insertTwoFAFallbackAudit, -// handlers/payments) carries target_user_id = the erased user PLUS -// details.card_last4 — the audit row MUST survive erasure (GDPR Art 30 records -// of processing / financial audit trail) but de-identified: the user link is -// NULLed exactly as delete_guest_user() does (which scrubs target_user_id only, -// leaving details untouched — anonymize_user mirrors that consistency). +// handlers/payments) carries target_user_id = the erased user, admin_id = the +// customer's own userID (the CIT actor), AND details.card_last4 — the audit row +// MUST survive erasure (GDPR Art 30 records of processing / financial audit +// trail) but be de-identified: the user links (both target_user_id and +// admin_id) are NULLed and the card PII in details is scrubbed. func TestAnonymizeUser_ScrubsAdminAuditLog2FAFallback(t *testing.T) { t.Parallel() ctx, tx := testutils.SetupTestTx(t) @@ -536,27 +536,37 @@ func TestAnonymizeUser_ScrubsAdminAuditLog2FAFallback(t *testing.T) { } // The audit row survives erasure (audit retention) but is de-identified: - // target_user_id is NULLed. details is left untouched, mirroring - // delete_guest_user() exactly (it scrubs target_user_id only). - var targetUserID interface{} + // both the target_user_id and the admin_id (the CIT actor was the erased + // customer) are NULLed, and the card_last4 PII is scrubbed from details. + var targetUserID, adminID interface{} var details json.RawMessage err = tx.QueryRow(ctx, ` - SELECT target_user_id, details FROM admin_audit_log WHERE id = $1 - `, auditID).Scan(&targetUserID, &details) + SELECT target_user_id, admin_id, details FROM admin_audit_log WHERE id = $1 + `, auditID).Scan(&targetUserID, &adminID, &details) if err != nil { t.Fatalf("failed to query audit row after anonymization: %v", err) } if targetUserID != nil { t.Errorf("expected target_user_id to be NULL after anonymization, got %v", targetUserID) } + if adminID != nil { + t.Errorf("expected admin_id to be NULL after anonymization (the CIT actor is the erased user), got %v", adminID) + } if len(details) == 0 { t.Error("expected the audit row to survive erasure (retained, de-identified)") } + var detailsMap map[string]any + if err := json.Unmarshal(details, &detailsMap); err != nil { + t.Fatalf("failed to parse retained audit details: %v", err) + } + if last4, ok := detailsMap["card_last4"]; ok && last4 != nil { + t.Errorf("expected details.card_last4 to be scrubbed after anonymization, got %v", last4) + } // No residual audit rows may still reference the erased user. var remaining int err = tx.QueryRow(ctx, ` - SELECT COUNT(*) FROM admin_audit_log WHERE target_user_id = $1 + SELECT COUNT(*) FROM admin_audit_log WHERE target_user_id = $1 OR admin_id = $1 `, userID).Scan(&remaining) if err != nil { t.Fatalf("failed to count residual audit rows: %v", err) diff --git a/backend/handlers/user/twofa.go b/backend/handlers/user/twofa.go index 63c3843..433ed37 100644 --- a/backend/handlers/user/twofa.go +++ b/backend/handlers/user/twofa.go @@ -8,7 +8,6 @@ import ( "errors" "fmt" "log" - "log/slog" "math/big" "net/http" "time" @@ -605,44 +604,6 @@ func SendVerificationCodeHandler(w http.ResponseWriter, r *http.Request) { } } -// insertAdminAudit records an admin action in admin_audit_log. Mirrors the -// insertAdminAuditCharge pattern (handlers/payments/handlers.go) — same table, -// same columns, same best-effort non-fatal failure handling. The insert runs in -// its OWN transaction (a savepoint in the test harness) so an audit-write -// failure — e.g. a synthetic admin id in tests violating the admin_id FK — -// rolls back only the audit write and can never abort the caller's transaction. -func insertAdminAudit(ctx context.Context, adminID, targetUserID, action string, details map[string]any) { - detailsJSON, err := json.Marshal(details) - if err != nil { - log.Printf("Failed to marshal admin_audit_log details (non-critical): %v", err) - return - } - var target any - if targetUserID != "" { - target = targetUserID - } - auditTx, err := db.Conn.Begin(ctx) - if err != nil { - log.Printf("Failed to record admin_audit_log (non-critical): %v", err) - return - } - defer func() { - if err := auditTx.Rollback(ctx); err != nil && !errors.Is(err, pgx.ErrTxClosed) { - slog.Error("failed to rollback admin audit transaction", "err", err) - } - }() - if _, err := auditTx.Exec(ctx, ` - INSERT INTO admin_audit_log (admin_id, action_type, target_user_id, details) - VALUES ($1, $2, $3, $4::jsonb) - `, adminID, action, target, string(detailsJSON)); err != nil { - log.Printf("Failed to record admin_audit_log (non-critical): %v", err) - return - } - if err := auditTx.Commit(ctx); err != nil { - log.Printf("Failed to record admin_audit_log (non-critical): %v", err) - } -} - // AdminSendVerificationCodeHandler mints (or reuses) a 2FA code for a TARGET // user, not the session user. The saved-card charge gate verifies the code // against the CARD OWNER (customer) — never the admin session (till.go:951, @@ -703,8 +664,8 @@ func AdminSendVerificationCodeHandler(w http.ResponseWriter, r *http.Request) { // `code` return discriminates reuse from a fresh mint — the plaintext is // only returned for a fresh delivery, never on reuse (EnsurePendingTwoFACode). if adminID, ok := mw.GetUserID(r.Context()); ok { - insertAdminAudit(r.Context(), adminID, targetUserID, "2fa_code_mint", map[string]any{ - "reused": code == "", + payments.InsertAdminAuditCharge(r.Context(), adminID, targetUserID, "2fa_code_mint", map[string]any{ + "reused": code == "", "remaining_seconds": int(remaining.Seconds()), }) } diff --git a/backend/internal/twofa/twofa.go b/backend/internal/twofa/twofa.go index 94fb638..5b841ed 100644 --- a/backend/internal/twofa/twofa.go +++ b/backend/internal/twofa/twofa.go @@ -417,16 +417,17 @@ func Check(ctx context.Context, userID string, st *AttemptState, reqCode string, return OK, nil } -// ConsumePendingCode NULLs the user's pending 2FA code digest and expiry. The -// payments saved-card charge gate verifies WITHOUT consuming (MEDIUM-2) and the -// handlers call this when the charge reaches a TERMINAL SUCCESS state — inside -// the transaction that records the completed charge when one exists — so the -// code is consumed atomically with the charge OUTCOME, not the gate. A failed -// or ambiguous Square charge leaves the code intact and the same-key retry can -// re-verify the SAME code. Idempotent: consuming an already-NULL pending code -// is a no-op, so a code still authorizes exactly one completed charge and can -// never authorize a second after success. Accepts a db.Querier so the write can -// ride the caller's transaction (pgx.Tx) or the pool proxy. +// ConsumePendingCode NULLs the user's pending 2FA code digest and expiry. +// Since finding 1 the saved-card CHARGE gates consume a FRESH charge's code at +// verify time (consume=true — single-use), so this is no longer the gate's +// consumption path: it is used by the PENDING-REUSE retry path, whose gate +// verified WITHOUT consuming (consume=false) so a retry that fails again keeps +// its code for one more attempt — the handlers call this when the retry reaches +// a TERMINAL SUCCESS state, inside the transaction that records the completed +// charge. Idempotent: consuming an already-NULL pending code is a no-op, so a +// code still authorizes exactly one completed charge and can never authorize a +// second after success. Accepts a db.Querier so the write can ride the caller's +// transaction (pgx.Tx) or the pool proxy. func ConsumePendingCode(ctx context.Context, q db.Querier, userID string) error { if userID == "" { return nil diff --git a/frontend/src/lib/components/admin/TillPurchases.svelte b/frontend/src/lib/components/admin/TillPurchases.svelte index d8547cf..abd5e4b 100644 --- a/frontend/src/lib/components/admin/TillPurchases.svelte +++ b/frontend/src/lib/components/admin/TillPurchases.svelte @@ -19,6 +19,7 @@ submitPaymentWithRetry, adminRequestNewTwoFactorCode, requestNewTwoFactorCode, + PAYMENT_METHOD_SAVED_CARD, VERIFICATION_REQUIRED_MESSAGE } from '$lib/square/square'; import { @@ -35,13 +36,13 @@ qty: number; }; - type TillPaymentMethod = 'cash' | 'card_machine' | 'online_square' | 'saved_card'; + type TillPaymentMethod = 'cash' | 'card_machine' | 'online_square' | (typeof PAYMENT_METHOD_SAVED_CARD); const PAYMENT_METHODS: Array<{ key: TillPaymentMethod; label: string }> = [ { key: 'cash', label: 'Cash' }, { key: 'card_machine', label: 'Card Machine' }, { key: 'online_square', label: 'Online Card' }, - { key: 'saved_card', label: 'Saved Card' } + { key: PAYMENT_METHOD_SAVED_CARD, label: 'Saved Card' } ]; let cart = $state([]); @@ -73,7 +74,7 @@ function idempotencyKeyFor(item: CartItem, qtyIndex: number): string { // saved_card charges also key on the selected card id so switching to a // different card (or back to another method) yields fresh keys. - const composite = `${item.id}:${qtyIndex}:${item.price}:${paymentMethod}:${paymentMethod === 'saved_card' ? (selectedSavedCardId ?? '') : ''}`; + const composite = `${item.id}:${qtyIndex}:${item.price}:${paymentMethod}:${paymentMethod === PAYMENT_METHOD_SAVED_CARD ? (selectedSavedCardId ?? '') : ''}`; let key = idempotencyKeys.get(composite); if (!key) { key = generateUUID(); @@ -152,7 +153,7 @@ const twoFactor = useTwoFactorCodeForSavedCard({ enabled: () => true, gateActive: () => - twoFactorEnforced && customerTwoFactorEnabled && paymentMethod === 'saved_card', + twoFactorEnforced && customerTwoFactorEnabled && paymentMethod === PAYMENT_METHOD_SAVED_CARD, scaAvailable: () => !shouldFallbackTo2FA(lastSCAOutcome), mint: () => selectedCustomer?.id @@ -165,14 +166,14 @@ const showSavedCardOption = $derived(selectedCustomer !== null && validCards.length > 0); const availablePaymentMethods = $derived( - PAYMENT_METHODS.filter((m) => m.key !== 'saved_card' || showSavedCardOption) + PAYMENT_METHODS.filter((m) => m.key !== PAYMENT_METHOD_SAVED_CARD || showSavedCardOption) ); // If the saved-card option disappears (customer cleared, no valid cards, or // a card expires mid-session) fall back to cash instead of leaving the till // on an unrenderable method. $effect(() => { - if (paymentMethod === 'saved_card' && !showSavedCardOption) { + if (paymentMethod === PAYMENT_METHOD_SAVED_CARD && !showSavedCardOption) { paymentMethod = 'cash'; selectedSavedCardId = null; } @@ -353,7 +354,7 @@ ); return; } - if (paymentMethod === 'saved_card' && (!selectedCustomer || !selectedSavedCardId)) { + if (paymentMethod === PAYMENT_METHOD_SAVED_CARD && (!selectedCustomer || !selectedSavedCardId)) { toast.error('Select a customer and a saved card before charging'); return; } @@ -374,7 +375,7 @@ payment_method: paymentMethod, idempotency_key: idempotencyKeyFor(item, i) }; - if (paymentMethod === 'saved_card') { + if (paymentMethod === PAYMENT_METHOD_SAVED_CARD) { body.user_id = selectedCustomer?.id; body.user_saved_card_id = selectedSavedCardId; // B6/B10: the backend requires the CARD OWNER's current 2FA @@ -398,12 +399,19 @@ } for (const body of saleBodies) { - const res = await submitPaymentWithRetry(() => - apiFetch('/api/admin/till/sale', { - method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify(body) - }) + const res = await submitPaymentWithRetry( + () => + apiFetch('/api/admin/till/sale', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify(body) + }), + // Finding 4: a saved-card till line gated on 2FA consumed its + // code at the backend gate — a 503 auto-retry would re-send a + // dead code and self-defeat. + { + verificationCodeGated: paymentMethod === PAYMENT_METHOD_SAVED_CARD && twoFactor.showInput + } ); if (!res.ok) { responseStatus = res.status; @@ -415,7 +423,7 @@ // its SAME cached idempotency key. runTillSavedCardSCA throws to // stop the whole sale on any non-verified outcome. if ( - paymentMethod === 'saved_card' && + paymentMethod === PAYMENT_METHOD_SAVED_CARD && isVerificationRequiredSignal(responseStatus, errText) ) { await runTillSavedCardSCA(body); @@ -830,7 +838,7 @@ {/if} - {#if paymentMethod === 'saved_card'} + {#if paymentMethod === PAYMENT_METHOD_SAVED_CARD}
{#if loadingSavedCards}
@@ -955,7 +963,7 @@ processing || twoFactor.missing || (paymentMethod === 'online_square' && !onlineSquareCardReady) || - (paymentMethod === 'saved_card' && !selectedSavedCardId)} + (paymentMethod === PAYMENT_METHOD_SAVED_CARD && !selectedSavedCardId)} > {processing ? 'Processing...' : `Charge ${formatCurrency(subtotal)}`} diff --git a/frontend/src/lib/components/booking/BookingFlow.svelte b/frontend/src/lib/components/booking/BookingFlow.svelte index 74b647a..9300652 100644 --- a/frontend/src/lib/components/booking/BookingFlow.svelte +++ b/frontend/src/lib/components/booking/BookingFlow.svelte @@ -527,18 +527,23 @@ if (!confirmedBooking) return; const bookingId = confirmedBooking.id; - const response = await submitPaymentWithRetry(() => - apiFetch(`/api/bookings/${bookingId}/payment`, { - method: 'POST', - headers: { - 'Content-Type': 'application/json', - ...getAuthHeaders() - }, - body: JSON.stringify({ - ...body, - ...(confirmOverflowTip ? { confirm_overflow_tip: true } : {}) - }) - }) + const response = await submitPaymentWithRetry( + () => + apiFetch(`/api/bookings/${bookingId}/payment`, { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + ...getAuthHeaders() + }, + body: JSON.stringify({ + ...body, + ...(confirmOverflowTip ? { confirm_overflow_tip: true } : {}) + }) + }), + // Finding 4: a 2FA-gated charge consumed its code at the backend gate + // — a 503 auto-retry would re-send a dead code and self-defeat. The + // code is the gate only when no SCA verification token is present. + { verificationCodeGated: twoFactor.showInput && !('verification_token' in body) } ); if (response.ok) { diff --git a/frontend/src/lib/components/payments/PaymentModal.svelte b/frontend/src/lib/components/payments/PaymentModal.svelte index 6a4eb62..fa33ba2 100644 --- a/frontend/src/lib/components/payments/PaymentModal.svelte +++ b/frontend/src/lib/components/payments/PaymentModal.svelte @@ -13,6 +13,7 @@ campaignDiscountPence, isTwoFactorVerificationGateFailure, isVerificationRequiredSignal, + PAYMENT_METHOD_SAVED_CARD, sanitizeDecimalInput, shouldFallbackTo2FA, SCA_UNAVAILABLE_2FA_FALLBACK_MESSAGE, @@ -63,7 +64,7 @@ amount: number; }; - type PaymentMethod = 'card' | 'cash' | 'giftcard' | 'savedcard' | null; + type PaymentMethod = 'card' | 'cash' | 'giftcard' | (typeof PAYMENT_METHOD_SAVED_CARD) | null; let status = $state('idle'); let selectedMethod = $state(null); @@ -103,7 +104,7 @@ const twoFactor = useTwoFactorCodeForSavedCard({ enabled: () => true, gateActive: () => - twoFactorEnforced && customerTwoFactorEnabled && selectedMethod === 'savedcard', + twoFactorEnforced && customerTwoFactorEnabled && selectedMethod === PAYMENT_METHOD_SAVED_CARD, scaAvailable: () => !shouldFallbackTo2FA(lastSCAOutcome), mint: () => { const customerID = booking.user_id ?? booking.user?.id; @@ -839,19 +840,23 @@ try { await applyLoyaltyRedemption(); - const response = await submitPaymentWithRetry(() => - apiFetch(`/api/admin/bookings/${booking.id}/payment`, { - method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ - amount: chargeAmount, - payment_type: 'full', - payment_method: 'saved_card', - saved_card_id: selectedSavedCardId, - ...(twoFactor.showInput ? { verification_code: twoFactor.code } : {}), - idempotency_key: savedCardIdempotencyKey - }) - }) + const response = await submitPaymentWithRetry( + () => + apiFetch(`/api/admin/bookings/${booking.id}/payment`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ + amount: chargeAmount, + payment_type: 'full', + payment_method: 'saved_card', + saved_card_id: selectedSavedCardId, + ...(twoFactor.showInput ? { verification_code: twoFactor.code } : {}), + idempotency_key: savedCardIdempotencyKey + }) + }), + // Finding 4: a 2FA-gated charge consumed its code at the backend + // gate — a 503 auto-retry would re-send a dead code and self-defeat. + { verificationCodeGated: twoFactor.showInput } ); if (!response.ok) { @@ -908,6 +913,18 @@ // code, brute-force lockout) is recoverable — keep the code populated // and reveal the input so the charge can be retried with a fresh code. if (isTwoFactorVerificationGateFailure(responseStatus, msg)) twoFactor.reveal = true; + // A DEFINITIVE 402 (declined card / stale token) means the charge did + // NOT land — Square's idempotency key would otherwise reject a retry + // that re-runs SCA and mints a fresh token. Regenerate the key on 402 + // so the next Pay click gets a fresh key + fresh pending row. Keep it + // on 503/network (ambiguous) and on the verification-required signal + // (that path runs the SCA challenge and returns before this catch). + if (responseStatus === 402) { + savedCardIdempotencyKey = ''; + savedCardKeyedBookingId = ''; + savedCardKeyedCardId = ''; + savedCardKeyedAmount = 0; + } error = msg; toast.error(msg); } finally { @@ -923,7 +940,7 @@ if (selectedMethod === 'giftcard') { giftCardId = ''; } - if (selectedMethod === 'savedcard') { + if (selectedMethod === PAYMENT_METHOD_SAVED_CARD) { fetchSavedCards(); } }); @@ -1271,14 +1288,14 @@