fix: review-loop hardening — identical-body replay, 2FA gates, webhook at-least-once, GDPR scrub

Follow-up to the comprehensive payment-system review. Fixes the issues the review found in the initial integration, plus the rough edges it introduced. Money-safety: - Replay-by-key now replays the FULL original request verbatim from a stored square_request_snapshot, so a retained idempotency key returns the original payment instead of IDEMPOTENCY_KEY_REUSED (previously the row sat pending forever). IDEMPOTENCY_KEY_REUSED remains ambiguous (never proof of no charge). - Dev mock mirrors real Square for unknown-key replays: ccof: saved-card sources are charged and rescued; spent cnon: nonces surface ErrReplayKeyNotRetained. (Fixes dev/prod parity divergence.) - Webhook dedup row committed AFTER dispatch (at-least-once); FAILED till sales claw back gift-card funding; event-type strings match Square's real catalog. - Expired-gift-card cancellation refunds set creditFailed (never a phantom 'completed' refund); cancellation refunds lock all payment rows ascending. - Sweep never rescue-completes a gift-card purchase without delivering the card. - Tip no-client-key fallback is a deterministic count-based key under the booking advisory lock (retry-safe, distinct tips don't collapse). - M-cap subtracts completed refunds, clamped to [0, total]. 2FA (PSD2 SCA stand-in) for online saved-card payments: - Full feature: status/setup/verify/disable endpoints, gating helper wired into all 7 saved-card charge paths (incl. BuyGiftCard + admin saved-card), account admin-tab settings UI, frontend gating across all payment surfaces. - Enforcement is FAIL-CLOSED: on unless REQUIRE_2FA=false or an explicit mock/dev SQUARE_ENVIRONMENT; startup warning when off in a non-dev env. - Verify is brute-force hardened (5-attempt lockout, timing-safe compare); plaintext codes only logged when enforcement is off (dev). - GDPR: anonymize_user also scrubs 2FA columns and staff notes. Infra/docs: - nginx: /api/ response cache removed (cross-user disclosure); port 80 redirects to HTTPS (localhost/RFC1918 exempt, end-anchored regexes); HSTS; separate webhook rate-limit zone. - Schema: users 2FA columns; payments/till_sales square_source_id + square_request_snapshot. - Legal docs: gift-card cooling-off, international-transfers section, tips policy; Gap Backlog P3 webhooks marked done; stale counts/wording corrected. - Flaky test race fixed (t.Parallel + global mock mutation); suite 26/26 packages green, 2,142 tests, svelte-check clean.
2026-08-22 00:34:49 +01:00
parent 4b28e93710
commit e9b0f0f2a7
50 changed files with 4223 additions and 413 deletions
@@ -11,6 +11,7 @@ import (
 	"testing"

 	"crussell/db"
+	"crussell/testutils/fixtures"
 )

 // =============================================================================
@@ -99,6 +100,157 @@ func deliverWebhook(t *testing.T, event SquareWebhookEvent) *httptest.ResponseRe
 	return makeWebhookRequest(body, sig, context.Background())
 }

