diff --git a/backend/handlers/payments/handlers.go b/backend/handlers/payments/handlers.go index 63eae62..cc7a23f 100644 --- a/backend/handlers/payments/handlers.go +++ b/backend/handlers/payments/handlers.go @@ -153,6 +153,35 @@ func GetDiscountPreviewHandler(w http.ResponseWriter, r *http.Request) { http.Error(w, "unauthorized", http.StatusUnauthorized) return } + userRole, _ := r.Context().Value(mw.UserRoleKey).(string) + + service := NewPaymentService() + + // Fail closed (R5): a non-admin request must own the booking. The previous + // handler computed the discount preview for ANY booking id the caller + // supplied — leaking another user's booking total and eligible discounts + // (IDOR). Mirror GetBookingPaymentSummary's ownership check exactly: a + // request with no user context gets 401, and a non-owner gets 403. + if userRole != "admin" { + if userID == "" { + http.Error(w, "Authentication required", http.StatusUnauthorized) + return + } + bookingUserID, err := service.GetBookingUserID(r.Context(), bookingID) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + http.Error(w, "Booking not found", http.StatusNotFound) + return + } + log.Printf("Failed to get booking user: %v", err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + if bookingUserID != userID { + http.Error(w, "Unauthorized", http.StatusForbidden) + return + } + } preview := calculateDiscountPreview(r.Context(), bookingID, userID) @@ -495,10 +524,32 @@ func CreateTerminalPayment(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 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 - // check. Mirrors the CreateBookingPayment lock (R4). + // check. Mirrors the CreateBookingPayment lock (R4). The lock is + // acquired with a bounded try-lock loop (R6): a blocking pg_advisory_lock + // 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) @@ -506,13 +557,17 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { return } defer pinConn.Release() - if _, err := pinConn.Exec(r.Context(), ` - SELECT pg_advisory_lock(hashtext('crussell:payment:' || $1)) - `, bookingID); err != nil { + 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 } + 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)) @@ -624,6 +679,32 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { return } + // Defensive post-charge recheck (R9): the window is tiny — this branch + // only runs on in_progress/completed bookings and the pending record + // committed moments ago — but a concurrent cancellation/eviction can + // still move the booking between the Square call and this record. A + // charge landing on a cancelled/lapsed booking must not be recorded as + // 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 { + 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) { + 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 { + log.Printf("CRITICAL: Square payment %s landed on %q booking %s but marking payment %s failed errored: %v — manual reconciliation required", + paymentResult.SquarePayID, recheckStatus, bookingID, paymentID, upErr) + } + http.Error(w, "This booking is no longer accepting payments", http.StatusConflict) + return + } + if _, upErr := db.Conn.Exec(r.Context(), `UPDATE payments SET status = 'completed', square_payment_id = $1 WHERE id = $2`, paymentResult.SquarePayID, paymentID, @@ -668,7 +749,8 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { // Serialize terminal-checkout creation per booking. This is the backend // half of the double-submit fix: a lost-response retry must not create a // second live Square checkout for the same booking while the first is in - // flight. + // 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) @@ -676,13 +758,17 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { return } defer pinConn.Release() - if _, err := pinConn.Exec(r.Context(), ` - SELECT pg_advisory_lock(hashtext('crussell:payment:' || $1)) - `, bookingID); err != nil { + 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 } + 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)) @@ -701,6 +787,25 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { return } + // Insert the tracked terminal_checkouts row FIRST with a provisional + // (pre-Square) checkout_id, THEN create the checkout at Square, THEN update + // the row with the real checkout_id (R3). A hard crash between the insert + // and the Square call leaves a visible PENDING row the in-flight guard and + // sweep can resolve as failed — the old order (CreateCheckout first) left a + // live untracked checkout the sweep could not see. The provisional id is + // synthetic ("tmp-") because the column is a NOT NULL + // PRIMARY KEY; a row carrying one is provably pre-Square (no checkout was + // ever created for it). + provisionalID := "tmp-" + idempotencyKey + if _, err := db.Conn.Exec(r.Context(), ` + INSERT INTO terminal_checkouts (checkout_id, booking_id, payment_type, status, amount) + VALUES ($1, $2, $3, 'PENDING', $4) + `, provisionalID, bookingID, req.PaymentType, float64(amount)/100.0); err != nil { + log.Printf("Failed to record provisional terminal checkout for booking %s: %v", bookingID, err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + checkoutReq := square.CreateCheckoutReq{ Amount: amount, Currency: "GBP", @@ -712,21 +817,43 @@ func CreateTerminalPayment(w http.ResponseWriter, r *http.Request) { checkout, err := SquareClient.CreateCheckout(r.Context(), checkoutReq) if err != nil { log.Printf("Failed to create checkout: %v", err) + // The provisional row is pre-Square and can never produce a charge — + // mark it failed so a retry can proceed (best-effort; log CRITICAL if + // the row update itself fails, since the row would then wedge the + // booking's in-flight guard). + if _, upErr := db.Conn.Exec(r.Context(), ` + UPDATE terminal_checkouts SET status = 'failed', updated_at = NOW() + WHERE checkout_id = $1 AND status = 'PENDING' + `, provisionalID); upErr != nil { + log.Printf("CRITICAL: failed to mark provisional terminal checkout %s failed after CreateCheckout error (%v): %v — MANUAL RECONCILIATION REQUIRED", provisionalID, err, upErr) + } http.Error(w, "Failed to create payment", http.StatusInternalServerError) return } - // Record the checkout with the payment type the admin charged so - // GetCheckoutStatus records the payment with that type. - if _, err := db.Conn.Exec(r.Context(), ` - INSERT INTO terminal_checkouts (checkout_id, booking_id, payment_type, status, amount) - VALUES ($1, $2, $3, 'PENDING', $4) - `, checkout.ID, bookingID, req.PaymentType, float64(amount)/100.0); err != nil { - log.Printf("Failed to record terminal checkout %s: %v", checkout.ID, err) + // Attach the real Square checkout id to the tracked row (the provisional + // id was never seen by the client, so no poller can race this). + tag, upErr := db.Conn.Exec(r.Context(), ` + UPDATE terminal_checkouts SET checkout_id = $1, updated_at = NOW() + WHERE checkout_id = $2 + `, checkout.ID, provisionalID) + if upErr != nil { + log.Printf("CRITICAL: terminal checkout %s was created at Square but the tracking UPDATE (from provisional %s) failed: %v — manual reconciliation required", checkout.ID, provisionalID, upErr) // The checkout is live at Square but untracked — best-effort cancel so // a customer cannot complete a charge the backend can't record. if cErr := SquareClient.CancelCheckout(r.Context(), checkout.ID); cErr != nil { - log.Printf("CRITICAL: failed to cancel orphaned terminal checkout %s after the DB insert failed: %v — MANUAL RECONCILIATION REQUIRED: the checkout may still be live at Square", checkout.ID, cErr) + log.Printf("CRITICAL: failed to cancel orphaned terminal checkout %s after the tracking UPDATE failed: %v — MANUAL RECONCILIATION REQUIRED: the checkout may still be live at Square", checkout.ID, cErr) + } + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + if tag.RowsAffected() == 0 { + // The provisional row vanished while the Square call was in flight + // (the sweep resolved it as stale) — the checkout is now live at + // Square but untracked. + log.Printf("CRITICAL: terminal checkout %s was created at Square but provisional row %s was already resolved — the checkout is untracked; MANUAL RECONCILIATION REQUIRED", checkout.ID, provisionalID) + if cErr := SquareClient.CancelCheckout(r.Context(), checkout.ID); cErr != nil { + log.Printf("CRITICAL: failed to cancel untracked terminal checkout %s: %v — MANUAL RECONCILIATION REQUIRED", checkout.ID, cErr) } http.Error(w, "internal server error", http.StatusInternalServerError) return @@ -768,6 +895,24 @@ func activeTerminalCheckoutID(ctx context.Context, bookingID string) string { return "" } + // A provisional (pre-Square) row carries a synthetic "tmp-" checkout_id (or + // an empty one for legacy rows) — no checkout was ever created at Square + // for it, so it is PROVABLY not live (R3). A hard crash between the + // terminal_checkouts insert and the Square CreateCheckout call is the only + // way one exists. Mark it failed so a fresh checkout can be created instead + // of wedging the booking behind a non-existent Square checkout. + if checkoutID == "" || strings.HasPrefix(checkoutID, "tmp-") { + log.Printf("Provisional (pre-Square) terminal checkout row %q for booking %s resolved as failed — no live checkout at Square", checkoutID, bookingID) + if checkoutID != "" { + if _, upErr := db.Conn.Exec(ctx, ` + UPDATE terminal_checkouts SET status = 'failed', updated_at = NOW() WHERE checkout_id = $1 + `, checkoutID); upErr != nil { + log.Printf("Failed to mark provisional terminal checkout %s failed: %v", checkoutID, upErr) + } + } + return "" + } + // Resolve against Square: once the terminal charge finished, the checkout // is COMPLETED and its payment may already be recorded — it must not // block a subsequent charge on the same booking. @@ -814,6 +959,16 @@ func GetCheckoutStatus(w http.ResponseWriter, r *http.Request) { http.Error(w, "not found", http.StatusNotFound) return } + // Reject provisional "tmp-" checkout ids: CreateTerminalPayment stores a + // synthetic "tmp-" id in terminal_checkouts until Square + // returns the real checkout id (provisional-row design), so such an id was + // never a real checkout — resolving it against Square would come back + // NOT_FOUND and surface as a 500. Answer 404 instead (mirrors the guard in + // CreateTerminalPayment and the sweep). + if strings.HasPrefix(checkoutID, "tmp-") { + http.Error(w, "Checkout not found", http.StatusNotFound) + return + } bookingID := r.URL.Query().Get("booking_id") if bookingID == "" { @@ -857,6 +1012,7 @@ func GetCheckoutStatus(w http.ResponseWriter, r *http.Request) { // dedup SELECT and BOTH INSERT, with the second dying on the // idempotency_key UNIQUE constraint after the customer already paid — // the same double-record race every other payment path guards against. + // Bounded try-lock (R6) so a contended lock never blocks the pool. terminalLockKey := paymentResult.SquarePayID pinConn, err := db.Conn.Acquire(r.Context()) if err != nil { @@ -865,13 +1021,17 @@ func GetCheckoutStatus(w http.ResponseWriter, r *http.Request) { return } defer pinConn.Release() - if _, err := pinConn.Exec(r.Context(), ` - SELECT pg_advisory_lock(hashtext('crussell:terminal:' || $1)) - `, terminalLockKey); err != nil { + lockOK, err := acquireAdvisoryLock(r.Context(), pinConn, "crussell:terminal:"+terminalLockKey) + if err != nil { log.Printf("Failed to acquire terminal-completion serialization lock for %s: %v", terminalLockKey, err) http.Error(w, "internal server error", http.StatusInternalServerError) return } + if !lockOK { + log.Printf("Terminal-completion serialization lock for %s not acquired within bound — a poll is already recording this checkout", terminalLockKey) + 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:terminal:' || $1)) @@ -1145,6 +1305,11 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { // Using db.Conn.Exec() for both would be unsafe — each call may get a // different pool connection, and pg_advisory_unlock on a different session // is a silent no-op, leaking the lock. + // + // R6: the lock is acquired with a bounded try-lock loop rather than the + // 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) @@ -1153,13 +1318,17 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { } defer pinConn.Release() - if _, err := pinConn.Exec(r.Context(), ` - SELECT pg_advisory_lock(hashtext('crussell:payment:' || $1)) - `, bookingID); err != nil { + 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 } + 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)) @@ -1294,42 +1463,51 @@ func CreateBookingPayment(w http.ResponseWriter, r *http.Request) { var savedCardCustomerID string if req.NewCardToken != nil && *req.NewCardToken != "" { - // 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, and Square returns the same card — deleting it would break that - // retry. The orphan is harmless (Square-side only, never charged). - // - // 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 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 { - var custErr error - squareCustomerID, custErr = service.EnsureSquareCustomer(r.Context(), 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) — 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 - } + 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 - sourceID = cardOnFile.CardID - - if req.SaveCard { - cardID, err := service.SaveCardForUser(r.Context(), userID, cardOnFile.CardID, cardOnFile.Brand, cardOnFile.Last4, cardOnFile.ExpMonth, cardOnFile.ExpYear, cardOnFile.Fingerprint) + // 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) @@ -1345,6 +1523,20 @@ func CreateBookingPayment(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). 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 @@ -1930,7 +2122,8 @@ func RefundPayment(w http.ResponseWriter, r *http.Request) { // 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. + // tip/gift-card advisory-lock pattern. Bounded try-lock (R6) so a + // contended lock never blocks the pool across the Square round-trip. refundLockKey := paymentID pinConn, err := db.Conn.Acquire(r.Context()) if err != nil { @@ -1939,13 +2132,17 @@ func RefundPayment(w http.ResponseWriter, r *http.Request) { return } defer pinConn.Release() - if _, err := pinConn.Exec(r.Context(), ` - SELECT pg_advisory_lock(hashtext('crussell:refund:' || $1)) - `, refundLockKey); err != nil { + lockOK, err := acquireAdvisoryLock(r.Context(), pinConn, "crussell:refund:"+refundLockKey) + if err != nil { log.Printf("Failed to acquire refund serialization lock for %s: %v", paymentID, err) http.Error(w, "internal server error", http.StatusInternalServerError) return } + if !lockOK { + log.Printf("Refund serialization lock for %s not acquired within bound — a refund is already in progress", paymentID) + http.Error(w, "Refund in progress, try again", http.StatusConflict) + return + } defer func() { if _, err := pinConn.Exec(context.Background(), ` SELECT pg_advisory_unlock(hashtext('crussell:refund:' || $1)) @@ -2497,42 +2694,48 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { var savedCardCustomerID string if req.NewCardToken != nil && *req.NewCardToken != "" { - // 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, and Square returns the same card — deleting it would break that - // retry. The orphan is harmless (Square-side only, never charged). - // - // 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 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 { - var custErr error - squareCustomerID, custErr = service.EnsureSquareCustomer(r.Context(), 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) — 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 - } + 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 - sourceID = cardOnFile.CardID - - if req.SaveCard { - cardID, err := service.SaveCardForUser(r.Context(), userID, cardOnFile.CardID, cardOnFile.Brand, cardOnFile.Last4, cardOnFile.ExpMonth, cardOnFile.ExpYear, cardOnFile.Fingerprint) + // 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) @@ -2548,6 +2751,19 @@ func CreateTipPayment(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 := 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 @@ -2557,6 +2773,8 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { // 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) @@ -2565,13 +2783,17 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { } defer pinConn.Release() - if _, err := pinConn.Exec(r.Context(), ` - SELECT pg_advisory_lock(hashtext('crussell:tip:' || $1)) - `, bookingID); err != nil { + 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 } + 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)) @@ -2726,6 +2948,32 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { return } + // Step 3a: post-charge recheck (R9). A concurrent cancellation/eviction + // can move the booking out of a payable state between the pre-charge + // status check and the Square charge completing. A tip landing on a + // cancelled/lapsed booking must NOT be recorded as completed — the + // cancellation refund path computes refunds from completed payments and + // 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 { + 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) { + 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 { + log.Printf("CRITICAL: Square tip payment %s (ID=%s) landed on %q booking %s but marking tip %s failed errored: %v — manual reconciliation required", + paymentResult.Status, paymentResult.SquarePayID, tipRecheckStatus, bookingID, paymentID, upErr) + } + http.Error(w, "This booking is no longer accepting tips", http.StatusConflict) + return + } + // Step 3: Square succeeded — update the payment record. _, upErr := db.Conn.Exec(r.Context(), `UPDATE payments SET status = 'completed', square_payment_id = $1 WHERE id = $2`, diff --git a/backend/handlers/payments/service.go b/backend/handlers/payments/service.go index f371278..6d8613d 100644 --- a/backend/handlers/payments/service.go +++ b/backend/handlers/payments/service.go @@ -11,6 +11,8 @@ import ( "log" "log/slog" "math" + "strings" + "sync" "time" "github.com/jackc/pgx/v5" @@ -493,7 +495,7 @@ func (s *PaymentService) GetBookingRemainingBalanceCents(ctx context.Context, bo func (s *PaymentService) GetUserPaymentMethods(ctx context.Context, userID string) ([]SavedCard, error) { rows, err := db.Conn.Query(ctx, ` - SELECT id, square_card_id, brand, last_4, exp_month, exp_year, fingerprint, is_default, COALESCE(square_customer_id, '') + SELECT id, COALESCE(square_card_id, ''), brand, last_4, exp_month, exp_year, fingerprint, is_default, COALESCE(square_customer_id, '') FROM user_saved_cards WHERE user_id = $1 AND deleted_at IS NULL ORDER BY is_default DESC, created_at DESC @@ -522,6 +524,35 @@ func (s *PaymentService) GetUserPaymentMethods(ctx context.Context, userID strin } func (s *PaymentService) DeletePaymentMethod(ctx context.Context, cardID, userID string) error { + // Load the Square card id and disable the card at Square BEFORE the local + // soft-delete (R8). Without this the card stays ENABLED at Square and keeps + // accepting ccof: charges even though the user deleted it locally — the + // account-deletion path already calls DeleteCardOnFile; this mirrors it for + // single-card deletes. The Square call is best-effort: a local delete must + // never be blocked by a Square failure. A NOT_FOUND answer means Square no + // longer has the card (nothing to disable); any other error is logged and + // ignored so the local delete proceeds regardless. + var sqCardID sql.NullString + err := db.Conn.QueryRow(ctx, ` + SELECT square_card_id FROM user_saved_cards + WHERE id = $1 AND user_id = $2 + `, cardID, userID).Scan(&sqCardID) + if err != nil && !errors.Is(err, pgx.ErrNoRows) { + return err + } + // ErrNoRows: the card is not owned by this user — the soft-delete below is + // a silent no-op (matching the pre-R8 behaviour), so skip the Square call. + if sqCardID.Valid && sqCardID.String != "" { + if err := SquareClient.DeleteCardOnFile(ctx, sqCardID.String); err != nil { + msg := strings.ToUpper(err.Error()) + if square.ErrorCode(err) == "NOT_FOUND" || strings.Contains(msg, "NOT_FOUND") || strings.Contains(msg, "NOT FOUND") { + // Square already removed/disabled the card — nothing to do. + } else { + slog.Warn("failed to disable Square card on local delete — card may remain enabled at Square", "square_card_id", sqCardID.String, "err", err) + } + } + } + tx, err := db.Conn.Begin(ctx) if err != nil { log.Printf("Failed to begin transaction: %v", err) @@ -618,7 +649,18 @@ func (s *PaymentService) CreatePaymentMethodFromToken(ctx context.Context, userI // subsequent card save by the same user. Square dedups on a deterministic // idempotency key derived from the email, so a response-lost retry returns the // same customer instead of minting a duplicate. +// +// R7: the created id is written back to the user's saved-card rows (when any +// exist) AND cached in a package-level map, so a second ensureSquareCustomer +// call in the same request flow (e.g. the handler's save-card branch followed +// by SaveCardForUser) never re-hits the DB and never re-mints a customer. The +// cache is process-local and dev-friendly; the row write makes it durable for +// the next process/request. func (s *PaymentService) ensureSquareCustomer(ctx context.Context, userID string) (string, error) { + if v, ok := squareCustomerCache.Load(userID); ok { + return v.(string), nil + } + var customerID sql.NullString err := db.Conn.QueryRow(ctx, ` SELECT square_customer_id FROM user_saved_cards @@ -627,6 +669,7 @@ func (s *PaymentService) ensureSquareCustomer(ctx context.Context, userID string LIMIT 1 `, userID).Scan(&customerID) if err == nil && customerID.Valid { + squareCustomerCache.Store(userID, customerID.String) return customerID.String, nil } if err != nil && !errors.Is(err, pgx.ErrNoRows) { @@ -644,6 +687,18 @@ func (s *PaymentService) ensureSquareCustomer(ctx context.Context, userID string if err != nil { return "", fmt.Errorf("failed to create Square customer for card save: %w", err) } + + // Persist the minted id so the NEXT process/request reuses it instead of + // re-running CreateCustomer (the cache above only serves this process). + // Best-effort: the cache covers the immediate double-ensure within one + // request, and the save-card INSERT below carries the id anyway. + if _, upErr := db.Conn.Exec(ctx, ` + UPDATE user_saved_cards SET square_customer_id = $1 + WHERE user_id = $2 AND square_customer_id IS NULL + `, customer.ID, userID); upErr != nil { + log.Printf("Failed to persist Square customer id for user %s (non-fatal): %v", userID, upErr) + } + squareCustomerCache.Store(userID, customer.ID) return customer.ID, nil } @@ -656,17 +711,43 @@ func (s *PaymentService) EnsureSquareCustomer(ctx context.Context, userID string return s.ensureSquareCustomer(ctx, userID) } -func (s *PaymentService) SaveCardForUser(ctx context.Context, userID, squareCardID, brand, last4 string, expMonth, expYear int, fingerprint string) (string, error) { - // P14: SaveCardForUser is only ever called in save-card flows, so lazily - // ensure the Square customer exists and persist its id on the saved-card - // row for reuse by subsequent card saves from the same user. - squareCustomerID, err := s.ensureSquareCustomer(ctx, userID) +// EnsureSquareCustomerForSavedCard returns the Square customer id for a +// saved-card row, lazily provisioning + persisting one when the row predates +// P14 (square_customer_id empty). A ccof: source can NEVER be charged without +// a CustomerID — Square rejects the payment — so every saved-card charge path +// calls this before CreatePayment. Provisioning failure aborts the charge. +func (s *PaymentService) EnsureSquareCustomerForSavedCard(ctx context.Context, savedCardID, userID string) (string, error) { + var customerID sql.NullString + if err := db.Conn.QueryRow(ctx, ` + SELECT square_customer_id FROM user_saved_cards + WHERE id = $1 AND user_id = $2 + `, savedCardID, userID).Scan(&customerID); err != nil { + return "", err + } + if customerID.Valid && customerID.String != "" { + return customerID.String, nil + } + provisioned, err := s.EnsureSquareCustomer(ctx, userID) if err != nil { return "", err } + if _, upErr := db.Conn.Exec(ctx, ` + UPDATE user_saved_cards SET square_customer_id = $1 + WHERE id = $2 AND user_id = $3 + `, provisioned, savedCardID, userID); upErr != nil { + return "", upErr + } + return provisioned, nil +} +// SaveCardForUser persists a tokenized card as a saved card for the user. +// squareCustomerID is the user's provisioned Square customer profile id +// (P14) — the caller has already ensured it via EnsureSquareCustomer, so this +// method NEVER re-provisions (R7: a second ensureSquareCustomer would re-query +// the DB and, on a first-save flow, re-run CreateCustomer). +func (s *PaymentService) SaveCardForUser(ctx context.Context, userID, squareCustomerID, squareCardID, brand, last4 string, expMonth, expYear int, fingerprint string) (string, error) { var id string - err = db.Conn.QueryRow(ctx, ` + err := db.Conn.QueryRow(ctx, ` INSERT INTO user_saved_cards ( user_id, square_card_id, square_customer_id, brand, last_4, exp_month, exp_year, fingerprint, is_default, created_at ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, false, NOW()) @@ -689,7 +770,7 @@ func (s *PaymentService) GetCardByID(ctx context.Context, cardID, userID string) func (s *PaymentService) GetCardByIDQuerier(ctx context.Context, q db.Querier, cardID, userID string) (*SavedCard, error) { var c SavedCard err := q.QueryRow(ctx, ` - SELECT id, square_card_id, brand, last_4, exp_month, exp_year, fingerprint, is_default, COALESCE(square_customer_id, '') + SELECT id, COALESCE(square_card_id, ''), brand, last_4, exp_month, exp_year, fingerprint, is_default, COALESCE(square_customer_id, '') FROM user_saved_cards WHERE id = $1 AND user_id = $2 AND deleted_at IS NULL `, cardID, userID).Scan(&c.ID, &c.SquareCardID, &c.Brand, &c.Last4, &c.ExpMonth, &c.ExpYear, &c.Fingerprint, &c.IsDefault, &c.SquareCustomerID) @@ -700,4 +781,11 @@ func (s *PaymentService) GetCardByIDQuerier(ctx context.Context, q db.Querier, c return &c, nil } +// squareCustomerCache is a package-level process-local cache of +// userID → Square customer id, populated on the first successful provisioning +// (R7). It prevents a second ensureSquareCustomer call in the same request +// flow (or a rapid retry) from re-running the DB query and re-minting a +// customer. The durable record remains the user_saved_cards row. +var squareCustomerCache sync.Map + var SquareClient square.SquareClient diff --git a/backend/handlers/payments/sweep.go b/backend/handlers/payments/sweep.go index 678d137..61f2861 100644 --- a/backend/handlers/payments/sweep.go +++ b/backend/handlers/payments/sweep.go @@ -296,6 +296,20 @@ func SweepStaleTerminalCheckouts(ctx context.Context) (int, error) { resolved := 0 for _, r := range pending { + // A provisional (pre-Square) terminal_checkouts row carries a synthetic + // "tmp-" checkout_id (or an empty one) — no checkout was ever created + // at Square for it, so it is PROVABLY not live (R3). Resolve it to + // failed directly without a Square round-trip; a hard crash between the + // row insert and the Square CreateCheckout call is the only way one + // exists. + if r.CheckoutID == "" || strings.HasPrefix(r.CheckoutID, "tmp-") { + if markTerminalCheckoutRowFailed(ctx, r) { + resolved++ + } + log.Printf("Provisional (pre-Square) terminal checkout row %s (%s) resolved as failed — no live checkout at Square", r.RowID, r.Kind) + continue + } + // Conservative status check: only cancel a checkout that is provably // still waiting at Square. A COMPLETED checkout must never be // cancelled, and an ambiguous status (network error) is left alone for