diff --git a/backend/auth/jwt.go b/backend/auth/jwt.go index 0d6d481..458eb76 100644 --- a/backend/auth/jwt.go +++ b/backend/auth/jwt.go @@ -231,19 +231,26 @@ func GenerateRefreshToken(ctx context.Context, userID string, role string) (stri return token, nil } -// VerifyRefreshToken checks a refresh token and returns user details if valid -// The token is consumed (deleted) upon successful verification, implementing rotation. -func VerifyRefreshToken(ctx context.Context, tokenString string) (userID string, role string, err error) { +// GenerateRefreshTokenInFamily creates a refresh token in the SAME rotation +// family as its parent (the family_id returned by VerifyRefreshToken). Rotation +// must mint the descendant in the parent's family so a replayed (already-used) +// ancestor can revoke the ENTIRE lineage — the descendant included — instead of +// leaving a fresh 90-day token alive after theft is detected. +func GenerateRefreshTokenInFamily(ctx context.Context, userID string, role string, familyID string) (string, error) { + token, err := generateRefreshTokenString() + if err != nil { + return "", err + } + + // Store hashed version in DB with 90-day expiry, in the given family query := ` - DELETE FROM refresh_tokens - WHERE token_hash = encode(sha256($1::bytea), 'hex') - AND expires_at > NOW() - AND NOT revoked - RETURNING user_id, role` + INSERT INTO refresh_tokens (user_id, token_hash, role, family_id, expires_at) + VALUES ($1, encode(sha256($2::bytea), 'hex'), $3, $4, NOW() + INTERVAL '90 days') + RETURNING id` tx, err := db.Conn.Begin(ctx) if err != nil { - return "", "", fmt.Errorf("failed to begin transaction: %w", err) + return "", fmt.Errorf("failed to begin transaction: %w", err) } defer func() { if err := tx.Rollback(ctx); err != nil && !errors.Is(err, pgx.ErrTxClosed) { @@ -251,19 +258,102 @@ func VerifyRefreshToken(ctx context.Context, tokenString string) (userID string, } }() - err = tx.QueryRow(ctx, query, tokenString).Scan(&userID, &role) + var tokenID int64 + err = tx.QueryRow(ctx, query, userID, token, role, familyID).Scan(&tokenID) if err != nil { - if errors.Is(err, pgx.ErrNoRows) { - return "", "", fmt.Errorf("invalid or expired refresh token") - } - return "", "", fmt.Errorf("failed to verify refresh token: %w", err) + return "", fmt.Errorf("failed to store refresh token: %w", err) } if err := tx.Commit(ctx); err != nil { - return "", "", fmt.Errorf("failed to commit transaction: %w", err) + return "", fmt.Errorf("failed to commit transaction: %w", err) } - // Token was consumed (DELETE returned it) — this is rotation - // If a token is used twice, the second DELETE returns no rows = invalid - return userID, role, nil + return token, nil +} + +// VerifyRefreshToken checks a refresh token and returns user details if valid. +// The token is consumed (marked used) upon successful verification — rotation — +// and its family_id is returned so the caller can mint the descendant in the +// SAME family. If an ALREADY-ROTATED token is presented again (a replay: the +// attacker rotated it, then the victim replayed it), the entire rotation family +// is revoked (the descendant minted at rotation dies too) and a critical admin +// notification (reason 'refresh_token_reuse') is raised. The caller always gets +// the generic "invalid or expired refresh token" error so reuse is never leaked. +func VerifyRefreshToken(ctx context.Context, tokenString string) (userID string, role string, familyID string, err error) { + query := ` + UPDATE refresh_tokens SET used_at = NOW() + WHERE token_hash = encode(sha256($1::bytea), 'hex') + AND expires_at > NOW() + AND NOT revoked + AND used_at IS NULL + RETURNING user_id, role, family_id` + + tx, err := db.Conn.Begin(ctx) + if err != nil { + return "", "", "", fmt.Errorf("failed to begin transaction: %w", err) + } + defer func() { + if err := tx.Rollback(ctx); err != nil && !errors.Is(err, pgx.ErrTxClosed) { + slog.Error("failed to rollback transaction", "err", err) + } + }() + + err = tx.QueryRow(ctx, query, tokenString).Scan(&userID, &role, &familyID) + if err == nil { + // Rotation: the token is marked used (kept in the row) so a later + // replay can be detected, and its family_id is returned. + if err := tx.Commit(ctx); err != nil { + return "", "", "", fmt.Errorf("failed to commit transaction: %w", err) + } + return userID, role, familyID, nil + } + if !errors.Is(err, pgx.ErrNoRows) { + return "", "", "", fmt.Errorf("failed to verify refresh token: %w", err) + } + + // The rotation UPDATE matched nothing: the token is expired, revoked, or + // never issued — OR it was already used (replayed). A used token is theft: + // the descendant minted at rotation would otherwise stay valid for 90 days. + var reusedUserID, reusedFamilyID string + reuseErr := tx.QueryRow(ctx, ` + SELECT user_id, family_id FROM refresh_tokens + WHERE token_hash = encode(sha256($1::bytea), 'hex') + AND used_at IS NOT NULL + `, tokenString).Scan(&reusedUserID, &reusedFamilyID) + + if reuseErr == nil { + // (i) Revoke the ENTIRE family — the reused token and every descendant. + if _, err := tx.Exec(ctx, `DELETE FROM refresh_tokens WHERE family_id = $1`, reusedFamilyID); err != nil { + slog.Error("CRITICAL: refresh token reuse detected but family revocation failed", "userID", reusedUserID, "familyID", reusedFamilyID, "err", err) + } + // (ii) Surface the theft in the admin notification centre. The NOT + // EXISTS guard keeps ONE alert per reused family until an admin + // acknowledges it — mirroring insertCriticalPaymentNotification. + if _, err := tx.Exec(ctx, ` + INSERT INTO admin_notifications (reason, user_id, created_at) + SELECT 'refresh_token_reuse', $1, NOW() + WHERE NOT EXISTS ( + SELECT 1 FROM admin_notifications an + WHERE an.reason = 'refresh_token_reuse' + AND an.user_id = $1 + AND an.acknowledged_at IS NULL + ) + `, reusedUserID); err != nil { + slog.Error("CRITICAL: refresh token reuse detected but admin alert insert failed", "userID", reusedUserID, "err", err) + } + // Commit the family revocation + alert — NOT the deferred rollback. + if err := tx.Commit(ctx); err != nil { + return "", "", "", fmt.Errorf("failed to commit transaction: %w", err) + } + // (iii) CRITICAL log; (iv) generic error — never leak that reuse was seen. + slog.Error("CRITICAL: refresh token reuse detected — rotation family revoked", "userID", reusedUserID, "familyID", reusedFamilyID) + return "", "", "", fmt.Errorf("invalid or expired refresh token") + } + if !errors.Is(reuseErr, pgx.ErrNoRows) { + return "", "", "", fmt.Errorf("failed to verify refresh token: %w", reuseErr) + } + + // Never-issued / expired / revoked token — indistinguishable from a replay + // to the client, as before. + return "", "", "", fmt.Errorf("invalid or expired refresh token") } diff --git a/backend/auth/jwt_test.go b/backend/auth/jwt_test.go index 89c62b4..acdb824 100644 --- a/backend/auth/jwt_test.go +++ b/backend/auth/jwt_test.go @@ -424,7 +424,7 @@ func TestVerifyRefreshToken_Success(t *testing.T) { } // First verify should succeed - retUserID, retRole, err := VerifyRefreshToken(ctx, token) + retUserID, retRole, _, err := VerifyRefreshToken(ctx, token) if err != nil { t.Fatalf("VerifyRefreshToken() failed: %v", err) } @@ -436,7 +436,7 @@ func TestVerifyRefreshToken_Success(t *testing.T) { } // Second verify with same token must fail (rotation — token consumed) - _, _, err = VerifyRefreshToken(ctx, token) + _, _, _, err = VerifyRefreshToken(ctx, token) if err == nil { t.Fatal("expected error for consumed token, got nil") } @@ -461,13 +461,13 @@ func TestVerifyRefreshToken_Rotation(t *testing.T) { } // First call should succeed - _, _, err = VerifyRefreshToken(ctx, token) + _, _, _, err = VerifyRefreshToken(ctx, token) if err != nil { t.Fatalf("first verification should succeed, got: %v", err) } // Second call with the same token must fail - _, _, err = VerifyRefreshToken(ctx, token) + _, _, _, err = VerifyRefreshToken(ctx, token) if err == nil { t.Fatal("expected error for rotated token, got nil") } @@ -476,12 +476,96 @@ func TestVerifyRefreshToken_Rotation(t *testing.T) { } } +// TestVerifyRefreshToken_ReuseRevokesFamilyAndAlerts verifies the reuse +// detection: generate a token → rotate it once (minting a descendant in the +// SAME family via GenerateRefreshTokenInFamily) → present the ORIGINAL token +// again. The replay must (i) delete the ENTIRE rotation family (the descendant +// included) from refresh_tokens and (ii) insert an admin_notifications row with +// reason 'refresh_token_reuse' for the user. +func TestVerifyRefreshToken_ReuseRevokesFamilyAndAlerts(t *testing.T) { + ctx, tx := testtx.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + if err != nil { + t.Fatalf("failed to create test user: %v", err) + } + + // 1. Generate a refresh token (new family) and rotate it once. + original, err := GenerateRefreshToken(ctx, userID, "verified_email") + if err != nil { + t.Fatalf("GenerateRefreshToken() failed: %v", err) + } + + _, _, familyID, err := VerifyRefreshToken(ctx, original) + if err != nil { + t.Fatalf("first verification should succeed, got: %v", err) + } + if familyID == "" { + t.Fatal("expected non-empty family_id from rotation") + } + + // 2. Mint the descendant in the SAME family (as RefreshTokenHandler does). + descendant, err := GenerateRefreshTokenInFamily(ctx, userID, "verified_email", familyID) + if err != nil { + t.Fatalf("GenerateRefreshTokenInFamily() failed: %v", err) + } + + var famCount int + if err := tx.QueryRow(ctx, + `SELECT COUNT(*) FROM refresh_tokens WHERE family_id = $1`, familyID).Scan(&famCount); err != nil { + t.Fatalf("failed to count family rows: %v", err) + } + if famCount != 2 { + t.Fatalf("expected 2 refresh tokens in family, got %d", famCount) + } + + // 3. Replay the ORIGINAL token — theft. + _, _, _, err = VerifyRefreshToken(ctx, original) + if err == nil { + t.Fatal("expected error for replayed token, got nil") + } + if !strings.Contains(err.Error(), "invalid or expired") { + t.Errorf("expected 'invalid or expired' error, got: %v", err) + } + + // (i) The entire family is revoked: the used original AND the descendant. + var famAfter int + if err := tx.QueryRow(ctx, + `SELECT COUNT(*) FROM refresh_tokens WHERE family_id = $1`, familyID).Scan(&famAfter); err != nil { + t.Fatalf("failed to count family rows after replay: %v", err) + } + if famAfter != 0 { + t.Errorf("expected 0 refresh tokens in family after reuse (descendant killed), got %d", famAfter) + } + + var descHashCount int + if err := tx.QueryRow(ctx, + `SELECT COUNT(*) FROM refresh_tokens WHERE token_hash = encode(sha256($1::bytea), 'hex')`, + descendant).Scan(&descHashCount); err != nil { + t.Fatalf("failed to check descendant: %v", err) + } + if descHashCount != 0 { + t.Errorf("expected descendant to be deleted, got %d rows", descHashCount) + } + + // (ii) An admin alert with reason 'refresh_token_reuse' exists for the user. + var alertCount int + if err := tx.QueryRow(ctx, + `SELECT COUNT(*) FROM admin_notifications WHERE reason = 'refresh_token_reuse' AND user_id = $1`, + userID).Scan(&alertCount); err != nil { + t.Fatalf("failed to query admin_notifications: %v", err) + } + if alertCount != 1 { + t.Errorf("expected 1 'refresh_token_reuse' alert, got %d", alertCount) + } +} + // TestVerifyRefreshToken_InvalidToken calls VerifyRefreshToken with a fake // token string and expects it to fail with "invalid or expired". func TestVerifyRefreshToken_InvalidToken(t *testing.T) { ctx, _ := testtx.SetupTestTx(t) - _, _, err := VerifyRefreshToken(ctx, "this-is-a-completely-fake-token-string") + _, _, _, err := VerifyRefreshToken(ctx, "this-is-a-completely-fake-token-string") if err == nil { t.Fatal("expected error for invalid token, got nil") } diff --git a/backend/handlers/auth/auth_test.go b/backend/handlers/auth/auth_test.go index a4cfb3d..bc1d02b 100644 --- a/backend/handlers/auth/auth_test.go +++ b/backend/handlers/auth/auth_test.go @@ -1781,7 +1781,7 @@ func TestRefreshToken_Generation(t *testing.T) { t.Errorf("expected 1 refresh_token, got %d", count) } - retrievedUserID, retrievedRole, err := auth.VerifyRefreshToken(ctx, refreshToken) + retrievedUserID, retrievedRole, _, err := auth.VerifyRefreshToken(ctx, refreshToken) if err != nil { t.Fatalf("failed to verify refresh token: %v", err) } @@ -1792,7 +1792,7 @@ func TestRefreshToken_Generation(t *testing.T) { t.Errorf("expected role 'verified_email', got %q", retrievedRole) } - _, _, err = auth.VerifyRefreshToken(ctx, refreshToken) + _, _, _, err = auth.VerifyRefreshToken(ctx, refreshToken) if err == nil { t.Error("expected error on second refresh token verification (rotated)") } @@ -1942,17 +1942,20 @@ func TestRefreshToken_RotatesRefreshToken_DBBacked(t *testing.T) { t.Fatalf("refresh failed: %d. body: %s", w.Code, w.Body.String()) } - // The used token was deleted (rotated) and a fresh one inserted — the row - // count is unchanged at 1, but the consumed token no longer verifies. + // Rotation is DB-backed: the presented token was consumed (marked used, so a + // replay can be detected and the whole family revoked) and a fresh descendant + // minted in the same family. The used row is RETAINED for reuse detection, so + // 2 rows now exist for the user (used original + new descendant), and the + // consumed token no longer verifies. var after int if err := tx.QueryRow(ctx, "SELECT COUNT(*) FROM refresh_tokens WHERE user_id = $1", userID).Scan(&after); err != nil { t.Fatalf("failed to count refresh tokens after refresh: %v", err) } - if after != 1 { - t.Errorf("expected exactly 1 refresh token after rotation, got %d", after) + if after != 2 { + t.Errorf("expected 2 refresh tokens after rotation (used original retained + descendant), got %d", after) } - if _, _, err := auth.VerifyRefreshToken(ctx, refreshToken); err == nil { + if _, _, _, err := auth.VerifyRefreshToken(ctx, refreshToken); err == nil { t.Error("the consumed refresh token must no longer verify (rotated)") } } diff --git a/backend/handlers/auth/local.go b/backend/handlers/auth/local.go index d3e6b17..a290733 100644 --- a/backend/handlers/auth/local.go +++ b/backend/handlers/auth/local.go @@ -485,10 +485,11 @@ func LoginHandler(w http.ResponseWriter, r *http.Request) { // (rotated), and exchanged for a fresh access token + a NEW refresh token. // // B5 (security): the handler deliberately does NOT accept the access token. -// VerifyRefreshToken rotates (DELETEs) the presented refresh token, so a stolen -// access token can never self-renew — it expires in 1 hour and only a valid, -// unexpired, unrevoked refresh token can mint a new pair. A replayed refresh -// token (used twice) returns 401, detecting theft via rotation. +// VerifyRefreshToken rotates (marks used) the presented refresh token, so a +// stolen access token can never self-renew — it expires in 1 hour and only a +// valid, unexpired, unrevoked refresh token can mint a new pair. A replayed +// refresh token (used twice) returns 401, detecting theft via rotation and +// revoking the entire rotation family with a critical admin alert. func RefreshTokenHandler(w http.ResponseWriter, r *http.Request) { authHeader := r.Header.Get("Authorization") if authHeader == "" || !strings.HasPrefix(authHeader, "Bearer ") { @@ -498,9 +499,11 @@ func RefreshTokenHandler(w http.ResponseWriter, r *http.Request) { refreshToken := strings.TrimPrefix(authHeader, "Bearer ") // VerifyRefreshToken consumes (rotates) the refresh token: the used token - // is deleted from refresh_tokens, so a stolen/leaked refresh token cannot - // be replayed and an access token alone can never mint a new session. - userID, role, err := auth.VerifyRefreshToken(r.Context(), refreshToken) + // is marked used in refresh_tokens, so a stolen/leaked refresh token cannot + // be replayed and an access token alone can never mint a new session. A + // replayed (already-rotated) token revokes the entire rotation family and + // raises a critical admin alert, but still surfaces as this generic 401. + userID, role, familyID, err := auth.VerifyRefreshToken(r.Context(), refreshToken) if err != nil { mw.RespondJSON(w, http.StatusUnauthorized, map[string]string{"error": "invalid or expired refresh token"}) return @@ -522,13 +525,15 @@ func RefreshTokenHandler(w http.ResponseWriter, r *http.Request) { return } - // Issue a fresh access token + refresh token pair. + // Issue a fresh access token + refresh token pair. The rotated refresh + // token is minted in the SAME family (familyID from VerifyRefreshToken) so + // a replayed ancestor can revoke the whole lineage, descendants included. newToken, jti, err := auth.GenerateToken(userID, currentRole) if err != nil { mw.RespondError(w, http.StatusInternalServerError, "could not generate token") return } - newRefreshToken, err := auth.GenerateRefreshToken(r.Context(), userID, currentRole) + newRefreshToken, err := auth.GenerateRefreshTokenInFamily(r.Context(), userID, currentRole, familyID) if err != nil { log.Printf("failed to issue rotated refresh token for user %s: %v", userID, err) mw.RespondError(w, http.StatusInternalServerError, "could not generate refresh token") diff --git a/backend/handlers/payments/discounts.go b/backend/handlers/payments/discounts.go index abcc4cd..45a5edf 100644 --- a/backend/handlers/payments/discounts.go +++ b/backend/handlers/payments/discounts.go @@ -2,10 +2,14 @@ package payments import ( "context" + "errors" "log" + "math" "time" "crussell/db" + + "github.com/jackc/pgx/v5" ) // EligibleDiscount describes a single discount that is currently eligible for a @@ -266,14 +270,25 @@ func ComputeEligibleDiscounts(ctx context.Context, q db.Querier, bookingID, user // transaction so these writes commit atomically with the payment. It is // idempotent per booking because ComputeEligibleDiscounts excludes discounts // whose source_id is already recorded for the booking. -func ApplyEligibleDiscount(ctx context.Context, q db.Querier, bookingID, userID string, bookingTotal float64, d EligibleDiscount) { +// +// For campaign discounts the redemption counter is incremented FIRST, as an +// ATOMIC CONDITIONAL UPDATE guarded by max_redemptions (B13): two concurrent +// payments on different bookings for the same campaign can both pass the +// caller's unlocked "is it exhausted?" read, but only the first conditional +// increment matches — the loser's UPDATE affects zero rows (a 0-row result is +// returned) and this function returns a *campaignExhaustedAtApplyError with +// NOTHING written, so the caller can surface campaign_fully_redeemed. Doing the +// reservation before the booking_discounts/payment inserts keeps the +// transaction clean when a campaign is exhausted at apply time: no discount +// rows are minted for a redemption that never happened. +func ApplyEligibleDiscount(ctx context.Context, q db.Querier, bookingID, userID string, bookingTotal float64, d EligibleDiscount) error { if d.IsReferral { if _, err := q.Exec(ctx, ` INSERT INTO booking_discounts (booking_id, user_id, discount_source, source_id, campaign_type, milestone_type, discount_percent, original_total, discount_amount) VALUES ($1, $2, 'referral', $3, NULL, NULL, $4, $5, $6) `, bookingID, userID, d.SourceID, d.Percent, bookingTotal, d.Amount); err != nil { log.Printf("Failed to insert referral discount: %v", err) - return + return nil } if _, err := q.Exec(ctx, ` INSERT INTO payments (booking_id, payment_type, payment_method, amount, status, created_by) @@ -291,34 +306,56 @@ func ApplyEligibleDiscount(ctx context.Context, q db.Querier, bookingID, userID `, d.SourceID); err != nil { log.Printf("ALERT: failed to mark referral discount as used, booking %s: %v", bookingID, err) } - return + return nil } var milestoneType any if d.MilestoneType != nil { milestoneType = *d.MilestoneType } + + // B13: atomic conditional reservation. The UPDATE increments the counter + // ONLY while the campaign still has headroom; PostgreSQL makes this safe + // under READ COMMITTED — a concurrent same-row UPDATE blocks, then + // re-evaluates this WHERE against the post-increment row, so the loser + // matches zero rows instead of over-redeeming past max_redemptions. Zero + // rows means a concurrent redemption on another booking exhausted the + // campaign between the caller's preview computation and this apply-time + // re-check; nothing has been written yet, so the caller surfaces the + // campaign_fully_redeemed path (B13). + var reservedID string + if err := q.QueryRow(ctx, ` + UPDATE discount_campaigns SET times_redeemed = times_redeemed + 1 + WHERE id = $1 AND (max_redemptions IS NULL OR times_redeemed < max_redemptions) + RETURNING id + `, d.SourceID).Scan(&reservedID); err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return &campaignExhaustedAtApplyError{campaignID: d.SourceID, lostPence: int64(math.Round(d.Amount * 100))} + } + log.Printf("ALERT: failed to reserve redemption for campaign %s, booking %s: %v — discount NOT applied", d.SourceID, bookingID, err) + return nil + } + if _, err := q.Exec(ctx, ` INSERT INTO booking_discounts (booking_id, user_id, discount_source, source_id, campaign_type, milestone_type, discount_percent, original_total, discount_amount) VALUES ($1, $2, 'campaign', $3, $4, $5, $6, $7, $8) `, bookingID, userID, d.SourceID, d.CampaignType, milestoneType, d.Percent, bookingTotal, d.Amount); err != nil { - log.Printf("Failed to insert %s campaign discount: %v", d.CampaignType, err) - return + // The reservation (counter increment) already stands in this tx, so the + // redemption was consumed. Log ALERT and skip the payment record — a + // discount payment row without a booking_discounts row would be a + // ledger anomaly. max_redemptions bounds the lost reservation: the next + // eligible booking finds the campaign with one fewer redemption. + log.Printf("ALERT: failed to insert campaign discount for campaign %s, booking %s: %v", d.SourceID, bookingID, err) + return nil } if _, err := q.Exec(ctx, ` INSERT INTO payments (booking_id, payment_type, payment_method, amount, status, created_by) VALUES ($1, 'partial', 'discount', $2, 'completed', $3) `, bookingID, d.Amount, userID); err != nil { // The booking_discounts row was already inserted in this tx, so the - // campaign WAS redeemed — the times_redeemed counter must still be - // incremented below. Log ALERT and fall through to the UPDATE instead - // of returning early (a lost increment would let the campaign exceed - // its max_redemptions cap). + // campaign WAS redeemed and the reservation already stands. Log ALERT + // and return (a lost record would hide the discount from the ledger). log.Printf("ALERT: failed to insert discount payment record for campaign %s, booking %s: %v", d.SourceID, bookingID, err) } - if _, err := q.Exec(ctx, ` - UPDATE discount_campaigns SET times_redeemed = times_redeemed + 1 WHERE id = $1 - `, d.SourceID); err != nil { - log.Printf("ALERT: failed to increment times_redeemed for campaign %s, booking %s: %v", d.SourceID, bookingID, err) - } + return nil } diff --git a/backend/handlers/payments/giftcards_test.go b/backend/handlers/payments/giftcards_test.go index a928a14..b7a7c35 100644 --- a/backend/handlers/payments/giftcards_test.go +++ b/backend/handlers/payments/giftcards_test.go @@ -527,7 +527,9 @@ func TestAdminRecordPayment_CashAndGiftCard(t *testing.T) { // Setup user account with some balance first _, _ = tx.Exec(ctx, "INSERT INTO user_giftcard_balances (user_id, balance) VALUES ($1, 60.00)", adminID) - // Now pay £25 using user account balance + // Now try to pay £25 using user account balance — the booking is ALREADY + // fully paid (£30 cash + £20 clamped gift card = £50), so B3 rejects the + // overcharge instead of recording it (the user balance must be untouched). reqBody3, _ := json.Marshal(map[string]interface{}{ "amount": 2500, // £25.00 in pence "payment_type": "full", @@ -544,18 +546,18 @@ func TestAdminRecordPayment_CashAndGiftCard(t *testing.T) { r3.Post("/api/admin/bookings/{id}/payment", CreateTerminalPayment) r3.ServeHTTP(w3, req3) - if w3.Code != http.StatusOK { - t.Errorf("expected status 200, got %d. Body: %s", w3.Code, w3.Body.String()) + if w3.Code != http.StatusBadRequest { + t.Errorf("expected status 400 (booking already fully paid), got %d. Body: %s", w3.Code, w3.Body.String()) } - // Verify user account balance was deducted + // Verify user account balance was NOT deducted. var userBalance float64 err = tx.QueryRow(ctx, "SELECT balance FROM user_giftcard_balances WHERE user_id = $1", adminID).Scan(&userBalance) if err != nil { t.Fatalf("failed to query user balance: %v", err) } - if userBalance != 35.00 { - t.Errorf("expected user balance to be 35.00, got %.2f", userBalance) + if userBalance != 60.00 { + t.Errorf("expected user balance unchanged at 60.00 (overcharge rejected), got %.2f", userBalance) } } diff --git a/backend/handlers/payments/handlers.go b/backend/handlers/payments/handlers.go index 85d68fb..5467322 100644 --- a/backend/handlers/payments/handlers.go +++ b/backend/handlers/payments/handlers.go @@ -282,6 +282,13 @@ func calculateDiscountPreview(ctx context.Context, bookingID string, userID stri return resp } +// maxTerminalTipPence caps the gratuity portion of a tip-enabled terminal +// checkout (B3): the frontend embeds the tip in the charge amount, so the +// booking portion must still not exceed the remaining obligation. £50 is a +// generous single-tip bound for this business; the total charge is capped at +// remaining + this bound. +const maxTerminalTipPence = int64(5000) // £50 + // clampTerminalChargeToRemainingBalance caps a requested terminal charge at the // booking's remaining obligation (B3). The admin "Take Payment" PaymentModal // sends subtotal - discounts - campaignDiscountPence, which ignores PRIOR @@ -293,11 +300,13 @@ func calculateDiscountPreview(ctx context.Context, bookingID string, userID stri // concurrent same-booking payment. The frontend must handle the discrepancy // between the amount it displayed and the clamped amount that was charged. // -// A fully-paid booking (remaining <= 0) is NOT clamped: a deliberate -// overpayment the admin records is real money received and must stay on the -// ledger (the app's documented "overpayment handled manually at the counter" -// semantics — e.g. two identical cash receipts). The clamp protects the common -// B3 case where prior payments left a POSITIVE remaining obligation. +// A fully-paid booking (remaining <= 0) is clamped to 0 (clamped=true, +// effective=0): no obligation remains, so recording the requested amount +// verbatim would overcharge a customer who already paid in full. The callers +// reject the resulting zero-charge with 400 "already fully paid" — the only +// legitimate money on a fully-paid booking is an EXPLICIT tip, which the +// tip-enabled terminal path handles separately (it caps the total at +// remaining + maxTerminalTipPence instead of clamping here). func clampTerminalChargeToRemainingBalance(ctx context.Context, bookingID string, amount int64) (effective, remaining int64, clamped bool, err error) { remaining, err = NewPaymentService().GetBookingRemainingBalancePence(ctx, bookingID) if err != nil { @@ -306,6 +315,9 @@ func clampTerminalChargeToRemainingBalance(ctx context.Context, bookingID string if amount > remaining && remaining > 0 { return remaining, remaining, true, nil } + if remaining <= 0 { + return 0, remaining, true, nil + } return amount, remaining, false, nil } @@ -421,14 +433,23 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { // B3: clamp the recorded amount to the booking's remaining obligation. // The booking row FOR UPDATE lock above serializes concurrent cash/ // giftcard payments on this booking, so this read races no same-method - // payment. A fully-paid booking records the requested amount verbatim - // (the helper returns clamped=false for remaining <= 0). - effectiveAmount, _, clamped, cErr := clampTerminalChargeToRemainingBalance(r.Context(), bookingID, amount) + // payment. A fully-paid booking is rejected below (nothing left to + // record). + effectiveAmount, remaining, clamped, cErr := clampTerminalChargeToRemainingBalance(r.Context(), bookingID, amount) if cErr != nil { log.Printf("Failed to compute remaining balance for terminal %s payment on booking %s: %v", *req.PaymentMethod, bookingID, cErr) http.Error(w, "internal server error", http.StatusInternalServerError) return } + if clamped && effectiveAmount <= 0 { + // B3: the clamp zeroed the amount because the booking is fully paid + // (remaining <= 0). Reject rather than record a phantom £0 payment — + // an overpayment is handled manually at the counter, not minted + // into the ledger. + log.Printf("Terminal %s payment on booking %s rejected: booking already fully paid (remaining %d pence, requested %d pence)", *req.PaymentMethod, bookingID, remaining, amount) + http.Error(w, "Booking is already fully paid", http.StatusBadRequest) + return + } if clamped { log.Printf("Terminal %s payment on booking %s clamped from %d to %d pence (remaining obligation) — the frontend PaymentModal sent an amount that ignored prior payments; the customer is charged the remaining obligation only", *req.PaymentMethod, bookingID, amount, effectiveAmount) amount = effectiveAmount @@ -622,12 +643,6 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { http.Error(w, "internal server error", http.StatusInternalServerError) return } - // 2FA gating (C5): charging the customer's SAVED card requires 2FA when - // the feature is enforced. Gate on the card's owner — the booking's - // user, not the admin. New-card/terminal paths are not gated. - if bookingUserID.Valid && !requireTwoFactorForCardAccess(w, r, service, bookingUserID.String, req.VerificationCode) { - return - } // Resolve the saved-card Square source for the booking's user (the // card's owner, not the admin) — shared new-card-vs-saved-card @@ -650,21 +665,6 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { } defer releaseBookingPaymentLock(pinConn, "crussell:payment:"+bookingID) - // B3: clamp the amount to the booking's remaining obligation. The - // advisory lock above serializes all same-booking payment attempts, so - // this read races no concurrent charge. A fully-paid booking records - // the requested amount verbatim (clamped=false for remaining <= 0). - effectiveAmount, _, clamped, cErr := clampTerminalChargeToRemainingBalance(r.Context(), bookingID, amount) - if cErr != nil { - log.Printf("Failed to compute remaining balance for saved-card payment on booking %s: %v", bookingID, cErr) - http.Error(w, "internal server error", http.StatusInternalServerError) - return - } - if clamped { - log.Printf("Saved-card payment on booking %s clamped from %d to %d pence (remaining obligation) — the frontend PaymentModal sent an amount that ignored prior payments; the customer is charged the remaining obligation only", bookingID, amount, effectiveAmount) - amount = effectiveAmount - } - // Idempotency key — two tiers: // 1. Client-supplied per-attempt UUID (preferred): the frontend // generates one per DISTINCT charge and reuses it across retries of @@ -680,6 +680,12 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { // never a second charge (old-client retry safety). // Both stay ≤45 chars for Square's limit (36-char UUID / ~38-char // deterministic key). + // + // The fallback is derived from the REQUEST amount (before the B3 clamp): + // a retry sends the same request and must derive the same key to hit the + // dedup SELECT below, and the clamp runs AFTER that SELECT's + // short-circuits — so a retry of an already-completed payment on a now + // fully-paid booking still dedups instead of being clamped/rejected. scKey := req.IdempotencyKey if scKey == "" { // The candidate is built verbatim, then routed through @@ -693,7 +699,10 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { // Idempotency switch inside the lock: completed → dedup; pending → // reuse (re-attempt Square with the same key, which dedups Square-side); - // failed → clean rejection. + // failed → clean rejection. Runs BEFORE the B3 clamp so a same-key + // retry of a completed payment (on a now fully-paid booking) returns + // the existing result instead of being clamped/rejected — the money + // already moved, so the amount is no longer material. var existingID, existingStatus sql.NullString var existingAmount sql.NullFloat64 err = db.Conn.QueryRow(r.Context(), ` @@ -726,13 +735,8 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { return case err == nil && existingStatus.String == "pending": // Reuse the pending record: a prior attempt's Square outcome is - // unknown. Guard the amount — a retry with a different amount must - // not reuse the old record's charge. - if int64(math.Round(existingAmount.Float64*100)) != amount { - log.Printf("Saved-card retry amount mismatch: pending %s has %d pence, request has %d pence", existingID.String, int64(math.Round(existingAmount.Float64*100)), amount) - http.Error(w, "Amount does not match the pending payment", http.StatusBadRequest) - return - } + // unknown. The amount-match guard runs below, AFTER the clamp, so + // the clamped retry amount is compared against the original record. paymentID = existingID.String case err == nil && existingStatus.String == "failed": log.Printf("Saved-card payment %s was previously marked failed (swept) — refusing retry", existingID.String) @@ -744,6 +748,56 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { return } + // B3: clamp the amount to the booking's remaining obligation. The + // advisory lock above serializes all same-booking payment attempts, so + // this read races no concurrent charge. Runs AFTER the idempotency + // short-circuits so a same-key retry of a completed payment (booking + // now fully paid) dedups above instead of being rejected here. A fully- + // paid booking is rejected below (nothing left to charge). + effectiveAmount, remaining, clamped, cErr := clampTerminalChargeToRemainingBalance(r.Context(), bookingID, amount) + if cErr != nil { + log.Printf("Failed to compute remaining balance for saved-card payment on booking %s: %v", bookingID, cErr) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + if clamped && effectiveAmount <= 0 { + // B3: the clamp zeroed the amount because the booking is fully paid + // (remaining <= 0). Reject before any pending row or Square charge — + // charging £0 (or the requested overcharge) on a fully-paid booking + // is never legitimate. + log.Printf("Saved-card payment on booking %s rejected: booking already fully paid (remaining %d pence, requested %d pence)", bookingID, remaining, amount) + http.Error(w, "Booking is already fully paid", http.StatusBadRequest) + return + } + if clamped { + log.Printf("Saved-card payment on booking %s clamped from %d to %d pence (remaining obligation) — the frontend PaymentModal sent an amount that ignored prior payments; the customer is charged the remaining obligation only", bookingID, amount, effectiveAmount) + amount = effectiveAmount + } + + // Pending-reuse amount-match guard (moved after the clamp so the + // CLAMPED retry amount is compared against the original pending record, + // which was itself created from the clamped amount): a retry with a + // different effective amount must not reuse the old record's charge. + if paymentID != "" { + if int64(math.Round(existingAmount.Float64*100)) != amount { + log.Printf("Saved-card retry amount mismatch: pending %s has %d pence, request has %d pence", existingID.String, int64(math.Round(existingAmount.Float64*100)), amount) + http.Error(w, "Amount does not match the pending payment", http.StatusBadRequest) + return + } + } + + // 2FA gating (C5): charging the customer's SAVED card requires 2FA when + // the feature is enforced. Gate on the card's owner — the booking's + // user, not the admin. New-card/terminal paths are not gated. Runs + // 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. + if bookingUserID.Valid && !requireTwoFactorForCardAccess(w, r, service, bookingUserID.String, req.VerificationCode) { + return + } + // B13: the pre-charge discount SET for the post-charge apply-time // re-check. The online booking path (CreateBookingPayment) keeps the set // computed BEFORE the charge so applyEligibleCampaignsAtPayment can @@ -1051,16 +1105,39 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { // An accidental overpayment must never be presented to the card reader as a // charge that the record path would later carve into an unintended tip. The // advisory lock above serializes this read against concurrent same-booking - // payments. A fully-paid booking keeps the requested amount verbatim - // (clamped=false for remaining <= 0). + // payments. A fully-paid booking is rejected below (nothing left to charge). checkoutAmount := amount - if !req.TipEnabled { - effectiveAmount, _, clamped, cErr := clampTerminalChargeToRemainingBalance(r.Context(), bookingID, amount) + if req.TipEnabled { + // The tip is embedded in the amount (totalWithTip) and its value is + // unknown server-side, so cap the TOTAL at the remaining obligation + // plus a generous max tip bound: the booking portion can never exceed + // what is owed, and the tip portion can never exceed £50. + remainingPence, remErr := service.GetBookingRemainingBalancePence(r.Context(), bookingID) + if remErr != nil { + log.Printf("Failed to compute remaining balance for tip-enabled terminal checkout on booking %s: %v", bookingID, remErr) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + maxChargePence := remainingPence + maxTerminalTipPence + if checkoutAmount > maxChargePence { + log.Printf("Terminal checkout for booking %s clamped from %d to %d pence (remaining obligation %d + max tip bound £%.2f) — the requested total exceeded the booking remainder plus the tip cap", bookingID, amount, maxChargePence, remainingPence, float64(maxTerminalTipPence)/100.0) + checkoutAmount = maxChargePence + } + } else { + effectiveAmount, remaining, clamped, cErr := clampTerminalChargeToRemainingBalance(r.Context(), bookingID, amount) if cErr != nil { log.Printf("Failed to compute remaining balance for terminal checkout on booking %s: %v", bookingID, cErr) http.Error(w, "internal server error", http.StatusInternalServerError) return } + if clamped && effectiveAmount <= 0 { + // B3: the clamp zeroed the amount because the booking is fully paid + // (remaining <= 0). Reject rather than present a £0 (or overpaid) + // checkout to the card reader. + log.Printf("Terminal checkout for booking %s rejected: booking already fully paid (remaining %d pence, requested %d pence)", bookingID, remaining, amount) + http.Error(w, "Booking is already fully paid", http.StatusBadRequest) + return + } if clamped { log.Printf("Terminal checkout for booking %s clamped from %d to %d pence (remaining obligation) — the frontend PaymentModal sent an amount that ignored prior payments; the card reader will present the remaining obligation only", bookingID, amount, effectiveAmount) checkoutAmount = effectiveAmount @@ -2367,6 +2444,11 @@ func refundLostCampaignAsBalanceCredit(ctx context.Context, bookingID, userID st // discount row and the booking would silently not complete. In that case a // *campaignExhaustedAtApplyError is returned so the handler can surface a clear // "campaign fully redeemed" error and return the promised discount value. +// The pre-check loop below is a fast-fail only; the RACE is closed inside +// ApplyEligibleDiscount, whose atomic conditional increment (guarded by +// max_redemptions) is the real enforcement point — the loser of a concurrent +// same-campaign redemption gets a zero-row result there and the same error +// surfaces from the apply loop. func applyEligibleCampaignsAtPayment(ctx context.Context, q db.Querier, bookingID, userID string, expected []EligibleDiscount) error { for _, d := range expected { if d.Source != "campaign" { @@ -2404,7 +2486,13 @@ func applyEligibleCampaignsAtPayment(ctx context.Context, q db.Querier, bookingI continue } d.Amount = capped - ApplyEligibleDiscount(ctx, q, bookingID, userID, bookingTotal, d) + if applyErr := ApplyEligibleDiscount(ctx, q, bookingID, userID, bookingTotal, d); applyErr != nil { + var exErr *campaignExhaustedAtApplyError + if errors.As(applyErr, &exErr) { + return applyErr + } + log.Printf("Failed to apply %s discount %s for booking %s: %v", d.Source, d.SourceID, bookingID, applyErr) + } } return nil } diff --git a/backend/handlers/payments/loop_b_fixes_test.go b/backend/handlers/payments/loop_b_fixes_test.go index 301be85..e8817ef 100644 --- a/backend/handlers/payments/loop_b_fixes_test.go +++ b/backend/handlers/payments/loop_b_fixes_test.go @@ -68,13 +68,12 @@ func TestTerminalCash_ClampsToRemainingObligation(t *testing.T) { assert.InDelta(t, 20.00, clamped, 0.001, "the cash payment must be clamped to the £20 remaining obligation") } -// TestTerminalCash_FullyPaid_RecordsVerbatim locks the fully-paid edge of -// B3(a): when the booking has no remaining obligation, a cash payment is -// recorded VERBATIM (a deliberate admin overpayment is real money received and -// must stay on the ledger — the app's "overpayment handled at the counter" -// semantics). The clamp only protects the common B3 case where prior payments -// left a positive remaining obligation. -func TestTerminalCash_FullyPaid_RecordsVerbatim(t *testing.T) { +// TestTerminalCash_FullyPaid_RejectsOvercharge locks the fully-paid edge of +// B3(a): when the booking has no remaining obligation, a no-tip charge is +// rejected with 400 — recording the requested amount verbatim would overcharge +// a customer who already paid in full (overpayment is handled manually at the +// counter, not minted into the ledger). +func TestTerminalCash_FullyPaid_RejectsOvercharge(t *testing.T) { t.Parallel() ctx, tx := testutils.SetupTestTx(t) @@ -89,13 +88,11 @@ func TestTerminalCash_FullyPaid_RecordsVerbatim(t *testing.T) { PaymentMethod: strPtr("cash"), } w := makePaymentRequest(handler, "POST", "/api/admin/bookings/"+bookingID+"/payment", req, adminToken, ctx) - require.Equal(t, http.StatusOK, w.Code, "a fully-paid booking must still record the admin's deliberate overpayment, body: %s", w.Body.String()) + require.Equal(t, http.StatusBadRequest, w.Code, "a charge on a fully-paid booking must be rejected, body: %s", w.Body.String()) - var resp CheckoutResponse - require.NoError(t, json.Unmarshal(w.Body.Bytes(), &resp), "cash response must carry the payment id") - var lastCash float64 - require.NoError(t, tx.QueryRow(ctx, `SELECT amount FROM payments WHERE id = $1`, resp.CheckoutID).Scan(&lastCash)) - assert.InDelta(t, 45.00, lastCash, 0.001, "the fully-paid cash receipt must be recorded verbatim") + var payCount int + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND payment_method = 'cash'`, bookingID).Scan(&payCount)) + assert.Equal(t, 1, payCount, "only the £50 prior payment may exist — the overcharge must not be recorded") } // TestTerminalSavedCard_ClampsToRemainingObligation locks B3(a) for the diff --git a/backend/handlers/payments/m4_tip_refund_redesign_test.go b/backend/handlers/payments/m4_tip_refund_redesign_test.go index 1c4480a..c25ff66 100644 --- a/backend/handlers/payments/m4_tip_refund_redesign_test.go +++ b/backend/handlers/payments/m4_tip_refund_redesign_test.go @@ -323,6 +323,91 @@ func TestGetCheckoutStatus_TerminalTipSplit(t *testing.T) { assert.Equal(t, int64(5000), refundable, "tips must not be part of the refundable total") } +// TestGetCheckoutStatus_TerminalTipSplit_DiscountedBooking locks the M4 fix +// for a discounted charge: a terminal charge priced to the DISCOUNTED amount +// plus an explicit tip must carve the tip against the DISCOUNTED obligation +// (total − pending campaign discount − paid), NOT the full total — otherwise +// the whole charge is booked as the booking portion and the tip is silently +// absorbed into deposit/balance as service revenue. £50 booking with a pending +// £5 (10%) campaign, charge £49.50 = £45 service + £4.50 tip → split into +// deposit £25 + balance £20 + tip £4.50; the £5 discount then mints and the +// booking completes fully paid. +func TestGetCheckoutStatus_TerminalTipSplit_DiscountedBooking(t *testing.T) { + origClient := SquareClient + SquareClient = &testCheckoutClient{ + SquareClient: square.NewDevClient(), + hexIDs: make(map[string]string), + } + defer func() { SquareClient = origClient }() + + ctx, tx := testutils.SetupTestTx(t) + _, bookingID, _ := setupTestData(t, ctx, tx) + adminToken := jwt.GenerateAdminToken() + + now := clock.Now() + var campaignID string + require.NoError(t, tx.QueryRow(ctx, ` + INSERT INTO discount_campaigns (name, campaign_type, discount_percent, status, start_date, end_date, max_redemptions, times_redeemed) + VALUES ($1, 'time_based', 10, 'active', $2, $3, 10, 0) + RETURNING id + `, "M4 Terminal Sale", now.Add(-24*time.Hour), now.Add(24*time.Hour)).Scan(&campaignID)) + + handler := CreateTerminalPayment + req := CreateTerminalPaymentRequest{ + Amount: 4950, // £45 discounted service + £4.50 explicit tip + PaymentType: "full", + TipEnabled: true, + } + w := makePaymentRequest(handler, "POST", "/api/admin/bookings/"+bookingID+"/payment", req, adminToken, ctx) + require.Equal(t, http.StatusOK, w.Code, "body: %s", w.Body.String()) + + var createResp CheckoutResponse + require.NoError(t, json.NewDecoder(w.Body).Decode(&createResp)) + require.NotEmpty(t, createResp.CheckoutID) + + resp := pollCheckoutStatus(t, ctx, createResp.CheckoutID, bookingID, adminToken) + require.Equal(t, "COMPLETED", resp.Status) + + // £49.50 charge on a £50 booking with a pending £5 discount → booking + // portion £45 (deposit £25 + balance £20) and tip £4.50 — the tip must NOT + // be absorbed into the booking portion. (The discount row is a separate + // payment_method='discount' ledger row and is excluded here.) + rows, err := tx.Query(ctx, ` + SELECT payment_type, amount FROM payments + WHERE booking_id = $1 AND status = 'completed' + AND payment_method NOT IN ('discount', 'on_the_house') + ORDER BY payment_type + `, bookingID) + require.NoError(t, err) + defer rows.Close() + amounts := map[string]float64{} + for rows.Next() { + var pt string + var amt float64 + require.NoError(t, rows.Scan(&pt, &amt)) + amounts[pt] = amt + } + require.NoError(t, rows.Err()) + require.Len(t, amounts, 3, "the discounted tip charge must split into deposit + balance + tip records") + assert.InDelta(t, 25.0, amounts["deposit"], 0.001, "deposit = 50% of the £50 booking total") + assert.InDelta(t, 20.0, amounts["balance"], 0.001, "balance = booking portion (£45) minus deposit") + assert.InDelta(t, 4.5, amounts["tip"], 0.001, "the £4.50 explicit tip must be carved out, not absorbed") + + // The pending £5 campaign discount must still mint (real money £45 + + // discount £5 = £50) and the booking completes fully paid. + var discountPay float64 + require.NoError(t, tx.QueryRow(ctx, `SELECT COALESCE(SUM(amount), 0) FROM payments WHERE booking_id = $1 AND payment_method = 'discount' AND status = 'completed'`, bookingID).Scan(&discountPay)) + assert.InDelta(t, 5.0, discountPay, 0.001, "the pending £5 campaign discount must mint") + + var bookingStatus string + require.NoError(t, tx.QueryRow(ctx, `SELECT status FROM bookings WHERE id = $1`, bookingID).Scan(&bookingStatus)) + assert.Equal(t, "completed", bookingStatus, "real money + discount must complete the booking") + + var redeemed int + require.NoError(t, tx.QueryRow(ctx, `SELECT times_redeemed FROM discount_campaigns WHERE id = $1`, campaignID).Scan(&redeemed)) + assert.Equal(t, 1, redeemed, "the campaign redemption counter must be incremented exactly once") +} + func TestBuildTerminalSplitRecords_SplitsDepositBalanceTip(t *testing.T) { record := makeTestRecord("b-t-term", "full", 55) info := &BookingPaymentInfo{ diff --git a/backend/handlers/payments/payments_fixes_test.go b/backend/handlers/payments/payments_fixes_test.go index 0cddfab..7482fb9 100644 --- a/backend/handlers/payments/payments_fixes_test.go +++ b/backend/handlers/payments/payments_fixes_test.go @@ -348,11 +348,13 @@ func TestCreateTerminalPayment_InFlightGuard_AllowsAfterCompletion(t *testing.T) _, bookingID, _ := setupTestData(t, ctx, tx) adminToken := jwt.GenerateAdminToken() - checkoutA := createTerminalCheckout(t, ctx, bookingID, adminToken, 5000) + checkoutA := createTerminalCheckout(t, ctx, bookingID, adminToken, 3000) pollCheckoutStatus(t, ctx, checkoutA, bookingID, adminToken) - // Once the first checkout is recorded COMPLETED, a new charge is allowed. - checkoutB := createTerminalCheckout(t, ctx, bookingID, adminToken, 3000) + // Once the first checkout is recorded COMPLETED, a new charge is allowed + // (B3: a second charge is clamped to the remaining obligation, so a £20 + // charge after a £30 one on the £50 booking is fine). + checkoutB := createTerminalCheckout(t, ctx, bookingID, adminToken, 2000) if checkoutB == checkoutA { t.Error("expected a new checkout after the previous one completed") } @@ -747,14 +749,16 @@ func TestApplyEligibleDiscount_CampaignPaymentInsertFailure_StillIncrementsCount // The payments INSERT fails AFTER the booking_discounts row was inserted, // so the redemption happened — the counter MUST still increment. failing := failingExecQuerier{Querier: tx, failSQLContains: "INSERT INTO payments"} - ApplyEligibleDiscount(ctx, failing, bookingID, userID, 100.00, EligibleDiscount{ + if err := ApplyEligibleDiscount(ctx, failing, bookingID, userID, 100.00, EligibleDiscount{ Source: "campaign", Name: "Test Campaign", Percent: 10.00, Amount: 10.00, SourceID: campaignID, CampaignType: "time_based", - }) + }); err != nil { + t.Fatalf("ApplyEligibleDiscount must not return an error here: %v", err) + } var redeemed int if err := tx.QueryRow(ctx, `SELECT times_redeemed FROM discount_campaigns WHERE id = $1`, campaignID).Scan(&redeemed); err != nil { @@ -805,14 +809,16 @@ func TestApplyEligibleDiscount_ReferralPaymentInsertFailure_StillMarksUsed(t *te // The payments INSERT fails AFTER the referral's booking_discounts row was // inserted, so the discount WAS redeemed — the used flag MUST still be set. failing := failingExecQuerier{Querier: tx, failSQLContains: "INSERT INTO payments"} - ApplyEligibleDiscount(ctx, failing, bookingID, userID, 100.00, EligibleDiscount{ + if err := ApplyEligibleDiscount(ctx, failing, bookingID, userID, 100.00, EligibleDiscount{ Source: "referral", Name: "Referral Discount (10%)", Percent: 10.00, Amount: 10.00, SourceID: rdID, IsReferral: true, - }) + }); err != nil { + t.Fatalf("ApplyEligibleDiscount must not return an error here: %v", err) + } var used bool if err := tx.QueryRow(ctx, `SELECT used FROM referral_discounts WHERE id = $1`, rdID).Scan(&used); err != nil { @@ -831,30 +837,43 @@ func TestApplyEligibleDiscount_ReferralPaymentInsertFailure_StillMarksUsed(t *te } } -func TestApplyEligibleDiscount_BookingDiscountsInsertFailure_NoCounterIncrement(t *testing.T) { - // The booking_discounts-INSERT failure is the ONE early return that is - // correct: nothing was recorded, so the redemption never happened and the - // campaign counter must NOT increment. +func TestApplyEligibleDiscount_BookingDiscountsInsertFailure_CounterStillReserved(t *testing.T) { + // B13 reservation-first semantics: the campaign counter increment (the + // atomic conditional reservation) happens BEFORE the booking_discounts + // insert, so a booking_discounts-INSERT failure no longer leaves the + // counter untouched — the redemption slot was consumed and max_redemptions + // bounds the lost reservation (the next eligible booking finds the campaign + // with one fewer redemption). No discount payment row is minted. ctx, tx := testutils.SetupTestTx(t) userID, bookingID, _ := setupTestData(t, ctx, tx) campaignID := seedTestCampaign(t, ctx, tx) failing := failingExecQuerier{Querier: tx, failSQLContains: "INSERT INTO booking_discounts"} - ApplyEligibleDiscount(ctx, failing, bookingID, userID, 100.00, EligibleDiscount{ + if err := ApplyEligibleDiscount(ctx, failing, bookingID, userID, 100.00, EligibleDiscount{ Source: "campaign", Name: "Test Campaign", Percent: 10.00, Amount: 10.00, SourceID: campaignID, CampaignType: "time_based", - }) + }); err != nil { + t.Fatalf("ApplyEligibleDiscount must not return an error here: %v", err) + } var redeemed int if err := tx.QueryRow(ctx, `SELECT times_redeemed FROM discount_campaigns WHERE id = $1`, campaignID).Scan(&redeemed); err != nil { t.Fatalf("failed to read campaign counter: %v", err) } - if redeemed != 0 { - t.Errorf("expected times_redeemed unchanged (0) when the booking_discounts insert fails, got %d", redeemed) + if redeemed != 1 { + t.Errorf("expected times_redeemed incremented to 1 by the reservation despite the booking_discounts insert failure, got %d", redeemed) + } + + var discountPayments int + if err := tx.QueryRow(ctx, `SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND payment_method = 'discount'`, bookingID).Scan(&discountPayments); err != nil { + t.Fatalf("failed to count discount payments: %v", err) + } + if discountPayments != 0 { + t.Errorf("expected NO discount payment row (the booking_discounts insert was simulated to fail), got %d", discountPayments) } } diff --git a/backend/handlers/payments/payments_test.go b/backend/handlers/payments/payments_test.go index 762cd0a..22b59e3 100644 --- a/backend/handlers/payments/payments_test.go +++ b/backend/handlers/payments/payments_test.go @@ -4366,11 +4366,13 @@ func TestSavedCardPayment_ClientKey_DistinctCharges_NoDedup(t *testing.T) { handler := CreateTerminalPayment - // Two legitimately distinct £50 'full' charges on the same booking — the - // frontend sends a different per-attempt UUID for each. + // Two legitimately distinct £20 'full' charges on the same booking — the + // frontend sends a different per-attempt UUID for each. Each stays within + // the £50 booking's remaining obligation (a second £50 charge would be a + // B3 overcharge on the now fully-paid booking and correctly rejected). for i, key := range []string{"saved-card-uuid-0001", "saved-card-uuid-0002"} { reqBody := CreateTerminalPaymentRequest{ - Amount: 5000, + Amount: 2000, PaymentType: "full", PaymentMethod: strPtr("saved_card"), UserSavedCardID: &cardID, @@ -4450,9 +4452,11 @@ func TestSavedCardPayment_ClientKey_SameKeyRetry_Dedups(t *testing.T) { } // TestTerminalPayment_TwoIdenticalCashReceipts_NoDedup verifies the cash/ -// giftcard branch: two identical £50 cash receipts on the same booking are +// giftcard branch: two identical cash receipts on the same booking are // legitimate distinct payments and must each insert their own row (there is -// deliberately no idempotency dedup in this branch). +// deliberately no idempotency dedup in this branch). Each £20 receipt stays +// within the £50 booking's remaining obligation (B3 clamps only what exceeds +// the remaining balance, and a fully-paid booking rejects further charges). func TestTerminalPayment_TwoIdenticalCashReceipts_NoDedup(t *testing.T) { t.Parallel() ctx, tx := testutils.SetupTestTx(t) @@ -4466,7 +4470,7 @@ func TestTerminalPayment_TwoIdenticalCashReceipts_NoDedup(t *testing.T) { handler := CreateTerminalPayment reqBody := CreateTerminalPaymentRequest{ - Amount: 5000, + Amount: 2000, PaymentType: "full", PaymentMethod: strPtr("cash"), } diff --git a/backend/handlers/payments/refunds.go b/backend/handlers/payments/refunds.go index ac4bd17..add69ff 100644 --- a/backend/handlers/payments/refunds.go +++ b/backend/handlers/payments/refunds.go @@ -1457,8 +1457,27 @@ type manualPendingRow struct { // otherwise stay 'pending' forever, blocking the over-refund guard. They are // marked 'failed' and surfaced in the admin notification centre, mirroring the // Square-less cancellation pre-pass (refunds.go:554-583) with a DISTINCT -// origin='manual' filter so the two passes never double-process a row. +// origin='manual' filter so the two passes never double-process a row. Rows +// WITH a square_refund_id are exempt: a sweep auto-refund of a replay-induced +// duplicate charge (B1) attaches its refunds row to the still-pending parent +// payment (which has no square_payment_id) — demoting it here would kill an +// in-flight refund whose parent Square later settles. +// +// A B1 re-poll pass runs FIRST (sweepPendingB1Refunds): it re-polls sweep +// auto-refunds Square left PENDING by square_refund_id and, ONLY when Square +// reports the refund COMPLETED, marks the parent payment/till-sale row failed +// and claws back a funded gift card — the duplicate charge has been reversed +// and only then is the money state settled. func sweepManualPendingSquareRefunds(ctx context.Context) (int, error) { + // (a0) B1-origin sweep auto-refunds (refunds rows with a square_refund_id + // that Square left PENDING): re-poll each by square_refund_id and + // resolve the parent row ONLY on COMPLETED. Runs before the passes + // below so an in-flight B1 refund is never demoted or re-issued. + b1Count, b1Err := sweepPendingB1Refunds(ctx) + if b1Err != nil { + log.Printf("Failed to re-poll B1 sweep auto-refunds: %v", b1Err) + } + // (a) Terminal pre-pass: legacy MANUAL card refunds whose payment has no // Square reference can never be refunded via Square → mark them failed // so they stop blocking the over-refund guard, and surface the affected @@ -1467,7 +1486,9 @@ func sweepManualPendingSquareRefunds(ctx context.Context) (int, error) { // refunds.go:554-583) with origin='manual' so the filters stay distinct // and no row is swept by both passes. Must run OUTSIDE any GROUP BY — // Postgres lumps NULLs together, so these rows can't be handled in the - // per-payment grouping below. + // per-payment grouping below. Rows WITH a square_refund_id are exempt — + // a B1 auto-refund attaches to a parent payment with no square_payment_id + // and its in-flight refund must not be demoted to failed. rows, err := db.Conn.Query(ctx, fmt.Sprintf(` UPDATE refunds r SET status = 'failed' FROM payments p @@ -1476,6 +1497,7 @@ func sweepManualPendingSquareRefunds(ctx context.Context) (int, error) { AND p.payment_method IN ('online_square', 'in_person_card') AND p.square_payment_id IS NULL AND r.origin = 'manual' + AND r.square_refund_id IS NULL RETURNING r.id `, maxManualRefundAttempts)) if err != nil { @@ -1559,9 +1581,191 @@ func sweepManualPendingSquareRefunds(ctx context.Context) (int, error) { } processed += n } + return processed + b1Count, nil +} + +// b1PendingRefund is one refunds row for a sweep auto-refund of a +// replay-induced duplicate charge (B1) that Square left PENDING. +type b1PendingRefund struct { + RefundID string + PaymentID string // refunds.payment_id — the parent payment row + Amount float64 + SquareRefundID string + IdempotencyKey string // deterministic "sweepdup-" + duplicate payment id (payments-table rows) + Reason string // carries the parent till_sale id for till_sale rows + SquarePaymentID string // synthetic till-sale payment row's square_payment_id ("" for payments-table rows) + CreatedBy string +} + +// sweepPendingB1Refunds re-polls refunds rows for sweep auto-refunds of +// replay-induced duplicate charges (B1, refundSweepDuplicateCharge in sweep.go) +// that Square left PENDING. The refund is NON-terminal: the parent row must +// stay pending (never marked failed, a funded gift card never clawed back) +// until Square settles. When Square reports the refund COMPLETED the parent is +// finally resolved — the pending payment marked failed, and a till sale's +// funded gift card clawed back (the duplicate charge has been reversed; only +// then is the money state settled). FAILED/REJECTED refunds are left pending +// for the webhook FAILED-refund reconciliation (coordinated); a reconcile +// error is an UNKNOWN state and is never resolved here. The stale-pending +// sweeps skip rows with an in-flight B1 refund (hasInFlightSweepDuplicateRefund, +// sweep.go), so the parent is only ever resolved from here. +func sweepPendingB1Refunds(ctx context.Context) (int, error) { + rows, err := db.Conn.Query(ctx, ` + SELECT r.id, r.payment_id, r.amount, r.square_refund_id, COALESCE(r.idempotency_key, ''), + r.reason, COALESCE(p.square_payment_id, ''), COALESCE(r.created_by, '') + FROM refunds r + LEFT JOIN payments p ON p.id = r.payment_id + WHERE r.status = 'pending' AND r.square_refund_id IS NOT NULL + AND r.origin = 'manual' AND r.reason LIKE 'duplicate charge — sweep replay%' + ORDER BY r.id + `) + if err != nil { + log.Printf("Failed to query B1 sweep auto-refunds for re-poll: %v", err) + return 0, nil + } + defer rows.Close() + var pending []b1PendingRefund + for rows.Next() { + var pr b1PendingRefund + if err := rows.Scan(&pr.RefundID, &pr.PaymentID, &pr.Amount, &pr.SquareRefundID, &pr.IdempotencyKey, &pr.Reason, &pr.SquarePaymentID, &pr.CreatedBy); err != nil { + log.Printf("Failed to scan B1 sweep auto-refund: %v", err) + continue + } + pending = append(pending, pr) + } + if err := rows.Err(); err != nil { + log.Printf("Failed to iterate B1 sweep auto-refunds: %v", err) + return 0, nil + } + if len(pending) == 0 { + return 0, nil + } + + processed := 0 + for i := range pending { + pr := &pending[i] + // The Square payment id the refund targets. A till_sale's refund is + // attached to the synthetic payments row (square_payment_id = the + // duplicate charge); a payments-table refund is attached to the + // still-pending payment row (no square_payment_id), so the duplicate id + // is recovered from the deterministic refund idempotency key. + dupPayID := pr.SquarePaymentID + if dupPayID == "" { + dupPayID = strings.TrimPrefix(pr.IdempotencyKey, "sweepdup-") + if dupPayID == pr.IdempotencyKey { + log.Printf("Cannot re-poll B1 refund %s: no Square payment id and the stored key %q is not the sweepdup form — leaving pending", pr.RefundID, pr.IdempotencyKey) + continue + } + } + refundList, lErr := SquareClient.ListPaymentRefunds(ctx, dupPayID, time.Time{}) + if lErr != nil { + log.Printf("Re-poll of B1 refund %s failed (%v) — leaving pending", pr.RefundID, lErr) + continue + } + status := "" + for j := range refundList { + if refundList[j].ID == pr.SquareRefundID { + status = refundList[j].Status + break + } + } + switch status { + case "COMPLETED": + if resolveB1RefundCompleted(ctx, pr) { + processed++ + } + case "PENDING", "APPROVED": + log.Printf("B1 refund %s is still %q at Square — leaving the parent row pending", pr.RefundID, status) + case "": + log.Printf("B1 refund %s (%s) was not found at Square via ListPaymentRefunds — leaving pending; manual reconciliation may be required", pr.RefundID, dupPayID) + default: + // FAILED / REJECTED — the webhook FAILED-refund reconciliation + // (coordinated) owns this terminal state; never resolve it here. + log.Printf("B1 refund %s is %q at Square — leaving pending for the webhook FAILED-refund reconciliation", pr.RefundID, status) + } + } return processed, nil } +// resolveB1RefundCompleted resolves a B1 sweep auto-refund that Square reports +// COMPLETED: the refunds row is marked completed (money moved), and the parent +// row — the pending payment, or the pending till sale whose gift-card funding +// is clawed back — is resolved to failed. Returns true when the parent was +// resolved. +func resolveB1RefundCompleted(ctx context.Context, pr *b1PendingRefund) bool { + if _, err := db.Conn.Exec(ctx, ` + UPDATE refunds SET status = 'completed' + WHERE id = $1 AND status = 'pending' + `, pr.RefundID); err != nil { + log.Printf("Failed to mark B1 refund %s completed after Square settle: %v", pr.RefundID, err) + } + if pr.Reason == sweepDuplicateRefundReason { + // payments-table parent: the refund's payment_id IS the pending row. + if failStaleRow(ctx, "payments", pr.PaymentID) { + log.Printf("B1 refund %s COMPLETED at Square — marked the pending payment %s failed (the duplicate charge was reversed)", pr.RefundID, pr.PaymentID) + return true + } + log.Printf("B1 refund %s COMPLETED at Square but payment %s was already resolved", pr.RefundID, pr.PaymentID) + return false + } + // till_sale parent: the sale id is encoded in the reason. + if idx := strings.Index(pr.Reason, "(till_sale "); idx >= 0 { + tillSaleID := strings.TrimSuffix(pr.Reason[idx+len("(till_sale "):], ")") + ts, ok := loadTillSaleStaleRow(ctx, tillSaleID) + if !ok { + log.Printf("CRITICAL: B1 refund %s COMPLETED at Square but parent till sale %s could not be loaded — MANUAL RECONCILIATION REQUIRED: gift card may still be funded", pr.RefundID, tillSaleID) + return false + } + if ts.HasGiftCard { + if clawbackTillSaleFunding(ctx, ts) { + log.Printf("B1 refund %s COMPLETED at Square — clawed back till sale %s's funded gift card and marked it failed", pr.RefundID, tillSaleID) + return true + } + log.Printf("CRITICAL: B1 refund %s COMPLETED at Square but clawing back till sale %s's funding failed — MANUAL RECONCILIATION REQUIRED: gift card may still be funded", pr.RefundID, tillSaleID) + return false + } + if failStaleRow(ctx, "till_sales", tillSaleID) { + log.Printf("B1 refund %s COMPLETED at Square — marked till sale %s failed", pr.RefundID, tillSaleID) + return true + } + log.Printf("B1 refund %s COMPLETED at Square but till sale %s was already resolved", pr.RefundID, tillSaleID) + return false + } + log.Printf("B1 refund %s COMPLETED at Square but the parent row could not be identified from reason %q — MANUAL RECONCILIATION REQUIRED", pr.RefundID, pr.Reason) + return false +} + +// loadTillSaleStaleRow reads a till_sale's gift-card context for the B1 +// re-poll pass's clawback — the same fields fetchStaleRows/scanStaleRow +// populate for the stale-pending sweep. +func loadTillSaleStaleRow(ctx context.Context, id string) (staleRow, bool) { + var r staleRow + var itemID, redeemedBy sql.NullString + var isCreate *bool + var hasGiftCard bool + var total float64 + err := db.Conn.QueryRow(ctx, ` + SELECT ts.id, ts.item_id, gc.redeemed_by, (ts.created_at = gc.created_at) AS is_create, + (gc.id IS NOT NULL) AS has_gift_card, ts.total_amount + FROM till_sales ts + LEFT JOIN gift_cards gc ON gc.id = ts.item_id + WHERE ts.id = $1 + `, id).Scan(&r.ID, &itemID, &redeemedBy, &isCreate, &hasGiftCard, &total) + if err != nil { + log.Printf("Failed to load till sale %s for B1 refund clawback: %v", id, err) + return r, false + } + r.ItemID = itemID.String + if redeemedBy.Valid && redeemedBy.String != "" { + r.RedeemToUserID = &redeemedBy.String + } + r.IsCreate = isCreate != nil && *isCreate + r.HasGiftCard = hasGiftCard + r.TotalAmount = total + r.AmountPence = int64(math.Round(total * 100)) + return r, true +} + // ensureRefundKey returns the idempotency key to use when re-issuing a manual // refund, persisting a generated fallback to the refunds row BEFORE Square is // called so every retry reuses the SAME key — Square dedups same-key retries, diff --git a/backend/handlers/payments/sweep.go b/backend/handlers/payments/sweep.go index 2e744e4..9df2417 100644 --- a/backend/handlers/payments/sweep.go +++ b/backend/handlers/payments/sweep.go @@ -220,6 +220,14 @@ func sweepStaleRows(ctx context.Context, table string, cutoff time.Time) (resolv return 0, 0, err } for _, r := range stale { + // B1: a row whose auto-refund of a replay-induced duplicate charge is + // still PENDING at Square must never be blind-failed (or have its gift + // card clawed back) — the refund is non-terminal and may still settle. + // The B1 re-poll pass resolves it on settlement. + if hasInFlightSweepDuplicateRefund(ctx, table, r.ID) { + log.Printf("Stale pending %s row %s has a sweep auto-refund of a duplicate charge still pending at Square — leaving pending until the refund settles", table, r.ID) + continue + } if r.SquarePaymentID != "" { switch reconcileStalePaymentAtSquare(ctx, table, r.SquarePaymentID) { case staleReconcileCompleted: @@ -323,6 +331,15 @@ func sweepKeyedStaleRows(ctx context.Context, table string, cutoff time.Time) (r // a row older than that when swept can no longer be replayed trustworthily. replayExpired := clock.Now().Add(-stalePendingPaymentAge) for _, r := range stale { + // B1: a row whose auto-refund of a replay-induced duplicate charge is + // still PENDING at Square must be left alone — re-playing its expired + // idempotency key can land ANOTHER charge, and blind-failing or clawing + // back while the refund is non-terminal would reverse money that may + // still come back. The B1 re-poll pass resolves it on settlement. + if hasInFlightSweepDuplicateRefund(ctx, table, r.ID) { + log.Printf("Stale pending %s row %s has a sweep auto-refund of a duplicate charge still pending at Square — leaving pending until the refund settles", table, r.ID) + continue + } if r.CreatedAt.Before(replayExpired) { // Key retention window already closed — replaying would misread an // expired key as "never charged". Blind-fail + WARN exactly as the @@ -948,11 +965,16 @@ func reconcileStalePaymentByKey(ctx context.Context, table string, r staleRow) ( // failed (the original charge was never found — the caller's // definitively-failed branch does that; a till sale's gift-card // funding is clawed back there too, since the duplicate has been - // refunded). Only a refund FAILURE keeps the CRITICAL - // manual-reconciliation path. + // refunded). A refund Square leaves PENDING is NON-terminal: the row + // stays pending (no fail, no clawback) and the B1 re-poll pass + // resolves it when Square settles. Only a refund FAILURE keeps the + // CRITICAL manual-reconciliation path. if refundErr := refundSweepDuplicateCharge(ctx, table, r, pr); refundErr == nil { log.Printf("stale pending %s row %s: the replayed COMPLETED payment %s was a NEW charge under an expired idempotency key (lag %s) — auto-refunded the duplicate at Square and marking the row failed (the original charge was never found)", table, r.ID, pr.ID, lag) return staleReconcileDefinitivelyFailed, "" + } else if errors.Is(refundErr, errSweepRefundPending) { + log.Printf("stale pending %s row %s: the auto-refund of the replay-induced duplicate charge %s is PENDING at Square (non-terminal) — leaving the row pending; the B1 refund re-poll resolves it when Square settles", table, r.ID, pr.ID) + return staleReconcileLeavePending, "" } else { return leavePendingCritical(ctx, r, "stale pending %s reconcile by key: the replayed COMPLETED payment %s was created after the pending row %s (lag %s) — a NEW charge under an expired idempotency key; auto-refund FAILED (%v) — leaving the row PENDING — MANUAL RECONCILIATION REQUIRED: check Square for both charges and refund the duplicate", table, pr.ID, r.ID, lag, refundErr) } @@ -970,6 +992,38 @@ func reconcileStalePaymentByKey(ctx context.Context, table string, r staleRow) ( } } +// errSweepRefundPending is the sentinel refundSweepDuplicateCharge returns when +// Square accepted the auto-refund of a replay-induced duplicate charge but left +// it PENDING — a NON-terminal state: the refund may still complete (the +// duplicate is reversed) or fail. The caller must NOT treat it as success: the +// parent row is left PENDING (never marked failed, a till sale's funded gift +// card is never clawed back) so a later sweep run can re-poll the refund. The +// refund's idempotency key ("sweepdup-"+paymentID) is deterministic, so the +// re-poll finds the SAME refund at Square. +var errSweepRefundPending = errors.New("sweep duplicate-charge refund pending at Square") + +// sweepDuplicateRefundReason is the audit-trail reason carried by every +// refunds row for a sweep auto-refund of a replay-induced duplicate charge +// (B1). The B1 re-poll pass (sweepPendingB1Refunds, refunds.go) and the +// in-flight guard (hasInFlightSweepDuplicateRefund) match on it, so it must +// stay in lockstep with refunds.go. For a till_sale the reason carries the +// parent till_sale id (see sweepDuplicateRefundReasonFor) — refunds has no +// till_sale column and the refund must link back to the sale the re-poll pass +// claws back when Square settles. +const sweepDuplicateRefundReason = "duplicate charge — sweep replay" + +// sweepDuplicateRefundReasonFor returns the refunds-row reason for a sweep +// auto-refund. tillSaleID is "" for payments-table rows (whose parent is the +// refund's own payment_id); a till_sale's parent is encoded in the reason +// because refunds.payment_id is FK'd to payments and the sale has no payments +// row of its own. +func sweepDuplicateRefundReasonFor(tillSaleID string) string { + if tillSaleID == "" { + return sweepDuplicateRefundReason + } + return sweepDuplicateRefundReason + " (till_sale " + tillSaleID + ")" +} + // refundSweepDuplicateCharge auto-refunds a replayed COMPLETED payment that the // sweep proved to be a NEW charge under an expired idempotency key (B1) — money // the customer never authorized. The refund reuses the existing Square refund @@ -983,13 +1037,23 @@ func reconcileStalePaymentByKey(ctx context.Context, table string, r staleRow) ( // gets a fresh key and is refunded too; // - reason "duplicate charge — sweep replay" for the audit trail. // -// On success a refunds row is recorded for payments-table rows (a till_sale -// row has no payments row to attach the refund to — the Square refund plus the -// deduped critical-payment admin notification cover the audit trail there) and -// an admin notification is inserted so an operator sees the auto-refund. The -// caller then marks the row definitively failed (the ORIGINAL charge was never -// found). Any failure leaves the money state at Square untouched and returns -// the error so the caller keeps the CRITICAL manual-reconciliation path. +// A refunds row is recorded for BOTH payments-table and till_sale rows (a +// till_sale's refund attaches to a synthetic payments row created for the +// duplicate charge, since refunds.payment_id is FK'd to payments; the insert +// is idempotent on the deterministic refund key, whose UNIQUE constraint +// doubles as the dedup guard), and an admin notification is inserted so an +// operator sees the auto-refund. +// +// Return semantics: +// +// - nil — the refund COMPLETED at Square; the caller marks the row +// definitively failed (the ORIGINAL charge was never found) and, for a till +// sale, claws back the funded gift card — the duplicate has been reversed; +// - errSweepRefundPending — the refund is PENDING (non-terminal); the caller +// leaves the row pending; the B1 re-poll pass (sweepPendingB1Refunds, +// refunds.go) resolves the row when Square settles; +// - any other error — the money state at Square is untouched; the caller +// keeps the CRITICAL manual-reconciliation path. func refundSweepDuplicateCharge(ctx context.Context, table string, r staleRow, pr *square.PaymentResult) error { if pr == nil || pr.ID == "" { return errors.New("replayed payment has no Square payment id to refund") @@ -1004,39 +1068,127 @@ func refundSweepDuplicateCharge(ctx context.Context, table string, r staleRow, p if len(refundKey) > maxIdempotencyKeyLength { refundKey = truncateIdempotencyKey("sweepdup", pr.ID) } - reason := "duplicate charge — sweep replay" res, refundErr := SquareClient.RefundPayment(ctx, square.RefundPaymentReq{ PaymentID: pr.ID, Amount: r.AmountPence, IdempotencyKey: refundKey, - Reason: reason, + Reason: sweepDuplicateRefundReason, }) if refundErr != nil { return fmt.Errorf("auto-refund of replay-induced duplicate charge %s failed: %w", pr.ID, refundErr) } status := "completed" - if res.Status == "PENDING" { + pending := false + switch res.Status { + case "PENDING", "APPROVED": + // Square's non-terminal refund states — the money has not moved yet but + // the refund is in flight. NON-terminal: the caller must leave the row + // pending and never claw back a funded gift card (reversing the funding + // before Square settles the refund could leave a customer charged with + // no gift card if the refund later fails). status = "pending" - } else if res.Status == "FAILED" || res.Status == "REJECTED" { + pending = true + case "FAILED", "REJECTED": // Square definitively rejected the refund — the duplicate charge stands. return fmt.Errorf("auto-refund of replay-induced duplicate charge %s was %s at Square", pr.ID, res.Status) } - if table == "payments" { - amountPounds := float64(r.AmountPence) / 100.0 - if _, insErr := db.Conn.Exec(ctx, ` - INSERT INTO refunds (payment_id, booking_id, amount, square_refund_id, status, origin, reason, idempotency_key, created_by, created_at) - VALUES ($1, $2, $3, $4, $5, 'manual', $6, $7, $8, NOW()) - `, r.ID, r.BookingID, amountPounds, res.ID, status, reason, refundKey, r.CreatedBy); insErr != nil { - log.Printf("CRITICAL: auto-refunded duplicate charge %s at Square (refund %s) but recording the refunds row for pending %s failed: %v — MANUAL RECONCILIATION REQUIRED", pr.ID, res.ID, r.ID, insErr) - } - } + + recordSweepDuplicateRefundRow(ctx, table, r, pr, res.ID, status, refundKey) insertCriticalPaymentNotification(ctx, r.BookingID, r.CreatedBy) + if pending { + log.Printf("Auto-refund of replay-induced duplicate charge %s (%d pence) for pending row %s is PENDING at Square (refund %s) — leaving the row pending for the refund re-poll", pr.ID, r.AmountPence, r.ID, res.ID) + return errSweepRefundPending + } log.Printf("Auto-refunded replay-induced duplicate charge %s (%d pence) for pending row %s — refund %s", pr.ID, r.AmountPence, r.ID, res.ID) return nil } +// recordSweepDuplicateRefundRow writes the refunds row for a sweep auto-refund +// of a replay-induced duplicate charge (B1), idempotently on the deterministic +// refund idempotency key (UNIQUE — a re-run can never mint a second row). For +// a payments-table row the refund attaches to the pending payment row itself. +// A till_sale has no payments row (refunds.payment_id is NOT NULL + FK), so a +// synthetic payments row is created FIRST — 'completed' (the duplicate charge +// genuinely landed at Square) with the duplicate's square_payment_id, so the +// B1 re-poll pass can look the refund up by payment id — and the refund is +// attached to it. The parent till_sale id is carried in the reason so the +// re-poll pass claws it back when Square settles. +func recordSweepDuplicateRefundRow(ctx context.Context, table string, r staleRow, pr *square.PaymentResult, refundID, status, refundKey string) { + amountPounds := float64(r.AmountPence) / 100.0 + paymentID := r.ID + var bookingID *string + reason := sweepDuplicateRefundReason + if table == "payments" { + bookingID = r.BookingID + } else { + // till_sale: create the synthetic payments row for the duplicate charge. + payKey := truncateIdempotencyKey("sweepdup-pay", pr.ID) + err := db.Conn.QueryRow(ctx, ` + INSERT INTO payments (payment_type, payment_method, status, amount, square_payment_id, idempotency_key, created_by, created_at) + VALUES ('full', 'in_person_card', 'completed', $1, $2, $3, $4, NOW()) + ON CONFLICT (idempotency_key) DO NOTHING + RETURNING id + `, amountPounds, pr.ID, payKey, r.CreatedBy).Scan(&paymentID) + if err != nil { + if !errors.Is(err, pgx.ErrNoRows) { + log.Printf("CRITICAL: auto-refunded duplicate charge %s at Square (refund %s) but creating the payments row for till sale %s failed: %v — MANUAL RECONCILIATION REQUIRED", pr.ID, refundID, r.ID, err) + return + } + // Re-run: the synthetic row already exists — reuse it. + if rErr := db.Conn.QueryRow(ctx, `SELECT id FROM payments WHERE idempotency_key = $1`, payKey).Scan(&paymentID); rErr != nil { + log.Printf("CRITICAL: auto-refunded duplicate charge %s at Square (refund %s) but re-reading the payments row for till sale %s failed: %v — MANUAL RECONCILIATION REQUIRED", pr.ID, refundID, r.ID, rErr) + return + } + } + reason = sweepDuplicateRefundReasonFor(r.ID) + } + if _, insErr := db.Conn.Exec(ctx, ` + INSERT INTO refunds (payment_id, booking_id, amount, square_refund_id, status, origin, reason, idempotency_key, created_by, created_at) + VALUES ($1, $2, $3, $4, $5, 'manual', $6, $7, $8, NOW()) + ON CONFLICT (idempotency_key) DO UPDATE SET status = EXCLUDED.status, square_refund_id = EXCLUDED.square_refund_id + `, paymentID, bookingID, amountPounds, refundID, status, reason, refundKey, r.CreatedBy); insErr != nil { + log.Printf("CRITICAL: auto-refunded duplicate charge %s at Square (refund %s) but recording the refunds row failed: %v — MANUAL RECONCILIATION REQUIRED", pr.ID, refundID, insErr) + } +} + +// hasInFlightSweepDuplicateRefund reports whether a stale pending row carries a +// sweep auto-refund of a replay-induced duplicate charge (B1) that Square left +// PENDING. While the refund is in flight the row must NOT be replayed (a replay +// of the expired-key row could land ANOTHER charge), blind-failed or clawed +// back (the refund is non-terminal — money may still reverse). The B1 re-poll +// pass (sweepPendingB1Refunds, refunds.go) resolves the parent row when Square +// settles. For a till_sale the refund row's reason carries the sale id (see +// sweepDuplicateRefundReasonFor); a payments-table refund's payment_id IS the +// parent row. +func hasInFlightSweepDuplicateRefund(ctx context.Context, table, id string) bool { + var exists bool + var err error + if table == "till_sales" { + err = db.Conn.QueryRow(ctx, ` + SELECT EXISTS ( + SELECT 1 FROM refunds + WHERE status = 'pending' AND square_refund_id IS NOT NULL + AND reason = $1 + ) + `, sweepDuplicateRefundReasonFor(id)).Scan(&exists) + } else { + err = db.Conn.QueryRow(ctx, ` + SELECT EXISTS ( + SELECT 1 FROM refunds + WHERE payment_id = $1 AND status = 'pending' AND square_refund_id IS NOT NULL + AND reason = $2 + ) + `, id, sweepDuplicateRefundReason).Scan(&exists) + } + if err != nil { + log.Printf("Failed to check for an in-flight sweep duplicate refund on %s row %s: %v", table, id, err) + return false + } + return exists +} + // leaveGiftCardPurchasePending keeps a gift-card-purchase payment row (payments // table, booking_id NULL) pending after Square confirms the charge COMPLETED, // instead of rescuing it to 'completed'. Completing the row would permanently @@ -1745,6 +1897,14 @@ func recordTerminalPaymentTx(ctx context.Context, tx pgx.Tx, checkoutID, booking UpdatedAt: clock.Now(), } + // The booking user is read here (before the M4 carve) because the carve + // computes the campaign discounts applyEligibleCampaignsAtPayment is about + // to apply below, and that apply needs the payer's user id. + var bookingUserID string + if err := tx.QueryRow(ctx, `SELECT COALESCE(user_id, '') FROM bookings WHERE id = $1`, bookingID).Scan(&bookingUserID); err != nil { + slog.Error("Failed to load booking user for terminal campaign apply", "booking_id", bookingID, "err", err) + } + // M4 split: a terminal charge above the remaining booking value is a tip — // record it as its own record so only the booking portion is refundable. // B3: only when the customer EXPLICITLY requested a tip (checkout @@ -1758,13 +1918,50 @@ func recordTerminalPaymentTx(ctx context.Context, tx pgx.Tx, checkoutID, booking if bErr == nil && bookingInfo != nil { charged := float64(pr.Amount) / 100.0 remainingBookingValue := math.Max(0, bookingInfo.TotalAmount-bookingInfo.TotalPaid) + // Reserve the headroom of the campaign discounts that + // applyEligibleCampaignsAtPayment is about to mint: a + // terminal charge already priced to the discounted amount (the frontend + // charges total−discount plus any explicit tip) must carve the tip + // against the DISCOUNTED obligation, not the full total — otherwise the + // whole charge lands as booking portion and the tip is silently + // absorbed into deposit/balance instead of being recorded as gratuity + // (M4). The no-tip case is unchanged: the booking portion exactly + // covers the discounted obligation and the discount fills the rest. + if pending := pendingCampaignDiscountAmount(ctx, tx, bookingID, bookingUserID, bookingInfo.TotalAmount); pending > 0.004 { + remainingBookingValue = math.Max(0, remainingBookingValue-pending) + } bookingPortion := math.Min(charged, remainingBookingValue) bookingPortion = math.Round(bookingPortion*100) / 100 - tipAmount := math.Round((charged-bookingPortion)*100) / 100 + tipAmount := math.Max(0, math.Round((charged-bookingPortion)*100)/100) if checkoutTipEnabled && tipAmount > 0.004 { records = buildTerminalSplitRecords(record, bookingInfo, bookingPortion, tipAmount) } } + hasTip := false + for _, rec := range records { + if rec.PaymentType == "tip" { + hasTip = true + break + } + } + // Apply eligible campaigns BEFORE the records are inserted when the M4 + // carve minted a TIP record and a campaign discount is pending. The apply + // path refuses discounts once a booking has 2+ completed real payments, and + // the split records (deposit + balance + tip) would count as 2+ even though + // they are ONE charge — refusing the discount the frontend already priced + // in would leave the booking under-discounted and never complete. Applying + // before the insert is money-safe: a tip record only exists when the charge + // exceeds the DISCOUNTED obligation, so the booking portion plus the + // pending discount can never exceed the total — the F1 full-amount + // over-credit cannot occur here (a full-amount charge never produces a tip + // record, so it keeps the after-insert apply whose cap refuses it). + appliedCampaignsBeforeInsert := false + if hasTip && bookingInfo != nil && bErr == nil { + if pendingCampaignDiscountAmount(ctx, tx, bookingID, bookingUserID, bookingInfo.TotalAmount) > 0.004 { + applyEligibleCampaignsAtPayment(ctx, tx, bookingID, bookingUserID, nil) + appliedCampaignsBeforeInsert = true + } + } if len(records) == 0 { records = []PaymentRecord{record} } @@ -1804,13 +2001,14 @@ func recordTerminalPaymentTx(ctx context.Context, tx pgx.Tx, checkoutID, booking // skip re-application via their already-recorded guards. The apply runs // AFTER the payment record insert so the headroom counts the charge as real // money (never over-credits), and after the M4 tip split so tip records - // never affect the discount computation. - var bookingUserID string - if err := tx.QueryRow(ctx, `SELECT COALESCE(user_id, '') FROM bookings WHERE id = $1`, bookingID).Scan(&bookingUserID); err != nil { - slog.Error("Failed to load booking user for terminal campaign apply", "booking_id", bookingID, "err", err) - } - if applyErr := applyEligibleCampaignsAtPayment(ctx, tx, bookingID, bookingUserID, nil); applyErr != nil { - slog.Error("Failed to apply eligible campaigns for terminal checkout", "booking_id", bookingID, "err", applyErr) + // never affect the discount computation. When the M4 carve minted a tip + // record the discount was already applied BEFORE the insert (see above), so + // the apply is skipped here — a second run is idempotent but unnecessary. + // The booking user was already read above for the M4 carve. + if !appliedCampaignsBeforeInsert { + if applyErr := applyEligibleCampaignsAtPayment(ctx, tx, bookingID, bookingUserID, nil); applyErr != nil { + slog.Error("Failed to apply eligible campaigns for terminal checkout", "booking_id", bookingID, "err", applyErr) + } } // Release the in-flight guard: this checkout is now recorded. @@ -1831,6 +2029,25 @@ func recordTerminalPaymentTx(ctx context.Context, tx pgx.Tx, checkoutID, booking return paymentID, nil } +// pendingCampaignDiscountAmount sums the campaign discounts that +// applyEligibleCampaignsAtPayment is about to apply for the booking (its +// 'expected' argument is nil in the terminal flow, so it recomputes the +// eligible set from scratch). The discount rows do not exist yet when the M4 +// tip carve runs — they are minted later in the same transaction — so the +// carve must reserve their headroom NOW or a terminal charge already priced to +// the discounted amount absorbs the customer's explicit tip into the booking +// portion (M4). Referral discounts are excluded: the terminal apply path never +// mints them. +func pendingCampaignDiscountAmount(ctx context.Context, q db.Querier, bookingID, userID string, bookingTotal float64) float64 { + var total float64 + for _, d := range ComputeEligibleDiscounts(ctx, q, bookingID, userID, bookingTotal) { + if d.Source == "campaign" { + total += d.Amount + } + } + return math.Round(total*100) / 100 +} + // recordUntrackedTillSalePayment records a stale card-machine till sale whose // checkout COMPLETED at Square but was never polled/recorded: the sale is // marked 'completed' with the returned square_payment_id written back. A diff --git a/backend/handlers/payments/sweep_test.go b/backend/handlers/payments/sweep_test.go index 32d6d90..a5abef9 100644 --- a/backend/handlers/payments/sweep_test.go +++ b/backend/handlers/payments/sweep_test.go @@ -837,6 +837,246 @@ func TestSweepStalePendingPayments_KeyedReplayNewCharge_AutoRefunded(t *testing. } } +// pendingRefundClient forces RefundPayment to return a PENDING result so the +// B1 auto-refund branch is exercised: Square accepted the refund but left it +// non-terminal. +type pendingRefundClient struct { + square.SquareClient +} + +func (c *pendingRefundClient) RefundPayment(ctx context.Context, req square.RefundPaymentReq) (*square.RefundResult, error) { + res, err := c.SquareClient.RefundPayment(ctx, req) + if err != nil { + return nil, err + } + res.Status = "PENDING" + return res, nil +} + +// settledRefundClient reports every refund Square holds as COMPLETED — as if a +// PENDING refund settled — so the B1 re-poll pass resolves the parent row. +type settledRefundClient struct { + square.SquareClient +} + +func (c *settledRefundClient) ListPaymentRefunds(ctx context.Context, paymentID string, beginTime time.Time) ([]square.RefundResult, error) { + refunds, err := c.SquareClient.ListPaymentRefunds(ctx, paymentID, beginTime) + if err != nil { + return nil, err + } + for i := range refunds { + refunds[i].Status = "COMPLETED" + } + return refunds, nil +} + +// TestSweepStalePendingPayments_KeyedReplayNewCharge_RefundPending_LeavesRowPending +// locks the B1 PENDING fix: when Square accepts the auto-refund of a +// replay-induced duplicate charge but leaves it PENDING (non-terminal), the +// sweep must NOT mark the parent payment failed and must record the refunds +// row with status 'pending' + square_refund_id. The re-poll pass +// (SweepPendingSquareRefunds) then resolves the parent row ONLY once Square +// reports the refund COMPLETED. Sequential (flips SQUARE_ENVIRONMENT), like the +// sibling B1 tests. +func TestSweepStalePendingPayments_KeyedReplayNewCharge_RefundPending_LeavesRowPending(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + if err != nil { + t.Fatalf("failed to create user: %v", err) + } + serviceID, err := fixtures.CreateTestService(tx) + if err != nil { + t.Fatalf("failed to create service: %v", err) + } + bookingID, err := fixtures.CreateTestBookingAtTime(tx, userID, serviceID, + time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC)) + if err != nil { + t.Fatalf("failed to create booking: %v", err) + } + + staleID, err := fixtures.CreateTestPayment(tx, bookingID, 2000.00, "online_square", "full", "pending") + if err != nil { + t.Fatalf("failed to create stale pending payment: %v", err) + } + const key = "key-expired-replay-pending-refund" + const dupPayID = "pay_expired_key_pending_refund" + if _, err := tx.Exec(ctx, "UPDATE payments SET created_at = NOW() - INTERVAL '23 hours', idempotency_key = $1, square_source_id = 'ccof:test-saved-card', created_by = $2 WHERE id = $3", key, userID, staleID); err != nil { + t.Fatalf("failed to age the stale payment: %v", err) + } + + origClient := SquareClient + mock := square.NewDevClient() + t.Setenv("SQUARE_ENVIRONMENT", "production") + SquareClient = &pendingRefundClient{SquareClient: &staleReplayClient{SquareClient: mock, result: &square.PaymentResult{ + Status: "COMPLETED", + ID: dupPayID, + SquarePayID: dupPayID, + CreatedAt: clock.Now().Format(time.RFC3339), + }}} + defer func() { SquareClient = origClient }() + + 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 test tx: %v", err) + } + + t.Cleanup(func() { + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM refunds WHERE payment_id = $1`, staleID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM admin_notifications WHERE reason = 'critical_payment_log' AND user_id = $1`, userID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM payments WHERE id = $1`, staleID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM bookings WHERE id = $1`, bookingID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM services WHERE id = $1`, serviceID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM users WHERE id = $1`, userID) + }) + + freshCtx := context.Background() + if _, err := SweepStalePendingPayments(freshCtx); err != nil { + t.Fatalf("sweep failed: %v", err) + } + + // PENDING refund → NON-terminal: the parent row must NOT be marked failed. + var status string + if err := db.Conn.QueryRow(freshCtx, "SELECT status FROM payments WHERE id = $1", staleID).Scan(&status); err != nil { + t.Fatalf("failed to query payment: %v", err) + } + if status != "pending" { + t.Errorf("expected a PENDING auto-refund to leave the row pending (never failed), got %q", status) + } + + // The in-flight refund must be recorded with status 'pending' + square_refund_id. + var refundStatus string + var sqRefundID *string + var reason string + if err := db.Conn.QueryRow(freshCtx, `SELECT status, square_refund_id, reason FROM refunds WHERE payment_id = $1`, staleID).Scan(&refundStatus, &sqRefundID, &reason); err != nil { + t.Fatalf("failed to query refunds row: %v", err) + } + if refundStatus != "pending" { + t.Errorf("expected the refunds row to be pending, got %q", refundStatus) + } + if sqRefundID == nil || *sqRefundID == "" { + t.Error("expected the refunds row to carry the Square refund id") + } + if reason != sweepDuplicateRefundReason { + t.Errorf("expected reason %q, got %q", sweepDuplicateRefundReason, reason) + } + + // Now the refund settles at Square: the re-poll pass resolves the parent. + SquareClient = &settledRefundClient{SquareClient: mock} + if _, err := SweepPendingSquareRefunds(freshCtx); err != nil { + t.Fatalf("refund re-poll sweep failed: %v", err) + } + if err := db.Conn.QueryRow(freshCtx, "SELECT status FROM payments WHERE id = $1", staleID).Scan(&status); err != nil { + t.Fatalf("failed to re-query payment: %v", err) + } + if status != "failed" { + t.Errorf("expected a COMPLETED refund to resolve the parent row to failed, got %q", status) + } + if err := db.Conn.QueryRow(freshCtx, `SELECT status FROM refunds WHERE payment_id = $1`, staleID).Scan(&refundStatus); err != nil { + t.Fatalf("failed to re-query refunds row: %v", err) + } + if refundStatus != "completed" { + t.Errorf("expected the refunds row to complete once Square settles, got %q", refundStatus) + } +} + +// TestSweepStalePendingPayments_KeyedTillReplayNewCharge_RefundPending_NoClawback +// locks the B1 PENDING fix for till sales: a PENDING auto-refund of a +// replay-induced duplicate charge must leave the till sale pending with its +// funded gift card intact (NO clawback, NO fail), and the refunds row must be +// recorded (attached to a synthetic payments row for the duplicate charge) +// with the parent sale encoded in the reason. Once Square settles the refund +// COMPLETED, the re-poll pass claws back the funding and marks the sale failed. +// Sequential (flips SQUARE_ENVIRONMENT), like the sibling B1 tests. +func TestSweepStalePendingPayments_KeyedTillReplayNewCharge_RefundPending_NoClawback(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) + } + pool := context.Background() + + saleID, giftCardID := seedStaleTillSaleWithCard(t, ctx, tx, adminID, 50.00, "", true) + // Age the sale AND its created gift card inside the key window (23h, + // created_at equality preserved → is_create stays true) and add the key. + const dupPayID = "pay_expired_key_till_pending_refund" + if _, err := tx.Exec(ctx, "UPDATE till_sales SET created_at = NOW() - INTERVAL '23 hours', idempotency_key = 'key-till-expired-pending-refund', square_source_id = 'ccof:test-saved-card' WHERE id = $1", saleID); err != nil { + t.Fatalf("failed to age the till sale: %v", err) + } + if _, err := tx.Exec(ctx, "UPDATE gift_cards SET created_at = NOW() - INTERVAL '23 hours' WHERE id = $1", giftCardID); err != nil { + t.Fatalf("failed to age the gift card: %v", err) + } + + origClient := SquareClient + mock := square.NewDevClient() + t.Setenv("SQUARE_ENVIRONMENT", "production") + SquareClient = &pendingRefundClient{SquareClient: &staleReplayClient{SquareClient: mock, result: &square.PaymentResult{ + Status: "COMPLETED", + ID: dupPayID, + SquarePayID: dupPayID, + CreatedAt: clock.Now().Format(time.RFC3339), + }}} + defer func() { SquareClient = origClient }() + + 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) + } + + if _, err := SweepStalePendingPayments(pool); err != nil { + t.Fatalf("sweep failed: %v", err) + } + + var status string + if err := db.Conn.QueryRow(pool, `SELECT status FROM till_sales WHERE id = $1`, saleID).Scan(&status); err != nil { + t.Fatalf("failed to query till sale: %v", err) + } + if status != "pending" { + t.Errorf("expected a PENDING auto-refund to leave the till sale pending (never failed, no clawback), got %q", status) + } + var cardCount int + if err := db.Conn.QueryRow(pool, `SELECT COUNT(*) FROM gift_cards WHERE id = $1`, giftCardID).Scan(&cardCount); err != nil { + t.Fatalf("failed to count gift cards: %v", err) + } + if cardCount != 1 { + t.Errorf("expected the funded gift card NOT clawed back while the refund is pending, got %d cards", cardCount) + } + + // The refund row exists with the parent till sale encoded in the reason. + var reason, refundStatus string + if err := db.Conn.QueryRow(pool, `SELECT reason, status FROM refunds WHERE reason = $1`, sweepDuplicateRefundReasonFor(saleID)).Scan(&reason, &refundStatus); err != nil { + t.Fatalf("failed to query B1 refund row: %v", err) + } + if refundStatus != "pending" { + t.Errorf("expected the B1 refunds row to be pending, got %q", refundStatus) + } + + // Refund settles COMPLETED → re-poll claws back the funding and fails the sale. + SquareClient = &settledRefundClient{SquareClient: mock} + if _, err := SweepPendingSquareRefunds(pool); err != nil { + t.Fatalf("refund re-poll sweep failed: %v", err) + } + if err := db.Conn.QueryRow(pool, `SELECT status FROM till_sales WHERE id = $1`, saleID).Scan(&status); err != nil { + t.Fatalf("failed to re-query till sale: %v", err) + } + if status != "failed" { + t.Errorf("expected a COMPLETED refund to resolve the till sale to failed, got %q", status) + } + if err := db.Conn.QueryRow(pool, `SELECT COUNT(*) FROM gift_cards WHERE id = $1`, giftCardID).Scan(&cardCount); err != nil { + t.Fatalf("failed to count gift cards: %v", err) + } + if cardCount != 0 { + t.Errorf("expected the created gift card clawed back after the refund settled COMPLETED, got %d cards", cardCount) + } +} + // TestSweepStalePendingPayments_KeyedReplayUnparseableCreatedAt_LeavesPending // locks the B1 caveat: a replayed COMPLETED payment whose created_at CANNOT be // parsed must NOT be auto-refunded. An unparseable created_at does not prove diff --git a/backend/handlers/payments/twofa.go b/backend/handlers/payments/twofa.go index 514e328..5db93ad 100644 --- a/backend/handlers/payments/twofa.go +++ b/backend/handlers/payments/twofa.go @@ -75,11 +75,14 @@ func (s *PaymentService) UserTwoFactorEnabled(ctx context.Context, userID string // stored pending 2FA code. It is a thin delegation shim over // twofa.VerifyForUser — the single source of truth for the verification core // (per-user brute-force lockout, constant-time compare, legacy pre-pepper -// hash fallback, code lifetime). It returns nil on a valid code, or a -// classified twofa.ErrIncorrect / twofa.ErrLockedOut / twofa.ErrMissingOrExpired -// (or a wrapped DB error) for the caller to map to the correct HTTP status. +// hash fallback, code lifetime). consume=true is passed so a verified code is +// SINGLE-USE: the gate NULLs the pending code on success, so one code +// authorizes exactly one saved-card charge (not unlimited charges for its +// 10-minute lifetime). It returns nil on a valid code, or a classified +// twofa.ErrIncorrect / twofa.ErrLockedOut / twofa.ErrMissingOrExpired (or a +// wrapped DB error) for the caller to map to the correct HTTP status. func verifyPendingTwoFactorCode(ctx context.Context, userID, code string) error { - return twofa.VerifyForUser(ctx, userID, code) + return twofa.VerifyForUser(ctx, userID, code, true) } // requireTwoFactorForCardAccess gates the saved-card online payment paths diff --git a/backend/handlers/payments/twofa_test.go b/backend/handlers/payments/twofa_test.go index d125772..86aa3e1 100644 --- a/backend/handlers/payments/twofa_test.go +++ b/backend/handlers/payments/twofa_test.go @@ -13,6 +13,7 @@ package payments import ( "bytes" "context" + "database/sql" "encoding/json" "net/http" "net/http/httptest" @@ -176,6 +177,37 @@ func TestRequireTwoFactorForCardAccess_Enforced(t *testing.T) { }) } +// TestRequireTwoFactorForCardAccess_CodeIsSingleUse pins the finding-1 fix: a +// code verified through the gate is CONSUMED (the pending code is NULLed), so +// the same code cannot authorize a second saved-card charge within its +// 10-minute lifetime. The second attempt with the same code is denied with the +// documented "expired — request a new one" 400. +func TestRequireTwoFactorForCardAccess_CodeIsSingleUse(t *testing.T) { + helperEnvEnforce2FA(t) + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + seedTwoFAPendingCode(t, tx, userID, "424242") + + req := httptest.NewRequest(http.MethodPost, "/", nil).WithContext(ctx) + w := httptest.NewRecorder() + require.True(t, requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID, "424242"), "first use of the code must pass the gate") + require.Equal(t, http.StatusOK, w.Code) + + // The verified code must now be consumed (NULLed) in the DB. + var pendingHash sql.NullString + require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_pending_code_hash FROM users WHERE id = $1", userID).Scan(&pendingHash)) + require.False(t, pendingHash.Valid, "a verified gate code must be consumed (NULLed)") + + // A second charge attempt with the same code must be denied as expired. + w = httptest.NewRecorder() + require.False(t, requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID, "424242"), "a consumed code must not pass the gate twice") + require.Equal(t, http.StatusBadRequest, w.Code) + var body map[string]string + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) + require.Equal(t, "Verification code expired — request a new one", body["error"]) +} + // TestTwoFactorEnforced_CreateBookingPayment_SaveCard_Blocked verifies the // end-to-end gate on the save-card path: enforced + user without 2FA → 403 with // no payment row and no saved card (Square never called). diff --git a/backend/handlers/user/profile_test.go b/backend/handlers/user/profile_test.go index 9f97380..175e824 100644 --- a/backend/handlers/user/profile_test.go +++ b/backend/handlers/user/profile_test.go @@ -213,7 +213,7 @@ func TestPasswordChange_RevokesTokens(t *testing.T) { } // The consumed refresh token no longer verifies. - if _, _, err := auth.VerifyRefreshToken(ctx, refreshToken); err == nil { + if _, _, _, err := auth.VerifyRefreshToken(ctx, refreshToken); err == nil { t.Error("refresh token must be invalid after a password change (B9)") } } diff --git a/backend/handlers/user/twofa.go b/backend/handlers/user/twofa.go index a44241d..177c26d 100644 --- a/backend/handlers/user/twofa.go +++ b/backend/handlers/user/twofa.go @@ -322,9 +322,11 @@ const ( // DisableTwoFAHandler and VerifyTwoFACodeForUser. The caller must hold st.Mu // (from twoFAAttemptStateFor) so concurrent attempts from the same user cannot // race the limit check. Delegates to the shared implementation in -// crussell/internal/twofa. +// crussell/internal/twofa with consume=false: the interactive setup/disable +// flows clear the pending code themselves on success (enableTwoFA / +// disableTwoFA), so the code must stay valid through the whole handshake here. func checkTwoFACode(r *http.Request, userID string, st *twoFAAttemptState, reqCode string) (twoFACodeCheckResult, error) { - res, err := twofa.Check(r.Context(), userID, st, reqCode) + res, err := twofa.Check(r.Context(), userID, st, reqCode, false) return twoFACodeCheckResult(res), err } @@ -345,7 +347,7 @@ func VerifyTwoFACodeForUser(ctx context.Context, userID, code string) error { st.Mu.Lock() defer st.Mu.Unlock() - result, err := twofa.Check(ctx, userID, st, code) + result, err := twofa.Check(ctx, userID, st, code, false) if err != nil { return err } @@ -506,7 +508,7 @@ func SendDisableCodeHandler(w http.ResponseWriter, r *http.Request) { st.Mu.Lock() defer st.Mu.Unlock() - if err := ensurePendingTwoFACode(r, userID, st); err != nil { + if _, err := ensurePendingTwoFACode(r, userID, st, "disable 2FA"); err != nil { if errors.Is(err, errTwoFAMintThrottled) { http.Error(w, "Too many attempts. Wait before requesting a new code.", http.StatusTooManyRequests) return @@ -525,6 +527,84 @@ func SendDisableCodeHandler(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) } +// POST /api/user/2fa/code +// Lets an ENABLED user request a fresh verification code for a saved-card +// charge (the B6/B10 gate). This closes the enforced-deployment dead-end where +// 2FA setup clears the pending code and SetupTwoFAHandler refuses already +// enabled users (409): without it there is no way to mint a code for a +// saved-card charge, so every charge returned 400 "Verification code expired — +// request a new one" with no way to get a new one. +// +// The mint machinery is shared with the disable flow: ensurePendingTwoFACode +// reuses a still-valid pending code when one exists and otherwise mints + +// delivers a fresh one via the same build-dependent channel as setup +// (deliverTwoFACode — [2FA] log in dev/test; pepper- and delivery-channel +// gated in production). Fresh-code mints are throttled per-user +// (twoFAMintCooldown) and never reset the failed-attempt counter (B11b). +// +// Contract: 200 {"message":"Code sent"} (+ a dev-only "code" field when 2FA is +// unenforced, matching setup); 409 when the user has not enabled 2FA; 429 on +// the mint cooldown; 503 when no delivery channel is configured (production +// without TWO_FACTOR_ALLOW_LOG_DELIVERY=true); 500 on DB failure. The route is +// mounted with RequireAuth + RequireNonGuest + the shared per-user 2FA limiter +// (plus the group's per-IP limiter), so an enabled user cannot hammer code +// requests faster than the surface budget. +func SendVerificationCodeHandler(w http.ResponseWriter, r *http.Request) { + userID, ok := mw.GetUserID(r.Context()) + if !ok { + http.Error(w, "unauthorized", http.StatusUnauthorized) + return + } + + var enabled bool + err := db.Conn.QueryRow(r.Context(), `SELECT two_factor_enabled FROM users WHERE id = $1`, userID).Scan(&enabled) + if err != nil { + log.Printf("failed to check 2FA state for user %s: %v", userID, err) + http.Error(w, "server error", http.StatusInternalServerError) + return + } + if !enabled { + http.Error(w, "Two-factor authentication is not enabled", http.StatusConflict) + return + } + + // The per-user mutex serializes the mint with the charge gate's verify + // critical section so concurrent requests from the same user cannot race + // the cooldown or lockout counters. + st := twoFAAttemptStateFor(userID) + st.Mu.Lock() + defer st.Mu.Unlock() + + code, err := ensurePendingTwoFACode(r, userID, st, "saved-card charge") + if err != nil { + if errors.Is(err, errTwoFAMintThrottled) { + http.Error(w, "Too many attempts. Wait before requesting a new code.", http.StatusTooManyRequests) + return + } + if errors.Is(err, errTwoFADeliveryUnavailable) { + // Production with no delivery channel: no fresh code can be minted, + // so the saved-card charge cannot be re-challenged. Surface the + // actionable setup error instead of a silent 500. + http.Error(w, err.Error(), http.StatusServiceUnavailable) + return + } + log.Printf("failed to prepare 2FA code for saved-card charge for user %s: %v", userID, err) + http.Error(w, "server error", http.StatusInternalServerError) + return + } + + resp := map[string]any{"message": "Code sent"} + if !twoFARequired() && code != "" { + // Dev convenience (matches setup): return the freshly minted code so + // the request path is testable without grepping the backend log. The + // code is never included when 2FA is enforced. + resp["code"] = code + } + if err := json.NewEncoder(w).Encode(resp); err != nil { + log.Printf("failed to encode 2FA code response: %v", err) + } +} + // POST /api/user/2fa/disable // Turns 2FA off and clears method + pending fields for the authenticated user. // @@ -578,7 +658,7 @@ func DisableTwoFAHandler(w http.ResponseWriter, r *http.Request) { // The per-user mint cooldown still bounds how often a fresh code can be // minted — at most one per twoFAMintCooldown — but it cannot grant a fresh // guessing budget. - if err := ensurePendingTwoFACode(r, userID, st); err != nil { + if _, err := ensurePendingTwoFACode(r, userID, st, "disable 2FA"); err != nil { if errors.Is(err, errTwoFAMintThrottled) { http.Error(w, "Too many attempts. Wait before requesting a new code.", http.StatusTooManyRequests) return @@ -625,8 +705,16 @@ func DisableTwoFAHandler(w http.ResponseWriter, r *http.Request) { // ensurePendingTwoFACode guarantees the user has a valid (unexpired) pending // code to verify against, generating + delivering a fresh one via the same // build-dependent delivery channel as setup (see deliverTwoFACode) when the -// stored code is missing or expired. The caller must hold the user's -// attempt-state mutex. +// stored code is missing or expired. purpose labels the delivery for the [2FA] +// log line (e.g. "disable 2FA", "saved-card charge"). The caller must hold the +// user's attempt-state mutex. +// +// It returns the plaintext code only when a FRESH code was minted and +// delivered (dev/test builds always deliver it; production builds only when +// the operator opted into log delivery — see twofa_prod.go). When a valid +// pending code was reused, the return is empty: only the digest is stored, so +// the plaintext is unavailable. Callers must only expose the returned code in +// unenforced environments (matching SetupTwoFAHandler's dev convenience). // // Minting a fresh code does NOT reset the failed-attempt counter (B11b): the // counter resets only on a successful verify or when the 10-minute attempt @@ -638,7 +726,7 @@ func DisableTwoFAHandler(w http.ResponseWriter, r *http.Request) { // who exhausts the budget must wait out the window, not the mint cooldown. A // failed delivery does not start the cooldown (the stamp is written only after // the UPDATE persisted). -func ensurePendingTwoFACode(r *http.Request, userID string, st *twoFAAttemptState) error { +func ensurePendingTwoFACode(r *http.Request, userID string, st *twoFAAttemptState, purpose string) (string, error) { var pendingHash sql.NullString var pendingExpires sql.NullTime err := db.Conn.QueryRow(r.Context(), ` @@ -647,20 +735,21 @@ func ensurePendingTwoFACode(r *http.Request, userID string, st *twoFAAttemptStat WHERE id = $1 `, userID).Scan(&pendingHash, &pendingExpires) if err != nil { - return err + return "", err } if pendingHash.Valid && pendingExpires.Valid && pendingExpires.Time.After(clock.Now()) { - return nil + return "", nil } now := clock.Now() if !st.LastMintAt.IsZero() && now.Sub(st.LastMintAt) < twoFAMintCooldown { - return errTwoFAMintThrottled + return "", errTwoFAMintThrottled } - if _, err := deliverTwoFACode(r, userID, "", "disable 2FA"); err != nil { - return err + code, err := deliverTwoFACode(r, userID, "", purpose) + if err != nil { + return "", err } st.LastMintAt = now - return nil + return code, nil } // disableTwoFA clears two_factor_enabled and the method + pending code fields. diff --git a/backend/handlers/user/twofa_test.go b/backend/handlers/user/twofa_test.go index 9fa1342..47fa238 100644 --- a/backend/handlers/user/twofa_test.go +++ b/backend/handlers/user/twofa_test.go @@ -1251,6 +1251,140 @@ func TestTwoFADisableCode_UnenforcedStillMints(t *testing.T) { require.True(t, pendingHash.Valid, "unenforced env must still mint a pending code") } +// TestTwoFASendVerificationCode_Enabled_MintsFresh verifies POST +// /api/user/2fa/code: an ENABLED user with no pending code gets a fresh code +// minted + delivered ([2FA] log labelled "saved-card charge"), with only the +// hash + a future expiry persisted and no code in the enforced response. +func TestTwoFASendVerificationCode_Enabled_MintsFresh(t *testing.T) { + twofaEnvEnforced(t) + var buf bytes.Buffer + log.SetOutput(&buf) + t.Cleanup(func() { log.SetOutput(os.Stderr) }) + + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + _, err = tx.Exec(ctx, `UPDATE users SET two_factor_enabled = true, two_factor_method = 'email' WHERE id = $1`, userID) + require.NoError(t, err) + + w := performUser2FARequest(t, SendVerificationCodeHandler, ctx, http.MethodPost, "/api/user/2fa/code", nil, userID) + require.Equal(t, http.StatusOK, w.Code, w.Body.String()) + + var resp map[string]any + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &resp)) + require.Equal(t, "Code sent", resp["message"]) + _, hasCode := resp["code"] + require.False(t, hasCode, "enforced env must NOT return the code in the response") + + var pendingHash sql.NullString + var expires sql.NullTime + require.NoError(t, tx.QueryRow(ctx, ` + SELECT two_factor_pending_code_hash, two_factor_pending_code_expires + FROM users WHERE id = $1`, userID).Scan(&pendingHash, &expires)) + require.True(t, pendingHash.Valid, "endpoint must mint a pending code hash") + require.True(t, expires.Valid && expires.Time.After(clock.Now()), "minted code must have a future expiry") + require.Contains(t, buf.String(), "saved-card charge", "delivery log must label the charge purpose") + require.Regexp(t, regexp.MustCompile(`\[2FA\].*\d{6}`), buf.String(), "endpoint must log the code as the delivery channel") +} + +// TestTwoFASendVerificationCode_ReusesValidPendingCode verifies that a valid +// unexpired pending code is reused (the stored hash is unchanged) instead of a +// fresh mint, so a mid-flow charge retry is not throttled. +func TestTwoFASendVerificationCode_ReusesValidPendingCode(t *testing.T) { + twofaEnvEnforced(t) + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + _, err = tx.Exec(ctx, `UPDATE users SET two_factor_enabled = true, two_factor_method = 'email' WHERE id = $1`, userID) + require.NoError(t, err) + seedPendingTwoFA(t, ctx, tx, userID, "123456") + + w := performUser2FARequest(t, SendVerificationCodeHandler, ctx, http.MethodPost, "/api/user/2fa/code", nil, userID) + require.Equal(t, http.StatusOK, w.Code, w.Body.String()) + + var pendingHash sql.NullString + require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_pending_code_hash FROM users WHERE id = $1", userID).Scan(&pendingHash)) + require.True(t, pendingHash.Valid) + require.Equal(t, hashTwoFACode("123456"), pendingHash.String, "existing valid pending code must be reused, not re-minted") +} + +// TestTwoFASendVerificationCode_NotEnabled_409 verifies that a user who has NOT +// enabled 2FA is refused with 409 (the endpoint exists only to re-challenge an +// enabled user's saved-card charge; setup covers the not-enabled path). +func TestTwoFASendVerificationCode_NotEnabled_409(t *testing.T) { + twofaEnvEnforced(t) + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + + w := performUser2FARequest(t, SendVerificationCodeHandler, ctx, http.MethodPost, "/api/user/2fa/code", nil, userID) + require.Equal(t, http.StatusConflict, w.Code, w.Body.String()) +} + +// TestTwoFASendVerificationCode_MintThrottled verifies the per-user mint +// cooldown applies: a second code request inside twoFAMintCooldown returns 429. +// The pending code is dropped first (as a lockout does) because a still-valid +// code is reused by ensurePendingTwoFACode, which short-circuits the cooldown. +func TestTwoFASendVerificationCode_MintThrottled(t *testing.T) { + twofaEnvEnforced(t) + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + _, err = tx.Exec(ctx, `UPDATE users SET two_factor_enabled = true, two_factor_method = 'email' WHERE id = $1`, userID) + require.NoError(t, err) + + w := performUser2FARequest(t, SendVerificationCodeHandler, ctx, http.MethodPost, "/api/user/2fa/code", nil, userID) + require.Equal(t, http.StatusOK, w.Code, w.Body.String()) + + // Drop the pending code so the next request cannot reuse it and must hit + // the cooldown check instead. + _, err = tx.Exec(ctx, `UPDATE users + SET two_factor_pending_code_hash = NULL, two_factor_pending_code_expires = NULL + WHERE id = $1`, userID) + require.NoError(t, err) + + w = performUser2FARequest(t, SendVerificationCodeHandler, ctx, http.MethodPost, "/api/user/2fa/code", nil, userID) + require.Equal(t, http.StatusTooManyRequests, w.Code, w.Body.String()) + require.Contains(t, w.Body.String(), "Too many attempts. Wait before requesting a new code.") +} + +// TestTwoFASendVerificationCode_Unenforced_ReturnsCode verifies the dev +// convenience: in an unenforced env the endpoint mints and returns the code in +// the response (matching setup), and the DB holds the digest of exactly it. +func TestTwoFASendVerificationCode_Unenforced_ReturnsCode(t *testing.T) { + twofaEnvUnenforced(t) + t.Setenv("TWO_FACTOR_PEPPER", "") + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + _, err = tx.Exec(ctx, `UPDATE users SET two_factor_enabled = true, two_factor_method = 'email' WHERE id = $1`, userID) + require.NoError(t, err) + + w := performUser2FARequest(t, SendVerificationCodeHandler, ctx, http.MethodPost, "/api/user/2fa/code", nil, userID) + require.Equal(t, http.StatusOK, w.Code, w.Body.String()) + + var resp struct { + Message string `json:"message"` + Code string `json:"code"` + } + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &resp)) + require.Equal(t, "Code sent", resp.Message) + require.Len(t, resp.Code, 6, "unenforced env must return the dev-convenience code") + + var pendingHash sql.NullString + require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_pending_code_hash FROM users WHERE id = $1", userID).Scan(&pendingHash)) + require.True(t, pendingHash.Valid) + sum := sha256.Sum256([]byte(resp.Code)) + require.Equal(t, hex.EncodeToString(sum[:]), pendingHash.String, "stored hash must be the SHA-256 of the returned code") +} + +// TestTwoFASendVerificationCode_Unauthorized verifies that an unauthenticated +// request is rejected with 401 before any minting happens. +func TestTwoFASendVerificationCode_Unauthorized(t *testing.T) { + w := performUser2FARequest(t, SendVerificationCodeHandler, context.Background(), http.MethodPost, "/api/user/2fa/code", nil, "") + require.Equal(t, http.StatusUnauthorized, w.Code) +} + // TestTwoFACodeVerifyForUser exercises the exported helper that backs the // B6/B10 payments gate (the saved-card charge must present a real 2FA // challenge): correct code → nil, wrong code → twofa.ErrIncorrect, exhausting diff --git a/backend/internal/twofa/twofa.go b/backend/internal/twofa/twofa.go index 72f5731..5c4d305 100644 --- a/backend/internal/twofa/twofa.go +++ b/backend/internal/twofa/twofa.go @@ -11,7 +11,7 @@ // // Contract for the payments gate: // -// err := twofa.VerifyForUser(ctx, userID, code) +// err := twofa.VerifyForUser(ctx, userID, code, true) // consume = true // if err != nil { // switch { // case errors.Is(err, twofa.ErrIncorrect): @@ -25,6 +25,14 @@ // } // } // +// A correct code is SINGLE-USE on the payments gate: the gate passes +// consume=true, so the stored pending-code digest and its expiry are NULLed in +// the same critical section as the successful check. One code therefore +// authorizes exactly one saved-card charge, never unlimited charges for its +// 10-minute lifetime. The interactive setup/disable flows pass consume=false — +// they clear the pending fields themselves on success (enableTwoFA / +// disableTwoFA), so the code must stay valid through their whole handshake. +// // The failed-attempt counter is keyed per user and resets ONLY on a successful // verify (or after the 10-minute attempt window elapses) — never on a fresh // code mint, so minting a new code cannot grant a fresh guessing budget (B11b). @@ -259,10 +267,14 @@ const ( // check. A correct code resets the attempt counter and returns OK. An incorrect // code increments the counter and, on the 5th consecutive failure, invalidates // the pending code (lockout). A missing or expired pending code returns -// MissingOrExpired. The returned error is non-nil only for DB failures +// MissingOrExpired. consume makes a correct code single-use: the stored digest +// and its expiry are NULLed immediately, so one code cannot authorize a second +// operation within its lifetime (the payments saved-card gate passes true; the +// interactive setup/disable flows pass false and clear the pending fields +// themselves on success). The returned error is non-nil only for DB failures // (callers return 500); a lockout's pending-code invalidation failure is logged // here and still reported as a lockout. -func Check(ctx context.Context, userID string, st *AttemptState, reqCode string) (Result, error) { +func Check(ctx context.Context, userID string, st *AttemptState, reqCode string, consume bool) (Result, error) { if now := clock.Now(); now.Sub(st.LastActive()) > AttemptWindow { st.Count.Store(0) st.SetLastActive(now) @@ -309,8 +321,11 @@ func Check(ctx context.Context, userID string, st *AttemptState, reqCode string) } // Success: a legacy (pre-pepper) hash that verified is re-hashed with the - // pepper so the plain digest is retired on the next successful verify. - if legacy { + // pepper so the plain digest is retired on the next successful verify. This + // matters only for the interactive paths (consume=false), where the pending + // code stays valid for the rest of the handshake — consume mode destroys + // the digest outright, so there is nothing to upgrade. + if legacy && !consume { if _, err := db.Conn.Exec(ctx, ` UPDATE users SET two_factor_pending_code_hash = $2 @@ -325,6 +340,25 @@ func Check(ctx context.Context, userID string, st *AttemptState, reqCode string) st.SetLastActive(clock.Now()) st.LastMintAt = time.Time{} ResetAttempts(userID) + if consume { + // Consume mode (the payments saved-card gate, B6/B10): a verified code + // is single-use. NULL the stored digest and its expiry so the same code + // cannot authorize a second saved-card charge within its 10-minute + // lifetime. The interactive setup/disable flows pass consume=false: + // they clear the pending fields themselves on success (enableTwoFA / + // disableTwoFA), so the code must stay valid through the whole + // verification handshake here. The write goes through the same + // context-routed connection as the rest of Check, so verification and + // consumption are one unit. + if _, err := db.Conn.Exec(ctx, ` + UPDATE users + SET two_factor_pending_code_hash = NULL, + two_factor_pending_code_expires = NULL + WHERE id = $1 + `, userID); err != nil { + log.Printf("failed to consume 2FA pending code for user %s: %v", userID, err) + } + } return OK, nil } @@ -345,13 +379,17 @@ var ( // It returns nil on a correct code, or one of ErrIncorrect / ErrLockedOut / // ErrMissingOrExpired (or a DB error, wrapped). This is the entry point for // the payments card-access gate (B6/B10): a saved-card charge must present a -// real, freshly-verified challenge. -func VerifyForUser(ctx context.Context, userID, code string) error { +// real, freshly-verified challenge. consume makes a correct code single-use: +// the pending-code digest and its expiry are NULLed in the same critical +// section as the successful check (see Check), so one code authorizes exactly +// one gate pass. The interactive setup/disable flows pass false — they clear +// the pending fields themselves on success (enableTwoFA / disableTwoFA). +func VerifyForUser(ctx context.Context, userID, code string, consume bool) error { st := StateFor(userID) st.Mu.Lock() defer st.Mu.Unlock() - result, err := Check(ctx, userID, st, code) + result, err := Check(ctx, userID, st, code, consume) if err != nil { return fmt.Errorf("2FA verify: %w", err) } diff --git a/backend/internal/twofa/twofa_test.go b/backend/internal/twofa/twofa_test.go index 20515d1..47098f8 100644 --- a/backend/internal/twofa/twofa_test.go +++ b/backend/internal/twofa/twofa_test.go @@ -10,6 +10,7 @@ package twofa import ( "context" + "database/sql" "testing" "time" @@ -33,12 +34,26 @@ func seedPending(t *testing.T, ctx context.Context, tx db.Querier, userID, code func TestVerifyForUser_CorrectAndWrongCode(t *testing.T) { ctx, tx := testutils.SetupTestTx(t) + + // consume=false (interactive setup/disable path): a success keeps the + // pending code valid, so a wrong follow-up code reports ErrIncorrect. userID, err := fixtures.CreateTestUser(tx) require.NoError(t, err) seedPending(t, ctx, tx, userID, "123456") + require.NoError(t, VerifyForUser(ctx, userID, "123456", false), "correct code must verify") + require.ErrorIs(t, VerifyForUser(ctx, userID, "999999", false), ErrIncorrect) - require.NoError(t, VerifyForUser(ctx, userID, "123456"), "correct code must verify") - require.ErrorIs(t, VerifyForUser(ctx, userID, "999999"), ErrIncorrect) + // consume=true (payments saved-card gate path): a success DESTROYS the + // pending code, so re-verifying the same code reports ErrMissingOrExpired + // — a verified code is single-use and cannot authorize a second charge. + userID2, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + seedPending(t, ctx, tx, userID2, "123456") + require.NoError(t, VerifyForUser(ctx, userID2, "123456", true), "correct code must verify") + require.ErrorIs(t, VerifyForUser(ctx, userID2, "123456", true), ErrMissingOrExpired, "a consumed code must be single-use") + var pendingHash sql.NullString + require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_pending_code_hash FROM users WHERE id = $1", userID2).Scan(&pendingHash)) + require.False(t, pendingHash.Valid, "a consumed code must be NULLed in the DB") } func TestVerifyForUser_LockoutAndMissing(t *testing.T) { @@ -48,16 +63,16 @@ func TestVerifyForUser_LockoutAndMissing(t *testing.T) { seedPending(t, ctx, tx, userID, "123456") // Wrong code #1 → ErrIncorrect; four more reach the 5-attempt cap. - require.ErrorIs(t, VerifyForUser(ctx, userID, "999999"), ErrIncorrect) + require.ErrorIs(t, VerifyForUser(ctx, userID, "999999", true), ErrIncorrect) for i := 0; i < 4; i++ { - _ = VerifyForUser(ctx, userID, "999999") + _ = VerifyForUser(ctx, userID, "999999", true) } - require.ErrorIs(t, VerifyForUser(ctx, userID, "999999"), ErrLockedOut) + require.ErrorIs(t, VerifyForUser(ctx, userID, "999999", true), ErrLockedOut) // A fresh user with no pending code → ErrMissingOrExpired. userID2, err := fixtures.CreateTestUser(tx) require.NoError(t, err) - require.ErrorIs(t, VerifyForUser(ctx, userID2, "123456"), ErrMissingOrExpired) + require.ErrorIs(t, VerifyForUser(ctx, userID2, "123456", true), ErrMissingOrExpired) } // TestVerifyForUser_AttemptStateMapPersists exercises the shared per-user diff --git a/backend/main.go b/backend/main.go index ae36e7a..17d643e 100644 --- a/backend/main.go +++ b/backend/main.go @@ -624,22 +624,29 @@ func main() { // able to reach these, not just verified accounts. r.With(mw.RequireNonGuest).Get("/user/2fa/status", user.GetTwoFAStatusHandler) // The code-issuing/verifying endpoints get a dedicated per-user - // limiter (10/min) on top of the group's generic 120/min limiter: + // limiter (10/min) on top of the group's generic 120/min per-IP + // limiter: // the 6-digit codes live in a 1M space, so a single user must not be - // able to hammer setup/verify/disable faster than the per-user - // 5-attempt lockout can trip. The key is the authenticated userID - // ALONE (RateLimitByUser) — NOT user+IP (B8): with the IP in the - // key, a client that can rotate its source IP (or that sits behind + // able to hammer setup/verify/disable/code-mint faster than the + // per-user 5-attempt lockout can trip. The key is the authenticated + // userID ALONE (RateLimitByUser) — NOT user+IP (B8): with the IP in + // the key, a client that can rotate its source IP (or that sits behind // a proxy echoing a client-supplied CF-Connecting-IP when // TRUST_PROXY_HEADERS=true) mints a fresh bucket per IP for the // same account, collapsing the per-account budget. One shared - // limiter for all four so the whole 2FA surface counts against a + // limiter for all five so the whole 2FA surface counts against a // single per-user budget. twoFALimiter := mw.RateLimitByUser(10, time.Minute) r.With(mw.RequireNonGuest, twoFALimiter).Post("/user/2fa/setup", user.SetupTwoFAHandler) r.With(mw.RequireNonGuest, twoFALimiter).Post("/user/2fa/verify", user.VerifyTwoFAHandler) r.With(mw.RequireNonGuest, twoFALimiter).Post("/user/2fa/disable", user.DisableTwoFAHandler) r.With(mw.RequireNonGuest, twoFALimiter).Post("/user/2fa/disable/code", user.SendDisableCodeHandler) + // Fresh-code request for an already-ENABLED user making a saved-card + // charge (the B6/B10 gate). Setup refuses enabled users (409) and + // setup clears the pending code on success, so this is the only mint + // path for an enabled user. Same middleware chain + shared limiter as + // the rest of the 2FA surface. + r.With(mw.RequireNonGuest, twoFALimiter).Post("/user/2fa/code", user.SendVerificationCodeHandler) r.Delete("/user/account", user.DeleteAccountHandler) r.Get("/user/gdpr-export", user.GetGDPRExportHandler) r.Get("/user/loyalty", user.GetLoyaltyHandler) diff --git a/frontend/src/lib/components/payments/PaymentModal.svelte b/frontend/src/lib/components/payments/PaymentModal.svelte index 0833f3b..b1aec5e 100644 --- a/frontend/src/lib/components/payments/PaymentModal.svelte +++ b/frontend/src/lib/components/payments/PaymentModal.svelte @@ -1,5 +1,5 @@