+// createWebhookTestGiftCardAndSale seeds a pending gift-card till sale tied to
+// a Square payment id and returns the sale id and gift card id. When
+// cardCreatedAt == saleCreatedAt the sale created the card (is_create → action
+// 'create'); otherwise the card pre-exists (action 'topup'). cardAmount is the
+// card's starting total_funds_added/amount_remaining.
+func createWebhookTestGiftCardAndSale(t *testing.T, squarePaymentID, cardCreatedAt, saleCreatedAt string, cardAmount float64) (saleID, giftCardID string) {
+	t.Helper()
+	adminID, err := fixtures.CreateTestAdminUser(db.Conn)
+	if err != nil {
+		t.Fatalf("failed to create admin user: %v", err)
+	}
+	if err := db.Conn.QueryRow(context.Background(), `
+		INSERT INTO gift_cards (total_funds_added, amount_remaining, created_by, is_inventory, voucher_type_at_purchase, created_at)
+		VALUES ($1, $1, $2, FALSE, 'SPV', $3::timestamptz)
+		RETURNING id
+	`, cardAmount, adminID, cardCreatedAt).Scan(&giftCardID); err != nil {
+		t.Fatalf("failed to create gift card: %v", err)
+	}
+	if err := db.Conn.QueryRow(context.Background(), `
+		INSERT INTO till_sales (item_type, item_id, description, quantity, unit_price, total_amount,
+			payment_method, status, square_payment_id, created_by, created_at, updated_at)
+		VALUES ('gift_card', $1, 'webhook clawback test', 1, 40.00, 40.00, 'online_square', 'pending',
+			$2, $3, $4::timestamptz, NOW())
+		RETURNING id
+	`, giftCardID, squarePaymentID, adminID, saleCreatedAt).Scan(&saleID); err != nil {
+		t.Fatalf("failed to create pending till sale: %v", err)
+	}
+	return saleID, giftCardID
+}
+
+// deliverPaymentUpdatedFailed dispatches a payment.updated webhook carrying a
+// definitively FAILED Square status for the given Square payment id.
+func deliverPaymentUpdatedFailed(t *testing.T, squarePaymentID string) *httptest.ResponseRecorder {
+	t.Helper()
+	event := SquareWebhookEvent{
+		Type:      "payment.updated",
+		EventID:   "evt_" + squarePaymentID,
+		CreatedAt: "2025-01-01T00:00:00Z",
+		Data: json.RawMessage(`{
+			"type": "payment",
+			"id": "` + squarePaymentID + `",
+			"object": {
+				"payment": {
+					"id": "` + squarePaymentID + `",
+					"status": "FAILED"
+				}
+			}
+		}`),
+	}
+	return deliverWebhook(t, event)
+}
+
+func getTillSaleStatus(t *testing.T, id string) string {
+	t.Helper()
+	var status string
+	if err := db.Conn.QueryRow(context.Background(),
+		"SELECT status FROM till_sales WHERE id = $1", id).Scan(&status); err != nil {
+		t.Fatalf("failed to read till_sales status: %v", err)
+	}
+	return status
+}
+
+func getGiftCardFunding(t *testing.T, id string) (totalFundsAdded, amountRemaining float64) {
+	t.Helper()
+	if err := db.Conn.QueryRow(context.Background(),
+		"SELECT total_funds_added, amount_remaining FROM gift_cards WHERE id = $1", id).Scan(&totalFundsAdded, &amountRemaining); err != nil {
+		t.Fatalf("failed to read gift card funding: %v", err)
+	}
+	return totalFundsAdded, amountRemaining
+}
+
+// =============================================================================
+// Till-sale gift-card clawback — payment.updated FAILED/CANCELED
+// =============================================================================
+
+// TestWebhook_PaymentUpdated_Failed_ClawsBackCreatedCard verifies that a
+// definitively-failed Square charge (FAILED) claws back the gift-card funding
+// of a pending till sale that CREATED the card: the card and its purchase
+// transaction are deleted and the sale is marked failed, exactly as the sweep
+// does.
+func TestWebhook_PaymentUpdated_Failed_ClawsBackCreatedCard(t *testing.T) {
+	const squarePaymentID = "sqp_clawback_create"
+	const cardCreatedAt = "2025-01-01T00:00:00Z"
+	saleID, giftCardID := createWebhookTestGiftCardAndSale(t, squarePaymentID, cardCreatedAt, cardCreatedAt, 40.00)
+
+	w := deliverPaymentUpdatedFailed(t, squarePaymentID)
+	if w.Code != http.StatusOK {
+		t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String())
+	}
+	if got := getTillSaleStatus(t, saleID); got != "failed" {
+		t.Errorf("expected till sale 'failed', got %q", got)
+	}
+	if exists := giftCardExists(t, giftCardID); exists {
+		t.Error("expected created gift card to be deleted by the clawback")
+	}
+}
+
+// TestWebhook_PaymentUpdated_Failed_ClawsBackTopup verifies the top-up
+// clawback for a pre-existing card: the sale's funding is subtracted back out
+// of the card and the sale is marked failed.
+func TestWebhook_PaymentUpdated_Failed_ClawsBackTopup(t *testing.T) {
+	const squarePaymentID = "sqp_clawback_topup"
+	saleID, giftCardID := createWebhookTestGiftCardAndSale(t, squarePaymentID, "2025-01-01T00:00:00Z", "2025-01-02T00:00:00Z", 60.00)
+
+	w := deliverPaymentUpdatedFailed(t, squarePaymentID)
+	if w.Code != http.StatusOK {
+		t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String())
+	}
+	if got := getTillSaleStatus(t, saleID); got != "failed" {
+		t.Errorf("expected till sale 'failed', got %q", got)
+	}
+	total, remaining := getGiftCardFunding(t, giftCardID)
+	if total != 20.00 || remaining != 20.00 {
+		t.Errorf("expected top-up clawback to leave £20.00 on the card, got total=%v remaining=%v", total, remaining)
+	}
+}
+
+// TestWebhook_PaymentUpdated_Failed_AlreadyResolved_Skipped verifies the
+// clawback skips without error when the till sale is already resolved (not
+// pending): the webhook still acknowledges 200 and leaves the terminal state
+// untouched.
+func TestWebhook_PaymentUpdated_Failed_AlreadyResolved_Skipped(t *testing.T) {
+	const squarePaymentID = "sqp_clawback_resolved"
+	saleID, giftCardID := createWebhookTestGiftCardAndSale(t, squarePaymentID, "2025-01-01T00:00:00Z", "2025-01-01T00:00:00Z", 40.00)
+	if _, err := db.Conn.Exec(context.Background(),
+		"UPDATE till_sales SET status = 'completed', updated_at = NOW() WHERE id = $1", saleID); err != nil {
+		t.Fatalf("failed to resolve till sale: %v", err)
+	}
+
+	w := deliverPaymentUpdatedFailed(t, squarePaymentID)
+	if w.Code != http.StatusOK {
+		t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String())
+	}
+	if got := getTillSaleStatus(t, saleID); got != "completed" {
+		t.Errorf("expected resolved till sale to stay 'completed', got %q", got)
+	}
+	if total, remaining := getGiftCardFunding(t, giftCardID); total != 40.00 || remaining != 40.00 {
+		t.Errorf("expected gift card untouched when the sale is already resolved, got total=%v remaining=%v", total, remaining)
+	}
+}
+
+func giftCardExists(t *testing.T, id string) bool {
+	t.Helper()
+	var n int
+	if err := db.Conn.QueryRow(context.Background(),
+		"SELECT COUNT(*) FROM gift_cards WHERE id = $1", id).Scan(&n); err != nil {
+		t.Fatalf("failed to count gift cards: %v", err)
+	}
+	return n > 0
+}
+
 // =============================================================================
 // Dispute handling — dispute.created
 // =============================================================================