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

Jump to bottom

Drop mkdirp and replace it with fs.mkdirSync #4200

Merged
merged 1 commit into from
Mar 19, 2020
Merged

Drop mkdirp and replace it with fs.mkdirSync #4200

merged 1 commit into from
Mar 19, 2020

Conversation

HyunSangHan
Copy link
Contributor

@HyunSangHan HyunSangHan commented Mar 14, 2020 •
edited
Loading

Description of the Change

  • Replace mkdirp with fs.mkdirSync using {recursive: true}
  • Drop the dependency mkdirp from Mocha

Alternate Designs

There is also a way to update to the latest mkdirp version, but Node.js version 10.12.0 has already added a native support for mkdirSync to create a directory recursively with {recursive: true} option as the following:

fs.mkdirSync(pathname, { recursive: true });

So, there is no longer necessary to depend on third-party packages. fs module is enough to us.

Why should this be in core?

mkdirp depends on an old version of minimist, and it has a prototype pollution vulnerability.

Benefits

Can create a new directory and any necessary subdirectories at the directory without mkdirp.

Possible Drawbacks

Can't think of any.

Applicable issues

I think it have to be released with semver-major(maybe v8.0.0), because the {recursive: true} option of fs.mkdir is supported by Node.js v10.12.0 or later.

Closes #4199

@HyunSangHan HyunSangHan force-pushed the issue-4199 branch 2 times, most recently from bc05d0c to 58e8216 Compare March 14, 2020 11:52
@coveralls
Copy link

coveralls commented Mar 14, 2020 •
edited
Loading

Coverage Status

Coverage increased (+0.06%) to 92.867% when pulling d09ad1b on HyunSangHan:issue-4199 into 2f26478 on mochajs:master.

@fabiosantoscode
Copy link
Contributor

An alternative implementation would be to update to the latest mkdirp version, which is not vulnerable.

@outsideris outsideris added area: node.js command-line-or-Node.js-specific area: security involving vulnerabilities labels Mar 15, 2020
@HyunSangHan HyunSangHan force-pushed the issue-4199 branch 2 times, most recently from 4946a1a to c94e09e Compare March 15, 2020 16:36
@HyunSangHan HyunSangHan changed the title [WIP] Replace mkdirp to fs.mkdir with recursive option Drop mkdirp and replace it with fs.mkdir Mar 15, 2020
@HyunSangHan HyunSangHan changed the title Drop mkdirp and replace it with fs.mkdir Drop mkdirp and replace it with fs.mkdirSync Mar 15, 2020
@HyunSangHan
Copy link
Contributor Author

HyunSangHan commented Mar 15, 2020 •
edited
Loading

@fabiosantoscode
Thank you for your comment.
That's also one of the valid methods, but Mocha is ready to drop Node.js version 8.x.x support (ref: #4164). After Mocha v8.0.0 release, I think, Mocha can use fs.mkdirSync so no longer depend on mkdirp.

@fabiosantoscode
Copy link
Contributor

Uh oh :) I still support node 8 in my library.

@bigman73
Copy link

Updating mkdirp seems like an easy fix without breaking backward compatibility

@JimmyBjorklund
Copy link

Adding this patch or upgrading to the latest mkdirp ( do not use minimst) is relevant to:
GHSA-7fhm-mqm4-2wp7

@walro walro mentioned this pull request Mar 16, 2020
Closed
@juergba
Copy link
Contributor

juergba commented Mar 16, 2020

The latest version of mkdirp supports Node >=10. It seems we have to drop Node v8 anyway.

@juergba juergba added the semver-major implementation requires increase of "major" version number; "breaking changes" label Mar 16, 2020
@juergba juergba added this to the v8.0.0 milestone Mar 16, 2020
@bigman73
Copy link

nodejs 8 is not supported as of 2020, so indeed why bother.

juergba
juergba reviewed Mar 17, 2020
View reviewed changes
Copy link
Contributor

@juergba juergba left a comment

Choose a reason for hiding this comment

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

@HyunSangHan thank you for this PR.

Please update the engines field in package.json and our docs to >=10.12.0.

test/reporters/xunit.spec.js Outdated Show resolved Hide resolved
@SebastianSpeitel
Copy link

An alternative implementation would be to update to the latest mkdirp version, which is not vulnerable.

Updating mkdirp seems like an easy fix without breaking backward compatibility

There is no update of mkdirp that fixes this.
The PR isn't even merged yet.

@pezzullig
Copy link

Great work on getting this PR done! 👍 Any chance of getting this reviewed and tagged any time soon?

@Hypnosphi
Copy link

An alternative implementation would be to update to the latest mkdirp version, which is not vulnerable.

Or even just to 0.5.3, to make the change minimal:

% npm info [email protected] dependencies 
{ minimist: '^1.2.5' }

@juergba
Copy link
Contributor

juergba commented Mar 18, 2020 •
edited
Loading

I will update today to [email protected] and publish as Mocha v7.1.1. This version is new and deprecated, but seems to fix this security issue.
yargs-parser is also reported to have security issues, so I will update to [email protected] and [email protected] .

@HyunSangHan this PR remains valid for Mocha v8.0.0.

Edit: [email protected] published

juergba
juergba reviewed Mar 19, 2020
View reviewed changes
Copy link
Contributor

@juergba juergba left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@HyunSangHan could you rebase, resolve conflicts and squash, please? lgtm

@HyunSangHan
Drop mkdirp and replace it with fs.mkdirSync
758868f 
- Replace mkdirp with fs.mkdirSync using {recursive: true}
- Drop the dependency mkdirp from Mocha
- Fix version number of docs and package.json
@HyunSangHan
Copy link
Contributor Author

@juergba
I did rebase and squash just now. Please let me know if there is something wrong. :-)

juergba
juergba approved these changes Mar 19, 2020
View reviewed changes
Copy link
Contributor

@juergba juergba left a comment

Choose a reason for hiding this comment

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

@HyunSangHan thank you

@juergba juergba merged commit f87825a into mochajs:master Mar 19, 2020
@jayaddison jayaddison mentioned this pull request Apr 7, 2020
Closed
@Omrisnyk Omrisnyk mentioned this pull request Dec 7, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juergba juergba juergba approved these changes

Assignees
No one assigned
Labels
area: node.js command-line-or-Node.js-specific area: security involving vulnerabilities semver-major implementation requires increase of "major" version number; "breaking changes"
Projects
None yet
Milestone
v8.0.0
Development

Successfully merging this pull request may close these issues.

Replace vulnerable mkdirp with fs.mkdir
10 participants
@HyunSangHan @coveralls @fabiosantoscode @bigman73 @JimmyBjorklund @juergba @SebastianSpeitel @pezzullig @Hypnosphi @outsideris