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

Eliminate transitive JS dependencies on old versions of minimist #3516

Closed
nfelt opened this issue Apr 14, 2020 · 2 comments · Fixed by #3232
Closed

Eliminate transitive JS dependencies on old versions of minimist #3516

nfelt opened this issue Apr 14, 2020 · 2 comments · Fixed by #3232

Comments

@nfelt
Copy link
Contributor

nfelt commented Apr 14, 2020

Old versions of the miminist CLI parsing JS library have a security hole that allows polluting the Object prototype: https://www.npmjs.com/advisories/1179 The fix is to update to version 1.2.3 or later (0.2.1 is also patched for 0.x but it would be best to just update to 1.2.3+).

The only way that we depend on old versions of minimist is via a wrapper library optimist that was abandoned at version 0.6.1 back in 2014, so realistically what this means is upgrading our dependencies to avoid a dependency on optimist at all.

We have two dependency paths that lead via optimist to the old minimist:

minimist_deps

Note that since the direct dependencies involved are both JS test dependencies, I don't think there is any actual security implication to the old minimist since the vulnerability involves being able to control the CLI arguments passed to the parser, and so at best this could be controlled during test execution time to "attack" the javascript being run within a test. That said, GitHub is still reporting the issue, and it would be good to clean this up just so it stops cropping up.

The handlebars dependency path is easy to fix since a compatible recent release no longer has the optimist dep, so we can just update it: handlebars-lang/handlebars.js#1658

The karma dependency path is more annoying. The recently released karma 5.0.0 package removes the bad dep thanks to karma-runner/karma#3451. However, our dependency on @bazel/karma@^0.36.1 depends on karma@^4.0.0 meaning we need to update rules_nodejs (which provides @bazel/karma) in order to safely update karma.

Updating rules_nodejs has proven to be a pain. It would look like 0.41.0 would suffice since it includes bazel-contrib/rules_nodejs#1346 which relaxes the karma dep to just >=4.0.0, but when I tried that I hit various issues getting a combination of deps that would actually work, including a version of @angular/bazel (which we have at ^8.2.14) that played nicely with the respective version of rules_nodejs - instead I hit some variation of bazel-contrib/rules_nodejs#1337. Fixing that requires an update to @angular/bazel that in turns requires a more recent rules_nodejs, and then we start hitting other incompatible changes (for example https://github.com/bazelbuild/rules_nodejs/wiki#migrating-off-internalrollup among others).

@nfelt
Copy link
Contributor Author

nfelt commented Apr 15, 2020

PR #3517 addresses one dependency path.

The other dependency path requires updating rules_nodejs which #3232 already does, but it's blocked on a Bazel version bump #3239 which is hard to get past Travis.

@stephanwlee stephanwlee linked a pull request Apr 22, 2020 that will close this issue
@stephanwlee
Copy link
Contributor

Fixed by #3232.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants