fix: review-loop A — discount credit on admin payments, campaign over-credit cap, sweep replay window, dedup refund revalidation, duplication/modularisation, GBP pence naming
Round-A fresh review (6 agents) + fix + secondary cross-cutting + verification rounds: - F1: campaign discounts reduce the charged amount (deposit credit + admin PaymentModal discounted total); capDiscountToRemainingObligation prevents over-credit at completion in all four campaign blocks - F2: sweep replay rescue distinguishes legitimate same-key retries (21h window) from expired-key new charges; ccof blind-fails leave pending + CRITICAL instead of clawing back - F3: post-start online overflow carved as a tip record (mirrors terminal split builder) - A1: single-source Square decline-code classification (till delegates to square.IsDefinitivePaymentError) - A2/A5: refund attempt-cap literals consolidated; refund-failure counter capped + reset on terminal resolutions + admin notifications - A3/A9: idempotency helpers adopted across derivations; IsExplicitDevOrMockEnv relocated + all gates unified (incl. health-check) - A7: 2FA user+IP limiter + TRUST_PROXY_HEADERS startup warning; SNAPSHOT_ENC_KEY startup validation; TWO_FACTOR_PEPPER docs corrected - A8: snapshot encryption on all 6 write sites + marker-aware reuse paths; MPV->SPV effective voucher type (single VAT point) - A10/A11/A12/A16: gift-card slot scan advances past failed; amount-aware refund reconciliation; completed-booking refund re-check; PaymentWasRefunded on SquareClient interface - Dedup refund revalidation on tip/terminal/gift-card paths; sweep acknowledged_at IS NULL parity; refund-notification single source (exported payments.InsertRefundFailedNotifications) - Duplication/modularisation round: shared frontend helpers (sanitizeDecimalInput, campaignDiscountCents, twoFactorBlocksSavedCards getter, generateUUID), single-source MaxIdempotencyKeyLength, notification-helper consolidation, snapshot-guard comments - Cross-cutting GBP rename: Cents->Pence across backend + frontend + tests (26 identifiers, 16 files) - Tests: 11 behavior-change tests updated to new invariants; coverage for fixed functions; frontend vitest 55 tests; docs corrected (test counts, 2FA delivery, pre-launch checklist, resolution status) - gitleaks: allowlist backend/internal/square test fixtures (mock idempotency keys) All 25 backend packages pass; frontend 55/55 + build clean; env-docs 41/41.
This commit is contained in:
@@ -420,6 +420,25 @@ When you click on a customer in the Users list, you see their full profile.
|
||||
- Whether they've agreed to data retention
|
||||
- When these consents were last updated
|
||||
|
||||
### Treatment & Safety Notes — IMPORTANT
|
||||
|
||||
The notes field on a booking (and in the customer's profile) is kept as a single
|
||||
health-and-safety record. On account deletion the rest of the customer's record is
|
||||
wiped, but these notes are **retained** so the salon can still make safe adjustments if
|
||||
the customer returns and to defend any future legal claim. That retention is only
|
||||
compliant because the notes are treated as **de-identified** after deletion — so:
|
||||
|
||||
- **Never** enter a customer's name, phone number, address, or email address in the notes.
|
||||
- Refer to the customer by the system's customer record, not by personal identifiers.
|
||||
- Keep notes to treatment facts: colour and preference, allergies and skin sensitivities,
|
||||
lateness, access needs.
|
||||
- If a customer asks you to note down their phone/address "for next time", do not write it
|
||||
in the notes — use the customer's phone field on their profile instead.
|
||||
|
||||
If a note is entered that contains a direct identifier, the de-identified-retention claim
|
||||
for that record no longer holds — the note must be edited to remove the identifier.
|
||||
(There is no automated check for this; it is a procedural requirement.)
|
||||
|
||||
---
|
||||
|
||||
## Booking Details
|
||||
|
||||
@@ -98,7 +98,7 @@ These don't add features but reduce maintenance cost and risk.
|
||||
| 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. |
|
||||
| T5 | **Resolve 2 route-conflicted lint-ignored handlers** | S (1h) | Backend | `manage.go:27,314` — handlers exist only for tests but routes conflict. |
|
||||
| T6 | **Resolve portfolio lint-ignored handler** | S (1h) | Backend | `images.go:53` — handler referenced from tests only, never routed. |
|
||||
| T7 | **Fix README job count: 22 not 21** | S (5min) | Docs | ✅ **COMPLETED Aug 2026** — README now documents 25 maintenance jobs (the three payment sweeps: `sweep-pending-square-refunds`, `sweep-stale-pending-payments`, `sweep-stale-terminal-checkouts`, plus `sweep-square-webhook-events` and `scan-critical-payment-logs`). |
|
||||
| T7 | **Fix README job count: 22 not 21** | S (5min) | Docs | ✅ **COMPLETED Aug 2026** — README now documents 26 maintenance jobs (the three payment sweeps: `sweep-pending-square-refunds`, `sweep-stale-pending-payments`, `sweep-stale-terminal-checkouts`, plus `sweep-square-webhook-events` and `scan-critical-payment-logs`). |
|
||||
| T8 | **Audit 18 silent catch blocks** | M (1d) | Frontend | 1 `catch (e) {}`, 17 `catch (_err)` — errors swallowed silently. Many should show user-facing toasts. |
|
||||
| T9 | **33 `svelte/no-navigation-without-resolve` suppressions** | M (1d) | Frontend | Create a project-wide `goto` wrapper instead of suppressing per-file. |
|
||||
| T10 | **Replace `as any` in HolidayHours** | S (30min) | Frontend | `HolidayHours.svelte:234` — `(group.hours as any[])?.map(…)`. Hours array has known shape. |
|
||||
|
||||
@@ -220,7 +220,7 @@ npm run dev # Dev server with HMR
|
||||
|
||||
```bash
|
||||
cd backend
|
||||
go test -tags "test,dev" ./... # 2,269 tests passed (4 skipped), as of 13 Aug 2026
|
||||
go test -tags "test,dev" ./... # 2,333 tests passed (4 skipped), as of 14 Aug 2026
|
||||
go test -tags "test,dev" -v -run TestName ./... # Single test
|
||||
```
|
||||
|
||||
|
||||
@@ -832,7 +832,7 @@ validTransitions := map[string]map[string]bool{
|
||||
- A mistyped or unset `SQUARE_ENVIRONMENT` can never silently disarm the gate. `REQUIRE_2FA=false` disables enforcement even in a deployed environment, for local testing.
|
||||
- **Residual brute-force exposure (accepted):** a fresh-code delivery (setup, or a disable that mints because no pending code exists) resets the shared 5-attempt counter. An authenticated attacker who already holds the victim's password can therefore loop `disable` with wrong codes to obtain an unlimited series of fresh codes, each granting 5 guesses — the 2FA gate then reduces to a 6-digit guessing game bounded only by the per-IP rate limit (120 req/min on `/api/user`) and the 10-minute code TTL. This is the same reset-on-delivery tradeoff that makes codes deliverable to locked-out users; it is documented rather than fixed because a hard per-user lockout would strand a legitimate user who lost their code, with no email/SMS transport to recover (P6). Revisit when real delivery lands.
|
||||
|
||||
**State:** stored on `users` — `two_factor_enabled BOOLEAN DEFAULT FALSE`, `two_factor_method` (`'email'` / `'sms'`), `two_factor_pending_code_hash` (SHA-256), `two_factor_pending_code_expires` (10-minute TTL). Only the digest is stored in the DB; the plaintext code is delivered via the server log with a `[2FA]` prefix in **all** modes — enforced and unenforced alike — the operator reads it and relays it to the customer. This is the fake delivery channel until real email/SMS infrastructure replaces that log line (P6); there is no email/SMS transport yet. When enforcement is off (dev), the setup endpoint also returns the code in its response and verify accepts any code, so the flow is testable without grepping backend logs.
|
||||
**State:** stored on `users` — `two_factor_enabled BOOLEAN DEFAULT FALSE`, `two_factor_method` (`'email'` / `'sms'`), `two_factor_pending_code_hash`, `two_factor_pending_code_expires` (10-minute TTL). Only a digest of the code is stored in the DB — never the plaintext. The digest is **HMAC-SHA256 keyed by `TWO_FACTOR_PEPPER`** when that env var is set (`hashTwoFACode`, `handlers/user/twofa.go`); an unset pepper falls back to the legacy unsalted SHA-256 digest **only** in dev/test builds and for the legacy-row migration window — production builds can never persist an unsalted digest because code issuance **fails closed** without the pepper (see `handlers/user/twofa_prod.go`). **Code delivery is build-dependent and production fails closed:** dev/test builds always write the plaintext code to the server log with a `[2FA]` prefix (and, when enforcement is off, the setup endpoint also returns the code and verify accepts any code, so the flow is testable without grepping logs). Production builds **NEVER** log the code unless the operator explicitly opts in with `TWO_FACTOR_ALLOW_LOG_DELIVERY=true`; without it, code issuance is refused (503 / `errTwoFADeliveryUnavailable`) so no user can complete setup or disable 2FA, and every enforced saved-card payment 403s with no way forward. This is the fake delivery channel until real email/SMS infrastructure replaces that log line (P6); there is no email/SMS transport yet. Each fresh code is checked under a shared **5-attempt lockout** (`twoFAMaxAttempts = 5` consecutive failed verifies invalidate the pending code); a fresh-code delivery resets that counter (see the residual brute-force note above).
|
||||
|
||||
**Gate:** `requireTwoFactorForCardAccess` (`handlers/payments/twofa.go`) is called on the saved-card online charge paths — booking payments, tips, and saved-card till sales. New-card (nonce) charges are **not** gated; a verification token from Square's own SDK covers the SCA step on new-card entry. Disabling 2FA requires a verification code when enforcement is ON (a password-only attacker must not be able to lift the protection) — the disable flow reuses a still-valid pending code when one exists, otherwise it generates and delivers a fresh one via the same `[2FA]` log channel; the submitted code is checked under the shared 5-attempt lockout (the same per-user counter as verify). The "always generate a fresh code on disable" alternative was deliberately **not** adopted: with an out-of-band log-delivery channel, a code generated by a request could never be submitted within that same request. In dev (unenforced) environments no code is required to disable.
|
||||
|
||||
@@ -1323,7 +1323,7 @@ Files with this pattern: `bookings.go` (4 handlers), `custom_services.go`, `user
|
||||
|
||||
### Test Coverage
|
||||
|
||||
**2,269 tests compiled** across all packages (4 skipped, 0 failures) — as of 13 Aug 2026. 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,333 tests compiled** across all packages (4 skipped, 0 failures) — as of 14 Aug 2026. 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 |
|
||||
|---------|--------------|
|
||||
@@ -1348,7 +1348,10 @@ Items that must be closed before a production go-live. This is a living list; ad
|
||||
|
||||
- **Set `SUPPORT_EMAIL`.** Every consumer-facing legal doc ([[Terms & Conditions - Overall App]], [[Privacy Policy]], [[Gift Card Terms & Conditions]], and the `/terms`, `/privacy-policy`, `/cancellation-policy` routes) currently uses the `{{SUPPORT_EMAIL}}` placeholder for the support address. The real address must be substituted in **all** of those places before launch — a placeholder in a live policy is a consumer-law exposure.
|
||||
- **Legal review of the DRAFT-bannered legal docs.** The T&Cs, Privacy Policy, Gift Card Terms, and the policy routes are still drafts for go-live review; have the wording checked by a solicitor before launch.
|
||||
- **Wire real email/SMS or keep the `[2FA]` log relay.** 2FA codes are delivered via the server log until email/SMS lands (see the Two-Factor Authentication section in this manual); confirm the delivery channel before launch.
|
||||
- **Wire real email/SMS or keep the `[2FA]` log relay.** 2FA codes are delivered via the server log until email/SMS lands (see the Two-Factor Authentication section in this manual); confirm the delivery channel before launch. In a production build the relay is **explicitly opt-in**: set `TWO_FACTOR_ALLOW_LOG_DELIVERY=true` to deliver codes via the `[2FA]` log line, otherwise code issuance fails closed (503) and no user can complete 2FA setup or disable — every enforced saved-card online payment will 403. This is the **only** production 2FA delivery channel until email/SMS (P6) is wired, so it must be a deliberate decision at launch (with restricted log access), not a silent default.
|
||||
- **Set `SNAPSHOT_ENC_KEY`.** `square_request_snapshot` rows contain buyer PII (email + `ccof:` card tokens). Without `SNAPSHOT_ENC_KEY` (base64-encoded 32-byte AES-256 key, `openssl rand -base64 32`), non-mock deployments store those rows **PLAINTEXT at rest** with only a one-time CRITICAL startup log (see `checkSnapshotEncKey`, `backend/main.go`). Money-safety first: the process does **not** fail at startup, so the misconfiguration is otherwise silent — set the key before go-live.
|
||||
- **Set `TRUST_PROXY_HEADERS=true`.** The backend is deployed behind nginx and/or Cloudflare, which overwrite `X-Real-IP`/`CF-Connecting-IP` with the real client IP. `TRUST_PROXY_HEADERS` defaults to false; without it every per-IP rate-limit key collapses onto the proxy's IP and any one client can exhaust the shared per-IP budget for everyone (and per-IP limiter protection is effectively bypassed). Keep it false only when the backend is origin-exposed. The var ships via `.env` (`env_file` in `compose.yml`) — `compose.yml` deliberately never sets it, the operator decides per deployment.
|
||||
- **Treatment/safety notes retention — operator assertion (documented residual risk).** The privacy-policy route promises notes are "retained in a form that cannot be traced back to you" (de-identified at account erasure). This is a **business decision, not a technical guarantee**: notes are free-text `TEXT` (no backend PII validation, UI-capped at 1,000,000 chars) and are kept on the anonymised booking row after `anonymize_user` wipes the surrounding record. The operator asserts notes never contain direct identifiers. The residual risk is that a note entered with a name/phone/address could still re-identify the customer after erasure — see the Admin Manual procedure ("never enter direct identifiers in notes") and the `anonymize_user` RETENTION POLICY comment in `init-scripts/init-script.sql`.
|
||||
- **Production storage (S3/R2)** and **SMTP** are unimplemented stubs (see README "Limitations") — required for prod.
|
||||
|
||||
---
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
# Testing Architecture & DB Management
|
||||
|
||||
**Last Updated:** August 2026 (v6 — coverage 50.4%→65.0%, 2,269 tests compiled, 4 skipped)
|
||||
**Last Updated:** August 2026 (v6 — coverage 50.4%→65.0%, 2,333 tests compiled, 4 skipped)
|
||||
|
||||
---
|
||||
|
||||
@@ -502,7 +502,7 @@ This appears in `TestAccount_DeleteGuest` and `TestLoyalty_Get`. The `dav.Servic
|
||||
|--------|-------|
|
||||
| Quick check (`-count=1`) | **~2min** |
|
||||
| Packages | 25 tested, 0 failures |
|
||||
| Tests | 2,269 compiled under test,dev tags (as of 13 Aug 2026) |
|
||||
| Tests | 2,333 compiled under test,dev tags (as of 14 Aug 2026) |
|
||||
|
||||
New test additions in this batch:
|
||||
| Test | Coverage |
|
||||
@@ -521,7 +521,7 @@ New test additions in this batch:
|
||||
| `TestCancelReservation_DoesNotTouchAnonReservations` | Inverse-isolation test — user cancel ignores `RESERVATION:anon:%` (defensive — the WHERE clause only matches `RESERVATION:user:%`) |
|
||||
| `TestCancelReservation_DoesNotTouchAdminReservations` | Inverse-isolation test — user cancel ignores `RESERVATION:admin:%`. Pairs with the admin-side test that verifies admin cancel ignores `RESERVATION:user:%`. Proves the two endpoints are properly partitioned. |
|
||||
|
||||
**Total tests:** 2,269 compiled across all packages (4 skipped) — as of 13 Aug 2026. 0 failures. Growth driven by: coverage improvement pass (new test files for bookings, user, payments, giftcards, till, refunds, DAV, auth, middleware, validators, zxcvbn — 56 new files, coverage 50.4%→65.0%), VAT lifecycle and parallel-deadlock regression tests, savepoint/transaction-context pattern for time-sensitive tests, split-lunch detection tests, removal of 10 dead test functions flagged by staticcheck U1000, and the Square payments test-gap round (terminal CreateCheckout-failure, GetCheckoutStatus reference_id mismatch, deadline wire shape, loyalty lock contention 409, GDPR saved-card scrubbing, ValidateAmount/isTokenLike/lock helpers direct units, buildSplitRecords tip overflow).
|
||||
**Total tests:** 2,333 compiled across all packages (4 skipped) — as of 14 Aug 2026. 0 failures. Growth driven by: coverage improvement pass (new test files for bookings, user, payments, giftcards, till, refunds, DAV, auth, middleware, validators, zxcvbn — 56 new files, coverage 50.4%→65.0%), VAT lifecycle and parallel-deadlock regression tests, savepoint/transaction-context pattern for time-sensitive tests, split-lunch detection tests, removal of 10 dead test functions flagged by staticcheck U1000, and the Square payments test-gap round (terminal CreateCheckout-failure, GetCheckoutStatus reference_id mismatch, deadline wire shape, loyalty lock contention 409, GDPR saved-card scrubbing, ValidateAmount/isTokenLike/lock helpers direct units, buildSplitRecords tip overflow).
|
||||
|
||||
### What Drives Test Time
|
||||
|
||||
@@ -638,7 +638,7 @@ This shouldn't appear anymore — the auth package's TestMain was updated to use
|
||||
|
||||
### Q: What's the total test count?
|
||||
|
||||
2,269 tests compiled across all packages (4 skipped), as of 13 Aug 2026. 0 failures.
|
||||
2,333 tests compiled across all packages (4 skipped), as of 14 Aug 2026. 0 failures.
|
||||
|
||||
**Notable new tests:** Centralised job scheduler tests (3 — RegisterAll count, schedules, handler signatures), scheduled-cleanup handler tests (21 — NotifyUnpaidOneWeek/Month, TransitionDiscountCampaigns, CleanupExpiredVerificationCodes/RefreshTokens), GDPR export cache cleanup (4), stale login entry cleanup (4), rate limiter cleanup tests (6), rate limiter production behavior tests (6). Duplicate completion guard (idempotent second `"completed"` call), daily stamp cap (two completions same day → 1 stamp), invalid status transitions (no-show→completed rejected with 400), sequential edit (two edits in sequence), timezone independence (UTC in, UTC out — no shift), past-booking no-show guard (past confirmed booking cancelled → `client_cancelled`, not `no_show`). New closing_time tests (3), content-type middleware tests (2), clock package tests, expanded admin reserve overlap tests, expanded gift card buy flow tests with VAT, and full admin reservation cancel coverage (12 tests covering walkin + callin + isolation + no-op + idempotency + response format parity).
|
||||
|
||||
|
||||
Reference in New Issue
Block a user