docs: consolidated payments review session report (findings -> fixes, full verification matrix)

2026-08-22 00:34:50 +01:00
parent 6bd952238e
commit 95d2ee3ccf
1 changed files with 88 additions and 0 deletions
@@ -0,0 +1,88 @@
 # Crussell Payments Review — CONSOLIDATED SESSION REPORT
 **Base:** commit `503c326297e6edc6b9681bd8025cd904a6a4965f` → working tree
 **Scope:** 279 files, +84,719 / −7,584 lines — the Square payments mock→realistic integration + all fixes.
 **Process:** 12 concern-review agents → review-work skill (5 agents: goal/QA/code-quality/security/context) → fix batch 1 (7 agents) → docs+tests batch 2 (2 agents) → re-review loop (regression, adversarial, SCA/2FA) → adversarial round 3 → final hardening. All backend tests run via `backend/run-tests.sh` (flock-wrapped) to prevent test-DB clobber.
 ## FINAL STATE — ALL TESTS GREEN
 - Backend: full suite `go test -tags "test,dev" ./...` — **28 packages pass**, 2,611 test functions.
 - Prod-tag fail-closed suite (`run-prod-tag-tests.sh`, `-tags "test,!dev"`) — passes (real prod 2FA branches now compiled + tested).
 - Frontend: `svelte-check` 0 errors, `vitest` **147 tests pass** (was 130; +policy cross-check, +SCA dialog component test, +mock token-shape parity).
 - `scripts/check-env-docs.py`: OK (39 env vars documented).
 - Runtime smoke (live mock backend): all keystone journeys verified end-to-end.
 ---
 ## EVERY ISSUE FOUND → FIX
 ### CRITICAL
 1. **SCA wire contract broken on all 4 customer charge surfaces** — BookingFlow/UserPaymentModal/TipPayment/account gift-card buy sent `card_id` + `new_card_token` together → backend `ValidateCardInfo` rejected 400. Backend `CreateTipPaymentRequest`/`BuyGiftCardRequest` had no `saved_card_id`. **Fixed:** 3-arg `ValidateCardInfo` accepts saved-ref+tokenize coexistence (matches `resolveChargeSource` + pinned contract); frontend uses `new_card_token: newCardToken ?? verificationToken` precedence everywhere. Verified at runtime (SourceID = `cnon:sca-...`).
 2. **Admin/till SCA token silently dropped** — `CreateTerminalPaymentRequest`/`TillSaleRequest` lacked `new_card_token` → Go JSON decoder discarded it → naked ccof → 402 in enforced env. **Fixed:** field added + routed through `resolveChargeSource`; gate-skip added for terminal/till. Runtime-verified (SourceID carries token).
 3. **`{{SUPPORT_EMAIL}}` + fake contact data shipped live** in terms/privacy/contact pages. **Fixed:** centralised `frontend/src/lib/constants/contact.ts`; placeholders removed; contact page uses constants.
 ### HIGH
 4. **Delete-account always failed** — frontend sent empty DELETE body; backend requires `current_password` + 2FA code. **Fixed:** dialog collects password + on-demand 2FA code (minted via `/api/user/2fa/code`), sent in body. Runtime-verified 204.
 5. **Admin loyalty redemption 404** — `/api/admin/bookings/{id}/apply-redemption` had no route. **Fixed:** route registered under RequireAdmin; `loyalty.go` owner check relaxed to admin-or-owner. Runtime-verified.
 6. **Admin "mark completed" unreachable** — `PUT /admin/bookings/{id}/progress` had zero frontend callers. **Fixed:** wired "Begin appointment" (CurrentAppointment) + "Complete" (BookingModal). Runtime-verified.
 7. **DAV default admin password defeated fail-closed guard** — `.env.example` shipped `changeme-admin-password` not in the weak list → copy-paste deployment exposes all customer vCards. **Fixed:** weak list expanded + entropy gate; `.env.example` ships empty `DAV_ADMIN_PASSWORD=` (compose `:?` fails closed).
 8. **Login lockout DoS with no recovery** — 5-fail lockout, dead reset flow. **Fixed:** dummy-bcrypt on no-user login (kills timing oracle); `password_reset` purpose now clears `failed_attempts`/`locked_until` after code verify.
 9. **User gift-card £500/day cap bypassable by concurrency** — lock keyed on idempotency key. **Fixed:** per-user advisory lock `crussell:giftcard-user-cap:<userID>` held across read-modify-write.
 10. **TopUpGiftCard could top up an EXPIRED card** (reviving forfeited balance). **Fixed:** expiry gate added to admin top-up (till already had it).
 11. **UserPaymentModal infinite fetch loop** — `$effect` re-triggered on store `loading` toggle → 983+ API calls. **Fixed:** one-shot guard + idempotent store.
 12. **Money displayed 100× too small** — `formatCurrency(totalPaid / 100)` where totalPaid already pounds. **Fixed:** `formatCurrency(totalPaid)`.
 13. **postChargeRecheck stranded money silently** — no refund row, no notification. **Fixed:** flood-capped critical notification raised.
 14. **`payment.completed` orphan detection could fail a legit completed charge** (delayed webhook). **Fixed in two passes:** (a) `SweepKeyedReplayAge` age gate; (b) **B1-EVIDENCE GATE** (round 3): failed-mark now requires the sweep's actual B1 markers (`b1_attempts > 0` or a `duplicate charge — sweep replay` refund row); no evidence → origin left pending for the sweep to reconcile. New test locks the no-evidence path.
 ### MEDIUM
 15. **Online card SAVE unusable in enforced env** — gate matched only mock's fictional `cnon:sca-`. **Fixed:** genuineness derived from Square acceptance of any token-like source.
 16. **Payment 2FA gate dead UI + fallback machinery** — frontend sent codes backend never read; `insertTwoFAFallbackAudit`/`reissueTwoFACodeAfterFailedCharge`/`enforceSCAFallbackConsent` unreachable. **Fixed:** dead machinery deleted, `verification_code` removed from charge structs, TwoFactorCodeInput removed from payment flows (account flows intact).
 17. **Privacy policy false statements + Art 13 gaps** — "never store card expiry" (false: last_4/exp_month/exp_year stored), missing processors (Cloudflare/R2/CardDAV/Google Fonts/CARTO), no international transfers. **Fixed:** reworded + added sections.
 18. **No right-to-cancel at gift-card purchase + self-purchase forfeits 14-day right without disclosure.** **Fixed:** disclosure + explicit acknowledgement checkbox + links to gift-card-terms.
 19. **Terms page omitted Distance Contracts & Right to Cancel section.** **Fixed:** ported from obsidian §5.
 20. **Business cancellation full-refund policy vs notice-tier code.** **Fixed:** policies rewritten to the accurate "full refund or free reschedule; refunds under standard tiers unless waived".
 21. **Terms §2.2 contradicted privacy policy on deletion balance.** **Fixed:** rewritten to "balance retained on anonymised record".
 22. **Idle-account warnings promised but commented out.** **Fixed:** docs corrected to "not yet implemented".
 23. **Till responses hand-rolled JSON** (~50 `http.Error` + 6 `json.NewEncoder`) instead of `mw.RespondJSON/RespondError`. **Fixed:** migrated.
 24. **Dev/mock new-card charges broken** — `verify_mock_` overwrote the cnon nonce; mock token ≠ backend contract. **Fixed:** mock mints `cnon:sca-...`; `tokenizeWithVerification` returns `verificationToken: null` for new cards (real-SDK parity) so `save_card` works in dev.
 25. **Gift-card buy (account page) kept the old overwrite pattern + sent dead consent fields.** **Fixed:** explicit precedence + `scaFallbackConsentFields` removed.
 26. **`GetBookingRemainingBalancePence` counted `on_the_house`/discounts as paid.** **Fixed:** excluded.
 27. **refund_failed notification dedup missing `acknowledged_at IS NULL`.** **Fixed.**
 28. **Verification codes stored plaintext + brute-force budget keyed per code value.** **Fixed:** HMAC-peppered hashing at rest (CHAR(64)), per-user budget, `[VERIFY]` log relay in dev / fail-closed in prod.
 ### MEDIUM-HIGH (adversarial round 3)
 29. **£250 online-tip cap bypassable via the overflow-tip carve** — `confirm_overflow_tip=true` + £10,000 minted uncapped tip rows. **Fixed:** gate rejects `req.Amount − realRemaining > maxOnlineTipPence` (£250) in BOTH carve paths + `buildSplitRecords` belt-and-braces returns an error (signature `([]PaymentRecord, error)`). Runtime-verified: £10k → 400; £250 → succeeds with £215 tip carve.
 30. **Completion campaign auto-apply over-redemption race** — read-then-write + non-conditional increments. **Fixed:** all 4 increments atomic reserve-first (`... AND times_redeemed < max_redemptions RETURNING id`), skip-on-ErrNoRows; per-user milestone + anniversary get `ON CONFLICT DO NOTHING`; schema backstops added (`chk_times_redeemed` CHECK + partial unique index `uq_booking_discounts_user_milestone_campaign`). Deterministic race tests added.
 ### LOW / MINOR / NITPICK
 31. **4 gift-card admin handlers lacked in-handler `isAdminRequest` backstop.** **Fixed** (CreateGiftCard/TopUpGiftCard/TransferGiftCard/ClaimExpiredBalance).
 32. **Tip lock key inconsistent** (`crussell:tip:` vs `crussell:payment:`). **Fixed:** aligned.
 33. **Float→int64 overflow in gift-card cap checks.** **Fixed:** NaN/Inf/oversize guard before pence conversion.
 34. **Mock SCA error-shape parity unpinned + prod 2FA branches never compiled** (dead under `test,dev`). **Fixed:** `run-prod-tag-tests.sh` + handler-level verification_required tests + mock parity tests.
 35. **Test-gap fixes:** ApplyScheduledDefaultHours test; webhook tests hardcoded-2025 dates → clock-relative; ValidateCardInfo table tests; listCards 20-page guard test; penceLess/roundingEpsilon boundaries; main.go startup-check tests; policy.ts↔refund_policy.go cross-check tests; vitest svelte-component project (happy-dom) + SCA dialog test.
 36. **Mobile parity (26 findings):** touch targets ≥44px on UserBookingModal/PaymentModal/TipPayment/BookingFlow/OverflowTipConfirm/TillPurchases; dialog close 44px; `active:` pressed feedback; TimeSlotPicker 50dvh+44px; `.no-scrollbar` utility; receipt document.write fields escaped; policyPopover mobile anchor; CSP meta added to app.html.
 37. **Docs parity (17 findings):** refresh-token grace 60s→20s; session 30d→90d; notification reasons 16→17; test counts recomputed; deposit advance 24h→36h; gift_card_expiry default 12→24; patch-test 24-48h→24h; phantom `no_deposit` reason removed; line-ref drift → function-name refs; 5-min header mislabel; route tables + cancellation/gift-card-terms; cash-refund claim verified; flood-cap enumeration; idle-account warnings; M1 VAT undercount; Testing Architecture package counts; `.env` stale `TWO_FACTOR_FALLBACK=true` removed; verification-code delivery channel documented.
 38. **Policy minors:** cancel-confirm dialog overstatement; deposit 20–50% phrasing; obsidian User/Admin Manual inconsistencies (24h/36h, forfeit vs full-refund); gift-card expiry reminders; last-updated date format; footer links; deletion balance in obsidian.
 39. **stale comment/struct-field cleanup:** `twofa.go:197` clock sentinel; sweep-cap coordination comment; `releaseBookingPaymentLock` note; `ConsentVersion/ConsentAccepted` dead struct fields removed from charge structs; stale references to deleted 2FA machinery.
 ---
 ## ADVERSARIAL PASSES — CLEARED (no fix needed)
 - Refund capacity/over-refund/pence rounding — **CLEARED** (exact pence math, per-payment + per-booking caps, idempotency replay returns stored result).
 - Refund webhook replay double-apply — **CLEARED** (status-guarded UPDATEs, event_id dedup).
 - Webhook signature verification — **CLEARED** (HMAC-SHA256 URL+body, constant-time compare).
 - Sweep refund of a just-succeeded payment / cancel of a just-completed checkout — **CLEARED** (age gates + replay-window discriminator).
 - Sweep notification flood caps — **CLEARED** at every insert site.
 - Gift-card transfer TOCTOU / clawback / expiry boundary / per-admin cap — **CLEARED**.
 - Loyalty stamps — **CLEARED** (atomic daily-cap + once-per-booking UPDATE).
 - Route guards — **CLEARED** (admin group + in-handler backstops; all 4 missing backstops now added).
 - SCA/2FA boundary — **VERIFIED-OK**: SCA-only for charges (token required in enforced env), homegrown 2FA only for account/admin actions, fail-closed default, no header-based gate bypass.
 - `snapshot` encryption plaintext fallback — **KNOWN/ACCEPTED** (documented money-safety-over-PII tradeoff; CRITICAL startup warning).
 - App-clock vs DB-clock money gates — **THEORETICAL/LOW**, no exploit path (server clock not client-controlled).
 ## REMAINING KNOWLEDGE (intentional, documented)
 - `SNAPSHOT_ENC_KEY` unset → plaintext snapshot fallback (startup CRITICAL; ops must set it in prod).
 - **2FA/verification-code delivery**: intended channel is email/SMS (P6), not yet wired. Stdout-log delivery (`[2FA]`/`[VERIFY]`) is a **local DEV ONLY feature** — dev/test builds only. Production builds have no delivery channel; code issuance FAILS CLOSED (503) until email/SMS ships. The `TWO_FACTOR_ALLOW_LOG_DELIVERY` production opt-in was **removed entirely**.
 - TRUST_PROXY_HEADERS=false default means rate limiting sees the proxy IP behind nginx (startup warning; ops decision).
 - LocalStorage tokens + no hard CSP on the SvelteKit shell beyond the added meta (residual XSS-exposure risk; documented).
 **Session verdict:** review FAILED at start (2 critical wire-contract breaks + 30+ issues) → after 4 fix/review loops: **all issues fixed, all tests green, all keystone journeys runtime-verified.**