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) }