Skip to content

fix: do not corrupt streamed response bodies - #267

Open
likemusic wants to merge 2 commits into
pestphp:5.xfrom
likemusic:fix/streamed-response-corruption-5x
Open

likemusic wants to merge 2 commits into
pestphp:5.xfrom
likemusic:fix/streamed-response-corruption-5x

Conversation

@likemusic

Copy link
Copy Markdown
Contributor

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/Visit/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 5.x as it stands 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:types, composer test:type-coverage (100%) and the full suite pass on this branch (343 passed, 27 skipped, PHP 8.4.24, Playwright 1.62.1). The fix also removes the @phpstan-ignore-next-line that the mb_trim() call required.

Note on 4.x

I opened the same fix against 4.x as #236 before noticing that branch is dormant; this PR supersedes it and #236 is now closed. The bug is present in both branches — the line is identical — so if 4.x ever sees another release, the same two-line change applies there.

`LaravelHttpServer` buffers a streamed response and passes the buffer
through `mb_trim()`. When the body begins or ends with a whitespace byte,
`mb_trim()` decodes the whole buffer as UTF-8 and replaces every invalid
byte with `?`, so a binary body reaches the browser mangled. It also
changes the body length while the original `Content-Length` is forwarded
as is.

The buffer is now passed through untouched.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A `text/event-stream` body ends every event with a blank line, so the trailing "\n\n" is protocol rather than whitespace. `mb_trim()` removed it, and the browser then never dispatched the last event: a page fed "data: first\n\ndata: second\n\n" received `first` only, with nothing reported anywhere. Measured on `5.x` at c98e8a5, where the new test fails with `-'first,second' +'first'` and passes with the two-line change in this branch.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@likemusic

Copy link
Copy Markdown
Contributor Author

Added a fourth test, because the strongest case for this change turns out not to be binary bodies at all: mb_trim() silently swallows a server-sent event.

A text/event-stream body terminates each event with a blank line, so the trailing \n\n is protocol, not whitespace. Trimming it means the last event never gets dispatched:

Route::get('/events', fn () => response()->stream(function (): void {
    echo "data: first\n\n";
    echo "data: second\n\n";
}, 200, ['Content-Type' => 'text/event-stream']));

A page listening with EventSource receives first and nothing else. Measured on 5.x at c98e8a5: -'first,second' +'first', and green with the two-line change here. No error, no warning, nothing in the response to suggest bytes went missing — the last event simply never happens, which is the kind of failure a test suite is least able to explain.

That shape is ordinary Laravel: response()->eventStream(), Broadcasting over SSE, and anything driving @laravel/stream-vue's useStream all end their frames this way.

All four tests pass on this branch, and Pint and Rector are clean on the changed file. Run in WSL2 (Linux 6.18 kernel on a Windows host), PHP 8.4.24, Playwright 1.62.1.

One thing this PR does not fix, so it does not get claimed here: a streamed response whose callback calls flush() — which is what real streaming code does — escapes the output buffer entirely. The browser then receives an empty body and the payload lands in the terminal running the test. That is a separate defect in the same block, reported as pestphp/pest#1604, and I have put the measurements there.

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