diff --git a/backend/handlers/auth/auth_test.go b/backend/handlers/auth/auth_test.go
index 89db81c..74443db 100644
--- a/backend/handlers/auth/auth_test.go
+++ b/backend/handlers/auth/auth_test.go
@@ -1665,6 +1665,133 @@ func TestLoginInProgress_Cap(t *testing.T) {
}
}
+// TestLoginInProgress_SameUserReentry_429 pins finding 5: a second login for
+// the same account while one is mid-flight is rejected 429 (previously 409 — a
+// Conflict response leaks that a login is in progress for this account and is
+// semantically wrong for "try again in a moment").
+func TestLoginInProgress_SameUserReentry_429(t *testing.T) {
+ ctx, tx := resetTestData(t)
+
+ handler := http.HandlerFunc(LoginHandler)
+
+ userID, err := fixtures.CreateTestUserWithEmail(tx, "inprogress@test.com", "verified_email")
+ if err != nil {
+ t.Fatalf("failed to create test user: %v", err)
+ }
+ defer fixtures.DeleteUser(tx, userID)
+
+ // Simulate a login already in flight for this account.
+ loginStateMu.Lock()
+ loginInProgress[userID] = clock.Now()
+ loginStateMu.Unlock()
+ defer func() {
+ loginStateMu.Lock()
+ delete(loginInProgress, userID)
+ loginStateMu.Unlock()
+ }()
+
+ body := LoginRequest{
+ Email: "inprogress@test.com",
+ Password: "testpassword123",
+ }
+ w := testutils.MakeRequestNoAuth(handler, "POST", "/api/login", body, ctx)
+ if w.Code != http.StatusTooManyRequests {
+ t.Errorf("expected 429 for a re-entry into an in-progress login, got %d. body: %s", w.Code, w.Body.String())
+ }
+}
+
+// TestLoginInProgress_StaleEntriesEvictedBeforeCap pins finding 5b: stale
+// loginInProgress entries are evicted BEFORE the map cap is consulted, so a
+// single attacker holding many fake (stale) entries can no longer trip the
+// global "server busy" 429 for legitimate users — only genuinely concurrent
+// in-flight logins occupy the budget.
+func TestLoginInProgress_StaleEntriesEvictedBeforeCap(t *testing.T) {
+ ctx, tx := resetTestData(t)
+
+ handler := http.HandlerFunc(LoginHandler)
+
+ userID, err := fixtures.CreateTestUserWithEmail(tx, "evict-before-cap@test.com", "verified_email")
+ if err != nil {
+ t.Fatalf("failed to create test user: %v", err)
+ }
+ defer fixtures.DeleteUser(tx, userID)
+
+ // Fill the map to the cap with STALE entries (older than the 30s window).
+ loginStateMu.Lock()
+ for i := 0; i < maxLoginInProgress; i++ {
+ loginInProgress[fmt.Sprintf("stale-user-%d", i)] = clock.Now().Add(-loginInProgressWindow - time.Second)
+ }
+ loginStateMu.Unlock()
+ defer func() {
+ loginStateMu.Lock()
+ for i := 0; i < maxLoginInProgress; i++ {
+ delete(loginInProgress, fmt.Sprintf("stale-user-%d", i))
+ }
+ loginStateMu.Unlock()
+ }()
+
+ body := LoginRequest{
+ Email: "evict-before-cap@test.com",
+ Password: "testpassword123",
+ }
+ w := testutils.MakeRequestNoAuth(handler, "POST", "/api/login", body, ctx)
+ if w.Code == http.StatusTooManyRequests {
+ t.Error("stale entries must be evicted before the cap check — a legitimate login must not get the global 429")
+ }
+}
+
+// TestVerifyCheck_AttemptBudget_LocksOutAfterFive verifies finding 8: POST
+// /verify/check now bounds guesses per submitted code — the 6th failed attempt
+// for a code is rejected 429, mirroring the 2FA attempt pattern — and a
+// successful verify clears the budget.
+func TestVerifyCheck_AttemptBudget_LocksOutAfterFive(t *testing.T) {
+ ctx, tx := resetTestData(t)
+
+ handler := http.HandlerFunc(VerifyCodeHandler)
+
+ // Create a user + a real code so the success path is exercised.
+ userID, err := fixtures.CreateTestUser(tx)
+ if err != nil {
+ t.Fatalf("failed to create test user: %v", err)
+ }
+ defer fixtures.DeleteUser(tx, userID)
+ var realCode string
+ expiresAt := clock.Now().Add(24 * time.Hour)
+ if err := tx.QueryRow(ctx,
+ `INSERT INTO verification_codes (user_id, purpose, expires_at) VALUES ($1, 'email_verify', $2) RETURNING code`,
+ userID, expiresAt).Scan(&realCode); err != nil {
+ t.Fatalf("failed to create verification code: %v", err)
+ }
+ defer tx.Exec(ctx, "DELETE FROM verification_codes WHERE user_id = $1", userID)
+
+ // 4 wrong guesses for a code that does not exist → 400 each (the 2FA
+ // pattern: the 5th failure is the lockout).
+ guess := "000000000000"
+ for i := 0; i < 4; i++ {
+ w := testutils.MakeRequestNoAuth(handler, "POST", "/api/verify/check", VerifyCodeRequest{Code: guess}, ctx)
+ if w.Code != http.StatusBadRequest {
+ t.Fatalf("wrong guess %d: expected 400, got %d. body: %s", i+1, w.Code, w.Body.String())
+ }
+ }
+ // The 5th failed attempt exhausts the budget → 429.
+ w := testutils.MakeRequestNoAuth(handler, "POST", "/api/verify/check", VerifyCodeRequest{Code: guess}, ctx)
+ if w.Code != http.StatusTooManyRequests {
+ t.Errorf("expected 429 on the 5th failed attempt for the same code, got %d. body: %s", w.Code, w.Body.String())
+ }
+ // A further attempt is rejected before any DB work.
+ w = testutils.MakeRequestNoAuth(handler, "POST", "/api/verify/check", VerifyCodeRequest{Code: guess}, ctx)
+ if w.Code != http.StatusTooManyRequests {
+ t.Errorf("expected 429 for a spent budget, got %d. body: %s", w.Code, w.Body.String())
+ }
+
+ // A DIFFERENT code (the real one) is unaffected by the spent budget and
+ // verifies successfully — budgets are per code.
+ w = testutils.MakeRequestNoAuth(handler, "POST", "/api/verify/check", VerifyCodeRequest{Code: realCode}, ctx)
+ if w.Code != http.StatusOK {
+ t.Errorf("a valid code must still verify after another code's budget was spent, got %d. body: %s", w.Code, w.Body.String())
+ }
+}
+
// ValidateUKPhoneNumber Security Tests
//
// These tests verify that ValidateUKPhoneNumber rejects or sanitises
diff --git a/backend/handlers/auth/local.go b/backend/handlers/auth/local.go
index c215f61..3cc2c3b 100644
--- a/backend/handlers/auth/local.go
+++ b/backend/handlers/auth/local.go
@@ -32,28 +32,143 @@ import (
"golang.org/x/text/language"
)
+// maxLoginInProgress caps the loginInProgress map (Round 2 Loop A finding 5):
+// at most this many logins may be mid-flight at once before the next is
+// rejected 429. Stale entries are evicted before the cap is consulted (see
+// evictStaleLoginEntriesLocked), so a single attacker holding N fake entries
+// cannot permanently exhaust the global budget — only genuinely concurrent
+// logins occupy it, and each entry self-releases via the handler's deferred
+// delete.
const maxLoginInProgress = 20
+// loginInProgressWindow is how long a loginInProgress entry is considered
+// live before it is stale and evictable.
+const loginInProgressWindow = 30 * time.Second
+
+// maxConcurrentLoginBcrypt bounds how many login requests may run their bcrypt
+// comparison concurrently (Round 2 Loop A finding 4b). The progressive per-IP
+// middleware sleeps BEFORE this handler, so without the cap a flood of
+// throttled login requests could stack an unbounded number of goroutines that
+// all hit bcrypt the moment their sleeps elapse — a CPU-amplification vector.
+// Beyond the cap the login is rejected 429 immediately (nothing has been
+// processed, so nothing leaks).
+const maxConcurrentLoginBcrypt = 20
+
// Login state management
var (
loginStateMu sync.Mutex
loginInProgress = make(map[string]time.Time)
+ // loginBcryptSlots is the counting semaphore backing maxConcurrentLoginBcrypt.
+ loginBcryptSlots = make(chan struct{}, maxConcurrentLoginBcrypt)
)
-// CleanupStaleLoginEntries removes stuck loginInProgress entries older than 30 seconds.
-// Called by the centralised jobs scheduler.
-func CleanupStaleLoginEntries(ctx context.Context) (int, error) {
- loginStateMu.Lock()
- defer loginStateMu.Unlock()
- now := clock.Now()
+// acquireLoginBcryptSlot tries to reserve a concurrent bcrypt slot. ok=false
+// means the handler must respond 429.
+func acquireLoginBcryptSlot() (release func(), ok bool) {
+ select {
+ case loginBcryptSlots <- struct{}{}:
+ return func() { <-loginBcryptSlots }, true
+ default:
+ return nil, false
+ }
+}
+
+// evictStaleLoginEntriesLocked removes loginInProgress entries older than
+// loginInProgressWindow. Caller must hold loginStateMu.
+func evictStaleLoginEntriesLocked(now time.Time) {
for userID, startedAt := range loginInProgress {
- if now.Sub(startedAt) > 30*time.Second {
+ if now.Sub(startedAt) > loginInProgressWindow {
delete(loginInProgress, userID)
}
}
+}
+
+// CleanupStaleLoginEntries removes stuck loginInProgress entries older than
+// loginInProgressWindow. Called by the centralised jobs scheduler.
+func CleanupStaleLoginEntries(ctx context.Context) (int, error) {
+ loginStateMu.Lock()
+ defer loginStateMu.Unlock()
+ evictStaleLoginEntriesLocked(clock.Now())
return 0, nil
}
+// Email-verification attempt budget (Round 2 Loop A finding 8): POST
+// /verify/check had no per-user attempt counter, so a client holding a code
+// could fail it indefinitely and the endpoint doubled as an unbounded guessing
+// oracle. Mirror the 2FA attempt pattern: an in-memory map keys a 5-attempt
+// budget per submitted code. The code is the only identifier a wrong guess
+// carries, and every code is user-scoped (one code belongs to exactly one
+// user), so the budget is effectively per-user-per-code — a distinct user can
+// never drain another's budget for the same code. A successful verify clears
+// the entry; the 5th failed attempt exhausts the budget (429). The map is
+// bounded and stale entries are evicted, so a flood of random guesses cannot
+// grow it without bound.
+const (
+ emailVerifyMaxAttempts = 5
+ emailVerifyAttemptWindow = 30 * time.Minute
+ emailVerifyMaxTrackedCodes = 10_000
+)
+
+type emailVerifyAttempt struct {
+ count int
+ lastAt time.Time
+}
+
+var (
+ emailVerifyMu sync.Mutex
+ emailVerifyAttempts = make(map[string]emailVerifyAttempt)
+)
+
+// emailVerifyAttemptsExhausted reports whether the submitted code's attempt
+// budget is already spent, rejecting the request before any DB work.
+func emailVerifyAttemptsExhausted(code string) bool {
+ emailVerifyMu.Lock()
+ defer emailVerifyMu.Unlock()
+ evictStaleEmailVerifyAttemptsLocked()
+ a, ok := emailVerifyAttempts[code]
+ return ok && a.count >= emailVerifyMaxAttempts
+}
+
+// emailVerifyAttemptFailed registers one failed verification attempt for the
+// submitted code and reports whether the budget for that code is now exhausted
+// (the handler should respond 429).
+func emailVerifyAttemptFailed(code string) bool {
+ emailVerifyMu.Lock()
+ defer emailVerifyMu.Unlock()
+ evictStaleEmailVerifyAttemptsLocked()
+ now := clock.Now()
+ a := emailVerifyAttempts[code]
+ if now.Sub(a.lastAt) > emailVerifyAttemptWindow {
+ a.count = 0
+ }
+ a.count++
+ a.lastAt = now
+ emailVerifyAttempts[code] = a
+ return a.count >= emailVerifyMaxAttempts
+}
+
+// emailVerifyAttemptsClear drops the budget for a code after a successful
+// verify (the code is consumed; the entry would only leak stale state).
+func emailVerifyAttemptsClear(code string) {
+ emailVerifyMu.Lock()
+ delete(emailVerifyAttempts, code)
+ emailVerifyMu.Unlock()
+}
+
+// evictStaleEmailVerifyAttemptsLocked bounds the attempts map. Caller must
+// hold emailVerifyMu.
+func evictStaleEmailVerifyAttemptsLocked() {
+ if len(emailVerifyAttempts) < emailVerifyMaxTrackedCodes {
+ return
+ }
+ now := clock.Now()
+ for k, a := range emailVerifyAttempts {
+ if now.Sub(a.lastAt) > emailVerifyAttemptWindow {
+ delete(emailVerifyAttempts, k)
+ }
+ }
+}
+
type RegisterRequest struct {
FirstName string `json:"firstName" validate:"required,min=1,max=50"`
LastName string `json:"lastName" validate:"required,min=1,max=50"`
@@ -349,11 +464,19 @@ func LoginHandler(w http.ResponseWriter, r *http.Request) {
return
}
- // Check if user is already logging in
+ // Per-user in-flight slot (finding 5): the same account cannot have two
+ // concurrent login flows. A re-entry inside the slot window is rejected 429
+ // (not 409 — no conflict with a finished attempt, and a Conflict response
+ // would leak that a login for this account is mid-flight). Stale entries are
+ // evicted before the cap check so a single attacker holding N fake entries
+ // cannot exhaust the global budget: at most maxLoginInProgress genuinely
+ // concurrent logins occupy the map, each self-releasing via the deferred
+ // delete below.
loginStateMu.Lock()
- if t, ok := loginInProgress[userID]; ok && time.Since(t) < 30*time.Second {
+ evictStaleLoginEntriesLocked(clock.Now())
+ if t, ok := loginInProgress[userID]; ok && clock.Now().Sub(t) < loginInProgressWindow {
loginStateMu.Unlock()
- mw.RespondError(w, http.StatusConflict, "login already in progress")
+ mw.RespondError(w, http.StatusTooManyRequests, "login already in progress")
return
}
// Cap the map size - drop new request if at capacity
@@ -372,8 +495,19 @@ func LoginHandler(w http.ResponseWriter, r *http.Request) {
loginStateMu.Unlock()
}()
- // Verify password
- if err := bcrypt.CompareHashAndPassword([]byte(passwordHash), []byte(req.Password)); err != nil {
+ // Verify password under the concurrency cap (finding 4b). The slot is
+ // released immediately after the compare — bcrypt is the expensive,
+ // amplifier-prone part; the DB work below is cheap. The deferred delete
+ // above releases this user's in-flight slot on every path.
+ release, ok := acquireLoginBcryptSlot()
+ if !ok {
+ mw.RespondError(w, http.StatusTooManyRequests, "server busy, try again later")
+ return
+ }
+ passwordOK := bcrypt.CompareHashAndPassword([]byte(passwordHash), []byte(req.Password)) == nil
+ release()
+
+ if !passwordOK {
// Increment failed attempts in DB with progressive lockout
var newFailed int
var newLockedUntil *time.Time
@@ -701,6 +835,13 @@ func VerifyCodeHandler(w http.ResponseWriter, r *http.Request) {
return
}
+ // Finding 8: a spent attempt budget rejects before any DB work — the code
+ // can no longer be guessed against.
+ if emailVerifyAttemptsExhausted(code) {
+ http.Error(w, "too many attempts. request a new code.", http.StatusTooManyRequests)
+ return
+ }
+
var userID string
var purpose string
var expiresAt time.Time
@@ -718,16 +859,25 @@ func VerifyCodeHandler(w http.ResponseWriter, r *http.Request) {
`SELECT used_at FROM verification_codes WHERE code = $1`, code,
).Scan(&checkUsedAt)
if checkErr != nil {
- // Code doesn't exist at all
+ // Code doesn't exist at all — a guess. Count it against the
+ // code's attempt budget.
+ if emailVerifyAttemptFailed(code) {
+ http.Error(w, "too many attempts. request a new code.", http.StatusTooManyRequests)
+ return
+ }
http.Error(w, "invalid or expired code", http.StatusBadRequest)
return
}
- // Code exists but was already used
+ // Code exists but was already used — a definite state, not a guess.
if checkUsedAt != nil {
http.Error(w, "code already used", http.StatusForbidden)
return
}
- // Code exists but expired
+ // Code exists but expired — count it against the budget too.
+ if emailVerifyAttemptFailed(code) {
+ http.Error(w, "too many attempts. request a new code.", http.StatusTooManyRequests)
+ return
+ }
http.Error(w, "invalid or expired code", http.StatusBadRequest)
return
}
@@ -776,6 +926,9 @@ func VerifyCodeHandler(w http.ResponseWriter, r *http.Request) {
return
}
+ // Finding 8: a successful verify clears the code's attempt budget.
+ emailVerifyAttemptsClear(code)
+
if err := json.NewEncoder(w).Encode(VerificationResponse{Success: true, Message: "Email verified successfully"}); err != nil {
log.Printf("Failed to encode JSON response: %v", err)
}
diff --git a/backend/handlers/payments/errors.go b/backend/handlers/payments/errors.go
index 2e1f9eb..83c301a 100644
--- a/backend/handlers/payments/errors.go
+++ b/backend/handlers/payments/errors.go
@@ -9,25 +9,35 @@ 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
+// roundingEpsilon is the "effectively zero" guard for pound-denominated
+// payment splits (0.004 = 0.4 pence). Amounts at or below this threshold —
+// pure float64 rounding residue from dividing pence by 100 — are treated as
+// zero so a sub-penny slice never becomes a phantom payment row. Single
+// shared constant so the split builders and the cash/gift-card terminal
+// branches can never drift on the threshold.
+const roundingEpsilon = 0.004
+
+// SquareRefundStatusToLocal maps Square's refund status to the local refunds
+// status enum, returning a (localStatus, terminal) pair. 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 {
+// (developer.squareup.com/reference/square/objects/PaymentRefund). COMPLETED
+// and APPROVED are terminal-completed — APPROVED explicitly, because the
+// synchronous refund handlers resolve a returned APPROVED to 'completed' and
+// that behaviour must not be lost. FAILED/REJECTED are terminal-failed (Square
+// declined the refund and it must be surfaced as a definitive local failure).
+// Everything else (PENDING — money in flight, the sweep reconciles it later —
+// CANCELED, or any unknown status) is NON-terminal: the caller leaves the row
+// untouched rather than guessing. This is the single shared implementation for
+// both the payments refund handlers and the webhooks package, so the two can
+// never drift on the same Square status again.
+func SquareRefundStatusToLocal(status string) (string, bool) {
switch status {
- case "PENDING":
- return "pending"
+ case "COMPLETED", "APPROVED":
+ return "completed", true
case "FAILED", "REJECTED":
- return "failed"
+ return "failed", true
default:
- return "completed"
+ return "", false
}
}
diff --git a/backend/handlers/payments/giftcards.go b/backend/handlers/payments/giftcards.go
index 265c1d7..d37a18b 100644
--- a/backend/handlers/payments/giftcards.go
+++ b/backend/handlers/payments/giftcards.go
@@ -1207,7 +1207,11 @@ func GetGiftCardBalance(w http.ResponseWriter, r *http.Request) {
err := db.Conn.QueryRow(ctx, "SELECT balance FROM user_giftcard_balances WHERE user_id = $1", userID).Scan(&balance)
if err != nil {
if errors.Is(err, pgx.ErrNoRows) {
- if err := json.NewEncoder(w).Encode(map[string]float64{"balance": 0.00}); err != nil {
+ if err := json.NewEncoder(w).Encode(map[string]any{
+ "balance": 0.00,
+ "daily_buy_limit": float64(maxUserGiftCardDailyPence) / 100.0,
+ "daily_buy_spent": 0.00,
+ }); err != nil {
log.Printf("Failed to encode JSON response: %v", err)
}
return
@@ -1217,7 +1221,20 @@ func GetGiftCardBalance(w http.ResponseWriter, r *http.Request) {
return
}
- if err := json.NewEncoder(w).Encode(map[string]float64{"balance": balance}); err != nil {
+ // daily_buy_limit / daily_buy_spent expose the backend's authoritative
+ // £/day online purchase cap (maxUserGiftCardDailyPence) and today's spend,
+ // so the account page's client-side cap mirror (account/+page.svelte) can
+ // never drift from the server constant again.
+ spentToday, spentErr := userGiftCardSpentToday(ctx, db.Conn, userID)
+ if spentErr != nil {
+ log.Printf("Failed to query today's gift-card spend: %v", spentErr)
+ }
+
+ if err := json.NewEncoder(w).Encode(map[string]any{
+ "balance": balance,
+ "daily_buy_limit": float64(maxUserGiftCardDailyPence) / 100.0,
+ "daily_buy_spent": spentToday,
+ }); err != nil {
log.Printf("Failed to encode JSON response: %v", err)
}
}
@@ -1252,29 +1269,10 @@ func GetUserGiftCardBalanceAdmin(w http.ResponseWriter, r *http.Request) {
return
}
- detailsJSON := fmt.Sprintf(`{"balance": %.2f}`, balance)
-
- tx, err := db.Conn.Begin(ctx)
- if err != nil {
- log.Printf("Failed to begin transaction: %v", err)
- } else {
- defer func() {
- if err := tx.Rollback(ctx); err != nil && !errors.Is(err, pgx.ErrTxClosed) {
- slog.Error("failed to rollback transaction", "err", err)
- }
- }()
-
- if _, err := tx.Exec(ctx, `
- INSERT INTO admin_audit_log (admin_id, action_type, target_user_id, details)
- VALUES ($1, 'balance_check', $2, $3::jsonb)
- `, adminID, userID, detailsJSON); err != nil {
- log.Printf("Failed to record admin_audit_log (non-critical): %v", err)
- }
-
- if err := tx.Commit(ctx); err != nil {
- log.Printf("Failed to commit transaction: %v", err)
- }
- }
+ // Record the balance check through the shared admin-audit helper (same
+ // columns as every other admin money action; best-effort, non-fatal, and
+ // never able to abort this read-only handler).
+ InsertAdminAuditCharge(ctx, adminID, userID, "balance_check", map[string]any{"balance": balance})
if err := json.NewEncoder(w).Encode(map[string]float64{"balance": balance}); err != nil {
log.Printf("Failed to encode JSON response: %v", err)
@@ -2801,13 +2799,13 @@ func cancelGiftCardForUser(ctx context.Context, w http.ResponseWriter, r *http.R
switch {
case sqErr == nil:
- status := squareRefundStatusToLocal(refundResult.Status)
- if status == "pending" {
+ status, terminal := SquareRefundStatusToLocal(refundResult.Status)
+ if !terminal {
// 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.
- log.Printf("Square refund %s for gift-card purchase payment %s is PENDING — refund row left pending for reconciliation", refundResult.ID, paymentID)
+ log.Printf("Square refund %s for gift-card purchase payment %s is non-terminal (%s) — refund row left pending for reconciliation", refundResult.ID, paymentID, refundResult.Status)
} else if status == "failed" {
log.Printf("Square refund %s for gift-card purchase payment %s FAILED — refund row marked failed", refundResult.ID, paymentID)
}
diff --git a/backend/handlers/payments/handlers.go b/backend/handlers/payments/handlers.go
index ef65316..0b7489d 100644
--- a/backend/handlers/payments/handlers.go
+++ b/backend/handlers/payments/handlers.go
@@ -590,7 +590,7 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) {
var paymentID string
if *req.PaymentMethod == "cash" {
- if bookingPortionPounds > 0.004 {
+ if bookingPortionPounds > roundingEpsilon {
err = tx.QueryRow(r.Context(), `
INSERT INTO payments (
booking_id, payment_type, payment_method, status, amount, idempotency_key, created_by, created_at, updated_at
@@ -609,7 +609,7 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) {
// When the charge is tip-only (fully-paid booking), the tip row is
// the ONLY record and its id is returned as the checkout id,
// mirroring the card-terminal carve (primary := records[0]).
- if tipPounds > 0.004 {
+ if tipPounds > roundingEpsilon {
tipKey := splitIdempotencyKey(idempotencyKey, "-split-tip")
tipID, tipErr := service.CreatePaymentRecordTx(r.Context(), tx, PaymentRecord{
BookingID: bookingID,
@@ -631,6 +631,19 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) {
paymentID = tipID
}
}
+
+ // MEDIUM-3a: record the admin-initiated CASH charge in
+ // admin_audit_log (best-effort, non-fatal — an audit-write failure
+ // can never roll back a completed charge).
+ var cashCustomerID sql.NullString
+ if cuErr := tx.QueryRow(r.Context(), "SELECT user_id FROM bookings WHERE id = $1", bookingID).Scan(&cashCustomerID); cuErr == nil && cashCustomerID.Valid {
+ InsertAdminAuditCharge(r.Context(), adminID, cashCustomerID.String, "admin_cash_charge", map[string]any{
+ "booking_id": bookingID,
+ "payment_id": paymentID,
+ "amount": amountPounds,
+ "payment_type": req.PaymentType,
+ })
+ }
} else { // giftcard
var customerID sql.NullString
err = tx.QueryRow(r.Context(), "SELECT user_id FROM bookings WHERE id = $1", bookingID).Scan(&customerID)
@@ -733,7 +746,7 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) {
// gift card (C3 source-of-funds tracking), so the booking portion
// is the only money that counts toward the obligation.
bookingPayID := ""
- if bookingPortionPounds > 0.004 {
+ if bookingPortionPounds > roundingEpsilon {
err = tx.QueryRow(r.Context(), `
INSERT INTO payments (
booking_id, payment_type, payment_method, status, amount, idempotency_key, created_by, created_at, updated_at, gift_card_id
@@ -747,7 +760,7 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) {
}
paymentID = bookingPayID
}
- if tipPounds > 0.004 {
+ if tipPounds > roundingEpsilon {
tipKey := splitIdempotencyKey(idempotencyKey, "-split-tip")
tipID, tipErr := service.CreatePaymentRecordTx(r.Context(), tx, PaymentRecord{
BookingID: bookingID,
@@ -787,6 +800,18 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) {
}
}
}
+
+ // MEDIUM-3a: record the admin-initiated gift-card payment in
+ // admin_audit_log (best-effort, non-fatal — an audit-write failure
+ // can never roll back a completed charge).
+ if customerID.Valid {
+ InsertAdminAuditCharge(r.Context(), adminID, customerID.String, "admin_giftcard_payment", map[string]any{
+ "booking_id": bookingID,
+ "payment_id": paymentID,
+ "amount": amountPounds,
+ "payment_type": req.PaymentType,
+ })
+ }
}
if err := tx.Commit(r.Context()); err != nil {
@@ -2335,19 +2360,46 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) {
// mock used to ACCEPT £0 and mint a completed £0 deposit that consumed
// the discount — leaving the booking unpaid and the 'full' balance
// charge to overcharge later. There is nothing to charge, so skip the
- // Square call entirely and report the discount-covered deposit; the
- // flow completes without moving any money. The discount rows themselves
- // are applied by the next real charge or at booking completion
- // (applyEligibleCampaignsAtPayment).
+ // Square call entirely. CRITICAL: the eligible campaign discount rows
+ // must be applied IMMEDIATELY (in this transaction) — the OLD deferral to
+ // the next real charge let the promised discount go unrecorded: the next
+ // balance/terminal charge F1-skipped it (discountHeadroomPence already
+ // spent by the real money) and the booking completed at the FULL price
+ // with no discount row, overcharging the customer (finding A6).
if chargeAmount <= 0 {
- log.Printf("Deposit for booking %s fully covered by %d pence of eligible campaign credit — skipping the Square charge", bookingID, eligibleDiscountPence)
+ discountBefore := bookingDiscountPence(r.Context(), tx, bookingID)
+ if applyErr := applyEligibleCampaignsAtPayment(r.Context(), tx, bookingID, userID, preChargeDiscounts); applyErr != nil {
+ var exErr *campaignExhaustedAtApplyError
+ if errors.As(applyErr, &exErr) {
+ log.Printf("B13: campaign %s exhausted between preview and apply for booking %s — the deposit is NOT discount-covered; no charge issued; the retry will charge the full deposit", exErr.campaignID, bookingID)
+ } else {
+ log.Printf("Failed to apply eligible campaigns on the discount-covered deposit for booking %s: %v", bookingID, applyErr)
+ }
+ }
+ discountAfter := bookingDiscountPence(r.Context(), tx, bookingID)
+ discountApplied := discountAfter > discountBefore
+ // A fully discount-covered deposit can settle the booking: run the same
+ // fully-paid completion the real charge path runs, so a booking whose
+ // obligation is entirely covered by discount + real money completes
+ // instead of staying active but unpayable.
+ if bookingIsFullyPaid(r.Context(), tx, bookingID) {
+ completeActiveBookingFromPayment(r.Context(), tx, bookingID)
+ }
+ // Commit the discount rows (and any completion) — the deferred
+ // rollback must not undo them.
+ if err := tx.Commit(r.Context()); err != nil {
+ log.Printf("Failed to commit transaction: %v", err)
+ http.Error(w, "internal server error", http.StatusInternalServerError)
+ return
+ }
+ log.Printf("Deposit for booking %s fully covered by %d pence of eligible campaign credit — skipping the Square charge (discount applied: %v)", bookingID, eligibleDiscountPence, discountApplied)
mw.RespondJSON(w, http.StatusOK, map[string]any{
"id": "",
"booking_id": bookingID,
"payment_type": req.PaymentType,
"status": "completed",
"amount": 0,
- "deposit_covered_by_discount": true,
+ "deposit_covered_by_discount": discountApplied,
})
return
}
@@ -2489,13 +2541,17 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) {
log.Printf("Failed to create payment: %v (error_code=%q)", err, square.ErrorCode(err))
// The gate consumed the 2FA code for a fresh saved-card charge —
// re-issue so the same-key retry has a live code to verify. A NEW-CARD
- // (cnon) charge never gated and involves no code: re-issuing here would
- // overwrite the customer's standing pending code with a fresh
- // undelivered one, silently burning the code the operator relayed
- // (finding 5). 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 != "" {
+ // (cnon) charge with save_card=false never gated and involves no code:
+ // re-issuing there would overwrite the customer's standing pending code
+ // with a fresh undelivered one, silently burning the code the operator
+ // relayed (finding 5). But a NEW-CARD charge with save_card=true DID
+ // gate — the SAVE gate above consumed the code before the charge — so
+ // the code must be re-issued there too or every retry hits "Verification
+ // code expired" forever (finding: save-gate burned code never re-issued).
+ // 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 != "") || req.SaveCard {
reissueTwoFACodeAfterFailedCharge(r.Context(), db.Conn, userID, true, twoFAFallbackUsed && !reusePendingRecord, r)
}
// SCA-required failures must surface the structured verification_required
@@ -2854,6 +2910,25 @@ func refundLostCampaignAsBalanceCredit(ctx context.Context, bookingID, userID st
// max_redemptions) is the real enforcement point — the loser of a concurrent
// same-campaign redemption gets a zero-row result there and the same error
// surfaces from the apply loop.
+// bookingDiscountPence returns the total of completed discount payment rows on
+// a booking, in pence. The A6 discount-covered deposit skip path (chargeAmount
+// <= 0) measures this BEFORE and AFTER applyEligibleCampaignsAtPayment to learn
+// whether a discount was actually applied — the deposit_covered_by_discount
+// response flag must only be true when a discount row really was recorded (a
+// campaign exhausted between preview and apply records nothing).
+func bookingDiscountPence(ctx context.Context, q db.Querier, bookingID string) int64 {
+ var pence int64
+ if err := q.QueryRow(ctx, `
+ SELECT COALESCE(ROUND(SUM(amount) * 100), 0)
+ FROM payments
+ WHERE booking_id = $1 AND status = 'completed' AND payment_method = 'discount'
+ `, bookingID).Scan(&pence); err != nil {
+ log.Printf("Failed to read applied discount total for booking %s: %v", bookingID, err)
+ return 0
+ }
+ return pence
+}
+
func applyEligibleCampaignsAtPayment(ctx context.Context, q db.Querier, bookingID, userID string, expected []EligibleDiscount) error {
for _, d := range expected {
if d.Source != "campaign" {
@@ -2945,7 +3020,7 @@ func buildSplitRecords(primary PaymentRecord, reqPaymentType string, info *Booki
bookingPortion := math.Min(paymentAmount, remaining)
bookingPortion = math.Round(bookingPortion*100) / 100
tipPortion := math.Round((paymentAmount-bookingPortion)*100) / 100
- if tipPortion > 0.004 {
+ if tipPortion > roundingEpsilon {
records := []PaymentRecord{primary}
records[0].Amount = bookingPortion
tip := primary
@@ -2982,7 +3057,7 @@ func buildSplitRecords(primary PaymentRecord, reqPaymentType string, info *Booki
splitIdx := 0
// 1. Deposit portion (always present when there's deposit room left).
- if depositAmount > 0.004 {
+ if depositAmount > roundingEpsilon {
dep := primary
dep.PaymentType = "deposit"
dep.Amount = depositAmount
@@ -2991,7 +3066,7 @@ func buildSplitRecords(primary PaymentRecord, reqPaymentType string, info *Booki
}
// 2. Balance / partial / full record — covers the remaining booking total.
- if balancePortion > 0.004 {
+ if balancePortion > roundingEpsilon {
bal := primary
bal.Amount = balancePortion
bal.Fees = 0
@@ -3019,7 +3094,7 @@ func buildSplitRecords(primary PaymentRecord, reqPaymentType string, info *Booki
// reached the full total), the tip record carries the ENTIRE payment.
// Appending the primary first would double-count the charged amount
// (primary at the full amount + tip at the same full amount).
- if tipPortion > 0.004 {
+ if tipPortion > roundingEpsilon {
tip := primary
tip.PaymentType = "tip"
tip.Amount = tipPortion
@@ -3065,7 +3140,7 @@ func buildTerminalSplitRecords(primary PaymentRecord, info *BookingPaymentInfo,
var records []PaymentRecord
splitIdx := 0
- if depositAmount > 0.004 {
+ if depositAmount > roundingEpsilon {
dep := primary
dep.PaymentType = "deposit"
dep.Amount = depositAmount
@@ -3073,7 +3148,7 @@ func buildTerminalSplitRecords(primary PaymentRecord, info *BookingPaymentInfo,
splitIdx++
}
- if balancePortion > 0.004 {
+ if balancePortion > roundingEpsilon {
bal := primary
bal.PaymentType = "balance"
bal.Amount = balancePortion
@@ -3085,7 +3160,7 @@ func buildTerminalSplitRecords(primary PaymentRecord, info *BookingPaymentInfo,
records = append(records, bal)
}
- if tipAmount > 0.004 {
+ if tipAmount > roundingEpsilon {
tip := primary
tip.PaymentType = "tip"
tip.Amount = tipAmount
@@ -3558,21 +3633,27 @@ func RefundPayment(w http.ResponseWriter, r *http.Request) {
reissueResult, reissueErr := SquareClient.RefundPayment(r.Context(), reissueReq)
switch {
case reissueErr == nil:
- // Resolve by Square's status: PENDING stays pending (sweep
- // reconciles), FAILED/REJECTED is definitive, COMPLETED resolves.
- 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)
+ // Resolve by Square's status: terminal COMPLETED/APPROVED
+ // resolves, FAILED/REJECTED is definitive; non-terminal states
+ // (PENDING — sweep reconciles — CANCELED, unknown) leave the
+ // refund pending.
+ reissueStatus, reissueTerminal := SquareRefundStatusToLocal(reissueResult.Status)
+ if !reissueTerminal {
+ log.Printf("Square reissue %s is non-terminal (%s) — leaving refund %s pending for the sweep", reissueResult.ID, reissueResult.Status, existingRefundID.String)
} 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(),
- `UPDATE refunds SET status = $1, square_refund_id = $2 WHERE id = $3`,
- reissueStatus, reissueResult.ID, existingRefundID.String,
- ); upErr != nil {
- log.Printf("CRITICAL: Square refund committed (%s) but DB update for refund %s failed — manual reconciliation required: %v", reissueResult.ID, existingRefundID.String, upErr)
- http.Error(w, "internal server error", http.StatusInternalServerError)
- return
+ if reissueTerminal {
+ if _, upErr := db.Conn.Exec(r.Context(),
+ `UPDATE refunds SET status = $1, square_refund_id = $2 WHERE id = $3`,
+ reissueStatus, reissueResult.ID, existingRefundID.String,
+ ); upErr != nil {
+ log.Printf("CRITICAL: Square refund committed (%s) but DB update for refund %s failed — manual reconciliation required: %v", reissueResult.ID, existingRefundID.String, upErr)
+ http.Error(w, "internal server error", http.StatusInternalServerError)
+ return
+ }
+ } else {
+ reissueStatus = "pending"
}
if err := json.NewEncoder(w).Encode(RefundResponse{
ID: existingRefundID.String,
@@ -3797,22 +3878,27 @@ func RefundPayment(w http.ResponseWriter, r *http.Request) {
// synchronous refund response can be PENDING (money in flight, e.g. an
// async card network): marking it completed while Square later fails it
// 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 := 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)
+ // terminal COMPLETED/APPROVED resolves to completed; non-terminal states
+ // (PENDING — sweep reconciles — CANCELED, unknown) stay pending;
+ // FAILED/REJECTED is a real failure.
+ status, terminal := SquareRefundStatusToLocal(refundResult.Status)
+ if !terminal {
+ log.Printf("Square refund %s is non-terminal (%s) — leaving refund %s pending for the sweep to resolve", refundResult.ID, refundResult.Status, refundID)
} else if status == "failed" {
log.Printf("Square refund %s FAILED — marking refund %s failed", refundResult.ID, refundID)
}
- if _, upErr := db.Conn.Exec(r.Context(),
- `UPDATE refunds SET status = $1, square_refund_id = $2 WHERE id = $3`,
- status, refundResult.ID, refundID,
- ); upErr != nil {
- log.Printf("CRITICAL: Square refund committed (%s) but DB update for refund %s failed — manual reconciliation required: %v", refundResult.ID, refundID, upErr)
- http.Error(w, "internal server error", http.StatusInternalServerError)
- return
+ if terminal {
+ if _, upErr := db.Conn.Exec(r.Context(),
+ `UPDATE refunds SET status = $1, square_refund_id = $2 WHERE id = $3`,
+ status, refundResult.ID, refundID,
+ ); upErr != nil {
+ log.Printf("CRITICAL: Square refund committed (%s) but DB update for refund %s failed — manual reconciliation required: %v", refundResult.ID, refundID, upErr)
+ http.Error(w, "internal server error", http.StatusInternalServerError)
+ return
+ }
+ } else {
+ status = "pending"
}
if err := json.NewEncoder(w).Encode(RefundResponse{
@@ -3887,23 +3973,28 @@ func resumeManualPendingRefund(w http.ResponseWriter, r *http.Request, paymentID
http.Error(w, "Refund failed", http.StatusInternalServerError)
return
}
- // 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 := squareRefundStatusToLocal(resumeResult.Status)
- if status == "pending" {
- log.Printf("Square refund %s is PENDING — leaving refund %s pending for the sweep", resumeResult.ID, refundID)
+ // Resolve by Square's status — a non-terminal resume (PENDING — money in
+ // flight — CANCELED, unknown) 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, terminal := SquareRefundStatusToLocal(resumeResult.Status)
+ if !terminal {
+ log.Printf("Square refund %s is non-terminal (%s) — leaving refund %s pending for the sweep", resumeResult.ID, resumeResult.Status, refundID)
} else if status == "failed" {
log.Printf("Square refund %s FAILED — marking refund %s failed", resumeResult.ID, refundID)
}
- if _, upErr := db.Conn.Exec(r.Context(),
- `UPDATE refunds SET status = $1, square_refund_id = $2 WHERE id = $3`,
- status, resumeResult.ID, refundID,
- ); upErr != nil {
- log.Printf("CRITICAL: Square refund committed (%s) but DB update for refund %s failed — manual reconciliation required: %v", resumeResult.ID, refundID, upErr)
- http.Error(w, "internal server error", http.StatusInternalServerError)
- return
+ if terminal {
+ if _, upErr := db.Conn.Exec(r.Context(),
+ `UPDATE refunds SET status = $1, square_refund_id = $2 WHERE id = $3`,
+ status, resumeResult.ID, refundID,
+ ); upErr != nil {
+ log.Printf("CRITICAL: Square refund committed (%s) but DB update for refund %s failed — manual reconciliation required: %v", resumeResult.ID, refundID, upErr)
+ http.Error(w, "internal server error", http.StatusInternalServerError)
+ return
+ }
+ } else {
+ status = "pending"
}
if err := json.NewEncoder(w).Encode(RefundResponse{
ID: refundID,
@@ -4256,15 +4347,20 @@ func AdminRefundBooking(w http.ResponseWriter, r *http.Request) {
})
switch {
case rErr == nil:
- 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 status == "failed" {
+ sqStatus, terminal := SquareRefundStatusToLocal(result.Status)
+ if !terminal {
+ log.Printf("Square refund %s is non-terminal (%s) — leaving refund %s pending for the sweep", result.ID, result.Status, cf.refundID)
+ } else if sqStatus == "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 {
- log.Printf("CRITICAL: Square refund committed (%s) but DB update for refund %s failed — manual reconciliation required: %v", result.ID, cf.refundID, upErr)
+ if terminal {
+ if _, upErr := db.Conn.Exec(r.Context(), `UPDATE refunds SET status = $1, square_refund_id = $2 WHERE id = $3`, sqStatus, result.ID, cf.refundID); upErr != nil {
+ log.Printf("CRITICAL: Square refund committed (%s) but DB update for refund %s failed — manual reconciliation required: %v", result.ID, cf.refundID, upErr)
+ }
+ } else {
+ sqStatus = "pending"
}
+ status = sqStatus
case errors.Is(rErr, square.ErrRefundAlreadyProcessed):
if _, upErr := db.Conn.Exec(r.Context(), `UPDATE refunds SET status = 'completed' WHERE id = $1`, cf.refundID); upErr != nil {
log.Printf("Failed to resolve refund %s completed after PAYMENT_ALREADY_REFUNDED: %v", cf.refundID, upErr)
@@ -4711,9 +4807,11 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) {
// 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). A pending-reuse retry verified WITHOUT consuming, so its code is
- // still live and no re-issue runs.
- if req.CardID != nil && *req.CardID != "" {
+ // 5). A NEW-CARD charge with save_card=true DID gate (the SAVE gate
+ // consumed the code), so the code is re-issued there too (finding:
+ // save-gate burned code never re-issued). A pending-reuse retry verified
+ // WITHOUT consuming, so its code is still live and no re-issue runs.
+ if (req.CardID != nil && *req.CardID != "") || req.SaveCard {
reissueTwoFACodeAfterFailedCharge(r.Context(), db.Conn, userID, true, twoFAFallbackUsed && !reusePendingRecord, r)
}
// SCA-required failures must surface the structured verification_required
diff --git a/backend/handlers/payments/loop_b_money_fixes_test.go b/backend/handlers/payments/loop_b_money_fixes_test.go
index 73b422a..b31126e 100644
--- a/backend/handlers/payments/loop_b_money_fixes_test.go
+++ b/backend/handlers/payments/loop_b_money_fixes_test.go
@@ -55,6 +55,16 @@ func (c *failOnChargeClient) CreatePayment(ctx context.Context, req square.Creat
return nil, nil
}
+// TestBookingPayment_DepositFullyCoveredByDiscount_SkipsSquareCharge pins the
+// A6 skip path: a deposit whose eligible campaign credit covers the ENTIRE
+// remaining obligation skips the Square charge (never charges £0) AND applies
+// the eligible campaign discount rows IMMEDIATELY (finding: deferring the
+// discount to the next real charge let the booking complete at full price with
+// no discount row — the customer overpaid the promised discount). The
+// discount row is a completed payments row (payment_method='discount'), the
+// response reports deposit_covered_by_discount only because a discount was
+// actually applied, and a fully-covered booking completes like the real charge
+// path.
func TestBookingPayment_DepositFullyCoveredByDiscount_SkipsSquareCharge(t *testing.T) {
ctx, tx := testutils.SetupTestTx(t)
@@ -84,15 +94,142 @@ func TestBookingPayment_DepositFullyCoveredByDiscount_SkipsSquareCharge(t *testi
require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body))
assert.Equal(t, true, body["deposit_covered_by_discount"], "the response must signal the discount-covered deposit")
+ // The discount rows are now applied AT the skip path (never deferred to a
+ // later charge that F1-skips them — the overcharge bug). The single
+ // payments row IS the completed discount row.
var payCount int
require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM payments WHERE booking_id = $1`, bookingID).Scan(&payCount))
- assert.Zero(t, payCount, "no payment row may be recorded for a discount-covered deposit")
+ assert.Equal(t, 1, payCount, "exactly one discount payment row must be recorded for the discount-covered deposit")
- // The discount rows are applied by the next real charge / at completion,
- // never minted for a charge that did not happen.
var discountCount int
require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM booking_discounts WHERE booking_id = $1`, bookingID).Scan(&discountCount))
- assert.Zero(t, discountCount, "no discount may be applied for a charge that never happened")
+ assert.Equal(t, 1, discountCount, "the eligible campaign discount must be recorded at the skip path")
+
+ var discountAmount float64
+ require.NoError(t, tx.QueryRow(ctx, `SELECT amount FROM payments WHERE booking_id = $1 AND payment_method = 'discount'`, bookingID).Scan(&discountAmount))
+ assert.Equal(t, 50.00, discountAmount, "the £50 campaign discount must be recorded in full")
+
+ // The discount fully covers the booking — it completes exactly like a fully
+ // paid booking on the real charge path.
+ var bookingStatus string
+ require.NoError(t, tx.QueryRow(ctx, `SELECT status FROM bookings WHERE id = $1`, bookingID).Scan(&bookingStatus))
+ assert.Equal(t, "completed", bookingStatus, "a booking whose entire obligation is discount-covered must complete")
+}
+
+// TestBookingPayment_DepositCoveredDiscount_RecordsDiscount_NoOvercharge is the
+// exact A6 scenario from the finding: £100 booking, 50% campaign, post-start
+// partial £50 already paid, then a £50 deposit whose eligible credit covers the
+// ENTIRE remaining obligation. The skip path must record the £50 discount row
+// IMMEDIATELY so the promised discount is never lost — before the fix the
+// deposit returned deposit_covered_by_discount with NO discount row, the later
+// balance charge F1-skipped the discount (headroom already spent by the real
+// money) and the booking completed at the full £100 with the customer overpaying
+// the promised £50.
+func TestBookingPayment_DepositCoveredDiscount_RecordsDiscount_NoOvercharge(t *testing.T) {
+ ctx, tx := testutils.SetupTestTx(t)
+
+ userID, bookingID, _ := setupTestDataPast(t, ctx, tx)
+ userToken := jwt.GenerateUserToken(userID)
+ _, err := tx.Exec(ctx, `UPDATE bookings SET total_amount = 100.00 WHERE id = $1`, bookingID)
+ require.NoError(t, err)
+ // 50% time-based campaign = £50 eligible credit on the £100 booking.
+ seedActiveCampaign(t, ctx, tx, 50)
+ // Post-start partial £50 already paid — remaining obligation is £50.
+ _, err = fixtures.CreateTestPayment(tx, bookingID, 50.00, "online_square", "partial", "completed")
+ require.NoError(t, err)
+
+ origClient := SquareClient
+ SquareClient = &failOnChargeClient{SquareClient: square.NewDevClient(), t: t}
+ defer func() { SquareClient = origClient }()
+
+ // £50 deposit — the eligible £50 credit covers the ENTIRE remaining £50,
+ // so chargeAmount clamps to £0 and the skip path runs.
+ cardToken := "cnon:deposit-covered-partial"
+ req := CreateBookingPaymentRequest{
+ Amount: 5000,
+ PaymentType: "deposit",
+ NewCardToken: &cardToken,
+ IdempotencyKey: "deposit-covered-partial-" + bookingID,
+ }
+
+ w := makePaymentRequest(CreateBookingPayment, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx)
+ require.Equal(t, http.StatusOK, w.Code, "a discount-covered deposit must complete without a Square charge, body: %s", w.Body.String())
+
+ var body map[string]any
+ require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body))
+ assert.Equal(t, true, body["deposit_covered_by_discount"], "the response must signal the discount-covered deposit")
+
+ // The £50 campaign discount row is recorded AT the skip path.
+ var discountAmount float64
+ require.NoError(t, tx.QueryRow(ctx, `SELECT amount FROM payments WHERE booking_id = $1 AND payment_method = 'discount'`, bookingID).Scan(&discountAmount))
+ assert.Equal(t, 50.00, discountAmount, "the promised £50 discount must be recorded at the skip path")
+
+ // Ledger: real £50 + discount £50 = £100 = total. The customer pays the
+ // discounted £50, never the full £100.
+ var remainingPence int64
+ remainingPence, err = NewPaymentService().GetBookingRemainingBalancePence(ctx, bookingID)
+ require.NoError(t, err)
+ assert.Equal(t, int64(0), remainingPence, "the discounted booking must have £0 remaining after the covered deposit")
+
+ // A later unconfirmed balance charge of £50 must be REJECTED — the booking
+ // auto-completed when the deposit + discount settled it, so the completed-
+ // booking guard refuses the charge outright. The booking can never be
+ // silently overcharged the remaining £50 (the old bug: the balance charge
+ // completed at full price with no discount row).
+ w = makePaymentRequest(CreateBookingPayment, "POST", "/api/bookings/"+bookingID+"/payment", CreateBookingPaymentRequest{
+ Amount: 5000,
+ PaymentType: "balance",
+ NewCardToken: &cardToken,
+ IdempotencyKey: "balance-after-covered-deposit-" + bookingID,
+ }, userToken, ctx)
+ require.Equal(t, http.StatusConflict, w.Code, "a balance charge on a completed discounted booking must be rejected, body: %s", w.Body.String())
+}
+
+// TestGetBookingPaymentSummary_ExcludesTipsFromRemaining pins finding 4: the
+// payment summary must not count tip rows as "paid" — a tip is gratuity paid
+// beyond the booking total and must not reduce the balance owed. Before the
+// fix PaidAmount included the tip and RemainingAmount (total - paid + refunded)
+// understated the authoritative tip-excluded balance, so an admin relying on
+// the summary could under-collect.
+func TestGetBookingPaymentSummary_ExcludesTipsFromRemaining(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.CreateTestBooking(tx, userID, serviceID)
+ require.NoError(t, err)
+
+ svc := NewPaymentService()
+ // Pay the full £50 booking + a £20 tip.
+ _, err = svc.CreatePaymentRecord(ctx, PaymentRecord{
+ BookingID: bookingID,
+ PaymentType: "full",
+ PaymentMethod: "online_square",
+ Status: "completed",
+ Amount: 50.00,
+ }, nil)
+ require.NoError(t, err)
+ _, err = svc.CreatePaymentRecord(ctx, PaymentRecord{
+ BookingID: bookingID,
+ PaymentType: "tip",
+ PaymentMethod: "online_square",
+ Status: "completed",
+ Amount: 20.00,
+ }, nil)
+ require.NoError(t, err)
+
+ summary, err := svc.GetBookingPaymentSummary(ctx, bookingID)
+ require.NoError(t, err)
+
+ require.Equal(t, 50.00, summary.PaidAmount, "PaidAmount must exclude the £20 tip row")
+ require.Equal(t, 0.00, summary.RemainingAmount, "RemainingAmount must exclude the £20 tip row (the £50 booking is fully paid)")
+
+ // Cross-check against the authoritative charge-guard balance.
+ remaining, err := svc.GetBookingRemainingBalancePence(ctx, bookingID)
+ require.NoError(t, err)
+ require.Equal(t, remaining, int64(summary.RemainingAmount*100), "RemainingAmount must match GetBookingRemainingBalancePence")
}
// =============================================================================
diff --git a/backend/handlers/payments/refunds.go b/backend/handlers/payments/refunds.go
index 3a7a04e..ff2e4bc 100644
--- a/backend/handlers/payments/refunds.go
+++ b/backend/handlers/payments/refunds.go
@@ -1356,25 +1356,29 @@ func processChargeGroup(ctx context.Context, chargeID string, rows []pendingChar
}
switch {
case sqErr == nil:
- // Resolve by Square's status: COMPLETED resolves the group; PENDING
- // leaves the rows pending (a later sweep reconciles them via
- // 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 := 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)
+ // Resolve by Square's status: COMPLETED/APPROVED resolves the group;
+ // non-terminal states (PENDING — a later sweep reconciles them via
+ // ListPaymentRefunds — CANCELED, unknown) leave the rows pending;
+ // FAILED/REJECTED is a definitive failure that must not be marked
+ // completed (that would block the amount in the over-refund guard
+ // forever).
+ sqStatus, sqTerminal := SquareRefundStatusToLocal(sqResult.Status)
+ if !sqTerminal {
+ log.Printf("Square refund %s for charge %s is non-terminal (%s) — leaving refunds pending for the sweep", sqResult.ID, chargeID, sqResult.Status)
} 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
// crash-retry amounts identical so Square's key-dedup returns the
- // original refund.
- if _, upErr := db.Conn.Exec(ctx, `
- UPDATE refunds SET status = $1, square_refund_id = $2
- WHERE id = ANY($3) AND status = 'pending'
- `, sqStatus, sqResult.ID, idsOf(pending)); upErr != nil {
- log.Printf("CRITICAL: Square refund committed (%s) but DB update for charge %s failed — manual reconciliation required: %v", sqResult.ID, chargeID, upErr)
+ // original refund. Only terminal states write a status: a non-terminal
+ // outcome must leave the rows pending untouched.
+ if sqTerminal {
+ if _, upErr := db.Conn.Exec(ctx, `
+ UPDATE refunds SET status = $1, square_refund_id = $2
+ WHERE id = ANY($3) AND status = 'pending'
+ `, sqStatus, sqResult.ID, idsOf(pending)); upErr != nil {
+ log.Printf("CRITICAL: Square refund committed (%s) but DB update for charge %s failed — manual reconciliation required: %v", sqResult.ID, chargeID, upErr)
+ }
}
return len(pending), nil
@@ -2259,21 +2263,24 @@ func processManualPaymentGroup(ctx context.Context, paymentID string, rows []man
// Resolve by Square's status: a synchronous refund response can be
// PENDING (money in flight, e.g. an async card network) — marking it
// completed while Square later fails it 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. Mirrors
- // processChargeGroup and the RefundPayment handler (handlers.go).
- 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)
+ // amount in the over-refund guard. Only a terminal COMPLETED/APPROVED
+ // resolves to completed; non-terminal states (PENDING, CANCELED,
+ // unknown) stay pending for the sweep to reconcile; FAILED/REJECTED
+ // is a real failure. Mirrors processChargeGroup and the RefundPayment
+ // handler (handlers.go).
+ sqStatus, sqTerminal := SquareRefundStatusToLocal(sqResult.Status)
+ if !sqTerminal {
+ log.Printf("Square refund %s for manual refund %s is non-terminal (%s) — leaving the row pending for the sweep to resolve", sqResult.ID, pr.ID, sqResult.Status)
} 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, `
- UPDATE refunds SET status = $1, square_refund_id = $2
- WHERE id = $3
- `, sqStatus, sqResult.ID, pr.ID); upErr != nil {
- log.Printf("CRITICAL: Square refund committed (%s) but DB update for manual refund %s failed — manual reconciliation required: %v", sqResult.ID, pr.ID, upErr)
+ if sqTerminal {
+ if _, upErr := db.Conn.Exec(ctx, `
+ UPDATE refunds SET status = $1, square_refund_id = $2
+ WHERE id = $3
+ `, sqStatus, sqResult.ID, pr.ID); upErr != nil {
+ log.Printf("CRITICAL: Square refund committed (%s) but DB update for manual refund %s failed — manual reconciliation required: %v", sqResult.ID, pr.ID, upErr)
+ }
}
processed++
diff --git a/backend/handlers/payments/refunds_test.go b/backend/handlers/payments/refunds_test.go
index b3603e0..ae74cb3 100644
--- a/backend/handlers/payments/refunds_test.go
+++ b/backend/handlers/payments/refunds_test.go
@@ -4695,11 +4695,14 @@ func TestSweepPendingB1Refunds_PendingPastAge_EscalatedStopsRepoll(t *testing.T)
// =============================================================================
// TestSweepManualRefund_SyncPendingResponse_LeavesPending locks the DRIFT-REAL
-// fix: when the sweep re-issues a manual refund and Square returns PENDING
-// synchronously (money in flight, e.g. an async card network), the refund row
-// must be left 'pending' — never 'completed' (a later Square failure would
-// permanently block the amount in the over-refund guard). Mirrors the
-// processChargeGroup / RefundPayment handler status handling.
+// semantics for a synchronous PENDING refund response: Square accepts the
+// refund but leaves it PENDING (money in flight, e.g. an async card network) —
+// a NON-terminal state. The refund row must be left 'pending' — never
+// 'completed' (a later Square failure would permanently block the amount in
+// the over-refund guard) — and square_refund_id must NOT be written (only
+// terminal states record the Square reference; a later sweep run re-attempts
+// the same deterministic key, which Square dedups, or re-discovers the refund).
+// Mirrors the processChargeGroup / RefundPayment handler status handling.
func TestSweepManualRefund_SyncPendingResponse_LeavesPending(t *testing.T) {
ctx, tx := testutils.SetupTestTx(t)
@@ -4779,7 +4782,7 @@ func TestSweepManualRefund_SyncPendingResponse_LeavesPending(t *testing.T) {
if status != "pending" {
t.Errorf("expected a synchronous-PENDING Square refund to leave the row 'pending', got %q", status)
}
- if squareRefundID == nil || *squareRefundID == "" {
- t.Error("expected square_refund_id to be set (Square holds the refund)")
+ if squareRefundID != nil && *squareRefundID != "" {
+ t.Error("expected square_refund_id NOT to be written for a non-terminal PENDING response (only terminal states record the Square reference)")
}
}
diff --git a/backend/handlers/payments/service.go b/backend/handlers/payments/service.go
index 00e5603..c5af7fa 100644
--- a/backend/handlers/payments/service.go
+++ b/backend/handlers/payments/service.go
@@ -18,6 +18,17 @@ import (
"github.com/jackc/pgx/v5"
)
+// Payment-fee formulas used by CalculateFees (integer pence math):
+// - online: amount×feeRatePencePerPound/feeDenominator + minFeePence → 1.4% + 25p
+// - in-person (terminal/cash): amount×terminalFeeRatePencePerPound/terminalFeeDenominator → 1.75%
+const (
+ feeRatePencePerPound = 14
+ feeDenominator = 1000
+ minFeePence = 25
+ terminalFeeRatePencePerPound = 175
+ terminalFeeDenominator = 10000
+)
+
type SavedCard struct {
ID string `json:"id"`
// SquareCustomerID is the user's provisioned Square customer profile id
@@ -97,9 +108,9 @@ type PaymentSummary struct {
func (s *PaymentService) CalculateFees(amount int64, method string) float64 {
if method == "online" {
- return float64((amount*14/1000)+25) / 100.0
+ return float64((amount*feeRatePencePerPound/feeDenominator)+minFeePence) / 100.0
}
- return float64(amount*175/10000) / 100.0
+ return float64(amount*terminalFeeRatePencePerPound/terminalFeeDenominator) / 100.0
}
func (s *PaymentService) CreatePaymentRecord(ctx context.Context, record PaymentRecord, giftCardID *string) (string, error) {
@@ -237,7 +248,15 @@ func (s *PaymentService) GetBookingPaymentSummary(ctx context.Context, bookingID
return nil, err
}
summary.Payments = append(summary.Payments, p)
- if p.Status == "completed" {
+ // A tip is money paid beyond the booking total — gratuity, not payment
+ // toward the booking. It must not reduce the balance owed: excluding it
+ // here keeps paid_amount / remaining_amount aligned with the
+ // authoritative tip-excluded GetBookingRemainingBalancePence (a tip
+ // row would otherwise understate the remaining balance and an admin
+ // relying on the summary could under-collect). Discount rows stay
+ // counted (real value applied toward the booking), matching the VAT
+ // ledger tests.
+ if p.Status == "completed" && p.PaymentType != "tip" {
paidAmount += p.Amount
if p.VATAmount != nil {
totalVATAmount += *p.VATAmount
@@ -282,7 +301,19 @@ func (s *PaymentService) GetBookingPaymentSummary(ctx context.Context, bookingID
refundedAmount += r.Amount
}
summary.RefundedAmount = refundedAmount
- summary.RemainingAmount = totalAmount - paidAmount + refundedAmount
+ // RemainingAmount is routed through the authoritative tip-excluded balance
+ // (GetBookingRemainingBalancePence — the same value the charge guards use,
+ // including pending refunds and the LEAST/GREATEST clamps) instead of the
+ // naive total - paid + refunded: the naive sum counts tips as "paid", so a
+ // booking with a £20 tip would report £30 remaining where the authoritative
+ // balance is £50 — an admin relying on the summary could under-collect
+ // (finding 4). Falls back to the legacy formula only when the authoritative
+ // read fails (e.g. a missing booking).
+ if remaining, remErr := s.GetBookingRemainingBalancePence(ctx, bookingID); remErr == nil {
+ summary.RemainingAmount = float64(remaining) / 100.0
+ } else {
+ summary.RemainingAmount = totalAmount - paidAmount + refundedAmount
+ }
return summary, nil
}
diff --git a/backend/handlers/payments/sweep.go b/backend/handlers/payments/sweep.go
index 2ceacc4..8e5b3f7 100644
--- a/backend/handlers/payments/sweep.go
+++ b/backend/handlers/payments/sweep.go
@@ -63,6 +63,17 @@ const stalePendingPaymentAge = 24 * time.Hour
// trustworthily and fall back to the legacy blind-fail + WARN.
const stalePendingKeyedAge = 22 * time.Hour
+// b1DuplicateRefundAttemptCap caps how many times the sweep may auto-refund a
+// replay-induced duplicate charge (B1) under one stale pending row's expired
+// idempotency key. A REJECTED refund writes NO refunds row, so the in-flight
+// guard (hasInFlightSweepDuplicateRefund) stays false and the next run would
+// re-replay the SAME expired key — Square mints ANOTHER charge, the refund is
+// rejected again, and the loop stacks unauthorized charges over the row's 24h
+// life. The cap stops the loop: a row at the cap is failed with a CRITICAL
+// notification and its key is never replayed. Mirrors maxManualRefundAttempts
+// (refunds.go).
+const b1DuplicateRefundAttemptCap = 3
+
// SweepStalePendingPayments resolves stale pending payments and till sales; see the rationale block on stalePendingPaymentAge above.
func SweepStalePendingPayments(ctx context.Context) (int, error) {
cutoff := clock.Now().Add(-stalePendingPaymentAge)
@@ -199,6 +210,10 @@ type staleRow struct {
IsCreate bool // true when this sale created the gift card (timestamps equal)
HasGiftCard bool // false when the LEFT JOIN found no gift_cards row (gc.id IS NULL)
TotalAmount float64 // till_sales.total_amount — the funding this sale added
+ // B1Attempts counts how many times the sweep has auto-refunded a
+ // replay-induced duplicate charge under this row's expired idempotency key.
+ // A REJECTED refund (or the cap) means the key must never be replayed again.
+ B1Attempts int
}
// sweepStaleRows resolves the stale pending rows of one table. Rows with a
@@ -341,6 +356,24 @@ func sweepKeyedStaleRows(ctx context.Context, table string, cutoff time.Time) (r
log.Printf("Stale pending %s row %s has a sweep auto-refund of a duplicate charge still pending at Square — leaving pending until the refund settles", table, r.ID)
continue
}
+ // B1 (CRITICAL-HIGH): never re-replay a key whose B1 auto-refund of a
+ // replay-induced duplicate charge has already FAILED. hasFailedSweepDuplicateRefund
+ // catches a refund Square rejected AFTER the sweep accepted it pending —
+ // the FAILED-webhook demotion clears the in-flight guard above, so without
+ // this check the next run would re-replay the expired key and mint ANOTHER
+ // charge. The b1_attempts cap bounds a refund that keeps failing without
+ // ever writing a refunds row (REJECTED at call time) or keeps erroring
+ // (transport) — the current row count is re-read from the DB each run.
+ // A row here is failed (NOT clawed back — the duplicate money may stand
+ // at Square) with a CRITICAL notification for manual reconciliation.
+ if hasFailedSweepDuplicateRefund(ctx, table, r.ID) || r.B1Attempts >= b1DuplicateRefundAttemptCap {
+ if failStaleRow(ctx, table, r.ID) {
+ resolved++
+ }
+ notifyStaleRowCritical(ctx, r)
+ log.Printf("Stale pending %s row %s has a FAILED or attempt-capped B1 auto-refund of a replay-induced duplicate charge (b1_attempts=%d) — never re-replaying the expired key; marked failed — MANUAL RECONCILIATION REQUIRED: check Square for the duplicate charge and refund it", table, r.ID, r.B1Attempts)
+ continue
+ }
if r.CreatedAt.Before(replayExpired) {
// Key retention window already closed — replaying would misread an
// expired key as "never charged". Blind-fail + WARN exactly as the
@@ -399,6 +432,19 @@ func sweepKeyedStaleRows(ctx context.Context, table string, cutoff time.Time) (r
if failStaleRow(ctx, table, r.ID) {
resolved++
}
+ case staleReconcileDefinitivelyFailedNoClawback:
+ // B1 (CRITICAL-HIGH): the auto-refund of the replay-induced
+ // duplicate charge was definitively REJECTED at Square — the
+ // duplicate charge stands and the money is REAL at Square, so a
+ // till sale's funded gift card is NOT clawed back. Mark the parent
+ // row failed, raise the CRITICAL notification, and set b1_attempts
+ // to the cap so the expired key is never replayed (each replay
+ // would mint ANOTHER charge).
+ if failStaleRow(ctx, table, r.ID) {
+ resolved++
+ }
+ notifyStaleRowCritical(ctx, r)
+ setB1AttemptsToCap(ctx, table, r.ID)
}
}
return resolved, completed, unverifiable, nil
@@ -428,7 +474,7 @@ func fetchStaleRows(ctx context.Context, table string, cutoff time.Time, keyedOn
COALESCE(ts.square_source_id, ''), COALESCE(ts.square_request_snapshot, ''),
ts.created_at, ts.item_id, gc.redeemed_by,
(ts.created_at = gc.created_at) AS is_create,
- (gc.id IS NOT NULL) AS has_gift_card, ts.total_amount
+ (gc.id IS NOT NULL) AS has_gift_card, ts.total_amount, ts.b1_attempts
FROM till_sales ts
LEFT JOIN gift_cards gc ON gc.id = ts.item_id
WHERE ts.status = 'pending' AND ts.created_at < $1`+methodFilter+keyedPredicate+`
@@ -441,7 +487,7 @@ func fetchStaleRows(ctx context.Context, table string, cutoff time.Time, keyedOn
rows, err = db.Conn.Query(ctx, `
SELECT id, COALESCE(square_payment_id, ''), COALESCE(idempotency_key, ''),
COALESCE(square_source_id, ''), COALESCE(square_request_snapshot, ''),
- created_at, amount, booking_id, created_by
+ created_at, amount, booking_id, created_by, b1_attempts
FROM `+pgx.Identifier{table}.Sanitize()+`
WHERE status = 'pending' AND created_at < $1`+keyedPredicate+`
`, cutoff)
@@ -472,7 +518,7 @@ func scanStaleRow(table string, rows pgx.Rows) (staleRow, error) {
var isCreate *bool
var hasGiftCard bool
if err := rows.Scan(&r.ID, &r.SquarePaymentID, &r.IdempotencyKey, &r.SquareSourceID, &snapshot, &r.CreatedAt,
- &itemID, &redeemedBy, &isCreate, &hasGiftCard, &r.TotalAmount); err != nil {
+ &itemID, &redeemedBy, &isCreate, &hasGiftCard, &r.TotalAmount, &r.B1Attempts); err != nil {
return r, err
}
r.SquareRequestSnapshot = []byte(snapshot.String)
@@ -487,7 +533,7 @@ func scanStaleRow(table string, rows pgx.Rows) (staleRow, error) {
}
var amount float64
var bookingID, createdBy, snapshot sql.NullString
- if err := rows.Scan(&r.ID, &r.SquarePaymentID, &r.IdempotencyKey, &r.SquareSourceID, &snapshot, &r.CreatedAt, &amount, &bookingID, &createdBy); err != nil {
+ if err := rows.Scan(&r.ID, &r.SquarePaymentID, &r.IdempotencyKey, &r.SquareSourceID, &snapshot, &r.CreatedAt, &amount, &bookingID, &createdBy, &r.B1Attempts); err != nil {
return r, err
}
r.SquareRequestSnapshot = []byte(snapshot.String)
@@ -1162,6 +1208,18 @@ func reconcileStalePaymentByKey(ctx context.Context, table string, r staleRow) (
} else if errors.Is(refundErr, errSweepRefundPending) {
log.Printf("stale pending %s row %s: the auto-refund of the replay-induced duplicate charge %s is PENDING at Square (non-terminal) — leaving the row pending; the B1 refund re-poll resolves it when Square settles", table, r.ID, pr.ID)
return staleReconcileLeavePending, ""
+ } else if errors.Is(refundErr, errSweepRefundRejected) {
+ // B1 (CRITICAL-HIGH): the auto-refund was DEFINITIVELY REJECTED
+ // at Square — the duplicate charge stands and can never be
+ // auto-refunded. The parent row is failed WITHOUT a gift-card
+ // clawback (the money is real at Square), the CRITICAL
+ // notification is raised, and b1_attempts is set to the cap so
+ // the expired key is never replayed (each replay would mint
+ // ANOTHER charge). The outer loop's
+ // staleReconcileDefinitivelyFailedNoClawback branch does all of
+ // this.
+ log.Printf("stale pending %s row %s: the auto-refund of the replay-induced duplicate charge %s was DEFINITIVELY REJECTED at Square (%v) — marking the row failed with a CRITICAL notification; the expired key will never be replayed", table, r.ID, pr.ID, refundErr)
+ return staleReconcileDefinitivelyFailedNoClawback, ""
} else {
return leavePendingCritical(ctx, r, "stale pending %s reconcile by key: the replayed COMPLETED payment %s was created after the pending row %s (lag %s) — a NEW charge under an expired idempotency key; auto-refund FAILED (%v) — leaving the row PENDING — MANUAL RECONCILIATION REQUIRED: check Square for both charges and refund the duplicate", table, pr.ID, r.ID, lag, refundErr)
}
@@ -1189,6 +1247,16 @@ func reconcileStalePaymentByKey(ctx context.Context, table string, r staleRow) (
// re-poll finds the SAME refund at Square.
var errSweepRefundPending = errors.New("sweep duplicate-charge refund pending at Square")
+// errSweepRefundRejected is the sentinel refundSweepDuplicateCharge returns
+// when Square DEFINITIVELY rejected the auto-refund of a replay-induced
+// duplicate charge (refund status FAILED/REJECTED). The duplicate charge
+// stands at Square and can never be auto-refunded, so the caller must mark the
+// parent row failed, raise the CRITICAL notification and set b1_attempts to
+// the cap — replaying the expired key would mint ANOTHER charge (CRITICAL-HIGH
+// B1). Unlike errSweepRefundPending (non-terminal, row stays pending), this is
+// terminal and MUST NOT be left for the next sweep run.
+var errSweepRefundRejected = errors.New("sweep duplicate-charge refund rejected at Square")
+
// sweepDuplicateRefundReason is the audit-trail reason carried by every
// refunds row for a sweep auto-refund of a replay-induced duplicate charge
// (B1). The B1 re-poll pass (sweepPendingB1Refunds, refunds.go) and the
@@ -1251,6 +1319,11 @@ func refundSweepDuplicateCharge(ctx context.Context, table string, r staleRow, p
if pr.Amount <= 0 {
return fmt.Errorf("refusing to auto-refund a non-positive amount %d pence for replayed payment %s (row %s)", pr.Amount, pr.ID, r.ID)
}
+ // B1: count THIS attempt against the row's b1_attempts cap so a refund
+ // that keeps failing (REJECTED at call time writes no refunds row; a
+ // transport error writes none either) can never be replayed forever.
+ // Each replay mints another charge at Square.
+ incrementB1Attempts(ctx, table, r.ID)
// Deterministic per-duplicate key: Square dedups same-key refunds, so a
// re-run replaying the same C2 never double-refunds. "sweepdup-" + Square
// payment id stays well under Square's 45-char idempotency-key limit.
@@ -1280,8 +1353,12 @@ func refundSweepDuplicateCharge(ctx context.Context, table string, r staleRow, p
status = "pending"
pending = true
case "FAILED", "REJECTED":
- // Square definitively rejected the refund — the duplicate charge stands.
- return fmt.Errorf("auto-refund of replay-induced duplicate charge %s was %s at Square", pr.ID, res.Status)
+ // Square definitively rejected the refund — the duplicate charge stands
+ // and can never be auto-refunded. The caller must NOT leave the row
+ // pending for another replay (a replay mints ANOTHER charge): it marks
+ // the parent failed, raises the CRITICAL notification and sets the
+ // b1_attempts cap so the expired key is never replayed.
+ return fmt.Errorf("auto-refund of replay-induced duplicate charge %s was %s at Square: %w", pr.ID, res.Status, errSweepRefundRejected)
}
recordSweepDuplicateRefundRow(ctx, table, r, pr, res.ID, status, refundKey)
@@ -1379,6 +1456,71 @@ func hasInFlightSweepDuplicateRefund(ctx context.Context, table, id string) bool
return exists
}
+// hasFailedSweepDuplicateRefund reports whether a stale pending row carries a
+// sweep auto-refund of a replay-induced duplicate charge (B1) that Square
+// definitively REJECTED (refunds row status 'failed'). The FAILED-webhook
+// demotion (refund.updated → status failed) or the B1 re-poll's terminal-failed
+// resolution marks a refund that was accepted PENDING and later failed — which
+// clears the in-flight guard (hasInFlightSweepDuplicateRefund only matches
+// 'pending'). Replaying such a row's expired key would mint ANOTHER charge, so
+// the sweep must never do it (CRITICAL-HIGH B1). A till_sale's reason carries
+// the sale id (see sweepDuplicateRefundReasonFor), mirroring the in-flight
+// guard's scoping.
+func hasFailedSweepDuplicateRefund(ctx context.Context, table, id string) bool {
+ var exists bool
+ var err error
+ if table == "till_sales" {
+ err = db.Conn.QueryRow(ctx, `
+ SELECT EXISTS (
+ SELECT 1 FROM refunds
+ WHERE status = 'failed' AND square_refund_id IS NOT NULL
+ AND reason = $1
+ )
+ `, sweepDuplicateRefundReasonFor(id)).Scan(&exists)
+ } else {
+ err = db.Conn.QueryRow(ctx, `
+ SELECT EXISTS (
+ SELECT 1 FROM refunds
+ WHERE payment_id = $1 AND status = 'failed' AND square_refund_id IS NOT NULL
+ AND reason = $2
+ )
+ `, id, sweepDuplicateRefundReason).Scan(&exists)
+ }
+ if err != nil {
+ log.Printf("Failed to check for a failed sweep duplicate refund on %s row %s: %v", table, id, err)
+ return false
+ }
+ return exists
+}
+
+// incrementB1Attempts records one more B1 auto-refund attempt against a stale
+// pending row's b1_attempts cap. Runs BEFORE the Square refund call so every
+// attempt (success, pending, rejected, transport error) counts — a refund that
+// keeps failing without writing a refunds row can never exceed the cap and
+// force an unbounded replay loop. Best-effort: a failed UPDATE must not block
+// the refund itself.
+func incrementB1Attempts(ctx context.Context, table, id string) {
+ if _, err := db.Conn.Exec(ctx, `
+ UPDATE `+pgx.Identifier{table}.Sanitize()+` SET b1_attempts = b1_attempts + 1, updated_at = NOW()
+ WHERE id = $1
+ `, id); err != nil {
+ log.Printf("Failed to increment b1_attempts on %s row %s: %v", table, id, err)
+ }
+}
+
+// setB1AttemptsToCap pins a stale pending row's b1_attempts to the cap after a
+// B1 auto-refund was definitively REJECTED at Square — the belt-and-suspenders
+// guarantee that the expired key is never replayed even if the row somehow
+// stays pending. Best-effort like incrementB1Attempts.
+func setB1AttemptsToCap(ctx context.Context, table, id string) {
+ if _, err := db.Conn.Exec(ctx, `
+ UPDATE `+pgx.Identifier{table}.Sanitize()+` SET b1_attempts = $2, updated_at = NOW()
+ WHERE id = $1 AND b1_attempts < $2
+ `, id, b1DuplicateRefundAttemptCap); err != nil {
+ log.Printf("Failed to set b1_attempts cap on %s row %s: %v", table, id, err)
+ }
+}
+
// leaveGiftCardPurchasePending keeps a gift-card-purchase payment row (payments
// table, booking_id NULL) pending after Square confirms the charge COMPLETED,
// instead of rescuing it to 'completed'. Completing the row would permanently
@@ -1483,6 +1625,12 @@ const (
// completed (payment not found / non-completed status); mark the row
// 'failed' exactly as the legacy bulk sweep did.
staleReconcileDefinitivelyFailed
+ // staleReconcileDefinitivelyFailedNoClawback — Square definitively rejected
+ // the B1 auto-refund of a replay-induced duplicate charge (the duplicate
+ // stands at Square, so the money is REAL and a till sale's funded gift card
+ // must NOT be clawed back). The outer loop marks the parent failed without a
+ // clawback, raises the CRITICAL notification and sets the b1_attempts cap.
+ staleReconcileDefinitivelyFailedNoClawback
)
// reconcileStalePaymentAtSquare asks Square for the authoritative status of a
diff --git a/backend/handlers/payments/sweep_test.go b/backend/handlers/payments/sweep_test.go
index f76e27f..3516a55 100644
--- a/backend/handlers/payments/sweep_test.go
+++ b/backend/handlers/payments/sweep_test.go
@@ -3,6 +3,7 @@
package payments
import (
+ "bytes"
"context"
"errors"
"fmt"
@@ -1094,6 +1095,312 @@ func TestSweepStalePendingPayments_KeyedTillReplayNewCharge_RefundPending_NoClaw
}
}
+// rejectedRefundClient forces RefundPayment to return a REJECTED result so the
+// B1 auto-refund's definitive-rejection branch is exercised: Square rejects the
+// refund of the replay-induced duplicate charge (the duplicate stands).
+type rejectedRefundClient struct {
+ square.SquareClient
+}
+
+func (c *rejectedRefundClient) RefundPayment(ctx context.Context, req square.RefundPaymentReq) (*square.RefundResult, error) {
+ res, err := c.SquareClient.RefundPayment(ctx, req)
+ if err != nil {
+ return nil, err
+ }
+ res.Status = "REJECTED"
+ return res, nil
+}
+
+// failOnReplayClient fails the test if the sweep attempts to re-replay the
+// SPECIFIC expired idempotency key it was constructed with — proving the B1
+// guard never re-replays a key whose auto-refund of a replay-induced duplicate
+// has failed or hit the attempt cap (each replay would mint ANOTHER charge at
+// Square). The sweep processes the whole shared test database, so replays of
+// OTHER tests' rows are delegated to the embedded client instead of failing.
+type failOnReplayClient struct {
+ square.SquareClient
+ t *testing.T
+ key string
+}
+
+func (c *failOnReplayClient) ReplayPaymentByKey(ctx context.Context, snapshotJSON []byte) (*square.PaymentResult, error) {
+ if bytes.Contains(snapshotJSON, []byte(c.key)) {
+ c.t.Fatalf("the sweep must NEVER re-replay the expired key %q whose B1 refund failed / hit the attempt cap", c.key)
+ }
+ return c.SquareClient.ReplayPaymentByKey(ctx, snapshotJSON)
+}
+
+// TestSweepStalePendingPayments_KeyedReplayNewCharge_RefundRejected_FailsRowNoReplay
+// locks the CRITICAL-HIGH B1 fix for a refund Square REJECTS at call time: the
+// auto-refund of the replay-induced duplicate is definitively rejected (the
+// duplicate stands at Square). The parent row must be marked failed (the
+// original charge was never found), the CRITICAL notification raised, and
+// b1_attempts pinned to the cap so the expired key is NEVER replayed — a
+// rejected refund writes NO refunds row, so without the cap the next sweep
+// would re-replay the key and mint ANOTHER charge (the stacking-unauthorized-
+// charges loop the finding describes). No refunds row exists (the refund was
+// never accepted). Sequential (flips SQUARE_ENVIRONMENT), like the sibling B1
+// tests.
+func TestSweepStalePendingPayments_KeyedReplayNewCharge_RefundRejected_FailsRowNoReplay(t *testing.T) {
+ ctx, tx := testutils.SetupTestTx(t)
+
+ userID, err := fixtures.CreateTestUser(tx)
+ if err != nil {
+ t.Fatalf("failed to create user: %v", err)
+ }
+ serviceID, err := fixtures.CreateTestService(tx)
+ if err != nil {
+ t.Fatalf("failed to create service: %v", err)
+ }
+ bookingID, err := fixtures.CreateTestBookingAtTime(tx, userID, serviceID,
+ time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC))
+ if err != nil {
+ t.Fatalf("failed to create booking: %v", err)
+ }
+
+ staleID, err := fixtures.CreateTestPayment(tx, bookingID, 2000.00, "online_square", "full", "pending")
+ if err != nil {
+ t.Fatalf("failed to create stale pending payment: %v", err)
+ }
+ const key = "key-expired-replay-rejected-refund"
+ const dupPayID = "pay_expired_key_rejected_refund"
+ if _, err := tx.Exec(ctx, "UPDATE payments SET created_at = NOW() - INTERVAL '23 hours', idempotency_key = $1, square_source_id = 'ccof:test-saved-card', created_by = $2 WHERE id = $3", key, userID, staleID); err != nil {
+ t.Fatalf("failed to age the stale payment: %v", err)
+ }
+ var rowCreatedAt time.Time
+ if err := tx.QueryRow(ctx, "SELECT created_at FROM payments WHERE id = $1", staleID).Scan(&rowCreatedAt); err != nil {
+ t.Fatalf("failed to read aged payment created_at: %v", err)
+ }
+
+ origClient := SquareClient
+ mock := square.NewDevClient()
+ t.Setenv("SQUARE_ENVIRONMENT", "production")
+ counting := &countingRefundClient{SquareClient: &rejectedRefundClient{SquareClient: &staleReplayClient{SquareClient: mock, result: &square.PaymentResult{
+ Status: "COMPLETED",
+ ID: dupPayID,
+ SquarePayID: dupPayID,
+ Amount: 200000,
+ CreatedAt: rowCreatedAt.Add(25 * time.Hour).Format(time.RFC3339),
+ }}}}
+ SquareClient = counting
+ defer func() { SquareClient = origClient }()
+
+ pgxTx := db.TxFromContext(ctx)
+ if pgxTx == nil {
+ t.Fatal("no transaction in context")
+ }
+ if err := pgxTx.Commit(ctx); err != nil {
+ t.Fatalf("failed to commit test tx: %v", err)
+ }
+
+ t.Cleanup(func() {
+ _, _ = db.Conn.Exec(context.Background(), `DELETE FROM refunds WHERE payment_id = $1`, staleID)
+ _, _ = db.Conn.Exec(context.Background(), `DELETE FROM admin_notifications WHERE reason = 'critical_payment_log' AND user_id = $1`, userID)
+ _, _ = 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)
+ }
+
+ // The REJECTED auto-refund marks the parent failed (the original charge was
+ // never found) — with the CRITICAL notification and the b1_attempts cap set.
+ var status string
+ var b1Attempts int
+ if err := db.Conn.QueryRow(freshCtx, "SELECT status, b1_attempts FROM payments WHERE id = $1", staleID).Scan(&status, &b1Attempts); err != nil {
+ t.Fatalf("failed to query payment: %v", err)
+ }
+ if status != "failed" {
+ t.Errorf("expected a REJECTED auto-refund to mark the row failed, got %q", status)
+ }
+ if b1Attempts != b1DuplicateRefundAttemptCap {
+ t.Errorf("expected b1_attempts pinned to the cap %d after a REJECTED refund, got %d", b1DuplicateRefundAttemptCap, b1Attempts)
+ }
+
+ // Exactly one auto-refund was ATTEMPTED (and rejected) — and, crucially, NO
+ // refunds row was written (the refund was never accepted), which is exactly
+ // the state that used to re-arm the replay loop.
+ calls := counting.refundCalls()
+ if len(calls) != 1 {
+ t.Fatalf("expected exactly one auto-refund attempt of the duplicate charge, got %d", len(calls))
+ }
+ var refundCount int
+ if err := db.Conn.QueryRow(freshCtx, `SELECT COUNT(*) FROM refunds WHERE payment_id = $1`, staleID).Scan(&refundCount); err != nil {
+ t.Fatalf("failed to count refunds: %v", err)
+ }
+ if refundCount != 0 {
+ t.Errorf("expected NO refunds row for a REJECTED auto-refund, got %d", refundCount)
+ }
+
+ var notifCount int
+ if err := db.Conn.QueryRow(freshCtx, `SELECT COUNT(*) FROM admin_notifications WHERE reason = 'critical_payment_log' AND user_id = $1`, userID).Scan(¬ifCount); err != nil {
+ t.Fatalf("failed to count admin notifications: %v", err)
+ }
+ if notifCount < 1 {
+ t.Errorf("expected a critical-payment admin notification for the rejected auto-refund, got %d", notifCount)
+ }
+}
+
+// TestSweepStalePendingPayments_KeyedReplay_NoReplayAtAttemptCap locks the B1
+// cap guard: a stale pending row whose b1_attempts already reached the cap must
+// be failed with a CRITICAL notification and its expired key NEVER replayed
+// (a replay would mint ANOTHER charge at Square). The failOnReplayClient
+// proves the replay never happens.
+func TestSweepStalePendingPayments_KeyedReplay_NoReplayAtAttemptCap(t *testing.T) {
+ ctx, tx := testutils.SetupTestTx(t)
+
+ userID, err := fixtures.CreateTestUser(tx)
+ if err != nil {
+ t.Fatalf("failed to create user: %v", err)
+ }
+ serviceID, err := fixtures.CreateTestService(tx)
+ if err != nil {
+ t.Fatalf("failed to create service: %v", err)
+ }
+ bookingID, err := fixtures.CreateTestBookingAtTime(tx, userID, serviceID,
+ time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC))
+ if err != nil {
+ t.Fatalf("failed to create booking: %v", err)
+ }
+
+ staleID, err := fixtures.CreateTestPayment(tx, bookingID, 2000.00, "online_square", "full", "pending")
+ if err != nil {
+ t.Fatalf("failed to create stale pending payment: %v", err)
+ }
+ const key = "key-expired-replay-at-cap"
+ if _, err := tx.Exec(ctx, "UPDATE payments SET created_at = NOW() - INTERVAL '23 hours', idempotency_key = $1, square_source_id = 'ccof:test-saved-card', created_by = $2, b1_attempts = $3 WHERE id = $4", key, userID, b1DuplicateRefundAttemptCap, staleID); err != nil {
+ t.Fatalf("failed to age the stale payment and set the attempt cap: %v", err)
+ }
+
+ origClient := SquareClient
+ SquareClient = &failOnReplayClient{SquareClient: square.NewDevClient(), t: t, key: key}
+ defer func() { SquareClient = origClient }()
+
+ pgxTx := db.TxFromContext(ctx)
+ if pgxTx == nil {
+ t.Fatal("no transaction in context")
+ }
+ if err := pgxTx.Commit(ctx); err != nil {
+ t.Fatalf("failed to commit test tx: %v", err)
+ }
+
+ t.Cleanup(func() {
+ _, _ = db.Conn.Exec(context.Background(), `DELETE FROM admin_notifications WHERE reason = 'critical_payment_log' AND user_id = $1`, userID)
+ _, _ = 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 string
+ if err := db.Conn.QueryRow(freshCtx, "SELECT status FROM payments WHERE id = $1", staleID).Scan(&status); err != nil {
+ t.Fatalf("failed to query payment: %v", err)
+ }
+ if status != "failed" {
+ t.Errorf("expected a row at the b1_attempts cap to be failed without a replay, got %q", status)
+ }
+
+ var notifCount int
+ if err := db.Conn.QueryRow(freshCtx, `SELECT COUNT(*) FROM admin_notifications WHERE reason = 'critical_payment_log' AND user_id = $1`, userID).Scan(¬ifCount); err != nil {
+ t.Fatalf("failed to count admin notifications: %v", err)
+ }
+ if notifCount < 1 {
+ t.Errorf("expected a critical-payment admin notification for the attempt-capped row, got %d", notifCount)
+ }
+}
+
+// TestSweepStalePendingPayments_KeyedReplay_NoReplayWithFailedRefundRow locks
+// the FAILED-webhook demotion path (finding d): a B1 auto-refund that was
+// accepted PENDING and later demoted to 'failed' by the refund.updated webhook
+// clears the in-flight guard — the sweep must then see the FAILED refunds row
+// and fail the parent WITHOUT ever re-replaying the expired key. The
+// failOnReplayClient proves the replay never happens.
+func TestSweepStalePendingPayments_KeyedReplay_NoReplayWithFailedRefundRow(t *testing.T) {
+ ctx, tx := testutils.SetupTestTx(t)
+
+ userID, err := fixtures.CreateTestUser(tx)
+ if err != nil {
+ t.Fatalf("failed to create user: %v", err)
+ }
+ serviceID, err := fixtures.CreateTestService(tx)
+ if err != nil {
+ t.Fatalf("failed to create service: %v", err)
+ }
+ bookingID, err := fixtures.CreateTestBookingAtTime(tx, userID, serviceID,
+ time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC))
+ if err != nil {
+ t.Fatalf("failed to create booking: %v", err)
+ }
+
+ staleID, err := fixtures.CreateTestPayment(tx, bookingID, 2000.00, "online_square", "full", "pending")
+ if err != nil {
+ t.Fatalf("failed to create stale pending payment: %v", err)
+ }
+ const key = "key-expired-replay-failed-refund"
+ if _, err := tx.Exec(ctx, "UPDATE payments SET created_at = NOW() - INTERVAL '23 hours', idempotency_key = $1, square_source_id = 'ccof:test-saved-card', created_by = $2, b1_attempts = 1 WHERE id = $3", key, userID, staleID); err != nil {
+ t.Fatalf("failed to age the stale payment: %v", err)
+ }
+ // The webhook-demoted B1 refund row: accepted PENDING, then FAILED at Square.
+ const dupPayID = "pay_demoted_failed_duplicate"
+ if _, err := tx.Exec(ctx, `
+ INSERT INTO refunds (payment_id, booking_id, amount, square_refund_id, status, origin, reason, idempotency_key, created_by, created_at)
+ VALUES ($1, $2, 2000.00, 'ref_sweep_demoted_failed', 'failed', 'manual', $3, 'sweepdup-' || $4, $5, NOW())
+ `, staleID, bookingID, sweepDuplicateRefundReason, dupPayID, userID); err != nil {
+ t.Fatalf("failed to insert the demoted failed refund row: %v", err)
+ }
+
+ origClient := SquareClient
+ SquareClient = &failOnReplayClient{SquareClient: square.NewDevClient(), t: t, key: key}
+ defer func() { SquareClient = origClient }()
+
+ pgxTx := db.TxFromContext(ctx)
+ if pgxTx == nil {
+ t.Fatal("no transaction in context")
+ }
+ if err := pgxTx.Commit(ctx); err != nil {
+ t.Fatalf("failed to commit test tx: %v", err)
+ }
+
+ t.Cleanup(func() {
+ _, _ = db.Conn.Exec(context.Background(), `DELETE FROM refunds WHERE payment_id = $1`, staleID)
+ _, _ = db.Conn.Exec(context.Background(), `DELETE FROM admin_notifications WHERE reason = 'critical_payment_log' AND user_id = $1`, userID)
+ _, _ = 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 string
+ if err := db.Conn.QueryRow(freshCtx, "SELECT status FROM payments WHERE id = $1", staleID).Scan(&status); err != nil {
+ t.Fatalf("failed to query payment: %v", err)
+ }
+ if status != "failed" {
+ t.Errorf("expected a row with a FAILED B1 refund to be failed without a replay, got %q", status)
+ }
+
+ var notifCount int
+ if err := db.Conn.QueryRow(freshCtx, `SELECT COUNT(*) FROM admin_notifications WHERE reason = 'critical_payment_log' AND user_id = $1`, userID).Scan(¬ifCount); err != nil {
+ t.Fatalf("failed to count admin notifications: %v", err)
+ }
+ if notifCount < 1 {
+ t.Errorf("expected a critical-payment admin notification for the failed-B1-refund row, got %d", notifCount)
+ }
+}
+
// TestSweepStalePendingPayments_KeyedReplayUnparseableCreatedAt_LeavesPending
// locks the B1 caveat: a replayed COMPLETED payment whose created_at CANNOT be
// parsed must NOT be auto-refunded. An unparseable created_at does not prove
diff --git a/backend/handlers/payments/twofa.go b/backend/handlers/payments/twofa.go
index 31ce3c0..93b50c0 100644
--- a/backend/handlers/payments/twofa.go
+++ b/backend/handlers/payments/twofa.go
@@ -308,13 +308,31 @@ func reissueTwoFACodeAfterFailedCharge(ctx context.Context, q db.Querier, userID
// knows the customer is blocked and can mint a code manually or fix the
// config (TWO_FACTOR_PEPPER / delivery channel).
log.Printf("CRITICAL: failed to re-issue a 2FA code for user %s after a failed saved-card charge (%v) — the customer's code was consumed by the fresh charge and NO live code remains, so the same-key retry cannot succeed; the operator must mint a code manually or configure TWO_FACTOR_PEPPER and a 2FA delivery channel", userID, err)
+ // Finding 1 (Round 2 Loop A): bound the operator-facing flood. The
+ // deduped insert (sweep.go's insertCriticalPaymentNotification, keyed on
+ // reason/booking_id/user_id) is unbounded across attacker-registered
+ // accounts, so a hostile flood of failed re-issues could bury the
+ // single-operator notification centre. The global cap skips the insert —
+ // the CRITICAL log line above still fires, so no alert information is
+ // lost to the operator's log pipeline — once
+ // maxUnacknowledgedNotifications unacknowledged 'critical_payment_log'
+ // rows exist. Acknowledging rows re-arms inserts.
+ if notificationsCapExceeded(ctx, "critical_payment_log") {
+ log.Printf("2FA: critical-payment admin notification suppressed for user %s — %d unacknowledged 'critical_payment_log' notifications already exist; acknowledge outstanding notifications to re-arm", userID, maxUnacknowledgedNotifications)
+ return
+ }
insertCriticalPaymentNotification(ctx, nil, &userID)
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.
+ // Mint cooldown (B11a + Round 2 Loop A finding 2): 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 NO LONGER clears the
+ // stamp (internal/twofa.Check keeps it — it is cleared only at terminal
+ // charge success via twofa.ConsumePendingCode), so this check now genuinely
+ // bounds a charge-failure loop: the first re-issue after a mint is skipped
+ // while clock.Now().Sub(LastMintAt) < twoFAMintCooldown, bounding code churn
+ // and dev log flooding. The customer requests a fresh code through the
+ // normal mint endpoint once the window elapses.
st := twofa.StateFor(userID)
st.Mu.Lock()
defer st.Mu.Unlock()
@@ -350,6 +368,21 @@ func reissueTwoFACodeAfterFailedCharge(ctx context.Context, q db.Querier, userID
// 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.
+//
+// Round 2 Loop A finding 2: the stamp survives a successful gate verify
+// (internal/twofa.Check no longer clears it) and is cleared only at TERMINAL
+// charge success via twofa.ConsumePendingCode — so this check below is what
+// actually bounds a charge-failure loop: after a fresh charge consumed a code
+// at the gate and failed, the re-issue is skipped while the last mint is
+// inside the cooldown (logged, not silent), bounding code churn + dev log
+// flooding. COORDINATION (money agent): the FRESH-charge terminal-success path
+// in handlers.go does NOT call ConsumePendingCode (the gate already burned the
+// code with consume=true), so its stamp survives — a customer who completes a
+// fresh charge within the cooldown of their last mint and immediately requests
+// a new code gets 429 until the window elapses. That is the intended bounded
+// behaviour; if immediate re-mint after a completed fresh charge is wanted,
+// the money agent should clear the stamp there (twofa.ClearMintCooldownForUser
+// or ConsumePendingCode) on terminal success.
const twoFAMintCooldown = 1 * time.Minute
// generatePaymentsTwoFACode returns a random 6-digit verification code,
@@ -365,3 +398,55 @@ func generatePaymentsTwoFACode() (string, error) {
// twoFAPendingCodeLifetime is how long a re-issued 2FA code stays valid,
// mirroring the user package's pending-code expiry.
const twoFAPendingCodeLifetime = 10 * time.Minute
+
+// maxUnacknowledgedNotifications is the GLOBAL cap on unacknowledged
+// admin_notifications rows for one reason (Round 2 Loop A finding 1). The
+// dedup guards keyed on (reason, booking_id, user_id) are bounded per issue but
+// UNBOUNDED across attacker-registered accounts, so a hostile flood could bury
+// the single-operator notification centre. Insert sites check
+// notificationsCapExceeded before inserting and skip the row (the CRITICAL log
+// line still fires) once the unacknowledged queue for that reason is at the
+// cap — the operator acknowledges rows to re-arm.
+const maxUnacknowledgedNotifications = 100
+
+// notificationsCapExceeded reports whether the number of unacknowledged
+// admin_notifications rows for reason has reached maxUnacknowledgedNotifications.
+// Best-effort and fail-OPEN: a count error is logged and the cap is NOT
+// enforced (a money alert must never be dropped because the count query failed).
+func notificationsCapExceeded(ctx context.Context, reason string) bool {
+ var n int
+ err := db.Conn.QueryRow(ctx, `
+ SELECT COUNT(*) FROM admin_notifications
+ WHERE reason = $1::admin_notification_reason AND acknowledged_at IS NULL
+ `, reason).Scan(&n)
+ if err != nil {
+ log.Printf("2FA: failed to count unacknowledged %s admin notifications: %v", reason, err)
+ return false
+ }
+ return n >= maxUnacknowledgedNotifications
+}
+
+// COORDINATION NOTE (Round 2 Loop A finding 1) — the cap pattern must be
+// applied by the other two insert sites this finding calls out, which live in
+// files owned by other agents:
+//
+// - handlers/payments/sweep.go:1414 insertCriticalPaymentNotification (the
+// money agent): its INSERT ... WHERE NOT EXISTS dedup is keyed on (reason,
+// booking_id, user_id) and is the same unbounded-across-accounts shape.
+// Fold the global guard into the SELECT: `AND (SELECT COUNT(*) FROM
+// admin_notifications WHERE reason = 'critical_payment_log' AND
+// acknowledged_at IS NULL) < 100`.
+//
+// - auth/jwt.go:666 VerifyRefreshToken's 'refresh_token_reuse' alert (the
+// auth agent): dedups on (reason, user_id) only, so a single attacker
+// replaying MANY rotated families can flood the same single-operator
+// centre. Apply the identical cap for reason 'refresh_token_reuse' (its
+// insert is also `INSERT ... SELECT ... WHERE NOT EXISTS`, so the same
+// COUNT subquery folds in).
+//
+// This helper lives in payments/twofa.go because that is where this task's
+// reissue-fail alert (reissueTwoFACodeAfterFailedCharge, above) applies it; the
+// other sites copy the pattern since the auth/jobs packages cannot import
+// payments. Fail-closed behaviour (a REFUSED re-issue still CRITICAL-logs and
+// leaves the operator to mint manually) is unchanged — only the notification
+// INSERT is bounded.
diff --git a/backend/handlers/payments/twofa_gate_consume_test.go b/backend/handlers/payments/twofa_gate_consume_test.go
index d7f0f8a..8296477 100644
--- a/backend/handlers/payments/twofa_gate_consume_test.go
+++ b/backend/handlers/payments/twofa_gate_consume_test.go
@@ -233,6 +233,85 @@ func TestTwoFactorEnforced_BuyGiftCard_SavedCard_PendingReuse_Failure_KeepsCode(
require.Equal(t, twofa.Hash("334455"), hash.String, "a failed pending-reuse retry must keep its seeded code unchanged (no re-issue)")
}
+// TestTwoFactorEnforced_CreateBookingPayment_NewCardSaveCard_Failure_Reissues
+// pins the save-gate code-burn re-issue for the BOOKING path: a NEW-card
+// (cnon) charge with save_card=true passes the SAVE gate (consume=true — the
+// single-use code is burned there), so when the subsequent Square charge is
+// definitively declined the re-issue guard must fire (req.SaveCard) and mint a
+// fresh code — otherwise every same-key retry hits "Verification code expired"
+// forever (finding: 2FA code burned by the SAVE gate never re-issued). The
+// stored hash must differ from the seeded one: if the re-issue did not run the
+// gate's consumption would leave no pending code at all.
+func TestTwoFactorEnforced_CreateBookingPayment_NewCardSaveCard_Failure_Reissues(t *testing.T) {
+ t.Setenv("REQUIRE_2FA", "true")
+ t.Setenv("SQUARE_ENVIRONMENT", "staging")
+ ctx, tx := testutils.SetupTestTx(t)
+
+ userID, bookingID, _ := setupTestData(t, ctx, tx)
+ userToken := jwt.GenerateUserToken(userID)
+ seedTwoFAPendingCode(t, tx, userID, "999001")
+
+ origClient := SquareClient
+ SquareClient = &definitiveChargeClient{SquareClient: square.NewDevClient(), createErr: structuredSquareErrorWithCode(t, http.StatusPaymentRequired, "CARD_DECLINED")}
+ defer func() { SquareClient = origClient }()
+
+ cardToken := "cnon:2fa-booking-newcard-savecard-decline"
+ req := CreateBookingPaymentRequest{
+ Amount: 2500,
+ PaymentType: "deposit",
+ NewCardToken: &cardToken,
+ SaveCard: true,
+ IdempotencyKey: "2fa-booking-newcard-savecard-decline",
+ VerificationCode: "999001",
+ }
+
+ w := makePaymentRequest(withNonGuest(CreateBookingPayment), "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx)
+ require.Equal(t, http.StatusPaymentRequired, w.Code, w.Body.String())
+
+ var hash sql.NullString
+ require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_pending_code_hash FROM users WHERE id = $1", userID).Scan(&hash))
+ require.True(t, hash.Valid, "a failed new-card + save_card booking charge must re-issue a live 2FA code for the same-key retry")
+ require.NotEqual(t, twofa.Hash("999001"), hash.String, "the SAVE gate consumed the seeded code at verification time — the failure must re-issue a fresh one")
+}
+
+// TestTwoFactorEnforced_CreateTipPayment_NewCardSaveCard_Failure_Reissues pins
+// the same save-gate code-burn re-issue for the TIP path (mirrors the booking
+// test above): a NEW-card tip with save_card=true burns its code at the SAVE
+// gate, so a definitively declined charge must re-issue a fresh code for the
+// same-key retry.
+func TestTwoFactorEnforced_CreateTipPayment_NewCardSaveCard_Failure_Reissues(t *testing.T) {
+ t.Setenv("REQUIRE_2FA", "true")
+ t.Setenv("SQUARE_ENVIRONMENT", "staging")
+ ctx, tx := testutils.SetupTestTx(t)
+
+ userID, bookingID, _ := setupTestDataPast(t, ctx, tx)
+ userToken := jwt.GenerateUserToken(userID)
+ seedTwoFAPendingCode(t, tx, userID, "999002")
+ _, err := fixtures.CreateTestPayment(tx, bookingID, 5000.00, "online_square", "full", "completed")
+ require.NoError(t, err)
+
+ origClient := SquareClient
+ SquareClient = &definitiveChargeClient{SquareClient: square.NewDevClient(), createErr: structuredSquareErrorWithCode(t, http.StatusPaymentRequired, "CARD_DECLINED")}
+ defer func() { SquareClient = origClient }()
+
+ cardToken := "cnon:2fa-tip-newcard-savecard-decline"
+ req := CreateTipPaymentRequest{
+ Amount: 500,
+ NewCardToken: &cardToken,
+ SaveCard: true,
+ IdempotencyKey: "2fa-tip-newcard-savecard-decline",
+ VerificationCode: "999002",
+ }
+
+ w := makePaymentRequest(withNonGuest(CreateTipPayment), "POST", "/api/bookings/"+bookingID+"/tip", req, userToken, ctx)
+ require.Equal(t, http.StatusPaymentRequired, w.Code, w.Body.String())
+
+ var hash sql.NullString
+ require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_pending_code_hash FROM users WHERE id = $1", userID).Scan(&hash))
+ require.True(t, hash.Valid, "a failed new-card + save_card tip charge must re-issue a live 2FA code for the same-key retry")
+ require.NotEqual(t, twofa.Hash("999002"), hash.String, "the SAVE gate consumed the seeded code at verification time — the failure must re-issue a fresh one")
+}
+
// 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
diff --git a/backend/handlers/payments/twofa_test.go b/backend/handlers/payments/twofa_test.go
index 0760d2a..6896e84 100644
--- a/backend/handlers/payments/twofa_test.go
+++ b/backend/handlers/payments/twofa_test.go
@@ -832,3 +832,44 @@ func TestTwoFactorFallbackEnabled(t *testing.T) {
func TestTwoFADeliveryAvailable_DevBuild_TriviallyTrue(t *testing.T) {
require.True(t, twoFADeliveryAvailable())
}
+
+// TestNotificationsCapExceeded pins finding 1: the GLOBAL cap on unacknowledged
+// 'critical_payment_log' admin notifications (maxUnacknowledgedNotifications)
+// suppresses new inserts once the unacknowledged queue reaches the cap, so a
+// hostile flood of attacker-registered accounts cannot bury the single-operator
+// notification centre. Acknowledging rows re-arms inserts. The reissue-fail
+// alert site (twofa.go) checks this helper before calling the sweep's
+// insertCriticalPaymentNotification; the same pattern is documented for the
+// sweep.go and auth/jwt.go insert sites (see the coordination note in twofa.go).
+func TestNotificationsCapExceeded(t *testing.T) {
+ ctx, tx := testutils.SetupTestTx(t)
+ userID, err := fixtures.CreateTestUser(tx)
+ require.NoError(t, err)
+
+ // Empty queue → below the cap, inserts allowed.
+ require.False(t, notificationsCapExceeded(ctx, "critical_payment_log"))
+
+ // Fill the unacknowledged queue to the cap.
+ _, err = tx.Exec(ctx, `DELETE FROM admin_notifications WHERE reason = 'critical_payment_log'`)
+ require.NoError(t, err)
+ for i := 0; i < maxUnacknowledgedNotifications; i++ {
+ _, err = tx.Exec(ctx, `
+ INSERT INTO admin_notifications (reason, user_id, created_at)
+ VALUES ('critical_payment_log', $1, NOW())
+ `, userID)
+ require.NoError(t, err)
+ }
+ require.True(t, notificationsCapExceeded(ctx, "critical_payment_log"), "at the cap the insert must be suppressed")
+
+ // Acknowledging one row drops below the cap → inserts re-arm.
+ _, err = tx.Exec(ctx, `
+ UPDATE admin_notifications SET acknowledged_at = NOW()
+ WHERE ctid = (
+ SELECT ctid FROM admin_notifications
+ WHERE reason = 'critical_payment_log' AND acknowledged_at IS NULL
+ LIMIT 1
+ )
+ `)
+ require.NoError(t, err)
+ require.False(t, notificationsCapExceeded(ctx, "critical_payment_log"), "acknowledging one row must re-arm inserts")
+}
diff --git a/backend/handlers/user/twofa_test.go b/backend/handlers/user/twofa_test.go
index bb16029..2f1201b 100644
--- a/backend/handlers/user/twofa_test.go
+++ b/backend/handlers/user/twofa_test.go
@@ -955,11 +955,13 @@ func TestTwoFAAttemptMap_InLockoutRecordNotEvicted(t *testing.T) {
}
}
-// TestTwoFAAttemptMap_FullOfLockedOut_ReturnsTransient verifies the pathological
-// case: when every entry is a locked-out in-window record (a flood), the map
-// does NOT evict one and does NOT grow past the cap — the new user gets a
-// transient, untracked state for this request instead.
-func TestTwoFAAttemptMap_FullOfLockedOut_ReturnsTransient(t *testing.T) {
+// TestTwoFAAttemptMap_FullOfLockedOut_ReturnsPermanentlyLocked verifies the
+// pathological case: when every entry is a locked-out in-window record (a
+// flood), the map does NOT evict one and does NOT grow past the cap — the new
+// user gets the SHARED permanently-locked state instead of a transient state
+// with a fresh guessing budget (Round 2 Loop A finding 3: the old per-request
+// transient silently disabled the 5-attempt lockout exactly under attack).
+func TestTwoFAAttemptMap_FullOfLockedOut_ReturnsPermanentlyLocked(t *testing.T) {
twofa.MapMu.Lock()
origMap := twofa.Map
origCap := twofa.MaxTrackedAttempts
@@ -983,8 +985,9 @@ func TestTwoFAAttemptMap_FullOfLockedOut_ReturnsTransient(t *testing.T) {
st := twoFAAttemptStateFor("new_user")
require.NotNil(t, st)
+ require.True(t, st.LockedOut(clock.Now()), "an untracked user under map saturation must be treated as permanently locked out")
if _, ok := twofa.Map["new_user"]; ok {
- t.Error("expected the transient state NOT to be stored when the map is full of in-lockout records")
+ t.Error("expected the shared permanently-locked state NOT to be stored when the map is full of in-lockout records")
}
if len(twofa.Map) != 3 {
t.Errorf("expected all 3 locked-out records to survive, got %d", len(twofa.Map))
@@ -1099,7 +1102,13 @@ func TestTwoFA_ConcurrentCheckTwoFACode_NoDeadlock(t *testing.T) {
origMap := twofa.Map
origCap := twofa.MaxTrackedAttempts
twofa.Map = make(map[string]*twoFAAttemptState)
- twofa.MaxTrackedAttempts = 64
+ // Cap high enough that the ~90 locked-out users this test accumulates
+ // (6 workers × 15 iters) never saturate the map: under saturation the
+ // shared permanently-locked fallback (finding 3) would hand fresh users a
+ // pre-locked state and every "attempt 1" would wrongly report locked out.
+ // Saturation itself is covered by
+ // TestTwoFAAttemptMap_FullOfLockedOut_ReturnsPermanentlyLocked.
+ twofa.MaxTrackedAttempts = 512
twofa.MapMu.Unlock()
t.Cleanup(func() {
twofa.MapMu.Lock()
diff --git a/backend/handlers/webhooks/square.go b/backend/handlers/webhooks/square.go
index 8757f77..572162d 100644
--- a/backend/handlers/webhooks/square.go
+++ b/backend/handlers/webhooks/square.go
@@ -232,6 +232,15 @@ func HandleSquareWebhook(w http.ResponseWriter, r *http.Request) {
signingKey := os.Getenv("SQUARE_WEBHOOK_SIGNATURE_KEY")
notificationURL := os.Getenv("SQUARE_WEBHOOK_NOTIFICATION_URL")
if notificationURL == "" {
+ // Fail-closed fallback (Round 2 Loop A finding 6): an unset URL falls
+ // back to the public dev default so the handler always has a string to
+ // HMAC against. The subscription is fail-closed — the key AND the URL
+ // must exactly match the Square Dashboard configuration, so with the
+ // URL unset every GENUINE Square event fails signature verification
+ // here (403) and no event is ever processed; only the operator can fix
+ // the config. main.go's startup check (checkWebhookSignatureKey) warns
+ // when the key is set but the URL is unset — the reverse
+ // misconfiguration — so the breakage is visible at boot, not silent.
notificationURL = "http://localhost:8080/webhooks/square"
}
if signingKey == "" {
@@ -551,26 +560,6 @@ func squarePaymentStatusToLocal(status string) (string, bool) {
}
}
-// squareRefundStatusToLocal maps Square's refund status to the local
-// payment_status enum. Square's PaymentRefund states are PENDING, APPROVED,
-// COMPLETED, CANCELED, FAILED and REJECTED (developer.squareup.com/reference/
-// square/objects/PaymentRefund). COMPLETED/FAILED/REJECTED are TERMINAL —
-// REJECTED (Square declined the refund) is a definitive failure and must be
-// surfaced as local 'failed' instead of leaving the row pending until the slow
-// sweep; PENDING and APPROVED are NON-terminal (the refund may still complete
-// or be rejected) and map to a zero local status so the caller leaves the row
-// untouched.
-func squareRefundStatusToLocal(status string) (string, bool) {
- switch status {
- case "COMPLETED":
- return "completed", true
- case "FAILED", "REJECTED":
- return "failed", true
- default:
- return "", false
- }
-}
-
// squareDisputeStateToLocal maps Square's dispute state to the local
// disputes.status. Only the terminal resolutions move the row to won/lost;
// ACCEPTED (seller accepted the dispute) is a loss — the money is gone.
@@ -1159,7 +1148,9 @@ func handleRefundUpdated(data json.RawMessage) error {
// A refund object is present but unusable: cannot apply money state — retry.
return fmt.Errorf("refund.updated payload for data.id=%q missing/invalid refund object (id=%q status=%q): %w", env.ID, refund.ID, refund.Status, errWebhookParseFailure)
}
- localStatus, terminal := squareRefundStatusToLocal(refund.Status)
+ // Single shared Square → local refund-status mapping (payments package) so
+ // the webhook and the synchronous refund handlers can never drift.
+ localStatus, terminal := payments.SquareRefundStatusToLocal(refund.Status)
if !terminal {
log.Printf("[SQUARE-WEBHOOK] refund.updated: square refund %s status %q is non-terminal — no local state change", refund.ID, refund.Status)
return nil
diff --git a/backend/internal/dav/service_prod.go b/backend/internal/dav/service_prod.go
index 2716420..5857984 100644
--- a/backend/internal/dav/service_prod.go
+++ b/backend/internal/dav/service_prod.go
@@ -12,6 +12,10 @@ import (
var Service *BaseService
+// defaultPostgresHost is the fallback Postgres address used when
+// POSTGRES_HOST is unset — the pipeline/CI postgres service default.
+const defaultPostgresHost = "127.0.0.1"
+
func init() {
// Test builds wire their own pool in TestMain (testutils/testdb) — a real
// connect here would race that and panic a bare-shell `go test`. Skip.
@@ -26,7 +30,7 @@ func init() {
func connect() error {
host := getEnv("POSTGRES_HOST")
if host == "" {
- host = "127.0.0.1" // pipeline postgres service default
+ host = defaultPostgresHost
}
dsn := fmt.Sprintf(
"postgres://%s:%s@%s:5432/%s?timezone=UTC&require_auth=scram-sha-256",
diff --git a/backend/internal/twofa/twofa.go b/backend/internal/twofa/twofa.go
index 5b841ed..112ce7f 100644
--- a/backend/internal/twofa/twofa.go
+++ b/backend/internal/twofa/twofa.go
@@ -157,6 +157,26 @@ var (
Map = make(map[string]*AttemptState)
)
+// saturatedLockedState is the SHARED attempt state returned by StateFor when
+// the attempt map is at capacity and every tracked record is inside its
+// lockout window (finding 3, Round 2 Loop A — see StateFor). Its last-activity
+// stamp is pinned FAR in the future, so LockedOut always holds and Check's
+// window-reset branch (now.Sub(LastActive) > AttemptWindow) can never reach it:
+// every untracked user is treated as PERMANENTLY locked out instead of being
+// granted a fresh 5-guess budget per request. It is a package-level singleton
+// rather than a per-call allocation so the pathological path allocates nothing
+// and all saturated requests share one record.
+var saturatedLockedState = newSaturatedLockedState()
+
+func newSaturatedLockedState() *AttemptState {
+ st := &AttemptState{}
+ st.Count.Store(MaxAttempts)
+ // Pinned so far in the future that now.Sub(LastActive) is always
+ // <= AttemptWindow (LockedOut true) and never > AttemptWindow (no reset).
+ st.SetLastActive(time.Now().Add(24 * 365 * 24 * time.Hour))
+ return st
+}
+
// StateFor returns the per-user attempt state, creating it if needed. The map
// is bounded: stale (window-expired) entries are evicted opportunistically and,
// when at capacity, the least-recently-active non-locked-out entry is dropped.
@@ -194,11 +214,17 @@ func StateFor(userID string) *AttemptState {
if len(Map) >= MaxTrackedAttempts {
// Every entry is a locked-out in-window record. Do not evict one
// (that would reset its rate limit) and do not grow past the cap:
- // return a transient, untracked state so THIS request still
- // proceeds under a fresh budget.
- st := &AttemptState{}
- st.SetLastActive(now)
- return st
+ // return the SHARED permanently-locked state (finding 3, Round 2
+ // Loop A). Previously a fresh transient state was returned per
+ // call, so every untracked user received a fresh 5-guess budget
+ // per request — silently disabling the brute-force lockout exactly
+ // under the hostile flood that saturated the map. The shared state
+ // treats every untracked user as locked out instead. It is never
+ // stored in Map (so the eviction scan / ResetAttempts /
+ // DeleteAttempts never touch it) and self-heals: as soon as one of
+ // the real locked-out records lapses out of its window, StateFor
+ // evicts it and normal per-user tracking resumes.
+ return saturatedLockedState
}
}
@@ -377,11 +403,20 @@ func Check(ctx context.Context, userID string, st *AttemptState, reqCode string,
log.Printf("failed to upgrade legacy 2FA pending code hash for user %s: %v", userID, err)
}
}
- // Success: clear the attempt counter (and any mint cooldown) before the
- // caller performs its action.
+ // Success: clear the attempt counter before the caller performs its
+ // action. The mint-cooldown stamp (LastMintAt) is deliberately NOT cleared
+ // here (Round 2 Loop A finding 2): a code verified at the saved-card gate
+ // may still be followed by a FAILED Square charge that re-issues a fresh
+ // code (payments.reissueTwoFACodeAfterFailedCharge), and that re-issue path
+ // enforces the per-user mint cooldown against this stamp. Clearing it on a
+ // gate-verify let a charge-failure loop mint a fresh code on every
+ // iteration with no 60s cooldown (code churn + dev log flooding). The stamp
+ // is cleared only at a TERMINAL SUCCESS — the completed-charge consumption
+ // path (ConsumePendingCode, called by the money agent inside the
+ // transaction that records the completed charge) — so a customer who just
+ // completed a charge can immediately request a fresh code.
st.Count.Store(0)
st.SetLastActive(clock.Now())
- st.LastMintAt = time.Time{}
ResetAttempts(userID)
// LOW 6b: a correct code proves control of the account's second factor, so
// lift any password-guessing login lockout (users.failed_attempts /
@@ -417,7 +452,8 @@ 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.
+// ConsumePendingCode NULLs the user's pending 2FA code digest and expiry, and
+// clears the per-user mint-cooldown stamp (AttemptState.LastMintAt).
// 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
@@ -428,6 +464,13 @@ func Check(ctx context.Context, userID string, st *AttemptState, reqCode string,
// 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.
+//
+// Round 2 Loop A finding 2: this is the ONLY place the mint-cooldown stamp is
+// cleared on the charge path. A successful gate VERIFY (twofa.Check) must NOT
+// clear it — the charge may still fail and the re-issue path
+// (payments.reissueTwoFACodeAfterFailedCharge) enforces its cooldown against
+// the stamp. Reaching terminal SUCCESS is what re-arms immediate re-minting,
+// so consumption (which runs only at that terminal state) clears it.
func ConsumePendingCode(ctx context.Context, q db.Querier, userID string) error {
if userID == "" {
return nil
@@ -441,9 +484,26 @@ func ConsumePendingCode(ctx context.Context, q db.Querier, userID string) error
if err != nil {
return fmt.Errorf("2FA consume pending code: %w", err)
}
+ // Clear the mint-cooldown stamp. Best-effort and in-memory: a missing or
+ // evicted entry (e.g. after a process restart) only lets the cooldown
+ // lapse — it never grants a fresh guessing budget.
+ ClearMintCooldownForUser(userID)
return nil
}
+// ClearMintCooldownForUser zeroes the user's mint-cooldown stamp (LastMintAt)
+// under the per-user mutex — LastMintAt is only ever touched under Mu. Exported
+// so the payments re-issue path's coordination contract is actionable (Round 2
+// Loop A finding 2): a FRESH-charge terminal-success path that consumed the
+// code at the gate (consume=true, so ConsumePendingCode is not called) can call
+// this to re-arm immediate re-minting after a completed charge.
+func ClearMintCooldownForUser(userID string) {
+ st := StateFor(userID)
+ st.Mu.Lock()
+ st.LastMintAt = time.Time{}
+ st.Mu.Unlock()
+}
+
// Classifying errors returned by VerifyForUser.
var (
// ErrIncorrect reports a code that does not match the user's pending code.
diff --git a/backend/internal/twofa/twofa_test.go b/backend/internal/twofa/twofa_test.go
index 8ac53e9..a1e56dc 100644
--- a/backend/internal/twofa/twofa_test.go
+++ b/backend/internal/twofa/twofa_test.go
@@ -144,6 +144,9 @@ func TestConsumePendingCode(t *testing.T) {
// TestVerifyForUser_AttemptStateMapPersists exercises the shared per-user
// attempt map directly (the state the payments gate shares with the interactive
// endpoints): the map is bounded and a locked-out record is never evicted.
+// Round 2 Loop A finding 3: when the map is full of in-window locked-out
+// records, a new untracked user gets the SHARED permanently-locked state —
+// treated as locked out, not handed a fresh 5-guess budget per request.
func TestVerifyForUser_AttemptStateMapPersists(t *testing.T) {
t.Cleanup(func() {
MapMu.Lock()
@@ -165,8 +168,58 @@ func TestVerifyForUser_AttemptStateMapPersists(t *testing.T) {
st.Count.Store(MaxAttempts)
Map[id] = st
}
- _ = StateFor("new_user") // transient, untracked (map full of lockouts)
+ st := StateFor("new_user") // saturated — shared permanently-locked state
+ require.True(t, st.LockedOut(clock.Now()), "an untracked user under map saturation must be treated as locked out")
MapMu.Lock()
defer MapMu.Unlock()
require.Len(t, Map, 2, "locked-out records must survive the cap pressure")
}
+
+// TestVerifyForUser_SuccessPreservesMintCooldownStamp pins Round 2 Loop A
+// finding 2: a successful verify must NOT clear the per-user mint-cooldown
+// stamp (LastMintAt), so the payments re-issue path
+// (reissueTwoFACodeAfterFailedCharge) can enforce its 60s cooldown against a
+// charge-failure loop. Previously Check cleared the stamp on every verify,
+// letting a fresh charge that failed at Square mint a new code per iteration
+// with no cooldown. The stamp is cleared only at terminal success via
+// ConsumePendingCode (see TestConsumePendingCode_ClearsMintCooldownStamp).
+func TestVerifyForUser_SuccessPreservesMintCooldownStamp(t *testing.T) {
+ ctx, tx := testutils.SetupTestTx(t)
+ userID, err := fixtures.CreateTestUser(tx)
+ require.NoError(t, err)
+ seedPending(t, ctx, tx, userID, "123456")
+
+ st := StateFor(userID)
+ st.Mu.Lock()
+ st.LastMintAt = clock.Now().Add(-10 * time.Second)
+ st.Mu.Unlock()
+
+ require.NoError(t, VerifyForUser(ctx, userID, "123456", DeferredConsume), "correct code must verify")
+
+ st.Mu.Lock()
+ defer st.Mu.Unlock()
+ require.False(t, st.LastMintAt.IsZero(), "a successful verify must preserve the mint-cooldown stamp (finding 2)")
+}
+
+// TestConsumePendingCode_ClearsMintCooldownStamp pins the other half of finding
+// 2: the mint-cooldown stamp is cleared at TERMINAL SUCCESS — the completed-
+// charge consumption path — so a customer who just completed a charge can
+// immediately request a fresh code. This is the only charge-path place the
+// stamp dies.
+func TestConsumePendingCode_ClearsMintCooldownStamp(t *testing.T) {
+ ctx, tx := testutils.SetupTestTx(t)
+ userID, err := fixtures.CreateTestUser(tx)
+ require.NoError(t, err)
+ seedPending(t, ctx, tx, userID, "123456")
+
+ st := StateFor(userID)
+ st.Mu.Lock()
+ st.LastMintAt = clock.Now().Add(-10 * time.Second)
+ st.Mu.Unlock()
+
+ require.NoError(t, ConsumePendingCode(ctx, tx, userID), "terminal-success consumption must succeed")
+
+ st.Mu.Lock()
+ defer st.Mu.Unlock()
+ require.True(t, st.LastMintAt.IsZero(), "terminal-success consumption must clear the mint-cooldown stamp (finding 2)")
+}
diff --git a/backend/main.go b/backend/main.go
index 78193cb..ef5311c 100644
--- a/backend/main.go
+++ b/backend/main.go
@@ -299,17 +299,32 @@ func checkProxyRateLimitConfig() {
// rejected would strand payment reconciliation. When the URL is also unset
// (webhooks not in use — the documented optional setup) a loud warning is
// logged instead, so the fail-fast never breaks webhook-less deployments.
+//
+// Round 2 Loop A finding 6 (the reverse misconfiguration): when the KEY is set
+// but the URL is unset, the handler falls back to the public default
+// http://localhost:8080/webhooks/square (webhooks/square.go) and HMAC
+// verification runs against that string, so every GENUINE Square event fails
+// signature verification (403) and payment/refund reconciliation is silently
+// broken. The key signals intent to use webhooks; the missing URL breaks them.
+// This is WARN not FATAL: the handler is fail-closed, so no event — genuine or
+// forged — can mutate state, making it availability-only (like
+// checkProxyRateLimitConfig), not a security hole.
func checkWebhookSignatureKey() {
if payments.IsExplicitDevOrMockEnv() {
return
}
- if os.Getenv("SQUARE_WEBHOOK_SIGNATURE_KEY") != "" {
+ keySet := os.Getenv("SQUARE_WEBHOOK_SIGNATURE_KEY") != ""
+ urlSet := os.Getenv("SQUARE_WEBHOOK_NOTIFICATION_URL") != ""
+ switch {
+ case keySet && urlSet:
return
- }
- if os.Getenv("SQUARE_WEBHOOK_NOTIFICATION_URL") != "" {
+ case keySet && !urlSet:
+ log.Printf("WARNING: SQUARE_WEBHOOK_SIGNATURE_KEY IS set but SQUARE_WEBHOOK_NOTIFICATION_URL is unset with SQUARE_ENVIRONMENT=%q (non-mock) — the webhook handler falls back to the default http://localhost:8080/webhooks/square, so every GENUINE Square event fails signature verification (403, fail-closed) and payment/refund reconciliation is silently broken. Set SQUARE_WEBHOOK_NOTIFICATION_URL to exactly the notification URL configured in the Square Dashboard webhook subscription.", os.Getenv("SQUARE_ENVIRONMENT"))
+ case !keySet && urlSet:
log.Fatalf("FATAL: SQUARE_WEBHOOK_SIGNATURE_KEY environment variable not set with SQUARE_ENVIRONMENT=%q (non-mock) while SQUARE_WEBHOOK_NOTIFICATION_URL IS set — Square webhook events (payment/refund reconciliation) would be rejected fail-closed at runtime. Generate the signing key in the Square Dashboard webhook subscription and set it in .env.", os.Getenv("SQUARE_ENVIRONMENT"))
+ default:
+ log.Printf("CRITICAL: SQUARE_WEBHOOK_SIGNATURE_KEY is not set with SQUARE_ENVIRONMENT=%q (non-mock) and SQUARE_WEBHOOK_NOTIFICATION_URL is unset — Square webhooks are not configured; any webhook event Square sends will be rejected (503, fail-closed). Set both in .env if you rely on webhook payment/refund reconciliation.", os.Getenv("SQUARE_ENVIRONMENT"))
}
- log.Printf("CRITICAL: SQUARE_WEBHOOK_SIGNATURE_KEY is not set with SQUARE_ENVIRONMENT=%q (non-mock) and SQUARE_WEBHOOK_NOTIFICATION_URL is unset — Square webhooks are not configured; any webhook event Square sends will be rejected (503, fail-closed). Set both in .env if you rely on webhook payment/refund reconciliation.", os.Getenv("SQUARE_ENVIRONMENT"))
}
func healthCheckHandler(w http.ResponseWriter, r *http.Request) {
diff --git a/backend/mw/ratelimit.go b/backend/mw/ratelimit.go
index 5090978..0228ef6 100644
--- a/backend/mw/ratelimit.go
+++ b/backend/mw/ratelimit.go
@@ -102,12 +102,28 @@ func (prl *ProgressiveRateLimiter) Check(ip string) (delayMs int) {
}
}
+// maxProgressiveSleepDelayMs is the largest delay the progressive per-IP
+// limiter still absorbs by sleeping. Beyond it the request is rejected 429
+// immediately instead (Round 2 Loop A finding 4a): sleeping 5-10s ties up a
+// goroutine per throttled request while the client keeps hammering, so one
+// client can stack many sleeping goroutines in front of the bcrypt wall on
+// /login and /register. The small progressive tiers (500ms / 2s) are
+// unchanged.
+const maxProgressiveSleepDelayMs = 2000
+
func ProgressiveRateLimit(next http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
ip := clientIP(r)
delay := globalProgressiveLimiter.Check(ip)
- if delay > 0 {
+ switch {
+ case delay > maxProgressiveSleepDelayMs:
+ // Far past the sustained budget — reject now instead of parking a
+ // goroutine for 5-10s. The rejection is a clean 429; the client
+ // retries after the burst window lapses.
+ RespondJSON(w, http.StatusTooManyRequests, map[string]string{"error": "Rate limit exceeded"})
+ return
+ case delay > 0:
time.Sleep(time.Duration(delay) * time.Millisecond)
w.Header().Set("X-RateLimit-Delay", fmt.Sprintf("%d", delay))
}
diff --git a/backend/mw/ratelimit_test.go b/backend/mw/ratelimit_test.go
index c26af9f..0c14162 100644
--- a/backend/mw/ratelimit_test.go
+++ b/backend/mw/ratelimit_test.go
@@ -651,3 +651,45 @@ func TestProgressiveRateLimiter_DelayEscalatesWithSustainedRate(t *testing.T) {
}
}
}
+
+// TestProgressiveRateLimit_RejectsBeyondSleepCap pins finding 4a: once the
+// computed delay exceeds maxProgressiveSleepDelayMs (2s), the middleware
+// rejects the request 429 immediately instead of sleeping a goroutine for
+// 5-10s (a per-client goroutine-parking amplifier in front of bcrypt). The
+// small progressive tiers (500ms / 2s) still sleep.
+func TestProgressiveRateLimit_RejectsBeyondSleepCap(t *testing.T) {
+ globalProgressiveLimiter.mu.Lock()
+ saved := globalProgressiveLimiter.requests
+ globalProgressiveLimiter.requests = make(map[string]*ipProgressiveState)
+ globalProgressiveLimiter.mu.Unlock()
+ t.Cleanup(func() {
+ globalProgressiveLimiter.mu.Lock()
+ globalProgressiveLimiter.requests = saved
+ globalProgressiveLimiter.mu.Unlock()
+ })
+
+ // Seed the 10s abuse tier (sustained > 300) for this IP.
+ seedProgressiveTimestamps(t, globalProgressiveLimiter, "198.51.100.99", 31, 320)
+
+ nextCalled := false
+ handler := ProgressiveRateLimit(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ nextCalled = true
+ w.WriteHeader(http.StatusOK)
+ }))
+
+ req := httptest.NewRequest(http.MethodPost, "/login", nil)
+ req.RemoteAddr = "198.51.100.99:1234"
+ w := httptest.NewRecorder()
+ start := time.Now()
+ handler.ServeHTTP(w, req)
+
+ if nextCalled {
+ t.Error("next handler must NOT be called when the delay exceeds the sleep cap")
+ }
+ if w.Code != http.StatusTooManyRequests {
+ t.Errorf("expected 429, got %d", w.Code)
+ }
+ if elapsed := time.Since(start); elapsed >= 5*time.Second {
+ t.Errorf("the 5-10s tiers must reject immediately, not sleep (took %s)", elapsed)
+ }
+}
diff --git a/frontend/src/lib/components/admin/GiftCardsManagement.svelte b/frontend/src/lib/components/admin/GiftCardsManagement.svelte
index 1c4a0ec..540522f 100644
--- a/frontend/src/lib/components/admin/GiftCardsManagement.svelte
+++ b/frontend/src/lib/components/admin/GiftCardsManagement.svelte
@@ -12,7 +12,7 @@
import SquareCardInput from '$lib/components/payments/SquareCardInput.svelte';
import CardEntryUnavailable from '$lib/components/payments/CardEntryUnavailable.svelte';
import { isSquareConfigured, submitPaymentWithRetry } from '$lib/square/square';
- import { range } from '$lib/utils/format';
+ import { formatCurrency, range } from '$lib/utils/format';
import { formatUserName } from '$lib/utils/nameDisplay';
import { parseWallClockDate } from '$lib/utils/timeSlots';
import { SvelteURLSearchParams } from 'svelte/reactivity';
@@ -775,10 +775,6 @@
return `${id.slice(0, 4)}-${id.slice(4, 8)}-${id.slice(8, 12)}`.toUpperCase();
}
- function formatCurrency(amount: number): string {
- return new Intl.NumberFormat('en-GB', { style: 'currency', currency: 'GBP' }).format(amount);
- }
-
function formatDate(dateStr: string): string {
return parseWallClockDate(dateStr).toLocaleDateString('en-GB', {
day: 'numeric',
diff --git a/frontend/src/lib/components/admin/TillPurchases.svelte b/frontend/src/lib/components/admin/TillPurchases.svelte
index abd5e4b..0d381ec 100644
--- a/frontend/src/lib/components/admin/TillPurchases.svelte
+++ b/frontend/src/lib/components/admin/TillPurchases.svelte
@@ -3,6 +3,7 @@
import { Input } from '$lib/components/ui/input';
import { Separator } from '$lib/components/ui/separator';
import { generateUUID } from '$lib/utils/uuid';
+ import { formatCurrency } from '$lib/utils/format';
import { toast } from 'svelte-sonner';
import { extractErrorMessage } from '$lib/utils/toast-safe';
import { apiFetch } from '$lib/utils/api';
@@ -14,6 +15,7 @@
isSquareConfigured,
isTwoFactorVerificationGateFailure,
isVerificationRequiredSignal,
+ runSavedCardSCAProactively,
shouldFallbackTo2FA,
SCA_UNAVAILABLE_2FA_FALLBACK_MESSAGE,
submitPaymentWithRetry,
@@ -22,10 +24,6 @@
PAYMENT_METHOD_SAVED_CARD,
VERIFICATION_REQUIRED_MESSAGE
} from '$lib/square/square';
- import {
- tokenizeSavedCardWithVerification,
- type SavedCardVerificationResult
- } from '$lib/components/payments/SquareCardInput.svelte';
import { authStore } from '$lib/stores/auth.svelte';
import { useTwoFactorCodeForSavedCard } from '$lib/stores/twoFactorCode.svelte';
@@ -282,10 +280,6 @@
!isNaN(parsedGiftCardAmount) && parsedGiftCardAmount > GIFT_CARD_MAX_AMOUNT
);
- function formatCurrency(n: number): string {
- return new Intl.NumberFormat('en-GB', { style: 'currency', currency: 'GBP' }).format(n);
- }
-
function addItem(label: string, price: number) {
const existing = cart.find((i) => i.label === label);
if (existing) {
diff --git a/frontend/src/lib/components/booking/BookingFlow.svelte b/frontend/src/lib/components/booking/BookingFlow.svelte
index 9300652..c348092 100644
--- a/frontend/src/lib/components/booking/BookingFlow.svelte
+++ b/frontend/src/lib/components/booking/BookingFlow.svelte
@@ -27,6 +27,7 @@
import { ensureBusinessInfo, getBusinessInfo } from '$lib/stores/businessInfo.svelte';
import { toast } from 'svelte-sonner';
import { extractErrorMessage } from '$lib/utils/toast-safe';
+ import { formatCurrency } from '$lib/utils/format';
import { onDestroy } from 'svelte';
// Components
@@ -49,15 +50,11 @@
isOverflowTipConfirmationRequired,
isTwoFactorVerificationGateFailure,
isVerificationRequiredSignal,
+ runSavedCardSCAProactively,
shouldFallbackTo2FA,
submitPaymentWithRetry,
VERIFICATION_REQUIRED_MESSAGE
} from '$lib/square/square';
- import {
- tokenizeSavedCardWithVerification,
- type SavedCardVerificationOutcome,
- type SavedCardVerificationResult
- } from '$lib/components/payments/SquareCardInput.svelte';
import UserPaymentModal from '$lib/components/payments/UserPaymentModal.svelte';
import OverflowTipConfirm from '$lib/components/payments/OverflowTipConfirm.svelte';
import { extractBookedSlots, getLunchProtectionForSlots } from '$lib/lunchProtection';
@@ -276,13 +273,6 @@
discounted_total: number;
} | null>(null);
- function formatCurrency(pence: number): string {
- return new Intl.NumberFormat('en-GB', {
- style: 'currency',
- currency: 'GBP'
- }).format(pence / 100);
- }
-
// =============== Payment Functions ===============
async function fetchUserDepositsRequired() {
if (!authStore.isAuthenticated) {
@@ -462,7 +452,19 @@
if (selectedPaymentMethod && !verificationToken) {
waitingForSCA = true;
try {
- const proactive = await runDepositSCAProactively(amountPence);
+ const squareCardId = paymentMethods.find(
+ (c) => c.id === selectedPaymentMethod
+ )?.square_card_id;
+ const proactive = await runSavedCardSCAProactively({
+ amountPence,
+ squareCardId: squareCardId ?? '',
+ buyer: {
+ givenName: customerInfo.firstName || authStore.currentUser?.firstName,
+ familyName: customerInfo.lastName || authStore.currentUser?.lastName,
+ email: customerInfo.email || authStore.currentUser?.email
+ },
+ onOutcome: (o) => (lastSCAOutcome = o)
+ });
if (proactive.outcome === 'challenge-cancelled' || proactive.outcome === 'sca-failed') {
depositError = CARD_VERIFICATION_RETRY_MESSAGE;
toast.error(depositError);
@@ -686,42 +688,6 @@
}
}
- /**
- * Saved-card (ccof) SCA challenge, run PROACTIVELY before the first deposit
- * charge attempt (never after a 402). Square's tokenize(verificationDetails,
- * squareCardId) determines UP FRONT whether buyer verification is required
- * and returns a fresh verification_token bound to the exact amount:
- * - 'verified' → the caller charges with the returned token (the first
- * attempt carries it — a naked ccof is never sent);
- * - 'sca-unavailable' → SCA can't run; the 2FA gate is demoted from backup
- * to the only available gate and the caller proceeds WITHOUT a token;
- * - 'challenge-cancelled' / 'sca-failed' → the caller must NOT charge: the
- * deposit step stays retryable and the user taps Pay again to re-run the
- * challenge.
- */
- async function runDepositSCAProactively(
- amountPence: number
- ): Promise<{ outcome: SavedCardVerificationOutcome; verificationToken?: string }> {
- const squareCardId = paymentMethods.find((c) => c.id === selectedPaymentMethod)?.square_card_id;
- if (!squareCardId) {
- lastSCAOutcome = 'sca-unavailable';
- return { outcome: 'sca-unavailable' };
- }
- let result: SavedCardVerificationResult;
- try {
- result = await tokenizeSavedCardWithVerification(amountPence, squareCardId, {
- givenName: customerInfo.firstName || authStore.currentUser?.firstName,
- familyName: customerInfo.lastName || authStore.currentUser?.lastName,
- email: customerInfo.email || authStore.currentUser?.email
- });
- } catch (_err) {
- lastSCAOutcome = 'sca-unavailable';
- return { outcome: 'sca-unavailable' };
- }
- lastSCAOutcome = result.outcome;
- return { outcome: result.outcome, verificationToken: result.verificationToken ?? undefined };
- }
-
let paymentAttempted = $state(false);
// Pre-start overpayment confirmation (mirrors UserPaymentModal). The backend
@@ -2592,12 +2558,12 @@
{#each discountPreview.discounts as d (d.name)}
{d.name}
- -£{d.amount.toFixed(2)}
+ -{formatCurrency(d.amount)}
{/each}
Estimated Total After Discount
- £{discountPreview.discounted_total.toFixed(2)}
+ {formatCurrency(discountPreview.discounted_total)}
{:else}
@@ -2641,7 +2607,7 @@
Deposit Paid
- Your deposit of £{calculateDepositAmount().toFixed(2)} has been
+ Your deposit of {formatCurrency(calculateDepositAmount())} has been
paid successfully. See you at your appointment!
@@ -2650,7 +2616,7 @@
Deposit Not Paid
Your booking is confirmed but the deposit of £{calculateDepositAmount().toFixed(2)}{formatCurrency(calculateDepositAmount())}
was not paid. If the deposit remains unpaid within 24 hours of your appointment,
the slot may be released and the booking could be cancelled or rebooked by someone
@@ -2718,8 +2684,8 @@
discountNote={overflowConfirm.chargePence !== undefined &&
overflowConfirm.chargePence < overflowConfirm.amountPence
? `An eligible campaign discount of ${formatCurrency(
- Math.max(0, overflowConfirm.amountPence - overflowConfirm.chargePence)
- )} applies — you'll be charged ${formatCurrency(overflowConfirm.chargePence)}.`
+ Math.max(0, overflowConfirm.amountPence - overflowConfirm.chargePence) / 100
+ )} applies — you'll be charged ${formatCurrency(overflowConfirm.chargePence / 100)}.`
: undefined}
loading={isProcessingPayment}
onConfirm={confirmOverflowPayment}
@@ -2845,7 +2811,7 @@
depositChargePence(
Math.round(calculateDepositAmount() * 100),
campaignDiscountPence(discountPreview)
- )
+ ) / 100
)}`}
diff --git a/frontend/src/lib/components/payments/CardSelection.svelte b/frontend/src/lib/components/payments/CardSelection.svelte
index 39a06d3..5891b70 100644
--- a/frontend/src/lib/components/payments/CardSelection.svelte
+++ b/frontend/src/lib/components/payments/CardSelection.svelte
@@ -6,6 +6,7 @@
import PolicyPopover from '$lib/components/ui/policyPopover.svelte';
import { isSquareConfigured } from '$lib/square/square';
import { authStore } from '$lib/stores/auth.svelte';
+ import { generateUUID } from '$lib/utils/uuid';
import { resolve } from '$app/paths';
export interface SelectableCard {
@@ -43,7 +44,7 @@
// Unique per instance: a plain counter would be instance-scoped in Svelte 5
// (every instance restarting at 0), so two mounted CardSelection instances
// would collide on the same checkbox id. Pure SPA, so no SSR concern.
- const consentId = `save-card-consent-${crypto.randomUUID()}`;
+ const consentId = `save-card-consent-${generateUUID()}`;
// B6/B10: saved-card charges require the customer's current 2FA verification
// code. This no longer BLOCKS saved-card selection — the code is collected
diff --git a/frontend/src/lib/components/payments/MockCardForm.svelte b/frontend/src/lib/components/payments/MockCardForm.svelte
index 4bc7127..6a5aea1 100644
--- a/frontend/src/lib/components/payments/MockCardForm.svelte
+++ b/frontend/src/lib/components/payments/MockCardForm.svelte
@@ -30,6 +30,7 @@