diff --git a/.sisyphus/plans/payments-review-master-issues.md b/.sisyphus/plans/payments-review-master-issues.md new file mode 100644 index 0000000..493b1a3 --- /dev/null +++ b/.sisyphus/plans/payments-review-master-issues.md @@ -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:` 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.**