Files
Crussell/backend/handlers/payments/loop_b_money_fixes_test.go
T
popertots 3866cc5963 fix: round-2 loop-A fresh review (503c326 baseline) — B1 replay cap, A6 discount record, 2FA reissue+cooldown, notification flood, lockout saturation, VAT/refund-status consolidation
Round 2 Loop A fresh money/security/dup-mod review. 23 findings fixed:

MONEY:
- CRITICAL: B1 duplicate auto-refund gains an attempt cap (b1_attempts col, cap 3) —
  a rejected auto-refund no longer re-replays the expired key every sweep run
  (which minted a stacking unauthorized charge each time); FAILED-webhook
  demotion respects the cap; never re-replay a key whose B1 refund failed
- HIGH: A6 deposit_covered_by_discount skip path now APPLIES the eligible
  campaign discount rows immediately (capped) instead of skipping with no
  discount recorded — no more promised-discount-not-recorded overcharge
- MEDIUM: 2FA code burned by the SAVE gate is re-issued on failed
  new-card+save_card charges (re-issue guard now covers req.SaveCard)
- LOW: GetBookingPaymentSummary excludes tip rows from paidAmount (remaining
  now matches the authoritative tip-excluded balance)

SECURITY:
- MEDIUM: unacknowledged CRITICAL admin-notification flood capped (global cap
  on critical_payment_log + refresh_token_reuse rows)
- MEDIUM: 2FA reissue no longer bypasses the mint cooldown (Check no longer
  clears LastMintAt on gate-verify; cleared on terminal charge success)
- MEDIUM: twofa.StateFor map-saturation returns a shared permanently-locked
  state instead of a fresh 5-guess budget per request
- MEDIUM: ProgressiveRateLimit rejects 429 past maxProgressiveSleepDelayMs
  instead of sleeping unboundedly; login bcrypt concurrency semaphore added
- LOW: loginInProgress 409->429; webhook key-set/URL-unset startup check;
  email-verification per-user attempt counter

DUP/MOD:
- formatCurrency single source (frontend format.ts, 7 files consolidated);
  SquareRefundStatusToLocal single source (errors.go, all sites); admin
  audit-log helper dedup; SCA retry model unified (proactive on all 6
  surfaces); buyDailyTotal/daily-cap mirror via backend; lock TTL from
  backend; generateUUID at all card-form sites; magic numbers named
  (defaultPostgresHost, epsilon, fee constants); admin CASH + gift-card
  terminal charges now audited; DAV_SKIP_INIT documented in manuals

Verified: 26/26 dev + 24/24 prod (GO_TESTING=1, the CI condition), both vet
tags, frontend tests+build, env-docs 42/42.
2026-08-22 00:34:50 +01:00

290 lines
13 KiB
Go

