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

fix: full-scope review — tip-inclusive amount_due, sweep deposit-strand, A6 clamp cap, B13 clawback, 2FA single-use, mint audit, account-deletion re-auth, refresh dedup

Browse Source 
Full-scope Loop A restart review (18 findings across money/security/dup-mod):

MONEY:
- HIGH: amount_paid/amount_due CTEs now exclude payment_type='tip' (bookings.go x6, today.go) — a tip before the final balance no longer undercharges the booking
- MEDIUM-HIGH: pending payment row stores the actual chargeAmount (not req.Amount) so the sweep replay amount-match rescues deposit-with-discount rows instead of auto-refunding them; refundSweepDuplicateCharge refunds the replayed payment's actual amount
- MEDIUM: A6 deposit clamp-up now caps at the discounted obligation (remainingPence - eligibleDiscountPence) — no more silent overcharge when a campaign discount >= deposit
- MEDIUM: B13 campaign-loss balance credits are clawed back on cancellation (clawbackB13CampaignCredit in ProcessCancellationRefundTx)
- LOW: replayLegitimateRetryWindow extended 22h->24h so a legitimate same-key retry in the retry-eligible window is rescued, not auto-refunded

SECURITY:
- 2FA single-use strengthened (consume-at-gate for fresh charges, re-issue on failure)
- Admin 2FA mint now writes admin_audit_log + logs code reuse
- Account deletion requires current password (and 2FA when enforced) — stolen token can no longer destroy the account
- Multi-tab refresh-token replay deduped via cross-tab lock (no false family-kill alerts)
- family-alive cache invalidated on password change / GDPR erasure
- Login lockout keyed per user+IP with a capped ceiling

FRONTEND/DUP-MOD:
- OverflowTipConfirm shared component (UserPaymentModal + BookingFlow); overflow computation aligned (deposit-discount-aware)
- PaymentModal admin 2FA gate now method-conditioned (no over-reveal on cash/giftcard)
- requestTwoFactorCode shared helper (requestNewTwoFactorCode + adminRequestNewTwoFactorCode)
- BookingFlow deposit display aligned to the discounted amount; formatCurrency used consistently

