feat(auth): add email verification using OTP authentication#566
feat(auth): add email verification using OTP authentication#566adityayadav176 wants to merge 6 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 52 minutes and 36 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ 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 ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR introduces email-based OTP verification for user account validation. The backend generates and validates 6-digit OTPs via Nodemailer, updates the User schema with verification fields, and exposes two new protected endpoints. The frontend provides pages to request and verify OTPs, with ProfilePage triggering the flow for unverified users. ChangesEmail OTP Verification Flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
H! @UTKARSHH20 Successfully implemented and completed this feature. The email verification system using OTP has been added and thoroughly tested. Features Implemented
Kindly review and merge this PR. Please add the appropriate GSSoC labels, including Critical or Advanced, if applicable. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
frontend/src/App.jsx (1)
54-54: ⚡ Quick winUse one canonical casing for the verify-email route path.
Line 54 uses
/verify-Email, while navigation uses/verify-email. Standardizing to one exact path avoids brittle links and confusion.🔧 Suggested fix
- <Route path="/verify-Email" element={authUser ? <VerifyEmail /> : <Navigate to="/login" />} /> + <Route path="/verify-email" element={authUser ? <VerifyEmail /> : <Navigate to="/login" />} />🤖 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 `@frontend/src/App.jsx` at line 54, The route path in App.jsx uses inconsistent casing "/verify-Email" which conflicts with other navigation that uses "/verify-email"; update the Route declaration in the App component (the <Route ... /> that renders VerifyEmail) to use the canonical lowercase path "/verify-email" and search for any references to "/verify-Email" (links, Navigate, useNavigate calls, NavLink, router redirects) and normalize them to "/verify-email" so all route declarations and navigations match exactly.
🤖 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/src/controllers/auth.controller.js`:
- Around line 301-303: Replace the insecure Math.random-based OTP with a
cryptographically secure randomInt call: import or require node:crypto (e.g.,
import { randomInt } from 'crypto' or const { randomInt } = require('crypto')),
then change the otp assignment (the otp variable in auth.controller.js) to const
otp = randomInt(100000, 1000000).toString(); so the OTP remains a 6-digit string
generated securely.
- Around line 330-352: In verifyOtp, add strict validation to prevent marking
accounts verified when OTP is missing: first validate req.body.otp exists and is
the expected type/format (e.g., non-empty string or numeric string) and return
400 if not; then ensure the stored fields user.otp and user.otpExpires are
present and that user.otpExpires is a future timestamp before doing any
comparison; if stored OTP or expiry is missing or expired return 400; finally
perform a strict comparison between the sanitized input OTP and user.otp and
only proceed to mark verified when both checks pass. Target the verifyOtp
function and the user.otp / user.otpExpires checks for these changes.
In `@backend/src/models/user.model.js`:
- Around line 44-45: The OTP fields otp and otpExpires in the user schema are
currently selectable and store raw OTPs; change the schema to remove raw otp and
instead store a non-selectable derived value (e.g., otpHash: String with select:
false) and keep otpExpires as select: false (or rename to otpExpires and set
select: false) so these values are not returned by default; update any code that
sets/checks OTPs (e.g., functions that create OTPs and verify them) to hash the
generated OTP into otpHash when saving and to compare hashes when verifying,
rather than storing or comparing the raw otp field.
In `@frontend/pages/SendVerifyEmailOtp.jsx`:
- Around line 13-17: The axios.post in SendVerifyEmailOtp.jsx currently
hardcodes "http://localhost:5001" which will break non-local deployments; update
the request to use a deployable API base (e.g. a NEXT_PUBLIC_API_BASE or
runtime-config/env var or a relative path) instead of the literal origin, keep
the same path "/api/auth/send-verification-email-otp" and existing options ({
withCredentials: true }), and mirror the same pattern used in VerifyEmail.jsx so
both pages use the shared API base configuration rather than localhost.
In `@frontend/pages/VerifyEmail.jsx`:
- Around line 41-56: handleResendOtp currently allows multiple concurrent
requests; add an in-flight guard state (e.g., isResending) and check it at the
top of handleResendOtp to return early if true, set isResending = true before
the axios.post and reset it in a finally block so it always clears; also disable
the resend button in the JSX when isResending is true and (optionally) show a
spinner or change label to indicate progress to prevent OTP churn and mail spam
(apply same guard to other resend handlers around lines ~95-100).
---
Nitpick comments:
In `@frontend/src/App.jsx`:
- Line 54: The route path in App.jsx uses inconsistent casing "/verify-Email"
which conflicts with other navigation that uses "/verify-email"; update the
Route declaration in the App component (the <Route ... /> that renders
VerifyEmail) to use the canonical lowercase path "/verify-email" and search for
any references to "/verify-Email" (links, Navigate, useNavigate calls, NavLink,
router redirects) and normalize them to "/verify-email" so all route
declarations and navigations match exactly.
🪄 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: 909c0e87-8b0f-415a-a137-2069bc8c18ac
⛔ Files ignored due to path filters (1)
backend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
backend/.env.examplebackend/package.jsonbackend/src/controllers/auth.controller.jsbackend/src/models/user.model.jsbackend/src/routes/auth.route.jsbackend/src/utils/nodemailer.jsfrontend/pages/ProfilePage.jsxfrontend/pages/SendVerifyEmailOtp.jsxfrontend/pages/VerifyEmail.jsxfrontend/src/App.jsx
| const otp = Math.floor( | ||
| 100000 + Math.random() * 900000 | ||
| ).toString(); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
According to official Node.js documentation, is Math.random() cryptographically secure for OTP generation? What API is recommended instead for secure random integer generation?
💡 Result:
According to official documentation and industry standards, Math.random is not cryptographically secure and must not be used for security-sensitive operations such as OTP generation [1][2]. It is designed as a pseudo-random number generator that is not intended for cryptographic purposes [1]. For secure random integer generation in Node.js, the node:crypto module provides the recommended API, specifically crypto.randomInt(min, max) [3][4]. This function generates a cryptographically secure random integer [3][4]. Alternatively, for broader cryptographic requirements, crypto.getRandomValues() (part of the Web Crypto API) is also available and recommended for generating cryptographically strong random values [5][6][7].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/random
- 2: https://stackoverflow.com/questions/5651789/is-math-random-cryptographically-secure
- 3: https://beta.docs.nodejs.org/crypto.html
- 4: https://dev.to/mahendra_singh_7500/generating-a-secure-6-digit-otp-in-javascript-and-nodejs-2nbo
- 5: https://nodejs.org/docs/latest-v23.x/api/webcrypto.html
- 6: https://nodejs.org/api/webcrypto.html
- 7: https://dev.to/pavkode/mathrandom-non-compliant-with-nist-800-63b-adopt-cryptographically-secure-random-number-1ibh
Replace Math.random() OTP generation with crypto.randomInt()
Math.random() is not cryptographically secure and must not be used for security-sensitive OTPs. Use node:crypto (e.g., crypto.randomInt(100000, 1000000)) to generate the OTP.
File: backend/src/controllers/auth.controller.js (lines 301-303)
const otp = Math.floor(
100000 + Math.random() * 900000
).toString();🤖 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/src/controllers/auth.controller.js` around lines 301 - 303, Replace
the insecure Math.random-based OTP with a cryptographically secure randomInt
call: import or require node:crypto (e.g., import { randomInt } from 'crypto' or
const { randomInt } = require('crypto')), then change the otp assignment (the
otp variable in auth.controller.js) to const otp = randomInt(100000,
1000000).toString(); so the OTP remains a 6-digit string generated securely.
| export const verifyOtp = async (req, res) => { | ||
| try { | ||
| const { otp } = req.body; | ||
|
|
||
| const user = await User.findById(req.userId); | ||
|
|
||
| if (!user) { | ||
| return res.status(404).json({ | ||
| message: "User not found", | ||
| }); | ||
| } | ||
|
|
||
| if (user.otp !== otp) { | ||
| return res.status(400).json({ | ||
| message: "Invalid OTP", | ||
| }); | ||
| } | ||
|
|
||
| if (user.otpExpires < Date.now()) { | ||
| return res.status(400).json({ | ||
| message: "OTP expired", | ||
| }); | ||
| } |
There was a problem hiding this comment.
verifyOtp can mark accounts verified without a valid OTP.
Current checks allow a bypass when OTP is missing/unset (e.g., omitted body OTP + unset stored OTP/expiry). Add strict validation for input format and stored OTP state before any compare.
Proposed fix
export const verifyOtp = async (req, res) => {
try {
- const { otp } = req.body;
+ const otp = String(req.body?.otp ?? "").trim();
+ if (!/^\d{6}$/.test(otp)) {
+ return res.status(400).json({ message: "Please provide a valid 6-digit OTP" });
+ }
const user = await User.findById(req.userId);
if (!user) {
return res.status(404).json({
message: "User not found",
});
}
+
+ if (!user.otp || !user.otpExpires) {
+ return res.status(400).json({ message: "No active OTP. Please request a new OTP." });
+ }
+
+ if (user.otpExpires < Date.now()) {
+ return res.status(400).json({ message: "OTP expired" });
+ }
if (user.otp !== otp) {
return res.status(400).json({
message: "Invalid OTP",
});
}
-
- if (user.otpExpires < Date.now()) {
- return res.status(400).json({
- message: "OTP expired",
- });
- }🤖 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/src/controllers/auth.controller.js` around lines 330 - 352, In
verifyOtp, add strict validation to prevent marking accounts verified when OTP
is missing: first validate req.body.otp exists and is the expected type/format
(e.g., non-empty string or numeric string) and return 400 if not; then ensure
the stored fields user.otp and user.otpExpires are present and that
user.otpExpires is a future timestamp before doing any comparison; if stored OTP
or expiry is missing or expired return 400; finally perform a strict comparison
between the sanitized input OTP and user.otp and only proceed to mark verified
when both checks pass. Target the verifyOtp function and the user.otp /
user.otpExpires checks for these changes.
| otp: String, | ||
| otpExpires: Date |
There was a problem hiding this comment.
OTP fields are modeled as normal readable fields, which leaks verification secrets.
Because these fields are selectable by default, responses like checkAuth can expose the OTP/expiry to the client, bypassing the email-verification trust model. Mark OTP fields as non-selectable and store only a derived value (hash), not raw OTP text.
Suggested direction
- otp: String,
- otpExpires: Date
+ otpHash: {
+ type: String,
+ default: null,
+ select: false
+ },
+ otpExpires: {
+ type: Date,
+ default: null,
+ select: false
+ }🤖 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/src/models/user.model.js` around lines 44 - 45, The OTP fields otp
and otpExpires in the user schema are currently selectable and store raw OTPs;
change the schema to remove raw otp and instead store a non-selectable derived
value (e.g., otpHash: String with select: false) and keep otpExpires as select:
false (or rename to otpExpires and set select: false) so these values are not
returned by default; update any code that sets/checks OTPs (e.g., functions that
create OTPs and verify them) to hash the generated OTP into otpHash when saving
and to compare hashes when verifying, rather than storing or comparing the raw
otp field.
| const res = await axios.post( | ||
| "http://localhost:5001/api/auth/send-verification-email-otp", | ||
| {}, | ||
| { withCredentials: true } | ||
| ); |
There was a problem hiding this comment.
Hardcoded localhost API origin will break non-local deployments.
Line 14 pins requests to http://localhost:5001, which fails outside local dev and bypasses deploy-time API configuration.
🔧 Suggested fix
+const API_BASE = import.meta.env.VITE_API_BASE_URL ?? "";
+
const res = await axios.post(
- "http://localhost:5001/api/auth/send-verification-email-otp",
+ `${API_BASE}/api/auth/send-verification-email-otp`,
{},
{ withCredentials: true }
);Apply the same pattern to frontend/pages/VerifyEmail.jsx endpoints.
📝 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.
| const res = await axios.post( | |
| "http://localhost:5001/api/auth/send-verification-email-otp", | |
| {}, | |
| { withCredentials: true } | |
| ); | |
| const API_BASE = import.meta.env.VITE_API_BASE_URL ?? ""; | |
| const res = await axios.post( | |
| `${API_BASE}/api/auth/send-verification-email-otp`, | |
| {}, | |
| { withCredentials: 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 `@frontend/pages/SendVerifyEmailOtp.jsx` around lines 13 - 17, The axios.post
in SendVerifyEmailOtp.jsx currently hardcodes "http://localhost:5001" which will
break non-local deployments; update the request to use a deployable API base
(e.g. a NEXT_PUBLIC_API_BASE or runtime-config/env var or a relative path)
instead of the literal origin, keep the same path
"/api/auth/send-verification-email-otp" and existing options ({ withCredentials:
true }), and mirror the same pattern used in VerifyEmail.jsx so both pages use
the shared API base configuration rather than localhost.
| const handleResendOtp = async () => { | ||
| try { | ||
| const res = await axios.post( | ||
| "http://localhost:5001/api/auth/send-verification-email-otp", | ||
| {}, | ||
| { withCredentials: true } | ||
| ); | ||
|
|
||
| toast.success(res.data.message); | ||
| } catch (error) { | ||
| toast.error( | ||
| error.response?.data?.message || | ||
| "Failed to resend OTP" | ||
| ) | ||
| } | ||
| }; |
There was a problem hiding this comment.
Guard resend requests to prevent OTP churn and mail spam.
Resend currently has no in-flight guard, so repeated clicks can trigger multiple OTP generations and invalidate the code the user just entered.
🔧 Suggested fix
const [loading, setLoading] = useState(false);
+ const [resending, setResending] = useState(false);
@@
const handleResendOtp = async () => {
+ if (resending) return;
try {
+ setResending(true);
const res = await axios.post(
- "http://localhost:5001/api/auth/send-verification-email-otp",
+ `${API_BASE}/api/auth/send-verification-email-otp`,
{},
{ withCredentials: true }
);
@@
} catch (error) {
toast.error(
error.response?.data?.message ||
"Failed to resend OTP"
)
+ } finally {
+ setResending(false);
}
};
@@
<button
onClick={handleResendOtp}
+ disabled={resending || loading}
className="btn btn-outline w-full mt-3"
>
- Resend OTP
+ {resending ? "Resending..." : "Resend OTP"}
</button>Also applies to: 95-100
🤖 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 `@frontend/pages/VerifyEmail.jsx` around lines 41 - 56, handleResendOtp
currently allows multiple concurrent requests; add an in-flight guard state
(e.g., isResending) and check it at the top of handleResendOtp to return early
if true, set isResending = true before the axios.post and reset it in a finally
block so it always clears; also disable the resend button in the JSX when
isResending is true and (optionally) show a spinner or change label to indicate
progress to prevent OTP churn and mail spam (apply same guard to other resend
handlers around lines ~95-100).
UTKARSHH20
added
documentation
2 participants
Successfully completed and implemented this critical and advanced feature in the chat application.
Features Added
This issue has been fully resolved and tested. Kindly review and merge this PR.
Please add all applicable GSSoC labels, including Critical or Advanced, if it meets the project's criteria.
Button Inside Profile Page
Send Otp Page
Verify Email Page
backend
Also I add Two Routes
/send-verification-email-otp
Verified-account
I Create a Notemailer Utility To Send Mail
utils/nodemailer.js
Thank you for your review.
Summary by CodeRabbit