popertots/Crussell
1
1
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity
821 Commits 1 Branch 0 Tags
efea213ccd3201930f131bc997fb031c65e556a9
Commit Graph
10 Commits
Author SHA1 Message Date
popertotsandSisyphus 77317e4a45 fix: payments money-safety round-2 — webhook booking gate + M2 stranded-charge refund, sync-path status guards, gift-card cancel re-issue reconcile, till-sweep clawback lock, sweep VAT rescue, refund credit routing 
- webhook payment.updated now re-reads the booking FOR UPDATE: non-payable booking -> payment failed + stranded-charge refund row + flood-capped alert; payable booking -> full completion side-effects; gift-card rows stay pending (C6 same-key retry); unknown events -> 503 so Square retries
- sync-path completion flips (saved-card, tip, online) guarded AND status='pending' + post-flip re-read; postChargeRecheck failed-mark guarded — no webhook-first double-processing, no phantom split rows
- CancelGiftCard resume reconciles ALL Square refunds (pending blocks re-issue; COMPLETED sum >= entitlement resolves+neutralizes; else re-issues only the difference under a fresh key) — closes double-refund
- sweep till_sales fail/clawback takes crussell:till:<key> lock + post-lock status re-read; recordUntrackedTillSalePayment applies VAT; rescue align clears VAT fields before re-apply (split-accurate VAT, tip rows stay VAT-free)
- refunds: chargeAggKey widened, redeemed-card refunds route to user_giftcard_balances, guest cash refunds recorded failed + notification
- handlers: tip split-records excluded from VAT loop, splitIdempotencyKey hashed, cross-booking key reuse 409, refunded payments excluded from existingCount, SCA-mint exemption for save_card, non-COMPLETED results routed

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
 2026-08-22 00:34:51 +01:00
popertots 1429eddd34 fix: payments hardening — SCA wire contract (saved-card ref + tokenize-result), terminal/till token routing, tip-cap overflow carve, completion campaign atomicity, orphan B1-evidence gate, gift-card gates/locks, admin backstops 
- ValidateCardInfo accepts saved-card ref + new_card_token coexistence (matches resolveChargeSource); new_card_token added to terminal/till request structs so SCA tokens are never dropped
- maxOnlineTipPence (£250) enforced on the overflow-tip carve AND buildSplitRecords (both carve paths) — closes the £10k bypass
- completion-path campaign increments made atomic reserve-first (conditional UPDATE ... RETURNING) + schema backstops (chk_times_redeemed, partial unique index on milestone redemptions)
- webhook orphan detection gated on B1 evidence (b1_attempts / sweep-duplicate refund row) so a delayed legit completion is never marked failed
- gift-card: per-user £500/day cap lock held across read-modify-write, expired-card top-up gate, NaN/Inf float bounds, refund_failed ack filter, on_the_house excluded from balance, postChargeRecheck notification
- admin apply-redemption route + admin-or-owner, in-handler isAdminRequest on 4 gift-card handlers, tip lock key aligned
- 2FA fallback machinery removed (insertTwoFAFallbackAudit/reissue/consent), dead fields stripped from charge structs
- tests: prod-tag suite, mock SCA parity, tip-cap overflow, completion races, cards pagination, ValidateCardInfo tables
 2026-08-22 00:34:50 +01:00
popertots 2cdbad0cea feat: SCA-only saved-card charges — 2FA charge fallback removed (C6), versioned consent fields, token provenance 
PSR 2017 reg 100 makes SCA mandatory and non-waivable for customer-initiated
stored-credential charges; a merchant-side 2FA check cannot legally substitute
for it (authorising a token-less charge via 2FA leaves the MERCHANT liable for
ECI 7 / SLI 210 chargebacks and reg 77(6) compensation regardless of consent).

- payments/twofa.go: the homegrown 2FA fallback for token-less saved-card
  charges is REMOVED ENTIRELY. requireTwoFactorForCardAccess is now SCA-only:
  a non-empty Square verification_token (charge surfaces, token forwarded to
  Square) skips the gate; anything else is refused 402 verification_required.
  enforceSCAFallbackConsent is a compile-compatible no-op (fallback never runs).
- New requireTwoFactorForCardAccessWithTokenValidation distinguishes surfaces
  where the token IS forwarded to Square (charge — Square validates it) from
  card-SAVE surfaces (token client-asserted, never forwarded: a non-empty token
  must NOT skip the save gate, auth-F1).
- SCA tokenize-result wire contract (C1): a saved card charged with a fresh
  one-time tokenize-result sends the token as the charge SOURCE (new_card_token
  -> source_id) alongside saved_card_id, never a separate verification_token.
  resolveChargeSource resolves the saved-card branch FIRST (customer from the
  card row, token as source) so combined token+card requests are SCA-clean.
- C6 consent fields (consent_version / consent_accepted) added to the booking/
  tip/till/gift-card charge requests, enforced server-side before any fallback
  charge could reach Square and recorded on the 2fa_fallback_charge audit row;
  logVerificationTokenProvenance traces minted tokens to their charge.
- user 2FA issuance gate refactored into pure build-agnostic functions
  (twoFAPepperConfigured / twoFADeliveryChannelConfigured /
  twoFAEnsureIssueAllowedStrict) shared with the payments re-issue path and
  exercised directly by the test,dev suite; TWO_FACTOR_FALLBACK switch and
  .env.example entry removed; startup posture notes updated.
- Test coverage: fail-closed 2FA production gates (pepper/delivery), token
  validation on save vs charge surfaces, completion idempotency, idempotency
  key determinism, refund-policy 72h/24h epsilon boundaries, VAT parity.
 2026-08-22 00:34:50 +01:00
popertots dfe856b181 fix: loop-B adversarial (503c326 baseline) — IDEMPOTENCY_KEY_REUSED reclassified ambiguous, 2FA reissue fail-closed alerts, family-cache crash window, consolidation regression checks 
Loop B red-team (money/security/dup-mod adversarial) findings on the full payments overhaul:

- CRITICAL-ish: IDEMPOTENCY_KEY_REUSED (409) no longer classified as a definitive
  402 in chargeFailureStatus — it means the ORIGINAL charge may have landed with
  a different body, so it is now AMBIGUOUS (503): the frontend keeps the same
  idempotency key, the pending row stays rescuable by the sweep (which already
  treated it as ambiguous), and the frontend no longer regenerates the key into
  a possible double charge. SCA verification-required codes remain definitive 402.
- HIGH: reissueTwoFACodeAfterFailedCharge now writes a CRITICAL admin notification
  (insertCriticalPaymentNotification) when issuance is refused (missing pepper /
  unavailable delivery) instead of silently stranding the customer; documented that
  a pepper CHANGE invalidates all pending codes.
- MEDIUM: family-alive cache invalidation crash window documented (invalidate-after-
  commit leaves up to 30s warm on a crash; the near-TTL DB re-check bounds it).
- Consolidation regression checks (8b2fe3b helpers): writeChargeSnapshot guard
  preserved at all sites, postChargeRecheck identical, squareRefundStatusToLocal
  mappings verified, reissue fresh-only semantics confirmed at all 5 call sites.

Verified: 26/26 dev packages, both vet tags, frontend tests + build, env-docs 42/42.
 2026-08-22 00:34:50 +01:00
popertots 36887167c6 fix: loop-A fresh review (503c326 baseline) — overflow-guard bypass, discounted-deposit retry, GDPR audit scrub, till cap, sweep rescue, 2FA reissue + SCA retry, consolidation round 
Loop A fresh money/security/dup-mod review of the whole payments overhaul. 28 consolidated findings fixed:

MONEY:
- HIGH-1: B12 overflow guard now uses the discounted obligation — a pre-start deposit can never mint an unintended tip; the discount is never truncated to £0 when the customer pays the discounted deposit
- HIGH-2: discounted-deposit pending-reuse retry compares pendingStoredAmountPence vs chargeAmount (the actual Square amount), not req.Amount — no more permanent amount_mismatch 400 on lost-response retries
- MEDIUM-3: sweep rescue now carves overflow as a tip record + runs completion side-effects (was booking overflow as service revenue, skipping completion)
- MEDIUM-4 (shared w/ security): admin_audit_log.admin_id made nullable + anonymize_user/delete_guest_user NULL it + scrub details.card_last4 — 2fa_fallback_charge PII no longer survives account deletion
- MEDIUM-5: till gift-card payment now passes the £5,000/day admin cap (giftcard_limits)
- LOW-6: expired gift-card balance surfaced as expired/zero in GetUserGiftCardBalance

