sec(auth): collapse MFA error paths to prevent enrollment status enumeration by cristim · Pull Request #532 · LeanerCloud/CUDly

cristim · 2026-05-19T23:53:33Z

Fixes #388

Summary by CodeRabbit

Bug Fixes
- Standardized authentication responses so credential and MFA failures now return a single generic error message visible to users.
Tests
- Added regression tests to verify consistent generic MFA/error responses and that repeated MFA failures (including missing codes) still increment failed-login counters and trigger account lockout.

…eration Fixes #388: Login previously returned distinct error messages for "MFA code required" (correct password, MFA enabled, no code supplied) vs "invalid email or password" (wrong password). An attacker could submit the correct password with no MFA code and receive a different error, revealing both that the password was correct and that the account has MFA enabled. All MFA-related failure paths in verifyPasswordAndMFA now return the same generic "invalid email or password" message: - missing MFA code (was: "MFA code required") - wrong MFA code (was: "invalid MFA code") - MFA enabled but secret not configured (was: "MFA is enabled but not configured") Internal debug/warn logs still distinguish the paths for operator diagnostics. Updated tests in service_test.go and service_lockout_test.go to expect the generic message. Added TestLogin_MFAErrorsAreGeneric as a dedicated regression test covering all three error paths.

coderabbitai · 2026-05-19T23:53:38Z

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 6c5e4d42-f827-46fb-8a1b-020c6a794fb7

📥 Commits

Reviewing files that changed from the base of the PR and between 84f0e52 and 39d6c9c.

📒 Files selected for processing (2)

internal/auth/service.go
internal/auth/service_lockout_test.go

📝 Walkthrough

Walkthrough

The PR unifies all MFA-related client-facing authentication failures to return "invalid email or password", updates internal logging, and adds/updates tests to ensure generic errors and correct failed-login and lockout behavior.

Changes

MFA Error Enumeration Vulnerability Fix

Layer / File(s)	Summary
MFA service logic and error handling `internal/auth/service.go`	Documented `verifyPasswordAndMFA` behavior and changed MFA failure paths (missing code, missing secret, invalid TOTP) to return the generic client error `"invalid email or password"` while emitting operator-facing debug/warn logs and preserving failed-login accounting.
MFA test assertions and regressions `internal/auth/service_test.go`, `internal/auth/service_lockout_test.go`	Updated MFA-related test assertions to expect the generic error for multiple failure scenarios and added regression tests (`TestLogin_MFAErrorsAreGeneric`, `TestLogin_AccountLockout_MFAMissingCodePath`) verifying generic errors, nil responses where appropriate, failed-login increments, and lockout at `MaxFailedLoginAttempts`.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

refactor(auth/mfa): promote substring-matched error messages in mapMFAServiceError to sentinel-typed errors #512: The change unifies MFA-related error text in internal/auth/service.go, which affects substring-based error mapping logic referenced in that issue.

Possibly related PRs

LeanerCloud/CUDly#485: Also changed backend MFA failure responses to the generic "invalid email or password", overlapping at the client-visible error mapping and tests.
LeanerCloud/CUDly#551: Makes related edits to authentication error generation/mapping in internal/auth/service.go, overlapping at unified login error behavior.

Poem

🐰 In burrows deep the secrets stay, errors now dressed in gray,
No clue to tell which gates are set, attackers led astray.
Logs still whisper to the keepers, while the outside stays mute,
A gentle hop for safety's sake — unified, quiet, astute. 🥕

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: collapsing MFA error paths to prevent enrollment status enumeration, which is the primary security fix in this PR.
Linked Issues check	✅ Passed	The PR implements the core requirement from `#388`: MFA-related authentication failures now return a generic error message to prevent status enumeration while preserving detailed logs.
Out of Scope Changes check	✅ Passed	All changes are directly related to fixing the MFA error enumeration vulnerability. No unrelated refactoring, dependencies, or out-of-scope modifications are present.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch sec/login-mfa-error-collapse

⚔️ Resolve merge conflicts

