From 6f3dda01cf4c99c225122d38f6f1f4d29eb5fed3 Mon Sep 17 00:00:00 2001 From: Devin Hadley <68879608+devinhadley@users.noreply.github.com> Date: Sun, 30 Aug 2026 17:53:55 -0700 Subject: [PATCH 1/2] fix(user): rate limit internal reauthentication attempts Password change and email reset requests re-verify the caller's current password but never rate limited or recorded those attempts, letting a hijacked session brute-force the account password. Add a shared reauthentication auth_action, enforce the same failed-attempt limit login uses, and route both flows through one verifyReauthentication helper so switching endpoints can't widen the guessing budget. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01VSYkKjv6rTX4ys77muo5jQ --- internal/db/models.go | 7 +- internal/handlers/user.go | 5 + internal/service/user/service.go | 81 +++++-- internal/service/user/user_test.go | 235 ++++++++++++++++++- sqlc/migrations/20260510222217_ratelimit.sql | 2 +- 5 files changed, 292 insertions(+), 38 deletions(-) diff --git a/internal/db/models.go b/internal/db/models.go index 6869351..8cff935 100644 --- a/internal/db/models.go +++ b/internal/db/models.go @@ -14,9 +14,10 @@ import ( type AuthAction string const ( - AuthActionLogin AuthAction = "login" - AuthActionPasswordReset AuthAction = "password_reset" - AuthActionEmailReset AuthAction = "email_reset" + AuthActionLogin AuthAction = "login" + AuthActionPasswordReset AuthAction = "password_reset" + AuthActionEmailReset AuthAction = "email_reset" + AuthActionReauthentication AuthAction = "reauthentication" ) func (e *AuthAction) Scan(src interface{}) error { diff --git a/internal/handlers/user.go b/internal/handlers/user.go index 192dc68..da1a24a 100644 --- a/internal/handlers/user.go +++ b/internal/handlers/user.go @@ -310,6 +310,11 @@ func writeAuthenticatedPasswordResetError(w http.ResponseWriter, err error) bool return true } + if errors.Is(err, user.ErrRateLimit) { + web.WriteJSONResponse(w, http.StatusTooManyRequests, map[string]any{"error": "try again later"}) + return true + } + if writeWeakPasswordError(w, err) { return true } diff --git a/internal/service/user/service.go b/internal/service/user/service.go index 8ab5fc1..5427fe3 100644 --- a/internal/service/user/service.go +++ b/internal/service/user/service.go @@ -5,9 +5,6 @@ import ( "context" "crypto/rand" "crypto/sha256" - "devinhadley/gobootstrapweb/internal/db" - "devinhadley/gobootstrapweb/internal/pgerr" - "devinhadley/gobootstrapweb/internal/service/email" "encoding/base64" "errors" "fmt" @@ -17,6 +14,10 @@ import ( "time" "unicode/utf8" + "devinhadley/gobootstrapweb/internal/db" + "devinhadley/gobootstrapweb/internal/pgerr" + "devinhadley/gobootstrapweb/internal/service/email" + "github.com/jackc/pgx/v5" "github.com/jackc/pgx/v5/pgtype" "github.com/matthewhartstonge/argon2" @@ -206,21 +207,16 @@ func (s *Service) LogIn(ctx context.Context, input AuthenticateBody) (User, erro } func (s *Service) ResetPasswordForAuthenticatedUser(ctx context.Context, usr User, input AuthenticatedPasswordResetBody) error { - err := s.isValidPassword(input.NewPassword) + err := s.verifyReauthentication(ctx, usr.DBUser().Email, input.Password, usr.DBUser().PasswordHash) if err != nil { return err } - // TODO: No rate-limiting here. An authenticated session could be used to brute-force. - ok, err := verifyPassword(input.Password, usr.DBUser().PasswordHash) + err = s.isValidPassword(input.NewPassword) if err != nil { return err } - if !ok { - return ErrInvalidCredentials - } - newPasswordHash, err := createPasswordHash(input.NewPassword) if err != nil { return fmt.Errorf("hashing password during authenticated reset: %w", err) @@ -367,18 +363,9 @@ func (s *Service) CreateEmailResetRequest(ctx context.Context, usr User, input C return ErrRateLimit } - // TODO: rate limit internal auth attempts. - ok, err = verifyPassword(input.Password, usr.DBUser().PasswordHash) + err = s.verifyReauthentication(ctx, currentEmail, input.Password, usr.DBUser().PasswordHash) if err != nil { - return fmt.Errorf("validating password hash: %w", err) - } - - if !ok { - err = s.createAuthAttempt(ctx, db.AuthActionEmailReset, currentEmail, db.AuthOutcomeFailed) - if err != nil { - log.Printf("creating auth attempt for email reset request: %v", err) - } - return ErrInvalidCredentials + return err } if newEmail == currentEmail { @@ -522,10 +509,22 @@ func (s *Service) isValidPassword(password string) error { } func (s *Service) isLoginRateLimited(ctx context.Context, email string) (bool, error) { - timeBefore := time.Now().Add(-(rateLimitLoginDurationMinutes * time.Minute)) + return s.isFailedAttemptRateLimited(ctx, db.AuthActionLogin, email, rateLimitLoginDurationMinutes*time.Minute, rateLimitLoginAttemptsAllowed) +} + +// Reauthentication deliberately reuses the login policy so an attacker cannot get a +// bigger guessing budget by switching endpoints. +func (s *Service) isReauthenticationRateLimited(ctx context.Context, email string) (bool, error) { + return s.isFailedAttemptRateLimited(ctx, db.AuthActionReauthentication, email, rateLimitLoginDurationMinutes*time.Minute, rateLimitLoginAttemptsAllowed) +} + +// isFailedAttemptRateLimited enforces a single-window, failed-only rate limit for the +// given action. +func (s *Service) isFailedAttemptRateLimited(ctx context.Context, action db.AuthAction, email string, window time.Duration, allowed int64) (bool, error) { + timeBefore := time.Now().Add(-window) - loginAttemptsForEmail, err := s.queries.CountFailedAuthAttemptsSince(ctx, db.CountFailedAuthAttemptsSinceParams{ - Action: db.AuthActionLogin, + attemptsForEmail, err := s.queries.CountFailedAuthAttemptsSince(ctx, db.CountFailedAuthAttemptsSinceParams{ + Action: action, Email: email, CreatedAt: pgtype.Timestamptz{ Time: timeBefore, @@ -536,7 +535,7 @@ func (s *Service) isLoginRateLimited(ctx context.Context, email string) (bool, e return false, err } - return loginAttemptsForEmail >= rateLimitLoginAttemptsAllowed, nil + return attemptsForEmail >= allowed, nil } func (s *Service) isCreatePasswordResetRateLimited(ctx context.Context, email string) (bool, error) { @@ -589,6 +588,38 @@ func (s *Service) failLoginAttempt(ctx context.Context, email string) (User, err return User{}, ErrInvalidCredentials } +// It records the outcome as a reauthentication auth attempt so the shared +// failed-attempt rate limit applies across every call site, preventing a hijacked session +// from being used to brute-force the account password. +func (s *Service) verifyReauthentication(ctx context.Context, email, password, passwordHash string) error { + isLimited, err := s.isReauthenticationRateLimited(ctx, email) + if err != nil { + return fmt.Errorf("checking if reauthentication rate limited: %w", err) + } + + if isLimited { + return ErrRateLimit + } + + ok, err := verifyPassword(password, passwordHash) + if err != nil { + return err + } + + if !ok { + if err := s.createAuthAttempt(ctx, db.AuthActionReauthentication, email, db.AuthOutcomeFailed); err != nil { + log.Printf("creating auth attempt for reauthentication: %v", err) + } + return ErrInvalidCredentials + } + + if err := s.createAuthAttempt(ctx, db.AuthActionReauthentication, email, db.AuthOutcomeSucceeded); err != nil { + log.Printf("creating auth attempt for reauthentication: %v", err) + } + + return nil +} + func normalizeAndValidateEmail(input string) (string, bool) { email := strings.TrimSpace(input) diff --git a/internal/service/user/user_test.go b/internal/service/user/user_test.go index 01c876d..655f189 100644 --- a/internal/service/user/user_test.go +++ b/internal/service/user/user_test.go @@ -55,6 +55,8 @@ func TestPasswordReset(t *testing.T) { t.Run("can reset password when authenticated", testResetPasswordForAuthenticatedUser) t.Run("password reset fails with authenticated user if existing password incorrect", testResetPasswordForAuthenticatedUserWrongCurrentPassword) t.Run("authenticated password reset doesn't allow weak pass", testResetPasswordForAuthenticatedUserRejectsWeakPassword) + t.Run("authenticated password reset is rate limited after repeated failed reauthentication", testResetPasswordForAuthenticatedUserReauthRateLimited) + t.Run("authenticated password reset records a failed reauthentication attempt", testResetPasswordForAuthenticatedUserRecordsFailedReauthAttempt) t.Run("can request token password reset", testCanRequestPasswordReset) t.Run("cant create password reset with malformed email", testCantCreatePasswordResetWithMalformedEmail) t.Run("requesting token password reset response for unkown email", testRequestingTokenPasswordResetForUnknownEmail) @@ -74,6 +76,8 @@ func TestCreateEmailResetRequest(t *testing.T) { t.Run("email reset request propagates unexpected error checking new email", testCreateEmailResetRequestPropagatesUnexpectedGetUserByEmailError) t.Run("cant request more than 3 email resets for a particular email in 120 minutes", testCantRequestMoreThanThreeEmailResetsIn120Minutes) t.Run("cant request more than 2 email resets for a particular email in 15 minutes", testCantRequestMoreThanTwoEmailResetsIn15Minutes) + t.Run("email reset request is rate limited after repeated failed reauthentication", testCreateEmailResetRequestReauthRateLimited) + t.Run("email reset request and password reset share the reauthentication rate limit budget", testReauthenticationRateLimitSharedAcrossFlows) } func testUserSignUp(t *testing.T) { @@ -739,6 +743,98 @@ func testResetPasswordForAuthenticatedUserWrongCurrentPassword(t *testing.T) { } } +func testResetPasswordForAuthenticatedUserReauthRateLimited(t *testing.T) { + ctx := context.Background() + currentPassword := "correct-current-password" + + usr := UserFromDB(db.User{ + ID: 42, + Email: "current@example.com", + PasswordHash: hashPassword(t, currentPassword), + }) + + rateLimitChecked := false + + userService := setupUserService(t, mockQueries{ + CountFailedAuthAttemptsSinceFn: func(_ context.Context, arg db.CountFailedAuthAttemptsSinceParams) (int64, error) { + if arg.Action != db.AuthActionReauthentication { + t.Fatalf("CountFailedAuthAttemptsSince got action %q, want %q", arg.Action, db.AuthActionReauthentication) + } + if arg.Email != usr.DBUser().Email { + t.Fatalf("CountFailedAuthAttemptsSince got email %q, want %q", arg.Email, usr.DBUser().Email) + } + rateLimitChecked = true + return rateLimitLoginAttemptsAllowed, nil + }, + UpdatePasswordHashFn: func(context.Context, db.UpdatePasswordHashParams) error { + t.Fatal("UpdatePasswordHash should not be called for rate limited reauthentication") + return nil + }, + CreateLoginAuthAttemptFn: func(context.Context, db.CreateLoginAuthAttemptParams) error { + t.Fatal("CreateLoginAuthAttempt should not be called for rate limited reauthentication") + return nil + }, + }) + + err := userService.ResetPasswordForAuthenticatedUser(ctx, usr, AuthenticatedPasswordResetBody{ + Password: currentPassword, + NewPassword: "brand-new-password", + }) + if !errors.Is(err, ErrRateLimit) { + t.Fatalf("got error %v, want %v", err, ErrRateLimit) + } + + if !rateLimitChecked { + t.Fatal("CountFailedAuthAttemptsSince was not called") + } +} + +func testResetPasswordForAuthenticatedUserRecordsFailedReauthAttempt(t *testing.T) { + ctx := context.Background() + actualCurrentPassword := "correct-current-password" + currentEmail := "current@example.com" + + usr := UserFromDB(db.User{ + ID: 42, + Email: currentEmail, + PasswordHash: hashPassword(t, actualCurrentPassword), + }) + + authAttemptCreated := false + + userService := setupUserService(t, mockQueries{ + UpdatePasswordHashFn: func(context.Context, db.UpdatePasswordHashParams) error { + t.Fatal("UpdatePasswordHash should not be called when current password is incorrect") + return nil + }, + CreateLoginAuthAttemptFn: func(_ context.Context, arg db.CreateLoginAuthAttemptParams) error { + if arg.Action != db.AuthActionReauthentication { + t.Fatalf("CreateLoginAuthAttempt got action %q, want %q", arg.Action, db.AuthActionReauthentication) + } + if arg.Email != currentEmail { + t.Fatalf("CreateLoginAuthAttempt got email %q, want %q", arg.Email, currentEmail) + } + if arg.Outcome != db.AuthOutcomeFailed { + t.Fatalf("CreateLoginAuthAttempt got outcome %q, want %q", arg.Outcome, db.AuthOutcomeFailed) + } + authAttemptCreated = true + return nil + }, + }) + + err := userService.ResetPasswordForAuthenticatedUser(ctx, usr, AuthenticatedPasswordResetBody{ + Password: "wrong-current-password", + NewPassword: "brand-new-password", + }) + if !errors.Is(err, ErrInvalidCredentials) { + t.Fatalf("got error %v, want %v", err, ErrInvalidCredentials) + } + + if !authAttemptCreated { + t.Fatal("CreateLoginAuthAttempt was not called") + } +} + func testResetPasswordForAuthenticatedUserRejectsWeakPassword(t *testing.T) { ctx := context.Background() currentPassword := "correct-current-password" @@ -1208,7 +1304,8 @@ func testCanRequestEmailReset(t *testing.T) { resetRequested := false newEmailMailed := false oldEmailNotified := false - authAttemptCreated := false + reauthAttemptCreated := false + emailResetAttemptCreated := false emailResetURL := "http://example.com/email-reset" userService := setupUserServiceWithEmailReset(t, mockQueries{ @@ -1233,16 +1330,20 @@ func testCanRequestEmailReset(t *testing.T) { return db.EmailResetRequest{ID: arg.ID, UserID: arg.UserID, NewEmail: arg.NewEmail}, nil }, CreateLoginAuthAttemptFn: func(_ context.Context, arg db.CreateLoginAuthAttemptParams) error { - if arg.Action != db.AuthActionEmailReset { - t.Fatalf("CreateLoginAuthAttempt got action %q, want %q", arg.Action, db.AuthActionEmailReset) - } if arg.Email != currentEmail { t.Fatalf("CreateLoginAuthAttempt got email %q, want %q", arg.Email, currentEmail) } if arg.Outcome != db.AuthOutcomeSucceeded { t.Fatalf("CreateLoginAuthAttempt got outcome %q, want %q", arg.Outcome, db.AuthOutcomeSucceeded) } - authAttemptCreated = true + switch arg.Action { + case db.AuthActionReauthentication: + reauthAttemptCreated = true + case db.AuthActionEmailReset: + emailResetAttemptCreated = true + default: + t.Fatalf("CreateLoginAuthAttempt got unexpected action %q", arg.Action) + } return nil }, }, email.MockEmailService{ @@ -1292,8 +1393,11 @@ func testCanRequestEmailReset(t *testing.T) { if !oldEmailNotified { t.Fatal("SendMail to old email was not called") } - if !authAttemptCreated { - t.Fatal("CreateLoginAuthAttempt was not called") + if !reauthAttemptCreated { + t.Fatal("CreateLoginAuthAttempt was not called for reauthentication") + } + if !emailResetAttemptCreated { + t.Fatal("CreateLoginAuthAttempt was not called for email_reset") } } @@ -1425,8 +1529,8 @@ func testCreateEmailResetRequestFailsWithIncorrectPassword(t *testing.T) { return db.EmailResetRequest{}, nil }, CreateLoginAuthAttemptFn: func(_ context.Context, arg db.CreateLoginAuthAttemptParams) error { - if arg.Action != db.AuthActionEmailReset { - t.Fatalf("CreateLoginAuthAttempt got action %q, want %q", arg.Action, db.AuthActionEmailReset) + if arg.Action != db.AuthActionReauthentication { + t.Fatalf("CreateLoginAuthAttempt got action %q, want %q", arg.Action, db.AuthActionReauthentication) } if arg.Email != currentEmail { t.Fatalf("CreateLoginAuthAttempt got email %q, want %q", arg.Email, currentEmail) @@ -1457,6 +1561,119 @@ func testCreateEmailResetRequestFailsWithIncorrectPassword(t *testing.T) { } } +func testCreateEmailResetRequestReauthRateLimited(t *testing.T) { + ctx := context.Background() + currentPassword := "correct-current-password" + currentEmail := "current@example.com" + + usr := UserFromDB(db.User{ + ID: 42, + Email: currentEmail, + PasswordHash: hashPassword(t, currentPassword), + }) + + rateLimitChecked := false + + userService := setupUserServiceWithEmailReset(t, mockQueries{ + CountFailedAuthAttemptsSinceFn: func(_ context.Context, arg db.CountFailedAuthAttemptsSinceParams) (int64, error) { + if arg.Action != db.AuthActionReauthentication { + t.Fatalf("CountFailedAuthAttemptsSince got action %q, want %q", arg.Action, db.AuthActionReauthentication) + } + if arg.Email != currentEmail { + t.Fatalf("CountFailedAuthAttemptsSince got email %q, want %q", arg.Email, currentEmail) + } + rateLimitChecked = true + return rateLimitLoginAttemptsAllowed, nil + }, + GetUserByEmailFn: func(context.Context, string) (db.User, error) { + t.Fatal("GetUserByEmail should not be called for rate limited reauthentication") + return db.User{}, nil + }, + CreateEmailResetRequestFn: func(context.Context, db.CreateEmailResetRequestParams) (db.EmailResetRequest, error) { + t.Fatal("CreateEmailResetRequest should not be called for rate limited reauthentication") + return db.EmailResetRequest{}, nil + }, + CreateLoginAuthAttemptFn: func(context.Context, db.CreateLoginAuthAttemptParams) error { + t.Fatal("CreateLoginAuthAttempt should not be called for rate limited reauthentication") + return nil + }, + }, email.MockEmailService{ + SendMailFn: func(string, string, string) error { + t.Fatal("SendMail should not be called for rate limited reauthentication") + return nil + }, + }, "http://example.com/email-reset") + + err := userService.CreateEmailResetRequest(ctx, usr, CreateEmailResetRequestBody{ + Password: currentPassword, + NewEmail: "new@example.com", + }) + if !errors.Is(err, ErrRateLimit) { + t.Fatalf("got error %v, want %v", err, ErrRateLimit) + } + + if !rateLimitChecked { + t.Fatal("CountFailedAuthAttemptsSince was not called") + } +} + +// testReauthenticationRateLimitSharedAcrossFlows asserts that password-change and +// email-reset-request reauthentication both check and record against the same +// db.AuthActionReauthentication + email budget, so an attacker cannot get a bigger +// guessing budget by switching endpoints. +func testReauthenticationRateLimitSharedAcrossFlows(t *testing.T) { + ctx := context.Background() + currentPassword := "correct-current-password" + currentEmail := "current@example.com" + + usr := UserFromDB(db.User{ + ID: 42, + Email: currentEmail, + PasswordHash: hashPassword(t, currentPassword), + }) + + var checkedActions, checkedEmails []string + + newQueries := func() mockQueries { + return mockQueries{ + CountFailedAuthAttemptsSinceFn: func(_ context.Context, arg db.CountFailedAuthAttemptsSinceParams) (int64, error) { + checkedActions = append(checkedActions, string(arg.Action)) + checkedEmails = append(checkedEmails, arg.Email) + return rateLimitLoginAttemptsAllowed, nil + }, + } + } + + passwordResetQueries := newQueries() + passwordResetService := setupUserService(t, passwordResetQueries) + err := passwordResetService.ResetPasswordForAuthenticatedUser(ctx, usr, AuthenticatedPasswordResetBody{ + Password: currentPassword, + NewPassword: "brand-new-password", + }) + if !errors.Is(err, ErrRateLimit) { + t.Fatalf("ResetPasswordForAuthenticatedUser got error %v, want %v", err, ErrRateLimit) + } + + emailResetQueries := newQueries() + emailResetService := setupUserServiceWithEmailReset(t, emailResetQueries, email.MockEmailService{}, "http://example.com/email-reset") + err = emailResetService.CreateEmailResetRequest(ctx, usr, CreateEmailResetRequestBody{ + Password: currentPassword, + NewEmail: "new@example.com", + }) + if !errors.Is(err, ErrRateLimit) { + t.Fatalf("CreateEmailResetRequest got error %v, want %v", err, ErrRateLimit) + } + + for i, action := range checkedActions { + if action != string(db.AuthActionReauthentication) { + t.Fatalf("check %d: got action %q, want %q", i, action, db.AuthActionReauthentication) + } + if checkedEmails[i] != currentEmail { + t.Fatalf("check %d: got email %q, want %q", i, checkedEmails[i], currentEmail) + } + } +} + func testCantRequestEmailResetWithMalformedNewEmail(t *testing.T) { ctx := context.Background() diff --git a/sqlc/migrations/20260510222217_ratelimit.sql b/sqlc/migrations/20260510222217_ratelimit.sql index dde4d7f..b61498f 100644 --- a/sqlc/migrations/20260510222217_ratelimit.sql +++ b/sqlc/migrations/20260510222217_ratelimit.sql @@ -1,5 +1,5 @@ -- +goose Up -CREATE TYPE auth_action AS ENUM ('login', 'password_reset', 'email_reset'); +CREATE TYPE auth_action AS ENUM ('login', 'password_reset', 'email_reset', 'reauthentication'); CREATE TYPE auth_outcome AS ENUM ('succeeded', 'failed'); CREATE TABLE auth_attempts ( From be45505e4de87b6a201ac8f0f08c0abaa8492235 Mon Sep 17 00:00:00 2001 From: Devin Hadley <68879608+devinhadley@users.noreply.github.com> Date: Sun, 30 Aug 2026 18:14:49 -0700 Subject: [PATCH 2/2] refactor(user): pass User to verifyReauthentication instead of email+hash Both call sites already had the User in scope; deriving email and password hash inside the helper drops two redundant parameters. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01VSYkKjv6rTX4ys77muo5jQ --- internal/service/user/service.go | 17 +++------ internal/service/user/user_test.go | 59 ------------------------------ 2 files changed, 6 insertions(+), 70 deletions(-) diff --git a/internal/service/user/service.go b/internal/service/user/service.go index 5427fe3..b35a3d3 100644 --- a/internal/service/user/service.go +++ b/internal/service/user/service.go @@ -207,7 +207,7 @@ func (s *Service) LogIn(ctx context.Context, input AuthenticateBody) (User, erro } func (s *Service) ResetPasswordForAuthenticatedUser(ctx context.Context, usr User, input AuthenticatedPasswordResetBody) error { - err := s.verifyReauthentication(ctx, usr.DBUser().Email, input.Password, usr.DBUser().PasswordHash) + err := s.verifyReauthentication(ctx, usr, input.Password) if err != nil { return err } @@ -363,7 +363,7 @@ func (s *Service) CreateEmailResetRequest(ctx context.Context, usr User, input C return ErrRateLimit } - err = s.verifyReauthentication(ctx, currentEmail, input.Password, usr.DBUser().PasswordHash) + err = s.verifyReauthentication(ctx, usr, input.Password) if err != nil { return err } @@ -512,14 +512,10 @@ func (s *Service) isLoginRateLimited(ctx context.Context, email string) (bool, e return s.isFailedAttemptRateLimited(ctx, db.AuthActionLogin, email, rateLimitLoginDurationMinutes*time.Minute, rateLimitLoginAttemptsAllowed) } -// Reauthentication deliberately reuses the login policy so an attacker cannot get a -// bigger guessing budget by switching endpoints. func (s *Service) isReauthenticationRateLimited(ctx context.Context, email string) (bool, error) { return s.isFailedAttemptRateLimited(ctx, db.AuthActionReauthentication, email, rateLimitLoginDurationMinutes*time.Minute, rateLimitLoginAttemptsAllowed) } -// isFailedAttemptRateLimited enforces a single-window, failed-only rate limit for the -// given action. func (s *Service) isFailedAttemptRateLimited(ctx context.Context, action db.AuthAction, email string, window time.Duration, allowed int64) (bool, error) { timeBefore := time.Now().Add(-window) @@ -588,10 +584,9 @@ func (s *Service) failLoginAttempt(ctx context.Context, email string) (User, err return User{}, ErrInvalidCredentials } -// It records the outcome as a reauthentication auth attempt so the shared -// failed-attempt rate limit applies across every call site, preventing a hijacked session -// from being used to brute-force the account password. -func (s *Service) verifyReauthentication(ctx context.Context, email, password, passwordHash string) error { +func (s *Service) verifyReauthentication(ctx context.Context, usr User, password string) error { + email := usr.DBUser().Email + isLimited, err := s.isReauthenticationRateLimited(ctx, email) if err != nil { return fmt.Errorf("checking if reauthentication rate limited: %w", err) @@ -601,7 +596,7 @@ func (s *Service) verifyReauthentication(ctx context.Context, email, password, p return ErrRateLimit } - ok, err := verifyPassword(password, passwordHash) + ok, err := verifyPassword(password, usr.DBUser().PasswordHash) if err != nil { return err } diff --git a/internal/service/user/user_test.go b/internal/service/user/user_test.go index 655f189..069de0d 100644 --- a/internal/service/user/user_test.go +++ b/internal/service/user/user_test.go @@ -676,7 +676,6 @@ func testResetPasswordForAuthenticatedUser(t *testing.T) { userService := setupUserService(t, mockQueries{ UpdatePasswordHashFn: func(_ context.Context, arg db.UpdatePasswordHashParams) error { - if arg.ID != usr.DBUser().ID { t.Fatalf("UpdatePasswordHash got id %v, want %v", arg.ID, usr.DBUser().ID) } @@ -1073,7 +1072,6 @@ func testRequestingTokenPasswordResetForUnknownEmail(t *testing.T) { return db.PasswordResetRequest{}, nil }, CreateLoginAuthAttemptFn: func(_ context.Context, arg db.CreateLoginAuthAttemptParams) error { - if arg.Action != db.AuthActionPasswordReset { t.Fatalf("got action %v, want %v", arg.Action, db.AuthActionPasswordReset) } @@ -1617,63 +1615,6 @@ func testCreateEmailResetRequestReauthRateLimited(t *testing.T) { } } -// testReauthenticationRateLimitSharedAcrossFlows asserts that password-change and -// email-reset-request reauthentication both check and record against the same -// db.AuthActionReauthentication + email budget, so an attacker cannot get a bigger -// guessing budget by switching endpoints. -func testReauthenticationRateLimitSharedAcrossFlows(t *testing.T) { - ctx := context.Background() - currentPassword := "correct-current-password" - currentEmail := "current@example.com" - - usr := UserFromDB(db.User{ - ID: 42, - Email: currentEmail, - PasswordHash: hashPassword(t, currentPassword), - }) - - var checkedActions, checkedEmails []string - - newQueries := func() mockQueries { - return mockQueries{ - CountFailedAuthAttemptsSinceFn: func(_ context.Context, arg db.CountFailedAuthAttemptsSinceParams) (int64, error) { - checkedActions = append(checkedActions, string(arg.Action)) - checkedEmails = append(checkedEmails, arg.Email) - return rateLimitLoginAttemptsAllowed, nil - }, - } - } - - passwordResetQueries := newQueries() - passwordResetService := setupUserService(t, passwordResetQueries) - err := passwordResetService.ResetPasswordForAuthenticatedUser(ctx, usr, AuthenticatedPasswordResetBody{ - Password: currentPassword, - NewPassword: "brand-new-password", - }) - if !errors.Is(err, ErrRateLimit) { - t.Fatalf("ResetPasswordForAuthenticatedUser got error %v, want %v", err, ErrRateLimit) - } - - emailResetQueries := newQueries() - emailResetService := setupUserServiceWithEmailReset(t, emailResetQueries, email.MockEmailService{}, "http://example.com/email-reset") - err = emailResetService.CreateEmailResetRequest(ctx, usr, CreateEmailResetRequestBody{ - Password: currentPassword, - NewEmail: "new@example.com", - }) - if !errors.Is(err, ErrRateLimit) { - t.Fatalf("CreateEmailResetRequest got error %v, want %v", err, ErrRateLimit) - } - - for i, action := range checkedActions { - if action != string(db.AuthActionReauthentication) { - t.Fatalf("check %d: got action %q, want %q", i, action, db.AuthActionReauthentication) - } - if checkedEmails[i] != currentEmail { - t.Fatalf("check %d: got email %q, want %q", i, checkedEmails[i], currentEmail) - } - } -} - func testCantRequestEmailResetWithMalformedNewEmail(t *testing.T) { ctx := context.Background()