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.

Sign up for GitHub

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
Closed

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

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
54542ac
Updated lodash in order to fix CVE-2018-3721
Oct 31, 2018
8f77398
Revert bumping version package
Oct 31, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "eslint-plugin-import",
"version": "2.14.0",
"version": "2.14.1",
adam-golab marked this conversation as resolved.
Show resolved Hide resolved
"description": "Import with sanity.",
"engines": {
"node": ">=4"
Expand All @@ -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",

"coveralls": "nyc report --reporter lcovonly && cat ./coverage/lcov.info | coveralls"
},
"repository": {
Expand Down Expand Up @@ -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.

Copy link

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.

"minimatch": "^3.0.3",
"read-pkg-up": "^2.0.0",
"resolve": "^1.6.0"
Expand Down