fix: don't abort uploads when file writes are in progress#1379
Closed
mario-duna wants to merge 2 commits intoexpressjs:mainfrom
Closed
fix: don't abort uploads when file writes are in progress#1379mario-duna wants to merge 2 commits intoexpressjs:mainfrom
mario-duna wants to merge 2 commits intoexpressjs:mainfrom
Conversation
Author
|
@UlisesGascon would this change make sense? |
Member
|
duplicate of #1373 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Multer 2.1.0 introduced stricter request lifecycle handling (via the
abortedandcloseevent handlers) to prevent DoS from hung connections. However, this can cause legitimate uploads to fail with "Request aborted" or "Request closed" errors in certain environments.When This Happens
This issue manifests when using multer with in-process HTTP testing tools like supertest. In these environments:
closeorabortedevent on the request can fire beforereq.readableEndedis set totrue, even when the file data has been fully received and is being written to storageThe existing check
if (req.readableEnded) returnin theclosehandler is timing-dependent and doesn't protect against this race condition.Reproduction
This is most reliably reproduced on ARM-based CI runners (e.g., GitHub Actions ARM64 runners, AWS Graviton) where event loop scheduling differs from x86. A typical test like this fails intermittently or consistently:
Root Cause
In
make-middleware.js, the request event handlers immediately triggerhandleRequestFailure():With supertest, the sequence can be:
storage._handleFile()is called and starts writingclose/abortedevent firesreq.readableEndedmay not betrueyet (timing-dependent)handleRequestFailure()is called, which setserrorOccured = trueand eventually deletes the uploaded fileSolution
Add a check for
!pendingWrites.isZero()before aborting. If files are actively being written to storage, we should let them complete rather than aborting prematurely:This is safe because:
indicateDone()callsdone()normallyabortWithError()handles itSecurity Consideration
This change preserves the DoS protection added in the security fix (commit cccf0fe). The original issue was that clients could start uploads and disconnect, leaving multer waiting indefinitely. With this fix: