diff --git a/backend/handlers/payments/till.go b/backend/handlers/payments/till.go index 67c5abf..88f6dce 100644 --- a/backend/handlers/payments/till.go +++ b/backend/handlers/payments/till.go @@ -370,6 +370,42 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { } } + // Defense-in-depth fallback: resolve a pending TOP-UP by the gift card + // itself, not just the idempotency key. A lost-response retry carrying a + // fresh/absent key (an old frontend, or a key regenerated for a changed + // cart) must still reuse the pending till_sale row and adopt its STORED + // idempotency key — otherwise the retry would issue a second Square charge + // and fund the card twice. Only the row's own key may be charged: adopting + // it makes Square dedup the retry against the original charge. An amount + // mismatch proves this is a genuinely different sale (same card, new + // amount), so the pending row is left untouched and the request proceeds + // as a new charge below. 'create' cannot be resolved this way (the request + // carries no gift-card id — the card is created by the first attempt), so + // create retries rely on the client-supplied key, which the till frontend + // caches per cart line. + if existingPendingID == "" && req.Action == "topup" && req.GiftCardID != nil && *req.GiftCardID != "" { + cardID := validators.NormalizeGiftCardCode(*req.GiftCardID) + var existingID, existingItemID, existingKey string + var existingTotal float64 + err := db.Conn.QueryRow(ctx, ` + SELECT id, item_id, total_amount, idempotency_key + FROM till_sales + WHERE item_id = $1 AND status = 'pending' + ORDER BY created_at DESC + LIMIT 1 + `, cardID).Scan(&existingID, &existingItemID, &existingTotal, &existingKey) + if err == nil && existingKey != "" { + if int64(math.Round(existingTotal*100)) == int64(math.Round(req.Amount*100)) { + log.Printf("Till-sale retry resolved by gift card %s: reusing pending sale %s with stored idempotency key", cardID, existingID) + existingPendingID = existingID + existingPendingGiftCard = existingItemID + req.IdempotencyKey = existingKey + } else { + log.Printf("Till-sale retry by gift card %s skipped: pending sale %s has %.2f, request has %.2f — treated as a new sale", cardID, existingID, existingTotal, req.Amount) + } + } + } + service := NewPaymentService() // squarePaymentID/squareCheckoutID are set inside the payment-method switch diff --git a/backend/handlers/payments/till_test.go b/backend/handlers/payments/till_test.go index 15c6790..f547301 100644 --- a/backend/handlers/payments/till_test.go +++ b/backend/handlers/payments/till_test.go @@ -10,6 +10,7 @@ import ( "fmt" "net/http" "net/http/httptest" + "sync" "testing" "time" @@ -2219,3 +2220,341 @@ func TestGetTillCheckoutStatus_AlreadyFailed_DoesNotResurrect(t *testing.T) { t.Errorf("expected failed sale to remain failed after the guarded poll, got %q", status) } } + +// countingPaymentClient wraps the Square client and records every CreatePayment +// invocation (its idempotency key and the returned payment) so tests can prove +// a retry issued exactly ONE charge (Square dedup returns the original payment) +// and that the charge reused the expected idempotency key. +type countingPaymentClient struct { + square.SquareClient + mu sync.Mutex + keys []string + payments []*square.PaymentResult +} + +func (c *countingPaymentClient) CreatePayment(ctx context.Context, req square.CreatePaymentReq) (*square.PaymentResult, error) { + res, err := c.SquareClient.CreatePayment(ctx, req) + c.mu.Lock() + defer c.mu.Unlock() + c.keys = append(c.keys, req.IdempotencyKey) + if err == nil { + c.payments = append(c.payments, res) + } + return res, err +} + +func (c *countingPaymentClient) callCount() int { + c.mu.Lock() + defer c.mu.Unlock() + return len(c.keys) +} + +// makeTillSaleRequest POSTs a till sale through the real router with the test +// transaction embedded in the request context. +func makeTillSaleRequest(t *testing.T, req TillSaleRequest, adminToken string, ctx context.Context, tx pgx.Tx) *httptest.ResponseRecorder { + t.Helper() + bodyBytes, _ := json.Marshal(req) + r := httptest.NewRequest("POST", "/api/admin/till/sale", bytes.NewReader(bodyBytes)) + r.Header.Set("Authorization", "Bearer "+adminToken) + r.Header.Set("Content-Type", "application/json") + r = r.WithContext(db.ContextWithTx(r.Context(), tx)) + w := httptest.NewRecorder() + router := chi.NewRouter() + router.Use(mw.RequireAuth) + router.Post("/api/admin/till/sale", CreateTillSale) + router.ServeHTTP(w, r) + return w +} + +// TestCreateTillSale_PendingRetry_LostResponse_SingleSquareCharge simulates +// the P0 double-charge window: a charge lands at Square but the response is +// lost, leaving the till_sale row 'pending'. A same-key retry must re-attempt +// the charge with the SAME idempotency key — Square dedups on the key and +// returns the ORIGINAL payment — so the customer is charged exactly once, the +// same row is reused, and the gift card is funded exactly once. +func TestCreateTillSale_PendingRetry_LostResponse_SingleSquareCharge(t *testing.T) { + t.Parallel() + 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") + + origClient := SquareClient + rec := &countingPaymentClient{SquareClient: square.NewDevClient()} + SquareClient = rec + defer func() { SquareClient = origClient }() + + key := "till-lost-response-single-charge" + reqBody := TillSaleRequest{ + ItemType: "gift_card", + Action: "create", + Amount: 50.00, + PaymentMethod: "online_square", + CardToken: "cnon:lost-response-card", + IdempotencyKey: key, + } + + // First attempt: the charge succeeds at Square and the sale completes. + first := makeTillSaleRequest(t, reqBody, adminToken, ctx, tx.(pgx.Tx)) + if first.Code != http.StatusCreated { + t.Fatalf("first attempt: expected 201, got %d. body: %s", first.Code, first.Body.String()) + } + var firstResp TillSaleResponse + if err := json.NewDecoder(first.Body).Decode(&firstResp); err != nil { + t.Fatalf("failed to decode first response: %v", err) + } + + // Lost-response window: the charge keyed on `key` is live at Square (the + // mock keeps it in paymentByKey) but the response never reached the + // handler, so the sale row is stuck 'pending' with no payment id recorded. + if _, err := tx.Exec(ctx, `UPDATE till_sales SET status = 'pending', square_payment_id = NULL WHERE id = $1`, firstResp.ID); err != nil { + t.Fatalf("failed to simulate the lost response: %v", err) + } + + // Retry with the same key: the pending-reuse path re-charges with the SAME + // key, Square dedups to the original payment, and the same row completes. + second := makeTillSaleRequest(t, reqBody, adminToken, ctx, tx.(pgx.Tx)) + if second.Code != http.StatusOK && second.Code != http.StatusCreated { + t.Fatalf("retry: expected 200/201, got %d. body: %s", second.Code, second.Body.String()) + } + var secondResp TillSaleResponse + if err := json.NewDecoder(second.Body).Decode(&secondResp); err != nil { + t.Fatalf("failed to decode retry response: %v", err) + } + + // Exactly ONE till_sales row — the retry reused the pending row. + if secondResp.ID != firstResp.ID { + t.Errorf("expected the SAME till_sale row on retry, got %s then %s", firstResp.ID, secondResp.ID) + } + var saleCount int + if err := tx.QueryRow(ctx, `SELECT COUNT(*) FROM till_sales WHERE id = $1`, firstResp.ID).Scan(&saleCount); err != nil { + t.Fatalf("failed to count till_sales: %v", err) + } + if saleCount != 1 { + t.Errorf("expected 1 till_sale row (reused), got %d", saleCount) + } + + // Square was invoked twice (attempt + retry) but the retry deduped on the + // key: both invocations returned the SAME payment — one charge in total. + if rec.callCount() != 2 { + t.Errorf("expected 2 CreatePayment invocations (attempt + dedup retry), got %d", rec.callCount()) + } + rec.mu.Lock() + oneCharge := len(rec.payments) == 2 && rec.payments[0].SquarePayID == rec.payments[1].SquarePayID + rec.mu.Unlock() + if !oneCharge { + t.Errorf("expected the retry to dedup to the original payment (single charge), got %d distinct payments", len(rec.payments)) + } + + // Exactly one funded gift card for this sale, still at the single value. + var giftCardCount int + if err := tx.QueryRow(ctx, `SELECT COUNT(*) FROM gift_cards WHERE total_funds_added = 50.00`).Scan(&giftCardCount); err != nil { + t.Fatalf("failed to count gift cards: %v", err) + } + if giftCardCount != 1 { + t.Errorf("expected 1 funded gift card (single funding), got %d", giftCardCount) + } + + // The reused row must be completed with the payment id recorded. + var saleStatus, sqPaymentID string + if err := tx.QueryRow(ctx, `SELECT status, COALESCE(square_payment_id, '') FROM till_sales WHERE id = $1`, firstResp.ID).Scan(&saleStatus, &sqPaymentID); err != nil { + t.Fatalf("failed to read sale status: %v", err) + } + if saleStatus != "completed" { + t.Errorf("expected sale to be completed after the retry, got %s", saleStatus) + } + if sqPaymentID == "" { + t.Error("expected square_payment_id to be recorded after the retry") + } +} + +// TestCreateTillSale_PendingRetry_FreshKey_ReusesPendingRowViaGiftCard proves +// the backend defense-in-depth: even when a retry arrives with a FRESH +// idempotency key (an old frontend, or a regenerated key), a same-amount +// top-up on the SAME gift card reuses the pending till_sale row and adopts its +// STORED idempotency key — the Square charge dedups on the stored key instead +// of charging the customer twice. +func TestCreateTillSale_PendingRetry_FreshKey_ReusesPendingRowViaGiftCard(t *testing.T) { + t.Parallel() + 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") + + // Seed a funded gift card with a pending top-up till_sale (the first + // attempt's Square charge failed after the DB commit — card already funded). + var giftCardID string + if err := tx.QueryRow(ctx, ` + INSERT INTO gift_cards (total_funds_added, amount_remaining, created_by, is_inventory, voucher_type_at_purchase) + VALUES (50.00, 50.00, $1, FALSE, 'SPV') + RETURNING id + `, adminID).Scan(&giftCardID); err != nil { + t.Fatalf("failed to create gift card: %v", err) + } + storedKey := "till-original-topup-key" + if _, err := tx.Exec(ctx, ` + INSERT INTO till_sales (item_type, item_id, description, quantity, unit_price, total_amount, + payment_method, status, user_id, user_saved_card_id, idempotency_key, created_by, created_at, updated_at) + VALUES ('gift_card', $1, 'Gift Card topup', 1, 50.00, 50.00, 'online_square', 'pending', + NULL, NULL, $2, $3, NOW(), NOW()) + `, giftCardID, storedKey, adminID); err != nil { + t.Fatalf("failed to seed pending top-up: %v", err) + } + + origClient := SquareClient + rec := &countingPaymentClient{SquareClient: square.NewDevClient()} + SquareClient = rec + defer func() { SquareClient = origClient }() + + // Retry with a GENUINELY FRESH key: same gift card, same amount, but a key + // the backend has never seen (simulating an old/regenerated key). + reqBody := TillSaleRequest{ + ItemType: "gift_card", + Action: "topup", + Amount: 50.00, + GiftCardID: &giftCardID, + PaymentMethod: "online_square", + CardToken: "cnon:fresh-key-topup-card", + IdempotencyKey: "till-fresh-key-topup", + } + w := makeTillSaleRequest(t, reqBody, adminToken, ctx, tx.(pgx.Tx)) + if w.Code != http.StatusOK && w.Code != http.StatusCreated { + t.Fatalf("expected 200/201, got %d. body: %s", w.Code, w.Body.String()) + } + + // The pending row was reused, not duplicated, and keeps its stored key. + var saleCount int + var saleStatus, usedKey string + if err := tx.QueryRow(ctx, ` + SELECT COUNT(*), MAX(status), MAX(idempotency_key) FROM till_sales WHERE item_id = $1 + `, giftCardID).Scan(&saleCount, &saleStatus, &usedKey); err != nil { + t.Fatalf("failed to query till_sales: %v", err) + } + if saleCount != 1 { + t.Errorf("expected 1 till_sale row (reused), got %d", saleCount) + } + if saleStatus != "completed" { + t.Errorf("expected the reused row to complete, got %s", saleStatus) + } + if usedKey != storedKey { + t.Errorf("expected the sale row to keep its stored key %q, got %q", storedKey, usedKey) + } + + // The Square charge reused the STORED key (the dedup target), not the + // fresh request key — a second charge on the fresh key would double-charge. + rec.mu.Lock() + gotKey := rec.keys[0] + rec.mu.Unlock() + if gotKey != storedKey { + t.Errorf("expected Square charge to use the stored key %q, got %q", storedKey, gotKey) + } + + // The gift card was funded exactly once — a second charge would have funded + // it again. + var remaining float64 + if err := tx.QueryRow(ctx, `SELECT amount_remaining FROM gift_cards WHERE id = $1`, giftCardID).Scan(&remaining); err != nil { + t.Fatalf("failed to read gift card: %v", err) + } + if remaining != 50.00 { + t.Errorf("expected gift card funded once (£50.00), got £%.2f (double-funding!)", remaining) + } +} + +// TestCreateTillSale_PendingRetry_FreshKey_DifferentAmount_NewCharge proves a +// retry with a genuinely different amount on the same gift card is a NEW sale: +// the pending row is left untouched and a new Square charge is issued (the +// amount mismatch is the only signal distinguishing a retry from a fresh +// top-up). +func TestCreateTillSale_PendingRetry_FreshKey_DifferentAmount_NewCharge(t *testing.T) { + t.Parallel() + 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") + + var giftCardID string + if err := tx.QueryRow(ctx, ` + INSERT INTO gift_cards (total_funds_added, amount_remaining, created_by, is_inventory, voucher_type_at_purchase) + VALUES (50.00, 50.00, $1, FALSE, 'SPV') + RETURNING id + `, adminID).Scan(&giftCardID); err != nil { + t.Fatalf("failed to create gift card: %v", err) + } + if _, err := tx.Exec(ctx, ` + INSERT INTO till_sales (item_type, item_id, description, quantity, unit_price, total_amount, + payment_method, status, user_id, user_saved_card_id, idempotency_key, created_by, created_at, updated_at) + VALUES ('gift_card', $1, 'Gift Card topup', 1, 50.00, 50.00, 'online_square', 'pending', + NULL, NULL, 'till-original-topup-key-2', $2, NOW(), NOW()) + `, giftCardID, adminID); err != nil { + t.Fatalf("failed to seed pending top-up: %v", err) + } + + origClient := SquareClient + rec := &countingPaymentClient{SquareClient: square.NewDevClient()} + SquareClient = rec + defer func() { SquareClient = origClient }() + + // Different amount on the same card with a fresh key: a genuinely + // different sale — a new charge is allowed. + reqBody := TillSaleRequest{ + ItemType: "gift_card", + Action: "topup", + Amount: 25.00, + GiftCardID: &giftCardID, + PaymentMethod: "online_square", + CardToken: "cnon:new-amount-topup-card", + IdempotencyKey: "till-fresh-key-topup-25", + } + w := makeTillSaleRequest(t, reqBody, adminToken, ctx, tx.(pgx.Tx)) + if w.Code != http.StatusOK && w.Code != http.StatusCreated { + t.Fatalf("expected 200/201 for the new sale, got %d. body: %s", w.Code, w.Body.String()) + } + + // Two till_sales rows now: the untouched pending one and the new completed + // one. + var saleCount int + if err := tx.QueryRow(ctx, `SELECT COUNT(*) FROM till_sales WHERE item_id = $1`, giftCardID).Scan(&saleCount); err != nil { + t.Fatalf("failed to count till_sales: %v", err) + } + if saleCount != 2 { + t.Errorf("expected 2 till_sale rows (pending original + new sale), got %d", saleCount) + } + + // The new sale charged Square exactly once, with its own fresh key. + if rec.callCount() != 1 { + t.Errorf("expected exactly 1 new Square charge, got %d", rec.callCount()) + } + rec.mu.Lock() + gotKey := rec.keys[0] + rec.mu.Unlock() + if gotKey != "till-fresh-key-topup-25" { + t.Errorf("expected the new charge to use the request's fresh key, got %q", gotKey) + } + + // The card now holds £50 (original pending funding) + £25 (new top-up). + var totalAdded float64 + if err := tx.QueryRow(ctx, `SELECT total_funds_added FROM gift_cards WHERE id = $1`, giftCardID).Scan(&totalAdded); err != nil { + t.Fatalf("failed to read gift card: %v", err) + } + if totalAdded != 75.00 { + t.Errorf("expected gift card total funded to £75.00, got £%.2f", totalAdded) + } + + // The original pending row is untouched. + var origStatus string + if err := tx.QueryRow(ctx, `SELECT status FROM till_sales WHERE idempotency_key = 'till-original-topup-key-2'`).Scan(&origStatus); err != nil { + t.Fatalf("failed to read original row: %v", err) + } + if origStatus != "pending" { + t.Errorf("expected the original pending row to be untouched, got %s", origStatus) + } +}