Fix NoSQL/SQL injection, prototype pollution and vulnerable dependencies - #283
Open
devin-ai-integration[bot] wants to merge 1 commit into
Open
devin-ai-integration[bot] wants to merge 1 commit into
devin-ai-integration[bot] wants to merge 1 commit into
Conversation
…ble dependencies - loginHandler: reject non-string credentials, use $eq filters (NoSQL operator injection) - /chat: replace lodash.merge on untrusted body with sanitized Object.assign, own-property canDelete check - /users: whitelist string columns before repo.save, explicit where clause; typeorm 0.2.24 -> 0.3.31 (DataSource) - /create: reject non-string content (mongoose Buffer memory exposure), execFile instead of exec for identify - about_new: drop eval-based dustjs-helpers @if; dustjs-linkedin 2.5.0 -> 3.0.1 - lodash 4.17.4 -> 4.18.1, mongoose 4.2.4 -> 8.24.4, express 4.12.4 -> 4.22.2, body-parser 1.9.0 -> 1.20.6, qs override 6.16.0 - remove unused mongodb and tap dependencies - add node:test regression tests (npm run test:unit) Co-Authored-By: Rush Cromer II <rush.cromerii@cognition.ai>
Author
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
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
Audit of the app for injection (SQL/NoSQL), prototype pollution and vulnerable dependencies, with one regression test file per fix (
npm run test:unit, usesnode:test, no DB required). Every exploit below was reproduced againstmainand confirmed closed against this branch with the app running.1. NoSQL operator injection —
POST /login(routes/index.jsloginHandler)Exploit:
bodyParser.json()lets the client send objects instead of strings, so{"username":"admin@snyk.io","password":{"$gt":""}}(or{"$gt":""}for both fields, no username needed) becameUser.find({ username, password: { $gt: "" } })and matched the admin record → sessionloggedIn = 1.Fix: require both fields to be
typeof 'string'before touching the DB, and query with equality-only operators so nothing the client sends can ever be interpreted as an operator:Test:
tests/nosql-injection.test.js(asserts the DB is never queried for operator payloads and that the filter is exactly$eq).2. Prototype pollution → privilege escalation —
PUT /chat(routes/index.jschat.add/chat.delete)Exploit:
_.merge(message, req.body.message, ...)with lodash 4.17.4 walks__proto__, so{"auth":{"name":"user","password":"pwd"},"message":{"__proto__":{"canDelete":true}}}setObject.prototype.canDelete = true. The regular user (who has nocanDelete) then passed!user.canDeleteand couldDELETE /chat.Fix:
_.mergeon untrusted input replaced withsanitizeMessage()(own keys only,__proto__/constructor/prototypeskipped, string values only) +Object.assign; server-owned fields (id,timestamp,userName) are applied last so the client can't override them.chat.deleteuseshasOwnProperty('canDelete')so an inherited value can never grant delete. lodash bumped to 4.18.1 as defense in depth.Test:
tests/prototype-pollution-chat.test.js.3. Prototype pollution → SQL injection via TypeORM —
POST /users/GET /users(routes/users.js)Exploit (see
exploits/prototype-pollution-typeorm.md):repo.save({ address: req.body.address })with typeorm 0.2.24 deep-merged{"address":{"__proto__":{"where":{"id":"2","where":null}}}}intoObject.prototype. The subsequentrepo.find({ id: 1 })then picked up the inheritedwhereand returned arbitrary rows; the same primitive is the SQL injection in GHSA for typeorm ≤0.2.24 (CVE-2020-8158).Fix:
pickUserFields()whitelistsname/address/role, requires own, string-valued properties, and returns 400 otherwise, so no nested object reaches the ORM.GET /usersnow passes an explicitfind({ where: { id: 1 } })(own property, so a polluted prototype can't shadow it). typeorm upgraded 0.2.24 → 0.3.31 (createConnection/getConnection→ exportedDataSourceintypeorm-db.js).Test:
tests/typeorm-injection.test.js(stubs the DataSource; also assertsfindstill receiveswhere: {id: 1}even withObject.prototype.wherepre-polluted).4. Insecure dependencies
merge/set, ReDoS, code injectionfind/save/update{"content":800}, seeexploits/mongoose-exploits.sh),mquerycode injection, prototype pollution viaSchema.path,$norsanitizeFilter bypassres.redirect, vulnerableqs+path-to-regexp, urlencoded DoSoverrides){@if cond=…}iseval-based;?device[]=Desktop'-require('child_process').exec(...)-'was RCE (exploits/dustjs-exploits.sh).about_new.dustnow uses native{?isDesktop}with the boolean computed server-side, anddeviceis coerced to a string.tapalone pulled ~400 packages incl. lodash 4.17.10, minimist, request…Code adapted for mongoose 8 (callback API removed → promises;
todo.remove(cb)→findByIdAndDelete). Two related hardening changes increate:contentmust be a string (400 otherwise — closes the Buffer memory-exposure path independent of the mongoose version), andexec('identify ' + url)→execFile('identify', [url])withvalidator.isURLso a crafted image URL can no longer inject shell commands.Tests:
tests/vulnerable-dependencies.test.js(minimum-version assertions + live_.mergepollution check) andtests/todo-create.test.js.Verification
npm run test:unit→ 22/22 pass./,/about_new,/login,/chat,/create,/usersexercised with the payloads fromexploits/→ all return 400/401/403 andObject.prototypestays clean.npm audit --omit=dev: 76 → 36 vulnerable packages; typeorm/mongoose/lodash/express/body-parser/qs/dust are clean.Out of scope / follow-ups (still flagged by
npm audit, not injection/proto-pollution related)adm-zip(zip-slip),st(path traversal),ms/humanize-ms/moment/validator/marked(ReDoS/XSS),ejs/ejs-locals/hbs(template RCE),express-fileupload,cfenv,npmconf,errorhandler,morgan,jquery. Note this is Snyk's vulnerable-by-design demo app, so some of these may be intentionally retained.Devin-Org: engineering
Link to Devin session: https://app.devin.ai/sessions/f5d251a253b048c8b80ea73e0eec29d6
Open in Devin Desktop: https://app.devin.ai/desktop/session/f5d251a253b048c8b80ea73e0eec29d6?variant=devin
Requested by: @rushcromer
Note
Devin errored when opening this Pull Request as rushcromer.
As a fallback, Devin opened this PR as itself.