From 515b828550caff98ac3af873b7ee05261ec774c7 Mon Sep 17 00:00:00 2001 From: Stephen Adamson Date: Mon, 3 Aug 2026 14:55:25 +0100 Subject: [PATCH] Fix gift-card, till, and refund paths: nonce-direct charges, customer_id forwarding, blocking refund lock BuyGiftCard and till online_square charge the cnon: nonce directly (no synthetic card-on-file); BuyGiftCard.IdempotencyKey is now validate-required (empty key previously collided on the UNIQUE constraint). Saved-card branches forward customer_id and lazily provision legacy cards. lockCancellationPayments uses the deliberately-blocking xact lock so a cancellation never silently drops a refund under contention (admins see the full manual refund round-trip). --- backend/handlers/payments/giftcards.go | 94 ++++++++++++++++++++------ backend/handlers/payments/refunds.go | 36 +++++++--- backend/handlers/payments/till.go | 81 +++++++++++++++------- 3 files changed, 156 insertions(+), 55 deletions(-) diff --git a/backend/handlers/payments/giftcards.go b/backend/handlers/payments/giftcards.go index e1574e7..ea9f17d 100644 --- a/backend/handlers/payments/giftcards.go +++ b/backend/handlers/payments/giftcards.go @@ -81,7 +81,11 @@ type BuyGiftCardRequest struct { CardID *string `json:"card_id,omitempty"` NewCardToken *string `json:"new_card_token,omitempty"` SaveCard bool `json:"save_card"` - IdempotencyKey string `json:"idempotency_key"` + // 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"` VerificationToken *string `json:"verification_token,omitempty"` } @@ -879,6 +883,22 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { return } + // R2: idempotency_key is required (validate:"required,max=64"). 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 + // below is defense-in-depth only — the validator rejects the empty key + // first, but if it is ever relaxed the fallback keeps the UNIQUE + // constraint from firing. + if err := validators.Validate.Struct(&req); err != nil { + log.Printf("Failed to process request: %v", err) + http.Error(w, "Invalid request", http.StatusBadRequest) + return + } + if req.IdempotencyKey == "" { + req.IdempotencyKey = uniqueTillKey() + } + allowedAmounts := map[int64]bool{1000: true, 2000: true, 5000: true} if !allowedAmounts[req.Amount] { http.Error(w, "Invalid amount. Must be £10, £20, or £50.", http.StatusBadRequest) @@ -909,6 +929,8 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { // executing the gift-card creation (2× value for 1 charge). Mirrors the tip // advisory-lock pattern (handlers.go). Lock is keyed on the idempotency key // so distinct purchases are unaffected; falls back to userID when absent. + // Bounded try-lock (R6) so a contended lock never blocks the pool across + // the Square round-trip. lockKey := req.IdempotencyKey if lockKey == "" { lockKey = userID @@ -920,13 +942,17 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { return } defer pinConn.Release() - if _, err := pinConn.Exec(ctx, ` - SELECT pg_advisory_lock(hashtext('crussell:giftcard:' || $1)) - `, lockKey); err != nil { + lockOK, err := acquireAdvisoryLock(ctx, pinConn, "crussell:giftcard:"+lockKey) + if err != nil { log.Printf("Failed to acquire gift-card serialization lock for %s: %v", lockKey, err) http.Error(w, "internal server error", http.StatusInternalServerError) return } + if !lockOK { + log.Printf("Gift-card serialization lock for %s not acquired within bound — a purchase is already in progress", lockKey) + http.Error(w, "Purchase in progress, try again", http.StatusConflict) + return + } defer func() { if _, err := pinConn.Exec(context.Background(), ` SELECT pg_advisory_unlock(hashtext('crussell:giftcard:' || $1)) @@ -976,38 +1002,44 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { var sourceID string var savedCardID *string + var savedCardCustomerID string if req.NewCardToken != nil && *req.NewCardToken != "" { - // P14: when the card is being SAVED, provision (or reuse) the user's - // Square customer profile BEFORE tokenizing so the new card is created - // against that customer. One-off non-save charges pass "" — a cnon: - // nonce charge needs no customer. - squareCustomerID := "" + // A cnon: nonce charge needs NO card-on-file and NO customer (R6). The + // old code tokenized every new card via CreateCardOnFile even for + // one-off non-save purchases, creating an orphan card at Square for a + // payment that only ever uses the nonce once. if req.SaveCard { - var custErr error - squareCustomerID, custErr = paymentService.EnsureSquareCustomer(ctx, userID) + // Save path: provision (or reuse) the user's Square customer BEFORE + // tokenizing so the new card is created against that customer, and + // forward the customer id on the charge. A ccof: source MUST carry + // its customer (R6). + squareCustomerID, custErr := paymentService.EnsureSquareCustomer(ctx, userID) if custErr != nil { log.Printf("Failed to provision Square customer for user %s: %v", userID, custErr) http.Error(w, "Failed to process card", http.StatusInternalServerError) return } - } - cardOnFile, err := SquareClient.CreateCardOnFile(ctx, userID, *req.NewCardToken, squareCustomerID) - if err != nil { - log.Printf("Failed to create card on file: %v", err) - http.Error(w, "Failed to process card", http.StatusInternalServerError) - return - } + cardOnFile, err := SquareClient.CreateCardOnFile(ctx, userID, *req.NewCardToken, squareCustomerID) + if err != nil { + log.Printf("Failed to create card on file: %v", err) + http.Error(w, "Failed to process card", http.StatusInternalServerError) + return + } + sourceID = cardOnFile.CardID + savedCardCustomerID = squareCustomerID - sourceID = cardOnFile.CardID - - if req.SaveCard { - cardID, err := paymentService.SaveCardForUser(ctx, userID, cardOnFile.CardID, cardOnFile.Brand, cardOnFile.Last4, cardOnFile.ExpMonth, cardOnFile.ExpYear, cardOnFile.Fingerprint) + cardID, err := paymentService.SaveCardForUser(ctx, userID, squareCustomerID, cardOnFile.CardID, cardOnFile.Brand, cardOnFile.Last4, cardOnFile.ExpMonth, cardOnFile.ExpYear, cardOnFile.Fingerprint) if err != nil { log.Printf("Failed to save card: %v", err) } else { savedCardID = &cardID } + } else { + // One-off new-card purchase: use the cnon: nonce DIRECTLY as the + // source. No card-on-file is created (nothing to orphan, no + // customer needed). + sourceID = *req.NewCardToken } } else if req.CardID != nil { card, err := paymentService.GetCardByID(ctx, *req.CardID, userID) @@ -1020,8 +1052,22 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { http.Error(w, "internal server error", http.StatusInternalServerError) return } + // A ccof: source can NEVER be charged without a CustomerID — Square + // rejects the payment. A saved-card row created before P14 has an empty + // square_customer_id; lazily provision the user's Square customer and + // persist it on the row BEFORE charging (R6). + if card.SquareCustomerID == "" { + provisioned, provErr := paymentService.EnsureSquareCustomerForSavedCard(ctx, *req.CardID, userID) + if provErr != nil { + log.Printf("Failed to provision Square customer for saved card %s (user %s): %v", *req.CardID, userID, provErr) + http.Error(w, "Failed to process card", http.StatusInternalServerError) + return + } + card.SquareCustomerID = provisioned + } sourceID = card.SquareCardID savedCardID = req.CardID + savedCardCustomerID = card.SquareCustomerID } amountPounds := float64(req.Amount) / 100.0 @@ -1117,6 +1163,10 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { Amount: req.Amount, Currency: "GBP", SourceID: sourceID, + // CustomerID carries the saved-card row's Square customer id on ccof: + // charges (save-card path); a cnon: nonce charge (one-off) needs none + // (R6). + CustomerID: savedCardCustomerID, IdempotencyKey: req.IdempotencyKey, Note: "Gift Card Purchase", BuyerEmail: buyerEmail, diff --git a/backend/handlers/payments/refunds.go b/backend/handlers/payments/refunds.go index a47a62e..7407898 100644 --- a/backend/handlers/payments/refunds.go +++ b/backend/handlers/payments/refunds.go @@ -106,7 +106,7 @@ func CalculateRefundForCancellation( // manual guard read precedes the cancellation's commit). Locks are acquired in // ascending payment_id order (matching processChargeGroup) to avoid deadlocks, // and only for card methods the manual handler can touch. -func lockCancellationPayments(ctx context.Context, q db.Querier, payments []paymentRow) error { +func lockCancellationPayments(ctx context.Context, tx pgx.Tx, payments []paymentRow) error { var ids []string for _, p := range payments { if p.PaymentMethod == "online_square" || p.PaymentMethod == "in_person_card" { @@ -118,7 +118,15 @@ func lockCancellationPayments(ctx context.Context, q db.Querier, payments []paym } sort.Strings(ids) for _, pid := range ids { - if _, err := q.Exec(ctx, `SELECT pg_advisory_xact_lock(hashtext('crussell:refund:' || $1))`, pid); err != nil { + // Blocking xact lock (NOT the bounded try-lock used elsewhere): this is + // the admin-only cancellation path, and the manual RefundPayment handler + // can hold the same key across its up-to-30s Square round-trip. A timed + // out acquire here would abort the cancellation transaction — the caller + // (manage.go) would commit the cancellation with ZERO refund rows and no + // sweep retry could ever recover the money. Blocking guarantees the + // refund runs; the lock auto-releases at the caller's commit/rollback. + // See acquireAdvisoryXactLockBlocking for the full rationale. + if err := acquireAdvisoryXactLockBlocking(ctx, tx, "crussell:refund:"+pid); err != nil { return fmt.Errorf("failed to acquire cancellation refund lock for payment %s: %w", pid, err) } } @@ -749,10 +757,15 @@ func processChargeGroup(ctx context.Context, chargeID string, rows []pendingChar locked := 0 for _, pid := range paymentIDs { - if _, err := pinConn.Exec(ctx, ` - SELECT pg_advisory_lock(hashtext('crussell:refund:' || $1)) - `, pid); err != nil { - log.Printf("Failed to acquire refund lock for payment %s (charge %s): %v", pid, chargeID, err) + // Bounded try-lock (R6) so the sweep never blocks a pool connection + // while the manual handler holds the same key across its Square call. + ok, lockErr := acquireAdvisoryLock(ctx, pinConn, "crussell:refund:"+pid) + if lockErr != nil { + log.Printf("Failed to acquire refund lock for payment %s (charge %s): %v", pid, chargeID, lockErr) + break + } + if !ok { + log.Printf("Refund lock for payment %s (charge %s) not acquired within bound — a refund is in progress", pid, chargeID) break } locked++ @@ -1083,11 +1096,16 @@ func processManualPaymentGroup(ctx context.Context, paymentID string, rows []man return 0, err } defer pinConn.Release() - if _, err := pinConn.Exec(ctx, ` - SELECT pg_advisory_lock(hashtext('crussell:refund:' || $1)) - `, paymentID); err != nil { + // Bounded try-lock (R6) so the sweep never blocks a pool connection while + // the manual handler holds the same key across its Square call. + lockOK, err := acquireAdvisoryLock(ctx, pinConn, "crussell:refund:"+paymentID) + if err != nil { return 0, err } + if !lockOK { + log.Printf("Refund lock for payment %s not acquired within bound — a manual refund is in progress; leaving rows pending for the next sweep", paymentID) + return 0, nil + } defer func() { if _, err := pinConn.Exec(context.Background(), ` SELECT pg_advisory_unlock(hashtext('crussell:refund:' || $1)) diff --git a/backend/handlers/payments/till.go b/backend/handlers/payments/till.go index f5983d9..cde04a7 100644 --- a/backend/handlers/payments/till.go +++ b/backend/handlers/payments/till.go @@ -247,7 +247,9 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { // constraint after the funding already committed. Mirrors the gift-card // advisory-lock pattern (giftcards.go). Lock is keyed on the idempotency // key so distinct sales are unaffected; falls back to a per-request key - // when absent (client-supplied key is always used in practice). + // when absent (client-supplied key is always used in practice). Bounded + // try-lock (R6) so a contended lock never blocks the pool across the Square + // round-trip. lockKey := req.IdempotencyKey if lockKey == "" { lockKey = "till-" + rand.Text() @@ -259,13 +261,17 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { return } defer pinConn.Release() - if _, err := pinConn.Exec(ctx, ` - SELECT pg_advisory_lock(hashtext('crussell:till:' || $1)) - `, lockKey); err != nil { + lockOK, err := acquireAdvisoryLock(ctx, pinConn, "crussell:till:"+lockKey) + if err != nil { log.Printf("Failed to acquire till-sale serialization lock for %s: %v", lockKey, err) http.Error(w, "internal server error", http.StatusInternalServerError) return } + if !lockOK { + log.Printf("Till-sale serialization lock for %s not acquired within bound — a sale is already in progress", lockKey) + http.Error(w, "Sale in progress, try again", http.StatusConflict) + return + } defer func() { if _, err := pinConn.Exec(context.Background(), ` SELECT pg_advisory_unlock(hashtext('crussell:till:' || $1)) @@ -531,17 +537,45 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { } } + // The till path is not user-scoped, so fetch the card's owner along with + // the charge details — the owner is needed to lazily provision a Square + // customer if the row predates P14 (R6). + var cardUserID sql.NullString err = tx.QueryRow(ctx, ` - SELECT square_card_id, COALESCE(square_customer_id, '') + SELECT user_id, COALESCE(square_card_id, ''), COALESCE(square_customer_id, '') FROM user_saved_cards WHERE id = $1 AND deleted_at IS NULL - `, *req.UserSavedCardID).Scan(&savedCardSqCardID, &savedCardCustomerID) + `, *req.UserSavedCardID).Scan(&cardUserID, &savedCardSqCardID, &savedCardCustomerID) if err != nil { log.Printf("Failed to get saved card details: %v", err) http.Error(w, "Card not found", http.StatusNotFound) return } + // A ccof: source can NEVER be charged without a CustomerID — Square + // rejects the payment. Legacy pre-P14 rows have an empty + // square_customer_id; provision + persist for the card's owner BEFORE + // charging (R6). + if savedCardCustomerID == "" { + if !cardUserID.Valid { + http.Error(w, "Saved card has no owner and cannot be charged", http.StatusBadRequest) + return + } + provisioned, provErr := service.EnsureSquareCustomer(ctx, cardUserID.String) + if provErr != nil { + log.Printf("Failed to provision Square customer for saved card %s (user %s): %v", *req.UserSavedCardID, cardUserID.String, provErr) + http.Error(w, "Failed to process card", http.StatusInternalServerError) + return + } + savedCardCustomerID = provisioned + if _, upErr := db.Conn.Exec(ctx, ` + UPDATE user_saved_cards SET square_customer_id = $1 + WHERE id = $2 + `, provisioned, *req.UserSavedCardID); upErr != nil { + log.Printf("Failed to persist Square customer id on saved card %s (non-fatal): %v", *req.UserSavedCardID, upErr) + } + } + if req.IdempotencyKey == "" { req.IdempotencyKey = uniqueTillKey() } @@ -697,29 +731,23 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { paymentResult, squareErr = SquareClient.CreatePayment(ctx, paymentReq) } else if req.PaymentMethod == "online_square" { // PCI-DSS: raw PANs are never accepted. The admin till must supply a - // Square Web Payments nonce (cnon:xxx), tokenized via the Cards API. + // Square Web Payments nonce (cnon:xxx). 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 } - // Tokenize the till card. The reference_id is the synthetic - // "till-" namespace, NOT a real user — this card is - // ephemeral (used once for this charge) and is never stored in - // user_saved_cards or re-listed. The prefix can't collide with a - // real CHAR(12)-hex user ID. No Square customer is provisioned for - // it ("" as the customerID): a cnon: nonce charge needs none. - cardOnFile, cardErr := SquareClient.CreateCardOnFile(ctx, "till-"+giftCardID, req.CardToken, "") - if cardErr != nil { - log.Printf("Failed to tokenize card: %v", cardErr) - http.Error(w, "Card tokenization failed", http.StatusInternalServerError) - return - } - + // Charge the cnon: nonce DIRECTLY (R6). The old code tokenized the + // till card via CreateCardOnFile under a synthetic "till-" + // reference and charged the resulting ccof: id — but that card was + // never saved or re-listed anywhere, so the CreateCardOnFile call was + // pure overhead AND would have charged a card-on-file source without a + // customer (Square rejects ccof without CustomerID). A cnon: nonce + // needs neither card-on-file nor customer. paymentReq := square.CreatePaymentReq{ Amount: penceAmount, Currency: "GBP", - SourceID: cardOnFile.CardID, + SourceID: req.CardToken, IdempotencyKey: req.IdempotencyKey, Note: "Gift Card " + req.Action, BuyerEmail: buyerEmail, @@ -854,6 +882,7 @@ func GetTillCheckoutStatus(w http.ResponseWriter, r *http.Request) { // path uses. Two concurrent polls of the same checkout could both run // the UPDATE + VAT (idempotent today, but a double-apply is a latent // bug). Lock on the till-sale id so only one goroutine completes it. + // Bounded try-lock (R6) so a contended lock never blocks the pool. pinConn, err := db.Conn.Acquire(r.Context()) if err != nil { log.Printf("Failed to acquire connection for till-completion lock: %v", err) @@ -861,13 +890,17 @@ func GetTillCheckoutStatus(w http.ResponseWriter, r *http.Request) { return } defer pinConn.Release() - if _, err := pinConn.Exec(r.Context(), ` - SELECT pg_advisory_lock(hashtext('crussell:tillcomplete:' || $1)) - `, tillSaleID); err != nil { + lockOK, err := acquireAdvisoryLock(r.Context(), pinConn, "crussell:tillcomplete:"+tillSaleID) + if err != nil { log.Printf("Failed to acquire till-completion serialization lock for %s: %v", tillSaleID, err) http.Error(w, "internal server error", http.StatusInternalServerError) return } + if !lockOK { + log.Printf("Till-completion serialization lock for %s not acquired within bound — a poll is already completing this sale", tillSaleID) + http.Error(w, "Payment in progress, try again", http.StatusConflict) + return + } defer func() { if _, err := pinConn.Exec(context.Background(), ` SELECT pg_advisory_unlock(hashtext('crussell:tillcomplete:' || $1))