fix(auth): prevent email enumeration via resend-verification endpoint (#1414) - #1742
Open
saidai-bhuvanesh wants to merge 1 commit into
Conversation
…-Labs#1414) The resend-verification endpoint returned distinguishable responses for unknown emails vs already-verified accounts, letting an attacker enumerate registered and verified emails. All non-validation outcomes now return the same generic 200 message; a link is only sent for existing unverified users. Co-authored-by: openhands <openhands@all-hands.dev>
📝 WalkthroughWalkthroughThe resend-verification endpoint now returns a generic success response for nonexistent, verified, and unverified accounts. Tests verify response uniformity, conditional email delivery, token persistence, and input validation. ChangesVerification email enumeration protection
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| function buildApp() { | ||
| const app = express(); | ||
| app.use(express.json()); | ||
| app.post("/api/auth/resend-verification", resendVerificationEmail); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/controllers/authController.js (1)
369-388: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winKeep the response generic when account-specific resend work fails.
At Line 383, a rejected
user.save()reaches the outer catch and returns 500. Only an existing unverified account reaches this write. During a write failure, this restores an account-state response difference.
backend/controllers/authController.js#L369-L388: Catch token persistence and delivery failures in the unverified-user branch. Log the failure. ReturngenericMessage.backend/tests/authController.resendVerification.enumeration.unit.test.js#L45-L107: Mockuser.save()to reject. Assert the endpoint still returns the generic 200 response.Proposed controller update
- user.emailVerificationToken = crypto.randomBytes(32).toString("hex"); - user.emailVerificationExpires = new Date(Date.now() + 24 * 60 * 60 * 1000); - await user.save(); - - const verificationUrl = `${process.env.FRONTEND_URL}/verify-email?token=${user.emailVerificationToken}`; - await sendVerificationEmail(user.email, verificationUrl); - - res.json({ success: true, message: genericMessage }); + try { + user.emailVerificationToken = crypto.randomBytes(32).toString("hex"); + user.emailVerificationExpires = new Date(Date.now() + 24 * 60 * 60 * 1000); + await user.save(); + + const verificationUrl = `${process.env.FRONTEND_URL}/verify-email?token=${user.emailVerificationToken}`; + await sendVerificationEmail(user.email, verificationUrl); + } catch (error) { + console.error("Resend verification work failed:", error); + } + + return res.json({ success: true, message: genericMessage });🤖 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 `@backend/controllers/authController.js` around lines 369 - 388, Wrap the unverified-user token persistence and email delivery flow in the resend-verification handler with failure handling that logs the error and returns the existing genericMessage with a successful response instead of reaching the outer 500 handler. In backend/controllers/authController.js lines 369-388, update the branch after the !user/user.isEmailVerified check; in backend/tests/authController.resendVerification.enumeration.unit.test.js lines 45-107, mock user.save() to reject and assert the endpoint returns status 200 with genericMessage.
🤖 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 `@backend/tests/authController.resendVerification.enumeration.unit.test.js`:
- Around line 68-80: Strengthen the resend-verification test around the
user.save mock by capturing the token and expiry values persisted by the
handler. Assert that the saved token is a newly generated 64-character
hexadecimal value and that the saved expiry is later than the current time,
while retaining the existing response and email assertions.
---
Outside diff comments:
In `@backend/controllers/authController.js`:
- Around line 369-388: Wrap the unverified-user token persistence and email
delivery flow in the resend-verification handler with failure handling that logs
the error and returns the existing genericMessage with a successful response
instead of reaching the outer 500 handler. In
backend/controllers/authController.js lines 369-388, update the branch after the
!user/user.isEmailVerified check; in
backend/tests/authController.resendVerification.enumeration.unit.test.js lines
45-107, mock user.save() to reject and assert the endpoint returns status 200
with genericMessage.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 378c52bd-e40e-4e0f-81f5-8dba04620a05
📒 Files selected for processing (2)
backend/controllers/authController.jsbackend/tests/authController.resendVerification.enumeration.unit.test.js
Comment on lines
+68
to
+80
| it("returns the SAME generic message for an unverified user but actually sends the email", async () => { | ||
| const user = fakeUser({ isEmailVerified: false }); | ||
| User.findOne.mockResolvedValue(user); | ||
| const res = await request(buildApp()) | ||
| .post("/api/auth/resend-verification") | ||
| .send({ email: "Victim@Example.com" }); | ||
|
|
||
| expect(res.status).toBe(200); | ||
| expect(res.body.success).toBe(true); | ||
| expect(res.body.message).toBe(GENERIC_MESSAGE); | ||
| // A fresh token must be issued and the verification email actually sent. | ||
| expect(user.save).toHaveBeenCalled(); | ||
| expect(sendMail).toHaveBeenCalledTimes(1); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the values persisted by user.save.
The test passes if the handler saves the existing "old" token. Capture the token and expiry inside the user.save mock. Assert that the saved token is a new 64-character hex value and that the saved expiry is in the future.
Proposed test update
it("returns the SAME generic message for an unverified user but actually sends the email", async () => {
const user = fakeUser({ isEmailVerified: false });
+ let savedToken;
+ let savedExpiry;
+ user.save.mockImplementation(async () => {
+ savedToken = user.emailVerificationToken;
+ savedExpiry = user.emailVerificationExpires;
+ });
User.findOne.mockResolvedValue(user);
const res = await request(buildApp())
.post("/api/auth/resend-verification")
.send({ email: "Victim@Example.com" });
expect(res.status).toBe(200);
expect(res.body.success).toBe(true);
expect(res.body.message).toBe(GENERIC_MESSAGE);
- // A fresh token must be issued and the verification email actually sent.
expect(user.save).toHaveBeenCalled();
+ expect(savedToken).not.toBe("old");
+ expect(savedToken).toMatch(/^[0-9a-f]{64}$/);
+ expect(savedExpiry.getTime()).toBeGreaterThan(Date.now());
expect(sendMail).toHaveBeenCalledTimes(1);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it("returns the SAME generic message for an unverified user but actually sends the email", async () => { | |
| const user = fakeUser({ isEmailVerified: false }); | |
| User.findOne.mockResolvedValue(user); | |
| const res = await request(buildApp()) | |
| .post("/api/auth/resend-verification") | |
| .send({ email: "Victim@Example.com" }); | |
| expect(res.status).toBe(200); | |
| expect(res.body.success).toBe(true); | |
| expect(res.body.message).toBe(GENERIC_MESSAGE); | |
| // A fresh token must be issued and the verification email actually sent. | |
| expect(user.save).toHaveBeenCalled(); | |
| expect(sendMail).toHaveBeenCalledTimes(1); | |
| it("returns the SAME generic message for an unverified user but actually sends the email", async () => { | |
| const user = fakeUser({ isEmailVerified: false }); | |
| let savedToken; | |
| let savedExpiry; | |
| user.save.mockImplementation(async () => { | |
| savedToken = user.emailVerificationToken; | |
| savedExpiry = user.emailVerificationExpires; | |
| }); | |
| User.findOne.mockResolvedValue(user); | |
| const res = await request(buildApp()) | |
| .post("/api/auth/resend-verification") | |
| .send({ email: "Victim@Example.com" }); | |
| expect(res.status).toBe(200); | |
| expect(res.body.success).toBe(true); | |
| expect(res.body.message).toBe(GENERIC_MESSAGE); | |
| expect(user.save).toHaveBeenCalled(); | |
| expect(savedToken).not.toBe("old"); | |
| expect(savedToken).toMatch(/^[0-9a-f]{64}$/); | |
| expect(savedExpiry.getTime()).toBeGreaterThan(Date.now()); | |
| expect(sendMail).toHaveBeenCalledTimes(1); |
🤖 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 `@backend/tests/authController.resendVerification.enumeration.unit.test.js`
around lines 68 - 80, Strengthen the resend-verification test around the
user.save mock by capturing the token and expiry values persisted by the
handler. Assert that the saved token is a newly generated 64-character
hexadecimal value and that the saved expiry is later than the current time,
while retaining the existing response and email assertions.
|
@saidai-bhuvanesh, please resolve the commit so that it will be merged soon ...... |
github-actions
Bot
added
merge conflicts
Contributor
|
@saidai-bhuvanesh resolve the conflicts |
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
4 participants
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
This PR fixes the email enumeration vulnerability reported in #1414 in the
/api/auth/resend-verificationendpoint. Previously, the endpoint returned distinguishable responses depending on the state of the supplied email address, which allowed an attacker to infer whether an email was registered and whether it was already verified — a classic account-enumeration vector that can be leveraged for targeted phishing, credential stuffing, and user-base scraping.Before (vulnerable behavior)
200 { success: true, message: "If this email is registered, a verification link has been sent." }400 { success: false, message: "This email is already verified. Please log in." }200 { success: true, message: "Verification email resent. Please check your inbox." }An attacker could probe any email address and tell, from the status code and message alone, whether the account existed and whether it had completed verification — all without authentication or rate limiting on the signal.
After (fixed behavior)
All non-validation outcomes now return the identical response:
This single, indistinguishable response is returned for unknown emails, already-verified emails, and successfully re-sent verification emails. A verification link is only actually generated and dispatched when the account exists and is still unverified, so legitimate users are unaffected. The empty-email validation (
400) is preserved.Changes
backend/controllers/authController.js: collapsed the not-found, already-verified, and success branches into one generic200response with a unified message; a fresh token + email dispatch now happens only for existing unverified accounts.backend/tests/authController.resendVerification.enumeration.unit.test.js: new unit test suite (5 cases) asserting that not-found and already-verified produce byte-identical responses, that the unverified path still sends the email, and that empty-email validation remains400.Test plan
npx vitest run tests/authController.resendVerification.enumeration.unit.test.js— 5/5 passingnpx vitest run— 132/132 passing (13 files)Closes #1414.
Looks good to me. Ready to merge.