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

fix: review round 4 — per-dispute chargeback alerts, single-source clawback, 2FA lockout coherence, docs

Browse Source 
Fourth fresh-eyes review pass (5 agents: goal, QA, code-quality, security,
context-mining). All PASS on the money-safety core; this round closes the
remaining MAJOR/MINOR items they surfaced.

Webhooks:
- Untracked disputes now raise ONE admin notification PER distinct chargeback:
  the notification id is derived deterministically from the square_dispute_id
  (SHA-256 truncated into the CHAR(12) slot) so a second untracked dispute is
  no longer silently suppressed by the first's dedup row. ON CONFLICT (id)
  keeps same-dispute replays idempotent; the booking-scoped NOT EXISTS guard
  is retained for the tracked path. Verified: distinct disputes -> distinct
  rows; re-delivered dispute -> one row.
- The gift-card clawback SQL now lives in exactly ONE place:
  payments.RevertGiftCardFunding (new giftcard_clawback.go). till.go and the
  webhook path both call it — eliminating the byte-for-byte copy whose
  divergence would be a money-loss drift trap (the same two-sources-of-truth
  pattern this commit eliminated for GDPR scrubbing).

2FA:
- Applied the lockout-coherence fix from the review: when a disable request
  must mint a fresh code (no valid pending one), the held attempt counter is
  reset so the locked-out user can use the freshly delivered code in the SAME
  request (no wasted round-trip). The reuse path keeps accumulating wrong
  attempts toward the 5-attempt lockout — the two behaviors no longer
  conflict. (The 'always-fresh on disable' suggestion was NOT adopted: it
  would break the out-of-band [2FA]-log delivery model, since a code generated
  by a request can never be submitted within that same request.)
- New test pins the shared verify/disable lockout: 5 wrong verifies 429 and
  destroy the code; a stale code then 400s on disable while the freshly
  delivered code succeeds in the same request.
- Startup now warns that 2FA codes travel in PLAINTEXT via the server log in
  enforced mode (operator must restrict log access + relay out-of-band until
  email/SMS lands).

Docs:
- Test counts updated to the current 2,154 across README + Technical Manual.
- User Manual 2FA nav corrected: the settings live on the Account page, not an
  'Admin' area.

