popertots/Crussell
1
1
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix review findings: aggregated-refund/saved-card/legacy-refund idempotency keys, structured Square error classification, CSP for Square SDK

Browse Source 
Money-safety idempotency fixes (external review bugs 1-3):
- processChargeGroup: aggregated refund key now hashes the sorted pending-row
  set (chargeID-square-agg-<sha256 suffix>) so a changed group can never mark
  a new row completed against an old smaller refund; >45-char chargeIDs use a
  hashed prefix instead of verbatim truncation (which would collide charges on
  Square's global key dedup). Same-set crash-retry keeps Square's dedup.
- CreateTerminalPayment saved_card: two-tier idempotency key — client-supplied
  per-attempt UUID preferred (distinct identical charges no longer collapse),
  deterministic booking+type+amount+card fallback for no-key retry safety.
  PaymentModal sends a per-charge UUID cleared after success.
- ensureRefundKey: legacy NULL-key manual refunds persist a generated key to
  the row BEFORE the Square call (race-safe AND idempotency_key IS NULL guard),
  so a lost-response retry reuses the key and never double-refunds. Wired into
  resumeManualPendingRefund and the sweep's manual-retry loop.

Classification + money-safety hardening:
- till.go/sweep.go: structured square.ErrorCode/IsNotFound are authoritative
  when present; message-substring matching only for non-structured errors
  (dev mock, client-side status errors). Fixes fragile string-matching driving
  sweep retries and gift-card clawbacks.
- SaveCardForUser: ON CONFLICT (user_id, square_card_id) DO NOTHING + re-select
  (was a latent UNIQUE-violation 500 on save-card retry).
- CreateBookingPayment: partial payments re-validated against remaining balance
  inside the advisory lock (closes concurrent-overpayment race).
- InvalidateSquareCustomerCache on GDPR erasure paths (account.go,
  time-blockers.go stale-guest anonymization).
- GetUserGiftCardBalanceAdmin: in-handler admin check (defense-in-depth).
- getCheckoutHTTP: warn on multi-payment checkouts instead of dropping
  payments[1:].
- Cash/giftcard terminal branch: removed dead idempotency SELECT, "tip-" ->
  "till-" prefix.
- UserPaymentModal: removed vestigial polling state; proper interval cleanup.
- account/+page.svelte: gift-card redeem dialog links /terms.
- nginx CSP: allow *.squarecdn.com and js.squareup.com so the Square Web
  Payments SDK + card iframe can tokenize behind the proxy.

Tests: +8 regression tests covering changed-set refund keys, legacy NULL-key
single-refund, saved-card client-key dedup/no-dedup, concurrent partials, and
cache invalidation. Full suite + race detector clean via run-tests.sh lockfile.
This commit is contained in:
popertots
2026-08-22 00:34:49 +01:00
parent 726ac8cb65
commit a8d54f1e2a
21 changed files with 1279 additions and 111 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/handlers/scheduling/time-blockers.go
+39 -5
View File
@@ -416,6 +416,10 @@ func CleanupOldReservations(ctx context.Context) (int, error) {
// Financial records (bookings, payments) remain intact — only PII is scrubbed.
// Active/pending bookings are excluded so the salon can still contact the guest.
func AnonymizeStaleGuestAccounts(ctx context.Context) (int, error) {
// Stale guests whose Square customers are deleted below — their
// process-local cache entries must be invalidated after the anonymization.
var staleGuestUserIDs []string
// Best-effort: disable stale-guests' saved cards at Square BEFORE the SQL
// below NULLs square_card_id, so those cards can't keep accepting ccof:
// charges after anonymization (GDPR erasure completeness). A Square failure
@@ -431,7 +435,7 @@ func AnonymizeStaleGuestAccounts(ctx context.Context) (int, error) {
// local anonymization must never be blocked by Square. Rows are
// selected with the same stale-guest predicate the users UPDATE uses.
rows, err := db.Conn.Query(ctx, `
SELECT usc.square_card_id, usc.square_customer_id
SELECT usc.user_id, usc.square_card_id, usc.square_customer_id
FROM user_saved_cards usc
JOIN users u ON u.id = usc.user_id
WHERE u.account_role = 'guest'
@@ -449,12 +453,17 @@ func AnonymizeStaleGuestAccounts(ctx context.Context) (int, error) {
// customer) are skipped.
customerSeen := map[string]bool{}
var customerIDs []string
userSeen := map[string]bool{}
for rows.Next() {
var cardID, customerID sql.NullString
if err := rows.Scan(&cardID, &customerID); err != nil {
var userID, cardID, customerID sql.NullString
if err := rows.Scan(&userID, &cardID, &customerID); err != nil {
log.Printf("Warning: Failed to scan stale-guest saved card: %v", err)
continue
}
if userID.Valid && userID.String != "" && !userSeen[userID.String] {
userSeen[userID.String] = true
staleGuestUserIDs = append(staleGuestUserIDs, userID.String)
}
if cardID.Valid && cardID.String != "" {
cardIDs = append(cardIDs, cardID.String)
}
@@ -607,7 +616,18 @@ func AnonymizeStaleGuestAccounts(ctx context.Context) (int, error) {
}
totalRows += int(tag.RowsAffected())
return totalRows, tx.Commit(ctx)
if err := tx.Commit(ctx); err != nil {
return 0, err
}
// The guests' Square customer ids were deleted above and the DB columns are
// now NULLed — drop their process-local cache entries so an erased guest's
// stale Square customer id cannot resurface on a later save-card flow.
for _, uid := range staleGuestUserIDs {
payments.InvalidateSquareCustomerCache(uid)
}
return totalRows, nil
}
func CleanupExpiredLoyaltyRedemptions(ctx context.Context) (int, error) {
@@ -1111,7 +1131,21 @@ func CleanupIdleAccounts(ctx context.Context) (int, error) {
return 0, fmt.Errorf("failed to anonymize idle accounts: %w", err)
}
return len(accountsWithBalance) + len(accountsNoBalance), tx.Commit(ctx)
if err := tx.Commit(ctx); err != nil {
return 0, err
}
// The anonymize_user(unnest(...)) calls above erased these accounts — drop
// their process-local Square customer cache entries so a stale id cannot
// resurface for an erased user.
for _, acc := range accountsWithBalance {
payments.InvalidateSquareCustomerCache(acc.id)
}
for _, id := range accountsNoBalance {
payments.InvalidateSquareCustomerCache(id)
}
return len(accountsWithBalance) + len(accountsNoBalance), nil
}
// CleanupOldIdempotencyKeys clears idempotency keys from bookings, payments, and
backend/handlers/scheduling/time_blockers_test.go
+63
View File
@@ -3797,3 +3797,66 @@ func TestTimeBlockers_List_WithDateFilter_RecurringOnly(t *testing.T) {
t.Errorf("expected 'Only Recurring', got %s", response[0].Description)
}
}
// TestAnonymizeStaleGuestAccounts_InvalidatesSquareCustomerCache verifies the
// stale-guest anonymization drops each erased guest's process-local Square
// customer cache entry (GDPR erasure completeness): after the guest is
// anonymized (email → anon-{id}@anon.invalid, square_customer_id NULLed), a
// later save-card flow must re-mint a fresh Square customer instead of reusing
// the deleted one's stale cached id. Deliberately NOT t.Parallel: it swaps the
// package-level payments.SquareClient.
func TestAnonymizeStaleGuestAccounts_InvalidatesSquareCustomerCache(t *testing.T) {
ctx, tx := resetTestData(t)
guestID, err := fixtures.CreateTestUser(tx)
if err != nil {
t.Fatalf("failed to create guest user: %v", err)
}
if _, err := tx.Exec(ctx, `UPDATE users SET account_role = 'guest' WHERE id = $1`, guestID); err != nil {
t.Fatalf("failed to set guest role: %v", err)
}
// Stale booking (> 6 months) so the stale-guest predicate matches.
if _, err := tx.Exec(ctx, `
INSERT INTO bookings (user_id, start_time, status, deposit_required)
VALUES ($1, NOW() - INTERVAL '7 months', 'completed', false)
`, guestID); err != nil {
t.Fatalf("failed to create stale booking: %v", err)
}
// A saved card so the provisioning path has a persistence point.
if _, err := tx.Exec(ctx, `
INSERT INTO user_saved_cards (user_id, square_card_id, brand, last_4, exp_month, exp_year, fingerprint, is_default)
VALUES ($1, 'ccof:cache_stale_card', 'Visa', '4242', 12, 2030, 'fp1', true)
`, guestID); err != nil {
t.Fatalf("failed to insert saved card: %v", err)
}
origSquare := payments.SquareClient
payments.SquareClient = square.NewDevClient()
defer func() { payments.SquareClient = origSquare }()
t.Cleanup(func() { payments.InvalidateSquareCustomerCache(guestID) })
svc := payments.NewPaymentService()
originalID, err := svc.EnsureSquareCustomer(ctx, guestID)
if err != nil {
t.Fatalf("failed to provision Square customer: %v", err)
}
if originalID == "" {
t.Fatal("expected a provisioned Square customer id")
}
if _, err := AnonymizeStaleGuestAccounts(ctx); err != nil {
t.Fatalf("AnonymizeStaleGuestAccounts failed: %v", err)
}
// The guest is anonymized and the cache must have been invalidated:
// re-provisioning re-queries the NULLed DB and mints a fresh customer from
// the anonymized email — a different id. Without the invalidation it would
// return the stale originalID for the erased guest.
reprovisioned, err := svc.EnsureSquareCustomer(ctx, guestID)
if err != nil {
t.Fatalf("failed to re-provision Square customer: %v", err)
}
if reprovisioned == originalID {
t.Errorf("anonymized stale guest must not reuse the deleted Square customer id %q from the cache", originalID)
}
}