fix: do not corrupt streamed response bodies#236
Open
likemusic wants to merge 1 commit into
Open
Conversation
Streamed responses (`StreamedResponse`, `BinaryFileResponse`) are captured via an output buffer, because `getContent()` returns `false` for them. The captured buffer was passed through `mb_trim()`, which is UTF-8 aware and therefore unsafe for arbitrary bytes. When the body starts or ends with a whitespace byte, `mb_trim()` decodes the whole buffer as UTF-8 and replaces every invalid byte with `?` (0x3F), so a binary body is silently destroyed. Trimming also changes the body length while the original `Content-Length` header is forwarded untouched, leaving the response inconsistent. The buffer is now forwarded verbatim, which also matches the non-streamed branch above, where `getContent()` is never trimmed. Co-Authored-By: Claude Opus 4.8 (1M context) <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.
Problem
Any route that returns a streamed response (
StreamedResponse,BinaryFileResponse— images, video, PDF,response()->download(),response()->streamDownload()) can be served with a corrupted body. In the browser this surfaces asTypeError: Failed to fetch, an<img>that never loads, or a file that silently arrives with mangled bytes, which makes it impossible to test file delivery in browser tests.Root cause
For streamed responses
getContent()returnsfalse, soLaravelHttpServer::handleRequest()captures the body through an output buffer. The captured buffer was then passed throughmb_trim():mb_trim()is UTF-8 aware, so it is not safe for arbitrary bytes. Two things go wrong:Byte mangling. When the body starts or ends with a whitespace byte (
0x20,0x0A,0x0D,0x09, …),mb_trim()decodes the entire buffer as UTF-8 and replaces every byte that is not valid UTF-8 with?(0x3F):This is why the bug can look intermittent: it depends on the first and last byte of the payload. A JPEG (
FFD8…FFD9) is left untouched, but anything ending in a newline — a PDF ending with%%EOF\n, a generated CSV, a text-ish export — is destroyed. Behaviour is identical on PHP 8.4's nativemb_trim()and on thesymfony/polyfill-php84implementation used on PHP 8.3.Content-Lengthdesync. Trimming changes the body length, while$response->headers->all()still forwards the originalContent-Length(the real file size). The body then no longer matches the advertised length and the browser tears down the response.The UTF-8 semantics look incidental rather than intended. The call was introduced in 8c1939e (a
wipcommit) together with the buffering block itself, with no test covering the streamed branch, and the surrounding intent was clearly just to tidy up text output. Worth noting for anyone touching this line:pint.jsonenables themb_str_functionsrule, so writing a plaintrim()here gets rewritten tomb_trim()automatically — a byte-level trim silently becomes a UTF-8 decode. That is also why the fix drops the call rather than downgrading it totrim().The fix
Forward the captured buffer verbatim:
I removed the trim entirely rather than restricting it to textual content types, because:
getContent()untrimmed. Trimming only in the streamed branch made two paths that should be equivalent behave differently.Content-Lengthfor text too, so a content-type check would leave a latent bug rather than fix it.The practical effect on text is that a streamed body keeps its leading/trailing whitespace, which matches how every non-streamed response is already served.
assertSee()and friends match on rendered DOM text, so they are unaffected.Note that static assets are served by the separate
asset()path, which already streams raw bytes — that is why assets on disk were fine while application-generated file responses were not.Tests
tests/Browser/StreamedResponseTest.phpadds three tests driven through the browser:0xFFbytes and whitespace at both edges, compared byte for byte viafetch()→arrayBuffer();StreamedResponseand decoded by the browser (naturalWidth > 0);The first and third fail on
4.xand pass with this change. The JPEG test passes either way —v4.jpghas no whitespace at its edges, so it is not affected by the bug — and is included as an end-to-end check of the real-world format rather than as a regression guard.composer test:lint,composer test:type-coverage(100%) and the browser suite pass. The pre-existingignore.unmatchedLinePHPStan error insrc/Api/AwaitableWebpage.php:63on4.xis untouched and unrelated.