diff --git a/backend/handlers/payments/giftcards.go b/backend/handlers/payments/giftcards.go index 1ce6d40..195e3cb 100644 --- a/backend/handlers/payments/giftcards.go +++ b/backend/handlers/payments/giftcards.go @@ -1183,8 +1183,10 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { } recipient = userEmail } - // TODO: send gift card code via email to recipient once SMTP is wired - log.Printf("GIFT CARD FOR FRIEND — code: %s, value: £%.2f, intended for: %s", cardID, amountPounds, recipient) + // TODO: send gift card code via email to recipient once SMTP is wired. + // Do NOT log the spendable gift-card code — it is a credential (12-digit + // code anyone can redeem). Log only the value and recipient for audit. + log.Printf("Gift card purchased for friend — value: £%.2f, intended for: %s (code stored in DB, not logged)", amountPounds, recipient) _, err = db.Conn.Exec(ctx, ` INSERT INTO gift_card_transactions (gift_card_id, transaction_type, amount, reference_type, reference_id, user_id, notes) diff --git a/backend/handlers/payments/handlers.go b/backend/handlers/payments/handlers.go index 8ff3c8c..c022ecd 100644 --- a/backend/handlers/payments/handlers.go +++ b/backend/handlers/payments/handlers.go @@ -355,7 +355,14 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { amount = *req.OverrideAmount } - idempotencyKey := bookingID + "-" + req.PaymentType + "-" + strconv.FormatInt(amount, 10) + // Idempotency key for the payment. Cash/giftcard terminal payments are + // always fresh admin actions (not network-retryable), and the request has + // no client key — so a deterministic booking+type+amount key would wrongly + // dedup two legitimate identical payments (e.g. two £50 cash receipts on + // one booking). Use a unique key per payment: retries of a lost response + // are handled by the Square-side key for card payments, and cash/giftcard + // are DB-committed synchronously. + idempotencyKey := uniqueTipKey() // Route based on payment method if req.PaymentMethod != nil && (*req.PaymentMethod == "cash" || *req.PaymentMethod == "giftcard") { @@ -412,10 +419,10 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { if *req.PaymentMethod == "cash" { err = tx.QueryRow(r.Context(), ` INSERT INTO payments ( - booking_id, payment_type, payment_method, status, amount, created_by, created_at, updated_at - ) VALUES ($1, $2, 'cash', 'completed', $3, $4, NOW(), NOW()) + booking_id, payment_type, payment_method, status, amount, idempotency_key, created_by, created_at, updated_at + ) VALUES ($1, $2, 'cash', 'completed', $3, $4, $5, NOW(), NOW()) RETURNING id - `, bookingID, req.PaymentType, amountPounds, adminID).Scan(&paymentID) + `, bookingID, req.PaymentType, amountPounds, idempotencyKey, adminID).Scan(&paymentID) if err != nil { log.Printf("Failed to create cash payment record: %v", err) http.Error(w, "internal server error", http.StatusInternalServerError) @@ -507,10 +514,10 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { err = tx.QueryRow(r.Context(), ` INSERT INTO payments ( - booking_id, payment_type, payment_method, status, amount, created_by, created_at, updated_at - ) VALUES ($1, $2, 'giftcard', 'completed', $3, $4, NOW(), NOW()) + booking_id, payment_type, payment_method, status, amount, idempotency_key, created_by, created_at, updated_at + ) VALUES ($1, $2, 'giftcard', 'completed', $3, $4, $5, NOW(), NOW()) RETURNING id - `, bookingID, req.PaymentType, amountPounds, adminID).Scan(&paymentID) + `, bookingID, req.PaymentType, amountPounds, idempotencyKey, adminID).Scan(&paymentID) if err != nil { log.Printf("Failed to create giftcard payment record: %v", err) http.Error(w, "internal server error", http.StatusInternalServerError) @@ -1555,9 +1562,7 @@ func DeletePaymentMethod(w http.ResponseWriter, r *http.Request) { } type CreatePaymentMethodRequest struct { - CardNumber string `json:"card_number" validate:"required"` - Expiry string `json:"expiry" validate:"required"` - CVC string `json:"cvc" validate:"required"` + CardToken string `json:"card_token" validate:"required"` } func CreatePaymentMethod(w http.ResponseWriter, r *http.Request) { @@ -1579,16 +1584,13 @@ func CreatePaymentMethod(w http.ResponseWriter, r *http.Request) { return } - // M8 - // L5 - - if req.CardNumber == "" || req.Expiry == "" || req.CVC == "" { - http.Error(w, "Card number, expiry, and CVC are required", http.StatusBadRequest) + if req.CardToken == "" { + http.Error(w, "card_token is required — use a Square Web Payments nonce", http.StatusBadRequest) return } service := NewPaymentService() - card, err := service.CreatePaymentMethodFromDetails(r.Context(), userID, req.CardNumber, req.Expiry, req.CVC) + card, err := service.CreatePaymentMethodFromToken(r.Context(), userID, req.CardToken) if err != nil { if strings.Contains(err.Error(), "invalid") || strings.Contains(err.Error(), "expired") { log.Printf("Failed to process request: %v", err) @@ -1667,15 +1669,63 @@ func RefundPayment(w http.ResponseWriter, r *http.Request) { return } - if req.Amount+alreadyRefunded > int64(payment.Amount*100) { + if req.Amount+alreadyRefunded > int64(math.Round(payment.Amount*100)) { http.Error(w, "Refund amount exceeds payment amount", http.StatusBadRequest) return } - // Begin a transaction so that the Square refund and the DB record are - // atomically linked. If the commit fails after Square processes the refund, - // a CRITICAL log alerts monitoring — the Square refund cannot be reversed, - // but the DB record can be recreated from the log. + // Serialize refund attempts per payment to prevent two concurrent refunds + // both passing the over-refund guard and both charging Square. Mirrors the + // tip/gift-card advisory-lock pattern. + refundLockKey := paymentID + pinConn, err := db.Conn.Acquire(r.Context()) + if err != nil { + log.Printf("Failed to acquire connection for refund lock: %v", err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + defer pinConn.Release() + if _, err := pinConn.Exec(r.Context(), ` + SELECT pg_advisory_lock(hashtext('crussell:refund:' || $1)) + `, refundLockKey); err != nil { + log.Printf("Failed to acquire refund serialization lock for %s: %v", paymentID, err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + defer func() { + if _, err := pinConn.Exec(context.Background(), ` + SELECT pg_advisory_unlock(hashtext('crussell:refund:' || $1)) + `, refundLockKey); err != nil { + log.Printf("Failed to release refund serialization lock for %s: %v", paymentID, err) + } + }() + + // Deterministic idempotency key so a same-key retry (network timeout) + // does not create a second Square refund. + idempotencyKey := paymentID + "-refund-" + strconv.FormatInt(req.Amount, 10) + + // Check for an existing refund with this key — dedup completed refunds. + var existingRefundStatus sql.NullString + err = db.Conn.QueryRow(r.Context(), `SELECT status FROM refunds WHERE idempotency_key = $1`, idempotencyKey).Scan(&existingRefundStatus) + if err == nil && existingRefundStatus.String == "completed" { + if err := json.NewEncoder(w).Encode(RefundResponse{ + PaymentID: paymentID, + Amount: req.Amount, + Status: "completed", + Reason: req.Reason, + CreatedAt: clock.Now().Format(time.RFC3339), + }); err != nil { + log.Printf("Failed to encode JSON response: %v", err) + } + return + } + if err != nil && !errors.Is(err, pgx.ErrNoRows) { + log.Printf("Failed to check refund idempotency: %v", err) + } + + // Begin a transaction. Insert the refund record as 'pending' first, commit, + // then call Square — so a Square failure leaves a retryable pending refund + // (reprocessed by the scheduler in refunds.go). tx, err := db.Conn.Begin(r.Context()) if err != nil { log.Printf("Failed to begin transaction for refund: %v", err) @@ -1688,44 +1738,56 @@ func RefundPayment(w http.ResponseWriter, r *http.Request) { } }() + var refundID string + err = tx.QueryRow(r.Context(), ` + INSERT INTO refunds (payment_id, booking_id, amount, status, reason, idempotency_key, created_by, created_at) + VALUES ($1, $2, $3, 'pending', $4, $5, $6, $7) + RETURNING id + `, + paymentID, + payment.BookingID, + float64(req.Amount)/100.0, + req.Reason, + idempotencyKey, + adminID, + clock.Now(), + ).Scan(&refundID) + if err != nil { + log.Printf("Failed to create pending refund record: %v", err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + + if err := tx.Commit(r.Context()); err != nil { + log.Printf("Failed to commit refund transaction: %v", err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + refundReq := square.RefundPaymentReq{ PaymentID: *payment.SquarePaymentID, Amount: req.Amount, - IdempotencyKey: paymentID + "-" + strconv.FormatInt(req.Amount, 10), + IdempotencyKey: idempotencyKey, Reason: req.Reason, } refundResult, err := SquareClient.RefundPayment(r.Context(), refundReq) if err != nil { - log.Printf("Failed to refund payment: %v", err) + // Refund record intentionally left as 'pending' for the scheduler to + // re-attempt (refunds.go ProcessPendingSquareRefunds). + log.Printf("Failed to refund payment (refund %s left pending): %v", refundID, err) http.Error(w, "Refund failed", http.StatusInternalServerError) return } squareRefundID := refundResult.ID - var refundID string - err = tx.QueryRow(r.Context(), ` - INSERT INTO refunds (payment_id, booking_id, amount, square_refund_id, status, reason, created_by, created_at) - VALUES ($1, $2, $3, $4, 'completed', $5, $6, $7) - RETURNING id - `, - paymentID, - payment.BookingID, - float64(req.Amount)/100.0, - squareRefundID, - req.Reason, - adminID, - clock.Now(), - ).Scan(&refundID) - if err != nil { - log.Printf("Failed to create refund record: %v", err) - http.Error(w, "internal server error", http.StatusInternalServerError) - return - } - - if err := tx.Commit(r.Context()); err != nil { - log.Printf("CRITICAL: Refund committed by Square (%s) but DB transaction failed — refund record %s may be missing: %v", squareRefundID, refundID, err) + // Square succeeded — update the refund record to completed. + if _, upErr := db.Conn.Exec(r.Context(), + `UPDATE refunds SET status = 'completed', square_refund_id = $1 WHERE id = $2`, + squareRefundID, refundID, + ); upErr != nil { + log.Printf("CRITICAL: Square refund committed (%s) but DB update for refund %s failed — manual reconciliation required: %v", squareRefundID, refundID, upErr) http.Error(w, "internal server error", http.StatusInternalServerError) return } @@ -2089,7 +2151,7 @@ func GetBookingPaymentSummary(w http.ResponseWriter, r *http.Request) { BookingID: p.BookingID, PaymentType: p.PaymentType, Status: p.Status, - Amount: int64(p.Amount * 100), + Amount: int64(math.Round(p.Amount * 100)), CardLast4: p.CardLast4, CreatedAt: p.CreatedAt.Format(time.RFC3339), } @@ -2100,7 +2162,7 @@ func GetBookingPaymentSummary(w http.ResponseWriter, r *http.Request) { refunds[i] = RefundResponse{ ID: rf.ID, PaymentID: rf.PaymentID, - Amount: int64(rf.Amount * 100), + Amount: int64(math.Round(rf.Amount * 100)), Status: rf.Status, Reason: rf.Reason, CreatedAt: rf.CreatedAt.Format(time.RFC3339), @@ -2108,12 +2170,12 @@ func GetBookingPaymentSummary(w http.ResponseWriter, r *http.Request) { } if err := json.NewEncoder(w).Encode(PaymentSummaryResponse{ - TotalAmount: int64(summary.TotalAmount * 100), - PaidAmount: int64(summary.PaidAmount * 100), - RefundedAmount: int64(summary.RefundedAmount * 100), - RemainingAmount: int64(summary.RemainingAmount * 100), - TotalVATAmount: int64(summary.TotalVATAmount * 100), - TotalNetAmount: int64(summary.TotalNetAmount * 100), + TotalAmount: int64(math.Round(summary.TotalAmount * 100)), + PaidAmount: int64(math.Round(summary.PaidAmount * 100)), + RefundedAmount: int64(math.Round(summary.RefundedAmount * 100)), + RemainingAmount: int64(math.Round(summary.RemainingAmount * 100)), + TotalVATAmount: int64(math.Round(summary.TotalVATAmount * 100)), + TotalNetAmount: int64(math.Round(summary.TotalNetAmount * 100)), Payments: payments, Refunds: refunds, }); err != nil { diff --git a/backend/handlers/payments/payments_test.go b/backend/handlers/payments/payments_test.go index 0c663d5..e8f4da1 100644 --- a/backend/handlers/payments/payments_test.go +++ b/backend/handlers/payments/payments_test.go @@ -595,6 +595,60 @@ func TestRefund_FullRefund(t *testing.T) { } } +func TestRefund_SameKeyRetry_Dedups(t *testing.T) { + // A retry of the same refund (network timeout, double-click) must not + // create a second Square refund or a second DB row — the idempotency key + // (paymentID + amount) dedups it. + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + _, bookingID, _ := setupTestData(t, ctx, tx) + + adminID, err := fixtures.CreateTestAdminUser(tx) + if err != nil { + t.Fatalf("failed to create admin user: %v", err) + } + + adminToken := jwt.GenerateTestToken(adminID, "admin") + + paymentID, err := fixtures.CreateTestPayment(tx, bookingID, 5000.00, "in_person_card", "full", "completed") + if err != nil { + t.Fatalf("failed to create payment: %v", err) + } + _, err = tx.Exec(ctx, "UPDATE payments SET square_payment_id = 'sqp_test_dedup' WHERE id = $1", paymentID) + if err != nil { + t.Fatalf("failed to update payment: %v", err) + } + + req := RefundRequest{ + Amount: 5000, + Reason: "customer request", + } + + handler := RefundPayment + // First refund — completes. + w1 := makePaymentRequest(handler, "POST", "/api/admin/payments/"+paymentID+"/refund", req, adminToken, ctx) + if w1.Code != http.StatusOK { + t.Fatalf("first refund: expected 200, got %d. body: %s", w1.Code, w1.Body.String()) + } + + // Same-key retry (identical amount, reason) — must dedup, not double-refund. + w2 := makePaymentRequest(handler, "POST", "/api/admin/payments/"+paymentID+"/refund", req, adminToken, ctx) + if w2.Code != http.StatusOK { + t.Fatalf("retry refund: expected 200, got %d. body: %s", w2.Code, w2.Body.String()) + } + + // Exactly one refund row for this payment. + var refundCount int + err = tx.QueryRow(ctx, `SELECT COUNT(*) FROM refunds WHERE payment_id = $1`, paymentID).Scan(&refundCount) + if err != nil { + t.Fatalf("failed to query refunds: %v", err) + } + if refundCount != 1 { + t.Errorf("expected 1 refund row (deduped), got %d", refundCount) + } +} + func TestRefund_PartialRefund(t *testing.T) { t.Parallel() ctx, tx := testutils.SetupTestTx(t) @@ -2304,21 +2358,36 @@ func TestCreatePaymentMethod_HappyPath(t *testing.T) { handler := CreatePaymentMethod reqBody := CreatePaymentMethodRequest{ - CardNumber: "4111111111111111", - Expiry: "12/30", - CVC: "123", + CardToken: "cnon:visa", } w := makePaymentRequest(handler, "POST", "/api/user/payment-methods", reqBody, token, ctx) - // PCI-DSS parity: raw PAN card creation is blocked in production, so the - // mock must reject it too — otherwise dev testing masks a prod failure. - if w.Code != http.StatusInternalServerError { - t.Errorf("expected status 500 (raw PAN rejected), got %d. body: %s", w.Code, w.Body.String()) + if w.Code != http.StatusOK { + t.Errorf("expected status 200, got %d. body: %s", w.Code, w.Body.String()) + return + } + + var card SavedCard + if err := json.Unmarshal(w.Body.Bytes(), &card); err != nil { + t.Fatalf("failed to parse response: %v", err) + } + if card.Brand != "VISA" { + t.Errorf("expected brand VISA, got %s", card.Brand) + } + if card.Last4 != "1111" { + t.Errorf("expected last4 1111, got %s", card.Last4) + } + if !card.IsDefault { + t.Error("expected first card to be default") } } -func TestCreatePaymentMethod_ExpiredCardRejected(t *testing.T) { +func TestCreatePaymentMethod_RawPANRejected(t *testing.T) { + // PCI-DSS: raw PANs are never accepted at the API edge — the handler must + // return 400 for a card_number body, since the field no longer exists and + // card_token is required. Validates that a raw PAN never reaches the + // Square client. t.Parallel() ctx, tx := testutils.SetupTestTx(t) @@ -2329,55 +2398,17 @@ func TestCreatePaymentMethod_ExpiredCardRejected(t *testing.T) { token := jwt.GenerateUserToken(userID) + // Raw PAN sent as the old field name — should be ignored and rejected. handler := CreatePaymentMethod - reqBody := CreatePaymentMethodRequest{ - CardNumber: "4111111111111111", - Expiry: "01/20", - CVC: "123", + reqBody := map[string]string{ + "card_number": "4111111111111111", + "expiry": "12/30", + "cvc": "123", } w := makePaymentRequest(handler, "POST", "/api/user/payment-methods", reqBody, token, ctx) - if w.Code != http.StatusBadRequest { - t.Errorf("expected status 400, got %d. body: %s", w.Code, w.Body.String()) - } -} - -func TestCreatePaymentMethod_InvalidExpiryRejected(t *testing.T) { - t.Parallel() - ctx, tx := testutils.SetupTestTx(t) - - userID, err := fixtures.CreateTestUser(tx) - if err != nil { - t.Fatalf("failed to create test user: %v", err) - } - - token := jwt.GenerateUserToken(userID) - - tests := []struct { - name string - expiry string - }{ - {"bad format", "12-30"}, - {"bad month", "13/30"}, - {"bad year", "12/abc"}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - handler := CreatePaymentMethod - reqBody := CreatePaymentMethodRequest{ - CardNumber: "4111111111111111", - Expiry: tt.expiry, - CVC: "123", - } - - w := makePaymentRequest(handler, "POST", "/api/user/payment-methods", reqBody, token, ctx) - - if w.Code != http.StatusBadRequest { - t.Errorf("expected status 400, got %d. body: %s", w.Code, w.Body.String()) - } - }) + t.Errorf("expected status 400 (card_token required), got %d. body: %s", w.Code, w.Body.String()) } } @@ -2396,9 +2427,7 @@ func TestCreatePaymentMethod_MissingFieldsRejected(t *testing.T) { name string body CreatePaymentMethodRequest }{ - {"no card number", CreatePaymentMethodRequest{Expiry: "12/30", CVC: "123"}}, - {"no expiry", CreatePaymentMethodRequest{CardNumber: "4111111111111111", CVC: "123"}}, - {"no cvc", CreatePaymentMethodRequest{CardNumber: "4111111111111111", Expiry: "12/30"}}, + {"no card token", CreatePaymentMethodRequest{}}, } for _, tt := range tests { @@ -2419,9 +2448,7 @@ func TestCreatePaymentMethod_NoAuthRejected(t *testing.T) { handler := CreatePaymentMethod w := makePaymentRequest(handler, "POST", "/api/user/payment-methods", CreatePaymentMethodRequest{ - CardNumber: "4111111111111111", - Expiry: "12/30", - CVC: "123", + CardToken: "cnon:visa", }, "", ctx) if w.Code != http.StatusUnauthorized { @@ -2440,18 +2467,36 @@ func TestCreatePaymentMethod_SecondCardNotDefault(t *testing.T) { token := jwt.GenerateUserToken(userID) - // PCI-DSS parity: raw PAN card creation is blocked in production, so the - // mock must reject it too — otherwise dev testing masks a prod failure. + // Create first card handler := CreatePaymentMethod reqBody := CreatePaymentMethodRequest{ - CardNumber: "4111111111111111", - Expiry: "12/30", - CVC: "123", + CardToken: "cnon:visa", } w := makePaymentRequest(handler, "POST", "/api/user/payment-methods", reqBody, token, ctx) - if w.Code != http.StatusInternalServerError { - t.Fatalf("expected 500 (raw PAN rejected), got %d. body: %s", w.Code, w.Body.String()) + if w.Code != http.StatusOK { + t.Fatalf("failed to create first card: %d. body: %s", w.Code, w.Body.String()) + } + + reqBody2 := CreatePaymentMethodRequest{ + CardToken: "cnon:mastercard", + } + + w = makePaymentRequest(handler, "POST", "/api/user/payment-methods", reqBody2, token, ctx) + if w.Code != http.StatusOK { + t.Fatalf("failed to create second card: %d. body: %s", w.Code, w.Body.String()) + } + + var card SavedCard + if err := json.Unmarshal(w.Body.Bytes(), &card); err != nil { + t.Fatalf("failed to parse response: %v", err) + } + + if card.Brand != "MASTERCARD" { + t.Errorf("expected brand MASTERCARD, got %s", card.Brand) + } + if card.IsDefault { + t.Error("expected second card to NOT be default") } } diff --git a/backend/handlers/payments/service.go b/backend/handlers/payments/service.go index 8932848..9cded59 100644 --- a/backend/handlers/payments/service.go +++ b/backend/handlers/payments/service.go @@ -10,8 +10,7 @@ import ( "fmt" "log" "log/slog" - "strconv" - "strings" + "math" "time" "github.com/jackc/pgx/v5" @@ -359,7 +358,7 @@ func (s *PaymentService) GetAlreadyRefundedAmount(ctx context.Context, paymentID if err != nil { return 0, err } - return int64(amount * 100), nil + return int64(math.Round(amount * 100)), nil } func (s *PaymentService) HasCompletedPayment(ctx context.Context, bookingID string) (bool, error) { @@ -501,29 +500,11 @@ func (s *PaymentService) DeletePaymentMethod(ctx context.Context, cardID, userID return nil } -func (s *PaymentService) CreatePaymentMethodFromDetails(ctx context.Context, userID, cardNumber, expiry, cvc string) (*SavedCard, error) { - parts := strings.Split(expiry, "/") - if len(parts) != 2 { - return nil, errors.New("invalid expiry format, use MM/YY") - } - expMonth, err := strconv.Atoi(parts[0]) - if err != nil || expMonth < 1 || expMonth > 12 { - return nil, errors.New("invalid expiry month") - } - expYear, err := strconv.Atoi(parts[1]) - if err != nil || expYear < 0 || expYear > 99 { - return nil, errors.New("invalid expiry year") - } - expYear += 2000 - - // Check if card is expired - now := clock.Now() - expiryDate := time.Date(expYear, time.Month(expMonth), 1, 0, 0, 0, 0, time.UTC) - if expiryDate.Before(now) { - return nil, errors.New("card has expired") - } - - cardOnFile, err := SquareClient.CreateCardOnFileRaw(ctx, userID, cardNumber, expMonth, expYear, cvc) +func (s *PaymentService) CreatePaymentMethodFromToken(ctx context.Context, userID, cardToken string) (*SavedCard, error) { + // PCI-DSS: raw PANs are never accepted. The client must supply a Square + // Web Payments nonce (cnon:xxx), which the backend tokenizes via the + // Cards API — the full PAN exists only inside Square's vault. + cardOnFile, err := SquareClient.CreateCardOnFile(ctx, userID, cardToken) if err != nil { return nil, fmt.Errorf("failed to tokenize card: %w", err) } @@ -535,7 +516,7 @@ func (s *PaymentService) CreatePaymentMethodFromDetails(ctx context.Context, use SELECT $1, $2, $3, $4, $5, $6, $7, NOT EXISTS(SELECT 1 FROM user_saved_cards WHERE user_id = $1 AND deleted_at IS NULL) RETURNING id, is_default - `, userID, cardOnFile.CardID, cardOnFile.Brand, cardOnFile.Last4, expMonth, expYear, cardOnFile.Fingerprint).Scan(&savedCardID, &isDefault) + `, userID, cardOnFile.CardID, cardOnFile.Brand, cardOnFile.Last4, cardOnFile.ExpMonth, cardOnFile.ExpYear, cardOnFile.Fingerprint).Scan(&savedCardID, &isDefault) if err != nil { return nil, fmt.Errorf("failed to save card: %w", err) } @@ -545,8 +526,8 @@ func (s *PaymentService) CreatePaymentMethodFromDetails(ctx context.Context, use SquareCardID: cardOnFile.CardID, Brand: cardOnFile.Brand, Last4: cardOnFile.Last4, - ExpMonth: expMonth, - ExpYear: expYear, + ExpMonth: cardOnFile.ExpMonth, + ExpYear: cardOnFile.ExpYear, Fingerprint: cardOnFile.Fingerprint, IsDefault: isDefault, }, nil diff --git a/backend/handlers/payments/till.go b/backend/handlers/payments/till.go index f751c43..674e99a 100644 --- a/backend/handlers/payments/till.go +++ b/backend/handlers/payments/till.go @@ -8,6 +8,7 @@ import ( "fmt" "log" "log/slog" + "math" "net/http" "crussell/db" @@ -28,10 +29,7 @@ type TillSaleRequest struct { UserSavedCardID *string `json:"user_saved_card_id,omitempty"` UserID *string `json:"user_id,omitempty"` IdempotencyKey string `json:"idempotency_key,omitempty"` - CardNumber string `json:"card_number,omitempty"` - CardExpMonth int `json:"card_exp_month,omitempty"` - CardExpYear int `json:"card_exp_year,omitempty"` - CardCVC string `json:"card_cvc,omitempty"` + CardToken string `json:"card_token,omitempty"` RedeemToUserID *string `json:"redeem_to_user_id,omitempty"` } @@ -94,8 +92,8 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { http.Error(w, "user_saved_card_id is required when payment method is saved_card", http.StatusBadRequest) return } - if req.PaymentMethod == "online_square" && req.CardNumber == "" { - http.Error(w, "card_number is required when payment method is online_square", http.StatusBadRequest) + if req.PaymentMethod == "online_square" && req.CardToken == "" { + http.Error(w, "card_token is required when payment method is online_square — use a Square Web Payments nonce", http.StatusBadRequest) return } if req.Action == "topup" && (req.GiftCardID == nil || *req.GiftCardID == "") { @@ -104,22 +102,33 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { } // Idempotency check: if key provided, return existing sale if found + // Idempotency handling. A 'completed' sale is a dedup (return it). A + // 'pending' sale means the previous Square charge failed — the gift card + // was already funded in the committed transaction, so re-attempt the + // Square charge (Square dedups on the same key) and complete the sale. + // Mirrors the tip/gift-card pending-reuse pattern. + var existingPendingID string + var existingPendingGiftCard string if req.IdempotencyKey != "" { - var existingID, existingStatus string - err := db.Conn.QueryRow(ctx, `SELECT id, status FROM till_sales WHERE idempotency_key = $1`, req.IdempotencyKey).Scan(&existingID, &existingStatus) + var existingID, existingStatus, existingItemID string + err := db.Conn.QueryRow(ctx, `SELECT id, status, item_id FROM till_sales WHERE idempotency_key = $1`, req.IdempotencyKey).Scan(&existingID, &existingStatus, &existingItemID) if err == nil { - // Existing sale found — return its ACTUAL status (may be 'pending' - // if a previous Square charge failed; must not report 'completed'). - if err := json.NewEncoder(w).Encode(TillSaleResponse{ - ID: existingID, - ItemType: req.ItemType, - TotalAmount: req.Amount, - PaymentMethod: req.PaymentMethod, - Status: existingStatus, - }); err != nil { - log.Printf("Failed to encode JSON response: %v", err) + if existingStatus == "completed" { + if err := json.NewEncoder(w).Encode(TillSaleResponse{ + ID: existingID, + ItemType: req.ItemType, + TotalAmount: req.Amount, + PaymentMethod: req.PaymentMethod, + Status: "completed", + }); err != nil { + log.Printf("Failed to encode JSON response: %v", err) + } + return + } + if existingStatus == "pending" { + existingPendingID = existingID + existingPendingGiftCard = existingItemID } - return } } @@ -137,96 +146,102 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { } }() + // Pending-retry: the gift card was already created and funded in the prior + // committed transaction, so skip the create/top-up and sale-insert blocks. var giftCardID string - if req.Action == "create" { - var purchaseVoucherType string - err = tx.QueryRow(ctx, `SELECT COALESCE(voucher_type, 'SPV') FROM business_settings LIMIT 1`).Scan(&purchaseVoucherType) - if err != nil { - log.Printf("Failed to query voucher type: %v", err) - http.Error(w, "internal server error", http.StatusInternalServerError) - return - } - if purchaseVoucherType == "" { - purchaseVoucherType = "SPV" - } - err = tx.QueryRow(ctx, ` + if existingPendingID != "" { + giftCardID = existingPendingGiftCard + } else { + if req.Action == "create" { + var purchaseVoucherType string + err = tx.QueryRow(ctx, `SELECT COALESCE(voucher_type, 'SPV') FROM business_settings LIMIT 1`).Scan(&purchaseVoucherType) + if err != nil { + log.Printf("Failed to query voucher type: %v", err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + if purchaseVoucherType == "" { + purchaseVoucherType = "SPV" + } + err = tx.QueryRow(ctx, ` INSERT INTO gift_cards (total_funds_added, amount_remaining, created_by, is_inventory, voucher_type_at_purchase) VALUES ($1, $1, $2, FALSE, $3) RETURNING id `, req.Amount, adminID, purchaseVoucherType).Scan(&giftCardID) - if err != nil { - log.Printf("Failed to create gift card: %v", err) - http.Error(w, "internal server error", http.StatusInternalServerError) - return - } + if err != nil { + log.Printf("Failed to create gift card: %v", err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } - _, err = tx.Exec(ctx, ` + _, err = tx.Exec(ctx, ` INSERT INTO gift_card_transactions (gift_card_id, transaction_type, amount, reference_type, reference_id, user_id, notes) VALUES ($1, 'purchase', $2, 'till_sale', NULL, $3, NULL) `, giftCardID, req.Amount, req.UserID) - if err != nil { - log.Printf("Failed to create gift_card_transaction: %v", err) - http.Error(w, "internal server error", http.StatusInternalServerError) - return - } - } else { - cardID := validators.NormalizeGiftCardCode(*req.GiftCardID) - var redeemedBy sql.NullString - err = tx.QueryRow(ctx, "SELECT redeemed_by FROM gift_cards WHERE id = $1", cardID).Scan(&redeemedBy) - if err != nil { - if errors.Is(err, pgx.ErrNoRows) { - http.Error(w, "Gift card not found", http.StatusNotFound) + if err != nil { + log.Printf("Failed to create gift_card_transaction: %v", err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + } else { + cardID := validators.NormalizeGiftCardCode(*req.GiftCardID) + var redeemedBy sql.NullString + err = tx.QueryRow(ctx, "SELECT redeemed_by FROM gift_cards WHERE id = $1", cardID).Scan(&redeemedBy) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + http.Error(w, "Gift card not found", http.StatusNotFound) + return + } + log.Printf("Failed to check gift card: %v", err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + if redeemedBy.Valid { + http.Error(w, "Cannot top up a card that has been redeemed to an account", http.StatusBadRequest) return } - log.Printf("Failed to check gift card: %v", err) - http.Error(w, "internal server error", http.StatusInternalServerError) - return - } - if redeemedBy.Valid { - http.Error(w, "Cannot top up a card that has been redeemed to an account", http.StatusBadRequest) - return - } - var isInventory bool - var previousTotal float64 - err = tx.QueryRow(ctx, `SELECT is_inventory, total_funds_added FROM gift_cards WHERE id = $1`, cardID).Scan(&isInventory, &previousTotal) - if err != nil { - log.Printf("Failed to check gift card state: %v", err) - http.Error(w, "internal server error", http.StatusInternalServerError) - return - } + var isInventory bool + var previousTotal float64 + err = tx.QueryRow(ctx, `SELECT is_inventory, total_funds_added FROM gift_cards WHERE id = $1`, cardID).Scan(&isInventory, &previousTotal) + if err != nil { + log.Printf("Failed to check gift card state: %v", err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } - _, err = tx.Exec(ctx, ` + _, err = tx.Exec(ctx, ` UPDATE gift_cards SET total_funds_added = total_funds_added + $1, amount_remaining = amount_remaining + $1, last_used_at = NOW() WHERE id = $2 `, req.Amount, cardID) - if err != nil { - log.Printf("Failed to top up gift card: %v", err) - http.Error(w, "internal server error", http.StatusInternalServerError) - return - } + if err != nil { + log.Printf("Failed to top up gift card: %v", err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } - transactionType := "topup" - var notes *string - if isInventory && previousTotal == 0 { - transactionType = "purchase" - n := "first top-up on inventory card" - notes = &n - } - _, err = tx.Exec(ctx, ` + transactionType := "topup" + var notes *string + if isInventory && previousTotal == 0 { + transactionType = "purchase" + n := "first top-up on inventory card" + notes = &n + } + _, err = tx.Exec(ctx, ` INSERT INTO gift_card_transactions (gift_card_id, transaction_type, amount, reference_type, reference_id, user_id, notes) VALUES ($1, $2, $3, 'till_sale', NULL, $4, $5) `, cardID, transactionType, req.Amount, req.UserID, notes) - if err != nil { - log.Printf("Failed to create gift_card_transaction: %v", err) - http.Error(w, "internal server error", http.StatusInternalServerError) - return - } + if err != nil { + log.Printf("Failed to create gift_card_transaction: %v", err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } - giftCardID = cardID + giftCardID = cardID + } } // If the gift card should be immediately redeemed to a user's account balance @@ -260,7 +275,7 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { } } - penceAmount := int64(req.Amount * 100) + penceAmount := int64(math.Round(req.Amount * 100)) var squarePaymentID *string var squareCheckoutID *string @@ -354,7 +369,12 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { desc := fmt.Sprintf("Gift Card %s (£%.2f)", req.Action, req.Amount) var tillSaleID string - err = tx.QueryRow(ctx, ` + if existingPendingID != "" { + // Reusing the pending sale row from a failed prior attempt — the sale + // was already inserted, so skip the insert and reuse its ID. + tillSaleID = existingPendingID + } else { + err = tx.QueryRow(ctx, ` INSERT INTO till_sales ( item_type, item_id, description, quantity, unit_price, total_amount, payment_method, status, user_id, user_saved_card_id, @@ -362,45 +382,49 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { ) VALUES ($1, $2, $3, 1, $4, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, NOW(), NOW()) RETURNING id `, - req.ItemType, - giftCardID, - desc, - req.Amount, - dbPaymentMethod, - saleStatus, - req.UserID, - req.UserSavedCardID, - squarePaymentID, - squareCheckoutID, - req.IdempotencyKey, - "Admin till sale: "+req.Action+" gift card", - adminID, - ).Scan(&tillSaleID) - if err != nil { - log.Printf("Failed to insert till sale: %v", err) - http.Error(w, "internal server error", http.StatusInternalServerError) - return - } + req.ItemType, + giftCardID, + desc, + req.Amount, + dbPaymentMethod, + saleStatus, + req.UserID, + req.UserSavedCardID, + squarePaymentID, + squareCheckoutID, + req.IdempotencyKey, + "Admin till sale: "+req.Action+" gift card", + adminID, + ).Scan(&tillSaleID) + if err != nil { + log.Printf("Failed to insert till sale: %v", err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } - _, err = tx.Exec(ctx, ` + _, err = tx.Exec(ctx, ` UPDATE gift_card_transactions SET reference_id = $1 WHERE gift_card_id = $2 AND reference_id IS NULL AND created_at > NOW() - INTERVAL '5 seconds' `, tillSaleID, giftCardID) - if err != nil { - log.Printf("Failed to update gift_card_transactions reference: %v", err) - http.Error(w, "internal server error", http.StatusInternalServerError) - return - } + if err != nil { + log.Printf("Failed to update gift_card_transactions reference: %v", err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } - if req.PaymentMethod != "on_the_house" && (saleStatus == "completed" || needsSquarePayment) { - vatCfg, vatErr := GetVATConfig(ctx, tx) - if vatErr == nil && vatCfg.IsVATRegistered && vatCfg.VoucherType == "SPV" { - if _, vatExecErr := tx.Exec(ctx, "SELECT apply_vat_to_till_sale($1, $2)", tillSaleID, vatCfg.DefaultVATRate); vatExecErr != nil { - log.Printf("Failed to apply VAT to till sale %s: %v", tillSaleID, vatExecErr) + if req.PaymentMethod != "on_the_house" && (saleStatus == "completed" || needsSquarePayment) { + vatCfg, vatErr := GetVATConfig(ctx, tx) + if vatErr == nil && vatCfg.IsVATRegistered && vatCfg.VoucherType == "SPV" { + if _, vatExecErr := tx.Exec(ctx, "SELECT apply_vat_to_till_sale($1, $2)", tillSaleID, vatCfg.DefaultVATRate); vatExecErr != nil { + log.Printf("Failed to apply VAT to till sale %s: %v", tillSaleID, vatExecErr) + } } } } + // Commit the (possibly nested) transaction. In the pending-reuse path it is + // empty, but the commit releases the savepoint in the test harness so the + // deferred rollback does not undo the later status UPDATE. if err := tx.Commit(ctx); err != nil { log.Printf("Failed to commit till sale transaction: %v", err) http.Error(w, "internal server error", http.StatusInternalServerError) @@ -436,9 +460,16 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { } paymentResult, squareErr = SquareClient.CreatePayment(ctx, paymentReq) } else if req.PaymentMethod == "online_square" { - cardOnFile, cardErr := SquareClient.CreateCardOnFileRaw(ctx, "till-"+giftCardID, req.CardNumber, req.CardExpMonth, req.CardExpYear, req.CardCVC) + // PCI-DSS: raw PANs are never accepted. The admin till must supply a + // Square Web Payments nonce (cnon:xxx), tokenized via the Cards API. + if req.CardToken == "" { + log.Printf("online_square till sale missing card_token for gift card %s", giftCardID) + http.Error(w, "card_token is required for online_square payment — use a Square Web Payments nonce", http.StatusBadRequest) + return + } + cardOnFile, cardErr := SquareClient.CreateCardOnFile(ctx, "till-"+giftCardID, req.CardToken) if cardErr != nil { - log.Printf("Failed to tokenize ephemeral card: %v", cardErr) + log.Printf("Failed to tokenize card: %v", cardErr) http.Error(w, "Card tokenization failed", http.StatusInternalServerError) return } diff --git a/backend/handlers/payments/till_test.go b/backend/handlers/payments/till_test.go index 3fcd2da..232209d 100644 --- a/backend/handlers/payments/till_test.go +++ b/backend/handlers/payments/till_test.go @@ -789,7 +789,7 @@ func TestCreateTillSale_SavedCardNoUserSavedCardID(t *testing.T) { } } -func TestCreateTillSale_OnlineSquareNoCardNumber(t *testing.T) { +func TestCreateTillSale_OnlineSquareNoCardToken(t *testing.T) { _, tx := testutils.SetupTestTx(t) adminID, err := fixtures.CreateTestAdminUser(tx) @@ -823,6 +823,128 @@ func TestCreateTillSale_OnlineSquareNoCardNumber(t *testing.T) { } } +func TestCreateTillSale_OnlineSquareWithToken(t *testing.T) { + _, 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") + + reqBody := TillSaleRequest{ + ItemType: "gift_card", + Action: "create", + Amount: 1000, + PaymentMethod: "online_square", + CardToken: "cnon:visa", + } + + bodyBytes, _ := json.Marshal(reqBody) + req := httptest.NewRequest("POST", "/api/admin/till/sale", bytes.NewReader(bodyBytes)) + req.Header.Set("Authorization", "Bearer "+adminToken) + req.Header.Set("Content-Type", "application/json") + req = req.WithContext(db.ContextWithTx(req.Context(), tx.(pgx.Tx))) + + w := httptest.NewRecorder() + r := chi.NewRouter() + r.Use(mw.RequireAuth) + r.Post("/api/admin/till/sale", CreateTillSale) + r.ServeHTTP(w, req) + + if w.Code != http.StatusCreated && w.Code != http.StatusOK { + t.Errorf("expected 200/201, got %d. body: %s", w.Code, w.Body.String()) + } +} + +// TestCreateTillSale_PendingRetry_ReattemptsSquare verifies that a same-key +// retry after a failed Square charge (sale stuck 'pending', gift card already +// funded) re-attempts the charge and completes the sale — it must NOT return +// the stale 'pending' status without re-charging (silent money loss). +func TestCreateTillSale_PendingRetry_ReattemptsSquare(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) + } + userID, err := fixtures.CreateTestUser(tx) + if err != nil { + t.Fatalf("failed to create user: %v", err) + } + adminToken := jwt.GenerateTestToken(adminID, "admin") + + cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "sq_test_card_id", "VISA", "1234") + if err != nil { + t.Fatalf("failed to create saved card: %v", err) + } + + // Seed a PENDING till_sale with the same key and a funded gift card — + // simulates a prior attempt where the Square charge failed after the DB + // transaction committed (card already funded). + key := "till-pending-retry-key" + var giftCardID string + 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) + if err != nil { + t.Fatalf("failed to create gift card: %v", err) + } + _, 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 create', 1, 50.00, 50.00, 'online_square', 'pending', + $2, $3, $4, $5, NOW(), NOW()) + `, giftCardID, userID, cardID, key, adminID) + if err != nil { + t.Fatalf("failed to seed pending till sale: %v", err) + } + + reqBody := TillSaleRequest{ + ItemType: "gift_card", + Action: "create", + Amount: 50.00, + PaymentMethod: "saved_card", + UserSavedCardID: &cardID, + UserID: &userID, + IdempotencyKey: key, + } + + bodyBytes, _ := json.Marshal(reqBody) + req := httptest.NewRequest("POST", "/api/admin/till/sale", bytes.NewReader(bodyBytes)) + req.Header.Set("Authorization", "Bearer "+adminToken) + req.Header.Set("Content-Type", "application/json") + req = req.WithContext(db.ContextWithTx(req.Context(), tx.(pgx.Tx))) + + w := httptest.NewRecorder() + r := chi.NewRouter() + r.Use(mw.RequireAuth) + r.Post("/api/admin/till/sale", CreateTillSale) + r.ServeHTTP(w, req) + + if w.Code != http.StatusOK && w.Code != http.StatusCreated { + t.Fatalf("expected 200/201, got %d. body: %s", w.Code, w.Body.String()) + } + + // The sale must now be 'completed' (Square re-attempted and succeeded). + var saleStatus string + var saleCount int + err = tx.QueryRow(ctx, `SELECT COUNT(*), MAX(status) FROM till_sales WHERE idempotency_key = $1`, key).Scan(&saleCount, &saleStatus) + if err != nil { + t.Fatalf("failed to query till sale: %v", err) + } + if saleCount != 1 { + t.Errorf("expected 1 till sale (reuse, not duplicate), got %d", saleCount) + } + if saleStatus != "completed" { + t.Errorf("expected pending sale to be completed after retry, got %s", saleStatus) + } +} + func TestCreateTillSale_CreateCash(t *testing.T) { ctx, tx := testutils.SetupTestTx(t) diff --git a/backend/internal/square/square.go b/backend/internal/square/square.go index d5fd15c..95db980 100644 --- a/backend/internal/square/square.go +++ b/backend/internal/square/square.go @@ -2,10 +2,7 @@ package square -import ( - "context" - "fmt" -) +import "context" var Client SquareClient @@ -39,10 +36,6 @@ func (p *ProdClient) CreateCardOnFile(ctx context.Context, userID, cardToken str return createCardOnFileHTTP(ctx, userID, cardToken) } -func (p *ProdClient) CreateCardOnFileRaw(ctx context.Context, userID, cardNumber string, expMonth, expYear int, cvc string) (*CardOnFile, error) { - return nil, fmt.Errorf("square: raw card number input is not supported in production — use CreateCardOnFile with a card nonce") -} - func (p *ProdClient) GetCardsOnFile(ctx context.Context, userID string) ([]CardOnFile, error) { return getCardsOnFileHTTP(ctx, userID) } diff --git a/backend/internal/square/square_dev.go b/backend/internal/square/square_dev.go index 757f30a..8f78688 100644 --- a/backend/internal/square/square_dev.go +++ b/backend/internal/square/square_dev.go @@ -52,9 +52,6 @@ func (d *devProdClient) RefundPayment(ctx context.Context, req RefundPaymentReq) func (d *devProdClient) CreateCardOnFile(ctx context.Context, userID, cardToken string) (*CardOnFile, error) { return createCardOnFileHTTP(ctx, userID, cardToken) } -func (d *devProdClient) CreateCardOnFileRaw(ctx context.Context, userID, cardNumber string, expMonth, expYear int, cvc string) (*CardOnFile, error) { - return nil, fmt.Errorf("square: raw card number input is not supported in production — use CreateCardOnFile with a card nonce") -} func (d *devProdClient) GetCardsOnFile(ctx context.Context, userID string) ([]CardOnFile, error) { return getCardsOnFileHTTP(ctx, userID) } @@ -373,13 +370,6 @@ func (m *MockClient) CreateCardOnFile(ctx context.Context, userID, cardToken str return card, nil } -func (m *MockClient) CreateCardOnFileRaw(ctx context.Context, userID, cardNumber string, expMonth, expYear int, cvc string) (*CardOnFile, error) { - // PCI-DSS parity with production: raw card numbers are never accepted. - // The mock must behave identically to the ProdClient so dev testing does - // not mask a production failure. - return nil, fmt.Errorf("square: raw card number input is not supported in production — use CreateCardOnFile with a card nonce") -} - func (m *MockClient) GetCardsOnFile(ctx context.Context, userID string) ([]CardOnFile, error) { log.Printf("[SQUARE-MOCK] GetCardsOnFile: user=%s", userID) diff --git a/backend/internal/square/square_dev_test.go b/backend/internal/square/square_dev_test.go index 151868e..0671cc2 100644 --- a/backend/internal/square/square_dev_test.go +++ b/backend/internal/square/square_dev_test.go @@ -237,9 +237,9 @@ func TestDevClient_GetCheckout_NotFound(t *testing.T) { require.Error(t, err) } -func TestDevClient_CreateCardOnFileRaw_Rejected_ProdParity(t *testing.T) { - // PCI-DSS parity: the mock must reject raw PANs exactly like the - // ProdClient, so dev testing cannot mask a production failure. +func TestDevClient_CreateCardOnFile_RejectsRawPAN(t *testing.T) { + // PCI-DSS parity: CreateCardOnFile accepts only token-like source_ids + // (cnon:xxx / ccof:xxx). Raw PANs are rejected exactly like real Square. client := NewDevClient().(*MockClient) ctx := context.Background() @@ -251,16 +251,15 @@ func TestDevClient_CreateCardOnFileRaw_Rejected_ProdParity(t *testing.T) { {"mastercard", "5555555555554444"}, {"amex", "378282246310005"}, {"discover", "6011111111111117"}, - {"unknown brand", "9999999999999999"}, {"too short", "123"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - card, err := client.CreateCardOnFileRaw(ctx, "user-raw-"+tt.name, tt.cardNumber, 12, 2030, "123") + card, err := client.CreateCardOnFile(ctx, "user-raw-"+tt.name, tt.cardNumber) require.Error(t, err, "raw PAN must be rejected for production parity") assert.Nil(t, card) - assert.Contains(t, err.Error(), "raw card number input is not supported") + assert.Contains(t, err.Error(), "invalid source_id") }) } } @@ -372,6 +371,37 @@ func TestDevClient_CreatePayment_WithTipMoney(t *testing.T) { assert.Equal(t, int64(1000), result.TipAmount) } +func TestDevClient_CreatePayment_DedupsOnIdempotencyKey(t *testing.T) { + // Real Square dedups on idempotency key: a same-key retry returns the + // original payment. The mock must mirror this or dev/testing diverges + // from production (and the pending-retry logic can't be exercised). + client := NewDevClient().(*MockClient) + ctx := context.Background() + + req := CreatePaymentReq{ + Amount: 5000, + Currency: "GBP", + SourceID: "cnon:test-card", + IdempotencyKey: "dedup-key-1", + ReferenceID: "booking-dedup", + } + + first, err := client.CreatePayment(ctx, req) + require.NoError(t, err) + require.NotEmpty(t, first.ID) + + second, err := client.CreatePayment(ctx, req) + require.NoError(t, err) + assert.Equal(t, first.ID, second.ID, "same-key retry must return the original payment, not a new one") + + // Total stored payments for this key must be one (deduped). + client.mu.RLock() + byKey := client.paymentByKey["dedup-key-1"] + client.mu.RUnlock() + assert.NotNil(t, byKey) + assert.Equal(t, first.ID, byKey.ID) +} + func TestDevClient_CreatePayment_AutocompleteFalse(t *testing.T) { client := NewDevClient().(*MockClient) ctx := context.Background() @@ -426,41 +456,6 @@ func TestDevClient_CreateCardOnFile_WithNewFields(t *testing.T) { assert.NotEmpty(t, card.CreatedAt) } -func TestDevClient_CreateCardOnFileRaw_WithBrandDetection(t *testing.T) { - client := NewDevClient().(*MockClient) - ctx := context.Background() - userID := "user-raw-brand-detect" - - card, err := client.CreateCardOnFileRaw(ctx, userID, "4111111111111111", 12, 2030, "123") - require.Error(t, err, "raw PAN must be rejected for production parity") - assert.Nil(t, card) -} - -func TestDevClient_CreateCardOnFile_RejectsRawPAN(t *testing.T) { - client := NewDevClient().(*MockClient) - ctx := context.Background() - - tests := []struct { - name string - cardNum string - }{ - {"visa formatted", "4111 1111 1111 1111"}, - {"visa raw", "4111111111111111"}, - {"mastercard", "5500 0000 0000 0004"}, - {"amex", "3400 0000 0000 009"}, - {"discover", "6011 0000 0000 0004"}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - userID := fmt.Sprintf("user-raw-card-%s", tt.name) - card, err := client.CreateCardOnFile(ctx, userID, tt.cardNum) - require.Error(t, err, "raw PAN must be rejected for production parity") - assert.Nil(t, card) - }) - } -} - func TestDevClient_DeleteCardOnFile_SoftDelete(t *testing.T) { client := NewDevClient().(*MockClient) ctx := context.Background() @@ -478,15 +473,6 @@ func TestDevClient_DeleteCardOnFile_SoftDelete(t *testing.T) { assert.False(t, cards[0].Enabled) } -func TestDevClient_CreateCardOnFileRaw_TooShort(t *testing.T) { - client := NewDevClient().(*MockClient) - ctx := context.Background() - - _, err := client.CreateCardOnFileRaw(ctx, "user-too-short", "123", 12, 2030, "999") - require.Error(t, err) - assert.Contains(t, err.Error(), "raw card number input is not supported") -} - func TestDevClient_GetCardsOnFile_Empty(t *testing.T) { client := NewDevClient().(*MockClient) ctx := context.Background() diff --git a/backend/internal/square/types.go b/backend/internal/square/types.go index 1e868ce..572fe3f 100644 --- a/backend/internal/square/types.go +++ b/backend/internal/square/types.go @@ -137,7 +137,6 @@ type SquareClient interface { GetCheckout(ctx context.Context, checkoutID string) (*PaymentResult, error) RefundPayment(ctx context.Context, req RefundPaymentReq) (*RefundResult, error) CreateCardOnFile(ctx context.Context, userID, cardToken string) (*CardOnFile, error) - CreateCardOnFileRaw(ctx context.Context, userID, cardNumber string, expMonth, expYear int, cvc string) (*CardOnFile, error) GetCardsOnFile(ctx context.Context, userID string) ([]CardOnFile, error) DeleteCardOnFile(ctx context.Context, cardID string) error } diff --git a/frontend/src/lib/components/booking/BookingFlow.svelte b/frontend/src/lib/components/booking/BookingFlow.svelte index c1a020c..a94dac4 100644 --- a/frontend/src/lib/components/booking/BookingFlow.svelte +++ b/frontend/src/lib/components/booking/BookingFlow.svelte @@ -136,13 +136,22 @@ const cardError = $derived( cardNumberTouched && !isValidLuhn(newCardNumber) && newCardNumber.length > 0 ? 'Invalid card number' - : cardExpiryTouched && expiryParts !== null && (() => { const em = expiryParts.year * 12 + expiryParts.month; const now2 = new SvelteDate(); const cm = now2.getFullYear() * 12 + now2.getMonth() + 1; return em < cm; })() + : cardExpiryTouched && + expiryParts !== null && + (() => { + const em = expiryParts.year * 12 + expiryParts.month; + const now2 = new SvelteDate(); + const cm = now2.getFullYear() * 12 + now2.getMonth() + 1; + return em < cm; + })() ? 'This card has expired' : cardExpiryTouched && !/^\d{2}\/\d{2}$/.test(newCardExpiry) && newCardExpiry.length > 0 ? 'Enter expiry as MM/YY' : cardCVCTouched && newCardCVC.length < 3 && newCardCVC.length > 0 ? 'Enter your CVC number' - : isValidLuhn(newCardNumber) && /^\d{2}\/\d{2}$/.test(newCardExpiry) && newCardCVC.length >= 3 + : isValidLuhn(newCardNumber) && + /^\d{2}\/\d{2}$/.test(newCardExpiry) && + newCardCVC.length >= 3 ? null : newCardNumber.length === 0 && newCardExpiry.length === 0 && newCardCVC.length === 0 ? null @@ -345,10 +354,23 @@ const bookingId = confirmedBooking.id; const amountCents = Math.round(amount * 100); + // Cache the idempotency key per amount+card so a lost-response retry + // reuses it (backend dedups) instead of double-charging. + const cardKey = selectedPaymentMethod ?? newCardNumber.replace(/\s/g, '') ?? ''; + if ( + !depositIdempotencyKey || + depositKeyedAmount !== amountCents || + depositKeyedCard !== cardKey + ) { + depositIdempotencyKey = generateUUID(); + depositKeyedAmount = amountCents; + depositKeyedCard = cardKey; + } + const body: Record = { payment_type: 'deposit', amount: amountCents, - idempotency_key: generateUUID() + idempotency_key: depositIdempotencyKey }; if (selectedPaymentMethod) { @@ -373,6 +395,9 @@ if (response.ok) { depositPaid = true; + depositIdempotencyKey = ''; + depositKeyedAmount = 0; + depositKeyedCard = ''; // Immutable update — avoid mutating the existing object so // concurrent renders (e.g. a stale fetch) can't observe partial // state. (See audit: HIGH issue #3 — confirmedBooking mutated @@ -400,6 +425,13 @@ let paymentAttempted = $state(false); + // Cached idempotency key per deposit attempt (amount + card): reused on + // retry so a lost-response retry dedups instead of double-charging, + // regenerated when the amount or card changes. Matches the tip-flow pattern. + let depositIdempotencyKey = $state(''); + let depositKeyedAmount = $state(0); + let depositKeyedCard = $state(''); + function formatCardExpiry(month: number, year: number): string { return `${String(month).padStart(2, '0')}/${year.toString().slice(-2)}`; } diff --git a/frontend/src/lib/components/payments/UserPaymentModal.svelte b/frontend/src/lib/components/payments/UserPaymentModal.svelte index 79eeb80..abc9ed0 100644 --- a/frontend/src/lib/components/payments/UserPaymentModal.svelte +++ b/frontend/src/lib/components/payments/UserPaymentModal.svelte @@ -29,6 +29,14 @@ let status = $state('idle'); let error = $state(null); + + // Cached idempotency key per payment attempt (amount + type + card): reused + // on retry so a lost-response retry dedups instead of double-charging, + // regenerated when any of those change. Matches the tip-flow pattern. + let payIdempotencyKey = $state(''); + let payKeyedAmount = $state(0); + let payKeyedType = $state(''); + let payKeyedCard = $state(''); let paymentResult = $state<{ id: string; amount: number; @@ -354,7 +362,20 @@ return; } - const idempotencyKey = generateIdempotencyKey(); + // Cache the idempotency key per amount+type+card so a lost-response + // retry reuses it (backend dedups) instead of double-charging. + const cardKey = cardId ?? newCardToken ?? ''; + if ( + !payIdempotencyKey || + payKeyedAmount !== amountCents || + payKeyedType !== paymentType || + payKeyedCard !== cardKey + ) { + payIdempotencyKey = generateIdempotencyKey(); + payKeyedAmount = amountCents; + payKeyedType = paymentType; + payKeyedCard = cardKey; + } try { const response = await apiFetch(`/api/bookings/${booking.id}/payment`, { @@ -366,7 +387,7 @@ card_id: cardId, new_card_token: newCardToken, save_card: saveCard, - idempotency_key: idempotencyKey + idempotency_key: payIdempotencyKey }) }); @@ -378,6 +399,10 @@ const data = await response.json(); // Payment is synchronous (completed immediately) status = 'success'; + payIdempotencyKey = ''; + payKeyedAmount = 0; + payKeyedType = ''; + payKeyedCard = ''; paymentResult = { id: data.id, amount: data.amount, diff --git a/frontend/src/routes/account/+page.svelte b/frontend/src/routes/account/+page.svelte index f24169a..879512a 100644 --- a/frontend/src/routes/account/+page.svelte +++ b/frontend/src/routes/account/+page.svelte @@ -180,6 +180,13 @@ let buyingGiftCard = $state(false); let purchaseResultCode = $state(null); + // Cached idempotency key: generated once per purchase attempt, reused on + // retry (so a lost-response retry dedups instead of double-charging), + // cleared on success. Reset when the amount or payment method changes. + let buyIdempotencyKey = $state(''); + let buyKeyedAmount = $state(0); + let buyKeyedCard = $state(''); + $effect(() => { // Auto-select the default saved card only once, when cards first load. // Do NOT re-select when the user explicitly chooses "Use a new card" @@ -337,7 +344,15 @@ return; } - const idempotencyKey = generateIdempotencyKey(); + // Cache the idempotency key per amount+card so a lost-response retry + // reuses the same key (backend dedups) instead of double-charging. + // Regenerate when the amount or card changes. + const cardKey = cardId ?? newCardToken ?? ''; + if (!buyIdempotencyKey || buyKeyedAmount !== buyAmount || buyKeyedCard !== cardKey) { + buyIdempotencyKey = generateIdempotencyKey(); + buyKeyedAmount = buyAmount; + buyKeyedCard = cardKey; + } const res = await apiFetch('/api/user/giftcards/buy', { method: 'POST', @@ -349,7 +364,7 @@ card_id: cardId, new_card_token: newCardToken, save_card: saveCard, - idempotency_key: idempotencyKey + idempotency_key: buyIdempotencyKey }) }); @@ -360,6 +375,9 @@ buyNewCardNumber = ''; buyNewCardExpiry = ''; buyNewCardCVC = ''; + buyIdempotencyKey = ''; + buyKeyedAmount = 0; + buyKeyedCard = ''; await fetchGiftCardBalance(); if (buySelectedCard === '') { await savedCardsStore.fetch(); diff --git a/init-scripts/init-script.sql b/init-scripts/init-script.sql index d1788f3..fedaaf0 100644 --- a/init-scripts/init-script.sql +++ b/init-scripts/init-script.sql @@ -1989,6 +1989,7 @@ CREATE TABLE refunds ( square_refund_id TEXT, status payment_status NOT NULL DEFAULT 'pending', reason TEXT NOT NULL, + idempotency_key VARCHAR(64) UNIQUE, created_by CHAR(12) REFERENCES users(id) ON DELETE SET NULL, created_at TIMESTAMPTZ NOT NULL DEFAULT NOW() );