Files
Crussell/backend/handlers/payments/payments_round8_test.go
T
popertots fe88f2084d fix: review-loop B — adversarial findings (sweep auto-refund, admin clamp, 2FA real challenge, opaque refresh tokens, gated client IP, GBP pence)
Loop B aggressive adversarial round (3 attack agents) + fix + secondary + verification:
- CRITICAL: sweep replay auto-refunds provably-created-later duplicate charges (gated on parseable CreatedAt); 22h legitimate-retry window == 22h sweep cutoff (no dead zone)
- HIGH: admin Take Payment clamps to remaining obligation (cash/giftcard/saved-card/terminal); no unintended tip from overflow; campaign credit against remaining
- HIGH: /api/services/eligible-for/{id} requires auth + owner-or-admin (DOB/age + patch-test health-data leak closed)
- HIGH: opaque refresh-token rotation (login/refresh return {token, jti, refreshToken}; refresh REQUIRES opaque token; single-use rotation; logout revokes; access token rejected at refresh)
- HIGH: saved-card charges require a REAL 2FA verification code (B6/B10) — backend gate on all 8 charge paths + shared TwoFactorCodeInput frontend component on all 7 surfaces; 2FA gate is no longer setup-flag-only
- MEDIUM: ungated CF-Connecting-IP in reserve/admin_reserve gated via exported mw.ClientIP; 2FA limiter keyed on userID alone (no header-rotation bypass); ChangePassword actually revokes JTI + refresh tokens; 2FA setup mint cooldown + persistent failed-attempt counter; campaign redemption race surfaces campaign_fully_redeemed
- Terminal saved-card VAT applied (was under-collected); age-guard reconcile failures notify; isWeakJWTSecret entropy gate; gift-card redeem per-card counter + per-user limiter; webhook signature key startup validation
- NEW internal/twofa package (single source of truth breaking the payments<->user import cycle); consolidation of duplicate 2FA hash/verify
- Frontend: refresh-token storage + rotation, TwoFactorCodeInput component, amountPaidPence in admin modal, B5/B6/B10 contract wiring; 70 frontend tests
- Tests: loop_b_fixes_test.go, internal/twofa tests, updated auth/services/profile/twofa/mw tests

All 26 backend packages pass (incl. internal/twofa); frontend 70/70 + build clean; env-docs 41/41.
2026-08-22 00:34:50 +01:00

367 lines
17 KiB
Go

