Skip to content

fix: prevent JS asset truncation for files larger than 8KB#199

Open
riasvdv wants to merge 2 commits intopestphp:4.xfrom
riasvdv:fix/js-asset-truncation
Open

fix: prevent JS asset truncation for files larger than 8KB#199
riasvdv wants to merge 2 commits intopestphp:4.xfrom
riasvdv:fix/js-asset-truncation

Conversation

@riasvdv
Copy link

@riasvdv riasvdv commented Feb 6, 2026

Summary

  • Fixes JS asset serving in LaravelHttpServer::asset() where files >8KB could be silently truncated
  • The old code used fread() + ReadableResourceStream which has two bugs: fread() returns at most 8,192 bytes per call inside Amp's event loop, and ReadableResourceStream causes the content-length header to be stripped
  • The fix uses file_get_contents() and returns the content as a string body directly, which correctly sets content-length and avoids truncation

The Bug

The asset() method served JS files through this pipeline:

$temporaryContent = fread($file, (int) filesize($filepath));
// ... write to php://temp ...
return new Response(200, [...], new ReadableResourceStream($file));

Three issues:

  1. fread() short-read — Inside Amp's non-blocking event loop, fread() returns at most 8,192 bytes per call, not the full file
  2. Missing content-lengthResponse::setBody(ReadableStream) calls removeHeader('content-length'), forcing chunked transfer encoding
  3. stream_socket_shutdown() on php://tempReadableResourceStream::close() calls stream_socket_shutdown() on non-socket streams (undefined behavior)

This caused Craft CMS Control Panel pages to fail loading because their JS bundles (~50KB+) were truncated to 8KB.

The Fix

fclose($file);
$rawContent = file_get_contents($filepath);
$content = $this->rewriteAssetUrl($rawContent);
$response = new Response(200, [], $content);
$response->setHeader('Content-Type', $contentType);
return $response;
  • file_get_contents() reads the entire file regardless of event loop context
  • String body → ReadableBuffer → correct content-length header
  • No stream cleanup issues

Tests

Three regression tests added:

  • serves JS assets as string body instead of stream — Verifies response body is ReadableBuffer (not ReadableResourceStream) via reflection. Fails with old code
  • sets content-length header on JS asset responses — Verifies content-length header matches file size. Fails with old code
  • serves large JS files completely — End-to-end test via Playwright: creates >8KB JS file, loads it in a page, verifies a marker variable at the end of the file is accessible

The asset() method used fread() to read JS files, but fread() is limited
to 8192 bytes per call when running inside Amp's event loop. This caused
every JS file larger than 8KB to be silently truncated.

Additionally, the rewritten content was written to a php://temp stream
and served via ReadableResourceStream, which calls
stream_socket_shutdown() on close — undefined behavior for non-socket
streams that could also cause data loss.

Replace fopen/fread with file_get_contents to read the full file, and
return the string body directly in the Response. The Content-Type header
is set via setHeader() after construction to avoid the constructor's
setHeaders() call clearing the auto-set Content-Length.
Two reflection-based tests that deterministically fail with the old
fread()+ReadableResourceStream implementation and pass with the fix:
- Verify JS assets return string body (ReadableBuffer) not stream
- Verify content-length header is set correctly

Plus a Playwright-based test serving a >8KB JS file end-to-end.
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