mapbox / node-pre-gyp

Node.js tool for easy binary deployment of C++ addons
BSD 3-Clause "New" or "Revised" License
1.11k stars 260 forks source link

Bump rimraf to 5.0.5 to fix DoS #707

Closed pnappa closed 3 months ago

pnappa commented 9 months ago

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).

pnappa commented 9 months ago

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 commented 9 months ago

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 commented 7 months ago

any plans on this ?

sabex commented 6 months ago

accepting this PR would be very useful!

cclauss commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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.

benmccann commented 3 months ago

I've sent an alternative fix of removing the library entirely. It would greatly reduce the number of transitive dependencies and fix a number of deprecation warnings as well. See https://github.com/mapbox/node-pre-gyp/pull/720

cclauss commented 3 months ago

Please rebase because I have merged

pnappa commented 3 months ago

No action is needed from me here anymore, right?