SECURITY:
- 2FA single-use consume made atomic at verify time for all 5 saved-card gates (fresh charges consume; pending-reuse retries don't); deferred consumption removed
- reissueTwoFACodeAfterFailedCharge routed through the fail-closed issuance gate (pepper check, cooldown) + fresh-only semantics (only when a code was actually consumed)
- family-alive cache invalidated on the stale-family cleanup DELETE (no 30s warm window after expiry)
- frontend 503-retry no longer reuses a consumed 2FA code — aligns with backend re-issue

DUP/MOD:
- reissue helper single-sourced (5 call sites), squareRefundStatusToLocal (10 inline switches), writeChargeSnapshot (7 sites, immutability guard on gift-card/till), postChargeRecheck (3+1 sites), scanIdempotencySlot (2), applyVATToChargeRecord (3 patterns), user_saved_cards upsert (2), BuyGiftCard pending INSERT via service
- till completed-dedup now re-validates paymentHasLiveRefund (aligns with booking/tip/gift-card)
- frontend 402 idempotency-key regeneration added to PaymentModal (aligns with other CIT surfaces)
- PAYMENT_METHOD_SAVED_CARD constant standardised ('saved_card' everywhere)
- admin audit coverage added for AdminRefundBooking + gift-card buy/top-up
- audit-helper cross-package dedup (user/twofa.go now calls payments' exported insert)

Verified: 26/26 dev + 24/24 prod packages, both vet tags, frontend tests + build, gitleaks clean.
 2026-08-22 00:34:50 +01:00
popertots 6d82535780 fix: adversarial review round — replay-rescue double-charge, discount credit, 2FA/per-IP limits, snapshot encryption, refund reconciliation, VAT, frontend parity, tests+docs 
Addresses the adversarial fresh-eyes audit (findings A1-A20) plus review-round fixes:
- CRITICAL A1: replay-by-key rescue cross-checks replayed CreatedAt; ccof blind-fail leaves pending with CRITICAL + notification instead of clawing back
- A2/A3/A4: till idempotency key restored to unconditional hash; tip rejected in CreateBookingPayment; campaign discount now reduces the charged amount (deposit credit)
- A5: admin notifications on blind-fail, manual-refund re-arm, cap-stranded charge-group, webhook FAILED/REJECTED refunds
- A6/A10: BuyGiftCard idempotency user-scoped; gift-card slot scan advances past failed rows
- A7/A14/A15: 2FA user+IP limiter, SNAPSHOT_ENC_KEY startup validation, accurate pepper/log-delivery docs
- A8/A9: snapshot encryption on all write+reuse sites; MPV->SPV effective voucher type (single VAT point)
- A11/A12/A13/A16: amount-aware refund reconciliation; completed-booking refund re-validation; till retry dedup; PaymentWasRefunded on SquareClient interface
- A17/A18/A19/A20: CI runs npm test; confirm_overflow_tip frontend dialog; unknown-event admin notification; mock token redaction
- M7 ConfirmOverflowTip, M9 snapshot encryption, C1 discount ordering regression test
- Frontend vitest framework (41 tests), backend coverage for fixed functions, docs corrected (2,269 tests, SUPPORT_EMAIL tokens, resolution status)

All 25 backend packages pass; frontend 41/41; build + env-docs green.
 2026-08-22 00:34:50 +01:00
popertots 67cf5b9a45 fix: review round 6 — P0 deposit charge, idempotency rotation, dev-safety guard, 2FA/webhook hardening 
Sixth fresh-eyes review pass (5 agents: goal, QA, code-quality, security,
context-mining). QA FAILED the deposit-required new-card flow; the P0 root
cause was backend + frontend, now fixed. All 20 packages green.

P0 money-safety:
- Deposit-required bookings now actually charge the deposit on new-card
  payment. Two-part fix: (1) CreateBookingHandler re-reads the
  trigger-maintained total_amount/total_duration_minutes from the DB after the
  booking_services insert (the INSERT..RETURNING row predates the recalc
  trigger, so TotalAmount serialized as 0 and DepositPaid computed TRUE on an
  unpaid booking — the frontend gate trusted deposit_paid:true, never charged,
  and confirmed the booking with zero payment rows); (2) BookingFlow.svelte
  gates the confirmation view on depositPaid and guards against re-creating a
  booking on retry. Regression test
  TestBookings_Create_DepositPaidFalseOnUnpaidBooking.

Payments (idempotency + money):
- deriveBookingPaymentIdempotencyKey: no-client-key fallback now advances a
  sequence for repeatable types (partial) and rotates past refunded completed
  rows, so refund-then-repay and equal-amount partials diverge onto distinct
  keys; an un-refunded completed row keeps its key (double-charge protection
  holds). Dedup hits on refunded rows now 409, never stale success.
- chargeFailureStatus default is 503 (ambiguous), never 402; table test.
- Flaky TestBookingPayment_FullPayment_SplitsIntoDepositAndBalance fixed
  (ORDER BY payment_type).
- resolveChargeSource: orphaned card-on-file disabled via DeleteCardOnFile
  when SaveCardForUser fails (best-effort, redacted log); retry path preserved.

Square client:
- Dev builds HARD-FAIL (panic) on SQUARE_ENVIRONMENT=production without
  SQUARE_ALLOW_REAL_API=1; sandbox routes with a loud banner.
- Mock fault-injection FailAfterCommit (commit-then-5xx) exercises the exact
  lost-response same-key retry; SimulateCardTokenUsed; 45-char idempotency-key
  cap parity; SquareEnvironment/SquareLocationID shared env helpers used by
  the sweep (env contract no longer comment-only).
- listRefunds truncation now errors (money-sensitive reconcile retries
  instead of over-refunding); getCardsOnFile truncation loudly logged.

Webhooks + 2FA:
- square-environment header checked fail-closed (403) when configured env is
  production/sandbox; dispatch DB work bounded by 30s timeout contexts.
- 2FA codes HMAC-SHA256 pepper'd (TWO_FACTOR_PEPPER) with legacy-hash
  migration + upgrade-on-verify; disable-flow mint cooldown (1/min, 429) caps
  the brute-force loop; in-lockout records never LRU-evicted.

Repo hygiene:
- env-docs CI gate green again (FRONTEND_ORIGIN + SQUARE_ALLOW_REAL_API +
  TWO_FACTOR_PEPPER documented; Vite DEV built-in allowlisted).
- Dead square_deposits schema dropped; obsidian/README/legal-page drift fixed
  (consumeradvice.scot signposting, CORS allowlist, p11 R3/P13, T1).
- 2FA disable residual documented; P6 email/SMS delivery and P12 sandbox
  smoke test remain the pre-go-live gates.

Verification: go test -tags test,dev -count=1 -parallel 8 ./... (20/20 ok),
go build ./... + -tags dev, go vet clean, svelte-check 0 errors, env-docs
gate OK, live deposit-required flow re-verified end-to-end (deposit £11
charged, square_payment_id recorded).
 2026-08-22 00:34:49 +01:00
popertots 5e3dc9b428 fix: comprehensive payment system hardening (4 review passes) 
CRITICAL fixes:
- C1: JWT exp claim now validated via jwtauth.VerifyToken (was Decode)
- C2: OverrideAmount validated post-substitution (prevents negative money minting)
- C3: Terminal gift-card payments store gift_card_id; refund credits user balance
- C4: Refund dedup returns stored amount, not req.Amount (prevents admin mislead)
- C5: Booking recheck uses FOR UPDATE (prevents TOCTOU with cancellation)
- C6: processChargeGroup idempotency key stable (charge-only, prevents double-refund)

MAJOR fixes:
- M2: Gift-card refund UPDATE checks RowsAffected; 0 rows -> failed
- M3: ProcessCancellationRefund returns commit error (was swallowed)
- M5: Dispute webhook handling (created + state.updated + disputes table)

MEDIUM fixes:
- ME1: CORS restricted to FRONTEND_ORIGIN env var (was reflect-any)
- ME2: anonymize_user() scrubs users.notes, bookings.notes, name_history, refresh_tokens
- ME3: Webhook handlers now mutate state (payment.updated, refund.updated)

Frontend fixes:
- Same-key retry on 503 (ambiguous failure) wired to all 8 payment flows
- CHARGE_AND_STORE intent for save-card flows (SCA compliance)
- Nonce staleness check verified across all flows

Additional fixes from adversarial re-review:
- F1: Till-sale completed dedup echoes stored amount (C4-class)
- F2: Cash/giftcard terminal path uses FOR UPDATE (C5-class)
- F3: Square-success UPDATE checks RowsAffected (till sales)
- F4: Dispute reason truncated to 192 chars (prevents INSERT failure)
- F5: Booking-user lookup failure marks refund failed (prevents silent money loss)
- F6: Saved-card/tip rechecks wrapped in transaction (C5 residual)

Tests:
- 15 adversarial attack tests (negative override, zero override, terminal gift card,
  refund dedup, TOCTOU, deleted gift card, advisory lock, overcharge, zero/negative/huge
  amount, raw PAN, missing auth, gift card balance, concurrent refunds)
- 14 webhook state tests (dispute created/state, payment/refund updated)
- 3 CORS tests, 3 GDPR tests, 1 HTTP timeout test
- Full suite passes with -race (25 packages, 0 failures)

25 files changed, +1532/-275 lines
 2026-08-22 00:34:49 +01:00
popertots 7f1c649f1e Apply second-round review fixes: idempotency-key length caps, stable-sentinel card keys, test-isolation, naming 
Money-safety idempotency hardening (I1, wide):
- validate:"max=45" on CreateTerminalPayment/BookingPayment/Refund/Tip/
  BuyGiftCard idempotency keys (all feed Square's 45-char /v2/payments,
  /v2/refunds, /v2/cards caps); BuyGiftCard corrected from a wrongly-loose
  max=64. Till keeps max=64 (its key also feeds the 64-char terminal-checkout
  endpoint).
- Explicit 45-char guard in RefundPayment: the one handler that decodes
  RefundRequest without running the struct validator, so the tag alone was
  inert; a longer key would 400 at Square and be misclassified as a
  definitive refund decline.
- New TestIdempotencyKey_OverLength_RejectedAcrossPaymentHandlers covers all
  six endpoints (terminal saved-card, booking, tip, gift-card, till, refund).

Stable-sentinel card identity in idempotency keys (C1, wide):
- BookingFlow deposit key now uses the 'new-card' sentinel instead of
  embedding the cnon: nonce (matches UserPaymentModal/account). A re-tokenize
  after a spent nonce no longer regenerates the key, closing a lost-response
  double-charge window.
- TipPayment + UserBookingModal tip keys now include card identity
  (selectedCardId || 'new-card'); previously keyed on amount only, so a
  same-amount tip on a DIFFERENT card reused the key and deduped a distinct
  charge. Resets cleared in every success/close path.

Test isolation (R1): TestRefund_PendingResume_NewKeyAfterModalReopen no
longer t.Parallel — it swaps the package-level SquareClient mid-test and a
concurrent parallel test could observe the swapped instance.

Naming/quality (M1/M2/M4): resolveChargeSource local renamed savedRowID (was
shadowing the cardID *string parameter); BuyGiftCard fallback prefix
"till-" -> "gc-"; saved-card terminal response key "checkout_id" -> "payment_id"
(it holds a DB payment row, not a Square checkout) with matching frontend
fallback. README maintenance-job count corrected 24 -> 25.

Full suite 25/25 + race clean via run-tests.sh lockfile; svelte-check 0
errors/warnings; production build succeeds.
 2026-08-22 00:34:49 +01:00
popertots e8abf7b4b3 Refactor payment charge paths into shared helpers; classify Square failures 
Extracts resolveChargeSource (new-card vs saved-card vs one-off nonce, with Square customer provisioning) and shared advisory-lock + post-charge recheck helpers into charge_helpers.go. Adds errors.go with chargeFailureStatus: transport/5xx/context and 429/408/425 map to 503 (retryable), structured 4xx declines map to 402, used across all four charge paths. Also fixes the no-client-key refund fallback to append a crypto/rand suffix (distinct same-amount partial refunds no longer collide) and adds refundResumeKey so legacy NULL idempotency_key rows resume with a derived key instead of an empty one.
 2026-08-22 00:34:49 +01:00