popertots/Crussell
1
1
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: round-2 loop-A fresh review (503c326 baseline) — B1 replay cap, A6 discount record, 2FA reissue+cooldown, notification flood, lockout saturation, VAT/refund-status consolidation

Browse Source 
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.
This commit is contained in:
popertots
2026-08-22 00:34:50 +01:00
parent 4e64e32f09
commit 3866cc5963
36 changed files with 2032 additions and 719 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/internal/dav/service_prod.go
+5 -1
View File
@@ -12,6 +12,10 @@ import (
var Service *BaseService
// defaultPostgresHost is the fallback Postgres address used when
// POSTGRES_HOST is unset — the pipeline/CI postgres service default.
const defaultPostgresHost = "127.0.0.1"
func init() {
// Test builds wire their own pool in TestMain (testutils/testdb) — a real
// connect here would race that and panic a bare-shell `go test`. Skip.
@@ -26,7 +30,7 @@ func init() {
func connect() error {
host := getEnv("POSTGRES_HOST")
if host == "" {
host = "127.0.0.1" // pipeline postgres service default
host = defaultPostgresHost
}
dsn := fmt.Sprintf(
"postgres://%s:%s@%s:5432/%s?timezone=UTC&require_auth=scram-sha-256",
backend/internal/twofa/twofa.go
+69 -9
View File
@@ -157,6 +157,26 @@ var (
Map = make(map[string]*AttemptState)
)
// saturatedLockedState is the SHARED attempt state returned by StateFor when
// the attempt map is at capacity and every tracked record is inside its
// lockout window (finding 3, Round 2 Loop A — see StateFor). Its last-activity
// stamp is pinned FAR in the future, so LockedOut always holds and Check's
// window-reset branch (now.Sub(LastActive) > AttemptWindow) can never reach it:
// every untracked user is treated as PERMANENTLY locked out instead of being
// granted a fresh 5-guess budget per request. It is a package-level singleton
// rather than a per-call allocation so the pathological path allocates nothing
// and all saturated requests share one record.
var saturatedLockedState = newSaturatedLockedState()
func newSaturatedLockedState() *AttemptState {
st := &AttemptState{}
st.Count.Store(MaxAttempts)
// Pinned so far in the future that now.Sub(LastActive) is always
// <= AttemptWindow (LockedOut true) and never > AttemptWindow (no reset).
st.SetLastActive(time.Now().Add(24 * 365 * 24 * time.Hour))
return st
}
// StateFor returns the per-user attempt state, creating it if needed. The map
// is bounded: stale (window-expired) entries are evicted opportunistically and,
// when at capacity, the least-recently-active non-locked-out entry is dropped.
@@ -194,11 +214,17 @@ func StateFor(userID string) *AttemptState {
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 a transient, untracked state so THIS request still
// proceeds under a fresh budget.
st := &AttemptState{}
st.SetLastActive(now)
return st
// 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.
return saturatedLockedState
}
}
@@ -377,11 +403,20 @@ func Check(ctx context.Context, userID string, st *AttemptState, reqCode string,
log.Printf("failed to upgrade legacy 2FA pending code hash for user %s: %v", userID, err)
}
}
// Success: clear the attempt counter (and any mint cooldown) before the
// caller performs its action.
// Success: clear the attempt counter before the caller performs its
// action. The mint-cooldown stamp (LastMintAt) is deliberately NOT cleared
// here (Round 2 Loop A finding 2): a code verified at the saved-card gate
// may still be followed by a FAILED Square charge that re-issues a fresh
// code (payments.reissueTwoFACodeAfterFailedCharge), and that re-issue path
// enforces the per-user mint cooldown against this stamp. Clearing it on a
// gate-verify let a charge-failure loop mint a fresh code on every
// iteration with no 60s cooldown (code churn + dev log flooding). The stamp
// is cleared only at a TERMINAL SUCCESS — the completed-charge consumption
// path (ConsumePendingCode, called by the money agent inside the
// transaction that records the completed charge) — so a customer who just
// completed a charge can immediately request a fresh code.
st.Count.Store(0)
st.SetLastActive(clock.Now())
st.LastMintAt = time.Time{}
ResetAttempts(userID)
// LOW 6b: a correct code proves control of the account's second factor, so
// lift any password-guessing login lockout (users.failed_attempts /
@@ -417,7 +452,8 @@ func Check(ctx context.Context, userID string, st *AttemptState, reqCode string,
return OK, nil
}
// ConsumePendingCode NULLs the user's pending 2FA code digest and expiry.
// ConsumePendingCode NULLs the user's pending 2FA code digest and expiry, and
// clears the per-user mint-cooldown stamp (AttemptState.LastMintAt).
// Since finding 1 the saved-card CHARGE gates consume a FRESH charge's code at
// verify time (consume=true — single-use), so this is no longer the gate's
// consumption path: it is used by the PENDING-REUSE retry path, whose gate
@@ -428,6 +464,13 @@ func Check(ctx context.Context, userID string, st *AttemptState, reqCode string,
// code still authorizes exactly one completed charge and can never authorize a
// second after success. Accepts a db.Querier so the write can ride the caller's
// transaction (pgx.Tx) or the pool proxy.
//
// Round 2 Loop A finding 2: this is the ONLY place the mint-cooldown stamp is
// cleared on the charge path. A successful gate VERIFY (twofa.Check) must NOT
// clear it — the charge may still fail and the re-issue path
// (payments.reissueTwoFACodeAfterFailedCharge) enforces its cooldown against
// the stamp. Reaching terminal SUCCESS is what re-arms immediate re-minting,
// so consumption (which runs only at that terminal state) clears it.
func ConsumePendingCode(ctx context.Context, q db.Querier, userID string) error {
if userID == "" {
return nil
@@ -441,9 +484,26 @@ func ConsumePendingCode(ctx context.Context, q db.Querier, userID string) error
if err != nil {
return fmt.Errorf("2FA consume pending code: %w", err)
}
// Clear the mint-cooldown stamp. Best-effort and in-memory: a missing or
// evicted entry (e.g. after a process restart) only lets the cooldown
// lapse — it never grants a fresh guessing budget.
ClearMintCooldownForUser(userID)
return nil
}
// 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.
func ClearMintCooldownForUser(userID string) {
st := StateFor(userID)
st.Mu.Lock()
st.LastMintAt = time.Time{}
st.Mu.Unlock()
}
// Classifying errors returned by VerifyForUser.
var (
// ErrIncorrect reports a code that does not match the user's pending code.
backend/internal/twofa/twofa_test.go
+54 -1
View File
@@ -144,6 +144,9 @@ func TestConsumePendingCode(t *testing.T) {
// TestVerifyForUser_AttemptStateMapPersists exercises the shared per-user
// attempt map directly (the state the payments gate shares with the interactive
// endpoints): the map is bounded and a locked-out record is never evicted.
// Round 2 Loop A finding 3: when the map is full of in-window locked-out
// records, a new untracked user gets the SHARED permanently-locked state —
// treated as locked out, not handed a fresh 5-guess budget per request.
func TestVerifyForUser_AttemptStateMapPersists(t *testing.T) {
t.Cleanup(func() {
MapMu.Lock()
@@ -165,8 +168,58 @@ func TestVerifyForUser_AttemptStateMapPersists(t *testing.T) {
st.Count.Store(MaxAttempts)
Map[id] = st
}
_ = StateFor("new_user") // transient, untracked (map full of lockouts)
st := StateFor("new_user") // saturated — shared permanently-locked state
require.True(t, st.LockedOut(clock.Now()), "an untracked user under map saturation must be treated as locked out")
MapMu.Lock()
defer MapMu.Unlock()
require.Len(t, Map, 2, "locked-out records must survive the cap pressure")
}
// TestVerifyForUser_SuccessPreservesMintCooldownStamp pins Round 2 Loop A
// finding 2: a successful verify must NOT clear the per-user mint-cooldown
// stamp (LastMintAt), so the payments re-issue path
// (reissueTwoFACodeAfterFailedCharge) can enforce its 60s cooldown against a
// charge-failure loop. Previously Check cleared the stamp on every verify,
// letting a fresh charge that failed at Square mint a new code per iteration
// with no cooldown. The stamp is cleared only at terminal success via
// ConsumePendingCode (see TestConsumePendingCode_ClearsMintCooldownStamp).
func TestVerifyForUser_SuccessPreservesMintCooldownStamp(t *testing.T) {
ctx, tx := testutils.SetupTestTx(t)
userID, err := fixtures.CreateTestUser(tx)
require.NoError(t, err)
seedPending(t, ctx, tx, userID, "123456")
st := StateFor(userID)
st.Mu.Lock()
st.LastMintAt = clock.Now().Add(-10 * time.Second)
st.Mu.Unlock()
require.NoError(t, VerifyForUser(ctx, userID, "123456", DeferredConsume), "correct code must verify")
st.Mu.Lock()
defer st.Mu.Unlock()
require.False(t, st.LastMintAt.IsZero(), "a successful verify must preserve the mint-cooldown stamp (finding 2)")
}
// TestConsumePendingCode_ClearsMintCooldownStamp pins the other half of finding
// 2: the mint-cooldown stamp is cleared at TERMINAL SUCCESS — the completed-
// charge consumption path — so a customer who just completed a charge can
// immediately request a fresh code. This is the only charge-path place the
// stamp dies.
func TestConsumePendingCode_ClearsMintCooldownStamp(t *testing.T) {
ctx, tx := testutils.SetupTestTx(t)
userID, err := fixtures.CreateTestUser(tx)
require.NoError(t, err)
seedPending(t, ctx, tx, userID, "123456")
st := StateFor(userID)
st.Mu.Lock()
st.LastMintAt = clock.Now().Add(-10 * time.Second)
st.Mu.Unlock()
require.NoError(t, ConsumePendingCode(ctx, tx, userID), "terminal-success consumption must succeed")
st.Mu.Lock()
defer st.Mu.Unlock()
require.True(t, st.LastMintAt.IsZero(), "terminal-success consumption must clear the mint-cooldown stamp (finding 2)")
}