fix: review round 7 — fresh-eyes audit fixes (6 agents) + full test suites for every backend change

Fresh-eyes review round with 6 independent agents (money-safety, concurrency, Square wire parity, security, frontend flow, testing-gaps). Every finding was independently verified against the code before fixing. All backend changes now carry full test suites (10+ new tests, each verified to FAIL without its guard). All 20 packages green, race detector clean. Money-safety: - Gift-card purchase refunds no longer create money: manual refunds of a no-booking (gift-card purchase) payment are rejected with a clear message in the direct handler AND never re-issued by the sweep-resume path (processManualPaymentGroup skips them; reconcile-then-fail, no re-issue). - BuyGiftCard no-client-key fallback: derived deterministically under the advisory lock (pending-row reuse fixes lost-response double-charge; completed-row sequence advance preserves distinct-purchase collapse fix). - Terminal completion is never unrecorded: activeTerminalCheckoutID now calls recordUntrackedTerminalPayment when a provisional (tmp-) checkout is found COMPLETED at Square (previously only marked the row COMPLETED — a lost poll left the payment invisible and unrefundable). - Sweep: provisional tmp- checkout rows are resolved against Square first (COMPLETED → record; live → keep guard; NOT_FOUND/CANCELED → fail; ambiguous → leave pending) instead of blind-failing a possibly-live checkout. recordUntrackedTerminalPayment re-checks the booking status (FOR UPDATE) and refuses to record on a cancelled booking, inserting a critical_payment_log admin notification instead. Till-sale post-charge UPDATE now requires status='pending' (no resurrection of a clawed-back sale). Frontend (Svelte 5): - UserPaymentModal keeps CardSelection mounted through processing (bind:this ref + Square iframe survive the loyalty/tokenize awaits) — new-card payments work again. - BookingFlow clears the cached nonce/verification pair on any failure (retry re-tokenizes fresh; idempotency key retained for dedup); 409 'already paid' refetches the booking and reconciles depositPaid so the confirmation gate opens; Back button disabled during processing. - Synchronous double-submit guards on buyGiftCard/redeemGiftCard/submitTip. Square wire parity (mock vs real): - processing_fee sign unified (negated at paymentFromSquare; mock agrees). - SimulateSourceUsed (SOURCE_USED, 400) matches real CreateCard. - GetCardsOnFile excludes disabled cards (matches ListCards). - ForcePaymentStatus toggle + tests prove the charge path can't be status-blind. - CreateCheckout rejects empty device_id (env fallback SQUARE_TERMINAL_DEVICE_ID); completed terminal checkout's payment resolvable by id. Security: - 2FA attempt-map data race fixed: lastAt is atomic.Int64 (nanos) — eviction scan reads race-free; concurrent verify+evict tests under -race. - Backend refuses to start on weak/placeholder JWT_SECRET_KEY (<32 chars or known public placeholders) with openssl rand -hex 32 guidance. - Dockerfile no longer COPYs .env (secrets injected via compose env_file). - SabreDAV requires DAV_ADMIN_PASSWORD (no admin/admin default); compose fails at config time when missing. Testing gaps closed (each verified to FAIL without its guard): - refunded-dedup 409 (CreateBookingPayment), keyed sweep past-retention blind-fail, reconcile status-switch (CANCELED/FAILED/APPROVED/PENDING/unknown in both by-key and by-id paths), resolveChargeSource Square-failure branches, structured 500 / CARD_DECLINED / cancelled-context E2E (row stays pending), deriveBookingPaymentIdempotencyKey >45-char truncation, webhook findPaymentByDisputeID fallback, clawbackOneTillSale non-gift-card branch, dispute.evidence / terminal.checkout dispatch. Infra: - local-dev-2.sh fails loudly on port-5432 squatters / docker compose failures (previously died silently under ERR_EXIT with hidden output). - Test harness defaults SQUARE_TERMINAL_DEVICE_ID; money_safety_fixes_test.go gained the missing build tag. Verification: go test -tags test,dev -count=1 -parallel 8 ./... (20/20 ok), -race clean on 2FA + payments money paths, go build ./... + -tags dev, go vet clean, svelte-check 0 errors, env-docs gate OK (36 vars), docker compose config valid.
2026-08-22 00:34:49 +01:00
parent 67cf5b9a45
commit 5cc5a7f6d2
28 changed files with 3328 additions and 208 deletions
@@ -1686,3 +1686,100 @@ func TestDeleteCustomerHTTP(t *testing.T) {
 		}
 	})
 }
