fix: loop-B adversarial findings — tip-type double-charge, tip-refund capacity, loyalty stamp farming, gate ordering, auth amplification, admin audit log
Loop B restart (money/security/dup-mod adversarial) fixes: - CRITICAL: CreateTerminalPayment rejects payment_type='tip' (mirrors CreateBookingPayment) — a tip-typed admin charge no longer records the FULL amount as a tip and double-collects (all is-paid computations exclude tip rows) - HIGH: tip refunds can no longer re-open booking capacity — refunded_total subqueries filter payment_type <> 'tip' (service.go) and RefundPayment rejects tip rows - MEDIUM: loyalty-stamp farming closed — stamp award once-per-booking via loyalty_stamp_awarded_at column (init-script.sql) + existing same-day guard - MEDIUM: CreateTipPayment/CreateBookingPayment 2FA gates moved AFTER the idempotency completed-dedup (code consumed only on new money paths; terminal path already correct) — lost-response retries return the completed payment instead of 400 - MEDIUM: replayRescueLowerBoundSkew widened to 5m (DB-clock-skew stranded originals now rescued) - MEDIUM-1: verifyFamilyAlive DB amplification reduced via 30s bounded family-alive cache; admin route group rate-limited - MEDIUM-3: admin saved-card charges now write admin_audit_log (handlers.go helper + till); [2FA] log line decoupled from user identity - LOW-1: logout scoped to the presented token's family (no cross-session kill) - LOW-2: refresh-reuse grace widened for same-IP replays - LOW-4: squareEnvironmentMismatch enforced for empty env - LOW-5: uuid.ts hard-fails on Math.random fallback (crypto.randomUUID) - Cash/giftcard tip-enabled overflow mirrors the card-terminal carve 26/26 backend packages; 72/72 frontend tests + build; env-docs 41/41.
This commit is contained in:
@@ -11,7 +11,7 @@
|
||||
//
|
||||
// Contract for the payments gate:
|
||||
//
|
||||
// err := twofa.VerifyForUser(ctx, userID, code, true) // consume = true
|
||||
// err := twofa.VerifyForUser(ctx, userID, code, false) // consume = false
|
||||
// if err != nil {
|
||||
// switch {
|
||||
// case errors.Is(err, twofa.ErrIncorrect):
|
||||
@@ -25,13 +25,19 @@
|
||||
// }
|
||||
// }
|
||||
//
|
||||
// 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 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
|
||||
// terminal operation with no downstream charge to attach consumption to.
|
||||
//
|
||||
// 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
|
||||
@@ -278,13 +284,16 @@ 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. 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.
|
||||
// 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.
|
||||
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)
|
||||
@@ -373,6 +382,32 @@ 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. The
|
||||
// payments saved-card charge gate verifies WITHOUT consuming (MEDIUM-2) and the
|
||||
// handlers call this when the charge reaches a TERMINAL SUCCESS state — inside
|
||||
// the transaction that records the completed charge when one exists — so the
|
||||
// code is consumed atomically with the charge OUTCOME, not the gate. A failed
|
||||
// or ambiguous Square charge leaves the code intact and the same-key retry can
|
||||
// re-verify the SAME code. Idempotent: consuming an already-NULL pending code
|
||||
// is a no-op, so a 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.
|
||||
func ConsumePendingCode(ctx context.Context, q db.Querier, userID string) error {
|
||||
if userID == "" {
|
||||
return nil
|
||||
}
|
||||
_, err := q.Exec(ctx, `
|
||||
UPDATE users
|
||||
SET two_factor_pending_code_hash = NULL,
|
||||
two_factor_pending_code_expires = NULL
|
||||
WHERE id = $1
|
||||
`, userID)
|
||||
if err != nil {
|
||||
return fmt.Errorf("2FA consume pending code: %w", err)
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
// Classifying errors returned by VerifyForUser.
|
||||
var (
|
||||
// ErrIncorrect reports a code that does not match the user's pending code.
|
||||
@@ -390,11 +425,13 @@ 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. 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).
|
||||
// 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).
|
||||
func VerifyForUser(ctx context.Context, userID, code string, consume bool) error {
|
||||
st := StateFor(userID)
|
||||
st.Mu.Lock()
|
||||
|
||||
@@ -75,6 +75,31 @@ func TestVerifyForUser_LockoutAndMissing(t *testing.T) {
|
||||
require.ErrorIs(t, VerifyForUser(ctx, userID2, "123456", true), ErrMissingOrExpired)
|
||||
}
|
||||
|
||||
// TestConsumePendingCode pins the MEDIUM-2 contract: ConsumePendingCode NULLs
|
||||
// the stored pending-code digest and expiry (idempotently), and is the ONLY
|
||||
// place a verified-but-unconsumed code dies on the saved-card charge path.
|
||||
func TestConsumePendingCode(t *testing.T) {
|
||||
ctx, tx := testutils.SetupTestTx(t)
|
||||
userID, err := fixtures.CreateTestUser(tx)
|
||||
require.NoError(t, err)
|
||||
seedPending(t, ctx, tx, userID, "123456")
|
||||
|
||||
// A code verified WITHOUT consuming stays valid (the saved-card charge gate
|
||||
// path, MEDIUM-2) — re-verification must keep working until consumption.
|
||||
require.NoError(t, VerifyForUser(ctx, userID, "123456", false), "verify-without-consume must pass")
|
||||
require.NoError(t, VerifyForUser(ctx, userID, "123456", false), "an unconsumed code must still verify on a same-key retry")
|
||||
|
||||
require.NoError(t, ConsumePendingCode(ctx, tx, userID), "explicit consumption at charge success must succeed")
|
||||
require.ErrorIs(t, VerifyForUser(ctx, userID, "123456", false), ErrMissingOrExpired, "a consumed code must no longer verify")
|
||||
|
||||
// Consumption is idempotent — a second call (e.g. a retried completed
|
||||
// charge) is a no-op, never an error.
|
||||
require.NoError(t, ConsumePendingCode(ctx, tx, userID), "consuming an already-consumed code must be a no-op")
|
||||
|
||||
// An unknown user is a no-op too.
|
||||
require.NoError(t, ConsumePendingCode(ctx, tx, "000000000000"))
|
||||
}
|
||||
|
||||
// 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.
|
||||
|
||||
Reference in New Issue
Block a user