fix: stop body parsers from responding twice to an oversized body - #356
Merged
Merged
Conversation
Once the accumulated body passed the limit, onData called next() with an error but kept being invoked for every remaining chunk, and the stream path went on to emit 'end' as well. Each of those calls ran the error handler again, so the second response threw "Response was already sent" and took the process down with an uncaught exception. A request with a chunked body over the limit was enough to trigger it, which any client can send. Mark the parse as finished on the first outcome and ignore anything that arrives afterwards, dropping the buffered chunks at the same time so an oversized body isn't kept in memory while the error propagates.
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.
Sending a chunked request body larger than a body parser limit crashes the server with an uncaught exception, so any client can take the process down.
What happens
onDatacallsnext(new Error("Request entity too large"))as soon as the accumulated size passes the limit, but it keeps being called for every chunk that is still on the way, and the stream path emitsendafterwards too. Each of those calls runs the error handler again. The first one answers the request, the second one throwsCan't write body: Response was already sent, and nothing catches it.A body sent with
content-lengthis caught earlier by the header check, so it does not reach this path. A chunked body has nocontent-length, so the limit can only be caught while reading, which is where the repeat calls happen.Reproduction
On
mainthe server dies withError: Can't write body: Response was already sent. Express 4 and Express 5 answer413and stay up.Fix
Mark the parse as finished on the first outcome, whether that is the limit being hit or the body ending, and ignore whatever arrives after. The buffered chunks are dropped at the same time, so an oversized body is not held in memory while the error travels through the middleware chain.
Tests
Added
tests/middlewares/body-limit-chunked.js, which sends an oversized chunked body and then a normal one to check the server is still answering. It fails onmainand matches Express 4 and Express 5 output with this change.