From 4f5dd5c4266ce2426d39d1a2d75c983fbd46b56a Mon Sep 17 00:00:00 2001 From: Stephen Adamson Date: Fri, 31 Jul 2026 15:08:11 +0100 Subject: [PATCH] Fix review findings: BuyGiftCard concurrency lock, amount guards, NULL scan, mock dedup, docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit N1 (HIGH) — BuyGiftCard concurrent same-key retry could double-issue gift cards (2× value for 1 charge). Added pg_advisory_lock on the idempotency key (mirroring the tip pattern) acquired before the idempotency check, so concurrent same-key retries serialize and only one executes gift-card creation. N2 — Amount-equality guards in both reuse branches (CreateTipPayment and BuyGiftCard). A same-key retry with a different amount now returns 400 instead of silently mutating the pending record's books/VAT/refund caps. N3 — test coverage: - TestBuyGiftCard_RetryPending_ReattemptsCharge: pending record + same-key retry re-attempts, reuses the record (count=1), completes, and issues the gift card exactly once. - TestCreateCheckoutHTTP_DeviceOptionsWireShape: httptest.Server asserts device_id is under checkout.device_options (not top-level). Extracted createCheckoutHTTPWithClient for injectable base URL. - MockClient.CreatePayment now dedups on idempotency key (paymentByKey map), matching real Square behaviour. N4 — Corrected the savepoint comments in handlers.go and giftcards.go: the savepoint only exists in the test harness; in production db.Conn.Begin is a plain tx and the status UPDATE runs on a separate pooled connection. Commit is a harmless no-op in prod but required in tests. Bonus bug fixed: CheckIdempotencyByKey scanned NULL booking_id/gift_card_id (gift-card purchases) into plain string, failing with 'cannot scan NULL'. Now uses sql.NullString. Docs: Technical Manual.md:53 and Feature Catalog.md (2.1, 2.5) corrected — no longer claim Web Payments SDK is live; new-card entry is documented as pending P11, saved-card flow works via ccof tokens, dev mock rejects raw PANs. --- backend/handlers/payments/giftcards.go | 49 +++++++++++- backend/handlers/payments/giftcards_test.go | 75 +++++++++++++++++++ backend/handlers/payments/handlers.go | 18 ++++- backend/handlers/payments/service.go | 12 ++- backend/internal/square/square_dev.go | 26 +++++-- backend/internal/square/square_http_client.go | 5 +- .../square/square_http_client_test.go | 59 ++++++++++++++- obsidian/Crussell/Feature Catalog.md | 6 +- obsidian/Crussell/Technical Manual.md | 2 +- 9 files changed, 231 insertions(+), 21 deletions(-) diff --git a/backend/handlers/payments/giftcards.go b/backend/handlers/payments/giftcards.go index 9ae74d2..330316f 100644 --- a/backend/handlers/payments/giftcards.go +++ b/backend/handlers/payments/giftcards.go @@ -1,6 +1,7 @@ package payments import ( + "context" "database/sql" "encoding/json" "errors" @@ -891,6 +892,37 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { paymentService := NewPaymentService() + // Serialize gift-card purchase attempts on the idempotency key to prevent + // concurrent same-key retries from both reusing a pending record and both + // 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. + lockKey := req.IdempotencyKey + if lockKey == "" { + lockKey = userID + } + pinConn, err := db.Conn.Acquire(ctx) + if err != nil { + log.Printf("Failed to acquire connection for gift-card lock: %v", err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + defer pinConn.Release() + if _, err := pinConn.Exec(ctx, ` + SELECT pg_advisory_lock(hashtext('crussell:giftcard:' || $1)) + `, lockKey); 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 + } + defer func() { + if _, err := pinConn.Exec(context.Background(), ` + SELECT pg_advisory_unlock(hashtext('crussell:giftcard:' || $1)) + `, lockKey); err != nil { + log.Printf("Failed to release gift-card serialization lock for %s: %v", lockKey, err) + } + }() + // Idempotency: only short-circuit when the existing record is 'completed'. // A 'pending' record means the previous Square call failed — returning it // as 200 would show a success without ever charging. Re-attempt below with @@ -909,6 +941,14 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { return } if existing.Status == "pending" { + // Guard the amount: a retry with a different amount must not + // reuse the pending record (gift card would be issued at the + // new amount against the old charge record). + if int64(existing.Amount*100) != req.Amount { + log.Printf("Gift card retry amount mismatch: pending record %s has %.2f, request has %d pence", existing.ID, existing.Amount, req.Amount) + http.Error(w, "Amount does not match the pending gift card payment", http.StatusBadRequest) + return + } reusePendingID = existing.ID log.Printf("[PAYMENTS] Reusing pending payment %s for idempotent gift-card retry (key %s)", existing.ID, req.IdempotencyKey) } @@ -1015,10 +1055,11 @@ func BuyGiftCard(w http.ResponseWriter, r *http.Request) { } } - // Commit the (nested/savepoint) transaction in the reuse path too. It is - // empty, but committing releases the savepoint so the deferred rollback - // becomes a no-op — otherwise that rollback would undo the status UPDATE - // executed later on the same connection. + // Commit in the reuse path too. No rows were written, but the commit is + // required in the test harness: there the context carries an outer test tx, + // so Begin creates a nested savepoint whose deferred rollback would + // otherwise undo the status UPDATE executed later on the same connection. + // In production Begin is a plain tx and this commit is a harmless no-op. if reusePendingID != "" { if err := tx.Commit(ctx); err != nil { log.Printf("Failed to commit buy transaction (reuse): %v", err) diff --git a/backend/handlers/payments/giftcards_test.go b/backend/handlers/payments/giftcards_test.go index 2a62b72..1457d48 100644 --- a/backend/handlers/payments/giftcards_test.go +++ b/backend/handlers/payments/giftcards_test.go @@ -1189,6 +1189,81 @@ func TestBuyGiftCard_Idempotency(t *testing.T) { } } +// TestBuyGiftCard_RetryPending_ReattemptsCharge verifies that a same-key retry +// after a failed Square call (record left 'pending') re-attempts the charge and +// completes — it must NOT return the stale pending record as a false success, +// and must NOT issue the gift card twice. +func TestBuyGiftCard_RetryPending_ReattemptsCharge(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + if err != nil { + t.Fatalf("failed to create user: %v", err) + } + + token := jwt.GenerateTestToken(userID, "verified_email") + + idempotencyKey := "buy-gc-pending-retry" + + // Seed a PENDING payment record with the same key — simulates a prior + // attempt where the Square call failed. + _, err = tx.Exec(ctx, ` + INSERT INTO payments (payment_type, payment_method, status, amount, idempotency_key, created_at, updated_at, created_by) + VALUES ('full', 'online_square', 'pending', 20.00, $1, NOW(), NOW(), $2) + `, idempotencyKey, userID) + if err != nil { + t.Fatalf("failed to seed pending payment: %v", err) + } + + reqBody := map[string]interface{}{ + "amount": 2000, + "recipient_type": "self", + "new_card_token": "cnon:card-nonce-ok", + "idempotency_key": idempotencyKey, + } + + body1, _ := json.Marshal(reqBody) + req1 := httptest.NewRequest("POST", "/api/user/giftcards/buy", bytes.NewBuffer(body1)) + req1.Header.Set("Authorization", "Bearer "+token) + req1.Header.Set("Content-Type", "application/json") + req1 = req1.WithContext(db.ContextWithTx(req1.Context(), tx.(pgx.Tx))) + + w1 := httptest.NewRecorder() + r1 := chi.NewRouter() + r1.Use(mw.RequireAuth) + r1.Post("/api/user/giftcards/buy", BuyGiftCard) + r1.ServeHTTP(w1, req1) + + if w1.Code != http.StatusOK && w1.Code != http.StatusCreated { + t.Fatalf("buy request: expected 200/201, got %d. Body: %s", w1.Code, w1.Body.String()) + } + + // Exactly one payment record for the key, now completed. + var payCount int + var payStatus string + err = tx.QueryRow(ctx, "SELECT COUNT(*), MAX(status) FROM payments WHERE idempotency_key = $1", idempotencyKey).Scan(&payCount, &payStatus) + if err != nil { + t.Fatalf("failed to query payments: %v", err) + } + if payCount != 1 { + t.Errorf("expected 1 payment record (reuse, not duplicate), got %d", payCount) + } + if payStatus != "completed" { + t.Errorf("expected pending record to be completed after retry, got %s", payStatus) + } + + // Exactly one gift card issued for the single charge. + var cardCount int + err = tx.QueryRow(ctx, "SELECT COUNT(*) FROM gift_cards WHERE redeemed_by = $1", userID).Scan(&cardCount) + if err != nil { + t.Fatalf("failed to query gift cards: %v", err) + } + if cardCount != 1 { + t.Errorf("expected 1 gift card issued, got %d", cardCount) + } +} + // TestAdminCreateGiftCard_ExpiryDateIsNull verifies that gift cards created via // CreateGiftCard no longer have expiry_date set (rolling 24-month expiry via last_used_at). func TestAdminCreateGiftCard_ExpiryDateIsNull(t *testing.T) { diff --git a/backend/handlers/payments/handlers.go b/backend/handlers/payments/handlers.go index 63637fb..8a8c13c 100644 --- a/backend/handlers/payments/handlers.go +++ b/backend/handlers/payments/handlers.go @@ -1937,6 +1937,14 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { return case err == nil && existingStatus.String == "pending": // Previous Square call failed — reuse the pending record and re-attempt. + // Guard the amount: a retry with a different amount must not mutate the + // original record (books, VAT, refund caps) or silently charge the new + // amount against the old record. + if int64(existingAmount.Float64*100) != req.Amount { + log.Printf("Tip retry amount mismatch: pending record %s has %d pence, request has %d pence", existingID.String, int64(existingAmount.Float64*100), req.Amount) + http.Error(w, "Amount does not match the pending tip payment", http.StatusBadRequest) + return + } paymentID = existingID.String reusePendingRecord = true case err != nil && !errors.Is(err, pgx.ErrNoRows): @@ -1967,10 +1975,12 @@ func CreateTipPayment(w http.ResponseWriter, r *http.Request) { ApplyVATToBookingPayment(r.Context(), tx, paymentID) } - // Always commit the (possibly nested/savepoint) transaction. In the reuse - // path the savepoint is empty, but committing it releases the savepoint so - // the deferred rollback becomes a no-op — otherwise that rollback would - // undo the status UPDATE executed later on the same connection. + // Always commit the transaction. In the reuse path no rows were written, + // but the commit is required in the test harness: there the context carries + // an outer test tx, so Begin creates a nested savepoint whose deferred + // rollback would otherwise undo the status UPDATE executed later on the + // same connection. In production Begin is a plain tx and this commit is a + // harmless no-op that keeps both paths identical. if err := tx.Commit(r.Context()); err != nil { log.Printf("Failed to commit transaction: %v", err) http.Error(w, "internal server error", http.StatusInternalServerError) diff --git a/backend/handlers/payments/service.go b/backend/handlers/payments/service.go index bb8d759..8932848 100644 --- a/backend/handlers/payments/service.go +++ b/backend/handlers/payments/service.go @@ -5,6 +5,7 @@ import ( "crussell/clock" "crussell/db" "crussell/internal/square" + "database/sql" "errors" "fmt" "log" @@ -296,6 +297,9 @@ func (s *PaymentService) CheckIdempotency(ctx context.Context, bookingID, idempo func (s *PaymentService) CheckIdempotencyByKey(ctx context.Context, idempotencyKey string) (*PaymentRecord, error) { var p PaymentRecord + // booking_id and gift_card_id are NULL for gift-card purchases — scan into + // NullString to avoid "cannot scan NULL into *string". + var bookingID, giftCardID sql.NullString err := db.Conn.QueryRow(ctx, ` SELECT id, booking_id, payment_type, payment_method, vendor_code, invoice_number, status, amount, is_vat_applicable, vat_rate, vat_amount, net_amount, @@ -304,11 +308,15 @@ func (s *PaymentService) CheckIdempotencyByKey(ctx context.Context, idempotencyK FROM payments WHERE idempotency_key = $1 `, idempotencyKey).Scan( - &p.ID, &p.BookingID, &p.PaymentType, &p.PaymentMethod, &p.VendorCode, &p.InvoiceNumber, + &p.ID, &bookingID, &p.PaymentType, &p.PaymentMethod, &p.VendorCode, &p.InvoiceNumber, &p.Status, &p.Amount, &p.IsVATApplicable, &p.VATRate, &p.VATAmount, &p.NetAmount, &p.UserSavedCardID, &p.SquarePaymentID, &p.IdempotencyKey, &p.Fees, - &p.CreatedAt, &p.UpdatedAt, &p.CreatedBy, &p.GiftCardID, + &p.CreatedAt, &p.UpdatedAt, &p.CreatedBy, &giftCardID, ) + p.BookingID = bookingID.String + if giftCardID.Valid { + p.GiftCardID = &giftCardID.String + } if err != nil { if errors.Is(err, pgx.ErrNoRows) { diff --git a/backend/internal/square/square_dev.go b/backend/internal/square/square_dev.go index 76c508f..757f30a 100644 --- a/backend/internal/square/square_dev.go +++ b/backend/internal/square/square_dev.go @@ -28,6 +28,7 @@ type MockClient struct { cards map[string]map[string]*CardOnFile checkouts map[string]*CheckoutResult payments map[string]*PaymentResult + paymentByKey map[string]*PaymentResult refunds map[string]*RefundResult completed map[string]*PaymentResult HoldCheckouts bool @@ -73,11 +74,12 @@ func NewDevClient() SquareClient { } log.Println("[SQUARE-MOCK] Using in-memory mock client") return &MockClient{ - cards: make(map[string]map[string]*CardOnFile), - checkouts: make(map[string]*CheckoutResult), - payments: make(map[string]*PaymentResult), - refunds: make(map[string]*RefundResult), - completed: make(map[string]*PaymentResult), + cards: make(map[string]map[string]*CardOnFile), + checkouts: make(map[string]*CheckoutResult), + payments: make(map[string]*PaymentResult), + paymentByKey: make(map[string]*PaymentResult), + refunds: make(map[string]*RefundResult), + completed: make(map[string]*PaymentResult), } } @@ -106,6 +108,17 @@ func (m *MockClient) CreatePayment(ctx context.Context, req CreatePaymentReq) (* m.mu.Lock() defer m.mu.Unlock() + // Real Square dedups on idempotency key: a retry with the same key returns + // the original payment rather than creating a second charge. The mock + // mirrors this so dev/testing behaves like production (also why the tip + // retry regression test can rely on the mock). + if req.IdempotencyKey != "" { + if existing, ok := m.paymentByKey[req.IdempotencyKey]; ok { + log.Printf("[SQUARE-MOCK] CreatePayment dedup hit: key=%s → id=%s", req.IdempotencyKey, existing.ID) + return existing, nil + } + } + now := clock.Now().UTC() status := "COMPLETED" @@ -162,6 +175,9 @@ func (m *MockClient) CreatePayment(ctx context.Context, req CreatePaymentReq) (* } m.payments[paymentID] = result m.payments[result.SquarePayID] = result + if req.IdempotencyKey != "" { + m.paymentByKey[req.IdempotencyKey] = result + } log.Printf("[SQUARE-MOCK] Payment created: id=%s, status=%s, amount=%d, fees=%d", paymentID, status, amount, fees) return result, nil } diff --git a/backend/internal/square/square_http_client.go b/backend/internal/square/square_http_client.go index 517ab7b..ac4d3bc 100644 --- a/backend/internal/square/square_http_client.go +++ b/backend/internal/square/square_http_client.go @@ -297,7 +297,10 @@ func createPaymentHTTP(ctx context.Context, req CreatePaymentReq) (*PaymentResul } func createCheckoutHTTP(ctx context.Context, req CreateCheckoutReq) (*CheckoutResult, error) { - hc := newHTTPClient() + return createCheckoutHTTPWithClient(ctx, req, newHTTPClient()) +} + +func createCheckoutHTTPWithClient(ctx context.Context, req CreateCheckoutReq, hc *httpClient) (*CheckoutResult, error) { body := sqTerminalCheckoutRequest{ IdempotencyKey: req.IdempotencyKey, Checkout: sqTerminalCheckoutPayload{ diff --git a/backend/internal/square/square_http_client_test.go b/backend/internal/square/square_http_client_test.go index 7f0acbb..8f0c608 100644 --- a/backend/internal/square/square_http_client_test.go +++ b/backend/internal/square/square_http_client_test.go @@ -2,7 +2,13 @@ package square -import "testing" +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" +) func TestPaymentFromSquare_ElseBranch_SurfacesBrandWithoutCardID(t *testing.T) { p := &sqPayment{ @@ -43,3 +49,54 @@ func TestPaymentFromSquare_NilCardDetails(t *testing.T) { t.Errorf("expected amount 2500, got %d", result.Amount) } } + +// TestCreateCheckoutHTTP_DeviceOptionsWireShape verifies the terminal checkout +// request puts device_id inside checkout.device_options (Square's required +// shape), not at the top level. A top-level device_id is rejected with 400 by +// Square's real API. +func TestCreateCheckoutHTTP_DeviceOptionsWireShape(t *testing.T) { + var captured map[string]any + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPost { + t.Errorf("expected POST, got %s", r.Method) + } + if r.URL.Path != "/v2/terminals/checkouts" { + t.Errorf("expected /v2/terminals/checkouts, got %s", r.URL.Path) + } + if err := json.NewDecoder(r.Body).Decode(&captured); err != nil { + t.Errorf("failed to decode request body: %v", err) + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"checkout":{"id":"chk_test","status":"PENDING","amount_money":{"amount":5000,"currency":"GBP"},"created_at":"2026-07-31T00:00:00Z","updated_at":"2026-07-31T00:00:00Z"}}`)) + })) + defer srv.Close() + + hc := &httpClient{baseURL: srv.URL, token: "test-token", http: srv.Client()} + + _, err := createCheckoutHTTPWithClient(context.Background(), CreateCheckoutReq{ + Amount: 5000, + Currency: "GBP", + IdempotencyKey: "ik-1", + DeviceID: "dvc_test", + }, hc) + if err != nil { + t.Fatalf("createCheckoutHTTP failed: %v", err) + } + + checkout, ok := captured["checkout"].(map[string]any) + if !ok { + t.Fatalf("expected checkout object in body, got %v", captured) + } + // device_id must NOT be at the top level + if _, hasTopLevel := captured["device_id"]; hasTopLevel { + t.Errorf("device_id must not be top-level in terminal checkout request: %v", captured) + } + // device_id must live under checkout.device_options + devOpts, ok := checkout["device_options"].(map[string]any) + if !ok { + t.Fatalf("expected checkout.device_options in body, got %v", checkout) + } + if devOpts["device_id"] != "dvc_test" { + t.Errorf("expected checkout.device_options.device_id = dvc_test, got %v", devOpts) + } +} diff --git a/obsidian/Crussell/Feature Catalog.md b/obsidian/Crussell/Feature Catalog.md index 8b2321c..f853e0d 100644 --- a/obsidian/Crussell/Feature Catalog.md +++ b/obsidian/Crussell/Feature Catalog.md @@ -162,8 +162,8 @@ Multi-method payment system accepting Square (card terminal & online), cash, gif **Related:** [[Booking System|1. Booking System]] (deposits), [[Gift Cards|4. Gift Cards]] (pay by gift card), [[Admin Dashboard|5. Admin Dashboard]] (till purchases) -### 2.1 Online Card Payment (Square Web Payments SDK) -**What it does:** Customers pay online using a credit/debit card via Square's Web Payments SDK. Used for deposits, full payments, balance payments, and tips. +### 2.1 Online Card Payment (Square — saved cards; new-card entry pending P11) +**What it does:** Customers pay online with a card. Saved-card payments work end-to-end via Square tokenized card IDs (`ccof:`). New-card entry (raw PAN entry in the UI) is a documented dead end until Square Web Payments SDK nonces land — see `plans/p11-square-web-payments-sdk.md`. The dev mock rejects raw PANs to mirror production. Used for deposits, full payments, balance payments, and tips. **Layman summary:** "Pay online with your card — just like any online shop." @@ -191,7 +191,7 @@ Multi-method payment system accepting Square (card terminal & online), cash, gif **Related:** [[Gift Cards|4. Gift Cards]], [[VAT Calculation|2.10 VAT Calculation]] ### 2.5 Saved Cards -**What it does:** Customers can save their card details for faster checkout next time. Cards are tokenized via Square (raw card numbers never touch the server). Soft-deleted with 7-year UK retention. +**What it does:** Customers can save their card details for faster checkout next time. Cards are tokenized via Square (`ccof:` card IDs; the full PAN exists only in Square's vault — our DB stores only the reference + brand/last4/fingerprint). The dev mock mirrors this (raw PANs rejected). Soft-deleted with 7-year UK retention. Note: the "Add Card" UI currently sends raw PAN and is a dead end until P11 (Web Payments SDK nonces) — see `plans/p11-square-web-payments-sdk.md`. **Layman summary:** "Save your card for next time — one-click payment." diff --git a/obsidian/Crussell/Technical Manual.md b/obsidian/Crussell/Technical Manual.md index 8647ca3..0ef7d6b 100644 --- a/obsidian/Crussell/Technical Manual.md +++ b/obsidian/Crussell/Technical Manual.md @@ -50,7 +50,7 @@ Backend (:8080) |---------|--------|---------| | SabreDAV (CardDAV/CalDAV) | Active | Contact sync (profile photos), calendar events | | S3/R2 | Active (dev) | Portfolio images (AVIF), profile pictures (WebP) | -| Square | **Active** | Payment processing — in-person Terminal + online Web Payments SDK. Dev mock (`//go:build dev`) simulates async checkout; prod stub (`//go:build !dev`) connects to live API. | +| Square | **Active** | Payment processing — in-person Terminal (`CreateTerminalCheckout`) + online card payments (saved cards working; new-card entry pending Web Payments SDK nonces — backlog P11, see `plans/p11-square-web-payments-sdk.md`). Dev mock (`//go:build dev`) mirrors production PCI-DSS behaviour (rejects raw PANs; accepts `cnon:`/`ccof:` tokens); prod client (`!dev`) connects to live API. | | SMTP | Not implemented | Email/SMS notifications — backend not wired | ---