diff --git a/backend/handlers/payments/giftcard_clawback.go b/backend/handlers/payments/giftcard_clawback.go index 1bd0252..2d08dbb 100644 --- a/backend/handlers/payments/giftcard_clawback.go +++ b/backend/handlers/payments/giftcard_clawback.go @@ -6,12 +6,28 @@ import ( "fmt" "log" "log/slog" + "math" "crussell/db" "github.com/jackc/pgx/v5" ) +// errClawbackPartiallyReversed is returned by RevertGiftCardFunding when some +// of the funding it was asked to revert had already been SPENT before the +// charge failed — the clawback reverts everything still on the card/balance +// but the spent portion cannot be reclaimed. The CRITICAL admin notification +// for reconciliation is inserted inside RevertGiftCardFunding itself, so every +// caller (till handler, stale-pending sweep, Square webhook) surfaces the +// residual without forking the money logic. +var errClawbackPartiallyReversed = errors.New("gift-card funding clawback partially reversed — funding was already spent") + +// penceLess reports whether a < b comparing two pound-float balances in pence, +// the only float-safe way to compare money. +func penceLess(a, b float64) bool { + return int64(math.Round(a*100)) < int64(math.Round(b*100)) +} + // RevertGiftCardFunding undoes the gift-card funding performed earlier in the // SAME till-sale request after a definitive Square charge rejection, matching // the gift_card_transactions accounting: a created card is deleted (with its @@ -50,6 +66,13 @@ func RevertGiftCardFunding(ctx context.Context, action, giftCardID string, amoun return errTillSaleNotPending } + // M6: partial flags that some of the funding was already spent before the + // charge failed — the clawback reverts everything still on the card/balance + // but the spent portion is unrecoverable. Set by the branches below and + // resolved into a CRITICAL admin notification + errClawbackPartiallyReversed + // after the commit. + partial := false + if action == "create" { // A newly created card's transactions are scoped to THIS sale's // funding (reference_type='till_sale' AND reference_id=sale id) — never @@ -63,44 +86,66 @@ func RevertGiftCardFunding(ctx context.Context, action, giftCardID string, amoun return fmt.Errorf("failed to delete gift card: %w", err) } // If the card was immediately redeemed to a user balance in this - // request, reverse that credit (guarded so it can never go negative). + // request, reverse that credit. M6: the reversal CLAMPS to zero instead + // of the old guarded `balance >= amount` 0-row block — a partially-spent + // redemption must still give back everything that remains on the + // balance; the unreclaimable spent portion is surfaced as a CRITICAL + // admin notification + errClawbackPartiallyReversed below. if redeemToUserID != nil && *redeemToUserID != "" { - bTag, bErr := tx.Exec(ctx, ` - UPDATE user_giftcard_balances - SET balance = user_giftcard_balances.balance - $1, updated_at = NOW() - WHERE user_id = $2 AND balance >= $1 - `, amount, *redeemToUserID) - if bErr != nil { - return fmt.Errorf("failed to reverse redeemed gift card balance: %w", bErr) - } - if bTag.RowsAffected() == 0 { - // The guard blocked the reversal because some of the credited - // balance was already spent. The sale is still marked failed - // below — do not fail the whole clawback tx — but the - // un-reversed credit must be flagged for manual reconciliation - // (mirrors the top-up branch). - log.Printf("CRITICAL: ... MANUAL RECONCILIATION REQUIRED: create-with-redeem clawback for gift card %s could not fully reverse the £%.2f balance credited to user %s (balance < amount)", giftCardID, amount, *redeemToUserID) + var balanceBefore float64 + err := tx.QueryRow(ctx, `SELECT balance FROM user_giftcard_balances WHERE user_id = $1`, *redeemToUserID).Scan(&balanceBefore) + if err != nil { + // Balance row missing (the credit was never recorded / already + // fully spent — nothing to reverse) or unreadable. A + // non-ErrNoRows read failure means the debit could not be + // verified — flag the sale as partially reversed so the owner + // reconciles instead of silently keeping the credit. + if !errors.Is(err, pgx.ErrNoRows) { + partial = true + log.Printf("CRITICAL: ... MANUAL RECONCILIATION REQUIRED: create-with-redeem clawback for gift card %s could not read the balance credited to user %s (%v) — MANUAL RECONCILIATION REQUIRED", giftCardID, *redeemToUserID, err) + } + } else { + if _, bErr := tx.Exec(ctx, ` + UPDATE user_giftcard_balances + SET balance = GREATEST(0, balance - $1), updated_at = NOW() + WHERE user_id = $2 + `, amount, *redeemToUserID); bErr != nil { + return fmt.Errorf("failed to reverse redeemed gift card balance: %w", bErr) + } + if penceLess(balanceBefore, amount) { + partial = true + log.Printf("CRITICAL: ... MANUAL RECONCILIATION REQUIRED: create-with-redeem clawback for gift card %s could not fully reverse the £%.2f balance credited to user %s (some was already spent — remaining balance clamped to zero)", giftCardID, amount, *redeemToUserID) + } } } } else { - // Top-up: subtract the amount back out of the card. The guard keeps - // amount_remaining from ever going negative in the pathological case - // where some of the top-up was already spent before the charge failed. - tag, err := tx.Exec(ctx, ` - UPDATE gift_cards - SET total_funds_added = total_funds_added - $1, - amount_remaining = amount_remaining - $1 - WHERE id = $2 AND amount_remaining >= $1 - `, amount, giftCardID) - if err != nil { - return fmt.Errorf("failed to reverse gift card top-up: %w", err) - } - if tag.RowsAffected() == 0 { - // The guard blocked the reversal because some of the top-up was - // already spent. The sale is still marked failed below — do not - // fail the whole clawback tx — but the unreversed money must be - // flagged for manual reconciliation. - log.Printf("CRITICAL: ... MANUAL RECONCILIATION REQUIRED: top-up %v on gift card %s could not be fully reversed (amount_remaining < top-up)", amount, giftCardID) + // Top-up: subtract the amount back out of the card. M6: the reversal + // CLAMPS amount_remaining and total_funds_added to zero instead of the + // old guarded `amount_remaining >= amount` 0-row block — a partially- + // spent top-up must still give back everything still on the card; the + // unreclaimable spent portion is surfaced as a CRITICAL admin + // notification + errClawbackPartiallyReversed below. + var remainingBefore float64 + if err := tx.QueryRow(ctx, `SELECT amount_remaining FROM gift_cards WHERE id = $1`, giftCardID).Scan(&remainingBefore); err != nil { + // The card is gone (no FK — a concurrent gift-card cancellation can + // remove it) or unreadable: nothing can be reverted, but the sale + // must STILL be marked failed. Flag the funding as unrecovered so + // the owner reconciles instead of the sale staying pending forever. + partial = true + log.Printf("CRITICAL: ... MANUAL RECONCILIATION REQUIRED: top-up %v on gift card %s could not be reversed (card missing/unreadable: %v)", amount, giftCardID, err) + } else { + if _, err := tx.Exec(ctx, ` + UPDATE gift_cards + SET total_funds_added = GREATEST(0, total_funds_added - $1), + amount_remaining = GREATEST(0, amount_remaining - $1) + WHERE id = $2 + `, amount, giftCardID); err != nil { + return fmt.Errorf("failed to reverse gift card top-up: %w", err) + } + if penceLess(remainingBefore, amount) { + partial = true + log.Printf("CRITICAL: ... MANUAL RECONCILIATION REQUIRED: top-up %v on gift card %s could not be fully reversed (some of it was already spent — card balance clamped to zero)", amount, giftCardID) + } } // Remove only this request's top-up transaction (reference_id = till // sale) so prior sales' accounting on the same card is untouched. @@ -117,15 +162,30 @@ func RevertGiftCardFunding(ctx context.Context, action, giftCardID string, amoun } // MEDIUM-3a coverage: every funding clawback is a money reversal that must - // be auditable. Record it in admin_audit_log (best-effort, own tx — - // InsertAdminAuditCharge's separate transaction keeps a write failure from - // aborting the committed clawback). The create branch DELETES the card, so - // target_gift_card_id must stay NULL (the FK would otherwise block the card - // deletion); the card id is carried in the details. admin_id is NULL too — - // this helper is the shared implementation for the till handler, the - // stale-pending sweep and the Square webhook, which carry no admin actor. + // be auditable — the PARTIAL clawback included (it reverted the balance to + // zero and must leave the same trail). Record it in admin_audit_log + // (best-effort, own tx — InsertAdminAuditCharge's separate transaction + // keeps a write failure from aborting the committed clawback). The create + // branch DELETES the card, so target_gift_card_id must stay NULL (the FK + // would otherwise block the card deletion); the card id is carried in the + // details. admin_id is NULL too — this helper is the shared implementation + // for the till handler, the stale-pending sweep and the Square webhook, + // which carry no admin actor. insertGiftCardClawbackAudit(ctx, giftCardID, tillSaleID, action, amount) + if partial { + // M6: the funding was partially spent before the charge failed — the + // clawback reverted everything still on the card/balance but the spent + // portion is gone. Surface a CRITICAL admin notification so the owner + // reconciles the residual (the card/balance were clamped to zero and + // the sale is failed; the spent money is unrecoverable and must be + // reviewed) and return the sentinel so every caller knows the reversal + // was not complete. + insertCriticalPaymentNotification(ctx, nil, redeemToUserID) + log.Printf("CRITICAL: till sale %s's gift-card funding clawback was PARTIAL (funding had already been spent) — card/balance reverted to zero; the spent portion is unrecoverable — MANUAL RECONCILIATION REQUIRED", tillSaleID) + return errClawbackPartiallyReversed + } + return nil } diff --git a/backend/handlers/payments/giftcard_clawback_test.go b/backend/handlers/payments/giftcard_clawback_test.go new file mode 100644 index 0000000..7b0656f --- /dev/null +++ b/backend/handlers/payments/giftcard_clawback_test.go @@ -0,0 +1,180 @@ +//go:build test && dev + +package payments + +import ( + "context" + "errors" + "testing" + + "crussell/db" + "crussell/testutils" + "crussell/testutils/fixtures" +) + +// TestRevertGiftCardFunding_TopupPartiallySpent_InsertsCriticalNotification +// locks the M6 neither-path: when the top-up funding was already partially spent +// before the charge failed, RevertGiftCardFunding clamps the card to zero, +// returns errClawbackPartiallyReversed AND inserts a critical-payment admin +// notification so the residual is surfaced in the admin notification centre +// (a bare log line is not enough — an operator must see the reconciliation +// item). +func TestRevertGiftCardFunding_TopupPartiallySpent_InsertsCriticalNotification(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + adminID, err := fixtures.CreateTestAdminUser(tx) + if err != nil { + t.Fatalf("failed to create admin user: %v", err) + } + + var cardID string + if err := tx.QueryRow(ctx, ` + INSERT INTO gift_cards (total_funds_added, amount_remaining, created_by, is_inventory) + VALUES (50.00, 10.00, $1, FALSE) + RETURNING id + `, adminID).Scan(&cardID); err != nil { + t.Fatalf("failed to seed gift card: %v", err) + } + + var saleID string + if err := tx.QueryRow(ctx, ` + INSERT INTO till_sales (item_type, description, quantity, unit_price, total_amount, payment_method, status, created_by, created_at, updated_at) + VALUES ('gift_card', 'Gift Card topup', 1, 50.00, 50.00, 'online_square', 'pending', $1, NOW(), NOW()) + RETURNING id + `, adminID).Scan(&saleID); err != nil { + t.Fatalf("failed to seed till sale: %v", err) + } + + 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 setup tx: %v", err) + } + + pool := context.Background() + t.Cleanup(func() { + _, _ = db.Conn.Exec(pool, `DELETE FROM admin_notifications WHERE reason = 'critical_payment_log'`) + _, _ = db.Conn.Exec(pool, `DELETE FROM gift_card_transactions WHERE gift_card_id = $1`, cardID) + _, _ = db.Conn.Exec(pool, `DELETE FROM till_sales WHERE id = $1`, saleID) + _, _ = db.Conn.Exec(pool, `DELETE FROM gift_cards WHERE id = $1`, cardID) + _, _ = db.Conn.Exec(pool, `DELETE FROM users WHERE id = $1`, adminID) + }) + + err = revertGiftCardFunding(pool, "topup", cardID, 50.00, nil, saleID) + if !errors.Is(err, errClawbackPartiallyReversed) { + t.Fatalf("expected errClawbackPartiallyReversed for the partially-spent top-up, got %v", err) + } + + // M6: the CRITICAL admin notification must exist for the residual (a till + // sale has no booking or user attribution, so the notification dedup key is + // (nil, nil)). + var notifCount int + if err := db.Conn.QueryRow(pool, `SELECT COUNT(*) FROM admin_notifications WHERE reason = 'critical_payment_log' AND booking_id IS NULL AND user_id IS NULL`).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 partial clawback, got %d", notifCount) + } +} + +// TestRevertGiftCardFunding_CreateWithRedeemPartiallySpent_ClampsAndFlags locks +// the M6 create-with-redeem neither-path: a created card immediately redeemed to +// a user balance that was partially spent before the charge failed is deleted, +// the balance is clamped to zero, errClawbackPartiallyReversed is returned and +// a critical admin notification is inserted for the unrecoverable spent +// portion. +func TestRevertGiftCardFunding_CreateWithRedeemPartiallySpent_ClampsAndFlags(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + adminID, err := fixtures.CreateTestAdminUser(tx) + if err != nil { + t.Fatalf("failed to create admin user: %v", err) + } + customerID, err := fixtures.CreateTestUser(tx) + if err != nil { + t.Fatalf("failed to create customer user: %v", err) + } + + var cardID string + if err := tx.QueryRow(ctx, ` + INSERT INTO gift_cards (total_funds_added, amount_remaining, created_by, is_inventory) + VALUES (50.00, 0.00, $1, FALSE) + RETURNING id + `, adminID).Scan(&cardID); err != nil { + t.Fatalf("failed to seed gift card: %v", err) + } + // The redeem credited £50 to the customer, who spent £40 before the charge + // failed. + if _, err := tx.Exec(ctx, ` + INSERT INTO user_giftcard_balances (user_id, balance, updated_at) + VALUES ($1, 10.00, NOW()) + `, customerID); err != nil { + t.Fatalf("failed to seed user balance: %v", err) + } + + var saleID string + if err := tx.QueryRow(ctx, ` + INSERT INTO till_sales (item_type, description, quantity, unit_price, total_amount, payment_method, status, created_by, created_at, updated_at) + VALUES ('gift_card', 'Gift Card create', 1, 50.00, 50.00, 'online_square', 'pending', $1, NOW(), NOW()) + RETURNING id + `, adminID).Scan(&saleID); err != nil { + t.Fatalf("failed to seed till sale: %v", err) + } + + 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 setup tx: %v", err) + } + + pool := context.Background() + t.Cleanup(func() { + _, _ = db.Conn.Exec(pool, `DELETE FROM admin_notifications WHERE reason = 'critical_payment_log'`) + _, _ = db.Conn.Exec(pool, `DELETE FROM user_giftcard_balances WHERE user_id = $1`, customerID) + _, _ = db.Conn.Exec(pool, `DELETE FROM gift_card_transactions WHERE gift_card_id = $1`, cardID) + _, _ = db.Conn.Exec(pool, `DELETE FROM till_sales WHERE id = $1`, saleID) + _, _ = db.Conn.Exec(pool, `DELETE FROM gift_cards WHERE id = $1`, cardID) + _, _ = db.Conn.Exec(pool, `DELETE FROM users WHERE id = ANY($1)`, []string{adminID, customerID}) + }) + + err = revertGiftCardFunding(pool, "create", cardID, 50.00, &customerID, saleID) + if !errors.Is(err, errClawbackPartiallyReversed) { + t.Fatalf("expected errClawbackPartiallyReversed for the partially-spent redeemed balance, got %v", err) + } + + // The created card is deleted and the balance clamped to zero. + var cardCount int + if err := db.Conn.QueryRow(pool, `SELECT COUNT(*) FROM gift_cards WHERE id = $1`, cardID).Scan(&cardCount); err != nil { + t.Fatalf("failed to count gift cards: %v", err) + } + if cardCount != 0 { + t.Errorf("expected the created card deleted by the clawback, got %d cards", cardCount) + } + var balance float64 + if err := db.Conn.QueryRow(pool, `SELECT balance FROM user_giftcard_balances WHERE user_id = $1`, customerID).Scan(&balance); err != nil { + t.Fatalf("failed to query user balance: %v", err) + } + if balance != 0.00 { + t.Errorf("expected the partially-spent redeemed balance clamped to zero, got £%.2f", balance) + } + + // The sale is failed and a CRITICAL notification surfaced. + var saleStatus string + if err := db.Conn.QueryRow(pool, `SELECT status FROM till_sales WHERE id = $1`, saleID).Scan(&saleStatus); err != nil { + t.Fatalf("failed to query till sale: %v", err) + } + if saleStatus != "failed" { + t.Errorf("expected till sale marked failed, got %q", saleStatus) + } + var notifCount int + if err := db.Conn.QueryRow(pool, `SELECT COUNT(*) FROM admin_notifications WHERE reason = 'critical_payment_log' AND user_id = $1`, customerID).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 partial clawback, got %d", notifCount) + } +} diff --git a/backend/handlers/payments/giftcard_redeem_touctoc_test.go b/backend/handlers/payments/giftcard_redeem_touctoc_test.go new file mode 100644 index 0000000..979c267 --- /dev/null +++ b/backend/handlers/payments/giftcard_redeem_touctoc_test.go @@ -0,0 +1,147 @@ +//go:build test && dev + +package payments + +// M16 gift-card double-redeem TOCTOU test. RedeemGiftCard serializes on a +// `SELECT ... FOR UPDATE` of the gift_cards row (READ COMMITTED re-reads the +// latest committed version after the lock is granted), so two CONCURRENT +// redemptions of the same code can never both succeed: exactly one wins the +// row lock, zeroes the balance and credits its user; the loser re-reads the +// committed row, sees redeemed_by set and is rejected 400. This test drives +// two real HTTP requests in parallel goroutines and asserts exactly one +// success, one rejection, and a single balance decrement + single +// redeem_to_balance audit transaction. + +import ( + "bytes" + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "sync" + "testing" + + "crussell/db" + "crussell/mw" + "crussell/testutils/fixtures" + "crussell/testutils/jwt" + + "github.com/go-chi/chi/v5" +) + +func TestGiftCardRedeem_ConcurrentSameCode_ExactlyOneWins(t *testing.T) { + // Seed on the pool directly (no SetupTestTx): each concurrent request + // must run its own transaction on the shared pool. + ctx := context.Background() + userA, err := fixtures.CreateTestUser(db.Conn) + if err != nil { + t.Fatalf("failed to create user A: %v", err) + } + userB, err := fixtures.CreateTestUser(db.Conn) + if err != nil { + t.Fatalf("failed to create user B: %v", err) + } + + var code string + if err := db.Conn.QueryRow(ctx, `INSERT INTO gift_cards (total_funds_added, amount_remaining) VALUES (50.00, 50.00) RETURNING id`).Scan(&code); err != nil { + t.Fatalf("failed to seed gift card: %v", err) + } + + t.Cleanup(func() { + _, _ = db.Conn.Exec(ctx, `DELETE FROM gift_card_transactions WHERE gift_card_id = $1`, code) + _, _ = db.Conn.Exec(ctx, `DELETE FROM user_giftcard_balances WHERE user_id IN ($1, $2)`, userA, userB) + _, _ = db.Conn.Exec(ctx, `DELETE FROM gift_cards WHERE id = $1`, code) + _, _ = db.Conn.Exec(ctx, `DELETE FROM users WHERE id IN ($1, $2)`, userA, userB) + }) + + router := chi.NewRouter() + router.Use(mw.RequireAuth) + router.Post("/api/user/giftcards/redeem", RedeemGiftCard) + + // Both redemptions start at the same barrier instant so the row lock + // genuinely contends. + start := make(chan struct{}) + type attempt struct { + status int + } + attempts := make([]attempt, 2) + var wg sync.WaitGroup + + redeem := func(userID, token string, idx int) { + defer wg.Done() + <-start + body, _ := json.Marshal(map[string]any{"code": code}) + req := httptest.NewRequest("POST", "/api/user/giftcards/redeem", bytes.NewBuffer(body)) + req.Header.Set("Authorization", "Bearer "+token) + req.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + attempts[idx].status = w.Code + } + + wg.Add(2) + go redeem(userA, jwt.GenerateTestToken(userA, "verified_email"), 0) + go redeem(userB, jwt.GenerateTestToken(userB, "verified_email"), 1) + close(start) + wg.Wait() + + successes, failures := 0, 0 + for _, a := range attempts { + switch a.status { + case http.StatusOK: + successes++ + case http.StatusBadRequest: + failures++ + default: + t.Errorf("unexpected redeem status %d", a.status) + } + } + if successes != 1 { + t.Errorf("expected exactly ONE successful redemption, got %d", successes) + } + if failures != 1 { + t.Errorf("expected exactly ONE rejected redemption, got %d", failures) + } + + // The balance was decremented exactly once (amount_remaining zeroed). + var amountRemaining float64 + var redeemedBy *string + if err := db.Conn.QueryRow(ctx, `SELECT amount_remaining, redeemed_by FROM gift_cards WHERE id = $1`, code).Scan(&amountRemaining, &redeemedBy); err != nil { + t.Fatalf("failed to query gift card: %v", err) + } + if amountRemaining != 0 { + t.Errorf("expected the card balance decremented to 0 exactly once, got %.2f", amountRemaining) + } + if redeemedBy == nil { + t.Fatal("expected the card redeemed to exactly one user") + } + + // Exactly one user holds the credited balance; the other has none. + var winners int + var balanceSum float64 + if err := db.Conn.QueryRow(ctx, ` + SELECT COUNT(*), COALESCE(SUM(balance), 0) + FROM user_giftcard_balances + WHERE user_id IN ($1, $2) + `, userA, userB).Scan(&winners, &balanceSum); err != nil { + t.Fatalf("failed to query user balances: %v", err) + } + if winners != 1 { + t.Errorf("expected exactly one credited balance row, got %d", winners) + } + if balanceSum != 50 { + t.Errorf("expected the winner credited 50.00, got %.2f", balanceSum) + } + + // Exactly one redeem_to_balance audit transaction exists. + var txCount int + if err := db.Conn.QueryRow(ctx, ` + SELECT COUNT(*) FROM gift_card_transactions + WHERE gift_card_id = $1 AND transaction_type = 'redeem_to_balance' + `, code).Scan(&txCount); err != nil { + t.Fatalf("failed to count redeem transactions: %v", err) + } + if txCount != 1 { + t.Errorf("expected exactly one redeem_to_balance transaction, got %d", txCount) + } +} diff --git a/backend/handlers/payments/giftcards.go b/backend/handlers/payments/giftcards.go index b5f4b05..ffd05d0 100644 --- a/backend/handlers/payments/giftcards.go +++ b/backend/handlers/payments/giftcards.go @@ -24,6 +24,7 @@ import ( "github.com/go-chi/chi/v5" "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgxpool" ) // --- Types --- @@ -120,6 +121,11 @@ type BuyGiftCardRequest struct { // enforced environment charges/persists a saved card only when this matches // the customer's pending code. VerificationCode string `json:"verification_code,omitempty"` + // ConsentVersion / ConsentAccepted: the customer's explicit versioned + // consent to the SCA-unavailable → 2FA fallback (C6), enforced server-side + // (403 consent_required) and recorded on the 2fa_fallback_charge audit row. + ConsentVersion *string `json:"consent_version,omitempty"` + ConsentAccepted bool `json:"consent_accepted"` } type RedeemGiftCardRequest struct { @@ -128,6 +134,45 @@ type RedeemGiftCardRequest struct { // --- Admin Handlers --- +// giftCardDailyCapLockKey is the session advisory-lock key that serializes one +// admin's gift-card value operations (CreateGiftCard / TopUpGiftCard / +// TransferGiftCard) so the £5,000/day cap check and the mutation recording the +// new value are atomic (M7). Keyed per admin: distinct admins never contend. +const giftCardDailyCapLockKey = "crussell:giftcard-daily-cap:" + +// acquireGiftCardDailyCapLock acquires the per-admin gift-card daily-cap lock +// (a bounded try-lock on a pinned pool connection, mirroring the payment +// handlers' serialization pattern). The daily cap check +// (adminGiftCardValueToday) and the transaction that records the new value must +// run under the same lock, or two concurrent top-ups could both read the day's +// value before either writes and both pass the cap — over-issuing value. +// Returns the pinned connection once the lock is held (the caller must defer +// capConn.Release() and releasePaymentLock(capConn, key)) or nil after writing +// the error response. +func acquireGiftCardDailyCapLock(ctx context.Context, w http.ResponseWriter, adminID string) (*pgxpool.Conn, bool) { + lockKey := giftCardDailyCapLockKey + adminID + pinConn, err := db.Conn.Acquire(ctx) + if err != nil { + log.Printf("Failed to acquire connection for gift-card daily-cap lock %s: %v", lockKey, err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return nil, false + } + lockOK, err := acquireAdvisoryLock(ctx, pinConn, lockKey) + if err != nil { + pinConn.Release() + log.Printf("Failed to acquire gift-card daily-cap lock %s: %v", lockKey, err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return nil, false + } + if !lockOK { + pinConn.Release() + log.Printf("Gift-card daily-cap lock %s not acquired within bound — another gift-card value operation for this admin is in progress", lockKey) + http.Error(w, "Another gift-card value operation is already in progress for this admin — please try again in a moment", http.StatusConflict) + return nil, false + } + return pinConn, true +} + func GetGiftCards(w http.ResponseWriter, r *http.Request) { ctx := r.Context() @@ -456,7 +501,16 @@ func CreateGiftCard(w http.ResponseWriter, r *http.Request) { } // Daily limit (owner decision): an admin may create/top-up/transfer at - // most £5,000 of gift-card value per UTC day. Enforced before any write. + // most £5,000 of gift-card value per UTC day. The cap check and the value + // recording transaction are serialized per admin (M7) so two concurrent + // operations cannot both read the day's value before either writes. + capPinConn, lockOK := acquireGiftCardDailyCapLock(ctx, w, adminID) + if !lockOK { + return + } + defer capPinConn.Release() + defer releasePaymentLock(capPinConn, giftCardDailyCapLockKey+adminID) + adminValueToday, err := adminGiftCardValueToday(ctx, db.Conn, adminID) if err != nil { log.Printf("Failed to query admin gift-card value today for %s: %v", adminID, err) @@ -593,7 +647,16 @@ func TopUpGiftCard(w http.ResponseWriter, r *http.Request) { } // Daily limit (owner decision): an admin may create/top-up/transfer at - // most £5,000 of gift-card value per UTC day. Enforced before any write. + // most £5,000 of gift-card value per UTC day. The cap check and the value + // recording transaction are serialized per admin (M7) so two concurrent + // top-ups cannot both read the day's value before either writes. + capPinConn, lockOK := acquireGiftCardDailyCapLock(ctx, w, adminID) + if !lockOK { + return + } + defer capPinConn.Release() + defer releasePaymentLock(capPinConn, giftCardDailyCapLockKey+adminID) + adminValueToday, err := adminGiftCardValueToday(ctx, db.Conn, adminID) if err != nil { log.Printf("Failed to query admin gift-card value today for %s: %v", adminID, err) @@ -757,8 +820,16 @@ func TransferGiftCard(w http.ResponseWriter, r *http.Request) { } // Daily limit (owner decision): an admin may create/top-up/transfer at - // most £5,000 of gift-card value per UTC day. Enforced before any write - // (and before the advisory lock acquisition below). + // most £5,000 of gift-card value per UTC day. The cap check and the value + // recording transaction are serialized per admin (M7) so two concurrent + // operations cannot both read the day's value before either writes. + capPinConn, lockOK := acquireGiftCardDailyCapLock(ctx, w, adminID) + if !lockOK { + return + } + defer capPinConn.Release() + defer releasePaymentLock(capPinConn, giftCardDailyCapLockKey+adminID) + adminValueToday, err := adminGiftCardValueToday(ctx, db.Conn, adminID) if err != nil { log.Printf("Failed to query admin gift-card value today for %s: %v", adminID, err) @@ -786,7 +857,7 @@ func TransferGiftCard(w http.ResponseWriter, r *http.Request) { return } defer pinConn.Release() - lockOK, err := acquireAdvisoryLock(ctx, pinConn, "crussell:giftcard-cancel:"+fromCardID) + lockOK, err = acquireAdvisoryLock(ctx, pinConn, "crussell:giftcard-cancel:"+fromCardID) if err != nil { log.Printf("Failed to acquire gift-card cancel lock for %s: %v", fromCardID, err) http.Error(w, "internal server error", http.StatusInternalServerError) @@ -1136,8 +1207,17 @@ func RedeemGiftCard(w http.ResponseWriter, r *http.Request) { // CleanupExpiredGiftCards the card still carries amount_remaining; without // this check the holder could redeem value that is already forfeit (the // nightly job moves it to gift_card_expired_balances and zeroes the card). - // Legacy cards with a NULL expiry_date are treated as unexpired. - if expiryDate.Valid && expiryDate.Time.Before(clock.Now()) { + // Legacy cards with a NULL expiry_date are treated as unexpired. The + // comparison uses the DATABASE clock (SELECT NOW()), the same clock that + // wrote expiry_date, so an app-clock drift can neither extend nor shorten + // card life. + expired, err := giftCardExpired(ctx, tx, expiryDate) + if err != nil { + log.Printf("Failed to check gift card expiry: %v", err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + if expired { http.Error(w, "This gift card has expired", http.StatusBadRequest) return } @@ -1219,9 +1299,9 @@ func GetGiftCardBalance(w http.ResponseWriter, r *http.Request) { if err != nil { if errors.Is(err, pgx.ErrNoRows) { if err := json.NewEncoder(w).Encode(map[string]any{ - "balance": 0.00, - "daily_buy_limit": float64(maxUserGiftCardDailyPence) / 100.0, - "daily_buy_spent": 0.00, + "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) } @@ -1242,9 +1322,9 @@ func GetGiftCardBalance(w http.ResponseWriter, r *http.Request) { } if err := json.NewEncoder(w).Encode(map[string]any{ - "balance": balance, - "daily_buy_limit": float64(maxUserGiftCardDailyPence) / 100.0, - "daily_buy_spent": spentToday, + "balance": balance, + "daily_buy_limit": float64(maxUserGiftCardDailyPence) / 100.0, + "daily_buy_spent": spentToday, }); err != nil { log.Printf("Failed to encode JSON response: %v", err) } @@ -1528,6 +1608,11 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { if !gateOK { return } + // C6: a fallback-authorized purchase must carry the customer's accepted + // consent (403 consent_required otherwise). + if !enforceSCAFallbackConsent(w, req.ConsentVersion, req.ConsentAccepted, twoFAFallbackUsed) { + return + } } var sourceID string @@ -1901,7 +1986,7 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { // money transaction commits (a failed audit write must never roll back a // completed charge). The actor is the customer's own userID. if twoFAFallbackUsed { - insertTwoFAFallbackAudit(ctx, userID, userID, paymentResult.CardLast4, buyPaymentID, "gift-card purchase authorized via 2FA fallback (SCA unavailable)") + insertTwoFAFallbackAudit(ctx, userID, userID, paymentResult.CardLast4, buyPaymentID, "gift-card purchase authorized via 2FA fallback (SCA unavailable)", consentVersionValue(req.ConsentVersion), req.ConsentAccepted) } w.WriteHeader(http.StatusCreated) @@ -1968,6 +2053,25 @@ func redactEmail(email string) string { return email[:2] + "***@" + email[at+1:] } +// giftCardExpired reports whether a card's expiry_date has passed, comparing +// against the DATABASE clock (SELECT NOW()) — the SAME clock source the +// CreateGiftCard / TopUpGiftCard / BuyGiftCard / TransferGiftCard expiry WRITES +// use (expiry_date = NOW() + months). Every expiry comparison in giftcards.go +// must use this DB clock, never crussell/clock.Now(): an application clock that +// drifts behind the DB would extend card life past the expiry the DB itself +// enforces, while one running ahead would cut it short. A card with a NULL +// expiry_date (legacy) is treated as unexpired. +func giftCardExpired(ctx context.Context, q db.Querier, expiryDate sql.NullTime) (bool, error) { + if !expiryDate.Valid { + return false, nil + } + var dbNow time.Time + if err := q.QueryRow(ctx, `SELECT NOW()`).Scan(&dbNow); err != nil { + return false, err + } + return expiryDate.Time.Before(dbNow), nil +} + type ExpiredBalance struct { ID string `json:"id"` AccountID *string `json:"account_id,omitempty"` @@ -2314,6 +2418,16 @@ func assessGiftCardCancellation(ctx context.Context, q db.Querier, code string, } } + // The expiry comparison uses the DATABASE clock (SELECT NOW()), the same + // clock that wrote expiry_date, so the assessment can never disagree with + // the DB-enforced card life. + expired, err := giftCardExpired(ctx, q, expiryDate) + if err != nil { + log.Printf("Failed to check gift card expiry for %s: %v", code, err) + st.CancellationReason = "Unable to verify this gift card's expiry" + return st + } + switch { case isInventory: st.CancellationReason = "This card was created as shop stock, not purchased online" @@ -2325,7 +2439,7 @@ func assessGiftCardCancellation(ctx context.Context, q db.Querier, code string, } else { st.CancellationReason = "This gift card has been topped up, transferred, or partially spent in a way that cannot be verified" } - case expiryDate.Valid && expiryDate.Time.Before(clock.Now()): + case expired: st.CancellationReason = "This gift card has expired" case purchasedAt.Before(clock.Now().Add(-giftCardCoolingOffPeriod)): st.CancellationReason = "The 14-day cancellation period has expired" @@ -2574,7 +2688,16 @@ func cancelGiftCardForUser(ctx context.Context, w http.ResponseWriter, r *http.R partialRefund = true } - if expiryDate.Valid && expiryDate.Time.Before(clock.Now()) { + // The expiry comparison uses the DATABASE clock (SELECT NOW()), the same + // clock that wrote expiry_date, so an app-clock drift can neither extend + // nor shorten the cancellation window. + expired, err := giftCardExpired(ctx, tx, expiryDate) + if err != nil { + log.Printf("Failed to check gift card expiry for %s: %v", code, err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + if expired { http.Error(w, "This gift card has already expired and cannot be cancelled", http.StatusBadRequest) return } @@ -2773,7 +2896,15 @@ func cancelGiftCardForUser(ctx context.Context, w http.ResponseWriter, r *http.R if partialRefund { expectRemaining = remaining } - if revIsInventory || revRedeemedBy.Valid || !approxEqual(revTotalFunds, purchaseAmount) || !approxEqual(revRemaining, expectRemaining) || (revExpiry.Valid && revExpiry.Time.Before(clock.Now())) { + // The expiry comparison uses the DATABASE clock (SELECT NOW()) so the + // re-verification can never disagree with the clock that wrote expiry_date. + revExpired, err := giftCardExpired(ctx, rtx, revExpiry) + if err != nil { + log.Printf("Failed to re-check gift card expiry for %s: %v", code, err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + if revIsInventory || revRedeemedBy.Valid || !approxEqual(revTotalFunds, purchaseAmount) || !approxEqual(revRemaining, expectRemaining) || revExpired { // The card changed between the eligibility commit and the reversal — // redeemed, spent, transferred, or expired. Do NOT refund on top of // the live balance (double value). The reversal tx has no writes and diff --git a/backend/handlers/payments/giftcards_test.go b/backend/handlers/payments/giftcards_test.go index b7a7c35..cb0ccd1 100644 --- a/backend/handlers/payments/giftcards_test.go +++ b/backend/handlers/payments/giftcards_test.go @@ -7,8 +7,10 @@ import ( "context" "database/sql" "encoding/json" + "math" "net/http" "net/http/httptest" + "sync" "testing" "time" @@ -2650,3 +2652,163 @@ func TestBuyGiftCard_PendingRow_StoresSquareSourceID(t *testing.T) { t.Errorf("expected square_source_id %q (the exact CreatePayment SourceID), got %q", "cnon:card-nonce-ok", sourceID) } } + +// ============================================================================= +// M7 — £5,000/day admin gift-card cap is race-free +// ============================================================================= + +// TestTopUpGiftCard_DailyCap_Concurrent pins the M7 daily-cap TOCTOU fix: N +// concurrent top-ups by the same admin must never let the cumulative issued +// value exceed the £5,000/day cap. The cap check (adminGiftCardValueToday) and +// the transaction recording the top-up value run under one per-admin advisory +// lock, so every check sees the previous top-up's committed row — the excess +// requests are rejected. Without the lock two top-ups in the same batch read +// the same pre-write cumulative value and both pass, over-issuing value. +// +// The per-transaction £250 cap bounds each top-up, so exceeding the £5,000 +// daily cap needs 21 top-ups of £250; a semaphore bounds how many run +// simultaneously (each handler holds one pool conn for its advisory lock and +// one for its transaction). The admin, the card, and every handler invocation +// run directly against the REAL pool (no per-test transaction), so the batches +// exercise genuine cross-connection concurrency exactly like production. +func TestTopUpGiftCard_DailyCap_Concurrent(t *testing.T) { + ctx := context.Background() + + adminID, err := fixtures.CreateTestUser(db.Conn) + if err != nil { + t.Fatalf("failed to create admin: %v", err) + } + if _, err := db.Conn.Exec(ctx, "UPDATE users SET account_role = 'admin' WHERE id = $1", adminID); err != nil { + t.Fatalf("failed to promote admin: %v", err) + } + var cardID string + if err := db.Conn.QueryRow(ctx, ` + INSERT INTO gift_cards (total_funds_added, amount_remaining, created_by) + VALUES (0, 0, $1) RETURNING id`, adminID).Scan(&cardID); err != nil { + t.Fatalf("failed to create gift card: %v", err) + } + t.Cleanup(func() { + cctx := context.Background() + _, _ = db.Conn.Exec(cctx, `DELETE FROM admin_audit_log WHERE admin_id = $1 OR target_user_id = $1 OR target_gift_card_id = $2`, adminID, cardID) + _, _ = db.Conn.Exec(cctx, `DELETE FROM gift_card_transactions WHERE gift_card_id = $1`, cardID) + _, _ = db.Conn.Exec(cctx, `DELETE FROM gift_cards WHERE id = $1`, cardID) + _, _ = db.Conn.Exec(cctx, `DELETE FROM users WHERE id = $1`, adminID) + }) + + token := jwt.GenerateTestToken(adminID, "admin") + + const perTopUp = 250.00 // £250 per top-up (at the £250 per-transaction cap) + const totalOps = 21 // 21 × £250 = £5,250 > the £5,000 daily cap + const concurrencyLimit = 6 // at most 6 handlers in flight (bounded pool conns) + + sem := make(chan struct{}, concurrencyLimit) + start := make(chan struct{}) + var wg sync.WaitGroup + var mu sync.Mutex + successes := 0 + failCodes := map[int]int{} + for i := 0; i < totalOps; i++ { + wg.Add(1) + go func() { + defer wg.Done() + <-start + sem <- struct{}{} + defer func() { <-sem }() + body, _ := json.Marshal(TopUpGiftCardRequest{Amount: perTopUp, PaymentMethod: "cash"}) + r := httptest.NewRequest(http.MethodPut, "/api/admin/gift-cards/"+cardID+"/topup", bytes.NewReader(body)) + r.Header.Set("Authorization", "Bearer "+token) + r.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + router := chi.NewRouter() + router.Use(mw.RequireAuth) + router.With(mw.RequireAdmin).Put("/api/admin/gift-cards/{id}/topup", TopUpGiftCard) + router.ServeHTTP(w, r) + mu.Lock() + if w.Code == http.StatusOK { + successes++ + } else { + failCodes[w.Code]++ + } + mu.Unlock() + }() + } + close(start) + wg.Wait() + + // Money-safety invariant under serialization: at most 20 of the 21 top-ups + // may succeed (20 × £250 = £5,000 = the inclusive cap; the 21st would land + // the day on £5,250 and must be rejected). A rejected attempt surfaces as + // either the 400 cap rejection or a 409 from the bounded try-lock giving up + // under heavy contention — both are the designed backpressure and neither + // records value. The OLD cap check (no per-admin lock) lets each batch read + // the pre-write cumulative value so all 21 succeed, overshooting the cap — + // `successes > 20` (or a cumulative over the cap below) is the regression + // signal this test must catch. + if successes < 1 || successes > 20 { + t.Errorf("expected between 1 and 20 of 21 concurrent top-ups to succeed under the £5,000 cap, got %d (cumulative value £%.2f); failure codes: %v", successes, perTopUp*float64(successes), failCodes) + } + + // The day's issued value (the cap signal) must never exceed the cap. + issuedToday, err := adminGiftCardValueToday(ctx, db.Conn, adminID) + if err != nil { + t.Fatalf("failed to query today's issued value: %v", err) + } + if int64(math.Round(issuedToday*100)) > maxAdminGiftCardDailyPence { + t.Errorf("cumulative daily issued value £%.2f exceeds the £5,000 cap", issuedToday) + } + + // The card must hold exactly the value of the successful top-ups. + var remaining float64 + if err := db.Conn.QueryRow(ctx, `SELECT amount_remaining FROM gift_cards WHERE id = $1`, cardID).Scan(&remaining); err != nil { + t.Fatalf("failed to query gift card: %v", err) + } + if remaining != perTopUp*float64(successes) { + t.Errorf("expected card balance £%.2f, got £%.2f", perTopUp*float64(successes), remaining) + } +} + +// ============================================================================= +// Expiry enforcement — DB clock boundary +// ============================================================================= + +// TestRedeemGiftCard_ExpiryEdge_DBClock pins the unified expiry-clock source: +// redemption expiry enforcement compares expiry_date against the DATABASE clock +// (SELECT NOW()), the same clock that writes expiry_date. A card whose expiry +// passed one second before the DB clock is rejected as expired; a card whose +// expiry is a few seconds ahead of the DB clock still redeems. An app-clock +// drift can therefore neither extend nor shorten a card's life. +func TestRedeemGiftCard_ExpiryEdge_DBClock(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + if err != nil { + t.Fatalf("failed to create user: %v", err) + } + token := jwt.GenerateTestToken(userID, "verified_email") + + // A card whose expiry passed one second before the DB clock's now must be + // rejected as expired. + const expiredCardID = "3a3a3a3a3a01" + if _, err := tx.Exec(ctx, ` + INSERT INTO gift_cards (id, total_funds_added, amount_remaining, expiry_date) + VALUES ($1, 50.00, 50.00, NOW() - INTERVAL '1 second') + `, expiredCardID); err != nil { + t.Fatalf("failed to create expired-edge card: %v", err) + } + if w := redeemCodeRequest(t, token, tx.(pgx.Tx), expiredCardID); w.Code != http.StatusBadRequest { + t.Errorf("expected 400 for a card expired just before the DB clock, got %d. body: %s", w.Code, w.Body.String()) + } + + // A card whose expiry is a few seconds ahead of the DB clock must still + // redeem (the redemption check runs long before the +5s expiry passes). + const liveCardID = "3a3a3a3a3a02" + if _, err := tx.Exec(ctx, ` + INSERT INTO gift_cards (id, total_funds_added, amount_remaining, expiry_date) + VALUES ($1, 50.00, 50.00, NOW() + INTERVAL '5 seconds') + `, liveCardID); err != nil { + t.Fatalf("failed to create live-edge card: %v", err) + } + if w := redeemCodeRequest(t, token, tx.(pgx.Tx), liveCardID); w.Code != http.StatusOK { + t.Errorf("expected 200 for a card expiring just after the DB clock, got %d. body: %s", w.Code, w.Body.String()) + } +}