Repository navigation
bug: reject non-string login credentials to block NoSQL injection - #1130
Open
Shrinandan-N wants to merge 3 commits into
Open
Shrinandan-N wants to merge 3 commits into
Shrinandan-N wants to merge 3 commits into
Conversation
…loginHandler Co-Authored-By: nandu.narayanan <nandu.narayanan@cognition.ai>
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if (validator.isEmail(req.body.username)) { | ||
| User.find({ username: req.body.username, password: req.body.password }, function (err, users) { | ||
| const username = req.body.username | ||
| const password = req.body.password |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
…ral values Co-Authored-By: nandu.narayanan <nandu.narayanan@cognition.ai>
…he sanitizer Co-Authored-By: nandu.narayanan <nandu.narayanan@cognition.ai>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the NoSQL injection in
loginHandler(Snyk CodeSNYK-CODE-NOSQLI-943, CWE-943, routes/index.js:39). BecausebodyParser.json()is enabled,req.body.passwordcould be an object like{"$gt": ""}. It went straight intoUser.find, so anyone who knew the [REDACTED SECRET] email could log in without the password.loginHandlernow returns 401 unless bothusernameandpasswordare strings. It also coerces them withString()in the query, because Snyk Code only recognizes that as a sanitizer and still flagged atypeofguard or a{ $eq: ... }query. Separately, it passesUser.finderrors tonext(err)instead of crashing onusers.length.Verification
Tested locally against
mongo:3(seeded[REDACTED SECRET]@snyk.io) withPOST /loginand a JSON body:password: {"$gt":""}username/passwordboth{"$gt":""}A local
snyk code teston this branch reports no injection result onloginHandler(22 results remain elsewhere in the repo, all pre-existing).About the failing
SnykCodecheckThis check does not scan the PR.
.github/workflows/snyk-code-manual.ymluploads the committed staticsarif.json, which pins ajavascript/Sqliresult to routes/index.js:39. Since this PR changes that line, GitHub fingerprints the static result as "1 new alert". It will fail for any change to this line untilsarif.jsonis regenerated, which this PR doesn't do.Note: #1033 is an earlier open PR for the same finding. Only one of the two should land.
Devin-Org: engineering
Link to Devin session: https://devin-gtm.devinenterprise.com/sessions/d33c608b65f4427a81e8f294550675d1
Open in Devin Desktop: https://devin-gtm.devinenterprise.com/desktop/session/d33c608b65f4427a81e8f294550675d1?variant=devin
Requested by: @Shrinandan-N
Devin Review