popertots/Crussell
1
1
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Apply second-round review fixes: idempotency-key length caps, stable-sentinel card keys, test-isolation, naming

Browse Source 
Money-safety idempotency hardening (I1, wide):
- validate:"max=45" on CreateTerminalPayment/BookingPayment/Refund/Tip/
  BuyGiftCard idempotency keys (all feed Square's 45-char /v2/payments,
  /v2/refunds, /v2/cards caps); BuyGiftCard corrected from a wrongly-loose
  max=64. Till keeps max=64 (its key also feeds the 64-char terminal-checkout
  endpoint).
- Explicit 45-char guard in RefundPayment: the one handler that decodes
  RefundRequest without running the struct validator, so the tag alone was
  inert; a longer key would 400 at Square and be misclassified as a
  definitive refund decline.
- New TestIdempotencyKey_OverLength_RejectedAcrossPaymentHandlers covers all
  six endpoints (terminal saved-card, booking, tip, gift-card, till, refund).

Stable-sentinel card identity in idempotency keys (C1, wide):
- BookingFlow deposit key now uses the 'new-card' sentinel instead of
  embedding the cnon: nonce (matches UserPaymentModal/account). A re-tokenize
  after a spent nonce no longer regenerates the key, closing a lost-response
  double-charge window.
- TipPayment + UserBookingModal tip keys now include card identity
  (selectedCardId || 'new-card'); previously keyed on amount only, so a
  same-amount tip on a DIFFERENT card reused the key and deduped a distinct
  charge. Resets cleared in every success/close path.

Test isolation (R1): TestRefund_PendingResume_NewKeyAfterModalReopen no
longer t.Parallel — it swaps the package-level SquareClient mid-test and a
concurrent parallel test could observe the swapped instance.

Naming/quality (M1/M2/M4): resolveChargeSource local renamed savedRowID (was
shadowing the cardID *string parameter); BuyGiftCard fallback prefix
"till-" -> "gc-"; saved-card terminal response key "checkout_id" -> "payment_id"
(it holds a DB payment row, not a Square checkout) with matching frontend
fallback. README maintenance-job count corrected 24 -> 25.

