Fix review findings: till idempotency keys, sha256 card key, paymentFromSquare fallback, regression test

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

@@ -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()
 		}
 	}
 

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())
+	}
+}

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
 }