Upgrade Growl to 1.10.2

[brimtown]

• Growl 1.9.2 is vulnerable to arbitrary code injection,
  and causes security warnings for Mocha users
• Upgrade Growl to 1.10.2, and address issue Vulnerability in dependency growl 1.9.2 #2791

[jsf-clabot]

All committers have signed the CLA.

[brimtown]

@ScottFreeCode I just noticed that #2924 was opened and closed about this issue already. Let me know what I can do to help get this merged!

[coveralls]

Coverage increased (+0.04%) to 89.017% when pulling 4b9d654 on brimtown:issue-2791/bump-growl into 958fbb4 on mochajs:master.

[ScottFreeCode]

If Growl has released a working version containing the security fix without loss of compatibility with any of the browsers and Node versions Mocha 3.x supports, then we should use that exact version. (Based on the dependencies all being exact versions as it is, I'm going to say that whichever version of Growl we use should be specified exactly, by the way.) However, if updating Growl at all requires loss of support for some Node versions and/or browsers, then we'll need to get a few pieces of info:

• Whether it makes sense to use Growl in the browser bundle in the first place. (If it does not, then we should exclude Growl from the browser bundle anyway and not losing support for any browsers will be a side benefit.)
• With which Node versions and (if applicable, see point above) browsers is it incompatible.
• Whether Growl's vulnerability actually causes a vulnerability in Mocha. (For example, if it can only be accessed through Mocha by controlling the JS code executed for test cases, then an attacker would not be able to use the Growl vulnerability through Mocha unless they were in a position to send arbitrary JS code such that they could spawn and exec directly even with Growl fixed.)
• Whether the Growl integration can be recreated by users of Mocha without Mocha itself using the Growl code.
• Maybe even whether there's any demand for the integration in the first place (Growl is an interface to send desktop notifications, right? does a test runner even need desktop notifications?)

Then with that info the Mocha team would need to discuss which option we'd prefer:

• Releasing a semver "major" (backwards-incompatible) that drops support for those Node and (potentially) browser versions.
• Releasing a semver "major" (backwards-incompatible) that drops Growl integration.
• Updating Growl and importing (requireing) it only when it's actually used instead of always, assuming this is possible (using Mocha + Growl + Node < 4 would be unsupported, but Mocha + Node < 4 in general would still work).
• Forking Growl to include the vulnerability fix but exclude whatever breaks support for environments Mocha should still support.
• Releasing a semver "major" (backwards-incompatible) that turns Growl into a peer dependency (or removes it as an explicit dependency altogether if peer dependencies can't be satisfied by forks) only imported (required) if available, again assuming this is possible, so that which version or fork of Growl to use is up to the user.
• Telling people not to rely solely on the accuracy of security scans that only look at presence of reported vulnerabilities in the dependency tree, because security requires threat modelling and attack surface analysis. (NOTE: this one would ONLY be an option IF Growl does not actually cause a vulnerability in Mocha as discussed in the "things we'd need to find out" point.)

To reiterate the very first point: all that is if we can't just pin a Growl version that fixes the vulnerability without losing support for any particular environment, which would be the obvious easy solution as long as such a version of Growl exists.

[brimtown]

Thanks for such a quick and detailed response! From digging around the history in growl, it appears that the vulnerable line in question was some of the earliest code committed. It doesn't look likely that there's a version we can pin that would allow us to sidestep the issue.

It seems like the bulk of the compatibility issues are related to the (relatively little) ES6 that has been added since 1.9.2. I've opened an issue asking about whether that will continue to be the case moving forward. spawn was added way back in 0.1.90, so that shouldn't be a worry from a compatibility standpoint. A fork from growl 1.9.2 incorporating spawn instead of exec but without the ES6 would probably not be too difficult to accomplish.

I don't have much input on whether growl belongs in the browser bundle, what the attack vectors actually are, or whether there's any demand for growl as part of Mocha in the first place, but I'd be happy to hear others chime in!

[ScottFreeCode]

Even just a version 1.9.3 that fixed the vulnerability without changing anything else would presumably do. Despite the "minor" (semver new-feature) version bump, it's not clear to me that anything has been done to the code other than fix the vulnerability, linting and conversion to use let and const; just doing the first of those things in one step before the cleanup steps would let us keep using it, and then we could review any future changes that actually affect functionality if and when they happen (presumably they'd be less urgent than a security fix as long as no other vulnerabilities are discovered).

[ScottFreeCode]

Updated my list of possible options with a "conditional require if feasible" option that came to mind this morning.

By the way, if I forgot to say so before, thanks for the PR! Hopefully we'll be able to review this issue soon.

[ScottFreeCode]

Added a "peer dependency" option to the list of available solutions; it's similar to the "only require Growl if it's used" option in that it basically removes Growl from the picture for those who aren't using it, but requires a semver-major release for the sake of those that do use Growl (as they'll have to start installing it themselves), while on the other hand providing maximum flexibility because it keeps the integration while moving the choice of Growl version to the user.

[boneskull]

thanks. we've worked this in to another set of PRs