fix(security): P2-4 — sliding-window login rate limit + generic lockout error (#53)

* fix(security): sliding-window login rate limit + generic lockout error

Two related changes for the per-account brute-force defense (2026-05
audit P2-4).

Previously the failed-attempt counter incremented monotonically until a
successful login, with a hard lockout at 5 attempts → 15 minutes. That's
solid against single-IP brute force (the global IP rate limiter caps a
single source at 50 req/sec, so an attacker reaches 5 failures within a
fraction of a second and gets the 15-minute lockout). But:

1. UX is bad — a legitimate user who mistypes their password twice today
   and twice next week is locked out without the lockout window ever
   resetting.

2. The hard error message "account is temporarily locked due to too many
   failed attempts" was an enumeration oracle: a network observer running
   email→password sprays could distinguish "this email is locked" (which
   implies it exists) from "this email doesn't exist."

Fix:

- New `last_failed_attempt DATETIME` column on users via golang-migrate
  migration 000001. Tracks the timestamp of the most recent failure.
- RecordLoginAttempt now reads previous state under the existing DB mu,
  computes a NEW count based on whether the previous failure was inside
  the 5-minute sliding window (reset to 1 if outside), and applies a
  tiered cooldown:
    count 3 within window → 1 minute cooldown
    count 4 within window → 5 minute cooldown
    count 5+              → 15 minute hard lockout (existing)
- Login() now returns the SAME generic "invalid credentials" error for
  the locked-account path as for wrong-password and missing-user. The
  lockout is still ENFORCED — the attacker just doesn't learn it from
  the response. RecordLoginAttempt is still called on locked-account
  hits so probes against a locked account count against its window and
  can't be used as a rate-free oracle.

The new column is intentionally NOT surfaced to the User model or any
API response — lockout state stays server-side.

Tests cover:
- window-reset (>5min ago → count restarts at 1, not increments)
- tiered cooldown (count=3 sets locked_until ~60s, count=4 ~5m, count=5+ ~15m)
- locked-account login returns generic "invalid credentials", never
  mentions "lock" / "locked" / "attempt"

P2-4 from the 2026-05 security audit.

* fix(security): close P2-4 follow-ups from Copilot review

Three real issues plus test hygiene:

1. RecordLoginAttempt no longer clears a still-active lockout when the
   5-min sliding window resets the count. Previously, waiting 6 min
   mid-15-min-lockout and failing once more would set newCount=1 and
   write locked_until=NULL, releasing the lock 9 min early. We now
   SELECT the existing locked_until and keep max(existing, newly
   computed). Regression test added.

2. Down migration is now a hard failure instead of a comments-only
   no-op. Running it as a no-op would silently decrement the
   golang-migrate schema version while last_failed_attempt remains —
   confusing/unsafe rollback state.

3. Constant-time Login: always run CheckPassword exactly once, even on
   missing-user / locked-account paths, against a precomputed dummy
   hash when no real user exists. Without this, "user missing" and
   "account locked" returned in microseconds while "exists + wrong
   password" did the full ~100ms bcrypt — a timing oracle that
   defeated the generic error message.

4. Tests now assert errors from HashPassword / GetUserByEmail /
   RecordLoginAttempt setup so failures point to the root cause.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: sarg3nt

gearbox/internal/framework/auth/auth.go

@@ -28,6 +28,22 @@ const (
 	LockoutDuration    = 15 * time.Minute
 )
 
+// dummyPasswordHash is a bcrypt hash used to keep the Login() codepath
+// taking constant bcrypt time even when the user does not exist. Without
+// it, "user missing" returns in microseconds while "user exists + wrong
+// password" runs a ~100ms bcrypt compare — an obvious timing oracle for
+// account enumeration. Generated once at package init; the underlying
+// plaintext is never used. See 2026-05 audit P2-4 follow-up.
+var dummyPasswordHash string
+
+func init() {
+	h, err := HashPassword("timing-equivalence-dummy-not-a-real-password")
+	if err != nil {
+		panic(fmt.Sprintf("auth init: failed to generate dummy password hash: %v", err))
+	}
+	dummyPasswordHash = h
+}
+
 // Manager handles authentication and session management.
 type Manager struct {
 	db           *database.DB
@@ -87,14 +103,37 @@ func (m *Manager) Login(w http.ResponseWriter, r *http.Request, email, password
 		return nil, fmt.Errorf("database error: %w", err)
 	}
 
+	// Run CheckPassword exactly once on every Login call regardless of
+	// user state. Without this, "user missing" and "account locked" return
+	// in microseconds while "exists + wrong password" runs a ~100ms bcrypt
+	// compare — a timing oracle that defeats the generic error message.
+	// See 2026-05 audit P2-4 follow-up.
+	passwordHash := dummyPasswordHash
+	if user != nil {
+		passwordHash = user.PasswordHash
+	}
+	passwordOK := CheckPassword(password, passwordHash)
+
 	if user == nil {
 		// Don't reveal that the user doesn't exist
 		return nil, fmt.Errorf("invalid credentials")
 	}
 
-	// Check if account is locked
+	// Check if account is locked. Return the SAME generic error as the
+	// "user not found" and "wrong password" paths so an attacker can't
+	// distinguish "this email is locked" (which implies it exists) from
+	// "this email doesn't exist" via the error message. The lockout itself
+	// is still enforced — the attacker just doesn't learn it from the
+	// response. See 2026-05 audit P2-4.
 	if user.IsLocked() {
-		return nil, fmt.Errorf("account is temporarily locked due to too many failed attempts")
+		// Record the attempt (counts against the window) so an attacker
+		// can't bypass the rate limit by repeatedly probing a locked
+		// account with no cost.
+		if err := m.db.RecordLoginAttempt(user.ID, false); err != nil {
+			m.logger.Error("failed to record locked-attempt", "error", err, "user_id", user.ID)
+		}
+		m.logAudit(r, &user.ID, models.AuditActionLoginFailed, "")
+		return nil, fmt.Errorf("invalid credentials")
 	}
 
 	// Check if account is active
@@ -105,8 +144,8 @@ func (m *Manager) Login(w http.ResponseWriter, r *http.Request, email, password
 		return nil, fmt.Errorf("account is disabled")
 	}
 
-	// Validate password
-	if !CheckPassword(password, user.PasswordHash) {
+	// Validate password (result computed above for constant-time path)
+	if !passwordOK {
 		// Record failed attempt
 		if err := m.db.RecordLoginAttempt(user.ID, false); err != nil {
 			m.logger.Error("failed to record login attempt", "error", err, "user_id", user.ID)

gearbox/internal/framework/auth/auth_test.go

@@ -4,6 +4,7 @@ import (
 	"log/slog"
 	"net/http/httptest"
 	"os"
+	"strings"
 	"testing"
 	"time"
 
@@ -465,3 +466,219 @@ func TestGenerateToken(t *testing.T) {
 		t.Errorf("Token seems too short: %d characters", len(token1))
 	}
 }
+
+// 2026-05 audit P2-4: failed-attempt count resets when the previous
+// failure was outside the sliding window.
+func TestRecordLoginAttempt_WindowReset(t *testing.T) {
+	manager, db, cleanup := setupTestManager(t)
+	defer cleanup()
+	_ = manager // we only need the DB; manager.db is the same instance
+
+	passwordHash, err := HashPassword("correct_password")
+	if err != nil {
+		t.Fatalf("HashPassword: %v", err)
+	}
+	if _, _, err := db.EnsureAdminExists(passwordHash, false); err != nil {
+		t.Fatalf("EnsureAdminExists: %v", err)
+	}
+	user, err := db.GetUserByEmail("admin")
+	if err != nil || user == nil {
+		t.Fatalf("GetUserByEmail: %v", err)
+	}
+
+	// 2 fresh failures: count = 2, no cooldown yet.
+	for i := 0; i < 2; i++ {
+		if err := db.RecordLoginAttempt(user.ID, false); err != nil {
+			t.Fatalf("RecordLoginAttempt[%d]: %v", i, err)
+		}
+	}
+	u, _ := db.GetUserByID(user.ID)
+	if u.FailedLoginAttempts != 2 {
+		t.Errorf("after 2 failures: FailedLoginAttempts = %d, want 2", u.FailedLoginAttempts)
+	}
+	if u.IsLocked() {
+		t.Errorf("after 2 failures: IsLocked = true; want false (below threshold)")
+	}
+
+	// Rewind last_failed_attempt to >5min ago via direct DB update — the
+	// next failure should reset count to 1, not increment to 3.
+	if _, err := db.GetDB().Exec(
+		`UPDATE users SET last_failed_attempt = datetime('now', '-10 minutes') WHERE id = ?`, user.ID,
+	); err != nil {
+		t.Fatalf("rewind last_failed_attempt: %v", err)
+	}
+
+	if err := db.RecordLoginAttempt(user.ID, false); err != nil {
+		t.Fatalf("RecordLoginAttempt after rewind: %v", err)
+	}
+	u, _ = db.GetUserByID(user.ID)
+	if u.FailedLoginAttempts != 1 {
+		t.Errorf("after window-reset: FailedLoginAttempts = %d, want 1 (count restarted)", u.FailedLoginAttempts)
+	}
+}
+
+// Cooldown tiers: count=3 → 1m cooldown, count=4 → 5m, count=5+ → 15m.
+func TestRecordLoginAttempt_TieredCooldown(t *testing.T) {
+	manager, db, cleanup := setupTestManager(t)
+	defer cleanup()
+	_ = manager
+
+	passwordHash, err := HashPassword("correct_password")
+	if err != nil {
+		t.Fatalf("HashPassword: %v", err)
+	}
+	if _, _, err := db.EnsureAdminExists(passwordHash, false); err != nil {
+		t.Fatalf("EnsureAdminExists: %v", err)
+	}
+	user, err := db.GetUserByEmail("admin")
+	if err != nil || user == nil {
+		t.Fatalf("GetUserByEmail: user=%v err=%v", user, err)
+	}
+
+	// 2 failures: still no cooldown.
+	for i := 0; i < 2; i++ {
+		if err := db.RecordLoginAttempt(user.ID, false); err != nil {
+			t.Fatalf("RecordLoginAttempt[%d]: %v", i, err)
+		}
+	}
+	u, _ := db.GetUserByID(user.ID)
+	if u.LockedUntil != nil {
+		t.Errorf("after 2 failures: LockedUntil set; want nil")
+	}
+
+	// 3rd: ~1 minute cooldown.
+	if err := db.RecordLoginAttempt(user.ID, false); err != nil {
+		t.Fatalf("RecordLoginAttempt (3rd): %v", err)
+	}
+	u, _ = db.GetUserByID(user.ID)
+	if u.LockedUntil == nil {
+		t.Fatalf("after 3 failures: LockedUntil nil; want ~1min")
+	}
+	if d := time.Until(*u.LockedUntil); d < 50*time.Second || d > 70*time.Second {
+		t.Errorf("after 3 failures: LockedUntil in %v, want ~60s", d)
+	}
+
+	// 4th: ~5 minute cooldown.
+	if err := db.RecordLoginAttempt(user.ID, false); err != nil {
+		t.Fatalf("RecordLoginAttempt (4th): %v", err)
+	}
+	u, _ = db.GetUserByID(user.ID)
+	if d := time.Until(*u.LockedUntil); d < 4*time.Minute+50*time.Second || d > 5*time.Minute+10*time.Second {
+		t.Errorf("after 4 failures: LockedUntil in %v, want ~5min", d)
+	}
+
+	// 5th: hard 15-minute lockout (existing behavior).
+	if err := db.RecordLoginAttempt(user.ID, false); err != nil {
+		t.Fatalf("RecordLoginAttempt (5th): %v", err)
+	}
+	u, _ = db.GetUserByID(user.ID)
+	if d := time.Until(*u.LockedUntil); d < 14*time.Minute+50*time.Second || d > 15*time.Minute+10*time.Second {
+		t.Errorf("after 5 failures: LockedUntil in %v, want ~15min", d)
+	}
+}
+
+// A still-active lockout MUST survive a sliding-window reset. The 15-min
+// hard lock outlives the 5-min failure window; an attacker could otherwise
+// wait 6 min mid-lockout, attempt one more failure, watch the count reset
+// to 1, and have locked_until cleared back to NULL — releasing the lock
+// 9 min early. Regression test for 2026-05 audit P2-4 follow-up.
+func TestRecordLoginAttempt_LockoutSurvivesWindowReset(t *testing.T) {
+	manager, db, cleanup := setupTestManager(t)
+	defer cleanup()
+	_ = manager
+
+	passwordHash, err := HashPassword("correct_password")
+	if err != nil {
+		t.Fatalf("HashPassword: %v", err)
+	}
+	if _, _, err := db.EnsureAdminExists(passwordHash, false); err != nil {
+		t.Fatalf("EnsureAdminExists: %v", err)
+	}
+	user, err := db.GetUserByEmail("admin")
+	if err != nil || user == nil {
+		t.Fatalf("GetUserByEmail: user=%v err=%v", user, err)
+	}
+
+	// Trigger the 15-min hard lockout.
+	for i := 0; i < 5; i++ {
+		if err := db.RecordLoginAttempt(user.ID, false); err != nil {
+			t.Fatalf("RecordLoginAttempt[%d]: %v", i, err)
+		}
+	}
+	u, _ := db.GetUserByID(user.ID)
+	if u.LockedUntil == nil {
+		t.Fatal("setup: expected lockout after 5 failures")
+	}
+	originalLock := *u.LockedUntil
+
+	// Rewind last_failed_attempt to >5 min ago — the failure window has
+	// expired, but the 15-min lockout has not.
+	if _, err := db.GetDB().Exec(
+		`UPDATE users SET last_failed_attempt = datetime('now', '-10 minutes') WHERE id = ?`,
+		user.ID,
+	); err != nil {
+		t.Fatalf("rewind last_failed_attempt: %v", err)
+	}
+
+	// One more failure: the count resets to 1 (window expired), but the
+	// existing future locked_until MUST be preserved.
+	if err := db.RecordLoginAttempt(user.ID, false); err != nil {
+		t.Fatalf("RecordLoginAttempt after rewind: %v", err)
+	}
+	u, _ = db.GetUserByID(user.ID)
+	if u.LockedUntil == nil {
+		t.Fatal("locked_until cleared by sliding-window reset; should still be active")
+	}
+	// The preserved lock should match (or be later than) the original.
+	if u.LockedUntil.Before(originalLock.Add(-time.Second)) {
+		t.Errorf("locked_until shortened from %v to %v", originalLock, *u.LockedUntil)
+	}
+}
+
+// The Login() flow returns the SAME generic error for a locked account as
+// for a wrong-password or missing-user — preventing email enumeration via
+// error-message variance.
+func TestManager_Login_LockedAccountReturnsGenericError(t *testing.T) {
+	manager, db, cleanup := setupTestManager(t)
+	defer cleanup()
+
+	passwordHash, err := HashPassword("correct_password")
+	if err != nil {
+		t.Fatalf("HashPassword: %v", err)
+	}
+	if _, _, err := db.EnsureAdminExists(passwordHash, false); err != nil {
+		t.Fatalf("EnsureAdminExists: %v", err)
+	}
+	user, err := db.GetUserByEmail("admin")
+	if err != nil || user == nil {
+		t.Fatalf("GetUserByEmail: user=%v err=%v", user, err)
+	}
+
+	// Lock the account by hitting the cooldown threshold.
+	for i := 0; i < 5; i++ {
+		if err := db.RecordLoginAttempt(user.ID, false); err != nil {
+			t.Fatalf("RecordLoginAttempt[%d]: %v", i, err)
+		}
+	}
+	u, _ := db.GetUserByID(user.ID)
+	if !u.IsLocked() {
+		t.Fatalf("expected account to be locked after 5 failures")
+	}
+
+	// Now try to log in — even with the CORRECT password, we should get
+	// the generic "invalid credentials" error, not "account locked".
+	req := httptest.NewRequest("GET", "/login", nil)
+	w := httptest.NewRecorder()
+	_, err = manager.Login(w, req, "admin", "correct_password")
+	if err == nil {
+		t.Fatal("expected error for locked account; got nil")
+	}
+	if !strings.Contains(err.Error(), "invalid credentials") {
+		t.Errorf("locked-account error = %q, want to contain 'invalid credentials' (generic). "+
+			"Leaking lockout state lets attackers enumerate valid emails.", err.Error())
+	}
+	if strings.Contains(strings.ToLower(err.Error()), "lock") {
+		t.Errorf("locked-account error mentions 'lock': %q. This is an enumeration aid; "+
+			"must be the same generic error as wrong-password.", err.Error())
+	}
+}

gearbox/internal/framework/database/migrations/files/000001_add_failed_attempt_window.down.sql

@@ -0,0 +1,13 @@
+-- INTENTIONAL FAILURE: this migration has no safe automatic rollback.
+--
+-- SQLite < 3.35 cannot DROP COLUMN at all. Newer SQLite versions can, but
+-- we don't gate on runtime version, and the standard fallback (rebuild
+-- the table) is risky for the users table mid-deploy. Letting this run
+-- as a no-op would decrement the golang-migrate schema version while
+-- last_failed_attempt remained — a confusing/unsafe rollback state that
+-- silently desyncs application code from the database.
+--
+-- If you genuinely need to roll this back, drop the column manually using
+-- the table-rebuild recipe (CREATE new, copy, drop, rename) and then
+-- update the schema_migrations table by hand.
+INSERT INTO _down_migration_000001_not_supported_run_manually VALUES (1);

gearbox/internal/framework/database/migrations/files/000001_add_failed_attempt_window.up.sql

@@ -0,0 +1,7 @@
+-- 2026-05 security audit P2-4: track per-account timestamp of the most
+-- recent failed login attempt so RecordLoginAttempt can implement a
+-- sliding window over failures (rather than the previous "increment
+-- forever until success" semantic). Used purely server-side; never
+-- exposed in API responses or UI — surfacing lockout state to the
+-- caller is itself an enumeration aid for attackers.
+ALTER TABLE users ADD COLUMN last_failed_attempt DATETIME;