Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
erwinsetiawan opened this issue May 11, 2018 · 14 comments
Open

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

erwinsetiawan opened this issue May 11, 2018 · 14 comments
Assignees
Labels
clutter from npm install https://twitter.com/mikermcneil/status/966895462706954240 helpful info or workaround npm install issue An issue running `npm install`, whether that's inside of a Sails app, or with `npm install sails -g`

Comments

@erwinsetiawan
Copy link

erwinsetiawan commented May 11, 2018

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
Copy link

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
Copy link

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
Copy link

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
Copy link
Member

mikermcneil commented May 11, 2018

@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.

@mikermcneil mikermcneil added needs better error message clutter from npm install https://twitter.com/mikermcneil/status/966895462706954240 npm install issue An issue running `npm install`, whether that's inside of a Sails app, or with `npm install sails -g` and removed needs cleanup needs better error message clutter from npm install https://twitter.com/mikermcneil/status/966895462706954240 labels May 11, 2018
@mikermcneil mikermcneil self-assigned this May 11, 2018
@NachtRitter
Copy link

@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...

@erwinsetiawan erwinsetiawan changed the title Found 17 vulnerabilities, Severity:68 Low | 3 high Found 71 vulnerabilities, Severity:68 Low | 3 high May 14, 2018
@elipeters
Copy link

elipeters commented Jul 30, 2018

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
Copy link

JedI-O commented Aug 14, 2018

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 sailshq/machinepack-process@a7e0bd0 , 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
Copy link

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
Copy link

dancrumb commented Oct 5, 2018

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
Copy link

cupofjoakim commented Oct 6, 2018

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
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

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
Copy link
Member

@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 #4699 (comment), which has some more info

@sailsbot sailsbot added the clutter from npm install https://twitter.com/mikermcneil/status/966895462706954240 label Apr 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clutter from npm install https://twitter.com/mikermcneil/status/966895462706954240 helpful info or workaround npm install issue An issue running `npm install`, whether that's inside of a Sails app, or with `npm install sails -g`
Development

No branches or pull requests