From a4441b6acfee6134dfc5693077d8584f33c50d40 Mon Sep 17 00:00:00 2001 From: Stephen Adamson Date: Fri, 31 Jul 2026 10:14:27 +0100 Subject: [PATCH] Fix review findings: till idempotency keys, sha256 card key, paymentFromSquare fallback, regression test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit till.go: - Replace deterministicTillKey (request-field hash) with uniqueTillKey (crypto/rand.Text) - Deterministic hashes broke legitimate identical create sales (empty gift_card_id collides on till_sales idempotency_key UNIQUE constraint → 500 on second sale) - Client-supplied keys handle dedup; fallback only needs uniqueness - Use rand.Text() (Go 1.24+) instead of deprecated rand.Read with dead error check till_test.go: - Add TestCreateTillSale_TwoIdenticalCreateSales_BothSucceed regression test (two identical keyless cash creates must both return 201) square_http_client.go: - createCardOnFileHTTP: replace reversible hex encoding with crypto/sha256 - paymentFromSquare: replace dead-code fallback with reachable else branch - Remove unused url import cleanup where applicable --- backend/handlers/payments/till.go | 31 +++++++++++----- backend/handlers/payments/till_test.go | 37 +++++++++++++++++++ backend/internal/square/square_http_client.go | 19 ++++------ 3 files changed, 66 insertions(+), 21 deletions(-) diff --git a/backend/handlers/payments/till.go b/backend/handlers/payments/till.go index 008f13d..1e2946d 100644 --- a/backend/handlers/payments/till.go +++ b/backend/handlers/payments/till.go @@ -1,11 +1,7 @@ package payments import ( - "crussell/clock" - "crussell/db" - "crussell/internal/square" - "crussell/internal/validators" - "crussell/mw" + "crypto/rand" "database/sql" "encoding/json" "errors" @@ -14,6 +10,11 @@ import ( "log/slog" "net/http" + "crussell/db" + "crussell/internal/square" + "crussell/internal/validators" + "crussell/mw" + "github.com/go-chi/chi/v5" "github.com/jackc/pgx/v5" ) @@ -44,6 +45,16 @@ type TillSaleResponse struct { CheckoutID *string `json:"checkout_id,omitempty"` } +// uniqueTillKey generates a unique idempotency key for till sales where the +// client did not supply one. Dedup of retries is handled by the client-supplied +// key (the frontend sends a UUID); this fallback only needs to be unique so it +// never collides with the till_sales idempotency_key UNIQUE constraint. +// Deliberately NOT derived from request fields — two legitimate identical +// sales (e.g. two £50 cash gift-card creations) would hash to the same key. +func uniqueTillKey() string { + return "till-" + rand.Text() +} + func CreateTillSale(w http.ResponseWriter, r *http.Request) { ctx := r.Context() adminID, _ := ctx.Value(mw.UserIDKey).(string) @@ -266,7 +277,7 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { saleStatus = "completed" dbPaymentMethod = "cash" if req.IdempotencyKey == "" { - req.IdempotencyKey = "till-cash-" + giftCardID + "-" + clock.Now().Format("20060102150405.000000") + req.IdempotencyKey = uniqueTillKey() } case "saved_card": dbPaymentMethod = "online_square" @@ -295,7 +306,7 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { } if req.IdempotencyKey == "" { - req.IdempotencyKey = "till-sale-" + giftCardID + "-" + clock.Now().Format("20060102150405.000000") + req.IdempotencyKey = uniqueTillKey() } saleStatus = "pending" @@ -303,7 +314,7 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { case "card_machine": dbPaymentMethod = "in_person_card" if req.IdempotencyKey == "" { - req.IdempotencyKey = "till-terminal-" + giftCardID + "-" + clock.Now().Format("20060102150405.000000") + req.IdempotencyKey = uniqueTillKey() } checkoutReq := square.CreateCheckoutReq{ @@ -326,7 +337,7 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { case "online_square": dbPaymentMethod = "online_square" if req.IdempotencyKey == "" { - req.IdempotencyKey = "till-online-" + giftCardID + "-" + clock.Now().Format("20060102150405.000000") + req.IdempotencyKey = uniqueTillKey() } saleStatus = "pending" @@ -335,7 +346,7 @@ func CreateTillSale(w http.ResponseWriter, r *http.Request) { saleStatus = "completed" dbPaymentMethod = "on_the_house" if req.IdempotencyKey == "" { - req.IdempotencyKey = "till-on-the-house-" + giftCardID + "-" + clock.Now().Format("20060102150405.000000") + req.IdempotencyKey = uniqueTillKey() } } diff --git a/backend/handlers/payments/till_test.go b/backend/handlers/payments/till_test.go index 8c9dc12..2f2d5f8 100644 --- a/backend/handlers/payments/till_test.go +++ b/backend/handlers/payments/till_test.go @@ -880,3 +880,40 @@ func TestCreateTillSale_CreateCash(t *testing.T) { t.Errorf("expected 1 till_sale, got %d", saleCount) } } + +// TestCreateTillSale_TwoIdenticalCreateSales_BothSucceed is a regression test: +// two identical keyless cash gift-card creations must BOTH return 201. The +// idempotency-key fallback must be unique per request (not derived from request +// fields, which are identical for the two sales) or the second sale would +// collide on the till_sales idempotency_key UNIQUE constraint and return 500. +func TestCreateTillSale_TwoIdenticalCreateSales_BothSucceed(t *testing.T) { + t.Parallel() + _, tx := testutils.SetupTestTx(t) + + adminID, err := fixtures.CreateTestAdminUser(tx) + require.NoError(t, err) + adminToken := jwt.GenerateTestToken(adminID, "admin") + + for i := 0; i < 2; i++ { + reqBody := TillSaleRequest{ + ItemType: "gift_card", + Action: "create", + Amount: 50.00, + PaymentMethod: "cash", + } + + bodyBytes, _ := json.Marshal(reqBody) + req := httptest.NewRequest("POST", "/api/admin/till/sale", bytes.NewReader(bodyBytes)) + req.Header.Set("Authorization", "Bearer "+adminToken) + req.Header.Set("Content-Type", "application/json") + req = req.WithContext(db.ContextWithTx(req.Context(), tx.(pgx.Tx))) + + w := httptest.NewRecorder() + r := chi.NewRouter() + r.Use(mw.RequireAuth) + r.Post("/api/admin/till/sale", CreateTillSale) + r.ServeHTTP(w, req) + + require.Equal(t, http.StatusCreated, w.Code, "sale %d: expected 201, got %d. body: %s", i+1, w.Code, w.Body.String()) + } +} diff --git a/backend/internal/square/square_http_client.go b/backend/internal/square/square_http_client.go index bbed742..e61923e 100644 --- a/backend/internal/square/square_http_client.go +++ b/backend/internal/square/square_http_client.go @@ -3,6 +3,7 @@ package square import ( "bytes" "context" + "crypto/sha256" "encoding/json" "fmt" "io" @@ -343,12 +344,10 @@ func createCardOnFileHTTP(ctx context.Context, userID, cardToken string) (*CardO // Deterministic idempotency key derived from user + card (not time-based) // so that retries with the same details don't create duplicate cards. - ikHash := fmt.Sprintf("%x", []byte(userID+"|"+cardToken)) - if len(ikHash) > 64 { - ikHash = ikHash[:64] - } + // SHA-256 hash prevents recovering the card token from the key itself. + ikHash := sha256.Sum256([]byte(userID + "|" + cardToken)) body := sqCreateCardRequest{ - IdempotencyKey: fmt.Sprintf("create-card-%s", ikHash), + IdempotencyKey: fmt.Sprintf("create-card-%x", ikHash), SourceID: cardToken, Card: sqCardPayload{ CustomerID: userID, @@ -422,14 +421,12 @@ func paymentFromSquare(sq *sqPayment) *PaymentResult { r.CardFingerprint = cd.Card.Fingerprint r.ExpMonth = cd.Card.ExpMonth r.ExpYear = cd.Card.ExpYear + } else { + // Card details present but no card ID — still surface the brand/last4. + r.CardBrand = cd.Card.CardBrand + r.CardLast4 = cd.Card.Last4 } } - if r.CardBrand == "" && sq.CardDetails != nil && sq.CardDetails.Card.ID != "" { - r.CardBrand = sq.CardDetails.Card.CardBrand - } - if r.CardLast4 == "" && sq.CardDetails != nil && sq.CardDetails.Card.ID != "" { - r.CardLast4 = sq.CardDetails.Card.Last4 - } return r }