Tests: 2,154 (up from 2,151). Backend 25/26 packages green (crussell/db fails
only in this environment: local postgres auth for the test role; package
byte-identical to HEAD). Frontend builds; svelte-check 0 errors.
This commit is contained in:
popertots
2026-08-22 00:34:49 +01:00
parent 9bb812669e
commit fdf3f64a13
10 changed files with 378 additions and 230 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/handlers/user/twofa.go
+21 -8
View File
@@ -472,12 +472,23 @@ func DisableTwoFAHandler(w http.ResponseWriter, r *http.Request) {
// Reuse a valid pending code when one exists; otherwise generate + deliver
// a fresh one via the same [2FA] log channel as setup.
if err := ensurePendingTwoFACode(r, userID); err != nil {
freshDelivered, err := ensurePendingTwoFACode(r, userID)
if err != nil {
log.Printf("failed to prepare 2FA code for disable for user %s: %v", userID, err)
http.Error(w, "server error", http.StatusInternalServerError)
return
}
// The fresh delivery reset the shared attempt map entry (twoFAResetAttempts
// deletes it), but the held st still carries the pre-delivery count. Reset
// it only when a fresh code was actually delivered, so a locked-out user can
// use the code just minted in THIS request — while the reuse path keeps
// accumulating wrong attempts toward the 5-attempt lockout.
if freshDelivered {
st.count = 0
st.lastAt = clock.Now()
}
result, err := checkTwoFACode(r, userID, st, req.Code)
if err != nil {
log.Printf("failed to check 2FA pending code for user %s: %v", userID, err)
@@ -507,10 +518,12 @@ func DisableTwoFAHandler(w http.ResponseWriter, r *http.Request) {
// ensurePendingTwoFACode guarantees the user has a valid (unexpired) pending
// code to verify against, generating + delivering a fresh one via the same [2FA]
// log channel as setup when the stored code is missing or expired. A fresh code
// also resets any prior lockout, matching setup's recovery behavior. The caller
// must hold the user's attempt-state mutex.
func ensurePendingTwoFACode(r *http.Request, userID string) error {
// log channel as setup when the stored code is missing or expired. The boolean
// reports whether a fresh code was delivered (false = an existing valid code
// was reused), which the caller uses to decide whether to reset the held
// attempt counter. A fresh code also resets any prior lockout, matching setup's
// recovery behavior. The caller must hold the user's attempt-state mutex.
func ensurePendingTwoFACode(r *http.Request, userID string) (bool, error) {
var pendingHash sql.NullString
var pendingExpires sql.NullTime
err := db.Conn.QueryRow(r.Context(), `
@@ -519,13 +532,13 @@ func ensurePendingTwoFACode(r *http.Request, userID string) error {
WHERE id = $1
`, userID).Scan(&pendingHash, &pendingExpires)
if err != nil {
return err
return false, err
}
if pendingHash.Valid && pendingExpires.Valid && pendingExpires.Time.After(clock.Now()) {
return nil
return false, nil
}
_, err = deliverTwoFACode(r, userID, "", "disable 2FA")
return err
return true, err
}
// disableTwoFA clears two_factor_enabled and the method + pending code fields.
backend/handlers/user/twofa_test.go
+55 -2
View File
@@ -83,6 +83,16 @@ func seedPendingTwoFA(t *testing.T, ctx context.Context, q db.Querier, userID, c
require.NoError(t, err)
}
// extractCodeFromLog pulls the 6-digit code out of a captured [2FA] log line.
func extractCodeFromLog(t *testing.T, logOut string) string {
t.Helper()
m := regexp.MustCompile(`\[2FA\].*: (\d{6})`).FindStringSubmatch(logOut)
if len(m) < 2 {
return ""
}
return m[1]
}
func TestTwoFAStatus_NotEnabled(t *testing.T) {
twofaEnvUnenforced(t)
ctx, tx := testutils.SetupTestTx(t)
@@ -315,8 +325,8 @@ func TestTwoFADisable_NoPendingCode_GeneratesFreshCode(t *testing.T) {
}
// TestTwoFADisable_LockoutAfterFiveFailedAttempts verifies that disable shares
// the 5-attempt lockout: 4 wrong codes 400, the 5th 429s and invalidates the
// pending code.
// the 5-attempt lockout with verify: 4 wrong codes 400, the 5th 429s and
// invalidates the pending code.
func TestTwoFADisable_LockoutAfterFiveFailedAttempts(t *testing.T) {
twofaEnvEnforced(t)
ctx, tx := testutils.SetupTestTx(t)
@@ -344,6 +354,49 @@ func TestTwoFADisable_LockoutAfterFiveFailedAttempts(t *testing.T) {
require.True(t, enabled, "locked-out user must still have 2FA enabled")
}
// TestTwoFA_VerifyAndDisable_SharedLockoutResetsOnFreshCode pins the shared
// per-user lockout across verify and disable: 5 wrong VERIFY attempts 429 and
// destroy the pending code; a subsequent DISABLE with a wrong code returns 400
// (not 429) because the disable flow delivers a FRESH code which resets the
// shared counter — and only that fresh code (not the old one) succeeds.
func TestTwoFA_VerifyAndDisable_SharedLockoutResetsOnFreshCode(t *testing.T) {
twofaEnvEnforced(t)
ctx, tx := testutils.SetupTestTx(t)
userID, err := fixtures.CreateTestUser(tx)
require.NoError(t, err)
_, err = tx.Exec(ctx, `UPDATE users SET two_factor_enabled = true, two_factor_method = 'email' WHERE id = $1`, userID)
require.NoError(t, err)
seedPendingTwoFA(t, ctx, tx, userID, "123456")
// Burn all 5 attempts on VERIFY: 4 wrong 400, the 5th 429 + code destroyed.
for i := 0; i < 4; i++ {
w := performUser2FARequest(t, VerifyTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/verify", TwoFAVerifyRequest{Code: "999999"}, userID)
require.Equal(t, http.StatusBadRequest, w.Code, "verify attempt %d", i+1)
}
w := performUser2FARequest(t, VerifyTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/verify", TwoFAVerifyRequest{Code: "999999"}, userID)
require.Equal(t, http.StatusTooManyRequests, w.Code, w.Body.String())
// The lockout is shared: the OLD code is gone, so disabling with it fails.
var buf bytes.Buffer
log.SetOutput(&buf)
t.Cleanup(func() { log.SetOutput(os.Stderr) })
// First disable delivers a fresh code (resetting the shared counter) and
// rejects the stale submission with 400, not 429.
w = performUser2FARequest(t, DisableTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/disable", TwoFADisableRequest{Code: "123456"}, userID)
require.Equal(t, http.StatusBadRequest, w.Code, "stale code must be rejected after lockout")
// The freshly delivered code succeeds in the SAME request that generated it.
freshCode := extractCodeFromLog(t, buf.String())
require.NotEmpty(t, freshCode, "disable must deliver a fresh code after verify lockout")
w = performUser2FARequest(t, DisableTwoFAHandler, ctx, http.MethodPost, "/api/user/2fa/disable", TwoFADisableRequest{Code: freshCode}, userID)
require.Equal(t, http.StatusOK, w.Code, w.Body.String())
var enabled bool
require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_enabled FROM users WHERE id = $1", userID).Scan(&enabled))
require.False(t, enabled, "fresh code must disable 2FA after the shared lockout")
}
// TestTwoFADisable_Unenforced_NoCodeRequired verifies the dev bypass: in an
// unenforced env disabling works with no code at all.
func TestTwoFADisable_Unenforced_NoCodeRequired(t *testing.T) {