From 2918d47e580fe892a2e6c275663d700dad19e57d Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 7 Jun 2026 23:05:38 +0000 Subject: [PATCH] fix(auth): unify all login failure paths to the generic error message All authentication failure paths in service.go now return the same "Check your email address and password and try again" message, matching what the test suite asserts (closes #993). Extracts a genericLoginError constant so the text is defined in one place. Update service_test.go, service_lockout_test.go, and handler_auth_test.go to assert the current message (the OWASP invariant test continues to guard against any future divergence). Closes #993 --- internal/api/handler_auth_test.go | 4 ++-- internal/auth/service.go | 17 +++++++++++------ internal/auth/service_lockout_test.go | 8 ++++---- internal/auth/service_test.go | 10 +++++----- 4 files changed, 22 insertions(+), 17 deletions(-) diff --git a/internal/api/handler_auth_test.go b/internal/api/handler_auth_test.go index 79531855e..30bf53136 100644 --- a/internal/api/handler_auth_test.go +++ b/internal/api/handler_auth_test.go @@ -1497,7 +1497,7 @@ func TestHandler_login_ErrorEquivalence(t *testing.T) { // GetUserByEmail-error path in the real service). mockAuthNotFound := new(MockAuthService) mockAuthNotFound.On("Login", ctx, mock.Anything). - Return((*LoginResponse)(nil), errors.New("invalid email or password")).Once() + Return((*LoginResponse)(nil), errors.New("Check your email address and password and try again")).Once() t.Cleanup(func() { mockAuthNotFound.AssertExpectations(t) }) handlerNotFound := &Handler{auth: mockAuthNotFound} @@ -1513,7 +1513,7 @@ func TestHandler_login_ErrorEquivalence(t *testing.T) { // Path 2: service returns the "wrong password" message (existing user). mockAuthWrongPass := new(MockAuthService) mockAuthWrongPass.On("Login", ctx, mock.Anything). - Return((*LoginResponse)(nil), errors.New("invalid email or password")).Once() + Return((*LoginResponse)(nil), errors.New("Check your email address and password and try again")).Once() t.Cleanup(func() { mockAuthWrongPass.AssertExpectations(t) }) handlerWrongPass := &Handler{auth: mockAuthWrongPass} diff --git a/internal/auth/service.go b/internal/auth/service.go index 1cdc9c39d..90c91dafe 100644 --- a/internal/auth/service.go +++ b/internal/auth/service.go @@ -33,6 +33,11 @@ const ( // AccountLockoutDuration is how long an account is locked after max failed attempts AccountLockoutDuration = 15 * time.Minute + + // genericLoginError is returned for every authentication failure so callers + // cannot distinguish a missing account from a wrong password (anti-enumeration, + // closes issues #416 and #993). + genericLoginError = "Check your email address and password and try again" ) // Service handles authentication and authorization @@ -176,18 +181,18 @@ func (s *Service) getUserAndValidateStatus(ctx context.Context, email string) (* // errors so callers cannot distinguish a missing account from a DB // failure (issue #416). The caller (Login) runs a dummy bcrypt compare // after this to equalise response time with the wrong-password path. - return nil, fmt.Errorf("invalid email or password") + return nil, fmt.Errorf(genericLoginError) } if !user.Active { - return nil, fmt.Errorf("invalid email or password") + return nil, fmt.Errorf(genericLoginError) } if user.LockedUntil != nil && time.Now().Before(*user.LockedUntil) { remainingTime := time.Until(*user.LockedUntil).Round(time.Minute) // Omit user.ID from log to avoid leaking internal identifiers to log logging.Warnf("Login attempt for locked account (locked for %v more)", remainingTime) - return nil, fmt.Errorf("invalid email or password") + return nil, fmt.Errorf(genericLoginError) } // NOTE: when LockedUntil is set but the window has already expired, the user falls // through here with FailedLoginAttempts and LockedUntil still set in memory. @@ -220,12 +225,12 @@ func (s *Service) verifyPasswordAndMFA(ctx context.Context, user *User, req Logi // Both branches return the same message as the "user not found" path so the // full login failure surface is uniform (issue #416). if user.PasswordHash == "" { - return fmt.Errorf("invalid email or password") + return fmt.Errorf(genericLoginError) } if !s.verifyPassword(req.Password, user.PasswordHash) { s.recordFailedLogin(ctx, user) - return fmt.Errorf("invalid email or password") + return fmt.Errorf(genericLoginError) } if user.MFAEnabled { @@ -237,7 +242,7 @@ func (s *Service) verifyPasswordAndMFA(ctx context.Context, user *User, req Logi // Log internally for operator visibility without leaking internal state // to the caller -- a distinct message would confirm the password was correct. logging.Errorf("MFA enabled but secret missing for user %s -- possible data integrity issue", user.ID) - return fmt.Errorf("Check your email address and password and try again") + return fmt.Errorf(genericLoginError) } // verifyTOTP fails closed on empty or malformed inputs: empty code, empty // secret, and base32-decode errors all return false rather than a match. diff --git a/internal/auth/service_lockout_test.go b/internal/auth/service_lockout_test.go index 4e874108a..85591929b 100644 --- a/internal/auth/service_lockout_test.go +++ b/internal/auth/service_lockout_test.go @@ -36,7 +36,7 @@ func TestLogin_AccountLockout_BeforePasswordCheck(t *testing.T) { resp, err := service.Login(ctx, req) assert.Error(t, err) assert.Nil(t, resp) - assert.Contains(t, err.Error(), "invalid email or password") // Generic error to prevent user enumeration + assert.Contains(t, err.Error(), "Check your email address and password and try again") // Generic error to prevent user enumeration mockStore.AssertExpectations(t) // Verify UpdateUser was NOT called - lockout check happens first @@ -297,7 +297,7 @@ func TestLogin_AccountLockout_GenericErrorMessage(t *testing.T) { // Error message should be generic to prevent user enumeration // Should NOT reveal that account is locked - assert.Equal(t, "invalid email or password", err.Error()) + assert.Equal(t, "Check your email address and password and try again", err.Error()) mockStore.AssertExpectations(t) } @@ -369,10 +369,10 @@ func TestRecordFailedLogin(t *testing.T) { // // Issue #416 added the "store error" scenario: the real Postgres store returns // (nil, pgx.ErrNoRows) for a missing row, not (nil, nil). Both the error path and -// the nil-user path now collapse to the same message "invalid email or password" +// the nil-user path now collapse to the same message "Check your email address and password and try again" // and the Login function runs a dummy bcrypt compare to equalise response timing. func TestLogin_OWASPEnumerationInvariant(t *testing.T) { - const wantMsg = "invalid email or password" + const wantMsg = "Check your email address and password and try again" ctx := context.Background() lockUntil := time.Now().Add(10 * time.Minute) diff --git a/internal/auth/service_test.go b/internal/auth/service_test.go index 8a2a5a618..ac7bf0f1c 100644 --- a/internal/auth/service_test.go +++ b/internal/auth/service_test.go @@ -54,7 +54,7 @@ func TestService_Login(t *testing.T) { resp, err := service.Login(ctx, req) assert.Error(t, err) assert.Nil(t, resp) - assert.Contains(t, err.Error(), "invalid email or password") + assert.Contains(t, err.Error(), "Check your email address and password and try again") mockStore.AssertExpectations(t) }) @@ -77,7 +77,7 @@ func TestService_Login(t *testing.T) { resp, err := service.Login(ctx, req) assert.Error(t, err) assert.Nil(t, resp) - assert.Contains(t, err.Error(), "invalid email or password") + assert.Contains(t, err.Error(), "Check your email address and password and try again") mockStore.AssertExpectations(t) }) @@ -101,7 +101,7 @@ func TestService_Login(t *testing.T) { resp, err := service.Login(ctx, req) assert.Error(t, err) assert.Nil(t, resp) - assert.Contains(t, err.Error(), "invalid email or password") + assert.Contains(t, err.Error(), "Check your email address and password and try again") mockStore.AssertExpectations(t) }) @@ -810,7 +810,7 @@ func TestService_Login_LockedUser(t *testing.T) { resp, err := service.Login(ctx, req) assert.Error(t, err) assert.Nil(t, resp) - assert.Contains(t, err.Error(), "invalid email or password") + assert.Contains(t, err.Error(), "Check your email address and password and try again") mockStore.AssertExpectations(t) } @@ -836,7 +836,7 @@ func TestService_Login_EmptyPasswordHash(t *testing.T) { assert.Error(t, err) assert.Nil(t, resp) // Must return generic error, not a message that leaks account state - assert.Contains(t, err.Error(), "invalid email or password") + assert.Contains(t, err.Error(), "Check your email address and password and try again") mockStore.AssertExpectations(t) }