feat: harden developer auth (JWT expiry, OTP hashing, brute-force pro… - #33
Conversation
…tection, password reset)
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds OTP-based password reset (generate/hash/store OTP, attempt tracking, verify/delete), per-endpoint strict auth rate limiting, JWT expiry on login, email subject configurable, Dockerfile hardened to non-root with ownership on COPY and CMD, and docker-compose removed DB/Redis host port mappings. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Client
participant AuthController
participant UserModel
participant OTPModel
participant EmailService
participant Database
User->>Client: Request forgot password (email)
Client->>AuthController: POST /api/auth/forgot-password
AuthController->>UserModel: find user by email
AuthController->>OTPModel: delete existing OTP (if any)
AuthController->>AuthController: generate OTP, hash it
AuthController->>Database: store hashed OTP (+attempts:0, TTL)
AuthController->>EmailService: send raw OTP to email
AuthController-->>Client: generic success response
User->>Client: Submit reset (email, otp, newPassword)
Client->>AuthController: POST /api/auth/reset-password
AuthController->>OTPModel: fetch OTP doc by email
alt OTP not found
AuthController-->>Client: error (OTP missing/expired)
else OTP found
AuthController->>AuthController: check attempts < OTP_MAX_ATTEMPTS
alt attempts exceeded
AuthController-->>Client: error (too many attempts)
else within limit
AuthController->>AuthController: compare provided OTP with hash
alt mismatch
AuthController->>OTPModel: increment attempts
AuthController-->>Client: error (invalid OTP)
else match
AuthController->>UserModel: update user password
AuthController->>OTPModel: delete OTP record
AuthController-->>Client: success (password reset)
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the security posture of the developer authentication system. It addresses critical vulnerabilities by introducing time-limited JWTs, securing OTP storage with hashing, and implementing robust brute-force and OTP attempt protections. Additionally, a new password reset mechanism improves account recovery, and Docker configurations are hardened for better isolation. These changes aim to elevate the authentication system to production-grade security standards. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly hardens the developer authentication system by introducing JWT expiry, OTP hashing, brute-force protection, and a secure password reset flow. While these are positive changes, two critical security vulnerabilities were identified: a rate limiter bypass due to incorrect middleware ordering in app.js, and the use of a cryptographically insecure random number generator for OTP generation in auth.controller.js. Addressing these is crucial. Additionally, consider adding missing input validation and refactoring duplicated logic for improved maintainability.
| app.use('/api/auth', dashboardLimiter, authRoute); // Developer Auth (general) | ||
| app.use('/api/auth/login', authLimiter); // Strict limiter on login | ||
| app.use('/api/auth/register', authLimiter); // Strict limiter on register |
There was a problem hiding this comment.
The rate limiter (authLimiter) for login and registration is bypassed due to incorrect middleware ordering. In Express, middleware is processed sequentially, and the general authRoute handles requests like /api/auth/login before the stricter authLimiter can be applied. This allows attackers to bypass brute-force protection and utilize the higher limits of the dashboardLimiter. To fix this, declare the more specific routes with the stricter rate limiter before the more general /api/auth route.
| app.use('/api/auth', dashboardLimiter, authRoute); // Developer Auth (general) | |
| app.use('/api/auth/login', authLimiter); // Strict limiter on login | |
| app.use('/api/auth/register', authLimiter); // Strict limiter on register | |
| app.use('/api/auth/login', authLimiter); // Strict limiter on login | |
| app.use('/api/auth/register', authLimiter); // Strict limiter on register | |
| app.use('/api/auth', dashboardLimiter, authRoute); // Developer Auth (general) |
| const { email } = onlyEmailSchema.parse(req.body); | ||
| const { otp } = req.body; |
There was a problem hiding this comment.
The otp value is taken directly from req.body without validation. This can lead to unexpected behavior or errors if the otp is missing or has an incorrect format. It's a security best practice to always validate all user inputs.
I recommend creating a new Zod schema in input.validation.js for this endpoint:
module.exports.verifyOtpSchema = z.object({
email: z.string().email("Invalid email format"),
otp: z.string().length(6, "OTP must be 6 digits")
});Then, after importing it, use this schema to parse and validate the request body.
| const { email } = onlyEmailSchema.parse(req.body); | |
| const { otp } = req.body; | |
| const { email, otp } = verifyOtpSchema.parse(req.body); |
| const { email } = onlyEmailSchema.parse(req.body); | ||
| const otp = Math.floor(100000 + Math.random() * 900000); | ||
| // Secure: OTP logging removed | ||
| const otp = Math.floor(100000 + Math.random() * 900000).toString(); |
There was a problem hiding this comment.
Using Math.random() for generating One-Time Passwords (OTPs) is a security vulnerability because it is not cryptographically secure and its output can be predictable. For security-sensitive values like OTPs, a Cryptographically Secure Pseudo-Random Number Generator (CSPRNG) such as crypto.randomInt() from Node.js's crypto module should always be used to ensure unpredictability.
| const otp = Math.floor(100000 + Math.random() * 900000).toString(); | |
| const otp = require('crypto').randomInt(100000, 1000000).toString(); |
| // Return same message regardless of whether email exists (prevents user enumeration) | ||
| if (!dev) return res.status(200).json({ message: "If this email is registered, an OTP has been sent." }); | ||
|
|
||
| const otp = Math.floor(100000 + Math.random() * 900000).toString(); |
There was a problem hiding this comment.
Using Math.random() for generating One-Time Passwords (OTPs) is a security vulnerability because it is not cryptographically secure and its output can be predictable. For security-sensitive values like OTPs, a Cryptographically Secure Pseudo-Random Number Generator (CSPRNG) such as crypto.randomInt() from Node.js's crypto module should always be used to ensure unpredictability.
| const otp = Math.floor(100000 + Math.random() * 900000).toString(); | |
| const otp = require('crypto').randomInt(100000, 1000000).toString(); |
| const otp = Math.floor(100000 + Math.random() * 900000).toString(); | ||
|
|
||
| // Delete any existing OTP | ||
| await otpSchema.deleteOne({ userId: dev._id }); | ||
|
|
||
| // Hash OTP before storing | ||
| const salt = await bcrypt.genSalt(10); | ||
| const hashedOtp = await bcrypt.hash(otp, salt); | ||
|
|
||
| await new otpSchema({ userId: dev._id, otp: hashedOtp }).save(); |
There was a problem hiding this comment.
This block for generating, hashing, and storing an OTP is duplicated from the sendOtp function (lines 121-136).
To improve maintainability and reduce redundancy, consider extracting this logic into a dedicated helper function, for example createAndStoreOtp(userId, email). This would make the code cleaner and easier to manage, and ensure that any future changes to OTP logic only need to be made in one place.
| const otpDoc = await otpSchema.findOne({ userId: dev._id }); | ||
| if (!otpDoc) return res.status(400).json({ error: "No OTP found. Please request a new one." }); | ||
|
|
||
| // Attempt tracking | ||
| if (otpDoc.attempts >= OTP_MAX_ATTEMPTS) { | ||
| await otpDoc.deleteOne(); | ||
| return res.status(429).json({ error: "Too many incorrect attempts. Please request a new OTP." }); | ||
| } | ||
|
|
||
| const isMatch = await bcrypt.compare(otp.toString(), otpDoc.otp); | ||
| if (!isMatch) { | ||
| otpDoc.attempts += 1; | ||
| await otpDoc.save(); | ||
| const remaining = OTP_MAX_ATTEMPTS - otpDoc.attempts; | ||
| return res.status(400).json({ error: `Incorrect OTP. ${remaining} attempt(s) remaining.` }); | ||
| } |
There was a problem hiding this comment.
The OTP verification logic in this block is nearly identical to the logic in verifyOtp (lines 155-171). This includes finding the OTP document, checking attempts, and comparing the hash.
To avoid code duplication and improve maintainability, you could refactor this logic into a reusable helper function, like validateOtp(userId, otp). This function could handle all the verification steps and return the OTP document on success or throw an appropriate error on failure. Both resetPassword and verifyOtp could then call this helper, simplifying the controller logic.
|
|
||
| module.exports.resetPasswordSchema = z.object({ | ||
| email: z.string().email("Invalid email format"), | ||
| otp: z.string().min(6, "OTP is required"), |
There was a problem hiding this comment.
Since OTPs are fixed-length (6 digits in this implementation), it's more precise to validate for an exact length rather than a minimum length. This ensures the input matches the expected format strictly.
| otp: z.string().min(6, "OTP is required"), | |
| otp: z.string().length(6, "OTP must be 6 digits"), |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app.js`:
- Around line 80-82: The strict authLimiter is mounted after the broader
'/api/auth' router (app.use('/api/auth', dashboardLimiter, authRoute)), so
requests like '/api/auth/login' are handled by authRoute first and bypass the
limiter; move the authLimiter (the middleware referenced in
app.use('/api/auth/login', authLimiter) and app.use('/api/auth/register',
authLimiter)) so it runs before the general auth router (or apply authLimiter
directly inside the authRoute before the login/register handlers), ensuring
authLimiter executes for the login and register handlers and cannot be bypassed
by the prefix route.
In `@backend/controllers/auth.controller.js`:
- Around line 149-151: The code reads otp directly from req.body and later calls
otp.toString(), which throws if otp is missing; validate otp up front by parsing
it from the request with a dedicated schema (e.g., create/ use an otpSchema or
extend onlyEmailSchema to include otp) instead of using raw req.body, and return
a 400 for invalid/missing payloads; specifically, replace the direct read of otp
with a validated parse (referencing onlyEmailSchema.parse / your new
otpSchema.parse) and ensure any later use (the comparison logic that calls
otp.toString()) operates on the validated value so it cannot be null/undefined.
- Around line 214-217: The catch block after the onlyEmailSchema.parse() call is
treating validation failures as generic server errors; update the catch in the
function that handles forgot-password so it checks for ZodError (from zod) and
returns res.status(400).json(...) with the validation details instead of the
current res.status(500) response; keep the existing console.error for
non-validation errors and fall back to res.status(500).json({ error: "Internal
Server Error" }) for other exceptions.
- Around line 226-230: The reset-password endpoint currently returns a specific
"User not found" when Developer.findOne({ email }) fails, leaking account
existence; change the response behavior in the reset-password flow (the code
around Developer.findOne and otpSchema.findOne) to use the same generic response
as forgot-password (e.g., a non‑revealing message like "If an account exists, an
OTP has been sent" or a generic error message) for both the missing developer
and missing otpDoc cases so callers cannot distinguish whether the email exists
or not; update the error branches that return res.status(400).json(...) to
return the unified generic response.
- Around line 254-255: Update the Zod error handling in the auth controller to
use the Zod v4 property: replace the deprecated err.errors with err.issues in
the catch branch that checks "if (err instanceof z.ZodError)" (the same pattern
used earlier on line 64); return res.status(400).json({ error: err.issues }) so
validation responses are consistent and compatible with Zod v4.
- Line 121: Replace the insecure Math.random() OTP generation in
auth.controller.js (the otp variable assignments at lines 121 and 201) with a
cryptographically secure generator: import/require Node's crypto module and call
crypto.randomInt(100000, 1000000) to produce a 6-digit integer, then convert to
string where you currently set otp; update both places where otp is created so
email verification and password reset use crypto.randomInt instead of
Math.random.
In `@backend/middleware/auth_limiter.js`:
- Around line 4-11: The current rate-limiters (authLimiter, api_usage, and
dashboardLimiter) use the default in-memory store and must be switched to a
shared Redis store for multi-instance deployments; install and require
rate-limit-redis, import the existing Redis client (from
backend/config/redis.js), and pass a new RedisStore instance as the store option
when creating each limiter (e.g., set store: new RedisStore({ client: redis,
prefix: 'rl:auth:' }) for authLimiter), keeping other options (windowMs, max,
message, skip, standardHeaders, legacyHeaders) unchanged; apply the same pattern
to api_usage and the dashboardLimiter in app.js so all limiters use the
centralized Redis client.
In `@backend/models/otp.js`:
- Around line 4-6: Add a uniqueness constraint and safe upsert flow so only one
active OTP per user exists: update the OTP mongoose schema to create a unique
index on userId (e.g., schema.index({ userId: 1 }, { unique: true })) and remove
relying on non-unique field definitions; also change OTP creation code to use
findOneAndUpdate({ userId }, { $set: { otp, attempts: 0, ... } }, { upsert:
true, new: true }) (or handle duplicate key errors) so new requests replace
existing documents instead of inserting duplicates; reference the userId, otp,
and attempts symbols in backend/models/otp.js for these changes.
In `@backend/utils/input.validation.js`:
- Around line 41-45: The resetPasswordSchema's otp currently uses
z.string().min(6, ...) allowing any string of length >=6; tighten validation to
require exactly six numeric digits by replacing the otp rule with a Zod pattern
such as z.string().regex(/^\d{6}$/, "OTP must be a 6-digit code") so the schema
(resetPasswordSchema) only accepts the generated 6-digit OTP format.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 01532d29-0a3c-4452-9cf0-6f450e163d3e
📒 Files selected for processing (8)
backend/Dockerfilebackend/app.jsbackend/controllers/auth.controller.jsbackend/middleware/auth_limiter.jsbackend/models/otp.jsbackend/routes/auth.jsbackend/utils/input.validation.jsdocker-compose.yml
💤 Files with no reviewable changes (1)
- docker-compose.yml
| const dev = await Developer.findOne({ email }); | ||
| if (!dev) return res.status(400).json({ error: "User not found" }); | ||
|
|
||
| const otpDoc = await otpSchema.findOne({ userId: dev._id }); | ||
| if (!otpDoc) return res.status(400).json({ error: "No OTP found. Please request a new one." }); |
There was a problem hiding this comment.
Reset-password path leaks account existence.
Line [227] returns "User not found", while forgot-password intentionally uses a generic response. This reintroduces user-enumeration risk on a public endpoint.
🔒 Suggested response hardening
- const dev = await Developer.findOne({ email });
- if (!dev) return res.status(400).json({ error: "User not found" });
+ const dev = await Developer.findOne({ email });
+ if (!dev) {
+ return res.status(400).json({ error: "Invalid email or OTP." });
+ }📝 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 dev = await Developer.findOne({ email }); | |
| if (!dev) return res.status(400).json({ error: "User not found" }); | |
| const otpDoc = await otpSchema.findOne({ userId: dev._id }); | |
| if (!otpDoc) return res.status(400).json({ error: "No OTP found. Please request a new one." }); | |
| const dev = await Developer.findOne({ email }); | |
| if (!dev) { | |
| return res.status(400).json({ error: "Invalid email or OTP." }); | |
| } | |
| const otpDoc = await otpSchema.findOne({ userId: dev._id }); | |
| if (!otpDoc) return res.status(400).json({ error: "No OTP found. Please request a new one." }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/controllers/auth.controller.js` around lines 226 - 230, The
reset-password endpoint currently returns a specific "User not found" when
Developer.findOne({ email }) fails, leaking account existence; change the
response behavior in the reset-password flow (the code around Developer.findOne
and otpSchema.findOne) to use the same generic response as forgot-password
(e.g., a non‑revealing message like "If an account exists, an OTP has been sent"
or a generic error message) for both the missing developer and missing otpDoc
cases so callers cannot distinguish whether the email exists or not; update the
error branches that return res.status(400).json(...) to return the unified
generic response.
| if (err instanceof z.ZodError) return res.status(400).json({ error: err.errors }); | ||
| console.error(err); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Zod v4 ZodError issues vs errors property
💡 Result:
In Zod v4, the canonical array of validation problems on a ZodError is:
error.issues: ZodIssue[]
The old v3 property:
error.errors
is no longer reliable in v4 (many users hit it as “removed” during v3→v4 migration), and the expected replacement is to read error.issues instead. [1], [2]
Fix: replace err.errors with err.issues.
If you need temporary compatibility (supporting both v3 + v4 in shared code), use:
const issues = (err as any).issues ?? (err as any).errors ?? [];Sources: Zod v4 migration/changelog (notes error-handling changes and uses error.issues) [3], plus migration discussions noting .errors → .issues [1], [2].
References: [1] [2] [3]
🏁 Script executed:
# Find and examine the auth.controller.js file
find . -name "auth.controller.js" -type fRepository: yash-pouranik/urBackend
Length of output: 107
🏁 Script executed:
# Examine the auth.controller.js file, focusing on lines around 64 and 254-255
cat -n ./backend/controllers/auth.controller.js | sed -n '60,70p;250,260p'Repository: yash-pouranik/urBackend
Length of output: 887
Change line 254 to use err.issues for Zod v4 compatibility.
Line 254 uses err.errors, which is the deprecated Zod v3 property and unreliable in Zod v4. The canonical validation details array in Zod v4 is err.issues, already used correctly on line 64. Update this handler for consistency:
Suggested fix
- if (err instanceof z.ZodError) return res.status(400).json({ error: err.errors });
+ if (err instanceof z.ZodError) return res.status(400).json({ error: err.issues });📝 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.
| if (err instanceof z.ZodError) return res.status(400).json({ error: err.errors }); | |
| console.error(err); | |
| if (err instanceof z.ZodError) return res.status(400).json({ error: err.issues }); | |
| console.error(err); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/controllers/auth.controller.js` around lines 254 - 255, Update the
Zod error handling in the auth controller to use the Zod v4 property: replace
the deprecated err.errors with err.issues in the catch branch that checks "if
(err instanceof z.ZodError)" (the same pattern used earlier on line 64); return
res.status(400).json({ error: err.issues }) so validation responses are
consistent and compatible with Zod v4.
| const authLimiter = rateLimit({ | ||
| windowMs: 15 * 60 * 1000, // 15 minutes | ||
| max: 10, | ||
| message: { error: "Too many attempts. Please try again in 15 minutes." }, | ||
| skip: (req) => process.env.NODE_ENV === 'development', | ||
| standardHeaders: true, | ||
| legacyHeaders: false, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for rate-limit store configurations and dependencies
echo "=== Searching for rate-limit store references ==="
rg -n "express-rate-limit|rate-limit-redis|RedisStore|store:" backend --max-count=20
echo -e "\n=== Checking package.json for rate-limit related dependencies ==="
cat package.json | grep -A 10 '"dependencies"' | grep -i "rate-limit\|redis"
echo -e "\n=== Looking for Redis configuration ==="
rg -n "redis|REDIS" --max-count=20
echo -e "\n=== Checking for multi-instance/deployment configs ==="
ls -la | grep -E "docker|compose|kubernetes|helm|deploy"
find . -maxdepth 2 -type f \( -name "docker-compose*" -o -name "Dockerfile" -o -name "*.yml" -o -name "*.yaml" \) 2>/dev/null | head -10Repository: yash-pouranik/urBackend
Length of output: 5703
🏁 Script executed:
#!/bin/bash
echo "=== auth_limiter.js (lines 1-20) ==="
head -20 backend/middleware/auth_limiter.js
echo -e "\n=== api_usage.js (lines 1-20) ==="
head -20 backend/middleware/api_usage.js
echo -e "\n=== app.js rateLimit usage (line 13 context) ==="
sed -n '10,30p' backend/app.js
echo -e "\n=== redis.js config ==="
cat backend/config/redis.jsRepository: yash-pouranik/urBackend
Length of output: 2902
Integrate Redis store with rate-limiters for multi-instance deployments.
The middleware currently uses the default in-memory store. While Redis is already configured in the project (backend/config/redis.js, docker-compose), the rate-limiters (authLimiter, api_usage, and dashboardLimiter) bypass it. In production with multiple instances, each process maintains separate counters, allowing distributed clients to bypass limits.
Pass the Redis client to express-rate-limit via the store option. This requires installing rate-limit-redis and integrating it with the existing Redis configuration:
Example fix for auth_limiter.js
const rateLimit = require('express-rate-limit');
const RedisStore = require('rate-limit-redis');
const redis = require('../config/redis');
const authLimiter = rateLimit({
store: new RedisStore({
client: redis,
prefix: 'rl:auth:'
}),
windowMs: 15 * 60 * 1000,
max: 10,
message: { error: "Too many attempts. Please try again in 15 minutes." },
skip: (req) => process.env.NODE_ENV === 'development',
standardHeaders: true,
legacyHeaders: false,
});
module.exports = { authLimiter };Apply the same pattern to api_usage.js and the dashboard limiter in app.js.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/middleware/auth_limiter.js` around lines 4 - 11, The current
rate-limiters (authLimiter, api_usage, and dashboardLimiter) use the default
in-memory store and must be switched to a shared Redis store for multi-instance
deployments; install and require rate-limit-redis, import the existing Redis
client (from backend/config/redis.js), and pass a new RedisStore instance as the
store option when creating each limiter (e.g., set store: new RedisStore({
client: redis, prefix: 'rl:auth:' }) for authLimiter), keeping other options
(windowMs, max, message, skip, standardHeaders, legacyHeaders) unchanged; apply
the same pattern to api_usage and the dashboardLimiter in app.js so all limiters
use the centralized Redis client.
| userId: { type: mongoose.Schema.Types.ObjectId, required: true, ref: 'Developer' }, | ||
| otp: { type: String, required: true }, // bcrypt hash (FIX 2) | ||
| attempts: { type: Number, default: 0 }, // brute-force guard (FIX 4) |
There was a problem hiding this comment.
Enforce one active OTP document per user.
Line [4] is not unique. Concurrent OTP requests can create multiple records, weakening Line [6] attempt tracking and allowing stale OTP documents to persist.
🧱 Suggested schema hardening
- userId: { type: mongoose.Schema.Types.ObjectId, required: true, ref: 'Developer' },
+ userId: { type: mongoose.Schema.Types.ObjectId, required: true, ref: 'Developer', unique: true, index: true },
otp: { type: String, required: true }, // bcrypt hash (FIX 2)
- attempts: { type: Number, default: 0 }, // brute-force guard (FIX 4)
+ attempts: { type: Number, default: 0, min: 0 }, // brute-force guard (FIX 4)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/models/otp.js` around lines 4 - 6, Add a uniqueness constraint and
safe upsert flow so only one active OTP per user exists: update the OTP mongoose
schema to create a unique index on userId (e.g., schema.index({ userId: 1 }, {
unique: true })) and remove relying on non-unique field definitions; also change
OTP creation code to use findOneAndUpdate({ userId }, { $set: { otp, attempts:
0, ... } }, { upsert: true, new: true }) (or handle duplicate key errors) so new
requests replace existing documents instead of inserting duplicates; reference
the userId, otp, and attempts symbols in backend/models/otp.js for these
changes.
| module.exports.resetPasswordSchema = z.object({ | ||
| email: z.string().email("Invalid email format"), | ||
| otp: z.string().min(6, "OTP is required"), | ||
| newPassword: z.string().min(6, "Password must be at least 6 characters").max(100, "Password is too long.") | ||
| }); |
There was a problem hiding this comment.
Tighten OTP validation to match the generated format.
Line [43] currently accepts any string with length ≥ 6. Since OTPs are generated as 6-digit codes, enforce exactly six digits to avoid wasting attempts on invalid formats.
🔧 Suggested change
module.exports.resetPasswordSchema = z.object({
email: z.string().email("Invalid email format"),
- otp: z.string().min(6, "OTP is required"),
+ otp: z.string().regex(/^\d{6}$/, "OTP must be a 6-digit code"),
newPassword: z.string().min(6, "Password must be at least 6 characters").max(100, "Password is too long.")
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/utils/input.validation.js` around lines 41 - 45, The
resetPasswordSchema's otp currently uses z.string().min(6, ...) allowing any
string of length >=6; tighten validation to require exactly six numeric digits
by replacing the otp rule with a Zod pattern such as z.string().regex(/^\d{6}$/,
"OTP must be a 6-digit code") so the schema (resetPasswordSchema) only accepts
the generated 6-digit OTP format.
… OTP helper refactor, strict zod validation)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/controllers/auth.controller.js`:
- Line 217: The call to sendOtp(email, otp, { subject: "Password Reset OTP —
urBackend" }) passes an unsupported third parameter; update either the caller or
the function. Option A (preferred): extend backend/utils/emailService.js sendOtp
to accept an optional options param (e.g., sendOtp(email, otp, options)), read
options.subject and use it instead of the hardcoded "Verify your urBackend
account" when composing the email; ensure backward compatibility by defaulting
to the existing subject. Option B: remove the third argument from
backend/controllers/auth.controller.js and rely on the current sendOtp signature
so the hardcoded subject remains. Locate sendOtp and the call site to apply one
consistent change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a3c21d5a-af0c-47a6-87bb-144f6ae58032
📒 Files selected for processing (3)
backend/app.jsbackend/controllers/auth.controller.jsbackend/utils/input.validation.js
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
backend/controllers/auth.controller.js (2)
231-233:⚠️ Potential issue | 🟠 MajorAvoid account-enumeration responses in reset-password path.
Line [232] exposes
"User not found". Also, passing through specific 400 messages fromvalidateOtpcan still leak account/reset state.💡 Proposed fix
- const dev = await Developer.findOne({ email }); - if (!dev) return res.status(400).json({ error: "User not found" }); + const dev = await Developer.findOne({ email }); + if (!dev) return res.status(400).json({ error: "Invalid email or OTP." }); @@ - if (err.status) return res.status(err.status).json({ error: err.message }); + if (err.status === 429) return res.status(429).json({ error: err.message }); + if (err.status === 400) return res.status(400).json({ error: "Invalid email or OTP." });Also applies to: 244-245
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/controllers/auth.controller.js` around lines 231 - 233, Replace account-specific responses in the reset-password flow to avoid user enumeration: when calling Developer.findOne({ email }) in the reset-password route, do not return "User not found" — instead always return a generic success response (e.g. 200 with "If an account with that email exists, a reset was initiated") so callers cannot detect existence; similarly, wrap calls to validateOtp and any error paths (the checks around validateOtp and the subsequent 400 responses at the similar block around lines 244-245) and map all validation/failure results to the same generic error/success message/status instead of forwarding specific error text. Ensure the change is applied to the functions handling the reset request and OTP validation so Developer.findOne and validateOtp never leak account/reset state.
168-168:⚠️ Potential issue | 🟠 MajorUse
err.issuesin these Zod handlers for v4 compatibility.Lines [168], [198], [219], and [245] still return
err.errors. In Zod v4,issuesis the canonical property.💡 Proposed fix
- if (err instanceof z.ZodError) return res.status(400).json({ error: err.errors }); + if (err instanceof z.ZodError) return res.status(400).json({ error: err.issues }); @@ - if (err instanceof z.ZodError) return res.status(400).json({ error: err.errors }); + if (err instanceof z.ZodError) return res.status(400).json({ error: err.issues }); @@ - if (err instanceof z.ZodError) return res.status(400).json({ error: err.errors }); + if (err instanceof z.ZodError) return res.status(400).json({ error: err.issues }); @@ - if (err instanceof z.ZodError) return res.status(400).json({ error: err.errors }); + if (err instanceof z.ZodError) return res.status(400).json({ error: err.issues });#!/bin/bash set -euo pipefail # Check zod version in package manifests fd '^package\.json$' -x sh -c 'echo "== $1 =="; rg -n "\"zod\"\\s*:" "$1"' sh {} # Find deprecated property usage in this controller rg -nP --type=js '\berr\.errors\b' backend/controllers/auth.controller.js rg -nP --type=js '\berr\.issues\b' backend/controllers/auth.controller.jsAlso applies to: 198-198, 219-219, 245-245
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/controllers/auth.controller.js` at line 168, Handlers that catch Zod validation failures are returning the deprecated err.errors property; update each handler that checks "err instanceof z.ZodError" (e.g., the response lines currently using "err.errors") to return "err.issues" instead, replacing all occurrences in auth.controller.js (the handlers around the checks at the spots that currently use err.errors) so Zod v4-compatible validation issues are returned in the response body.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/controllers/auth.controller.js`:
- Around line 46-50: The code increments otpDoc.attempts after a failed OTP
check but still treats the 5th failure as a normal failure; update the isMatch
false branch (where otpDoc.attempts, OTP_MAX_ATTEMPTS, otpDoc.save are used) to
increment and persist attempts, then immediately check if otpDoc.attempts >=
OTP_MAX_ATTEMPTS and if so perform the lock/delete action on the otpDoc (or set
a locked flag) and throw a lockout error; otherwise compute remaining =
OTP_MAX_ATTEMPTS - otpDoc.attempts and throw the existing 400 with remaining
attempts so the OTP is removed/locked on the 5th failed attempt rather than the
6th.
In `@backend/utils/emailService.js`:
- Line 8: The sendOtp function currently destructures the third argument
directly causing a crash when callers (like the sendOtp(email, otp) call in
auth.controller.js) pass only two args; change the signature to accept an
optional options object (e.g., async function sendOtp(email, otp, options = {})
or async function sendOtp(email, otp, { subject = "Verify your urBackend
account" } = {})) so the {subject} destructuring has a safe default and won't
throw when undefined; update the function definition in
backend/utils/emailService.js and leave existing call sites unchanged.
---
Duplicate comments:
In `@backend/controllers/auth.controller.js`:
- Around line 231-233: Replace account-specific responses in the reset-password
flow to avoid user enumeration: when calling Developer.findOne({ email }) in the
reset-password route, do not return "User not found" — instead always return a
generic success response (e.g. 200 with "If an account with that email exists, a
reset was initiated") so callers cannot detect existence; similarly, wrap calls
to validateOtp and any error paths (the checks around validateOtp and the
subsequent 400 responses at the similar block around lines 244-245) and map all
validation/failure results to the same generic error/success message/status
instead of forwarding specific error text. Ensure the change is applied to the
functions handling the reset request and OTP validation so Developer.findOne and
validateOtp never leak account/reset state.
- Line 168: Handlers that catch Zod validation failures are returning the
deprecated err.errors property; update each handler that checks "err instanceof
z.ZodError" (e.g., the response lines currently using "err.errors") to return
"err.issues" instead, replacing all occurrences in auth.controller.js (the
handlers around the checks at the spots that currently use err.errors) so Zod
v4-compatible validation issues are returned in the response body.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e9d8e6dd-e551-42ab-8ac7-0f30619292ad
📒 Files selected for processing (2)
backend/controllers/auth.controller.jsbackend/utils/emailService.js
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Render
Destroyed
1 participant
🛡️ feat: Harden Developer Auth System
Overview
This PR hardens the core developer authentication (
/api/auth) to fix several security vulnerabilities and improve the overall robustness of the platform.What's Changed
jwt.sign()) now expire in 7 days (7d). Previously, they had no expiry and were valid forever.bcryptbefore storing in the database.otp: Numberchanged tootp: String.authLimiter(10 req/15min) specifically for/api/auth/loginand/api/auth/registerto prevent credential stuffing.attemptscounter to the OTP model. Locks out and deletes the OTP document after 5 incorrect guesses (429 Too Many Attempts).POST /api/auth/forgot-passwordandPOST /api/auth/reset-passwordroutes to allow locked-out users to recover their accounts securely.nodeuser and removed host-bound ports for MongoDB and Redis (only accessible inside the Docker network now).Why
To bring the core authentication system up to production-grade security standards before moving on to advanced features like Granular API Keys or OAuth.
Type of Change
Summary by CodeRabbit
New Features
Security
Chores