From bb7eb531ef0a9307246e0e1e2499eb1440d7d955 Mon Sep 17 00:00:00 2001 From: "John R. D'Orazio" Date: Thu, 9 Apr 2026 21:08:21 +0200 Subject: [PATCH 1/8] fix: Improve static.php range request handling and built-in server compatibility - 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) --- public_html/static.php | 114 ++++++++++++++++++++++-------------- tests/Public/StaticTest.php | 39 ++++++++++++ 2 files changed, 108 insertions(+), 45 deletions(-) diff --git a/public_html/static.php b/public_html/static.php index 43d5f34977..138fa1972e 100644 --- a/public_html/static.php +++ b/public_html/static.php @@ -124,6 +124,11 @@ function validateStaticFile(string $path): ?string */ function serveStaticFile($path): void { + // Clean any output buffers to prevent Content-Length mismatch with PHP built-in server + while (ob_get_level()) { + ob_end_clean(); + } + $lastModifiedTime = filemtime($path); header('Last-Modified: ' . gmdate('D, d M Y H:i:s \G\M\T', $lastModifiedTime)); @@ -133,76 +138,95 @@ function serveStaticFile($path): void } $size = filesize($path); - $fp = fopen($path, 'r'); - $range = [0, $size - 1]; + $ext = pathinfo($path, \PATHINFO_EXTENSION); + $headers = [ + 'Accept-Ranges' => 'bytes', + 'Content-Type' => SUPPORTED_TYPES[strtolower($ext)], + 'Cache-Control' => 'public, max-age=604800', + 'Expires' => gmdate('D, d M Y H:i:s \G\M\T', time() + 30 * 86400), + ]; + + // Handle range requests if (isset($_SERVER['HTTP_RANGE'])) { - // $valid = preg_match('^bytes=\d*-\d*(,\d*-\d*)*$', $_SERVER['HTTP_RANGE']); if (!str_starts_with($_SERVER['HTTP_RANGE'], 'bytes=')) { http_response_code(416); // "Range Not Satisfiable" - header('Content-Range: bytes */' . $size); // Required in 416. + header('Content-Range: bytes */' . $size); return; } $ranges = explode(',', substr($_SERVER['HTTP_RANGE'], 6)); - $range = explode('-', $ranges[0]); // TODO: only support the first range now. + $range = explode('-', $ranges[0]); + // Handle suffix-range (e.g., "bytes=-500" means last 500 bytes) if ($range[0] === '') { - $range[0] = 0; - } - if ($range[1] === '') { - $range[1] = $size - 1; + $start = max(0, $size - (int) $range[1]); + $end = $size - 1; + } else { + $start = (int) $range[0]; + $end = $range[1] === '' ? $size - 1 : (int) $range[1]; } - if ($range[0] >= 0 && ($range[1] <= $size - 1) && $range[0] <= $range[1]) { + if ($start >= 0 && $end <= $size - 1 && $start <= $end) { http_response_code(206); // "Partial Content" - header('Content-Range: bytes ' . sprintf('%u-%u/%u', $range[0], $range[1], $size)); + header('Content-Range: bytes ' . sprintf('%u-%u/%u', $start, $end, $size)); + $headers['Content-Length'] = $end - $start + 1; + + foreach ($headers as $k => $v) { + header("{$k}: {$v}", true); + } + + // For range requests, use chunked reading + $fp = fopen($path, 'r'); + if ($fp === false) { + http_response_code(500); + return; + } + fseek($fp, $start); + $remaining = $end - $start + 1; + + while ($remaining > 0 && !feof($fp) && connection_status() === \CONNECTION_NORMAL) { + $chunk = fread($fp, min($remaining, 8192)); + if ($chunk === false) { + break; + } + echo $chunk; + $remaining -= strlen($chunk); + } + + fclose($fp); } else { http_response_code(416); // "Range Not Satisfiable" header('Content-Range: bytes */' . $size); - return; } - } - $contentLength = $range[1] - $range[0] + 1; - $ext = pathinfo($path, \PATHINFO_EXTENSION); + return; + } - $headers = [ - 'Accept-Ranges' => 'bytes', - 'Content-Length' => $contentLength, - 'Content-Type' => SUPPORTED_TYPES[strtolower($ext)], - // 'Content-Disposition: attachment; filename="xxxxx"', - 'Cache-Control' => 'public, max-age=604800', - 'Expires' => gmdate('D, d M Y H:i:s \G\M\T', time() + 30 * 86400), - ]; + // For full file requests + $headers['Content-Length'] = $size; foreach ($headers as $k => $v) { header("{$k}: {$v}", true); } - if ($range[0] > 0) { - fseek($fp, $range[0]); - } - - $sentSize = 0; - - while (!feof($fp) && (connection_status() === \CONNECTION_NORMAL)) { - $readingSize = $contentLength - $sentSize; - $readingSize = min($readingSize, 512 * 1024); - - if ($readingSize <= 0) { - break; + // Use chunked reading with flush for PHP built-in server compatibility + if (\PHP_SAPI === 'cli-server') { + $fp = fopen($path, 'r'); + if ($fp === false) { + http_response_code(500); + return; } - - $data = fread($fp, $readingSize); - if ($data === false) { - break; + while (!feof($fp) && connection_status() === \CONNECTION_NORMAL) { + $chunk = fread($fp, 8192); + if ($chunk === false) { + break; + } + echo $chunk; + flush(); } - - $sentSize += strlen($data); - echo $data; - flush(); + fclose($fp); + } else { + readfile($path); } - - fclose($fp); } diff --git a/tests/Public/StaticTest.php b/tests/Public/StaticTest.php index 4040f75c95..deae5a5a7e 100644 --- a/tests/Public/StaticTest.php +++ b/tests/Public/StaticTest.php @@ -130,5 +130,44 @@ public function testRangeHeader(): void $this->assertSame(['41'], $response->getHeader('Content-Length')); $this->assertSame(['bytes 10-50/' . strlen($file)], $response->getHeader('Content-Range')); $this->assertSame(substr($file, 10, 41), (string) $response->getBody()); + + // Open-ended range (bytes=10- means from byte 10 to end) + $headers = [ + 'Range' => 'bytes=10-', + ]; + + $response = $this->request('GET', 'static.php/' . $path, ['headers' => $headers]); + + $expectedLength = strlen($file) - 10; + $this->assertSame(206, $response->getStatusCode()); + $this->assertSame([(string) $expectedLength], $response->getHeader('Content-Length')); + $this->assertSame(['bytes 10-' . (strlen($file) - 1) . '/' . strlen($file)], $response->getHeader('Content-Range')); + $this->assertSame(substr($file, 10), (string) $response->getBody()); + + // Suffix-range (bytes=-500 means last 500 bytes) + $headers = [ + 'Range' => 'bytes=-500', + ]; + + $response = $this->request('GET', 'static.php/' . $path, ['headers' => $headers]); + + $suffixStart = max(0, strlen($file) - 500); + $suffixLength = strlen($file) - $suffixStart; + $this->assertSame(206, $response->getStatusCode()); + $this->assertSame([(string) $suffixLength], $response->getHeader('Content-Length')); + $this->assertSame(['bytes ' . $suffixStart . '-' . (strlen($file) - 1) . '/' . strlen($file)], $response->getHeader('Content-Range')); + $this->assertSame(substr($file, $suffixStart), (string) $response->getBody()); + + // Full file via range (bytes=0-) + $headers = [ + 'Range' => 'bytes=0-', + ]; + + $response = $this->request('GET', 'static.php/' . $path, ['headers' => $headers]); + + $this->assertSame(206, $response->getStatusCode()); + $this->assertSame([(string) strlen($file)], $response->getHeader('Content-Length')); + $this->assertSame(['bytes 0-' . (strlen($file) - 1) . '/' . strlen($file)], $response->getHeader('Content-Range')); + $this->assertSame($file, (string) $response->getBody()); } } From dfe4f813ec37477a87c7529056062187ff4395db Mon Sep 17 00:00:00 2001 From: "John R. D'Orazio" Date: Thu, 9 Apr 2026 21:12:59 +0200 Subject: [PATCH 2/8] test: Add comprehensive tests justifying static.php range request fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- tests/Public/StaticTest.php | 206 ++++++++++++++++++++++++++++-------- 1 file changed, 164 insertions(+), 42 deletions(-) diff --git a/tests/Public/StaticTest.php b/tests/Public/StaticTest.php index deae5a5a7e..e758911eea 100644 --- a/tests/Public/StaticTest.php +++ b/tests/Public/StaticTest.php @@ -90,84 +90,206 @@ public function testModifiedSinceHeader(): void } /** - * Test handling of Range header + * Test handling of Range header - invalid requests */ - public function testRangeHeader(): void + public function testRangeHeaderInvalid(): void { $path = 'program/resources/dummy.pdf'; $file = file_get_contents(INSTALL_PATH . $path); - // Invalid header - $headers = [ - 'Range' => 'invalid', - ]; - - $response = $this->request('GET', 'static.php/' . $path, ['headers' => $headers]); + // Non-"bytes=" prefix must be rejected per RFC 7233 Section 3.1 + $response = $this->request('GET', 'static.php/' . $path, [ + 'headers' => ['Range' => 'invalid'], + ]); $this->assertSame(416, $response->getStatusCode()); $this->assertSame(['bytes */' . strlen($file)], $response->getHeader('Content-Range')); $this->assertSame('', (string) $response->getBody()); - // Invalid header - $headers = [ - 'Range' => 'bytes=1000-10', - ]; - - $response = $this->request('GET', 'static.php/' . $path, ['headers' => $headers]); + // Start position greater than end position is invalid + $response = $this->request('GET', 'static.php/' . $path, [ + 'headers' => ['Range' => 'bytes=1000-10'], + ]); $this->assertSame(416, $response->getStatusCode()); $this->assertSame(['bytes */' . strlen($file)], $response->getHeader('Content-Range')); $this->assertSame('', (string) $response->getBody()); + } - // Valid request - $headers = [ - 'Range' => 'bytes=10-50', - ]; + /** + * Test handling of Range header - standard byte range (bytes=start-end) + */ + public function testRangeHeaderStandard(): void + { + $path = 'program/resources/dummy.pdf'; + $file = file_get_contents(INSTALL_PATH . $path); - $response = $this->request('GET', 'static.php/' . $path, ['headers' => $headers]); + $response = $this->request('GET', 'static.php/' . $path, [ + 'headers' => ['Range' => 'bytes=10-50'], + ]); $this->assertSame(206, $response->getStatusCode()); $this->assertSame(['41'], $response->getHeader('Content-Length')); $this->assertSame(['bytes 10-50/' . strlen($file)], $response->getHeader('Content-Range')); $this->assertSame(substr($file, 10, 41), (string) $response->getBody()); + } - // Open-ended range (bytes=10- means from byte 10 to end) - $headers = [ - 'Range' => 'bytes=10-', - ]; + /** + * Test handling of Range header - open-ended range (bytes=offset-) + * + * RFC 7233 Section 2.1: "If the last-byte-pos value is absent [...] the + * byte range extends to the end of the representation's data." + * + * This verifies that an open-ended range like "bytes=10-" correctly returns + * all bytes from offset 10 to the end of the file, rather than being + * misinterpreted or rejected. + */ + public function testRangeHeaderOpenEnded(): void + { + $path = 'program/resources/dummy.pdf'; + $file = file_get_contents(INSTALL_PATH . $path); + $size = strlen($file); - $response = $this->request('GET', 'static.php/' . $path, ['headers' => $headers]); + $response = $this->request('GET', 'static.php/' . $path, [ + 'headers' => ['Range' => 'bytes=10-'], + ]); - $expectedLength = strlen($file) - 10; + $expectedLength = $size - 10; + $expectedEnd = $size - 1; $this->assertSame(206, $response->getStatusCode()); $this->assertSame([(string) $expectedLength], $response->getHeader('Content-Length')); - $this->assertSame(['bytes 10-' . (strlen($file) - 1) . '/' . strlen($file)], $response->getHeader('Content-Range')); + $this->assertSame(["bytes 10-{$expectedEnd}/{$size}"], $response->getHeader('Content-Range')); + // Verify actual byte content matches - not just length $this->assertSame(substr($file, 10), (string) $response->getBody()); + } - // Suffix-range (bytes=-500 means last 500 bytes) - $headers = [ - 'Range' => 'bytes=-500', - ]; + /** + * Test handling of Range header - suffix-range (bytes=-N) + * + * RFC 7233 Section 2.1: "A client can request the last N bytes of the + * selected representation using a suffix-byte-range-spec." + * suffix-byte-range-spec = "-" suffix-length + * For example, "bytes=-500" means "the last 500 bytes". + * + * BUG FIXED: The previous implementation treated the empty first element + * of "bytes=-500" (split on "-" yields ["", "500"]) by setting start=0, + * which incorrectly served bytes 0-500 (the FIRST 501 bytes) instead of + * the last 500 bytes. For a 1058-byte file, "bytes=-500" returned bytes + * 0-500 with Content-Range "bytes 0-500/1058", but RFC 7233 requires + * bytes 558-1057 with Content-Range "bytes 558-1057/1058". + */ + public function testRangeHeaderSuffix(): void + { + $path = 'program/resources/dummy.pdf'; + $file = file_get_contents(INSTALL_PATH . $path); + $size = strlen($file); + $suffixLength = 500; - $response = $this->request('GET', 'static.php/' . $path, ['headers' => $headers]); + $response = $this->request('GET', 'static.php/' . $path, [ + 'headers' => ['Range' => "bytes=-{$suffixLength}"], + ]); + + $expectedStart = $size - $suffixLength; // 1058 - 500 = 558 + $expectedEnd = $size - 1; // 1057 - $suffixStart = max(0, strlen($file) - 500); - $suffixLength = strlen($file) - $suffixStart; $this->assertSame(206, $response->getStatusCode()); $this->assertSame([(string) $suffixLength], $response->getHeader('Content-Length')); - $this->assertSame(['bytes ' . $suffixStart . '-' . (strlen($file) - 1) . '/' . strlen($file)], $response->getHeader('Content-Range')); - $this->assertSame(substr($file, $suffixStart), (string) $response->getBody()); + $this->assertSame( + ["bytes {$expectedStart}-{$expectedEnd}/{$size}"], + $response->getHeader('Content-Range'), + ); - // Full file via range (bytes=0-) - $headers = [ - 'Range' => 'bytes=0-', - ]; + // Verify the actual returned bytes are from the END of the file, + // not the beginning. This is the core assertion that proves the fix: + // the old code would return substr($file, 0, 501) here instead. + $expectedContent = substr($file, $expectedStart); + $actualContent = (string) $response->getBody(); + $this->assertSame($expectedContent, $actualContent); - $response = $this->request('GET', 'static.php/' . $path, ['headers' => $headers]); + // Double-check: the old (buggy) response would have started with the + // file's first bytes - verify we are NOT getting those + $firstBytes = substr($file, 0, 10); + $this->assertNotSame($firstBytes, substr($actualContent, 0, 10)); + } + + /** + * Test suffix-range larger than the file (bytes=-N where N > filesize) + * + * RFC 7233 Section 2.1: "If the selected representation is shorter than + * the specified suffix-length, the entire representation is used." + * + * BUG FIXED: The previous implementation would interpret "bytes=-9999" + * on a 1058-byte file as "bytes=0-9999", which fails the bounds check + * ($range[1] <= $size - 1) and returns 416. The correct behavior per + * RFC 7233 is to clamp and serve the entire file as a 206 response. + */ + public function testRangeHeaderSuffixLargerThanFile(): void + { + $path = 'program/resources/dummy.pdf'; + $file = file_get_contents(INSTALL_PATH . $path); + $size = strlen($file); + + $response = $this->request('GET', 'static.php/' . $path, [ + 'headers' => ['Range' => 'bytes=-9999'], + ]); + // Should clamp to the full file: bytes 0-(size-1) $this->assertSame(206, $response->getStatusCode()); - $this->assertSame([(string) strlen($file)], $response->getHeader('Content-Length')); - $this->assertSame(['bytes 0-' . (strlen($file) - 1) . '/' . strlen($file)], $response->getHeader('Content-Range')); + $this->assertSame([(string) $size], $response->getHeader('Content-Length')); + $this->assertSame( + ['bytes 0-' . ($size - 1) . '/' . $size], + $response->getHeader('Content-Range'), + ); $this->assertSame($file, (string) $response->getBody()); } + + /** + * Test full file retrieval via range (bytes=0-) + * + * Verifies that requesting from byte 0 with no end returns the complete + * file contents with a 206 status and correct Content-Range header. + */ + public function testRangeHeaderFullFileViaRange(): void + { + $path = 'program/resources/dummy.pdf'; + $file = file_get_contents(INSTALL_PATH . $path); + $size = strlen($file); + + $response = $this->request('GET', 'static.php/' . $path, [ + 'headers' => ['Range' => 'bytes=0-'], + ]); + + $this->assertSame(206, $response->getStatusCode()); + $this->assertSame([(string) $size], $response->getHeader('Content-Length')); + $this->assertSame( + ['bytes 0-' . ($size - 1) . '/' . $size], + $response->getHeader('Content-Range'), + ); + $this->assertSame($file, (string) $response->getBody()); + } + + /** + * Test Content-Length accuracy for full (non-range) file responses + * + * On PHP's built-in server (cli-server SAPI), output buffering can cause + * the actual response body to be larger than the declared Content-Length, + * leading to truncated or corrupted downloads. This verifies that the + * Content-Length header exactly matches the actual response body size. + */ + public function testContentLengthAccuracy(): void + { + $path = 'program/resources/dummy.pdf'; + $file = file_get_contents(INSTALL_PATH . $path); + + $response = $this->request('GET', 'static.php/' . $path); + + $declaredLength = $response->getHeader('Content-Length')[0] ?? null; + $actualBody = (string) $response->getBody(); + + $this->assertNotNull($declaredLength); + $this->assertSame((int) $declaredLength, strlen($actualBody), + 'Content-Length header must match actual body size to prevent download corruption'); + $this->assertSame(strlen($file), strlen($actualBody), + 'Response body size must match the actual file size on disk'); + } } From f9ac3a0fb7d2cea4af23eeafabaf5b5f376dc9a7 Mon Sep 17 00:00:00 2001 From: "John R. D'Orazio" Date: Thu, 9 Apr 2026 21:17:09 +0200 Subject: [PATCH 3/8] test: Add built-in server CSS/JS integrity tests for static.php 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) --- tests/Public/StaticTest.php | 127 +++++++++++++++++++++++++++++++++--- 1 file changed, 118 insertions(+), 9 deletions(-) diff --git a/tests/Public/StaticTest.php b/tests/Public/StaticTest.php index e758911eea..8d55485170 100644 --- a/tests/Public/StaticTest.php +++ b/tests/Public/StaticTest.php @@ -269,27 +269,136 @@ public function testRangeHeaderFullFileViaRange(): void } /** - * Test Content-Length accuracy for full (non-range) file responses + * Test that CSS files are served correctly on the PHP built-in server + * + * The PHP built-in server (cli-server SAPI) can corrupt static file + * responses in two ways: + * 1. Output buffering adds extra bytes before the file content, causing + * the actual body to exceed the declared Content-Length. HTTP clients + * then truncate the response at Content-Length, cutting off the end + * of the file — resulting in broken stylesheets. + * 2. Without explicit flush() between chunks, large files may be + * incompletely delivered, again producing truncated CSS. + * + * These tests run against the built-in server (see ServerTestCase) and + * verify byte-for-byte integrity of the served content. + */ + #[DataProvider('provide_CssFileIntegrity_cases')] + public function testCssFileIntegrity($path): void + { + $file = file_get_contents(INSTALL_PATH . $path); + $response = $this->request('GET', 'static.php/' . $path); + + $this->assertSame(200, $response->getStatusCode()); + + $declaredLength = $response->getHeader('Content-Length')[0] ?? null; + $actualBody = (string) $response->getBody(); + + // Content-Length must match the file on disk — a mismatch here means + // output buffering injected extra bytes or the size was miscalculated + $this->assertSame((string) strlen($file), $declaredLength, + "Content-Length header must match file size on disk for {$path}"); + + // The response body must be exactly the file content — truncation or + // prepended buffer output would break stylesheet parsing + $this->assertSame(strlen($file), strlen($actualBody), + "Response body length must match file size for {$path}"); + $this->assertSame($file, $actualBody, + "Response body must be byte-for-byte identical to {$path} on disk"); + } + + /** + * Dataset for testCssFileIntegrity() + */ + public static function provide_CssFileIntegrity_cases(): iterable + { + return [ + 'small less file' => ['skins/elastic/styles/global.less'], + 'large less file' => ['skins/elastic/styles/styles.less'], + ]; + } + + /** + * Test that JavaScript files are served correctly on the PHP built-in server + * + * Same output buffering and chunked delivery issues as CSS (see above). + * JavaScript files are particularly affected because app.js (~387KB) is + * large enough that without chunked reading + flush(), the built-in server + * may not deliver the full content before the connection is considered + * complete, leaving the browser with a truncated and unparseable script. + */ + #[DataProvider('provide_JsFileIntegrity_cases')] + public function testJsFileIntegrity($path): void + { + $file = file_get_contents(INSTALL_PATH . $path); + $response = $this->request('GET', 'static.php/' . $path); + + $this->assertSame(200, $response->getStatusCode()); + + $declaredLength = $response->getHeader('Content-Length')[0] ?? null; + $actualBody = (string) $response->getBody(); + + $this->assertSame((string) strlen($file), $declaredLength, + "Content-Length header must match file size on disk for {$path}"); + $this->assertSame(strlen($file), strlen($actualBody), + "Response body length must match file size for {$path}"); + $this->assertSame($file, $actualBody, + "Response body must be byte-for-byte identical to {$path} on disk"); + } + + /** + * Dataset for testJsFileIntegrity() + */ + public static function provide_JsFileIntegrity_cases(): iterable + { + return [ + 'small js file' => ['plugins/acl/acl.js'], + 'large js file' => ['program/js/app.js'], + ]; + } + + /** + * Test Content-Length accuracy across multiple file types * * On PHP's built-in server (cli-server SAPI), output buffering can cause * the actual response body to be larger than the declared Content-Length, - * leading to truncated or corrupted downloads. This verifies that the - * Content-Length header exactly matches the actual response body size. + * leading to truncated or corrupted downloads. The fix cleans any active + * output buffers (ob_end_clean) before serving, and uses chunked + * fread()+flush() instead of readfile() on the built-in server to ensure + * complete delivery. This test verifies that Content-Length exactly matches + * the actual response body size for various file types and sizes. */ - public function testContentLengthAccuracy(): void + #[DataProvider('provide_ContentLengthAccuracy_cases')] + public function testContentLengthAccuracy($path): void { - $path = 'program/resources/dummy.pdf'; $file = file_get_contents(INSTALL_PATH . $path); - $response = $this->request('GET', 'static.php/' . $path); + $this->assertSame(200, $response->getStatusCode()); + $declaredLength = $response->getHeader('Content-Length')[0] ?? null; $actualBody = (string) $response->getBody(); - $this->assertNotNull($declaredLength); + $this->assertNotNull($declaredLength, + "Content-Length header must be present for {$path}"); $this->assertSame((int) $declaredLength, strlen($actualBody), - 'Content-Length header must match actual body size to prevent download corruption'); + "Content-Length must match actual body size for {$path}"); $this->assertSame(strlen($file), strlen($actualBody), - 'Response body size must match the actual file size on disk'); + "Response body size must match file on disk for {$path}"); + } + + /** + * Dataset for testContentLengthAccuracy() + */ + public static function provide_ContentLengthAccuracy_cases(): iterable + { + return [ + 'tiny binary (54 bytes)' => ['program/resources/blank.gif'], + 'small pdf (1058 bytes)' => ['program/resources/dummy.pdf'], + 'medium css (4KB)' => ['skins/elastic/styles/global.less'], + 'medium js (12KB)' => ['plugins/acl/acl.js'], + 'large css (10KB)' => ['skins/elastic/styles/styles.less'], + 'large js (387KB)' => ['program/js/app.js'], + ]; } } From cd13f0324085e2fc9be1fcf6fa389fbced3b9f52 Mon Sep 17 00:00:00 2001 From: "John R. D'Orazio" Date: Thu, 9 Apr 2026 21:22:31 +0200 Subject: [PATCH 4/8] test: Prove original static.php bugs with side-by-side comparison tests 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) --- tests/Public/StaticTest.php | 203 +++++++++++++++++++++ tests/fixtures/static_original_router.php | 209 ++++++++++++++++++++++ 2 files changed, 412 insertions(+) create mode 100644 tests/fixtures/static_original_router.php diff --git a/tests/Public/StaticTest.php b/tests/Public/StaticTest.php index 8d55485170..248f69a92b 100644 --- a/tests/Public/StaticTest.php +++ b/tests/Public/StaticTest.php @@ -4,12 +4,54 @@ use PHPUnit\Framework\Attributes\DataProvider; use Roundcube\Tests\ServerTestCase; +use Symfony\Component\Process\Process; /** * Test class to test static resources server */ class StaticTest extends ServerTestCase { + /** @var Process PHP built-in server running the ORIGINAL (unfixed) static.php logic */ + protected static $originalProcess; + + #[\Override] + public static function setUpBeforeClass(): void + { + parent::setUpBeforeClass(); + + // Start a second PHP built-in server with the original (unfixed) code + // as a router script, so we can compare its behavior to the fixed version. + $router = realpath(__DIR__ . '/../fixtures/static_original_router.php'); + $cmd = ['php', '-S', 'localhost:8001', $router]; + + static::$originalProcess = new Process($cmd); + static::$originalProcess->start(); + usleep(100 * 1000); + } + + #[\Override] + public static function tearDownAfterClass(): void + { + static::$originalProcess->stop(); + parent::tearDownAfterClass(); + } + + /** + * HTTP client request to the ORIGINAL (unfixed) server on port 8001 + */ + protected function requestOriginal($method, $path, $options = []) + { + $config = [ + 'base_uri' => 'http://localhost:8001', + 'http_errors' => false, + 'handler' => null, + ]; + + $client = \rcmail::get_instance()->get_http_client($config); + + return $client->request($method, $path, $options); + } + /** * Test valid resources */ @@ -401,4 +443,165 @@ public static function provide_ContentLengthAccuracy_cases(): iterable 'large js (387KB)' => ['program/js/app.js'], ]; } + + // ------------------------------------------------------------------------- + // Tests against the ORIGINAL (unfixed) server to demonstrate the bugs + // that motivated the static.php changes. + // ------------------------------------------------------------------------- + + /** + * Prove the original code returns WRONG bytes for suffix-range requests + * + * RFC 7233 Section 2.1 defines "bytes=-500" as "the last 500 bytes". + * The original code splits on "-" to get ["", "500"], then sets the + * empty first element to 0, turning it into "bytes=0-500" — serving + * the FIRST 501 bytes instead of the LAST 500 bytes. + * + * This test hits both the original (buggy) and fixed servers with the + * same request and proves they return different content, then verifies + * only the fixed server returns the correct bytes. + */ + public function testOriginalSuffixRangeReturnsWrongBytes(): void + { + $path = 'program/resources/dummy.pdf'; + $file = file_get_contents(INSTALL_PATH . $path); + $size = strlen($file); + $suffixLength = 500; + + $headers = ['Range' => "bytes=-{$suffixLength}"]; + + // Original (unfixed) server + $original = $this->requestOriginal('GET', $path, ['headers' => $headers]); + + // Fixed server + $fixed = $this->request('GET', 'static.php/' . $path, ['headers' => $headers]); + + // Both return 206, but with different content + $this->assertSame(206, $original->getStatusCode(), 'Original server should return 206'); + $this->assertSame(206, $fixed->getStatusCode(), 'Fixed server should return 206'); + + $originalBody = (string) $original->getBody(); + $fixedBody = (string) $fixed->getBody(); + + // The original server returns bytes 0-500 (first 501 bytes) — WRONG + $buggyContent = substr($file, 0, $suffixLength + 1); + $this->assertSame($buggyContent, $originalBody, + 'Original server should return first 501 bytes (the bug)'); + $this->assertSame( + ['bytes 0-500/' . $size], + $original->getHeader('Content-Range'), + 'Original server reports wrong Content-Range starting at byte 0', + ); + + // The fixed server returns bytes 558-1057 (last 500 bytes) — CORRECT + $correctStart = $size - $suffixLength; + $correctContent = substr($file, $correctStart); + $this->assertSame($correctContent, $fixedBody, + 'Fixed server should return last 500 bytes (correct per RFC 7233)'); + $this->assertSame( + ["bytes {$correctStart}-" . ($size - 1) . "/{$size}"], + $fixed->getHeader('Content-Range'), + 'Fixed server reports correct Content-Range from end of file', + ); + + // The two responses must differ — proving the bug changes the output + $this->assertNotSame($originalBody, $fixedBody, + 'Original and fixed servers must return different content for suffix-range'); + } + + /** + * Prove the original code returns 416 for suffix-range larger than file + * + * RFC 7233 Section 2.1: "If the selected representation is shorter than + * the specified suffix-length, the entire representation is used." + * + * The original code interprets "bytes=-9999" on a 1058-byte file as + * "bytes=0-9999". The bounds check ($range[1] <= $size - 1) fails + * because 9999 > 1057, so it returns 416 "Range Not Satisfiable". + * The fixed version correctly clamps to the full file and returns 206. + */ + public function testOriginalSuffixRangeLargerThanFileReturns416(): void + { + $path = 'program/resources/dummy.pdf'; + $file = file_get_contents(INSTALL_PATH . $path); + $size = strlen($file); + + $headers = ['Range' => 'bytes=-9999']; + + // Original (unfixed) server — returns 416 (the bug) + $original = $this->requestOriginal('GET', $path, ['headers' => $headers]); + + $this->assertSame(416, $original->getStatusCode(), + 'Original server incorrectly returns 416 for suffix-range larger than file'); + $this->assertSame( + ['bytes */' . $size], + $original->getHeader('Content-Range'), + 'Original server reports unsatisfiable range', + ); + + // Fixed server — returns 206 with full file (correct per RFC 7233) + $fixed = $this->request('GET', 'static.php/' . $path, ['headers' => $headers]); + + $this->assertSame(206, $fixed->getStatusCode(), + 'Fixed server correctly returns 206 for suffix-range larger than file'); + $this->assertSame([(string) $size], $fixed->getHeader('Content-Length')); + $this->assertSame( + ['bytes 0-' . ($size - 1) . '/' . $size], + $fixed->getHeader('Content-Range'), + ); + $this->assertSame($file, (string) $fixed->getBody(), + 'Fixed server returns the entire file content'); + } + + /** + * Prove that output buffering corrupts responses without ob_end_clean() + * + * When output buffering is active (e.g. php.ini output_buffering=On, + * or a plugin/framework calling ob_start()), any content in the buffer + * is prepended to the file output. But Content-Length is calculated from + * filesize(), so the declared length doesn't account for the buffer junk. + * + * The HTTP client reads exactly Content-Length bytes, which now STARTS + * with the buffer content and CUTS OFF the end of the actual file. + * The result: the body is the right length, but contains wrong data. + * + * The test fixture's "?ob=..." parameter triggers ob_start() + echo + * to simulate this condition on the original server. + */ + public function testOriginalOutputBufferCorruptsResponse(): void + { + $path = 'program/resources/dummy.pdf'; + $file = file_get_contents(INSTALL_PATH . $path); + $size = strlen($file); + $junk = 'BUFFER_JUNK_DATA'; + + // Original server WITH output buffer junk injected via ?ob= param + $original = $this->requestOriginal('GET', $path . '?ob=' . $junk); + + // Content-Length is still the file size (doesn't know about the buffer) + $declaredLength = $original->getHeader('Content-Length')[0] ?? null; + $this->assertSame((string) $size, $declaredLength, + 'Original server declares Content-Length as file size, ignoring buffer content'); + + // The actual body the HTTP client received (Content-Length bytes) + // starts with the buffer junk, not the file's real first bytes + $originalBody = (string) $original->getBody(); + $this->assertStringStartsWith($junk, $originalBody, + 'Original server response begins with output buffer junk'); + $this->assertNotSame($file, $originalBody, + 'Original server response is NOT the correct file content'); + + // The end of the file is truncated because buffer junk displaced it + $expectedTruncatedFile = substr($file, 0, $size - strlen($junk)); + $this->assertSame( + $junk . $expectedTruncatedFile, + $originalBody, + 'Response is buffer junk + truncated file (last bytes cut off)', + ); + + // Fixed server — ob_end_clean() prevents this corruption + $fixed = $this->request('GET', 'static.php/' . $path); + $this->assertSame($file, (string) $fixed->getBody(), + 'Fixed server returns correct file content regardless of output buffering'); + } } diff --git a/tests/fixtures/static_original_router.php b/tests/fixtures/static_original_router.php new file mode 100644 index 0000000000..b58756081e --- /dev/null +++ b/tests/fixtures/static_original_router.php @@ -0,0 +1,209 @@ + 'image/avif', + 'css' => 'text/css', + 'gif' => 'image/gif', + 'jpg' => 'image/jpeg', + 'jpeg' => 'image/jpeg', + 'html' => 'text/html', + 'ico' => 'image/x-icon', + 'js' => 'text/javascript', + 'json' => 'application/json', + 'less' => 'text/less', + 'mp3' => 'audio/mpeg', + 'png' => 'image/png', + 'pdf' => 'application/pdf', + 'svg' => 'image/svg+xml', + 'tiff' => 'image/tiff', + 'wav' => 'audio/wav', + 'webp' => 'image/webp', + 'woff' => 'font/woff', + 'woff2' => 'font/woff2', +]; + +const ALLOWED_PATHS = [ + 'installer/', + 'plugins/', + 'program/', + 'skins/', +]; + +define('INSTALL_PATH', realpath(__DIR__ . '/../../') . '/'); + +// Simulate output buffering being active (as can happen with php.ini +// output_buffering=On, or framework/plugin code calling ob_start()). +// The "ob" query parameter controls this for targeted testing. +if (isset($_GET['ob'])) { + ob_start(); + // This junk represents any incidental output that might end up in the + // buffer (warnings, debug output, whitespace from included files, etc.) + echo $_GET['ob']; +} + +// Router script receives the path in REQUEST_URI, not PATH_INFO +$pathInfo = parse_url($_SERVER['REQUEST_URI'], \PHP_URL_PATH); + +$path = validateStaticFile($pathInfo ?? ''); + +if (!$path) { + http_response_code(404); + exit; +} + +serveStaticFile($path); + +// --- Original validateStaticFile (unchanged) --- + +function validateStaticFile(string $path): ?string +{ + $path = trim($path, "/ \t\r\n"); + $path = preg_replace('/[?&].*$/', '', $path); + + if (str_contains($path, '..')) { + return null; + } + + $ext = pathinfo($path, \PATHINFO_EXTENSION); + + if (empty($ext) || !isset(SUPPORTED_TYPES[strtolower($ext)])) { + return null; + } + + if (preg_match('/(README.*|CHANGELOG.*|SECURITY.*|meta\.json|composer\..*)/', $path)) { + return null; + } + + $found = false; + foreach (ALLOWED_PATHS as $prefix) { + if (str_starts_with($path, $prefix) && !preg_match('~skins/.+/templates/~', $path)) { + $found = true; + break; + } + } + + if (!$found) { + return null; + } + + $path = realpath(INSTALL_PATH . $path); + + if ($path === false) { + return null; + } + + return $path; +} + +// --- Original serveStaticFile (WITHOUT fixes) --- +// - No ob_end_clean() call +// - Suffix-range "bytes=-N" sets start=0 instead of start=size-N +// - File handle opened unconditionally and leaked on early return + +function serveStaticFile($path): void +{ + $lastModifiedTime = filemtime($path); + + header('Last-Modified: ' . gmdate('D, d M Y H:i:s \G\M\T', $lastModifiedTime)); + if (!empty($_SERVER['HTTP_IF_MODIFIED_SINCE']) && strtotime($_SERVER['HTTP_IF_MODIFIED_SINCE']) == $lastModifiedTime) { + http_response_code(304); + return; + } + + $size = filesize($path); + $fp = fopen($path, 'r'); + $range = [0, $size - 1]; + + if (isset($_SERVER['HTTP_RANGE'])) { + if (!str_starts_with($_SERVER['HTTP_RANGE'], 'bytes=')) { + http_response_code(416); + header('Content-Range: bytes */' . $size); + return; + } + + $ranges = explode(',', substr($_SERVER['HTTP_RANGE'], 6)); + $range = explode('-', $ranges[0]); + + // BUG: suffix-range "bytes=-500" splits to ["", "500"]. + // Setting $range[0]=0 turns it into "bytes=0-500" (first 501 bytes) + // instead of "last 500 bytes" per RFC 7233. + if ($range[0] === '') { + $range[0] = 0; + } + if ($range[1] === '') { + $range[1] = $size - 1; + } + + // BUG: for "bytes=-9999" on a 1058-byte file, this becomes + // $range=[0, 9999], and 9999 <= 1057 is false, so we get 416 + // instead of serving the full file. + if ($range[0] >= 0 && ($range[1] <= $size - 1) && $range[0] <= $range[1]) { + http_response_code(206); + header('Content-Range: bytes ' . sprintf('%u-%u/%u', $range[0], $range[1], $size)); + } else { + http_response_code(416); + header('Content-Range: bytes */' . $size); + return; + } + } + + $contentLength = $range[1] - $range[0] + 1; + $ext = pathinfo($path, \PATHINFO_EXTENSION); + + $headers = [ + 'Accept-Ranges' => 'bytes', + 'Content-Length' => $contentLength, + 'Content-Type' => SUPPORTED_TYPES[strtolower($ext)], + 'Cache-Control' => 'public, max-age=604800', + 'Expires' => gmdate('D, d M Y H:i:s \G\M\T', time() + 30 * 86400), + ]; + + foreach ($headers as $k => $v) { + header("{$k}: {$v}", true); + } + + if ($range[0] > 0) { + fseek($fp, $range[0]); + } + + $sentSize = 0; + + while (!feof($fp) && (connection_status() === \CONNECTION_NORMAL)) { + $readingSize = $contentLength - $sentSize; + $readingSize = min($readingSize, 512 * 1024); + + if ($readingSize <= 0) { + break; + } + + $data = fread($fp, $readingSize); + if ($data === false) { + break; + } + + $sentSize += strlen($data); + echo $data; + flush(); + } + + fclose($fp); +} From 3f2f4171e02a5c3d85cdca41f7b7ba84931d3e10 Mon Sep 17 00:00:00 2001 From: "John R. D'Orazio" Date: Thu, 9 Apr 2026 21:28:04 +0200 Subject: [PATCH 5/8] fix: Open file handle before sending headers, add flush to range path 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) --- public_html/static.php | 28 +++++++++++++++-------- tests/fixtures/static_original_router.php | 2 +- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/public_html/static.php b/public_html/static.php index 138fa1972e..2535dd7eab 100644 --- a/public_html/static.php +++ b/public_html/static.php @@ -168,6 +168,13 @@ function serveStaticFile($path): void } if ($start >= 0 && $end <= $size - 1 && $start <= $end) { + // Open file before sending headers so we can return 500 on failure + $fp = fopen($path, 'r'); + if ($fp === false) { + http_response_code(500); + return; + } + http_response_code(206); // "Partial Content" header('Content-Range: bytes ' . sprintf('%u-%u/%u', $start, $end, $size)); $headers['Content-Length'] = $end - $start + 1; @@ -176,12 +183,6 @@ function serveStaticFile($path): void header("{$k}: {$v}", true); } - // For range requests, use chunked reading - $fp = fopen($path, 'r'); - if ($fp === false) { - http_response_code(500); - return; - } fseek($fp, $start); $remaining = $end - $start + 1; @@ -192,6 +193,9 @@ function serveStaticFile($path): void } echo $chunk; $remaining -= strlen($chunk); + if (\PHP_SAPI === 'cli-server') { + flush(); + } } fclose($fp); @@ -206,10 +210,6 @@ function serveStaticFile($path): void // For full file requests $headers['Content-Length'] = $size; - foreach ($headers as $k => $v) { - header("{$k}: {$v}", true); - } - // Use chunked reading with flush for PHP built-in server compatibility if (\PHP_SAPI === 'cli-server') { $fp = fopen($path, 'r'); @@ -217,6 +217,11 @@ function serveStaticFile($path): void http_response_code(500); return; } + + foreach ($headers as $k => $v) { + header("{$k}: {$v}", true); + } + while (!feof($fp) && connection_status() === \CONNECTION_NORMAL) { $chunk = fread($fp, 8192); if ($chunk === false) { @@ -227,6 +232,9 @@ function serveStaticFile($path): void } fclose($fp); } else { + foreach ($headers as $k => $v) { + header("{$k}: {$v}", true); + } readfile($path); } } diff --git a/tests/fixtures/static_original_router.php b/tests/fixtures/static_original_router.php index b58756081e..0a1a0b5af7 100644 --- a/tests/fixtures/static_original_router.php +++ b/tests/fixtures/static_original_router.php @@ -63,7 +63,7 @@ // Router script receives the path in REQUEST_URI, not PATH_INFO $pathInfo = parse_url($_SERVER['REQUEST_URI'], \PHP_URL_PATH); -$path = validateStaticFile($pathInfo ?? ''); +$path = validateStaticFile(is_string($pathInfo) ? $pathInfo : ''); if (!$path) { http_response_code(404); From ff608b89d8555accc57ddd178ad4f1148d02ff4a Mon Sep 17 00:00:00 2001 From: "John R. D'Orazio" Date: Fri, 1 May 2026 18:06:58 +0200 Subject: [PATCH 6/8] refactor: Unify static.php file-serving into single code path 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 #10144. Co-Authored-By: Claude Opus 4.7 (1M context) --- public_html/static.php | 90 +++++++++++++++--------------------------- 1 file changed, 31 insertions(+), 59 deletions(-) diff --git a/public_html/static.php b/public_html/static.php index 2535dd7eab..f2e8b4f835 100644 --- a/public_html/static.php +++ b/public_html/static.php @@ -147,6 +147,9 @@ function serveStaticFile($path): void 'Expires' => gmdate('D, d M Y H:i:s \G\M\T', time() + 30 * 86400), ]; + $start = 0; + $end = $size - 1; + // Handle range requests if (isset($_SERVER['HTTP_RANGE'])) { if (!str_starts_with($_SERVER['HTTP_RANGE'], 'bytes=')) { @@ -161,80 +164,49 @@ function serveStaticFile($path): void // Handle suffix-range (e.g., "bytes=-500" means last 500 bytes) if ($range[0] === '') { $start = max(0, $size - (int) $range[1]); - $end = $size - 1; } else { $start = (int) $range[0]; $end = $range[1] === '' ? $size - 1 : (int) $range[1]; } - if ($start >= 0 && $end <= $size - 1 && $start <= $end) { - // Open file before sending headers so we can return 500 on failure - $fp = fopen($path, 'r'); - if ($fp === false) { - http_response_code(500); - return; - } - - http_response_code(206); // "Partial Content" - header('Content-Range: bytes ' . sprintf('%u-%u/%u', $start, $end, $size)); - $headers['Content-Length'] = $end - $start + 1; - - foreach ($headers as $k => $v) { - header("{$k}: {$v}", true); - } - - fseek($fp, $start); - $remaining = $end - $start + 1; - - while ($remaining > 0 && !feof($fp) && connection_status() === \CONNECTION_NORMAL) { - $chunk = fread($fp, min($remaining, 8192)); - if ($chunk === false) { - break; - } - echo $chunk; - $remaining -= strlen($chunk); - if (\PHP_SAPI === 'cli-server') { - flush(); - } - } - - fclose($fp); - } else { + if ($start < 0 || $end > $size - 1 || $start > $end) { http_response_code(416); // "Range Not Satisfiable" header('Content-Range: bytes */' . $size); + return; } + http_response_code(206); // "Partial Content" + header('Content-Range: bytes ' . sprintf('%u-%u/%u', $start, $end, $size)); + } + + // Open file before sending headers so we can return 500 on failure + $fp = fopen($path, 'r'); + if ($fp === false) { + http_response_code(500); return; } - // For full file requests - $headers['Content-Length'] = $size; + $headers['Content-Length'] = $end - $start + 1; - // Use chunked reading with flush for PHP built-in server compatibility - if (\PHP_SAPI === 'cli-server') { - $fp = fopen($path, 'r'); - if ($fp === false) { - http_response_code(500); - return; - } + foreach ($headers as $k => $v) { + header("{$k}: {$v}", true); + } - foreach ($headers as $k => $v) { - header("{$k}: {$v}", true); - } + if ($start > 0) { + fseek($fp, $start); + } - while (!feof($fp) && connection_status() === \CONNECTION_NORMAL) { - $chunk = fread($fp, 8192); - if ($chunk === false) { - break; - } - echo $chunk; - flush(); - } - fclose($fp); - } else { - foreach ($headers as $k => $v) { - header("{$k}: {$v}", true); + $remaining = $end - $start + 1; + + while ($remaining > 0 && !feof($fp) && connection_status() === \CONNECTION_NORMAL) { + $chunk = fread($fp, min($remaining, 8192)); + if ($chunk === false) { + break; } - readfile($path); + echo $chunk; + $remaining -= strlen($chunk); + flush(); } + + fclose($fp); } From f756487ffbeb03b36fbeaf3d881f496aa239eda8 Mon Sep 17 00:00:00 2001 From: "John R. D'Orazio" Date: Fri, 1 May 2026 18:19:41 +0200 Subject: [PATCH 7/8] test: Drop side-by-side fixture, keep tests for current code only 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 #10144. Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/Public/StaticTest.php | 202 --------------------- tests/fixtures/static_original_router.php | 209 ---------------------- 2 files changed, 411 deletions(-) delete mode 100644 tests/fixtures/static_original_router.php diff --git a/tests/Public/StaticTest.php b/tests/Public/StaticTest.php index 248f69a92b..b0e4ed549c 100644 --- a/tests/Public/StaticTest.php +++ b/tests/Public/StaticTest.php @@ -4,54 +4,12 @@ use PHPUnit\Framework\Attributes\DataProvider; use Roundcube\Tests\ServerTestCase; -use Symfony\Component\Process\Process; /** * Test class to test static resources server */ class StaticTest extends ServerTestCase { - /** @var Process PHP built-in server running the ORIGINAL (unfixed) static.php logic */ - protected static $originalProcess; - - #[\Override] - public static function setUpBeforeClass(): void - { - parent::setUpBeforeClass(); - - // Start a second PHP built-in server with the original (unfixed) code - // as a router script, so we can compare its behavior to the fixed version. - $router = realpath(__DIR__ . '/../fixtures/static_original_router.php'); - $cmd = ['php', '-S', 'localhost:8001', $router]; - - static::$originalProcess = new Process($cmd); - static::$originalProcess->start(); - usleep(100 * 1000); - } - - #[\Override] - public static function tearDownAfterClass(): void - { - static::$originalProcess->stop(); - parent::tearDownAfterClass(); - } - - /** - * HTTP client request to the ORIGINAL (unfixed) server on port 8001 - */ - protected function requestOriginal($method, $path, $options = []) - { - $config = [ - 'base_uri' => 'http://localhost:8001', - 'http_errors' => false, - 'handler' => null, - ]; - - $client = \rcmail::get_instance()->get_http_client($config); - - return $client->request($method, $path, $options); - } - /** * Test valid resources */ @@ -444,164 +402,4 @@ public static function provide_ContentLengthAccuracy_cases(): iterable ]; } - // ------------------------------------------------------------------------- - // Tests against the ORIGINAL (unfixed) server to demonstrate the bugs - // that motivated the static.php changes. - // ------------------------------------------------------------------------- - - /** - * Prove the original code returns WRONG bytes for suffix-range requests - * - * RFC 7233 Section 2.1 defines "bytes=-500" as "the last 500 bytes". - * The original code splits on "-" to get ["", "500"], then sets the - * empty first element to 0, turning it into "bytes=0-500" — serving - * the FIRST 501 bytes instead of the LAST 500 bytes. - * - * This test hits both the original (buggy) and fixed servers with the - * same request and proves they return different content, then verifies - * only the fixed server returns the correct bytes. - */ - public function testOriginalSuffixRangeReturnsWrongBytes(): void - { - $path = 'program/resources/dummy.pdf'; - $file = file_get_contents(INSTALL_PATH . $path); - $size = strlen($file); - $suffixLength = 500; - - $headers = ['Range' => "bytes=-{$suffixLength}"]; - - // Original (unfixed) server - $original = $this->requestOriginal('GET', $path, ['headers' => $headers]); - - // Fixed server - $fixed = $this->request('GET', 'static.php/' . $path, ['headers' => $headers]); - - // Both return 206, but with different content - $this->assertSame(206, $original->getStatusCode(), 'Original server should return 206'); - $this->assertSame(206, $fixed->getStatusCode(), 'Fixed server should return 206'); - - $originalBody = (string) $original->getBody(); - $fixedBody = (string) $fixed->getBody(); - - // The original server returns bytes 0-500 (first 501 bytes) — WRONG - $buggyContent = substr($file, 0, $suffixLength + 1); - $this->assertSame($buggyContent, $originalBody, - 'Original server should return first 501 bytes (the bug)'); - $this->assertSame( - ['bytes 0-500/' . $size], - $original->getHeader('Content-Range'), - 'Original server reports wrong Content-Range starting at byte 0', - ); - - // The fixed server returns bytes 558-1057 (last 500 bytes) — CORRECT - $correctStart = $size - $suffixLength; - $correctContent = substr($file, $correctStart); - $this->assertSame($correctContent, $fixedBody, - 'Fixed server should return last 500 bytes (correct per RFC 7233)'); - $this->assertSame( - ["bytes {$correctStart}-" . ($size - 1) . "/{$size}"], - $fixed->getHeader('Content-Range'), - 'Fixed server reports correct Content-Range from end of file', - ); - - // The two responses must differ — proving the bug changes the output - $this->assertNotSame($originalBody, $fixedBody, - 'Original and fixed servers must return different content for suffix-range'); - } - - /** - * Prove the original code returns 416 for suffix-range larger than file - * - * RFC 7233 Section 2.1: "If the selected representation is shorter than - * the specified suffix-length, the entire representation is used." - * - * The original code interprets "bytes=-9999" on a 1058-byte file as - * "bytes=0-9999". The bounds check ($range[1] <= $size - 1) fails - * because 9999 > 1057, so it returns 416 "Range Not Satisfiable". - * The fixed version correctly clamps to the full file and returns 206. - */ - public function testOriginalSuffixRangeLargerThanFileReturns416(): void - { - $path = 'program/resources/dummy.pdf'; - $file = file_get_contents(INSTALL_PATH . $path); - $size = strlen($file); - - $headers = ['Range' => 'bytes=-9999']; - - // Original (unfixed) server — returns 416 (the bug) - $original = $this->requestOriginal('GET', $path, ['headers' => $headers]); - - $this->assertSame(416, $original->getStatusCode(), - 'Original server incorrectly returns 416 for suffix-range larger than file'); - $this->assertSame( - ['bytes */' . $size], - $original->getHeader('Content-Range'), - 'Original server reports unsatisfiable range', - ); - - // Fixed server — returns 206 with full file (correct per RFC 7233) - $fixed = $this->request('GET', 'static.php/' . $path, ['headers' => $headers]); - - $this->assertSame(206, $fixed->getStatusCode(), - 'Fixed server correctly returns 206 for suffix-range larger than file'); - $this->assertSame([(string) $size], $fixed->getHeader('Content-Length')); - $this->assertSame( - ['bytes 0-' . ($size - 1) . '/' . $size], - $fixed->getHeader('Content-Range'), - ); - $this->assertSame($file, (string) $fixed->getBody(), - 'Fixed server returns the entire file content'); - } - - /** - * Prove that output buffering corrupts responses without ob_end_clean() - * - * When output buffering is active (e.g. php.ini output_buffering=On, - * or a plugin/framework calling ob_start()), any content in the buffer - * is prepended to the file output. But Content-Length is calculated from - * filesize(), so the declared length doesn't account for the buffer junk. - * - * The HTTP client reads exactly Content-Length bytes, which now STARTS - * with the buffer content and CUTS OFF the end of the actual file. - * The result: the body is the right length, but contains wrong data. - * - * The test fixture's "?ob=..." parameter triggers ob_start() + echo - * to simulate this condition on the original server. - */ - public function testOriginalOutputBufferCorruptsResponse(): void - { - $path = 'program/resources/dummy.pdf'; - $file = file_get_contents(INSTALL_PATH . $path); - $size = strlen($file); - $junk = 'BUFFER_JUNK_DATA'; - - // Original server WITH output buffer junk injected via ?ob= param - $original = $this->requestOriginal('GET', $path . '?ob=' . $junk); - - // Content-Length is still the file size (doesn't know about the buffer) - $declaredLength = $original->getHeader('Content-Length')[0] ?? null; - $this->assertSame((string) $size, $declaredLength, - 'Original server declares Content-Length as file size, ignoring buffer content'); - - // The actual body the HTTP client received (Content-Length bytes) - // starts with the buffer junk, not the file's real first bytes - $originalBody = (string) $original->getBody(); - $this->assertStringStartsWith($junk, $originalBody, - 'Original server response begins with output buffer junk'); - $this->assertNotSame($file, $originalBody, - 'Original server response is NOT the correct file content'); - - // The end of the file is truncated because buffer junk displaced it - $expectedTruncatedFile = substr($file, 0, $size - strlen($junk)); - $this->assertSame( - $junk . $expectedTruncatedFile, - $originalBody, - 'Response is buffer junk + truncated file (last bytes cut off)', - ); - - // Fixed server — ob_end_clean() prevents this corruption - $fixed = $this->request('GET', 'static.php/' . $path); - $this->assertSame($file, (string) $fixed->getBody(), - 'Fixed server returns correct file content regardless of output buffering'); - } } diff --git a/tests/fixtures/static_original_router.php b/tests/fixtures/static_original_router.php deleted file mode 100644 index 0a1a0b5af7..0000000000 --- a/tests/fixtures/static_original_router.php +++ /dev/null @@ -1,209 +0,0 @@ - 'image/avif', - 'css' => 'text/css', - 'gif' => 'image/gif', - 'jpg' => 'image/jpeg', - 'jpeg' => 'image/jpeg', - 'html' => 'text/html', - 'ico' => 'image/x-icon', - 'js' => 'text/javascript', - 'json' => 'application/json', - 'less' => 'text/less', - 'mp3' => 'audio/mpeg', - 'png' => 'image/png', - 'pdf' => 'application/pdf', - 'svg' => 'image/svg+xml', - 'tiff' => 'image/tiff', - 'wav' => 'audio/wav', - 'webp' => 'image/webp', - 'woff' => 'font/woff', - 'woff2' => 'font/woff2', -]; - -const ALLOWED_PATHS = [ - 'installer/', - 'plugins/', - 'program/', - 'skins/', -]; - -define('INSTALL_PATH', realpath(__DIR__ . '/../../') . '/'); - -// Simulate output buffering being active (as can happen with php.ini -// output_buffering=On, or framework/plugin code calling ob_start()). -// The "ob" query parameter controls this for targeted testing. -if (isset($_GET['ob'])) { - ob_start(); - // This junk represents any incidental output that might end up in the - // buffer (warnings, debug output, whitespace from included files, etc.) - echo $_GET['ob']; -} - -// Router script receives the path in REQUEST_URI, not PATH_INFO -$pathInfo = parse_url($_SERVER['REQUEST_URI'], \PHP_URL_PATH); - -$path = validateStaticFile(is_string($pathInfo) ? $pathInfo : ''); - -if (!$path) { - http_response_code(404); - exit; -} - -serveStaticFile($path); - -// --- Original validateStaticFile (unchanged) --- - -function validateStaticFile(string $path): ?string -{ - $path = trim($path, "/ \t\r\n"); - $path = preg_replace('/[?&].*$/', '', $path); - - if (str_contains($path, '..')) { - return null; - } - - $ext = pathinfo($path, \PATHINFO_EXTENSION); - - if (empty($ext) || !isset(SUPPORTED_TYPES[strtolower($ext)])) { - return null; - } - - if (preg_match('/(README.*|CHANGELOG.*|SECURITY.*|meta\.json|composer\..*)/', $path)) { - return null; - } - - $found = false; - foreach (ALLOWED_PATHS as $prefix) { - if (str_starts_with($path, $prefix) && !preg_match('~skins/.+/templates/~', $path)) { - $found = true; - break; - } - } - - if (!$found) { - return null; - } - - $path = realpath(INSTALL_PATH . $path); - - if ($path === false) { - return null; - } - - return $path; -} - -// --- Original serveStaticFile (WITHOUT fixes) --- -// - No ob_end_clean() call -// - Suffix-range "bytes=-N" sets start=0 instead of start=size-N -// - File handle opened unconditionally and leaked on early return - -function serveStaticFile($path): void -{ - $lastModifiedTime = filemtime($path); - - header('Last-Modified: ' . gmdate('D, d M Y H:i:s \G\M\T', $lastModifiedTime)); - if (!empty($_SERVER['HTTP_IF_MODIFIED_SINCE']) && strtotime($_SERVER['HTTP_IF_MODIFIED_SINCE']) == $lastModifiedTime) { - http_response_code(304); - return; - } - - $size = filesize($path); - $fp = fopen($path, 'r'); - $range = [0, $size - 1]; - - if (isset($_SERVER['HTTP_RANGE'])) { - if (!str_starts_with($_SERVER['HTTP_RANGE'], 'bytes=')) { - http_response_code(416); - header('Content-Range: bytes */' . $size); - return; - } - - $ranges = explode(',', substr($_SERVER['HTTP_RANGE'], 6)); - $range = explode('-', $ranges[0]); - - // BUG: suffix-range "bytes=-500" splits to ["", "500"]. - // Setting $range[0]=0 turns it into "bytes=0-500" (first 501 bytes) - // instead of "last 500 bytes" per RFC 7233. - if ($range[0] === '') { - $range[0] = 0; - } - if ($range[1] === '') { - $range[1] = $size - 1; - } - - // BUG: for "bytes=-9999" on a 1058-byte file, this becomes - // $range=[0, 9999], and 9999 <= 1057 is false, so we get 416 - // instead of serving the full file. - if ($range[0] >= 0 && ($range[1] <= $size - 1) && $range[0] <= $range[1]) { - http_response_code(206); - header('Content-Range: bytes ' . sprintf('%u-%u/%u', $range[0], $range[1], $size)); - } else { - http_response_code(416); - header('Content-Range: bytes */' . $size); - return; - } - } - - $contentLength = $range[1] - $range[0] + 1; - $ext = pathinfo($path, \PATHINFO_EXTENSION); - - $headers = [ - 'Accept-Ranges' => 'bytes', - 'Content-Length' => $contentLength, - 'Content-Type' => SUPPORTED_TYPES[strtolower($ext)], - 'Cache-Control' => 'public, max-age=604800', - 'Expires' => gmdate('D, d M Y H:i:s \G\M\T', time() + 30 * 86400), - ]; - - foreach ($headers as $k => $v) { - header("{$k}: {$v}", true); - } - - if ($range[0] > 0) { - fseek($fp, $range[0]); - } - - $sentSize = 0; - - while (!feof($fp) && (connection_status() === \CONNECTION_NORMAL)) { - $readingSize = $contentLength - $sentSize; - $readingSize = min($readingSize, 512 * 1024); - - if ($readingSize <= 0) { - break; - } - - $data = fread($fp, $readingSize); - if ($data === false) { - break; - } - - $sentSize += strlen($data); - echo $data; - flush(); - } - - fclose($fp); -} From 0cb5cf15d3ea4da1d559c513e224a48f5a95f609 Mon Sep 17 00:00:00 2001 From: "John R. D'Orazio" Date: Fri, 1 May 2026 18:25:02 +0200 Subject: [PATCH 8/8] style: Drop trailing blank line before class closing brace 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) --- tests/Public/StaticTest.php | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/Public/StaticTest.php b/tests/Public/StaticTest.php index b0e4ed549c..8d55485170 100644 --- a/tests/Public/StaticTest.php +++ b/tests/Public/StaticTest.php @@ -401,5 +401,4 @@ public static function provide_ContentLengthAccuracy_cases(): iterable 'large js (387KB)' => ['program/js/app.js'], ]; } - }