sec(auth): stop leaking MFA enrollment via login error responses (closes #388)#886
sec(auth): stop leaking MFA enrollment via login error responses (closes #388)#886cristim wants to merge 1 commit into
Conversation
#388) The login handler previously forwarded err.Error() verbatim for any auth failure not matching ErrMFARequired or ErrInvalidMFACode. This included "MFA is enabled but not configured", which revealed to an attacker that credentials were correct and MFA was enrolled (but broken). Two attack vectors were closed: 1. Add ErrMFANotConfigured sentinel to internal/auth/errors.go and return it (wrapped via %w) from verifyPasswordAndMFA. Map it to "mfa_required" in the login handler -- identical to ErrMFARequired -- so "MFA enrolled + working" is indistinguishable from "MFA enrolled + broken secret" in the HTTP response. 2. Replace the bare err.Error() fallthrough in the login handler with a fixed "invalid credentials" string so no internal error message ever reaches the client. Tests positively assert response equivalence: ErrMFARequired and ErrMFANotConfigured produce the same 401 + "mfa_required" body; all wrong-password paths produce the same 401 + "invalid credentials" body.
|
@coderabbitai review |
|
Warning Review limit reached
More reviews will be available in 37 minutes. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
✅ Actions performedReview triggered.
|
Summary
ErrMFANotConfiguredsentinel tointernal/auth/errors.goand wrap it inverifyPasswordAndMFA. Map it to"mfa_required"in the login handler (same asErrMFARequired) so an attacker cannot distinguish "MFA enrolled and working" from "MFA enrolled but secret missing" via the 401 response.err.Error()fallthrough in the login handler with a fixed"invalid credentials"string so no internal error message ever reaches the client.ErrMFANotConfiguredmessage string is identical to the pre-sentinel literal, somapMFAServiceError's substring fallback still fires correctly until refactor(auth/mfa): use sentinel errors in mapMFAServiceError #883 lands.Test plan
TestLogin_FailedAuth_ResponseEquivalencepositively asserts that wrong-password for a no-MFA user and wrong-password for an MFA-enrolled user both return HTTP 401 +"invalid credentials"-- identical bodies.TestLogin_MFANotConfigured_ReturnsMFARequiredassertsErrMFANotConfiguredproduces 401 +"mfa_required", not the internal error string.TestLogin_MFARequired_And_NotConfigured_ProduceSameResponsecross-checksErrMFARequiredandErrMFANotConfiguredproduce the exact same status code and response body.TestLogin_WithMFA_NoSecret(auth package) updated to useerrors.Is(err, ErrMFANotConfigured)-- a stronger assertion thanContains.go test ./internal/auth/... ./internal/api/...-- 1857 tests pass.