fix(security): harden OTP generation, sanitize(), and verifyApiKey project select#106
fix(security): harden OTP generation, sanitize(), and verifyApiKey project select#106prathamtagad wants to merge 2 commits intogeturbackend:mainfrom
Conversation
…lect
- Replace Math.random() with crypto.randomInt() for OTP generation in
userAuth.controller.js (lines 970, 995, 1306, 1344). Math.random() is
not cryptographically secure; crypto.randomInt uses Node's CSPRNG,
consistent with how dashboard-api generates OTPs in auth.controller.js.
- Make sanitize() in input.validation.js recursive so nested himBHsprefixed
keys (e.g. { profile: { "$gt": "" } }) are stripped at every depth,
closing a NoSQL injection vector through nested update payloads.
- Add siteUrl and authProviders to the Project.findOne().select() clause
in verifyApiKey.js. Without these fields the cached project object is
missing data required by getFrontendCallbackBaseUrl(), causing social
auth callbacks to always redirect to localhost:5173 in production.
All 90 public-api tests pass.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 37 minutes and 55 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (17)
📝 WalkthroughWalkthroughThree changes enhance security and validation: OTP generation switched to cryptographically secure RNG in user authentication controllers, API key middleware now retrieves additional project fields (siteUrl, authProviders), and input validation function extended to recursively sanitize arrays and nested object properties. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
There was a problem hiding this comment.
Pull request overview
Hardens parts of the public API by improving OTP generation security, strengthening request sanitization against NoSQL operator injection, and expanding the Project projection used during API key verification.
Changes:
- Replaced OTP generation sites using
Math.random()withcrypto.randomInt()inuserAuth.controller.js. - Updated
sanitize()to recursively strip$-prefixed keys across nested objects/arrays. - Added
siteUrlandauthProvidersto the Project fields selected inverifyApiKeymiddleware.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/common/src/utils/input.validation.js | Makes sanitize() recursive to remove $ keys at all depths. |
| apps/public-api/src/middlewares/verifyApiKey.js | Extends Project .select() projection to include siteUrl and authProviders. |
| apps/public-api/src/controllers/userAuth.controller.js | Switches OTP generation to use Node crypto CSPRNG. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| siteUrl | ||
| authProviders |
There was a problem hiding this comment.
Adding siteUrl/authProviders to this middleware’s Project projection may not resolve the social-auth redirect issue described in the PR: the social flow re-fetches the project via getSocialProviderConfig() (apps/public-api/src/controllers/userAuth.controller.js around the selectClause construction) and that select does not include siteUrl, which is what getFrontendCallbackBaseUrl() reads. Consider including siteUrl in the getSocialProviderConfig() projection (or reusing req.project there) so the callback URL is built from the actual project setting.
| for (const key in obj) { | ||
| if (!key.startsWith("$")) { |
There was a problem hiding this comment.
sanitize() now recursively copies keys into a fresh object, but it still allows special keys like __proto__, constructor, and prototype. If an attacker submits these keys in JSON payloads (e.g. in insert/update endpoints that call sanitize()), assigning them onto clean can mutate the object's prototype and lead to prototype-pollution style issues. Please explicitly block these keys (in addition to $-prefixed keys) and consider iterating only own-properties (e.g., Object.keys() / hasOwnProperty) when building clean.
| for (const key in obj) { | |
| if (!key.startsWith("$")) { | |
| const blockedKeys = new Set(["__proto__", "constructor", "prototype"]); | |
| for (const key of Object.keys(obj)) { | |
| if (!key.startsWith("$") && !blockedKeys.has(key)) { |
| const otp = crypto.randomInt(100000, 1000000).toString(); | ||
| await redis.set(`project:${project._id}:otp:verification:${email}`, otp, 'EX', 300); | ||
| await setPublicOtpCooldown(project._id, email, 'verification'); |
There was a problem hiding this comment.
OTP/cooldown Redis keys here are built with the raw email from the request, but verifyEmail later looks up the OTP using a normalized/lowercased email in the Redis key. If the user signs up with mixed-case or surrounding whitespace, the OTP will be stored under a different key and verification (and cooldown enforcement) can fail / be bypassed. Use the same normalized email value consistently for OTP storage and cooldown keys (and ideally for downstream email sending payloads too).
| const otp = crypto.randomInt(100000, 1000000).toString(); | ||
|
|
There was a problem hiding this comment.
In this signup path, normalizedEmail is computed earlier but the OTP is later stored in Redis under a key that uses the raw email value (see the redis.set(...otp:verification:${email}...) a few lines below). This can break verification/cooldown when the input email contains uppercase or whitespace. Use the normalized email consistently for OTP storage and subsequent lookups.
| const otp = crypto.randomInt(100000, 1000000).toString(); | ||
| await redis.set(`project:${project._id}:otp:verification:${email}`, otp, 'EX', 300); | ||
| await setPublicOtpCooldown(project._id, email, 'verification'); |
There was a problem hiding this comment.
Same issue as in signup: OTP/cooldown Redis keys use the raw email value, but the verification flow normalizes email before reading the OTP key. This can cause resend + verification to fail for mixed-case/whitespace emails and can allow cooldown bypass by changing email casing. Store OTP and set cooldown using the normalized email consistently.
- enforce correct api response formats - add graceful shutdown for redis/bullmq - fix email normalization on verify/resend - remove dead code and noisy logs - secure storage delete endpoints
|
@prathamtagad |
Summary
Fixes three security and functional issues found in the public-api.
1. OTP generated with
Math.random()— Cryptographically WeakFiles:
apps/public-api/src/controllers/userAuth.controller.jsLines: 970, 995, 1306, 1344
Math.random()is not a CSPRNG and its output is predictable. Replacedall four OTP generation sites with
crypto.randomInt(100000, 1000000),which uses Node's secure random source. This matches the pattern already
in use in
apps/dashboard-api/src/controllers/auth.controller.js.2.
sanitize()only strips top-level$-prefixed keys — NoSQL Injection RiskFile:
packages/common/src/utils/input.validation.jsThe previous implementation only removed
$-operator keys at the toplevel. A payload like
{ "profile": { "$gt": "" } }would pass throughunchanged, enabling NoSQL injection via nested update/query paths.
The function is now recursive: it walks plain objects and arrays,
stripping
$-prefixed keys at every depth.3. Missing
siteUrlinverifyApiKeyselect — Social Auth Callback BreaksFile:
apps/public-api/src/middlewares/verifyApiKey.jssiteUrlandauthProviderswere not included in the Mongoose.select()projection. As a result, the project object cached andforwarded to downstream controllers was missing these fields.
getFrontendCallbackBaseUrl()would always fall back tolocalhost:5173,breaking social auth redirects in production.
Both fields are now included in the select clause.
Testing
public-apitests pass (npm test -- --runInBand).Summary by CodeRabbit