Fix static.php RFC 7233 range handling and built-in server compatibility#10144
Merged
Conversation
…mpatibility - Fix suffix-range requests (e.g., "bytes=-500" for last 500 bytes) - Fix open-ended range requests (e.g., "bytes=10-" for byte 10 to end) - Clean output buffers before serving to prevent Content-Length mismatch - Use chunked reading with flush for PHP built-in server compatibility - Use readfile() for efficient full-file serving on non-cli-server SAPIs - Add tests for suffix-range, open-ended range, and full-file range requests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Split the monolithic testRangeHeader into focused test methods with RFC 7233 references documenting what was broken and why: - testRangeHeaderSuffix: Proves suffix-range "bytes=-500" returns the LAST 500 bytes. The old code set start=0 for empty prefix, serving bytes 0-500 (first 501 bytes) instead — completely wrong content. - testRangeHeaderSuffixLargerThanFile: Proves "bytes=-9999" on a 1058-byte file returns the whole file (206). The old code interpreted this as "bytes=0-9999", failed bounds check, returned 416 error. - testRangeHeaderOpenEnded: Verifies "bytes=10-" returns byte 10 to EOF. - testRangeHeaderFullFileViaRange: Verifies "bytes=0-" returns full file. - testContentLengthAccuracy: Verifies Content-Length matches actual body size, catching output buffer corruption on PHP built-in server. - testRangeHeaderInvalid/Standard: Preserved from original test suite. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The core motivation for the static.php changes is that the PHP built-in server (cli-server SAPI) could not correctly serve static CSS and JS files. Output buffering could inject extra bytes before file content, causing Content-Length mismatch and truncated downloads. Large files like app.js (~387KB) also required chunked fread()+flush() to deliver completely. New tests: - testCssFileIntegrity: Verifies .less files are served byte-for-byte identically to disk, with matching Content-Length - testJsFileIntegrity: Same for JS files, including the large app.js that triggers chunked delivery issues - testContentLengthAccuracy: Parameterized across file types/sizes (54B gif to 387KB js) to catch Content-Length vs body mismatches All tests run against the built-in server via ServerTestCase. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add a test fixture (tests/fixtures/static_original_router.php) that runs the ORIGINAL unfixed serveStaticFile logic on a second PHP built-in server, allowing direct comparison against the fixed version. Three new tests demonstrate the actual bugs: - testOriginalSuffixRangeReturnsWrongBytes: Proves "bytes=-500" on the original server returns bytes 0-500 (first 501 bytes) while the fixed server correctly returns bytes 558-1057 (last 500 bytes). The two responses contain completely different data. - testOriginalSuffixRangeLargerThanFileReturns416: Proves "bytes=-9999" on a 1058-byte file returns 416 on the original server (because it interprets it as "bytes=0-9999" which fails bounds check), while the fixed server correctly clamps and returns 206 with the full file. - testOriginalOutputBufferCorruptsResponse: Proves that when output buffering is active, the original server's response begins with buffer junk and the file's last bytes are truncated (because Content-Length only accounts for the file size, not the buffer). The fixed server's ob_end_clean() prevents this corruption. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Address critical issues found by CodeRabbit review: - Move fopen() before http_response_code(206) and header() calls in the range request path, so a 500 error can be sent if fopen fails (previously headers were already sent, making 500 impossible) - Same fix for the cli-server full-file path: open file before headers - Add flush() after each chunk in the range request loop for cli-server SAPI, matching the full-file path behavior for consistency - Fix parse_url() false return handling in test fixture (parse_url can return false, not just null, on malformed URLs) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
8 tasks
alecpl
requested changes
May 1, 2026
Drop the PHP_SAPI === 'cli-server' branch and the readfile() vs fread() split. Range and full-file requests now share the same fopen, headers, and chunked fread+flush loop, with $start/$end initialized for the full-file case and overridden when an HTTP_RANGE header is present. Addresses review feedback on roundcube#10144. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
I took a look at the three failing checks, they don't seem related to this PR: The last Unit workflow run before the current one was 3 days ago (April 28, on master) and succeeded with the same CI config. Between then and now (May 1), the ondrej/php PPA's GPG signing setup broke the way setup-php consumes it — the apt step now fails with NO_PUBKEY 71DAEAAB4AD4CAB6 NO_PUBKEY 4F4EA0AAE5267A6C before any PHP code runs.
This will resolve when either (a) ondrej's PPA gets re-signed, (b) setup-php ships a fix, or (c) a maintainer re-runs the workflow once upstream is healthy. UPDATE: shivammathur says this should be solved now, the next trigger of the CI actions should no longer present the problem |
Remove tests/fixtures/static_original_router.php and the three testOriginal* tests that compared the new server's behavior against a frozen copy of the old, buggy static.php. The remaining tests (testRangeHeaderSuffix, testRangeHeaderSuffixLargerThanFile, testRangeHeaderOpenEnded, testCssFileIntegrity, testJsFileIntegrity, testContentLengthAccuracy, etc.) already exercise the fixed code's correctness against RFC 7233 and verify byte-for-byte serving. Addresses review feedback on roundcube#10144. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
php-cs-fixer's class_attributes_separation rule disallows a blank line between the last class member and the closing brace. Co-Authored-By: Claude Opus 4.7 (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
2 participants
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.
Summary
Fixes several bugs in
static.php's range request handling and file serving logic:bytes=-500): was serving bytes 0-500 (first 501 bytes) instead of the last 500 bytes per RFC 7233 §2.1bytes=-9999): was returning 416 instead of clamping to the full file per RFC 7233fopen(), making it impossible to return 500 on failureflush()to range request loop for PHP built-in server consistencyfread()+flush()on cli-server SAPI for reliable CSS/JS deliveryCloses #10143
Split out from #10085 per maintainer request.
Test plan
tests/fixtures/static_original_router.php) runs the original unfixed code on a second PHP built-in servertestOriginalSuffixRangeReturnsWrongBytes— proves old code returns wrong bytes, new code returns correct bytestestOriginalSuffixRangeLargerThanFileReturns416— proves old code returns 416, new code returns 206testOriginalOutputBufferCorruptsResponse— proves output buffering corrupts responses withoutob_end_clean()testCssFileIntegrity/testJsFileIntegrity— byte-for-byte verification of CSS/JS serving on built-in servertestContentLengthAccuracy— parameterized across file sizes (54B to 387KB)testRangeHeaderSuffix,testRangeHeaderOpenEnded,testRangeHeaderFullFileViaRange— RFC 7233 compliancetestRangeHeaderInvalid,testRangeHeaderStandard,testModifiedSinceHeader,testExistingResources,testForbiddenResources🤖 Generated with Claude Code