Updated lodash in order to fix CVE-2018-3721 #1224
Conversation
| @@ -21,7 +21,7 @@ | |||
| "test": "cross-env BABEL_ENV=test NODE_PATH=./src nyc -s mocha -R dot --recursive tests/src -t 5s", | |||
| "test-compiled": "npm run prepublish && NODE_PATH=./lib mocha --compilers js:babel-register --recursive tests/src", | |||
| "test-all": "npm test && for resolver in ./resolvers/*; do cd $resolver && npm test && cd ../..; done", | |||
| "prepublish": "gulp prepublish", | |||
| "prepare": "gulp prepublish", | |||
There was a problem hiding this comment.
please revert this; "prepublish" is the correct script name, despite npm's badly worded warning.
There was a problem hiding this comment.
https://www.balena.io/blog/safely-migrating-away-from-prepublish-with-npm-4/
It's deprecated. I don't understand "badly worded warning". NPM's developers clearly described why this script is deprecated and how one can update it to new script name.
There was a problem hiding this comment.
There's no value in doing so; "prepublish" works in every version of npm. I doubt npm will ever remove it, and if/when they do, we can handle it then.
There was a problem hiding this comment.
| "prepare": "gulp prepublish", | |
| "prepublish": "gulp prepublish", |
| @@ -79,7 +79,7 @@ | |||
| "eslint-import-resolver-node": "^0.3.1", | |||
| "eslint-module-utils": "^2.2.0", | |||
| "has": "^1.0.1", | |||
| "lodash": "^4.17.4", | |||
| "lodash": "^4.17.11", | |||
There was a problem hiding this comment.
This is unnecessary, since ^4.17.4 is a semver range that includes 4.17.11 already. There's no CVE to resolve.
There was a problem hiding this comment.
I agree, it includes, but version 4.17.4 has vulnerability, so there should be no possibility to install this version, in order to prevent installing buggy code. Leaving it as ^4.17.4 still allows the vulnerable version to be installed (and it's already happening in one of my library that uses eslint-plugin-import). For a test, I forked and installed it locally with this change and it resolves the issue. So, at least we should bump this version to ^4.17.5
There was a problem hiding this comment.
If you install a newer version of lodash at the top level, it will dedupe across this package, since it's in the compatible version range. Separately, the CVE doesn't apply to this package's usage of lodash - so there's no actual issue here.
There was a problem hiding this comment.
Just wanted to clarify this with you @ljharb : I realise that it's been a while since you looked at this.
I have a project using eslint-plugin-import, and when I run npm audit it identifies this package as introducing a version of lodash vulnerable to prototype pollution - but to fix this issue I need to install a new version of lodash - so I'd create a dependancy in my project to lodash (even though it has no dependancy)
Services such as Snyk would - in all likelihood - identify this as a potential risk - but the way to mitigate that is to replace what was installed with this library to a newer version through my consuming project
There was a problem hiding this comment.
Since you have a lockfile in order to use npm audit, you can update the version of lodash without adding it to package.json using npm audit’s suggested commands.
prepareinstead of deprecatedprepublish(more information: resolving prepublish-on-install npm/npm#10074)