diff --git a/backend/handlers/payments/giftcards_round10_adversarial_test.go b/backend/handlers/payments/giftcards_round10_adversarial_test.go new file mode 100644 index 0000000..2533a79 --- /dev/null +++ b/backend/handlers/payments/giftcards_round10_adversarial_test.go @@ -0,0 +1,242 @@ +//go:build test && dev + +package payments + +import ( + "database/sql" + "net/http" + "testing" + + "crussell/clock" + "crussell/internal/square" + "crussell/testutils" + "crussell/testutils/fixtures" + "crussell/testutils/jwt" + + "github.com/jackc/pgx/v5" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// ============================================================================= +// ROUND 10 — CancelGiftCard resume vs an in-flight / differently-amounted +// Square refund (adversarial double-refund probe) +// +// The resume path must inspect EVERY Square refund for the payment (pending +// AND completed, any amount), not just an exact-amount COMPLETED one: +// - a still-PENDING Square refund means money may still land — re-issuing +// under a fresh amount-derived key would mint a SECOND refund that lands +// on top of the first (double refund); +// - a COMPLETED refund at a DIFFERENT amount than the pending row claims +// still counts toward the entitlement — the exact-amount reconcile misses +// it and would re-issue money that has already moved. +// ============================================================================= + +// TestRound10_CancelGiftCard_Resume_PriorSquareRefundPending_NoReissue pins +// the in-flight guard: the prior attempt is still PENDING at Square and the +// entitlement has dropped (a £5 till spend between attempts). The resume must +// NOT re-issue a second Square refund — the row stays 'pending' for the sweep +// and the card stays live. Exploit before the fix: the exact-amount reconcile +// sees no COMPLETED £50 refund, re-issues £45 under a fresh key while the £50 +// is still in flight, and the customer is refunded £95 total. +func TestRound10_CancelGiftCard_Resume_PriorSquareRefundPending_NoReissue(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + token := jwt.GenerateTestToken(userID, "verified_email") + + origClient := SquareClient + mock := square.NewDevClient().(*square.MockClient) + counting := &countingRefundClient{SquareClient: mock} + SquareClient = counting + defer func() { SquareClient = origClient }() + + cardID, paymentID, squarePaymentID := seedGiftCardCancelResume(t, ctx, tx.(pgx.Tx), userID, 50.00, 5.00, "pending") + + // The prior attempt is STILL in flight at Square (PENDING). Seed it via + // the raw mock so it bypasses the counting wrapper's refund-call log. + mock.ForceRefundPending = true + _, err = counting.SquareClient.RefundPayment(ctx, square.RefundPaymentReq{ + PaymentID: squarePaymentID, + Amount: 5000, + IdempotencyKey: "pending-inflight-" + paymentID, + Reason: giftCardCancelRefundReason, + }) + require.NoError(t, err) + mock.ForceRefundPending = false + + w := round9CancelGiftCardWithPaymentID(t, ctx, tx.(pgx.Tx), token, cardID, paymentID) + require.Equal(t, http.StatusConflict, w.Code, "body: %s", w.Body.String()) + + // No second Square refund may be issued while a prior one is in flight: + // the handler minted none, and the mock ledger still holds exactly the one + // pre-seeded refund. + require.Empty(t, counting.refundCalls(), "no re-issue while a prior Square refund is still PENDING") + require.Equal(t, 1, mock.RefundKeyCount(), "exactly one Square refund minted for the payment — the pending one") + + // The refund row stays 'pending' (this request made no writes) and the + // card stays live at its spend-verified balance. + var status string + var amount float64 + require.NoError(t, tx.QueryRow(ctx, `SELECT status, amount FROM refunds WHERE payment_id = $1`, paymentID).Scan(&status, &amount)) + assert.Equal(t, "pending", status, "the row must remain pending for sweep reconciliation") + assert.Equal(t, 50.00, amount, "the row must not be rewritten with a new amount or key") + + var rem float64 + var expiry sql.NullTime + require.NoError(t, tx.QueryRow(ctx, `SELECT amount_remaining, expiry_date FROM gift_cards WHERE id = $1`, cardID).Scan(&rem, &expiry)) + assert.Equal(t, 45.00, rem, "the card must stay live with its spend-verified remaining balance") + require.True(t, expiry.Valid) + assert.True(t, expiry.Time.After(clock.Now()), "the card must not be expired") +} + +// TestRound10_CancelGiftCard_Resume_CompletedDifferentAmount_ResolvesAndNeutralizes +// pins the sum-based terminal reconcile: the prior attempt landed at Square at +// a DIFFERENT amount than the pending row claims (£45 of a £50 card after a £5 +// till spend — the pending row still says £50). The exact-amount reconcile +// (expecting £50) would miss the landed £45 and re-issue it. The resume must +// instead recognise the entitlement is already covered, resolve the row +// completed with the landed Square refund id, neutralise the card, and issue no +// new refund. +func TestRound10_CancelGiftCard_Resume_CompletedDifferentAmount_ResolvesAndNeutralizes(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + token := jwt.GenerateTestToken(userID, "verified_email") + + origClient := SquareClient + mock := square.NewDevClient().(*square.MockClient) + counting := &countingRefundClient{SquareClient: mock} + SquareClient = counting + defer func() { SquareClient = origClient }() + + cardID, paymentID, squarePaymentID := seedGiftCardCancelResume(t, ctx, tx.(pgx.Tx), userID, 50.00, 5.00, "pending") + + prelanded, err := counting.SquareClient.RefundPayment(ctx, square.RefundPaymentReq{ + PaymentID: squarePaymentID, + Amount: 4500, + IdempotencyKey: "prelanded-diff-amount-" + paymentID, + Reason: giftCardCancelRefundReason, + }) + require.NoError(t, err) + + w := round9CancelGiftCardWithPaymentID(t, ctx, tx.(pgx.Tx), token, cardID, paymentID) + require.Equal(t, http.StatusOK, w.Code, "body: %s", w.Body.String()) + assert.Contains(t, w.Body.String(), "already been refunded") + + require.Empty(t, counting.refundCalls(), "no new Square refund when the full entitlement already completed") + + var status string + var sqRefundID sql.NullString + require.NoError(t, tx.QueryRow(ctx, `SELECT status, square_refund_id FROM refunds WHERE payment_id = $1`, paymentID).Scan(&status, &sqRefundID)) + assert.Equal(t, "completed", status, "the pending row must resolve to completed") + assert.Equal(t, prelanded.ID, sqRefundID.String, "the row records the landed Square refund id") + + var rem float64 + var expiry sql.NullTime + require.NoError(t, tx.QueryRow(ctx, `SELECT amount_remaining, expiry_date FROM gift_cards WHERE id = $1`, cardID).Scan(&rem, &expiry)) + assert.Equal(t, 0.00, rem, "the card must be neutralised") + require.True(t, expiry.Valid) + assert.False(t, expiry.Time.After(clock.Now()), "the card must be expired") +} + +// TestRound10_CancelGiftCard_Resume_CompletedPartial_DifferenceOnlyReissued pins +// the difference-only re-issue: £30 of the £50 card already completed at +// Square, so the resume must issue ONLY the £20 remainder under a fresh +// deterministic key for that difference — never the full £50. +func TestRound10_CancelGiftCard_Resume_CompletedPartial_DifferenceOnlyReissued(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + token := jwt.GenerateTestToken(userID, "verified_email") + + origClient := SquareClient + mock := square.NewDevClient().(*square.MockClient) + counting := &countingRefundClient{SquareClient: mock} + SquareClient = counting + defer func() { SquareClient = origClient }() + + cardID, paymentID, squarePaymentID := seedGiftCardCancelResume(t, ctx, tx.(pgx.Tx), userID, 50.00, 0, "pending") + + prelanded, err := counting.SquareClient.RefundPayment(ctx, square.RefundPaymentReq{ + PaymentID: squarePaymentID, + Amount: 3000, + IdempotencyKey: "prelanded-partial-" + paymentID, + Reason: giftCardCancelRefundReason, + }) + require.NoError(t, err) + + w := round9CancelGiftCardWithPaymentID(t, ctx, tx.(pgx.Tx), token, cardID, paymentID) + require.Equal(t, http.StatusOK, w.Code, "body: %s", w.Body.String()) + + calls := counting.refundCalls() + require.Len(t, calls, 1, "exactly one Square refund: the outstanding difference") + assert.Equal(t, int64(2000), calls[0].Amount, "only the £20 difference may be re-issued") + assert.Equal(t, paymentID+"-gccancel-diff-2000", calls[0].IdempotencyKey, "a fresh deterministic key for the difference") + + var amount float64 + var status, key string + var sqRefundID sql.NullString + require.NoError(t, tx.QueryRow(ctx, ` + SELECT amount, status, idempotency_key, square_refund_id FROM refunds WHERE payment_id = $1 + `, paymentID).Scan(&amount, &status, &key, &sqRefundID)) + assert.Equal(t, 20.00, amount, "the refund row records the re-issued £20 difference") + assert.Equal(t, "completed", status) + assert.Equal(t, paymentID+"-gccancel-diff-2000", key, "the row carries the fresh difference key") + require.True(t, sqRefundID.Valid && sqRefundID.String != "", "the new Square refund id must be recorded") + assert.NotEqual(t, prelanded.ID, sqRefundID.String, "the row must carry the NEW refund id, not the pre-landed one") + + var rem float64 + var expiry sql.NullTime + require.NoError(t, tx.QueryRow(ctx, `SELECT amount_remaining, expiry_date FROM gift_cards WHERE id = $1`, cardID).Scan(&rem, &expiry)) + assert.Equal(t, 0.00, rem, "the card must be zeroed once the balance of the entitlement is refunded") + require.True(t, expiry.Valid) + assert.False(t, expiry.Time.After(clock.Now()), "the card must be expired") +} + +// TestRound10_CancelGiftCard_HappyPath_NoRegression pins the fresh-cancellation +// path (no prior refund row): the resume rewrite must not disturb the happy +// path — one Square refund for the full value, the row completed, the card +// neutralised. +func TestRound10_CancelGiftCard_HappyPath_NoRegression(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + token := jwt.GenerateTestToken(userID, "verified_email") + + origClient := SquareClient + mock := square.NewDevClient().(*square.MockClient) + counting := &countingRefundClient{SquareClient: mock} + SquareClient = counting + defer func() { SquareClient = origClient }() + + cardID, code := round9BuyGiftCardForFriend(t, ctx, tx.(pgx.Tx), token, 5000) + require.Equal(t, http.StatusCreated, code, "buy must succeed") + require.NotEmpty(t, cardID) + + var payID string + require.NoError(t, tx.QueryRow(ctx, ` + SELECT id FROM payments WHERE created_by = $1 AND payment_method = 'online_square' AND booking_id IS NULL + `, userID).Scan(&payID)) + + w := round9CancelGiftCardWithPaymentID(t, ctx, tx.(pgx.Tx), token, cardID, payID) + require.Equal(t, http.StatusOK, w.Code, "body: %s", w.Body.String()) + assert.Contains(t, w.Body.String(), "success") + + calls := counting.refundCalls() + require.Len(t, calls, 1, "exactly one Square refund for the fresh cancellation") + assert.Equal(t, int64(5000), calls[0].Amount, "the full purchase value must be refunded in pence") + + var status string + var amount float64 + require.NoError(t, tx.QueryRow(ctx, `SELECT status, amount FROM refunds WHERE payment_id = $1`, payID).Scan(&status, &amount)) + assert.Equal(t, "completed", status) + assert.Equal(t, 50.00, amount) + + var rem float64 + var expiry sql.NullTime + require.NoError(t, tx.QueryRow(ctx, `SELECT amount_remaining, expiry_date FROM gift_cards WHERE id = $1`, cardID).Scan(&rem, &expiry)) + assert.Equal(t, 0.00, rem, "the card must be zeroed after cancellation") + require.True(t, expiry.Valid) + assert.False(t, expiry.Time.After(clock.Now()), "the cancelled card must be expired") +} diff --git a/backend/handlers/payments/handlers_round10_adversarial_test.go b/backend/handlers/payments/handlers_round10_adversarial_test.go new file mode 100644 index 0000000..5a992aa --- /dev/null +++ b/backend/handlers/payments/handlers_round10_adversarial_test.go @@ -0,0 +1,246 @@ +//go:build test && dev + +package payments + +// Round-10 adversarial tests: the synchronous charge path vs the Square +// payment.completed webhook race. The webhook can win the booking FOR UPDATE +// lock between the Square call returning and the sync path's post-charge +// completion flip, resolving the payment row to 'completed' (and running the +// booking-completion side-effects) first. These tests simulate the webhook +// landing DURING the sync CreatePayment call and verify the sync path's +// guarded flips (R10) no-op instead of re-flipping the row, overwriting the +// ledger, or re-inserting split records. They follow the existing sequential +// (non-parallel) conventions of the money-path tests. + +import ( + "context" + "log" + "net/http" + "net/http/httptest" + "testing" + + "crussell/db" + "crussell/internal/square" + "crussell/testutils" + "crussell/testutils/jwt" + + "github.com/stretchr/testify/require" +) + +// webhookRaceClient wraps the dev Square client to simulate the +// payment.completed webhook landing DURING the synchronous CreatePayment call +// — the exact race window the R10 guards close. On a COMPLETED charge it +// resolves the just-created pending payment row for this booking to +// 'completed' BEFORE the sync handler's own post-charge completion flip runs. +// When simulateBookingCompletion is set it also completes the booking and +// awards the loyalty stamp, mirroring the webhook's booking-completion +// side-effects. +type webhookRaceClient struct { + square.SquareClient + bookingID string + userID string + simulateBookingCompletion bool +} + +func (c *webhookRaceClient) CreatePayment(ctx context.Context, req square.CreatePaymentReq) (*square.PaymentResult, error) { + res, err := c.SquareClient.CreatePayment(ctx, req) + if err != nil || res == nil || res.Status != "COMPLETED" { + return res, err + } + // The webhook completes the pending row for this charge (same booking + + // idempotency key) before the sync handler's flip runs. + if _, uErr := db.Conn.Exec(ctx, ` + UPDATE payments + SET status = 'completed', square_payment_id = $1 + WHERE booking_id = $2 AND idempotency_key = $3 AND status = 'pending' + `, res.SquarePayID, c.bookingID, req.IdempotencyKey); uErr != nil { + log.Printf("webhookRaceClient: failed to simulate payment.completed webhook: %v", uErr) + } + if c.simulateBookingCompletion { + if _, uErr := db.Conn.Exec(ctx, ` + UPDATE bookings SET status = 'completed', loyalty_stamp_awarded_at = NOW() + WHERE id = $1 + `, c.bookingID); uErr != nil { + log.Printf("webhookRaceClient: failed to simulate booking completion: %v", uErr) + } + if _, uErr := db.Conn.Exec(ctx, `UPDATE users SET loyalty_stamps = loyalty_stamps + 1 WHERE id = $1`, c.userID); uErr != nil { + log.Printf("webhookRaceClient: failed to simulate loyalty award: %v", uErr) + } + } + return res, err +} + +// TestCreateBookingPayment_WebhookCompletedFirst_NoLedgerOverwrite locks the +// R10 fix on the synchronous online booking path: when the Square +// payment.completed webhook wins the race and completes the pending payment +// row (plus the booking side-effects) before the sync completion flip, the +// sync path must NOT re-flip the already-completed row, OVERWRITE the primary +// row's amount/payment_type/VAT with buildSplitRecords' carved values, or +// re-insert split tip/balance rows. The response stays 200 (the payment IS +// completed) and the booking side-effects are not double-run. +func TestCreateBookingPayment_WebhookCompletedFirst_NoLedgerOverwrite(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + // VAT-registered so a buggy split/VAT overwrite would visibly clear or + // recompute the row's VAT fields. + _, err := tx.Exec(ctx, `UPDATE business_settings SET is_vat_registered = TRUE, default_vat_rate = 20.00`) + require.NoError(t, err) + + // Past-start booking (total £50, per the round-9 fixture), in_progress. + userID, bookingID, _ := setupTestDataPast(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + + origClient := SquareClient + SquareClient = &webhookRaceClient{ + SquareClient: square.NewDevClient(), + bookingID: bookingID, + userID: userID, + simulateBookingCompletion: true, + } + defer func() { SquareClient = origClient }() + + // £55 'full' on a £50 past-start booking: buildSplitRecords would carve + // [£50 booking, £5 tip]. The webhook completing the row FIRST must make + // the sync flip a no-op — no tip split inserted, primary not overwritten. + cardToken := "cnon:round10-webhook-first" + req := CreateBookingPaymentRequest{ + Amount: 5500, + PaymentType: "full", + NewCardToken: &cardToken, + IdempotencyKey: "round10-webhook-first-" + bookingID, + ConfirmOverflowTip: true, // B12: post-start overflow requires explicit confirmation + } + w := makePaymentRequest(CreateBookingPayment, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusOK, w.Code, "a webhook-resolved payment must still report success, body: %s", w.Body.String()) + + // Exactly ONE completed payment row — no duplicate split tip/balance rows. + var payCount int + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM payments WHERE booking_id = $1`, bookingID).Scan(&payCount)) + require.Equal(t, 1, payCount, "the webhook-completed row must NOT be re-split into duplicate tip/balance rows") + + // The primary row keeps the charged (webhook) values — the sync flip + // must not overwrite amount/payment_type with the split-carved values. + var amount float64 + var paymentType string + var sqPayID string + require.NoError(t, tx.QueryRow(ctx, ` + SELECT amount, payment_type, COALESCE(square_payment_id, '') FROM payments WHERE booking_id = $1 + `, bookingID).Scan(&amount, &paymentType, &sqPayID)) + require.InDelta(t, 55.0, amount, 0.001, "the primary row must keep the charged amount (webhook's value), not the split-carved £50") + require.Equal(t, "full", paymentType, "the primary row must keep its original payment_type") + require.NotEmpty(t, sqPayID, "the webhook-completed row must carry the Square payment id") + + // Booking side-effects not double-run: the webhook completed the booking + // and awarded one stamp; the sync path must not re-run completion. + var bookingStatus string + require.NoError(t, tx.QueryRow(ctx, `SELECT status FROM bookings WHERE id = $1`, bookingID).Scan(&bookingStatus)) + require.Equal(t, "completed", bookingStatus, "the booking must stay completed — the sync path must not touch it") + + var stamps int + require.NoError(t, tx.QueryRow(ctx, `SELECT loyalty_stamps FROM users WHERE id = $1`, userID).Scan(&stamps)) + require.Equal(t, 1, stamps, "the loyalty stamp must be awarded exactly once (webhook's), not re-awarded by the sync path") + + var discountCount int + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM booking_discounts WHERE booking_id = $1`, bookingID).Scan(&discountCount)) + require.Equal(t, 0, discountCount, "no campaign discount rows may be created by a no-op completion") +} + +// TestCreateTipPayment_WebhookCompletedFirst_NoDuplicateTip locks the R10 fix +// on the tip path: when the webhook completes the pending tip row before the +// sync completion flip, the sync path must not re-flip it or double-record the +// tip — exactly one completed tip row exists and the response is 200. +func TestCreateTipPayment_WebhookCompletedFirst_NoDuplicateTip(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, bookingID, _ := setupTestDataPast(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + + // The tip flow requires an existing completed payment on the booking. + _, err := tx.Exec(ctx, ` + INSERT INTO payments (booking_id, payment_type, payment_method, status, amount, idempotency_key, created_by, created_at, updated_at) + VALUES ($1, 'full', 'online_square', 'completed', 50.00, $2, $3, NOW(), NOW()) + `, bookingID, "round10-tip-primary-"+bookingID, userID) + require.NoError(t, err) + + origClient := SquareClient + SquareClient = &webhookRaceClient{ + SquareClient: square.NewDevClient(), + bookingID: bookingID, + userID: userID, + } + defer func() { SquareClient = origClient }() + + req := CreateTipPaymentRequest{ + Amount: 500, + NewCardToken: strPtr("cnon:round10-tip-webhook-first"), + IdempotencyKey: "round10-tip-webhook-first-" + bookingID, + } + w := makePaymentRequest(CreateTipPayment, "POST", "/api/bookings/"+bookingID+"/tip", req, userToken, ctx) + require.Equal(t, http.StatusOK, w.Code, "a webhook-resolved tip must still report success, body: %s", w.Body.String()) + + // Exactly ONE completed tip row — the sync flip must not create a second. + var tipCount int + require.NoError(t, tx.QueryRow(ctx, ` + SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND payment_type = 'tip' AND status = 'completed' + `, bookingID).Scan(&tipCount)) + require.Equal(t, 1, tipCount, "the webhook-completed tip row must not be duplicated by the sync path") + + var tipAmount float64 + var sqPayID string + require.NoError(t, tx.QueryRow(ctx, ` + SELECT amount, COALESCE(square_payment_id, '') FROM payments + WHERE booking_id = $1 AND payment_type = 'tip' + `, bookingID).Scan(&tipAmount, &sqPayID)) + require.InDelta(t, 5.0, tipAmount, 0.001, "the tip row must keep the charged amount") + require.NotEmpty(t, sqPayID, "the webhook-completed tip row must carry the Square payment id") + + // Total ledger: the pre-existing full payment + exactly one tip. + var totalCount int + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM payments WHERE booking_id = $1`, bookingID).Scan(&totalCount)) + require.Equal(t, 2, totalCount, "the ledger must hold the full payment + exactly one tip row") +} + +// TestPostChargeRecheck_WebhookCompletedRow_NotClobberedWithFailed locks the +// R10 fix in postChargeRecheck (charge_helpers.go): a charge landing on a +// cancelled booking must not mark 'failed' a payment row the webhook already +// resolved to 'completed'. The guarded failed-mark UPDATE no-ops on the +// completed row, the 409 conflict is still returned, and the completed row +// stays completed so the cancellation refund path (which computes refunds +// from completed payments) can reverse the money. +func TestPostChargeRecheck_WebhookCompletedRow_NotClobberedWithFailed(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, bookingID, _ := setupTestDataPast(t, ctx, tx) + + // The booking was cancelled between the Square call and the recheck. + _, err := tx.Exec(ctx, `UPDATE bookings SET status = 'client_cancelled' WHERE id = $1`, bookingID) + require.NoError(t, err) + + // The webhook already completed the payment row. + var paymentID string + err = tx.QueryRow(ctx, ` + INSERT INTO payments (booking_id, payment_type, payment_method, status, amount, square_payment_id, idempotency_key, created_by, created_at, updated_at) + VALUES ($1, 'full', 'online_square', 'completed', 55.00, 'pay_round10_webhook_resolved', $2, $3, NOW(), NOW()) + RETURNING id + `, bookingID, "round10-clobber-key", userID).Scan(&paymentID) + require.NoError(t, err) + + recheckTx, bErr := db.Conn.Begin(ctx) + require.NoError(t, bErr) + defer func() { _ = recheckTx.Rollback(ctx) }() + + w := httptest.NewRecorder() + payable, pErr := postChargeRecheck(ctx, w, recheckTx, bookingID, paymentID, "COMPLETED", "pay_round10_webhook_resolved", "payment", "This booking is no longer accepting payments") + require.NoError(t, pErr) + require.False(t, payable, "a cancelled booking must not accept the completed payment") + require.Equal(t, http.StatusConflict, w.Code, "the recheck must surface the 409 conflict") + + // The completed row must survive the recheck — never clobbered to 'failed'. + var status string + var sqPayID string + require.NoError(t, tx.QueryRow(ctx, `SELECT status, COALESCE(square_payment_id, '') FROM payments WHERE id = $1`, paymentID).Scan(&status, &sqPayID)) + require.Equal(t, "completed", status, "a webhook-completed row must not be marked failed by the cancelled-booking recheck") + require.Equal(t, "pay_round10_webhook_resolved", sqPayID, "the webhook's square_payment_id must survive untouched") + + var bookingStatus string + require.NoError(t, tx.QueryRow(ctx, `SELECT status FROM bookings WHERE id = $1`, bookingID).Scan(&bookingStatus)) + require.Equal(t, "client_cancelled", bookingStatus, "the booking must stay cancelled") +} diff --git a/backend/handlers/payments/handlers_round9_fixes_test.go b/backend/handlers/payments/handlers_round9_fixes_test.go new file mode 100644 index 0000000..5f0918a --- /dev/null +++ b/backend/handlers/payments/handlers_round9_fixes_test.go @@ -0,0 +1,407 @@ +//go:build test && dev + +package payments + +// Round-9 money-fix tests: tip-record VAT exclusion (R11), hashed split +// idempotency keys, cross-booking idempotency-key collisions, refund-then-repay +// on the same payment type, the SCA token-like save-gate exemption, and the +// non-COMPLETED Square payment classification. These tests exercise the shared +// package mock and the handler integration paths, so they follow the existing +// sequential (non-parallel) conventions of the money-path tests they guard. + +import ( + "database/sql" + "net/http" + "testing" + + "crussell/internal/square" + "crussell/testutils" + "crussell/testutils/jwt" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestCreateBookingPayment_VATApplied_OverflowTipSkipped locks the R11 fix: an +// online full payment whose overflow carves a payment_type='tip' split record +// must NOT have VAT applied to the tip portion. vat.go's single-source policy +// excludes tip payments entirely (ApplyVATToBookingPayment), and every other +// tip path (CreateTipPayment, the terminal sweep rescue) applies VAT to the +// booking portion only — the post-charge split loop must match. A £55 charge on +// a £50 past-start booking splits into [£50 booking portion, £5 tip]; the tip +// record keeps vat_amount NULL while the booking portion gets VAT, and the two +// records still partition the charged amount exactly. +func TestCreateBookingPayment_VATApplied_OverflowTipSkipped(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + // Enable VAT for this test's transaction (the handler routes through the + // same per-test tx via the context savepoint, so the change is visible). + _, err := tx.Exec(ctx, `UPDATE business_settings SET is_vat_registered = TRUE, default_vat_rate = 20.00`) + require.NoError(t, err) + + userID, bookingID, _ := setupTestDataPast(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + + cardToken := "cnon:round9-vat-tip-overflow" + req := CreateBookingPaymentRequest{ + Amount: 5500, // £55 on a £50 booking — £5 overflows into a tip record + PaymentType: "full", + NewCardToken: &cardToken, + IdempotencyKey: "round9-vat-tip-overflow-" + bookingID, + ConfirmOverflowTip: true, // B12: a post-start overflow always requires explicit confirmation + } + + w := makePaymentRequest(CreateBookingPayment, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusOK, w.Code, "overflow payment must succeed, body: %s", w.Body.String()) + + rows, err := tx.Query(ctx, ` + SELECT payment_type, amount, is_vat_applicable, vat_amount, net_amount + FROM payments + WHERE booking_id = $1 AND status = 'completed' + ORDER BY payment_type + `, bookingID) + require.NoError(t, err) + defer rows.Close() + + var bookingVAT, tipVAT sql.NullFloat64 + var bookingNet, tipNet sql.NullFloat64 + var bookingVATFlag, tipVATFlag bool + var bookingAmount, tipAmount float64 + var gotBooking, gotTip bool + for rows.Next() { + var pt string + var amt float64 + var applicable bool + var vat, net sql.NullFloat64 + require.NoError(t, rows.Scan(&pt, &amt, &applicable, &vat, &net)) + switch pt { + case "tip": + gotTip = true + tipAmount = amt + tipVAT = vat + tipVATFlag = applicable + tipNet = net + case "full": + gotBooking = true + bookingAmount = amt + bookingVAT = vat + bookingVATFlag = applicable + bookingNet = net + } + } + require.NoError(t, rows.Err()) + require.True(t, gotBooking, "the booking portion record must exist") + require.True(t, gotTip, "the tip split record must exist") + + // The tip portion is gratuity, never VAT-applicable. + require.False(t, tipVATFlag, "tip record must never be VAT-applicable") + require.False(t, tipVAT.Valid, "tip record must have vat_amount NULL — the fix skips it in the apply_vat_to_payment loop") + require.False(t, tipNet.Valid, "tip record must have net_amount NULL") + + // The booking portion carries VAT at the 20% rate (net = 50/1.2 = 41.67, + // vat = 50 − 41.67 = 8.33). + require.True(t, bookingVATFlag, "the booking portion must be VAT-applicable") + require.True(t, bookingVAT.Valid, "booking portion vat_amount must be set") + require.InDelta(t, 8.33, bookingVAT.Float64, 0.01, "£50 at 20% → £8.33 VAT") + require.True(t, bookingNet.Valid, "booking portion net_amount must be set") + + // The split records still partition the charged amount exactly. + require.InDelta(t, 5500, int64((bookingAmount+tipAmount)*100), 0.5, "booking portion + tip must equal the £55 charge") + require.InDelta(t, 50.0, bookingAmount, 0.001, "booking portion = the £50 booking value") + require.InDelta(t, 5.0, tipAmount, 0.001, "tip portion = the £5 overflow") +} + +// TestSplitIdempotencyKey_DistinctBases_DistinctKeys locks the R12 fix for +// splitIdempotencyKey: two distinct terminal base keys that share a long common +// prefix (e.g. Square payment IDs differing only in their tail) must derive +// DISTINCT keys. The old raw `base[:maxBase]` prefix cut collapsed such bases +// onto the same key, 500-ing the second insert on the UNIQUE(idempotency_key) +// index. The keys must also stay within the payments.idempotency_key VARCHAR(64) +// column (and, via truncateIdempotencyKey, within Square's 45-char cap). +func TestSplitIdempotencyKey_DistinctBases_DistinctKeys(t *testing.T) { + // Both bases are long enough that a raw prefix cut would keep only the + // identical leading bytes — the OLD code returned the same key for both. + base1 := "pay-booking-000000000001-deposit-2500-pay_mock_12345678901234567890" + base2 := "pay-booking-000000000001-deposit-2500-pay_mock_12345678901234567899" + + k1 := splitIdempotencyKey(base1, "-split-tip") + k2 := splitIdempotencyKey(base2, "-split-tip") + + assert.NotEqual(t, k1, k2, + "two distinct base keys differing only in their tail must derive distinct split keys — the raw prefix cut collapses them") + // The split keys are DB-only (the Square charge already used the primary + // key) — the binding constraint is the payments.idempotency_key VARCHAR(64) + // column, and the hashed base keeps the whole key well under it. + assert.LessOrEqual(t, len(k1), 64, "split key must fit the payments.idempotency_key VARCHAR(64) column") + assert.LessOrEqual(t, len(k2), 64, "split key must fit the payments.idempotency_key VARCHAR(64) column") + + // The derivation stays deterministic — a retry of the same base must + // reproduce the same key (same-key dedup relies on it). + assert.Equal(t, k1, splitIdempotencyKey(base1, "-split-tip"), "split key derivation must be deterministic") + + // A short candidate is preserved verbatim (≤45 chars), so the existing + // terminal-split tests that assert the "-split-tip" suffix still pass. + short := splitIdempotencyKey("test-key", "-split-tip") + assert.Equal(t, "test-key-split-tip", short, "a short candidate is returned verbatim") + + // Distinct suffixes on the same base must stay distinct (the suffix is + // part of the hashed candidate). + tipped := splitIdempotencyKey(base1, "-split-tip") + splitted := splitIdempotencyKey(base1, "-split-2") + assert.NotEqual(t, tipped, splitted, "distinct split suffixes must derive distinct keys") +} + +// TestCreateBookingPayment_CrossBookingIdempotencyKey_CollisionRecovers locks +// the R12 fix: a client-supplied idempotency_key that already belongs to a +// payment on a DIFFERENT booking must NOT die on the global +// UNIQUE(idempotency_key) index (500 → frontend same-key retry → forever 500). +// Mirroring the A6 cross-user pattern (giftcards.go), the handler derives a +// fresh deterministic key for the colliding booking and the charge succeeds. +func TestCreateBookingPayment_CrossBookingIdempotencyKey_CollisionRecovers(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + // Booking A pays first and burns the key. + userA, bookingA, _ := setupTestData(t, ctx, tx) + tokenA := jwt.GenerateUserToken(userA) + const sharedKey = "client-key-reused-across-bookings" + reqA := CreateBookingPaymentRequest{ + Amount: 2500, + PaymentType: "deposit", + NewCardToken: strPtr("cnon:round9-cross-booking-a"), + IdempotencyKey: sharedKey, + } + wA := makePaymentRequest(CreateBookingPayment, "POST", "/api/bookings/"+bookingA+"/payment", reqA, tokenA, ctx) + require.Equal(t, http.StatusOK, wA.Code, "booking A must charge, body: %s", wA.Body.String()) + + // Booking B (different user) reuses the SAME key. Before the fix this + // 500'd on the UNIQUE constraint; after the fix it derives a fresh key and + // charges cleanly. + userB, bookingB, _ := setupTestData(t, ctx, tx) + tokenB := jwt.GenerateUserToken(userB) + reqB := CreateBookingPaymentRequest{ + Amount: 2500, + PaymentType: "deposit", + NewCardToken: strPtr("cnon:round9-cross-booking-b"), + IdempotencyKey: sharedKey, + } + wB := makePaymentRequest(CreateBookingPayment, "POST", "/api/bookings/"+bookingB+"/payment", reqB, tokenB, ctx) + require.Equal(t, http.StatusOK, wB.Code, "the cross-booking key reuse must recover with a fresh key, body: %s", wB.Body.String()) + + // Booking B got its own payment under a key distinct from the shared one. + var bID string + var bKey string + require.NoError(t, tx.QueryRow(ctx, ` + SELECT id, idempotency_key FROM payments WHERE booking_id = $1 AND status = 'completed' + `, bookingB).Scan(&bID, &bKey)) + require.NotEqual(t, sharedKey, bKey, "booking B must have been charged under a freshly derived key, not the collided one") + assert.Contains(t, bKey, bookingB, "the fresh key must be derived from booking B's identity") + + // Booking A's original payment is untouched and still keyed by the shared key. + var aCount int + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND idempotency_key = $2 AND status = 'completed'`, bookingA, sharedKey).Scan(&aCount)) + require.Equal(t, 1, aCount, "booking A's payment must remain intact under the shared key") +} + +// TestCreateBookingPayment_RefundedDeposit_RepaySucceeds locks the R11 fix for +// the existingCount check: a completed payment of the same non-partial type +// whose money is no longer live (it has a completed/pending refund) must NOT +// block a fresh equal-type charge — refund-then-repay is a legitimate flow +// (deriveBookingPaymentIdempotencyKey already rotates past refunded rows). +func TestCreateBookingPayment_RefundedDeposit_RepaySucceeds(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupPaymentStatusTest(t, ctx, tx, "confirmed") + userToken := jwt.GenerateUserToken(userID) + + cardToken := "cnon:round9-refund-repay" + req := CreateBookingPaymentRequest{ + Amount: 2500, + PaymentType: "deposit", + NewCardToken: &cardToken, + IdempotencyKey: "round9-refund-repay-first", + } + w1 := makePaymentRequest(CreateBookingPayment, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusOK, w1.Code, "the initial deposit must succeed, body: %s", w1.Body.String()) + + var paymentID string + require.NoError(t, tx.QueryRow(ctx, `SELECT id FROM payments WHERE booking_id = $1 AND idempotency_key = $2 AND status = 'completed'`, bookingID, req.IdempotencyKey).Scan(&paymentID)) + + // The deposit is refunded in full — a live (completed) refund row means its + // money is no longer collectable, so the type slot must re-open. + _, err := tx.Exec(ctx, ` + INSERT INTO refunds (payment_id, booking_id, amount, status, reason, created_at) + VALUES ($1, $2, 25.00, 'completed', 'admin refund', NOW()) + `, paymentID, bookingID) + require.NoError(t, err) + + // Same type, fresh key: before the fix the existingCount check counted the + // refunded completed deposit and 409'd; after the fix the refunded row is + // excluded and the repay succeeds. + req2 := CreateBookingPaymentRequest{ + Amount: 2500, + PaymentType: "deposit", + NewCardToken: &cardToken, + IdempotencyKey: "round9-refund-repay-second", + } + w2 := makePaymentRequest(CreateBookingPayment, "POST", "/api/bookings/"+bookingID+"/payment", req2, userToken, ctx) + require.Equal(t, http.StatusOK, w2.Code, "a refunded deposit must re-open the type slot, body: %s", w2.Body.String()) + + var secondKeyCount int + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND idempotency_key = $2 AND status = 'completed'`, bookingID, req2.IdempotencyKey).Scan(&secondKeyCount)) + require.Equal(t, 1, secondKeyCount, "the refund-then-repay deposit must be recorded as completed") + + // An UN-refunded completed deposit still blocks a second charge — the + // double-charge protection must survive the change. + req3 := CreateBookingPaymentRequest{ + Amount: 2500, + PaymentType: "deposit", + NewCardToken: &cardToken, + IdempotencyKey: "round9-refund-repay-third", + } + w3 := makePaymentRequest(CreateBookingPayment, "POST", "/api/bookings/"+bookingID+"/payment", req3, userToken, ctx) + require.Equal(t, http.StatusConflict, w3.Code, "an un-refunded completed deposit must still block a second equal-type charge") +} + +// TestCreateBookingPayment_SaveCard_SCATokenizeResult_Enforced2FA_PersistsCard +// locks the R13 fix: in an enforced-2FA deployment, a genuine SCA tokenize-result +// charge with save_card=true must skip the SAVE gate (the token was minted by a +// tokenization flow that ran the STORE-intent SCA — the same token-like +// exemption CreatePaymentMethod applies) and persist the card. Before the fix +// the tokenForwardedToSquare=false gate refused 402, making save_card +// unreachable in production. +func TestCreateBookingPayment_SaveCard_SCATokenizeResult_Enforced2FA_PersistsCard(t *testing.T) { + // Any non-mock SQUARE_ENVIRONMENT enforces 2FA (fail-closed). + t.Setenv("REQUIRE_2FA", "true") + t.Setenv("SQUARE_ENVIRONMENT", "staging") + + ctx, tx := testutils.SetupTestTx(t) + userID, bookingID, _ := setupTestDataPast(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + + // A genuine tokenize-result token (cnon:sca-...) for a NEW card — no saved + // card reference, so scaTokenizedSavedCard is false and the SAVE gate runs. + cardToken := "cnon:sca-round9-save-new-card" + req := CreateBookingPaymentRequest{ + Amount: 5000, + PaymentType: "full", + NewCardToken: &cardToken, + SaveCard: true, + IdempotencyKey: "round9-sca-save-enforced-" + bookingID, + } + w := makePaymentRequest(CreateBookingPayment, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusOK, w.Code, "a token-like SCA save must skip the 2FA gate in an enforced deployment, body: %s", w.Body.String()) + + var cardCount int + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM user_saved_cards WHERE user_id = $1`, userID).Scan(&cardCount)) + require.Equal(t, 1, cardCount, "the SCA tokenize-result save_card=true charge must persist the card in an enforced env") +} + +// TestCreateBookingPayment_NonCompletedSquareStatus_StaysPending locks the R11 +// fix: a CreatePayment that returns nil error but a non-COMPLETED status must +// NOT be recorded as completed. APPROVED/PENDING are non-terminal (mirroring +// the sweep's staleReconcileLeavePending — the row stays pending for a later +// sweep run to re-poll); the handler returns 5xx and no split/completed +// side-effects run. +func TestCreateBookingPayment_NonCompletedSquareStatus_StaysPending(t *testing.T) { + for _, status := range []string{"PENDING", "APPROVED"} { + t.Run(status, func(t *testing.T) { + origClient := SquareClient + mock := square.NewDevClient().(*square.MockClient) + mock.ForcePaymentStatus = status + SquareClient = mock + defer func() { SquareClient = origClient }() + + ctx, tx := testutils.SetupTestTx(t) + userID, bookingID, _ := setupTestDataPast(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + + req := CreateBookingPaymentRequest{ + Amount: 5000, + PaymentType: "full", + NewCardToken: strPtr("cnon:round9-nonterminal-" + status), + IdempotencyKey: "round9-nonterminal-" + status + "-" + bookingID, + } + w := makePaymentRequest(CreateBookingPayment, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusInternalServerError, w.Code, "%q must NOT be reported as success, body: %s", status, w.Body.String()) + + var rowStatus string + var rowCount int + require.NoError(t, tx.QueryRow(ctx, `SELECT status FROM payments WHERE booking_id = $1 AND idempotency_key = $2`, bookingID, req.IdempotencyKey).Scan(&rowStatus)) + require.Equal(t, "pending", rowStatus, "a non-terminal %q payment must stay pending (staleReconcileLeavePending semantics)", status) + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM payments WHERE booking_id = $1`, bookingID).Scan(&rowCount)) + require.Equal(t, 1, rowCount, "no split records may be created for a non-completed payment") + + // The booking is untouched — no promotion/completion side-effects. + var bookingStatus string + require.NoError(t, tx.QueryRow(ctx, `SELECT status FROM bookings WHERE id = $1`, bookingID).Scan(&bookingStatus)) + require.Equal(t, "in_progress", bookingStatus, "a non-terminal payment must not promote or complete the booking") + }) + } +} + +// TestCreateBookingPayment_TerminalNonSuccessSquareStatus_MarksFailed locks the +// R11 fix for terminal non-success statuses: a CANCELED/FAILED payment never +// landed at Square, so the row is marked failed (a same-key retry can never +// issue a second charge under the key) and the handler surfaces the failure. +func TestCreateBookingPayment_TerminalNonSuccessSquareStatus_MarksFailed(t *testing.T) { + for _, status := range []string{"CANCELED", "FAILED"} { + t.Run(status, func(t *testing.T) { + origClient := SquareClient + mock := square.NewDevClient().(*square.MockClient) + mock.ForcePaymentStatus = status + SquareClient = mock + defer func() { SquareClient = origClient }() + + ctx, tx := testutils.SetupTestTx(t) + userID, bookingID, _ := setupTestDataPast(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + + req := CreateBookingPaymentRequest{ + Amount: 5000, + PaymentType: "full", + NewCardToken: strPtr("cnon:round9-terminal-" + status), + IdempotencyKey: "round9-terminal-" + status + "-" + bookingID, + } + w := makePaymentRequest(CreateBookingPayment, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusPaymentRequired, w.Code, "%q must surface as a charge failure, body: %s", status, w.Body.String()) + + var rowStatus string + require.NoError(t, tx.QueryRow(ctx, `SELECT status FROM payments WHERE booking_id = $1 AND idempotency_key = $2`, bookingID, req.IdempotencyKey).Scan(&rowStatus)) + require.Equal(t, "failed", rowStatus, "a terminal non-success %q payment must be marked failed", status) + }) + } +} + +// TestCreateTipPayment_NonCompletedSquareStatus_StaysPending locks the R11 fix +// on the tip path too: a tip charge that returns non-COMPLETED must stay pending +// (never flipped to completed), so the sweep can reconcile it. +func TestCreateTipPayment_NonCompletedSquareStatus_StaysPending(t *testing.T) { + origClient := SquareClient + mock := square.NewDevClient().(*square.MockClient) + mock.ForcePaymentStatus = "PENDING" + SquareClient = mock + defer func() { SquareClient = origClient }() + + ctx, tx := testutils.SetupTestTx(t) + userID, bookingID, _ := setupTestDataPast(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + + // The tip flow requires an existing completed payment on the booking. + _, err := tx.Exec(ctx, ` + INSERT INTO payments (booking_id, payment_type, payment_method, status, amount, idempotency_key, created_by, created_at, updated_at) + VALUES ($1, 'full', 'online_square', 'completed', 50.00, $2, $3, NOW(), NOW()) + `, bookingID, "round9-tip-primary-"+bookingID, userID) + require.NoError(t, err) + + req := CreateTipPaymentRequest{ + Amount: 500, + NewCardToken: strPtr("cnon:round9-tip-nonterminal"), + IdempotencyKey: "round9-tip-nonterminal-" + bookingID, + } + w := makePaymentRequest(CreateTipPayment, "POST", "/api/bookings/"+bookingID+"/tip", req, userToken, ctx) + require.Equal(t, http.StatusInternalServerError, w.Code, "a non-COMPLETED tip must not be reported as success, body: %s", w.Body.String()) + + var rowStatus string + require.NoError(t, tx.QueryRow(ctx, `SELECT status FROM payments WHERE booking_id = $1 AND idempotency_key = $2`, bookingID, req.IdempotencyKey).Scan(&rowStatus)) + require.Equal(t, "pending", rowStatus, "a non-terminal tip payment must stay pending for the sweep to reconcile") +} diff --git a/backend/handlers/payments/refunds_round9_fixes_test.go b/backend/handlers/payments/refunds_round9_fixes_test.go new file mode 100644 index 0000000..087dcb1 --- /dev/null +++ b/backend/handlers/payments/refunds_round9_fixes_test.go @@ -0,0 +1,346 @@ +//go:build test && dev + +package payments + +import ( + "strings" + "testing" + "time" + + "crussell/clock" + "crussell/testutils" + "crussell/testutils/fixtures" +) + +// ============================================================================= +// ROUND 9 — minor money bugs in the cancellation-refund loop +// ============================================================================= + +// TestAggRefundKeySuffix_Widened_DistinctChargeIDs_NoCollision pins FIX 1: the +// charge-level aggregate idempotency key suffix is the FULL SHA-256 hex +// trimmed to the "-square-agg" budget (34 chars / 136 bits), never the old +// 12-hex-char (48-bit) truncation. Two distinct over-length charge IDs must +// produce distinct keys — a collision on Square's global idempotency-key dedup +// would silently swallow the second charge's refund (lost money, no refund +// row). The prefix structure (`-square-agg`) and determinism are kept +// intact so existing pending rows still resolve. +func TestAggRefundKeySuffix_Widened_DistinctChargeIDs_NoCollision(t *testing.T) { + t.Parallel() + + // Two long, DISTINCT charge IDs that exceed the verbatim budget (a + // verbatim key needs chargeID <= 45-len("-square-agg") = 34 chars). + longChargeA := "sqp_charge_alpha_payment_id_which_is_quite_long_001" + longChargeB := "sqp_charge_beta_payment_id_which_is_quite_long_002" + if len(longChargeA) <= maxIdempotencyKeyLength-len("-square-agg") { + t.Fatal("precondition: charge A must exceed the verbatim key budget") + } + if longChargeA == longChargeB { + t.Fatal("precondition: the two charge IDs must be distinct") + } + + keyA := chargeAggKey(longChargeA) + keyB := chargeAggKey(longChargeB) + + if keyA == keyB { + t.Errorf("FIX 1: distinct charge IDs %q and %q produced the SAME key %q — Square's idempotency dedup would swallow one refund", longChargeA, longChargeB, keyA) + } + if len(keyA) > maxIdempotencyKeyLength || len(keyB) > maxIdempotencyKeyLength { + t.Errorf("keys must stay within Square's %d-char idempotency-key limit, got %q (%d) and %q (%d)", maxIdempotencyKeyLength, keyA, len(keyA), keyB, len(keyB)) + } + // The prefix structure is intact. + if !strings.HasSuffix(keyA, "-square-agg") || !strings.HasSuffix(keyB, "-square-agg") { + t.Errorf("expected keys to keep the '-square-agg' structure, got %q and %q", keyA, keyB) + } + + // The suffix is widened to the full budget (34 hex chars = 136 bits). + if suffix := aggRefundKeySuffix([]string{longChargeA}); len(suffix) <= 12 { + t.Errorf("FIX 1: suffix is only %d hex chars — expected the widened full-budget form", len(suffix)) + } + + // Same charge -> same key (deterministic retries still dedup at Square). + if chargeAggKey(longChargeA) != keyA { + t.Error("the same charge must produce the same key") + } +} + +// TestProcessCancellationRefund_RedeemedGiftCard_CreditsUserBalance pins +// FIX 2: when a booking's gift-card payment is refunded after the card was +// REDEEMED (redeemed_by set — the terminal path rejects redeemed cards and +// RedeemGiftCard refuses a second redemption), the credit must land on the +// booking user's gift-card account balance, NOT on the card itself. Crediting +// amount_remaining onto a redeemed card would strand the refunded money +// permanently. +func TestProcessCancellationRefund_RedeemedGiftCard_CreditsUserBalance(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + if err != nil { + t.Fatalf("failed to create user: %v", err) + } + serviceID, err := fixtures.CreateTestService(tx) + if err != nil { + t.Fatalf("failed to create service: %v", err) + } + bookingID, err := fixtures.CreateTestBookingAtTime(tx, userID, serviceID, + time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC)) + if err != nil { + t.Fatalf("failed to create booking: %v", err) + } + _, err = tx.Exec(ctx, "UPDATE bookings SET status = 'confirmed' WHERE id = $1", bookingID) + if err != nil { + t.Fatalf("failed to confirm booking: %v", err) + } + + // A card redeemed to the user's account balance: amount_remaining zeroed, + // redeemed_by set — the exact state RedeemGiftCard leaves behind after the + // user redeemed the residual balance. + var giftCardID string + if err := tx.QueryRow(ctx, ` + INSERT INTO gift_cards (total_funds_added, amount_remaining, created_by, redeemed_at, redeemed_by, is_inventory, expiry_date, last_used_at) + VALUES (50, 0, $1, NOW(), $1, false, NULL, NOW()) + RETURNING id + `, userID).Scan(&giftCardID); err != nil { + t.Fatalf("failed to create redeemed gift card: %v", err) + } + + // The booking's £30 was paid FROM this card while it was still unredeemed. + var paymentID string + if err := tx.QueryRow(ctx, ` + INSERT INTO payments (booking_id, payment_type, payment_method, status, amount, gift_card_id, created_at, updated_at) + VALUES ($1, 'full', 'giftcard', 'completed', 30, $2, NOW(), NOW()) + RETURNING id + `, bookingID, giftCardID).Scan(&paymentID); err != nil { + t.Fatalf("failed to create giftcard payment: %v", err) + } + + farFuture := time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC) + result, err := ProcessCancellationRefund(ctx, bookingID, 100, 30, farFuture, clock.Now(), "client_cancelled", &userID) + if err != nil { + t.Fatalf("ProcessCancellationRefund failed: %v", err) + } + if result == nil || result.RefundableAmount != 30 { + t.Fatalf("expected refundable 30, got %+v", result) + } + + // The redeemed card must NOT be credited. + var amountRemaining float64 + if err := tx.QueryRow(ctx, + "SELECT amount_remaining FROM gift_cards WHERE id = $1", giftCardID).Scan(&amountRemaining); err != nil { + t.Fatalf("failed to query gift card balance: %v", err) + } + if amountRemaining != 0 { + t.Errorf("FIX 2: redeemed card must NOT be credited — expected amount_remaining 0, got %.2f", amountRemaining) + } + + // The credit lands on the booking user's gift-card account balance. + var balance float64 + if err := tx.QueryRow(ctx, + "SELECT COALESCE(balance, 0) FROM user_giftcard_balances WHERE user_id = $1", userID).Scan(&balance); err != nil { + t.Fatalf("failed to query balance: %v", err) + } + if balance != 30 { + t.Errorf("FIX 2: expected user gift-card balance credited 30, got %.2f", balance) + } + + // No refund-to-card transaction may be recorded (the money did not move + // onto the card). + var txCount int + if err := tx.QueryRow(ctx, + "SELECT COUNT(*) FROM gift_card_transactions WHERE gift_card_id = $1 AND transaction_type = 'refund'", giftCardID).Scan(&txCount); err != nil { + t.Fatalf("failed to query gift card transactions: %v", err) + } + if txCount != 0 { + t.Errorf("expected NO refund-to-card transaction for a redeemed card, got %d", txCount) + } + + // The refund record is 'completed' — the money DID move (to the balance). + var status string + if err := tx.QueryRow(ctx, + "SELECT status FROM refunds WHERE payment_id = $1", paymentID).Scan(&status); err != nil { + t.Fatalf("failed to query refund status: %v", err) + } + if status != "completed" { + t.Errorf("expected refund record status 'completed' (money credited to balance), got %q", status) + } +} + +// TestProcessCancellationRefund_RedeemedGiftCard_Guest_FailedRow pins FIX 2's +// guest/ownerless handling: with a redeemed card and a genuine guest booking +// there is no account balance to credit, so the refund record must be 'failed' +// (never 'completed' — money never moved) for admin reconciliation, mirroring +// the cash branch's guest handling. +func TestProcessCancellationRefund_RedeemedGiftCard_Guest_FailedRow(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + if err != nil { + t.Fatalf("failed to create user: %v", err) + } + _, err = tx.Exec(ctx, "UPDATE users SET account_role = 'guest' WHERE id = $1", userID) + if err != nil { + t.Fatalf("failed to set guest role: %v", err) + } + serviceID, err := fixtures.CreateTestService(tx) + if err != nil { + t.Fatalf("failed to create service: %v", err) + } + bookingID, err := fixtures.CreateTestBookingAtTime(tx, userID, serviceID, + time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC)) + if err != nil { + t.Fatalf("failed to create booking: %v", err) + } + _, err = tx.Exec(ctx, "UPDATE bookings SET status = 'confirmed' WHERE id = $1", bookingID) + if err != nil { + t.Fatalf("failed to confirm booking: %v", err) + } + + var giftCardID string + if err := tx.QueryRow(ctx, ` + INSERT INTO gift_cards (total_funds_added, amount_remaining, created_by, redeemed_at, redeemed_by, is_inventory, expiry_date, last_used_at) + VALUES (50, 0, $1, NOW(), $1, false, NULL, NOW()) + RETURNING id + `, userID).Scan(&giftCardID); err != nil { + t.Fatalf("failed to create redeemed gift card: %v", err) + } + _, err = tx.Exec(ctx, ` + INSERT INTO payments (booking_id, payment_type, payment_method, status, amount, gift_card_id, created_at, updated_at) + VALUES ($1, 'full', 'giftcard', 'completed', 30, $2, NOW(), NOW()) + `, bookingID, giftCardID) + if err != nil { + t.Fatalf("failed to create giftcard payment: %v", err) + } + + farFuture := time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC) + result, err := ProcessCancellationRefund(ctx, bookingID, 100, 30, farFuture, clock.Now(), "client_cancelled", &userID) + if err != nil { + t.Fatalf("ProcessCancellationRefund failed: %v", err) + } + if result == nil || result.RefundableAmount != 30 { + t.Fatalf("expected refundable 30, got %+v", result) + } + + var status string + if err := tx.QueryRow(ctx, + "SELECT status FROM refunds WHERE booking_id = $1", bookingID).Scan(&status); err != nil { + t.Fatalf("failed to query refund status: %v", err) + } + if status != "failed" { + t.Errorf("FIX 2: guest redeemed-card refund must be 'failed' (no balance credit possible), got %q", status) + } + + // The guest must NOT receive a balance credit. + var balance float64 + if err := tx.QueryRow(ctx, + "SELECT COALESCE(balance, 0) FROM user_giftcard_balances WHERE user_id = $1", userID).Scan(&balance); err != nil { + balance = 0 + } + if balance != 0 { + t.Errorf("expected guest balance 0 (guests do not receive balance credits), got %.2f", balance) + } +} + +// TestProcessCancellationRefund_GuestCash_FailedRow_AdminNotified pins FIX 3: +// a genuine-guest cash refund records a 'failed' row (money never moved — the +// guest has no balance to credit) and surfaces a 'refund_failed' admin +// notification via the post-commit pre-pass. Because the row is 'failed', not +// 'completed', the over-refund guard stays open: a subsequent refund attempt +// for the same amount is allowed and re-runs dedup cleanly (no second row, no +// double notification). +func TestProcessCancellationRefund_GuestCash_FailedRow_AdminNotified(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + if err != nil { + t.Fatalf("failed to create user: %v", err) + } + _, err = tx.Exec(ctx, "UPDATE users SET account_role = 'guest' WHERE id = $1", userID) + if err != nil { + t.Fatalf("failed to set guest role: %v", err) + } + serviceID, err := fixtures.CreateTestService(tx) + if err != nil { + t.Fatalf("failed to create service: %v", err) + } + bookingID, err := fixtures.CreateTestBookingAtTime(tx, userID, serviceID, + time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC)) + if err != nil { + t.Fatalf("failed to create booking: %v", err) + } + _, err = tx.Exec(ctx, "UPDATE bookings SET status = 'confirmed' WHERE id = $1", bookingID) + if err != nil { + t.Fatalf("failed to confirm booking: %v", err) + } + + paymentID, err := fixtures.CreateTestPayment(tx, bookingID, 30, "cash", "full", "completed") + if err != nil { + t.Fatalf("failed to create cash payment: %v", err) + } + + farFuture := time.Date(2099, 12, 31, 10, 0, 0, 0, time.UTC) + result, err := ProcessCancellationRefund(ctx, bookingID, 100, 30, farFuture, clock.Now(), "client_cancelled", &userID) + if err != nil { + t.Fatalf("ProcessCancellationRefund failed: %v", err) + } + if result == nil || result.RefundableAmount != 30 { + t.Fatalf("expected refundable 30, got %+v", result) + } + + // FIX 3: the row must be 'failed' — recording 'completed' would let the + // over-refund guard permanently block re-issuance if the admin never + // hands out the cash. + var status string + if err := tx.QueryRow(ctx, + "SELECT status FROM refunds WHERE payment_id = $1", paymentID).Scan(&status); err != nil { + t.Fatalf("failed to query refund status: %v", err) + } + if status != "failed" { + t.Errorf("FIX 3: guest cash refund must be 'failed' (no money moved), got %q", status) + } + + // The post-commit pre-pass surfaces a 'refund_failed' admin notification + // so the pending payout stays visible. + var notifCount int + if err := tx.QueryRow(ctx, + "SELECT COUNT(*) FROM admin_notifications WHERE booking_id = $1 AND reason = 'refund_failed'", bookingID).Scan(¬ifCount); err != nil { + t.Fatalf("failed to query admin_notifications: %v", err) + } + if notifCount < 1 { + t.Errorf("FIX 3: expected at least 1 'refund_failed' admin notification for the guest cash refund, got %d", notifCount) + } + + // The over-refund guard stays open: failed rows are excluded from + // GetAlreadyRefundedAmount, so the full £30 is still refundable. + svc := NewPaymentService() + alreadyRefunded, err := svc.GetAlreadyRefundedAmount(ctx, paymentID) + if err != nil { + t.Fatalf("failed to query already-refunded amount: %v", err) + } + if alreadyRefunded != 0 { + t.Errorf("FIX 3: expected 0 already-refunded (failed row must not block re-issuance), got %d pence", alreadyRefunded) + } + + // A subsequent refund attempt for the same amount is allowed: it recomputes + // the full residual, dedups onto the same failed row (no money moves + // twice), and does not double-notify. + if _, err := ProcessCancellationRefund(ctx, bookingID, 100, 30, farFuture, clock.Now(), "client_cancelled", &userID); err != nil { + t.Fatalf("subsequent ProcessCancellationRefund failed: %v", err) + } + var rowCount int + if err := tx.QueryRow(ctx, + "SELECT COUNT(*) FROM refunds WHERE payment_id = $1", paymentID).Scan(&rowCount); err != nil { + t.Fatalf("failed to count refund rows: %v", err) + } + if rowCount != 1 { + t.Errorf("expected exactly 1 refund row after the re-run (idempotency dedup), got %d", rowCount) + } + if err := tx.QueryRow(ctx, + "SELECT COUNT(*) FROM admin_notifications WHERE booking_id = $1 AND reason = 'refund_failed'", bookingID).Scan(¬ifCount); err != nil { + t.Fatalf("failed to re-query admin_notifications: %v", err) + } + if notifCount != 1 { + t.Errorf("expected the (reason, booking_id) dedup to keep exactly 1 notification after the re-run, got %d", notifCount) + } +} diff --git a/backend/handlers/payments/sweep_round10_adversarial_test.go b/backend/handlers/payments/sweep_round10_adversarial_test.go new file mode 100644 index 0000000..24f1382 --- /dev/null +++ b/backend/handlers/payments/sweep_round10_adversarial_test.go @@ -0,0 +1,427 @@ +//go:build test && dev + +package payments + +import ( + "context" + "database/sql" + "testing" + "time" + + "crussell/clock" + "crussell/db" + "crussell/internal/square" + "crussell/testutils" + "crussell/testutils/fixtures" +) + +// ============================================================================= +// Round 10 — adversarial sweep tests +// ============================================================================= +// +// 1. BUG 1 (MAJOR): the sweep's payments-table rescue aligned the primary row +// to the split amount but did NOT clear the pending row's VAT fields, and +// apply_vat_to_payment is guarded on vat_amount IS NULL — so the re-apply +// was a silent no-op and the rescued row kept VAT computed on the FULL +// pre-split charge (a larger base than the split primary). The all-tip +// rescue was worse: the completed primary row carried VAT on the whole +// tip, violating the tip-never-VAT invariant. +// 2. BUG 2 (MAJOR): a keyless till-sale retry locks the derived BASE key while +// its slot scan resolves a SUFFIXED final key that becomes the STORED key +// the sweep locks — the sweep and the retry do not serialize. If the retry +// resolves the stale sale (completes it, or re-keys it) between the sweep's +// fetch and its lock acquisition, the sweep must SKIP the row instead of +// failing/clawing back. + +// flipTillSaleClient simulates a same-key till-sale retry racing the sweep: it +// mutates the stale sale's row inside ReplayPaymentByKey (the window between +// the sweep's fetch and its lock acquisition) and then answers the probe with +// the definitive no-charge rejection that would normally drive the sweep's +// fail/clawback. Used with a cnon source so the rejection classifies as +// staleReconcileDefinitivelyFailed (a ccof source would take the A1 +// leave-pending path instead). +type flipTillSaleClient struct { + square.SquareClient + t *testing.T + saleID string + // flipTo is the status to set on the row before answering the probe + // ("" = leave the status unchanged). + flipTo string + // newKey is the idempotency_key to set on the row before answering the + // probe ("" = leave the stored key unchanged). + newKey string +} + +func (c *flipTillSaleClient) ReplayPaymentByKey(ctx context.Context, snapshotJSON []byte) (*square.PaymentResult, error) { + c.t.Helper() + if c.newKey != "" { + if _, err := db.Conn.Exec(ctx, `UPDATE till_sales SET idempotency_key = $1, updated_at = NOW() WHERE id = $2`, c.newKey, c.saleID); err != nil { + c.t.Fatalf("failed to re-key till sale %s during the sweep reconcile: %v", c.saleID, err) + } + } + if c.flipTo != "" { + if _, err := db.Conn.Exec(ctx, `UPDATE till_sales SET status = $1, updated_at = NOW() WHERE id = $2`, c.flipTo, c.saleID); err != nil { + c.t.Fatalf("failed to flip till sale %s during the sweep reconcile: %v", c.saleID, err) + } + } + return nil, square.ErrReplayKeyNotRetained +} + +// seedKeyedStaleTillSale ages a seedStaleTillSaleWithCard sale to 23h (inside +// Square's key-retention window so the keyed pass replays it) with a stored +// idempotency key and a chargeable cnon source. +func seedKeyedStaleTillSale(t *testing.T, ctx context.Context, q db.Querier, adminID, key string) (saleID, giftCardID string) { + t.Helper() + saleID, giftCardID = seedStaleTillSaleWithCard(t, ctx, q, adminID, 50.00, "", true) + if _, err := q.Exec(ctx, "UPDATE till_sales SET created_at = NOW() - INTERVAL '23 hours', idempotency_key = $1, square_source_id = 'cnon:test-card' WHERE id = $2", key, saleID); err != nil { + t.Fatalf("failed to age the keyed till sale: %v", err) + } + if _, err := q.Exec(ctx, "UPDATE gift_cards SET created_at = NOW() - INTERVAL '23 hours' WHERE id = $1", giftCardID); err != nil { + t.Fatalf("failed to age the gift card: %v", err) + } + return saleID, giftCardID +} + +// TestSweepStalePendingPayments_RescuedSplitPrimary_VATOnSplitAmount locks +// BUG 1's payments-table rescue VAT fix: a VAT-registered booking's deposit +// charge that COMPLETED at Square but whose response was lost is rescued by the +// sweep, the primary row is aligned to the split booking portion, and its VAT +// is recomputed on the SPLIT amount — not carried over from the full pre-split +// charge the pending insert taxed. Pre-fix the align left vat_amount from the +// full £80 charge (13.33) on a row aligned to the £50 split (correct 8.33). +func TestSweepStalePendingPayments_RescuedSplitPrimary_VATOnSplitAmount(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + if err != nil { + t.Fatalf("failed to create user: %v", err) + } + serviceID, err := fixtures.CreateTestService(tx) + if err != nil { + t.Fatalf("failed to create service: %v", err) + } + // Post-start £50 booking: the £80 rescued charge partitions into a £50 + // booking portion (the aligned primary) + a £30 overflow tip + // (buildSplitRecords' post-start carve). + bookingID, err := fixtures.CreateTestBookingAtTime(tx, userID, serviceID, clock.Now().Add(-2*time.Hour)) + if err != nil { + t.Fatalf("failed to create booking: %v", err) + } + if _, err := tx.Exec(ctx, `UPDATE business_settings SET is_vat_registered = TRUE, default_vat_rate = 20.00, voucher_type = 'SPV'`); err != nil { + t.Fatalf("failed to enable VAT in business_settings: %v", err) + } + + const key = "key-round10-vat-split" + var payID string + err = tx.QueryRow(ctx, ` + INSERT INTO payments (booking_id, payment_type, payment_method, status, amount, idempotency_key, square_source_id, created_by, created_at, updated_at) + VALUES ($1, 'deposit', 'online_square', 'pending', 80.00, $2, 'cnon:test-card', $3, NOW() - INTERVAL '23 hours', NOW()) + RETURNING id + `, bookingID, key, userID).Scan(&payID) + if err != nil { + t.Fatalf("failed to seed stale keyed pending payment: %v", err) + } + // Step-1 path parity: the pending insert applied VAT on the FULL pre-split + // £80 charge → vat_amount 13.33. The rescue must recompute on the split £50. + ApplyVATToBookingPayment(ctx, tx, payID) + + origClient := SquareClient + mock := square.NewDevClient().(*square.MockClient) + pay, err := mock.CreatePayment(context.Background(), square.CreatePaymentReq{ + Amount: 8000, + Currency: "GBP", + SourceID: "cnon:test-card", + IdempotencyKey: key, + }) + if err != nil { + t.Fatalf("failed to seed completed Square payment: %v", err) + } + SquareClient = mock + defer func() { SquareClient = origClient }() + + pgxTx := db.TxFromContext(ctx) + if pgxTx == nil { + t.Fatal("no transaction in context") + } + if err := pgxTx.Commit(ctx); err != nil { + t.Fatalf("failed to commit test tx: %v", err) + } + + t.Cleanup(func() { + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM payments WHERE booking_id = $1`, bookingID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM bookings WHERE id = $1`, bookingID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM services WHERE id = $1`, serviceID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM users WHERE id = $1`, userID) + _, _ = db.Conn.Exec(context.Background(), `UPDATE business_settings SET is_vat_registered = FALSE, voucher_type = 'SPV'`) + }) + + pool := context.Background() + if _, err := SweepStalePendingPayments(pool); err != nil { + t.Fatalf("sweep failed: %v", err) + } + + var status, ptype string + var amount float64 + var isVAT bool + var vatAmount, vatRate, netAmount sql.NullFloat64 + if err := db.Conn.QueryRow(pool, `SELECT status, payment_type, amount, is_vat_applicable, vat_amount, vat_rate, net_amount FROM payments WHERE id = $1`, payID).Scan(&status, &ptype, &amount, &isVAT, &vatAmount, &vatRate, &netAmount); err != nil { + t.Fatalf("failed to query rescued payment: %v", err) + } + if status != "completed" { + t.Errorf("expected the genuinely-charged stale payment rescued to 'completed', got %q", status) + } + if ptype != "deposit" { + t.Errorf("expected the primary row aligned to the split deposit type, got %q", ptype) + } + if amount != 50.00 { + t.Errorf("expected the primary row aligned to the £50 split booking portion, got %.2f", amount) + } + if !isVAT { + t.Errorf("expected is_vat_applicable=TRUE on the rescue-recomputed primary, got false") + } + if !vatAmount.Valid || vatAmount.Float64 != 8.33 { + t.Errorf("expected vat_amount 8.33 (VAT on the £50 split, not 13.33 on the full £80), got %v", vatAmount) + } + if !netAmount.Valid || netAmount.Float64 != 41.67 { + t.Errorf("expected net_amount 41.67 on the £50 split primary, got %v", netAmount) + } + if !vatRate.Valid || vatRate.Float64 != 20.00 { + t.Errorf("expected vat_rate 20.00 on the rescue-recomputed primary, got %v", vatRate) + } + if pay.SquarePayID == "" { + t.Errorf("expected the mock charge to carry a square payment id") + } + // The carved £30 tip record is inserted and must carry NO VAT. + var tipCount int + if err := db.Conn.QueryRow(pool, `SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND payment_type = 'tip' AND status = 'completed' AND vat_amount IS NULL`, bookingID).Scan(&tipCount); err != nil { + t.Fatalf("failed to count the carved tip record: %v", err) + } + if tipCount != 1 { + t.Errorf("expected exactly one VAT-free carved tip record, got %d", tipCount) + } +} + +// TestSweepStalePendingPayments_RescuedAllTip_VATNull locks BUG 1's all-tip +// edge: a fully-paid booking's £55 overflow rescue carves the ENTIRE charge as +// the tip record, the primary row is aligned to it, and a tip must NEVER carry +// VAT. Pre-fix the completed primary kept vat_amount 9.17 (VAT on the £55 tip) +// because the align did not clear it and the tip guard had nothing to skip. +func TestSweepStalePendingPayments_RescuedAllTip_VATNull(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + if err != nil { + t.Fatalf("failed to create user: %v", err) + } + serviceID, err := fixtures.CreateTestService(tx) + if err != nil { + t.Fatalf("failed to create service: %v", err) + } + // Pre-start £50 booking already fully paid by a completed £50 payment: the + // rescued £55 charge has zero booking obligation left, so buildSplitRecords + // carves the ENTIRE charge as the tip record (the tip-only branch). + bookingID, err := fixtures.CreateTestBooking(tx, userID, serviceID) + if err != nil { + t.Fatalf("failed to create booking: %v", err) + } + if _, err := fixtures.CreateTestPayment(tx, bookingID, 50.00, "online_square", "full", "completed"); err != nil { + t.Fatalf("failed to fully pay the booking: %v", err) + } + if _, err := tx.Exec(ctx, `UPDATE business_settings SET is_vat_registered = TRUE, default_vat_rate = 20.00, voucher_type = 'SPV'`); err != nil { + t.Fatalf("failed to enable VAT in business_settings: %v", err) + } + + const key = "key-round10-all-tip" + var payID string + err = tx.QueryRow(ctx, ` + INSERT INTO payments (booking_id, payment_type, payment_method, status, amount, idempotency_key, square_source_id, created_by, created_at, updated_at) + VALUES ($1, 'deposit', 'online_square', 'pending', 55.00, $2, 'cnon:test-card', $3, NOW() - INTERVAL '23 hours', NOW()) + RETURNING id + `, bookingID, key, userID).Scan(&payID) + if err != nil { + t.Fatalf("failed to seed stale keyed pending payment: %v", err) + } + // Step-1 path parity: the pending insert computed VAT on the full £55 → + // vat_amount 9.17. The pre-fix rescue left that on the tip-aligned primary. + ApplyVATToBookingPayment(ctx, tx, payID) + + origClient := SquareClient + mock := square.NewDevClient().(*square.MockClient) + if _, err := mock.CreatePayment(context.Background(), square.CreatePaymentReq{ + Amount: 5500, + Currency: "GBP", + SourceID: "cnon:test-card", + IdempotencyKey: key, + }); err != nil { + t.Fatalf("failed to seed completed Square payment: %v", err) + } + SquareClient = mock + defer func() { SquareClient = origClient }() + + pgxTx := db.TxFromContext(ctx) + if pgxTx == nil { + t.Fatal("no transaction in context") + } + if err := pgxTx.Commit(ctx); err != nil { + t.Fatalf("failed to commit test tx: %v", err) + } + + t.Cleanup(func() { + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM payments WHERE booking_id = $1`, bookingID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM bookings WHERE id = $1`, bookingID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM services WHERE id = $1`, serviceID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM users WHERE id = $1`, userID) + _, _ = db.Conn.Exec(context.Background(), `UPDATE business_settings SET is_vat_registered = FALSE, voucher_type = 'SPV'`) + }) + + pool := context.Background() + if _, err := SweepStalePendingPayments(pool); err != nil { + t.Fatalf("sweep failed: %v", err) + } + + var status, ptype string + var amount float64 + var isVAT bool + var vatAmount, vatRate, netAmount sql.NullFloat64 + if err := db.Conn.QueryRow(pool, `SELECT status, payment_type, amount, is_vat_applicable, vat_amount, vat_rate, net_amount FROM payments WHERE id = $1`, payID).Scan(&status, &ptype, &amount, &isVAT, &vatAmount, &vatRate, &netAmount); err != nil { + t.Fatalf("failed to query rescued payment: %v", err) + } + if status != "completed" { + t.Errorf("expected the all-tip charge rescued to 'completed', got %q", status) + } + if ptype != "tip" { + t.Errorf("expected the all-tip primary row aligned to the carved tip record, got %q", ptype) + } + if amount != 55.00 { + t.Errorf("expected the primary row aligned to the full £55 tip amount, got %.2f", amount) + } + if isVAT { + t.Errorf("expected is_vat_applicable=FALSE on the all-tip rescue (a tip never carries VAT), got true") + } + if vatAmount.Valid { + t.Errorf("expected vat_amount NULL on the all-tip rescue (a tip never carries VAT), got %v", vatAmount) + } + if vatRate.Valid { + t.Errorf("expected vat_rate NULL on the all-tip rescue, got %v", vatRate) + } + if netAmount.Valid { + t.Errorf("expected net_amount NULL on the all-tip rescue, got %v", netAmount) + } +} + +// TestSweepStalePendingPayments_TillStatusFlippedAfterFetch_SkipsClawback locks +// BUG 2's re-read: a stale till sale whose row status flips to 'completed' +// between the sweep's fetch/reconcile and its lock acquisition (a same-key +// retry landed its charge while the sweep waited) must be SKIPPED — the sweep +// must not fail the sale or claw back the funded gift card. +func TestSweepStalePendingPayments_TillStatusFlippedAfterFetch_SkipsClawback(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + adminID, err := fixtures.CreateTestAdminUser(tx) + if err != nil { + t.Fatalf("failed to create admin user: %v", err) + } + pool := context.Background() + + saleID, giftCardID := seedKeyedStaleTillSale(t, ctx, tx, adminID, "key-round10-status-flip") + + // The reconcile probe answers "no payment under the stored key" BUT flips + // the row to 'completed' first — the retry completed the sale while the + // sweep's probe was in flight, before the sweep's lock acquisition. + origClient := SquareClient + SquareClient = &flipTillSaleClient{SquareClient: square.NewDevClient(), t: t, saleID: saleID, flipTo: "completed"} + defer func() { SquareClient = origClient }() + + pgxTx := db.TxFromContext(ctx) + if pgxTx == nil { + t.Fatal("no transaction in context") + } + if err := pgxTx.Commit(ctx); err != nil { + t.Fatalf("failed to commit setup tx: %v", err) + } + + if _, err := SweepStalePendingPayments(pool); err != nil { + t.Fatalf("sweep failed: %v", err) + } + + // The sweep must leave the retry-completed row alone. + var status string + if err := db.Conn.QueryRow(pool, `SELECT status FROM till_sales WHERE id = $1`, saleID).Scan(&status); err != nil { + t.Fatalf("failed to query till sale: %v", err) + } + if status != "completed" { + t.Errorf("expected the retry-completed till sale left 'completed' (sweep skipped), got %q", status) + } + + // The funded gift card must NOT have been clawed back (deleted). + var cardCount int + if err := db.Conn.QueryRow(pool, `SELECT COUNT(*) FROM gift_cards WHERE id = $1`, giftCardID).Scan(&cardCount); err != nil { + t.Fatalf("failed to count gift cards: %v", err) + } + if cardCount != 1 { + t.Errorf("expected the funded gift card untouched when the row flipped to completed mid-sweep, got %d cards", cardCount) + } +} + +// TestSweepStalePendingPayments_TillKeyChangedWhilePending_SkipsClawback locks +// the re-read's key-change check — the genuinely money-saving half of BUG 2: a +// stale till sale whose STORED idempotency key is rewritten while the row stays +// 'pending' between the sweep's fetch and its lock acquisition. The sweep's +// lock (on the stale key) no longer matches the row the retry is charging +// under, so the fail/clawback must be skipped: pre-fix the clawback's +// status='pending' claim would still match and DELETE the funded gift card + +// fail the sale while the retry's charge is mid-flight under the new key. +func TestSweepStalePendingPayments_TillKeyChangedWhilePending_SkipsClawback(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + adminID, err := fixtures.CreateTestAdminUser(tx) + if err != nil { + t.Fatalf("failed to create admin user: %v", err) + } + pool := context.Background() + + saleID, giftCardID := seedKeyedStaleTillSale(t, ctx, tx, adminID, "key-round10-key-change") + + // The reconcile probe answers "no payment under the stored key" BUT rewrites + // the row's stored key first (still 'pending') — a same-key retry re-scanned + // its slot and is charging under the NEW key while the sweep holds a lock on + // the stale one. + const newKey = "key-round10-key-change-new" + origClient := SquareClient + SquareClient = &flipTillSaleClient{SquareClient: square.NewDevClient(), t: t, saleID: saleID, newKey: newKey} + defer func() { SquareClient = origClient }() + + pgxTx := db.TxFromContext(ctx) + if pgxTx == nil { + t.Fatal("no transaction in context") + } + if err := pgxTx.Commit(ctx); err != nil { + t.Fatalf("failed to commit setup tx: %v", err) + } + + if _, err := SweepStalePendingPayments(pool); err != nil { + t.Fatalf("sweep failed: %v", err) + } + + // The sweep must leave the still-pending row alone (it is reconciled again + // next sweep under its current key) — never claimed to 'failed'. + var status, storedKey string + if err := db.Conn.QueryRow(pool, `SELECT status, idempotency_key FROM till_sales WHERE id = $1`, saleID).Scan(&status, &storedKey); err != nil { + t.Fatalf("failed to query till sale: %v", err) + } + if status != "pending" { + t.Errorf("expected the re-keyed till sale left 'pending' (sweep skipped the fail/clawback), got %q", status) + } + if storedKey != newKey { + t.Errorf("expected the till sale's stored key untouched at %q, got %q", newKey, storedKey) + } + + // The funded gift card must NOT have been clawed back (deleted). + var cardCount int + if err := db.Conn.QueryRow(pool, `SELECT COUNT(*) FROM gift_cards WHERE id = $1`, giftCardID).Scan(&cardCount); err != nil { + t.Fatalf("failed to count gift cards: %v", err) + } + if cardCount != 1 { + t.Errorf("expected the funded gift card untouched when the stored key changed mid-sweep, got %d cards", cardCount) + } +} diff --git a/backend/handlers/payments/till_round10_adversarial_test.go b/backend/handlers/payments/till_round10_adversarial_test.go new file mode 100644 index 0000000..05240eb --- /dev/null +++ b/backend/handlers/payments/till_round10_adversarial_test.go @@ -0,0 +1,214 @@ +//go:build test && dev + +package payments + +// ============================================================================= +// ROUND 10 ADVERSARIAL — the till-sale FINAL-key lock vs the sweep's clawback +// ============================================================================= +// +// The sweep (sweep.go acquireTillSaleSweepLock) fails a stale pending +// till_sale + claws back its funded gift card under the advisory lock +// "crussell:till:". CreateTillSale's base lock is keyed +// on the REQUEST key, but a keyless sale's slot scan can advance the FINAL +// stored key past the base (a COMPLETED/FAILED sale occupying the base slot +// forces "base-1", "base-2", ...). Without the FIX 2 re-acquisition the retry +// would hold "crussell:till:" while the sweep holds +// "crussell:till:" — two locks that do not serialize — so the sweep can +// fail the sale + claw back the funded card while the retry's Square charge is +// mid-flight: customer charged AND funding clawed back. +// +// This test pins the retry side of the invariant: the retry must hold +// "crussell:till:" from before the Square charge until it +// completes. It seeds a COMPLETED sale on the deterministic base key so the +// keyless sale's slot scan advances to the suffixed key, then proves a second +// connection attempting the SAME advisory lock is blocked (try-lock returns +// false) the whole time the charge is in flight and only acquires after the +// retry finishes. + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + "time" + + "crussell/db" + "crussell/internal/square" + "crussell/testutils" + "crussell/testutils/fixtures" + "crussell/testutils/jwt" +) + +// TestCreateTillSale_KeylessSuffixedSlot_HoldsFinalKeyLockAcrossCharge locks +// the FIX 2 retry-side invariant for a keyless sale whose slot scan resolves a +// SUFFIXED final idempotency key: +// +// 1. seed a COMPLETED till_sale occupying the deterministic base key (so the +// keyless request's slot scan advances to "-1"); +// 2. run the keyless sale with a SLOW Square client (the charge stays +// in-flight ~500ms after the pending row commits); +// 3. once the pending row with the suffixed key is visible (the tx committed +// immediately before the charge), a second connection's try-lock on +// "crussell:till:" must return FALSE (the retry holds it) and +// stay FALSE while the charge is mid-flight; +// 4. after the handler returns, the same try-lock must return TRUE (released) +// and the sale must be completed on the suffixed key. +// +// The lock being held exactly across the charge round-trip is what serializes +// against the sweep's "crussell:till:" fail/claw-back. +func TestCreateTillSale_KeylessSuffixedSlot_HoldsFinalKeyLockAcrossCharge(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + adminID, err := fixtures.CreateTestAdminUser(tx) + if err != nil { + t.Fatalf("failed to create admin user: %v", err) + } + adminToken := jwt.GenerateTestToken(adminID, "admin") + pool := context.Background() + + // The keyless request the test replays. CardToken is deliberately excluded + // from deriveTillIdempotencyKey (it changes between retries), so the base + // key below is exactly what the handler derives for this request. + keylessReq := TillSaleRequest{ + ItemType: "gift_card", + Action: "create", + Amount: 50.00, + PaymentMethod: "online_square", + CardToken: "cnon:final-lock-adversarial", + } + baseKey := deriveTillIdempotencyKey(keylessReq, adminID) + suffixedKey := nextIdempotencyCandidate(baseKey, 1) + + // Seed a COMPLETED sale on the base key — it occupies the base slot so the + // keyless sale's slot scan must advance to the suffixed key. + if _, err := tx.Exec(ctx, ` + INSERT INTO till_sales (item_type, description, quantity, unit_price, total_amount, + payment_method, status, idempotency_key, created_by, created_at, updated_at) + VALUES ('gift_card', 'Gift Card create', 1, 50.00, 50.00, 'online_square', 'completed', + $1, $2, NOW(), NOW()) + `, baseKey, adminID); err != nil { + t.Fatalf("failed to seed the completed base-slot sale: %v", err) + } + + // Cleanup the committed rows (the completed seed + the retry's sale and its + // handler-created gift card). + t.Cleanup(func() { + _, _ = db.Conn.Exec(pool, `DELETE FROM till_sales WHERE idempotency_key IN ($1, $2)`, baseKey, suffixedKey) + _, _ = db.Conn.Exec(pool, `DELETE FROM gift_card_transactions WHERE gift_card_id IN (SELECT id FROM gift_cards WHERE created_by = $1)`, adminID) + _, _ = db.Conn.Exec(pool, `DELETE FROM gift_cards WHERE created_by = $1`, adminID) + _, _ = db.Conn.Exec(pool, `DELETE FROM users WHERE id = $1`, adminID) + }) + + // Commit the setup so the handler and the lock-checking connection operate + // at pool level on independent sessions — advisory locks only serialize + // across separate sessions, and a per-test tx would mask the contention. + innerTx := db.TxFromContext(ctx) + if innerTx == nil { + t.Fatal("no transaction in context") + } + if err := innerTx.Commit(ctx); err != nil { + t.Fatalf("failed to commit setup tx: %v", err) + } + + // Slow the Square charge so the handler provably holds the final-key lock + // across an in-flight round-trip long enough to observe it from another + // connection. + origClient := SquareClient + slow := &slowCreatePaymentClient{SquareClient: square.NewDevClient(), delay: 500 * time.Millisecond} + SquareClient = slow + defer func() { SquareClient = origClient }() + + // Run the keyless sale in a goroutine — the handler blocks ~500ms inside + // the Square charge and must not block this test's lock observations. + recCh := make(chan *httptest.ResponseRecorder, 1) + go func() { + recCh <- makePaymentRequest(CreateTillSale, "POST", "/api/admin/till/sale", keylessReq, adminToken, pool) + }() + + // Wait for the pending till_sales row on the SUFFIXED key to be committed — + // the tx commit precedes the Square charge, so once it is visible the + // handler holds the final-key lock and is about to call Square. + deadline := time.Now().Add(10 * time.Second) + for { + var status string + err := db.Conn.QueryRow(pool, `SELECT status FROM till_sales WHERE idempotency_key = $1`, suffixedKey).Scan(&status) + if err == nil { + break + } + if time.Now().After(deadline) { + t.Fatalf("timed out waiting for the pending till_sale on suffixed key %s (err: %v)", suffixedKey, err) + } + time.Sleep(10 * time.Millisecond) + } + + lockConn, err := db.Conn.Acquire(pool) + if err != nil { + t.Fatalf("failed to acquire lock-check connection: %v", err) + } + defer lockConn.Release() + + tryLock := func() bool { + t.Helper() + var acquired bool + if err := lockConn.QueryRow(pool, `SELECT pg_try_advisory_lock(hashtext($1))`, "crussell:till:"+suffixedKey).Scan(&acquired); err != nil { + t.Fatalf("failed to try the final-key advisory lock: %v", err) + } + return acquired + } + + // The charge is mid-flight: the retry must hold the final-key lock, so a + // second session cannot acquire it. If the lock is free the retry is NOT + // serialized against the sweep's stored-key clawback — the exact FIX 2 bug. + if tryLock() { + t.Fatalf("retry does NOT hold the final-key lock %q while the Square charge is in flight — the sweep could fail the sale and claw back the funded card mid-charge", "crussell:till:"+suffixedKey) + } + // Still mid-flight (the slow client keeps the charge in the air for ~500ms + // after the row appeared): the lock must stay held for the whole window. + time.Sleep(200 * time.Millisecond) + if tryLock() { + t.Fatalf("final-key lock %q was released mid-charge — the sweep could claw back the funded card before the charge completed", "crussell:till:"+suffixedKey) + } + + // The handler must complete the sale on the suffixed key. + var w *httptest.ResponseRecorder + select { + case w = <-recCh: + case <-time.After(30 * time.Second): + t.Fatal("timed out waiting for the till-sale handler") + } + if w.Code != http.StatusCreated { + t.Fatalf("expected 201, got %d. body: %s", w.Code, w.Body.String()) + } + + // After the handler released it, the final-key lock must be free again — + // proving it was held for exactly the charge round-trip. + if !tryLock() { + t.Fatalf("final-key lock %q not released after the retry completed", "crussell:till:"+suffixedKey) + } + defer func() { + _, _ = lockConn.Exec(context.Background(), `SELECT pg_advisory_unlock(hashtext($1))`, "crussell:till:"+suffixedKey) + }() + + // The sale is stored under the SUFFIXED key and completed — the key the + // sweep will lock on, and the one this retry just held. + var storedKey, status string + if err := db.Conn.QueryRow(pool, `SELECT idempotency_key, status FROM till_sales WHERE idempotency_key = $1`, suffixedKey).Scan(&storedKey, &status); err != nil { + t.Fatalf("failed to query the suffixed-key sale: %v", err) + } + if storedKey != suffixedKey { + t.Fatalf("stored idempotency_key %q != suffixed key %q", storedKey, suffixedKey) + } + if status != "completed" { + t.Fatalf("expected the suffixed-key sale to be completed, got %q", status) + } + + // The base slot stays occupied by the seeded completed sale (the keyless + // retry did not collapse onto it). + var baseCount int + if err := db.Conn.QueryRow(pool, `SELECT COUNT(*) FROM till_sales WHERE idempotency_key = $1 AND status = 'completed'`, baseKey).Scan(&baseCount); err != nil { + t.Fatalf("failed to count base-slot sales: %v", err) + } + if baseCount != 1 { + t.Fatalf("expected the base-slot sale to remain the only completed sale on the base key, got %d", baseCount) + } +} diff --git a/backend/handlers/user/account_round9_fixes_test.go b/backend/handlers/user/account_round9_fixes_test.go new file mode 100644 index 0000000..f0ebd8f --- /dev/null +++ b/backend/handlers/user/account_round9_fixes_test.go @@ -0,0 +1,398 @@ +//go:build test + +package user + +import ( + "bytes" + "context" + "database/sql" + "encoding/json" + "fmt" + "io" + "net/http" + "net/http/httptest" + "sync" + "testing" + "time" + + "crussell/clock" + "crussell/db" + "crussell/internal/s3" + "crussell/mw" + "crussell/testutils" + "crussell/testutils/fixtures" + + "github.com/stretchr/testify/require" + "golang.org/x/crypto/bcrypt" +) + +// ============================================================================ +// FIX 2 — DeleteAccountHandler current-password lockout +// ============================================================================ + +// TestDeleteAccount_CurrentPasswordLockout verifies the FIX 2 budget: after 5 +// consecutive wrong current passwords the account is locked (even the CORRECT +// password is rejected with 429), and a cleared lockout lets the correct +// password through. Sequential (no t.Parallel): the handler reads the +// process-global s3.Client / payments.SquareClient. +func TestDeleteAccount_CurrentPasswordLockout(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + + // Five wrong current passwords: each rejected with 401 and counted toward + // the shared failed_attempts budget. + for i := 0; i < 5; i++ { + req := deleteAccountRequest(t, ctx, userID, "not-the-password", "") + rr := httptest.NewRecorder() + DeleteAccountHandler(rr, req) + require.Equal(t, http.StatusUnauthorized, rr.Code, "wrong current password must be rejected (attempt %d)", i+1) + } + + // The budget is now locked: even the correct password is rejected. FIX 4: + // a locked account returns the SAME uniform 401 as a wrong password (never + // distinguishable), with a distinct body the UI can surface. + req := deleteAccountRequest(t, ctx, userID, "testpassword123", "") + rr := httptest.NewRecorder() + DeleteAccountHandler(rr, req) + require.Equal(t, http.StatusUnauthorized, rr.Code, "delete-account must be rejected with a lockout after 5 wrong current passwords") + require.Contains(t, rr.Body.String(), "too many failed attempts", "the locked body must stay distinct for the UI") + + // The account survives the lockout. + var firstName string + require.NoError(t, tx.QueryRow(ctx, `SELECT n_first_name FROM users WHERE id = $1`, userID).Scan(&firstName)) + require.Equal(t, "Test", firstName) + + // Clearing the lockout (the documented operator / password-reset recovery) + // lets the correct password through. + _, err = tx.Exec(ctx, `UPDATE users SET failed_attempts = 0, locked_until = NULL WHERE id = $1`, userID) + require.NoError(t, err) + req = deleteAccountRequest(t, ctx, userID, "testpassword123", "") + rr = httptest.NewRecorder() + DeleteAccountHandler(rr, req) + require.Equal(t, http.StatusNoContent, rr.Code, "correct password must succeed after the lockout is reset") +} + +// ============================================================================ +// FIX 3 — ChangePasswordHandler current-password lockout +// ============================================================================ + +// changePasswordRequest builds the PUT /api/user/change-password request with +// the given current/new password and an authenticated context. +func changePasswordRequest(t *testing.T, ctx context.Context, userID, currentPassword, newPassword string) *http.Request { + t.Helper() + body, err := json.Marshal(ChangePasswordRequest{ + CurrentPassword: currentPassword, + NewPassword: newPassword, + }) + require.NoError(t, err) + req := httptest.NewRequest(http.MethodPut, "/api/user/change-password", bytes.NewReader(body)) + req = req.WithContext(context.WithValue(ctx, mw.UserIDKey, userID)) + req.Header.Set("Content-Type", "application/json") + return req +} + +// TestPasswordChange_CurrentPasswordLockout verifies the FIX 3 budget shares +// the same failed-attempt/lockout columns as delete-account: 5 wrong current +// passwords lock the change-password flow (correct password → 429), and a +// cleared lockout lets it through. +func TestPasswordChange_CurrentPasswordLockout(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + + for i := 0; i < 5; i++ { + req := changePasswordRequest(t, ctx, userID, "not-the-password", "newpassword456") + rr := httptest.NewRecorder() + ChangePasswordHandler(rr, req) + require.Equal(t, http.StatusUnauthorized, rr.Code, "wrong current password must be rejected (attempt %d)", i+1) + } + + // Locked out: the correct current password is rejected too. FIX 4: uniform + // 401 (never distinguishable from a wrong password), distinct body text. + req := changePasswordRequest(t, ctx, userID, "testpassword123", "newpassword456") + rr := httptest.NewRecorder() + ChangePasswordHandler(rr, req) + require.Equal(t, http.StatusUnauthorized, rr.Code, "change-password must be rejected with a lockout after 5 wrong current passwords") + require.Contains(t, rr.Body.String(), "too many failed attempts", "the locked body must stay distinct for the UI") + + // The correct password works again once the lockout is cleared. + _, err = tx.Exec(ctx, `UPDATE users SET failed_attempts = 0, locked_until = NULL WHERE id = $1`, userID) + require.NoError(t, err) + req = changePasswordRequest(t, ctx, userID, "testpassword123", "newpassword456") + rr = httptest.NewRecorder() + ChangePasswordHandler(rr, req) + require.Equal(t, http.StatusOK, rr.Code, "correct current password must succeed after the lockout is reset") +} + +// ============================================================================ +// FIX 1 — Durable S3/R2 profile-picture deletion outbox +// ============================================================================ + +// recordingUploader records S3/R2 Delete calls so the account-deletion outbox +// tests can assert what was (and wasn't) deleted. Every Uploader method is +// implemented explicitly (no embedded nil) so parallel profile-picture tests +// that read the swapped global s3.Client never panic. +type recordingUploader struct { + mu sync.Mutex + deleted []string // "bucket/key" +} + +func (r *recordingUploader) Upload(context.Context, string, string, io.Reader, string) error { + return nil +} + +func (r *recordingUploader) Download(context.Context, string, string, io.Writer) error { + return nil +} + +func (r *recordingUploader) GetURL(context.Context, string, string) (string, error) { + return "https://cdn.example.com/x/y", nil +} + +func (r *recordingUploader) HealthCheck(context.Context) error { + return nil +} + +func (r *recordingUploader) Delete(_ context.Context, bucket, key string) error { + r.mu.Lock() + defer r.mu.Unlock() + r.deleted = append(r.deleted, bucket+"/"+key) + return nil +} + +func (r *recordingUploader) Deleted() []string { + r.mu.Lock() + defer r.mu.Unlock() + return append([]string(nil), r.deleted...) +} + +// waitFor polls cond until it holds or the timeout elapses, failing the test. +func waitFor(t *testing.T, timeout time.Duration, cond func() bool) { + t.Helper() + deadline := clock.Now().Add(timeout) + for clock.Now().Before(deadline) { + if cond() { + return + } + time.Sleep(5 * time.Millisecond) + } + t.Fatal("timed out waiting for condition") +} + +// TestDeleteAccount_S3OutboxPersisted_AndDrained verifies the FIX 1 durable +// pattern: with a configured bucket the erasure transaction persists a +// pending_s3_deletions outbox row, and the async goroutine drains it after +// deleting the object. Sequential: swaps the process-global s3.Client and env. +func TestDeleteAccount_S3OutboxPersisted_AndDrained(t *testing.T) { + savedClient := s3.Client + rec := &recordingUploader{} + s3.Client = rec + t.Cleanup(func() { s3.Client = savedClient }) + t.Setenv("S3_PROFILE_PICS_BUCKET", "test-profile-pics-bucket") + + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + + _, err = tx.Exec(ctx, `UPDATE users SET profile_pic_url = 'https://cdn.example.com/pics/old.jpg' WHERE id = $1`, userID) + require.NoError(t, err) + + req := deleteAccountRequest(t, ctx, userID, "testpassword123", "") + rr := httptest.NewRecorder() + DeleteAccountHandler(rr, req) + require.Equal(t, http.StatusNoContent, rr.Code, rr.Body.String()) + + // The durable outbox row was written inside the erasure transaction. + var outboxID, bucket, objectKey string + err = tx.QueryRow(ctx, `SELECT id, bucket, object_key FROM pending_s3_deletions WHERE user_id = $1`, userID). + Scan(&outboxID, &bucket, &objectKey) + require.NoError(t, err, "pending_s3_deletions outbox row must be persisted in the erasure transaction") + require.NotEmpty(t, outboxID) + require.Equal(t, "test-profile-pics-bucket", bucket) + require.Equal(t, "profiles/"+userID+".jpg", objectKey) + + // The async goroutine (primary drain) deletes the object from the store. + waitFor(t, 2*time.Second, func() bool { return len(rec.Deleted()) > 0 }) + require.Equal(t, []string{"test-profile-pics-bucket/profiles/" + userID + ".jpg"}, rec.Deleted()) +} + +// TestDeleteAccount_S3Outbox_FailClosedOnEmptyBucket verifies the FIX 4 +// fail-closed behavior: with S3_PROFILE_PICS_BUCKET unset the handler must +// NOT guess the dev bucket — no outbox row is written and no deletion is +// attempted, even though a profile picture exists. +func TestDeleteAccount_S3Outbox_FailClosedOnEmptyBucket(t *testing.T) { + savedClient := s3.Client + rec := &recordingUploader{} + s3.Client = rec + t.Cleanup(func() { s3.Client = savedClient }) + t.Setenv("S3_PROFILE_PICS_BUCKET", "") + + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + + _, err = tx.Exec(ctx, `UPDATE users SET profile_pic_url = 'https://cdn.example.com/pics/old.jpg' WHERE id = $1`, userID) + require.NoError(t, err) + + req := deleteAccountRequest(t, ctx, userID, "testpassword123", "") + rr := httptest.NewRecorder() + DeleteAccountHandler(rr, req) + require.Equal(t, http.StatusNoContent, rr.Code, rr.Body.String()) + + // Fail-closed: no outbox row, no deletion attempt against a guessed bucket. + var count int + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM pending_s3_deletions WHERE user_id = $1`, userID).Scan(&count)) + require.Zero(t, count, "no outbox row may be written when the bucket is unset") + require.Empty(t, rec.Deleted(), "no S3 deletion may be attempted against a guessed bucket") +} + +// ============================================================================ +// FIX 3 — atomic current-password failure record (no lost updates) +// ============================================================================ + +// TestDeleteAccount_ConcurrentWrongPassword_NoLostUpdates verifies the FIX 3 +// atomic failure record: 20 concurrent wrong-current-password requests each +// increment the shared failed-attempt budget exactly once (no lost updates — +// the final count is 20, not a smaller racy subset) and the escalating lockout +// is armed. Runs against the real pool because a per-test pgx.Tx cannot serve +// concurrent queries; the requests carry no test-tx context. +func TestDeleteAccount_ConcurrentWrongPassword_NoLostUpdates(t *testing.T) { + savedClient := s3.Client + s3.Client = nil + t.Cleanup(func() { s3.Client = savedClient }) + + hash, err := bcrypt.GenerateFromPassword([]byte("testpassword123"), bcrypt.DefaultCost) + require.NoError(t, err) + + ctx := context.Background() + var userID string + require.NoError(t, db.Conn.QueryRow(ctx, ` + INSERT INTO users (n_first_name, n_last_name, email, phone, date_of_birth, password_hash, account_role, account_type) + VALUES ('Concurrent', 'User', $1, '+447123456789', '1990-01-01', $2, 'verified_email', 'email') + RETURNING id + `, fmt.Sprintf("concurrent.%d@test.com", time.Now().UnixNano()), string(hash)).Scan(&userID)) + t.Cleanup(func() { + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM users WHERE id = $1`, userID) + }) + + const n = 20 + var wg sync.WaitGroup + for i := 0; i < n; i++ { + wg.Add(1) + go func() { + defer wg.Done() + req := httptest.NewRequest(http.MethodDelete, "/api/user/account", bytes.NewBufferString(`{"current_password":"not-the-password"}`)) + req = req.WithContext(context.WithValue(context.Background(), mw.UserIDKey, userID)) + rr := httptest.NewRecorder() + DeleteAccountHandler(rr, req) + }() + } + wg.Wait() + + var failedAttempts int + var lockedUntil *time.Time + require.NoError(t, db.Conn.QueryRow(ctx, `SELECT failed_attempts, locked_until FROM users WHERE id = $1`, userID).Scan(&failedAttempts, &lockedUntil)) + require.Equal(t, n, failedAttempts, "every concurrent wrong password must be counted — no lost updates") + require.NotNil(t, lockedUntil, "the lockout must be armed after the burst") + require.True(t, lockedUntil.After(clock.Now()), "locked_until must be in the future") +} + +// ============================================================================ +// FIX 5 — passwordless (NULL password_hash) accounts +// ============================================================================ + +// TestDeleteAccount_Passwordless_Requires2FAUnconditionally verifies FIX 5a: a +// NULL-password-hash (social-only) account has no current password to +// re-verify, so deleting it requires the 2FA code gate UNCONDITIONALLY — even +// when 2FA is not otherwise enforced — so a session holder cannot erase a +// passwordless account with zero credential proof. +func TestDeleteAccount_Passwordless_Requires2FAUnconditionally(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + _, err = tx.Exec(ctx, `UPDATE users SET password_hash = NULL WHERE id = $1`, userID) + require.NoError(t, err) + + // No code → rejected with the exact message the frontend uses to reveal the + // 2FA step (deleteRevealTwoFactor). + req := deleteAccountRequest(t, ctx, userID, "", "") + rr := httptest.NewRecorder() + DeleteAccountHandler(rr, req) + require.Equal(t, http.StatusBadRequest, rr.Code, rr.Body.String()) + require.Contains(t, rr.Body.String(), "a two-factor verification code is required to delete the account") + + // A wrong code is rejected too (the account survives). + seedPendingTwoFA(t, ctx, tx, userID, "424242") + req = deleteAccountRequest(t, ctx, userID, "", "000000") + rr = httptest.NewRecorder() + DeleteAccountHandler(rr, req) + require.Equal(t, http.StatusBadRequest, rr.Code, rr.Body.String()) + + var firstName string + require.NoError(t, tx.QueryRow(ctx, `SELECT n_first_name FROM users WHERE id = $1`, userID).Scan(&firstName)) + require.Equal(t, "Test", firstName, "the account must survive a rejected code") + + // A correct fresh code is the sole credential — it deletes the account. + req = deleteAccountRequest(t, ctx, userID, "", "424242") + rr = httptest.NewRecorder() + DeleteAccountHandler(rr, req) + require.Equal(t, http.StatusNoContent, rr.Code, rr.Body.String()) +} + +// TestPasswordChange_NullHash_NoPasswordToChange verifies FIX 5b: changing the +// password on a passwordless (NULL hash) account is a clear 400 with an +// actionable message — not the old 500 from scanning NULL into a plain string. +func TestPasswordChange_NullHash_NoPasswordToChange(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + _, err = tx.Exec(ctx, `UPDATE users SET password_hash = NULL WHERE id = $1`, userID) + require.NoError(t, err) + + req := changePasswordRequest(t, ctx, userID, "whatever", "newpassword456") + rr := httptest.NewRecorder() + ChangePasswordHandler(rr, req) + require.Equal(t, http.StatusBadRequest, rr.Code, rr.Body.String()) + require.Contains(t, rr.Body.String(), "this account has no password to change") + + // The password must be untouched. + var stored sql.NullString + require.NoError(t, tx.QueryRow(ctx, `SELECT password_hash FROM users WHERE id = $1`, userID).Scan(&stored)) + require.False(t, stored.Valid, "a passwordless account must remain passwordless after a rejected change") +} + +// ============================================================================ +// FIX 1a — CardDAV vCard deleted inside the erasure transaction +// ============================================================================ + +// TestDeleteAccount_DavCardDeletedInErasureTx verifies FIX 1a: the dav_cards +// row (full name/email/phone/DOB/photo URL PII) is deleted INSIDE the erasure +// transaction — the old fire-and-forget dav.Service.DeleteContact goroutine is +// gone, so the contact PII can never be stranded by a crash or a log-only +// failure. +func TestDeleteAccount_DavCardDeletedInErasureTx(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + + uri := userID + ".vcf" + _, err = tx.Exec(ctx, ` + INSERT INTO dav_cards (addressbookid, uri, carddata, lastmodified, etag, size) + VALUES (1, $1, 'BEGIN:VCARD', 0, '0', 0) + `, uri) + require.NoError(t, err) + + var countBefore int + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM dav_cards WHERE uri = $1`, uri).Scan(&countBefore)) + require.Equal(t, 1, countBefore) + + req := deleteAccountRequest(t, ctx, userID, "testpassword123", "") + rr := httptest.NewRecorder() + DeleteAccountHandler(rr, req) + require.Equal(t, http.StatusNoContent, rr.Code, rr.Body.String()) + + var countAfter int + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM dav_cards WHERE uri = $1`, uri).Scan(&countAfter)) + require.Zero(t, countAfter, "the dav_cards row must be deleted inside the erasure transaction") +} diff --git a/backend/handlers/webhooks/webhooks_round8_test.go b/backend/handlers/webhooks/webhooks_round8_test.go new file mode 100644 index 0000000..f894250 --- /dev/null +++ b/backend/handlers/webhooks/webhooks_round8_test.go @@ -0,0 +1,369 @@ +//go:build test + +package webhooks + +// Round 8 regression tests — CRITICAL/MAJOR webhook money-safety fixes in +// handlePaymentUpdated / handleRefundUpdated (backend/handlers/webhooks/square.go): +// +// 1. payment.completed for a CANCELLED booking → the pending row is marked +// FAILED (never completed), an M2 auto-refund row for the full stranded +// charge (origin 'cancellation', deterministic paymentID+"-square-"+pence +// key) is inserted, and a critical admin notification is raised. +// 2. payment.completed for a payable booking → the pending row is promoted to +// 'completed', the charge is re-split (deposit + balance), and the +// fully-paid booking is completed with the loyalty/campaign side-effects. +// 3. a genuinely unknown payment.completed (no local payments/till_sales row, +// no orphan origin) → 503, no dedup row (Square retries). +// 4. refund.updated APPROVED/COMPLETED arriving before the refund row exists +// → 503, no dedup row (the row may be inserted in the same transaction as +// the charge in some paths). +// 5. C6: a booking-less payment row (gift-card purchase) is left pending by a +// COMPLETED payment.updated — the same-key retry delivers the card. + +import ( + "context" + "encoding/json" + "net/http" + "testing" + + "crussell/db" + "crussell/testutils/fixtures" +) + +// attachWebhookTestBooking creates a fresh pending booking with total_amount +// set to total and attaches the given payment to it. The webhook's booking +// gate (round-8) only completes booking-attached rows; the fixture-created +// booking has no total_amount, which would make the fully-paid completion +// check a no-op, so the explicit total lets the payable-booking path run its +// split/completion side-effects deterministically. +func attachWebhookTestBooking(t *testing.T, payID string, total float64) (bookingID string) { + t.Helper() + userID, err := fixtures.CreateTestUser(db.Conn) + if err != nil { + t.Fatalf("failed to create test user: %v", err) + } + serviceID, err := fixtures.CreateTestService(db.Conn) + if err != nil { + t.Fatalf("failed to create test service: %v", err) + } + bookingID, err = fixtures.CreateTestBooking(db.Conn, userID, serviceID) + if err != nil { + t.Fatalf("failed to create test booking: %v", err) + } + if _, err := db.Conn.Exec(context.Background(), + "UPDATE bookings SET total_amount = $1 WHERE id = $2", total, bookingID); err != nil { + t.Fatalf("failed to set booking total: %v", err) + } + if _, err := db.Conn.Exec(context.Background(), + "UPDATE payments SET booking_id = $1 WHERE id = $2", bookingID, payID); err != nil { + t.Fatalf("failed to attach payment to booking: %v", err) + } + return bookingID +} + +// TestWebhook_Round8_PaymentCompleted_CancelledBooking_FailsWithAutoRefund +// locks fix 1: a payment.completed whose booking is cancelled/lapsed/no-show +// must NEVER be completed — the cancellation refund path computes refunds from +// completed payments and would miss it, charging a customer with NO automatic +// refund (F3). The pending row is marked FAILED, an M2 auto-refund row for the +// full stranded charge (origin 'cancellation') is inserted so the +// pending-refund sweep issues it at Square, and a critical admin notification +// is raised — mirroring the stale-pending sweep's gate refused branch. +func TestWebhook_Round8_PaymentCompleted_CancelledBooking_FailsWithAutoRefund(t *testing.T) { + const squarePaymentID = "sqp_round8_cancelled" + payID := createWebhookTestPayment(t, squarePaymentID, "pending") + bookingID := attachWebhookTestBooking(t, payID, 40.00) + // The payment amount drives the auto-refund row (the deterministic key is + // paymentID + "-square-" + amount pence). + if _, err := db.Conn.Exec(context.Background(), + "UPDATE payments SET amount = 40.00 WHERE id = $1", payID); err != nil { + t.Fatalf("failed to set payment amount: %v", err) + } + if _, err := db.Conn.Exec(context.Background(), + "UPDATE bookings SET status = 'client_cancelled' WHERE id = $1", bookingID); err != nil { + t.Fatalf("failed to cancel booking: %v", err) + } + + event := SquareWebhookEvent{ + Type: "payment.completed", + EventID: "evt_round8_cancelled_1", + CreatedAt: nowInRFC3339(0), + Data: json.RawMessage(`{ + "type": "payment", + "id": "` + squarePaymentID + `", + "object": { + "payment": { + "id": "` + squarePaymentID + `", + "status": "COMPLETED", + "amount_money": {"amount": 4000, "currency": "GBP"} + } + } + }`), + } + w := deliverWebhook(t, event) + if w.Code != http.StatusOK { + t.Fatalf("expected 200 (the refused row is still resolved in-app), got %d: %s", w.Code, w.Body.String()) + } + if got := getPaymentStatus(t, payID); got != "failed" { + t.Errorf("expected the cancelled-booking payment to be marked 'failed', got %q", got) + } + var ( + refundBookingID string + amount float64 + origin string + key string + ) + err := db.Conn.QueryRow(context.Background(), ` + SELECT booking_id, amount, origin, idempotency_key FROM refunds WHERE payment_id = $1 + `, payID).Scan(&refundBookingID, &amount, &origin, &key) + if err != nil { + t.Fatalf("expected an M2 auto-refund row for the stranded charge, got: %v", err) + } + if refundBookingID != bookingID || amount != 40.00 || origin != "cancellation" { + t.Errorf("expected a cancellation-origin refund of 40.00 on the booking, got booking=%s amount=%v origin=%q", refundBookingID, amount, origin) + } + if want := payID + "-square-4000"; key != want { + t.Errorf("expected deterministic refund key %q, got %q", want, key) + } + if n := countUnackedCriticalNotificationsForBooking(t, bookingID); n != 1 { + t.Errorf("expected exactly 1 unacknowledged critical notification for the booking, got %d", n) + } + if n := countWebhookEvents(t, event.EventID); n != 1 { + t.Errorf("expected 1 dedup row, got %d", n) + } +} + +// TestWebhook_Round8_PaymentCompleted_PayableBooking_CompletesWithSplits locks +// fix 2: a payment.completed on a payable booking promotes the pending row to +// 'completed' and runs the SAME split/VAT/completion side-effects the sweep +// rescue applies — the charge is re-split (deposit + balance), a deposit-paid +// pending_release booking is promoted to confirmed, and a fully-paid booking is +// completed with the loyalty/campaign bookkeeping (the payment-time campaign +// discount row is preserved, never re-applied). +func TestWebhook_Round8_PaymentCompleted_PayableBooking_CompletesWithSplits(t *testing.T) { + const ( + squarePaymentID = "sqp_round8_payable" + campaignName = "Round8 Completion Campaign" + ) + payID := createWebhookTestPayment(t, squarePaymentID, "pending") + bookingID := attachWebhookTestBooking(t, payID, 50.00) + var userID string + if err := db.Conn.QueryRow(context.Background(), + "SELECT user_id FROM bookings WHERE id = $1", bookingID).Scan(&userID); err != nil { + t.Fatalf("failed to read booking user: %v", err) + } + + // Active time_based campaign — the completion side-effects would apply it + // were it not already recorded at payment time. + var campaignID string + if err := db.Conn.QueryRow(context.Background(), ` + INSERT INTO discount_campaigns (name, campaign_type, discount_percent, status, start_date, end_date, max_redemptions, times_redeemed) + VALUES ($1, 'time_based', 10, 'active', NOW() - INTERVAL '1 day', NOW() + INTERVAL '1 day', 100, 0) + RETURNING id + `, campaignName).Scan(&campaignID); err != nil { + t.Fatalf("failed to seed campaign: %v", err) + } + // The online flow applies the campaign at PAYMENT time (before the charge): + // a discount ledger row + the booking_discounts record. The completion's + // already-recorded guard then skips re-application. + if _, err := db.Conn.Exec(context.Background(), ` + INSERT INTO payments (booking_id, payment_type, payment_method, amount, status, created_by) + VALUES ($1, 'partial', 'discount', 5.00, 'completed', $2) + `, bookingID, userID); err != nil { + t.Fatalf("failed to seed discount payment row: %v", err) + } + if _, err := db.Conn.Exec(context.Background(), ` + INSERT INTO booking_discounts (booking_id, user_id, discount_source, source_id, campaign_type, discount_percent, original_total, discount_amount) + VALUES ($1, $2, 'campaign', $3, 'time_based', 10, 50.00, 5.00) + `, bookingID, userID, campaignID); err != nil { + t.Fatalf("failed to seed booking discount: %v", err) + } + // The charge that lands at Square is the DISCOUNTED amount (£45 for a £50 + // booking at 10% off); the pending row records it. + if _, err := db.Conn.Exec(context.Background(), + "UPDATE payments SET amount = 45.00, idempotency_key = 'round8-payable-key' WHERE id = $1", payID); err != nil { + t.Fatalf("failed to set payment amount/key: %v", err) + } + + event := SquareWebhookEvent{ + Type: "payment.completed", + EventID: "evt_round8_payable_1", + CreatedAt: nowInRFC3339(0), + Data: json.RawMessage(`{ + "type": "payment", + "id": "` + squarePaymentID + `", + "object": { + "payment": { + "id": "` + squarePaymentID + `", + "status": "COMPLETED", + "idempotency_key": "round8-payable-key", + "amount_money": {"amount": 4500, "currency": "GBP"} + } + } + }`), + } + w := deliverWebhook(t, event) + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + if got := getPaymentStatus(t, payID); got != "completed" { + t.Fatalf("expected payment 'completed', got %q", got) + } + var bookingStatus string + if err := db.Conn.QueryRow(context.Background(), + "SELECT status FROM bookings WHERE id = $1", bookingID).Scan(&bookingStatus); err != nil { + t.Fatalf("failed to read booking status: %v", err) + } + if bookingStatus != "completed" { + t.Errorf("expected the fully-paid booking to be 'completed', got %q", bookingStatus) + } + // Split records: the £45 charge carves a £25 deposit primary + £20 balance + // (pre-start split) — plus the £5 discount row, so the booking ledger holds + // 3 completed rows and is fully paid (45 + 5 = 50). + var payCount int + if err := db.Conn.QueryRow(context.Background(), + "SELECT COUNT(*) FROM payments WHERE booking_id = $1", bookingID).Scan(&payCount); err != nil { + t.Fatalf("failed to count payment records: %v", err) + } + if payCount != 3 { + t.Errorf("expected 3 payment records (deposit + balance + discount), got %d", payCount) + } + // The payment-time campaign discount row is preserved (never re-applied or + // dropped by the completion side-effects). + var discCount int + if err := db.Conn.QueryRow(context.Background(), ` + SELECT COUNT(*) FROM booking_discounts + WHERE booking_id = $1 AND discount_source = 'campaign' AND campaign_type = 'time_based' + `, bookingID).Scan(&discCount); err != nil { + t.Fatalf("failed to count booking discounts: %v", err) + } + if discCount != 1 { + t.Errorf("expected the campaign discount row to be preserved, got %d", discCount) + } +} + +// TestWebhook_Round8_UnknownPaymentCompleted_Returns503 locks fix 3: a +// payment.completed whose square_payment_id matches NO local row (payments or +// till_sales) and NO pending orphan origin is a genuinely unknown money event. +// It is NOT acked — the handler returns 503 so Square re-delivers (its retry +// budget bounds the retries) and writes NO dedup row, so the event can never be +// dropped permanently. +func TestWebhook_Round8_UnknownPaymentCompleted_Returns503(t *testing.T) { + const squarePaymentID = "sqp_round8_unknown" + event := SquareWebhookEvent{ + Type: "payment.completed", + EventID: "evt_round8_unknown_1", + CreatedAt: nowInRFC3339(0), + Data: json.RawMessage(`{ + "type": "payment", + "id": "` + squarePaymentID + `", + "object": { + "payment": { + "id": "` + squarePaymentID + `", + "status": "COMPLETED", + "idempotency_key": "round8-never-used", + "amount_money": {"amount": 1000, "currency": "GBP"} + } + } + }`), + } + w := deliverWebhook(t, event) + if w.Code != http.StatusServiceUnavailable { + t.Fatalf("expected 503 for a genuinely unknown COMPLETED payment (Square must retry), got %d: %s", w.Code, w.Body.String()) + } + if n := countWebhookEvents(t, event.EventID); n != 0 { + t.Errorf("expected NO dedup row for the unresolved unknown payment, got %d", n) + } +} + +// TestWebhook_Round8_RefundUpdated_BeforeRowExists_Returns503 locks fix 4: an +// APPROVED or COMPLETED refund.updated arriving BEFORE the local refunds row +// exists must not be acked-and-dropped — the refund row can be created in the +// same transaction as the charge in some paths, and acking would lose the +// terminal settlement trail forever. The handler returns 503 so Square +// re-delivers once the row appears. +func TestWebhook_Round8_RefundUpdated_BeforeRowExists_Returns503(t *testing.T) { + approved := SquareWebhookEvent{ + Type: "refund.updated", + EventID: "evt_round8_refund_approved_1", + CreatedAt: nowInRFC3339(0), + Data: json.RawMessage(`{ + "type": "refund", + "id": "sqr_round8_unknown", + "object": { + "refund": { + "id": "sqr_round8_unknown", + "status": "APPROVED", + "payment_id": "sqp_round8_refund_unknown" + } + } + }`), + } + w := deliverWebhook(t, approved) + if w.Code != http.StatusServiceUnavailable { + t.Fatalf("expected 503 for APPROVED refund before its row exists, got %d: %s", w.Code, w.Body.String()) + } + if n := countWebhookEvents(t, approved.EventID); n != 0 { + t.Errorf("expected no dedup row for the APPROVED-before-row refund, got %d", n) + } + + completed := SquareWebhookEvent{ + Type: "refund.updated", + EventID: "evt_round8_refund_completed_1", + CreatedAt: nowInRFC3339(0), + Data: json.RawMessage(`{ + "type": "refund", + "id": "sqr_round8_unknown2", + "object": { + "refund": { + "id": "sqr_round8_unknown2", + "status": "COMPLETED", + "payment_id": "sqp_round8_refund_unknown2" + } + } + }`), + } + w2 := deliverWebhook(t, completed) + if w2.Code != http.StatusServiceUnavailable { + t.Fatalf("expected 503 for COMPLETED refund before its row exists, got %d: %s", w2.Code, w2.Body.String()) + } + if n := countWebhookEvents(t, completed.EventID); n != 0 { + t.Errorf("expected no dedup row for the COMPLETED-before-row refund, got %d", n) + } +} + +// TestWebhook_Round8_PaymentCompleted_BookinglessGiftCardRow_StaysPending +// locks the C6 semantics of fix 1: a COMPLETED payment.updated for a +// booking-less payments row (a gift-card purchase) must NOT auto-complete the +// row — the same-key retry delivers the card through the synchronous purchase +// path. The row stays pending and the dedup row still commits. +func TestWebhook_Round8_PaymentCompleted_BookinglessGiftCardRow_StaysPending(t *testing.T) { + const squarePaymentID = "sqp_round8_giftcard" + payID := createWebhookTestPayment(t, squarePaymentID, "pending") + + event := SquareWebhookEvent{ + Type: "payment.completed", + EventID: "evt_round8_giftcard_1", + CreatedAt: nowInRFC3339(0), + Data: json.RawMessage(`{ + "type": "payment", + "id": "` + squarePaymentID + `", + "object": { + "payment": { + "id": "` + squarePaymentID + `", + "status": "COMPLETED" + } + } + }`), + } + w := deliverWebhook(t, event) + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + if got := getPaymentStatus(t, payID); got != "pending" { + t.Errorf("expected the booking-less gift-card payment to stay 'pending' (C6), got %q", got) + } + if n := countWebhookEvents(t, event.EventID); n != 1 { + t.Errorf("expected 1 dedup row, got %d", n) + } +} diff --git a/backend/handlers/webhooks/webhooks_round9_adversarial_test.go b/backend/handlers/webhooks/webhooks_round9_adversarial_test.go new file mode 100644 index 0000000..44f6b19 --- /dev/null +++ b/backend/handlers/webhooks/webhooks_round9_adversarial_test.go @@ -0,0 +1,309 @@ +//go:build test + +package webhooks + +// Round 9 adversarial regression tests — the webhook-vs-synchronous-path +// double-completion race on the SAME booking/payment. +// +// The bug (R9): `reconcileCompletedPayments` runs the FULL completion +// side-effects (split records, VAT, booking completion, loyalty/campaigns) +// when a webhook promotes a pending payment row to completed. The synchronous +// saved-card path (handlers.go CreateBookingPayment) ALSO runs its own +// post-charge completion after Square returns. If the webhook arrives BETWEEN +// Square returning the charge and the sync path's recheck transaction +// committing, both paths contend on the same booking row, and the sync path — +// whose completion UPDATE had NO `status='pending'` guard — would re-run its +// side-effects on top of the webhook's: phantom duplicate split rows (whose +// deterministic idempotency keys are UNIQUE) and a re-aligned primary row that +// no longer reconciles to the Square charge. +// +// The coordinated fix has two halves: +// - the sync path (handlers.go, owned by another agent) guards its own +// completion flip on `status='pending'` and re-reads the payment status +// before re-running side-effects; +// - the webhook half (this suite): the webhook is ALREADY safe under that +// fix because its reconcile SELECT matches only `status='pending'` rows +// and its flip UPDATE is guarded on `status='pending'` — the payment +// row's status is the single mutual-exclusion point, so whichever path +// wins the flip, the side-effects run exactly once. +// +// These tests lock the webhook half: +// 1. the webhook arrives AFTER the sync path committed its flip → the +// pending-only SELECT matches nothing, the charge is acked as a known +// settled replay, and NO side-effect re-runs (no duplicate split rows, no +// re-completion of the booking); +// 2. the webhook's SELECT reads the row while it is STILL pending (the sync +// path's recheck tx holds the booking lock but has not committed), then +// the sync path commits before the webhook's guarded flip runs → the +// flip's `AND status='pending'` guard makes it a no-op and the webhook +// never re-runs the side-effects. + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + "time" + + "crussell/db" +) + +// seedRound9SyncCompletedState simulates the synchronous saved-card path having +// WON the race and fully completed the payment + booking: the primary row is +// flipped to 'completed' and re-aligned to the deposit split, the balance split +// is inserted with the deterministic idempotency key the split logic mints +// (-split-1), and the fully-paid booking is completed. This is exactly the +// ledger the webhook would produce if IT ran the side-effects, so any regression +// that makes the webhook re-run them would collide on the UNIQUE idempotency +// key or mint phantom rows. +func seedRound9SyncCompletedState(t *testing.T, payID, bookingID, squarePaymentID, idemKey, userID string, total float64) { + t.Helper() + deposit := total * 0.5 // protected deposit max: 50% of total, pre-start split + balance := total - deposit + if _, err := db.Conn.Exec(context.Background(), + `UPDATE payments SET status = 'completed', amount = $1, payment_type = 'deposit', updated_at = NOW() WHERE id = $2`, + deposit, payID); err != nil { + t.Fatalf("failed to flip payment to completed: %v", err) + } + if _, err := db.Conn.Exec(context.Background(), ` + INSERT INTO payments (booking_id, payment_type, payment_method, amount, status, square_payment_id, idempotency_key, created_by, created_at, updated_at) + VALUES ($1, 'balance', 'online_square', $2, 'completed', $3, $4, $5, NOW(), NOW()) + `, bookingID, balance, squarePaymentID, idemKey+"-split-1", userID); err != nil { + t.Fatalf("failed to seed balance split: %v", err) + } + if _, err := db.Conn.Exec(context.Background(), + `UPDATE bookings SET status = 'completed', updated_at = NOW() WHERE id = $1`, bookingID); err != nil { + t.Fatalf("failed to complete booking: %v", err) + } +} + +func countRound9BookingPayments(t *testing.T, bookingID string) int { + t.Helper() + var n int + if err := db.Conn.QueryRow(context.Background(), + "SELECT COUNT(*) FROM payments WHERE booking_id = $1", bookingID).Scan(&n); err != nil { + t.Fatalf("failed to count booking payments: %v", err) + } + return n +} + +func countRound9ByIdempotencyKey(t *testing.T, idemKey string) int { + t.Helper() + var n int + if err := db.Conn.QueryRow(context.Background(), + "SELECT COUNT(*) FROM payments WHERE idempotency_key = $1", idemKey).Scan(&n); err != nil { + t.Fatalf("failed to count payments by idempotency key: %v", err) + } + return n +} + +// round9CompletedEvent builds a payment.updated webhook carrying a COMPLETED +// Square charge. +func round9CompletedEvent(squarePaymentID, idemKey string) SquareWebhookEvent { + return SquareWebhookEvent{ + Type: "payment.updated", + EventID: "evt_round9_" + squarePaymentID, + CreatedAt: nowInRFC3339(0), + Data: json.RawMessage(`{ + "type": "payment", + "id": "` + squarePaymentID + `", + "object": { + "payment": { + "id": "` + squarePaymentID + `", + "status": "COMPLETED", + "idempotency_key": "` + idemKey + `", + "amount_money": {"amount": 5000, "currency": "GBP"} + } + } + }`), + } +} + +// TestWebhook_Round9_WebhookAfterSyncCompletion_NoOp locks fix half 1 (the R9 +// race outcome where the SYNC path wins): the synchronous saved-card path has +// already flipped the payment to 'completed', re-split it (deposit primary + +// balance), and completed the booking BEFORE the webhook is delivered. The +// webhook's reconcile SELECT matches only status='pending' rows, so it finds +// nothing and acks the charge as a known settled replay — the row is never +// re-flipped, no phantom split rows are minted, and the booking-completion +// side-effects are not double-run. +func TestWebhook_Round9_WebhookAfterSyncCompletion_NoOp(t *testing.T) { + const ( + squarePaymentID = "sqp_round9_sync_first" + idemKey = "round9-sync-key-1" + total = 50.00 + ) + payID := createWebhookTestPayment(t, squarePaymentID, "pending") + bookingID := attachWebhookTestBooking(t, payID, total) + var userID string + if err := db.Conn.QueryRow(context.Background(), + "SELECT user_id FROM bookings WHERE id = $1", bookingID).Scan(&userID); err != nil { + t.Fatalf("failed to read booking user: %v", err) + } + if _, err := db.Conn.Exec(context.Background(), + "UPDATE payments SET amount = $1, idempotency_key = $2 WHERE id = $3", total, idemKey, payID); err != nil { + t.Fatalf("failed to set payment amount/key: %v", err) + } + // The sync path completed the charge and its booking first (the race + // outcome this test locks). + seedRound9SyncCompletedState(t, payID, bookingID, squarePaymentID, idemKey, userID, total) + paymentsBefore := countRound9BookingPayments(t, bookingID) + + event := round9CompletedEvent(squarePaymentID, idemKey) + w := deliverWebhook(t, event) + if w.Code != http.StatusOK { + t.Fatalf("expected 200 (known settled charge, plain replay), got %d: %s", w.Code, w.Body.String()) + } + + // No double-completion: the payment stays exactly as the sync path left it. + if got := getPaymentStatus(t, payID); got != "completed" { + t.Errorf("expected payment to stay 'completed', got %q", got) + } + // The primary row keeps the sync path's split shape — never re-aligned or + // re-split by the webhook. + var primaryAmount float64 + var primaryType string + if err := db.Conn.QueryRow(context.Background(), + "SELECT amount, payment_type FROM payments WHERE id = $1", payID).Scan(&primaryAmount, &primaryType); err != nil { + t.Fatalf("failed to read primary payment: %v", err) + } + if primaryAmount != total/2 || primaryType != "deposit" { + t.Errorf("expected primary to stay the deposit split (%.2f/deposit), got amount=%v type=%q", total/2, primaryAmount, primaryType) + } + // No phantom split rows: the ledger is byte-identical to the sync path's. + if n := countRound9BookingPayments(t, bookingID); n != paymentsBefore { + t.Errorf("expected the payment ledger unchanged (%d rows), got %d", paymentsBefore, n) + } + // No duplicate balance split: the deterministic key exists exactly once. + if n := countRound9ByIdempotencyKey(t, idemKey+"-split-1"); n != 1 { + t.Errorf("expected the balance split key to exist exactly once, got %d", n) + } + // Booking side-effects are not double-run: the booking stays 'completed' + // (a second completion transition cannot fire). + var bookingStatus string + if err := db.Conn.QueryRow(context.Background(), + "SELECT status FROM bookings WHERE id = $1", bookingID).Scan(&bookingStatus); err != nil { + t.Fatalf("failed to read booking status: %v", err) + } + if bookingStatus != "completed" { + t.Errorf("expected booking to stay 'completed', got %q", bookingStatus) + } + // The replay is acked and its dedup row committed. + if n := countWebhookEvents(t, event.EventID); n != 1 { + t.Errorf("expected 1 dedup row, got %d", n) + } +} + +// TestWebhook_Round9_ConcurrentSyncFlip_Wins_FliGuardNoOps locks the flip-guard +// backstop — the IN-FLIGHT race (fix half 2). The webhook's pending-row SELECT +// reads the row while it is STILL 'pending' (the sync path's recheck tx holds +// the booking FOR UPDATE lock but has not committed its flip), then the sync +// path commits (flip + booking completion) BEFORE the webhook's guarded flip +// runs. The flip's `AND status='pending'` guard sees the committed 'completed' +// row and becomes a no-op: the webhook never re-runs the split/completion +// side-effects, so no duplicate rows and no double booking completion. +func TestWebhook_Round9_ConcurrentSyncFlip_Wins_FliGuardNoOps(t *testing.T) { + const ( + squarePaymentID = "sqp_round9_sync_race" + idemKey = "round9-race-key-1" + total = 50.00 + ) + payID := createWebhookTestPayment(t, squarePaymentID, "pending") + bookingID := attachWebhookTestBooking(t, payID, total) + if _, err := db.Conn.Exec(context.Background(), + "UPDATE payments SET amount = $1, idempotency_key = $2 WHERE id = $3", total, idemKey, payID); err != nil { + t.Fatalf("failed to set payment amount/key: %v", err) + } + + // The sync path (postChargeRecheck) takes the booking FOR UPDATE lock and + // holds it while its recheck tx is in flight. + ctx := context.Background() + syncTx, err := db.Conn.Begin(ctx) + if err != nil { + t.Fatalf("failed to begin sync-path tx: %v", err) + } + if _, err := syncTx.Exec(ctx, `SELECT 1 FROM bookings WHERE id = $1 FOR UPDATE`, bookingID); err != nil { + syncTx.Rollback(ctx) + t.Fatalf("failed to lock booking row: %v", err) + } + + // Deliver the webhook in a goroutine. Signature/body are prepared on the + // test goroutine (webhookTestEnv calls t.Setenv, which is test-goroutine + // only); makeWebhookRequest itself touches no *testing.T. + event := round9CompletedEvent(squarePaymentID, idemKey) + body, err := json.Marshal(event) + if err != nil { + t.Fatalf("failed to marshal webhook event: %v", err) + } + sig := webhookTestEnv(t, body) + done := make(chan *httptest.ResponseRecorder, 1) + go func() { + done <- makeWebhookRequest(body, sig, context.Background()) + }() + + // Wait (bounded) until the webhook tx is actually blocked on the booking + // FOR UPDATE lock — at that point its pending-row SELECT has ALREADY run and + // seen the row as 'pending', so the flip-guard interleaving is exercised + // deterministically. If it never blocks (a slow scheduler), the test still + // holds: the commit below turns it into the no-op replay interleaving. + waitDeadline := time.Now().Add(10 * time.Second) + blocked := false + for time.Now().Before(waitDeadline) { + var n int + if err := db.Conn.QueryRow(ctx, ` + SELECT COUNT(*) FROM pg_stat_activity + WHERE query ILIKE '%SELECT status FROM bookings WHERE id%' + AND wait_event_type = 'Lock' + AND pid <> pg_backend_pid() + `).Scan(&n); err == nil && n > 0 { + blocked = true + break + } + time.Sleep(25 * time.Millisecond) + } + if !blocked { + t.Logf("webhook never blocked on the booking lock — fell through to the no-op replay interleaving (assertions still hold)") + } + + // The sync path commits: guarded flip + booking completion. + if _, err := syncTx.Exec(ctx, + `UPDATE payments SET status = 'completed', amount = $1, payment_type = 'deposit', updated_at = NOW() WHERE id = $2`, + total/2, payID); err != nil { + syncTx.Rollback(ctx) + t.Fatalf("failed to flip payment in sync tx: %v", err) + } + if _, err := syncTx.Exec(ctx, + `UPDATE bookings SET status = 'completed', updated_at = NOW() WHERE id = $1`, bookingID); err != nil { + syncTx.Rollback(ctx) + t.Fatalf("failed to complete booking in sync tx: %v", err) + } + if err := syncTx.Commit(ctx); err != nil { + t.Fatalf("failed to commit sync-path tx: %v", err) + } + + w := <-done + if w.Code != http.StatusOK { + t.Fatalf("expected 200 (guarded flip no-op), got %d: %s", w.Code, w.Body.String()) + } + // The payment was completed exactly once — by the sync path. + if got := getPaymentStatus(t, payID); got != "completed" { + t.Errorf("expected payment 'completed', got %q", got) + } + // No split rows beyond the primary: the webhook's side-effects never ran. + if n := countRound9BookingPayments(t, bookingID); n != 1 { + t.Errorf("expected exactly 1 payment row (the sync-flipped primary), got %d", n) + } + // The booking was completed by the sync path exactly once. + var bookingStatus string + if err := db.Conn.QueryRow(context.Background(), + "SELECT status FROM bookings WHERE id = $1", bookingID).Scan(&bookingStatus); err != nil { + t.Fatalf("failed to read booking status: %v", err) + } + if bookingStatus != "completed" { + t.Errorf("expected booking 'completed' (the sync path completed it), got %q", bookingStatus) + } + if n := countWebhookEvents(t, event.EventID); n != 1 { + t.Errorf("expected 1 dedup row, got %d", n) + } +}