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

Bump rimraf to 5.0.5 to fix DoS #707

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pnappa
Copy link

@pnappa pnappa commented Dec 15, 2023

Older versions of rimraf transitively depended on a package called inflight, which is no longer maintained, and current has a medium severity security vulnerability associated with it. https://security.snyk.io/vuln/SNYK-JS-INFLIGHT-6095116

Newer versions of rimraf rely on a newer version of glob, which no longer imports inflight.

Due to the differences in major version, rimraf has since changed their API. They now return promises (as opposed to callbacks), and as a result, we need to provide two functions to the .then() continuation to invoke the callback correctly (with the parameters in the correct order).

Older versions of rimraf transitively depended on a package called
inflight, which is no longer maintained, and current has a medium
severity security vulnerability associated with it.

Newer versions of rimraf rely on a newer version of glob, which no
longer imports inflight.

Due to the differences in major version, rimraf has since changed their
API. They now return promises (as opposed to callbacks), and as a
result, we need to provide two functions to the .then() continuation to
invoke the callback correctly (with the parameters in the correct
order).
@pnappa pnappa changed the title Bump rimraf to 5.0.5 Bump rimraf to 5.0.5 to fix DoS Dec 15, 2023
@pnappa
Copy link
Author

pnappa commented Dec 15, 2023

Looks like the CI is failing for a couple of reasons:

The readme specifies node version 8 is supported, however I don't think there is a safe version of rimraf that will work. A transitive dependency in rimraf uses newer Ecmascript features which are not supported in some older node versions. Is v8 really the minimum still? Can I suggest bumping to v18 (LTS)?

@pnappa
Copy link
Author

pnappa commented Dec 18, 2023

FYI, the documentation in the README is incorrect. The minimum node version required is actually 10. Output from npx ls-engines:

❯ npx ls-engines
`node_modules` found; loading tree from disk...
┌────────┬─────────────────────────────────────────────────────────────────┐
│ engine │ Currently available latest release of each valid major version: │
├────────┼─────────────────────────────────────────────────────────────────┤
│ node   │ v21.4.0, v20.10.0, v19.9.0, v18.19.0, v17.9.1, v16.20.2,        │
│        │ v15.14.0, v14.21.3, v13.14.0, v12.22.12, v11.15.0, v10.24.1     │
└────────┴─────────────────────────────────────────────────────────────────┘

┌──────────────────┬───────────────────────────┐
│ package engines: │ dependency graph engines: │
├──────────────────┼───────────────────────────┤
│ "engines": {     │ "engines": {              │
│   "node": "*"    │   "node": ">= 10"         │
│ }                │ }                         │
└──────────────────┴───────────────────────────┘


┌────────┬─────────────────┬─────────────────┬──────────────────────────┐
│ engine │ current version │ valid (package) │ valid (dependency graph) │
├────────┼─────────────────┼─────────────────┼──────────────────────────┤
│ node   │ v20.3.0         │ yes!            │ yes!                     │
└────────┴─────────────────┴─────────────────┴──────────────────────────┘

Your “engines” field is missing! Prefer explicitly setting a supported engine range.

You can fix this by running `ls-engines --save`, or by manually adding the following to your `package.json`:
"engines": {
  "node": ">= 10"
}

This PR bumps the minimum node version required to 14.17:

❯ npx ls-engines
`node_modules` found; loading tree from disk...
┌────────┬─────────────────────────────────────────────────────────────────┐
│ engine │ Currently available latest release of each valid major version: │
├────────┼─────────────────────────────────────────────────────────────────┤
│ node   │ v21.4.0, v20.10.0, v19.9.0, v18.19.0, v17.9.1, v16.20.2,        │
│        │ v15.14.0, v14.21.3                                              │
└────────┴─────────────────────────────────────────────────────────────────┘

┌──────────────────┬───────────────────────────┐
│ package engines: │ dependency graph engines: │
├──────────────────┼───────────────────────────┤
│ "engines": {     │ "engines": {              │
│   "node": "*"    │   "node": ">= 14.17"      │
│ }                │ }                         │
└──────────────────┴───────────────────────────┘


┌────────┬─────────────────┬─────────────────┬──────────────────────────┐
│ engine │ current version │ valid (package) │ valid (dependency graph) │
├────────┼─────────────────┼─────────────────┼──────────────────────────┤
│ node   │ v20.3.0         │ yes!            │ yes!                     │
└────────┴─────────────────┴─────────────────┴──────────────────────────┘

Your “engines” field is missing! Prefer explicitly setting a supported engine range.

You can fix this by running `ls-engines --save`, or by manually adding the following to your `package.json`:
"engines": {
  "node": ">= 14.17"
}

I recommend we also add engines information to the package.json file, as recommended by the above outputs.

@harshagarwalsol
Copy link

any plans on this ?

@sabex
Copy link

sabex commented Mar 26, 2024

accepting this PR would be very useful!

@cclauss
Copy link

cclauss commented Apr 18, 2024

Please insert the line - npm run update-crosswalk after line 26 of the file appveyor.yml so we can see if your tests pass.

@pnappa
Copy link
Author

pnappa commented Apr 19, 2024

Thanks @cclauss, that fixed the build for NodeJS 14. The others are failing due to syntax errors, as expected.
image

I'm going to revert that commit and instead apply the patch from your PR, #709, which will update the CI to sane versions of NodeJS (and include the crosswalk fix).

Applied by:

wget https://github.com/mapbox/node-pre-gyp/pull/709.patch
git apply 709.patch

@pnappa
Copy link
Author

pnappa commented Apr 19, 2024

Preferably, let me know when your branch merges in (if ever), and I'll remove the latest commit prior to this being merged, to keep attribution clean.

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

4 participants