Skip to content

fix: do not corrupt streamed response bodies#236

Open
likemusic wants to merge 1 commit into
pestphp:4.xfrom
likemusic:fix/streamed-binary-response-corruption
Open

fix: do not corrupt streamed response bodies#236
likemusic wants to merge 1 commit into
pestphp:4.xfrom
likemusic:fix/streamed-binary-response-corruption

Conversation

@likemusic

@likemusic likemusic commented Jul 19, 2026

Copy link
Copy Markdown

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 as TypeError: 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() returns false, so LaravelHttpServer::handleRequest() captures the body through an output buffer. The captured buffer was then passed through mb_trim():

$content = mb_trim(ob_get_clean());

mb_trim() is UTF-8 aware, so it is not safe for arbitrary bytes. Two things go wrong:

  1. 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):

    bin2hex(mb_trim("\x20\x0A\xFF\xD8ABC\xFF\x0A\x20")); // 3f3f4142433f — expected ffd8414243ff

    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 native mb_trim() and on the symfony/polyfill-php84 implementation used on PHP 8.3.

  2. Content-Length desync. Trimming changes the body length, while $response->headers->all() still forwards the original Content-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 wip commit) 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.json enables the mb_str_functions rule, so writing a plain trim() here gets rewritten to mb_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 to trim().

The fix

Forward the captured buffer verbatim:

$buffer = ob_get_clean();

$content = $buffer === false ? '' : $buffer;

I removed the trim entirely rather than restricting it to textual content types, because:

  • The non-streamed branch directly above already forwards getContent() untrimmed. Trimming only in the streamed branch made two paths that should be equivalent behave differently.
  • Trimming desyncs Content-Length for text too, so a content-type check would leave a latent bug rather than fix it.
  • A response body is the application's output; the test HTTP server should not rewrite 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.php adds three tests driven through the browser:

  • a binary streamed body with 0xFF bytes and whitespace at both edges, compared byte for byte via fetch()arrayBuffer();
  • a real JPEG served as a StreamedResponse and decoded by the browser (naturalWidth > 0);
  • a textual streamed body, asserting the bytes arrive exactly as emitted.

The first and third fail on 4.x and pass with this change. The JPEG test passes either way — v4.jpg has 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-existing ignore.unmatchedLine PHPStan error in src/Api/AwaitableWebpage.php:63 on 4.x is untouched and unrelated.

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant