Remove `mkdirp@^0.5.1` due to CVE warnings, use native recursive `fs.mkdir` #768

ericanderson · 2022-02-01T23:20:18Z

Summary

mkdirp was triggering github security warnings. Upgrading version will resolve that.

Details

Github security warnings. The ones where you are the owner of a project and you visit the project's home page and get a giant warning from GitHub telling you there are CVE's in libraries within your repo.

I don't know for sure if metro was or was not vulnerable. I for sure know this wouldn't affect someone's production code. What I do know is that humans build tolerance and blindness to things over time. Having a security warning linger on a repo/project is a long term risk to other, real security vulnerabilities being seen and fixed.

This version bump fixes the issue because it is the recommended version to resolve the CVE.

The code delta is to deal with the vulnerable library having a major semver change and thus the code needed to be adapted.

Test plan

yarn test

facebook-github-bot · 2022-02-01T23:20:22Z

Hi @ericanderson!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

facebook-github-bot · 2022-02-02T00:21:35Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

philIip

thanks @ericanderson for working on this! in the diff summary, can you give some examples of the security warnings and why this version bump and implementation change fixes them? thanks!

packages/metro-memory-fs/src/index.js

ericanderson · 2022-02-03T19:38:02Z

@philIip Github security warnings. The ones where you are the owner of a project and you visit the project's home page and get a giant warning from GitHub telling you there are CVE's in libraries within your repo.

I don't know for sure if metro was or was not vulnerable. I'm sure this wouldn't affect someone's production code. What I do know is that humans build tolerance and blindness to things over time. Having a security warning linger on a repo/project is a long term risk to other, real security vulnerabilities being seen and fixed.

This version bump fixes the issue because it is the recommended version to resolve the CVE.

The code delta is to deal with the vulnerable library having a major semver change and thus the code needed to be adapted.

facebook-github-bot · 2022-02-03T20:32:22Z

@philIip has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

packages/metro-memory-fs/src/index.js

ericanderson · 2022-02-04T17:00:15Z

As an aside which shouldn't block this change, it seems like maintaining metro-memory-fs is an unnecessary use of resources. This package is only used for tests (at least within this repo).

The memfs library seems complete and well maintained. It is what webpack switched to after abandoning its own memory-fs (top right corner see deprecation notice).

facebook-github-bot · 2022-02-08T09:06:58Z

@rh389 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

robhogan · 2022-02-09T16:25:21Z

Hi @ericanderson - thanks for working on this. Unfortunately, it's proving problematic to merge internally due to the way we manage direct dependencies across the containing workspace (upshot being we'd have to update mkdirp in a lot of other places in order to merge this). Sorry that has an effect on a perfectly good PR.

To be honest, I'm thinking it'd be simpler to eliminate mkdirp from metro entirely - we support node >= 12 so the native recursive option is available to us and should be all we need. If you're interested in pivoting this PR in that direction it'd be very welcome, completely understand if not - apologies again for the wasted time.

ericanderson · 2022-02-09T16:39:41Z

@rh389 fair enough, will pivot as I'd prefer to get these warnings out of my repos.

robhogan

I think we could just tidy up the async call with the fs promises API, otherwise perfect (and thanks again)

Edit: Ah, also looks like we'll need to beef up the implementation of recursive in metro-memory-fs a bit

robhogan · 2022-02-09T18:30:01Z

packages/metro/src/shared/output/RamBundle/as-assets.js

+  const mkdirPromised = util.promisify(fs.mkdir);
+  return mkdirPromised(dirName, {recursive: true}).then(() => undefined);


Suggested change

const mkdirPromised = util.promisify(fs.mkdir);

return mkdirPromised(dirName, {recursive: true}).then(() => undefined);

return fsPromises.mkdir(dirName, {recursive: true});

robhogan · 2022-02-09T18:30:19Z

packages/metro/src/shared/output/RamBundle/as-assets.js

