fix(auth): prevent email enumeration during auth flows #1540
Conversation
Improve security by not exposing whether an email exists during login and registration: - Update login flow to always proceed to verification page - Update registration flow to silently redirect to login if email exists - Send appropriate verification emails (login vs register) without exposing status - Update verification page text to be consistent and ambiguous - Remove explicit error messages that could expose user existence - Use generic email verification messaging throughout This prevents malicious users from discovering which email addresses are registered in the system while maintaining a smooth user experience.
|
@princesinghrajput is attempting to deploy a commit to the rallly Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis pull request focuses on refining the user authentication and email verification flow across multiple components. The changes primarily involve updating localization strings, modifying the login and registration processes, and improving the email verification user experience. The modifications simplify the authentication logic, remove explicit user existence checks, and provide more consistent messaging around email verification across different parts of the application. Changes
Sequence DiagramsequenceDiagram
participant User
participant LoginForm
participant AuthSystem
participant EmailService
User->>LoginForm: Enter email
LoginForm->>AuthSystem: setVerificationEmail(email)
AuthSystem->>EmailService: Send verification email
EmailService-->>User: Email with verification code
User->>LoginForm: Enter verification code
LoginForm->>AuthSystem: Verify code
AuthSystem-->>User: Authentication result
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/web/public/locales/en/app.json(2 hunks)apps/web/src/app/[locale]/(auth)/login/components/login-email-form.tsx(1 hunks)apps/web/src/app/[locale]/(auth)/login/verify/page.tsx(1 hunks)apps/web/src/app/[locale]/(auth)/register/components/register-name-form.tsx(2 hunks)apps/web/src/app/[locale]/(auth)/register/verify/page.tsx(2 hunks)apps/web/src/auth.ts(1 hunks)
🔇 Additional comments (7)
apps/web/src/app/[locale]/(auth)/login/verify/page.tsx (1)
40-40: LGTM! Security improvement in messaging.The updated message "If an account exists, a verification code will be sent to your email" effectively prevents email enumeration by not confirming whether an account exists, while maintaining clear communication with users.
apps/web/src/app/[locale]/(auth)/register/verify/page.tsx (2)
18-19: LGTM! Code simplification.Removing the unused locale parameter simplifies the code without affecting functionality.
33-34: LGTM! Consistent messaging across auth flows.The updated i18n keys and default text maintain consistent messaging across the application, which improves user experience while maintaining security.
Also applies to: 41-42
apps/web/src/app/[locale]/(auth)/login/components/login-email-form.tsx (1)
52-60: LGTM! Strong security improvement in login flow.The changes effectively prevent email enumeration by:
- Removing the user existence check
- Maintaining consistent behavior regardless of email status
- Using clear code comments to explain the security-focused approach
This implementation aligns perfectly with the PR's security objectives.
apps/web/src/app/[locale]/(auth)/register/components/register-name-form.tsx (2)
52-57: LGTM! Improved security in registration flow.The silent redirection of existing users to the login flow effectively prevents email enumeration while maintaining a smooth user experience.
53-57: Verify error handling for signIn and redirect.Please ensure proper error handling for the signIn operation and subsequent redirect.
apps/web/public/locales/en/app.json (1)
306-309: LGTM! The localization changes effectively support email enumeration prevention.The updated strings successfully maintain ambiguity about user existence while providing clear instructions to users.
- Add try-catch block around signIn call - Add error message translation for authentication failures - Only redirect to verify page on successful sign-in
|
Thanks for picking this up @princesinghrajput |
apps/web/src/app/[locale]/(auth)/login/components/login-email-form.tsx
Outdated
Show resolved
Hide resolved
| }); | ||
|
|
||
| if (signInResult?.error) { | ||
| throw new Error(signInResult.error); |
There was a problem hiding this comment.
Can we just form.setError() here rather than throwing and catching the error?
There was a problem hiding this comment.
@princesinghrajput looks like this is still unresolved?
apps/web/src/auth.ts
Outdated
| ); | ||
| }); | ||
| } else { | ||
| // Send registration email |
There was a problem hiding this comment.
If the user doesn't exist we shouldn't send any email. It is not desirable or possible to complete a registration from the login form.
|
There are some e2e tests that will need to be updated to get this merged. You can run them by: # Switch to web directory
cd apps/web
# Run the tests and filter so that only the authentication tests run
yarn test:integration authIf you're not planning on updating them let me know and I can plan to do that myself. |
…l enumeration review feedback
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/src/app/[locale]/(auth)/login/components/login-email-form.tsx(1 hunks)apps/web/src/app/[locale]/(auth)/register/components/register-name-form.tsx(2 hunks)apps/web/src/auth.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/src/auth.ts
- apps/web/src/app/[locale]/(auth)/register/components/register-name-form.tsx
|
Thanks for the review @lukevella! I've implemented all your suggestions:
The changes maintain the security goal of preventing email enumeration while properly handling the authentication flow. Let me know if you'd like me to update the e2e tests as well. |
| "requiredString": "“{name}” is required", | ||
| "requiredString": "{name} is required", |
There was a problem hiding this comment.
I don't think this change is necessary or relevant to this PR. Can you please revert this?
| }); | ||
|
|
||
| if (signInResult?.error) { | ||
| throw new Error(signInResult.error); |
There was a problem hiding this comment.
@princesinghrajput looks like this is still unresolved?
| locale: true, | ||
| }, | ||
| }); | ||
| try { |
There was a problem hiding this comment.
I think this changes in this file should be reverted because:
- There's no point in adding a
try..catchif we are throwing the error in the catch statement. - Sending a registration email from the login form won't allow the user to register a new account. It can only be done from the registration form.
Description
This PR improves security by preventing email enumeration during authentication flows. Currently, the system exposes whether an email is registered through explicit error messages and different UI flows, which could be exploited by malicious users to discover valid user accounts.
Key Changes:
Login Flow
Registration Flow
UI/UX Improvements
Security Impact
This change prevents attackers from using the authentication flows to build lists of valid user accounts, which could be used for:
The improved flow maintains the same user experience while removing information disclosure vulnerabilities.
Testing
The changes have been tested for:
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation