From 2dc84e2d05159a9dffa073111028b4a46be25a13 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 28 May 2026 21:12:44 +0200 Subject: [PATCH] sec(auth): collapse MFA-not-configured into generic auth failure (closes #391) When MFA is enabled but the secret is missing (data-integrity anomaly), returning a distinct "MFA is enabled but not configured" error reveals that the supplied password was correct. Collapse the branch to the same generic "Check your email address and password and try again" message used for wrong passwords, and log the anomaly internally (by user ID, no PII) so operators can detect it without relying on the API response. Remove the dead substring match from mapMFAServiceError in handler_auth.go. Add a sub-test that asserts both paths return an identical response body. --- internal/api/handler_auth.go | 3 +- internal/auth/service.go | 6 +++- internal/auth/service_test.go | 64 +++++++++++++++++++++++++++-------- 3 files changed, 55 insertions(+), 18 deletions(-) diff --git a/internal/api/handler_auth.go b/internal/api/handler_auth.go index a039c59b4..bf924201a 100644 --- a/internal/api/handler_auth.go +++ b/internal/api/handler_auth.go @@ -519,8 +519,7 @@ func mapMFAServiceError(err error) error { strings.Contains(msg, "MFA code or recovery code required"), strings.Contains(msg, "no MFA enrollment in progress"), strings.Contains(msg, "MFA enrollment expired"), - strings.Contains(msg, "MFA is not enabled"), - strings.Contains(msg, "MFA is enabled but not configured"): + strings.Contains(msg, "MFA is not enabled"): return NewClientError(400, msg) case strings.Contains(msg, "authentication failed"): return NewClientError(401, msg) diff --git a/internal/auth/service.go b/internal/auth/service.go index bdd556372..2e414e693 100644 --- a/internal/auth/service.go +++ b/internal/auth/service.go @@ -195,7 +195,11 @@ func (s *Service) verifyPasswordAndMFA(ctx context.Context, user *User, req Logi return ErrMFARequired } if user.MFASecret == "" { - return fmt.Errorf("MFA is enabled but not configured") + // Data integrity anomaly: MFA is flagged enabled but has no secret. + // 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") } // verifyTOTP is panic-safe: a malformed secret causes generateTOTP to return "" // (base32 decode error), resulting in a comparison miss rather than a panic. diff --git a/internal/auth/service_test.go b/internal/auth/service_test.go index 107dae526..095304add 100644 --- a/internal/auth/service_test.go +++ b/internal/auth/service_test.go @@ -442,11 +442,12 @@ func TestLogin_WithMFA_MissingCode(t *testing.T) { assert.ErrorIs(t, err, ErrMFARequired) } +// TestLogin_WithMFA_NoSecret verifies that a correct password against an account +// where MFA is enabled but the secret is missing returns the same generic error +// as a wrong password -- the response must not reveal that the password was correct +// (closes #391). func TestLogin_WithMFA_NoSecret(t *testing.T) { ctx := context.Background() - mockStore := new(MockStore) - mockEmail := new(MockEmailSender) - service := createTestService(mockStore, mockEmail) s := newTestService() hash, _ := s.hashPassword("SecurePass@123") @@ -455,24 +456,57 @@ func TestLogin_WithMFA_NoSecret(t *testing.T) { ID: "user-123", Email: "mfa@example.com", PasswordHash: hash, - Salt: "", // Not used anymore + Salt: "", Active: true, MFAEnabled: true, - MFASecret: "", // No secret configured + MFASecret: "", // Data-integrity anomaly: MFA enabled but no secret stored } - mockStore.On("GetUserByEmail", ctx, "mfa@example.com").Return(user, nil) + const genericMsg = "Check your email address and password and try again" - req := LoginRequest{ - Email: "mfa@example.com", - Password: "SecurePass@123", - MFACode: "123456", - } + // Right password, no MFA secret -- must return the generic error. + t.Run("correct password no MFA secret returns generic error", func(t *testing.T) { + mockStore := new(MockStore) + mockEmail := new(MockEmailSender) + service := createTestService(mockStore, mockEmail) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) - resp, err := service.Login(ctx, req) - assert.Error(t, err) - assert.Nil(t, resp) - assert.Contains(t, err.Error(), "MFA is enabled but not configured") + mockStore.On("GetUserByEmail", ctx, "mfa@example.com").Return(user, nil).Once() + + req := LoginRequest{ + Email: "mfa@example.com", + Password: "SecurePass@123", + MFACode: "123456", + } + + resp, err := service.Login(ctx, req) + assert.Error(t, err) + assert.Nil(t, resp) + assert.Contains(t, err.Error(), genericMsg) + }) + + // Wrong password -- must return the identical generic error so the two cases + // are indistinguishable to the caller. + t.Run("wrong password returns same generic error", func(t *testing.T) { + mockStore := new(MockStore) + mockEmail := new(MockEmailSender) + service := createTestService(mockStore, mockEmail) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + mockStore.On("GetUserByEmail", ctx, "mfa@example.com").Return(user, nil).Once() + mockStore.On("UpdateUser", ctx, mock.AnythingOfType("*auth.User")).Return(nil).Maybe() + + req := LoginRequest{ + Email: "mfa@example.com", + Password: "wrongpassword", + MFACode: "123456", + } + + resp, err := service.Login(ctx, req) + assert.Error(t, err) + assert.Nil(t, resp) + assert.Contains(t, err.Error(), genericMsg) + }) } // Test UpdateUserProfile