balderdashy / sails

Realtime MVC Framework for Node.js
https://sailsjs.com
MIT License
22.82k stars 1.95k forks source link

Found 71 vulnerabilities, Severity:68 Low | 3 high #4402

Open erwinsetiawan opened 6 years ago

erwinsetiawan commented 6 years ago

Sails version: 1.0 Node version: 10 NPM version: 6.0.2 DB adapter name: sails-mongo DB adapter version: 1.0.1 Operating system: Mac OSX 10.11.6


Hi Just success installing sailsjs and can do sails lift.

But when i just remove directory node_modules and run npm install again it will appear:

71 vulnerabilities found [20220 packages audited] Severity: 68 Low | 3 High

Is it ok? and if I do npm audit there is a lot of outdate dependencies that need to update

sailsbot commented 6 years ago

Hi @erwinsetiawan! It looks like you may have removed some required elements from the initial comment template, without which I can't verify that this post meets our contribution guidelines. To re-open this issue, please copy the template from here, paste it at the beginning of your initial comment, and follow the instructions in the text. Then post a new comment (e.g. "ok, fixed!") so that I know to go back and check.

Sorry to be a hassle, but following these instructions ensures that we can help you in the best way possible and keep the Sails project running smoothly.

*If you feel this message is in error, or you want to debate the merits of my existence (sniffle), please contact inquiries@sailsjs.com

sailsbot commented 6 years ago

Sorry to be a hassle, but it looks like your issue is still missing some required info. Please double-check your initial comment and try again.

*If you feel this message is in error, or you want to debate the merits of my existence (sniffle), please contact inquiries@sailsjs.com

sailsbot commented 6 years ago

Sorry to be a hassle, but it looks like your issue is still missing some required info. Please double-check your initial comment and try again.

*If you feel this message is in error, or you want to debate the merits of my existence (sniffle), please contact inquiries@sailsjs.com

mikermcneil commented 6 years ago

@erwinsetiawan thanks for bringing this up!

See https://trello.com/c/W868yIWj for status on this: image

We've made some progress already and addressed and published patches re: the reports where I've been able to verify there is a real issue. More context about this new on-every-install feature of NPM 6: https://twitter.com/brianleroux/status/994609068105859075

tldr; I'm comfortable with the remaining security reports, and the explanations in that Trello card. I'm continuing to work on making those warnings go away, and PRs are definitely welcome if anyone has time to chip in.

NachtRitter commented 6 years ago

@mikermcneil, are you welcome PRs with such problems solving to v0.12 and v0.11 branches? Because it's difficult task to update sails in big enterprise projects.I could say it's unreal task. So in our company we use even v0.11.5 and 0.12.14 without any dreams about updating...

elipeters commented 6 years ago

Sorry to hi-jack this thread but after an audit fix...

  Critical        Command Injection

  Package         open

  Patched in      No patch available

  Dependency of   sails

  Path            sails > machinepack-process > open

  More info       https://nodesecurity.io/advisories/663

  Critical        Command Injection

  Package         open

  Patched in      No patch available

  Dependency of   sails

  Path            sails > sails-generate > machinepack-process > open

  More info       https://nodesecurity.io/advisories/663

  High            Denial of Service

  Package         http-proxy-agent

  Patched in      >=2.1.0

  Dependency of   sails-hook-organics

  Path            sails-hook-organics > mailgun-js > proxy-agent >
                  http-proxy-agent

  More info       https://nodesecurity.io/advisories/607

  High            Denial of Service

  Package         https-proxy-agent

  Patched in      >=2.2.0

  Dependency of   sails-hook-organics

  Path            sails-hook-organics > mailgun-js > proxy-agent >
                  https-proxy-agent

  More info       https://nodesecurity.io/advisories/593

...

found 70 vulnerabilities (63 low, 2 moderate, 3 high, 2 critical) in 4781 scanned packages
  run `npm audit fix` to fix 3 of them.
  67 vulnerabilities require manual review. See the full report for details.

Most of the vulnerabilities seem to be related to machinepack and mailgun.

JedI-O commented 6 years ago

Just some additional info: For Sails 1.0.2, I get now 26 vulnerabilities (24 low, 2 critical) of which the two critical issues have the same root: module open used in machinepack-process. However, in machinepack-process, the problem was already fixed with commit https://github.com/sailshq/machinepack-process/commit/a7e0bd00ce9813fc2e1527b00fe1eb85c36e275d , but in Sails and Sails-generate an old machinepack-process is still referenced. Many of the low priority vulnerabilities are related to an outdated lodash version referenced from various Sails packages.

dan-astiak commented 6 years ago

