fix(cve): bump busboy to fix CVE-2022-24434

[jlourenc]

This PR bumps busboy to at least 1.0.0 to remove dicer from the transitive dependencies as it contains a denial of service vulnerability: https://security.snyk.io/vuln/SNYK-JS-DICER-2311764.

The remaining of the PR is about adapting the code to the breaking changes introduced in busboy with the bump to v1 and fixing the tests:

• reverted lib: work around node.js bug #205 to fix should report errors from busboy parsing test as the error was no longer forwarded. I have not really looked into the details but reverting this workaround for an issue related to Node v0 seems like a win anyway,
• updated should handle unicode filenames test to have a more pertinent example of a unicode filename using both filename and filename* Content-Disposition directives.

This PR supersedes: #1096, #1092 and #1056.

[LinusU]

As far as I'm aware, this would be a breaking change, right? I think we need to do this in 2.x instead?

[jlourenc]

If the aim is to keep supporting Node v0 which is end-of-life since 2016, then yes it is a breaking change. However, such an aim is, in my opinion, counter-productive. Unless you're considering something else a breaking change?

Also, I've tried and run npm run test using node v0.12.12 and the project is already not compatible with Node v0 I believe:

$ node -v
v0.12.12

$ npm run test

> multer@1.4.4 test /Users/jeremylourenco/Code/multer
> mocha --harmony

/Users/jeremylourenco/Code/multer/node_modules/busboy/lib/index.js:3
const { readdirSync } = require('fs');
      ^
SyntaxError: Unexpected token {
    at exports.runInThisContext (vm.js:73:16)
    at Module._compile (module.js:443:25)
    at Object.Module._extensions..js (module.js:478:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Module.require (module.js:365:17)
    at require (module.js:384:17)
    at Object.<anonymous> (/Users/jeremylourenco/Code/multer/lib/make-middleware.js:2:14)
    at Module._compile (module.js:460:26)
    at Object.Module._extensions..js (module.js:478:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Module.require (module.js:365:17)
    at require (module.js:384:17)
    at Object.<anonymous> (/Users/jeremylourenco/Code/multer/index.js:1:84)
    at Module._compile (module.js:460:26)
    at Object.Module._extensions..js (module.js:478:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Module.require (module.js:365:17)
    at require (module.js:384:17)
    at Object.<anonymous> (/Users/jeremylourenco/Code/multer/test/disk-storage.js:9:14)
    at Module._compile (module.js:460:26)
    at Object.Module._extensions..js (module.js:478:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Module.require (module.js:365:17)
    at require (module.js:384:17)
    at /Users/jeremylourenco/Code/multer/node_modules/mocha/lib/mocha.js:231:27
    at Array.forEach (native)
    at Mocha.loadFiles (/Users/jeremylourenco/Code/multer/node_modules/mocha/lib/mocha.js:228:14)
    at Mocha.run (/Users/jeremylourenco/Code/multer/node_modules/mocha/lib/mocha.js:514:10)
    at Object.<anonymous> (/Users/jeremylourenco/Code/multer/node_modules/mocha/bin/_mocha:480:18)
    at Module._compile (module.js:460:26)
    at Object.Module._extensions..js (module.js:478:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Function.Module.runMain (module.js:501:10)
    at startup (node.js:129:16)
    at node.js:814:3

I'm not a Node expert but it seems "Object Destructuring" is a v6+ feature.

[erano067]

Hi, why not update to busboy atleast version ^1.6. 0?

[jlourenc]

Hi, why not update to busboy atleast version ^1.6. 0?

The aim of this PR is to remove dicer from the dependencies, which is achieved by upgrading to at least v1.0.0. There is no reason to enforce a higher version as far as this PR is concerned. You may chose to enforce a higher version in your project pulling multer.

[jlourenc]

I have now enforced the node engine to be >= 6.0.0.

[vaibhavkumar-sf]

Hi, Thanks for all the updates, can you please bump the busboy version to the latest 1.6.0 otherwise we may need to upgrade it again sooner for other vulnerabilities in the near future?

[senpai-notices]

lgtm. i approve this mr. do let any of us know if we can help you with any blockers.

[Dany-C]

This PR bumps busboy to at least 1.0.0 to remove dicer from the transitive dependencies as it contains a denial of service vulnerability: https://security.snyk.io/vuln/SNYK-JS-DICER-2311764.

The remaining of the PR is about adapting the code to the breaking changes introduced in busboy with the bump to v1 and fixing the tests:

• reverted lib: work around node.js bug #205 to fix should report errors from busboy parsing test as the error was no longer forwarded. I have not really looked into the details but reverting this workaround for an issue related to Node v0 seems like a win anyway,
• updated should handle unicode filenames test to have a more pertinent example of a unicode filename using both filename and filename* Content-Disposition directives.

This PR supersedes: #1096, #1092 and #1056.

My NestJs app didn't compile, so i found this temporary fix:

As of npm cli v8.3.0 (2021-12-09) this can be solved using the overrides field of package.json

For NestJs :
overrides": { "@nestjs/platform-express": { "multer": { "busboy": "1.0.0" } } }

For Express :
overrides": { "multer": { "busboy": "1.0.0" } }

[vaibhavkumar-sf]

override is a temporary solution to use until the package owner does not upgrade the package, also override is okay only if you are sure that the required version will not break the package @Dany-C

[LinusU]

Hey everyone!

I understand that this is frustrating for everyone involved. Unfortunately, I also see it as a big problem to release this as 1.x since it would break compatibility with older Node.js versions. Older versions of Node.js is unfortunately still used in many production environment, and it's Express policy to not break Node.js compatibility in non-major changes.

It also doesn't make sense to release this as 2.x since we already have several release candidates of Multer 2.x already, and they include an all new API.

Btw. I have released version 2.0.0-rc.4 which uses @fastify/busboy instead, and thus shouldn't be vulnerable.

I would love to get some input on how to proceed, some alternatives that I see:

1. Publish this branch as e.g. multer-classic on Npm
2. Publish this branch as something like 1.4.5-node.12, which I think won't get picked up as automatic update in semver?
3. Release this as 1.4.5 as a breaking change
4. Fork dicer (& busboy) and just fix the CVE, but keep Node.js compatibility
5. Inline the code for busboy and then restore old Node.js compatibility to it

@dougwilson do you have any input or guidance?

[RopoMen]

Hi,
I think that better fix to unicode issue can be achieved through this PR #1102

[zoeesilcock]

Another package that is affected by this is multer-gridfs-storage, see this issue. That package already requires Node 12 so it shouldn't require a major step for them. I would like to provide a PR for that package, am I correct in thinking that the solution for packages in this situation is to change their peer dependecy so it is pinned to 1.4.5-lts.1? Or would ^1.4.5-lts.1 work to allow future versions?

[ghost91-]

The latter should work, too.

[alasdairhurst]

From what i can see, busboy 1.0.0+ has documented a requirement of node >=10.16.0. Is this an oversight or is there a reason this is being ignored and the engines in multer being set to a lower version (6)?

[hongyanh]

npm uninstall --save multer and npm install --save multer fixed my problem.

[UlisesGascon]

This patch was included in multer@1.4.4 and the engines discussion is continuing in #1306