+
+// TestPaymentFromSquare_NegatesProcessingFees locks the processing-fee sign
+// convention (finding A): Square reports processing_fee amounts as NEGATIVE on
+// the wire, and paymentFromSquare must surface a POSITIVE PaymentResult.Fees
+// (the magnitude handlers store as p.fees). A fee of -95 on the wire must
+// become Fees == 95.
+func TestPaymentFromSquare_NegatesProcessingFees(t *testing.T) {
+	p := &sqPayment{
+		ID:         "pay_fee",
+		Status:     "COMPLETED",
+		TotalMoney: sqMoney{Amount: 5000, Currency: "GBP"},
+		ProcessingFee: []sqFee{
+			{AmountMoney: sqMoney{Amount: -95, Currency: "GBP"}, Type: "INITIAL"},
+			{AmountMoney: sqMoney{Amount: -20, Currency: "GBP"}, Type: "SECONDARY"},
+		},
+	}
+	result := paymentFromSquare(p)
+	if result.Fees != 115 {
+		t.Errorf("expected Fees 115 (sum of negated Square fees), got %d", result.Fees)
+	}
+
+	// A zero fee stays zero.
+	zero := paymentFromSquare(&sqPayment{ID: "pay_zero", Status: "COMPLETED", ProcessingFee: []sqFee{{AmountMoney: sqMoney{Amount: 0, Currency: "GBP"}}}})
+	if zero.Fees != 0 {
+		t.Errorf("expected Fees 0 for a zero fee, got %d", zero.Fees)
+	}
+}
+
+// TestPaymentFromSquare_StatusMapping verifies paymentFromSquare maps every
+// documented Square payment status FAITHFULLY (finding D): APPROVED, PENDING,
+// FAILED, CANCELED and COMPLETED all flow through into PaymentResult.Status.
+// The client never downgrades or drops a non-terminal status.
+func TestPaymentFromSquare_StatusMapping(t *testing.T) {
+	for _, status := range []string{"APPROVED", "PENDING", "FAILED", "CANCELED", "COMPLETED"} {
+		t.Run(status, func(t *testing.T) {
+			p := &sqPayment{ID: "pay_" + status, Status: status, TotalMoney: sqMoney{Amount: 5000, Currency: "GBP"}}
+			result := paymentFromSquare(p)
+			if result.Status != status {
+				t.Errorf("expected Status %q mapped verbatim, got %q", status, result.Status)
+			}
+		})
+	}
+}
+
+// TestDoJSON_200WithFailedPayment_NotDropped verifies doJSON does NOT silently
+// drop a 200-with-FAILED payment (finding D): a 2xx body carrying a FAILED
+// payment is parsed into a PaymentResult with Status "FAILED" and nil error —
+// the client surfaces the status faithfully instead of erroring, so a
+// status-blind handler (records 'completed' on nil error alone) is exposed.
+func TestDoJSON_200WithFailedPayment_NotDropped(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		_, _ = w.Write([]byte(`{"payment":{"id":"pay_failed_200","status":"FAILED","total_money":{"amount":5000,"currency":"GBP"},"location_id":"loc","created_at":"2026-07-31T00:00:00Z"}}`))
+	}))
+	defer srv.Close()
+
+	hc := &httpClient{baseURL: srv.URL, token: "t", http: srv.Client()}
+	res, err := createPaymentHTTPWithClient(context.Background(), CreatePaymentReq{
+		Amount: 5000, Currency: "GBP", SourceID: "cnon:test-card", IdempotencyKey: "ik-failed-200",
+	}, hc)
+	if err != nil {
+		t.Fatalf("a 200-with-FAILED payment must NOT error (the client is status-transparent), got %v", err)
+	}
+	if res.Status != "FAILED" {
+		t.Errorf("expected Status FAILED surfaced faithfully, got %q", res.Status)
+	}
+	if res.ID != "pay_failed_200" {
+		t.Errorf("expected the failed payment returned, got %+v", res)
+	}
+}
+
+// TestDoJSON_CardProcessingNotEnabled403 verifies a 403 CARD_PROCESSING_NOT_ENABLED
+// response surfaces the structured Square error with StatusCode 403 (finding E)
+// so the handlers agent can special-case it (errors.go, not owned here).
+func TestDoJSON_CardProcessingNotEnabled403(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		w.WriteHeader(http.StatusForbidden)
+		_, _ = w.Write([]byte(`{"errors":[{"category":"PAYMENT_METHOD_ERROR","code":"CARD_PROCESSING_NOT_ENABLED","detail":"Card processing is not enabled for this account."}]}`))
+	}))
+	defer srv.Close()
+
+	hc := &httpClient{baseURL: srv.URL, token: "t", http: srv.Client()}
+	err := hc.doJSON(context.Background(), http.MethodPost, "/v2/payments", nil, nil)
+	if err == nil {
+		t.Fatal("expected error for 403")
+	}
+	if got := ErrorStatusCode(err); got != http.StatusForbidden {
+		t.Errorf("expected ErrorStatusCode 403, got %d", got)
+	}
+	if got := ErrorCode(err); got != "CARD_PROCESSING_NOT_ENABLED" {
+		t.Errorf("expected ErrorCode CARD_PROCESSING_NOT_ENABLED, got %q", got)
+	}
+	if got := ErrorCategory(err); got != "PAYMENT_METHOD_ERROR" {
+		t.Errorf("expected ErrorCategory PAYMENT_METHOD_ERROR, got %q", got)
+	}
+}