fix(security): harden auth endpoints against NoSQL injection and add JWT warning#3268
fix(security): harden auth endpoints against NoSQL injection and add JWT warning#3268PierreBrisorgueil merged 3 commits intomasterfrom
Conversation
|
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. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR introduces input validation and sanitization across multiple modules: JWT secret validation on startup, string conversion for authentication tokens and emails, OAuth provider key validation, and object input rejection in user search operations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 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)
📝 Coding Plan
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
This PR aims to harden authentication-related flows against NoSQL/operator injection and add an operational warning when running with the default JWT secret.
Changes:
- Added input coercion/allowlisting in auth password + OAuth callback flows to reduce injection risk.
- Added repository-layer validation in
UserRepository.search()to reject object-valued query filters. - Added a startup warning when
config.jwt.secretis still set to the default value.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/users/repositories/users.repository.js | Adds object-value rejection to search() filters to prevent operator injection. |
| modules/auth/controllers/auth.password.controller.js | Coerces email/token to strings before DB lookups in forgot/reset flows. |
| modules/auth/controllers/auth.controller.js | Restricts OAuth req.body.key to an allowlist to prevent arbitrary providerData path injection. |
| lib/helpers/config.js | Introduces validateJwtSecret() to warn when default JWT secret is used. |
| config/index.js | Calls validateJwtSecret() during config initialization (non-test env). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
modules/auth/controllers/auth.controller.js (1)
315-321:⚠️ Potential issue | 🟡 MinorAdd missing
@returnsfor modifiedoauthCallbackJSDoc.
oauthCallbackwas modified and is async; JSDoc should document its resolved return type.Suggested patch
/** * `@desc` Endpoint for oautCallCallBack * `@param` {Object} req - Express request object * `@param` {Object} res - Express response object * `@param` {Function} next - Express next middleware function + * `@returns` {Promise<void>} Sends OAuth response or delegates to passport callback flow. */ const oauthCallback = async (req, res, next) => {As per coding guidelines: "
**/*.js: Every new or modified function must have a JSDoc header..." and "**/*.{js,ts,jsx,tsx}: Every function must have a JSDoc header with@paramand@returnsdocumentation".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/auth/controllers/auth.controller.js` around lines 315 - 321, The oauthCallback JSDoc is missing a `@returns` entry after being converted to an async function; update the JSDoc for oauthCallback to include a `@returns` tag documenting the resolved promise type (for example, `@returns` {Promise<void>} with a short description like "Resolves when the response has been sent" or `@returns` {Promise<Express.Response>} if you prefer to indicate the response object), ensuring the async nature is clearly documented alongside the existing `@param` tags.modules/auth/controllers/auth.password.controller.js (1)
65-72:⚠️ Potential issue | 🟡 MinorAdd missing
@returnsinvalidateResetTokenJSDoc.This function was modified and is async, but its JSDoc still omits
@returns.Suggested patch
/** * `@desc` Endpoint to validate Reset Token from link * `@param` {Object} req - Express request object * `@param` {Object} res - Express response object + * `@returns` {Promise<void>} Redirects to valid/invalid reset endpoints. */ const validateResetToken = async (req, res) => {As per coding guidelines: "
**/*.js: Every new or modified function must have a JSDoc header..." and "**/*.{js,ts,jsx,tsx}: Every function must have a JSDoc header with@paramand@returnsdocumentation".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/auth/controllers/auth.password.controller.js` around lines 65 - 72, The JSDoc for the async function validateResetToken is missing a `@returns` tag; update the comment block above validateResetToken to include a `@returns` describing the Promise returned (e.g. `@returns` {Promise<void>} or `@returns` {Promise<Express.Response>} with a short description like "resolves after sending HTTP response") so the async nature and return contract are documented alongside the existing `@param` entries for req and res.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/helpers/config.js`:
- Around line 57-61: Add a JSDoc header to the validateJwtSecret function
describing its purpose and include `@param` and `@returns` tags: document that the
single parameter is the config object (type object) and explain the expected
jwt.secret property, and state that the function returns void (or nothing).
Place the JSDoc immediately above the validateJwtSecret declaration so the
exported helper API is fully documented.
In `@modules/users/repositories/users.repository.js`:
- Around line 79-86: The search() function currently rejects any nested object
and crashes when input is undefined; remove the blanket Object.values loop and
instead guard against undefined/non-object inputs and allow nested query
operators (e.g., { email: { $in: [...] } }). In practice, change search to
normalize input: if input is undefined or typeof input !== 'object' set input =
{} (or throw only for explicitly invalid types), then call
User.find(input).exec(); keep the User.find(...) call and ensure no
Object.values(input) is used without checking input first.
---
Outside diff comments:
In `@modules/auth/controllers/auth.controller.js`:
- Around line 315-321: The oauthCallback JSDoc is missing a `@returns` entry after
being converted to an async function; update the JSDoc for oauthCallback to
include a `@returns` tag documenting the resolved promise type (for example,
`@returns` {Promise<void>} with a short description like "Resolves when the
response has been sent" or `@returns` {Promise<Express.Response>} if you prefer to
indicate the response object), ensuring the async nature is clearly documented
alongside the existing `@param` tags.
In `@modules/auth/controllers/auth.password.controller.js`:
- Around line 65-72: The JSDoc for the async function validateResetToken is
missing a `@returns` tag; update the comment block above validateResetToken to
include a `@returns` describing the Promise returned (e.g. `@returns`
{Promise<void>} or `@returns` {Promise<Express.Response>} with a short description
like "resolves after sending HTTP response") so the async nature and return
contract are documented alongside the existing `@param` entries for req and res.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1551db4f-7a93-49ff-be4a-6511cd75f801
📒 Files selected for processing (5)
config/index.jslib/helpers/config.jsmodules/auth/controllers/auth.controller.jsmodules/auth/controllers/auth.password.controller.jsmodules/users/repositories/users.repository.js
…ernal queries The repository-level object check rejected all object values including legitimate MongoDB operators ($in, etc.) used by seed and internal code. NoSQL injection is already prevented at controller boundaries via String() coercion and key allowlists.
Summary
req.body.keyinoauthCallbackto['id', 'sub', 'email']to prevent arbitrary property injection on provider data lookupsforgot/reset/validateResetToken: coerceemailandtokeninputs toString()before passing to DB queries, preventing NoSQL injection via object payloads (e.g.{"$gt": ""})UserRepository.search(): reject any search input containing object values to block operator injection at the repository layerDEVKIT_NODE_jwt_secretin productionTest plan
/api/auth/oauth/googlewithkey: "__proto__"returns 422/api/auth/forgotwithemail: {"$gt": ""}is coerced to string/api/auth/resetwithtoken: {"$gt": ""}is coerced to stringSummary by CodeRabbit
Release Notes
New Features
Bug Fixes