Expand payment test coverage: lock contention, nonce-direct, provisional rows, GDPR scrub, validators

Close the coverage-gap round: terminal CreateCheckout-failure marks the provisional row failed, GetCheckoutStatus reference_id mismatch 400, deadline wire shape, concurrent loyalty redemption 409, delete_guest_user + stale-guest saved-card scrubbing, ValidateAmount and isTokenLike direct units, bounded try-lock timeout, buildSplitRecords tip-overflow, and concurrent same-key dedup for gift card / booking / tip / checkout.

backend/handlers/payments/concurrency_test.go

@@ -423,3 +423,99 @@ func TestGetCheckoutStatus_ConcurrentPolls_SingleRecord(t *testing.T) {
 		t.Errorf("expected exactly 1 terminal payment record, got %d (double-record race!)", payCount)
 	}
 }
+
+// TestLoyaltyRedemption_ConcurrentSameBooking_SingleApply proves the loyalty
+// redemption advisory lock (loyalty.go): two goroutines redeeming the same
+// booking must apply the 10% discount exactly once. One request succeeds (200)
+// and the other is rejected by the in-lock re-check ("A loyalty discount has
+// already been applied", 400) — never two discount payments for one booking.
+func TestLoyaltyRedemption_ConcurrentSameBooking_SingleApply(t *testing.T) {
+	ctx, tx := testutils.SetupTestTx(t)
+
+	userID, bookingID, _ := setupLoyaltyUser(t, ctx, tx, 10)
+	token := jwt.GenerateUserToken(userID)
+	cleanupConcurrentTestRows(t, context.Background(), userID, bookingID)
+
+	// Commit the setup so both goroutines operate at pool level — the advisory
+	// lock only serializes across independent connections.
+	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)
+	}
+
+	pool := context.Background()
+
+	var wg sync.WaitGroup
+	startBoth := make(chan struct{})
+	recs := make([]*httptest.ResponseRecorder, 2)
+
+	for i := 0; i < 2; i++ {
+		wg.Add(1)
+		go func(idx int) {
+			defer wg.Done()
+			<-startBoth
+			recs[idx] = makeApplyRedemptionRequest(bookingID, token, pool)
+		}(i)
+	}
+	close(startBoth)
+	wg.Wait()
+
+	// Exactly one request wins the redemption; the other finds the already
+	// applied discount inside the lock and is rejected with 400 (the first
+	// handler completes in milliseconds, well under the ~3s lock bound, so the
+	// 409 lock-timeout path is not exercised).
+	okCount, rejectedCount := 0, 0
+	for i, rec := range recs {
+		switch {
+		case rec.Code == http.StatusOK:
+			okCount++
+		case rec.Code == http.StatusBadRequest || rec.Code == http.StatusConflict:
+			rejectedCount++
+		default:
+			t.Errorf("request %d unexpected status %d: %s", i, rec.Code, rec.Body.String())
+		}
+	}
+	if okCount != 1 {
+		t.Errorf("expected exactly 1 successful redemption, got %d", okCount)
+	}
+	if rejectedCount != 1 {
+		t.Errorf("expected exactly 1 rejected redemption, got %d", rejectedCount)
+	}
+
+	// Exactly one loyalty discount and one discount payment row.
+	var discountCount int
+	if err := db.Conn.QueryRow(pool,
+		`SELECT COUNT(*) FROM booking_discounts WHERE booking_id = $1 AND discount_source = 'loyalty'`, bookingID).Scan(&discountCount); err != nil {
+		t.Fatalf("failed to count loyalty discounts: %v", err)
+	}
+	if discountCount != 1 {
+		t.Errorf("expected exactly 1 loyalty discount, got %d (double-apply!)", discountCount)
+	}
+	var payCount int
+	if err := db.Conn.QueryRow(pool,
+		`SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND payment_method = 'discount'`, bookingID).Scan(&payCount); err != nil {
+		t.Fatalf("failed to count discount payments: %v", err)
+	}
+	if payCount != 1 {
+		t.Errorf("expected exactly 1 discount payment, got %d", payCount)
+	}
+
+	// The redemption is applied exactly once.
+	var appliedCount int
+	if err := db.Conn.QueryRow(pool,
+		`SELECT COUNT(*) FROM loyalty_redemptions WHERE user_id = $1 AND status = 'applied'`, userID).Scan(&appliedCount); err != nil {
+		t.Fatalf("failed to count applied redemptions: %v", err)
+	}
+	if appliedCount != 1 {
+		t.Errorf("expected exactly 1 applied redemption, got %d", appliedCount)
+	}
+
+	// Clean up the committed loyalty_redemptions row (cleanupConcurrentTestRows
+	// does not cover it; user_id is SET NULL by the FK so it would otherwise leak).
+	t.Cleanup(func() {
+		_, _ = db.Conn.Exec(pool, `DELETE FROM loyalty_redemptions WHERE user_id = $1 OR applied_to_booking_id = $2`, userID, bookingID)
+	})
+}

backend/handlers/payments/payment_status_test.go

@@ -809,3 +809,62 @@ func TestGetCheckoutStatus_TwoEqualAmountCharges_NoCollision(t *testing.T) {
 		t.Errorf("expected exactly 2 payment rows for two equal-amount charges, got %d", rowCount)
 	}
 }
