Update dependencies so "critical" vulnerabilities are no longer logged on the command line when running npm install

[paul23-git]

Sails version: 1.1
Node version: 8.12
NPM version: 6.9.0
DB adapter name: N/A
DB adapter version: N/A
Operating system: Linux

---

When one creates a new sails application, using:

sails new test-application

Many outdated libraries are installed, many of those libraries have some or several bugs and (more dangerously), vulnerabilities. Even worse, sails indicates those libraries are used by production, not making any difference between normal dependencies and dev dependencies in package.json.

This makes CI nearly impossible.

found 273 vulnerabilities (58 low, 211 moderate, 2 high, 2 critical) in 3874 scanned packages

Audit can fix the ones depending on a minor version of grunt (168 of them). However that still leaves a lot. I looked through the log and could summarise it as follow:

sails-hook-grunt
https://npmjs.com/advisories/785 (low) clean-css, patched in >= 4.1.11
https://npmjs.com/advisories/745 (moderate) underscore.string patched in >= 3.3.5
https://npmjs.com/advisories/543 (low) debug, patched in >= 3.1.0
https://npmjs.com/advisories/577 (low) lodash, patched in >=4.17.5
https://npmjs.com/advisories/782 (moderate) lodash, patched in >=4.17.11

sails-hook-sockets
https://npmjs.com/advisories/577 (low) lodash, patched in >=4.17.5
https://npmjs.com/advisories/782 (moderate) lodash, patched in >=4.17.11

sails
https://npmjs.com/advisories/612 (low) deep-extend, patched in >= 0.5.1
https://npmjs.com/advisories/663 (critical) open, no patch but alternative library opn
https://npmjs.com/advisories/577 (low) lodash, patched in >=4.17.5
https://npmjs.com/advisories/782 (moderate) lodash, patched in >=4.17.11

sails-hook-orm
https://npmjs.com/advisories/577 (low) lodash, patched in >=4.17.5
https://npmjs.com/advisories/782 (moderate) lodash, patched in >=4.17.11

I left out all intermediate libraries for brevity (some might be fixed by updating intermediate libraries). But as one can see most of the errors can simply be fixed by updating lodash.
What is left over can be fixed by moving dependencies

This can be solved in two ways:

Either bump all versions on the libraries. Or (and?) move the non-production libraries to the correct area in package.json.

[sailsbot]

@pulli23 Thanks for posting, we'll take a look as soon as possible.

---

For help with questions about Sails, click here. If you’re interested in hiring @sailsbot and her minions in Austin, click here.

[raqem]

Hi @pulli23,
Just like there were lots of major possibly breaking changes from Sails v12 to Sails v1 there is often major changes when dependencies Sails uses upgrade. For that reason we have locked what version of these dependences Sails uses. We realize some of these are starting to get on the older side but they work great for what Sails relies on them for!
Please check out this other issue for more information on were we stand with fixing these vulnerabilities.
~Cheers!

[johnabrams7]

@pulli23 also here's a video explaining the nature of these warnings further: https://www.youtube.com/watch?v=N7CwLAxISpM

[paul23-git]

@johnabrams7 While of course many warnings wouldn't be necessary "deadly" npm audit is one of the few tools we have verify that versions are safe and still actively maintained/fixes solved. Without that it would require to always manually check each and every commit for new dependencies, slowing down CI greatly. - That, or not using auditing software at all (which means you have no safety and might be running faulty code in useland, without you even knowing).

The problem is thus that either the maintainer is long gone, and thus bugs are in code used by the server. Or it is not used by the server (ie part of the grunt framework), and it should be a dev dependency.

This topic is not so much as solving the bugs (for that the other issue is better), but more about asking/officially wondering why tools are not installed as dev dependencies and if we can't move certain dependencies to this.

In the other topic "mikermcneil" already said that sails-hook-grunt is used "during development only": so that would mean that putting those not in dev-dependencies should be considered a bug? Or is there a better reason? I'm really interested in hearing the reasoning.

[mikermcneil]

@pulli23 We’re working hard to remove the noise and confusion that’s currently logged on npm. We’ve got more work to do, but we just want to take this opportunity be clear about where things currently stand, and to share our philosophy for dealing with these kinds of issues. Thank you for your attention to this issue and your positive attitude in getting it resolved. Most of us have been using sails in production for years, so it really helps when people remind us of the things they notice coming at the framework fresh.

Grunt-related things are dev dependencies as of sails v1 (e.g. check out the devDependencies in the package.json of a newly generated sails app using the web app template: grunt and sails-hook-grunt are there, rather than in dependencies)

The main issue is that npm audit (bizarrely, IMO) complains about dev dependencies by default. That’s strange, because dev dependencies are, by nature, not designed to be used for handling untrusted data and/or sensitive production scenarios.

Another issue is nested dependencies- npm install brings in deps and dev deps. It does not install dev deps OF those deps- but it still installs their normal deps.

So if a deeply nested dependency (even of a dev dependency!!) has some kind of vulnerability, even if the dependent package does not use the affected code path, a vulnerability notice is still shown.

This really, really sucks for open-source maintainers— especially for those of us who maintain something as ambitious as Sails. It creates a ton of work for us, as we have to wade through each and every “boy who cried wolf” notice and make sure it doesn’t actually pose a real risk for our users.

Despite the annoyance, that’s what we do.

It’s not fun, and I wish npm would change it. Again, just my 2¢, but I think this is happening because npm is incentivized to err on the side of being careful, even if that sometimes borders on security theater, and since they don’t make revenue from open source maintainers who deal with the fallout, there hasn’t been a huge push to change this kinda-weird default behavior. (Btw, I welcome everybody’s help in sharing this perspective and experience with the npm team, if you happen to agree with this point of view.)

Meanwhile, we’ll continue spending time where we can fluffing up our packages to try and make these annoying/scary messages go away, but bear in mind that, realistically, our top priorities are (1) identifying and fixing any real-world vulnerabilities that meaningfully endanger production use cases and (2) making it faster and easier to build better and better Node.js/Sails apps.

Making Sails look good when you run npm install is important to us, but it’ll never be as important as making sure the framework provides an efficient developer experience, remains stable, scales nicely, and maintains security best practices.

Hope that makes sense, and thanks again for bringing this up!

I’m going to wrap this issue up for now until we have more news to report. In the mean time, please just know that we keep a very close eye on these vulnerabilities and deprecation warnings, but that we also appreciate your help in notifying us about any critical vulnerability that you are able to recreate in a boilerplate Sails app.

Here’s what to do if you notice and verify a vulnerability in Sails:
https://sailsjs.com/security

[mikermcneil]

(Related to #4402 -- see #4402 (comment) which has some news on the latest related happenings)

[paul23-git]

@mikermcneil Thanks for the long response, I appreciate the time it must take. I just wish to add that in the generated package.json I see grunt (and everything else except the linter) under dependencies instead of devDependencies for empty configuration. (I notice they are under dev dependencies in the web app configuration).

Should I create a separate issue for that?

[mikermcneil]

Should I create a separate issue for that?

@pulli23 I have not experienced this, but if you're seeing that in a freshly-generated sails project (choosing the "Web app" template), with the latest version of sails globally installed, then yes- please do. Thanks!

[rachaelshaw]

@mikermcneil @pulli23 just took a look at some test projects I generated the other day with the latest version of Sails, and looks like grunt and sails-hook-grunt are listed as dev dependencies in the one generated with the "Web app" template, but are listed under dependencies for the "Empty" project.