//go:build test && dev
package payments
import (
"context"
"database/sql"
"net/http"
"sync"
"testing"
"time"
"crussell/db"
"crussell/internal/square"
"crussell/testutils"
"crussell/testutils/fixtures"
"crussell/testutils/jwt"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
// =============================================================================
// ROUND 8 — money-safety testing gaps
// =============================================================================
//
// This file pins four behaviors that keep money movements safe:
//
// 1. RefundPayment's manual guard rejects discount/on-the-house ledger rows
// (a discount row is not real money, so refunding it would pay money out
// of nothing) BEFORE any Square refund call or refund row is created.
//
// 2. The sweep's recordUntrackedTerminalPayment splits a COMPLETED terminal
// charge that exceeds the remaining booking balance into deposit/balance/
// tip records, applies per-record VAT via ApplyVATToBookingPayment, and
// completes the now-fully-paid booking.
//
// 3. CreateTerminalPayment always sends AllowTipping: false in the Square
// CreateCheckoutReq — the third leg of the tip double-count fix (the
// frontend embeds the tip in the charge amount, so the terminal must not
// prompt for a second one).
//
// 4. acquireAdvisoryXactLockBlocking (the deliberately-unbounded refund lock)
// blocks a second waiter until the holder's transaction commits — the
// "a refund must never be dropped" rationale for the unbounded wait.
// =============================================================================
// T5 — RefundPayment manual guard rejects discount / on-the-house payments
// =============================================================================
// TestRound8_RefundPayment_DiscountOrOnTheHouse_Rejected pins the T5 manual
// guard: RefundPayment rejects a completed discount/on-the-house payment row
// with 400 and a "Cannot refund a discount or complimentary payment" message,
// BEFORE issuing any Square refund call and BEFORE creating any refund row. A
// discount/on-the-house row is a ledger entry, not real money — the customer
// never paid it, so refunding it would pay money out of nothing. The row is
// seeded WITHOUT a square_payment_id so the rejection can only come from the
// discount guard (the later "Payment has no Square reference" guard would
// fire with a different message if the discount guard were ever removed).
func TestRound8_RefundPayment_DiscountOrOnTheHouse_Rejected(t *testing.T) {
for _, method := range []string{"discount", "on_the_house"} {
t.Run(method, func(t *testing.T) {
ctx, tx := testutils.SetupTestTx(t)
adminID, err := fixtures.CreateTestAdminUser(tx)
require.NoError(t, err)
_, bookingID, _ := setupTestData(t, ctx, tx)
payID, err := fixtures.CreateTestPayment(tx, bookingID, 50.00, method, "full", "completed")
require.NoError(t, err)
// Swap in a client that records every Square refund call so the
// test can prove the guard fires before any money would move.
origClient := SquareClient
counting := &countingRefundClient{SquareClient: square.NewDevClient()}
SquareClient = counting
defer func() { SquareClient = origClient }()
adminToken := jwt.GenerateTestToken(adminID, "admin")
req := RefundRequest{Amount: 1000, Reason: "round8 guard test"}
w := makePaymentRequest(RefundPayment, "POST", "/api/admin/payments/"+payID+"/refund", req, adminToken, ctx)
require.Equal(t, http.StatusBadRequest, w.Code, "body: %s", w.Body.String())
assert.Contains(t, w.Body.String(), "Cannot refund a discount or complimentary payment",
"the message must identify the discount/complimentary rejection")
require.Empty(t, counting.refundCalls(),
"no Square refund call may be issued for a discount/on-the-house payment")
var refundCount int
err = tx.QueryRow(ctx, `SELECT COUNT(*) FROM refunds WHERE payment_id = $1`, payID).Scan(&refundCount)
require.NoError(t, err)
assert.Equal(t, 0, refundCount,
"no refund row may be created for a discount/on-the-house payment")
})
}
}
// =============================================================================
// T6 — recordUntrackedTerminalPayment tip-split / VAT / completion branches
// =============================================================================
// TestRound8_SweepUntrackedTerminal_OverBalance_TipSplit_VAT_CompletesBooking
// pins the T6 money-safety contract of recordUntrackedTerminalPayment: a stale
// "tmp-" terminal checkout that COMPLETED at Square with an amount ABOVE the
// remaining booking balance (£55 on a £50 booking) must be recorded as THREE
// ledger rows (deposit £25 + balance £25 + tip £5), each booking row must get
// its VAT applied through ApplyVATToBookingPayment (the tip record must never
// carry VAT), and the now-fully-paid booking must be transitioned to
// 'completed' via completeFullyPaidBooking. This mirrors the existing
// TestSweepStaleTerminalCheckouts_TmpProvisional_Completed_RecordsPayment but
// exercises the over-balance split that its tipAmount=0 charge never reaches.
func TestRound8_SweepUntrackedTerminal_OverBalance_TipSplit_VAT_CompletesBooking(t *testing.T) {
ctx, tx := testutils.SetupTestTx(t)
userID, err := fixtures.CreateTestUser(tx)
require.NoError(t, err)
serviceID, err := fixtures.CreateTestService(tx)
require.NoError(t, err)
bookingID, err := fixtures.CreateTestBookingAtTime(tx, userID, serviceID,
time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC))
require.NoError(t, err)
// The booking must be in a payable state for the untracked charge to be
// recorded (bookingStatusAllowsCompletedPayment) and completable by
// completeFullyPaidBooking.
if _, err := tx.Exec(ctx, "UPDATE bookings SET status = 'in_progress' WHERE id = $1", bookingID); err != nil {
t.Fatalf("failed to set booking in_progress: %v", err)
}
// VAT-registered so the sweep's per-record ApplyVATToBookingPayment writes
// vat_amount/net_amount on the split booking rows. The update is part of the
// setup tx that is committed below, so the sweep sees it at pool level.
if _, err := tx.Exec(ctx, `UPDATE business_settings SET is_vat_registered = TRUE, default_vat_rate = 20.00`); err != nil {
t.Fatalf("failed to enable VAT registration: %v", err)
}
const tmpID = "tmp-round8-tip-split"
seedStaleProvisionalTerminalCheckout(t, ctx, tx, bookingID, tmpID)
// B3: the £5 overflow is only carved into a tip record when the customer
// EXPLICITLY requested a tip (tip_enabled) — mark it so the split is
// exercised here.
if _, err := tx.Exec(ctx, `UPDATE terminal_checkouts SET tip_enabled = TRUE WHERE checkout_id = $1`, tmpID); err != nil {
t.Fatalf("failed to mark the checkout tip-enabled: %v", err)
}
origClient := SquareClient
const sqPayID = "sqp_round8_tip_split"
SquareClient = &provisionalCheckoutClient{
SquareClient: square.NewDevClient(),
checkoutID: tmpID,
// £55 charged on a £50 booking: deposit £25 + balance £25 + tip £5.
result: &square.PaymentResult{Status: "COMPLETED", SquarePayID: sqPayID, Amount: 5500, Fees: 88, CardBrand: "VISA", CardLast4: "4242"},
}
defer func() { SquareClient = origClient }()
pgxTx := db.TxFromContext(ctx)
require.NotNil(t, pgxTx, "no transaction in context")
require.NoError(t, pgxTx.Commit(ctx), "failed to commit setup tx")
pool := context.Background()
t.Cleanup(func() {
_, _ = db.Conn.Exec(pool, `DELETE FROM payments WHERE square_payment_id = $1`, sqPayID)
_, _ = db.Conn.Exec(pool, `DELETE FROM bookings WHERE id = $1`, bookingID)
_, _ = db.Conn.Exec(pool, `DELETE FROM services WHERE id = $1`, serviceID)
_, _ = db.Conn.Exec(pool, `DELETE FROM users WHERE id = $1`, userID)
// Restore the shared business_settings row to the VAT-unregistered
// baseline so parallel tests keep their own VAT expectations.
_, _ = db.Conn.Exec(pool, `UPDATE business_settings SET is_vat_registered = FALSE, default_vat_rate = 20.00`)
})
// Drop any other stale terminal rows left by sequential tests so the count
// is deterministic.
if _, err := db.Conn.Exec(pool, `DELETE FROM terminal_checkouts WHERE status IN ('PENDING', 'IN_PROGRESS') AND checkout_id <> $1`, tmpID); err != nil {
t.Fatalf("failed to clean leftover stale terminal checkouts: %v", err)
}
if _, err := db.Conn.Exec(pool, `DELETE FROM till_sales WHERE status = 'pending' AND square_checkout_id IS NOT NULL`); err != nil {
t.Fatalf("failed to clean leftover stale till sales: %v", err)
}
n, err := SweepStaleTerminalCheckouts(pool)
require.NoError(t, err, "sweep failed")
assert.Equal(t, 1, n, "the COMPLETED over-balance provisional checkout must be resolved by the sweep")
var status string
require.NoError(t, db.Conn.QueryRow(pool, "SELECT status FROM terminal_checkouts WHERE checkout_id = $1", tmpID).Scan(&status))
assert.Equal(t, "COMPLETED", status, "the recorded checkout row must be marked COMPLETED")
// The untracked charge must be split: one £55 Square charge → deposit £25 +
// balance £25 + tip £5 (three ledger rows sharing the square_payment_id).
rows, err := db.Conn.Query(pool, `
SELECT payment_type, amount, is_vat_applicable, vat_amount, net_amount
FROM payments
WHERE booking_id = $1 AND square_payment_id = $2
ORDER BY payment_type
`, bookingID, sqPayID)
require.NoError(t, err, "failed to query recorded split payments")
defer rows.Close()
type splitRow struct {
paymentType string
amount float64
vatApplied bool
vatAmount sql.NullFloat64
netAmount sql.NullFloat64
}
splits := map[string]splitRow{}
for rows.Next() {
var r splitRow
require.NoError(t, rows.Scan(&r.paymentType, &r.amount, &r.vatApplied, &r.vatAmount, &r.netAmount))
splits[r.paymentType] = r
}
require.NoError(t, rows.Err())
require.Len(t, splits, 3, "the over-balance terminal charge must split into deposit + balance + tip records")
assert.InDelta(t, 25.0, splits["deposit"].amount, 0.001, "deposit = 50%% of the £50 booking total")
assert.InDelta(t, 25.0, splits["balance"].amount, 0.001, "balance = the remaining booking total")
assert.InDelta(t, 5.0, splits["tip"].amount, 0.001, "tip = the charged amount above the booking value")
// Per-record VAT (ApplyVATToBookingPayment): the deposit and balance rows
// carry 20% VAT of the £25 gross (£4.17 VAT, £20.83 net); the tip record
// must never have VAT applied.
for _, pt := range []string{"deposit", "balance"} {
r := splits[pt]
assert.True(t, r.vatApplied, "%s record must have VAT applied", pt)
require.True(t, r.vatAmount.Valid, "%s record must have vat_amount set", pt)
assert.InDelta(t, 4.17, r.vatAmount.Float64, 0.001, "%s record VAT (20%% of £25 gross)", pt)
require.True(t, r.netAmount.Valid, "%s record must have net_amount set", pt)
assert.InDelta(t, 20.83, r.netAmount.Float64, 0.001, "%s record net of 20%% VAT", pt)
}
assert.False(t, splits["tip"].vatApplied, "tip record must never have VAT applied")
assert.False(t, splits["tip"].vatAmount.Valid, "tip record must have NULL vat_amount")
assert.False(t, splits["tip"].netAmount.Valid, "tip record must have NULL net_amount")
// The £55 charge covers the full £50 booking (deposit + balance), so
// completeFullyPaidBooking must have transitioned the booking to
// 'completed' — the same completion the poll handler performs.
var bookingStatus string
require.NoError(t, db.Conn.QueryRow(pool, "SELECT status FROM bookings WHERE id = $1", bookingID).Scan(&bookingStatus))
assert.Equal(t, "completed", bookingStatus, "a fully-paid booking must be completed by the sweep")
}
// =============================================================================
// T8 — CreateTerminalPayment sends AllowTipping: false to Square
// =============================================================================
// recordingCheckoutClient records every CreateCheckoutReq so a test can assert
// exactly what the handler sends to Square while delegating the actual call to
// the underlying client (the same recording-client pattern as
// recordingPaymentClient / countingRefundClient in the sibling files).
type recordingCheckoutClient struct {
square.SquareClient
mu sync.Mutex
reqs []square.CreateCheckoutReq
}
func (c *recordingCheckoutClient) CreateCheckout(ctx context.Context, req square.CreateCheckoutReq) (*square.CheckoutResult, error) {
c.mu.Lock()
c.reqs = append(c.reqs, req)
c.mu.Unlock()
return c.SquareClient.CreateCheckout(ctx, req)
}
func (c *recordingCheckoutClient) checkoutReqs() []square.CreateCheckoutReq {
c.mu.Lock()
defer c.mu.Unlock()
return append([]square.CreateCheckoutReq(nil), c.reqs...)
}
// TestRound8_CreateTerminalPayment_AllowTippingFalse pins the T8 leg of the
// tip double-count fix: the frontend embeds the tip in the charge amount
// (totalWithTip), so CreateTerminalPayment must pass AllowTipping: false in
// the Square CreateCheckoutReq even when the client requests TipEnabled —
// otherwise the terminal would prompt for a second tip and the tip would be
// double-counted in production. The request is captured with a recording
// client and asserted verbatim.
func TestRound8_CreateTerminalPayment_AllowTippingFalse(t *testing.T) {
ctx, tx := testutils.SetupTestTx(t)
_, bookingID, _ := setupTestData(t, ctx, tx)
adminToken := jwt.GenerateAdminToken()
origClient := SquareClient
rec := &recordingCheckoutClient{SquareClient: square.NewDevClient()}
SquareClient = rec
defer func() { SquareClient = origClient }()
handler := CreateTerminalPayment
req := CreateTerminalPaymentRequest{
Amount: 5500,
PaymentType: "full",
TipEnabled: true,
}
w := makePaymentRequest(handler, "POST", "/api/admin/bookings/"+bookingID+"/payment", req, adminToken, ctx)
require.Equal(t, http.StatusOK, w.Code, "body: %s", w.Body.String())
reqs := rec.checkoutReqs()
require.Len(t, reqs, 1, "exactly one CreateCheckoutReq must be sent to Square")
assert.False(t, reqs[0].AllowTipping,
"AllowTipping must be false even with TipEnabled — the tip is already embedded in the amount")
assert.Equal(t, int64(5500), reqs[0].Amount, "the charge amount (tip embedded) must reach Square verbatim")
assert.Equal(t, bookingID, reqs[0].ReferenceID, "the checkout must be scoped to the booking")
}
// =============================================================================
// T9 — acquireAdvisoryXactLockBlocking blocks waiters until the holder commits
// =============================================================================
// TestRound8_AdvisoryXactLock_BlocksWaiterUntilCommit pins the T9 contract of
// the deliberately-unbounded transaction-scoped refund lock: a second waiter
// on the same "crussell:refund:" key must BLOCK (not time out, not proceed)
// while the holder's transaction is open, and must acquire the lock — returning
// nil — only after the holder commits. This is the "a refund must never be
// dropped" rationale: if the manual RefundPayment holds the key across its
// up-to-30s Square round-trip, a timed-out cancellation would abort and the
// caller would commit a cancellation with ZERO refund rows created (no sweep
// retry is possible because the rows never existed). Uses channels + timeouts
// so the assertion never depends on a sleep; both transactions are rolled back
// when the lock is not acquired.
func TestRound8_AdvisoryXactLock_BlocksWaiterUntilCommit(t *testing.T) {
ctx := context.Background()
key := "crussell:refund:round8-locktest"
// Goroutine A: the holder. Its transaction stays OPEN until we commit it,
// so the lock it holds is never released early.
holderTx, err := db.Conn.Begin(ctx)
require.NoError(t, err, "failed to begin holder tx")
defer func() { _ = holderTx.Rollback(ctx) }()
require.NoError(t, acquireAdvisoryXactLockBlocking(ctx, holderTx, key),
"the uncontended blocking xact lock must be acquired immediately")
// Goroutine B: the waiter. It signals that it has STARTED (its tx is open
// and it is about to issue the blocking acquire) and then reports the
// acquire result on a buffered channel.
started := make(chan struct{})
acquired := make(chan error, 1)
go func() {
waiterTx, err := db.Conn.Begin(ctx)
if err != nil {
acquired <- err
return
}
defer func() { _ = waiterTx.Rollback(ctx) }()
close(started)
acquired <- acquireAdvisoryXactLockBlocking(ctx, waiterTx, key)
}()
<-started
// While the holder's tx is open, the waiter must NOT have returned.
select {
case err := <-acquired:
t.Fatalf("waiter returned %v while the holder tx was still open — the blocking xact lock did not block", err)
case <-time.After(300 * time.Millisecond):
// Expected: the waiter is blocked on the holder's lock.
}
// Release the lock by committing the holder's transaction; the waiter must
// then acquire it and return nil.
require.NoError(t, holderTx.Commit(ctx), "failed to commit holder tx")
select {
case err := <-acquired:
require.NoError(t, err, "the waiter must acquire the lock once the holder commits")
case <-time.After(10 * time.Second):
t.Fatal("waiter never acquired the lock after the holder committed — the blocking xact lock did not release")
}
}