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

Updated lodash in order to fix CVE-2018-3721 #1224

Closed
wants to merge 2 commits into from

Conversation

adam-golab
Copy link

@coveralls
Copy link

coveralls commented Oct 31, 2018

Coverage Status

Coverage remained the same at 97.356% when pulling 8f77398 on adam-golab:master into db471a8 on benmosher:master.

package.json Outdated Show resolved Hide resolved
@@ -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",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please revert this; "prepublish" is the correct script name, despite npm's badly worded warning.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"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",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unnecessary, since ^4.17.4 is a semver range that includes 4.17.11 already. There's no CVE to resolve.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Successfully merging this pull request may close these issues.

4 participants