fix: round-2 loop-B adversarial (503c326 baseline) — B1 webhook race, APPROVED refund semantics, notification cap single-source, 2FA cooldown/StateFor hardening, register bcrypt semaphore

Round 2 Loop B red-team (money/security/dup-mod adversarial) findings on the full payments overhaul:

MONEY:
- HIGH: webhook COMPLETED promotion now resolves the B1 parent row (mirrors the re-poll resolveB1ParentFailed + till-sale clawback) — the sweep no longer re-replays an expired key into stacked unauthorized charges
- HIGH: A6 deposit-with-discount clamp — chargeAmount capped to max(0, remaining-discount) for ALL discount cases; overflow guard compares against the discounted remaining
- MED-HIGH: APPROVED refunds treated as NON-terminal at the webhook (event-driven, may still fail); payments call sites aligned; FAILED can now demote an APPROVED-then-failed row
- MED: B1 refund transport-error fails the row + CRITICAL immediately (no 3-charge stacking)
- MED: till_sales capped-fail surfaces the outstanding funding (gift_card_transactions trace) for manual reversal
- MED: guest-bookings cash/gift-card terminal charges now audited (NULL target); audit reordered post-commit; cancellation refunds audited
- MED: A6 no-discount skip-path returns campaign_fully_redeemed 400 (no success-shaped no-op); skip-path writes a marker row for idempotency

SECURITY:
- HIGH: notification cap centralized in adminnotify (MaxUnacknowledgedCriticalLogs) + applied at ALL insert sites (webhooks x2, jwt refresh_token_reuse, account erasure, sweep, twofa) with suppressed-insert logging; per-issue bucket for reissue alerts
- MED-HIGH: twofa.StateFor saturated state made IMMUTABLE (LastMintAt writes are no-ops; no cross-user throttling); eviction never drops in-window count>0 records
- MED: /register now uses the shared bcrypt semaphore (authBcryptSlots, 20) — botnet CPU burn bounded
- MED: NAT collateral reduced (429-reject only at top progressive tier; lower tiers sleep)
- MED: ClearMintCooldownForUser exposed for fresh-charge success; reissue cooldown-skip raises a capped alert
- LOW: audit coverage gaps (reschedule fee forgiveness, gift-card transfer, clawback) closed

DUP/MOD:
- Frontend deposit-percent literals -> POLICY constants (10 sites); LOYALTY_DISCOUNT_RATE single-sourced; generateUUID adopted; admin PaymentModal overflow-tip confirm path added; £500 gift-card cap named

