Skip to content

Conversation

max-mathieu
Copy link
Contributor

Fixes #1176

@MelleB
Copy link

MelleB commented Apr 6, 2023

I'm experiencing this as well. Can someone review this?

Tests which actually trigger this behavior are missing from this PR.

@nerder
Copy link

nerder commented Mar 23, 2024

Any chance this will be merged any time soon?

@nandorcsupor-met
Copy link

Any chance this will be merged any time soon?

Yep we desparately need this. Only solution is to downgrade the package.

@podplatnikm
Copy link

Bump, any update?

@raymond-tyr
Copy link

Any update on this?
I've validated this approach during testing and it works

@CodeFoxLk
Copy link

any update?

@IamLizu IamLizu requested a review from a team September 14, 2024 12:04
Copy link
Member

@IamLizu IamLizu left a comment

Choose a reason for hiding this comment

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

Perhaps the feature branch requires a rebase or it least the test commands needs to be updated to,

standard && mocha --reporter spec --bail --exit --check-leaks test/

I have noticed that more test fails if its just kept to standard && mocha.

And just ran the test after changing the test command, still 2 test fails. One of them looks like this,

  1) Disk Storage should process parser/form-data POST request:

      Uncaught AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

1803 !== 1778

      + expected - actual

      -1803
      +1778

      at test\disk-storage.js:45:14
      at test\_util.js:26:7
      at done (lib\make-middleware.js:47:7)
      at indicateDone (lib\make-middleware.js:51:68)
      at lib\make-middleware.js:157:11
      at WriteStream.<anonymous> (storage\disk.js:43:9)
      at WriteStream.emit (node:events:526:35)
      at finish (node:internal/streams/writable:937:10)
      at node:internal/streams/writable:918:13
      at process.processTicksAndRejections (node:internal/process/task_queues:82:21)

I am not posting the 2nd one because it says "operation not permitted", thats mostly because I am on personal PC, windows right now and I don't want to mess it up lol. I will run the test from a different PC tomorrow.

@max-mathieu max-mathieu force-pushed the fix/unhandled-busboy-error branch from 30bcca1 to 37241f8 Compare September 21, 2024 21:31
@max-mathieu
Copy link
Contributor Author

@IamLizu I branched off lts -- should I rebase to master? I ran this branch tests and they passed. I also merged master in and ran tests with standard && mocha --reporter spec --bail --exit --check-leaks test/ and they also all passed

Since this issue allows an attacker to craft a malformed request and take down any express app that uses multer, should that be a CVE? That's one reason why I branched off lts since no releases have been made since 1.4.5-lts.1

@RudyLang
Copy link

I agree with @max-mathieu that this should be labelled as a CVE, and that this solution needs to be merged sooner than later.
@UlisesGascon Just tagging you because you appear to be the most current and active approver.

rbliss added a commit to rbliss/multer that referenced this pull request Feb 25, 2025
@Ben-Lawrencee
Copy link

This would be greatly appreciated. Please merge soon! 🙏

@LinusU LinusU merged commit a4be1d5 into expressjs:lts Mar 20, 2025
@LinusU
Copy link
Member

LinusU commented Mar 20, 2025

Sorry for the way too long delay here!

tests added here: 4ce82b0
changelog added here: 502c03d
tagged a new release: 8ec534f

And published to npm 🚀

https://www.npmjs.com/package/multer/v/1.4.5-lts.2

@mhassan1
Copy link
Contributor

This fix doesn't handle the case where two error events are emitted by busboy; see #1296.

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.