+
+// mismatchedRefCheckoutClient forces GetCheckout to return a fixed COMPLETED
+// payment whose reference_id points at a DIFFERENT booking, deterministically
+// exercising GetCheckoutStatus's ownership check.
+type mismatchedRefCheckoutClient struct {
+	square.SquareClient
+	result *square.PaymentResult
+}
+
+func (c *mismatchedRefCheckoutClient) GetCheckout(ctx context.Context, checkoutID string) (*square.PaymentResult, error) {
+	return c.result, nil
+}
+
+func TestGetCheckoutStatus_ReferenceIDMismatch_Returns400(t *testing.T) {
+	// The terminal checkout's reference_id must match the booking being
+	// polled; a checkout that references a different booking is refused with
+	// 400 so its payment can never be recorded against the wrong booking.
+	ctx, tx := testutils.SetupTestTx(t)
+	_, bookingID, _ := setupTestData(t, ctx, tx)
+
+	origClient := SquareClient
+	SquareClient = &mismatchedRefCheckoutClient{
+		SquareClient: square.NewDevClient(),
+		result: &square.PaymentResult{
+			ID:          "pay_mismatch",
+			Status:      "COMPLETED",
+			Amount:      5000,
+			SquarePayID: "pay_mismatch",
+			ReferenceID: "00000000dead", // a DIFFERENT booking
+			CreatedAt:   "2026-07-31T00:00:00Z",
+			UpdatedAt:   "2026-07-31T00:00:00Z",
+		},
+	}
+	defer func() { SquareClient = origClient }()
+
+	checkoutID := "abcd1234ef56" // 12 hex chars, passes the checkout-id validation
+	req := httptest.NewRequest("GET", "/api/admin/payments/"+checkoutID+"/status?booking_id="+bookingID, nil)
+	rctx := chi.NewRouteContext()
+	rctx.URLParams.Add("checkout_id", checkoutID)
+	reqCtx := context.WithValue(req.Context(), chi.RouteCtxKey, rctx)
+	reqCtx = context.WithValue(reqCtx, mw.UserIDKey, "000000000001")
+	reqCtx = context.WithValue(reqCtx, mw.UserRoleKey, "admin")
+	req = req.WithContext(reqCtx)
+
+	w := httptest.NewRecorder()
+	GetCheckoutStatus(w, req)
+	if w.Code != http.StatusBadRequest {
+		t.Fatalf("expected 400 for reference_id mismatch, got %d: %s", w.Code, w.Body.String())
+	}
+
+	// No payment may be recorded for the mismatched checkout.
+	var rowCount int
+	if err := tx.QueryRow(ctx, "SELECT COUNT(*) FROM payments WHERE booking_id = $1", bookingID).Scan(&rowCount); err != nil {
+		t.Fatalf("failed to count payment rows: %v", err)
+	}
+	if rowCount != 0 {
+		t.Errorf("expected no payment rows after reference mismatch, got %d", rowCount)
+	}
+}

backend/handlers/payments/payments_fixes_test.go