26/26 backend packages; 80/80 frontend tests + build; env-docs 41/41.
This commit is contained in:
popertots
2026-08-22 00:34:50 +01:00
parent b46927336b
commit 9a182db932
27 changed files with 1279 additions and 300 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/internal/twofa/twofa.go
+61 -25
View File
@@ -11,7 +11,7 @@
//
// Contract for the payments gate:
//
// err := twofa.VerifyForUser(ctx, userID, code, false) // consume = false
// err := twofa.VerifyForUser(ctx, userID, code, twofa.ConsumeOnVerify)
// if err != nil {
// switch {
// case errors.Is(err, twofa.ErrIncorrect):
@@ -25,18 +25,25 @@
// }
// }
//
// The payments saved-card charge gate verifies WITH consume=false (MEDIUM-2):
// the code is checked at gate time but only NULLed when the charge reaches a
// TERMINAL SUCCESS state (the handlers call twofa.ConsumePendingCode inside the
// transaction that records the completed charge). A failed/ambiguous Square
// charge therefore does NOT burn the code — the same-key retry re-verifies the
// SAME operator-relayed code instead of hitting a 400 "expired". Consumption is
// idempotent, so a code still authorizes exactly one completed charge (and
// remains bounded by its 10-minute lifetime). The interactive setup/disable
// flows pass consume=false too — they clear the pending fields themselves on
// success (enableTwoFA / disableTwoFA), so the code must stay valid through
// their whole handshake. The ONLY remaining consume=true caller is the
// save-card SAVE gate (handlers/payments), where saving a card is itself a
// Consume mode (MEDIUM-2 remediation, finding 1): a successful verify with
// consume=true NULLs the pending code ATOMICALLY in the same critical section
// as the check, so one code authorizes exactly ONE operation — two concurrent
// charges can never both pass the gate with the same code (the per-user mutex
// serializes Check, and the second verify reads a NULLed digest and returns
// ErrMissingOrExpired). The payments saved-card CHARGE gates should therefore
// pass twofa.ConsumeOnVerify for FRESH charges: the code is burned at the gate,
// and a failed/ambiguous Square charge re-mints a fresh code (via the user
// package's exported EnsurePendingTwoFACode — reached through the HTTP mint
// endpoints, since handlers/payments cannot import handlers/user) instead of
// re-verifying the same code. This replaces the earlier MEDIUM-2 deferred
// consume (verify-with-consume=false at the gate + ConsumePendingCode at
// terminal success), which under concurrency let two gates both verify the same
// code before either charge consumed it.
//
// The interactive setup/disable flows pass DeferredConsume (false) — they clear
// the pending fields themselves on success (enableTwoFA / disableTwoFA), so the
// code must stay valid through their whole handshake. The save-card SAVE gate
// (handlers/payments) passes ConsumeOnVerify (true), since saving a card is a
// terminal operation with no downstream charge to attach consumption to.
//
// The failed-attempt counter is keyed per user and resets ONLY on a successful
@@ -66,6 +73,22 @@ import (
// before the pending code is invalidated and a new one must be requested.
const MaxAttempts = 5
// Consume mode for VerifyForUser / Check. Named so the magic bool cannot drift
// between call sites (the payments gate vs the interactive flows).
const (
// ConsumeOnVerify makes a successful verify SINGLE-USE immediately: the
// pending-code digest and expiry are NULLed in the same critical section as
// the successful check (see Check). Use this for FRESH terminal operations —
// the saved-card CHARGE gates (finding 1) and the SAVE gate — where one
// code must authorize exactly one operation.
ConsumeOnVerify = true
// DeferredConsume verifies WITHOUT consuming; the caller NULLs the code
// itself when its operation reaches terminal success (ConsumePendingCode) or
// clears the pending fields on success (the interactive enable/disable
// flows).
DeferredConsume = false
)
// AttemptWindow bounds how long a per-user attempt counter lives before
// resetting, and doubles as the stale-entry eviction horizon for the map.
const AttemptWindow = 10 * time.Minute
@@ -287,13 +310,13 @@ const (
// MissingOrExpired. consume makes a correct code single-use IMMEDIATELY: the
// stored digest and its expiry are NULLed right here, so one code cannot
// authorize a second operation within its lifetime. The interactive
// setup/disable flows pass false and clear the pending fields themselves on
// success. The payments saved-card charge gate now ALSO passes false (MEDIUM-2):
// it verifies at gate time and defers consumption to the completed-charge
// transaction via ConsumePendingCode, so a failed Square charge does not burn
// the code. The returned error is non-nil only for DB failures (callers return
// 500); a lockout's pending-code invalidation failure is logged here and still
// reported as a lockout.
// setup/disable flows pass DeferredConsume (false) and clear the pending fields
// themselves on success. The payments saved-card charge gates pass
// ConsumeOnVerify (true) for FRESH charges (finding 1): the code is burned at
// the gate, and a failed Square charge re-mints a fresh one. The returned
// error is non-nil only for DB failures (callers return 500); a lockout's
// pending-code invalidation failure is logged here and still reported as a
// lockout.
func Check(ctx context.Context, userID string, st *AttemptState, reqCode string, consume bool) (Result, error) {
if now := clock.Now(); now.Sub(st.LastActive()) > AttemptWindow {
st.Count.Store(0)
@@ -360,6 +383,18 @@ func Check(ctx context.Context, userID string, st *AttemptState, reqCode string,
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 /
// locked_until) — a successful 2FA challenge is a strong auth signal, and
// the only way to reach a 2FA verify is an already-authenticated session.
// Best-effort: a failure only logs; the verify has already succeeded.
if _, err := db.Conn.Exec(ctx, `
UPDATE users
SET failed_attempts = 0, locked_until = NULL
WHERE id = $1
`, userID); err != nil {
log.Printf("failed to clear login lockout on 2FA verify for user %s: %v", userID, err)
}
if consume {
// Consume mode (the payments saved-card gate, B6/B10): a verified code
// is single-use. NULL the stored digest and its expiry so the same code
@@ -427,11 +462,12 @@ var (
// the payments card-access gate (B6/B10): a saved-card charge must present a
// real, freshly-verified challenge. consume makes a correct code single-use
// IMMEDIATELY (the pending-code digest and expiry are NULLed in the same
// critical section as the successful check — see Check). The payments SAVED-
// CARD CHARGE gate passes false and consumes later via ConsumePendingCode
// (MEDIUM-2) so a failed charge does not burn the code; the save-card SAVE
// gate and the interactive setup/disable flows pass false and clear the
// pending fields themselves on success (enableTwoFA / disableTwoFA).
// critical section as the successful check — see Check). The payments saved-
// card CHARGE gate passes ConsumeOnVerify for FRESH charges (finding 1: a code
// authorizes exactly one charge, and a failed charge re-mints); the save-card
// SAVE gate passes ConsumeOnVerify too; the interactive setup/disable flows
// pass DeferredConsume and clear the pending fields themselves on success
// (enableTwoFA / disableTwoFA).
func VerifyForUser(ctx context.Context, userID, code string, consume bool) error {
st := StateFor(userID)
st.Mu.Lock()
backend/internal/twofa/twofa_test.go
+49 -8
View File
@@ -40,8 +40,8 @@ func TestVerifyForUser_CorrectAndWrongCode(t *testing.T) {
userID, err := fixtures.CreateTestUser(tx)
require.NoError(t, err)
seedPending(t, ctx, tx, userID, "123456")
require.NoError(t, VerifyForUser(ctx, userID, "123456", false), "correct code must verify")
require.ErrorIs(t, VerifyForUser(ctx, userID, "999999", false), ErrIncorrect)
require.NoError(t, VerifyForUser(ctx, userID, "123456", DeferredConsume), "correct code must verify")
require.ErrorIs(t, VerifyForUser(ctx, userID, "999999", DeferredConsume), ErrIncorrect)
// consume=true (payments saved-card gate path): a success DESTROYS the
// pending code, so re-verifying the same code reports ErrMissingOrExpired
@@ -49,8 +49,8 @@ func TestVerifyForUser_CorrectAndWrongCode(t *testing.T) {
userID2, err := fixtures.CreateTestUser(tx)
require.NoError(t, err)
seedPending(t, ctx, tx, userID2, "123456")
require.NoError(t, VerifyForUser(ctx, userID2, "123456", true), "correct code must verify")
require.ErrorIs(t, VerifyForUser(ctx, userID2, "123456", true), ErrMissingOrExpired, "a consumed code must be single-use")
require.NoError(t, VerifyForUser(ctx, userID2, "123456", ConsumeOnVerify), "correct code must verify")
require.ErrorIs(t, VerifyForUser(ctx, userID2, "123456", ConsumeOnVerify), ErrMissingOrExpired, "a consumed code must be single-use")
var pendingHash sql.NullString
require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_pending_code_hash FROM users WHERE id = $1", userID2).Scan(&pendingHash))
require.False(t, pendingHash.Valid, "a consumed code must be NULLed in the DB")
@@ -63,16 +63,57 @@ func TestVerifyForUser_LockoutAndMissing(t *testing.T) {
seedPending(t, ctx, tx, userID, "123456")
// Wrong code #1 → ErrIncorrect; four more reach the 5-attempt cap.
require.ErrorIs(t, VerifyForUser(ctx, userID, "999999", true), ErrIncorrect)
require.ErrorIs(t, VerifyForUser(ctx, userID, "999999", ConsumeOnVerify), ErrIncorrect)
for i := 0; i < 4; i++ {
_ = VerifyForUser(ctx, userID, "999999", true)
_ = VerifyForUser(ctx, userID, "999999", ConsumeOnVerify)
}
require.ErrorIs(t, VerifyForUser(ctx, userID, "999999", true), ErrLockedOut)
require.ErrorIs(t, VerifyForUser(ctx, userID, "999999", ConsumeOnVerify), ErrLockedOut)
// A fresh user with no pending code → ErrMissingOrExpired.
userID2, err := fixtures.CreateTestUser(tx)
require.NoError(t, err)
require.ErrorIs(t, VerifyForUser(ctx, userID2, "123456", true), ErrMissingOrExpired)
require.ErrorIs(t, VerifyForUser(ctx, userID2, "123456", ConsumeOnVerify), ErrMissingOrExpired)
}
// TestVerifyForUser_ConsumeOnVerifyConcurrency pins the finding-1 contract: a
// code verified with ConsumeOnVerify authorizes exactly ONE operation. Even
// though the per-user mutex serializes the critical section (so no test can
// actually race it), the observable guarantee is that the first verify burns the
// code and any subsequent verify of the same code fails with
// ErrMissingOrExpired — two concurrent charge gates can never both pass.
func TestVerifyForUser_ConsumeOnVerifyConcurrency(t *testing.T) {
ctx, tx := testutils.SetupTestTx(t)
userID, err := fixtures.CreateTestUser(tx)
require.NoError(t, err)
seedPending(t, ctx, tx, userID, "424242")
// Two "concurrent" charge-gate verifies of the same code, serialized by
// StateFor's per-user mutex exactly as the payments gate would experience
// them. Only the first may succeed.
require.NoError(t, VerifyForUser(ctx, userID, "424242", ConsumeOnVerify), "first charge gate must verify")
require.ErrorIs(t, VerifyForUser(ctx, userID, "424242", ConsumeOnVerify), ErrMissingOrExpired,
"second charge gate with the same code must fail — one code, one charge")
}
// TestVerifyForUser_SuccessClearsLoginLockout pins LOW 6b: a successful 2FA
// verify lifts any password-guessing login lockout (users.failed_attempts /
// locked_until) because a correct code proves control of the second factor.
func TestVerifyForUser_SuccessClearsLoginLockout(t *testing.T) {
ctx, tx := testutils.SetupTestTx(t)
userID, err := fixtures.CreateTestUser(tx)
require.NoError(t, err)
seedPending(t, ctx, tx, userID, "123456")
_, err = tx.Exec(ctx, `UPDATE users SET failed_attempts = 9, locked_until = NOW() + INTERVAL '30 minutes' WHERE id = $1`, userID)
require.NoError(t, err)
require.NoError(t, VerifyForUser(ctx, userID, "123456", ConsumeOnVerify))
var failedAttempts int
var lockedUntil *time.Time
require.NoError(t, tx.QueryRow(ctx, `SELECT failed_attempts, locked_until FROM users WHERE id = $1`, userID).Scan(&failedAttempts, &lockedUntil))
require.Zero(t, failedAttempts, "successful 2FA verify must reset the login lockout counter")
require.Nil(t, lockedUntil, "successful 2FA verify must clear locked_until")
}
// TestConsumePendingCode pins the MEDIUM-2 contract: ConsumePendingCode NULLs