diff --git a/backend/handlers/payments/charge_helpers.go b/backend/handlers/payments/charge_helpers.go new file mode 100644 index 0000000..635a8be --- /dev/null +++ b/backend/handlers/payments/charge_helpers.go @@ -0,0 +1,167 @@ +package payments + +import ( + "context" + "errors" + "log" + "net/http" + + "crussell/db" + + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgxpool" +) + +// resolveChargeSource resolves the Square payment source for a card charge, +// shared by CreateBookingPayment, CreateTipPayment, BuyGiftCard, and the +// CreateTerminalPayment saved-card branch (which passes the booking user's id +// and a nil new-card token). +// +// New-card path (cnon: nonce): the nonce is used DIRECTLY for one-off charges +// (no card-on-file is created — the old tokenize-then-charge flow left orphan +// cards at Square). When saveCard is true the user's Square customer is +// provisioned FIRST and the card is tokenized against it (a ccof: source MUST +// carry its customer — R6), then saved via SaveCardForUser. +// +// Saved-card path (ccof:): a saved-card row predating P14 has an empty +// square_customer_id; the user's Square customer is lazily provisioned and +// persisted on the row BEFORE charging (a ccof: source can never be charged +// without a CustomerID). +// +// On any error the helper writes the HTTP response and returns ok=false — the +// caller must return immediately. +func resolveChargeSource(ctx context.Context, w http.ResponseWriter, svc *PaymentService, userID string, newCardToken, cardID *string, saveCard bool, notFoundMsg string) (sourceID string, savedCardID *string, squareCustomerID string, ok bool) { + if newCardToken != nil && *newCardToken != "" { + if saveCard { + sqCustomerID, custErr := svc.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 "", nil, "", false + } + cardOnFile, err := SquareClient.CreateCardOnFile(ctx, userID, *newCardToken, sqCustomerID) + if err != nil { + log.Printf("Failed to create card on file: %v", err) + http.Error(w, "Failed to process card", http.StatusInternalServerError) + return "", nil, "", false + } + sourceID = cardOnFile.CardID + squareCustomerID = sqCustomerID + // CreateCardOnFile runs before the charge. If the subsequent payment + // fails, this card-on-file is intentionally NOT deleted: the pending + // 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) + } else { + savedCardID = &cardID + } + } else { + // One-off new-card charge: use the cnon: nonce DIRECTLY as the + // source. No card-on-file is created (nothing to orphan, no + // customer needed). + sourceID = *newCardToken + } + if savedCardID == nil && saveCard { + log.Printf("Card was not saved despite save_card=true for user %s", userID) + } + return sourceID, savedCardID, squareCustomerID, true + } + if cardID != nil { + card, err := svc.GetCardByID(ctx, *cardID, userID) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + http.Error(w, notFoundMsg, http.StatusNotFound) + return "", nil, "", false + } + log.Printf("Failed to get card: %v", err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return "", nil, "", false + } + if card.SquareCustomerID == "" { + if userID == "" { + // Defensive parity with the original saved-card block: a card + // with no bookable owner cannot be provisioned. Unreachable in + // practice — GetCardByID above filters on user_id and would + // have 404'd for an empty owner. + http.Error(w, "Saved card has no owner and cannot be charged", http.StatusBadRequest) + return "", nil, "", false + } + provisioned, provErr := svc.EnsureSquareCustomerForSavedCard(ctx, *cardID, userID) + if provErr != nil { + log.Printf("Failed to provision Square customer for saved card %s (user %s): %v", *cardID, userID, provErr) + http.Error(w, "Failed to process card", http.StatusInternalServerError) + return "", nil, "", false + } + card.SquareCustomerID = provisioned + } + return card.SquareCardID, cardID, card.SquareCustomerID, true + } + // Neither a new-card token nor a saved card — validation upstream + // (ValidateCardInfo) guarantees one of them is present. + return "", nil, "", false +} + +// acquireBookingPaymentLock acquires a pinned pool connection and a bounded +// try-lock (R6) on lockKey, serializing payment attempts per booking (the core +// defence against the two-tab double-payment race). A blocking pg_advisory_lock +// would hold the pinned pool connection for the full Square round-trip of +// whichever request holds the lock; the bounded try-lock loop gives up after +// ~3s and surfaces a 409 instead of exhausting the pool. On any failure the +// helper writes the HTTP response and returns ok=false — the caller must +// return. On success the caller MUST defer releaseBookingPaymentLock(pinConn, +// lockKey): the lock and connection stay held for the whole handler so +// pg_advisory_unlock runs on the SAME session that acquired the lock. +func acquireBookingPaymentLock(ctx context.Context, w http.ResponseWriter, lockKey, conflictMsg string) (*pgxpool.Conn, bool) { + pinConn, err := db.Conn.Acquire(ctx) + if err != nil { + log.Printf("Failed to acquire connection for payment lock (%s): %v", lockKey, err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return nil, false + } + lockOK, err := acquireAdvisoryLock(ctx, pinConn, lockKey) + if err != nil { + pinConn.Release() + log.Printf("Failed to acquire payment serialization lock %s: %v", lockKey, err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return nil, false + } + if !lockOK { + pinConn.Release() + log.Printf("Payment serialization lock %s not acquired within bound — a payment is already in progress", lockKey) + http.Error(w, conflictMsg, http.StatusConflict) + return nil, false + } + return pinConn, true +} + +// releaseBookingPaymentLock releases the advisory lock acquired by +// acquireBookingPaymentLock and returns the pinned connection to the pool. +// Both run on the same session that holds the lock. +func releaseBookingPaymentLock(pinConn *pgxpool.Conn, lockKey string) { + if _, err := pinConn.Exec(context.Background(), ` + SELECT pg_advisory_unlock(hashtext($1)) + `, lockKey); err != nil { + log.Printf("Failed to release payment serialization lock %s: %v", lockKey, err) + } + pinConn.Release() +} + +// recheckBookingPayable re-reads the booking status after a Square charge +// succeeded (R9): a concurrent cancellation/eviction can move the booking out +// of a payable state between the pre-charge status check and the charge +// completing. A charge landing on a cancelled/lapsed/no-show booking must NOT +// be recorded as a completed payment — the cancellation refund path computes +// refunds from completed payments and would silently exclude it. Returns the +// re-read status and whether a completed payment is still allowed; the caller +// owns the CRITICAL logging, the mark-failed write (whose target and +// transaction semantics differ per path), and the 409 conflict response. +func recheckBookingPayable(ctx context.Context, q db.Querier, bookingID string) (string, bool, error) { + var status string + if err := q.QueryRow(ctx, `SELECT status FROM bookings WHERE id = $1`, bookingID).Scan(&status); err != nil { + return "", false, err + } + return status, bookingStatusAllowsCompletedPayment(status), nil +} diff --git a/backend/handlers/payments/errors.go b/backend/handlers/payments/errors.go new file mode 100644 index 0000000..ba6af95 --- /dev/null +++ b/backend/handlers/payments/errors.go @@ -0,0 +1,54 @@ +package payments + +import ( + "context" + "errors" + "net/http" + + "crussell/internal/square" +) + +// chargeFailureStatus classifies a SquareClient.CreatePayment error into the +// HTTP status a payment handler should return: +// +// - 503 (Service Unavailable) for AMBIGUOUS failures: transport/network +// errors, Square 5xx responses, context cancellation/deadline, and the +// retryable 4xx statuses 429 (rate limited), 408 (request timeout), and +// 425 (too early) — the money state at Square is unknown, so the frontend +// should treat it as a retry (the pending record is resumed on a same-key +// retry). Square's own docs treat 429 as "retry later"; mapping it (or a +// timeout/early request) to 402 would mislabel a retryable condition as a +// permanent decline. +// - 402 (Payment Required) for DEFINITIVE declines: a structured Square +// error (squareAPIError) carrying any OTHER 4xx status (400/402/422 etc.) +// means Square positively rejected the charge (card declined/expired, +// AVS/CVV failure) — retrying with the same inputs cannot succeed. +// +// A nil error is never expected (callers only invoke this on the error path); +// it maps to 402 defensively. The dev mock returns plain errors for simulated +// failures, which classify as 503 (ambiguous) — correct for a mock standing in +// for an unreachable Square. +func chargeFailureStatus(err error) int { + if err == nil { + return http.StatusPaymentRequired + } + if errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled) { + return http.StatusServiceUnavailable + } + status := square.ErrorStatusCode(err) + if status == 0 || status >= 500 { + return http.StatusServiceUnavailable + } + // Retryable/ambiguous 4xx carve-outs: 429 (RATE_LIMITED), 408 (request + // timeout), and 425 (too early) are not definitive declines — Square's + // docs tell clients to retry later. Classify them as 503 so the pending + // record stays resumable on a same-key retry instead of being labelled a + // permanent decline. True declines (400/402/422 etc.) fall through to 402. + if status == http.StatusTooManyRequests || status == http.StatusRequestTimeout || status == http.StatusTooEarly { + return http.StatusServiceUnavailable + } + if status >= 400 && status < 500 { + return http.StatusPaymentRequired + } + return http.StatusServiceUnavailable +} diff --git a/backend/handlers/payments/handlers.go b/backend/handlers/payments/handlers.go index cc7a23f..a5fab2f 100644 --- a/backend/handlers/payments/handlers.go +++ b/backend/handlers/payments/handlers.go @@ -288,7 +288,7 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { // 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() + idempotencyKey := uniqueChargeKey("tip-") // Route based on payment method if req.PaymentMethod != nil && (*req.PaymentMethod == "cash" || *req.PaymentMethod == "giftcard") { @@ -514,34 +514,13 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { http.Error(w, "internal server error", http.StatusInternalServerError) return } - card, err := service.GetCardByID(r.Context(), *req.UserSavedCardID, bookingUserID.String) - if err != nil { - if errors.Is(err, pgx.ErrNoRows) { - http.Error(w, "Saved card not found", http.StatusNotFound) - return - } - log.Printf("Failed to get saved card: %v", err) - http.Error(w, "internal server error", http.StatusInternalServerError) + // Resolve the saved-card Square source for the booking's user (the + // card's owner, not the admin) — shared new-card-vs-saved-card + // resolution, see resolveChargeSource for the R6 rationale. + sourceID, _, savedCardCustomerID, sourceOK := resolveChargeSource(r.Context(), w, service, bookingUserID.String, nil, req.UserSavedCardID, false, "Saved card not found") + if !sourceOK { 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 booking user's Square - // customer and persist it on the row BEFORE charging (R6). A card with - // no bookable owner cannot be provisioned — refuse the charge. - if card.SquareCustomerID == "" { - if !bookingUserID.Valid { - http.Error(w, "Saved card has no owner and cannot be charged", http.StatusBadRequest) - return - } - provisioned, provErr := service.EnsureSquareCustomerForSavedCard(r.Context(), *req.UserSavedCardID, bookingUserID.String) - if provErr != nil { - log.Printf("Failed to provision Square customer for saved card %s (user %s): %v", *req.UserSavedCardID, bookingUserID.String, provErr) - http.Error(w, "Failed to process card", http.StatusInternalServerError) - return - } - card.SquareCustomerID = provisioned - } // Serialize saved-card charges per booking (same lock as online booking // payments) so concurrent double-clicks can't both pass the idempotency @@ -550,31 +529,11 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { // would hold the pinned pool connection for the full Square round-trip of // whichever request holds the lock, and ~4 concurrent same-booking // requests would exhaust the whole pool. - pinConn, err := db.Conn.Acquire(r.Context()) - if err != nil { - log.Printf("Failed to acquire connection for saved-card lock: %v", err) - http.Error(w, "internal server error", http.StatusInternalServerError) - return - } - defer pinConn.Release() - lockOK, err := acquireAdvisoryLock(r.Context(), pinConn, "crussell:payment:"+bookingID) - if err != nil { - log.Printf("Failed to acquire saved-card serialization lock for %s: %v", bookingID, err) - http.Error(w, "internal server error", http.StatusInternalServerError) - return - } + pinConn, lockOK := acquireBookingPaymentLock(r.Context(), w, "crussell:payment:"+bookingID, "Payment in progress, try again") if !lockOK { - log.Printf("Saved-card serialization lock for %s not acquired within bound — a payment is in progress", bookingID) - 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:payment:' || $1)) - `, bookingID); err != nil { - log.Printf("Failed to release saved-card serialization lock for %s: %v", bookingID, err) - } - }() + defer releaseBookingPaymentLock(pinConn, "crussell:payment:"+bookingID) // Deterministic idempotency key: booking+type+amount+card. A network // retry with the same inputs derives the same key → dedup, never a @@ -666,8 +625,8 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { paymentResult, err := SquareClient.CreatePayment(r.Context(), square.CreatePaymentReq{ Amount: amount, Currency: "GBP", - SourceID: card.SquareCardID, - CustomerID: card.SquareCustomerID, + SourceID: sourceID, + CustomerID: savedCardCustomerID, IdempotencyKey: scKey, ReferenceID: bookingID, Note: req.PaymentType, @@ -675,7 +634,7 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { }) if err != nil { log.Printf("Failed to process saved-card payment: %v", err) - http.Error(w, "Payment failed", http.StatusPaymentRequired) + http.Error(w, "Payment failed", chargeFailureStatus(err)) return } @@ -687,14 +646,14 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { // completed (the cancellation refund path computes refunds from // completed payments). Mark the row failed and alert ops: money was // taken at Square and MUST be refunded manually. - var recheckStatus string - if err := db.Conn.QueryRow(r.Context(), `SELECT status FROM bookings WHERE id = $1`, bookingID).Scan(&recheckStatus); err != nil { + recheckStatus, payable, err := recheckBookingPayable(r.Context(), db.Conn, bookingID) + if err != nil { log.Printf("CRITICAL: Square payment %s was processed for booking %s but re-reading booking status failed: %v — manual reconciliation required", paymentResult.SquarePayID, bookingID, err) http.Error(w, "internal server error", http.StatusInternalServerError) return } - if !bookingStatusAllowsCompletedPayment(recheckStatus) { + if !payable { log.Printf("CRITICAL: Square payment %s was processed but booking %s is now %q — marking saved-card payment %s failed; money taken at Square MUST be refunded manually", paymentResult.SquarePayID, bookingID, recheckStatus, paymentID) if _, upErr := db.Conn.Exec(r.Context(), `UPDATE payments SET status = 'failed' WHERE id = $1`, paymentID); upErr != nil { @@ -751,31 +710,11 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { // second live Square checkout for the same booking while the first is in // flight. Bounded try-lock (R6) so a contended lock never blocks the pool // across the Square round-trip. - pinConn, err := db.Conn.Acquire(r.Context()) - if err != nil { - log.Printf("Failed to acquire connection for terminal checkout lock: %v", err) - http.Error(w, "internal server error", http.StatusInternalServerError) - return - } - defer pinConn.Release() - lockOK, err := acquireAdvisoryLock(r.Context(), pinConn, "crussell:payment:"+bookingID) - if err != nil { - log.Printf("Failed to acquire terminal checkout serialization lock for %s: %v", bookingID, err) - http.Error(w, "internal server error", http.StatusInternalServerError) - return - } + pinConn, lockOK := acquireBookingPaymentLock(r.Context(), w, "crussell:payment:"+bookingID, "Payment in progress, try again") if !lockOK { - log.Printf("Terminal checkout serialization lock for %s not acquired within bound — a checkout is in progress", bookingID) - 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:payment:' || $1)) - `, bookingID); err != nil { - log.Printf("Failed to release terminal checkout serialization lock for %s: %v", bookingID, err) - } - }() + defer releaseBookingPaymentLock(pinConn, "crussell:payment:"+bookingID) if existing := activeTerminalCheckoutID(r.Context(), bookingID); existing != "" { if err := json.NewEncoder(w).Encode(CheckoutResponse{ @@ -997,9 +936,11 @@ func GetCheckoutStatus(w http.ResponseWriter, r *http.Request) { // CreateTerminalPayment sets reference_id = bookingID; without this check, // polling the wrong checkout ID would attach its payment to a different // booking (admin-only route, but a mis-scoped charge is a data-integrity - // bug worth rejecting). - if paymentResult.ReferenceID != "" && paymentResult.ReferenceID != bookingID { - log.Printf("Checkout %s references booking %s, not %s — refusing to record", checkoutID, paymentResult.ReferenceID, bookingID) + // bug worth rejecting). An EMPTY reference_id is also rejected: a checkout + // created outside this app with no reference must not be attachable to a + // booking (S-1) — fail closed on anything that is not exactly this booking. + if paymentResult.ReferenceID == "" || paymentResult.ReferenceID != bookingID { + log.Printf("Checkout %s does not reference booking %s (reference_id=%q) — refusing to record", checkoutID, bookingID, paymentResult.ReferenceID) http.Error(w, "Checkout does not belong to this booking", http.StatusBadRequest) return } @@ -1176,7 +1117,12 @@ func GetCheckoutStatus(w http.ResponseWriter, r *http.Request) { return } - http.Error(w, "Payment failed", http.StatusPaymentRequired) + // No non-COMPLETED fallthrough here: GetCheckout (via getCheckoutHTTP) + // only returns a nil error for a COMPLETED checkout — a non-COMPLETED + // status or an expired/cancelled checkout surfaces as an error, which was + // already handled above (ErrCheckoutPending → PENDING, everything else → + // 500). The previous trailing `http.Error(w, "Payment failed", 402)` was + // unreachable dead code and has been removed. } // IsValidBookingStatusForPayment returns true if the booking status allows @@ -1310,32 +1256,11 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { // blocking pg_advisory_lock. A blocking lock would pin the pool connection // for the whole Square round-trip (~30s), so ~4 concurrent same-booking // payments would exhaust the default pool and hang every request. - pinConn, err := db.Conn.Acquire(r.Context()) - if err != nil { - log.Printf("Failed to acquire connection for payment lock: %v", err) - http.Error(w, "Internal server error", http.StatusInternalServerError) - return - } - defer pinConn.Release() - - lockOK, err := acquireAdvisoryLock(r.Context(), pinConn, "crussell:payment:"+bookingID) - if err != nil { - log.Printf("Failed to acquire payment serialization lock for %s: %v", bookingID, err) - http.Error(w, "Internal server error", http.StatusInternalServerError) - return - } + pinConn, lockOK := acquireBookingPaymentLock(r.Context(), w, "crussell:payment:"+bookingID, "Payment in progress, try again") if !lockOK { - log.Printf("Payment serialization lock for %s not acquired within bound — a payment is already in progress", bookingID) - 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:payment:' || $1)) - `, bookingID); err != nil { - log.Printf("Failed to release payment serialization lock for %s: %v", bookingID, err) - } - }() + defer releaseBookingPaymentLock(pinConn, "crussell:payment:"+bookingID) // Now that we hold the serialization lock, begin a transaction and re-check // the booking status inside it. If another request (e.g. from a different tab) @@ -1461,85 +1386,12 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { var sourceID string var savedCardID *string var savedCardCustomerID string - - if req.NewCardToken != nil && *req.NewCardToken != "" { - // 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 charges, which (a) created an orphan card at Square - // for a payment that only ever uses the nonce once, and (b) would have - // charged the resulting ccof: source without a CustomerID — Square - // rejects a card-on-file source that carries no customer. - if req.SaveCard { - // 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) — the charge below sets - // CustomerID = savedCardCustomerID = squareCustomerID. - squareCustomerID, custErr := service.EnsureSquareCustomer(r.Context(), 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(r.Context(), 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 - - // CreateCardOnFile runs before the charge. If the subsequent payment - // fails, this card-on-file is intentionally NOT deleted: the pending - // 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 := service.SaveCardForUser(r.Context(), 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 charge: use the cnon: nonce DIRECTLY as the - // source. No card-on-file is created (nothing to orphan, no - // customer needed) — this also eliminates the orphan-card - // accumulation the old non-save tokenize-then-charge flow left - // behind at Square. - sourceID = *req.NewCardToken - } - if savedCardID == nil && req.SaveCard { - log.Printf("Card was not saved despite save_card=true for user %s", userID) - } - } else if req.CardID != nil { - card, err := service.GetCardByID(r.Context(), *req.CardID, userID) - if err != nil { - if errors.Is(err, pgx.ErrNoRows) { - http.Error(w, "Card not found", http.StatusNotFound) - return - } - log.Printf("Failed to get card: %v", err) - 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). Provisioning failure - // aborts the charge with a 500. - if card.SquareCustomerID == "" { - provisioned, provErr := service.EnsureSquareCustomerForSavedCard(r.Context(), *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 + // Resolve the new-card-vs-saved-card Square source (shared with + // CreateTipPayment, BuyGiftCard, and the saved-card branch of + // CreateTerminalPayment — see resolveChargeSource for the R6 rationale). + sourceID, savedCardID, savedCardCustomerID, sourceOK := resolveChargeSource(r.Context(), w, service, userID, req.NewCardToken, req.CardID, req.SaveCard, "Card not found") + if !sourceOK { + return } // If there is no pending record to reuse, insert one NOW and commit the @@ -1609,7 +1461,7 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { paymentResult, err := SquareClient.CreatePayment(r.Context(), paymentReq) if err != nil { log.Printf("Failed to create payment: %v", err) - http.Error(w, "Payment failed", http.StatusPaymentRequired) + http.Error(w, "Payment failed", chargeFailureStatus(err)) return } @@ -1643,14 +1495,14 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { // manually. The pending row is marked 'failed' in the tx below, so // idempotency dedup still blocks a second Square charge, but the row no // longer shows pending — the frontend's retry gets a 409 Conflict. - var recheckStatus string - if err := tx2.QueryRow(r.Context(), `SELECT status FROM bookings WHERE id = $1`, bookingID).Scan(&recheckStatus); err != nil { + recheckStatus, payable, err := recheckBookingPayable(r.Context(), tx2, bookingID) + if err != nil { log.Printf("CRITICAL: Square payment %s (ID=%s) was processed but re-reading booking %s status failed: %v — manual reconciliation required", paymentResult.Status, paymentResult.SquarePayID, bookingID, err) http.Error(w, "internal server error", http.StatusInternalServerError) return } - if !bookingStatusAllowsCompletedPayment(recheckStatus) { + if !payable { log.Printf("CRITICAL: Square payment %s (ID=%s) for booking %s was processed but booking is now %q — marking payment failed; money taken at Square MUST be refunded manually", paymentResult.Status, paymentResult.SquarePayID, bookingID, recheckStatus) if _, upErr := tx2.Exec(r.Context(), `UPDATE payments SET status = 'failed' WHERE id = $1`, paymentID); upErr != nil { @@ -1838,6 +1690,24 @@ func applyEligibleCampaignsAtPayment(ctx context.Context, q db.Querier, bookingI // The primary record carries the Square payment ID for refund routing; split // records share the same SquarePaymentID so the refund loop can avoid duplicate // Square API calls while still creating audit records. +// +// MONEY INVARIANT (deliberately kept exact): the returned records always +// partition paymentAmount — deposit + balance + tip === paymentAmount exactly +// (every component is rounded to the cent and the parts are derived from one +// another, so no rounding residue exists). The sum of the split records can +// therefore never exceed the amount actually charged at Square. When deposit +// AND balance are both zero (booking already fully paid) the tip record alone +// carries the whole payment — the primary must NOT be appended as well, or the +// amount would be recorded twice (see the tip block below). +// +// Discounts do NOT change this: a discount is applied at payment time as a +// SEPARATE ledger payment row (payment_method='discount'), and GetBookingPaymentInfo +// excludes those rows from TotalPaid (as do the refund and deposit-threshold +// computations). buildSplitRecords therefore runs against the full booking +// total and the REAL money already paid, so a discounted booking can at worst +// over-allocate toward balance and under-allocate toward tip (a bookkeeping +// simplification, not an overcharge) — the partition still equals the charged +// amount. See TestBuildSplitRecords_DiscountBooking_TipOverflow_SumNeverExceedsCharge. func buildSplitRecords(primary PaymentRecord, reqPaymentType string, info *BookingPaymentInfo, paymentAmount float64) []PaymentRecord { // After the booking starts there is no deposit protection window — // record the payment as a single entry with its original type. @@ -1896,14 +1766,13 @@ func buildSplitRecords(primary PaymentRecord, reqPaymentType string, info *Booki splitIdx++ } - // If neither deposit nor balance was created (deposit exhausted, booking - // fully paid), the primary is still a valid record — use it directly. - if len(records) == 0 { - primary.Fees = 0 - records = append(records, primary) - } - - // Tip record — overflow beyond the booking total. + // 3. Tip record — overflow beyond the booking total. Appended BEFORE the + // primary fallback below: when BOTH the deposit and balance portions are + // zero (deposit room exhausted AND the booking already fully paid — e.g. a + // discounted booking whose TotalPaid, which excludes discount rows, has + // reached the full total), the tip record carries the ENTIRE payment. + // Appending the primary first would double-count the charged amount + // (primary at the full amount + tip at the same full amount). if tipPortion > 0.004 { tip := primary tip.PaymentType = "tip" @@ -1917,10 +1786,12 @@ func buildSplitRecords(primary PaymentRecord, reqPaymentType string, info *Booki records = append(records, tip) } - // If nothing was appended (shouldn't happen given validation upstream), - // return the primary as a fallback. + // Defensive fallback: nothing was appended (deposit, balance, AND tip all + // zero — impossible given paymentAmount is validated > 0 upstream, so this + // is a pure safety net). The primary is still a valid single record. if len(records) == 0 { - return []PaymentRecord{primary} + primary.Fees = 0 + records = append(records, primary) } return records } @@ -2105,16 +1976,22 @@ func RefundPayment(w http.ResponseWriter, r *http.Request) { return } - // Deterministic idempotency key so a same-key retry (network timeout) - // does not create a second Square refund. When the client supplies an - // idempotency key (one per distinct refund attempt, reused on retry), use - // it — the amount-derived fallback would collide on two DISTINCT partial - // refunds of the same amount, silently swallowing the second. The client - // key is hashed+truncated: Square's idempotency-key limit is 45 chars, and + // Idempotency key for the refund. When the client supplies one (a UUID + // generated per distinct refund attempt and REUSED on retry), the key is + // hashed and truncated: Square's idempotency-key limit is 45 chars, and // paymentID (12) + "-refund-" (8) + a full 36-char UUID (56 total) would // be rejected with a 400. The hash stays deterministic, so a same-key // retry still dedups. - idempotencyKey := paymentID + "-refund-" + strconv.FormatInt(req.Amount, 10) + // + // When the client sends NO key, the fallback must be UNIQUE per refund + // attempt: the old amount-derived key (paymentID + "-refund-" + amount) + // collided on two DISTINCT partial refunds of the same amount, and the + // dedup lookup silently swallowed the second. The fallback appends a fresh + // crypto/rand hex suffix so distinct same-amount refunds never collide; + // a lost-response no-key retry still resumes via the (payment_id, amount) + // pending fallback below. 6 random bytes (12 hex chars) keeps the full key + // ≤45 chars even for a 9-digit pence amount. + idempotencyKey := paymentID + "-refund-" + strconv.FormatInt(req.Amount, 10) + "-" + randomHexSuffix(6) if req.IdempotencyKey != "" { ikHash := sha256.Sum256([]byte(req.IdempotencyKey)) idempotencyKey = paymentID + "-refund-" + fmt.Sprintf("%x", ikHash)[:24] @@ -2227,7 +2104,7 @@ func RefundPayment(w http.ResponseWriter, r *http.Request) { reissueReq := square.RefundPaymentReq{ PaymentID: refundSqPaymentID, Amount: resumeAmount, - IdempotencyKey: existingRefundKey.String, + IdempotencyKey: refundResumeKey(paymentID, resumeAmount, existingRefundKey.String), Reason: existingRefundReason.String, } reissueResult, reissueErr := SquareClient.RefundPayment(r.Context(), reissueReq) @@ -2512,12 +2389,28 @@ func RefundPayment(w http.ResponseWriter, r *http.Request) { // pending fallback. Using the stored key lets Square return the original // refund if the prior attempt actually completed (response loss), so no second // refund can ever be issued for a row whose money state is unknown. +// refundResumeKey returns the row's OWN stored idempotency key when present, or +// a fresh deterministic fallback when the row predates keyed refunds (legacy +// NULL idempotency_key). Square's RefundPayment REQUIRES a non-empty +// idempotency key — re-issuing with "" returns a 400 INVALID_REQUEST_ERROR, +// which classifies as ambiguous and leaves the refund pending forever. The +// fallback mirrors the no-client-key shape (paymentID + "-refund-" + amount + +// "-" + hex) and stays ≤45 chars (12 + 8 + up-to-9 + 1 + 12 ≈ 42), so the +// re-issue is never rejected for length either. A legit stored key is ALWAYS +// reused so Square's same-key dedup keeps returning the original refund. +func refundResumeKey(paymentID string, amount int64, storedKey string) string { + if storedKey != "" { + return storedKey + } + return paymentID + "-refund-" + strconv.FormatInt(amount, 10) + "-" + randomHexSuffix(6) +} + func resumeManualPendingRefund(w http.ResponseWriter, r *http.Request, paymentID string, payment *PaymentRecord, refundID string, refundAmount float64, refundReason, refundKey string) { resumeAmount := int64(math.Round(refundAmount * 100)) resumeReq := square.RefundPaymentReq{ PaymentID: *payment.SquarePaymentID, Amount: resumeAmount, - IdempotencyKey: refundKey, + IdempotencyKey: refundResumeKey(paymentID, resumeAmount, refundKey), Reason: refundReason, } resumeResult, resumeErr := SquareClient.RefundPayment(r.Context(), resumeReq) @@ -2685,122 +2578,29 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { // request fields alone (bookingID + amount would dedupe distinct tips). idempotencyKey := req.IdempotencyKey if idempotencyKey == "" { - idempotencyKey = uniqueTipKey() + idempotencyKey = uniqueChargeKey("tip-") } - // Resolve the card source ID — same pattern as CreateBookingPayment. + // Resolve the card source ID — same pattern as CreateBookingPayment (see + // resolveChargeSource for the R6 rationale). var sourceID string var savedCardID *string var savedCardCustomerID string - - if req.NewCardToken != nil && *req.NewCardToken != "" { - // 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 charges, which (a) created an orphan card at Square - // for a payment that only ever uses the nonce once, and (b) would have - // charged the resulting ccof: source without a CustomerID — Square - // rejects a card-on-file source that carries no customer. - if req.SaveCard { - // 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) — the charge below sets - // CustomerID = savedCardCustomerID = squareCustomerID. - squareCustomerID, custErr := service.EnsureSquareCustomer(r.Context(), 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(r.Context(), 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 - - // CreateCardOnFile runs before the charge. If the subsequent payment - // fails, this card-on-file is intentionally NOT deleted: the pending - // 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 := service.SaveCardForUser(r.Context(), 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 tip: use the cnon: nonce DIRECTLY as the source. - // No card-on-file is created (nothing to orphan, no customer needed). - sourceID = *req.NewCardToken - } - if savedCardID == nil && req.SaveCard { - log.Printf("Card was not saved despite save_card=true for user %s", userID) - } - } else if req.CardID != nil { - card, err := service.GetCardByID(r.Context(), *req.CardID, userID) - if err != nil { - if errors.Is(err, pgx.ErrNoRows) { - http.Error(w, "Card not found", http.StatusNotFound) - return - } - log.Printf("Failed to get card: %v", err) - 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 := service.EnsureSquareCustomerForSavedCard(r.Context(), *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 + sourceID, savedCardID, savedCardCustomerID, sourceOK := resolveChargeSource(r.Context(), w, service, userID, req.NewCardToken, req.CardID, req.SaveCard, "Card not found") + if !sourceOK { + return } // Serialize tip attempts for this booking to prevent concurrent duplicate // tip payments across browser tabs or retries. Uses a PostgreSQL session-level // advisory lock scoped to the booking ID. - // See CreateBookingPayment lines 815-846 for the same pattern. // Bounded try-lock (R6) so a contended lock never blocks the pool across // the Square round-trip. - pinConn, err := db.Conn.Acquire(r.Context()) - if err != nil { - log.Printf("Failed to acquire connection for tip lock: %v", err) - http.Error(w, "Internal server error", http.StatusInternalServerError) - return - } - defer pinConn.Release() - - lockOK, err := acquireAdvisoryLock(r.Context(), pinConn, "crussell:tip:"+bookingID) - if err != nil { - log.Printf("Failed to acquire tip serialization lock for %s: %v", bookingID, err) - http.Error(w, "Internal server error", http.StatusInternalServerError) - return - } + pinConn, lockOK := acquireBookingPaymentLock(r.Context(), w, "crussell:tip:"+bookingID, "Payment in progress, try again") if !lockOK { - log.Printf("Tip serialization lock for %s not acquired within bound — a tip payment is already in progress", bookingID) - 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:tip:' || $1)) - `, bookingID); err != nil { - log.Printf("Failed to release tip serialization lock for %s: %v", bookingID, err) - } - }() + defer releaseBookingPaymentLock(pinConn, "crussell:tip:"+bookingID) // Step 1: Insert payment record in 'pending' state inside a DB transaction. // Square is NOT called yet — if the tx fails, no harm done. @@ -2944,7 +2744,7 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { if err != nil { log.Printf("Failed to create tip payment: %v", err) // Payment record intentionally left as 'pending' for manual retry. - http.Error(w, "Payment failed", http.StatusPaymentRequired) + http.Error(w, "Payment failed", chargeFailureStatus(err)) return } @@ -2956,14 +2756,14 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { // would silently exclude it. Mark the tip row failed and alert ops: money // was taken at Square and MUST be refunded manually (mirrors // CreateBookingPayment's post-charge recheck). - var tipRecheckStatus string - if err := db.Conn.QueryRow(r.Context(), `SELECT status FROM bookings WHERE id = $1`, bookingID).Scan(&tipRecheckStatus); err != nil { + tipRecheckStatus, tipPayable, err := recheckBookingPayable(r.Context(), db.Conn, bookingID) + if err != nil { log.Printf("CRITICAL: Square tip payment %s (ID=%s) was processed but re-reading booking %s status failed: %v — manual reconciliation required", paymentResult.Status, paymentResult.SquarePayID, bookingID, err) http.Error(w, "internal server error", http.StatusInternalServerError) return } - if !bookingStatusAllowsCompletedPayment(tipRecheckStatus) { + if !tipPayable { log.Printf("CRITICAL: Square tip payment %s (ID=%s) for booking %s was processed but booking is now %q — marking tip %s failed; money taken at Square MUST be refunded manually", paymentResult.Status, paymentResult.SquarePayID, bookingID, tipRecheckStatus, paymentID) if _, upErr := db.Conn.Exec(r.Context(), `UPDATE payments SET status = 'failed' WHERE id = $1`, paymentID); upErr != nil { @@ -3246,9 +3046,29 @@ func ReleasePaymentLock(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusNoContent) } -// uniqueTipKey generates a unique idempotency key for tip payments where the -// client did not supply one. Client-supplied UUIDs handle retry dedup; this -// fallback only needs uniqueness so identical tips don't collapse. -func uniqueTipKey() string { - return "tip-" + rand.Text() +// uniqueChargeKey generates a unique idempotency key under the given prefix +// (e.g. "tip-", "till-") where the client did not supply one. Client-supplied +// keys handle retry dedup; this fallback only needs uniqueness so two +// legitimate identical requests never collapse on the same key. Deliberately +// NOT derived from request fields — two identical requests would hash to the +// same key (the "tip" fallback must not dedupe two distinct equal tips on one +// booking). Shared by the tip/till flows, which used to carry two identical +// copies (uniqueTipKey/uniqueTillKey) differing only in the prefix string. +func uniqueChargeKey(prefix string) string { + return prefix + rand.Text() +} + +// randomHexSuffix returns n random bytes hex-encoded (2n hex chars) from +// crypto/rand, used to disambiguate idempotency fallback keys that would +// otherwise collide on deterministic inputs (e.g. the no-client-key refund +// key). Falls back to a masked monotonic timestamp if the OS entropy source +// errors — effectively impossible on Linux (crypto/rand.Read blocks until +// entropy is available) — keeping the same width so the key stays within +// Square's 45-char idempotency-key limit. +func randomHexSuffix(n int) string { + b := make([]byte, n) + if _, err := rand.Read(b); err != nil { + return fmt.Sprintf("%0*x", 2*n, time.Now().UnixNano()&(int64(1)<<(8*int64(n))-1)) + } + return fmt.Sprintf("%x", b) }