@@ -898,9 +898,12 @@ func TestCreateTerminalPayment_RecordInsertFailure_CancelsOrphanedCheckout(t *te
 	_, bookingID, _ := setupTestData(t, ctx, tx)
 	adminToken := jwt.GenerateAdminToken()
 
-	// A COMPLETED terminal_checkouts row with the same checkout_id the client
-	// will return forces the handler's INSERT to collide on the PK while the
-	// active-checkout guard (PENDING/IN_PROGRESS only) does not fire.
+	// R3: the handler now inserts the tracked row FIRST with a provisional
+	// (tmp-) checkout_id, then CreateCheckout, then UPDATEs the row to the real
+	// Square id. A COMPLETED terminal_checkouts row with the same checkout_id
+	// the client will return makes that tracking UPDATE collide on the PK while
+	// the active-checkout guard (PENDING/IN_PROGRESS only) does not fire — the
+	// checkout is live at Square but untracked, so the handler must cancel it.
 	const dupCheckoutID = "chk_dup_insert_01"
 	if _, err := tx.Exec(ctx, `
 		INSERT INTO terminal_checkouts (checkout_id, booking_id, payment_type, status, amount)
@@ -926,3 +929,56 @@ func TestCreateTerminalPayment_RecordInsertFailure_CancelsOrphanedCheckout(t *te
 		t.Errorf("expected exactly one CancelCheckout for the orphaned checkout %q, got %v", dupCheckoutID, calls)
 	}
 }
+
+func TestCreateTerminalPayment_CreateCheckoutFailure_MarksProvisionalRowFailed(t *testing.T) {
+	// R3: the handler inserts the tracked row FIRST with a provisional
+	// ("tmp-") checkout_id, THEN calls Square CreateCheckout. When Square fails
+	// AFTER that insert, the handler must mark the provisional row failed so a
+	// retry can proceed — otherwise the "tmp-" row (provably pre-Square) wedges
+	// the booking's in-flight guard forever.
+	ctx, tx := testutils.SetupTestTx(t)
+	_, bookingID, _ := setupTestData(t, ctx, tx)
+	adminToken := jwt.GenerateAdminToken()
+
+	origClient := SquareClient
+	mc := square.NewDevClient().(*square.MockClient)
+	mc.FailCreateCheckout = true
+	SquareClient = mc
+	defer func() { SquareClient = origClient }()
+
+	handler := CreateTerminalPayment
+	req := CreateTerminalPaymentRequest{Amount: 5000, PaymentType: "full"}
+	w := makePaymentRequest(handler, "POST", "/api/admin/bookings/"+bookingID+"/payment", req, adminToken, ctx)
+	if w.Code != http.StatusInternalServerError {
+		t.Fatalf("expected 500 when Square CreateCheckout fails, got %d: %s", w.Code, w.Body.String())
+	}
+
+	// The provisional row must have been marked failed, and its checkout_id
+	// must still be the provisional "tmp-" value (Square never returned a real
+	// id, so nothing may overwrite it).
+	var status, checkoutID string
+	err := tx.QueryRow(ctx, `
+		SELECT status, checkout_id FROM terminal_checkouts WHERE booking_id = $1
+	`, bookingID).Scan(&status, &checkoutID)
+	if err != nil {
+		t.Fatalf("failed to query terminal checkout row: %v", err)
+	}
+	if status != "failed" {
+		t.Errorf("expected provisional terminal_checkouts row status 'failed', got %q", status)
+	}
+	if !strings.HasPrefix(checkoutID, "tmp-") {
+		t.Errorf("expected checkout_id to remain provisional (tmp- prefix), got %q", checkoutID)
+	}
+
+	// A retry after the failure must be able to create a fresh checkout: the
+	// in-flight guard must not see the failed row as active.
+	var activeCount int
+	if err := tx.QueryRow(ctx, `
+		SELECT COUNT(*) FROM terminal_checkouts WHERE booking_id = $1 AND status IN ('PENDING', 'IN_PROGRESS')
+	`, bookingID).Scan(&activeCount); err != nil {
+		t.Fatalf("failed to count active terminal checkouts: %v", err)
+	}
+	if activeCount != 0 {
+		t.Errorf("expected 0 active terminal checkouts after the failure, got %d", activeCount)
+	}
+}

backend/handlers/payments/payments_r6_review_test.go

@@ -0,0 +1,347 @@
+//go:build test && dev
+
+package payments
+
+import (
+	"context"
+	"fmt"
+	"net/http"
+	"net/http/httptest"
+	"sync"
+	"testing"
+
+	"crussell/db"
+	"crussell/internal/square"
+	"crussell/testutils"
+	"crussell/testutils/fixtures"
+	"crussell/testutils/jwt"
+
+	"github.com/go-chi/chi/v5"
+	"github.com/stretchr/testify/require"
+)
+
+// =============================================================================
+// R6: ccof charges must ALWAYS carry a CustomerID; one-off non-save charges
+// use the cnon: nonce directly (no card-on-file, no customer).
+// =============================================================================
+
+// TestCreateBookingPayment_SavedCard_LegacyNoCustomer_ProvisionsAndCharges
+// covers the lazy Square-customer provisioning for saved-card rows that
+// predate P14 (square_customer_id empty): the row must be provisioned AND the
+// provisioned id persisted on the row BEFORE the ccof: charge goes out.
+func TestCreateBookingPayment_SavedCard_LegacyNoCustomer_ProvisionsAndCharges(t *testing.T) {
+	ctx, tx := testutils.SetupTestTx(t)
+	userID, bookingID, userToken := setupPaymentStatusTest(t, ctx, tx, "confirmed")
+
+	cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "ccof:sq_legacy", "VISA", "4242")
+	require.NoError(t, err)
+	// Row is seeded with square_customer_id empty (legacy).
+
+	origClient := SquareClient
+	rec := &recordingPaymentClient{SquareClient: square.NewDevClient()}
+	SquareClient = rec
+	defer func() { SquareClient = origClient }()
+
+	req := CreateBookingPaymentRequest{
+		Amount:         2500,
+		PaymentType:    "deposit",
+		CardID:         &cardID,
+		IdempotencyKey: "legacy-cust-" + bookingID,
+	}
+	w := makePaymentRequest(CreateBookingPayment, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx)
+	require.Equal(t, http.StatusOK, w.Code, w.Body.String())
+
+	rec.mu.Lock()
+	got := rec.lastReq.CustomerID
+	rec.mu.Unlock()
+	require.NotEmpty(t, got, "a legacy ccof: charge must be provisioned a Square customer before charging")
+
+	var persisted string
+	require.NoError(t, tx.QueryRow(ctx, `SELECT COALESCE(square_customer_id, '') FROM user_saved_cards WHERE id = $1`, cardID).Scan(&persisted))
+	require.Equal(t, got, persisted, "the provisioned customer id must be persisted on the saved-card row")
+}
+
+// TestCreateBookingPayment_SaveCard_ChargeForwardsCustomerID covers the save
+// path of the new-card flow: the ccof: charge MUST carry the same provisioned
+// customer id that CreateCardOnFile used.
+func TestCreateBookingPayment_SaveCard_ChargeForwardsCustomerID(t *testing.T) {
+	ctx, tx := testutils.SetupTestTx(t)
+	_, bookingID, userToken := setupPaymentStatusTest(t, ctx, tx, "confirmed")
+
+	origClient := SquareClient
+	cof := &recordingCardOnFileClient{SquareClient: square.NewDevClient()}
+	rec := &recordingPaymentClient{SquareClient: cof}
+	SquareClient = rec
+	defer func() { SquareClient = origClient }()
+
+	cardToken := "cnon:save-card-nonce"
+	req := CreateBookingPaymentRequest{
+		Amount:         2500,
+		PaymentType:    "deposit",
+		NewCardToken:   &cardToken,
+		SaveCard:       true,
+		IdempotencyKey: "save-charge-cust-" + bookingID,
+	}
+	w := makePaymentRequest(CreateBookingPayment, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx)
+	require.Equal(t, http.StatusOK, w.Code, w.Body.String())
+
+	require.Equal(t, 1, cof.callCount(), "a save-card flow tokenizes via CreateCardOnFile exactly once")
+	require.NotEmpty(t, cof.lastCustomerID(), "a save-card flow must provision a Square customer")
+
+	rec.mu.Lock()
+	got := rec.lastReq.CustomerID
+	rec.mu.Unlock()
+	require.Equal(t, cof.lastCustomerID(), got, "a save-card (ccof:) charge must carry the provisioned customer id")
+	require.NotEmpty(t, got)
+}
+
+// TestBuyGiftCard_SaveCard_ChargeForwardsCustomerID is the BuyGiftCard
+// counterpart of the above: the gift-card purchase's ccof: charge must carry
+// the provisioned customer id.
+func TestBuyGiftCard_SaveCard_ChargeForwardsCustomerID(t *testing.T) {
+	ctx, tx := testutils.SetupTestTx(t)
+	userID, err := fixtures.CreateTestUser(tx)
+	require.NoError(t, err)
+	token := jwt.GenerateUserToken(userID)
+
+	origClient := SquareClient
+	cof := &recordingCardOnFileClient{SquareClient: square.NewDevClient()}
+	rec := &recordingPaymentClient{SquareClient: cof}
+	SquareClient = rec
+	defer func() { SquareClient = origClient }()
+
+	newToken := "cnon:save-gift-card-nonce"
+	req := BuyGiftCardRequest{
+		Amount:         1000,
+		RecipientType:  "self",
+		NewCardToken:   &newToken,
+		SaveCard:       true,
+		IdempotencyKey: "buy-gc-save-charge-cust",
+	}
+	w := makePaymentRequest(BuyGiftCard, "POST", "/api/gift-cards/buy", req, token, ctx)
+	require.Equal(t, http.StatusCreated, w.Code, w.Body.String())
+
+	require.Equal(t, 1, cof.callCount(), "a save-card flow tokenizes via CreateCardOnFile exactly once")
+	rec.mu.Lock()
+	got := rec.lastReq.CustomerID
+	rec.mu.Unlock()
+	require.Equal(t, cof.lastCustomerID(), got, "a save-card (ccof:) gift-card purchase must carry the provisioned customer id")
+	require.NotEmpty(t, got)
+}
+
+// =============================================================================
+// R2: BuyGiftCard requires an idempotency key
+// =============================================================================
+
+func TestBuyGiftCard_MissingIdempotencyKey_Rejected(t *testing.T) {
+	ctx, tx := testutils.SetupTestTx(t)
+	userID, err := fixtures.CreateTestUser(tx)
+	require.NoError(t, err)
+	token := jwt.GenerateUserToken(userID)
+
+	newToken := "cnon:test-card"
+	req := BuyGiftCardRequest{
+		Amount:        1000,
+		RecipientType: "self",
+		NewCardToken:  &newToken,
+		SaveCard:      false,
+	}
+	w := makePaymentRequest(BuyGiftCard, "POST", "/api/gift-cards/buy", req, token, ctx)
+	require.Equal(t, http.StatusBadRequest, w.Code, "an empty idempotency key must be rejected (R2)")
+}
+
+// =============================================================================
+// R3: provisional (pre-Square) terminal checkout rows
+// =============================================================================
+
+// TestActiveTerminalCheckoutID_ResolvesProvisionalRow covers the crash-window
+// guard: a PENDING row with a synthetic "tmp-" checkout_id never had a checkout
+// created at Square, so it is provably not live — the guard must mark it failed
+// and allow a fresh checkout instead of wedging the booking.
+func TestActiveTerminalCheckoutID_ResolvesProvisionalRow(t *testing.T) {
+	ctx, tx := testutils.SetupTestTx(t)
+	_, bookingID, _ := setupTestData(t, ctx, tx)
+
+	provisionalID := "tmp-crash-window-1"
+	if _, err := tx.Exec(ctx, `
+		INSERT INTO terminal_checkouts (checkout_id, booking_id, payment_type, status, amount)
+		VALUES ($1, $2, 'full', 'PENDING', 50.00)
+	`, provisionalID, bookingID); err != nil {
+		t.Fatalf("failed to seed provisional terminal checkout: %v", err)
+	}
+
+	got := activeTerminalCheckoutID(ctx, bookingID)
+	require.Equal(t, "", got, "a provisional (pre-Square) row must not block a new checkout")
+
+	var status string
+	require.NoError(t, tx.QueryRow(ctx, `SELECT status FROM terminal_checkouts WHERE checkout_id = $1`, provisionalID).Scan(&status))
+	require.Equal(t, "failed", status, "the provisional row must be marked failed")
+}
+
+// failingGetCheckoutClient errors on every GetCheckout — if the sweep wrongly
+// resolves a provisional row against Square, the row is left pending (the error
+// is not a terminal checkout state) and the test fails.
+type failingGetCheckoutClient struct {
+	square.SquareClient
+}
+
+func (c *failingGetCheckoutClient) GetCheckout(ctx context.Context, checkoutID string) (*square.PaymentResult, error) {
+	return nil, fmt.Errorf("GetCheckout was called for %s — provisional rows must resolve without a Square round-trip", checkoutID)
+}
+
+// TestSweepStaleTerminalCheckouts_ResolvesProvisionalRowWithoutSquare proves
+// the sweep resolves a stale provisional row to failed without calling Square.
+func TestSweepStaleTerminalCheckouts_ResolvesProvisionalRowWithoutSquare(t *testing.T) {
+	ctx, tx := testutils.SetupTestTx(t)
+	userID, bookingID, serviceID := setupTestData(t, ctx, tx)
+
+	provisionalID := "tmp-sweep-provisional-1"
+	if _, err := tx.Exec(ctx, `
+		INSERT INTO terminal_checkouts (checkout_id, booking_id, payment_type, status, amount, created_at)
+		VALUES ($1, $2, 'full', 'PENDING', 50.00, NOW() - INTERVAL '2 hours')
+	`, provisionalID, bookingID); err != nil {
+		t.Fatalf("failed to seed provisional terminal checkout: %v", err)
+	}
+
+	pgxTx := db.TxFromContext(ctx)
+	require.NotNil(t, pgxTx)
+	require.NoError(t, pgxTx.Commit(ctx))
+
+	t.Cleanup(func() {
+		_, _ = db.Conn.Exec(context.Background(), `DELETE FROM terminal_checkouts WHERE checkout_id = $1`, provisionalID)
+		_, _ = db.Conn.Exec(context.Background(), `DELETE FROM bookings WHERE id = $1`, bookingID)
+		_, _ = db.Conn.Exec(context.Background(), `DELETE FROM services WHERE id = $1`, serviceID)
+		_, _ = db.Conn.Exec(context.Background(), `DELETE FROM users WHERE id = $1`, userID)
+	})
+
+	freshCtx := context.Background()
+	// Drop stale rows left by other sweep tests so the count is deterministic.
+	if _, err := db.Conn.Exec(freshCtx, `DELETE FROM terminal_checkouts WHERE status IN ('PENDING', 'IN_PROGRESS') AND checkout_id <> $1`, provisionalID); err != nil {
+		t.Fatalf("failed to clean leftover stale terminal checkouts: %v", err)
+	}
+	if _, err := db.Conn.Exec(freshCtx, `DELETE FROM till_sales WHERE status = 'pending' AND square_checkout_id IS NOT NULL`); err != nil {
+		t.Fatalf("failed to clean leftover stale till sales: %v", err)
+	}
+
+	origClient := SquareClient
+	SquareClient = &failingGetCheckoutClient{SquareClient: square.NewDevClient()}
+	defer func() { SquareClient = origClient }()
+
+	n, err := SweepStaleTerminalCheckouts(freshCtx)
+	require.NoError(t, err)
+	require.Equal(t, 1, n, "the stale provisional terminal checkout must be resolved by the sweep")
+
+	var status string
+	require.NoError(t, db.Conn.QueryRow(freshCtx, `SELECT status FROM terminal_checkouts WHERE checkout_id = $1`, provisionalID).Scan(&status))
+	require.Equal(t, "failed", status)
+}
+
+// =============================================================================
+// R5: GetDiscountPreviewHandler ownership (IDOR)
+// =============================================================================
+
+// TestGetDiscountPreviewHandler_WrongOwner_Forbidden covers the IDOR fix: a
+// non-admin user must not read another user's booking discount preview.
+func TestGetDiscountPreviewHandler_WrongOwner_Forbidden(t *testing.T) {
+	ctx, tx := testutils.SetupTestTx(t)
+	_, bookingID, _ := setupDiscountPreviewTest(t, ctx, tx)
+
+	otherUserID, err := fixtures.CreateTestUser(tx)
+	require.NoError(t, err)
+	otherToken := jwt.GenerateUserToken(otherUserID)
+
+	w := serveDiscountPreviewHandler(bookingID, otherUserID, otherToken, ctx)
+	require.Equal(t, http.StatusForbidden, w.Code, "a non-owner must not read another user's discount preview")
+}
+
+// TestGetDiscountPreviewHandler_NoUserContext_Unauthorized covers the fail-closed
+// 401 for requests that carry no user id.
+func TestGetDiscountPreviewHandler_NoUserContext_Unauthorized(t *testing.T) {
+	ctx, tx := testutils.SetupTestTx(t)
+	_, bookingID, _ := setupDiscountPreviewTest(t, ctx, tx)
+
+	req := httptest.NewRequest("GET", "/api/bookings/"+bookingID+"/discount-preview", nil)
+	rctx := chi.NewRouteContext()
+	rctx.URLParams.Add("id", bookingID)
+	req = req.WithContext(ctx)
+	req = req.WithContext(context.WithValue(req.Context(), chi.RouteCtxKey, rctx))
+
+	w := httptest.NewRecorder()
+	GetDiscountPreviewHandler(w, req)
+	require.Equal(t, http.StatusUnauthorized, w.Code)
+}
+
+// =============================================================================
+// R8: DeletePaymentMethod disables the Square card before the local soft-delete
+// =============================================================================
+
+type deletingCardClient struct {
+	square.SquareClient
+	mu      sync.Mutex
+	deleted []string
+}
+
+func (c *deletingCardClient) DeleteCardOnFile(ctx context.Context, cardID string) error {
+	c.mu.Lock()
+	c.deleted = append(c.deleted, cardID)
+	c.mu.Unlock()
+	return c.SquareClient.DeleteCardOnFile(ctx, cardID)
+}
+
+func (c *deletingCardClient) deletedIDs() []string {
+	c.mu.Lock()
+	defer c.mu.Unlock()
+	return append([]string(nil), c.deleted...)
+}
+
+func TestDeletePaymentMethod_DisablesSquareCard(t *testing.T) {
+	ctx, tx := testutils.SetupTestTx(t)
+	userID, err := fixtures.CreateTestUser(tx)
+	require.NoError(t, err)
+	cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "ccof:sq_del_disable", "VISA", "9999")
+	require.NoError(t, err)
+
+	origClient := SquareClient
+	rec := &deletingCardClient{SquareClient: square.NewDevClient()}
+	SquareClient = rec
+	defer func() { SquareClient = origClient }()
+
+	svc := NewPaymentService()
+	require.NoError(t, svc.DeletePaymentMethod(ctx, cardID, userID))
+
+	require.Equal(t, []string{"ccof:sq_del_disable"}, rec.deletedIDs(), "the Square card must be disabled on local delete (R8)")
+
+	var deletedAt string
+	require.NoError(t, tx.QueryRow(ctx, `SELECT COALESCE(deleted_at::text, '') FROM user_saved_cards WHERE id = $1`, cardID).Scan(&deletedAt))
+	require.NotEqual(t, "", deletedAt, "the card must still be soft-deleted locally")
+}
+
+// TestDeletePaymentMethod_SquareFailure_StillDeletesLocally covers the
+// best-effort contract: a Square disable failure must never block the local
+// soft-delete.
+func TestDeletePaymentMethod_SquareFailure_StillDeletesLocally(t *testing.T) {
+	ctx, tx := testutils.SetupTestTx(t)
+	userID, err := fixtures.CreateTestUser(tx)
+	require.NoError(t, err)
+	cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "ccof:sq_del_fail", "VISA", "8888")
+	require.NoError(t, err)
+
+	origClient := SquareClient
+	SquareClient = &failingDeleteClient{SquareClient: square.NewDevClient()}
+	defer func() { SquareClient = origClient }()
+
+	svc := NewPaymentService()
+	require.NoError(t, svc.DeletePaymentMethod(ctx, cardID, userID))
+
+	var deletedAt string
+	require.NoError(t, tx.QueryRow(ctx, `SELECT COALESCE(deleted_at::text, '') FROM user_saved_cards WHERE id = $1`, cardID).Scan(&deletedAt))
+	require.NotEqual(t, "", deletedAt, "the local soft-delete must proceed even when the Square call fails")
+}
+
+type failingDeleteClient struct {
+	square.SquareClient
+}
+
+func (c *failingDeleteClient) DeleteCardOnFile(ctx context.Context, cardID string) error {
+	return fmt.Errorf("square: network error disabling card %s", cardID)
+}

backend/handlers/payments/payments_review_fixes_test.go

@@ -142,14 +142,15 @@ func TestBuyGiftCard_SaveCard_ProvisionsCustomerForCreateCardOnFile(t *testing.T
 	require.Equal(t, rec.lastCustomerID(), cid, "the stored square_customer_id must match the id passed to CreateCardOnFile")
 }
 
-func TestBuyGiftCard_NoSaveCard_CreateCardOnFileEmptyCustomer(t *testing.T) {
+func TestBuyGiftCard_NoSaveCard_ChargesNonceDirectly(t *testing.T) {
 	ctx, tx := testutils.SetupTestTx(t)
 	userID, err := fixtures.CreateTestUser(tx)
 	require.NoError(t, err)
 	token := jwt.GenerateUserToken(userID)
 
 	origClient := SquareClient
-	rec := &recordingCardOnFileClient{SquareClient: square.NewDevClient()}
+	cof := &recordingCardOnFileClient{SquareClient: square.NewDevClient()}
+	rec := &recordingPaymentClient{SquareClient: cof}
 	SquareClient = rec
 	defer func() { SquareClient = origClient }()
 
@@ -164,8 +165,18 @@ func TestBuyGiftCard_NoSaveCard_CreateCardOnFileEmptyCustomer(t *testing.T) {
 	w := makePaymentRequest(BuyGiftCard, "POST", "/api/gift-cards/buy", req, token, ctx)
 	require.Equal(t, http.StatusCreated, w.Code, w.Body.String())
 
-	require.Equal(t, 1, rec.callCount())
-	require.Equal(t, "", rec.lastCustomerID(), "a one-off non-save charge needs no Square customer")
+	// R6: a one-off non-save charge uses the cnon: nonce DIRECTLY — no
+	// card-on-file is created (nothing to orphan) and no customer is involved.
+	require.Equal(t, 0, cof.callCount(), "a one-off non-save charge must NOT tokenize via CreateCardOnFile")
+	rec.mu.Lock()
+	got := rec.lastReq.SourceID
+	rec.mu.Unlock()
+	require.Equal(t, newToken, got, "the nonce itself must be charged directly")
+	require.Equal(t, "", rec.lastReq.CustomerID, "a cnon: nonce charge carries no customer")
+
+	var cardCount int
+	require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM user_saved_cards WHERE user_id = $1`, userID).Scan(&cardCount))
+	require.Zero(t, cardCount, "a one-off buy must not persist a saved card")
 }
 
 func TestCreateTillSale_VerificationTokenPassthrough(t *testing.T) {
@@ -272,8 +283,8 @@ func TestCreateTillSale_OnlineSquare_NoCustomerProvisioned(t *testing.T) {
 	r.ServeHTTP(w, req)
 
 	require.Equal(t, http.StatusCreated, w.Code, w.Body.String())
-	require.Equal(t, 1, rec.callCount())
-	require.Equal(t, "", rec.lastCustomerID(), "the ephemeral till card is a one-off cnon: charge — no Square customer")
+	require.Equal(t, 0, rec.callCount())
+	require.Equal(t, "", rec.lastCustomerID(), "the ephemeral till card is a one-off cnon: charge — no CreateCardOnFile, no Square customer")
 }
 
 // =============================================================================
@@ -588,12 +599,13 @@ func TestCreateBookingPayment_SaveCard_ProvisionsCustomerForCreateCardOnFile(t *
 	require.Equal(t, rec.lastCustomerID(), cid)
 }
 
-func TestCreateBookingPayment_NoSaveCard_CreateCardOnFileEmptyCustomer(t *testing.T) {
+func TestCreateBookingPayment_NoSaveCard_ChargesNonceDirectly(t *testing.T) {
 	ctx, tx := testutils.SetupTestTx(t)
 	_, bookingID, userToken := setupPaymentStatusTest(t, ctx, tx, "confirmed")
 
 	origClient := SquareClient
-	rec := &recordingCardOnFileClient{SquareClient: square.NewDevClient()}
+	cof := &recordingCardOnFileClient{SquareClient: square.NewDevClient()}
+	rec := &recordingPaymentClient{SquareClient: cof}
 	SquareClient = rec
 	defer func() { SquareClient = origClient }()
 
@@ -609,6 +621,12 @@ func TestCreateBookingPayment_NoSaveCard_CreateCardOnFileEmptyCustomer(t *testin
 	w := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx)
 	require.Equal(t, http.StatusOK, w.Code, w.Body.String())
 
-	require.Equal(t, 1, rec.callCount())
-	require.Equal(t, "", rec.lastCustomerID(), "a one-off non-save charge needs no Square customer")
+	// R6: a one-off non-save charge uses the cnon: nonce DIRECTLY — no
+	// card-on-file is created (nothing to orphan) and no customer is involved.
+	require.Equal(t, 0, cof.callCount(), "a one-off non-save charge must NOT tokenize via CreateCardOnFile")
+	rec.mu.Lock()
+	got := rec.lastReq.SourceID
+	rec.mu.Unlock()
+	require.Equal(t, cardToken, got, "the nonce itself must be charged directly")
+	require.Equal(t, "", rec.lastReq.CustomerID, "a cnon: nonce charge carries no customer")
 }

backend/handlers/payments/payments_test.go

@@ -2023,6 +2023,52 @@ func TestBuildSplitRecords_PaymentLessThanDepositMax_NoSplit(t *testing.T) {
 	}
 }
 
+func TestBuildSplitRecords_OverflowBeyondTotal_BecomesTip(t *testing.T) {
+	// £60 on a £50 future booking: deposit caps at £25 (50%), balance covers
+	// the remaining £25 owed, and the £10 overflow beyond the booking total
+	// becomes a tip record.
+	record := makeTestRecord("b5", "full", 60)
+	info := &BookingPaymentInfo{
+		StartTime:   clock.Now().Add(48 * time.Hour),
+		TotalAmount: 50,
+		TotalPaid:   0,
+	}
+	records := buildSplitRecords(record, "full", info, 60)
+
+	if len(records) != 3 {
+		t.Fatalf("expected 3 records (deposit + balance + tip), got %d", len(records))
+	}
+	if records[0].PaymentType != "deposit" {
+		t.Errorf("expected first record 'deposit', got %q", records[0].PaymentType)
+	}
+	if records[0].Amount != 25 {
+		t.Errorf("expected deposit amount 25, got %.2f", records[0].Amount)
+	}
+	if records[1].PaymentType != "balance" {
+		t.Errorf("expected second record 'balance', got %q", records[1].PaymentType)
+	}
+	if records[1].Amount != 25 {
+		t.Errorf("expected balance amount 25, got %.2f", records[1].Amount)
+	}
+	if records[2].PaymentType != "tip" {
+		t.Errorf("expected third record 'tip', got %q", records[2].PaymentType)
+	}
+	if records[2].Amount != 10 {
+		t.Errorf("expected tip amount 10 (overflow beyond the booking total), got %.2f", records[2].Amount)
+	}
+	// The tip is a split: zero fees and a derived idempotency key.
+	if records[2].Fees != 0 {
+		t.Errorf("expected tip fees=0, got %.2f", records[2].Fees)
+	}
+	if *records[2].IdempotencyKey != *records[0].IdempotencyKey+"-split-3" {
+		t.Errorf("expected tip key derived from primary, got %q", *records[2].IdempotencyKey)
+	}
+	// The splits share the Square payment id of the primary record.
+	if *records[2].SquarePaymentID != *records[0].SquarePaymentID {
+		t.Error("tip split must share square_payment_id with the primary record")
+	}
+}
+
 // ---------------------------------------------------------------------------
 // Handler-level atomicity — verify the full handler succeeds with split.
 // ---------------------------------------------------------------------------
@@ -3405,7 +3451,7 @@ func TestSaveCardForUser_Success(t *testing.T) {
 		t.Fatalf("failed to create user: %v", err)
 	}
 	svc := NewPaymentService()
-	cardID, err := svc.SaveCardForUser(ctx, userID, "cfa_test_success", "VISA", "4242", 12, 2030, "fp_success")
+	cardID, err := svc.SaveCardForUser(ctx, userID, "cus_test_success", "cfa_test_success", "VISA", "4242", 12, 2030, "fp_success")
 	if err != nil {
 		t.Errorf("unexpected error: %v", err)
 	}
@@ -3418,7 +3464,7 @@ func TestSaveCardForUser_InvalidUserID(t *testing.T) {
 	ctx, tx := testutils.SetupTestTx(t)
 	_ = tx
 	svc := NewPaymentService()
-	_, err := svc.SaveCardForUser(ctx, "000000000001", "cfa_test", "VISA", "4242", 12, 2030, "fp_test")
+	_, err := svc.SaveCardForUser(ctx, "000000000001", "cus_test_invalid", "cfa_test", "VISA", "4242", 12, 2030, "fp_test")
 	if err == nil {
 		t.Error("expected error for non-existent user ID (FK violation)")
 	}
@@ -3584,8 +3630,6 @@ func TestTerminalPayment_ValidatePaymentTypeFails(t *testing.T) {
 	}
 }
 
-
-
 func TestSweepStalePendingPayments_MarksOldFailed(t *testing.T) {
 	t.Parallel()
 	ctx, tx := testutils.SetupTestTx(t)
@@ -3728,7 +3772,6 @@ func TestSavedCardPayment_SweptFailed_Rejected(t *testing.T) {
 	}
 }
 
-
 // TestSweepStalePendingPayments_CoversTillSales verifies the R3 fix: the sweep
 // also marks stale pending till_sales rows (card payments) as failed, so a
 // lost-response till sale can't stay pending past Square's key retention.
@@ -3782,3 +3825,41 @@ func TestSweepStalePendingPayments_CoversTillSales(t *testing.T) {
 		t.Errorf("expected fresh pending till sale to stay pending, got %q", freshStatus)
 	}
 }
