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

fix: restart-loop-A findings — pending sweep refunds, tip carve on discounts, TOCTOU redemption, single-use 2FA code + mint endpoint, refresh-token family revocation, admin 2FA code UX

Browse Source 
Restart of Loop A (fresh review -> fix -> verify) findings from commit 5e967fa:
- B1: sweep auto-refund treats Square PENDING refunds as NON-terminal (row stays pending, no gift-card clawback, refunds row inserted for payments AND till_sales, re-polls the deterministic sweepdup- key); Square-less pre-pass exempts square_refund_id IS NOT NULL rows
- M4: terminal tip carve accounts for pending campaign discounts (headroom = total - pending - paid) so explicit tips aren't absorbed as service revenue; no-tip case stays a single record
- max_redemptions TOCTOU closed with atomic conditional UPDATE ... RETURNING; exhausted-at-apply surfaces campaign_fully_redeemed
- 2FA: verification code is single-use on the saved-card gate (VerifyForUser consume=true, interactive flows unaffected); new POST /api/user/2fa/code mints a fresh code for enabled users (RequireAuth + RequireNonGuest + mint cooldown + per-user limiter)
- Refresh tokens: family_id + used_at columns; reuse of an already-rotated token revokes the ENTIRE family and inserts a refresh_token_reuse admin alert; rotation mints descendants in the same family
- Frontend: 2FA code input + Request-a-new-code on all saved-card surfaces; admin modal keys code input to customer 2FA + 403 self-heal; tip-display note for pending discounts; 76 frontend tests
- Verified: all 26 backend packages pass, frontend build+tests green, env-docs 41/41
This commit is contained in:
popertots
2026-08-22 00:34:50 +01:00
parent fe88f2084d
commit 4d5d2cd381
28 changed files with 2047 additions and 341 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/internal/twofa/twofa.go
+46 -8
View File
@@ -11,7 +11,7 @@
//
// Contract for the payments gate:
//
// err := twofa.VerifyForUser(ctx, userID, code)
// err := twofa.VerifyForUser(ctx, userID, code, true) // consume = true
// if err != nil {
// switch {
// case errors.Is(err, twofa.ErrIncorrect):
@@ -25,6 +25,14 @@
// }
// }
//
// A correct code is SINGLE-USE on the payments gate: the gate passes
// consume=true, so the stored pending-code digest and its expiry are NULLed in
// the same critical section as the successful check. One code therefore
// authorizes exactly one saved-card charge, never unlimited charges for its
// 10-minute lifetime. The interactive setup/disable flows pass consume=false —
// they clear the pending fields themselves on success (enableTwoFA /
// disableTwoFA), so the code must stay valid through their whole handshake.
//
// The failed-attempt counter is keyed per user and resets ONLY on a successful
// verify (or after the 10-minute attempt window elapses) — never on a fresh
// code mint, so minting a new code cannot grant a fresh guessing budget (B11b).
@@ -259,10 +267,14 @@ const (
// check. A correct code resets the attempt counter and returns OK. An incorrect
// code increments the counter and, on the 5th consecutive failure, invalidates
// the pending code (lockout). A missing or expired pending code returns
// MissingOrExpired. The returned error is non-nil only for DB failures
// MissingOrExpired. consume makes a correct code single-use: the stored digest
// and its expiry are NULLed immediately, so one code cannot authorize a second
// operation within its lifetime (the payments saved-card gate passes true; the
// interactive setup/disable flows pass false and clear the pending fields
// themselves on success). 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) (Result, error) {
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)
st.SetLastActive(now)
@@ -309,8 +321,11 @@ func Check(ctx context.Context, userID string, st *AttemptState, reqCode string)
}
// Success: a legacy (pre-pepper) hash that verified is re-hashed with the
// pepper so the plain digest is retired on the next successful verify.
if legacy {
// pepper so the plain digest is retired on the next successful verify. This
// matters only for the interactive paths (consume=false), where the pending
// code stays valid for the rest of the handshake — consume mode destroys
// the digest outright, so there is nothing to upgrade.
if legacy && !consume {
if _, err := db.Conn.Exec(ctx, `
UPDATE users
SET two_factor_pending_code_hash = $2
@@ -325,6 +340,25 @@ func Check(ctx context.Context, userID string, st *AttemptState, reqCode string)
st.SetLastActive(clock.Now())
st.LastMintAt = time.Time{}
ResetAttempts(userID)
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
// cannot authorize a second saved-card charge within its 10-minute
// lifetime. The interactive setup/disable flows pass consume=false:
// they clear the pending fields themselves on success (enableTwoFA /
// disableTwoFA), so the code must stay valid through the whole
// verification handshake here. The write goes through the same
// context-routed connection as the rest of Check, so verification and
// consumption are one unit.
if _, err := db.Conn.Exec(ctx, `
UPDATE users
SET two_factor_pending_code_hash = NULL,
two_factor_pending_code_expires = NULL
WHERE id = $1
`, userID); err != nil {
log.Printf("failed to consume 2FA pending code for user %s: %v", userID, err)
}
}
return OK, nil
}
@@ -345,13 +379,17 @@ var (
// It returns nil on a correct code, or one of ErrIncorrect / ErrLockedOut /
// ErrMissingOrExpired (or a DB error, wrapped). This is the entry point for
// the payments card-access gate (B6/B10): a saved-card charge must present a
// real, freshly-verified challenge.
func VerifyForUser(ctx context.Context, userID, code string) error {
// real, freshly-verified challenge. consume makes a correct code single-use:
// the pending-code digest and its expiry are NULLed in the same critical
// section as the successful check (see Check), so one code authorizes exactly
// one gate pass. The interactive setup/disable flows pass false — they 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()
defer st.Mu.Unlock()
result, err := Check(ctx, userID, st, code)
result, err := Check(ctx, userID, st, code, consume)
if err != nil {
return fmt.Errorf("2FA verify: %w", err)
}
backend/internal/twofa/twofa_test.go
+21 -6
View File
@@ -10,6 +10,7 @@ package twofa
import (
"context"
"database/sql"
"testing"
"time"
@@ -33,12 +34,26 @@ func seedPending(t *testing.T, ctx context.Context, tx db.Querier, userID, code
func TestVerifyForUser_CorrectAndWrongCode(t *testing.T) {
ctx, tx := testutils.SetupTestTx(t)
// consume=false (interactive setup/disable path): a success keeps the
// pending code valid, so a wrong follow-up code reports ErrIncorrect.
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"), "correct code must verify")
require.ErrorIs(t, VerifyForUser(ctx, userID, "999999"), ErrIncorrect)
// consume=true (payments saved-card gate path): a success DESTROYS the
// pending code, so re-verifying the same code reports ErrMissingOrExpired
// — a verified code is single-use and cannot authorize a second charge.
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")
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")
}
func TestVerifyForUser_LockoutAndMissing(t *testing.T) {
@@ -48,16 +63,16 @@ 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"), ErrIncorrect)
require.ErrorIs(t, VerifyForUser(ctx, userID, "999999", true), ErrIncorrect)
for i := 0; i < 4; i++ {
_ = VerifyForUser(ctx, userID, "999999")
_ = VerifyForUser(ctx, userID, "999999", true)
}
require.ErrorIs(t, VerifyForUser(ctx, userID, "999999"), ErrLockedOut)
require.ErrorIs(t, VerifyForUser(ctx, userID, "999999", true), 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"), ErrMissingOrExpired)
require.ErrorIs(t, VerifyForUser(ctx, userID2, "123456", true), ErrMissingOrExpired)
}
// TestVerifyForUser_AttemptStateMapPersists exercises the shared per-user