fix: account security + GDPR erasure — current-password lockout budgets (atomic 15/30/60 escalation, uniform 401), DAV + S3 deletion durable in-tx, batch erasure outbox

- ChangePassword/DeleteAccount: failed-attempt lockout matching login escalation, atomic check-increment (no burst), uniform 401 with distinct bodies, passwordless accounts require 2FA unconditionally to delete, NULL-password change-password clear error - erasure: CardDAV dav_cards rows deleted inside the erasure transaction (was fire-and-forget goroutine); S3 profile-pic deletion via pending_s3_deletions outbox + retry job; stale-guest/idle-account batch paths write the outbox in-tx and skip the guessed-bucket fallback - S3_PROFILE_PICS_BUCKET unset -> fail-closed warning (once per process) - scheduler test: 27 jobs (retry-s3-deletions) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai> Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
2026-08-22 00:34:51 +01:00
parent 1d6d3e2f8d
commit 62dca184df
6 changed files with 748 additions and 101 deletions
@@ -13,13 +13,14 @@ import (
 	"net/http"
 	"net/url"
 	"os"
+	"sync"
 	"time"

 	"crussell/auth"
+	"crussell/clock"
 	"crussell/db"
 	"crussell/handlers/payments"
 	"crussell/internal/adminnotify"
-	"crussell/internal/dav"
 	"crussell/internal/s3"
 	"crussell/internal/square"
 	"crussell/internal/twofa"
@@ -216,6 +217,94 @@ func clearSquareErasureOutboxRows(ctx context.Context, rowIDs []string) {
 	}
 }

+// --- Current-password re-verification lockout (FIX 2 / FIX 3) ---
+//
+// DeleteAccountHandler and ChangePasswordHandler both re-verify the current
+// password. Without a failed-attempt budget, a stolen session token lets an
+// attacker brute-force that password with unlimited guesses (the 2FA code gate
+// only applies when 2FA is enforced AND enabled). The budget below reuses the
+// SAME users.failed_attempts / users.locked_until columns the login path uses
+// (handlers/auth/local.go), so every current-password check shares one
+// counter: after 5 consecutive failures the account locks for 15 minutes
+// (escalating to 30 at 7+ and 60 at 10+, matching login), and a correct
+// password resets it. clock.Now() keeps the Go-side lock check on the same UTC
+// clock as the DB NOW() stamp that writes locked_until.
+
+// errCurrentPasswordLockedOut is returned by checkCurrentPasswordLockout while
+// the user's current-password budget is inside a lockout window.
+var errCurrentPasswordLockedOut = errors.New("current-password attempts exhausted; account locked")
+
+// checkCurrentPasswordLockout returns errCurrentPasswordLockedOut when the
+// user's shared failed-attempt budget is locked, nil otherwise (or the raw DB
+// error). Must run BEFORE the bcrypt compare so a locked account is rejected
+// without paying the bcrypt cost.
+func checkCurrentPasswordLockout(ctx context.Context, userID string) error {
+	var lockedUntil *time.Time
+	if err := db.Conn.QueryRow(ctx,
+		`SELECT locked_until FROM users WHERE id = $1`, userID).Scan(&lockedUntil); err != nil {
+		return err
+	}
+	if lockedUntil != nil && lockedUntil.After(clock.Now()) {
+		return errCurrentPasswordLockedOut
+	}
+	return nil
+}
+
+// recordCurrentPasswordFailure atomically records a wrong current password on
+// the shared failed-attempt budget and returns the NEW failed_attempts count
+// plus the resulting locked_until. FIX 3: the increment AND the lockout
+// escalation are a SINGLE atomic UPDATE ... RETURNING (the exact statement the
+// login path uses), so N concurrent wrong-password requests can never race a
+// check-then-increment — every failure is counted (no lost updates) and the
+// escalation decision is computed from the returned count. Mirrors the login
+// path's progressive lockout exactly (5 failures → 15 minutes, 7 → 30, 10 →
+// 60); the lock extends on every failure past the threshold.
+func recordCurrentPasswordFailure(ctx context.Context, userID string) (int, *time.Time, error) {
+	var newCount int
+	var newLockedUntil *time.Time
+	err := db.Conn.QueryRow(ctx, `
+		UPDATE users
+		SET failed_attempts = failed_attempts + 1,
+		    locked_until = CASE
+		        WHEN failed_attempts + 1 >= 5 THEN NOW() + (CASE
+		            WHEN failed_attempts + 1 >= 10 THEN INTERVAL '60 minutes'
+		            WHEN failed_attempts + 1 >= 7 THEN INTERVAL '30 minutes'
+		            ELSE INTERVAL '15 minutes'
+		        END)
+		        ELSE locked_until
+		    END
+		WHERE id = $1
+		RETURNING failed_attempts, locked_until
+	`, userID).Scan(&newCount, &newLockedUntil)
+	if err != nil {
+		return 0, nil, err
+	}
+	return newCount, newLockedUntil, nil
+}
+
+// resetCurrentPasswordFailures clears the shared failed-attempt budget after a
+// correct current password. Best-effort: a failed reset only leaves the stale
+// counter in place, and the next correct password clears it again.
+func resetCurrentPasswordFailures(ctx context.Context, userID string) {
+	if _, err := db.Conn.Exec(ctx,
+		`UPDATE users SET failed_attempts = 0, locked_until = NULL WHERE id = $1`, userID); err != nil {
+		slog.Error("failed to reset current-password lockout state", "user_id", userID, "error", err)
+	}
+}
+
+// s3BucketUnsetWarningOnce throttles the missing-S3_PROFILE_PICS_BUCKET CRITICAL
+// log to one line per process (FIX 2): the deletion-time skip is fail-closed,
+// and the single loud warning makes the misconfiguration impossible to miss.
+// A proper startup check for S3_PROFILE_PICS_BUCKET in main.go is tracked for
+// a later round (another agent owns main.go).
+var s3BucketUnsetWarningOnce sync.Once
+
+func warnS3ProfilePicsBucketUnset() {
+	s3BucketUnsetWarningOnce.Do(func() {
+		log.Printf("CRITICAL: S3_PROFILE_PICS_BUCKET is not set — profile-picture deletion skipped fail-closed; profile-pic objects may remain until the bucket is configured (main.go startup check pending in a later round)")
+	})
+}
+
 // DeleteAccountRequest carries the re-verification credentials the handler now
 // requires before erasing an account (finding 3): the current password (always)
 // and, in enforced environments for a user with 2FA enabled, a fresh one-time
@@ -262,13 +351,45 @@ func DeleteAccountHandler(w http.ResponseWriter, r *http.Request) {
 		http.Error(w, "invalid request", http.StatusBadRequest)
 		return
 	}
-	if passwordHash.Valid && passwordHash.String != "" {
+	// FIX 5: passwordless (social-only, NULL password_hash) accounts have no
+	// current password to re-verify — a stolen session token would otherwise
+	// erase the account with zero credential proof. Treat them as needing the
+	// 2FA gate (see below).
+	hasPassword := passwordHash.Valid && passwordHash.String != ""
+	if hasPassword {
+		// 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.
+		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.
+				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.
+			newCount, _, recordErr := recordCurrentPasswordFailure(ctx, userID)
+			if recordErr != nil {
+				log.Printf("Failed to record current-password failure for user %s: %v", userID, recordErr)
+			}
+			if newCount >= 5 {
+				http.Error(w, "too many failed attempts — try again later", http.StatusUnauthorized)
+				return
+			}
 			http.Error(w, "current password is incorrect", http.StatusUnauthorized)
 			return
 		}
+		resetCurrentPasswordFailures(ctx, userID)
 	}
