Delete temp file when http4s request body stream fails (413 leak)#5385
Merged
Conversation
When a request body is read into a file (RawBodyType.FileBody) and the body stream fails mid-write - e.g. with StreamMaxLengthExceededException when the configured max content length is exceeded (413) - the temp file created by createFile was never returned as a RawValue, so the interpreter's cleanup logic never saw it and it was never deleted. Every over-limit file upload thus leaked a temp file on disk. #5335 fixed exactly this in the akka, pekko, jdk-http, netty and zio-http backends, but missed http4s. The shared test "checks payload limit and returns 413 on exceeded max content length (request)" only detects the leak intermittently on CI (the leaked file must contain the test's marker bytes, which depends on chunking), which showed up as flaky Http4sServerTest failures. The fix deletes the file via serverOptions.deleteFile when the stream compilation fails; onError rethrows the original error, and the handler swallows its own failures so it cannot mask it. This also covers multipart file parts and the http4s-zio interpreter, which use the same class. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
The leak
In
Http4sRequestBody, theRawBodyType.FileBodycase creates a temp file viaserverOptions.createFileand then compiles the body stream into it. If the stream fails mid-write — e.g. withStreamMaxLengthExceededExceptionwhen the configured max content length is exceeded (resulting in a 413) — theRawValue(FileRange(file), ...)is never produced. The interpreter's body-cleanup logic only deletes files that were registered as raw values, so the temp file is never deleted.The leak happens on every over-limit file upload, making it a disk-fill DoS vector: a client can fill the server's temp directory simply by repeatedly sending oversized bodies to a file-body (or multipart file) endpoint.
#5335 fixed exactly this in the akka, pekko, jdk-http, netty (cats/future/sync/zio) and zio-http backends, but missed http4s.
Why the shared test only catches it intermittently
The shared test
checks payload limit and returns 413 on exceeded max content length (request)(ServerBasicTests) polls for leftover temp files containing the test's marker bytes. Whether the leaked file contains those bytes depends on how the HTTP client chunks the request and how much of the body was written before the limit interrupt — in local runs the leaked file is often 0 bytes, so the marker check misses it (and the oversized-request send may also end in theSttpClientExceptionrecover path). The leak itself is deterministic; only its detection is chunking-dependent. This is the source of the flakyHttp4sServerTest413 failure seen e.g. on #5373's CI.The fix
Attach an
onErrorhandler to the stream compilation that deletes the file viaserverOptions.deleteFile.onErrorrethrows the original error after running the handler, and the handler swallows its own failures (.attempt.void) so a failing delete cannot mask the original error. SincetoRawFromStreamis also used for multipart parts and by the http4s-zio interpreter, those are covered too.Verification
http4sServer/testOnly sttp.tapir.server.http4s.Http4sServerTest -- -z 413passes (5/5) but leaves 1 leaked/tmp/tapir*tmpfile (0 bytes — confirming why the marker-based detection is intermittent).http4sServer2_12/compile,http4sServer3/compile,http4sServerZio/compile,http4sServerZio3/compileall succeed.Http4sServerTestsuite: 302 tests, all passed, 0 leaked temp files.🤖 Generated with Claude Code