test: reproduce filesLimit Premature close on buffered bodies (10.1.1 regression) - #641
Open
noahnu wants to merge 1 commit into
Open
Conversation
Adding failing coverage that shows FilesLimitError is replaced by ERR_STREAM_PREMATURE_CLOSE when the multipart body is fully buffered (fastify.inject or http end(buffer)) before parts are consumed. Existing filesLimit tests still pass because they stream via form.pipe. Co-authored-by: Cursor <cursoragent@cursor.com>
2 tasks
cmiic
added a commit
to cmiic/fastify-multipart
that referenced
this pull request
Aug 22, 2026
The GHSA-vmph-573x-85f6 fix (556cdb1) stopped clearing `currentFile` in `onError` so that a request aborted mid-part would have its in-flight part destroyed, instead of leaving `saveRequestFiles` hanging on `pump()` with a leaked temp file. That part of the fix is correct and stays. The side effect is that `cleanup()` now destroys `currentFile` for *any* error, and the `partsLimit` / `filesLimit` / `fieldsLimit` handlers call `cleanup(err)` too. A parser limit does not truncate the part in flight: busboy keeps parsing, the part completes on its own, and the consumer has to read it before it can observe the limit error on the next iteration. The window is wide open whenever the body reaches busboy in one piece -- `fastify.inject({ payload: form.getBuffer() })` or `req.end(buffer)`. busboy then parses the whole body synchronously, so the limit fires and the part is destroyed before the consumer has resumed at all. What the consumer gets depends on how it reads: - `part.toBuffer()` iterates the destroyed stream and throws ERR_STREAM_PREMATURE_CLOSE instead of FilesLimitError (fastify#642), which is what the preceding commit's tests cover. - `onFile` + `stream.pipeline()` receives a stream that is already destroyed and closed, with `close` emitted and no error. `pipeline()` attaches its end-of-stream listener too late to observe any of it and never settles: the queued limit error is never delivered, no response is sent, and the request hangs until the socket is torn down. Pass the limit cleanup through `cleanup(err, false)` so it leaves the part alone. Two alternatives do not hold up: - Clearing `currentFile` in the limit handler covers `filesLimit` and `partsLimit`, which stop busboy emitting parts, but not `fieldsLimit`, which only suppresses further *fields*: a file emitted after it becomes the part in flight by the time the deferred cleanup runs. It passes the preceding commit's tests while still being wrong, which is why the `fieldsLimit` cases in this commit are worth having. - Destroying only parts busboy has not finished does not work either. At cleanup time `_readableState.ended` is still false even for a part that goes on to complete, because busboy has buffered input left to parse. Keeping `currentFile` assigned also means a request that dies mid-part after a limit is still torn down by `onRequestClose`, so the advisory's behaviour is unchanged -- its regression test still passes. The existing limit tests do not cover any of this because they use `form.pipe(req)`, which feeds busboy slowly enough that the consumer has usually drained the previous part before the limit fires. This leaves one adjacent hang in the same area untouched. It predates the advisory fix and reproduces identically on 10.1.0, so it is not part of the regression fixed here; it is being raised with the maintainers separately. Fixes: fastify#642 Closes: fastify#641
cmiic
added a commit
to cmiic/fastify-multipart
that referenced
this pull request
Aug 22, 2026
The GHSA-vmph-573x-85f6 fix (556cdb1) stopped clearing `currentFile` in `onError` so that a request aborted mid-part would have its in-flight part destroyed, instead of leaving `saveRequestFiles` hanging on `pump()` with a leaked temp file. That part of the fix is correct and stays. The side effect is that `cleanup()` now destroys `currentFile` for *any* error, and the `partsLimit` / `filesLimit` / `fieldsLimit` handlers call `cleanup(err)` too. A parser limit is raised between parts and does not truncate the one in flight: busboy keeps parsing, that part completes on its own, and the consumer still has to read it before it can observe the limit error on the next iteration. The window is wide open whenever the body reaches busboy in one piece -- `fastify.inject({ payload: form.getBuffer() })` or `req.end(buffer)`. busboy then parses the whole body synchronously, so the limit fires and the part is destroyed before the consumer has resumed at all. What the consumer gets depends on how it reads: - `part.toBuffer()` iterates the destroyed stream and throws ERR_STREAM_PREMATURE_CLOSE instead of FilesLimitError (fastify#642), which is what the preceding commit's tests cover. - `onFile` + `stream.pipeline()` receives a stream that is already destroyed and closed, with `close` emitted and no error. `pipeline()` attaches its end-of-stream listener too late to observe any of it and never settles: the queued limit error is never delivered, no response is sent, and the request hangs until the socket is torn down. Deliver the limit error without tearing the pipe down. busboy finishes the part it already opened, the consumer reads it and then observes the limit error, and the normal end-of-request path unpipes as usual. Two narrower fixes do not hold up. Clearing `currentFile` in the limit handler covers `filesLimit` and `partsLimit`, which stop busboy emitting parts, but not `fieldsLimit`, which only suppresses further *fields*: a file emitted after it becomes the part in flight by the time the deferred cleanup runs. Skipping the destroy for limit errors while still unpiping does cover that, but only for a part busboy has already finished -- one still arriving is then starved of the rest of its body and the consumer waits on it forever. Both are covered by the tests added here. Requests that die mid-part are unaffected: they reach `cleanup` through `onRequestClose` with `keepParsing` false, so the in-flight part is still destroyed and the advisory's regression test still passes. The existing limit tests do not cover any of this because they use `form.pipe(req)`, which feeds busboy slowly enough that the consumer has usually drained the previous part before the limit fires. Fixes: fastify#642 Closes: fastify#641
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
1 participant
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.
Summary
This PR adds failing regression tests only (no fix). CI is expected to be red until the issue is fixed.
On
@fastify/multipart@10.1.1, exceedinglimits.fileswhile consuming parts withpart.toBuffer()often throwsERR_STREAM_PREMATURE_CLOSE(Premature close) instead ofFilesLimitError(FST_FILES_LIMIT) when the request body is fully buffered up front.This appears to be an unintended side effect of the GHSA-vmph-573x-85f6 fix that stopped clearing
currentFileinonError:filesLimitcleanup now destroys the in-flight part stream, racing ahead of theFilesLimitErrordelivered on the async iterator.Passes on 10.1.0 / fails on 10.1.1
npm test -- test/multipart-files-limit-buffered-body.test.jsWhy existing filesLimit tests still pass
They use
form.pipe(req), which streams the body slowly enough that the previous part is usually fully consumed beforefilesLimitfires. The regression shows up with:fastify.inject({ payload: form.getBuffer() })http.request(...); req.end(form.getBuffer())Both are common in apps and Fastify tests.
Related
Will also open an issue with a write-up; this PR is the executable repro.