diff --git a/backend/handlers/payments/charge_helpers.go b/backend/handlers/payments/charge_helpers.go index bb6012b..0b12284 100644 --- a/backend/handlers/payments/charge_helpers.go +++ b/backend/handlers/payments/charge_helpers.go @@ -302,6 +302,18 @@ func postChargeRecheck(ctx context.Context, w http.ResponseWriter, tx pgx.Tx, bo log.Printf("CRITICAL: Square payment %s (ID=%s) landed on %q booking %s and committing the failed mark errored: %v — manual reconciliation required", sqStatus, sqPayID, recheckStatus, bookingID, cErr) } + // The CRITICAL log line alone was the only operator signal for a + // stranded charge — no admin-visible trace. Raise the flood-capped + // critical-payment admin notification (sweep.go's + // insertCriticalPaymentNotification, which applies the shared + // unacknowledged-queue cap atomically and dedups per + // booking/user). It runs AFTER the commit: this transaction held the + // booking row FOR UPDATE (recheckBookingPayable), and the notification + // INSERT's FK check on bookings(id) takes FOR KEY SHARE — inserting + // before the commit would self-deadlock against this tx's own booking + // lock (the sweep's gateStalePaymentRescueOnBooking commits first for + // the same reason). + insertCriticalPaymentNotification(ctx, &bookingID, nil) http.Error(w, conflictMsg, http.StatusConflict) return false, nil } diff --git a/backend/handlers/payments/completion.go b/backend/handlers/payments/completion.go index 3990c96..ba602aa 100644 --- a/backend/handlers/payments/completion.go +++ b/backend/handlers/payments/completion.go @@ -157,24 +157,42 @@ func ApplyBookingCompletionSideEffects(ctx context.Context, tx pgx.Tx, bookingID if capped, ok := capDiscountToRemainingObligation(ctx, tx, bookingID, discountAmount); ok { discountAmount = capped - if _, err := tx.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, 'time_based', NULL, $4, $5, $6) - `, bookingID, userID, campaignID, campaignPercent, bookingTotal, discountAmount); err != nil { - log.Printf("ALERT: failed to insert booking discount: %v", err) - } + // B13 atomic reservation FIRST — mirror the apply-at-payment + // path (discounts.go ApplyEligibleDiscount). The eligibility + // SELECT above is a plain read; a concurrent completion on + // another booking can exhaust the campaign between that read + // and here. The conditional UPDATE only increments while the + // campaign still has headroom (PostgreSQL re-evaluates the + // WHERE against the post-lock row), so exactly one concurrent + // completion wins the redemption. On pgx.ErrNoRows the + // discount is SKIPPED — the completion still succeeds, we just + // log and move on rather than minting a discount row for a + // redemption that never happened. + var reservedID string + if err := tx.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 + `, campaignID).Scan(&reservedID); err != nil { + if errors.Is(err, pgx.ErrNoRows) { + log.Printf("Skipping time_based campaign %s at completion for booking %s — campaign fully redeemed by a concurrent redemption", campaignID, bookingID) + } else { + log.Printf("ALERT: failed to reserve redemption for campaign %s at completion for booking %s: %v", campaignID, bookingID, err) + } + } else { + if _, err := tx.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, 'time_based', NULL, $4, $5, $6) + `, bookingID, userID, campaignID, campaignPercent, bookingTotal, discountAmount); err != nil { + log.Printf("ALERT: failed to insert booking discount: %v", err) + } - if _, err := tx.Exec(ctx, ` - INSERT INTO payments (booking_id, payment_type, payment_method, amount, status, created_by) - VALUES ($1, 'partial', 'discount', $2, 'completed', $3) - `, bookingID, discountAmount, userID); err != nil { - log.Printf("ALERT: failed to insert payment record: %v", err) - } - - if _, err := tx.Exec(ctx, ` - UPDATE discount_campaigns SET times_redeemed = times_redeemed + 1 WHERE id = $1 - `, campaignID); err != nil { - log.Printf("ALERT: failed to update discount campaign usage: %v", err) + if _, err := tx.Exec(ctx, ` + INSERT INTO payments (booking_id, payment_type, payment_method, amount, status, created_by) + VALUES ($1, 'partial', 'discount', $2, 'completed', $3) + `, bookingID, discountAmount, userID); err != nil { + log.Printf("ALERT: failed to insert payment record: %v", err) + } } } else { log.Printf("Skipping time_based campaign %s at completion for booking %s — obligation already covered by real money (would over-credit)", campaignID, bookingID) @@ -204,22 +222,43 @@ func ApplyBookingCompletionSideEffects(ctx context.Context, tx pgx.Tx, bookingID // F1 over-credit guard — see the time-based block above. if capped, ok := capDiscountToRemainingObligation(ctx, tx, bookingID, discountAmount); ok { discountAmount = capped - if _, err := tx.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, 'milestone', 'per_user_booking_count', $4, $5, $6) - `, bookingID, userID, milestoneCampaignID, milestonePercent, bookingTotal, discountAmount); err != nil { - log.Printf("ALERT: failed to insert booking discount: %v", err) - } - if _, err := tx.Exec(ctx, ` - INSERT INTO payments (booking_id, payment_type, payment_method, amount, status, created_by) - VALUES ($1, 'partial', 'discount', $2, 'completed', $3) - `, bookingID, discountAmount, userID); err != nil { - log.Printf("ALERT: failed to insert payment record: %v", err) - } - if _, err := tx.Exec(ctx, ` - UPDATE discount_campaigns SET times_redeemed = times_redeemed + 1 WHERE id = $1 - `, milestoneCampaignID); err != nil { - log.Printf("ALERT: failed to insert payment record: %v", err) + // B13 atomic reservation FIRST — see the time-based block above. + var reservedID string + if err := tx.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 + `, milestoneCampaignID).Scan(&reservedID); err != nil { + if errors.Is(err, pgx.ErrNoRows) { + log.Printf("Skipping per-user milestone campaign %s at completion for booking %s — campaign fully redeemed by a concurrent redemption", milestoneCampaignID, bookingID) + } else { + log.Printf("ALERT: failed to reserve redemption for campaign %s at completion for booking %s: %v", milestoneCampaignID, bookingID, err) + } + } else { + // Once-per-user backstop: the eligibility NOT EXISTS above + // is a plain read, so two concurrent completions of + // DIFFERENT bookings of this user can both pass it. The + // partial unique index uq_booking_discounts_user_milestone_campaign + // on (user_id, source_id) for milestone campaigns is the + // schema backstop — the second INSERT is suppressed by + // ON CONFLICT DO NOTHING and the discount is skipped. + tag, err := tx.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, 'milestone', 'per_user_booking_count', $4, $5, $6) + ON CONFLICT (user_id, source_id) WHERE discount_source = 'campaign' AND milestone_type IN ('per_user_booking_count', 'anniversary') DO NOTHING + `, bookingID, userID, milestoneCampaignID, milestonePercent, bookingTotal, discountAmount) + if err != nil { + log.Printf("ALERT: failed to insert booking discount: %v", err) + } else if tag.RowsAffected() == 0 { + log.Printf("Per-user milestone campaign %s already applied for user %s — skipping duplicate at completion for booking %s", milestoneCampaignID, userID, bookingID) + } else { + if _, err := tx.Exec(ctx, ` + INSERT INTO payments (booking_id, payment_type, payment_method, amount, status, created_by) + VALUES ($1, 'partial', 'discount', $2, 'completed', $3) + `, bookingID, discountAmount, userID); err != nil { + log.Printf("ALERT: failed to insert payment record: %v", err) + } + } } } else { log.Printf("Skipping per-user milestone campaign %s at completion for booking %s — obligation already covered by real money (would over-credit)", milestoneCampaignID, bookingID) @@ -260,22 +299,31 @@ func ApplyBookingCompletionSideEffects(ctx context.Context, tx pgx.Tx, bookingID // F1 over-credit guard — see the time-based block above. if capped, ok := capDiscountToRemainingObligation(ctx, tx, bookingID, discountAmount); ok { discountAmount = capped - if _, err := tx.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, 'milestone', 'global_booking_count', $4, $5, $6) - `, bookingID, userID, globalCampaignID, globalPercent, bookingTotal, discountAmount); err != nil { - log.Printf("ALERT: failed to insert booking discount: %v", err) - } - if _, err := tx.Exec(ctx, ` - INSERT INTO payments (booking_id, payment_type, payment_method, amount, status, created_by) - VALUES ($1, 'partial', 'discount', $2, 'completed', $3) - `, bookingID, discountAmount, userID); err != nil { - log.Printf("ALERT: failed to insert payment record: %v", err) - } - if _, err := tx.Exec(ctx, ` - UPDATE discount_campaigns SET times_redeemed = times_redeemed + 1 WHERE id = $1 - `, globalCampaignID); err != nil { - log.Printf("ALERT: failed to insert payment record: %v", err) + // B13 atomic reservation FIRST — see the time-based block above. + var reservedID string + if err := tx.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 + `, globalCampaignID).Scan(&reservedID); err != nil { + if errors.Is(err, pgx.ErrNoRows) { + log.Printf("Skipping global milestone campaign %s at completion for booking %s — campaign fully redeemed by a concurrent redemption", globalCampaignID, bookingID) + } else { + log.Printf("ALERT: failed to reserve redemption for campaign %s at completion for booking %s: %v", globalCampaignID, bookingID, err) + } + } else { + if _, err := tx.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, 'milestone', 'global_booking_count', $4, $5, $6) + `, bookingID, userID, globalCampaignID, globalPercent, bookingTotal, discountAmount); err != nil { + log.Printf("ALERT: failed to insert booking discount: %v", err) + } + if _, err := tx.Exec(ctx, ` + INSERT INTO payments (booking_id, payment_type, payment_method, amount, status, created_by) + VALUES ($1, 'partial', 'discount', $2, 'completed', $3) + `, bookingID, discountAmount, userID); err != nil { + log.Printf("ALERT: failed to insert payment record: %v", err) + } } } else { log.Printf("Skipping global milestone campaign %s at completion for booking %s — obligation already covered by real money (would over-credit)", globalCampaignID, bookingID) @@ -331,22 +379,38 @@ func ApplyBookingCompletionSideEffects(ctx context.Context, tx pgx.Tx, bookingID // F1 over-credit guard — see the time-based block above. if capped, ok := capDiscountToRemainingObligation(ctx, tx, bookingID, discountAmount); ok { discountAmount = capped - if _, err := tx.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, 'milestone', 'anniversary', $4, $5, $6) - `, bookingID, userID, c.id, c.pct, bookingTotal, discountAmount); err != nil { - log.Printf("ALERT: failed to insert booking discount: %v", err) - } - if _, err := tx.Exec(ctx, ` - INSERT INTO payments (booking_id, payment_type, payment_method, amount, status, created_by) - VALUES ($1, 'partial', 'discount', $2, 'completed', $3) - `, bookingID, discountAmount, userID); err != nil { - log.Printf("ALERT: failed to insert payment record: %v", err) - } - if _, err := tx.Exec(ctx, ` - UPDATE discount_campaigns SET times_redeemed = times_redeemed + 1 WHERE id = $1 - `, c.id); err != nil { - log.Printf("ALERT: failed to insert payment record: %v", err) + // B13 atomic reservation FIRST — see the time-based block above. + var reservedID string + if err := tx.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 + `, c.id).Scan(&reservedID); err != nil { + if errors.Is(err, pgx.ErrNoRows) { + log.Printf("Skipping anniversary campaign %s at completion for booking %s — campaign fully redeemed by a concurrent redemption", c.id, bookingID) + } else { + log.Printf("ALERT: failed to reserve redemption for campaign %s at completion for booking %s: %v", c.id, bookingID, err) + } + } else { + // Once-per-user backstop — see the per-user + // milestone block above. + tag, err := tx.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, 'milestone', 'anniversary', $4, $5, $6) + ON CONFLICT (user_id, source_id) WHERE discount_source = 'campaign' AND milestone_type IN ('per_user_booking_count', 'anniversary') DO NOTHING + `, bookingID, userID, c.id, c.pct, bookingTotal, discountAmount) + if err != nil { + log.Printf("ALERT: failed to insert booking discount: %v", err) + } else if tag.RowsAffected() == 0 { + log.Printf("Anniversary campaign %s already applied for user %s — skipping duplicate at completion for booking %s", c.id, userID, bookingID) + } else { + if _, err := tx.Exec(ctx, ` + INSERT INTO payments (booking_id, payment_type, payment_method, amount, status, created_by) + VALUES ($1, 'partial', 'discount', $2, 'completed', $3) + `, bookingID, discountAmount, userID); err != nil { + log.Printf("ALERT: failed to insert payment record: %v", err) + } + } } } else { log.Printf("Skipping anniversary campaign %s at completion for booking %s — obligation already covered by real money (would over-credit)", c.id, bookingID) diff --git a/backend/handlers/payments/completion_test.go b/backend/handlers/payments/completion_test.go index 6f0b76e..799c92f 100644 --- a/backend/handlers/payments/completion_test.go +++ b/backend/handlers/payments/completion_test.go @@ -4,11 +4,16 @@ package payments import ( "context" + "sync" "testing" + "time" "crussell/db" "crussell/testutils" "crussell/testutils/fixtures" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) // TestCompleteActiveBookingFromPayment_RefusesCancelledBooking locks the M2 @@ -83,3 +88,278 @@ func TestCompleteFullyPaidBooking_CompletesPayableBooking(t *testing.T) { t.Errorf("expected the fully-paid payable booking completed, got %q", status) } } + +// TestCompletion_ConcurrentCompletions_CampaignMaxRedemptions proves the B13 +// atomic reservation in the completion path (completion.go): two concurrent +// completions of two DIFFERENT bookings of the same user both pass the +// eligibility SELECT while a max_redemptions=1 campaign still has headroom, but +// only ONE may win the conditional reservation UPDATE. The loser matches zero +// rows (pgx.ErrNoRows) and SKIPS the discount — the booking still completes. +// times_redeemed must end at exactly 1, never 2. +func TestCompletion_ConcurrentCompletions_CampaignMaxRedemptions(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + serviceID, err := fixtures.CreateTestService(tx) + require.NoError(t, err) + + bookingIDs := make([]string, 2) + for i := range bookingIDs { + bookingID, err := fixtures.CreateTestBookingAtTime(tx, userID, serviceID, + time.Date(2099, 12, 31, 10, i+1, 0, 0, time.UTC)) + require.NoError(t, err) + if _, err := tx.Exec(ctx, "UPDATE bookings SET status = 'in_progress' WHERE id = $1", bookingID); err != nil { + t.Fatalf("failed to set booking in_progress: %v", err) + } + bookingIDs[i] = bookingID + } + pool := context.Background() + t.Cleanup(func() { + for _, bid := range bookingIDs { + _, _ = db.Conn.Exec(pool, `DELETE FROM booking_discounts WHERE booking_id = $1`, bid) + _, _ = db.Conn.Exec(pool, `DELETE FROM payments WHERE booking_id = $1`, bid) + _, _ = db.Conn.Exec(pool, `DELETE FROM booking_services WHERE booking_id = $1`, bid) + _, _ = db.Conn.Exec(pool, `DELETE FROM bookings WHERE id = $1`, bid) + } + _, _ = db.Conn.Exec(pool, `DELETE FROM discount_campaigns WHERE name = 'Completion Race Campaign'`) + _, _ = db.Conn.Exec(pool, `DELETE FROM services WHERE id = $1`, serviceID) + _, _ = db.Conn.Exec(pool, `DELETE FROM users WHERE id = $1`, userID) + }) + + // Commit the setup so both goroutines complete at pool level on independent + // connections — a shared per-test tx would serialize them. + pgxTx := db.TxFromContext(ctx) + require.NotNil(t, pgxTx, "no transaction in context") + require.NoError(t, pgxTx.Commit(ctx), "failed to commit setup tx") + + var campaignID string + require.NoError(t, db.Conn.QueryRow(pool, ` + INSERT INTO discount_campaigns (name, campaign_type, discount_percent, status, start_date, end_date, max_redemptions, times_redeemed) + VALUES ('Completion Race Campaign', 'time_based', 10, 'active', NOW() - INTERVAL '1 day', NOW() + INTERVAL '1 day', 1, 0) + RETURNING id + `).Scan(&campaignID)) + + var wg sync.WaitGroup + startBoth := make(chan struct{}) + commitErrs := make([]error, 2) + for i, bid := range bookingIDs { + wg.Add(1) + go func(idx int, booking string) { + defer wg.Done() + <-startBoth + cctx := context.Background() + gtx, err := db.Conn.Begin(cctx) + if err != nil { + commitErrs[idx] = err + return + } + defer func() { _ = gtx.Rollback(cctx) }() + completeActiveBookingFromPayment(cctx, gtx, booking) + commitErrs[idx] = gtx.Commit(cctx) + }(i, bid) + } + close(startBoth) + wg.Wait() + + for i, err := range commitErrs { + require.NoError(t, err, "completion tx %d must commit", i) + } + + // Both bookings must complete regardless of who won the discount race. + for _, bid := range bookingIDs { + var status string + require.NoError(t, db.Conn.QueryRow(pool, "SELECT status FROM bookings WHERE id = $1", bid).Scan(&status)) + assert.Equal(t, "completed", status, "a discount race must never fail a booking completion") + } + + // Exactly ONE booking got the campaign discount. + var discountCount int + require.NoError(t, db.Conn.QueryRow(pool, ` + SELECT COUNT(*) FROM booking_discounts + WHERE source_id = $1 AND discount_source = 'campaign' AND campaign_type = 'time_based' + `, campaignID).Scan(&discountCount)) + assert.Equal(t, 1, discountCount, "exactly one completion must win the max_redemptions=1 campaign") + + // The redemption counter never over-increments past max_redemptions. + var redeemed int + require.NoError(t, db.Conn.QueryRow(pool, "SELECT times_redeemed FROM discount_campaigns WHERE id = $1", campaignID).Scan(&redeemed)) + assert.Equal(t, 1, redeemed, "times_redeemed must end at 1, never 2") +} + +// TestCompletion_CampaignExhaustedAtReservation_SkipsDiscount locks the +// pgx.ErrNoRows skip branch of the completion's conditional reservation +// deterministically. A concurrent redemption exhausts the campaign +// (times_redeemed = max_redemptions) in an uncommitted transaction AFTER the +// completion's eligibility SELECT reads headroom but BEFORE its conditional +// UPDATE. Under READ COMMITTED the blocked UPDATE re-evaluates its WHERE +// against the post-increment row, matches zero rows, and the completion must +// STILL complete the booking while SKIPPING the discount — no booking_discounts +// row, no discount payment, counter unchanged. +func TestCompletion_CampaignExhaustedAtReservation_SkipsDiscount(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, bookingID, serviceID := setupTestData(t, ctx, tx) + + 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 ('Exhausted At Completion', 'time_based', 10, 'active', NOW() - INTERVAL '1 day', NOW() + INTERVAL '1 day', 1, 0) + RETURNING id + `).Scan(&campaignID)) + + pgxTx := db.TxFromContext(ctx) + require.NotNil(t, pgxTx, "no transaction in context") + require.NoError(t, pgxTx.Commit(ctx), "failed to commit setup tx") + + pool := context.Background() + t.Cleanup(func() { + _, _ = db.Conn.Exec(pool, `DELETE FROM booking_discounts WHERE booking_id = $1`, bookingID) + _, _ = db.Conn.Exec(pool, `DELETE FROM payments WHERE booking_id = $1`, bookingID) + _, _ = db.Conn.Exec(pool, `DELETE FROM booking_services WHERE booking_id = $1`, bookingID) + _, _ = db.Conn.Exec(pool, `DELETE FROM bookings WHERE id = $1`, bookingID) + _, _ = db.Conn.Exec(pool, `DELETE FROM discount_campaigns WHERE id = $1`, campaignID) + _, _ = db.Conn.Exec(pool, `DELETE FROM services WHERE id = $1`, serviceID) + _, _ = db.Conn.Exec(pool, `DELETE FROM users WHERE id = $1`, userID) + }) + + // A separate transaction exhausts the campaign (times_redeemed = 1) WITHOUT + // committing — its row lock makes the completion's conditional UPDATE block + // until it is released, while READ COMMITTED keeps the completion's earlier + // eligibility SELECT reading the old committed value (0). That is exactly + // the read-then-write window the race exploits. + htx, err := db.Conn.Begin(pool) + require.NoError(t, err, "failed to begin holder tx") + defer func() { _ = htx.Rollback(pool) }() + _, err = htx.Exec(pool, `UPDATE discount_campaigns SET times_redeemed = 1 WHERE id = $1`, campaignID) + require.NoError(t, err, "failed to exhaust campaign on holder tx") + + // Launch the completion on another connection — it passes the eligibility + // SELECT and blocks on the conditional UPDATE. Releasing the holder lets the + // blocked UPDATE re-evaluate and miss. + done := make(chan struct{}) + var completeErr error + go func() { + defer close(done) + cctx := context.Background() + gtx, err := db.Conn.Begin(cctx) + if err != nil { + completeErr = err + return + } + defer func() { _ = gtx.Rollback(cctx) }() + completeActiveBookingFromPayment(cctx, gtx, bookingID) + completeErr = gtx.Commit(cctx) + }() + + // Give the completion time to reach the blocked conditional UPDATE. + time.Sleep(300 * time.Millisecond) + require.NoError(t, htx.Commit(pool), "failed to release holder tx") + select { + case <-done: + case <-time.After(30 * time.Second): + t.Fatal("completion did not finish after holder release (deadlock?)") + } + require.NoError(t, completeErr, "completion must succeed despite the exhausted campaign") + + var status string + require.NoError(t, db.Conn.QueryRow(pool, "SELECT status FROM bookings WHERE id = $1", bookingID).Scan(&status)) + assert.Equal(t, "completed", status, "a discount race must never fail the booking completion") + + var discountCount int + require.NoError(t, db.Conn.QueryRow(pool, `SELECT COUNT(*) FROM booking_discounts WHERE booking_id = $1`, bookingID).Scan(&discountCount)) + assert.Zero(t, discountCount, "the exhausted campaign must NOT be applied at completion") + + var redeemed int + require.NoError(t, db.Conn.QueryRow(pool, "SELECT times_redeemed FROM discount_campaigns WHERE id = $1", campaignID).Scan(&redeemed)) + assert.Equal(t, 1, redeemed, "times_redeemed stays at the concurrent redemption's 1") +} + +// TestCompletion_PerUserMilestone_DuplicateSuppressed locks the once-per-user +// backstop for the per-user milestone (completion.go): the partial unique index +// uq_booking_discounts_user_milestone_campaign + ON CONFLICT DO NOTHING. A +// concurrent completion of ANOTHER of this user's bookings inserts the milestone +// discount row (uncommitted — invisible to this completion's NOT EXISTS +// eligibility read under READ COMMITTED). This completion's own INSERT then +// blocks on the conflicting unique index entry and is suppressed after the +// concurrent winner commits: exactly ONE milestone row exists for the user, no +// discount payment is minted, and the booking still completes. +func TestCompletion_PerUserMilestone_DuplicateSuppressed(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, bookingID, serviceID := setupTestData(t, ctx, tx) + + var campaignID string + require.NoError(t, tx.QueryRow(ctx, ` + INSERT INTO discount_campaigns (name, campaign_type, discount_percent, status, start_date, end_date, milestone_type, milestone_value, max_redemptions) + VALUES ('Milestone At Completion', 'milestone', 15, 'active', NOW() - INTERVAL '1 day', NOW() + INTERVAL '1 day', 'per_user_booking_count', 1, 10) + RETURNING id + `).Scan(&campaignID)) + + pgxTx := db.TxFromContext(ctx) + require.NotNil(t, pgxTx, "no transaction in context") + require.NoError(t, pgxTx.Commit(ctx), "failed to commit setup tx") + + pool := context.Background() + t.Cleanup(func() { + _, _ = db.Conn.Exec(pool, `DELETE FROM booking_discounts WHERE booking_id = $1`, bookingID) + _, _ = db.Conn.Exec(pool, `DELETE FROM payments WHERE booking_id = $1`, bookingID) + _, _ = db.Conn.Exec(pool, `DELETE FROM booking_services WHERE booking_id = $1`, bookingID) + _, _ = db.Conn.Exec(pool, `DELETE FROM bookings WHERE id = $1`, bookingID) + _, _ = db.Conn.Exec(pool, `DELETE FROM discount_campaigns WHERE id = $1`, campaignID) + _, _ = db.Conn.Exec(pool, `DELETE FROM services WHERE id = $1`, serviceID) + _, _ = db.Conn.Exec(pool, `DELETE FROM users WHERE id = $1`, userID) + }) + + // The concurrent winner already inserted the milestone row for this user + // (uncommitted — the completion's NOT EXISTS eligibility read passes, its + // INSERT then blocks on the unique index entry). + htx, err := db.Conn.Begin(pool) + require.NoError(t, err, "failed to begin holder tx") + defer func() { _ = htx.Rollback(pool) }() + _, err = htx.Exec(pool, ` + 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, 'milestone', 'per_user_booking_count', 15, 50.00, 7.50) + `, bookingID, userID, campaignID) + require.NoError(t, err, "failed to insert concurrent milestone discount on holder tx") + + done := make(chan struct{}) + var completeErr error + go func() { + defer close(done) + cctx := context.Background() + gtx, err := db.Conn.Begin(cctx) + if err != nil { + completeErr = err + return + } + defer func() { _ = gtx.Rollback(cctx) }() + completeActiveBookingFromPayment(cctx, gtx, bookingID) + completeErr = gtx.Commit(cctx) + }() + + // Give the completion time to reach the blocked booking_discounts INSERT. + time.Sleep(300 * time.Millisecond) + require.NoError(t, htx.Commit(pool), "failed to release holder tx") + select { + case <-done: + case <-time.After(30 * time.Second): + t.Fatal("completion did not finish after holder release (deadlock?)") + } + require.NoError(t, completeErr, "completion must succeed despite the suppressed duplicate") + + var status string + require.NoError(t, db.Conn.QueryRow(pool, "SELECT status FROM bookings WHERE id = $1", bookingID).Scan(&status)) + assert.Equal(t, "completed", status, "a duplicate milestone must never fail the booking completion") + + var milestoneCount int + require.NoError(t, db.Conn.QueryRow(pool, ` + SELECT COUNT(*) FROM booking_discounts + WHERE user_id = $1 AND source_id = $2 AND discount_source = 'campaign' + `, userID, campaignID).Scan(&milestoneCount)) + assert.Equal(t, 1, milestoneCount, "the once-per-user milestone must apply at most once per user") + + var payCount int + require.NoError(t, db.Conn.QueryRow(pool, ` + SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND payment_method = 'discount' + `, bookingID).Scan(&payCount)) + assert.Zero(t, payCount, "the suppressed duplicate must not mint a discount payment") +} diff --git a/backend/handlers/payments/dst_timezone_test.go b/backend/handlers/payments/dst_timezone_test.go index 225a0b6..1803bf7 100644 --- a/backend/handlers/payments/dst_timezone_test.go +++ b/backend/handlers/payments/dst_timezone_test.go @@ -90,7 +90,10 @@ func TestDepositProtectionWindow_UTCInstant(t *testing.T) { // protection window -> a single unsplit record. record := makeTestRecord("b-boundary-past", "full", 50) info := &BookingPaymentInfo{StartTime: boundaryUTC, TotalAmount: 50, TotalPaid: 0} - records := buildSplitRecords(record, "full", info, 50) + records, err := buildSplitRecords(record, "full", info, 50) + if err != nil { + t.Fatalf("buildSplitRecords: %v", err) + } if len(records) != 1 { t.Fatalf("past BST-midnight booking: expected 1 record (no split), got %d", len(records)) } @@ -109,7 +112,10 @@ func TestDepositProtectionWindow_UTCInstant(t *testing.T) { record2 := makeTestRecord("b-boundary-future", "full", 50) info2 := &BookingPaymentInfo{StartTime: futureBoundary, TotalAmount: 50, TotalPaid: 0} - records2 := buildSplitRecords(record2, "full", info2, 50) + records2, err2 := buildSplitRecords(record2, "full", info2, 50) + if err2 != nil { + t.Fatalf("buildSplitRecords: %v", err2) + } if len(records2) != 2 { t.Fatalf("future BST-midnight booking: expected 2 records (deposit split), got %d", len(records2)) } diff --git a/backend/handlers/payments/errors_test.go b/backend/handlers/payments/errors_test.go index cd0d995..1bc3ca6 100644 --- a/backend/handlers/payments/errors_test.go +++ b/backend/handlers/payments/errors_test.go @@ -730,9 +730,11 @@ func TestVerificationRequiredSurfacing_AllChargeSites(t *testing.T) { } // the dedicated add-card endpoint: with REQUIRE_2FA enforced, persisting a card -// is refused 402 verification_required (SCA-only — the 2FA fallback was -// removed) and no card row is created — the save-card endpoint is not an -// un-gated side door. +// from a NON-token-like source (a raw PAN — the only shape the gate still +// refuses) is rejected 402 verification_required (SCA-only — the 2FA fallback +// was removed) and no card row is created — the save-card endpoint is not an +// un-gated side door. A genuine Square token-like card token (cnon:/ccof:) is +// SCA-proven and skips the gate (see TestTwoFactorEnforced_CreatePaymentMethod_SCATokenizeResult_Save_Succeeds). func TestTwoFactorEnforced_CreatePaymentMethod_Tokenless_402(t *testing.T) { t.Setenv("REQUIRE_2FA", "true") t.Setenv("SQUARE_ENVIRONMENT", "production") @@ -745,7 +747,7 @@ func TestTwoFactorEnforced_CreatePaymentMethod_Tokenless_402(t *testing.T) { token := jwt.GenerateUserToken(userID) handler := CreatePaymentMethod - w := makePaymentRequest(handler, "POST", "/api/user/payment-methods", CreatePaymentMethodRequest{CardToken: "cnon:2fa-blocked"}, token, ctx) + w := makePaymentRequest(handler, "POST", "/api/user/payment-methods", CreatePaymentMethodRequest{CardToken: "4111111111111111"}, token, ctx) if w.Code != http.StatusPaymentRequired { t.Fatalf("expected 402 verification_required when 2FA is enforced (SCA-only), got %d: %s", w.Code, w.Body.String()) } @@ -782,7 +784,7 @@ func TestTwoFactorEnforced_CreatePaymentMethod_With2FA_Blocked(t *testing.T) { token := jwt.GenerateUserToken(userID) handler := CreatePaymentMethod - w := makePaymentRequest(handler, "POST", "/api/user/payment-methods", CreatePaymentMethodRequest{CardToken: "cnon:2fa-ok", VerificationCode: "778899"}, token, ctx) + w := makePaymentRequest(handler, "POST", "/api/user/payment-methods", CreatePaymentMethodRequest{CardToken: "4111111111111111"}, token, ctx) if w.Code != http.StatusPaymentRequired { t.Fatalf("expected 402 verification_required even with a valid 2FA code (SCA-only), got %d: %s", w.Code, w.Body.String()) } diff --git a/backend/handlers/payments/giftcards.go b/backend/handlers/payments/giftcards.go index ffd05d0..b766a0c 100644 --- a/backend/handlers/payments/giftcards.go +++ b/backend/handlers/payments/giftcards.go @@ -117,21 +117,31 @@ type BuyGiftCardRequest struct { // distinct purchases get different keys. Max=45 matches Square's limit. IdempotencyKey string `json:"idempotency_key,omitempty" validate:"omitempty,max=45"` VerificationToken *string `json:"verification_token,omitempty"` - // VerificationCode is the customer's current 2FA one-time code (B10): an - // enforced environment charges/persists a saved card only when this matches - // the customer's pending code. - VerificationCode string `json:"verification_code,omitempty"` - // ConsentVersion / ConsentAccepted: the customer's explicit versioned - // consent to the SCA-unavailable → 2FA fallback (C6), enforced server-side - // (403 consent_required) and recorded on the 2fa_fallback_charge audit row. - ConsentVersion *string `json:"consent_version,omitempty"` - ConsentAccepted bool `json:"consent_accepted"` } type RedeemGiftCardRequest struct { Code string `json:"code"` } +// giftCardAmountPence validates an admin gift-card transaction amount (pounds +// float64) and converts it to pence. Non-finite values (NaN/Inf) and amounts +// above the £250 per-transaction bound (maxAdminGiftCardTransactionPence) are +// rejected BEFORE the float→int64 conversion: a huge float would otherwise wrap +// int64(math.Round(...)) to INT64_MIN and silently bypass the cap (the check +// `int64(...) > maxAdminGiftCardTransactionPence` would be false for a negative +// wrap). Returns ok=false with the HTTP error already written on rejection. +func giftCardAmountPence(w http.ResponseWriter, amount float64) (int64, bool) { + if math.IsNaN(amount) || math.IsInf(amount, 0) { + http.Error(w, "Amount must be a finite number", http.StatusBadRequest) + return 0, false + } + if amount > maxAdminGiftCardTransactionPence/100.0 { + http.Error(w, "Amount exceeds the maximum of £250", http.StatusBadRequest) + return 0, false + } + return int64(math.Round(amount * 100)), true +} + // --- Admin Handlers --- // giftCardDailyCapLockKey is the session advisory-lock key that serializes one @@ -140,6 +150,48 @@ type RedeemGiftCardRequest struct { // new value are atomic (M7). Keyed per admin: distinct admins never contend. const giftCardDailyCapLockKey = "crussell:giftcard-daily-cap:" +// giftCardUserCapLockKey is the session advisory-lock key that serializes one +// USER's online gift-card purchases (BuyGiftCard) so the £500/day cap check +// (userGiftCardSpentToday) and the purchase that records the new spend are +// atomic. The idempotency-key lock (crussell:giftcard:) only serializes +// SAME-key retries; two concurrent DISTINCT purchases would otherwise both +// read spentToday=0 before either commits and both pass the cap. Keyed per +// user: distinct users never contend. +const giftCardUserCapLockKey = "crussell:giftcard-user-cap:" + +// acquireUserGiftCardCapLock acquires the per-user gift-card daily-cap lock +// (a bounded try-lock on a pinned pool connection, mirroring +// acquireGiftCardDailyCapLock). The cap read (userGiftCardSpentToday) and the +// purchase that records the new spend must run under the same lock, or two +// concurrent distinct purchases could both read the day's spend before either +// commits and both pass the cap — overshooting the £500/day ceiling. Returns +// the pinned connection once the lock is held (the caller must defer +// capConn.Release() and releasePaymentLock(capConn, key)) or nil after writing +// the error response. +func acquireUserGiftCardCapLock(ctx context.Context, w http.ResponseWriter, userID string) (*pgxpool.Conn, bool) { + lockKey := giftCardUserCapLockKey + userID + pinConn, err := db.Conn.Acquire(ctx) + if err != nil { + log.Printf("Failed to acquire connection for gift-card user-cap lock %s: %v", lockKey, err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return nil, false + } + lockOK, err := acquireAdvisoryLock(ctx, pinConn, lockKey) + if err != nil { + pinConn.Release() + log.Printf("Failed to acquire gift-card user-cap lock %s: %v", lockKey, err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return nil, false + } + if !lockOK { + pinConn.Release() + log.Printf("Gift-card user-cap lock %s not acquired within bound — another gift-card purchase for this user is in progress", lockKey) + http.Error(w, "Another gift-card purchase is already in progress — please try again in a moment", http.StatusConflict) + return nil, false + } + return pinConn, true +} + // acquireGiftCardDailyCapLock acquires the per-admin gift-card daily-cap lock // (a bounded try-lock on a pinned pool connection, mirroring the payment // handlers' serialization pattern). The daily cap check @@ -474,6 +526,13 @@ func GetGiftCards(w http.ResponseWriter, r *http.Request) { func CreateGiftCard(w http.ResponseWriter, r *http.Request) { ctx := r.Context() + // Defense-in-depth admin check (S-1) — creating a funded gift card moves + // money, so it must stay admin-only even if the route is ever re-registered + // on a router without mw.RequireAdmin. + if !isAdminRequest(r) { + http.Error(w, "Admin access required", http.StatusForbidden) + return + } adminID, _ := ctx.Value(mw.UserIDKey).(string) var req CreateGiftCardRequest @@ -494,9 +553,11 @@ func CreateGiftCard(w http.ResponseWriter, r *http.Request) { // C2: cap the admin-funded amount at £250 (25,000 pence) per transaction // (owner decision — tighter than the £10,000 ceiling ValidateAmount // enforces on other payment entry points, and matching the till's cap). - // An inventory card may still be created at £0. - if int64(math.Round(req.Amount*100)) > maxAdminGiftCardTransactionPence { - http.Error(w, "Amount exceeds the maximum of £250", http.StatusBadRequest) + // An inventory card may still be created at £0. The pence conversion is + // bounded against non-finite/oversized floats BEFORE the int64 conversion + // (a huge float would wrap to INT64_MIN and bypass the cap). + amountPence, amountOK := giftCardAmountPence(w, req.Amount) + if !amountOK { return } @@ -517,7 +578,7 @@ func CreateGiftCard(w http.ResponseWriter, r *http.Request) { http.Error(w, "internal server error", http.StatusInternalServerError) return } - if int64(math.Round(adminValueToday*100))+int64(math.Round(req.Amount*100)) > maxAdminGiftCardDailyPence { + if int64(math.Round(adminValueToday*100))+amountPence > maxAdminGiftCardDailyPence { http.Error(w, "You have reached your £5,000 daily gift-card value limit", http.StatusBadRequest) return } @@ -620,6 +681,13 @@ func CreateGiftCard(w http.ResponseWriter, r *http.Request) { func TopUpGiftCard(w http.ResponseWriter, r *http.Request) { ctx := r.Context() + // Defense-in-depth admin check (S-1) — topping up a gift card moves money, + // so it must stay admin-only even if the route is ever re-registered on a + // router without mw.RequireAdmin. + if !isAdminRequest(r) { + http.Error(w, "Admin access required", http.StatusForbidden) + return + } adminID, _ := ctx.Value(mw.UserIDKey).(string) cardID := chi.URLParam(r, "id") if cardID == "" || !validators.IsValidID(cardID) { @@ -640,9 +708,11 @@ func TopUpGiftCard(w http.ResponseWriter, r *http.Request) { // C2: cap the top-up at £250 (25,000 pence) per transaction (owner // decision — tighter than the £10,000 ceiling ValidateAmount enforces on - // other payment entry points, and matching the till's cap). - if int64(math.Round(req.Amount*100)) > maxAdminGiftCardTransactionPence { - http.Error(w, "Amount exceeds the maximum of £250", http.StatusBadRequest) + // other payment entry points, and matching the till's cap). The pence + // conversion is bounded against non-finite/oversized floats BEFORE the + // int64 conversion (a huge float would wrap to INT64_MIN and bypass the cap). + amountPence, amountOK := giftCardAmountPence(w, req.Amount) + if !amountOK { return } @@ -663,7 +733,7 @@ func TopUpGiftCard(w http.ResponseWriter, r *http.Request) { http.Error(w, "internal server error", http.StatusInternalServerError) return } - if int64(math.Round(adminValueToday*100))+int64(math.Round(req.Amount*100)) > maxAdminGiftCardDailyPence { + if int64(math.Round(adminValueToday*100))+amountPence > maxAdminGiftCardDailyPence { http.Error(w, "You have reached your £5,000 daily gift-card value limit", http.StatusBadRequest) return } @@ -695,7 +765,8 @@ func TopUpGiftCard(w http.ResponseWriter, r *http.Request) { var redeemedBy sql.NullString var isInventory bool var currentTotalFunds float64 - err = tx.QueryRow(ctx, "SELECT redeemed_by, is_inventory, total_funds_added FROM gift_cards WHERE id = $1 FOR UPDATE", cardID).Scan(&redeemedBy, &isInventory, ¤tTotalFunds) + var expiryDate sql.NullTime + err = tx.QueryRow(ctx, "SELECT redeemed_by, is_inventory, total_funds_added, expiry_date FROM gift_cards WHERE id = $1 FOR UPDATE", cardID).Scan(&redeemedBy, &isInventory, ¤tTotalFunds, &expiryDate) if err != nil { if errors.Is(err, pgx.ErrNoRows) { http.Error(w, "Gift card not found", http.StatusNotFound) @@ -711,6 +782,23 @@ func TopUpGiftCard(w http.ResponseWriter, r *http.Request) { return } + // money-F4: an expired gift card must never be topped up — the UPDATE + // below resets expiry_date to NOW()+months and would resurrect a card + // whose remaining value the nightly cleanup job already forfeited. + // Mirrors the till path's expiry gate (till.go:812-825) and + // RedeemGiftCard's (same DB-clock comparison, same 400); a NULL + // expiry_date (legacy) is treated as unexpired. + expired, err := giftCardExpired(ctx, tx, expiryDate) + if err != nil { + log.Printf("Failed to check gift card expiry: %v", err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + if expired { + http.Error(w, "Gift card has expired", http.StatusBadRequest) + return + } + expiryMonths, err := GetGiftCardExpiryMonths(ctx, tx) if err != nil { log.Printf("Failed to query gift card expiry months (using default %d): %v", defaultGiftCardExpiryMonths, err) @@ -782,6 +870,13 @@ func TopUpGiftCard(w http.ResponseWriter, r *http.Request) { func TransferGiftCard(w http.ResponseWriter, r *http.Request) { ctx := r.Context() + // Defense-in-depth admin check (S-1) — transferring gift-card value moves + // money, so it must stay admin-only even if the route is ever re-registered + // on a router without mw.RequireAdmin. + if !isAdminRequest(r) { + http.Error(w, "Admin access required", http.StatusForbidden) + return + } adminID, _ := ctx.Value(mw.UserIDKey).(string) fromCardID := chi.URLParam(r, "from") if fromCardID == "" || !validators.IsValidID(fromCardID) { @@ -813,9 +908,11 @@ func TransferGiftCard(w http.ResponseWriter, r *http.Request) { // C2: cap the transfer at £250 (25,000 pence) per transaction (owner // decision — tighter than the £10,000 ceiling ValidateAmount enforces on - // other payment entry points, and matching the till's cap). - if int64(math.Round(req.Amount*100)) > maxAdminGiftCardTransactionPence { - http.Error(w, "Amount exceeds the maximum of £250", http.StatusBadRequest) + // other payment entry points, and matching the till's cap). The pence + // conversion is bounded against non-finite/oversized floats BEFORE the + // int64 conversion (a huge float would wrap to INT64_MIN and bypass the cap). + amountPence, amountOK := giftCardAmountPence(w, req.Amount) + if !amountOK { return } @@ -836,7 +933,7 @@ func TransferGiftCard(w http.ResponseWriter, r *http.Request) { http.Error(w, "internal server error", http.StatusInternalServerError) return } - if int64(math.Round(adminValueToday*100))+int64(math.Round(req.Amount*100)) > maxAdminGiftCardDailyPence { + if int64(math.Round(adminValueToday*100))+amountPence > maxAdminGiftCardDailyPence { http.Error(w, "You have reached your £5,000 daily gift-card value limit", http.StatusBadRequest) return } @@ -1427,7 +1524,7 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { return } - if err := ValidateCardInfo(req.CardID, req.NewCardToken); err != nil { + if err := ValidateCardInfo(req.CardID, nil, req.NewCardToken); err != nil { log.Printf("Failed to process request: %v", err) http.Error(w, "Invalid request", http.StatusBadRequest) return @@ -1565,10 +1662,30 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { } } - // Daily limit (owner decision): a user may buy at most £500 of online + // daily limit (owner decision): a user may buy at most £500 of online // gift cards per UTC day. Sums only COMPLETED online purchases (the // gift_card_transactions rows BuyGiftCard writes) so a pending-retry of a // failed Square attempt is never blocked by its own un-issued value. + // + // The cap read and the purchase that records the new spend are serialized + // per user (acquireUserGiftCardCapLock — a bounded try-lock on a pinned + // pool connection, mirroring the admin per-admin cap lock): the + // idempotency-key lock above only serializes SAME-key retries, so two + // concurrent DISTINCT purchases could otherwise both read spentToday=0 + // before either commits and both pass the cap. The lock is held (via the + // defers) through the purchase's issue transaction, so the read-modify-write + // cycle is atomic per user. A pending-reuse retry still passes through here + // (only a COMPLETED purchase short-circuits above): its first attempt's + // spend was never recorded (the gift_card_transactions row is written in + // the issue transaction only after Square succeeds), so the re-read is + // correct. + userCapPinConn, userCapLockOK := acquireUserGiftCardCapLock(ctx, w, userID) + if !userCapLockOK { + return + } + defer userCapPinConn.Release() + defer releasePaymentLock(userCapPinConn, giftCardUserCapLockKey+userID) + spentToday, err := userGiftCardSpentToday(ctx, db.Conn, userID) if err != nil { log.Printf("Failed to query user gift-card spend today for %s: %v", userID, err) @@ -1584,33 +1701,16 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { // feature is enforced — both paying with an existing saved card (CardID) // and SAVING a new card during this purchase (SaveCard), mirroring // CreateBookingPayment/CreateTipPayment. A one-off new-card (nonce) charge - // that is not saved is not gated. - // consume=!reuse (LOW 6a): a FRESH purchase verifies WITH consumption — - // the code is single-use at the gate, closing the TOCTOU where a - // verified-but-unconsumed code could authorize a second charge within its - // lifetime — and a failed Square charge re-issues a fresh code - // (reissueTwoFACodeAfterFailedCharge below). A pending-reuse retry - // (reusePendingID != "") verifies WITHOUT consuming: the code was re-issued - // for exactly this retry and the completed-charge transaction below - // (ConsumePendingCode) burns it on terminal success, so a retry that fails - // again keeps its code for one more attempt. A charge carrying a Square - // verification_token (SCA performed) skips the gate; a token-less charge - // falls back to 2FA and twoFAFallbackUsed is set for the charge-success - // audit. + // that is not saved is not gated. A charge carrying a Square + // verification_token (SCA performed) skips the gate; a token-less charge is + // refused 402 verification_required (SCA-only — the homegrown 2FA fallback + // was removed). giftCardVerificationToken := "" if req.VerificationToken != nil { giftCardVerificationToken = *req.VerificationToken } - twoFAFallbackUsed := false if (req.CardID != nil && *req.CardID != "") || req.SaveCard { - var gateOK bool - gateOK, twoFAFallbackUsed = requireTwoFactorForCardAccess(w, r, paymentService, userID, req.VerificationCode, giftCardVerificationToken, reusePendingID == "") - if !gateOK { - return - } - // C6: a fallback-authorized purchase must carry the customer's accepted - // consent (403 consent_required otherwise). - if !enforceSCAFallbackConsent(w, req.ConsentVersion, req.ConsentAccepted, twoFAFallbackUsed) { + if gateOK, _ := requireTwoFactorForCardAccess(w, r, paymentService, userID, giftCardVerificationToken, reusePendingID == ""); !gateOK { return } } @@ -1804,13 +1904,6 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { paymentResult, err := SquareClient.CreatePayment(ctx, paymentReq) if err != nil { log.Printf("Failed to process gift card purchase payment: %v", err) - // The gate consumed the 2FA code for a FRESH saved-card purchase — - // re-issue so the same-key retry has a live code to verify. A - // pending-reuse retry verified without consuming at the gate, so its - // code survives for one more attempt. - if (req.CardID != nil && *req.CardID != "") || req.SaveCard { - reissueTwoFACodeAfterFailedCharge(ctx, db.Conn, userID, true, twoFAFallbackUsed && reusePendingID == "", r) - } // Payment record intentionally left as 'pending' for manual retry. // SCA-required failures must surface the structured verification_required // body so the frontend triggers the 3DS challenge, not a plain decline. @@ -1981,14 +2074,6 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { return } - // The 2FA BACKUP authorized this token-less saved-card gift-card purchase - // (SCA was unavailable) — record the strict fallback audit row AFTER the - // money transaction commits (a failed audit write must never roll back a - // completed charge). The actor is the customer's own userID. - if twoFAFallbackUsed { - insertTwoFAFallbackAudit(ctx, userID, userID, paymentResult.CardLast4, buyPaymentID, "gift-card purchase authorized via 2FA fallback (SCA unavailable)", consentVersionValue(req.ConsentVersion), req.ConsentAccepted) - } - w.WriteHeader(http.StatusCreated) if err := json.NewEncoder(w).Encode(map[string]any{ "status": "success", @@ -2155,6 +2240,13 @@ func ClaimExpiredBalance(w http.ResponseWriter, r *http.Request) { http.Error(w, "Unauthorized", http.StatusUnauthorized) return } + // Defense-in-depth admin check (S-1) — claiming expired gift-card balance + // moves money, so it must stay admin-only even if the route is ever + // re-registered on a router without mw.RequireAdmin. + if !isAdminRequest(r) { + http.Error(w, "Admin access required", http.StatusForbidden) + return + } var req ClaimExpiredBalanceRequest if err := json.NewDecoder(r.Body).Decode(&req); err != nil { diff --git a/backend/handlers/payments/giftcards_test.go b/backend/handlers/payments/giftcards_test.go index cb0ccd1..159ac71 100644 --- a/backend/handlers/payments/giftcards_test.go +++ b/backend/handlers/payments/giftcards_test.go @@ -10,6 +10,7 @@ import ( "math" "net/http" "net/http/httptest" + "strings" "sync" "testing" "time" @@ -2812,3 +2813,114 @@ func TestRedeemGiftCard_ExpiryEdge_DBClock(t *testing.T) { t.Errorf("expected 200 for a card expiring just after the DB clock, got %d. body: %s", w.Code, w.Body.String()) } } + +// Defense-in-depth (S-1): the money-moving gift-card admin handlers carry an +// in-handler isAdminRequest backstop, so a non-admin request must be refused +// with 403 even when the route is mounted on a router WITHOUT mw.RequireAdmin. +// Each router below applies ONLY mw.RequireAuth — the weaker-router scenario +// the backstop guards against. +func TestCreateGiftCard_NonAdminRefused(t *testing.T) { + t.Parallel() + _, tx := testutils.SetupTestTx(t) + + token := jwt.GenerateVerifiedUserToken("nonadminuser001") + reqBody, _ := json.Marshal(map[string]interface{}{"amount": 50.00}) + req := httptest.NewRequest("POST", "/api/admin/gift-cards", bytes.NewBuffer(reqBody)) + req.Header.Set("Authorization", "Bearer "+token) + req.Header.Set("Content-Type", "application/json") + req = req.WithContext(db.ContextWithTx(req.Context(), tx.(pgx.Tx))) + + w := httptest.NewRecorder() + r := chi.NewRouter() + r.Use(mw.RequireAuth) + r.Post("/api/admin/gift-cards", CreateGiftCard) + r.ServeHTTP(w, req) + + if w.Code != http.StatusForbidden { + t.Errorf("expected 403 for non-admin CreateGiftCard, got %d. body: %s", w.Code, w.Body.String()) + } + if !strings.Contains(w.Body.String(), "Admin access required") { + t.Errorf("expected 'Admin access required' body, got: %s", w.Body.String()) + } +} + +func TestTopUpGiftCard_NonAdminRefused(t *testing.T) { + t.Parallel() + _, tx := testutils.SetupTestTx(t) + + token := jwt.GenerateVerifiedUserToken("nonadminuser001") + reqBody, _ := json.Marshal(map[string]interface{}{ + "amount": 25.00, + "payment_method": "on_the_house", + }) + req := httptest.NewRequest("PUT", "/api/admin/gift-cards/aabbccddeeff/topup", bytes.NewBuffer(reqBody)) + req.Header.Set("Authorization", "Bearer "+token) + req.Header.Set("Content-Type", "application/json") + req = req.WithContext(db.ContextWithTx(req.Context(), tx.(pgx.Tx))) + + w := httptest.NewRecorder() + r := chi.NewRouter() + r.Use(mw.RequireAuth) + r.Put("/api/admin/gift-cards/{id}/topup", TopUpGiftCard) + r.ServeHTTP(w, req) + + if w.Code != http.StatusForbidden { + t.Errorf("expected 403 for non-admin TopUpGiftCard, got %d. body: %s", w.Code, w.Body.String()) + } + if !strings.Contains(w.Body.String(), "Admin access required") { + t.Errorf("expected 'Admin access required' body, got: %s", w.Body.String()) + } +} + +func TestTransferGiftCard_NonAdminRefused(t *testing.T) { + t.Parallel() + _, tx := testutils.SetupTestTx(t) + + token := jwt.GenerateVerifiedUserToken("nonadminuser001") + reqBody, _ := json.Marshal(map[string]interface{}{ + "to_card_id": "112233445566", + "amount": 30.00, + }) + req := httptest.NewRequest("POST", "/api/admin/gift-cards/aabbccddeeff/transfer", bytes.NewBuffer(reqBody)) + req.Header.Set("Authorization", "Bearer "+token) + req.Header.Set("Content-Type", "application/json") + req = req.WithContext(db.ContextWithTx(req.Context(), tx.(pgx.Tx))) + + w := httptest.NewRecorder() + r := chi.NewRouter() + r.Use(mw.RequireAuth) + r.Post("/api/admin/gift-cards/{from}/transfer", TransferGiftCard) + r.ServeHTTP(w, req) + + if w.Code != http.StatusForbidden { + t.Errorf("expected 403 for non-admin TransferGiftCard, got %d. body: %s", w.Code, w.Body.String()) + } + if !strings.Contains(w.Body.String(), "Admin access required") { + t.Errorf("expected 'Admin access required' body, got: %s", w.Body.String()) + } +} + +func TestClaimExpiredBalance_NonAdminRefused(t *testing.T) { + t.Parallel() + _, tx := testutils.SetupTestTx(t) + + token := jwt.GenerateVerifiedUserToken("nonadminuser001") + reqBody, _ := json.Marshal(map[string]interface{}{"balance_id": "aabbccddeeff"}) + req := httptest.NewRequest("POST", "/api/admin/gift-cards/expired-balances/claim", bytes.NewBuffer(reqBody)) + req.Header.Set("Authorization", "Bearer "+token) + req.Header.Set("Content-Type", "application/json") + req = req.WithContext(db.ContextWithTx(req.Context(), tx.(pgx.Tx))) + + w := httptest.NewRecorder() + r := chi.NewRouter() + r.Use(mw.RequireAuth) + r.Post("/api/admin/gift-cards/expired-balances/claim", ClaimExpiredBalance) + r.ServeHTTP(w, req) + + if w.Code != http.StatusForbidden { + t.Errorf("expected 403 for non-admin ClaimExpiredBalance, got %d. body: %s", w.Code, w.Body.String()) + } + if !strings.Contains(w.Body.String(), "Admin access required") { + t.Errorf("expected 'Admin access required' body, got: %s", w.Body.String()) + } +} diff --git a/backend/handlers/payments/handlers.go b/backend/handlers/payments/handlers.go index 6d1f719..1cb5576 100644 --- a/backend/handlers/payments/handlers.go +++ b/backend/handlers/payments/handlers.go @@ -68,38 +68,6 @@ func InsertAdminAuditCharge(ctx context.Context, adminID, targetUserID, action s } } -// insertTwoFAFallbackAudit records that a saved-card charge was authorized by -// the homegrown 2FA BACKUP because SCA was unavailable (the charge carried no -// Square verification_token). It is the strict-audit half of the SCA-primary / -// 2FA-backup decision model: every 2FA-fallback authorization of a saved-card -// charge must land an admin_audit_log row (action_type '2fa_fallback_charge', -// details {sca_performed:false, fallback_reason:"verification_unavailable"}) so -// the operator can distinguish SCA-authorized charges from fallback-authorized -// ones. Since C6 the row also captures the customer's versioned consent to the -// fallback (consent_version, consent_accepted, consent_versioned_at) — the -// exact values the server validated before the charge — so an operator/GDPR -// export can reconstruct which dialog version was shown, that it was accepted, -// when, and on which charge. Mirrors InsertAdminAuditCharge's best-effort, -// own-transaction, non-fatal failure handling (a failed audit write can never -// abort a completed charge). For customer-initiated online charges the actor -// (adminID) is the customer's own userID; for admin surfaces it is the admin -// from request context — the caller passes accordingly. cardLast4 and -// referenceID are filled by the caller at charge success -// (paymentResult.CardLast4, booking/till/payment id), where they are actually -// known. -func insertTwoFAFallbackAudit(ctx context.Context, adminID, userID, cardLast4, referenceID, notes string, consentVersion string, consentAccepted bool) { - InsertAdminAuditCharge(ctx, adminID, userID, "2fa_fallback_charge", map[string]any{ - "sca_performed": false, - "fallback_reason": "verification_unavailable", - "card_last4": cardLast4, - "reference_id": referenceID, - "notes": notes, - "consent_version": consentVersion, - "consent_accepted": consentAccepted, - "consent_versioned_at": clock.Now().Format(time.RFC3339), - }) -} - // logVerificationTokenProvenance records the charge context a legacy SCA // verification_token arrived with (saved-card reference + booking) so an // operator can correlate a minted token with the exact charge it authorized. @@ -134,10 +102,12 @@ type CreateTerminalPaymentRequest struct { // directly, bypassing the terminal. The frontend sends this for the admin // "Charge Saved Card" action. UserSavedCardID *string `json:"saved_card_id,omitempty"` - // verification_code: the customer's current 2FA one-time code (B10). An - // enforced environment charges a saved card only when this matches the - // customer's pending code; the operator relays it from the [2FA] log/email. - VerificationCode string `json:"verification_code,omitempty"` + // new_card_token: the SCA tokenize-result token (card.tokenize( + // verificationDetails, cardId)) for a saved-card charge. When present it + // coexists with saved_card_id — the token is the one-time charge SOURCE and + // the saved-card row supplies the customer (resolveChargeSource). Without a + // token the stored ccof: card id is the source (legacy saved-card charge). + NewCardToken *string `json:"new_card_token,omitempty"` // verification_token: Square 3DS/SCA verification token returned by the // frontend's buyer-verification flow (tokenizeWithVerification). When a // saved-card charge carries one, SCA has been performed by the issuer and @@ -153,14 +123,6 @@ type CreateTerminalPaymentRequest struct { // amount+card key for no-client-key retry safety. Cap ≤45 (Square's // idempotency-key limit for /v2/payments — this key feeds CreatePayment). IdempotencyKey string `json:"idempotency_key,omitempty" validate:"omitempty,max=45"` - // ConsentVersion / ConsentAccepted: the customer's explicit versioned - // consent to the SCA-unavailable → 2FA fallback (C6). The frontend sends - // consent_version:"v1" + consent_accepted:true (scaFallbackConsentFields) - // when the ScaFallbackConsentDialog was accepted; the server enforces it - // (403 consent_required) before a fallback charge can reach Square and - // records it on the 2fa_fallback_charge audit row. - ConsentVersion *string `json:"consent_version,omitempty"` - ConsentAccepted bool `json:"consent_accepted"` } type CreateBookingPaymentRequest struct { @@ -178,15 +140,6 @@ type CreateBookingPaymentRequest struct { SaveCard bool `json:"save_card"` IdempotencyKey string `json:"idempotency_key,omitempty" validate:"omitempty,max=45"` VerificationToken *string `json:"verification_token,omitempty"` - // VerificationCode is the customer's current 2FA one-time code (B10): an - // enforced environment charges a saved card only when this matches the - // customer's pending code. - VerificationCode string `json:"verification_code,omitempty"` - // ConsentVersion / ConsentAccepted: the customer's explicit versioned - // consent to the SCA-unavailable → 2FA fallback (C6), enforced server-side - // (403 consent_required) and recorded on the 2fa_fallback_charge audit row. - ConsentVersion *string `json:"consent_version,omitempty"` - ConsentAccepted bool `json:"consent_accepted"` // ConfirmOverflowTip acknowledges that an overpayment beyond the booking's // remaining balance will be recorded as a tip (M7). Tips cannot be paid in // advance, so a pre-start overpayment is rejected with 400 @@ -214,15 +167,6 @@ type CreateTipPaymentRequest struct { SaveCard bool `json:"save_card"` IdempotencyKey string `json:"idempotency_key,omitempty" validate:"omitempty,max=45"` VerificationToken *string `json:"verification_token,omitempty"` - // VerificationCode is the customer's current 2FA one-time code (B10): an - // enforced environment charges a saved card only when this matches the - // customer's pending code. - VerificationCode string `json:"verification_code,omitempty"` - // ConsentVersion / ConsentAccepted: the customer's explicit versioned - // consent to the SCA-unavailable → 2FA fallback (C6), enforced server-side - // (403 consent_required) and recorded on the 2fa_fallback_charge audit row. - ConsentVersion *string `json:"consent_version,omitempty"` - ConsentAccepted bool `json:"consent_accepted"` } type CheckoutResponse struct { @@ -418,6 +362,26 @@ func calculateDiscountPreview(ctx context.Context, bookingID string, userID stri // remaining + this bound. const maxTerminalTipPence = int64(5000) // £50 +// maxOnlineTipPence caps a single ONLINE tip (CreateTipPayment, the dedicated +// POST /api/bookings/{id}/tip endpoint). The terminal checkout (B3) caps its +// embedded gratuity at maxTerminalTipPence (£50) because the admin keys the tip +// in alongside the booking portion; the online path is customer-initiated and +// is deliberately more generous. £250 is the bound because: +// - tips are gratuity on a percentage of the service — the frontend presets +// are 10/15/20% of the booking subtotal (frontend/src/lib/constants/policy.ts +// TIP_PRESET_PCTS), so £250 is ~5x the 20% preset on even the salon's most +// expensive service; +// - it matches the £250 per-transaction ceiling the business already uses for +// gift-card creates/topups (maxAdminGiftCardTransactionPence, giftcard_limits.go), +// an owner-signed figure this codebase already treats as the generous +// single-transaction bound; +// - it sits far above the £50 till cap while staying well below +// ValidateAmount's generic £10,000 ceiling — without it a customer could tip +// £9,999 online when the till is capped at £50 (money/UX inconsistency). +// - 25,000 pence is the effective online ceiling: ValidateAmount passes first, +// and this stricter bound makes the generic £10,000 cap unreachable here. +const maxOnlineTipPence = int64(25000) // £250 + // 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 @@ -969,8 +933,12 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { // Resolve the saved-card Square source for the booking's user (the // card's owner, not the admin) — shared new-card-vs-saved-card - // resolution, see resolveChargeSource for the R6 rationale. - sourceID, _, savedCardCustomerID, sourceOK := resolveChargeSource(r.Context(), w, service, bookingUserID.String, nil, req.UserSavedCardID, false, "Saved card not found") + // resolution, see resolveChargeSource for the R6 rationale. When the + // request carries an SCA tokenize-result token (new_card_token), it is + // used as the one-time charge source and the saved-card row supplies + // the customer — mirroring the booking path (handlers.go:2050-2051, + // 2626). + sourceID, _, savedCardCustomerID, sourceOK := resolveChargeSource(r.Context(), w, service, bookingUserID.String, req.NewCardToken, req.UserSavedCardID, false, "Saved card not found") if !sourceOK { return } @@ -1140,17 +1108,14 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { // retry verifies WITHOUT consuming, so a retry that fails again keeps // its code for one more attempt (the completed-charge transaction // consumes it on terminal success). - twoFAFallbackUsed := false - if bookingUserID.Valid { - var gateOK bool - gateOK, twoFAFallbackUsed = requireTwoFactorForCardAccess(w, r, service, bookingUserID.String, req.VerificationCode, terminalVerificationToken, !reusePendingRecord) - if !gateOK { - return - } - // C6: a fallback-authorized charge must carry the customer's - // accepted consent (403 consent_required otherwise) — the server-side - // guard that stops a 2FA-path charge reaching Square without it. - if !enforceSCAFallbackConsent(w, req.ConsentVersion, req.ConsentAccepted, twoFAFallbackUsed) { + // scaTokenizedSavedCard (an SCA tokenize-result token charging a saved + // card) skips the 2FA gate exactly like a present verification_token: + // the token only exists after the issuer completed buyer verification for + // this card + amount (SCA-primary), so no homegrown fallback + // authorization is needed. + scaTokenizedSavedCard := req.NewCardToken != nil && *req.NewCardToken != "" + if bookingUserID.Valid && !scaTokenizedSavedCard { + if gateOK, _ := requireTwoFactorForCardAccess(w, r, service, bookingUserID.String, terminalVerificationToken, !reusePendingRecord); !gateOK { return } } @@ -1261,16 +1226,6 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { paymentResult, err := SquareClient.CreatePayment(r.Context(), paymentReq) if err != nil { log.Printf("Failed to process saved-card payment: %v (error_code=%q)", err, square.ErrorCode(err)) - // The gate consumed the 2FA code for a fresh saved-card charge — - // re-issue so the same-key retry has a live code to verify - // (mirrors CreateBookingPayment's post-failure re-issue, finding - // 4). Only runs for a FRESH charge whose gate consumed a code - // (!reusePendingRecord): a pending-reuse retry verified WITHOUT - // consuming, so its code is still live and re-issuing would - // silently invalidate the one the customer holds. - if bookingUserID.Valid { - reissueTwoFACodeAfterFailedCharge(r.Context(), db.Conn, bookingUserID.String, true, twoFAFallbackUsed && !reusePendingRecord, r) - } // SCA-required failures (Square demands buyer verification) must // surface the structured verification_required body so the frontend // triggers the 3DS challenge instead of treating the payment as a @@ -1392,11 +1347,6 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { "card_last4": paymentResult.CardLast4, "square_payment_id": paymentResult.SquarePayID, }) - // The 2FA BACKUP authorized this token-less saved-card charge - // (SCA was unavailable) — record the strict fallback audit row. - if twoFAFallbackUsed { - insertTwoFAFallbackAudit(r.Context(), adminID, bookingUserID.String, paymentResult.CardLast4, bookingID, "admin saved-card charge authorized via 2FA fallback (SCA unavailable)", consentVersionValue(req.ConsentVersion), req.ConsentAccepted) - } } // F6: a fully-paid saved-card charge completes the booking exactly like @@ -1985,7 +1935,7 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { return } - if err := ValidateCardInfo(req.CardID, req.NewCardToken); err != nil { + if err := ValidateCardInfo(req.CardID, req.UserSavedCardID, req.NewCardToken); err != nil { log.Printf("Failed to process request: %v", err) http.Error(w, "Invalid request", http.StatusBadRequest) return @@ -2041,8 +1991,9 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { // same user_saved_cards.id; card_id wins when both are sent). When a // NEW-card token arrives alongside it (SCA tokenize-result wire contract), // the token is the one-time charge source and this row supplies the - // customer. ValidateCardInfo above already rejected card_id + new_card_token - // together, so a coexistence can only be new_card_token + saved_card_id. + // customer. ValidateCardInfo above already validated the three legal card + // source shapes (saved-card ref alone, token alone, or ref + token as the + // SCA tokenize-result source). savedCardRef := req.CardID if savedCardRef == nil || *savedCardRef == "" { savedCardRef = req.UserSavedCardID @@ -2280,21 +2231,17 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { // 2FA gating (C5): persisting a card requires 2FA when the feature is // enforced. This runs AFTER the idempotency dedup's completed // short-circuit (Loop B MEDIUM): a same-key lost-response retry returns the - // already-completed payment above without re-entering the gate, so its - // single-use code (already consumed by the original attempt) is never - // re-rejected as "expired". Pending-reuse and fresh paths still gate — a - // new charge may move at Square. The gate also runs before - // resolveChargeSource below, so an un-2FA'd request never persists a card. + // already-completed payment above without re-entering the gate. Pending-reuse + // and fresh paths still gate — a new charge may move at Square. The gate + // also runs before resolveChargeSource below, so an un-2FA'd request never + // persists a card. // SCA-primary (auth-F1): the SAVE surface never forwards the client's // verification_token to Square (the card is persisted via CreateCardOnFile, // which takes no token), so a non-empty token is client-asserted and must - // NOT skip the gate — a token-less save falls back to the customer's 2FA - // code, and twoFAFallbackUsed records that the 2FA BACKUP authorized the - // operation (the caller audits it). Only the call-site's - // scaTokenizedSavedCard tokenize-result flow skips the save gate (the - // combined path never persists a card and Square validates the token as the - // source_id). - twoFAFallbackUsed := false + // NOT skip the gate — a token-less save is refused 402 (SCA-only). Only the + // call-site's scaTokenizedSavedCard tokenize-result flow skips the save gate + // (the combined path never persists a card and Square validates the token as + // the source_id). bookingVerificationToken := "" if req.VerificationToken != nil { bookingVerificationToken = *req.VerificationToken @@ -2308,14 +2255,7 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { // a card (resolveChargeSource uses the token as a one-time source, no // card-on-file is created). if req.SaveCard && !scaTokenizedSavedCard { - var gateOK bool - gateOK, twoFAFallbackUsed = requireTwoFactorForCardAccessWithTokenValidation(w, r, service, userID, req.VerificationCode, bookingVerificationToken, true, false) - if !gateOK { - return - } - // C6: a fallback-authorized save/charge must carry the customer's - // accepted consent (403 consent_required otherwise). - if !enforceSCAFallbackConsent(w, req.ConsentVersion, req.ConsentAccepted, twoFAFallbackUsed) { + if gateOK, _ := requireTwoFactorForCardAccessWithTokenValidation(w, r, service, userID, bookingVerificationToken, true, false); !gateOK { return } } @@ -2488,6 +2428,25 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { }) return } + // maxOnlineTipPence cap (the £250 bound the dedicated tip endpoint + // enforces): the portion buildSplitRecords will actually carve as a + // payment_type='tip' row is the excess beyond the booking's REAL + // remaining obligation (TotalAmount - TotalPaid, which excludes + // discount rows) — the post-start carve and the pre-start + // deposit/balance carve both compute tipPortion = + // paymentAmount - realRemaining. The deposit-with-discount + // overflowThreshold compared above is only the CONFIRMATION + // trigger (a raw deposit between the discounted obligation and the + // real remaining needs the flag but carves no tip); capping the + // real tip portion keeps every minted tip row within + // maxOnlineTipPence. Without this, a confirmed £10,000 payment on + // a booking with £50 remaining would mint a £9,950 tip row — far + // over the cap and unreachable by the tip-refund path. + if tipPortion := req.Amount - remainingPence; tipPortion > maxOnlineTipPence { + log.Printf("Overflow tip rejected: requested %d exceeds obligation %d for booking %s — the %d pence tip portion exceeds the £250 online tip cap (discount credit %d pence)", req.Amount, overflowThreshold, bookingID, tipPortion, eligibleDiscountPence) + http.Error(w, "Tip exceeds the maximum allowed amount (£250)", http.StatusBadRequest) + return + } chargeAmount = req.Amount log.Printf("Overflow accepted as tip: requested %d exceeds obligation %d for booking %s (discount credit %d pence, confirmed=%v)", req.Amount, overflowThreshold, bookingID, eligibleDiscountPence, req.ConfirmOverflowTip) } @@ -2593,26 +2552,12 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { var savedCardID *string var savedCardCustomerID string // 2FA gating (C5): charging a SAVED card requires 2FA when the feature is - // enforced. New-card (nonce) charges are not gated. consume=!reusePendingRecord - // (LOW 6a): a FRESH charge verifies WITH consumption — the code is single-use - // at the gate, closing the TOCTOU where a verified-but-unconsumed code could - // authorize a second charge within its lifetime — and a failed Square charge - // re-issues a fresh code (reissueTwoFACodeAfterFailedCharge). A pending-reuse - // retry verifies WITHOUT consuming: the code was re-issued for exactly this - // retry and the completed-charge transaction consumes it on terminal success, - // so a retry that fails again keeps its code for one more attempt. A charge - // carrying a Square verification_token (SCA performed) skips the gate; a - // token-less charge falls back to 2FA and twoFAFallbackUsed is set for the - // charge-success audit. + // enforced. New-card (nonce) charges are not gated. A charge carrying a + // Square verification_token (SCA performed) skips the gate; a token-less + // charge is refused 402 verification_required (SCA-only — the homegrown 2FA + // fallback was removed). if savedCardRef != nil && *savedCardRef != "" && !scaTokenizedSavedCard { - var gateOK bool - gateOK, twoFAFallbackUsed = requireTwoFactorForCardAccess(w, r, service, userID, req.VerificationCode, bookingVerificationToken, !reusePendingRecord) - if !gateOK { - return - } - // C6: a fallback-authorized charge must carry the customer's accepted - // consent (403 consent_required otherwise). - if !enforceSCAFallbackConsent(w, req.ConsentVersion, req.ConsentAccepted, twoFAFallbackUsed) { + if gateOK, _ := requireTwoFactorForCardAccess(w, r, service, userID, bookingVerificationToken, !reusePendingRecord); !gateOK { return } } @@ -2733,21 +2678,6 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { paymentResult, err := SquareClient.CreatePayment(r.Context(), paymentReq) if err != nil { log.Printf("Failed to create payment: %v (error_code=%q)", err, square.ErrorCode(err)) - // The gate consumed the 2FA code for a fresh saved-card charge — - // re-issue so the same-key retry has a live code to verify. A NEW-CARD - // (cnon) charge with save_card=false never gated and involves no code: - // re-issuing there would overwrite the customer's standing pending code - // with a fresh undelivered one, silently burning the code the operator - // relayed (finding 5). But a NEW-CARD charge with save_card=true DID - // gate — the SAVE gate above consumed the code before the charge — so - // the code must be re-issued there too or every retry hits "Verification - // code expired" forever (finding: save-gate burned code never re-issued). - // A pending-reuse retry verified WITHOUT consuming, so its code is still - // live and no re-issue runs (a re-issue would invalidate the code the - // customer already holds). - if (savedCardRef != nil && *savedCardRef != "") || req.SaveCard { - reissueTwoFACodeAfterFailedCharge(r.Context(), db.Conn, userID, true, twoFAFallbackUsed && !reusePendingRecord, r) - } // SCA-required failures must surface the structured verification_required // body so the frontend triggers the 3DS challenge, not a plain decline. if isVerificationRequiredError(err) { @@ -2843,7 +2773,13 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { var records []PaymentRecord if bErr == nil && bookingInfo != nil { - records = buildSplitRecords(primaryRecord, req.PaymentType, bookingInfo, paymentAmount) + var splitErr error + records, splitErr = buildSplitRecords(primaryRecord, req.PaymentType, bookingInfo, paymentAmount) + if splitErr != nil { + log.Printf("CRITICAL: Square payment %s (ID=%s) was processed but buildSplitRecords rejected the split for booking %s: %v — the pending record %s stays for the stale-pending sweep; manual reconciliation required", paymentResult.Status, paymentResult.SquarePayID, bookingID, splitErr, paymentID) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } } else { if bErr != nil { log.Printf("Failed to get booking info for split: %v — using single record", bErr) @@ -2967,15 +2903,6 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { return } - // The 2FA BACKUP authorized this token-less saved-card charge (SCA was - // unavailable) — record the strict fallback audit row AFTER the money - // transaction commits (a failed audit write must never roll back a - // completed charge). The actor is the customer's own userID - // (customer-initiated online charge). - if twoFAFallbackUsed { - insertTwoFAFallbackAudit(r.Context(), userID, userID, paymentResult.CardLast4, bookingID, "saved-card charge authorized via 2FA fallback (SCA unavailable)", consentVersionValue(req.ConsentVersion), req.ConsentAccepted) - } - // B13: a campaign was exhausted between the preview and the apply-time // re-check. The charge already succeeded at Square and the payment record // is committed, so the customer's promised discount must not silently @@ -3200,7 +3127,7 @@ func applyEligibleCampaignsAtPayment(ctx context.Context, q db.Querier, bookingI // over-allocate toward balance and under-allocate toward tip (a bookkeeping // simplification, not an overcharge) — the partition still equals the charged // amount. See TestBuildSplitRecords_DiscountBooking_TipOverflow_SumNeverExceedsCharge. -func buildSplitRecords(primary PaymentRecord, reqPaymentType string, info *BookingPaymentInfo, paymentAmount float64) []PaymentRecord { +func buildSplitRecords(primary PaymentRecord, reqPaymentType string, info *BookingPaymentInfo, paymentAmount float64) ([]PaymentRecord, error) { // After the booking starts there is no deposit protection window, but an // overpayment beyond the remaining booking value is still gratuity and must // be carved out as its own payment_type='tip' record (F3) — mirroring @@ -3215,6 +3142,15 @@ func buildSplitRecords(primary PaymentRecord, reqPaymentType string, info *Booki bookingPortion = math.Round(bookingPortion*100) / 100 tipPortion := math.Round((paymentAmount-bookingPortion)*100) / 100 if tipPortion > roundingEpsilon { + // Belt-and-braces: the online tip bound (maxOnlineTipPence) applies + // to EVERY tip row — including overflow tips carved from a booking + // charge. The B12 gate enforces it before the charge is taken; this + // check catches any future caller that skips the gate. Returning an + // error (never clamping) preserves the partition invariant: the + // records must always sum to the charged paymentAmount. + if tipPence := int64(math.Round(tipPortion * 100)); tipPence > maxOnlineTipPence { + return nil, fmt.Errorf("buildSplitRecords: post-start carve for booking %s would mint a tip of %d pence, exceeding the £250 online tip cap (charge %.2f)", primary.BookingID, tipPence, paymentAmount) + } records := []PaymentRecord{primary} records[0].Amount = bookingPortion tip := primary @@ -3225,9 +3161,9 @@ func buildSplitRecords(primary PaymentRecord, reqPaymentType string, info *Booki k := *primary.IdempotencyKey + "-split-tip" tip.IdempotencyKey = &k } - return append(records, tip) + return append(records, tip), nil } - return []PaymentRecord{primary} + return []PaymentRecord{primary}, nil } // 1. Deposit portion: up to 50% of total, minus what's already been paid. @@ -3247,6 +3183,18 @@ func buildSplitRecords(primary PaymentRecord, reqPaymentType string, info *Booki // 4. Tip: anything beyond the booking total. tipPortion := math.Round((remainingAfterDeposit-balancePortion)*100) / 100 + // Belt-and-braces (same bound as the post-start carve and the dedicated + // tip endpoint): the pre-start deposit/balance/tip carve can mint a tip + // row when the payment exceeds the booking's REAL remaining (e.g. a + // deposit-with-discount overflow). The B12 gate enforces the cap before + // the charge; this check catches any future caller that skips the gate. + // Never clamp — the records must partition the charged amount exactly. + if tipPortion > roundingEpsilon { + if tipPence := int64(math.Round(tipPortion * 100)); tipPence > maxOnlineTipPence { + return nil, fmt.Errorf("buildSplitRecords: pre-start carve for booking %s would mint a tip of %d pence, exceeding the £250 online tip cap (charge %.2f)", primary.BookingID, tipPence, paymentAmount) + } + } + var records []PaymentRecord splitIdx := 0 @@ -3308,7 +3256,7 @@ func buildSplitRecords(primary PaymentRecord, reqPaymentType string, info *Booki primary.Fees = 0 records = append(records, primary) } - return records + return records, nil } // buildTerminalSplitRecords splits a completed terminal checkout charge that @@ -3462,10 +3410,6 @@ func DeletePaymentMethod(w http.ResponseWriter, r *http.Request) { type CreatePaymentMethodRequest struct { CardToken string `json:"card_token" validate:"required"` - // VerificationCode is the customer's current 2FA one-time code (B10): an - // enforced environment persists a card only when this matches the - // customer's pending code. - VerificationCode string `json:"verification_code,omitempty"` // VerificationToken is a Square 3DS/SCA verification token. On the add-card // SAVE surface it is CLIENT-ASSERTED and never forwarded to Square // (CreateCardOnFile takes no verification_token), so the 2FA gate IGNORES it @@ -3473,8 +3417,9 @@ type CreatePaymentMethodRequest struct { // carried on the wire for parity with the charge surfaces and passed through // to the gate, whose save variant applies the token-less SCA-only refusal // when a genuine SCA proof is absent. The genuine proof for a SAVE is the - // STORE-intent tokenize-result submitted as card_token (see - // isSCATokenizeResultCardToken). + // token itself — any Square token-like card token (cnon:/ccof:) was minted + // by a tokenization flow that ran the STORE-intent SCA, so it is treated as + // SCA-proven and the gate is skipped (see CreatePaymentMethod). VerificationToken *string `json:"verification_token,omitempty"` // ConsentVersion / ConsentAccepted mirror the charge structs (C6): the // add-card endpoint never charges, so they are recorded on the fallback @@ -3513,30 +3458,27 @@ func CreatePaymentMethod(w http.ResponseWriter, r *http.Request) { // token itself. The frontend performs the STORE-intent SCA at tokenization // (SquareCardInput.tokenizeForStore) — the tokenize-result IS the card // token, and Square validates it as a card-on-file source when - // CreateCardOnFile persists it. A save that carries a genuine SCA - // tokenize-result as card_token is therefore SCA-compliant and skips the - // 2FA gate at the call site — exactly like the charge surfaces' - // scaTokenizedSavedCard skip: the STORE-intent SCA performed at tokenization - // IS the verification (PSR 2017 reg 100 compliant), so no homegrown fallback - // authorization is needed and no fallback audit is written. + // CreateCardOnFile persists it. Any genuine Square token-like card token + // (a cnon: nonce or ccof: card id — the only shapes the mock's + // CreateCardOnFile and real Square accept, plus the dev mock's + // verify_mock_ transition shape) was minted by a tokenization flow, so it is + // treated as SCA-proven and the homegrown gate is skipped; Square's own + // acceptance of the token is the authoritative check. The fictional + // cnon:sca- marker requirement is gone — real Square never produces it, so + // it made every save 402 in an enforced deployment. // - // Every other save stays gated through the save-surface variant - // (tokenForwardedToSquare=false): a verification_token is client-asserted - // and NEVER forwarded to Square on a SAVE surface, so it must NOT skip the - // gate (auth-F1 — a forged value cannot authorise a save, and no 2FA code - // can either, SCA-only). A token-less legacy save (a raw card.tokenize() - // nonce, or a token the backend cannot recognize as a genuine SCA - // tokenize-result) is refused 402 verification_required in an enforced - // deployment. + // A non-token-like value (a raw PAN or any other shape) fails closed + // through the save-surface variant (tokenForwardedToSquare=false): a + // verification_token is client-asserted and NEVER forwarded to Square on a + // SAVE surface, so it must NOT skip the gate (auth-F1 — a forged value + // cannot authorise a save, and no 2FA code can either, SCA-only). It is + // refused 402 verification_required in an enforced deployment. saveVerificationToken := "" if req.VerificationToken != nil { saveVerificationToken = *req.VerificationToken } - twoFAFallbackUsed := false - if !isSCATokenizeResultCardToken(req.CardToken) { - var gateOK bool - gateOK, twoFAFallbackUsed = requireTwoFactorForCardAccessWithTokenValidation(w, r, service, userID, req.VerificationCode, saveVerificationToken, true, false) - if !gateOK { + if !isTokenLikeSaveSource(req.CardToken) { + if gateOK, _ := requireTwoFactorForCardAccessWithTokenValidation(w, r, service, userID, saveVerificationToken, true, false); !gateOK { return } } @@ -3553,31 +3495,19 @@ func CreatePaymentMethod(w http.ResponseWriter, r *http.Request) { return } - // The 2FA BACKUP authorized persisting this card (no SCA was performed on - // the add-card endpoint); the actor is the customer's own userID. Unreachable - // under SCA-only (the gate never sets fallbackUsed), retained for parity - // with the charge surfaces. - if twoFAFallbackUsed && card != nil { - insertTwoFAFallbackAudit(r.Context(), userID, userID, card.Last4, "", "card persisted via 2FA fallback (SCA unavailable)", consentVersionValue(req.ConsentVersion), req.ConsentAccepted) - } - if err := json.NewEncoder(w).Encode(card); err != nil { log.Printf("Failed to encode JSON response: %v", err) } } -// isSCATokenizeResultCardToken reports whether a card token submitted to the -// add-card save surface is a GENUINE Square tokenizeWithVerification result — -// the STORE-intent tokenize-result the account page's tokenizeForStore -// produces (the SCA challenge runs at tokenization, so the returned token is -// the SCA-verified source the backend stores). Real Square returns opaque cnon: -// tokens, so the dev mock's contract marks a genuine tokenize-result with -// "sca-" immediately after the cnon: prefix (internal/square's -// isSCATokenizeResultSource) — the enforcement-parity stand-in the charge -// paths also rely on (money-F2). A raw card.tokenize() nonce or any other -// shape is NOT an SCA proof and never skips the gate. -func isSCATokenizeResultCardToken(cardToken string) bool { - return strings.HasPrefix(cardToken, "cnon:sca-") +// isTokenLikeSaveSource reports whether a card token submitted to the add-card +// save surface is a genuine Square token-like source — the shapes the mock's +// CreateCardOnFile accepts (cnon: nonces and ccof: card ids, plus the dev +// mock's verify_mock_ transition shape), which real Square only mints after a +// tokenization flow ran the STORE-intent SCA. Anything else (a raw PAN, an +// arbitrary string) is not token-like and fails closed through the 2FA gate. +func isTokenLikeSaveSource(cardToken string) bool { + return strings.HasPrefix(cardToken, "cnon:") || strings.HasPrefix(cardToken, "ccof:") || strings.HasPrefix(cardToken, "verify_mock_") } // isDefinitiveCardSaveFailure reports whether a CreatePaymentMethod error is a @@ -4729,30 +4659,18 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { service := NewPaymentService() - // 2FA gating (C5): persisting a card requires 2FA when the feature is enforced. - // consume=true: saving a card is a terminal operation (the card row is - // created right here), so the verified code is single-use immediately — - // unlike the saved-card CHARGE gate below, which defers consumption to the - // charge's terminal success (MEDIUM-2). SCA-primary (auth-F1): the SAVE - // surface never forwards the client's verification_token to Square (the - // card is persisted via CreateCardOnFile, which takes no token), so a - // non-empty token is client-asserted and must NOT skip the gate — a - // token-less save falls back to 2FA and twoFAFallbackUsed is set for the - // charge-success audit. - twoFAFallbackUsed := false + // 2FA gating (C5): persisting a card requires 2FA when the feature is + // enforced. SCA-primary (auth-F1): the SAVE surface never forwards the + // client's verification_token to Square (the card is persisted via + // CreateCardOnFile, which takes no token), so a non-empty token is + // client-asserted and must NOT skip the gate — a token-less save is refused + // 402 (SCA-only). tipVerificationToken := "" if req.VerificationToken != nil { tipVerificationToken = *req.VerificationToken } if req.SaveCard { - var gateOK bool - gateOK, twoFAFallbackUsed = requireTwoFactorForCardAccessWithTokenValidation(w, r, service, userID, req.VerificationCode, tipVerificationToken, true, false) - if !gateOK { - return - } - // C6: a fallback-authorized save/charge must carry the customer's - // accepted consent (403 consent_required otherwise). - if !enforceSCAFallbackConsent(w, req.ConsentVersion, req.ConsentAccepted, twoFAFallbackUsed) { + if gateOK, _ := requireTwoFactorForCardAccessWithTokenValidation(w, r, service, userID, tipVerificationToken, true, false); !gateOK { return } } @@ -4763,7 +4681,20 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { return } - if err := ValidateCardInfo(req.CardID, req.NewCardToken); err != nil { + // maxOnlineTipPence bound: ValidateAmount's generic £10,000 cap is the + // money-minting ceiling for booking charges, but a tip is gratuity on a + // percentage of the service — a single online tip over £250 is not a + // legitimate business transaction (the till's embedded-gratuity bound is + // only £50). Reject BEFORE any charge-source resolution, idempotency + // handling, or Square call, so no pending record or charge is ever minted + // for an over-bound tip. + if req.Amount > maxOnlineTipPence { + log.Printf("Tip rejected: booking %s requested a tip of %d pence which exceeds the £250 online tip cap", bookingID, req.Amount) + http.Error(w, "Tip exceeds the maximum allowed amount (£250)", http.StatusBadRequest) + return + } + + if err := ValidateCardInfo(req.CardID, nil, req.NewCardToken); err != nil { log.Printf("Failed to process request: %v", err) http.Error(w, "Invalid request", http.StatusBadRequest) return @@ -4858,14 +4789,17 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { // Serialize tip attempts for this booking to prevent concurrent duplicate // tip payments across browser tabs or retries. Uses a PostgreSQL session-level - // advisory lock scoped to the booking ID. + // advisory lock scoped to the booking ID. Shares the crussell:payment: + // key with the booking-payment handlers so a tip cannot race an in-flight + // payment on the same booking (they don't nest — no handler re-acquires + // this lock while holding it — so sharing the key cannot deadlock). // Bounded try-lock (R6) so a contended lock never blocks the pool across // the Square round-trip. - pinConn, lockOK := acquireBookingPaymentLock(r.Context(), w, "crussell:tip:"+bookingID, "Payment in progress, try again") + pinConn, lockOK := acquireBookingPaymentLock(r.Context(), w, "crussell:payment:"+bookingID, "Payment in progress, try again") if !lockOK { return } - defer releaseBookingPaymentLock(pinConn, "crussell:tip:"+bookingID) + defer releaseBookingPaymentLock(pinConn, "crussell:payment:"+bookingID) // Step 1: Insert payment record in 'pending' state inside a DB transaction. // Square is NOT called yet — if the tx fails, no harm done. @@ -4984,25 +4918,11 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { // enforced. New-card (nonce) charges are not gated. This runs AFTER the // idempotency dedup's completed short-circuit (Loop B MEDIUM): a same-key // lost-response retry returns the already-completed payment above without - // re-entering the gate, so its single-use code (already consumed by the - // original attempt) is never re-rejected as "expired". consume=!reusePendingRecord - // (finding 4): a FRESH charge verifies WITH consumption — the code is - // single-use at the gate, closing the TOCTOU where a verified-but- - // unconsumed code could authorize a second charge — and a pending-reuse - // retry verifies WITHOUT consuming, so a retry that fails again keeps its - // code for one more attempt (the completed-charge transaction consumes it - // on terminal success). A charge carrying a Square verification_token (SCA - // performed) skips the gate; a token-less charge falls back to 2FA and - // twoFAFallbackUsed is set for the charge-success audit. + // re-entering the gate. A charge carrying a Square verification_token (SCA + // performed) skips the gate; a token-less charge is refused 402 + // verification_required (SCA-only — the homegrown 2FA fallback was removed). if req.CardID != nil && *req.CardID != "" { - var gateOK bool - gateOK, twoFAFallbackUsed = requireTwoFactorForCardAccess(w, r, service, userID, req.VerificationCode, tipVerificationToken, !reusePendingRecord) - if !gateOK { - return - } - // C6: a fallback-authorized charge must carry the customer's accepted - // consent (403 consent_required otherwise). - if !enforceSCAFallbackConsent(w, req.ConsentVersion, req.ConsentAccepted, twoFAFallbackUsed) { + if gateOK, _ := requireTwoFactorForCardAccess(w, r, service, userID, tipVerificationToken, !reusePendingRecord); !gateOK { return } } @@ -5103,18 +5023,6 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { if err != nil { log.Printf("Failed to create tip payment: %v (error_code=%q)", err, square.ErrorCode(err)) // Payment record intentionally left as 'pending' for manual retry. - // The gate consumed the 2FA code for a fresh saved-card charge — - // re-issue so the same-key retry has a live code to verify (mirrors - // CreateBookingPayment's post-failure re-issue, finding 4). A NEW-CARD - // (cnon) charge never gated and involves no code — re-issuing would - // overwrite a standing pending code with an undelivered one (finding - // 5). A NEW-CARD charge with save_card=true DID gate (the SAVE gate - // consumed the code), so the code is re-issued there too (finding: - // save-gate burned code never re-issued). A pending-reuse retry verified - // WITHOUT consuming, so its code is still live and no re-issue runs. - if (req.CardID != nil && *req.CardID != "") || req.SaveCard { - reissueTwoFACodeAfterFailedCharge(r.Context(), db.Conn, userID, true, twoFAFallbackUsed && !reusePendingRecord, r) - } // SCA-required failures must surface the structured verification_required // body so the frontend triggers the 3DS challenge, not a plain decline. if isVerificationRequiredError(err) { @@ -5187,13 +5095,6 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { return } - // The 2FA BACKUP authorized this token-less saved-card tip charge (SCA was - // unavailable) — record the strict fallback audit row. The actor is the - // customer's own userID (customer-initiated online charge). - if twoFAFallbackUsed { - insertTwoFAFallbackAudit(r.Context(), userID, userID, paymentResult.CardLast4, bookingID, "saved-card tip charge authorized via 2FA fallback (SCA unavailable)", consentVersionValue(req.ConsentVersion), req.ConsentAccepted) - } - if err := json.NewEncoder(w).Encode(PaymentResponse{ ID: paymentID, BookingID: bookingID, diff --git a/backend/handlers/payments/handlers_test.go b/backend/handlers/payments/handlers_test.go index 494ef70..cb42008 100644 --- a/backend/handlers/payments/handlers_test.go +++ b/backend/handlers/payments/handlers_test.go @@ -437,9 +437,10 @@ func TestTwoFactorEnforced_CreatePaymentMethod_SCATokenizeResult_Save_Succeeds(t // TestTwoFactorEnforced_CreatePaymentMethod_ForgeVerificationToken_402 pins // auth-F1 on the add-card surface: a verification_token is CLIENT-ASSERTED and // never forwarded to Square on a SAVE surface (CreateCardOnFile takes no token), -// so it must NOT skip the gate — enforced + a raw nonce card_token + a (forged) -// non-empty verification_token is refused 402 verification_required and no card -// is persisted. The M11 fix cannot be a client-asserted token bypass. +// so it must NOT skip the gate — enforced + a NON-token-like card_token (a raw +// PAN — the only shape the gate still refuses) + a (forged) non-empty +// verification_token is refused 402 verification_required and no card is +// persisted. The M11 fix cannot be a client-asserted token bypass. func TestTwoFactorEnforced_CreatePaymentMethod_ForgeVerificationToken_402(t *testing.T) { helperEnvEnforce2FAStaging(t) ctx, tx := testutils.SetupTestTx(t) @@ -450,7 +451,7 @@ func TestTwoFactorEnforced_CreatePaymentMethod_ForgeVerificationToken_402(t *tes forged := "forged-verification-token" req := CreatePaymentMethodRequest{ - CardToken: "cnon:2fa-forged-save", + CardToken: "4111111111111111", VerificationToken: &forged, } w := makePaymentRequest(CreatePaymentMethod, "POST", "/api/user/payment-methods", req, token, ctx) @@ -466,10 +467,14 @@ func TestTwoFactorEnforced_CreatePaymentMethod_ForgeVerificationToken_402(t *tes } // TestTwoFactorEnforced_CreatePaymentMethod_Tokenless_Refused402 pins the M11 -// refusal half in the SCA suite's own staging helper: a legacy token-less save -// (a raw card.tokenize() nonce carrying no SCA proof) is refused 402 -// verification_required in an enforced deployment and no card is persisted. -// Companion to errors_test.go's TestTwoFactorEnforced_CreatePaymentMethod_Tokenless_402. +// refusal half in the SCA suite's own staging helper: a NON-token-like save +// source (a raw PAN — the gate's only remaining refusal shape) is refused 402 +// verification_required in an enforced deployment and no card is persisted. A +// GENUINE token-like source (cnon:/ccof:) is SCA-proven — the STORE-intent SCA +// ran at tokenization — and skips the gate (see +// TestTwoFactorEnforced_CreatePaymentMethod_SCATokenizeResult_Save_Succeeds). +// Companion to TestTwoFactorEnforced_CreatePaymentMethod_Tokenless_402 +// (errors_test.go). func TestTwoFactorEnforced_CreatePaymentMethod_Tokenless_Refused402(t *testing.T) { helperEnvEnforce2FAStaging(t) ctx, tx := testutils.SetupTestTx(t) @@ -478,7 +483,7 @@ func TestTwoFactorEnforced_CreatePaymentMethod_Tokenless_Refused402(t *testing.T require.NoError(t, err) token := jwt.GenerateUserToken(userID) - req := CreatePaymentMethodRequest{CardToken: "cnon:2fa-tokenless-save"} + req := CreatePaymentMethodRequest{CardToken: "4111111111111111"} w := makePaymentRequest(CreatePaymentMethod, "POST", "/api/user/payment-methods", req, token, ctx) require.Equal(t, http.StatusPaymentRequired, w.Code, "a token-less save must be refused 402 in an enforced deployment, body: %s", w.Body.String()) diff --git a/backend/handlers/payments/loop_b_money_fixes_test.go b/backend/handlers/payments/loop_b_money_fixes_test.go index b31126e..68ce852 100644 --- a/backend/handlers/payments/loop_b_money_fixes_test.go +++ b/backend/handlers/payments/loop_b_money_fixes_test.go @@ -165,11 +165,16 @@ func TestBookingPayment_DepositCoveredDiscount_RecordsDiscount_NoOvercharge(t *t assert.Equal(t, 50.00, discountAmount, "the promised £50 discount must be recorded at the skip path") // Ledger: real £50 + discount £50 = £100 = total. The customer pays the - // discounted £50, never the full £100. + // discounted £50, never the full £100. GetBookingRemainingBalancePence + // counts only REAL money — a discount row is a ledger entry, not a payment + // toward the balance (the real-money convention every other paid + // computation applies) — so the remaining balance reports the full £50 even + // though the booking auto-completed (bookingIsFullyPaid counts the discount + // row toward completion). var remainingPence int64 remainingPence, err = NewPaymentService().GetBookingRemainingBalancePence(ctx, bookingID) require.NoError(t, err) - assert.Equal(t, int64(0), remainingPence, "the discounted booking must have £0 remaining after the covered deposit") + assert.Equal(t, int64(5000), remainingPence, "the remaining balance counts real money only — the discount row is not 'paid'") // A later unconfirmed balance charge of £50 must be REJECTED — the booking // auto-completed when the deposit + discount settled it, so the completed- diff --git a/backend/handlers/payments/loyalty.go b/backend/handlers/payments/loyalty.go index 586446e..477db23 100644 --- a/backend/handlers/payments/loyalty.go +++ b/backend/handlers/payments/loyalty.go @@ -57,7 +57,14 @@ func ApplyLoyaltyRedemption(w http.ResponseWriter, r *http.Request) { http.Error(w, "internal server error", http.StatusInternalServerError) return } - if bookingUserID != userID { + // Ownership: the booking's own customer may redeem their pending + // redemption, and an admin acting on ANY booking may too (the admin + // "Take Payment" PaymentModal applies the customer's redemption on their + // behalf — the admin route /admin/bookings/{id}/apply-redemption mounts + // this same handler under RequireAdmin). All the writes below target the + // BOOKING's user (bookingUserID), never the acting admin. + userRole, _ := r.Context().Value(mw.UserRoleKey).(string) + if userRole != "admin" && bookingUserID != userID { http.Error(w, "Unauthorized", http.StatusForbidden) return } @@ -148,7 +155,7 @@ func ApplyLoyaltyRedemption(w http.ResponseWriter, r *http.Request) { if _, err := tx.Exec(r.Context(), ` 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, 'loyalty', $3, NULL, NULL, $6, $4, $5) - `, bookingID, userID, redemptionID, bookingTotal, discountAmount, LoyaltyDiscountPercent); err != nil { + `, bookingID, bookingUserID, redemptionID, bookingTotal, discountAmount, LoyaltyDiscountPercent); err != nil { log.Printf("Failed to insert booking discount: %v", err) http.Error(w, "internal server error", http.StatusInternalServerError) return @@ -157,7 +164,7 @@ func ApplyLoyaltyRedemption(w http.ResponseWriter, r *http.Request) { if _, err := tx.Exec(r.Context(), ` INSERT INTO payments (booking_id, payment_type, payment_method, amount, status, created_by) VALUES ($1, 'partial', 'discount', $2, 'completed', $3) - `, bookingID, discountAmount, userID); err != nil { + `, bookingID, discountAmount, bookingUserID); err != nil { log.Printf("Failed to insert payment record: %v", err) http.Error(w, "internal server error", http.StatusInternalServerError) return @@ -174,7 +181,7 @@ func ApplyLoyaltyRedemption(w http.ResponseWriter, r *http.Request) { if _, err := tx.Exec(r.Context(), ` UPDATE users SET loyalty_stamps = GREATEST(0, loyalty_stamps - $1) WHERE id = $2 - `, LoyaltyStampCost, userID); err != nil { + `, LoyaltyStampCost, bookingUserID); err != nil { log.Printf("Failed to update loyalty stamps: %v", err) http.Error(w, "internal server error", http.StatusInternalServerError) return diff --git a/backend/handlers/payments/max_online_tip_test.go b/backend/handlers/payments/max_online_tip_test.go new file mode 100644 index 0000000..5fd1fd5 --- /dev/null +++ b/backend/handlers/payments/max_online_tip_test.go @@ -0,0 +1,243 @@ +//go:build test && dev + +package payments + +import ( + "encoding/json" + "net/http" + "testing" + "time" + + "crussell/clock" + "crussell/testutils" + "crussell/testutils/fixtures" + "crussell/testutils/jwt" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// ============================================================================= +// maxOnlineTipPence: online tip business bound (CreateTipPayment) +// ============================================================================= + +// TestTipPayment_AtBound_Accepted pins the online tip bound's inclusive edge: a +// tip exactly at maxOnlineTipPence (£250) is a legitimate business amount and +// must flow through the normal happy path (charge, completed tip record). +func TestTipPayment_AtBound_Accepted(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestDataPast(t, ctx, tx) + + _, err := fixtures.CreateTestPayment(tx, bookingID, 5000.00, "online_square", "full", "completed") + require.NoError(t, err) + + userToken := jwt.GenerateUserToken(userID) + cardToken := "cnon:tip-at-bound" + req := CreateTipPaymentRequest{ + Amount: maxOnlineTipPence, + NewCardToken: &cardToken, + } + + handler := CreateTipPayment + w := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/tip", req, userToken, ctx) + + require.Equal(t, http.StatusOK, w.Code, "body: %s", w.Body.String()) + + var resp PaymentResponse + require.NoError(t, json.NewDecoder(w.Body).Decode(&resp)) + assert.Equal(t, "tip", resp.PaymentType) + assert.Equal(t, "completed", resp.Status) + assert.Equal(t, maxOnlineTipPence, resp.Amount, "the at-bound tip amount must be charged verbatim") +} + +// TestTipPayment_OverBound_Rejected pins the online tip bound's exclusive edge: +// any tip above maxOnlineTipPence (£250) is rejected with 400 and a clear +// message, and the rejection fires before any pending record insert or Square +// charge — no tip payment row may be created. +func TestTipPayment_OverBound_Rejected(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestDataPast(t, ctx, tx) + + _, err := fixtures.CreateTestPayment(tx, bookingID, 5000.00, "online_square", "full", "completed") + require.NoError(t, err) + + userToken := jwt.GenerateUserToken(userID) + cardToken := "cnon:tip-over-bound" + req := CreateTipPaymentRequest{ + Amount: maxOnlineTipPence + 1, + NewCardToken: &cardToken, + } + + handler := CreateTipPayment + w := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/tip", req, userToken, ctx) + + require.Equal(t, http.StatusBadRequest, w.Code, "body: %s", w.Body.String()) + assert.Contains(t, w.Body.String(), "Tip exceeds the maximum allowed amount") + + var tipCount int + err = tx.QueryRow(ctx, `SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND payment_type = 'tip'`, bookingID).Scan(&tipCount) + require.NoError(t, err) + assert.Equal(t, 0, tipCount, "an over-bound tip must not create any payment record") +} + +// ============================================================================= +// maxOnlineTipPence applies to the CreateBookingPayment OVERFLOW carve too: +// the B12 gate (confirm_overflow_tip) must cap the tip portion it mints at +// £250, mirroring the dedicated tip endpoint. A confirmed £10,000 payment on a +// booking with £50 remaining would otherwise carve a £9,950 tip row. +// ============================================================================= + +// TestBookingPayment_OverflowTip_OverCap_Rejected_PostStart is the confirmed +// bypass regression: a POST-START booking (the carve at handlers.go's +// buildSplitRecords post-start branch) with £50 remaining and a confirmed +// £10,000 payment must be rejected 400 with the tip-cap message — even though +// confirm_overflow_tip=true — and must not create any payment row. +func TestBookingPayment_OverflowTip_OverCap_Rejected_PostStart(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestDataPast(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + + cardToken := "cnon:overflow-over-cap-post" + req := CreateBookingPaymentRequest{ + Amount: 1000000, // £10,000 on a £50-remaining booking → £9,950 tip portion + PaymentType: "full", + NewCardToken: &cardToken, + IdempotencyKey: "overflow-over-cap-post-" + bookingID, + ConfirmOverflowTip: true, + } + + handler := CreateBookingPayment + w := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + + require.Equal(t, http.StatusBadRequest, w.Code, "an over-cap overflow must be rejected even when confirmed, body: %s", w.Body.String()) + assert.Contains(t, w.Body.String(), "Tip exceeds the maximum allowed amount") + + var payCount int + err := tx.QueryRow(ctx, `SELECT COUNT(*) FROM payments WHERE booking_id = $1`, bookingID).Scan(&payCount) + require.NoError(t, err) + assert.Zero(t, payCount, "the rejected over-cap overflow must not create any payment record") +} + +// TestBookingPayment_OverflowTip_OverCap_Rejected_PreStart is the same bypass +// regression for the PRE-START carve (deposit/balance/tip split): the +// confirmed £10,000 payment must be rejected before any pending row or Square +// charge, so no tip can be minted over the cap. +func TestBookingPayment_OverflowTip_OverCap_Rejected_PreStart(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestData(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + + cardToken := "cnon:overflow-over-cap-pre" + req := CreateBookingPaymentRequest{ + Amount: 1000000, // £10,000 on a £50-remaining booking → £9,950 tip portion + PaymentType: "full", + NewCardToken: &cardToken, + IdempotencyKey: "overflow-over-cap-pre-" + bookingID, + ConfirmOverflowTip: true, + } + + handler := CreateBookingPayment + w := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + + require.Equal(t, http.StatusBadRequest, w.Code, "an over-cap overflow must be rejected even when confirmed, body: %s", w.Body.String()) + assert.Contains(t, w.Body.String(), "Tip exceeds the maximum allowed amount") + + var payCount int + err := tx.QueryRow(ctx, `SELECT COUNT(*) FROM payments WHERE booking_id = $1`, bookingID).Scan(&payCount) + require.NoError(t, err) + assert.Zero(t, payCount, "the rejected over-cap overflow must not create any payment record") +} + +// TestBookingPayment_OverflowTip_WithinCap_CarvesTip pins the inclusive edge: +// a confirmed £250 payment on a £50-remaining booking (tip portion £200 — under +// the £250 cap) proceeds and buildSplitRecords carves a tip row of EXACTLY +// £200 alongside a £50 booking portion. +func TestBookingPayment_OverflowTip_WithinCap_CarvesTip(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestDataPast(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + + cardToken := "cnon:overflow-within-cap" + req := CreateBookingPaymentRequest{ + Amount: 25000, // £250 on a £50-remaining booking → tip portion £200 + PaymentType: "full", + NewCardToken: &cardToken, + IdempotencyKey: "overflow-within-cap-" + bookingID, + ConfirmOverflowTip: true, + } + + handler := CreateBookingPayment + w := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusOK, w.Code, "a within-cap overflow must proceed, body: %s", w.Body.String()) + + var tipCount int + var tipAmount float64 + err := tx.QueryRow(ctx, `SELECT COUNT(*), COALESCE(SUM(amount), 0) FROM payments WHERE booking_id = $1 AND status = 'completed' AND payment_type = 'tip'`, bookingID).Scan(&tipCount, &tipAmount) + require.NoError(t, err) + assert.Equal(t, 1, tipCount, "a within-cap overflow must carve exactly one tip record") + assert.InDelta(t, 200.0, tipAmount, 0.001, "the carved tip must equal the £200 overflow, not more") + + var bookingPortion float64 + err = tx.QueryRow(ctx, `SELECT COALESCE(SUM(amount), 0) FROM payments WHERE booking_id = $1 AND status = 'completed' AND payment_type = 'full'`, bookingID).Scan(&bookingPortion) + require.NoError(t, err) + assert.InDelta(t, 50.0, bookingPortion, 0.001, "the booking portion must remain the £50 obligation") +} + +// TestBuildSplitRecords_OverCapTip_Rejected proves the belt-and-braces cap in +// buildSplitRecords itself: even a caller that skips the B12 gate cannot mint a +// tip row over maxOnlineTipPence — both the post-start and pre-start carve +// return an error instead of minting the over-cap tip. A £10,000 charge on a +// £50 booking would otherwise carve a £9,950 tip row. +func TestBuildSplitRecords_OverCapTip_Rejected(t *testing.T) { + t.Parallel() + + record := makeTestRecord("b-cap-reject", "full", 10000) + info := &BookingPaymentInfo{ + StartTime: clock.Now().Add(-2 * time.Hour), + TotalAmount: 50, + TotalPaid: 0, + } + + // Post-start carve: booking portion £50, tip portion £9,950 — over the cap. + records, err := buildSplitRecords(record, "full", info, 10000) + require.Error(t, err, "the post-start carve must reject an over-cap tip portion") + assert.Contains(t, err.Error(), "exceeding the £250 online tip cap") + assert.Nil(t, records) + + // Pre-start carve: deposit £25 + balance £25, tip portion £9,950 — over the cap. + preInfo := &BookingPaymentInfo{ + StartTime: clock.Now().Add(48 * time.Hour), + TotalAmount: 50, + TotalPaid: 0, + } + records, err = buildSplitRecords(record, "full", preInfo, 10000) + require.Error(t, err, "the pre-start carve must reject an over-cap tip portion") + assert.Contains(t, err.Error(), "exceeding the £250 online tip cap") + assert.Nil(t, records) + + // Inclusive boundary: a £250 charge on the £50 booking (tip portion £200) + // stays below the cap and splits normally (single post-start record pair). + withinRecord := makeTestRecord("b-cap-within", "full", 250) + withinInfo := &BookingPaymentInfo{ + StartTime: clock.Now().Add(-2 * time.Hour), + TotalAmount: 50, + TotalPaid: 0, + } + within, err := buildSplitRecords(withinRecord, "full", withinInfo, 250) + require.NoError(t, err, "a within-cap tip portion must split normally") + require.Len(t, within, 2) + assert.Equal(t, "full", within[0].PaymentType) + assert.InDelta(t, 50.0, within[0].Amount, 0.001) + assert.Equal(t, "tip", within[1].PaymentType) + assert.InDelta(t, 200.0, within[1].Amount, 0.001) +} diff --git a/backend/handlers/payments/money_edge_test.go b/backend/handlers/payments/money_edge_test.go new file mode 100644 index 0000000..2cde21c --- /dev/null +++ b/backend/handlers/payments/money_edge_test.go @@ -0,0 +1,82 @@ +//go:build test && dev + +package payments + +import ( + "math" + "testing" + + "github.com/stretchr/testify/require" +) + +// TestPenceLess_RoundingBoundary pins the pence rounding that decides whether a +// gift-card clawback was PARTIAL: penceLess compares pound-float balances by +// rounding to integer pence, so sub-penny residue around roundingEpsilon +// (0.004) must not flip the comparison. Amounts at or below the epsilon (0.4 +// pence) round to zero pence; 0.5+ pence rounds up. +func TestPenceLess_RoundingBoundary(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + a, b float64 + less bool + }{ + {"both at the epsilon round to 0 pence — not less", 0.004, 0.004, false}, + {"0.004 vs 0.0041 both round to 0 pence", 0.004, 0.0041, false}, + {"0.0039 vs 0.004 both round to 0 pence", 0.0039, 0.004, false}, + {"0.0041 vs 0.0039 both round to 0 pence", 0.0041, 0.0039, false}, + {"0.004 (0 pence) is less than 0.005 (1 pence)", 0.004, 0.005, true}, + {"0.005 (1 pence) is NOT less than 0.004 (0 pence)", 0.005, 0.004, false}, + {"0.004 is less than 0.01 (1 pence)", 0.004, 0.01, true}, + {"equal amounts are never less", 12.34, 12.34, false}, + {"£12.34 is less than £12.35", 12.34, 12.35, true}, + {"£12.35 is NOT less than £12.34", 12.35, 12.34, false}, + {"a full pound difference", 0.0, 1.0, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + require.Equal(t, tt.less, penceLess(tt.a, tt.b), "penceLess(%v, %v)", tt.a, tt.b) + }) + } +} + +// TestPenceLess_FloatToPenceRoundingEdges pins the float64→pence rounding the +// comparison is built on: Go's math.Round rounds half away from zero, so +// exactly 0.5 pence rounds UP (0.005 → 1 pence) while anything below rounds +// down. The clawback's partial-detection must agree with this everywhere a +// pound-denominated balance is compared. +func TestPenceLess_FloatToPenceRoundingEdges(t *testing.T) { + t.Parallel() + + // Exactly 0.5 pence rounds up: 0.005 → 1p, so 0.005 < 0.006 is false + // (both round to 1) and 0.004999999 < 0.005000001 is true (0p vs 1p). + require.Equal(t, int64(1), int64(math.Round(0.005*100)), "0.005 pounds must round to 1 pence") + require.Equal(t, int64(0), int64(math.Round(0.004999999*100)), "0.004999999 pounds must round to 0 pence") + require.False(t, penceLess(0.005, 0.006), "0.005 and 0.006 both round to 1 pence") + require.True(t, penceLess(0.004999999, 0.005000001), "0p vs 1p — the partial boundary") + + // A whole-pence gap always compares by rounded pence, never raw floats. + require.False(t, penceLess(1.009, 1.01), "1.009 rounds to 1.01 → equal pence") + require.False(t, penceLess(1.009, 1.011), "1.009 and 1.011 both round to 1.01 → equal pence") + require.True(t, penceLess(1.009, 1.021), "1.009 rounds to 1.01, 1.021 rounds to 1.02 → less") +} + +// TestRoundingEpsilon_Value pins the "effectively zero" threshold shared by the +// split builders and the cash/gift-card terminal branches. It must stay 0.004 +// (0.4 pence): sub-penny float residue from dividing pence by 100 must round to +// zero pence, so a phantom payment row is never created, while a real 0.5-penny +// value still rounds to 1 pence. +func TestRoundingEpsilon_Value(t *testing.T) { + t.Parallel() + + require.Equal(t, 0.004, roundingEpsilon, "roundingEpsilon must be 0.004 (0.4 pence)") + + // The epsilon itself rounds to zero pence; the first rounding-up value is + // 0.005. Any epsilon above 0.004 would silently drop legitimate half-pence + // amounts; anything below would over-report residue. + require.Equal(t, int64(0), int64(math.Round(roundingEpsilon*100))) + require.Equal(t, int64(1), int64(math.Round(0.005*100))) + require.Equal(t, int64(0), int64(math.Round(0.0039*100))) + require.Equal(t, int64(0), int64(math.Round(0.0041*100))) +} diff --git a/backend/handlers/payments/payments_test.go b/backend/handlers/payments/payments_test.go index 22b59e3..c433f48 100644 --- a/backend/handlers/payments/payments_test.go +++ b/backend/handlers/payments/payments_test.go @@ -45,26 +45,29 @@ func adminRequestCtx(r *http.Request) *http.Request { func TestValidateCardInfo(t *testing.T) { empty := "" cardID := "card_123" + savedCardID := "card_456" token := "cnon:test" tests := []struct { - name string - cardID *string - newToken *string - wantError bool + name string + cardID *string + savedCardID *string + newToken *string + wantError bool }{ - {"both set rejected", &cardID, &token, true}, - {"card_id only ok", &cardID, nil, false}, - {"token only ok", nil, &token, false}, - {"neither set rejected", nil, nil, true}, - {"empty card_id rejected", &empty, nil, true}, - {"empty token rejected", nil, &empty, true}, - {"both empty rejected", &empty, &empty, true}, + {"card_id + token ok (SCA tokenize-result)", &cardID, nil, &token, false}, + {"saved_card_id + token ok (SCA tokenize-result)", nil, &savedCardID, &token, false}, + {"card_id only ok", &cardID, nil, nil, false}, + {"token only ok", nil, nil, &token, false}, + {"neither set rejected", nil, nil, nil, true}, + {"empty card_id rejected", &empty, nil, nil, true}, + {"empty token rejected", nil, nil, &empty, true}, + {"both empty rejected", &empty, nil, &empty, true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - err := ValidateCardInfo(tt.cardID, tt.newToken) + err := ValidateCardInfo(tt.cardID, tt.savedCardID, tt.newToken) if tt.wantError { assert.Error(t, err) } else { @@ -2181,7 +2184,8 @@ func TestBuildSplitRecords_FutureBooking_FullPayment_Splits(t *testing.T) { TotalAmount: 50, TotalPaid: 0, } - records := buildSplitRecords(record, "full", info, 50) + records, err := buildSplitRecords(record, "full", info, 50) + require.NoError(t, err) if len(records) != 2 { t.Fatalf("expected 2 records, got %d", len(records)) @@ -2220,7 +2224,8 @@ func TestBuildSplitRecords_PastBooking_NoSplit(t *testing.T) { TotalAmount: 50, TotalPaid: 0, } - records := buildSplitRecords(record, "full", info, 50) + records, err := buildSplitRecords(record, "full", info, 50) + require.NoError(t, err) if len(records) != 1 { t.Fatalf("expected 1 record (no split), got %d", len(records)) @@ -2238,7 +2243,8 @@ func TestBuildSplitRecords_DepositWithinCap_NoSplit(t *testing.T) { TotalAmount: 50, TotalPaid: 0, } - records := buildSplitRecords(record, "deposit", info, 20) + records, err := buildSplitRecords(record, "deposit", info, 20) + require.NoError(t, err) if len(records) != 1 { t.Fatalf("expected 1 record (within cap), got %d", len(records)) @@ -2256,7 +2262,8 @@ func TestBuildSplitRecords_PaymentLessThanDepositMax_NoSplit(t *testing.T) { TotalAmount: 100, TotalPaid: 0, } - records := buildSplitRecords(record, "deposit", info, 25) + records, err := buildSplitRecords(record, "deposit", info, 25) + require.NoError(t, err) if len(records) != 1 { t.Fatalf("expected 1 record (under 50%%), got %d", len(records)) @@ -2273,7 +2280,8 @@ func TestBuildSplitRecords_OverflowBeyondTotal_BecomesTip(t *testing.T) { TotalAmount: 50, TotalPaid: 0, } - records := buildSplitRecords(record, "full", info, 60) + records, err := buildSplitRecords(record, "full", info, 60) + require.NoError(t, err) if len(records) != 3 { t.Fatalf("expected 3 records (deposit + balance + tip), got %d", len(records)) @@ -2328,7 +2336,8 @@ func TestBuildSplitRecords_DiscountBooking_TipOverflow_SumNeverExceedsCharge(t * TotalAmount: 100, TotalPaid: 0, // the £10 discount payment row is excluded here } - records := buildSplitRecords(record, "full", info, 120) + records, err := buildSplitRecords(record, "full", info, 120) + require.NoError(t, err) var sum float64 for _, r := range records { @@ -2351,7 +2360,8 @@ func TestBuildSplitRecords_DiscountBooking_TipOverflow_SumNeverExceedsCharge(t * TotalAmount: 100, TotalPaid: 60, } - records2 := buildSplitRecords(record2, "full", info2, 70) + records2, err2 := buildSplitRecords(record2, "full", info2, 70) + require.NoError(t, err2) var sum2 float64 for _, r := range records2 { sum2 += r.Amount @@ -2399,7 +2409,8 @@ func TestGetBookingPaymentInfo_ExcludesDiscountPayments(t *testing.T) { // A £70 tip-overflow charge on top must partition exactly into £70 of split // records — the discount can never push the recorded sum past the charge. record := makeTestRecord(bookingID, "full", 70) - records := buildSplitRecords(record, "full", info, 70) + records, err := buildSplitRecords(record, "full", info, 70) + require.NoError(t, err) var sum float64 for _, r := range records { sum += r.Amount @@ -2448,7 +2459,8 @@ func TestBuildSplitRecords_DepositCarve_PenceExact(t *testing.T) { TotalAmount: tc.total, TotalPaid: tc.paid, } - records := buildSplitRecords(record, "full", info, chargedPounds) + records, err := buildSplitRecords(record, "full", info, chargedPounds) + require.NoError(t, err) var depositPence, balancePence, tipPence, partitionPence int64 for _, r := range records { @@ -4555,7 +4567,8 @@ func TestBuildSplitRecords_TipOverflow_SeparateTipRecord(t *testing.T) { TotalAmount: 50, TotalPaid: 0, } - records := buildSplitRecords(record, "full", info, 60) + records, err := buildSplitRecords(record, "full", info, 60) + require.NoError(t, err) if len(records) != 3 { t.Fatalf("expected 3 records (deposit + balance + tip), got %d", len(records)) diff --git a/backend/handlers/payments/policy_ts_crosscheck_test.go b/backend/handlers/payments/policy_ts_crosscheck_test.go new file mode 100644 index 0000000..20fea95 --- /dev/null +++ b/backend/handlers/payments/policy_ts_crosscheck_test.go @@ -0,0 +1,74 @@ +//go:build test && dev + +package payments + +import ( + "os" + "path/filepath" + "regexp" + "strconv" + "testing" + + "github.com/stretchr/testify/require" +) + +// policyTSValue extracts the numeric value of a `NAME: value,` entry from the +// frontend's POLICY object literal (policy.ts), tolerating the tab-indented +// formatting the file uses. Returns "" when the name is absent. +func policyTSValue(t *testing.T, src, name string) string { + t.Helper() + re := regexp.MustCompile(`(?m)^\s*` + regexp.QuoteMeta(name) + `:\s*(\d+(?:\.\d+)?),`) + m := re.FindStringSubmatch(src) + if m == nil { + return "" + } + return m[1] +} + +// TestPolicyTS_CrossCheck pins the frontend's single-source policy constants +// (frontend/src/lib/constants/policy.ts) to THIS package's refund_policy.go +// values by REACTING to drift: the test reads the .ts file and asserts each +// entry equals the backend constant, so a one-sided change on either side fails +// CI. The frontend's own vitest suite (policy.test.ts) pins the same values in +// the other direction, closing the drift loop both ways. +func TestPolicyTS_CrossCheck(t *testing.T) { + path := filepath.Join("..", "..", "..", "frontend", "src", "lib", "constants", "policy.ts") + data, err := os.ReadFile(path) + require.NoError(t, err, "policy.ts not found at %s (tests run from the package dir; repo layout is /backend/handlers/payments + /frontend)", path) + src := string(data) + + cases := []struct { + name string + want string + }{ + {"REQUIRED_DEPOSIT_PCT", "0.2"}, + {"PROTECTED_DEPOSIT_MAX_PCT", "0.5"}, + {"FULL_REFUND_THRESHOLD_HOURS", "72"}, + {"PARTIAL_REFUND_THRESHOLD_HOURS", "24"}, + {"NO_SHOW_THRESHOLD_HOURS", "24"}, + {"DEPOSIT_ADVANCE_HOURS", "36"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := policyTSValue(t, src, tc.name) + require.NotEmpty(t, got, "policy.ts no longer declares %q — the constant may have been renamed or removed", tc.name) + require.Equal(t, tc.want, got, "%s drifted from backend/handlers/payments/refund_policy.go", tc.name) + }) + } + + // The backend has no named reschedule constants; the frontend values must + // track the refund tiers they were derived from. + for _, tc := range []struct { + name string + want string + }{ + {"RESCHEDULE_BLOCK_HOURS_WITH_PAYMENTS", strconv.Itoa(int(FullRefundThreshold.Hours()))}, + {"RESCHEDULE_BLOCK_HOURS_NO_PAYMENTS", strconv.Itoa(int(PartialRefundThreshold.Hours()))}, + } { + t.Run(tc.name, func(t *testing.T) { + got := policyTSValue(t, src, tc.name) + require.NotEmpty(t, got, "policy.ts no longer declares %q", tc.name) + require.Equal(t, tc.want, got, "%s must track the backend refund threshold", tc.name) + }) + } +} diff --git a/backend/handlers/payments/refunds.go b/backend/handlers/payments/refunds.go index e5ac89b..2bca6ce 100644 --- a/backend/handlers/payments/refunds.go +++ b/backend/handlers/payments/refunds.go @@ -1136,6 +1136,7 @@ func insertRefundFailedNotifications(ctx context.Context, refundIDs []string) { AND NOT EXISTS ( SELECT 1 FROM admin_notifications an WHERE an.reason = 'refund_failed' AND an.booking_id = refunds.booking_id + AND an.acknowledged_at IS NULL ) AND (SELECT COUNT(*) FROM admin_notifications _an WHERE _an.reason = 'refund_failed' diff --git a/backend/handlers/payments/sca_mock_parity_test.go b/backend/handlers/payments/sca_mock_parity_test.go new file mode 100644 index 0000000..1f91165 --- /dev/null +++ b/backend/handlers/payments/sca_mock_parity_test.go @@ -0,0 +1,163 @@ +//go:build test && dev + +package payments + +import ( + "encoding/json" + "net/http" + "testing" + + "crussell/internal/square" + "crussell/testutils" + "crussell/testutils/fixtures" + "crussell/testutils/jwt" + + "github.com/stretchr/testify/require" +) + +// ============================================================================= +// MEDIUM-HIGH: dev mock SCA error-shape parity at the HANDLER level. +// +// square_dev.go's SimulateSavedCardVerificationRequired is exercised heavily in +// internal/square's own suite, but the frontend only ever sees the MOCK's SCA +// rejection through a payment HANDLER — the 402 + {"code":"verification_required"} +// body is what the SCA challenge flow keys on. These tests pin that the mock's +// simulated SCA rejection surfaces the structured body end to end, and that the +// ENFORCED-deployment gate refuses token-less saved-card charges while genuine +// SCA tokenize-results (new_card_token = cnon:sca-...) sail through. +// ============================================================================= + +// TestBookingPayment_MockSavedCardSCA_Tokenless_402_VerificationRequired drives +// the mock's SimulateSavedCardVerificationRequired toggle through +// CreateBookingPayment: a token-less saved-card (ccof) charge is rejected by +// the mock with CARD_DECLINED_VERIFICATION_REQUIRED, and the handler must +// surface it as 402 + the structured verification_required body — the exact +// shape the frontend's isVerificationRequiredSignal parses to trigger the +// client-side challenge. +func TestBookingPayment_MockSavedCardSCA_Tokenless_402_VerificationRequired(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestData(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "ccof:mock_sca_booking", "VISA", "4242") + require.NoError(t, err) + + origClient := SquareClient + mc := square.NewDevClient().(*square.MockClient) + mc.SimulateSavedCardVerificationRequired = true + SquareClient = mc + defer func() { SquareClient = origClient }() + + req := CreateBookingPaymentRequest{ + Amount: 2500, + PaymentType: "deposit", + CardID: &cardID, + IdempotencyKey: "mock-sca-booking-tokenless", + } + + w := makePaymentRequest(withNonGuest(CreateBookingPayment), "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + assertStructuredVerificationRequired(t, w) + + // A refused charge must not record a completed payment. + var payCount int + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND status = 'completed'`, bookingID).Scan(&payCount)) + require.Zero(t, payCount, "a mock-SCA-refused saved-card charge must not record a completed payment") +} + +// TestBookingPayment_MockSavedCardSCA_SCATokenizeResult_Succeeds proves the +// other half of the mock parity: under the SAME SimulateSavedCardVerificationRequired +// toggle, a saved-card charge carrying a GENUINE tokenize-result +// (new_card_token = cnon:sca-... — card.tokenize(verificationDetails, cardId)) +// is accepted by the mock (the token IS the buyer verification) and the handler +// completes the charge. +func TestBookingPayment_MockSavedCardSCA_SCATokenizeResult_Succeeds(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestData(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "ccof:mock_sca_booking_ok", "VISA", "4242") + require.NoError(t, err) + + origClient := SquareClient + mc := square.NewDevClient().(*square.MockClient) + mc.SimulateSavedCardVerificationRequired = true + SquareClient = mc + defer func() { SquareClient = origClient }() + + scaToken := "cnon:sca-4242_2500_ok" + req := CreateBookingPaymentRequest{ + Amount: 2500, + PaymentType: "deposit", + CardID: &cardID, + NewCardToken: &scaToken, + IdempotencyKey: "mock-sca-booking-tokenized", + } + + w := makePaymentRequest(withNonGuest(CreateBookingPayment), "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusOK, w.Code, "a genuine SCA tokenize-result must complete the saved-card charge, body: %s", w.Body.String()) +} + +// TestBookingPayment_EnforcedSavedCard_Tokenless_402 pins the SCA-only gate on +// the booking surface in an ENFORCED deployment: a token-less saved-card charge +// is refused 402 verification_required up front (PSR 2017 reg 100 — no homegrown +// 2FA fallback), before any charge reaches Square. +func TestBookingPayment_EnforcedSavedCard_Tokenless_402(t *testing.T) { + helperEnvEnforce2FAStaging(t) + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestData(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "ccof:mock_enforced_booking", "VISA", "4242") + require.NoError(t, err) + + req := CreateBookingPaymentRequest{ + Amount: 2500, + PaymentType: "deposit", + CardID: &cardID, + IdempotencyKey: "enforced-booking-tokenless", + } + + w := makePaymentRequest(withNonGuest(CreateBookingPayment), "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + assertStructuredVerificationRequired(t, w) + + var body map[string]string + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) + require.Equal(t, "verification_required", body["code"]) +} + +// TestBookingPayment_EnforcedSavedCard_SCATokenizeResult_Succeeds proves the +// SCA-primary skip in an ENFORCED deployment: a saved-card charge carrying a +// genuine SCA tokenize-result (new_card_token = cnon:sca-...) skips BOTH 2FA +// gates (scaTokenizedSavedCard) and completes — the wire contract the frontend +// sends after a successful tokenizeWithVerification challenge. +func TestBookingPayment_EnforcedSavedCard_SCATokenizeResult_Succeeds(t *testing.T) { + helperEnvEnforce2FAStaging(t) + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestData(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "ccof:mock_enforced_booking_ok", "VISA", "4242") + require.NoError(t, err) + + origClient := SquareClient + mc := square.NewDevClient().(*square.MockClient) + mc.SimulateSavedCardVerificationRequired = true + SquareClient = mc + defer func() { SquareClient = origClient }() + + scaToken := "cnon:sca-4242_2500_ok" + req := CreateBookingPaymentRequest{ + Amount: 2500, + PaymentType: "deposit", + CardID: &cardID, + NewCardToken: &scaToken, + IdempotencyKey: "enforced-booking-tokenized", + } + + w := makePaymentRequest(withNonGuest(CreateBookingPayment), "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusOK, w.Code, "an SCA tokenize-result must skip the enforced gate and complete, body: %s", w.Body.String()) + + var payCount int + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND status = 'completed'`, bookingID).Scan(&payCount)) + require.Equal(t, 1, payCount, "the SCA-tokenized charge must record exactly one completed payment") +} diff --git a/backend/handlers/payments/service.go b/backend/handlers/payments/service.go index c5af7fa..b92b827 100644 --- a/backend/handlers/payments/service.go +++ b/backend/handlers/payments/service.go @@ -550,6 +550,13 @@ func (s *PaymentService) GetBookingRemainingBalancePence(ctx context.Context, bo -- A tip is money paid beyond the booking total — it does not -- reduce the balance owed, so it must not count as "paid". AND payment_type <> 'tip' + -- A discount / on_the_house row is a ledger entry, not real money + -- toward the booking (the real-money convention every other paid + -- computation applies — e.g. the deposit-threshold query and + -- GetBookingRefundableAmountPence). Counting them as "paid" would + -- understate the remaining balance, and a fully-discounted + -- booking would appear fully paid and refuse legitimate charges. + AND payment_method NOT IN ('discount', 'on_the_house') ), refunded_total AS ( SELECT COALESCE(SUM(r.amount), 0) AS refunded_pounds diff --git a/backend/handlers/payments/sweep.go b/backend/handlers/payments/sweep.go index a4e773b..06294d5 100644 --- a/backend/handlers/payments/sweep.go +++ b/backend/handlers/payments/sweep.go @@ -75,6 +75,15 @@ const stalePendingPaymentAge = 24 * time.Hour // trustworthily and fall back to the legacy blind-fail + WARN. const stalePendingKeyedAge = 22 * time.Hour +// SweepKeyedReplayAge returns the age at which a keyed pending row (stored +// idempotency key, no square_payment_id) becomes eligible for the sweep's +// replay-by-key reconcile. Exported for the webhooks package so the +// payment.completed orphan-replay detection can gate on whether a pending +// origin row could actually have been replayed by the sweep: a pending row +// younger than this age can never be a sweep-minted duplicate's origin, so it +// must not be marked failed by a webhook that raced the charge response. +func SweepKeyedReplayAge() time.Duration { return stalePendingKeyedAge } + // b1DuplicateRefundAttemptCap caps how many times the sweep may auto-refund a // replay-induced duplicate charge (B1) under one stale pending row's expired // idempotency key. A REJECTED refund writes NO refunds row, so the in-flight @@ -729,7 +738,12 @@ func buildStaleRescueRecords(ctx context.Context, tx pgx.Tx, r staleRow, squareP k := r.IdempotencyKey primary.IdempotencyKey = &k } - return buildSplitRecords(primary, paymentType, info, amount) + records, err := buildSplitRecords(primary, paymentType, info, amount) + if err != nil { + log.Printf("MEDIUM-3: buildSplitRecords rejected the rescue split for payment %s (booking %s): %v — the row is completed un-split; manual reconciliation recommended", r.ID, *r.BookingID, err) + return nil + } + return records } // applyStaleRescueRecords applies the pre-computed split records inside the diff --git a/backend/handlers/payments/terminal_sca_test.go b/backend/handlers/payments/terminal_sca_test.go index dea0c50..7dacb27 100644 --- a/backend/handlers/payments/terminal_sca_test.go +++ b/backend/handlers/payments/terminal_sca_test.go @@ -171,7 +171,6 @@ func TestTerminalSavedCard_Tokenless_402(t *testing.T) { PaymentMethod: strPtr("saved_card"), UserSavedCardID: &cardID, IdempotencyKey: "terminal-tokenless-" + bookingID, - VerificationCode: "334411", } w := makePaymentRequest(CreateTerminalPayment, "POST", "/api/admin/bookings/"+bookingID+"/payment", req, adminToken, ctx) @@ -213,7 +212,6 @@ func TestTillSavedCard_Tokenless_402(t *testing.T) { UserSavedCardID: &cardID, UserID: &userID, IdempotencyKey: key, - VerificationCode: "665544", } w := makePaymentRequest(CreateTillSale, "POST", "/api/admin/till/sale", req, adminToken, ctx) diff --git a/backend/handlers/payments/till.go b/backend/handlers/payments/till.go index 9edb43e..2bbca1f 100644 --- a/backend/handlers/payments/till.go +++ b/backend/handlers/payments/till.go @@ -33,6 +33,13 @@ type TillSaleRequest struct { PaymentMethod string `json:"payment_method" validate:"required"` UserSavedCardID *string `json:"user_saved_card_id,omitempty"` UserID *string `json:"user_id,omitempty"` + // NewCardToken is the SCA tokenize-result token (card.tokenize( + // verificationDetails, cardId)) for a saved-card till sale. When present it + // coexists with user_saved_card_id: the token is the one-time charge SOURCE + // and the saved-card row supplies the customer (mirrors the booking SCA + // tokenize-result wire contract). Without it the stored ccof: card id is + // the source (legacy saved-card charge). + NewCardToken *string `json:"new_card_token,omitempty"` // IdempotencyKey is optional; an empty key is replaced with a DETERMINISTIC // fallback derived from the canonical request fields // (deriveTillIdempotencyKey) so a lost-response retry re-derives the SAME @@ -44,15 +51,6 @@ type TillSaleRequest struct { CardToken string `json:"card_token,omitempty"` RedeemToUserID *string `json:"redeem_to_user_id,omitempty"` VerificationToken *string `json:"verification_token,omitempty"` - // VerificationCode is the card owner's current 2FA one-time code (B10): an - // enforced environment charges a saved card only when this matches the - // customer's pending code. - VerificationCode string `json:"verification_code,omitempty"` - // ConsentVersion / ConsentAccepted: the customer's explicit versioned - // consent to the SCA-unavailable → 2FA fallback (C6), enforced server-side - // (403 consent_required) and recorded on the 2fa_fallback_charge audit row. - ConsentVersion *string `json:"consent_version,omitempty"` - ConsentAccepted bool `json:"consent_accepted"` } type TillSaleResponse struct { @@ -979,11 +977,6 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { // the post-charge 2FA consumption + audit after the Square call need it. var cardUserID sql.NullString - // twoFAFallbackUsed records whether the 2FA BACKUP authorized a token-less - // saved-card charge (SCA unavailable). Hoisted: set in the saved_card case - // below, consumed by the strict fallback audit after the Square call. - twoFAFallbackUsed := false - // SCA verification token (if any) — hoisted so the 2FA gate in the // saved_card case (a present token skips the gate: SCA-primary) and the // CreatePaymentReq after the switch share one extraction. @@ -1056,32 +1049,28 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { } // 2FA gating (C5): charging a customer's saved card requires 2FA when - // the feature is enforced. consume=!reuse (LOW 6a): a FRESH charge - // verifies WITH consumption — the code is single-use at the gate, - // closing the TOCTOU where a verified-but-unconsumed code could - // authorize a second charge within its lifetime — and a failed Square - // charge re-issues a fresh code (reissueTwoFACodeAfterFailedCharge - // below). A pending-reuse retry (existingPendingID != "") verifies - // WITHOUT consuming: the code was re-issued for exactly this retry and - // the post-charge success path below (ConsumePendingCode) burns it on - // terminal success, so a retry that fails again keeps its code for one - // more attempt. A charge carrying a Square verification_token (SCA - // performed) skips the gate; a token-less charge falls back to 2FA and - // twoFAFallbackUsed is set for the charge-success audit. - if cardUserID.Valid { - var gateOK bool - gateOK, twoFAFallbackUsed = requireTwoFactorForCardAccess(w, r, service, cardUserID.String, req.VerificationCode, tillVerificationToken, existingPendingID == "") - if !gateOK { - return - } - // C6: a fallback-authorized charge must carry the customer's - // accepted consent (403 consent_required otherwise). - if !enforceSCAFallbackConsent(w, req.ConsentVersion, req.ConsentAccepted, twoFAFallbackUsed) { + // the feature is enforced. A charge carrying a Square verification_token + // (SCA performed) skips the gate; a token-less charge is refused 402 + // verification_required (SCA-only — the homegrown 2FA fallback was + // removed). An SCA tokenize-result token (new_card_token) also skips the + // gate: the token only exists after the issuer completed buyer + // verification for this card + amount (SCA-primary), mirroring the + // booking path's scaTokenizedSavedCard skip. + scaTokenizedSavedCard := req.NewCardToken != nil && *req.NewCardToken != "" + if cardUserID.Valid && !scaTokenizedSavedCard { + if gateOK, _ := requireTwoFactorForCardAccess(w, r, service, cardUserID.String, tillVerificationToken, existingPendingID == ""); !gateOK { return } } + // SCA tokenize-result wire contract (mirrors resolveChargeSource's + // saved-card branch): when the request carries new_card_token, the + // token is the one-time charge source and the saved-card row supplies + // the customer; otherwise the stored ccof: card id is the source. tillSquareSourceID = savedCardSqCardID + if req.NewCardToken != nil && *req.NewCardToken != "" { + tillSquareSourceID = *req.NewCardToken + } saleStatus = "pending" needsSquarePayment = true case "card_machine": @@ -1255,7 +1244,7 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { paymentReq := square.CreatePaymentReq{ Amount: penceAmount, Currency: "GBP", - SourceID: savedCardSqCardID, + SourceID: tillSquareSourceID, CustomerID: savedCardCustomerID, IdempotencyKey: req.IdempotencyKey, Note: "Gift Card " + req.Action, @@ -1357,13 +1346,6 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { } } } - // The gate consumed the 2FA code for a FRESH saved-card charge — - // re-issue so the same-key retry has a live code to verify. A - // pending-reuse retry verified without consuming at the gate, so - // its code survives for one more attempt. - if req.PaymentMethod == "saved_card" && cardUserID.Valid { - reissueTwoFACodeAfterFailedCharge(ctx, db.Conn, cardUserID.String, true, twoFAFallbackUsed && existingPendingID == "", r) - } // 402 only for definitive declines; ambiguous transport/5xx must be // 503 so the pending sale stays resumable on a same-key retry // (M3). The clawback decision above stays keyed on @@ -1424,11 +1406,6 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { "card_last4": paymentResult.CardLast4, "square_payment_id": paymentResult.SquarePayID, }) - // The 2FA BACKUP authorized this token-less saved-card till charge - // (SCA was unavailable); the actor is the admin from context. - if twoFAFallbackUsed { - insertTwoFAFallbackAudit(ctx, adminID, cardUserID.String, paymentResult.CardLast4, tillSaleID, "admin till saved-card charge authorized via 2FA fallback (SCA unavailable)", consentVersionValue(req.ConsentVersion), req.ConsentAccepted) - } // F1 confused-deputy audit: a saved-card till charge that proceeded // WITHOUT the request naming the customer (user_id omitted) charged // the card under its RESOLVED owner — a card never associated with diff --git a/backend/handlers/payments/twofa.go b/backend/handlers/payments/twofa.go index f0b80fb..8cd54f0 100644 --- a/backend/handlers/payments/twofa.go +++ b/backend/handlers/payments/twofa.go @@ -1,20 +1,9 @@ package payments import ( - "context" - "crypto/rand" - "errors" - "fmt" - "log" - "math/big" "net/http" "os" "strings" - "time" - - "crussell/clock" - "crussell/db" - "crussell/internal/twofa" ) // require2FADisabled reports whether REQUIRE_2FA explicitly disables 2FA @@ -50,104 +39,19 @@ func (s *PaymentService) TwoFactorEnforced() bool { return twoFactorEnforced() } -// errTwoFAPepperRequired is returned when TWO_FACTOR_PEPPER is unset in a -// production-style re-issue gate. Refusing to re-issue is the only safe outcome: -// without the pepper the fresh code would be persisted as an unsalted SHA-256 -// digest in the 1M code space, which a log/DB leak could brute-force offline -// (mirrors handlers/user's errTwoFAPepperRequired). Defined here (no build tag) -// so the pure gate (twoFAReissueIssueAllowedStrict) and the test,dev suite -// share it. -var errTwoFAPepperRequired = errors.New("TWO_FACTOR_PEPPER is not set; refusing to issue a 2FA code (an unsalted digest would be offline-brute-forceable)") - -// errTwoFADeliveryUnavailable is returned when a production-style re-issue has -// no delivery channel: email/SMS unwired and the operator has not opted into -// the insecure log-delivery mode (TWO_FACTOR_ALLOW_LOG_DELIVERY=true). A fresh -// code the customer could never receive would strand them on the saved-card -// gate (mirrors handlers/user's errTwoFADeliveryUnavailable). -var errTwoFADeliveryUnavailable = errors.New("2FA requires an email or SMS delivery channel; contact the salon") - -// twoFAPepperConfigured reports whether TWO_FACTOR_PEPPER is set — the pure, -// build-agnostic read behind the strict re-issue gate. -func twoFAPepperConfigured() bool { return os.Getenv("TWO_FACTOR_PEPPER") != "" } - -// twoFADeliveryChannelConfigured reports whether the deployment has explicitly -// configured a 2FA code delivery channel: TWO_FACTOR_ALLOW_LOG_DELIVERY set to -// exactly "true" (the only production channel today — email/SMS unwired, P6). -// Pure env read, build-agnostic: the build-tagged twoFADeliveryAvailable -// (twofa_delivery_dev.go / twofa_delivery_prod.go) is the runtime-facing -// wrapper that turns this into the always-true dev channel or the prod env -// check. -func twoFADeliveryChannelConfigured() bool { return os.Getenv("TWO_FACTOR_ALLOW_LOG_DELIVERY") == "true" } - -// twoFAReissueIssueAllowedStrict is the pure, build-agnostic production-style -// re-issue gate: a re-issued 2FA code may be minted ONLY when BOTH -// TWO_FACTOR_PEPPER is set (an unsalted digest in the 1M code space would be -// offline-brute-forceable) AND a delivery channel is configured (otherwise the -// fresh code could never reach the customer). Either way it fails closed. The -// build-tagged twoFAReissueIssueAllowed wraps it for production builds; -// dev/test builds always allow re-issue and never consult it — but the test,dev -// suite exercises THIS function directly, so the fail-closed branches are -// CI-visible even though the prod file (!dev && !test) is excluded there. -func twoFAReissueIssueAllowedStrict() error { - if !twoFAPepperConfigured() { - return errTwoFAPepperRequired - } - if !twoFADeliveryChannelConfigured() { - return errTwoFADeliveryUnavailable - } - return nil -} - -// Single source of truth for 2FA verification: crussell/internal/twofa owns -// the code hashing (HMAC-SHA256 keyed by TWO_FACTOR_PEPPER, legacy SHA-256 -// fallback), the constant-time compare, the code-lifetime check, and the -// per-user brute-force lockout. The user package's interactive endpoints -// (setup/verify/disable) and this file's re-issue path share it; nothing is -// re-implemented locally here. The saved-card charge gate no longer verifies -// 2FA codes at all (SCA-only — the 2FA fallback was removed), so the only -// local consumer left is reissueTwoFACodeAfterFailedCharge below. - -// enforceSCAFallbackConsent is retained as a compile-compatible NO-OP so the -// saved-card charge handlers (handlers.go / till.go / giftcards.go) keep -// compiling unchanged. It used to enforce the C6 consent notice on the -// SCA-unavailable → 2FA fallback path, which is now REMOVED ENTIRELY: PSR 2017 -// reg 100 makes Strong Customer Authentication mandatory and non-waivable for -// customer-initiated stored-credential charges, and the homegrown 2FA (a -// merchant-side check with no bank involvement) cannot legally act as an SCA -// fallback — authorising a token-less charge via 2FA leaves the MERCHANT liable -// for ECI 7 / SLI 210 chargebacks and PSR 2017 reg 77(6) compensation, and -// customer consent does not cure that. The gate now refuses a token-less -// saved-card charge 402 verification_required BEFORE any fallback can be used, -// so fallbackUsed is always false and there is no consent to demand. The -// callers pass it through as before; it always returns true. -func enforceSCAFallbackConsent(w http.ResponseWriter, consentVersion *string, consentAccepted bool, fallbackUsed bool) bool { - return true -} - -// consentVersionValue normalizes the request's optional consent_version pointer -// to a string ("" when absent). Retained because the saved-card charge handlers -// still read the field when writing their (now unreachable) fallback audit row. -func consentVersionValue(version *string) string { - if version == nil { - return "" - } - return *version -} - // requireTwoFactorForCardAccess gates the saved-card online payment paths. // It returns (allowed, fallbackUsed): allowed is true when the request may // proceed; fallbackUsed is ALWAYS false — the homegrown 2FA fallback for // token-less saved-card charges was REMOVED ENTIRELY, so no charge is ever -// authorized by a 2FA code and no fallback audit row is ever written (the -// callers' insertTwoFAFallbackAudit branches are unreachable). +// authorized by a 2FA code and no fallback audit row is ever written. // // It is a thin wrapper over requireTwoFactorForCardAccessWithTokenValidation -// that passes tokenForwardedToSquare=true — the legacy signature is retained -// because the saved-card CHARGE surfaces (booking, tip, gift-card buy, till) -// forward the verification_token to Square in CreatePaymentReq.VerificationToken, -// so a non-empty token there IS Square-validated SCA and legitimately skips the -// gate. The card-SAVE surfaces call the WithTokenValidation variant directly -// with false (see that helper for the auth-F1 rationale). +// that passes tokenForwardedToSquare=true — the saved-card CHARGE surfaces +// (booking, tip, gift-card buy, till, terminal) forward the verification_token +// to Square in CreatePaymentReq.VerificationToken, so a non-empty token there +// IS Square-validated SCA and legitimately skips the gate. The card-SAVE +// surfaces call the WithTokenValidation variant directly with false (see that +// helper for the auth-F1 rationale). // // The decision model, in order: // @@ -175,8 +79,8 @@ func consentVersionValue(version *string) string { // On any denial an error JSON is written (parseable by the frontend via // extractErrorMessage) and allowed=false is returned — the caller must abort // the charge. -func requireTwoFactorForCardAccess(w http.ResponseWriter, r *http.Request, service *PaymentService, userID, verificationCode, verificationToken string, consume bool) (allowed, fallbackUsed bool) { - return requireTwoFactorForCardAccessWithTokenValidation(w, r, service, userID, verificationCode, verificationToken, consume, true) +func requireTwoFactorForCardAccess(w http.ResponseWriter, r *http.Request, service *PaymentService, userID, verificationToken string, consume bool) (allowed, fallbackUsed bool) { + return requireTwoFactorForCardAccessWithTokenValidation(w, r, service, userID, verificationToken, consume, true) } // requireTwoFactorForCardAccessWithTokenValidation is the real gate: @@ -192,11 +96,17 @@ func requireTwoFactorForCardAccess(w http.ResponseWriter, r *http.Request, servi // skip the gate — with tokenForwardedToSquare=false the token is ignored and // the token-less refusal below applies. An authenticated client can therefore // no longer persist a card to the account with `verification_token: "anything"` -// and no SCA. The one legitimate SAVE skip is the call-site's scaTokenizedSavedCard -// flow (new_card_token + saved_card_id): there the SCA tokenize-result token IS -// the charge source and Square validates it as source_id, so the gate is skipped -// by the caller before this helper is ever reached. -func requireTwoFactorForCardAccessWithTokenValidation(w http.ResponseWriter, r *http.Request, service *PaymentService, userID, verificationCode, verificationToken string, consume bool, tokenForwardedToSquare bool) (allowed, fallbackUsed bool) { +// and no SCA. The legitimate SAVE skips are handled by the call site before +// this helper is ever reached: a genuine Square token-like card token is +// SCA-proven (the STORE-intent SCA performed at tokenization IS the +// verification — see CreatePaymentMethod's save gate) and the +// scaTokenizedSavedCard flow (new_card_token + saved_card_id) never persists a +// card. +// +// The verification_code parameter that used to flow through this gate is GONE: +// the gate never read it (SCA-only), the request structs no longer carry it, +// and there is no homegrown fallback to authorise anything with it. +func requireTwoFactorForCardAccessWithTokenValidation(w http.ResponseWriter, r *http.Request, service *PaymentService, userID, verificationToken string, consume bool, tokenForwardedToSquare bool) (allowed, fallbackUsed bool) { if !twoFactorEnforced() { return true, false } @@ -219,238 +129,3 @@ func requireTwoFactorForCardAccessWithTokenValidation(w http.ResponseWriter, r * writeVerificationRequiredResponse(w) return false, false } - -// reissueTwoFACodeAfterFailedCharge mints a fresh 2FA code after a saved-card -// charge failed at Square — but ONLY when a code was actually consumed by a -// FRESH saved-card charge (fresh-only semantics). The charge gate consumes the -// verified code at gate time for fresh charges (single-use — closing the -// verify-then-consume TOCTOU where a verified-but-unconsumed code could -// authorize a second charge), so a failed fresh charge leaves no live code for -// the same-key retry; this re-issues one with the same 10-minute lifetime and -// delivery behaviour as the user package's code issuance (dev/test logs the -// code for the operator to relay; production logs only with -// TWO_FACTOR_ALLOW_LOG_DELIVERY=true, matching the fail-closed delivery -// contract). It is a NO-OP for every other outcome: a new-card (cnon) charge -// never gates (usedSavedCard=false), a pending-reuse retry verified WITHOUT -// consuming (fallbackUsed=false — its code survives for one more attempt and a -// re-issue would silently invalidate the one the customer holds), and an -// SCA-authorized charge never touched the 2FA gate at all. -// -// Callers pass: -// - usedSavedCard: whether this charge actually used a saved card (the 2FA -// gate applies only to saved-card ccof charges); -// - fallbackUsed: whether the 2FA fallback gate actually consumed a code on -// THIS attempt (true only for a fresh charge — the gate's -// twoFAFallbackUsed ANDed with the caller's not-a-pending-reuse test). -// -// LOW-MEDIUM (finding 2): the re-issue is routed through the same fail-closed -// issuance gate as the interactive mint paths (twoFAReissueIssueAllowed — -// mirrored from the user package's twoFAEnsureIssueAllowed via the build-tagged -// twofa_delivery_dev.go / twofa_delivery_prod.go): a production build refuses -// to re-issue when TWO_FACTOR_PEPPER is unset (an unsalted digest in the 1M -// code space would be offline-brute-forceable) or when no delivery channel is -// configured. It also respects the same per-user mint cooldown -// (twoFAMintCooldown via the shared twofa.AttemptState.LastMintAt), so a -// charge-failure loop cannot mint codes faster than the mint endpoints allow. -// Best-effort: a failure logs a CRITICAL line + raises a critical-payment -// admin notification (the operator must mint a code manually or fix the -// config) and the customer requests a fresh code through the normal 2FA flow. -// -// PEPPER-CHANGE NOTE (Loop B finding 2): TWO_FACTOR_PEPPER is the ONLY hard -// gate on issuance here (twoFAReissueIssueAllowed) AND on the interactive mint -// paths (twoFAEnsureIssueAllowed in handlers/user). The pepper keys the -// HMAC-SHA256 of every stored pending-code hash, so CHANGING it invalidates -// ALL pending codes — every stored hash was computed with the old pepper and -// can never match a code minted under the new one. A fresh saved-card charge -// that consumed a pre-change code then fails will re-issue a code hashed with -// the NEW pepper, which still cannot match anything the customer holds (their -// code was minted under the old pepper, or was consumed). Operators MUST NOT -// change the pepper without re-minting every user's code (or having the -// customer re-run 2FA setup); main.go's startup check should treat a changed -// pepper as a config incident. -func reissueTwoFACodeAfterFailedCharge(ctx context.Context, q db.Querier, userID string, usedSavedCard, fallbackUsed bool, r *http.Request) { - if userID == "" || !twoFactorEnforced() || !usedSavedCard || !fallbackUsed { - return - } - if err := twoFAReissueIssueAllowed(); err != nil { - // Loop B HIGH (finding 2): a REFUSED re-issue strands the customer. The - // gate consumed their code for the fresh charge (single-use) and the - // charge failed — with no re-issued code the same-key retry fails - // forever with 400 ErrMissingOrExpired and the customer is locked out. - // This is an OPERATOR-FACING incident, not a silent best-effort miss: - // log a CRITICAL line AND raise the per-issue-capped critical-payment - // admin notification (sweep.go's insertCriticalPaymentNotification — the - // DB-backed stand-in for the un-watched CRITICAL logs) so the operator - // knows the customer is blocked and can mint a code manually or fix the - // config (TWO_FACTOR_PEPPER / delivery channel). - log.Printf("CRITICAL: failed to re-issue a 2FA code for user %s after a failed saved-card charge (%v) — the customer's code was consumed by the fresh charge and NO live code remains, so the same-key retry cannot succeed; the operator must mint a code manually or configure TWO_FACTOR_PEPPER and a 2FA delivery channel", userID, err) - // Round 2 Loop B findings 2 + 7 — the alert is capped PER-ISSUE, NOT - // globally (see alertReissueFail below): at most ONE unacknowledged row - // per stranded customer, so one customer's alert can never be - // suppressed by OTHER users' rows filling the 'critical_payment_log' - // bucket, and the count-then-insert is atomic (a single INSERT ... WHERE - // NOT EXISTS — no TOCTOU). An attacker also cannot FLOOD the alert: - // raising it requires a real saved-card charge that consumed a real code - // AND a failed re-issue, and the per-issue dedup holds each user to one - // row until acknowledged. Fail-closed behaviour is unchanged (the - // CRITICAL log always fires); only the notification INSERT is bounded. - alertReissueFail(ctx, q, userID) - return - } - // Mint cooldown (B11a + Round 2 Loop A finding 2): the shared per-user mutex - // serializes the stamp read/write with the user package's mints and the - // gate's verify critical section. A successful verify NO LONGER clears the - // stamp (internal/twofa.Check keeps it — it is cleared only at terminal - // charge success via twofa.ConsumePendingCode), so this check now genuinely - // bounds a charge-failure loop: the first re-issue after a mint is skipped - // while clock.Now().Sub(LastMintAt) < twoFAMintCooldown, bounding code churn - // and dev log flooding. The customer requests a fresh code through the - // normal mint endpoint once the window elapses. - st := twofa.StateFor(userID) - st.Mu.Lock() - defer st.Mu.Unlock() - if !st.LastMintAt.IsZero() && clock.Now().Sub(st.LastMintAt) < twoFAMintCooldown { - // Round 2 Loop B finding 6b: this skip was SILENT before. A FRESH - // charge consumed the customer's code at the gate (single-use) and the - // charge failed; the re-issue is now skipped by the per-user mint - // cooldown — the customer holds NO live code for the same-key retry - // until the cooldown lapses. That is a stranded customer, so raise the - // same per-issue-capped reissue-fail alert the refused-issue branch - // above uses (alertReissueFail, deduped on reason+user_id) so the - // operator knows to mint a code manually. The alert is per-issue - // (finding 2): repeated skips for the same customer stay ONE row until - // acknowledged and can never be suppressed by other users' rows. - log.Printf("2FA: re-issue skipped for user %s after a failed charge (mint cooldown) — the customer's code was consumed by the fresh charge and NO live code remains until the cooldown lapses", userID) - alertReissueFail(ctx, q, userID) - return - } - code, err := generatePaymentsTwoFACode() - if err != nil { - log.Printf("2FA: failed to generate a re-issued code for user %s after a failed charge: %v", userID, err) - return - } - if _, err := q.Exec(ctx, ` - UPDATE users - SET two_factor_pending_code_hash = $2, - two_factor_pending_code_expires = $3 - WHERE id = $1 - `, userID, twofa.Hash(code), clock.Now().Add(twoFAPendingCodeLifetime)); err != nil { - log.Printf("2FA: failed to store a re-issued code for user %s after a failed charge: %v", userID, err) - return - } - st.SetLastMintAtLocked(clock.Now()) - // Delivery is build-dependent (twofa_delivery_dev.go / twofa_delivery_prod.go), - // mirroring the user package's twoFADeliverCode: dev/test builds always write - // the [2FA] log line (the operator relays the code); production writes it ONLY - // when the operator explicitly opted into log delivery - // (TWO_FACTOR_ALLOW_LOG_DELIVERY=true) — otherwise the plaintext code is never - // logged. - twoFAReissueDeliverCode(userID, code) -} - -// twoFAMintCooldown bounds how often the re-issue path mints a fresh 2FA code -// for one user after a failed saved-card charge, mirroring the user package's -// mint cooldown (handlers/user/twofa.go). The shared stamp lives on the -// per-user twofa.AttemptState.LastMintAt so both mint paths cohere. -// -// Round 2 Loop A finding 2: the stamp survives a successful gate verify -// (internal/twofa.Check no longer clears it) and is cleared only at TERMINAL -// charge success via twofa.ConsumePendingCode — so this check below is what -// actually bounds a charge-failure loop: after a fresh charge consumed a code -// at the gate and failed, the re-issue is skipped while the last mint is -// inside the cooldown (logged, not silent), bounding code churn + dev log -// flooding. Round 2 Loop B finding 6a — COORDINATION (money agent): the -// FRESH-charge terminal-success path in handlers.go does NOT call -// ConsumePendingCode (the gate already burned the code with consume=true), so -// its mint-cooldown stamp survives — a customer who completes a fresh charge -// within the cooldown of their last mint and immediately requests a new code -// gets 429 until the window elapses. To re-arm immediate re-minting after a -// completed fresh charge, the money agent should call -// twofa.ClearMintCooldownForUser(userID) on that terminal-success path (the -// exported, coordination-actionable entry point documented on -// internal/twofa.ClearMintCooldownForUser). -const twoFAMintCooldown = 1 * time.Minute - -// alertReissueFail surfaces a stranded-customer incident in the admin -// notification centre (reason 'critical_payment_log'). Round 2 Loop B finding -// 2: it is capped PER-ISSUE, NOT globally — the NOT EXISTS guard keeps exactly -// ONE unacknowledged row per (reason, user_id) (the booking is unresolvable at -// re-issue time), so one customer's alert is never suppressed by other users' -// rows filling the 'critical_payment_log' bucket, and the count-then-insert is -// atomic (a single INSERT ... WHERE NOT EXISTS — no TOCTOU). It is a dedicated -// local insert rather than the sweep's insertCriticalPaymentNotification so -// that the money agent's upcoming GLOBAL cap on the sweep insert (finding 1) -// can never swallow this alert — a stranded customer must always surface. -// Best-effort: a failure logs and the caller's CRITICAL log line still fires. -func alertReissueFail(ctx context.Context, q db.Querier, userID string) { - tag, err := q.Exec(ctx, ` - INSERT INTO admin_notifications (reason, user_id, created_at) - SELECT 'critical_payment_log', $1, NOW() - WHERE NOT EXISTS ( - SELECT 1 FROM admin_notifications an - WHERE an.reason = 'critical_payment_log' - AND an.user_id = $1 - AND an.acknowledged_at IS NULL - ) - `, userID) - if err != nil { - log.Printf("2FA: failed to insert critical-payment admin notification for reissue failure (user=%s): %v", userID, err) - return - } - if tag.RowsAffected() > 0 { - log.Printf("2FA: inserted critical-payment admin notification for reissue failure (user=%s) — customer stranded after a failed fresh saved-card charge", userID) - } -} - -// generatePaymentsTwoFACode returns a random 6-digit verification code, -// mirroring the user package's generator (crypto/rand, uniform 0-999999). -func generatePaymentsTwoFACode() (string, error) { - n, err := rand.Int(rand.Reader, big.NewInt(1_000_000)) - if err != nil { - return "", err - } - return fmt.Sprintf("%06d", n.Int64()), nil -} - -// twoFAPendingCodeLifetime is how long a re-issued 2FA code stays valid, -// mirroring the user package's pending-code expiry. -const twoFAPendingCodeLifetime = 10 * time.Minute - -// COORDINATION NOTE (Round 2 Loop B finding 1) — the shared notification flood -// cap now lives in crussell/internal/adminnotify -// (MaxUnacknowledgedCriticalLogs = 100 + CriticalLogsCapExceeded), applied -// ATOMICALLY (a conditional `INSERT ... SELECT ... WHERE (SELECT COUNT(*) ...) -// < $cap`) at every insert site this finding calls out. Status of each site: -// -// - THIS package's reissue-fail alert (reissueTwoFACodeAfterFailedCharge): -// capped PER-ISSUE instead (Round 2 Loop B finding 2) — the local -// alertReissueFail helper dedups atomically on (reason, user_id), so one -// customer's alert is never suppressed by other users' rows. NOT globally -// capped, and deliberately decoupled from sweep's generic insert so the -// money agent's global cap there can never swallow it. -// -// - auth/jwt.go VerifyRefreshToken's 'refresh_token_reuse' alert: NOW capped -// (Round 2 Loop B finding 1) — folded into its INSERT via the shared -// adminnotify cap. -// -// - handlers/webhooks/square.go (dispute/booking/unknown-event/orphan-replay) -// and handlers/user/account.go InsertSquareErasureCriticalNotification: -// NOW capped — same atomic fold. -// -// - handlers/payments/sweep.go:1556 insertCriticalPaymentNotification (the -// MONEY agent): STILL NEEDS the fold. Its INSERT ... SELECT ... WHERE NOT -// EXISTS is the same unbounded-across-accounts shape. Add -// `AND (SELECT COUNT(*) FROM admin_notifications _an WHERE _an.reason = -// 'critical_payment_log' AND _an.acknowledged_at IS NULL) < $N` (N = -// adminnotify.MaxUnacknowledgedCriticalLogs) to its WHERE clause. -// -// - handlers/scheduling/time-blockers.go:511 and internal/jobs/cleanup.go:333 -// (jobs/scheduling agent): same shape on the GDPR-cleanup and log-scan -// paths — apply the identical fold. -// -// - main.go has NO admin_notifications insert sites (it only mounts the -// notification read/ack routes), so nothing to cap there. -// -// Fail-closed behaviour (a REFUSED re-issue still CRITICAL-logs and leaves the -// operator to mint manually) is unchanged — only the notification INSERT is -// bounded. diff --git a/backend/handlers/payments/twofa_delivery_dev.go b/backend/handlers/payments/twofa_delivery_dev.go deleted file mode 100644 index 1a1a589..0000000 --- a/backend/handlers/payments/twofa_delivery_dev.go +++ /dev/null @@ -1,35 +0,0 @@ -//go:build dev || test - -package payments - -import "log" - -// twoFADeliveryAvailable reports whether a 2FA code delivery channel exists in -// this build. Dev/test builds always have one — the [2FA] log line is the -// documented loose-fake delivery channel — so the 2FA BACKUP authorization -// (the saved-card gate when SCA is unavailable) is always usable here. Mirrors -// handlers/user/twofa_dev.go; production builds decide in -// twofa_delivery_prod.go. -func twoFADeliveryAvailable() bool { return true } - -// twoFAReissueIssueAllowed is the re-issue path's issuance gate -// (reissueTwoFACodeAfterFailedCharge, handlers.go), mirroring the user -// package's twoFAEnsureIssueAllowed build-tagged semantics: dev/test builds -// always allow issuance — the [2FA] log line is the delivery channel and the -// unsalted-digest fallback is the documented loose-fake stand-in (matching -// twofa_dev.go). Production builds fail closed here — no pepper, no delivery -// channel, no codes (see twofa_delivery_prod.go). -func twoFAReissueIssueAllowed() error { return nil } - -// twoFAReissueDeliverCode delivers a re-issued code (a fresh saved-card charge -// consumed the customer's code at the gate and the charge failed at Square). -// Dev/test builds always deliver via the [2FA] log line — the documented -// loose-fake delivery channel — so the operator can relay the fresh code to the -// customer. Mirrors the user package's twoFADeliverCode; production logs it -// ONLY with the explicit TWO_FACTOR_ALLOW_LOG_DELIVERY opt-in (see -// twofa_delivery_prod.go). MEDIUM-3b: the user id and the plaintext code go to -// SEPARATE log lines so a single record cannot trivially pair them. -func twoFAReissueDeliverCode(userID, code string) { - log.Printf("[2FA] code delivery requested (user=%s, purpose=re-issue after failed saved-card charge)", userID) - log.Printf("[2FA] code: %s", code) -} diff --git a/backend/handlers/payments/twofa_delivery_prod.go b/backend/handlers/payments/twofa_delivery_prod.go deleted file mode 100644 index 768a498..0000000 --- a/backend/handlers/payments/twofa_delivery_prod.go +++ /dev/null @@ -1,63 +0,0 @@ -//go:build !dev && !test - -package payments - -import ( - "log" - "os" -) - -// twoFADeliveryAvailable reports whether a 2FA code delivery channel exists in -// this build. Production has no wired email/SMS transport (P6), so the ONLY -// channel is the operator's explicit opt-in to insecure log delivery -// (TWO_FACTOR_ALLOW_LOG_DELIVERY=true). Without a channel, codes can never -// reach the customer, so the 2FA BACKUP authorization (the saved-card gate -// when SCA is unavailable) cannot operate and a token-less saved-card charge is -// denied 503 (see requireTwoFactorForCardAccess). Delegates to the pure -// build-agnostic twoFADeliveryChannelConfigured (twofa.go); dev/test builds -// always deliver (twofa_delivery_dev.go). -func twoFADeliveryAvailable() bool { - return twoFADeliveryChannelConfigured() -} - -// twoFAReissueIssueAllowed is the re-issue path's issuance gate -// (reissueTwoFACodeAfterFailedCharge, handlers.go), mirroring the user -// package's twoFAEnsureIssueAllowed (handlers/user/twofa_prod.go) build-tagged -// semantics: production requires BOTH a delivery channel and TWO_FACTOR_PEPPER. -// Without a channel the code could never reach the customer, and without the -// pepper every stored code would be an offline-brute-forceable unsalted digest -// — either way the re-issue refuses (fail-closed), exactly like the interactive -// mint paths. Delegates to the pure build-agnostic gate -// twoFAReissueIssueAllowedStrict (twofa.go), which the test,dev suite also -// exercises directly; dev/test builds always allow issuance -// (twofa_delivery_dev.go). -// -// The pepper check is the ONLY hard gate on the re-issue (plus the delivery -// channel). PEPPER-CHANGE HAZARD (Loop B finding 2): the pepper keys the -// HMAC-SHA256 of every stored pending-code hash, so CHANGING TWO_FACTOR_PEPPER -// invalidates ALL pending codes — a re-issued code under the new pepper can -// never match a customer's code minted under the old one. An operator who -// changes the pepper must re-mint every user's code (or the customer must -// re-run 2FA setup), or a fresh saved-card charge whose code was consumed at -// the gate will strand the customer with 400 ErrMissingOrExpired on retry. -func twoFAReissueIssueAllowed() error { - return twoFAReissueIssueAllowedStrict() -} - -// twoFAReissueDeliverCode delivers a re-issued code after a failed fresh -// saved-card charge (the gate consumed the customer's code at verify time). -// Production's ONLY channel is the operator's explicit, insecure opt-in to log -// delivery (TWO_FACTOR_ALLOW_LOG_DELIVERY=true — anyone with backend log access -// could defeat the 2FA gate on saved-card charges). WITHOUT that flag the -// plaintext code is NEVER written to the log (issuance was already refused by -// twoFAReissueIssueAllowed, so this no-op is unreachable); with it, the code -// goes to the [2FA] log line for the operator to relay to the customer, exactly -// like the documented dev flow. MEDIUM-3b: the user id and the plaintext code -// go to SEPARATE log lines so a single record cannot trivially pair them. -func twoFAReissueDeliverCode(userID, code string) { - if os.Getenv("TWO_FACTOR_ALLOW_LOG_DELIVERY") == "true" { - log.Printf("[2FA] code delivery requested (user=%s, purpose=re-issue after failed saved-card charge)", userID) - log.Printf("[2FA] code: %s", code) - } - // Otherwise: deliberate no-op — never log the plaintext code by default. -} diff --git a/backend/handlers/payments/twofa_delivery_prod_test.go b/backend/handlers/payments/twofa_delivery_prod_test.go deleted file mode 100644 index 73b5257..0000000 --- a/backend/handlers/payments/twofa_delivery_prod_test.go +++ /dev/null @@ -1,84 +0,0 @@ -//go:build !dev - -package payments - -// Tests for the PRODUCTION 2FA delivery predicate (twofa_delivery_prod.go). -// -// LIMITATION (documented): the 503 "2FA requires an email or SMS delivery -// channel" branch in requireTwoFactorForCardAccess (twofa.go:193) is only -// reachable when twoFADeliveryAvailable() returns false, which happens ONLY in -// a production build (!dev && !test). Under BOTH required test runs — the -// "test,dev" run and the "test,!dev" prod-shape run — the dev/test delivery -// variant (twofa_delivery_dev.go, build tag `dev || test`) is the compiled -// function and is trivially true, so the 503 branch cannot be exercised there. -// The two test invocations DO however compile this file, and the prod-variant -// marker (twofaDeliveryProdVariant) tells the test which delivery function is -// live: a genuine production build (no dev/test tags, e.g. `go test ./...`) -// compiles twofa_delivery_prod.go, and this test then asserts the real prod -// predicate end to end. - -import ( - "os" - "testing" - - "github.com/stretchr/testify/require" -) - -// TestTwoFADeliveryAvailable_ProdPredicate asserts the production gating that -// twofa_delivery_prod.go implements: TWO_FACTOR_ALLOW_LOG_DELIVERY unset → -// no channel (false), exactly "true" → channel (true), any other value → -// no channel. In a dev/test build the marker is false and the test skips, -// because the always-true dev variant is compiled and the 503 branch is -// unreachable (documented limitation — see the file header). -func TestTwoFADeliveryAvailable_ProdPredicate(t *testing.T) { - if !twofaDeliveryProdVariant { - t.Skip("twoFADeliveryAvailable() is the dev/test build's trivially-true variant (twofa_delivery_dev.go, `dev || test`); the 503 delivery-unavailable branch is unreachable under the test tag — see the file header for the documented limitation") - } - - t.Run("unset_env_is_no_channel", func(t *testing.T) { - os.Unsetenv("TWO_FACTOR_ALLOW_LOG_DELIVERY") - require.False(t, twoFADeliveryAvailable(), "production without the explicit opt-in must have NO 2FA delivery channel") - }) - - t.Run("empty_env_is_no_channel", func(t *testing.T) { - os.Setenv("TWO_FACTOR_ALLOW_LOG_DELIVERY", "") - require.False(t, twoFADeliveryAvailable()) - }) - - t.Run("exact_true_is_a_channel", func(t *testing.T) { - os.Setenv("TWO_FACTOR_ALLOW_LOG_DELIVERY", "true") - require.True(t, twoFADeliveryAvailable(), "the explicit insecure log-delivery opt-in must open the channel") - }) - - t.Run("any_other_value_is_no_channel", func(t *testing.T) { - for _, v := range []string{"1", "yes", "on", "True", "TRUE", "false"} { - os.Setenv("TWO_FACTOR_ALLOW_LOG_DELIVERY", v) - require.False(t, twoFADeliveryAvailable(), "value %q must NOT open the delivery channel (exact 'true' only)", v) - } - }) -} - -// TestTwoFAReissueIssueAllowed_ProdPredicate pins the finding 2 re-issue -// issuance gate in a genuine production build (no dev/test tags): it fails -// closed without TWO_FACTOR_PEPPER (an unsalted digest would be -// offline-brute-forceable) or without a delivery channel, and allows issuance -// only when both are configured. In a dev/test build the marker is false and -// the test skips, because the always-allowed dev variant is compiled -// (twofa_delivery_dev.go) — same documented limitation as the delivery -// predicate above. -func TestTwoFAReissueIssueAllowed_ProdPredicate(t *testing.T) { - if !twofaDeliveryProdVariant { - t.Skip("twoFAReissueIssueAllowed() is the dev/test build's always-allowed variant (twofa_delivery_dev.go, `dev || test`); the prod fail-closed branches are unreachable under the test tag — see the file header for the documented limitation") - } - - os.Unsetenv("TWO_FACTOR_PEPPER") - os.Unsetenv("TWO_FACTOR_ALLOW_LOG_DELIVERY") - require.Error(t, twoFAReissueIssueAllowed(), "a production re-issue without the pepper must fail closed") - - os.Setenv("TWO_FACTOR_PEPPER", "test-pepper") - os.Unsetenv("TWO_FACTOR_ALLOW_LOG_DELIVERY") - require.Error(t, twoFAReissueIssueAllowed(), "a production re-issue without a delivery channel must fail closed") - - os.Setenv("TWO_FACTOR_ALLOW_LOG_DELIVERY", "true") - require.NoError(t, twoFAReissueIssueAllowed(), "a production re-issue with both the pepper and a delivery channel is allowed") -} diff --git a/backend/handlers/payments/twofa_delivery_variant_dev_test.go b/backend/handlers/payments/twofa_delivery_variant_dev_test.go deleted file mode 100644 index cc07331..0000000 --- a/backend/handlers/payments/twofa_delivery_variant_dev_test.go +++ /dev/null @@ -1,11 +0,0 @@ -//go:build dev || test - -package payments - -// twofaDeliveryProdVariant reports whether the PRODUCTION delivery predicate -// (twofa_delivery_prod.go, !dev && !test) is the compiled function in this -// build. Under `dev` OR `test` tags the dev/test delivery variant -// (twofa_delivery_dev.go) is compiled instead — always true, so the 503 -// delivery-unavailable branch is unreachable there. -//lint:ignore U1000 referenced only from the prod-tag test (twofa_delivery_prod_test.go, !dev && !test); deliberately unused under dev/test tags -const twofaDeliveryProdVariant = false diff --git a/backend/handlers/payments/twofa_delivery_variant_prod_test.go b/backend/handlers/payments/twofa_delivery_variant_prod_test.go deleted file mode 100644 index a24a2fd..0000000 --- a/backend/handlers/payments/twofa_delivery_variant_prod_test.go +++ /dev/null @@ -1,9 +0,0 @@ -//go:build !dev && !test - -package payments - -// twofaDeliveryProdVariant reports whether the PRODUCTION delivery predicate -// (twofa_delivery_prod.go, !dev && !test) is the compiled function in this -// build. True only in a genuine production build — neither dev nor test tag — -// where twoFADeliveryAvailable() gates on TWO_FACTOR_ALLOW_LOG_DELIVERY. -const twofaDeliveryProdVariant = true diff --git a/backend/handlers/payments/twofa_failclosed_test.go b/backend/handlers/payments/twofa_failclosed_test.go deleted file mode 100644 index 09ab6ca..0000000 --- a/backend/handlers/payments/twofa_failclosed_test.go +++ /dev/null @@ -1,60 +0,0 @@ -//go:build test - -package payments - -// M17 follow-up (ITEM 2): twofa_delivery_prod.go is excluded from the test,dev -// suite (`!dev && !test`), so its fail-closed re-issue branches were never -// exercised in CI — the old twofa_delivery_prod_test.go only runs its -// assertions in a genuine production build and skips under the test tag. The -// pure decision logic now lives build-agnostically in twofa.go -// (twoFAReissueIssueAllowedStrict / twoFAPepperConfigured / -// twoFADeliveryChannelConfigured); these tests exercise those branches in the -// STANDARD test,dev run, so a regression in the prod fail-closed behaviour is -// CI-visible even though the prod file itself is only compiled in a genuine -// production build. - -import ( - "testing" - - "github.com/stretchr/testify/require" -) - -// TestTwoFAReissueIssueAllowedStrict_FailClosed pins the production-style -// re-issue gate that twofa_delivery_prod.go's twoFAReissueIssueAllowed -// delegates to (reissueTwoFACodeAfterFailedCharge): (a) pepper unset → -// re-issue refused (errTwoFAPepperRequired — an unsalted digest in the 1M code -// space would be offline-brute-forceable); (b) delivery channel absent → -// re-issue refused (errTwoFADeliveryUnavailable — the 503-style error); -// (c) both configured → re-issue succeeds. -func TestTwoFAReissueIssueAllowedStrict_FailClosed(t *testing.T) { - t.Run("pepper_unset_refuses_reissue", func(t *testing.T) { - t.Setenv("TWO_FACTOR_PEPPER", "") - t.Setenv("TWO_FACTOR_ALLOW_LOG_DELIVERY", "true") - require.ErrorIs(t, twoFAReissueIssueAllowedStrict(), errTwoFAPepperRequired) - }) - - t.Run("delivery_channel_absent_refuses_reissue", func(t *testing.T) { - t.Setenv("TWO_FACTOR_PEPPER", "test-pepper") - t.Setenv("TWO_FACTOR_ALLOW_LOG_DELIVERY", "") - require.ErrorIs(t, twoFAReissueIssueAllowedStrict(), errTwoFADeliveryUnavailable) - }) - - t.Run("pepper_and_channel_present_allows_reissue", func(t *testing.T) { - t.Setenv("TWO_FACTOR_PEPPER", "test-pepper") - t.Setenv("TWO_FACTOR_ALLOW_LOG_DELIVERY", "true") - require.NoError(t, twoFAReissueIssueAllowedStrict()) - }) -} - -// TestPaymentsTwoFADeliveryChannelConfigured pins the pure delivery-channel -// predicate behind the payments 503 refusal: only the exact value "true" opens -// the channel. -func TestPaymentsTwoFADeliveryChannelConfigured(t *testing.T) { - t.Setenv("TWO_FACTOR_PEPPER", "test-pepper") - for _, v := range []string{"", "1", "yes", "on", "True", "TRUE", "false"} { - t.Setenv("TWO_FACTOR_ALLOW_LOG_DELIVERY", v) - require.False(t, twoFADeliveryChannelConfigured(), "value %q must NOT open the delivery channel (exact 'true' only)", v) - } - t.Setenv("TWO_FACTOR_ALLOW_LOG_DELIVERY", "true") - require.True(t, twoFADeliveryChannelConfigured()) -} diff --git a/backend/handlers/payments/twofa_gate_consume_test.go b/backend/handlers/payments/twofa_gate_consume_test.go index 6670c95..fa67a41 100644 --- a/backend/handlers/payments/twofa_gate_consume_test.go +++ b/backend/handlers/payments/twofa_gate_consume_test.go @@ -25,9 +25,7 @@ import ( "encoding/json" "net/http" "testing" - "time" - "crussell/db" "crussell/internal/square" "crussell/internal/twofa" "crussell/testutils" @@ -69,7 +67,6 @@ func TestTwoFactorEnforced_CreateTillSale_SavedCard_Tokenless_RefusedWithoutCons UserSavedCardID: &cardID, UserID: &userID, IdempotencyKey: "2fa-till-fresh-decline", - VerificationCode: "556677", } w := makePaymentRequest(CreateTillSale, "POST", "/api/admin/till/sale", req, adminToken, ctx) @@ -133,7 +130,6 @@ func TestTwoFactorEnforced_CreateTillSale_SavedCard_PendingReuse_Tokenless_Refus UserSavedCardID: &cardID, UserID: &userID, IdempotencyKey: key, - VerificationCode: "667788", } w := makePaymentRequest(CreateTillSale, "POST", "/api/admin/till/sale", req, adminToken, ctx) @@ -171,7 +167,6 @@ func TestTwoFactorEnforced_BuyGiftCard_SavedCard_Tokenless_RefusedWithoutConsumi RecipientType: "self", CardID: &cardID, IdempotencyKey: "2fa-buy-gc-fresh-decline", - VerificationCode: "112233", } w := makePaymentRequest(BuyGiftCard, "POST", "/api/user/giftcards/buy", req, token, ctx) @@ -221,7 +216,6 @@ func TestTwoFactorEnforced_BuyGiftCard_SavedCard_PendingReuse_Tokenless_Refused( RecipientType: "self", CardID: &cardID, IdempotencyKey: key, - VerificationCode: "334455", } w := makePaymentRequest(BuyGiftCard, "POST", "/api/user/giftcards/buy", req, token, ctx) @@ -258,7 +252,6 @@ func TestTwoFactorEnforced_CreateBookingPayment_NewCardSaveCard_Tokenless_Refuse NewCardToken: &cardToken, SaveCard: true, IdempotencyKey: "2fa-booking-newcard-savecard-decline", - VerificationCode: "999001", } w := makePaymentRequest(withNonGuest(CreateBookingPayment), "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) @@ -296,7 +289,6 @@ func TestTwoFactorEnforced_CreateTipPayment_NewCardSaveCard_Tokenless_Refused(t NewCardToken: &cardToken, SaveCard: true, IdempotencyKey: "2fa-tip-newcard-savecard-decline", - VerificationCode: "999002", } w := makePaymentRequest(withNonGuest(CreateTipPayment), "POST", "/api/bookings/"+bookingID+"/tip", req, userToken, ctx) @@ -307,47 +299,3 @@ func TestTwoFactorEnforced_CreateTipPayment_NewCardSaveCard_Tokenless_Refused(t require.True(t, hash.Valid, "the pending 2FA code must survive a refused token-less charge") require.Equal(t, twofa.Hash("999002"), hash.String, "the save gate must not consume the code — it was never consulted") } - -// TestReissueTwoFACodeAfterFailedCharge_RespectsMintCooldown pins the -// LOW-MEDIUM finding 2 contract on the retained re-issue helper: the re-issue -// mints a live code after a FRESH charge consumed one at the gate (in the -// pre-removal world), respects the same per-user mint cooldown as the -// interactive mint endpoints (a second re-issue inside the window is a no-op), -// and runs again once the cooldown elapses (simulated by clearing the shared -// stamp). The helper is retained because handlers.go / till.go / giftcards.go -// still call it with their (now always-false) fallbackUsed flag. -func TestReissueTwoFACodeAfterFailedCharge_RespectsMintCooldown(t *testing.T) { - t.Setenv("REQUIRE_2FA", "true") - t.Setenv("SQUARE_ENVIRONMENT", "staging") - ctx, tx := testutils.SetupTestTx(t) - - userID, err := fixtures.CreateTestUser(tx) - require.NoError(t, err) - - reissueTwoFACodeAfterFailedCharge(ctx, db.Conn, userID, true, true, nil) - var hash sql.NullString - require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_pending_code_hash FROM users WHERE id = $1", userID).Scan(&hash)) - require.True(t, hash.Valid, "a failed fresh saved-card charge must re-issue a live code for the same-key retry") - firstHash := hash.String - - reissueTwoFACodeAfterFailedCharge(ctx, db.Conn, userID, true, true, nil) - require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_pending_code_hash FROM users WHERE id = $1", userID).Scan(&hash)) - require.Equal(t, firstHash, hash.String, "a re-issue inside the mint cooldown must be a no-op (the stored code is untouched)") - - // Round 2 Loop B finding 6b: the cooldown-skipped re-issue must NOT be - // silent — the fresh charge consumed the customer's code at the gate, so - // they have NO live code for the same-key retry until the cooldown lapses. - // The per-issue-capped reissue-fail alert raises so the operator knows the - // customer is stranded (deduped on reason+user_id: one row per customer). - var alertCount int - require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM admin_notifications WHERE reason = 'critical_payment_log' AND user_id = $1 AND acknowledged_at IS NULL`, userID).Scan(&alertCount)) - require.Equal(t, 1, alertCount, "a cooldown-skipped re-issue after a fresh consumed charge must raise the reissue-fail alert (finding 6b)") - - st := twofa.StateFor(userID) - st.Mu.Lock() - st.LastMintAt = time.Time{} - st.Mu.Unlock() - reissueTwoFACodeAfterFailedCharge(ctx, db.Conn, userID, true, true, nil) - require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_pending_code_hash FROM users WHERE id = $1", userID).Scan(&hash)) - require.NotEqual(t, firstHash, hash.String, "an out-of-window re-issue must mint a fresh code") -} diff --git a/backend/handlers/payments/twofa_test.go b/backend/handlers/payments/twofa_test.go index c1d0de6..c91e923 100644 --- a/backend/handlers/payments/twofa_test.go +++ b/backend/handlers/payments/twofa_test.go @@ -119,7 +119,7 @@ func TestRequireTwoFactorForCardAccess_VerificationTokenSkips(t *testing.T) { require.NoError(t, err) req := httptest.NewRequest(http.MethodPost, "/", nil).WithContext(ctx) w := httptest.NewRecorder() - allowed, fallbackUsed := requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID, "", "vrf_sca_token_123", false) + allowed, fallbackUsed := requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID, "vrf_sca_token_123", false) require.True(t, allowed, "SCA performed — the gate must be skipped") require.False(t, fallbackUsed, "SCA is primary — no fallback is ever used") require.Equal(t, http.StatusOK, w.Code, "no denial response may be written when the token skips the gate") @@ -144,7 +144,7 @@ func TestRequireTwoFactorForCardAccess_TokenForwardedDistinction(t *testing.T) { _, err = tx.Exec(ctx, "UPDATE users SET two_factor_enabled = true WHERE id = $1", userID) require.NoError(t, err) w := httptest.NewRecorder() - allowed, fallbackUsed := requireTwoFactorForCardAccessWithTokenValidation(w, req, NewPaymentService(), userID, "", "forged-token", true, false) + allowed, fallbackUsed := requireTwoFactorForCardAccessWithTokenValidation(w, req, NewPaymentService(), userID, "forged-token", true, false) require.False(t, allowed, "a forged non-empty token must not skip the gate on the save path") require.False(t, fallbackUsed) require.Equal(t, http.StatusPaymentRequired, w.Code) @@ -158,7 +158,7 @@ func TestRequireTwoFactorForCardAccess_TokenForwardedDistinction(t *testing.T) { require.NoError(t, err) seedTwoFAPendingCode(t, tx, userID, "424242") w := httptest.NewRecorder() - allowed, fallbackUsed := requireTwoFactorForCardAccessWithTokenValidation(w, req, NewPaymentService(), userID, "424242", "forged-token", true, false) + allowed, fallbackUsed := requireTwoFactorForCardAccessWithTokenValidation(w, req, NewPaymentService(), userID, "forged-token", true, false) require.False(t, allowed, "a valid 2FA code cannot authorise a save (SCA-only — the 2FA fallback was removed)") require.False(t, fallbackUsed, "the 2FA fallback was removed — fallbackUsed is always false") require.Equal(t, http.StatusPaymentRequired, w.Code) @@ -171,7 +171,7 @@ func TestRequireTwoFactorForCardAccess_TokenForwardedDistinction(t *testing.T) { userID, err := fixtures.CreateTestUser(tx) require.NoError(t, err) w := httptest.NewRecorder() - allowed, _ := requireTwoFactorForCardAccessWithTokenValidation(w, req, NewPaymentService(), userID, "424242", "forged-token", true, false) + allowed, _ := requireTwoFactorForCardAccessWithTokenValidation(w, req, NewPaymentService(), userID, "forged-token", true, false) require.False(t, allowed, "a user without 2FA setup cannot save a card even with a token") require.Equal(t, http.StatusPaymentRequired, w.Code) var body map[string]string @@ -183,7 +183,7 @@ func TestRequireTwoFactorForCardAccess_TokenForwardedDistinction(t *testing.T) { userID, err := fixtures.CreateTestUser(tx) require.NoError(t, err) w := httptest.NewRecorder() - allowed, fallbackUsed := requireTwoFactorForCardAccessWithTokenValidation(w, req, NewPaymentService(), userID, "", "vrf_sca_token_123", false, true) + allowed, fallbackUsed := requireTwoFactorForCardAccessWithTokenValidation(w, req, NewPaymentService(), userID, "vrf_sca_token_123", false, true) require.True(t, allowed, "a token-forwarded charge path still skips on a non-empty token (SCA)") require.False(t, fallbackUsed) require.Equal(t, http.StatusOK, w.Code) @@ -247,7 +247,6 @@ func TestTwoFactorEnforced_CreateBookingPayment_SaveCard_ForgeToken_With2FA_Bloc NewCardToken: &cardToken, SaveCard: true, IdempotencyKey: "2fa-save-card-forge-token-ok", - VerificationCode: "112233", VerificationToken: &forged, } @@ -318,7 +317,7 @@ func TestRequireTwoFactorForCardAccess_Tokenless_402Structured(t *testing.T) { seedTwoFAPendingCode(t, tx, userID, "123456") req := httptest.NewRequest(http.MethodPost, "/", nil).WithContext(ctx) w := httptest.NewRecorder() - allowed, fallbackUsed := requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID, "123456", "", false) + allowed, fallbackUsed := requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID, "", false) require.False(t, allowed, "SCA-only: no 2FA fallback for a token-less charge") require.False(t, fallbackUsed, "fallbackUsed must be false — the 2FA fallback was removed") require.Equal(t, http.StatusPaymentRequired, w.Code) @@ -348,7 +347,6 @@ func TestTwoFactorEnforced_BookingSavedCard_Tokenless_402(t *testing.T) { PaymentType: "full", CardID: &cardID, IdempotencyKey: "2fa-fallback-audit", - VerificationCode: "445566", } w := makePaymentRequest(withNonGuest(CreateBookingPayment), "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) @@ -415,7 +413,6 @@ func TestTwoFactorEnforced_TipSavedCard_Tokenless_402(t *testing.T) { NewCardToken: &cardToken, SaveCard: true, IdempotencyKey: "2fa-tip-save-audit", - VerificationCode: "556600", } w := makePaymentRequest(withNonGuest(CreateTipPayment), "POST", "/api/bookings/"+bookingID+"/tip", req, userToken, ctx) @@ -445,7 +442,6 @@ func TestTwoFactorEnforced_TipChargeSavedCard_Tokenless_402(t *testing.T) { Amount: 500, CardID: &cardID, IdempotencyKey: "2fa-tip-charge-audit", - VerificationCode: "112211", } w := makePaymentRequest(withNonGuest(CreateTipPayment), "POST", "/api/bookings/"+bookingID+"/tip", req, userToken, ctx) @@ -475,7 +471,6 @@ func TestTwoFactorEnforced_BuyGiftCard_SavedCard_Tokenless_402(t *testing.T) { RecipientType: "self", CardID: &cardID, IdempotencyKey: key, - VerificationCode: "778811", } w := makePaymentRequest(BuyGiftCard, "POST", "/api/user/giftcards/buy", req, token, ctx) @@ -490,9 +485,10 @@ func TestTwoFactorEnforced_BuyGiftCard_SavedCard_Tokenless_402(t *testing.T) { } // TestTwoFactorEnforced_PaymentMethodSave_Tokenless_402 pins the SCA-only -// posture on the add-card save gate (handlers.go CreatePaymentMethod): the -// dedicated add-card endpoint carries no verification_token field, so a save is -// refused 402 verification_required even with a valid 2FA code (SCA-only). +// posture on the add-card save gate (handlers.go CreatePaymentMethod): a save +// from a NON-token-like source (a raw PAN — the gate's only remaining refusal +// shape) is refused 402 verification_required even with a valid 2FA code +// (SCA-only). A genuine token-like source is SCA-proven and skips the gate. func TestTwoFactorEnforced_PaymentMethodSave_Tokenless_402(t *testing.T) { helperEnvEnforce2FA(t) ctx, tx := testutils.SetupTestTx(t) @@ -503,8 +499,7 @@ func TestTwoFactorEnforced_PaymentMethodSave_Tokenless_402(t *testing.T) { seedTwoFAPendingCode(t, tx, userID, "998877") req := CreatePaymentMethodRequest{ - CardToken: "cnon:2fa-pm-save-audit", - VerificationCode: "998877", + CardToken: "4111111111111111", } w := makePaymentRequest(CreatePaymentMethod, "POST", "/api/user/payment-methods", req, token, ctx) @@ -527,7 +522,7 @@ func TestRequireTwoFactorForCardAccess_NotEnforced(t *testing.T) { t.Setenv("SQUARE_ENVIRONMENT", "mock") req := httptest.NewRequest(http.MethodPost, "/", nil) w := httptest.NewRecorder() - allowed, fallbackUsed := requireTwoFactorForCardAccess(w, req, nil, "000000000001", "", "", false) + allowed, fallbackUsed := requireTwoFactorForCardAccess(w, req, nil, "000000000001", "", false) require.True(t, allowed, "no response must be written when not enforced") require.False(t, fallbackUsed, "not enforced — no fallback is ever used") require.Equal(t, http.StatusOK, w.Code) @@ -546,7 +541,7 @@ func TestRequireTwoFactorForCardAccess_Enforced(t *testing.T) { require.NoError(t, err) req := httptest.NewRequest(http.MethodPost, "/", nil).WithContext(ctx) w := httptest.NewRecorder() - ok, _ := requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID, "123456", "", false) + ok, _ := requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID, "", false) require.False(t, ok) require.Equal(t, http.StatusPaymentRequired, w.Code) var body map[string]string @@ -561,7 +556,7 @@ func TestRequireTwoFactorForCardAccess_Enforced(t *testing.T) { require.NoError(t, err) req := httptest.NewRequest(http.MethodPost, "/", nil).WithContext(ctx) w := httptest.NewRecorder() - ok, _ := requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID, "", "", false) + ok, _ := requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID, "", false) require.False(t, ok) require.Equal(t, http.StatusPaymentRequired, w.Code) }) @@ -572,7 +567,7 @@ func TestRequireTwoFactorForCardAccess_Enforced(t *testing.T) { seedTwoFAPendingCode(t, tx, userID, "424242") req := httptest.NewRequest(http.MethodPost, "/", nil).WithContext(ctx) w := httptest.NewRecorder() - allowed, fallbackUsed := requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID, "424242", "", false) + allowed, fallbackUsed := requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID, "", false) require.False(t, allowed, "a valid 2FA code cannot authorise a token-less charge (SCA-only)") require.False(t, fallbackUsed, "fallbackUsed must be false — the 2FA fallback was removed") require.Equal(t, http.StatusPaymentRequired, w.Code) @@ -584,7 +579,7 @@ func TestRequireTwoFactorForCardAccess_Enforced(t *testing.T) { seedTwoFAPendingCode(t, tx, userID, "424242") req := httptest.NewRequest(http.MethodPost, "/", nil).WithContext(ctx) w := httptest.NewRecorder() - ok, _ := requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID, "000000", "", false) + ok, _ := requireTwoFactorForCardAccess(w, req, NewPaymentService(), userID, "", false) require.False(t, ok, "the gate does not verify codes anymore — any token-less charge is refused 402") require.Equal(t, http.StatusPaymentRequired, w.Code) }) @@ -592,7 +587,7 @@ func TestRequireTwoFactorForCardAccess_Enforced(t *testing.T) { t.Run("unknown_user_writes_402_json", func(t *testing.T) { req := httptest.NewRequest(http.MethodPost, "/", nil).WithContext(ctx) w := httptest.NewRecorder() - ok, _ := requireTwoFactorForCardAccess(w, req, NewPaymentService(), "000000000000", "123456", "", false) + ok, _ := requireTwoFactorForCardAccess(w, req, NewPaymentService(), "000000000000", "", false) require.False(t, ok) require.Equal(t, http.StatusPaymentRequired, w.Code) var body map[string]string @@ -682,7 +677,6 @@ func TestTwoFactorEnforced_CreateBookingPayment_SaveCard_With2FA_Blocked(t *test NewCardToken: &cardToken, SaveCard: true, IdempotencyKey: "2fa-save-card-ok", - VerificationCode: "112233", } w := makePaymentRequest(withNonGuest(CreateBookingPayment), "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) @@ -714,7 +708,6 @@ func TestTwoFactorEnforced_CreateBookingPayment_SavedCard_With2FA_Blocked(t *tes PaymentType: "full", CardID: &cardID, IdempotencyKey: "2fa-saved-card-ok", - VerificationCode: "334455", } w := makePaymentRequest(withNonGuest(CreateBookingPayment), "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) @@ -819,7 +812,6 @@ func TestTwoFactorEnforced_CreateTillSale_SavedCard_With2FA_Blocked(t *testing.T UserSavedCardID: &cardID, UserID: &userID, IdempotencyKey: "2fa-till-saved-ok", - VerificationCode: "556677", } bodyBytes, _ := json.Marshal(reqBody) @@ -870,17 +862,6 @@ func TestSCASuccess_BookingSavedCard_NeverRequiresConsent(t *testing.T) { require.Zero(t, auditCount, "an SCA-success charge must not write a fallback audit row") } -// TestTwoFADeliveryAvailable_DevBuild_TriviallyTrue documents the dev/test -// delivery predicate: twofa_delivery_dev.go (`dev || test`) always reports a -// delivery channel (the [2FA] log relay), so the 503 "2FA requires an email or -// SMS delivery channel" branch is UNREACHABLE in this build. The production -// predicate — TWO_FACTOR_ALLOW_LOG_DELIVERY gating — is covered by -// twofa_delivery_prod_test.go in a !dev build (see its header for the -// documented limitation). -func TestTwoFADeliveryAvailable_DevBuild_TriviallyTrue(t *testing.T) { - require.True(t, twoFADeliveryAvailable()) -} - // TestNotificationsCapExceeded pins Round 2 Loop B finding 1: the GLOBAL cap on // unacknowledged 'critical_payment_log' admin notifications // (adminnotify.MaxUnacknowledgedCriticalLogs, the shared cap living in @@ -890,11 +871,8 @@ func TestTwoFADeliveryAvailable_DevBuild_TriviallyTrue(t *testing.T) { // re-arms inserts. The same cap is now applied atomically (a conditional // INSERT ... SELECT ... WHERE (SELECT COUNT(*) ...) < $cap) at every // 'critical_payment_log' / 'refresh_token_reuse' insert site (webhooks, -// account erasure, jwt reuse); this test pins the shared count helper. NOTE: -// the REISSUE-FAIL alert (reissueTwoFACodeAfterFailedCharge) is intentionally -// NOT globally capped — Round 2 Loop B finding 2 caps it PER-ISSUE (dedup on -// reason+user_id) so one customer's alert is never suppressed by other users' -// rows. +// account erasure, jwt reuse, the payment sweep); this test pins the shared +// count helper. func TestNotificationsCapExceeded(t *testing.T) { ctx, tx := testutils.SetupTestTx(t) userID, err := fixtures.CreateTestUser(tx) diff --git a/backend/handlers/payments/validators.go b/backend/handlers/payments/validators.go index e040487..69c32ca 100644 --- a/backend/handlers/payments/validators.go +++ b/backend/handlers/payments/validators.go @@ -53,16 +53,31 @@ func ValidateRefundReason(reason string) error { return nil } -// ValidateCardInfo checks that exactly one of cardID or newCardToken is provided, -// non-nil, and non-empty. -func ValidateCardInfo(cardID, newCardToken *string) error { - hasCardID := cardID != nil && *cardID != "" +// ValidateCardInfo checks the card source shape of a payment request. Three +// shapes are legal: +// +// - saved-card only: a non-empty saved-card reference (card_id or +// saved_card_id — they are the same user_saved_cards.id, so callers pass +// the effective reference) with no new_card_token — a plain saved-card +// (ccof:) charge. +// - new-card only: a non-empty new_card_token with NO saved-card reference — +// a new-card (cnon:) one-off charge. +// - saved-card reference + new_card_token together: the SCA tokenize-result +// wire contract — the token (card.tokenize(verificationDetails, cardId) +// result) is the one-time charge SOURCE and the saved-card row supplies the +// Square customer. resolveChargeSource implements exactly this coexistence +// (see charge_helpers.go), so the validation must not reject it. +// +// Both absent is invalid ("either a card reference or new_card_token is +// required"). A bare new_card_token remains valid (the new-card path), and a +// card reference with no token remains valid (the legacy saved-card path); +// only the coexistence that used to be rejected — card ref + token — is now +// legal, resolved as the SCA tokenize-result source. +func ValidateCardInfo(cardID, savedCardID, newCardToken *string) error { + hasCardRef := (cardID != nil && *cardID != "") || (savedCardID != nil && *savedCardID != "") hasToken := newCardToken != nil && *newCardToken != "" - if hasCardID && hasToken { - return errors.New("provide either card_id or new_card_token, not both") - } - if !hasCardID && !hasToken { - return errors.New("either card_id or new_card_token is required") + if !hasCardRef && !hasToken { + return errors.New("either a saved card reference (card_id/saved_card_id) or new_card_token is required") } return nil } diff --git a/backend/handlers/payments/validators_test.go b/backend/handlers/payments/validators_test.go index 8686683..4acea89 100644 --- a/backend/handlers/payments/validators_test.go +++ b/backend/handlers/payments/validators_test.go @@ -3,6 +3,7 @@ package payments import ( + "strings" "testing" "github.com/stretchr/testify/require" @@ -33,3 +34,148 @@ func TestValidateAmount(t *testing.T) { t.Errorf("expected exactly £10,000 to be allowed, got %v", err) } } + +// TestValidateCardInfo_SCACoexistence pins the SCA saved-card wire contract on +// the card-source validator: a saved-card reference (card_id or saved_card_id) +// riding along WITH new_card_token is now VALID — the tokenize-result token is +// the one-time charge source and the saved-card row supplies the customer +// (resolveChargeSource implements the coexistence). A card reference alone +// (legacy saved-card), a token alone (new-card), empty references, and neither +// present keep their prior semantics. +func TestValidateCardInfo_SCACoexistence(t *testing.T) { + t.Parallel() + + cardID := "card-1" + savedCardID := "card-2" + token := "cnon:sca-tokenize" + empty := "" + + // card_id + new_card_token — now valid (SCA tokenize-result source). + require.NoError(t, ValidateCardInfo(&cardID, nil, &token), + "card_id + new_card_token must be valid (SCA tokenize-result)") + // saved_card_id + new_card_token — valid (SCA tokenize-result). + require.NoError(t, ValidateCardInfo(nil, &savedCardID, &token), + "saved_card_id + new_card_token must be valid (SCA tokenize-result)") + // card_id only — valid legacy saved-card charge. + require.NoError(t, ValidateCardInfo(&cardID, nil, nil), + "card_id alone must remain valid") + // new_card_token only — valid new-card charge. + require.NoError(t, ValidateCardInfo(nil, nil, &token), + "a bare new_card_token must remain valid (new-card path)") + // Neither present — invalid. + require.Error(t, ValidateCardInfo(nil, nil, nil), + "neither a card reference nor new_card_token must be rejected") + // Empty strings are treated as absent — invalid when nothing is present. + require.Error(t, ValidateCardInfo(&empty, &empty, &empty), + "empty-string values must be rejected as absent") + require.Error(t, ValidateCardInfo(&empty, nil, &empty), + "an empty card_id with an empty token must be rejected") + // An empty card reference with a real token is still the valid new-card path. + require.NoError(t, ValidateCardInfo(&empty, nil, &token), + "an empty card_id with a real new_card_token is the new-card path and must be valid") +} + +// TestValidateCardInfo_Table covers the full card-source shape space as a table: +// the three legal wire shapes (saved-card reference only, new-card token only, +// and both together as the SCA tokenize-result source) plus the both-absent and +// empty-string rejections. This is the drift guard for the SCA wire contract — +// every shape the frontend can send is enumerated here. +func TestValidateCardInfo_Table(t *testing.T) { + t.Parallel() + + cardID := "card-1" + savedCardID := "saved-2" + token := "cnon:sca-tokenize-3" + empty := "" + + tests := []struct { + name string + cardID *string + savedCard *string + token *string + wantValid bool + }{ + {"saved card reference (card_id) only — legacy saved-card charge", &cardID, nil, nil, true}, + {"saved card reference (saved_card_id) only", nil, &savedCardID, nil, true}, + {"new card token only — new-card charge", nil, nil, &token, true}, + {"saved-card reference + new card token — SCA tokenize-result source", &cardID, nil, &token, true}, + {"saved_card_id + new card token — SCA tokenize-result source", nil, &savedCardID, &token, true}, + {"neither present — invalid", nil, nil, nil, false}, + {"empty card_id + empty saved_card_id + empty token — all absent", &empty, &empty, &empty, false}, + {"empty card_id + empty token — absent card ref", &empty, nil, &empty, false}, + {"empty saved_card_id + empty token — absent card ref", nil, &empty, &empty, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := ValidateCardInfo(tt.cardID, tt.savedCard, tt.token) + if tt.wantValid { + require.NoErrorf(t, err, "expected shape to be valid: %v", tt) + } else { + require.Errorf(t, err, "expected shape to be rejected: %v", tt) + } + }) + } +} + +// TestValidatePartialAmount_PenceSemantics pins the pence comparison: amounts +// are integer pence, so the boundary is exact — equal pence passes, one penny +// over fails, and non-positive amounts are rejected regardless of the balance. +func TestValidatePartialAmount_PenceSemantics(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + amountPence int64 + remaining int64 + wantValid bool + }{ + {"partial equal to the remaining balance passes", 2500, 2500, true}, + {"partial below the remaining balance passes", 1000, 2500, true}, + {"one penny over the remaining balance fails", 2501, 2500, false}, + {"large partial over the balance fails", 5000, 2500, false}, + {"zero amount rejected even with balance", 0, 2500, false}, + {"negative amount rejected", -100, 2500, false}, + {"zero remaining rejects any positive partial", 1, 0, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := ValidatePartialAmount(tt.amountPence, tt.remaining) + if tt.wantValid { + require.NoErrorf(t, err, "expected %d pence against %d remaining to be valid", tt.amountPence, tt.remaining) + } else { + require.Errorf(t, err, "expected %d pence against %d remaining to be rejected", tt.amountPence, tt.remaining) + } + }) + } +} + +// TestValidateVerificationToken_Bound pins the 512-char bound on Square's +// verification token: empty/nil is fine (token-less charges are valid input), +// anything at or under 512 chars passes, and 513+ is rejected. +func TestValidateVerificationToken_Bound(t *testing.T) { + t.Parallel() + + ok := strings.Repeat("t", 512) + tooLong := strings.Repeat("t", 513) + + tests := []struct { + name string + token *string + wantErr bool + }{ + {"nil token passes (token-less is valid)", nil, false}, + {"empty token passes", strPtr(""), false}, + {"exactly 512 chars passes", &ok, false}, + {"513 chars rejected", &tooLong, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := ValidateVerificationToken(tt.token) + if tt.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + }) + } +} diff --git a/backend/handlers/payments/vat_test.go b/backend/handlers/payments/vat_test.go index 34affc3..39de3b9 100644 --- a/backend/handlers/payments/vat_test.go +++ b/backend/handlers/payments/vat_test.go @@ -3524,9 +3524,12 @@ func TestVAT_DiscountAndCashPayment_RemainingBalance(t *testing.T) { if summary.TotalNetAmount != 35.00 { t.Errorf("expected TotalNetAmount 35.00 (25.00 cash net + 10.00 discount), got %.2f", summary.TotalNetAmount) } - // RemainingAmount = total - paid = 100.00 - 40.00 = 60.00 - if summary.RemainingAmount != 60.00 { - t.Errorf("expected RemainingAmount 60.00 (100.00 - 40.00), got %.2f", summary.RemainingAmount) + // RemainingAmount routes through the authoritative + // GetBookingRemainingBalancePence, which counts REAL money only — the £10 + // discount row is a ledger entry, not a payment toward the balance — so + // remaining = 100.00 - 30.00 (cash) = 70.00. + if summary.RemainingAmount != 70.00 { + t.Errorf("expected RemainingAmount 70.00 (100.00 - 30.00 cash, discount not counted as paid), got %.2f", summary.RemainingAmount) } } diff --git a/backend/handlers/scheduling/scheduled_cleanup_test.go b/backend/handlers/scheduling/scheduled_cleanup_test.go index 1d83098..e8e5fb0 100644 --- a/backend/handlers/scheduling/scheduled_cleanup_test.go +++ b/backend/handlers/scheduling/scheduled_cleanup_test.go @@ -3,11 +3,14 @@ package scheduling import ( + "context" + "database/sql" "fmt" "testing" "time" "crussell/clock" + "crussell/db" "crussell/internal/adminnotify" "crussell/testutils/fixtures" ) @@ -677,6 +680,146 @@ func TestCleanupExpiredRefreshTokens_PreservesValid(t *testing.T) { } } +// ============================================================ +// ApplyScheduledDefaultHours Tests +// ============================================================ + +// seedStagedDefaultHoursChange inserts one pending default-hours change due on +// the given London date and returns its id. Weekday 1 (Monday) is staged to +// 10:00-18:00 open (the seeded default is 09:00-17:00 open), so an applied +// change is observable on the working_hours row. +func seedStagedDefaultHoursChange(t *testing.T, ctx context.Context, tx db.Querier, effectiveDate string) int { + t.Helper() + var id int + err := tx.QueryRow(ctx, ` + INSERT INTO default_hours_scheduled_changes (effective_date, hours) + VALUES ($1::date, $2::jsonb) + RETURNING id + `, effectiveDate, `[{"weekday":1,"startTime":"10:00","endTime":"18:00","isOpen":true}]`).Scan(&id) + if err != nil { + t.Fatalf("failed to seed default hours change: %v", err) + } + return id +} + +func getWorkingHoursForWeekday(t *testing.T, ctx context.Context, q db.Querier, weekday int) (start, end string, isOpen bool) { + t.Helper() + if err := q.QueryRow(ctx, `SELECT start_time, end_time, is_open FROM working_hours WHERE weekday = $1`, weekday).Scan(&start, &end, &isOpen); err != nil { + t.Fatalf("failed to read working_hours for weekday %d: %v", weekday, err) + } + return start, end, isOpen +} + +// TestApplyScheduledDefaultHours_AppliesStagedChange seeds a default-hours +// change with effective_date YESTERDAY (London), runs ApplyScheduledDefaultHours +// and asserts the working_hours row was updated, the change marked applied_at, +// and the flood-capped 'default_hours_changed' admin notification inserted. +func TestApplyScheduledDefaultHours_AppliesStagedChange(t *testing.T) { + ctx, tx := resetTestData(t) + + yesterday := LondonDateString(clock.Now().Add(-24 * time.Hour)) + changeID := seedStagedDefaultHoursChange(t, ctx, tx, yesterday) + + n, err := ApplyScheduledDefaultHours(ctx) + if err != nil { + t.Fatalf("ApplyScheduledDefaultHours failed: %v", err) + } + if n != 1 { + t.Errorf("expected 1 applied change, got %d", n) + } + + start, end, isOpen := getWorkingHoursForWeekday(t, ctx, tx, 1) + if start != "10:00:00" || end != "18:00:00" || !isOpen { + t.Errorf("expected working_hours weekday 1 updated to 10:00-18:00 open, got %q-%q open=%v", start, end, isOpen) + } + + var appliedAt sql.NullTime + if err := tx.QueryRow(ctx, `SELECT applied_at FROM default_hours_scheduled_changes WHERE id = $1`, changeID).Scan(&appliedAt); err != nil { + t.Fatalf("failed to read change applied_at: %v", err) + } + if !appliedAt.Valid { + t.Error("expected the scheduled change to be marked applied_at") + } + + var notifCount int + if err := tx.QueryRow(ctx, `SELECT COUNT(*) FROM admin_notifications WHERE reason = 'default_hours_changed'`).Scan(¬ifCount); err != nil { + t.Fatalf("failed to count notifications: %v", err) + } + if notifCount != 1 { + t.Errorf("expected 1 default_hours_changed admin notification, got %d", notifCount) + } +} + +// TestApplyScheduledDefaultHours_NoDueChange_Noop verifies the no-op path: with +// no change due (effective_date in the future) the job returns 0 and touches +// nothing. +func TestApplyScheduledDefaultHours_NoDueChange_Noop(t *testing.T) { + ctx, tx := resetTestData(t) + + tomorrow := LondonDateString(clock.Now().Add(24 * time.Hour)) + seedStagedDefaultHoursChange(t, ctx, tx, tomorrow) + + n, err := ApplyScheduledDefaultHours(ctx) + if err != nil { + t.Fatalf("ApplyScheduledDefaultHours failed: %v", err) + } + if n != 0 { + t.Errorf("expected 0 applied changes for a future effective_date, got %d", n) + } + + start, end, isOpen := getWorkingHoursForWeekday(t, ctx, tx, 1) + if start != "09:00:00" || end != "17:00:00" || !isOpen { + t.Errorf("expected working_hours untouched, got %q-%q open=%v", start, end, isOpen) + } +} + +// TestApplyScheduledDefaultHours_NotificationFloodCap pins C5 for the +// 'default_hours_changed' insert site: the unacknowledged queue is flood-capped +// at adminnotify.MaxUnacknowledgedCriticalLogs, so the change still applies +// (money/schedule-first) but no notification row is added past the cap. +func TestApplyScheduledDefaultHours_NotificationFloodCap(t *testing.T) { + ctx, tx := resetTestData(t) + + yesterday := LondonDateString(clock.Now().Add(-24 * time.Hour)) + changeID := seedStagedDefaultHoursChange(t, ctx, tx, yesterday) + + for i := 0; i < adminnotify.MaxUnacknowledgedCriticalLogs; i++ { + if _, err := tx.Exec(ctx, ` + INSERT INTO admin_notifications (reason, created_at) + VALUES ('default_hours_changed', NOW()) + `); err != nil { + t.Fatalf("failed to seed default_hours_changed notification %d: %v", i, err) + } + } + if !adminnotify.CriticalLogsCapExceeded(ctx, tx, "default_hours_changed") { + t.Fatal("expected the unacknowledged default_hours_changed queue to be at the cap") + } + + n, err := ApplyScheduledDefaultHours(ctx) + if err != nil { + t.Fatalf("ApplyScheduledDefaultHours failed: %v", err) + } + if n != 1 { + t.Errorf("expected the change to still apply at the notification cap, got %d", n) + } + + var appliedAt sql.NullTime + if err := tx.QueryRow(ctx, `SELECT applied_at FROM default_hours_scheduled_changes WHERE id = $1`, changeID).Scan(&appliedAt); err != nil { + t.Fatalf("failed to read change applied_at: %v", err) + } + if !appliedAt.Valid { + t.Error("expected the scheduled change to be marked applied_at despite the notification cap") + } + + var dbCount int + if err := tx.QueryRow(ctx, `SELECT COUNT(*) FROM admin_notifications WHERE reason = 'default_hours_changed'`).Scan(&dbCount); err != nil { + t.Fatalf("failed to count notifications: %v", err) + } + if dbCount != adminnotify.MaxUnacknowledgedCriticalLogs { + t.Errorf("expected the queue to stay capped at %d, got %d", adminnotify.MaxUnacknowledgedCriticalLogs, dbCount) + } +} + // ============================================================ // Multi-row Count Tests // ============================================================ diff --git a/backend/handlers/webhooks/square.go b/backend/handlers/webhooks/square.go index 34577d4..0b48f60 100644 --- a/backend/handlers/webhooks/square.go +++ b/backend/handlers/webhooks/square.go @@ -826,16 +826,28 @@ func squarePaymentKnown(ctx context.Context, squarePaymentID string) (bool, erro // verbatim, preserving both). Only 'pending' rows WITHOUT a square_payment_id // are candidates — that is exactly the population the keyed stale-pending sweep // replays (sweep.go), so a match is the sweep-minted duplicate's origin. +// +// AGE GATE (webhook-vs-response race): the candidates are additionally limited +// to rows old enough to have actually been replayed by the sweep +// (created_at <= NOW() - payments.SweepKeyedReplayAge()). The sweep only +// replays keyed rows past that age (sweep.go stalePendingKeyedAge), so a fresh +// pending row can never be a sweep-minted duplicate's origin — it is a legit +// charge whose completion webhook raced the handler's own square_payment_id +// write (response loss), and marking it failed would kill the customer's real +// payment. When the gate blocks a match the caller leaves the row pending +// (critical-log), never fails it — the sweep will reconcile it later. func findPendingByOrphanKeys(ctx context.Context, payment squarePaymentPayload) (paymentID, bookingID string, found bool, err error) { + replayEligibleSince := time.Now().Add(-payments.SweepKeyedReplayAge()) if payment.IdempotencyKey != "" { var pid string var bid *string err := db.Conn.QueryRow(ctx, ` SELECT id, booking_id FROM payments WHERE status = 'pending' AND idempotency_key = $1 AND square_payment_id IS NULL + AND created_at <= $2 ORDER BY created_at DESC, id DESC LIMIT 1 - `, payment.IdempotencyKey).Scan(&pid, &bid) + `, payment.IdempotencyKey, replayEligibleSince).Scan(&pid, &bid) if err == nil { if bid != nil { bookingID = *bid @@ -859,9 +871,10 @@ func findPendingByOrphanKeys(ctx context.Context, payment squarePaymentPayload) WHERE status = 'pending' AND square_payment_id IS NULL AND ABS(amount - $2) < 0.005 AND (booking_id = $1 OR gift_card_id = $1) + AND created_at <= $3 ORDER BY created_at DESC, id DESC LIMIT 1 - `, payment.ReferenceID, amount).Scan(&pid, &bid) + `, payment.ReferenceID, amount, replayEligibleSince).Scan(&pid, &bid) if err == nil { if bid != nil { bookingID = *bid @@ -957,6 +970,30 @@ func detectOrphanedReplayCharge(ctx context.Context, payment squarePaymentPayloa log.Printf("[SQUARE-WEBHOOK] payment.updated: COMPLETED square payment %s matches no local row and no pending origin row by idempotency key/reference_id — acknowledging (not a sweep-minted duplicate)", payment.ID) return nil } + // B1-EVIDENCE GATE: only treat the origin as a sweep-minted duplicate when + // the sweep actually minted+attempted to refund it (b1_attempts > 0 or a + // refunds row with the sweep-duplicate reason). Without that evidence a + // COMPLETED payment is far more likely the ORIGINAL charge whose completion + // webhook was delayed past the age gate — marking the origin failed would + // kill a customer's real payment. Leave it pending: the keyed sweep rescues + // the original 'completed' or triggers B1 if a duplicate was truly minted. + var b1Evidence bool + if err := db.Conn.QueryRow(ctx, ` + SELECT EXISTS( + SELECT 1 FROM payments + WHERE id = $1 AND b1_attempts > 0 + UNION ALL + SELECT 1 FROM refunds + WHERE payment_id = $1 AND reason = 'duplicate charge — sweep replay' + ) + `, originID).Scan(&b1Evidence); err != nil { + log.Printf("[SQUARE-WEBHOOK] Orphan-replay B1-evidence lookup failed for origin payment %s: %v", originID, err) + return err + } + if !b1Evidence { + log.Printf("[SQUARE-WEBHOOK] payment.updated: COMPLETED square payment %s matches pending origin %s by idempotency key but NO B1 evidence (b1_attempts=0, no sweep-duplicate refund) — leaving origin pending for the sweep to reconcile (delayed legit completion or the sweep has not replayed yet); not marking failed", payment.ID, originID) + return nil + } tag, err := db.Conn.Exec(ctx, `UPDATE payments SET status = 'failed', updated_at = NOW() WHERE id = $1 AND status = 'pending'`, originID) diff --git a/backend/handlers/webhooks/webhooks_completion_asymmetry_test.go b/backend/handlers/webhooks/webhooks_completion_asymmetry_test.go index cd174a0..5a3146a 100644 --- a/backend/handlers/webhooks/webhooks_completion_asymmetry_test.go +++ b/backend/handlers/webhooks/webhooks_completion_asymmetry_test.go @@ -77,7 +77,7 @@ func TestWebhook_AndSweep_DoNotDoubleComplete(t *testing.T) { event := SquareWebhookEvent{ Type: "payment.updated", EventID: "evt_webhook_and_sweep_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "payment", "id": "` + squarePaymentID + `", diff --git a/backend/handlers/webhooks/webhooks_dispatch_error_test.go b/backend/handlers/webhooks/webhooks_dispatch_error_test.go index 5eef0fc..0aa781c 100644 --- a/backend/handlers/webhooks/webhooks_dispatch_error_test.go +++ b/backend/handlers/webhooks/webhooks_dispatch_error_test.go @@ -27,7 +27,7 @@ func TestHandleSquareWebhook_DispatchError_NoDedup_RetryReDispatches(t *testing. overflowEvent := SquareWebhookEvent{ Type: "dispute.created", EventID: "evt_dispatch_err_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "dispute", "id": "dts_dispatch_err_1", @@ -55,7 +55,7 @@ func TestHandleSquareWebhook_DispatchError_NoDedup_RetryReDispatches(t *testing. retryEvent := SquareWebhookEvent{ Type: "dispute.created", EventID: "evt_dispatch_err_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "dispute", "id": "dts_dispatch_err_1", diff --git a/backend/handlers/webhooks/webhooks_round7_test.go b/backend/handlers/webhooks/webhooks_round7_test.go index 7a01c98..ba61376 100644 --- a/backend/handlers/webhooks/webhooks_round7_test.go +++ b/backend/handlers/webhooks/webhooks_round7_test.go @@ -144,7 +144,7 @@ func TestWebhook_DisputeStateUpdated_Lost_EmptyPaymentID_FallsBackToDisputeRow(t event := SquareWebhookEvent{ Type: "dispute.state.updated", EventID: "evt_dispute_fallback_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "dispute", "id": "dts_fallback_1", @@ -194,7 +194,7 @@ func TestWebhook_DisputeStateUpdated_EmptyPaymentID_NoDisputeRow_RaisesCritical( event := SquareWebhookEvent{ Type: "dispute.state.updated", EventID: "evt_dispute_no_row_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "dispute", "id": "dts_no_dispute_row_1", @@ -310,7 +310,7 @@ func TestWebhook_DisputeEvidence_InformationalOnly(t *testing.T) { event := SquareWebhookEvent{ Type: tc.eventType, EventID: tc.eventID, - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "dispute", "id": "` + tc.disputeID + `", @@ -348,7 +348,7 @@ func TestWebhook_DisputeEvidence_InformationalOnly(t *testing.T) { event := SquareWebhookEvent{ Type: "dispute.evidence.created", EventID: "evt_evidence_log_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "dispute", "id": "dts_evidence_log_1", @@ -379,7 +379,7 @@ func TestWebhook_TerminalCheckout_InformationalOnly(t *testing.T) { event := SquareWebhookEvent{ Type: tc.eventType, EventID: tc.eventID, - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{"type": "terminal.checkout", "id": "` + tc.checkoutID + `"}`), } w := deliverWebhook(t, event) @@ -407,7 +407,7 @@ func TestWebhook_TerminalCheckout_InformationalOnly(t *testing.T) { event := SquareWebhookEvent{ Type: "terminal.checkout.updated", EventID: "evt_terminal_log_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{"type": "terminal.checkout", "id": "chk_round7_log_1"}`), } if w := deliverWebhook(t, event); w.Code != http.StatusOK { diff --git a/backend/handlers/webhooks/webhooks_state_test.go b/backend/handlers/webhooks/webhooks_state_test.go index 5fe90a6..58343d1 100644 --- a/backend/handlers/webhooks/webhooks_state_test.go +++ b/backend/handlers/webhooks/webhooks_state_test.go @@ -10,12 +10,22 @@ import ( "net/http/httptest" "strings" "testing" + "time" "unicode/utf8" + "crussell/clock" "crussell/db" "crussell/testutils/fixtures" ) +// nowInRFC3339 returns the current UTC instant (plus an offset) as an RFC3339 +// string. Webhook-event timestamps and gift-card/till-sale created_at values +// must be clock-relative so sweep/expiry-window logic stays correct forever +// instead of drifting against a hardcoded 2025 date. +func nowInRFC3339(offset time.Duration) string { + return clock.Now().Add(offset).UTC().Format(time.RFC3339) +} + // ============================================================================= // Helpers — DB-backed state assertions // ============================================================================= @@ -157,7 +167,7 @@ func deliverPaymentUpdatedFailed(t *testing.T, squarePaymentID string) *httptest event := SquareWebhookEvent{ Type: "payment.updated", EventID: "evt_" + squarePaymentID, - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "payment", "id": "` + squarePaymentID + `", @@ -202,7 +212,7 @@ func getGiftCardFunding(t *testing.T, id string) (totalFundsAdded, amountRemaini // does. func TestWebhook_PaymentUpdated_Failed_ClawsBackCreatedCard(t *testing.T) { const squarePaymentID = "sqp_clawback_create" - const cardCreatedAt = "2025-01-01T00:00:00Z" + cardCreatedAt := nowInRFC3339(0) saleID, giftCardID := createWebhookTestGiftCardAndSale(t, squarePaymentID, cardCreatedAt, cardCreatedAt, 40.00) w := deliverPaymentUpdatedFailed(t, squarePaymentID) @@ -222,7 +232,8 @@ func TestWebhook_PaymentUpdated_Failed_ClawsBackCreatedCard(t *testing.T) { // of the card and the sale is marked failed. func TestWebhook_PaymentUpdated_Failed_ClawsBackTopup(t *testing.T) { const squarePaymentID = "sqp_clawback_topup" - saleID, giftCardID := createWebhookTestGiftCardAndSale(t, squarePaymentID, "2025-01-01T00:00:00Z", "2025-01-02T00:00:00Z", 60.00) + cardCreatedAt := nowInRFC3339(0) + saleID, giftCardID := createWebhookTestGiftCardAndSale(t, squarePaymentID, cardCreatedAt, nowInRFC3339(24*time.Hour), 60.00) w := deliverPaymentUpdatedFailed(t, squarePaymentID) if w.Code != http.StatusOK { @@ -243,7 +254,8 @@ func TestWebhook_PaymentUpdated_Failed_ClawsBackTopup(t *testing.T) { // untouched. func TestWebhook_PaymentUpdated_Failed_AlreadyResolved_Skipped(t *testing.T) { const squarePaymentID = "sqp_clawback_resolved" - saleID, giftCardID := createWebhookTestGiftCardAndSale(t, squarePaymentID, "2025-01-01T00:00:00Z", "2025-01-01T00:00:00Z", 40.00) + createdAt := nowInRFC3339(0) + saleID, giftCardID := createWebhookTestGiftCardAndSale(t, squarePaymentID, createdAt, createdAt, 40.00) if _, err := db.Conn.Exec(context.Background(), "UPDATE till_sales SET status = 'completed', updated_at = NOW() WHERE id = $1", saleID); err != nil { t.Fatalf("failed to resolve till sale: %v", err) @@ -282,7 +294,7 @@ func TestWebhook_DisputeCreated_InsertsDisputeRow(t *testing.T) { event := SquareWebhookEvent{ Type: "dispute.created", EventID: "evt_dispute_created_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "dispute", "id": "dts_dispute_created_1", @@ -347,7 +359,7 @@ func TestWebhook_DisputeCreated_NoLocalPayment_NoRow(t *testing.T) { event := SquareWebhookEvent{ Type: "dispute.created", EventID: "evt_dispute_orphan_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "dispute", "id": "dts_orphan_1", @@ -407,7 +419,7 @@ func TestWebhook_DisputeCreated_Untracked_DistinctDisputes_DistinctNotifications event := SquareWebhookEvent{ Type: "dispute.created", EventID: fmt.Sprintf("evt_untracked_distinct_%d", i), - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "dispute", "id": "` + d.disputeID + `", @@ -455,7 +467,7 @@ func TestWebhook_DisputeCreated_Untracked_SameDisputeRedelivered_SingleNotificat event := SquareWebhookEvent{ Type: "dispute.created", EventID: eventID, - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "dispute", "id": "` + disputeID + `", @@ -494,7 +506,7 @@ func TestWebhook_DisputeCreated_LongReason_Truncated(t *testing.T) { event := SquareWebhookEvent{ Type: "dispute.created", EventID: "evt_dispute_longreason_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "dispute", "id": "dts_longreason_1", @@ -544,7 +556,7 @@ func TestWebhook_DisputeCreated_Utf8Reason_StoredValid(t *testing.T) { event := SquareWebhookEvent{ Type: "dispute.created", EventID: "evt_dispute_utf8_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "dispute", "id": "dts_utf8_1", @@ -594,7 +606,7 @@ func TestWebhook_DisputeStateUpdated_Lost_MarksPaymentFailed(t *testing.T) { event := SquareWebhookEvent{ Type: "dispute.state.updated", EventID: "evt_dispute_lost_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "dispute", "id": "dts_lost_1", @@ -633,7 +645,7 @@ func TestWebhook_DisputeStateUpdated_Won_KeepsPaymentCompleted(t *testing.T) { event := SquareWebhookEvent{ Type: "dispute.state.updated", EventID: "evt_dispute_won_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "dispute", "id": "dts_won_1", @@ -672,7 +684,7 @@ func TestWebhook_DisputeStateUpdated_Open_KeepsOpen(t *testing.T) { event := SquareWebhookEvent{ Type: "dispute.state.updated", EventID: "evt_dispute_open_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "dispute", "id": "dts_open_1", @@ -709,7 +721,7 @@ func TestWebhook_PaymentUpdated_UpdatesPaymentStatus(t *testing.T) { event := SquareWebhookEvent{ Type: "payment.updated", EventID: "evt_payment_updated_completed_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "payment", "id": "` + squarePaymentID + `", @@ -737,7 +749,7 @@ func TestWebhook_PaymentUpdated_FailedStatus(t *testing.T) { event := SquareWebhookEvent{ Type: "payment.updated", EventID: "evt_payment_updated_failed_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "payment", "id": "` + squarePaymentID + `", @@ -765,7 +777,7 @@ func TestWebhook_PaymentUpdated_NonTerminal_LeavesPending(t *testing.T) { event := SquareWebhookEvent{ Type: "payment.updated", EventID: "evt_payment_updated_approved_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "payment", "id": "` + squarePaymentID + `", @@ -798,7 +810,7 @@ func TestWebhook_PaymentUpdated_DoesNotRevertRefunded(t *testing.T) { event := SquareWebhookEvent{ Type: "payment.updated", EventID: "evt_payment_updated_refunded_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "payment", "id": "` + squarePaymentID + `", @@ -833,7 +845,7 @@ func TestWebhook_PaymentUpdated_Completed_RescuesPendingTillSale(t *testing.T) { event := SquareWebhookEvent{ Type: "payment.updated", EventID: "evt_payment_updated_till_rescue_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "payment", "id": "` + squarePaymentID + `", @@ -864,7 +876,7 @@ func TestWebhook_PaymentUpdated_IdempotentReplay(t *testing.T) { event := SquareWebhookEvent{ Type: "payment.updated", EventID: "evt_payment_updated_idem_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "payment", "id": "` + squarePaymentID + `", @@ -905,9 +917,15 @@ func TestWebhook_PaymentUpdated_IdempotentReplay(t *testing.T) { func createWebhookTestPendingOrigin(t *testing.T, idempotencyKey string) string { t.Helper() var id string + // The origin row is aged past the sweep's keyed-replay age + // (payments.SweepKeyedReplayAge — 22h): the orphan detection only treats a + // pending row as a sweep-minted duplicate's origin once the row is old + // enough that the sweep could have replayed it (a fresh pending row is a + // legit charge whose response was lost and must never be failed by a + // webhook that raced it). err := db.Conn.QueryRow(context.Background(), ` INSERT INTO payments (payment_type, payment_method, status, amount, idempotency_key, created_at, updated_at) - VALUES ('full', 'online_square', 'pending', 10.00, $1, NOW(), NOW()) + VALUES ('full', 'online_square', 'pending', 10.00, $1, NOW() - INTERVAL '23 hours', NOW()) RETURNING id `, idempotencyKey).Scan(&id) if err != nil { @@ -916,6 +934,18 @@ func createWebhookTestPendingOrigin(t *testing.T, idempotencyKey string) string return id } +// seedWebhookTestB1Evidence simulates the sweep having replayed the origin's +// expired key, minted a duplicate charge, and attempted its B1 auto-refund +// (b1_attempts > 0). The orphan-detection B1-evidence gate requires this +// before marking the origin failed. +func seedWebhookTestB1Evidence(t *testing.T, originID string) { + t.Helper() + if _, err := db.Conn.Exec(context.Background(), + `UPDATE payments SET b1_attempts = 1 WHERE id = $1`, originID); err != nil { + t.Fatalf("failed to seed b1_attempts on origin payment: %v", err) + } +} + func countOrphanReplayNotifications(t *testing.T, originID string) int { t.Helper() var n int @@ -938,11 +968,14 @@ func TestWebhook_PaymentUpdated_OrphanedReplay_MarksOriginFailed(t *testing.T) { idemKey = "b1-orphan-key-001" ) originID := createWebhookTestPendingOrigin(t, idemKey) + // The sweep replayed the origin's expired key and attempted the B1 + // auto-refund — the evidence the orphan-detection gate requires. + seedWebhookTestB1Evidence(t, originID) event := SquareWebhookEvent{ Type: "payment.updated", EventID: "evt_orphan_c2_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "payment", "id": "` + orphanSquareID + `", @@ -993,7 +1026,7 @@ func TestWebhook_PaymentUpdated_OrphanedReplay_NoOrigin_Noop(t *testing.T) { event := SquareWebhookEvent{ Type: "payment.updated", EventID: "evt_orphan_noorigin_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "payment", "id": "` + orphanSquareID + `", @@ -1019,6 +1052,50 @@ func TestWebhook_PaymentUpdated_OrphanedReplay_NoOrigin_Noop(t *testing.T) { } } +// TestWebhook_PaymentUpdated_OrphanedReplay_NoB1Evidence_LeavesPending +// verifies the B1-evidence gate: a COMPLETED payment.updated that matches a +// pending origin row by idempotency key but with NO B1 evidence (the sweep +// never replayed + auto-refunded) is treated as a delayed legit completion — +// the origin is LEFT pending, never marked failed, and no orphan notification +// is raised. The stale-pending sweep reconciles the row instead. +func TestWebhook_PaymentUpdated_OrphanedReplay_NoB1Evidence_LeavesPending(t *testing.T) { + const ( + orphanSquareID = "sqp_orphan_noevidence" + idemKey = "b1-orphan-key-no-evidence" + ) + originID := createWebhookTestPendingOrigin(t, idemKey) + + event := SquareWebhookEvent{ + Type: "payment.updated", + EventID: "evt_orphan_noevidence_1", + CreatedAt: nowInRFC3339(0), + Data: json.RawMessage(`{ + "type": "payment", + "id": "` + orphanSquareID + `", + "object": { + "payment": { + "id": "` + orphanSquareID + `", + "status": "COMPLETED", + "idempotency_key": "` + idemKey + `", + "amount_money": {"amount": 1000, "currency": "GBP"} + } + } + }`), + } + w := deliverWebhook(t, event) + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + // The origin must remain pending — the delayed legit completion must never + // be failed without sweep B1 evidence. + if got := getPaymentStatus(t, originID); got != "pending" { + t.Errorf("expected origin pending payment to stay 'pending', got %q", got) + } + if n := countOrphanReplayNotifications(t, originID); n != 0 { + t.Errorf("expected 0 orphan-replay notifications without B1 evidence, got %d", n) + } +} + // TestWebhook_PaymentUpdated_OrphanedReplay_ReferenceFallback locks the // reference_id fallback of the origin lookup: a COMPLETED orphan event whose // payload carries no idempotency key still finds its pending origin row via the @@ -1029,18 +1106,22 @@ func TestWebhook_PaymentUpdated_OrphanedReplay_ReferenceFallback(t *testing.T) { refID = "b16bad0000aa" ) var originID string + // The origin row is aged past the sweep's keyed-replay age (22h) so the + // orphan detection treats it as a sweep-replayable origin (a fresh pending + // row is never failed by the orphan detection — see the age gate). if err := db.Conn.QueryRow(context.Background(), ` INSERT INTO payments (payment_type, payment_method, status, amount, gift_card_id, created_at, updated_at) - VALUES ('full', 'online_square', 'pending', 10.00, $1, NOW(), NOW()) + VALUES ('full', 'online_square', 'pending', 10.00, $1, NOW() - INTERVAL '23 hours', NOW()) RETURNING id `, refID).Scan(&originID); err != nil { t.Fatalf("failed to create reference-origin payment: %v", err) } + seedWebhookTestB1Evidence(t, originID) event := SquareWebhookEvent{ Type: "payment.updated", EventID: "evt_orphan_refc2_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "payment", "id": "` + orphanSquareID + `", @@ -1078,7 +1159,7 @@ func TestWebhook_PaymentUpdated_SettledRow_NoOrphanDetection(t *testing.T) { event := SquareWebhookEvent{ Type: "payment.updated", EventID: "evt_settled_replay_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "payment", "id": "` + squarePaymentID + `", @@ -1119,7 +1200,7 @@ func TestWebhook_RefundUpdated_UpdatesRefundStatus(t *testing.T) { event := SquareWebhookEvent{ Type: "refund.updated", EventID: "evt_refund_updated_completed_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "refund", "id": "` + squareRefundID + `", @@ -1152,7 +1233,7 @@ func TestWebhook_RefundUpdated_FailedStatus(t *testing.T) { event := SquareWebhookEvent{ Type: "refund.updated", EventID: "evt_refund_updated_failed_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "refund", "id": "` + squareRefundID + `", @@ -1189,7 +1270,7 @@ func TestWebhook_RefundUpdated_RejectedStatus(t *testing.T) { event := SquareWebhookEvent{ Type: "refund.updated", EventID: "evt_refund_updated_rejected_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "refund", "id": "` + squareRefundID + `", @@ -1229,7 +1310,7 @@ func TestWebhook_RefundUpdated_Failed_RaisesAdminNotification(t *testing.T) { event := SquareWebhookEvent{ Type: "refund.updated", EventID: "evt_refund_updated_failed_notify_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "refund", "id": "` + squareRefundID + `", @@ -1275,7 +1356,7 @@ func TestWebhook_RefundUpdated_NonTerminal_LeavesPending(t *testing.T) { event := SquareWebhookEvent{ Type: "refund.updated", EventID: "evt_refund_updated_pending_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "refund", "id": "` + squareRefundID + `", @@ -1316,7 +1397,7 @@ func TestWebhook_RefundUpdated_Approved_IsNonTerminal(t *testing.T) { approved := SquareWebhookEvent{ Type: "refund.updated", EventID: "evt_refund_updated_approved_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "refund", "id": "` + squareRefundID + `", @@ -1342,7 +1423,7 @@ func TestWebhook_RefundUpdated_Approved_IsNonTerminal(t *testing.T) { failed := SquareWebhookEvent{ Type: "refund.updated", EventID: "evt_refund_updated_approved_fail_1", - CreatedAt: "2025-01-01T00:00:01Z", + CreatedAt: nowInRFC3339(time.Second), Data: json.RawMessage(`{ "type": "refund", "id": "` + squareRefundID + `", @@ -1387,7 +1468,7 @@ func TestWebhook_RefundUpdated_Completed_SweepDupResolvesParent(t *testing.T) { event := SquareWebhookEvent{ Type: "refund.updated", EventID: "evt_refund_sweepdup_parent_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "refund", "id": "` + squareRefundID + `", @@ -1418,7 +1499,8 @@ func TestWebhook_RefundUpdated_Completed_SweepDupResolvesParent(t *testing.T) { // gift card and mark the sale failed — mirroring the sweep's re-poll resolution. func TestWebhook_RefundUpdated_Completed_SweepDupResolvesTillSale(t *testing.T) { const squareRefundID = "sqr_sweepdup_tillsale" - saleID, giftCardID := createWebhookTestGiftCardAndSale(t, "sqp_sweepdup_tillsale_pay", "2025-01-01T00:00:00Z", "2025-01-01T00:00:00Z", 40.00) + createdAt := nowInRFC3339(0) + saleID, giftCardID := createWebhookTestGiftCardAndSale(t, "sqp_sweepdup_tillsale_pay", createdAt, createdAt, 40.00) // A synthetic completed payments row anchors the refund (mirrors // recordSweepDuplicateRefundRow); the till_sale id lives in the reason. @@ -1439,7 +1521,7 @@ func TestWebhook_RefundUpdated_Completed_SweepDupResolvesTillSale(t *testing.T) event := SquareWebhookEvent{ Type: "refund.updated", EventID: "evt_refund_sweepdup_tillsale_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "refund", "id": "` + squareRefundID + `", @@ -1482,7 +1564,7 @@ func TestWebhook_RefundUpdated_DoesNotDemoteCompleted(t *testing.T) { event := SquareWebhookEvent{ Type: "refund.updated", EventID: "evt_refund_demote_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "refund", "id": "` + squareRefundID + `", @@ -1524,7 +1606,7 @@ func TestWebhook_EventTypeAliases_RouteToUpdatedHandlers(t *testing.T) { payEvent := SquareWebhookEvent{ Type: "payment.created", EventID: "evt_alias_payment_created_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "payment", "id": "` + sqPayID + `", @@ -1554,7 +1636,7 @@ func TestWebhook_EventTypeAliases_RouteToUpdatedHandlers(t *testing.T) { refundEvent := SquareWebhookEvent{ Type: "refund.created", EventID: "evt_alias_refund_created_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "refund", "id": "sqr_alias_refund", diff --git a/backend/handlers/webhooks/webhooks_test.go b/backend/handlers/webhooks/webhooks_test.go index 2602d17..cd5d826 100644 --- a/backend/handlers/webhooks/webhooks_test.go +++ b/backend/handlers/webhooks/webhooks_test.go @@ -211,7 +211,7 @@ func TestHandleSquareWebhook_PaymentUpdated(t *testing.T) { event := SquareWebhookEvent{ Type: "payment.updated", EventID: "evt_payment_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{"id":"payment_1"}`), } body, _ := json.Marshal(event) @@ -229,7 +229,7 @@ func TestHandleSquareWebhook_RefundUpdated(t *testing.T) { event := SquareWebhookEvent{ Type: "refund.updated", EventID: "evt_refund_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{"id":"refund_1"}`), } body, _ := json.Marshal(event) @@ -244,7 +244,7 @@ func TestHandleSquareWebhook_DisputeCreated(t *testing.T) { event := SquareWebhookEvent{ Type: "dispute.created", EventID: "evt_dispute_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{"id":"dispute_1"}`), } body, _ := json.Marshal(event) @@ -267,7 +267,7 @@ func TestHandleSquareWebhook_UnknownEventType(t *testing.T) { event := SquareWebhookEvent{ Type: "frobnicator.created", EventID: "evt_unknown_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{"id":"frob_1"}`), } body, _ := json.Marshal(event) @@ -305,7 +305,7 @@ func TestHandleSquareWebhook_KnownNonMoneyEvent_Acknowledged(t *testing.T) { event := SquareWebhookEvent{ Type: "customer.created", EventID: "evt_nonmoney_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{"id":"cust_1"}`), } body, _ := json.Marshal(event) @@ -344,7 +344,7 @@ func TestHandleSquareWebhook_UnhandledMoneyEvent_NotAcknowledged(t *testing.T) { event := SquareWebhookEvent{ Type: "payment.checkout_offer_created", EventID: "evt_money_unhandled_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{"id":"pco_1"}`), } body, _ := json.Marshal(event) @@ -427,7 +427,7 @@ func TestHandleSquareWebhook_ValidSignatureWithEnvKey(t *testing.T) { // returns 503 without a dedup row (Square retries), which is NOT this // test's intent. The unique event_id and square_payment_id avoid colliding // with the other tests' dedup rows and payment fixtures. - body := []byte(`{"type":"payment.updated","event_id":"evt_envkey_1","created_at":"2025-01-01T00:00:00Z","data":{"object":{"payment":{"id":"sqp_env_key_1","status":"COMPLETED","amount_money":{"amount":5000,"currency":"GBP"},"updated_at":"2025-01-01T00:00:00Z"}}}}`) + body := []byte(fmt.Sprintf(`{"type":"payment.updated","event_id":"evt_envkey_1","created_at":%q,"data":{"object":{"payment":{"id":"sqp_env_key_1","status":"COMPLETED","amount_money":{"amount":5000,"currency":"GBP"},"updated_at":%q}}}}`, nowInRFC3339(0), nowInRFC3339(0))) key := "env-signing-key" notificationURL := "http://localhost:8080/webhooks/square" @@ -486,7 +486,7 @@ func TestHandleSquareWebhook_NoRawPayloadInLogs(t *testing.T) { event := SquareWebhookEvent{ Type: "payment.updated", EventID: "evt_pii_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{"id":"payment_pii_1","buyer_email_address":"secret@example.com", "card_details":{"card":{"brand":"VISA","last_4":"1234","cardholder_name":"Jane Doe"}}}`), } @@ -525,7 +525,7 @@ func TestHandleSquareWebhook_DuplicateEventID(t *testing.T) { event := SquareWebhookEvent{ Type: "payment.updated", EventID: "evt_http_dup_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{"id":"payment_dup_1"}`), } body, _ := json.Marshal(event) @@ -557,7 +557,7 @@ func TestHandleSquareWebhook_DistinctEventIDs(t *testing.T) { event := SquareWebhookEvent{ Type: "payment.updated", EventID: id, - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{"id":"p"}`), } body, _ := json.Marshal(event) @@ -621,7 +621,7 @@ func TestHandleSquareWebhook_DedupDispatchOnce(t *testing.T) { event := SquareWebhookEvent{ Type: "payment.updated", EventID: "evt_dispatch_once_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{"id":"payment_dispatch_once_1"}`), } body, _ := json.Marshal(event) @@ -663,7 +663,7 @@ func TestHandleSquareWebhook_DedupPersistsAcrossRestart(t *testing.T) { event := SquareWebhookEvent{ Type: "payment.updated", EventID: "evt_restart_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{"id":"payment_restart_1"}`), } body, _ := json.Marshal(event) @@ -718,7 +718,7 @@ func TestHandleSquareWebhook_ConcurrentSameEvent_Serialized(t *testing.T) { event := SquareWebhookEvent{ Type: "payment.updated", EventID: eventID, - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "payment", "id": "` + squarePaymentID + `", @@ -795,7 +795,7 @@ func TestHandleSquareWebhook_DedupCacheEviction_Redispatches(t *testing.T) { event := SquareWebhookEvent{ Type: "payment.updated", EventID: "evt_dedup_eviction_target", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{ "type": "payment", "id": "` + squarePaymentID + `", @@ -858,7 +858,7 @@ func TestHandleSquareWebhook_DedupInsertFails_FailsClosed(t *testing.T) { event := SquareWebhookEvent{ Type: "payment.updated", EventID: "evt_db_down_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{"id":"payment_db_down_1"}`), } body, _ := json.Marshal(event) @@ -888,7 +888,7 @@ func TestHandleSquareWebhook_DedupNilConn_FailsClosed(t *testing.T) { event := SquareWebhookEvent{ Type: "payment.updated", EventID: "evt_nil_conn_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{"id":"payment_nil_conn_1"}`), } body, _ := json.Marshal(event) @@ -920,7 +920,7 @@ func TestHandleSquareWebhook_EnvMismatch_Rejected(t *testing.T) { event := SquareWebhookEvent{ Type: "payment.updated", EventID: "evt_env_mismatch_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{"id":"payment_env_mismatch_1"}`), } body, _ := json.Marshal(event) @@ -942,7 +942,7 @@ func TestHandleSquareWebhook_EnvMatch_Accepted(t *testing.T) { event := SquareWebhookEvent{ Type: "payment.updated", EventID: "evt_env_match_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{"id":"payment_env_match_1"}`), } body, _ := json.Marshal(event) @@ -965,7 +965,7 @@ func TestHandleSquareWebhook_EnvHeaderAbsent_Allowed(t *testing.T) { event := SquareWebhookEvent{ Type: "payment.updated", EventID: "evt_env_absent_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{"id":"payment_env_absent_1"}`), } body, _ := json.Marshal(event) @@ -986,7 +986,7 @@ func TestHandleSquareWebhook_EnvMismatch_DevNotEnforced(t *testing.T) { event := SquareWebhookEvent{ Type: "payment.updated", EventID: "evt_env_dev_1", - CreatedAt: "2025-01-01T00:00:00Z", + CreatedAt: nowInRFC3339(0), Data: json.RawMessage(`{"id":"payment_env_dev_1"}`), } body, _ := json.Marshal(event) diff --git a/backend/internal/square/cards_pagination_test.go b/backend/internal/square/cards_pagination_test.go new file mode 100644 index 0000000..790c093 --- /dev/null +++ b/backend/internal/square/cards_pagination_test.go @@ -0,0 +1,74 @@ +//go:build test + +package square + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// TestGetCardsOnFileHTTP_MultiPage_Combined verifies cursor-based pagination of +// the saved-cards list: multiple pages are fetched and combined into one +// result, with the cursor threaded through to each subsequent request. This +// mirrors the ListRefunds two-page test — a regression dropping the cursor loop +// would silently return only the first 25-card page for a user with more. +func TestGetCardsOnFileHTTP_MultiPage_Combined(t *testing.T) { + var paths []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + paths = append(paths, r.URL.RawQuery) + w.Header().Set("Content-Type", "application/json") + if strings.Contains(r.URL.RawQuery, "cursor=page2") { + _, _ = w.Write([]byte(`{"cards":[{"id":"ccof_3","card_brand":"VISA","last_4":"9999","exp_month":12,"exp_year":2030,"fingerprint":"fp3","reference_id":"user_1","enabled":true,"version":1,"created_at":"2026-07-31T00:00:00Z"}],"cursor":""}`)) + return + } + _, _ = w.Write([]byte(`{"cards":[{"id":"ccof_1","card_brand":"VISA","last_4":"4242","exp_month":12,"exp_year":2030,"fingerprint":"fp1","reference_id":"user_1","enabled":true,"version":1,"created_at":"2026-07-31T00:00:00Z"},{"id":"ccof_2","card_brand":"MASTERCARD","last_4":"1111","exp_month":6,"exp_year":2029,"fingerprint":"fp2","reference_id":"user_1","enabled":true,"version":1,"created_at":"2026-07-31T00:00:00Z"}],"cursor":"page2"}`)) + })) + defer srv.Close() + + hc := &httpClient{baseURL: srv.URL, token: "t", http: srv.Client()} + cards, err := getCardsOnFileHTTPWithClient(context.Background(), "user_1", hc) + if err != nil { + t.Fatalf("getCardsOnFileHTTP failed: %v", err) + } + if len(paths) != 2 { + t.Fatalf("expected 2 pages fetched, got %d: %v", len(paths), paths) + } + if !strings.Contains(paths[0], "reference_id=user_1") { + t.Errorf("expected reference_id filter on the first request, got %q", paths[0]) + } + if !strings.Contains(paths[1], "cursor=page2") { + t.Errorf("expected the cursor threaded into the second request, got %q", paths[1]) + } + if len(cards) != 3 { + t.Fatalf("expected 3 cards combined across pages, got %d: %+v", len(cards), cards) + } + // Page 1's cards must come first, page 2's appended after. + if cards[0].CardID != "ccof_1" || cards[2].CardID != "ccof_3" { + t.Errorf("unexpected combined card order: %+v", cards) + } +} + +// TestGetCardsOnFileHTTP_NoCards_EmptySlice verifies the empty case returns a +// non-nil empty slice (callers range over the result without a nil guard). +func TestGetCardsOnFileHTTP_NoCards_EmptySlice(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"cards":[],"cursor":""}`)) + })) + defer srv.Close() + + hc := &httpClient{baseURL: srv.URL, token: "t", http: srv.Client()} + cards, err := getCardsOnFileHTTPWithClient(context.Background(), "user_none", hc) + if err != nil { + t.Fatalf("getCardsOnFileHTTP failed: %v", err) + } + if cards == nil { + t.Fatal("expected a non-nil empty slice for a user with no cards") + } + if len(cards) != 0 { + t.Errorf("expected 0 cards, got %d", len(cards)) + } +} diff --git a/backend/internal/square/square_dev.go b/backend/internal/square/square_dev.go index 7dbba37..c8e1993 100644 --- a/backend/internal/square/square_dev.go +++ b/backend/internal/square/square_dev.go @@ -451,8 +451,24 @@ func verificationTokenPrefixForSource(sourceID string) string { // frontend's MockCardForm mints for saved-card verification). A raw nonce like // "cnon:test-card" — whatever customer_id rides along — is NOT a tokenize-result, // and real Square rejects it as a card-on-file charge source. +// +// During the token-shape transition the frontend mock may still emit a +// verify_mock__[_ok|_deny] verification token in the +// tokenize-result source slot; that shape is accepted here too so dev remains +// walkable either way (parseVerifyToken resolves its binding). func isSCATokenizeResultSource(sourceID string) bool { - return strings.HasPrefix(sourceID, "cnon:sca-") + return strings.HasPrefix(sourceID, "cnon:sca-") || strings.HasPrefix(sourceID, "verify_mock_") +} + +// isTokenLikeMock is the dev mock's PCI-DSS token predicate: it accepts the +// production token shapes (cnon: nonces / ccof: card ids — isTokenLike) PLUS +// the verify_mock__ verification-token shape the dev frontend +// mints, so dev remains walkable while the frontend's card form transitions to +// minting cnon:sca- tokenize-results. The PRODUCTION client stays strict +// (square_http_client.go isTokenLike — cnon:/ccof: only); this mock-only +// widening never reaches real Square. +func isTokenLikeMock(s string) bool { + return isTokenLike(s) || strings.HasPrefix(s, "verify_mock_") } // resolveVerificationToken validates a supplied 3DS/SCA verification token for @@ -564,8 +580,10 @@ func (m *MockClient) CreatePayment(ctx context.Context, req CreatePaymentReq) (* body := mockPaymentWireBody(req) // Match the real Square API: source_id must be a token (cnon:xxx nonce or // ccof:xxx card ID). Raw PANs are rejected exactly as Square would, so the - // mock behaves identically to production (PCI-DSS parity). - if !isTokenLike(body.SourceID) { + // mock behaves identically to production (PCI-DSS parity). The mock's token + // predicate additionally accepts the verify_mock_* transition shape the dev + // frontend mints (isTokenLikeMock). + if !isTokenLikeMock(body.SourceID) { return nil, fmt.Errorf("invalid source_id: %s — use a card nonce (cnon:xxx) or card ID (ccof:xxx)", tokenPrefix(body.SourceID)) } // Square's CreatePayment requires a positive amount_money — a missing or @@ -1328,8 +1346,10 @@ func (m *MockClient) CreateCardOnFile(ctx context.Context, userID, cardToken, cu // Match the real Square API: source_id must be a token (cnon:xxx nonce or // ccof:xxx card ID). Raw PANs are rejected exactly as Square would, so the - // mock behaves identically to production. - if !isTokenLike(cardToken) { + // mock behaves identically to production. The mock's token predicate + // additionally accepts the verify_mock_* transition shape the dev frontend + // mints (isTokenLikeMock). + if !isTokenLikeMock(cardToken) { return nil, fmt.Errorf("invalid source_id: %s — use a card nonce (cnon:xxx) or card ID (ccof:xxx)", tokenPrefix(cardToken)) } diff --git a/init-scripts/init-script.sql b/init-scripts/init-script.sql index 69b01bb..e9917c7 100644 --- a/init-scripts/init-script.sql +++ b/init-scripts/init-script.sql @@ -277,7 +277,15 @@ CREATE TYPE verification_purpose AS ENUM ('email_verify', 'password_reset'); CREATE TABLE verification_codes ( id CHAR(12) PRIMARY KEY DEFAULT generate_verification_codes_id(), - code CHAR(12) NOT NULL UNIQUE DEFAULT generate_verification_code(), + -- code stores the hex digest (HMAC-SHA256 keyed by TWO_FACTOR_PEPPER, or + -- the legacy plain SHA-256 in dev/test when the pepper is unset — see + -- crussell/internal/twofa Hash) of the verification code, NOT the + -- plaintext: the old 12-hex-char plaintext column exposed the credential + -- at rest. The plaintext is generated server-side (handlers/auth/local.go), + -- delivered out-of-band (dev [VERIFY] log relay / future SMTP), and the + -- submitted code is hashed the same way for the lookup. 64 hex chars holds + -- the SHA-256 digest. + code CHAR(64) NOT NULL UNIQUE, user_id CHAR(12) NOT NULL REFERENCES users(id) ON DELETE CASCADE, purpose verification_purpose NOT NULL, expires_at TIMESTAMPTZ NOT NULL, @@ -799,7 +807,8 @@ CREATE TABLE discount_campaigns ( created_by CHAR(12) REFERENCES users(id) ON DELETE SET NULL, CONSTRAINT chk_dates CHECK (campaign_type = 'milestone' OR (start_date IS NOT NULL AND end_date IS NOT NULL AND end_date > start_date)), CONSTRAINT chk_discount CHECK (discount_percent > 0 AND discount_percent <= 100), - CONSTRAINT chk_milestone CHECK (campaign_type = 'time_based' OR (milestone_type IS NOT NULL AND milestone_value IS NOT NULL AND (milestone_type != 'anniversary' OR milestone_unit IS NOT NULL))) + CONSTRAINT chk_milestone CHECK (campaign_type = 'time_based' OR (milestone_type IS NOT NULL AND milestone_value IS NOT NULL AND (milestone_type != 'anniversary' OR milestone_unit IS NOT NULL))), + CONSTRAINT chk_times_redeemed CHECK (max_redemptions IS NULL OR times_redeemed <= max_redemptions) ); CREATE INDEX idx_discount_campaigns_dates ON discount_campaigns(start_date, end_date) WHERE start_date IS NOT NULL; @@ -829,6 +838,16 @@ CREATE INDEX idx_booking_discounts_user ON booking_discounts(user_id); CREATE INDEX idx_booking_discounts_milestone ON booking_discounts(user_id, milestone_type, source_id); CREATE INDEX idx_booking_discounts_booking_source ON booking_discounts(booking_id, discount_source); +-- Once-per-user backstop: the completion path guards the per-user booking-count +-- and anniversary milestones with a NOT EXISTS read; two concurrent completions +-- of DIFFERENT bookings of the same user can both pass that read. This partial +-- unique index makes the write-side race impossible — at most one milestone +-- redemption per user per campaign. Time_based and global_booking_count +-- campaigns are per-booking and legitimately repeat, so they are excluded. +CREATE UNIQUE INDEX uq_booking_discounts_user_milestone_campaign +ON booking_discounts(user_id, source_id) +WHERE discount_source = 'campaign' AND milestone_type IN ('per_user_booking_count', 'anniversary'); + -- ======================================= -- BUSINESS SETTINGS TABLE (FOR COMPLIANCE) -- Stores legal business info for receipts, VAT status, currency, etc.