Skip to content

fix: keep binary uploads intact in the multipart parser - #285

Open
Shoxcie wants to merge 1 commit into
pestphp:5.xfrom
Shoxcie:fix/binary-multipart-uploads
Open

Shoxcie wants to merge 1 commit into
pestphp:5.xfrom
Shoxcie:fix/binary-multipart-uploads

Conversation

@Shoxcie

@Shoxcie Shoxcie commented Oct 12, 2026

Copy link
Copy Markdown

What's wrong

attach() delivers the file and the multipart body is parsed, but any binary upload reaches the application corrupted.

LaravelHttpServer::parseMultipartBody() trims each part with mb_ltrim() (src/Drivers/LaravelHttpServer.php#L491). mb_ltrim() re-encodes its whole subject as UTF-8, not just the prefix it strips, so every byte of the part that is not valid UTF-8 — most of any image, PDF or archive — is replaced with ? (0x3f) before file_put_contents() writes it out.

Reproducing it

Standalone, no browser required:

$bytes = "\x89PNG\r\n\x1a\n\xff\xfe\xc0\x80\x81";

var_dump(ltrim("\r\n".$bytes, "\r\n") === $bytes);     // true
var_dump(mb_ltrim("\r\n".$bytes, "\r\n") === $bytes);  // false

In an application it surfaces a long way from the cause. A 64x64 WebP uploaded through attach() arrived with two bytes rewritten, which still left a valid RIFF/WebP header — so Laravel's extensions and mimes rules both passed, and the failure only appeared once the image library tried to decode the file:

Illuminate\Image\ImageException: Failed to process image:
  Failed to decode unsupported image format from binary data

Why the current test doesn't catch it

tests/Browser/Webpage/UploadTest.php uploads 'test content' — 12 bytes of ASCII, which is valid UTF-8 and so passes through mb_ltrim() untouched.

Worth noting the corruption does not reliably change the file's length: for the bytes in the test added here, 13 go in and 13 come out. A size assertion would not catch it, so the new test hashes the content.

The fix

ltrim(). It is byte-based, and the only thing being stripped is an ASCII \r\n, so there is nothing here for a multibyte function to do.

Verification

  • The mb_ltrim/ltrim difference in isolation, as above.
  • The new test's exact assertion run against both variants of the parser: it fails on current 5.x and passes with this change.
  • End to end in a real Laravel 13 application: a browser test uploading a 64x64 WebP through a file input fails with the decode error above on v5.1.2, and passes with this one-line change applied to the vendored copy.
  • I could not run this repository's own suite locally (Windows): a fresh composer install resolved a paratest incompatible with pest's WrapperRunner (WrapperWorker::__construct() arity), and the non-parallel run hangs. CI will exercise the new test.

Related

mb_ltrim re-encodes its whole subject as UTF-8, not only the prefix it strips, so every byte of a multipart body that is not valid UTF-8 was replaced with "?" before the part was written to disk. Binary uploads reached the application corrupted.
@ecsol

ecsol commented Oct 14, 2026

Copy link
Copy Markdown

Confirmed independently on v5.1.2 (PHP 8.4.25, macOS): in a real Laravel 13 app, PNG and ZIP uploads come through attach() altered, and plain ltrim fixes it. A separate defect in the same parseMultipartBody(), where array or nested file names (files[], a[b]) stay flat and the last file wins, is fixed in #286. A browser test of a real files[] image upload needs both fixes.

@ecsol

ecsol commented Oct 14, 2026

Copy link
Copy Markdown

Heads-up: pint.json enables mb_str_functions, so composer test:lint fails on this branch and composer lint turns the ltrim() back into mb_ltrim() (and the test's strlen() into mb_strlen()). I opened Shoxcie#1 against this branch with a lint-stable version (preg_replace('/^[\r\n]+/', '', $part) ?? $part, filesize() in the test); the binary test still fails against the v5.1.2 parser with it.

🤖 Generated with Claude Code

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.

2 participants