DoS vulnerability in glob dependency

[lucioperca]

glob version 3.2.11 includes a dependency on minimatch 0.3.0, which contains a DoS vulnerability as documented here:

https://nodesecurity.io/advisories/118

This should be resolvable by updating to the current version of glob, which uses the up-to-date minimatch version.

[ScottFreeCode]

Unfortunately, we can't update to the latest version of Glob without losing compatibility with some of the Node versions currently supported (I can dig up the last place this was discussed if need be), so it's not going to be quite that simple... Ideally (for us), if Glob could publish a new 3.2.x version that doesn't change anything significant except updating Minimatch, that would allow us to fix this without needing a backward-incompatible version update on Mocha's part. I don't know if that's possible; I guess we'll have to ask them, or at least keep an eye out for new Glob 3.2.x versions.

I wonder, though, whether the way Glob uses Minimatch is subject to this vulnerability or not in the first place. Is there a corresponding Glob issue? (Then again, it may not matter -- people don't want to see warnings, especially security-related ones, and NPM's warning system is not fine-grained enough to prevent such false positives if they do exist.)

[lucioperca]

@ScottFreeCode For the second part, Glob has a corresponding issue to explicitly bump the version, since their dependency is technically on 2 || 3 right now:
isaacs/node-glob#268

This seems to be based on the advisory, rather than a specific instance of this vulnerability causing a problem, however.

[ScottFreeCode]

It just occurred to me that no matter the version of Glob, the version of Minimatch would have to be compatible with the versions of Node & NPM that Mocha supports -- e.g. not use ^ in its dependency versions in package.json. And it turns out the fixed Minimatch uses ^, so we can't upgrade to that without dropping support for those old Node versions.

It also just occurred to me that Mocha per se is not really vulnerable in any meaningful sense -- we're using it through Glob to allow pattern matching in the files to test... So while a user could blow up Glob using a malicious path, the only thing it would do is cause the test command to fail and the only people it'd hurt is themselves: you aren't taking test script paths from third parties -- well, maybe if you fail to look at a PR that changes the test script, but even then it would be far more effective for the PR author to flat out disable the tests and hide stuff than to force the tests to fail and make it obvious to continuous integration and such! So, yeah... This can't actually hurt us.

[EDITTED TO ADD: DISCLAIMER: I am not a security expert, just an overly analytical programmer; if anyone wants to get this reviewed by a security expert, I'd be happy to have my reasoning here confirmed or denied.]

What I wouldn't give to be able to suppress specific known-irrelevant NPM installation warnings from a project's dependencies...

[boneskull]

There's really no good solution to this other than what I just did, which was to fork minimatch, backport the patch, then fork glob to use the forked minimatch, then make mocha use the forked glob. See this branch. PR forthcoming if Travis passes.

[boneskull]

Watching this build

[boneskull]

Anyway, I did attempt to upgrade to [email protected], but that's evidently incompatible with IE8 (YAY BROWSER TESTS). So I'll need to downgrade and try again--but not tonight

[ScottFreeCode]

Huh; I didn't think the globbing/minimatch-related functionality was going into the browser code.

[boneskull]

Hmm good point. We should be able to ensure browserify skips glob

[boneskull]

Please see PR #2325 as a potential solution. I can't actually think of an alternative at this point that wouldn't require breaking Node.js v0.8 compat. If anyone can, I'm interested in hearing it

[hairyhenderson]

@boneskull - is Node.js v0.8 compatibility a requirement anymore? At this point it's pretty well unsupported by anyone else... Are there any documented mocha compatibility guarantees?

[gagern]

@hairyhenderson Semantic versioning implies this: Mocha 2 has been compatible with Node 0.8, so it must stay compatible. Mocha 3 would be free to drop support for 0.8. The current 3.0.0 milestone contains quite a number of items, so if that's the roadmap, we might have to wait a bit.

Others have suggested a major version bump just for the sake of getting rid of all these recent warnings due to outdated dependencies (graceful-fs, minimatch, jade resp. pug). Personally I'd welcome such a step. But perhaps it's possible to get all the breaking changes of the 3.0.0 milestone in place, release 3.0.0 including those changes, then add features mentioned in that milestone in a non-breaking way within the major version 3 line i.e. in 3.1.0 or so. I don't know.

[hairyhenderson]

@gagern - yeah, ok, that is a good point.

I'd definitely support bumping to v3.0.0 to drop 0.8 support and get dependencies up-to-date. Just because it's a major version bump doesn't mean it needs to include lots of new features...

[boneskull]

Just because it's a major version bump doesn't mean it needs to include lots of new features...

This is true. I'm starting to think #2325 is too heinous to merge and we should just bite the bullet and break v0.8. This would let us upgrade many dependencies.

cc @mochajs/core

[ScottFreeCode]

Sounds good to me.

[boneskull]

I'll see if I can't work on this over the weekend. Would love some help
sorting out all breaking changes. I'll be seeing what we can
upgrade--already started that.

[boneskull]

this is fixed in the v3.0.0 branch by 05c5e5c