fix(security): auth enumeration + prod error message leak#3790
Merged
Conversation
- forgot endpoint: uniform response for known/unknown/OAuth email paths + dummy bcrypt.compare on unknown-user path to equalise timing - initErrorRoutes: sanitise err.message/code in production (return generic 'Internal Server Error'); detailed body kept in non-prod - Unit tests: 4 for auth.password.controller + 3 for express.errorroutes Closes #3722 Closes #3726
|
Warning Review limit reached
More reviews will be available in 11 minutes and 2 seconds. 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughThis PR hardens two security findings from audit ChangesSecurity Hardening: Error Handler and Auth Enumeration Fixes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3790 +/- ##
==========================================
+ Coverage 90.19% 90.23% +0.03%
==========================================
Files 152 152
Lines 5020 5026 +6
Branches 1598 1599 +1
==========================================
+ Hits 4528 4535 +7
+ Misses 387 386 -1
Partials 105 105
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR addresses two audit findings by (1) hardening the auth “forgot password” flow against account/provider enumeration and timing signals, and (2) preventing the global Express error handler from leaking internal error details to clients in production.
Changes:
- Auth:
forgotnow returns a uniform success message for unknown emails and non-local (OAuth) users and runs a dummy bcrypt compare on those paths to reduce timing leakage. - Core:
initErrorRoutesnow returns a generic{ message: 'Internal Server Error' }body inNODE_ENV=production, while keeping detailed error bodies in non-prod. - Tests: adds new unit coverage for the auth forgot handler and for the production/non-production behavior of
initErrorRoutes.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| modules/auth/controllers/auth.password.controller.js | Makes forgot return a uniform success response for unknown/OAuth users and adds a dummy bcrypt compare to reduce timing signals. |
| modules/auth/tests/auth.password.controller.unit.tests.js | Adds unit tests guarding against enumeration and verifying the dummy bcrypt path. |
| lib/services/express.js | Adds a production-only guard to avoid leaking err.message/err.code to clients. |
| lib/services/tests/express.errorroutes.unit.tests.js | Adds unit tests verifying prod returns a generic error body and non-prod keeps detailed fields. |
Comment on lines
+28
to
+30
| // Uniform success message — never reveal whether an email is registered (anti-enumeration). | ||
| const uniformSuccess = () => responses.success(res, 'If that email exists, a reset link has been sent.')({ status: true }); | ||
|
|
Comment on lines
77
to
81
| if (!mail || !mail.accepted) return responses.error(res, 400, 'Bad Request', 'Failure sending email')(); | ||
| return responses.success(res, 'An email has been sent with further instructions')({ status: true }); | ||
| return uniformSuccess(); | ||
| } catch (_mailErr) { | ||
| return responses.error(res, 400, 'Bad Request', 'Failure sending email')(); | ||
| } |
Comment on lines
+94
to
+95
| // Must call responses.success (uniform positive signal) | ||
| expect(responses.success).toHaveBeenCalled(); |
|
|
||
| await PasswordController.forgot(req, res); | ||
|
|
||
| expect(responses.success).toHaveBeenCalled(); |
Comment on lines
+164
to
+165
| // Must call responses.success (uniform positive signal) | ||
| expect(responses.success).toHaveBeenCalled(); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/auth/controllers/auth.password.controller.js (1)
27-82:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate /api/auth/forgot integration-test expectations for uniform success
- In
modules/auth/tests/auth.integration.tests.js, change the two “should return 400” cases for (1) non-existent email and (2) non-local provider to.expect(200)and assertresult.body.message === 'If that email exists, a reset link has been sent.'(andresult.body.data.status === true).🤖 Prompt for 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. In `@modules/auth/controllers/auth.password.controller.js` around lines 27 - 82, Update the two integration tests that currently expect 400 for the "non-existent email" and "non-local provider" cases in auth.integration.tests.js to expect 200 instead, and add assertions that result.body.message === 'If that email exists, a reset link has been sent.' and result.body.data.status === true; locate the tests by their descriptions ("should return 400" for non-existent email and for OAuth/non-local provider) and change .expect(400) to .expect(200) and add the two equality checks on result.body.message and result.body.data.status.
🤖 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 `@lib/services/express.js`:
- Around line 326-333: Add a JSDoc header above the express error-handling
middleware callback to satisfy the JS guideline: include a one-line description
and `@param` entries for (err, req, res, next) with types (e.g., Error, Request,
Response, NextFunction) and an `@returns` describing the possible return (e.g.,
{Response|void} or express Response when sending an error); apply this to the
anonymous middleware function defined in app.use((err, req, res, next) => { ...
}) so the modified function has the required JSDoc contract.
In `@lib/services/tests/express.errorroutes.unit.tests.js`:
- Around line 21-24: Add a complete JSDoc header above the named async helper
getInitErrorRoutes describing its purpose in one line and include an `@returns`
tag documenting the resolved value (the extracted initErrorRoutes function
wrapped in a Promise); for example document it as returning Promise<Function>
(or a more specific function type if available) so callers know the async helper
resolves to the initErrorRoutes callable.
In `@modules/auth/controllers/auth.password.controller.js`:
- Around line 77-81: The current branch in auth.password.controller.js returns
responses.error(...) when mail sending fails (the if-check using mail.accepted
and the catch block catching _mailErr), which re-introduces account enumeration;
change both failure paths to return uniformSuccess() instead, and record the
underlying error server-side via the existing logging mechanism (e.g., use
processLogger.error or console.error) to log _mailErr and any mail response
details for ops. Ensure you update the if block that currently does "if (!mail
|| !mail.accepted) return responses.error(...)" and the catch that returns
responses.error(...) so both return uniformSuccess() while still logging the
failure.
In `@modules/auth/tests/auth.password.controller.unit.tests.js`:
- Around line 115-167: Add a unit test in
modules/auth/tests/auth.password.controller.unit.tests.js that simulates
mail-send failure and asserts the controller still returns the uniform success:
arrange a user with provider 'local', set mockGetBrut to resolve that user, set
mockUpdate to resolve the updated user, and set mockSendMail to simulate failure
(either mockResolvedValueOnce({ accepted: [] }) or mockRejectedValueOnce(...));
import PasswordController and responses, call PasswordController.forgot(req,
res) with the user's email, then assert responses.success was called and
responses.error was not called (and/or no call contains provider info),
referencing the existing mocks mockSendMail, mockGetBrut, mockUpdate and the
PasswordController.forgot and responses.success/error symbols.
---
Outside diff comments:
In `@modules/auth/controllers/auth.password.controller.js`:
- Around line 27-82: Update the two integration tests that currently expect 400
for the "non-existent email" and "non-local provider" cases in
auth.integration.tests.js to expect 200 instead, and add assertions that
result.body.message === 'If that email exists, a reset link has been sent.' and
result.body.data.status === true; locate the tests by their descriptions
("should return 400" for non-existent email and for OAuth/non-local provider)
and change .expect(400) to .expect(200) and add the two equality checks on
result.body.message and result.body.data.status.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4be0a2ee-8c08-400a-a07b-caf9515c7928
📒 Files selected for processing (4)
lib/services/express.jslib/services/tests/express.errorroutes.unit.tests.jsmodules/auth/controllers/auth.password.controller.jsmodules/auth/tests/auth.password.controller.unit.tests.js
…eration tests - auth.password.controller: mail failure now logs + returns uniformSuccess() instead of 400, closing enumeration oracle on SMTP errors (#3722) - auth.integration.tests: update all forgot-flow assertions to uniform 200 (unknown email, OAuth provider, mail failure, password-reset setup calls) - auth.password.controller.unit.tests: strengthen to exact UNIFORM_MSG; add tests for sendMail throws and sendMail !accepted paths - express.js initErrorRoutes: add JSDoc on function and 4-arity handler (#3726) - express.errorroutes.unit.tests: add @returns to getInitErrorRoutes JSDoc
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
PierreBrisorgueil
dismissed
coderabbitai[bot]’s stale review
Stale review from pre-fix commit; all 4 actionable comments addressed in 559140f, CI green + Snyk pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
[audit][P1][security] auth: forgot endpoint allows account enumeration + timing oracle #3722 — auth: account enumeration + timing oracle (
modules/auth/controllers/auth.password.controller.js)forgotendpoint now returns a single uniform success response ("If that email exists, a reset link has been sent.") for all paths: unknown email, OAuth user, and known local user — no longer leaks whether an address is registeredbcrypt.compareagainst a precomputed valid 60-char bcrypt hash (cost=10) so the full KDF runs and equalises response timing[audit][P1][security] core: initErrorRoutes leaks err.message/code to clients in production #3726 — core: initErrorRoutes leaks err.message/code in prod (
lib/services/express.js)NODE_ENV === 'production'guard in the global error handler: returns{ message: 'Internal Server Error' }in prod (no err.message/code); detailed body preserved in non-prod envs for debuggabilityTests: 4 new unit tests for
auth.password.controller.forgot+ 3 new unit tests forexpress.initErrorRoutes; all 1731 unit tests green, lint cleanTest plan
npm run test:unit— 1731 tests, 126 suites, all greennpm run lint— no issues/api/auth/password/forgotwith unknown email → same 200 response as known email, no enumeration signal{ message: 'Internal Server Error' }, not raw error detailCloses #3722
Closes #3726
Summary by CodeRabbit
Bug Fixes
Tests