Full suite 25/25 + race clean via run-tests.sh lockfile; svelte-check 0
errors/warnings; production build succeeds.
This commit is contained in:
popertots
2026-08-22 00:34:49 +01:00
parent 63226debb7
commit 7f1c649f1e
11 changed files with 149 additions and 23 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/handlers/payments/charge_helpers.go
+4 -4
View File
@@ -52,11 +52,11 @@ func resolveChargeSource(ctx context.Context, w http.ResponseWriter, svc *Paymen
// record's retry re-creates it via the deterministic sha256
// idempotency key (the SAVE path), and Square returns the same card —
// deleting it would break that retry.
cardID, err := svc.SaveCardForUser(ctx, userID, sqCustomerID, cardOnFile.CardID, cardOnFile.Brand, cardOnFile.Last4, cardOnFile.ExpMonth, cardOnFile.ExpYear, cardOnFile.Fingerprint)
if err != nil {
log.Printf("Failed to save card: %v", err)
savedRowID, saveErr := svc.SaveCardForUser(ctx, userID, sqCustomerID, cardOnFile.CardID, cardOnFile.Brand, cardOnFile.Last4, cardOnFile.ExpMonth, cardOnFile.ExpYear, cardOnFile.Fingerprint)
if saveErr != nil {
log.Printf("Failed to save card: %v", saveErr)
} else {
savedCardID = &cardID
savedCardID = &savedRowID
}
} else {
// One-off new-card charge: use the cnon: nonce DIRECTLY as the
backend/handlers/payments/giftcards.go
+4 -4
View File
@@ -83,8 +83,8 @@ type BuyGiftCardRequest struct {
// IdempotencyKey is required (R2): an empty key would be stored as '' on
// the pending payments row, and a second empty-key purchase would 500 on
// the UNIQUE(payments.idempotency_key) constraint. The frontend always
// sends a per-purchase UUID; the max=64 matches the column width.
IdempotencyKey string `json:"idempotency_key" validate:"required,max=64"`
// sends a per-purchase UUID; max=45 matches Square's /v2/payments limit.
IdempotencyKey string `json:"idempotency_key" validate:"required,max=45"`
VerificationToken *string `json:"verification_token,omitempty"`
}
@@ -915,7 +915,7 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) {
return
}
// R2: idempotency_key is required (validate:"required,max=64"). An empty
// R2: idempotency_key is required (validate:"required,max=45"). An empty
// key would be stored as '' on the pending payments row and a second
// empty-key purchase would 500 on the UNIQUE(payments.idempotency_key)
// constraint. The frontend always sends a per-purchase UUID. The fallback
@@ -928,7 +928,7 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) {
return
}
if req.IdempotencyKey == "" {
req.IdempotencyKey = uniqueChargeKey("till-")
req.IdempotencyKey = uniqueChargeKey("gc-")
}
allowedAmounts := map[int64]bool{1000: true, 2000: true, 5000: true}
backend/handlers/payments/handlers.go
+17 -7
View File
@@ -42,8 +42,9 @@ type CreateTerminalPaymentRequest struct {
// one booking (e.g. a second £50 'full' charge for a second service) get
// different keys and never collapse on the deterministic fallback key.
// When absent, the handler falls back to the deterministic booking+type+
// amount+card key for no-client-key retry safety.
IdempotencyKey string `json:"idempotency_key,omitempty"`
// amount+card key for no-client-key retry safety. Cap ≤45 (Square's
// idempotency-key limit for /v2/payments — this key feeds CreatePayment).
IdempotencyKey string `json:"idempotency_key,omitempty" validate:"omitempty,max=45"`
}
type CreateBookingPaymentRequest struct {
@@ -52,7 +53,7 @@ type CreateBookingPaymentRequest struct {
CardID *string `json:"card_id,omitempty"`
NewCardToken *string `json:"new_card_token,omitempty"`
SaveCard bool `json:"save_card"`
IdempotencyKey string `json:"idempotency_key" validate:"required"`
IdempotencyKey string `json:"idempotency_key" validate:"required,max=45"`
VerificationToken *string `json:"verification_token,omitempty"`
}
@@ -63,8 +64,8 @@ type RefundRequest struct {
// same amount against the same payment must not collide on the default
// amount-derived key (the dedup lookup would swallow the second refund).
// The frontend sends a UUID generated per refund attempt and reuses it on
// retry, mirroring the tip-flow pattern.
IdempotencyKey string `json:"idempotency_key,omitempty"`
// retry, mirroring the tip-flow pattern. Cap ≤45 (Square's /v2/refunds limit).
IdempotencyKey string `json:"idempotency_key,omitempty" validate:"omitempty,max=45"`
}
type CreateTipPaymentRequest struct {
@@ -72,7 +73,7 @@ type CreateTipPaymentRequest struct {
CardID *string `json:"card_id,omitempty"`
NewCardToken *string `json:"new_card_token,omitempty"`
SaveCard bool `json:"save_card"`
IdempotencyKey string `json:"idempotency_key,omitempty"`
IdempotencyKey string `json:"idempotency_key,omitempty" validate:"omitempty,max=45"`
VerificationToken *string `json:"verification_token,omitempty"`
}
@@ -684,7 +685,7 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) {
// Return the card details the frontend reads for the success state
// (MINOR-R2) — CheckoutResponse alone leaves card_brand/card_last4 blank.
if err := json.NewEncoder(w).Encode(map[string]any{
"checkout_id": paymentID,
"payment_id": paymentID,
"status": "COMPLETED",
"card_brand": paymentResult.CardBrand,
"card_last4": paymentResult.CardLast4,
@@ -1983,6 +1984,15 @@ func RefundPayment(w http.ResponseWriter, r *http.Request) {
return
}
// Client-supplied idempotency key feeds Square /v2/refunds (45-char cap).
// This handler decodes into RefundRequest without running the struct
// validator, so enforce the limit explicitly — a longer key would 400 at
// Square and be misclassified as a definitive refund decline.
if len(req.IdempotencyKey) > 45 {
http.Error(w, "Invalid request: idempotency_key exceeds 45 characters", http.StatusBadRequest)
return
}
service := NewPaymentService()
payment, err := service.GetPaymentByID(r.Context(), paymentID)
backend/handlers/payments/payments_r6_review_test.go
+88
View File
@@ -448,3 +448,91 @@ func TestSaveCardForUser_RevivesSoftDeletedCard(t *testing.T) {
require.NoError(t, tx.QueryRow(ctx, `SELECT deleted_at FROM user_saved_cards WHERE id = $1`, revivedID).Scan(&revivedDeletedAt))
require.False(t, revivedDeletedAt.Valid, "the revived card must have deleted_at cleared")
}
// TestIdempotencyKey_OverLength_RejectedAcrossPaymentHandlers proves the
// validate:"max=45/64" caps on client-supplied idempotency keys: a key longer
// than Square's per-endpoint limit would otherwise 400 at Square (misclassified
// as a definitive 402 by chargeFailureStatus) with a confusingly worded error.
func TestIdempotencyKey_OverLength_RejectedAcrossPaymentHandlers(t *testing.T) {
ctx, tx := testutils.SetupTestTx(t)
userID, bookingID, _ := setupTestData(t, ctx, tx)
_, err := fixtures.CreateTestAdminUser(tx)
require.NoError(t, err)
userToken := jwt.GenerateUserToken(userID)
adminToken := jwt.GenerateAdminToken()
paymentID, err := fixtures.CreateTestPayment(tx, bookingID, 5000.00, "in_person_card", "full", "completed")
require.NoError(t, err)
// 46 chars — exceeds Square's 45-char /v2/payments, /v2/cards, /v2/refunds cap.
tooLong := strings.Repeat("k", 46)
// 65 chars — exceeds Square's 64-char terminal-checkout cap.
tooLongCheckout := strings.Repeat("c", 65)
cases := []struct {
name string
handler http.HandlerFunc
method string
path string
body any
token string
}{
{
name: "terminal-saved-card",
handler: CreateTerminalPayment,
method: "POST",
path: "/api/admin/bookings/" + bookingID + "/payment",
body: CreateTerminalPaymentRequest{Amount: 5000, PaymentType: "full", UserSavedCardID: strPtr("000000000001"), IdempotencyKey: tooLong},
token: adminToken,
},
{
name: "booking-payment",
handler: CreateBookingPayment,
method: "POST",
path: "/api/bookings/" + bookingID + "/payment",
body: CreateBookingPaymentRequest{Amount: 5000, PaymentType: "deposit", IdempotencyKey: tooLong},
token: userToken,
},
{
name: "tip",
handler: CreateTipPayment,
method: "POST",
path: "/api/bookings/" + bookingID + "/tip",
body: CreateTipPaymentRequest{Amount: 500, IdempotencyKey: tooLong},
token: userToken,
},
{
name: "gift-card-buy",
handler: BuyGiftCard,
method: "POST",
path: "/api/gift-cards/buy",
body: BuyGiftCardRequest{Amount: 1000, RecipientType: "self", NewCardToken: strPtr("cnon:test-card"), IdempotencyKey: tooLong},
token: userToken,
},
{
name: "till-terminal",
handler: CreateTillSale,
method: "POST",
path: "/api/admin/till/sale",
body: TillSaleRequest{ItemType: "gift_card", Action: "create", Amount: 50.00, PaymentMethod: "cash", IdempotencyKey: tooLongCheckout},
token: adminToken,
},
{
name: "refund",
handler: RefundPayment,
method: "POST",
path: "/api/admin/payments/" + paymentID + "/refund",
body: RefundRequest{Amount: 1000, Reason: "customer request", IdempotencyKey: tooLong},
token: adminToken,
},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
w := makePaymentRequest(tc.handler, tc.method, tc.path, tc.body, tc.token, ctx)
require.Equal(t, http.StatusBadRequest, w.Code,
"an over-length idempotency key must be rejected before reaching Square (got %d: %s)", w.Code, w.Body.String())
})
}
}
backend/handlers/payments/payments_test.go
+3 -1
View File
@@ -807,7 +807,9 @@ func TestRefund_TwoEqualPartialRefunds_ClientKeyDisambiguates(t *testing.T) {
// a second pending row. Without the fallback, the sweep would process both
// pending rows and move twice the intended money.
func TestRefund_PendingResume_NewKeyAfterModalReopen(t *testing.T) {
t.Parallel()
// NOT t.Parallel: it swaps the package-level SquareClient (the counting
// client below), and a concurrent parallel test reading SquareClient would
// observe the swapped instance mid-test.
ctx, tx := testutils.SetupTestTx(t)
_, bookingID, _ := setupTestData(t, ctx, tx)
backend/handlers/payments/till.go
+4 -1
View File
@@ -30,7 +30,10 @@ type TillSaleRequest struct {
PaymentMethod string `json:"payment_method" validate:"required"`
UserSavedCardID *string `json:"user_saved_card_id,omitempty"`
UserID *string `json:"user_id,omitempty"`
IdempotencyKey string `json:"idempotency_key,omitempty"`
// IdempotencyKey is optional; an empty key is replaced with a fresh
// uniqueChargeKey below. Limit 64: Square's terminal-checkout cap — this
// key feeds CreateCheckout AND the CreatePayment paths.
IdempotencyKey string `json:"idempotency_key,omitempty" validate:"omitempty,max=64"`
CardToken string `json:"card_token,omitempty"`
RedeemToUserID *string `json:"redeem_to_user_id,omitempty"`
VerificationToken *string `json:"verification_token,omitempty"`