Resolve merge conflict in branch sec/login-mfa-error-collapse

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

cristim · 2026-05-19T23:53:45Z

@coderabbitai review

coderabbitai · 2026-05-19T23:53:49Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

cristim · 2026-05-19T23:54:47Z

@coderabbitai review

coderabbitai · 2026-05-19T23:54:51Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

cristim · 2026-05-20T07:56:09Z

@coderabbitai full review

coderabbitai · 2026-05-20T07:56:14Z

✅ Actions performed

Full review triggered.

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/auth/service.go`:
- Around line 191-203: The missing-MFA branches return a generic "invalid email
or password" without incrementing the failed-login counter; update the branches
that check req.MFACode == "" and user.MFASecret == "" to call
s.recordFailedLogin(ctx, user) (same as the invalid-MFA-code path) before
returning the generic error, preserving the existing logging and error text so
lockout counting is consistent for all MFA-related failures.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 321caa67-98fe-496c-8fc3-b5abb0e4f5dc

📥 Commits

Reviewing files that changed from the base of the PR and between b1ea4b1 and 84f0e52.

📒 Files selected for processing (3)

internal/auth/service.go
internal/auth/service_lockout_test.go
internal/auth/service_test.go

The missing-code and misconfigured-secret paths returned generic errors without calling recordFailedLogin, letting attackers avoid lockout by repeatedly omitting the MFA code. Add recordFailedLogin to both branches to match the invalid-code branch at line 207. Adds a regression test (TestLogin_AccountLockout_MFAMissingCodePath) that hammers the missing-code path and asserts lockout triggers after MaxFailedLoginAttempts. Fixes CodeRabbit finding on PR #532.

cristim · 2026-05-20T08:07:00Z

@coderabbitai review

coderabbitai · 2026-05-20T08:07:05Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

cristim · 2026-05-20T18:40:53Z

@coderabbitai resolve

coderabbitai · 2026-05-20T18:41:00Z

✅ Actions performed

Comments resolved. Approval is disabled; enable reviews.request_changes_workflow to allow explicit top-level @coderabbitai resolve or @coderabbitai approve commands.

cristim · 2026-05-20T23:26:39Z

Superseded — base branch already implements the MFA-enrollment-enumeration fix (#388) with a cleaner approach: ErrMFARequired sentinel + recordFailedLogin already wired into both the password-fail and invalid-MFA-code branches in internal/auth/service.go. This PR's diff is now empty after rebase.

cristim · 2026-05-21T12:26:30Z

@coderabbitai review

coderabbitai · 2026-05-21T12:26:35Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

cristim added priority/p2 Backlog-worthy severity/medium Moderate harm urgency/this-sprint Within the current sprint impact/many Affects most users effort/xs Trivial / one-liner type/security Security finding triaged Item has been triaged labels May 19, 2026

coderabbitai Bot reviewed May 20, 2026

View reviewed changes

Comment thread internal/auth/service.go

cristim closed this May 20, 2026

cristim deleted the sec/login-mfa-error-collapse branch June 3, 2026 21:55

Uh oh!

Conversation

cristim commented May 19, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review failed

Walkthrough

Changes

Estimated code review effort

Possibly related issues

Possibly related PRs

Poem

Uh oh!

cristim commented May 19, 2026

Uh oh!

coderabbitai Bot commented May 19, 2026

Uh oh!

cristim commented May 19, 2026

Uh oh!

coderabbitai Bot commented May 19, 2026

Uh oh!

cristim commented May 20, 2026

Uh oh!

coderabbitai Bot commented May 20, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cristim commented May 20, 2026

Uh oh!

coderabbitai Bot commented May 20, 2026

Uh oh!

cristim commented May 20, 2026

Uh oh!

coderabbitai Bot commented May 20, 2026

Uh oh!

cristim commented May 20, 2026

Uh oh!

cristim commented May 21, 2026

Uh oh!

coderabbitai Bot commented May 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

cristim commented May 19, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 19, 2026 •

edited

Loading