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

fix: round-3 — tip gate asymmetry, webhook VAT align + 503 notifications, cash-tip campaign overcharge, lockout DoS, erasure durability, S3 retry cap, env parsing, per-user rate limiters, consume dead code, frontend 2FA remnants

Browse Source 
- tip gate: CreateTipPayment saved-card 2FA gate now has scaTokenizedSavedCard skip matching every other charge surface (booking, terminal, gift-card); isSCATokenizeResultShape escape added to tip SAVE gate
- webhook: align UPDATE clears VAT fields before re-apply (matches sweep rescue); 503 unknown-event tracking with 24h timeout notification via square_webhook_events table
- cash-tip: cashChargeBasePence no longer restores campaign or subtracts loyalty — overcharge and tip shortfall fixed; 2FA dead code remnants removed from gift-card buy flow; TwoFactorCodeInput help text deconfused; refund pre-fill unit mismatch fixed (pounds vs pence); SCA buyer names split from full_name; passwordless delete UI accepts empty password
- lockout: successful current-password clears shared failed_attempts/locked_until (victim can recover from login lockout via password change); passwordless delete condition changed to require 2FA only in enforced env
- erasure: stale-guest batch erasure persists Square card/customer targets to durable outbox before NULLing them (crash-safe); S3 deletion retry capped at 10 attempts with admin notification; S3_PROFILE_PICS_BUCKET startup check added
- env parsing: IsExplicitDevOrMockEnv and Square HTTP client base-URL switch now normalize (ToLower+TrimSpace) for consistency
- auth: change-password/delete-account get per-user rate limiters (10/min); consume param dead code suppressed with TODO
- frontend: 2FA/SCA dead code removed from gift-card buy flow, TwoFactorCodeInput help text fixed, refund pre-fill unit mismatch fixed, buyer names populated from full_name

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
This commit is contained in:
popertots
2026-08-22 00:34:51 +01:00
co-authored by Sisyphus
parent fba00a10ad
commit 9a12a2d886
27 changed files with 1206 additions and 226 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/handlers/user/account.go
+28 -9
View File
@@ -360,19 +360,30 @@ func DeleteAccountHandler(w http.ResponseWriter, r *http.Request) {
// FIX 2: apply the shared current-password failed-attempt budget BEFORE
// the compare — a stolen session token must not be able to brute-force
// the current password with unlimited guesses.
//
// FIX 1 (round-9): a user locked out by LOGIN attacks (shared
// failed_attempts/locked_until columns) can still recover by providing
// the CORRECT current password here — the lockout is cleared on
// success. When locked out we still run the bcrypt compare (one
// attempt), and if the password is correct the lockout is lifted. If
// the password is wrong while locked out, no additional failure is
// recorded (the lockout stands).
lockedOut := false
if err := checkCurrentPasswordLockout(ctx, userID); err != nil {
if errors.Is(err, errCurrentPasswordLockedOut) {
// FIX 4: uniform 401 — the same status as a wrong password, so
// locked-vs-wrong is never distinguishable; the body text still
// tells the UI which one happened.
lockedOut = true
} else {
log.Printf("Failed to check current-password lockout for user %s: %v", userID, err)
http.Error(w, "server error", http.StatusInternalServerError)
return
}
}
if err := bcrypt.CompareHashAndPassword([]byte(passwordHash.String), []byte(req.CurrentPassword)); err != nil {
if lockedOut {
// FIX 1: already locked out — don't increment further.
http.Error(w, "too many failed attempts — try again later", http.StatusUnauthorized)
return
}
log.Printf("Failed to check current-password lockout for user %s: %v", userID, err)
http.Error(w, "server error", http.StatusInternalServerError)
return
}
if err := bcrypt.CompareHashAndPassword([]byte(passwordHash.String), []byte(req.CurrentPassword)); err != nil {
// FIX 3: the failure record is ONE atomic UPDATE ... RETURNING
// (increment + escalation) — concurrent wrong-password requests
// cannot race a check-then-increment and lose updates.
@@ -387,9 +398,17 @@ func DeleteAccountHandler(w http.ResponseWriter, r *http.Request) {
http.Error(w, "current password is incorrect", http.StatusUnauthorized)
return
}
// FIX 1: a correct current password clears the shared lockout, so a
// login-locked-out user can self-recover by deleting their account.
resetCurrentPasswordFailures(ctx, userID)
}
if !hasPassword || (twoFARequired() && twoFactorEnabled) {
// FIX 2 (round-9): passwordless accounts need 2FA only when enforcement is
// active (twoFARequired()). In unenforced environments (dev/test) the code
// cannot be minted (no delivery channel), so skip the 2FA gate — the
// passwordless property and the authenticated session are the protection.
// Has-password accounts need 2FA only when enforcement is active AND the
// user has 2FA enabled.
if twoFARequired() && (!hasPassword || twoFactorEnabled) {
if req.VerificationCode == "" {
http.Error(w, "a two-factor verification code is required to delete the account", http.StatusBadRequest)
return
backend/handlers/user/account_round9_fixes_test.go
+204 -67
View File
@@ -35,6 +35,10 @@ import (
// password is rejected with 429), and a cleared lockout lets the correct
// password through. Sequential (no t.Parallel): the handler reads the
// process-global s3.Client / payments.SquareClient.
//
// FIX 1 (round-9): a locked-out user who provides the CORRECT current password
// clears the lockout and succeeds — the test verifies that the 6th attempt
// with the correct password now succeeds (the lockout is lifted on success).
func TestDeleteAccount_CurrentPasswordLockout(t *testing.T) {
ctx, tx := testutils.SetupTestTx(t)
userID, err := fixtures.CreateTestUser(tx)
@@ -49,28 +53,19 @@ func TestDeleteAccount_CurrentPasswordLockout(t *testing.T) {
require.Equal(t, http.StatusUnauthorized, rr.Code, "wrong current password must be rejected (attempt %d)", i+1)
}
// The budget is now locked: even the correct password is rejected. FIX 4:
// a locked account returns the SAME uniform 401 as a wrong password (never
// distinguishable), with a distinct body the UI can surface.
// FIX 1: the correct password now clears the lockout and succeeds (the
// locked-out user can self-recover).
req := deleteAccountRequest(t, ctx, userID, "testpassword123", "")
rr := httptest.NewRecorder()
DeleteAccountHandler(rr, req)
require.Equal(t, http.StatusUnauthorized, rr.Code, "delete-account must be rejected with a lockout after 5 wrong current passwords")
require.Contains(t, rr.Body.String(), "too many failed attempts", "the locked body must stay distinct for the UI")
require.Equal(t, http.StatusNoContent, rr.Code, "a locked-out user with the correct current password must be able to recover (FIX 1)")
// The account survives the lockout.
var firstName string
require.NoError(t, tx.QueryRow(ctx, `SELECT n_first_name FROM users WHERE id = $1`, userID).Scan(&firstName))
require.Equal(t, "Test", firstName)
// Clearing the lockout (the documented operator / password-reset recovery)
// lets the correct password through.
_, err = tx.Exec(ctx, `UPDATE users SET failed_attempts = 0, locked_until = NULL WHERE id = $1`, userID)
require.NoError(t, err)
req = deleteAccountRequest(t, ctx, userID, "testpassword123", "")
rr = httptest.NewRecorder()
DeleteAccountHandler(rr, req)
require.Equal(t, http.StatusNoContent, rr.Code, "correct password must succeed after the lockout is reset")
// The lockout was cleared on success.
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, "failed_attempts must be reset to 0 after a successful recovery")
require.Nil(t, lockedUntil, "locked_until must be NULL after a successful recovery")
}
// ============================================================================
@@ -96,6 +91,9 @@ func changePasswordRequest(t *testing.T, ctx context.Context, userID, currentPas
// the same failed-attempt/lockout columns as delete-account: 5 wrong current
// passwords lock the change-password flow (correct password → 429), and a
// cleared lockout lets it through.
//
// FIX 1 (round-9): a locked-out user who provides the CORRECT current password
// clears the lockout and succeeds.
func TestPasswordChange_CurrentPasswordLockout(t *testing.T) {
ctx, tx := testutils.SetupTestTx(t)
userID, err := fixtures.CreateTestUser(tx)
@@ -108,21 +106,18 @@ func TestPasswordChange_CurrentPasswordLockout(t *testing.T) {
require.Equal(t, http.StatusUnauthorized, rr.Code, "wrong current password must be rejected (attempt %d)", i+1)
}
// Locked out: the correct current password is rejected too. FIX 4: uniform
// 401 (never distinguishable from a wrong password), distinct body text.
// FIX 1: the correct password now clears the lockout and succeeds.
req := changePasswordRequest(t, ctx, userID, "testpassword123", "newpassword456")
rr := httptest.NewRecorder()
ChangePasswordHandler(rr, req)
require.Equal(t, http.StatusUnauthorized, rr.Code, "change-password must be rejected with a lockout after 5 wrong current passwords")
require.Contains(t, rr.Body.String(), "too many failed attempts", "the locked body must stay distinct for the UI")
require.Equal(t, http.StatusOK, rr.Code, "a locked-out user with the correct current password must be able to recover (FIX 1)")
// The correct password works again once the lockout is cleared.
_, err = tx.Exec(ctx, `UPDATE users SET failed_attempts = 0, locked_until = NULL WHERE id = $1`, userID)
require.NoError(t, err)
req = changePasswordRequest(t, ctx, userID, "testpassword123", "newpassword456")
rr = httptest.NewRecorder()
ChangePasswordHandler(rr, req)
require.Equal(t, http.StatusOK, rr.Code, "correct current password must succeed after the lockout is reset")
// The lockout was cleared on success.
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, "failed_attempts must be reset to 0 after a successful recovery")
require.Nil(t, lockedUntil, "locked_until must be NULL after a successful recovery")
}
// ============================================================================
@@ -302,44 +297,6 @@ func TestDeleteAccount_ConcurrentWrongPassword_NoLostUpdates(t *testing.T) {
// FIX 5 — passwordless (NULL password_hash) accounts
// ============================================================================
// TestDeleteAccount_Passwordless_Requires2FAUnconditionally verifies FIX 5a: a
// NULL-password-hash (social-only) account has no current password to
// re-verify, so deleting it requires the 2FA code gate UNCONDITIONALLY — even
// when 2FA is not otherwise enforced — so a session holder cannot erase a
// passwordless account with zero credential proof.
func TestDeleteAccount_Passwordless_Requires2FAUnconditionally(t *testing.T) {
ctx, tx := testutils.SetupTestTx(t)
userID, err := fixtures.CreateTestUser(tx)
require.NoError(t, err)
_, err = tx.Exec(ctx, `UPDATE users SET password_hash = NULL WHERE id = $1`, userID)
require.NoError(t, err)
// No code → rejected with the exact message the frontend uses to reveal the
// 2FA step (deleteRevealTwoFactor).
req := deleteAccountRequest(t, ctx, userID, "", "")
rr := httptest.NewRecorder()
DeleteAccountHandler(rr, req)
require.Equal(t, http.StatusBadRequest, rr.Code, rr.Body.String())
require.Contains(t, rr.Body.String(), "a two-factor verification code is required to delete the account")
// A wrong code is rejected too (the account survives).
seedPendingTwoFA(t, ctx, tx, userID, "424242")
req = deleteAccountRequest(t, ctx, userID, "", "000000")
rr = httptest.NewRecorder()
DeleteAccountHandler(rr, req)
require.Equal(t, http.StatusBadRequest, rr.Code, rr.Body.String())
var firstName string
require.NoError(t, tx.QueryRow(ctx, `SELECT n_first_name FROM users WHERE id = $1`, userID).Scan(&firstName))
require.Equal(t, "Test", firstName, "the account must survive a rejected code")
// A correct fresh code is the sole credential — it deletes the account.
req = deleteAccountRequest(t, ctx, userID, "", "424242")
rr = httptest.NewRecorder()
DeleteAccountHandler(rr, req)
require.Equal(t, http.StatusNoContent, rr.Code, rr.Body.String())
}
// TestPasswordChange_NullHash_NoPasswordToChange verifies FIX 5b: changing the
// password on a passwordless (NULL hash) account is a clear 400 with an
// actionable message — not the old 500 from scanning NULL into a plain string.
@@ -396,3 +353,183 @@ func TestDeleteAccount_DavCardDeletedInErasureTx(t *testing.T) {
require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM dav_cards WHERE uri = $1`, uri).Scan(&countAfter))
require.Zero(t, countAfter, "the dav_cards row must be deleted inside the erasure transaction")
}
// ============================================================================
// FIX 1 (round-9) — current-password clears login lockout on success
// ============================================================================
// TestPasswordChange_LockedOutUserCanRecover verifies FIX 1: a user with
// locked_until set (locked out by LOGIN attacks) can still change their
// password by providing the CORRECT current password. The handler runs the
// bcrypt compare even when locked out, and on success clears the shared
// lockout (failed_attempts = 0, locked_until = NULL).
func TestPasswordChange_LockedOutUserCanRecover(t *testing.T) {
ctx, tx := testutils.SetupTestTx(t)
userID, err := fixtures.CreateTestUser(tx)
require.NoError(t, err)
// Simulate a login lockout: set locked_until in the future.
future := clock.Now().Add(30 * time.Minute)
_, err = tx.Exec(ctx, `UPDATE users SET failed_attempts = 5, locked_until = $2 WHERE id = $1`, userID, future)
require.NoError(t, err)
// The user is locked out but provides the CORRECT current password.
req := changePasswordRequest(t, ctx, userID, "testpassword123", "newpassword456")
rr := httptest.NewRecorder()
ChangePasswordHandler(rr, req)
require.Equal(t, http.StatusOK, rr.Code, "a locked-out user with the correct current password must be able to change their password")
// The lockout was cleared on success.
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, "failed_attempts must be reset to 0 after a successful password change")
require.Nil(t, lockedUntil, "locked_until must be NULL after a successful password change")
}
// TestPasswordChange_LockedOutUserWrongPassword verifies FIX 1: a locked-out
// user who provides a WRONG current password is rejected without incrementing
// the counter further (the lockout stands).
func TestPasswordChange_LockedOutUserWrongPassword(t *testing.T) {
ctx, tx := testutils.SetupTestTx(t)
userID, err := fixtures.CreateTestUser(tx)
require.NoError(t, err)
future := clock.Now().Add(30 * time.Minute)
_, err = tx.Exec(ctx, `UPDATE users SET failed_attempts = 5, locked_until = $2 WHERE id = $1`, userID, future)
require.NoError(t, err)
req := changePasswordRequest(t, ctx, userID, "wrong-password", "newpassword456")
rr := httptest.NewRecorder()
ChangePasswordHandler(rr, req)
require.Equal(t, http.StatusUnauthorized, rr.Code, "a locked-out user with a wrong password must be rejected")
require.Contains(t, rr.Body.String(), "too many failed attempts")
// The counter was NOT incremented (still 5).
var failedAttempts int
require.NoError(t, tx.QueryRow(ctx, `SELECT failed_attempts FROM users WHERE id = $1`, userID).Scan(&failedAttempts))
require.Equal(t, 5, failedAttempts, "failed_attempts must NOT be incremented when already locked out")
}
// TestDeleteAccount_LockedOutUserCanRecover verifies FIX 1: a user with
// locked_until set (locked out by LOGIN attacks) can still delete their
// account by providing the CORRECT current password. The lockout is cleared
// on success.
func TestDeleteAccount_LockedOutUserCanRecover(t *testing.T) {
savedClient := s3.Client
s3.Client = nil
t.Cleanup(func() { s3.Client = savedClient })
ctx, tx := testutils.SetupTestTx(t)
userID, err := fixtures.CreateTestUser(tx)
require.NoError(t, err)
// Simulate a login lockout.
future := clock.Now().Add(30 * time.Minute)
_, err = tx.Exec(ctx, `UPDATE users SET failed_attempts = 5, locked_until = $2 WHERE id = $1`, userID, future)
require.NoError(t, err)
// The user is locked out but provides the CORRECT current password.
req := deleteAccountRequest(t, ctx, userID, "testpassword123", "")
rr := httptest.NewRecorder()
DeleteAccountHandler(rr, req)
require.Equal(t, http.StatusNoContent, rr.Code, "a locked-out user with the correct current password must be able to delete their account")
// The lockout was cleared on success.
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, "failed_attempts must be reset to 0 after a successful delete")
require.Nil(t, lockedUntil, "locked_until must be NULL after a successful delete")
}
// TestDeleteAccount_LockedOutUserWrongPassword verifies FIX 1: a locked-out
// user who provides a WRONG current password is rejected without incrementing
// the counter further.
func TestDeleteAccount_LockedOutUserWrongPassword(t *testing.T) {
savedClient := s3.Client
s3.Client = nil
t.Cleanup(func() { s3.Client = savedClient })
ctx, tx := testutils.SetupTestTx(t)
userID, err := fixtures.CreateTestUser(tx)
require.NoError(t, err)
future := clock.Now().Add(30 * time.Minute)
_, err = tx.Exec(ctx, `UPDATE users SET failed_attempts = 5, locked_until = $2 WHERE id = $1`, userID, future)
require.NoError(t, err)
req := deleteAccountRequest(t, ctx, userID, "wrong-password", "")
rr := httptest.NewRecorder()
DeleteAccountHandler(rr, req)
require.Equal(t, http.StatusUnauthorized, rr.Code, "a locked-out user with a wrong password must be rejected")
require.Contains(t, rr.Body.String(), "too many failed attempts")
// The counter was NOT incremented (still 5).
var failedAttempts int
require.NoError(t, tx.QueryRow(ctx, `SELECT failed_attempts FROM users WHERE id = $1`, userID).Scan(&failedAttempts))
require.Equal(t, 5, failedAttempts, "failed_attempts must NOT be incremented when already locked out")
}
// ============================================================================
// FIX 2 (round-9) — passwordless delete-account 2FA condition
// ============================================================================
// TestDeleteAccount_Passwordless_Requires2FAInEnforcedEnv verifies FIX 2: a
// NULL-password-hash (social-only) account requires a 2FA code ONLY when 2FA
// enforcement is active. In enforced env, the code gate protects against a
// stolen session token erasing the account with zero credential proof.
func TestDeleteAccount_Passwordless_Requires2FAInEnforcedEnv(t *testing.T) {
twofaEnvEnforced(t)
savedClient := s3.Client
s3.Client = nil
t.Cleanup(func() { s3.Client = savedClient })
ctx, tx := testutils.SetupTestTx(t)
userID, err := fixtures.CreateTestUser(tx)
require.NoError(t, err)
_, err = tx.Exec(ctx, `UPDATE users SET password_hash = NULL WHERE id = $1`, userID)
require.NoError(t, err)
// No code → rejected with the 2FA-required message.
req := deleteAccountRequest(t, ctx, userID, "", "")
rr := httptest.NewRecorder()
DeleteAccountHandler(rr, req)
require.Equal(t, http.StatusBadRequest, rr.Code, rr.Body.String())
require.Contains(t, rr.Body.String(), "a two-factor verification code is required to delete the account")
// A correct fresh code deletes the account.
seedPendingTwoFA(t, ctx, tx, userID, "424242")
req = deleteAccountRequest(t, ctx, userID, "", "424242")
rr = httptest.NewRecorder()
DeleteAccountHandler(rr, req)
require.Equal(t, http.StatusNoContent, rr.Code, rr.Body.String())
}
// TestDeleteAccount_Passwordless_No2FARequiredInUnenforcedEnv verifies FIX 2:
// in an unenforced environment (dev/test), a passwordless account can delete
// without a 2FA code — the code cannot be minted (no delivery channel), and
// the passwordless property plus the authenticated session are the protection.
func TestDeleteAccount_Passwordless_No2FARequiredInUnenforcedEnv(t *testing.T) {
twofaEnvUnenforced(t)
savedClient := s3.Client
s3.Client = nil
t.Cleanup(func() { s3.Client = savedClient })
ctx, tx := testutils.SetupTestTx(t)
userID, err := fixtures.CreateTestUser(tx)
require.NoError(t, err)
_, err = tx.Exec(ctx, `UPDATE users SET password_hash = NULL WHERE id = $1`, userID)
require.NoError(t, err)
// No code required — the delete succeeds with just the authenticated session.
req := deleteAccountRequest(t, ctx, userID, "", "")
rr := httptest.NewRecorder()
DeleteAccountHandler(rr, req)
require.Equal(t, http.StatusNoContent, rr.Code, rr.Body.String())
// The account was anonymized (anonymize_user() sets name to 'Deleted').
var firstName string
require.NoError(t, tx.QueryRow(ctx, `SELECT n_first_name FROM users WHERE id = $1`, userID).Scan(&firstName))
require.Equal(t, "Deleted", firstName, "the passwordless account must be anonymized")
}
backend/handlers/user/profile.go
+20 -7
View File
@@ -725,18 +725,29 @@ func ChangePasswordHandler(w http.ResponseWriter, r *http.Request) {
// users.failed_attempts/locked_until). Without it a stolen session token
// would let an attacker brute-force the current password with unlimited
// guesses.
//
// FIX 1 (round-9): a user locked out by LOGIN attacks (shared
// failed_attempts/locked_until columns) can still recover by providing the
// CORRECT current password here — the lockout is cleared on success. When
// the user is locked out we still run the bcrypt compare (one attempt), and
// if the password is correct the lockout is lifted. If the password is wrong
// while locked out, no additional failure is recorded (the lockout stands).
lockedOut := false
if err := checkCurrentPasswordLockout(r.Context(), userID); err != nil {
if errors.Is(err, errCurrentPasswordLockedOut) {
// FIX 4: uniform 401 — locked-vs-wrong is never distinguishable;
// the body text still tells the UI which one happened.
lockedOut = true
} else {
log.Printf("Failed to check current-password lockout for user %s: %v", userID, err)
http.Error(w, "server error", http.StatusInternalServerError)
return
}
}
if err := bcrypt.CompareHashAndPassword([]byte(passwordHash.String), []byte(req.CurrentPassword)); err != nil {
if lockedOut {
// FIX 1: already locked out — don't increment further, just reject.
http.Error(w, "too many failed attempts — try again later", http.StatusUnauthorized)
return
}
log.Printf("Failed to check current-password lockout for user %s: %v", userID, err)
http.Error(w, "server error", http.StatusInternalServerError)
return
}
if err := bcrypt.CompareHashAndPassword([]byte(passwordHash.String), []byte(req.CurrentPassword)); err != nil {
// FIX 3: one atomic UPDATE ... RETURNING (increment + escalation) —
// concurrent wrong-password requests cannot race a check-then-increment.
newCount, _, recordErr := recordCurrentPasswordFailure(r.Context(), userID)
@@ -750,6 +761,8 @@ func ChangePasswordHandler(w http.ResponseWriter, r *http.Request) {
http.Error(w, "current password is incorrect", http.StatusUnauthorized)
return
}
// FIX 1: a correct current password clears the shared lockout, so a
// login-locked-out user can self-recover by changing their password.
resetCurrentPasswordFailures(r.Context(), userID)
newHash, err := bcrypt.GenerateFromPassword([]byte(req.NewPassword), bcrypt.DefaultCost)