Hello, hope these vulnerabilities get a fix soon, just upgraded to node 10 and npm 6.4 and have about 96 vulnerabilities most of them because of sails and it's dependencies.

dancrumb commented 5 years ago

Any movement on this? Since the root issue is resolved in sailshq/machinepack-process, is there anything further to do beyond updating the Sails dependency and pushing out a new release?

cupofjoakim commented 5 years ago

I'd like an update for this as well. Just did a fresh setup on my v8.12.0 environment after installing the cli and I got this:

found 62 vulnerabilities (58 low, 1 moderate, 1 high, 2 critical)

I mean, no sane person would deploy an app with that many vulnerabilities, right?

wulfsolter commented 5 years ago

FWIW: I'm in no way associated with Balderdashy / Sails.

@cupofjoakim Most sane people run many many products with many many more vulnerabilities. Try having a look at https://www.cvedetails.com/top-50-products.php and you'll find you have more than a few on your computer right now. Quite a bit more! I think PRs and contributing developers are always welcome 😉 Most software has many many more vulns, and it's great that npm now alerts people, but these alerts causing people to nag already busy developers.

Submit a PR, send a donation or subscribe to premium support! I mean, no sane person would expect things for free, right?

SethArchambault commented 5 years ago

This conversation popped up on google when I searched for npm audit vulnerabilities - @wulfsolter Thanks for that link, that was real eye opening!

I think this is still a very legit concern for all users of npm products - consider that having 71 vulnerabilities in 2019, would be place you at #5 on the list!

I can imagine this might be comparing apples to oranges.. perhaps CVEs are more dangerous that node advisories, or maybe the exploitable count is much lower, but still.

wulfsolter commented 5 years ago

Very much apples to oranges - most of the "vulnerabilities" are conceptual. A case of "Prototype Pollution" does not make for a vulnerability. Sure, the code could be written better, but to call it a vulnerability is scaremongering by npm.

If this amount of scaremongering means we'll have better code in a few years time, fantastic. If it means people give up, then it's a shame.

The single "critical" level vuln (npm lists it twice because the same package is installed twice due to npm being a bit less than optimal for de-duplication) is https://www.npmjs.com/advisories/663. A package that is designed to open files could do bad things if passed unsanitized data? Really simple solution, don't give it untrusted data from the users. I've had a look how that code is called, and I'm very happy running Sails in production.

So by that count, the 71 "vulnerabilities" becomes 1 thing to keep an eye on and 0 vulnerabilities. Upstream most of these things have been migrated, they'll trickle down soon enough but the reason they're not immediate is because they're really not "vulnerabilities" nor are they serious.

mikermcneil commented 5 years ago

@wulfsolter @SethArchambault @cupofjoakim @dancrumb @dan-astiak @JedI-O @elipeters @erwinsetiawan Thanks everyone for your input! I think that @wulfsolter is spot on here, but I also want to add my 2¢ (hopefully as a bit of reassurance):

  1. I understand where you're coming from, this is important to me, and I agree that this needs to change.
  2. Even though it's super annoying to deal with, and even though it's an ongoing battle that will resurface almost as soon as we get all of the packages cleaned up, we will deal with this eventually.
  3. We drive ourselves nuts with these things. The Sails core team has only dealt with this problem completely (every single vulnerability on Snyk/NS and then later on NPM) three times in the past, each time spending at least one dev week of full time work (split across 2-3 folks). But much more often, we've also dealt with specific vulnerabilities that we felt were of near-term concern because they actually impacted a code path that we use somewhere in Sails. Still, that's not a satisfactory answer and we have more work to do.
  4. We can't always prioritize issues like this, but this particular scenario is something that I know hurts the framework and our community, because regardless of whether or not any particular vulnerability is actually a concern, all that noise makes Sails look bad. This nags at me often-- especially because making the necessary changes can be a delicate exercise, and I often find that I need to be personally involved to make sure everything continues to work properly for all of the people out there trusting Sails for their production Node.js apps. For the foreseeable future, I am the bottleneck to getting this resolved. I want to address this, very badly.
  5. On three different occasions now, I've spent the afternoon working side by side on addressing this with other folks on the core team. I'm hoping to get to where we can spread some of the work around internally, but also to where we can a better job at (safely) capitalizing on the great efforts from other contributors who have made PRs related to removing these vulnerability alerts.
  6. I'm excited to say we made some good progress yesterday (thanks mostly to @rachaelshaw). I'm less excited to say that another vulnerability (in mocha, a dev dependency) emerged halfway through yesterday afternoon and erased half of the work she did. It's discouraging, but we'll keep trying. Thank you for everyone who has helped!

See also issue #4699 -- especially https://github.com/balderdashy/sails/issues/4699#issuecomment-483829719, which has some more info