sec(auth): unify login error response for unknown vs wrong-password (closes #416)

internal/api/handler_auth_test.go

@@ -1369,3 +1369,55 @@ func TestHandler_getCurrentUserPermissions_UserAPIKey(t *testing.T) {
 	require.Len(t, result.Permissions, 1)
 	assert.Equal(t, PermissionEntry{Action: "view", Resource: "recommendations"}, result.Permissions[0])
 }
+
+// TestHandler_login_ErrorEquivalence verifies that two failed login attempts --
+// one for a non-existent user and one for a wrong password on an existing user
+// -- produce IDENTICAL ClientError status codes and messages at the handler
+// layer (issue #416).
+//
+// This is the regression guard for username-enumeration via distinct error
+// messages: if either the HTTP status or the error body differ between the two
+// paths, an attacker can determine whether an email address is registered.
+func TestHandler_login_ErrorEquivalence(t *testing.T) {
+	ctx := context.Background()
+	encodedPassword := base64.StdEncoding.EncodeToString([]byte("SomePassword1!"))
+
+	// Path 1: service returns the "user not found" message (maps to the
+	// 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()
+	t.Cleanup(func() { mockAuthNotFound.AssertExpectations(t) })
+
+	handlerNotFound := &Handler{auth: mockAuthNotFound}
+	reqNotFound := &events.LambdaFunctionURLRequest{
+		Body: `{"email":"nonexistent@example.com","password":"` + encodedPassword + `"}`,
+	}
+	_, errNotFound := handlerNotFound.login(ctx, reqNotFound)
+	require.Error(t, errNotFound, "expected login to fail for unknown user")
+
+	ceNotFound, okNotFound := IsClientError(errNotFound)
+	require.True(t, okNotFound, "expected ClientError for unknown-user path")
+
+	// 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()
+	t.Cleanup(func() { mockAuthWrongPass.AssertExpectations(t) })
+
+	handlerWrongPass := &Handler{auth: mockAuthWrongPass}
+	reqWrongPass := &events.LambdaFunctionURLRequest{
+		Body: `{"email":"existing@example.com","password":"` + encodedPassword + `"}`,
+	}
+	_, errWrongPass := handlerWrongPass.login(ctx, reqWrongPass)
+	require.Error(t, errWrongPass, "expected login to fail for wrong password")
+
+	ceWrongPass, okWrongPass := IsClientError(errWrongPass)
+	require.True(t, okWrongPass, "expected ClientError for wrong-password path")
+
+	// Assert IDENTICAL status code and message body for both failure paths.
+	assert.Equal(t, ceNotFound.code, ceWrongPass.code,
+		"HTTP status must be identical for unknown-user and wrong-password paths")
+	assert.Equal(t, ceNotFound.Error(), ceWrongPass.Error(),
+		"error body must be identical for unknown-user and wrong-password paths to prevent enumeration")
+}

internal/auth/service.go

@@ -123,6 +123,11 @@ func (s *Service) Login(ctx context.Context, req LoginRequest) (*LoginResponse,
 
 	user, err := s.getUserAndValidateStatus(ctx, req.Email)
 	if err != nil {
+		// Run a dummy bcrypt compare so the response time for a non-existent
+		// user is indistinguishable from a wrong-password attempt on a real
+		// account. Without this, the missing-row path returns immediately
+		// (no bcrypt), leaking account existence via timing (issue #416).
+		s.verifyPassword(req.Password, dummyPasswordHash)
 		return nil, err
 	}
 
@@ -136,22 +141,23 @@ func (s *Service) Login(ctx context.Context, req LoginRequest) (*LoginResponse,
 // getUserAndValidateStatus retrieves user and checks if account is active and unlocked
 func (s *Service) getUserAndValidateStatus(ctx context.Context, email string) (*User, error) {
 	user, err := s.store.GetUserByEmail(ctx, email)
-	if err != nil {
-		return nil, fmt.Errorf("authentication failed")
-	}
-	if user == nil {
-		return nil, fmt.Errorf("Check your email address and password and try again")
+	if err != nil || user == nil {
+		// Return the same generic message for both "user not found" and store
+		// 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")
 	}
 
 	if !user.Active {
-		return nil, fmt.Errorf("Check your email address and password and try again")
+		return nil, fmt.Errorf("invalid email or password")
 	}
 
 	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("Check your email address and password and try again")
+		return nil, fmt.Errorf("invalid email or password")
 	}
 	// NOTE: when LockedUntil is set but the window has already expired, the user falls
 	// through here with FailedLoginAttempts and LockedUntil still set in memory.
@@ -180,14 +186,16 @@ func (s *Service) getUserAndValidateStatus(ctx context.Context, email string) (*
 // the slice on a match).
 func (s *Service) verifyPasswordAndMFA(ctx context.Context, user *User, req LoginRequest) error {
 	// Use a generic error for missing password hash to avoid leaking account state
-	// (a distinct message would reveal that the account exists but has no password set)
+	// (a distinct message would reveal that the account exists but has no password set).
+	// 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("Check your email address and password and try again")
+		return fmt.Errorf("invalid email or password")
 	}
 
 	if !s.verifyPassword(req.Password, user.PasswordHash) {
 		s.recordFailedLogin(ctx, user)
-		return fmt.Errorf("Check your email address and password and try again")
+		return fmt.Errorf("invalid email or password")
 	}
 
 	if user.MFAEnabled {

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(), "Check your email address and password and try again") // Generic error to prevent user enumeration
+	assert.Contains(t, err.Error(), "invalid email or password") // 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, "Check your email address and password and try again", err.Error())
+	assert.Equal(t, "invalid email or password", err.Error())
 
 	mockStore.AssertExpectations(t)
 }
@@ -360,28 +360,41 @@ func TestRecordFailedLogin(t *testing.T) {
 }
 
 // TestLogin_OWASPEnumerationInvariant guards against regression that re-introduces
-// distinct error messages for the 5 authentication failure modes (OWASP ASVS V3.3.4
-// / V14.2). All 5 paths MUST return an identical string so the login endpoint cannot
+// distinct error messages for the 6 authentication failure modes (OWASP ASVS V3.3.4
+// / V14.2). All 6 paths MUST return an identical string so the login endpoint cannot
 // be used as an email-existence oracle.
 //
 // If this test fails after a code change, that change almost certainly re-introduced
 // a username-enumeration vulnerability and MUST be reverted or fixed before landing.
+//
+// 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"
+// and the Login function runs a dummy bcrypt compare to equalise response timing.
 func TestLogin_OWASPEnumerationInvariant(t *testing.T) {
-	const wantMsg = "Check your email address and password and try again"
+	const wantMsg = "invalid email or password"
 	ctx := context.Background()
 
 	lockUntil := time.Now().Add(10 * time.Minute)
 
 	type scenario struct {
-		name    string
-		getUser func(t *testing.T) *User // nil means "user not found"
+		name       string
+		getUser    func(t *testing.T) *User // nil means "user not found (nil return)"
+		storeError error                    // non-nil means GetUserByEmail returns an error
 	}
 
 	scenarios := []scenario{
 		{
-			name:    "user not found",
+			name:    "user not found (nil, nil from store)",
 			getUser: func(t *testing.T) *User { return nil },
 		},
+		{
+			// Real Postgres store path: scanUser returns pgx.ErrNoRows for a
+			// missing row, so GetUserByEmail returns (nil, error) not (nil, nil).
+			name:       "user not found (nil, error from store)",
+			getUser:    func(t *testing.T) *User { return nil },
+			storeError: fmt.Errorf("no rows in result set"),
+		},
 		{
 			name: "inactive account",
 			getUser: func(t *testing.T) *User {
@@ -421,7 +434,7 @@ func TestLogin_OWASPEnumerationInvariant(t *testing.T) {
 			service := createTestService(mockStore, mockEmail)
 
 			user := sc.getUser(t)
-			mockStore.On("GetUserByEmail", ctx, "test@example.com").Return(user, nil).Once()
+			mockStore.On("GetUserByEmail", ctx, "test@example.com").Return(user, sc.storeError).Once()
 			mockStore.On("UpdateUser", ctx, mock.AnythingOfType("*auth.User")).Return(nil).Maybe()
 
 			req := LoginRequest{

internal/auth/service_password.go

@@ -18,6 +18,16 @@ import (
 // This adds about 4x more computational cost compared to DefaultCost (10).
 const bcryptCost = 12
 
+// dummyPasswordHash is a pre-computed bcrypt hash of a random constant string.
+// It is used by Login to run a timing-equalising bcrypt.CompareHashAndPassword
+// when the requested email does not exist, so an attacker cannot distinguish a
+// missing account from a wrong-password attempt via response time (issue #416).
+// The plain-text "dummy" value is intentionally unguessable and never stored.
+//
+// Generated once at compile time with cost bcryptCost (12).
+// nolint:gosec -- this is a public sentinel hash, not a credential
+var dummyPasswordHash = "$2a$12$iAMeexq41AwZ2Dj9oAvGfeVHQxK5ffLPPTNxwPB8bsf7olA730dxO"
+
 // Password validation constants following NIST guidelines
 const (
 	minPasswordLength       = 12  // Minimum password length

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(), "Check your email address and password and try again")
+		assert.Contains(t, err.Error(), "invalid email or password")
 
 		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(), "Check your email address and password and try again")
+		assert.Contains(t, err.Error(), "invalid email or password")
 
 		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(), "Check your email address and password and try again")
+		assert.Contains(t, err.Error(), "invalid email or password")
 
 		mockStore.AssertExpectations(t)
 	})
@@ -777,7 +777,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(), "Check your email address and password and try again")
+	assert.Contains(t, err.Error(), "invalid email or password")
 
 	mockStore.AssertExpectations(t)
 }
@@ -803,7 +803,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(), "Check your email address and password and try again")
+	assert.Contains(t, err.Error(), "invalid email or password")
 
 	mockStore.AssertExpectations(t)
 }