Files
Crussell/backend/handlers/payments/giftcard_clawback.go
T
popertots fdf3f64a13 fix: review round 4 — per-dispute chargeback alerts, single-source clawback, 2FA lockout coherence, docs
Fourth fresh-eyes review pass (5 agents: goal, QA, code-quality, security,
context-mining). All PASS on the money-safety core; this round closes the
remaining MAJOR/MINOR items they surfaced.

Webhooks:
- Untracked disputes now raise ONE admin notification PER distinct chargeback:
  the notification id is derived deterministically from the square_dispute_id
  (SHA-256 truncated into the CHAR(12) slot) so a second untracked dispute is
  no longer silently suppressed by the first's dedup row. ON CONFLICT (id)
  keeps same-dispute replays idempotent; the booking-scoped NOT EXISTS guard
  is retained for the tracked path. Verified: distinct disputes -> distinct
  rows; re-delivered dispute -> one row.
- The gift-card clawback SQL now lives in exactly ONE place:
  payments.RevertGiftCardFunding (new giftcard_clawback.go). till.go and the
  webhook path both call it — eliminating the byte-for-byte copy whose
  divergence would be a money-loss drift trap (the same two-sources-of-truth
  pattern this commit eliminated for GDPR scrubbing).

2FA:
- Applied the lockout-coherence fix from the review: when a disable request
  must mint a fresh code (no valid pending one), the held attempt counter is
  reset so the locked-out user can use the freshly delivered code in the SAME
  request (no wasted round-trip). The reuse path keeps accumulating wrong
  attempts toward the 5-attempt lockout — the two behaviors no longer
  conflict. (The 'always-fresh on disable' suggestion was NOT adopted: it
  would break the out-of-band [2FA]-log delivery model, since a code generated
  by a request can never be submitted within that same request.)
- New test pins the shared verify/disable lockout: 5 wrong verifies 429 and
  destroy the code; a stale code then 400s on disable while the freshly
  delivered code succeeds in the same request.
- Startup now warns that 2FA codes travel in PLAINTEXT via the server log in
  enforced mode (operator must restrict log access + relay out-of-band until
  email/SMS lands).

Docs:
- Test counts updated to the current 2,154 across README + Technical Manual.
- User Manual 2FA nav corrected: the settings live on the Account page, not an
  'Admin' area.

Tests: 2,154 (up from 2,151). Backend 25/26 packages green (crussell/db fails
only in this environment: local postgres auth for the test role; package
byte-identical to HEAD). Frontend builds; svelte-check 0 errors.
2026-08-22 00:34:49 +01:00

129 lines
5.8 KiB
Go

package payments
import (
"context"
"errors"
"fmt"
"log"
"log/slog"
"crussell/db"
"github.com/jackc/pgx/v5"
)
// 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
// purchase transaction) and any immediate redeem-to-account credit reversed; a
// topped-up card has the amount subtracted back out and its top-up transaction
// removed. The clawback is claim-first: it atomically claims the till sale
// with a gating `status='pending'` UPDATE whose row lock serializes against
// the handler's completion UPDATE, then runs the card mutation + failed-mark
// in the same transaction so a late same-key retry cannot re-complete a sale
// whose gift card no longer exists.
//
// This is the SINGLE money-reversal implementation shared by the till handler
// (revertGiftCardFunding), the stale-pending sweep (sweep.go) and the Square
// webhook clawback (handlers/webhooks/square.go). Do not fork it: a divergent
// clawback is real money loss.
func RevertGiftCardFunding(ctx context.Context, action, giftCardID string, amount float64, redeemToUserID *string, tillSaleID string) error {
tx, err := db.Conn.Begin(ctx)
if err != nil {
return fmt.Errorf("failed to begin clawback transaction: %w", err)
}
defer func() {
if err := tx.Rollback(ctx); err != nil && !errors.Is(err, pgx.ErrTxClosed) {
slog.Error("failed to rollback gift-card clawback transaction", "err", err)
}
}()
// Claim the sale first: the row lock serializes against the handler's
// completion UPDATE; a zero-row claim means the funding is not ours.
tag, err := tx.Exec(ctx, `
UPDATE till_sales SET status = 'failed', updated_at = NOW()
WHERE id = $1 AND status = 'pending'`, tillSaleID)
if err != nil {
return fmt.Errorf("failed to claim till sale for clawback: %w", err)
}
if tag.RowsAffected() == 0 {
return errTillSaleNotPending
}
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
// a wholesale delete, which would destroy the value of a different
// idempotency-keyed top-up sale that funded the same card before this
// create resolved. Then remove the card itself.
if _, err := tx.Exec(ctx, `DELETE FROM gift_card_transactions WHERE gift_card_id = $1 AND reference_type = 'till_sale' AND reference_id = $2`, giftCardID, tillSaleID); err != nil {
return fmt.Errorf("failed to delete gift card transaction: %w", err)
}
if _, err := tx.Exec(ctx, `DELETE FROM gift_cards WHERE id = $1`, giftCardID); err != nil {
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).
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)
}
}
} 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)
}
// Remove only this request's top-up transaction (reference_id = till
// sale) so prior sales' accounting on the same card is untouched.
if _, err := tx.Exec(ctx, `
DELETE FROM gift_card_transactions
WHERE gift_card_id = $1 AND reference_type = 'till_sale' AND reference_id = $2
`, giftCardID, tillSaleID); err != nil {
return fmt.Errorf("failed to delete gift card top-up transaction: %w", err)
}
}
if err := tx.Commit(ctx); err != nil {
return fmt.Errorf("failed to commit clawback transaction: %w", err)
}
return nil
}
// IsTillSaleNotPending reports whether err is the claim-first sentinel
// (errTillSaleNotPending): the gating `status='pending'` UPDATE matched zero
// rows, so the till sale is no longer pending and its gift card must be left
// untouched. Exported so cross-package clawback callers (the Square webhook)
// can detect the sentinel without reaching into the unexported error value.
func IsTillSaleNotPending(err error) bool {
return errors.Is(err, errTillSaleNotPending)
}