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).
This commit is contained in:
@@ -30,11 +30,11 @@ These are things that work fine in dev (with mocks) but need real implementation
|
||||
| P4 | **Payment reconciliation: add recovery for split-brain scenarios** | L (3-5d) | Backend | **Partial progress (Aug 2026).** 20 `log.Printf("CRITICAL: ... manual reconciliation required")` calls exist across payment, refund, and till handlers. When Square succeeds but the DB transaction fails afterwards, state diverges with no automated recovery. The three background sweeps now provide interim recovery for *pending* states (refund sweep retries up to 3 attempts; stale-pending and terminal-checkout sweeps fail/clean stale rows), but a DB-commit failure after a successful Square charge still leaves no automated path to reconcile the orphaned Square-side payment. | This happens when the application correctly processes a Square payment but then hits a DB error on commit. In dev, this was handled by just logging it. For prod, we need a reconciliation job or retry mechanism. (Count grew from 18 to 20 with the stale-pending sweep's manual-reconciliation warnings in the Aug 2026 review round.) |
|
||||
| P5 | **Till Purchases: wire the backend payment flow** | M (1d) | Frontend + Backend | ✅ **DONE (Aug 2026)** — `backend/handlers/payments/till.go` implements the till-sale endpoint (cash, `card_machine` Terminal checkout + polling, `saved_card`, `online_square` Web Payments SDK nonce, `on_the_house`); `TillPurchases.svelte` wires all payment methods and the Charge button is enabled (gated only for retail-item carts, which cannot be charged yet). | The till UI and backend sale path are fully connected. Only retail-item charging remains deferred (see `TillPurchases.svelte` `canCharge`). |
|
||||
| P6 | **Email/SMS notification delivery** | XL (5-7d) | Backend | `user_notification_preferences` table stores delivery preferences. 8 TODO markers reference this blocker. Notification creation works (admin_notifications table), but no delivery channel exists. No SMTP configuration, no SMS provider. 2 tests skipped as "WIP handler." | The notification queue works (reasons, priorities, acknowledging). What's missing is the delivery backend. Affects: slot eviction alerts, edit request approvals/denials, gift card codes, unpaid booking reminders, idle account warnings. |
|
||||
| P7 | **Production security headers** | S (1h) | Backend | HSTS and Referrer-Policy headers are commented out in `main.go:231-233` with TODO markers. They were left disabled for dev HTTP convenience. | Uncomment and configure for production. |
|
||||
| P7 | **Production security headers** | S (1h) | Backend | **Done (Aug 2026, partially):** `corsMiddleware` (`main.go:221-248`) now sets HSTS (`main.go:228`) and Referrer-Policy (`main.go:230`) unconditionally; only the stale TODO comments above them remain. | The headers are already live. Remaining cleanup is removing the now-misleading TODO markers in `main.go` and confirming header values against prod nginx/Cloudflare config. |
|
||||
| P8 | **Social auth stubs (Google/Microsoft/Facebook)** | L (2-3d) | Backend + Frontend | `handlers/auth/social.go` is 1 line (`package auth`). Frontend login page has 3 social buttons that show `toast.info("${provider} login coming soon")`. The `user_social_logins` table and `account_type` enum values exist from early schema design. | The schema was designed for social auth from the start (table + enum values). The OAuth flow itself was never implemented. Buttons exist as UI placeholders. |
|
||||
| P10 | **No automated database backups** | M (1d) | Infrastructure | PostgreSQL volume is persistent in Docker but no `pg_dump` cron, no point-in-time recovery. | Standard production DB setup task. |
|
||||
| P12 | **Square sandbox smoke test (pre-go-live gate)** | S-M (1d, once credentials available) | E2E | **BLOCKED — no real Square credentials available.** Must exercise the real API path end-to-end: new-card tokenization → payment → saved card → refund → reconcile, against Square's sandbox. Also verifies the M-8 open question (is `card.customer_id` enforced as Required?). | The dev mock cannot exercise Square's real wire contract (key-length limits, `device_options`, refund statuses, error codes). This is the sole remaining item before the production flip. See `plans/p11-square-web-payments-sdk.md` Remaining Items. |
|
||||
| P13 | **Reconcile deterministically-keyed saved-card charges** | S (2-3h) | Backend | **Deferred — deliberate trade-off (N-OBS-1).** The admin "Charge Saved Card" idempotency key `bookingID-sc-type-amount-cardID` dedups two *identical* repeat charges on one booking. Not UI-reachable today (PaymentModal always sends the current `totalDue`, which changes after each charge). | Revisit if the admin flow ever gains a "charge exact amount twice" path — the key would then need a client nonce or attempt counter. Tracked from the final payment review. |
|
||||
| P13 | **Reconcile deterministically-keyed saved-card charges** | S (2-3h) | Backend | ✅ **DONE (Aug 2026 payment hardening)** — `deriveBookingPaymentIdempotencyKey` (`handlers/payments/handlers.go`) now sequences repeatable types (`partial`) and rotates the deterministic fallback key past refunded completed rows, so refund-then-repay and equal-amount repeat charges no longer collapse. An un-refunded completed row still keeps its key, so the double-charge protection holds. See `plans/p11-square-web-payments-sdk.md` R3. | Closed by the payment-hardening review. |
|
||||
| P14 | **Square customer provisioning & consent** | S-M (1-2d) | Backend + Frontend + Docs | **IMPLEMENTED (Aug 2026)** — lazy customer provisioning on card-save, `square_customer_id` persisted + forwarded to Square as `card.customer_id`/CreatePayment `CustomerID`, one-off/guest no-customer, `/privacy-policy` route + consent pop-over, SCA verificationDetails wired across all charge flows. **Remaining:** P12 sandbox verification that Square enforces `customer_id`, and final privacy-policy copy review (route ships DRAFT-bannered). See `plans/p14-square-customer-provisioning-consent.md`. | Closed out of the deep post-implementation review (Aug 2026). |
|
||||
| P15 | **Accounting integration (Mettle bank feed + FreeAgent bookkeeping export)** | M (2-3d) | Backend | **Planned upcoming body of work.** No code yet. The `square_deposits` schema (backlog T1) was the placeholder for Square batch deposit reconciliation against Mettle. | Mettle bank feed: match Square batch deposits against bank statements. FreeAgent: bookkeeping export (VAT return / P&L data) feeding the existing HMRC MTD SQL functions (see M1/M9). |
|
||||
|
||||
@@ -92,7 +92,7 @@ These don't add features but reduce maintenance cost and risk.
|
||||
|
||||
| # | Task | Effort | Area | Notes |
|
||||
|---|---|---|---|---|
|
||||
| T1 | **Drop orphaned `square_deposits` table + function** | S (1h) | DB Schema | Full table + `generate_square_deposit_id()` function. Zero Go code references it. Square bank reconciliation was planned but never built. |
|
||||
| T1 | **Drop orphaned `square_deposits` table + function** | S (1h) | DB Schema | ✅ **DONE (Aug 2026)** — the table and `generate_square_deposit_id()` function were removed from `init-scripts/init-script.sql` in the fresh-DB recreate. They had zero Go code references; Square bank reconciliation was planned but never built. |
|
||||
| T2 | **Remove 6 unused DB enum values** | S (2h) | DB Schema | `account_role: 'affiliate'`, `account_type: 'google'/'microsoft'/'facebook'`, `payment_status: 'failed'/'refunded'`, `discount_campaign_scope: 'first_booking_only'/'new_customers_only'`, `till_item_type: 'retail_product'` — defined but never referenced in Go code. |
|
||||
| T3 | **Remove 4 unused admin_notification_reason values** | S (1h) | DB Schema | `'rescheduled_booking'`, `'gift_card_purchased_for_friend'`, `'edit_request'` (code uses `'edit_requested'`), `'deposit_paid'` (in priority ordering but never inserted). |
|
||||
| T4 | **Create or remove documented `update_data_consent()` function** | S (1h) | DB Schema | Listed in FUNCTION USAGE SUMMARY comment (~line 2401) but no `CREATE FUNCTION` exists. |
|
||||
|
||||
@@ -31,14 +31,14 @@ Idempotency keys (`idempotency_key VARCHAR(64) UNIQUE`) on bookings prevent dupl
|
||||
Multi-method payment modal for admin: Card (Square Terminal), Cash (with change calculation + "keep change as tip"), Gift Card (12-digit ID or account balance). User-facing payment modal for online deposits, partial payments, full payments, balance payments, and tips on completed bookings.
|
||||
|
||||
Square integration has two build-tagged implementations:
|
||||
- **Dev** (`//go:build dev`): Mock client simulates async checkout with polling. No real payments.
|
||||
- **Dev** (`//go:build dev`): In-memory mock (`internal/square/square_dev.go`) that mirrors production PCI-DSS behaviour: accepts only `cnon:`/`ccof:` tokens (raw PANs rejected), dedups by idempotency key, rescues keyed replays by source token, classifies refund outcomes (already-processed vs declined), and exposes opt-in fault-injection toggles (`ShouldFail`, `FailAfterCommit`, `SimulateCardTokenUsed`). No real money leaves the process.
|
||||
- **Prod** (`//go:build !dev`): Connects to live Square API. Requires Square credentials in `.env`.
|
||||
|
||||
Saved cards stored in `user_saved_cards` with soft delete (`retained_until` for 7-year UK compliance). Refunds tracked in `refunds` table — partial or full. Square webhooks at `/webhooks/square` (registered on the router root, proxied exact-match by nginx — not under `/api`) are HMAC-verified **fail-closed** (503 without the signing key, 403 on bad signature) and deduplicated by `event_id`: a fast-path in-memory cache plus a `square_webhook_events` DB row committed **after** dispatch, so delivery is at-least-once and Square retries on any failure. Events dispatch to state-mutating handlers that reconcile `payments`, `till_sales`, `refunds`, and `disputes` — a lost dispute marks the payment failed and a `critical_payment_log` admin notification is always raised, even when the disputed payment is not tracked locally (no sweep fallback exists for disputes). The background sweeps remain as the eventual backstop.
|
||||
|
||||
**2FA on online card payments:** a loosely-faked two-factor-authentication feature stands in for PSD2 Strong Customer Authentication. Charging a **saved card** requires the user to have 2FA enabled when it is enforced (enforcement is **fail-closed**: ON by default for any `SQUARE_ENVIRONMENT` except an explicit `mock`/`dev`/`development`/`test` value — empty or unknown values are treated as production-enforced — and disabled only by `REQUIRE_2FA=false` (case-insensitive, also `0`/`off`/`no`); new-card/nonce charges are not gated). The 6-digit code is delivered via the server log (`[2FA]` prefix) in ALL modes — the operator reads it and relays it to the customer — standing in for real email/SMS delivery until that infrastructure lands (P6). In unenforced/dev mode the setup response also returns the code, so the flow is testable without grepping backend logs; there is no email/SMS transport yet. UI: Account → Two-Factor Authentication. Details in the [[Technical Manual]].
|
||||
|
||||
Fees column on `payments` stores actual Square deductions. **`square_deposits` (and the `generate_square_deposit_id()` function) are DEAD SCHEMA — zero Go references; they were a placeholder for Square bank reconciliation against Mettle. Keep them unused; backlog item T1 tracks dropping them, and Mettle/FreeAgent integration is a planned upcoming body of work.**
|
||||
Fees column on `payments` stores actual Square deductions. **`square_deposits` (and the `generate_square_deposit_id()` function) were dead schema with zero Go references, a placeholder for Square bank reconciliation against Mettle; they were dropped from `init-scripts/init-script.sql` in the fresh-DB recreate (backlog T1 closed). Mettle/FreeAgent integration is a planned upcoming body of work.**
|
||||
|
||||
### Gift Cards
|
||||
|
||||
@@ -220,7 +220,7 @@ npm run dev # Dev server with HMR
|
||||
|
||||
```bash
|
||||
cd backend
|
||||
go test -tags "test,dev" ./... # 1,902 tests passed (4 skipped)
|
||||
go test -tags "test,dev" ./... # 2,169 tests passed (4 skipped)
|
||||
go test -tags "test,dev" -v -run TestName ./... # Single test
|
||||
```
|
||||
|
||||
|
||||
@@ -255,14 +255,12 @@ src/lib/components/
|
||||
|
||||
Added in the June 2026 security pass:
|
||||
|
||||
### Security Headers
|
||||
|
||||
| Header | Value | Location |
|
||||
|--------|-------|----------|
|
||||
| `Content-Security-Policy` | `default-src 'none'; frame-ancestors 'none'` | Global middleware (`main.go:139`) |
|
||||
| `Access-Control-Allow-Origin` | `*` (dev only) | Global middleware (`main.go:141`) |
|
||||
| `Access-Control-Allow-Methods` | `GET, POST, PUT, PATCH, DELETE, OPTIONS` | Global middleware |
|
||||
| `Access-Control-Allow-Headers` | `Authorization, Content-Type, Idempotency-Key` | Global middleware |
|
||||
| `Content-Security-Policy` | `default-src 'none'; frame-ancestors 'none'` | Global middleware (`main.go:231`) |
|
||||
| `Access-Control-Allow-Origin` | Exact match from the `FRONTEND_ORIGIN` allowlist (comma-separated, trimmed, blanks dropped; defaults to `http://localhost:5173` when unset). Set only when the request `Origin` is in the allowlist (never reflected), together with `Vary: Origin` | Global middleware (`main.go:235`; allowlist via `corsAllowedOrigins()` `main.go:192`, exact match via `originAllowed()` `main.go:208`) |
|
||||
| `Access-Control-Allow-Methods` | `GET, POST, PUT, PATCH, DELETE, OPTIONS` | Global middleware (`main.go:238`) |
|
||||
| `Access-Control-Allow-Headers` | `Authorization, Content-Type, Idempotency-Key` | Global middleware (`main.go:239`) |
|
||||
|
||||
### Other Security Fixes
|
||||
|
||||
@@ -272,7 +270,7 @@ Added in the June 2026 security pass:
|
||||
| Webhook signature fail-closed | `handlers/webhooks/square.go:84-102` | Webhook verification is fully **fail-closed**: 503 when `SQUARE_WEBHOOK_SIGNATURE_KEY` is unset (a misconfigured deployment must not silently accept forged events), 403 when the signature header is missing or invalid, 400 on an empty `event_id`. Previously it skipped verification when the key was empty. Dedup is now restart-safe: each handled event is recorded in the `square_webhook_events` table, with the row committed **after** successful dispatch (at-least-once; a failed dispatch writes no row and returns 5xx so Square retries), fronted by a bounded in-memory fast-path cache (the `squareWebhookDedup` struct, `square.go:36-82`). |
|
||||
| S3 delete error checking | `handlers/portfolio/images.go:975` | Changed `s3.Client.Delete(...)` (ignored return) → `if err := s3.Client.Delete(...); err != nil { log.Printf(...) }` |
|
||||
|
||||
CORS uses `*` in local dev. In production behind Cloudflare, nginx handles CORS. No CSP violations expected — the SvelteKit SPA doesn't load external scripts or fonts.
|
||||
CORS uses a `FRONTEND_ORIGIN` allowlist, not `*`. `corsAllowedOrigins()` (`main.go:192`) reads the comma-separated env var, trims each entry, drops blanks, and falls back to `http://localhost:5173` when the var is unset or empty. `originAllowed()` (`main.go:208`) does an exact match only, never reflecting the incoming `Origin`. `Access-Control-Allow-Origin` and `Vary: Origin` are set only when the request `Origin` is in the allowlist, so a leaked JWT cannot be used from a rogue site. In production behind Cloudflare, nginx handles CORS. No CSP violations expected, the SvelteKit SPA doesn't load external scripts or fonts.
|
||||
|
||||
---
|
||||
|
||||
@@ -462,7 +460,7 @@ CORS uses `*` in local dev. In production behind Cloudflare, nginx handles CORS.
|
||||
| `user_saved_cards` | Saved card details (square_card_id, square_customer_id TEXT nullable (P14), brand, last4, fingerprint, soft delete with retained_until; UNIQUE (user_id, square_card_id) constraint — per-user card uniqueness) |
|
||||
| `refunds` | Refund records linked to a payment (amount, reason, `square_refund_id`, `refund_attempts` int, `origin` manual|cancellation, `idempotency_key` unique, `created_by` FK to users, ON DELETE SET NULL; `booking_id` is nullable — NULL for non-booking payments such as gift-card purchase refunds) |
|
||||
| `financial_aggregates` | Monthly aggregated financial statistics (no PII) — populated when granular records expire |
|
||||
| `square_deposits` | Square deposit batch tracking for bank reconciliation (batch_id, total_amount, deposited_at) — **DEAD SCHEMA: zero Go references; it was a placeholder for Square bank reconciliation against Mettle. Do not rely on it (backlog T1 tracks dropping it, with `generate_square_deposit_id()`); Mettle/FreeAgent integration is a planned upcoming body of work.** |
|
||||
| `square_deposits` | ~~Square deposit batch tracking for bank reconciliation (batch_id, total_amount, deposited_at)~~. **REMOVED (Aug 2026):** the table and `generate_square_deposit_id()` function were dropped from `init-scripts/init-script.sql` in the fresh-DB recreate; they had zero Go references and were a placeholder for Square bank reconciliation against Mettle (backlog T1 closed). Mettle/FreeAgent integration is a planned upcoming body of work. |
|
||||
| `affiliate_payouts` | Affiliate commission tracking |
|
||||
| `loyalty_redemptions` | Loyalty stamp redemptions (pending → applied, 6-month expiry, FIFO) |
|
||||
| `discount_campaigns` | Discount campaigns: time-based and milestone (draft → active → completed lifecycle) |
|
||||
@@ -1307,7 +1305,7 @@ Files with this pattern: `bookings.go` (4 handlers), `custom_services.go`, `user
|
||||
|
||||
### Test Coverage
|
||||
|
||||
**2,137 tests compiled** across all packages (4 skipped, 0 failures). Coverage improved from 50.4% to 65.0% via 56 new test files covering booking handlers, user handlers, payments (giftcards, till, refunds), DAV, auth, middleware, validators, zxcvbn, and scheduling. Key additions: coverage improvement tests (bookings_coverage_test.go, user_coverage_test.go, payments coverage expansion — all meaningful error-path tests, not padding), split-lunch detection tests, savepoint/transaction-context tests for time-sensitive operations, VAT lifecycle and parallel-deadlock regression tests, and cleanup of 10 dead test functions flagged by staticcheck U1000.
|
||||
**2,169 tests compiled** across all packages (4 skipped, 0 failures). Coverage improved from 50.4% to 65.0% via 56 new test files covering booking handlers, user handlers, payments (giftcards, till, refunds), DAV, auth, middleware, validators, zxcvbn, and scheduling. Key additions: coverage improvement tests (bookings_coverage_test.go, user_coverage_test.go, payments coverage expansion — all meaningful error-path tests, not padding), split-lunch detection tests, savepoint/transaction-context tests for time-sensitive operations, VAT lifecycle and parallel-deadlock regression tests, and cleanup of 10 dead test functions flagged by staticcheck U1000.
|
||||
|
||||
| Package | Coverage Area |
|
||||
|---------|--------------|
|
||||
|
||||
@@ -128,7 +128,7 @@ All 8 flows render `SquareCardInput` and send the resulting `cnon:xxx` as `new_c
|
||||
|---|---|---|
|
||||
| R1 | **Sandbox smoke test** — new-card tokenization → payment → saved card → refund → reconcile, exercised against a real Square endpoint | **BLOCKED — no real Square credentials available.** The full end-to-end path (Web Payments SDK nonce → `POST /v2/cards` → `POST /v2/payments` → refund → `ListRefunds` reconcile) can only be validated against Square's sandbox. Must run before any production flip. |
|
||||
| R2 | **M-8 open question: is `card.customer_id` enforced as Required at runtime?** | **Superseded by P14 (Aug 2026).** Customer provisioning now happens on card-**save** only: the app lazily creates a Square customer, persists `square_customer_id`, and sends it as `card.customer_id` on create-card and as `CustomerID` on `ccof:` charges. One-off and guest payments still mint no customer. The remaining open question is now just the P12 sandbox check that Square accepts these fields on the real wire contract (and that `customer_id` is not required for non-`ccof:` one-off charges). |
|
||||
| R3 | **N-OBS-1: saved_card deterministic idempotency key dedups identical repeat charges** | **Deliberate, accepted trade-off** (not a credential blocker). The admin "Charge Saved Card" key `bookingID-sc-type-amount-cardID` means two *identical* charges on one booking dedup to the first. Not UI-reachable today (PaymentModal always sends the current `totalDue`, which changes after a charge), and the double-click protection is worth more than a hypothetical "charge exact amount twice" path. Flagged for revisit in the Gap Backlog if that path ever appears. |
|
||||
| R3 | **N-OBS-1: saved-card deterministic idempotency key dedups identical repeat charges** | **Resolved (Aug 2026 payment hardening).** The no-client-key fallback (`deriveBookingPaymentIdempotencyKey`, `handlers/payments/handlers.go`) now advances a sequence for repeatable types (`partial`) and rotates past refunded completed rows, so refund-then-repay and equal-amount partials no longer collapse onto one key. The "same 50% deposit twice on an un-refunded booking" invariant still holds (an un-refunded completed row keeps its key and the dedup lookup returns it). |
|
||||
|
||||
---
|
||||
|
||||
|
||||
Reference in New Issue
Block a user