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

Update mkdirp for security #85

Closed
wants to merge 2 commits into from
Closed

Update mkdirp for security #85

wants to merge 2 commits into from

Conversation

SakiiCode
Copy link

@SakiiCode SakiiCode commented Mar 17, 2020

minimist had to be upgraded to 1.2.3

https://www.npmjs.com/advisories/1179

Edit:

PSA: There is a TEMPORARY way to force minimist to a higher version. Although unlikely but it may cause other issues! Use this only until this PR gets merged!

1) Add this to the "scripts" section of your your package.json

    "preinstall": "npx npm-force-resolutions"

2) Add this to the end of your package.json

  "resolutions": {
    "minimist": "^1.2.5"
  }

3) Run npm install
4) Be sure to check your dependency tree with npm ls minimist. Only the necessarily overridden packages should be marked as invalid. (children of extract-zip in this case)

│     └─┬ extract-zip@1.6.7
│       ├── minimist@1.2.5  extraneous
│       └─┬ mkdirp@0.5.1
│         └── minimist@1.2.5  invalid

@Hypnosphi
Copy link

@malept can you please merge & release?

@abrom
Copy link

abrom commented Mar 18, 2020

For reference, the changes from v0.5.1 to v0.5.3 of mkdirp are: isaacs/node-mkdirp@d4eff0f...v0.5.3 (the only practical change being the minimist dependency bump from 0.0.8 to ^1.2.5)

package.json Outdated Show resolved Hide resolved
@brettz9
Copy link

brettz9 commented Mar 19, 2020

Just to connect to issue filed: #86

Co-Authored-By: Karl Horky <karl.horky@gmail.com>
@brettz9
Copy link

brettz9 commented Mar 21, 2020

Anything else holding this back?

@cyclingzealot cyclingzealot mentioned this pull request Mar 21, 2020
@cyclingzealot
Copy link

@brettz9 No, except that @maxogden, who I believe has sole authority to execute this merge, has not been active on this project since May 2018, despite being active in private repos. It could be that he just needs a few more days for this to get to the top of his queue. He seems very busy.

@brettz9 : Are you here because of the dependency from puppeteer or another package?

Looking at his github profile and https://maxogden.com/, I suspect he is located in Portland, Washington. That state, along with California and New York, has been particularly hit hard by COVID-19, and it's only going to get worse. Wishing everyone the best.

@cyclingzealot
Copy link

@SakiiCode's suggestion of using https://www.npmjs.com/package/npm-force-resolutions was tried successfully (along with npx and entries in package.json) as documented in vuejs/vue-cli#5285 (comment)

@cyclingzealot
Copy link

cyclingzealot commented Mar 21, 2020

@SakiiCode There's a suggestion of using 1.0.3 (#86 (comment)) instead as it doesn't depend on minimist. I'm not familiar enough with pull requests to see if I, as commentator, can edit them.

@SakiiCode
Copy link
Author

SakiiCode commented Mar 21, 2020

@cyclingzealot 1.0.3 (besides other changes) uses promises instead of callbacks so migration requires the rewrite of the main and the testing script as well. Switching to it may be possible in the future but it's a bit out of scope of this PR.

Thanks for pointing out though

@daveisfera
Copy link

daveisfera commented Mar 21, 2020

@SakiiCode There's a suggestion of using 1.0.3 (#86 (comment)) instead as it doesn't depend on minimist. I'm not familiar enough with pull requests to see if I, as commentator, can edit them.

In node 10 (the currently oldest supported version), there's also a {recursive: true} option so mkdirp could potentially be skipped all together

@jfoclpf

This comment has been minimized.

@cyclingzealot
Copy link

cyclingzealot commented Mar 22, 2020

As suggested by @SakiiCode, I used https://www.npmjs.com/package/npm-force-resolutions as a temporary fix. I documented on SO at https://stackoverflow.com/a/60795003/1611925 and https://stackoverflow.com/a/60794976/1611925 . It installed without difficulty but I have not tested execution yet.

@brettz9
Copy link

brettz9 commented Mar 22, 2020

@brettz9 : Are you here because of the dependency from puppeteer or another package?

svg2png -> phantomjs-prebuilt

@brettz9 No, except that @maxogden, who I believe has sole authority to execute this merge, has not been active on this project since May 2018, despite being active in private repos. It could be that he just needs a few more days for this to get to the top of his queue. He seems very busy.

Sure. Though per https://www.npmjs.com/package/extract-zip @malept is listed as an npm "collaborator" who should I think per this designation also be able to make an update.

Looking at his github profile and https://maxogden.com/, I suspect he is located in Portland, Washington. That state, along with California and New York, has been particularly hit hard by COVID-19, and it's only going to get worse. Wishing everyone the best.

Indeed.

@jfoclpf : Maintainers who put their name on a project typically want to take a good look, run tests, etc., so even a one-line change can need to be shelved until a person has the time to look everything over and be satisfied that the specific fix is the best way to go. Of course one hopes with vulnerabilities, there can be some added priority, but if it comes down to it, any of us can release a forked project.

@jfoclpf
Copy link

jfoclpf commented Mar 22, 2020

@brettz9 If I understood correctly the only proposed difference is the version of a dependency, thus it is one line in package.json that changes. If npm test is well designed and configured in travis you don't even need to worry about tests cause you may only pull request if the test on the proposed changes passes successfully. When it comes to automatic pull requests from github regarding vulnerability I just blindly accept them. Look, I am also a developer of open source, and I perfectly understand we have no duties at all regarding our products that we set available for free. But here is just a minor version change in a dependency which is ready to go. Anyway this is a great package ;)

@SakiiCode
Copy link
Author

SakiiCode commented Mar 22, 2020

@cyclingzealot I added a tutorial to the first post, please check if you did the same steps and got the same results.

@malept
Copy link
Collaborator

malept commented Mar 24, 2020

Thanks for your patience in these uncertain times. I've released a version of extract-zip depending on mkdir 0.5.4 in version 1.6.8.

@brettz9
Copy link

brettz9 commented Mar 25, 2020

Thank you very much, @malept, and thanks for your contributions to the project! Just may want to clarify that you released a version depending on "mkdirp" (as there is also a "mkdir" npm package as well). :-) Best wishes...

@jfoclpf
Copy link

jfoclpf commented Mar 25, 2020

Thanks @malept :) All the best!

jennifer-shehane pushed a commit to cypress-io/cypress that referenced this pull request Mar 26, 2020
* Upgrade extract-zip to address vulnerability

Versions of extract-zip before `1.6.8` depended on a vulnerable version of `minimist` via `mkdirp`:

max-mapper/extract-zip#85 (comment)

Minimist vulnerability: https://app.snyk.io/vuln/SNYK-JS-MINIMIST-559764

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

Successfully merging this pull request may close these issues.

None yet

9 participants