fix: gift-card money fixes — partial clawback surfaces CRITICAL (M6), per-admin daily-cap lock (M7), DB-clock expiry

- M6: RevertGiftCardFunding no longer silently drops unreclaimable money. A
  partially-spent create/top-up claws back everything still on the card/balance
  (GREATEST(0, ...) clamp instead of the old guarded 0-row block), inserts a
  CRITICAL admin notification, and returns errClawbackPartiallyReversed so every
  caller (till handler, stale-pending sweep, webhook) surfaces the residual
  without forking the money logic; balance comparisons use pence (penceLess).
- M7: the £5,000/day admin gift-card value cap (create/top-up/transfer) is now
  serialized per-admin under a bounded advisory try-lock
  (acquireGiftCardDailyCapLock) so two concurrent operations cannot both read
  the day's value before either writes and over-issue value.
- M4/M3: every gift-card expiry comparison now reads the DATABASE clock
  (giftCardExpired -> SELECT NOW()), the same clock that wrote expiry_date, so
  app-clock drift can neither extend nor shorten card life; applied on redeem,
  cancellation assessment, cancel-for-user and the reversal re-verification.
- C6 consent fields carried on BuyGiftCardRequest and enforced on the
  (now unreachable) 2FA fallback audit path; fallback audit row captures the
  versioned consent.
This commit is contained in:
2026-08-22 00:34:50 +01:00
parent 2a47021673
commit 42130865f4
5 changed files with 738 additions and 58 deletions
+101 -41
View File
@@ -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
}
@@ -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(&notifCount); 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(&notifCount); 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)
}
}
@@ -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)
}
}
+148 -17
View File
@@ -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
+162
View File
@@ -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())
}
}