-	if twoFARequired() && twoFactorEnabled {
+	if !hasPassword || (twoFARequired() && twoFactorEnabled) {
 		if req.VerificationCode == "" {
 			http.Error(w, "a two-factor verification code is required to delete the account", http.StatusBadRequest)
 			return
@@ -295,28 +416,30 @@ func DeleteAccountHandler(w http.ResponseWriter, r *http.Request) {

 	// --- External system scrubbing (BEFORE SQL anonymize) ---

-	// Delete profile picture from S3/R2
-	if profilePicURL.Valid && profilePicURL.String != "" && s3.Client != nil {
-		// #nosec G118 — intentional background goroutine for async profile pic cleanup
-		go func(picURL string) {
-			defer func() {
-				if r := recover(); r != nil {
-					log.Printf("Panic recovered in S3 profile picture deletion: %v", r)
-				}
-			}()
-			ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
-			defer cancel()
-			bucket := os.Getenv("S3_PROFILE_PICS_BUCKET")
-			if bucket == "" {
-				bucket = "crussell-profile-pics"
-			}
-			// profiles/{userID}.jpg — matches UploadProfilePictureHandler key format
-			key := fmt.Sprintf("profiles/%s.jpg", userID)
-			if err := s3.Client.Delete(ctx, bucket, key); err != nil {
-				log.Printf("Warning: Failed to delete profile picture for user %s: %v", userID, err)
-			}
-		}(profilePicURL.String)
+	// Durable S3/R2 profile-picture deletion (Fault A2 / FIX 1): instead of the
+	// old fire-and-forget goroutine, the deletion target is persisted as a
+	// pending_s3_deletions outbox row INSIDE the anonymization transaction
+	// below (before it commits), and an async goroutine drains it after the
+	// commit — mirroring the Square erasure outbox above. If the process dies
+	// between the local commit and the goroutine finishing, the
+	// retry-s3-deletions job (internal/jobs/cleanup.go) still finds the row and
+	// completes the object deletion, so the photo PII is never retained
+	// indefinitely.
+	profilePicS3Delete := profilePicURL.Valid && profilePicURL.String != "" && s3.Client != nil
+	// FIX 4 (fail-closed): never guess the dev bucket "crussell-profile-pics"
+	// when S3_PROFILE_PICS_BUCKET is unset — deleting from a guessed bucket
+	// would silently erase an unrelated deployment's objects. Skip the deletion
+	// and warn: the object may remain until the operator configures the bucket.
+	profilePicBucket := os.Getenv("S3_PROFILE_PICS_BUCKET")
+	if profilePicS3Delete && profilePicBucket == "" {
+		// FIX 2: one loud CRITICAL per process — a silent Warning can scroll by
+		// unnoticed in a busy log stream.
+		warnS3ProfilePicsBucketUnset()
+		profilePicS3Delete = false
 	}
+	// profiles/{userID}.jpg — matches UploadProfilePictureHandler key format
+	profilePicKey := fmt.Sprintf("profiles/%s.jpg", userID)
+	var profilePicDeletionID string

 	// Snapshot the Square card AND customer references per saved-card row
 	// synchronously BEFORE the SQL anonymization below NULLs
@@ -404,6 +527,36 @@ func DeleteAccountHandler(w http.ResponseWriter, r *http.Request) {
 			}
 		}

+		// FIX 1a: the CardDAV vCard (full name/email/phone/DOB/photo URL PII —
+		// dav_cards, keyed by uri "{userID}.vcf" under addressbook 1, the same
+		// Postgres) is deleted INSIDE the erasure transaction, atomically with
+		// the local erasure. No outbox or retry job is needed: a failed tx
+		// rolls both back together. Replaces the old fire-and-forget
+		// dav.Service.DeleteContact goroutine that could only log on failure,
+		// permanently stranding the contact PII.
+		if _, err := tx.Exec(ctx, `
+			DELETE FROM dav_cards WHERE addressbookid = 1 AND uri = $1
+		`, fmt.Sprintf("%s.vcf", userID)); err != nil {
+			log.Printf("Failed to delete CardDAV contact for guest %s: %v", userID, err)
+			http.Error(w, "server error", http.StatusInternalServerError)
+			return
+		}
+
+		// A2 durable outbox: persist the S3/R2 profile-picture deletion BEFORE
+		// this tx commits so the retry-s3-deletions job can complete it after
+		// a crash (the async goroutine below is the primary drain).
+		if profilePicS3Delete {
+			if err := tx.QueryRow(ctx, `
+				INSERT INTO pending_s3_deletions (user_id, bucket, object_key)
+				VALUES ($1, $2, $3)
+				RETURNING id
+			`, userID, profilePicBucket, profilePicKey).Scan(&profilePicDeletionID); err != nil {
+				log.Printf("Failed to persist S3 profile-pic deletion outbox for guest %s: %v", userID, err)
+				http.Error(w, "server error", http.StatusInternalServerError)
+				return
+			}
+		}
+
 		if err := tx.Commit(ctx); err != nil {
 			log.Printf("Failed to commit transaction for guest user deletion: %v", err)
 			http.Error(w, "server error", http.StatusInternalServerError)
@@ -446,6 +599,31 @@ func DeleteAccountHandler(w http.ResponseWriter, r *http.Request) {
 			}
 		}

+		// FIX 1a: delete the CardDAV vCard row inside the erasure transaction
+		// (atomic with the anonymization — see the guest path above).
+		if _, err := tx.Exec(ctx, `
+			DELETE FROM dav_cards WHERE addressbookid = 1 AND uri = $1
+		`, fmt.Sprintf("%s.vcf", userID)); err != nil {
+			log.Printf("Failed to delete CardDAV contact for user %s: %v", userID, err)
+			http.Error(w, "server error", http.StatusInternalServerError)
+			return
+		}
+
+		// A2 durable outbox: persist the S3/R2 profile-picture deletion BEFORE
+		// this tx commits so the retry-s3-deletions job can complete it after
+		// a crash (the async goroutine below is the primary drain).
+		if profilePicS3Delete {
+			if err := tx.QueryRow(ctx, `
+				INSERT INTO pending_s3_deletions (user_id, bucket, object_key)
+				VALUES ($1, $2, $3)
+				RETURNING id
+			`, userID, profilePicBucket, profilePicKey).Scan(&profilePicDeletionID); err != nil {
+				log.Printf("Failed to persist S3 profile-pic deletion outbox for user %s: %v", userID, err)
+				http.Error(w, "server error", http.StatusInternalServerError)
+				return
+			}
+		}
+
 		if err := tx.Commit(ctx); err != nil {
 			log.Printf("Failed to commit transaction for user anonymization: %v", err)
 			http.Error(w, "server error", http.StatusInternalServerError)
@@ -569,20 +747,44 @@ func DeleteAccountHandler(w http.ResponseWriter, r *http.Request) {
 		}(sqClient, erasureTargets, notifyCtx)
 	}

-	// Delete CardDAV contact (non-blocking, best-effort)
-	if dav.Service != nil {
-		go func() {
+	// Durable S3/R2 profile-picture deletion fires only after local
+	// anonymization/deletion commits, mirroring the Square goroutine above: the
+	// outbox row persisted in the erasure transaction survives a crash, this
+	// goroutine is the primary drain, and the retry-s3-deletions job
+	// (internal/jobs/cleanup.go) is the safety net.
+	if profilePicS3Delete {
+		// Capture the client synchronously so the async goroutine never reads
+		// the global s3.Client (which tests swap per-account).
+		picClient := s3.Client
+		// #nosec G118 — intentional background goroutine for async profile pic cleanup
+		go func(client s3.Uploader, bucket, key, outboxID string) {
 			defer func() {
 				if r := recover(); r != nil {
-					log.Printf("Panic recovered in CardDAV contact deletion: %v", r)
+					log.Printf("Panic recovered in S3 profile picture deletion: %v", r)
 				}
 			}()
-			uri := fmt.Sprintf("%s.vcf", userID)
-			if err := dav.Service.DeleteContact(1, uri); err != nil {
-				log.Printf("Warning: Failed to delete CardDAV contact for user %s: %v", userID, err)
+			ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+			defer cancel()
+			if err := client.Delete(ctx, bucket, key); err != nil {
+				// The outbox row is deliberately left in place for the
+				// retry-s3-deletions job, so the photo PII is not retained
+				// indefinitely.
+				log.Printf("Warning: Failed to delete profile picture for user %s: %v", userID, err)
+				return
 			}
-		}()
+			// Drain the durable outbox so the safety-net job has nothing to
+			// retry. Best-effort: a failed drain leaves the row for the job.
+			if _, err := db.Conn.Exec(ctx, `DELETE FROM pending_s3_deletions WHERE id = $1`, outboxID); err != nil {
+				slog.Error("failed to drain S3 profile-pic deletion outbox", "user_id", userID, "outbox_id", outboxID, "error", err)
+			}
+		}(picClient, profilePicBucket, profilePicKey, profilePicDeletionID)
 	}

+	// FIX 1a: the CardDAV vCard is deleted INSIDE the erasure transaction above
+	// (DELETE FROM dav_cards WHERE addressbookid = 1 AND uri = '{userID}.vcf'),
+	// atomically with the local erasure — the old fire-and-forget
+	// dav.Service.DeleteContact goroutine (which could only log on failure,
+	// permanently stranding the contact PII) is gone.
+
 	w.WriteHeader(http.StatusNoContent)
 }
@@ -701,7 +701,7 @@ func ChangePasswordHandler(w http.ResponseWriter, r *http.Request) {
 		return
 	}

-	var passwordHash string
+	var passwordHash sql.NullString
 	err := db.Conn.QueryRow(r.Context(), `SELECT password_hash FROM users WHERE id = $1`, userID).Scan(&passwordHash)
 	if err != nil {
 		if errors.Is(err, pgx.ErrNoRows) {
@@ -712,11 +712,45 @@ func ChangePasswordHandler(w http.ResponseWriter, r *http.Request) {
 		http.Error(w, "server error", http.StatusInternalServerError)
 		return
 	}
+	if !passwordHash.Valid || passwordHash.String == "" {
+		// FIX 5: a passwordless (social-only) account has no password to change.
+		// Scan NULL into a plain string used to 500; a clear 400 is actionable.
+		http.Error(w, "this account has no password to change", http.StatusBadRequest)
+		return
+	}

-	if err := bcrypt.CompareHashAndPassword([]byte(passwordHash), []byte(req.CurrentPassword)); err != nil {
+	// FIX 3: the current-password compare shares the same per-user
+	// failed-attempt/lockout budget as delete-account (checkCurrentPasswordLockout
+	// + recordCurrentPasswordFailure/resetCurrentPasswordFailures, backed by
+	// users.failed_attempts/locked_until). Without it a stolen session token
+	// would let an attacker brute-force the current password with unlimited
+	// guesses.
+	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.
+			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)
+		if recordErr != nil {
+			log.Printf("Failed to record current-password failure for user %s: %v", userID, recordErr)
+		}
+		if newCount >= 5 {
+			http.Error(w, "too many failed attempts — try again later", http.StatusUnauthorized)
+			return
+		}
 		http.Error(w, "current password is incorrect", http.StatusUnauthorized)
 		return
 	}
+	resetCurrentPasswordFailures(r.Context(), userID)

 	newHash, err := bcrypt.GenerateFromPassword([]byte(req.NewPassword), bcrypt.DefaultCost)
 	if err != nil {