Round 2 Loop A fresh money/security/dup-mod review. 23 findings fixed:
MONEY:
- CRITICAL: B1 duplicate auto-refund gains an attempt cap (b1_attempts col, cap 3) —
a rejected auto-refund no longer re-replays the expired key every sweep run
(which minted a stacking unauthorized charge each time); FAILED-webhook
demotion respects the cap; never re-replay a key whose B1 refund failed
- HIGH: A6 deposit_covered_by_discount skip path now APPLIES the eligible
campaign discount rows immediately (capped) instead of skipping with no
discount recorded — no more promised-discount-not-recorded overcharge
- MEDIUM: 2FA code burned by the SAVE gate is re-issued on failed
new-card+save_card charges (re-issue guard now covers req.SaveCard)
- LOW: GetBookingPaymentSummary excludes tip rows from paidAmount (remaining
now matches the authoritative tip-excluded balance)
SECURITY:
- MEDIUM: unacknowledged CRITICAL admin-notification flood capped (global cap
on critical_payment_log + refresh_token_reuse rows)
- MEDIUM: 2FA reissue no longer bypasses the mint cooldown (Check no longer
clears LastMintAt on gate-verify; cleared on terminal charge success)
- MEDIUM: twofa.StateFor map-saturation returns a shared permanently-locked
state instead of a fresh 5-guess budget per request
- MEDIUM: ProgressiveRateLimit rejects 429 past maxProgressiveSleepDelayMs
instead of sleeping unboundedly; login bcrypt concurrency semaphore added
- LOW: loginInProgress 409->429; webhook key-set/URL-unset startup check;
email-verification per-user attempt counter
DUP/MOD:
- formatCurrency single source (frontend format.ts, 7 files consolidated);
SquareRefundStatusToLocal single source (errors.go, all sites); admin
audit-log helper dedup; SCA retry model unified (proactive on all 6
surfaces); buyDailyTotal/daily-cap mirror via backend; lock TTL from
backend; generateUUID at all card-form sites; magic numbers named
(defaultPostgresHost, epsilon, fee constants); admin CASH + gift-card
terminal charges now audited; DAV_SKIP_INIT documented in manuals
Verified: 26/26 dev + 24/24 prod (GO_TESTING=1, the CI condition), both vet
tags, frontend tests+build, env-docs 42/42.
Rate limiting (backend):
- RateLimit/ProgressiveRateLimit now derive the per-client key from
CF-Connecting-IP, then chi's GetClientIP (the X-Real-IP value nginx sets at
main.go:323), then RemoteAddr. Previously only CF-Connecting-IP/RemoteAddr
were used, so behind the Docker nginx every client shared ONE bucket per
limiter — 10 logins/min site-wide blocked all users (the reported
'Error: Rate limit exceeded' after seeding was the login 10/min bucket
tripped by the seed's 11 logins, all keyed 127.0.0.1 in dev).
- Real implementation is now //go:build !dev || test; new
mw/ratelimit_dev.go (//go:build dev && !test) is a no-op passthrough, so
'go run -tags dev' (the dev harness) never rate-limits dev/seeding traffic,
while production and tests (-tags test,dev) keep the real limiter. The docs
(Technical Manual) already claimed dev no-op behaviour — the code now
matches. NewProgressiveRateLimiter is provided in the no-op build because
tag-free ratelimit_shared.go:104 initializes the global at package init.
Admin 2FA management (backend):
- users.two_factor_last_used_at TIMESTAMPTZ column (init-script, fresh-DB).
- AdminUserDetail now returns twoFactorEnabled/twoFactorMethod/
twoFactorLastUsedAt.
- New POST /api/admin/users/{id}/2fa/remove (admin-only): clears all 5 2FA
columns + drops the user's in-memory attempt/lockout state — an admin
recovery path when a user loses 2FA access.
- two_factor_last_used_at updated on every successful 2FA verification.
Account page (/account):
- 2FA section moved under the Notifications heading, visible to all roles;
Email/SMS toggles (Notifications styling) acting as a radio group with
'none' state; Apply button only when the selection differs from saved;
unselecting shows a payment-rules warning dialog; the dev-comment
'2FA is optional right now (REQUIRE_2FA is off)' and the 'Dev code:' debug
line are removed.
- Cards tab hidden from admin role.
Admin modals:
- User Details modal: new 'Two-Factor Authentication' section above Patch
Tests showing Enabled/Disabled, method, last-used timestamp, and a Remove
2FA button with a confirmation dialog (POST to the admin endpoint, refetch
on success).
- Booking Details modal: the customer's name now links to their User Details
modal (optional openUserModal prop threaded through admin/+page and
today/+page; other call sites unaffected).
Take Payment + /today:
- PaymentModal shows pre-tip (netTotal) and post-tip (totalWithTip) totals
with a tip-amount delta row only when a tip is selected; zero-tip flow
unchanged.
- The /today Payment button is hidden unless the booking is in_progress or
completed, matching the backend gate (was shown for confirmed/pending
bookings, producing the 'Booking must be in_progress or completed' error).
Verification: go test -tags test,dev -count=1 -parallel 8 ./... (20/20 ok
incl. new admin 2FA tests + mw tests), go build ./... and -tags dev both
compile, go vet clean, svelte-check 0 errors 0 warnings, env-docs gate OK,
docker compose config valid.
Guests (account_role='guest') have no login flow and never receive a JWT
normally, so this is defense-in-depth: any token whose role claim is 'guest'
(forged/minted guest tokens or future changes) is refused 403 before the money
handlers run. The check reads role from context ONLY — real guests are seeded
with account_type='email', so account_type is never the discriminator. A
missing role passes through (RequireAuth guarantees presence; same trust model
as isVerifiedRole).
Wired onto all user-facing money routes: booking payment, apply-redemption,
payment-lock POST/DELETE, tip, gift-card redeem and gift-card buy. Admin and
till routes (RequireAdmin) are untouched — admin can never be guest. The
payment-methods routes gain RequireVerified (verified_email, admin) alongside,
so only verified accounts can manage saved cards.
Tests: 6 middleware tests (reject guest, allow verified/unverified/admin/
affiliate/missing-role) + 4 integration tests (guest 403 on booking payment
with zero side effects, tip, gift-card buy; verified user still pays 200).
New test files cover previously untested paths across DAV, validators,
S3, Square, mw, bookings, user, and payments packages.
Includes mock fix: HoldCheckouts flag on MockClient allows tests to
pause auto-complete goroutine for testing PENDING checkout states.
Coverage: 50.4% → 65.0% (+14.6pp)
errcheck: add proper error handling with slog.Error for tx.Rollback, key generation, and s3/dav operations. Add nolint comments for intentionally discarded DB scan errors and HTTP write errors.
unused: remove dead code (svcRow type, processImage, nonDepositPaymentType, generateSecureCode, colorBold, nGreen, nRed)
gosimple S1021: merge var declaration with assignment in manage.go
ineffassign: remove dead assignments in settings.go, till.go, images.go
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
OptionalAuth extracts user info from Bearer token if present,
otherwise passes through without requiring authentication.
ReserveSlotHandler creates temporary time_blockers reservations:
- Logged-in users: max 1 active, 1hr TTL, tracked by user_id
- Anonymous users: global cap of 50, 10min TTL, tracked by IP hash
- Validates slot availability (working hours, booking overlap, blocker overlap)
- Returns reservation ID, start_time, duration, and expires_at
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
popertots