+
+func TestBuildSplitRecords_TipOverflow_SeparateTipRecord(t *testing.T) {
+	// A £60 payment on a £50 future booking with nothing paid yet:
+	// deposit = min(60, 25) = £25, balance = min(35, 25) = £25,
+	// tip = 60 - 25 - 25 = £10 — the overflow becomes a separate tip record.
+	record := makeTestRecord("b-overflow", "full", 60)
+	info := &BookingPaymentInfo{
+		StartTime:   clock.Now().Add(48 * time.Hour),
+		TotalAmount: 50,
+		TotalPaid:   0,
+	}
+	records := buildSplitRecords(record, "full", info, 60)
+
+	if len(records) != 3 {
+		t.Fatalf("expected 3 records (deposit + balance + tip), got %d", len(records))
+	}
+	if records[0].PaymentType != "deposit" || records[0].Amount != 25 {
+		t.Errorf("expected deposit 25, got %q %.2f", records[0].PaymentType, records[0].Amount)
+	}
+	if records[1].PaymentType != "balance" || records[1].Amount != 25 {
+		t.Errorf("expected balance 25, got %q %.2f", records[1].PaymentType, records[1].Amount)
+	}
+	if records[2].PaymentType != "tip" || records[2].Amount != 10 {
+		t.Errorf("expected tip record 10, got %q %.2f", records[2].PaymentType, records[2].Amount)
+	}
+	// Tip is an overflow split — zero fees, derived idempotency key -split-2.
+	if records[2].Fees != 0 {
+		t.Errorf("expected tip record fees=0, got %.2f", records[2].Fees)
+	}
+	wantKey := *record.IdempotencyKey + "-split-3"
+	if *records[2].IdempotencyKey != wantKey {
+		t.Errorf("expected tip key %q, got %q", wantKey, *records[2].IdempotencyKey)
+	}
+	// All three share the same SquarePaymentID (one charge, three ledger rows).
+	if *records[2].SquarePaymentID != *record.SquarePaymentID {
+		t.Error("tip split must share square_payment_id")
+	}
+}

backend/handlers/payments/validators_test.go

@@ -0,0 +1,35 @@
+//go:build test && dev
+
+package payments
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// TestValidateAmount covers the amount validator used by every payment
+// handler: positive and within the £10,000 (1,000,000 pence) cap. Amounts are
+// integer pence, so sub-penny "precision" is impossible by construction.
+func TestValidateAmount(t *testing.T) {
+	t.Parallel()
+
+	valid := []int64{1, 500, 10000, 999999, 1000000}
+	for _, amount := range valid {
+		require.NoErrorf(t, ValidateAmount(amount), "expected %d to be a valid amount", amount)
+	}
+
+	invalid := []int64{0, -1, -500, 1000001}
+	for _, amount := range invalid {
+		require.Errorf(t, ValidateAmount(amount), "expected %d to be rejected", amount)
+	}
+
+	// The cap is exclusive: exactly £10,000 (1,000,000 pence) is allowed, one
+	// penny more is rejected.
+	if err := ValidateAmount(1000001); err == nil {
+		t.Error("expected amount above £10,000 cap to be rejected")
+	}
+	if err := ValidateAmount(1000000); err != nil {
+		t.Errorf("expected exactly £10,000 to be allowed, got %v", err)
+	}
+}