//go:build test && dev
package payments
import (
"context"
"encoding/json"
"net/http"
"testing"
"time"
"crussell/clock"
"crussell/db"
"crussell/internal/square"
"crussell/testutils"
"crussell/testutils/fixtures"
"crussell/testutils/jwt"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
// =============================================================================
// Finding 2 — the A6 deposit clamp can produce chargeAmount=0 with no guard:
// the handler then charged £0 at Square (invalid in prod, minted a completed
// £0 deposit in the dev mock that consumed the discount). A deposit whose
// eligible campaign credit covers the ENTIRE remaining obligation must skip the
// Square call and report deposit_covered_by_discount.
// =============================================================================
// seedActiveCampaign inserts an active time-based campaign with the given
// discount percent and returns its id.
func seedActiveCampaign(t *testing.T, ctx context.Context, q db.Querier, percent int) string {
t.Helper()
now := clock.Now()
var id string
err := q.QueryRow(ctx, `
INSERT INTO discount_campaigns (name, campaign_type, discount_percent, status, start_date, end_date, times_redeemed)
VALUES ($1, 'time_based', $2, 'active', $3, $4, 0)
RETURNING id
`, "Money-Fix Campaign", percent, now.Add(-24*time.Hour), now.Add(24*time.Hour)).Scan(&id)
require.NoError(t, err)
return id
}
// failOnChargeClient fails the test if a Square charge is attempted. Proves a
// discount-covered deposit skips the Square call entirely (finding 2).
type failOnChargeClient struct {
square.SquareClient
t *testing.T
}
func (c *failOnChargeClient) CreatePayment(ctx context.Context, req square.CreatePaymentReq) (*square.PaymentResult, error) {
c.t.Fatalf("Square CreatePayment must NOT be called for a discount-covered deposit (amount=%d)", req.Amount)
return nil, nil
}
// 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)
userID, bookingID, _ := setupTestData(t, ctx, tx)
userToken := jwt.GenerateUserToken(userID)
// 100% time-based campaign = £50 discount on the £50 fixture booking, which
// fully covers the £25 deposit request (chargeAmount clamps to £0).
seedActiveCampaign(t, ctx, tx, 100)
origClient := SquareClient
SquareClient = &failOnChargeClient{SquareClient: square.NewDevClient(), t: t}
defer func() { SquareClient = origClient }()
cardToken := "cnon:deposit-covered"
req := CreateBookingPaymentRequest{
Amount: 2500, // £25 deposit
PaymentType: "deposit",
NewCardToken: &cardToken,
IdempotencyKey: "deposit-covered-" + bookingID,
}
handler := CreateBookingPayment
w := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx)
require.Equal(t, http.StatusOK, w.Code, "a discount-covered deposit must complete without a Square charge, body: %s", w.Body.String())
var body map[string]any
require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body))
assert.Equal(t, true, body["deposit_covered_by_discount"], "the response must signal the discount-covered deposit")
// 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.Equal(t, 1, payCount, "exactly one discount payment row must be recorded for the discount-covered deposit")
var discountCount int
require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM booking_discounts WHERE booking_id = $1`, bookingID).Scan(&discountCount))
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")
}
// =============================================================================
// Finding 7 — remaining-balance capacity ignored PENDING refunds: service.go
// counted only completed refunds in GetBookingRemainingBalancePence while
// GetBookingPaymentInfo counts completed + pending. An in-flight refund
// understated the remaining balance and blocked a legitimate retry.
// =============================================================================
func TestGetBookingRemainingBalancePence_PendingRefundsReopenCapacity(t *testing.T) {
t.Parallel()
ctx, tx := testutils.SetupTestTx(t)
userID, err := fixtures.CreateTestUser(tx)
require.NoError(t, err)
serviceID, err := fixtures.CreateTestService(tx)
require.NoError(t, err)
bookingID, err := fixtures.CreateTestBooking(tx, userID, serviceID)
require.NoError(t, err)
var bookingTotal int64
require.NoError(t, tx.QueryRow(ctx, `SELECT ROUND(total_amount * 100)::bigint FROM bookings WHERE id = $1`, bookingID).Scan(&bookingTotal))
svc := NewPaymentService()
// Pay the full booking amount.
_, err = svc.CreatePaymentRecord(ctx, PaymentRecord{
BookingID: bookingID,
PaymentType: "full",
PaymentMethod: "online_square",
Status: "completed",
Amount: float64(bookingTotal) / 100.0,
}, nil)
require.NoError(t, err)
remaining, err := svc.GetBookingRemainingBalancePence(ctx, bookingID)
require.NoError(t, err)
require.Equal(t, int64(0), remaining, "a fully-paid booking must have 0 remaining")
// A PENDING refund is money in flight that will come back — it must re-open
// capacity by its amount exactly like a completed refund.
var payRowID string
require.NoError(t, tx.QueryRow(ctx, `SELECT id FROM payments WHERE booking_id = $1 AND payment_type = 'full' ORDER BY created_at DESC LIMIT 1`, bookingID).Scan(&payRowID))
refundAmount := bookingTotal / 2
_, err = tx.Exec(ctx, `
INSERT INTO refunds (payment_id, booking_id, amount, status, reason, origin)
VALUES ($1, $2, $3, 'pending', 'in-flight test refund', 'manual')
`, payRowID, bookingID, float64(refundAmount)/100.0)
require.NoError(t, err)
remaining, err = svc.GetBookingRemainingBalancePence(ctx, bookingID)
require.NoError(t, err)
require.Equal(t, refundAmount, remaining, "a pending refund must re-open the remaining balance by its amount")
}