fix: loop-B full-scope adversarial findings — tip-excluded detail endpoints, £0-charge guard, 24h window, 2FA single-use everywhere
Loop B full-scope red-team (money/security/dup-mod) findings: - CRITICAL: booking detail handlers (GetBookingHandler/GetAdminBookingHandler) now exclude payment_type='tip' from amount_paid — a tip before the final balance no longer undercharges the booking (bookings.go x3 sites) - HIGH: A6 deposit clamp adds a zero-guard — when the eligible discount covers the entire deposit, the flow returns deposit_covered_by_discount instead of charging £0 at Square (real Square rejects £0; the mock accepted it); square_dev CreatePayment + CreateRefund now reject Amount <= 0 (mock/prod parity) - HIGH: replayLegitimateRetryWindow restored to 22h (== stalePendingKeyedAge) so sweep-produced duplicate charges are still auto-refunded, not rescued-and-hidden - HIGH: 2FA single-use consume-at-gate applied to ALL saved-card charge gates (booking 2263, admin saved-card 960, tip 4483, till 967, gift-card purchase 1482) with re-issue-on-failed-charge on each; pending-reuse retries keep their code - MEDIUM: 2FA re-issue now fires only when the gate actually consumed a code (fresh saved-card path) — new-card failures no longer silently burn a standing code - MEDIUM: pre_start tip-exclusion consistent across admin lists + detail handlers (bookings.go) - MEDIUM: remaining-balance counts pending refunds (service.go) — capacity consistent with GetBookingPaymentInfo - Mock CreatePayment/CreateRefund reject £0 amounts (INVALID_REQUEST_ERROR) for dev/prod parity 26/26 backend packages; 80/80 frontend tests + build; env-docs 41/41.
This commit is contained in:
@@ -681,6 +681,7 @@ func GetAllAdminBookingsHandler(w http.ResponseWriter, r *http.Request) {
|
||||
SELECT COALESCE(SUM(amount), 0) AS pre_start_amount_paid
|
||||
FROM payments
|
||||
WHERE booking_id = b.id AND status = 'completed' AND created_at < b.start_time
|
||||
AND payment_type <> 'tip'
|
||||
) pre_pay ON true
|
||||
`
|
||||
|
||||
@@ -1287,7 +1288,13 @@ func GetAdminBookingHandler(w http.ResponseWriter, r *http.Request) {
|
||||
p.InvoiceNumber = &num
|
||||
}
|
||||
booking.Payments = append(booking.Payments, p)
|
||||
if p.Status == "completed" {
|
||||
// A tip is gratuity paid beyond the booking total — it must not reduce
|
||||
// the balance owed. This mirrors the list views (which filter
|
||||
// payment_type <> 'tip' in SQL) and GetBookingPaymentInfo; without the
|
||||
// exclusion AmountDue would be understated by the tip and the frontend
|
||||
// would charge less than the true remaining balance, leaving the
|
||||
// booking never completed and the merchant short.
|
||||
if p.Status == "completed" && p.PaymentType != "tip" {
|
||||
amountPaid += p.Amount
|
||||
if p.CreatedAt.Before(booking.StartTime) {
|
||||
preStartAmountPaid += p.Amount
|
||||
@@ -1679,7 +1686,13 @@ func UpdateBookingServicesHandler(w http.ResponseWriter, r *http.Request) {
|
||||
p.InvoiceNumber = &num
|
||||
}
|
||||
booking.Payments = append(booking.Payments, p)
|
||||
if p.Status == "completed" {
|
||||
// A tip is gratuity paid beyond the booking total — it must not reduce
|
||||
// the balance owed. This mirrors the list views (which filter
|
||||
// payment_type <> 'tip' in SQL) and GetBookingPaymentInfo; without the
|
||||
// exclusion AmountDue would be understated by the tip and the frontend
|
||||
// would charge less than the true remaining balance, leaving the
|
||||
// booking never completed and the merchant short.
|
||||
if p.Status == "completed" && p.PaymentType != "tip" {
|
||||
amountPaid += p.Amount
|
||||
if p.CreatedAt.Before(booking.StartTime) {
|
||||
preStartAmountPaid += p.Amount
|
||||
@@ -1775,6 +1788,7 @@ func SearchAdminBookingsHandler(w http.ResponseWriter, r *http.Request) {
|
||||
SELECT COALESCE(SUM(amount), 0) AS pre_start_amount_paid
|
||||
FROM payments
|
||||
WHERE booking_id = b.id AND status = 'completed' AND created_at < b.start_time
|
||||
AND payment_type <> 'tip'
|
||||
) pre_pay ON true
|
||||
-- NOTE: ILIKE with leading wildcard prevents B-tree index usage.
|
||||
-- At scale, replace with pg_trgm GIN index: CREATE INDEX idx_bookings_search_trgm ON bookings USING GIN (id gin_trgm_ops, notes gin_trgm_ops);
|
||||
@@ -3304,7 +3318,13 @@ func GetBookingHandler(w http.ResponseWriter, r *http.Request) {
|
||||
p.CreatedBy = &pCreatedBy.String
|
||||
}
|
||||
|
||||
if p.Status == "completed" {
|
||||
// A tip is gratuity paid beyond the booking total — it must not reduce
|
||||
// the balance owed. This mirrors the list views (which filter
|
||||
// payment_type <> 'tip' in SQL) and GetBookingPaymentInfo; without the
|
||||
// exclusion AmountDue would be understated by the tip and the frontend
|
||||
// would charge less than the true remaining balance, leaving the
|
||||
// booking never completed and the merchant short.
|
||||
if p.Status == "completed" && p.PaymentType != "tip" {
|
||||
amountPaid += p.Amount
|
||||
if p.CreatedAt.Before(booking.StartTime) {
|
||||
preStartAmountPaid += p.Amount
|
||||
|
||||
@@ -1469,12 +1469,17 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) {
|
||||
// and SAVING a new card during this purchase (SaveCard), mirroring
|
||||
// CreateBookingPayment/CreateTipPayment. A one-off new-card (nonce) charge
|
||||
// that is not saved is not gated.
|
||||
// consume=false (MEDIUM-2): the code is verified here but only NULLed
|
||||
// inside the completed-charge transaction below (ConsumePendingCode), so a
|
||||
// failed/ambiguous Square charge does NOT burn the operator-relayed code
|
||||
// and a same-key retry can re-verify the SAME code.
|
||||
// consume=!reuse (LOW 6a): a FRESH purchase verifies WITH consumption —
|
||||
// the code is single-use at the gate, closing the TOCTOU where a
|
||||
// verified-but-unconsumed code could authorize a second charge within its
|
||||
// lifetime — and a failed Square charge re-issues a fresh code
|
||||
// (reissueTwoFACodeAfterFailedCharge below). A pending-reuse retry
|
||||
// (reusePendingID != "") verifies WITHOUT consuming: the code was re-issued
|
||||
// for exactly this retry and the completed-charge transaction below
|
||||
// (ConsumePendingCode) burns it on terminal success, so a retry that fails
|
||||
// again keeps its code for one more attempt.
|
||||
if (req.CardID != nil && *req.CardID != "") || req.SaveCard {
|
||||
if !requireTwoFactorForCardAccess(w, r, paymentService, userID, req.VerificationCode, false) {
|
||||
if !requireTwoFactorForCardAccess(w, r, paymentService, userID, req.VerificationCode, reusePendingID == "") {
|
||||
return
|
||||
}
|
||||
}
|
||||
@@ -1691,6 +1696,13 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) {
|
||||
paymentResult, err := SquareClient.CreatePayment(ctx, paymentReq)
|
||||
if err != nil {
|
||||
log.Printf("Failed to process gift card purchase payment: %v", err)
|
||||
// The gate consumed the 2FA code for a FRESH saved-card purchase —
|
||||
// re-issue so the same-key retry has a live code to verify. A
|
||||
// pending-reuse retry verified without consuming at the gate, so its
|
||||
// code survives for one more attempt.
|
||||
if ((req.CardID != nil && *req.CardID != "") || req.SaveCard) && reusePendingID == "" {
|
||||
reissueTwoFACodeAfterFailedCharge(ctx, userID)
|
||||
}
|
||||
// Payment record intentionally left as 'pending' for manual retry.
|
||||
http.Error(w, "Payment failed", chargeFailureStatus(err))
|
||||
return
|
||||
|
||||
@@ -949,13 +949,15 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) {
|
||||
// AFTER the idempotency dedup/reuse switch above: a same-key retry of
|
||||
// an already-completed payment short-circuits there and returns the
|
||||
// existing result WITHOUT demanding a fresh code — no new money moves,
|
||||
// so no new authorization is needed. Pending-reuse retries and fresh
|
||||
// charges still pass through the gate.
|
||||
// consume=false (MEDIUM-2): the code is verified here but only NULLed
|
||||
// inside the completed-charge transaction below (ConsumePendingCode),
|
||||
// so a failed/ambiguous Square charge does NOT burn the operator-relayed
|
||||
// code and a same-key retry can re-verify the SAME code.
|
||||
if bookingUserID.Valid && !requireTwoFactorForCardAccess(w, r, service, bookingUserID.String, req.VerificationCode, false) {
|
||||
// so no new authorization is needed. consume=!reusePendingRecord
|
||||
// (finding 4): a FRESH charge verifies WITH consumption — the code is
|
||||
// single-use at the gate, closing the TOCTOU where a verified-but-
|
||||
// unconsumed code could authorize a second charge — and a pending-reuse
|
||||
// retry verifies WITHOUT consuming, so a retry that fails again keeps
|
||||
// its code for one more attempt (the completed-charge transaction
|
||||
// consumes it on terminal success).
|
||||
reusePendingRecord := paymentID != ""
|
||||
if bookingUserID.Valid && !requireTwoFactorForCardAccess(w, r, service, bookingUserID.String, req.VerificationCode, !reusePendingRecord) {
|
||||
return
|
||||
}
|
||||
|
||||
@@ -1066,6 +1068,15 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) {
|
||||
paymentResult, err := SquareClient.CreatePayment(r.Context(), paymentReq)
|
||||
if err != nil {
|
||||
log.Printf("Failed to process saved-card payment: %v (error_code=%q)", err, square.ErrorCode(err))
|
||||
// The gate consumed the 2FA code for a fresh saved-card charge —
|
||||
// re-issue so the same-key retry has a live code to verify
|
||||
// (mirrors CreateBookingPayment's post-failure re-issue, finding
|
||||
// 4). Only runs when the gate actually ran (the booking's user is
|
||||
// known); a pending-reuse retry verified WITHOUT consuming, so a
|
||||
// fresh code never invalidates anything that still needs verifying.
|
||||
if bookingUserID.Valid {
|
||||
reissueTwoFACodeAfterFailedCharge(r.Context(), bookingUserID.String)
|
||||
}
|
||||
http.Error(w, "Payment failed", chargeFailureStatus(err))
|
||||
return
|
||||
}
|
||||
@@ -2212,6 +2223,30 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) {
|
||||
}
|
||||
}
|
||||
|
||||
// A6 (money): when the eligible campaign credit covers the ENTIRE
|
||||
// remaining obligation, the deposit charge clamps to £0. Charging £0 at
|
||||
// Square is a provable INVALID_REQUEST_ERROR in production (the pending
|
||||
// row + Square call would fail forever and block the flow), and the dev
|
||||
// mock used to ACCEPT £0 and mint a completed £0 deposit that consumed
|
||||
// the discount — leaving the booking unpaid and the 'full' balance
|
||||
// charge to overcharge later. There is nothing to charge, so skip the
|
||||
// Square call entirely and report the discount-covered deposit; the
|
||||
// flow completes without moving any money. The discount rows themselves
|
||||
// are applied by the next real charge or at booking completion
|
||||
// (applyEligibleCampaignsAtPayment).
|
||||
if chargeAmount <= 0 {
|
||||
log.Printf("Deposit for booking %s fully covered by %d pence of eligible campaign credit — skipping the Square charge", bookingID, eligibleDiscountPence)
|
||||
mw.RespondJSON(w, http.StatusOK, map[string]any{
|
||||
"id": "",
|
||||
"booking_id": bookingID,
|
||||
"payment_type": req.PaymentType,
|
||||
"status": "completed",
|
||||
"amount": 0,
|
||||
"deposit_covered_by_discount": true,
|
||||
})
|
||||
return
|
||||
}
|
||||
|
||||
var sourceID string
|
||||
var savedCardID *string
|
||||
var savedCardCustomerID string
|
||||
@@ -2353,9 +2388,17 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) {
|
||||
paymentResult, err := SquareClient.CreatePayment(r.Context(), paymentReq)
|
||||
if err != nil {
|
||||
log.Printf("Failed to create payment: %v (error_code=%q)", err, square.ErrorCode(err))
|
||||
// The gate consumed the 2FA code for a fresh charge — re-issue so the
|
||||
// same-key retry has a live code to verify.
|
||||
reissueTwoFACodeAfterFailedCharge(r.Context(), userID)
|
||||
// The gate consumed the 2FA code for a fresh saved-card charge —
|
||||
// re-issue so the same-key retry has a live code to verify. A NEW-CARD
|
||||
// (cnon) charge never gated and involves no code: re-issuing here would
|
||||
// overwrite the customer's standing pending code with a fresh
|
||||
// undelivered one, silently burning the code the operator relayed
|
||||
// (finding 5). Pending-reuse saved-card retries verified WITHOUT
|
||||
// consuming, so re-issuing keeps a live code available for the retry
|
||||
// (the completed-charge transaction burns it on terminal success).
|
||||
if req.CardID != nil && *req.CardID != "" {
|
||||
reissueTwoFACodeAfterFailedCharge(r.Context(), userID)
|
||||
}
|
||||
http.Error(w, "Payment failed", chargeFailureStatus(err))
|
||||
return
|
||||
}
|
||||
@@ -4429,10 +4472,15 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) {
|
||||
// idempotency dedup's completed short-circuit (Loop B MEDIUM): a same-key
|
||||
// lost-response retry returns the already-completed payment above without
|
||||
// re-entering the gate, so its single-use code (already consumed by the
|
||||
// original attempt) is never re-rejected as "expired". Pending-reuse and
|
||||
// fresh paths still gate — a new charge may move at Square.
|
||||
// original attempt) is never re-rejected as "expired". consume=!reusePendingRecord
|
||||
// (finding 4): a FRESH charge verifies WITH consumption — the code is
|
||||
// single-use at the gate, closing the TOCTOU where a verified-but-
|
||||
// unconsumed code could authorize a second charge — and a pending-reuse
|
||||
// retry verifies WITHOUT consuming, so a retry that fails again keeps its
|
||||
// code for one more attempt (the completed-charge transaction consumes it
|
||||
// on terminal success).
|
||||
if req.CardID != nil && *req.CardID != "" {
|
||||
if !requireTwoFactorForCardAccess(w, r, service, userID, req.VerificationCode, false) {
|
||||
if !requireTwoFactorForCardAccess(w, r, service, userID, req.VerificationCode, !reusePendingRecord) {
|
||||
return
|
||||
}
|
||||
}
|
||||
@@ -4544,6 +4592,14 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) {
|
||||
if err != nil {
|
||||
log.Printf("Failed to create tip payment: %v (error_code=%q)", err, square.ErrorCode(err))
|
||||
// Payment record intentionally left as 'pending' for manual retry.
|
||||
// The gate consumed the 2FA code for a fresh saved-card charge —
|
||||
// re-issue so the same-key retry has a live code to verify (mirrors
|
||||
// CreateBookingPayment's post-failure re-issue, finding 4). A NEW-CARD
|
||||
// (cnon) charge never gated and involves no code — re-issuing would
|
||||
// overwrite a standing pending code with an undelivered one (finding 5).
|
||||
if req.CardID != nil && *req.CardID != "" {
|
||||
reissueTwoFACodeAfterFailedCharge(r.Context(), userID)
|
||||
}
|
||||
http.Error(w, "Payment failed", chargeFailureStatus(err))
|
||||
return
|
||||
}
|
||||
|
||||
@@ -0,0 +1,152 @@
|
||||
//go:build test && dev
|
||||
|
||||
package payments
|
||||
|
||||
import (
|
||||
"context"
|
||||
"encoding/json"
|
||||
"net/http"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"crussell/clock"
|
||||
"crussell/db"
|
||||
"crussell/internal/square"
|
||||
"crussell/testutils"
|
||||
"crussell/testutils/fixtures"
|
||||
"crussell/testutils/jwt"
|
||||
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
)
|
||||
|
||||
// =============================================================================
|
||||
// Finding 2 — the A6 deposit clamp can produce chargeAmount=0 with no guard:
|
||||
// the handler then charged £0 at Square (invalid in prod, minted a completed
|
||||
// £0 deposit in the dev mock that consumed the discount). A deposit whose
|
||||
// eligible campaign credit covers the ENTIRE remaining obligation must skip the
|
||||
// Square call and report deposit_covered_by_discount.
|
||||
// =============================================================================
|
||||
|
||||
// seedActiveCampaign inserts an active time-based campaign with the given
|
||||
// discount percent and returns its id.
|
||||
func seedActiveCampaign(t *testing.T, ctx context.Context, q db.Querier, percent int) string {
|
||||
t.Helper()
|
||||
now := clock.Now()
|
||||
var id string
|
||||
err := q.QueryRow(ctx, `
|
||||
INSERT INTO discount_campaigns (name, campaign_type, discount_percent, status, start_date, end_date, times_redeemed)
|
||||
VALUES ($1, 'time_based', $2, 'active', $3, $4, 0)
|
||||
RETURNING id
|
||||
`, "Money-Fix Campaign", percent, now.Add(-24*time.Hour), now.Add(24*time.Hour)).Scan(&id)
|
||||
require.NoError(t, err)
|
||||
return id
|
||||
}
|
||||
|
||||
// failOnChargeClient fails the test if a Square charge is attempted. Proves a
|
||||
// discount-covered deposit skips the Square call entirely (finding 2).
|
||||
type failOnChargeClient struct {
|
||||
square.SquareClient
|
||||
t *testing.T
|
||||
}
|
||||
|
||||
func (c *failOnChargeClient) CreatePayment(ctx context.Context, req square.CreatePaymentReq) (*square.PaymentResult, error) {
|
||||
c.t.Fatalf("Square CreatePayment must NOT be called for a discount-covered deposit (amount=%d)", req.Amount)
|
||||
return nil, nil
|
||||
}
|
||||
|
||||
func TestBookingPayment_DepositFullyCoveredByDiscount_SkipsSquareCharge(t *testing.T) {
|
||||
ctx, tx := testutils.SetupTestTx(t)
|
||||
|
||||
userID, bookingID, _ := setupTestData(t, ctx, tx)
|
||||
userToken := jwt.GenerateUserToken(userID)
|
||||
// 100% time-based campaign = £50 discount on the £50 fixture booking, which
|
||||
// fully covers the £25 deposit request (chargeAmount clamps to £0).
|
||||
seedActiveCampaign(t, ctx, tx, 100)
|
||||
|
||||
origClient := SquareClient
|
||||
SquareClient = &failOnChargeClient{SquareClient: square.NewDevClient(), t: t}
|
||||
defer func() { SquareClient = origClient }()
|
||||
|
||||
cardToken := "cnon:deposit-covered"
|
||||
req := CreateBookingPaymentRequest{
|
||||
Amount: 2500, // £25 deposit
|
||||
PaymentType: "deposit",
|
||||
NewCardToken: &cardToken,
|
||||
IdempotencyKey: "deposit-covered-" + bookingID,
|
||||
}
|
||||
|
||||
handler := CreateBookingPayment
|
||||
w := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx)
|
||||
require.Equal(t, http.StatusOK, w.Code, "a discount-covered deposit must complete without a Square charge, body: %s", w.Body.String())
|
||||
|
||||
var body map[string]any
|
||||
require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body))
|
||||
assert.Equal(t, true, body["deposit_covered_by_discount"], "the response must signal the discount-covered deposit")
|
||||
|
||||
var payCount int
|
||||
require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM payments WHERE booking_id = $1`, bookingID).Scan(&payCount))
|
||||
assert.Zero(t, payCount, "no payment row may be recorded for a discount-covered deposit")
|
||||
|
||||
// The discount rows are applied by the next real charge / at completion,
|
||||
// never minted for a charge that did not happen.
|
||||
var discountCount int
|
||||
require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM booking_discounts WHERE booking_id = $1`, bookingID).Scan(&discountCount))
|
||||
assert.Zero(t, discountCount, "no discount may be applied for a charge that never happened")
|
||||
}
|
||||
|
||||
// =============================================================================
|
||||
// Finding 7 — remaining-balance capacity ignored PENDING refunds: service.go
|
||||
// counted only completed refunds in GetBookingRemainingBalancePence while
|
||||
// GetBookingPaymentInfo counts completed + pending. An in-flight refund
|
||||
// understated the remaining balance and blocked a legitimate retry.
|
||||
// =============================================================================
|
||||
|
||||
func TestGetBookingRemainingBalancePence_PendingRefundsReopenCapacity(t *testing.T) {
|
||||
t.Parallel()
|
||||
ctx, tx := testutils.SetupTestTx(t)
|
||||
|
||||
userID, err := fixtures.CreateTestUser(tx)
|
||||
require.NoError(t, err)
|
||||
|
||||
serviceID, err := fixtures.CreateTestService(tx)
|
||||
require.NoError(t, err)
|
||||
|
||||
bookingID, err := fixtures.CreateTestBooking(tx, userID, serviceID)
|
||||
require.NoError(t, err)
|
||||
|
||||
var bookingTotal int64
|
||||
require.NoError(t, tx.QueryRow(ctx, `SELECT ROUND(total_amount * 100)::bigint FROM bookings WHERE id = $1`, bookingID).Scan(&bookingTotal))
|
||||
|
||||
svc := NewPaymentService()
|
||||
|
||||
// Pay the full booking amount.
|
||||
_, err = svc.CreatePaymentRecord(ctx, PaymentRecord{
|
||||
BookingID: bookingID,
|
||||
PaymentType: "full",
|
||||
PaymentMethod: "online_square",
|
||||
Status: "completed",
|
||||
Amount: float64(bookingTotal) / 100.0,
|
||||
}, nil)
|
||||
require.NoError(t, err)
|
||||
|
||||
remaining, err := svc.GetBookingRemainingBalancePence(ctx, bookingID)
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, int64(0), remaining, "a fully-paid booking must have 0 remaining")
|
||||
|
||||
// A PENDING refund is money in flight that will come back — it must re-open
|
||||
// capacity by its amount exactly like a completed refund.
|
||||
var payRowID string
|
||||
require.NoError(t, tx.QueryRow(ctx, `SELECT id FROM payments WHERE booking_id = $1 AND payment_type = 'full' ORDER BY created_at DESC LIMIT 1`, bookingID).Scan(&payRowID))
|
||||
|
||||
refundAmount := bookingTotal / 2
|
||||
_, err = tx.Exec(ctx, `
|
||||
INSERT INTO refunds (payment_id, booking_id, amount, status, reason, origin)
|
||||
VALUES ($1, $2, $3, 'pending', 'in-flight test refund', 'manual')
|
||||
`, payRowID, bookingID, float64(refundAmount)/100.0)
|
||||
require.NoError(t, err)
|
||||
|
||||
remaining, err = svc.GetBookingRemainingBalancePence(ctx, bookingID)
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, refundAmount, remaining, "a pending refund must re-open the remaining balance by its amount")
|
||||
}
|
||||
@@ -524,10 +524,13 @@ func (s *PaymentService) GetBookingRemainingBalancePence(ctx context.Context, bo
|
||||
SELECT COALESCE(SUM(r.amount), 0) AS refunded_pounds
|
||||
FROM refunds r
|
||||
JOIN payments p ON r.payment_id = p.id
|
||||
WHERE p.booking_id = $1 AND r.status = 'completed'
|
||||
-- A tip refund returns gratuity, not booking money — it must not
|
||||
-- re-open booking charge capacity (mirror of the paid_total tip
|
||||
-- exclusion above).
|
||||
-- Pending refunds count too (matching GetBookingPaymentInfo): a
|
||||
-- refund in flight is money that will come back, so the remaining
|
||||
-- capacity must not be understated while it settles — understating
|
||||
-- it blocks a legitimate retry (finding 7). A tip refund returns
|
||||
-- gratuity, not booking money — it must not re-open booking charge
|
||||
-- capacity (mirror of the paid_total tip exclusion above).
|
||||
WHERE p.booking_id = $1 AND r.status IN ('completed', 'pending')
|
||||
AND p.payment_type <> 'tip'
|
||||
)
|
||||
-- Money-safety (M-cap): refunds return money, so they re-open booking
|
||||
|
||||
@@ -665,23 +665,22 @@ func clawbackTillSaleFunding(ctx context.Context, r staleRow) bool {
|
||||
// creation and a replayed payment's creation for the payment to be the REAL
|
||||
// charge under a legitimately replayed key. A same-key retry — the documented
|
||||
// retry path (handlers.go:1579-1591) — creates its charge somewhere between
|
||||
// the row's creation and the 24h retry-eligible window (the same-key retry
|
||||
// path stays valid until Square's ~24h idempotency-key retention expires), so
|
||||
// any COMPLETED payment created within [row.CreatedAt, row.CreatedAt +
|
||||
// replayLegitimateRetryWindow] can be that retry charge and must be rescued.
|
||||
// the row's creation and the retry-eligible window, so any COMPLETED payment
|
||||
// created within [row.CreatedAt, row.CreatedAt + replayLegitimateRetryWindow]
|
||||
// can be that retry charge and must be rescued.
|
||||
//
|
||||
// The window is 24h — the full Square idempotency-key retention window. The
|
||||
// sweep's own replay only runs once a row is at least 22h old
|
||||
// (stalePendingKeyedAge); a legitimate same-key retry can still land up to 24h
|
||||
// after the row, and a shorter window would misclassify a retry landing in the
|
||||
// 22-24h zone as a NEW expired-key replay and auto-refund the customer's
|
||||
// legitimate charge (B1). A payment created LATER than 24h after the row is the
|
||||
// classic expired-key replay-induced charge — the sweep just created it by
|
||||
// replaying the still valid saved-card source under a key Square no longer
|
||||
// retains — and rescuing it would hide the duplicate charge behind the
|
||||
// original row (finding A1). The boundary is inclusive: a payment created
|
||||
// EXACTLY 24h after the row is still within the legitimate window.
|
||||
const replayLegitimateRetryWindow = 24 * time.Hour
|
||||
// The window is 22h, matching stalePendingKeyedAge: the sweep first replays a
|
||||
// keyed row once it is 22h old, so a LEGITIMATE same-key retry charge can only
|
||||
// have landed before that first sweep replay (a retry made after the replay is
|
||||
// the sweep's OWN expired-key replay, which must NOT be rescued). A window
|
||||
// larger than the sweep cutoff would hide that second charge behind the
|
||||
// original row: the sweep replays the row at 22h, Square's ~24h key retention
|
||||
// may have lapsed, the replay creates a NEW charge at row+22h..24h that lands
|
||||
// INSIDE the window, and the sweep rescues it as the "legitimate retry" — a
|
||||
// real double-charge silently hidden (finding 3). The boundary is inclusive: a
|
||||
// payment created EXACTLY 22h after the row is still within the legitimate
|
||||
// window.
|
||||
const replayLegitimateRetryWindow = 22 * time.Hour
|
||||
|
||||
// replayRescueLowerBoundSkew is the lower-bound tolerance for a replayed
|
||||
// COMPLETED payment to still be treated as the ORIGINAL charge under a retained
|
||||
@@ -712,7 +711,7 @@ func replayMatchesRowAmount(r staleRow, pr *square.PaymentResult) bool {
|
||||
// replayWithinLegitimateWindow reports whether a replayed COMPLETED payment is
|
||||
// the REAL charge this pending row is waiting on — the ORIGINAL charge under a
|
||||
// retained key (created ~at row creation) or a later SAME-KEY RETRY charge
|
||||
// (created between the row's creation and the 24h retry-eligible window, F2).
|
||||
// (created between the row's creation and the 22h retry-eligible window, F2).
|
||||
// The amount must match the row (a retry can never change it) and the payment
|
||||
// must have been created within replayLegitimateRetryWindow of the row. The
|
||||
// source is matched by construction: the replay body is rebuilt from the row's
|
||||
|
||||
@@ -756,8 +756,9 @@ func TestSweepStalePendingPayments_KeyedReplayNewCharge_AutoRefunded(t *testing.
|
||||
}
|
||||
|
||||
// The replayed COMPLETED payment's created_at is 25h AFTER the pending row
|
||||
// — beyond the 24h legitimate-retry window (replayLegitimateRetryWindow),
|
||||
// so it is provably a NEW expired-key replay charge, not a same-key retry.
|
||||
// — beyond the 22h legitimate-retry window (replayLegitimateRetryWindow,
|
||||
// matching the sweep's 22h keyed cutoff), so it is provably a NEW expired-key
|
||||
// replay charge, not a same-key retry.
|
||||
// The cross-check runs only in a non-dev/mock env, so the env is flipped to
|
||||
// production for the sweep (sequential, like the 2FA tests). The dev mock
|
||||
// is constructed BEFORE the flip (NewDevClient refuses production without
|
||||
@@ -1493,8 +1494,10 @@ func TestSweepStalePendingPayments_KeyedReplaySlightlyBeforeRow_Rescues(t *testi
|
||||
// row (between the old 21h window and the 22h sweep cutoff) is the REAL charge
|
||||
// under a legitimately replayed key and MUST be rescued. The pre-B2 21h window
|
||||
// refused it and stranded the row pending. The legitimate-retry boundary is now
|
||||
// replayLegitimateRetryWindow (24h): only a payment created AFTER
|
||||
// row.CreatedAt+24h can be the sweep's own expired-key replay.
|
||||
// replayLegitimateRetryWindow (22h, matching stalePendingKeyedAge): only a
|
||||
// payment created AFTER row.CreatedAt+22h can be the sweep's own expired-key
|
||||
// replay — a 24h window would rescue a charge the sweep itself minted between
|
||||
// 22h and 24h and hide a real double-charge (finding 3).
|
||||
func TestSweepStalePendingPayments_KeyedReplayRetryAt215h_Rescues(t *testing.T) {
|
||||
ctx, tx := testutils.SetupTestTx(t)
|
||||
|
||||
@@ -1522,7 +1525,7 @@ func TestSweepStalePendingPayments_KeyedReplayRetryAt215h_Rescues(t *testing.T)
|
||||
}
|
||||
|
||||
// The replayed COMPLETED payment is a legitimate same-key retry created
|
||||
// 21.5h after the row — inside the 24h legitimate window, so it is the real
|
||||
// 21.5h after the row — inside the 22h legitimate window, so it is the real
|
||||
// charge and must be rescued, not refused as an expired-key duplicate.
|
||||
var rowCreatedAt time.Time
|
||||
if err := tx.QueryRow(ctx, "SELECT created_at FROM payments WHERE id = $1", staleID).Scan(&rowCreatedAt); err != nil {
|
||||
|
||||
@@ -954,12 +954,17 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) {
|
||||
}
|
||||
|
||||
// 2FA gating (C5): charging a customer's saved card requires 2FA when
|
||||
// the feature is enforced. consume=false (MEDIUM-2): the code is
|
||||
// verified here but only NULLed once the charge reaches its terminal
|
||||
// success state below (ConsumePendingCode), so a failed/ambiguous
|
||||
// Square charge does NOT burn the operator-relayed code and a same-key
|
||||
// retry can re-verify the SAME code.
|
||||
if cardUserID.Valid && !requireTwoFactorForCardAccess(w, r, service, cardUserID.String, req.VerificationCode, false) {
|
||||
// the feature is enforced. consume=!reuse (LOW 6a): a FRESH charge
|
||||
// verifies WITH consumption — the code is single-use at the gate,
|
||||
// closing the TOCTOU where a verified-but-unconsumed code could
|
||||
// authorize a second charge within its lifetime — and a failed Square
|
||||
// charge re-issues a fresh code (reissueTwoFACodeAfterFailedCharge
|
||||
// below). A pending-reuse retry (existingPendingID != "") verifies
|
||||
// WITHOUT consuming: the code was re-issued for exactly this retry and
|
||||
// the post-charge success path below (ConsumePendingCode) burns it on
|
||||
// terminal success, so a retry that fails again keeps its code for one
|
||||
// more attempt.
|
||||
if cardUserID.Valid && !requireTwoFactorForCardAccess(w, r, service, cardUserID.String, req.VerificationCode, existingPendingID == "") {
|
||||
return
|
||||
}
|
||||
|
||||
@@ -1252,6 +1257,13 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) {
|
||||
log.Printf("CRITICAL: till sale %s charge definitively failed (%v) but gift-card clawback also failed: %v — MANUAL RECONCILIATION REQUIRED: gift card %s may still be funded", tillSaleID, squareErr, revErr, giftCardID)
|
||||
}
|
||||
}
|
||||
// The gate consumed the 2FA code for a FRESH saved-card charge —
|
||||
// re-issue so the same-key retry has a live code to verify. A
|
||||
// pending-reuse retry verified without consuming at the gate, so
|
||||
// its code survives for one more attempt.
|
||||
if req.PaymentMethod == "saved_card" && cardUserID.Valid && existingPendingID == "" {
|
||||
reissueTwoFACodeAfterFailedCharge(ctx, cardUserID.String)
|
||||
}
|
||||
// 402 only for definitive declines; ambiguous transport/5xx must be
|
||||
// 503 so the pending sale stays resumable on a same-key retry
|
||||
// (M3). The clawback decision above stays keyed on
|
||||
|
||||
@@ -0,0 +1,232 @@
|
||||
//go:build test && dev
|
||||
|
||||
package payments
|
||||
|
||||
// Tests pinning the Loop B 2FA single-use consume-at-gate fix on the two
|
||||
// saved-card charge gates that were still consuming AFTER the charge landed:
|
||||
// the till saved-card charge (till.go) and the gift-card purchase saved-card
|
||||
// gate (giftcards.go).
|
||||
//
|
||||
// Semantics (mirroring CreateBookingPayment, handlers.go):
|
||||
// - A FRESH charge verifies the 2FA code WITH consumption at the gate
|
||||
// (consume = !reusePendingRecord). The code is single-use, closing the
|
||||
// TOCTOU where a verified-but-unconsumed code could authorize a second
|
||||
// concurrent charge. If Square then fails, a fresh code is re-issued
|
||||
// (reissueTwoFACodeAfterFailedCharge) so the same-key retry has a live
|
||||
// code to verify.
|
||||
// - A PENDING-REUSE retry verifies WITHOUT consuming: the code was re-issued
|
||||
// for exactly this retry, and the post-charge success path consumes it on
|
||||
// terminal success, so a retry that fails again keeps its code for one
|
||||
// more attempt.
|
||||
//
|
||||
// These tests flip REQUIRE_2FA/SQUARE_ENVIRONMENT via t.Setenv and therefore
|
||||
// must stay sequential (no t.Parallel) — see the note at the top of
|
||||
// twofa_test.go. They use SQUARE_ENVIRONMENT=staging (NOT production) for
|
||||
// enforcement: twoFactorEnforced() is fail-closed, so any non-mock/dev value
|
||||
// enforces the gate, while square.NewDevClient() — which the injected fault
|
||||
// client and structuredSquareErrorWithCode construct at call time — returns
|
||||
// the in-memory mock for every env except production/sandbox. The shared
|
||||
// helperEnvEnforce2FA sets production, which would panic NewDevClient in a
|
||||
// dev build.
|
||||
|
||||
import (
|
||||
"context"
|
||||
"database/sql"
|
||||
"net/http"
|
||||
"testing"
|
||||
|
||||
"crussell/internal/square"
|
||||
"crussell/internal/twofa"
|
||||
"crussell/testutils"
|
||||
"crussell/testutils/fixtures"
|
||||
"crussell/testutils/jwt"
|
||||
|
||||
"github.com/stretchr/testify/require"
|
||||
)
|
||||
|
||||
// TestTwoFactorEnforced_CreateTillSale_SavedCard_FreshCharge_Failure pins the
|
||||
// till saved-card gate on a FRESH charge: the 2FA code is consumed AT THE GATE
|
||||
// (single-use), and a failed Square charge re-issues a fresh code so the
|
||||
// same-key retry can verify again. The stored hash must differ from the seeded
|
||||
// one — if the gate still used consume=false the seeded hash would survive
|
||||
// unchanged and there would be no re-issue.
|
||||
func TestTwoFactorEnforced_CreateTillSale_SavedCard_FreshCharge_Failure_ConsumesAtGate_Reissues(t *testing.T) {
|
||||
t.Setenv("REQUIRE_2FA", "true")
|
||||
t.Setenv("SQUARE_ENVIRONMENT", "staging")
|
||||
ctx, tx := testutils.SetupTestTx(t)
|
||||
|
||||
adminID, err := fixtures.CreateTestAdminUser(tx)
|
||||
require.NoError(t, err)
|
||||
userID, err := fixtures.CreateTestUser(tx)
|
||||
require.NoError(t, err)
|
||||
adminToken := jwt.GenerateTestToken(adminID, "admin")
|
||||
seedTwoFAPendingCode(t, tx, userID, "556677")
|
||||
|
||||
cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "ccof:sq_test_card_id", "VISA", "1234")
|
||||
require.NoError(t, err)
|
||||
|
||||
origClient := SquareClient
|
||||
SquareClient = &definitiveChargeClient{SquareClient: square.NewDevClient(), createErr: structuredSquareErrorWithCode(t, http.StatusPaymentRequired, "CARD_DECLINED")}
|
||||
defer func() { SquareClient = origClient }()
|
||||
|
||||
req := TillSaleRequest{
|
||||
ItemType: "gift_card",
|
||||
Action: "create",
|
||||
Amount: 50.00,
|
||||
PaymentMethod: "saved_card",
|
||||
UserSavedCardID: &cardID,
|
||||
UserID: &userID,
|
||||
IdempotencyKey: "2fa-till-fresh-decline",
|
||||
VerificationCode: "556677",
|
||||
}
|
||||
|
||||
w := makePaymentRequest(CreateTillSale, "POST", "/api/admin/till/sale", req, adminToken, ctx)
|
||||
require.Equal(t, http.StatusPaymentRequired, w.Code, w.Body.String())
|
||||
|
||||
var hash sql.NullString
|
||||
require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_pending_code_hash FROM users WHERE id = $1", userID).Scan(&hash))
|
||||
require.True(t, hash.Valid, "a failed fresh saved-card charge must re-issue a live 2FA code for the same-key retry")
|
||||
require.NotEqual(t, twofa.Hash("556677"), hash.String, "the gate must have consumed the seeded code at verification time (single-use)")
|
||||
}
|
||||
|
||||
// TestTwoFactorEnforced_CreateTillSale_SavedCard_PendingReuse_Failure pins the
|
||||
// till saved-card gate on a PENDING-REUSE retry: the code is verified WITHOUT
|
||||
// consumption, so a retry that fails again keeps its seeded code unchanged and
|
||||
// no re-issue runs (the fresh-charge-only guard must not fire).
|
||||
func TestTwoFactorEnforced_CreateTillSale_SavedCard_PendingReuse_Failure_KeepsCode(t *testing.T) {
|
||||
t.Setenv("REQUIRE_2FA", "true")
|
||||
t.Setenv("SQUARE_ENVIRONMENT", "staging")
|
||||
ctx, tx := testutils.SetupTestTx(t)
|
||||
|
||||
adminID, err := fixtures.CreateTestAdminUser(tx)
|
||||
require.NoError(t, err)
|
||||
userID, err := fixtures.CreateTestUser(tx)
|
||||
require.NoError(t, err)
|
||||
adminToken := jwt.GenerateTestToken(adminID, "admin")
|
||||
seedTwoFAPendingCode(t, tx, userID, "667788")
|
||||
|
||||
cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "ccof:sq_test_card_id", "VISA", "1234")
|
||||
require.NoError(t, err)
|
||||
|
||||
// Seed a PENDING till_sale with the same key — a prior attempt whose
|
||||
// Square charge failed after the DB transaction committed (card funded).
|
||||
key := "2fa-till-pending-reuse"
|
||||
var giftCardID string
|
||||
require.NoError(t, tx.QueryRow(ctx, `
|
||||
INSERT INTO gift_cards (total_funds_added, amount_remaining, created_by, is_inventory, voucher_type_at_purchase)
|
||||
VALUES (50.00, 50.00, $1, FALSE, 'SPV') RETURNING id
|
||||
`, adminID).Scan(&giftCardID))
|
||||
_, err = tx.Exec(ctx, `
|
||||
INSERT INTO till_sales (item_type, item_id, description, quantity, unit_price, total_amount,
|
||||
payment_method, status, user_id, user_saved_card_id, idempotency_key, created_by, created_at, updated_at)
|
||||
VALUES ('gift_card', $1, 'Gift Card create', 1, 50.00, 50.00, 'online_square', 'pending',
|
||||
$2, $3, $4, $5, NOW(), NOW())
|
||||
`, giftCardID, userID, cardID, key, adminID)
|
||||
require.NoError(t, err)
|
||||
|
||||
origClient := SquareClient
|
||||
SquareClient = &definitiveChargeClient{SquareClient: square.NewDevClient(), createErr: structuredSquareErrorWithCode(t, http.StatusPaymentRequired, "CARD_DECLINED")}
|
||||
defer func() { SquareClient = origClient }()
|
||||
|
||||
req := TillSaleRequest{
|
||||
ItemType: "gift_card",
|
||||
Action: "create",
|
||||
Amount: 50.00,
|
||||
PaymentMethod: "saved_card",
|
||||
UserSavedCardID: &cardID,
|
||||
UserID: &userID,
|
||||
IdempotencyKey: key,
|
||||
VerificationCode: "667788",
|
||||
}
|
||||
|
||||
w := makePaymentRequest(CreateTillSale, "POST", "/api/admin/till/sale", req, adminToken, ctx)
|
||||
require.Equal(t, http.StatusPaymentRequired, w.Code, w.Body.String())
|
||||
|
||||
var hash sql.NullString
|
||||
require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_pending_code_hash FROM users WHERE id = $1", userID).Scan(&hash))
|
||||
require.True(t, hash.Valid, "a pending-reuse retry must not consume the code at the gate")
|
||||
require.Equal(t, twofa.Hash("667788"), hash.String, "a failed pending-reuse retry must keep its seeded code unchanged (no re-issue)")
|
||||
}
|
||||
|
||||
// TestTwoFactorEnforced_BuyGiftCard_SavedCard_Fresh_Failure pins the gift-card
|
||||
// purchase saved-card gate on a FRESH charge: consume-at-gate + re-issue on
|
||||
// failure, exactly as the till gate above.
|
||||
func TestTwoFactorEnforced_BuyGiftCard_SavedCard_Fresh_Failure_ConsumesAtGate_Reissues(t *testing.T) {
|
||||
t.Setenv("REQUIRE_2FA", "true")
|
||||
t.Setenv("SQUARE_ENVIRONMENT", "staging")
|
||||
ctx, tx := testutils.SetupTestTx(t)
|
||||
|
||||
userID, err := fixtures.CreateTestUser(tx)
|
||||
require.NoError(t, err)
|
||||
token := jwt.GenerateTestToken(userID, "verified_email")
|
||||
seedTwoFAPendingCode(t, tx, userID, "112233")
|
||||
|
||||
cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "ccof:sq_test_card_id", "VISA", "1234")
|
||||
require.NoError(t, err)
|
||||
|
||||
origClient := SquareClient
|
||||
SquareClient = &definitiveChargeClient{SquareClient: square.NewDevClient(), createErr: structuredSquareErrorWithCode(t, http.StatusPaymentRequired, "CARD_DECLINED")}
|
||||
defer func() { SquareClient = origClient }()
|
||||
|
||||
req := BuyGiftCardRequest{
|
||||
Amount: 2000,
|
||||
RecipientType: "self",
|
||||
CardID: &cardID,
|
||||
IdempotencyKey: "2fa-buy-gc-fresh-decline",
|
||||
VerificationCode: "112233",
|
||||
}
|
||||
|
||||
w := makePaymentRequest(BuyGiftCard, "POST", "/api/user/giftcards/buy", req, token, ctx)
|
||||
require.Equal(t, http.StatusPaymentRequired, w.Code, w.Body.String())
|
||||
|
||||
var hash sql.NullString
|
||||
require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_pending_code_hash FROM users WHERE id = $1", userID).Scan(&hash))
|
||||
require.True(t, hash.Valid, "a failed fresh saved-card purchase must re-issue a live 2FA code for the same-key retry")
|
||||
require.NotEqual(t, twofa.Hash("112233"), hash.String, "the gate must have consumed the seeded code at verification time (single-use)")
|
||||
}
|
||||
|
||||
// TestTwoFactorEnforced_BuyGiftCard_SavedCard_PendingReuse_Failure pins the
|
||||
// gift-card purchase gate on a PENDING-REUSE retry: verify-without-consume and
|
||||
// no re-issue, so the seeded code survives a second failed retry unchanged.
|
||||
func TestTwoFactorEnforced_BuyGiftCard_SavedCard_PendingReuse_Failure_KeepsCode(t *testing.T) {
|
||||
t.Setenv("REQUIRE_2FA", "true")
|
||||
t.Setenv("SQUARE_ENVIRONMENT", "staging")
|
||||
ctx, tx := testutils.SetupTestTx(t)
|
||||
|
||||
userID, err := fixtures.CreateTestUser(tx)
|
||||
require.NoError(t, err)
|
||||
token := jwt.GenerateTestToken(userID, "verified_email")
|
||||
seedTwoFAPendingCode(t, tx, userID, "334455")
|
||||
|
||||
cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "ccof:sq_test_card_id", "VISA", "1234")
|
||||
require.NoError(t, err)
|
||||
|
||||
// Seed a PENDING payment record with the same key — a prior attempt whose
|
||||
// Square charge failed (mirrors TestBuyGiftCard_RetryPending_ReattemptsCharge).
|
||||
key := "2fa-buy-gc-pending-reuse"
|
||||
_, err = tx.Exec(context.Background(), `
|
||||
INSERT INTO payments (payment_type, payment_method, status, amount, idempotency_key, created_at, updated_at, created_by)
|
||||
VALUES ('full', 'online_square', 'pending', 20.00, $1, NOW(), NOW(), $2)
|
||||
`, key, userID)
|
||||
require.NoError(t, err)
|
||||
|
||||
origClient := SquareClient
|
||||
SquareClient = &definitiveChargeClient{SquareClient: square.NewDevClient(), createErr: structuredSquareErrorWithCode(t, http.StatusPaymentRequired, "CARD_DECLINED")}
|
||||
defer func() { SquareClient = origClient }()
|
||||
|
||||
req := BuyGiftCardRequest{
|
||||
Amount: 2000,
|
||||
RecipientType: "self",
|
||||
CardID: &cardID,
|
||||
IdempotencyKey: key,
|
||||
VerificationCode: "334455",
|
||||
}
|
||||
|
||||
w := makePaymentRequest(BuyGiftCard, "POST", "/api/user/giftcards/buy", req, token, ctx)
|
||||
require.Equal(t, http.StatusPaymentRequired, w.Code, w.Body.String())
|
||||
|
||||
var hash sql.NullString
|
||||
require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_pending_code_hash FROM users WHERE id = $1", userID).Scan(&hash))
|
||||
require.True(t, hash.Valid, "a pending-reuse retry must not consume the code at the gate")
|
||||
require.Equal(t, twofa.Hash("334455"), hash.String, "a failed pending-reuse retry must keep its seeded code unchanged (no re-issue)")
|
||||
}
|
||||
Reference in New Issue
Block a user