Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions internal/api/handler_auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
6 changes: 5 additions & 1 deletion internal/auth/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
64 changes: 49 additions & 15 deletions internal/auth/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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
Expand Down
Loading