diff --git a/internal/api/handler_auth_test.go b/internal/api/handler_auth_test.go index 29a17fd39..172cda77d 100644 --- a/internal/api/handler_auth_test.go +++ b/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") +} diff --git a/internal/auth/service.go b/internal/auth/service.go index bdd556372..785dd6c3e 100644 --- a/internal/auth/service.go +++ b/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 { diff --git a/internal/auth/service_lockout_test.go b/internal/auth/service_lockout_test.go index 71fb3092f..4e874108a 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(), "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{ diff --git a/internal/auth/service_password.go b/internal/auth/service_password.go index 8e390368f..432cdf321 100644 --- a/internal/auth/service_password.go +++ b/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 diff --git a/internal/auth/service_test.go b/internal/auth/service_test.go index 107dae526..eab8ac248 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(), "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) }