@@ -86,9 +87,8 @@ function saveAsAssets(
 }

 function createDir(dirName: string): Promise<empty> {


Suggested change

function createDir(dirName: string): Promise<empty> {

function createDir(dirName: string): Promise<void> {

robhogan · 2022-02-09T18:30:49Z

packages/metro/src/shared/output/RamBundle/as-assets.js

+const fs = require('fs');
+const util = require('util');


Suggested change

const fs = require('fs');

const util = require('util');

const fsPromises = require('fs').promises;

facebook-github-bot · 2022-02-09T21:56:05Z

@rh389 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2022-02-09T22:18:53Z

@rh389 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

ericanderson · 2022-02-09T23:22:15Z

Thanks for the help getting it through @rh389. Sorry about the false start earlier. I was in and out of meetings and pushed before running tests.

robhogan · 2022-02-10T08:22:51Z

Not at all - I was just keen to get it pulled in before the end of my day yesterday. It's waiting for internal review now but everything's green so I'd expect it to be in the next fortnightly release, this PR will auto-close when it lands. Thanks again for the contribution (and patience!)

…mkdir` (facebook#768) Summary: **Summary** mkdirp was triggering github security warnings. **Details** Github security warnings. The ones where you are the owner of a project and you visit the project's home page and get a giant warning from GitHub telling you there are CVE's in libraries within your repo. I don't know for sure if metro was or was not vunerable. I for sure know this wouldn't affect someone's production code. What I do know is that humans build tolerance and blindness to things over time. Having a security warning linger on a repo/project is a long term risk to other, real security vulnerabilities being seen and fixed. **Test plan** `yarn test` Pull Request resolved: facebook#768 Reviewed By: GijsWeterings Differential Revision: D33986388 Pulled By: rh389 fbshipit-source-id: c1d3ebc3b4ed0c9bd8d3348cfc052ccd6a5cc4a5

ericanderson force-pushed the mkdirp-1.0.4 branch from fefe534 to a7556f3 Compare February 1, 2022 23:49

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 2, 2022

facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Feb 2, 2022

philIip requested changes Feb 2, 2022

View reviewed changes

packages/metro-memory-fs/src/index.js Outdated Show resolved Hide resolved

robhogan reviewed Feb 4, 2022

View reviewed changes

packages/metro-memory-fs/src/index.js Outdated Show resolved Hide resolved

robhogan reviewed Feb 9, 2022

View reviewed changes

Pivot to dropping mkdirp

4bfd7b4

ericanderson force-pushed the mkdirp-1.0.4 branch from 339a754 to 4bfd7b4 Compare February 9, 2022 21:41

robhogan changed the title ~~Upgrade to mkdirp 1.0.4 for security warnings~~ Remove mkdirp@^0.5.1 (due to CVE warnings) in favour of native recursive fs.mkdir Feb 9, 2022

rh389 added 2 commits February 9, 2022 22:16

Linter: prefer-const

fa2f779

Linter: sort requires

3a66663

robhogan changed the title ~~Remove mkdirp@^0.5.1 (due to CVE warnings) in favour of native recursive fs.mkdir~~ Remove mkdirp@^0.5.1 due to CVE warnings, use native recursive fs.mkdir Feb 9, 2022

facebook-github-bot closed this in 0ca606e Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `mkdirp@^0.5.1` due to CVE warnings, use native recursive `fs.mkdir` #768

Remove `mkdirp@^0.5.1` due to CVE warnings, use native recursive `fs.mkdir` #768

ericanderson commented Feb 1, 2022 •

edited

facebook-github-bot commented Feb 1, 2022

facebook-github-bot commented Feb 2, 2022

philIip left a comment

ericanderson commented Feb 3, 2022 •

edited

facebook-github-bot commented Feb 3, 2022

ericanderson commented Feb 4, 2022

facebook-github-bot commented Feb 8, 2022

robhogan commented Feb 9, 2022 •

edited

ericanderson commented Feb 9, 2022

robhogan left a comment •

edited

robhogan Feb 9, 2022

robhogan Feb 9, 2022

robhogan Feb 9, 2022

facebook-github-bot commented Feb 9, 2022

facebook-github-bot commented Feb 9, 2022

ericanderson commented Feb 9, 2022

robhogan commented Feb 10, 2022

		const mkdirPromised = util.promisify(fs.mkdir);
		return mkdirPromised(dirName, {recursive: true}).then(() => undefined);

	const mkdirPromised = util.promisify(fs.mkdir);
	return mkdirPromised(dirName, {recursive: true}).then(() => undefined);
	return fsPromises.mkdir(dirName, {recursive: true});

		@@ -86,9 +87,8 @@ function saveAsAssets(
		}

		function createDir(dirName: string): Promise<empty> {

	const fs = require('fs');
	const util = require('util');
	const fsPromises = require('fs').promises;

Remove mkdirp@^0.5.1 due to CVE warnings, use native recursive fs.mkdir #768

Remove mkdirp@^0.5.1 due to CVE warnings, use native recursive fs.mkdir #768

Conversation

ericanderson commented Feb 1, 2022 • edited

facebook-github-bot commented Feb 1, 2022

Action Required

Process

facebook-github-bot commented Feb 2, 2022

philIip left a comment

Choose a reason for hiding this comment

ericanderson commented Feb 3, 2022 • edited

facebook-github-bot commented Feb 3, 2022

ericanderson commented Feb 4, 2022

facebook-github-bot commented Feb 8, 2022

robhogan commented Feb 9, 2022 • edited

ericanderson commented Feb 9, 2022

robhogan left a comment • edited

Choose a reason for hiding this comment

robhogan Feb 9, 2022

Choose a reason for hiding this comment

robhogan Feb 9, 2022

Choose a reason for hiding this comment

robhogan Feb 9, 2022

Choose a reason for hiding this comment

facebook-github-bot commented Feb 9, 2022

facebook-github-bot commented Feb 9, 2022

ericanderson commented Feb 9, 2022

robhogan commented Feb 10, 2022

Remove `mkdirp@^0.5.1` due to CVE warnings, use native recursive `fs.mkdir` #768

Remove `mkdirp@^0.5.1` due to CVE warnings, use native recursive `fs.mkdir` #768

ericanderson commented Feb 1, 2022 •

edited

ericanderson commented Feb 3, 2022 •

edited

robhogan commented Feb 9, 2022 •

edited

robhogan left a comment •

edited