From d54f526b561a7eb81890a50b3f0a534a0d570206 Mon Sep 17 00:00:00 2001 From: Stephen Adamson Date: Fri, 31 Jul 2026 15:36:22 +0100 Subject: [PATCH] Fix tip retry amount guard false 400 on non-exact pence values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pending-retry amount guard compared pence via int64(pounds*100), which truncates instead of rounding. For non-exact pound values (e.g. £1.14 stored as the float64 1.1399999999999999) the truncation yields 113 != 114, falsely rejecting a legitimate same-amount retry with 400. Since the frontend reuses the idempotency key on same-amount retries, every retry was rejected, permanently stranding the pending record (and any orphaned Square charge) with no recovery path. Fix: compare in pence via math.Round — the existing pattern already used in refunds.go — so non-exact values round to the true pence. Also applied the same correction to the sibling lossy conversions: - CreateTipPayment / CreateBookingPayment completed-dedup responses (would have reported 113p for a 114p payment) - BuyGiftCard retry amount guard (latent: £10/£20/£50 are float-exact so it never bit, but the identical trap is now closed) Tests: - TestTipPayment_RetryPending_NonExactAmountSucceeds: 114p pending record + same-amount retry completes and charges 114p (failed on the old truncation with 400) - TestTipPayment_RetryPending_AmountMismatchRejected: a same-key retry at a different amount is still rejected with 400 and the pending record is left untouched Verified: full backend suite green (25/25 packages, 0 failures), -race clean on handlers/payments, go build + go vet clean. --- backend/handlers/payments/giftcards.go | 3 +- backend/handlers/payments/handlers.go | 13 ++- backend/handlers/payments/payments_test.go | 102 +++++++++++++++++++++ 3 files changed, 112 insertions(+), 6 deletions(-) diff --git a/backend/handlers/payments/giftcards.go b/backend/handlers/payments/giftcards.go index 330316f..1ce6d40 100644 --- a/backend/handlers/payments/giftcards.go +++ b/backend/handlers/payments/giftcards.go @@ -8,6 +8,7 @@ import ( "fmt" "log" "log/slog" + "math" "net/http" "strconv" "strings" @@ -944,7 +945,7 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { // Guard the amount: a retry with a different amount must not // reuse the pending record (gift card would be issued at the // new amount against the old charge record). - if int64(existing.Amount*100) != req.Amount { + if int64(math.Round(existing.Amount*100)) != req.Amount { log.Printf("Gift card retry amount mismatch: pending record %s has %.2f, request has %d pence", existing.ID, existing.Amount, req.Amount) http.Error(w, "Amount does not match the pending gift card payment", http.StatusBadRequest) return diff --git a/backend/handlers/payments/handlers.go b/backend/handlers/payments/handlers.go index 8a8c13c..8ff3c8c 100644 --- a/backend/handlers/payments/handlers.go +++ b/backend/handlers/payments/handlers.go @@ -907,7 +907,7 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { BookingID: existingBookingID.String, PaymentType: existingPaymentType.String, Status: existingStatus.String, - Amount: int64(existingAmount.Float64 * 100), + Amount: int64(math.Round(existingAmount.Float64 * 100)), CreatedAt: existingCreatedAt.Time.Format(time.RFC3339), }); err != nil { log.Printf("Failed to encode JSON response: %v", err) @@ -1929,7 +1929,7 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { BookingID: existingBookingID.String, PaymentType: existingPaymentType.String, Status: existingStatus.String, - Amount: int64(existingAmount.Float64 * 100), + Amount: int64(math.Round(existingAmount.Float64 * 100)), CreatedAt: existingCreatedAt.Time.Format(time.RFC3339), }); err != nil { log.Printf("Failed to encode JSON response: %v", err) @@ -1939,9 +1939,12 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { // Previous Square call failed — reuse the pending record and re-attempt. // Guard the amount: a retry with a different amount must not mutate the // original record (books, VAT, refund caps) or silently charge the new - // amount against the old record. - if int64(existingAmount.Float64*100) != req.Amount { - log.Printf("Tip retry amount mismatch: pending record %s has %d pence, request has %d pence", existingID.String, int64(existingAmount.Float64*100), req.Amount) + // amount against the old record. Compare in pence via math.Round — the + // stored pounds value is float64, so int64(pounds*100) truncation would + // reject legitimate same-amount retries for non-exact values (e.g. £1.14 + // stored as 1.1399999999999999 → int64 gives 113 ≠ 114). + if int64(math.Round(existingAmount.Float64*100)) != req.Amount { + log.Printf("Tip retry amount mismatch: pending record %s has %d pence, request has %d pence", existingID.String, int64(math.Round(existingAmount.Float64*100)), req.Amount) http.Error(w, "Amount does not match the pending tip payment", http.StatusBadRequest) return } diff --git a/backend/handlers/payments/payments_test.go b/backend/handlers/payments/payments_test.go index 001db0d..0c663d5 100644 --- a/backend/handlers/payments/payments_test.go +++ b/backend/handlers/payments/payments_test.go @@ -1989,6 +1989,108 @@ func TestTipPayment_RetryPending_ReattemptsCharge(t *testing.T) { assert.Equal(t, "completed", status) } +// TestTipPayment_RetryPending_NonExactAmountSucceeds verifies the pending-retry +// amount guard compares pence exactly. £1.14 is stored as the float64 +// 1.1399999999999999, so a naive int64(pounds*100) truncation yields 113 and +// falsely rejects a legitimate same-amount retry of 114 pence. +func TestTipPayment_RetryPending_NonExactAmountSucceeds(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestData(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + + // Create a completed payment so the tip is allowed. + _, err := fixtures.CreateTestPayment(tx, bookingID, 5000.00, "online_square", "full", "completed") + require.NoError(t, err) + + // Create a saved card for this user. + var savedCardID string + err = tx.QueryRow(ctx, ` + INSERT INTO user_saved_cards (user_id, square_card_id, brand, last_4, exp_month, exp_year, fingerprint) + VALUES ($1, 'ccof_mock_retry', 'VISA', '1111', 12, 2030, 'sqfp_mock_retry') + RETURNING id + `, userID).Scan(&savedCardID) + require.NoError(t, err) + + // Failed prior attempt: a PENDING tip of £1.14 (114 pence) with the same key. + key := "tip-retry-key-114" + _, err = tx.Exec(ctx, ` + INSERT INTO payments (booking_id, payment_type, payment_method, status, amount, idempotency_key, created_at, updated_at, created_by) + VALUES ($1, 'tip', 'online_square', 'pending', 1.14, $2, NOW(), NOW(), $3) + `, bookingID, key, userID) + require.NoError(t, err) + + req := CreateTipPaymentRequest{ + Amount: 114, + CardID: &savedCardID, + IdempotencyKey: key, + } + + handler := CreateTipPayment + w := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/tip", req, userToken, ctx) + require.Equal(t, http.StatusOK, w.Code, "body: %s", w.Body.String()) + + var resp PaymentResponse + require.NoError(t, parsePaymentResponseBody(w, &resp)) + assert.Equal(t, "completed", resp.Status, "a same-amount retry of a non-exact pence value must re-attempt and complete") + assert.Equal(t, int64(114), resp.Amount) + + // Exactly one payment record for this key, now completed. + var count int + var status string + err = tx.QueryRow(ctx, `SELECT COUNT(*), MAX(status) FROM payments WHERE idempotency_key = $1`, key).Scan(&count, &status) + require.NoError(t, err) + assert.Equal(t, 1, count, "must reuse the pending record, not insert a duplicate") + assert.Equal(t, "completed", status) +} + +// TestTipPayment_RetryPending_AmountMismatchRejected verifies the amount guard +// still rejects a retry that changes the amount on the same idempotency key. +func TestTipPayment_RetryPending_AmountMismatchRejected(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupTestData(t, ctx, tx) + userToken := jwt.GenerateUserToken(userID) + + _, err := fixtures.CreateTestPayment(tx, bookingID, 5000.00, "online_square", "full", "completed") + require.NoError(t, err) + + var savedCardID string + err = tx.QueryRow(ctx, ` + INSERT INTO user_saved_cards (user_id, square_card_id, brand, last_4, exp_month, exp_year, fingerprint) + VALUES ($1, 'ccof_mock_retry', 'VISA', '1111', 12, 2030, 'sqfp_mock_retry') + RETURNING id + `, userID).Scan(&savedCardID) + require.NoError(t, err) + + // Failed prior attempt: PENDING tip of £10.00 (1000 pence). + key := "tip-retry-key-mismatch" + _, err = tx.Exec(ctx, ` + INSERT INTO payments (booking_id, payment_type, payment_method, status, amount, idempotency_key, created_at, updated_at, created_by) + VALUES ($1, 'tip', 'online_square', 'pending', 10.00, $2, NOW(), NOW(), $3) + `, bookingID, key, userID) + require.NoError(t, err) + + // Retry with a DIFFERENT amount but the same key → must 400, not charge. + req := CreateTipPaymentRequest{ + Amount: 2000, + CardID: &savedCardID, + IdempotencyKey: key, + } + + handler := CreateTipPayment + w := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/tip", req, userToken, ctx) + require.Equal(t, http.StatusBadRequest, w.Code, "a same-key retry with a different amount must be rejected") + + // The pending record must be untouched. + var status string + err = tx.QueryRow(ctx, `SELECT status FROM payments WHERE idempotency_key = $1`, key).Scan(&status) + require.NoError(t, err) + assert.Equal(t, "pending", status) +} + func TestTipPayment_TransactionFailure_SkipsSquare(t *testing.T) { t.Parallel() ctx, tx := testutils.SetupTestTx(t)