Update dev deps to make tests work on MacOS and resolve Critical arbitrary code execution/command injection vulnerabilities #190
Conversation
This is latest version still compatible with Node.js v0.10.0. Fixes two critical command injection vulnerabilities.
This upgrades Grunt to v1.0.4, which is the latest of the v1.0.x series and resolves 6 critical vulnerabilities, including several arbitrary code execution and command injection vulnerabilities. There are newer versions of Grunt, but this is the minimum update. This also required upgrading some related packages that were not compatible with Grunt 1.x. It also required forking and vendoring grunt-template-jasmine-requirejs, which is no longer maintained and was not compatible. I've tried to make the minimum viable changes there and mark all the changes clearly. Finally, this required a bunch of minor changes to tests, since the compatible versions of grunt-contrib-jasmine all use Jasmine 2.x instead of 1.x, which we were currently on. Unfortunately, this leaves the Node.js tests on Jasmine 1.x (since they use a different Grunt plugin). I'll see if it's possible to update that in a follow-on commit.
It turns out that I added the polyfill midway through work on the last commit, and some other change later caused Jasmine 2.99.x to get installed, which means `expect().nothing()` is now built in.
The previous version *appeared* to work, but was not officially listed as compatible. This upgrades to v1.1.0, which also fixes some critical security vulnerabilities to boot!
This appeared to work fine already with Grunt 1.x, but was not declared to do so and triggered peer dependency issues. There is a newer version (2.x), but it requires newer versions of Node.js than we currently support here.
This upgrades grunt-contrib-uglify to to v3.4.0, which is the newest version compatible with Node.js v0.10.x. The previous version appeared to work fine with Grunt 1.x, but was declared to be OK, and caused peer dependency warnings. This resolves that issue as well as several security vulnerabilities (yay!).
The previous version appeared to work fine, but upgrade to grunt-contrib-qunit >= 0.6.0 fixes a peer dependency warning with current versions of Grunt. I've upgraded all the way v2.0.0 here, since that is still compatible with Node.js v0.10.0. v3.0 is also compatible, but switches to headless Chrome, and for now I think it keeps things simpler to have all browser tests using Phantom (but later we should switch to Chrome if we can, since the PhantomJS project is quite dead).
This upgrades grunt-preprocess to v5.1.0, which is both the latest version and the first version that declares itself compatible with Grunt 1.x. Previous versions *appeared* to work fine, but caused peer dependency warnings, which this fixes. It also resolves a few security vulnerabilities along the way.
|
Also! This does not work in Node.js 12+ (that is, it works in Node.js 6.0.0–10.x inclusive) There appear to be some issues with newer Node.js versions in either jasmine-node or Grunt somewhere that need more investigation. I figured it was better to get this incremental improvement in first. |
|
Also just realized I did not clarify this issue and its fix:
The core problem here was an old version of PhantomJS. The updates in this PR cause us to install and use PhantomJS v2.x, which works on current versions of MacOS. (Worth noting: the PhantomJS project is very dead, and newer versions of the tools that were using it have switched to Pupppeteer/headless Chrome. Those updates require newer versions of Node.js, though, so I did not update to them here.) |
|
This looks superb! This must've taken quite a bit of work, thank you 🙏
In both cases, I think that's sensible, and while this is very helpful, I don't think it's worth trying to incrementally update this further for modern node etc. It's going to be a significant amount of work, and it would probably be better spent just replacing the test setup entirely, to rewrite it completely with modern tools. Or, more realistically, as loglevel is mostly in 'done' maintenance don't-change-too-much mode just running the tests in an old version of node is likely to be a perfectly fine workaround for the few times that's required in future. In any world where somebody has the time & interest to build out loglevel v2 and take this forward with more substantial changes, I think the first step would be creating a new build env for modern node & tools, starting from a mostly clean slate.
I think that's a reasonable approach. Regarding version support, while the package aims to support v0.6.0 at runtime (entirely because updating that would be a breaking change) it's not the end of the world if the dev environment requires a later version. It would be nice to be able to run the tests in older versions, so far back as is practical, but again, the package is more or less done. Larger changes that might break things for those old versions should be few & far between, in reality there should be vanishingly small numbers of actual users on those versions nowadays, and so it seems OK not worry about that too much. |
I rebased this work on top of pimterry#190, which means the tests now run in Jasmine 2.x, which treats error strings differently from error objects in `expect(...).toThrow(...)`, so this needed some updating.
Two tests in the cookies + localstorage suite were handling async operations incorrectly (I think this is a change in Jasmine v2, and I missed these in pimterry#190 because these tests don't run from the command line). This fixes them, which resolves one test failure. The other is still failing because of an actual bug in the library.
I rebased this work on top of pimterry#190, which means the tests now run in Jasmine 2.x, which treats error strings differently from error objects in `expect(...).toThrow(...)`, so this needed some updating.
While working on #189, I discovered I couldn’t run tests locally on any of my Macs (both x86/Intel and ARM/Apple machines). Installing the dev deps also fired off a laundry list of warnings about security vulnerabilities, including several critically rated ones about arbitrary code execution or command injection.
Generally speaking, Loglevel doesn’t do much at build/test time and doesn’t read arbitrary input that could compromise things, but that doesn’t mean some transitive dependencies don’t read some config from surprising places (many tests also run in a version of PhantomJS that has and arbitrary file read vulnerability, and any network traffic that might happen to occur in Phantom could potentially be a route for compromise). Anyway, I don’t think the risks are super high here, but they aren’t non-existent, and are probably important to fix. Plus, I’d like to be able to develop locally instead of on a remote linux machine, which I had to do here. 🙂
So! I generally made the minimum updates possible here that fix critical arbitrary execution or command injection vulnerabilities (there are still other critical vulns left here, but they are IMO not a big risk given the dev-time-only context we are in).
I also only upgraded things as far as they could go without requiring a Node.js version >0.10.0. That said, this is still newer than v0.6.0, which is what this package aims to be compatible with, meaning we cannot run tests on all versions of Node.js that we want to support. But:
npm testand build (npm run dist) toolchain seem to work fine, so I ignored these warnings. (And the warnings predate this PR anyway.)This updates Uglify, which results in differences to
dist/loglevel.min.js, but everything seems to still work fine, so I think this is safe.There are also a bunch of test updates because the only versions of grunt-contrib-jasmine that solved issues upgrade from Jasmine v1 to v2:
expect().nothing()calls in these cases. That expectation was added to Jasmine specifically to use in these cases.Finally, the biggest change is that I had to vendor
grunt-template-jasmine-requirejsand alter some of the code in it. The package is no longer maintained, and no published versions are compatible with Grunt 1.x. Every change I made to it is marked with// LOGLEVEL-FORK: <description>comments. For example: https://github.com/Mr0grog/loglevel/blob/5f3764e7b1523c8052d3e30f3bb5f63933d6cfb0/vendor/grunt-template-jasmine-requirejs/src/template-jasmine-requirejs.js#L193-L195I chose to include the entire package source when doing this. Alternatively, we could:
package.json, etc.), or