Verified: 26/26 dev + 24/24 prod (CI condition), both vet tags, frontend tests+build, env-docs 42/42.
This commit is contained in:
2026-08-22 00:34:50 +01:00
parent 3866cc5963
commit 4e398a7a2b
31 changed files with 1703 additions and 342 deletions
+65 -14
View File
@@ -143,6 +143,27 @@ func (st *AttemptState) SetLastActive(t time.Time) {
st.LastAt.Store(t.UnixNano())
}
// SetLastMintAtLocked records the per-user mint-cooldown stamp. The caller
// MUST already hold st.Mu (matching how the stamp is read by
// twoFAMintThrottled in handlers/user and the payments re-issue cooldown).
//
// Round 2 Loop B finding 3a: writing to the SHARED saturated state is a
// NO-OP. saturatedLockedState is returned by StateFor for EVERY untracked user
// once the map is at capacity, so a mint stamped on it would throttle every
// untracked user for the whole cooldown (one user's mint blocks everyone for
// 60s) and ClearMintCooldownForUser would clear it for all of them. The
// singleton's stamp therefore stays permanently zeroed: under saturation,
// per-user mints are unthrottled, which is SAFE because a mint never grants a
// fresh guessing budget (B11b) and the saturated state is already permanently
// locked out for verification. The stamp must also never be written by the
// reissue/mint paths when st IS the singleton — the no-op below guarantees it.
func (st *AttemptState) SetLastMintAtLocked(t time.Time) {
if st == saturatedLockedState {
return
}
st.LastMintAt = t
}
// LockedOut reports whether the state is inside its lockout window: the attempt
// counter has reached the cap and the window has not yet elapsed. Such a record
// is the rate limit's source of truth for its user and must never be evicted
@@ -199,9 +220,14 @@ func StateFor(userID string) *AttemptState {
delete(Map, id)
continue
}
if st.LockedOut(now) {
// Inside its lockout window — the rate limit's source of truth
// for this user. Never evict (finding-e fix).
if st.LockedOut(now) || st.Count.Load() > 0 {
// Round 2 Loop B finding 3b: an in-window record with a
// NON-ZERO attempt counter is the rate limit's IN-PROGRESS
// state for a genuine user (a locked-out record, or a user
// mid-window with failed attempts banked). Evicting it would
// silently reset the counter and grant a fresh guessing
// budget. Only count==0 in-window records (fresh lookups /
// idle mint-cooldown stamps) are evictable.
continue
}
if at := st.LastActive(); oldestID == "" || at.Before(oldestAt) {
@@ -212,18 +238,30 @@ func StateFor(userID string) *AttemptState {
delete(Map, oldestID)
}
if len(Map) >= MaxTrackedAttempts {
// Every entry is a locked-out in-window record. Do not evict one
// (that would reset its rate limit) and do not grow past the cap:
// return the SHARED permanently-locked state (finding 3, Round 2
// Loop A). Previously a fresh transient state was returned per
// call, so every untracked user received a fresh 5-guess budget
// per request — silently disabling the brute-force lockout exactly
// Every entry is a protected in-window record (locked out, or
// carrying an in-progress counter). Do not evict one (that would
// reset its rate limit) and do not grow past the cap: return the
// SHARED permanently-locked state (finding 3, Round 2 Loop A).
// Previously a fresh transient state was returned per call, so
// every untracked user received a fresh 5-guess budget per
// request — silently disabling the brute-force lockout exactly
// under the hostile flood that saturated the map. The shared state
// treats every untracked user as locked out instead. It is never
// stored in Map (so the eviction scan / ResetAttempts /
// DeleteAttempts never touch it) and self-heals: as soon as one of
// the real locked-out records lapses out of its window, StateFor
// evicts it and normal per-user tracking resumes.
//
// Round 2 Loop B finding 3c — ACCEPTED RESIDUAL: an attacker can
// still fill the map with up to MaxTrackedAttempts (~10k)
// in-window locked-out records, forcing every other user into the
// shared saturated state. That is a bounded, FAIL-CLOSED outcome:
// the fallback is a locked-out-everyone state (brute force
// impossible, availability reduced), never a locked-out-nobody one.
// Mint cooldowns are unthrottled under saturation (the shared
// stamp is zeroed — see SetLastMintAtLocked), which is safe
// because a mint grants no guessing budget (B11b). The map drains
// as locked-out windows lapse.
return saturatedLockedState
}
}
@@ -492,13 +530,26 @@ func ConsumePendingCode(ctx context.Context, q db.Querier, userID string) error
}
// ClearMintCooldownForUser zeroes the user's mint-cooldown stamp (LastMintAt)
// under the per-user mutex — LastMintAt is only ever touched under Mu. Exported
// so the payments re-issue path's coordination contract is actionable (Round 2
// Loop A finding 2): a FRESH-charge terminal-success path that consumed the
// code at the gate (consume=true, so ConsumePendingCode is not called) can call
// this to re-arm immediate re-minting after a completed charge.
// under the per-user mutex — LastMintAt is only ever touched under Mu. A no-op
// when the user's state IS the shared saturated singleton (Round 2 Loop B
// finding 3a: the shared stamp must not be cleared for every untracked user by
// one user's terminal success).
//
// Round 2 Loop B finding 6a — COORDINATION CONTRACT (money agent): the FRESH
// saved-card charge path consumes the customer's 2FA code AT THE GATE
// (consume=true), so twofa.ConsumePendingCode is NOT called on its terminal
// success and the mint-cooldown stamp survives — a customer who completes a
// fresh charge within 60s of their last code mint and immediately requests a
// new code gets 429 for the rest of the window. The money agent's fresh-charge
// terminal-success path in handlers/payments/handlers.go should call
// ClearMintCooldownForUser(userID) at the same point a completed charge is
// recorded, so a just-completed charge re-arms immediate re-minting. This
// function is the exported, coordination-actionable entry point for that call.
func ClearMintCooldownForUser(userID string) {
st := StateFor(userID)
if st == saturatedLockedState {
return
}
st.Mu.Lock()
st.LastMintAt = time.Time{}
st.Mu.Unlock()