Commit e4daeeab7ce for woocommerce
commit e4daeeab7ced79ad127b8e518d55ea01a245a0d9
Author: Mike Jolley <mike.jolley@me.com>
Date: Wed Oct 14 15:28:23 2026 +0100
Stop stray buffered output from corrupting file downloads (#69549)
* Fix output buffer handling when serving file downloads
* Add changelog entry for download output buffer fix
* Warn about remaining output buffers that have a callback
* Skip setAccessible() in the download buffer test runner on PHP 8.1+
* Remove remaining output buffer warning from downloads
* Narrow the download output buffer changelog entry
* Log when a buffer that can't be removed or cleaned holds output before a download
diff --git a/plugins/woocommerce/changelog/51562-fix-download-output-buffers b/plugins/woocommerce/changelog/51562-fix-download-output-buffers
new file mode 100644
index 00000000000..e2e4f89692d
--- /dev/null
+++ b/plugins/woocommerce/changelog/51562-fix-download-output-buffers
@@ -0,0 +1,4 @@
+Significance: patch
+Type: fix
+
+Fix some cases where output from other plugins could corrupt file downloads.
diff --git a/plugins/woocommerce/includes/class-wc-download-handler.php b/plugins/woocommerce/includes/class-wc-download-handler.php
index 1f48006f1dc..b917442972f 100644
--- a/plugins/woocommerce/includes/class-wc-download-handler.php
+++ b/plugins/woocommerce/includes/class-wc-download-handler.php
@@ -814,16 +814,49 @@ class WC_Download_Handler {
* Can prevent errors, for example: transfer closed with 3 bytes remaining to read.
*/
private static function clean_buffers() {
- if ( ob_get_level() ) {
- $levels = ob_get_level();
- for ( $i = 0; $i < $levels; $i++ ) {
- @ob_end_clean(); // phpcs:ignore Generic.PHP.NoSilencedErrors.Discouraged
+ foreach ( array_reverse( ob_get_status( true ) ) as $buffer ) {
+ $flags = $buffer['flags'];
+
+ if ( $flags & PHP_OUTPUT_HANDLER_REMOVABLE ) {
+ ob_end_clean();
+ continue;
}
- } else {
- @ob_end_clean(); // phpcs:ignore Generic.PHP.NoSilencedErrors.Discouraged
+
+ // This buffer can't be removed, and the ones below it can't be reached, so the file has to go through them.
+ if ( $flags & PHP_OUTPUT_HANDLER_CLEANABLE ) {
+ ob_clean();
+ } elseif ( $buffer['buffer_used'] > 0 ) {
+ wc_get_logger()->warning(
+ sprintf(
+ /* translators: %1$s: output buffer handler name, %2$d: number of bytes. */
+ __( 'Output buffer "%1$s" cannot be removed or cleaned, so %2$d bytes of earlier output will be sent before the downloaded file.', 'woocommerce' ),
+ $buffer['name'],
+ $buffer['buffer_used']
+ )
+ );
+ }
+
+ return;
}
}
+ /**
+ * Send the output so far to the web server.
+ *
+ * PHP's flush() only pushes its own write buffer, so a remaining output buffer has to be flushed into it first.
+ * Only call this after file data has been output: some servers send the headers on flush(), and a read that
+ * fails before any data must still be able to send an error page instead.
+ */
+ private static function flush_output(): void {
+ $buffer = ob_get_status();
+
+ if ( $buffer && ( $buffer['flags'] & PHP_OUTPUT_HANDLER_FLUSHABLE ) ) {
+ ob_flush();
+ }
+
+ flush();
+ }
+
/**
*
* Get selected content disposition
@@ -918,12 +951,11 @@ class WC_Download_Handler {
}
echo $chunk; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped -- Download chunks are raw binary data and must not be HTML-escaped.
- $output_sent = $output_sent || '' !== $chunk;
- $p = @ftell( $handle ); // phpcs:ignore Generic.PHP.NoSilencedErrors.Discouraged,WordPress.PHP.NoSilencedErrors.Discouraged
+ $p = @ftell( $handle ); // phpcs:ignore Generic.PHP.NoSilencedErrors.Discouraged,WordPress.PHP.NoSilencedErrors.Discouraged
- if ( ob_get_length() ) {
- ob_flush();
- flush();
+ if ( '' !== $chunk ) {
+ $output_sent = true;
+ self::flush_output();
}
}
} else {
@@ -936,10 +968,9 @@ class WC_Download_Handler {
}
echo $chunk; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped -- Download chunks are raw binary data and must not be HTML-escaped.
- $output_sent = $output_sent || '' !== $chunk;
- if ( ob_get_length() ) {
- ob_flush();
- flush();
+ if ( '' !== $chunk ) {
+ $output_sent = true;
+ self::flush_output();
}
}
}
diff --git a/plugins/woocommerce/tests/php/helpers/download-handler-buffer-runner.php b/plugins/woocommerce/tests/php/helpers/download-handler-buffer-runner.php
new file mode 100644
index 00000000000..9637638a8be
--- /dev/null
+++ b/plugins/woocommerce/tests/php/helpers/download-handler-buffer-runner.php
@@ -0,0 +1,94 @@
+<?php
+/**
+ * Runs WC_Download_Handler's buffer handling against a given output buffer stack, in its own process.
+ *
+ * Buffers started without PHP_OUTPUT_HANDLER_REMOVABLE can't be removed until the process exits, which
+ * would break every later PHPUnit test, so WC_Download_Handler_Tests runs this script with the PHP CLI
+ * instead. Usage: `php download-handler-buffer-runner.php <flags>[,<flags>...] <file contents>`, with one
+ * flags value per buffer from the bottom of the stack up. Each buffer starts with "[junk-<level>]" in it.
+ *
+ * The process stdout is what a client would receive. A JSON report of the errors raised (including
+ * @-suppressed ones) and the warnings logged is written to stderr.
+ *
+ * @package WooCommerce\Tests
+ */
+
+declare( strict_types = 1 );
+
+// phpcs:disable WordPress.WP.AlternativeFunctions, WordPress.PHP.DevelopmentFunctions, WordPress.Security.EscapeOutput
+
+/**
+ * Hook stub; the class registers its hooks when loaded, but WordPress isn't loaded in this process.
+ */
+function add_action() {}
+
+/**
+ * Translation stub.
+ *
+ * @param string $text Text to translate.
+ * @return string
+ */
+function __( $text ) {
+ return $text;
+}
+
+/**
+ * Logger stub that records warnings for the report.
+ *
+ * @return object
+ */
+function wc_get_logger() {
+ return new class() {
+ /**
+ * Record a warning.
+ *
+ * @param string $message Message.
+ */
+ public function warning( $message ) {
+ $GLOBALS['wc_warnings'][] = $message;
+ }
+ };
+}
+
+define( 'ABSPATH', __DIR__ . '/' );
+
+require dirname( __DIR__, 3 ) . '/includes/class-wc-download-handler.php';
+
+$wc_buffer_flags = '' === ( $argv[1] ?? '' ) ? array() : array_map( 'intval', explode( ',', $argv[1] ) );
+$wc_warnings = array();
+$wc_errors = array();
+$wc_file = tempnam( sys_get_temp_dir(), 'wc-download-buffers' );
+file_put_contents( $wc_file, $argv[2] ?? '' );
+
+foreach ( $wc_buffer_flags as $wc_level => $wc_flags ) {
+ ob_start( null, 0, $wc_flags );
+ echo "[junk-$wc_level]";
+}
+
+set_error_handler(
+ function ( $errno, $errstr ) use ( &$wc_errors ) {
+ $wc_errors[] = $errstr;
+ return true;
+ }
+);
+
+$wc_clean_buffers = new ReflectionMethod( WC_Download_Handler::class, 'clean_buffers' );
+// Needed before PHP 8.1. PHP 8.5 deprecates it, and the error handler would record that.
+if ( PHP_VERSION_ID < 80100 ) {
+ $wc_clean_buffers->setAccessible( true );
+}
+$wc_clean_buffers->invoke( null );
+WC_Download_Handler::readfile_chunked( $wc_file );
+
+restore_error_handler();
+unlink( $wc_file );
+
+fwrite(
+ STDERR,
+ json_encode(
+ array(
+ 'errors' => $wc_errors,
+ 'warnings' => $wc_warnings,
+ )
+ )
+);
diff --git a/plugins/woocommerce/tests/php/includes/class-wc-download-handler-tests.php b/plugins/woocommerce/tests/php/includes/class-wc-download-handler-tests.php
index c6b8b1f358b..4cb9f4d2271 100644
--- a/plugins/woocommerce/tests/php/includes/class-wc-download-handler-tests.php
+++ b/plugins/woocommerce/tests/php/includes/class-wc-download-handler-tests.php
@@ -637,6 +637,209 @@ class WC_Download_Handler_Tests extends \WC_Unit_Test_Case {
);
}
+ /**
+ * @testdox readfile_chunked() should push each chunk through a flushable output buffer instead of holding the whole file in it.
+ *
+ * @dataProvider provider_download_sources
+ *
+ * @param bool $unknown_size Whether to stream from a source with no known size, which reads until EOF.
+ */
+ public function test_readfile_chunked_flushes_each_chunk_through_output_buffer( bool $unknown_size ): void {
+ $chunk_size = defined( 'WC_CHUNK_SIZE' ) ? (int) WC_CHUNK_SIZE : 1024 * 1024;
+ $contents = str_repeat( 'a', $chunk_size ) . str_repeat( 'b', $chunk_size ) . 'c';
+ $source = $this->create_download_source( $contents, $unknown_size );
+ $flushed = array();
+
+ ob_start(
+ function ( $buffer, $phase ) use ( &$flushed ) {
+ if ( $phase & PHP_OUTPUT_HANDLER_FLUSH ) {
+ $flushed[] = $buffer;
+ }
+ return '';
+ }
+ );
+
+ try {
+ $served = WC_Download_Handler::readfile_chunked( $source );
+ } finally {
+ ob_end_clean();
+ $this->delete_download_source( $source );
+ }
+
+ $this->assertTrue( $served, 'The file should be reported as served.' );
+ $this->assertGreaterThan( 1, count( $flushed ), 'Each chunk should be flushed as it is read.' );
+ $this->assertTrue( implode( '', $flushed ) === $contents, 'Every byte of the file should pass through the buffer by being flushed.' );
+ }
+
+ /**
+ * @testdox readfile_chunked() should not try to flush an output buffer that can't be flushed.
+ *
+ * @dataProvider provider_download_sources
+ *
+ * @param bool $unknown_size Whether to stream from a source with no known size, which reads until EOF.
+ */
+ public function test_readfile_chunked_does_not_flush_unflushable_output_buffer( bool $unknown_size ): void {
+ $source = $this->create_download_source( 'file-data', $unknown_size );
+ $errors = array();
+
+ // WordPress hides notices unless WP_DEBUG is on, and sites that turn it on get them printed into the download.
+ $error_reporting = error_reporting( E_ALL ); // phpcs:ignore WordPress.PHP.DevelopmentFunctions.prevent_path_disclosure_error_reporting, WordPress.PHP.DiscouragedPHPFunctions.runtime_configuration_error_reporting
+ ob_start( null, 0, PHP_OUTPUT_HANDLER_CLEANABLE | PHP_OUTPUT_HANDLER_REMOVABLE );
+ // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_set_error_handler -- Records notices rather than letting the first one abort the download mid-stream.
+ set_error_handler(
+ function ( $errno, $errstr ) use ( &$errors ) {
+ // Leave out errors silenced with @, such as filesize() failing on a stream of unknown size.
+ if ( error_reporting() & $errno ) { // phpcs:ignore WordPress.PHP.DevelopmentFunctions.prevent_path_disclosure_error_reporting, WordPress.PHP.DiscouragedPHPFunctions.runtime_configuration_error_reporting
+ $errors[] = $errstr;
+ }
+ return true;
+ }
+ );
+
+ try {
+ $served = WC_Download_Handler::readfile_chunked( $source );
+ $buffered = ob_get_contents();
+ } finally {
+ restore_error_handler();
+ error_reporting( $error_reporting ); // phpcs:ignore WordPress.PHP.DevelopmentFunctions.prevent_path_disclosure_error_reporting, WordPress.PHP.DiscouragedPHPFunctions.runtime_configuration_error_reporting
+ ob_end_clean();
+ $this->delete_download_source( $source );
+ }
+
+ $this->assertSame( array(), $errors, 'Streaming into a buffer that cannot be flushed should not raise notices, which would be written into the download.' );
+ $this->assertTrue( $served, 'The file should be reported as served.' );
+ $this->assertSame( 'file-data', $buffered, 'The file should be left intact in the buffer.' );
+ }
+
+ /**
+ * @testdox readfile_chunked() should not flush until file data has been read, so a failed read can still send an error page.
+ *
+ * @dataProvider provider_download_sources
+ *
+ * @param bool $unknown_size Whether to stream from a source with no known size, which reads until EOF.
+ */
+ public function test_readfile_chunked_does_not_flush_before_file_data( bool $unknown_size ): void {
+ // An empty source gives one empty read before EOF.
+ $source = $this->create_download_source( '', $unknown_size );
+ $flushes = 0;
+
+ ob_start(
+ function ( $buffer, $phase ) use ( &$flushes ) {
+ if ( $phase & PHP_OUTPUT_HANDLER_FLUSH ) {
+ ++$flushes;
+ }
+ return '';
+ }
+ );
+
+ try {
+ WC_Download_Handler::readfile_chunked( $source );
+ } finally {
+ ob_end_clean();
+ $this->delete_download_source( $source );
+ }
+
+ $this->assertSame( 0, $flushes, 'Some servers send the headers on flush(), so nothing should be flushed before file data is output.' );
+ }
+
+ /**
+ * Download sources for streaming coverage.
+ *
+ * @return array<string, array<bool>>
+ */
+ public function provider_download_sources(): array {
+ return array(
+ 'local file of known size' => array( false ),
+ 'stream of unknown size' => array( true ),
+ );
+ }
+
+ /**
+ * @testdox Should send only the file through any output buffers it can remove or clean, without raising errors, and log a buffer whose output reaches the client.
+ *
+ * @dataProvider provider_output_buffer_stacks
+ *
+ * @param int[] $buffer_flags Flags for each output buffer, from the bottom of the stack up. Each buffer holds "[junk-<level>]".
+ * @param string $expected_body Response body the client should receive.
+ * @param string[] $expected_warnings Warnings that should be logged.
+ */
+ public function test_download_output_through_output_buffers( array $buffer_flags, string $expected_body, array $expected_warnings = array() ): void {
+ // Buffers that can't be removed would outlive this test, so run the download handler in its own process.
+ // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.system_calls_proc_open -- Runs the PHP CLI on a fixed script with no user input.
+ $process = proc_open(
+ array( PHP_BINARY, '-d', 'display_errors=stderr', dirname( __DIR__ ) . '/helpers/download-handler-buffer-runner.php', implode( ',', $buffer_flags ), 'FILE-DATA' ),
+ array(
+ 1 => array( 'pipe', 'w' ),
+ 2 => array( 'pipe', 'w' ),
+ ),
+ $pipes
+ );
+ $body = stream_get_contents( $pipes[1] );
+ $stderr = stream_get_contents( $pipes[2] );
+ fclose( $pipes[1] ); // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_operations_fclose
+ fclose( $pipes[2] ); // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_operations_fclose
+ proc_close( $process );
+
+ $report = json_decode( $stderr, true );
+
+ $this->assertIsArray( $report, "The runner script did not complete: $stderr" );
+ $this->assertSame( $expected_body, $body, 'The client should receive the file without the content of buffers that could be cleaned.' );
+ $this->assertSame( array(), $report['errors'], 'Cleaning and flushing buffers should not raise errors, even silenced ones.' );
+ $this->assertSame( $expected_warnings, $report['warnings'], 'Only a buffer whose output reaches the client should be logged.' );
+ }
+
+ /**
+ * Output buffer stacks a download can be served through.
+ *
+ * @return array<string, array{0: int[], 1: string, 2?: string[]}>
+ */
+ public function provider_output_buffer_stacks(): array {
+ $cleanable_flushable = PHP_OUTPUT_HANDLER_CLEANABLE | PHP_OUTPUT_HANDLER_FLUSHABLE;
+
+ return array(
+ 'no buffers' => array( array(), 'FILE-DATA' ),
+ 'standard buffers' => array( array( PHP_OUTPUT_HANDLER_STDFLAGS, PHP_OUTPUT_HANDLER_STDFLAGS ), 'FILE-DATA' ),
+ 'cleanable and flushable, not removable' => array( array( $cleanable_flushable ), 'FILE-DATA' ),
+ 'cleanable only' => array( array( PHP_OUTPUT_HANDLER_CLEANABLE ), 'FILE-DATA' ),
+ 'standard below non-removable' => array( array( PHP_OUTPUT_HANDLER_STDFLAGS, $cleanable_flushable ), '[junk-0]FILE-DATA' ),
+ 'buffers from issue 51562' => array( array( 0, 0, PHP_OUTPUT_HANDLER_CLEANABLE, PHP_OUTPUT_HANDLER_REMOVABLE ), '[junk-0][junk-1]FILE-DATA' ),
+ 'not removable or cleanable' => array(
+ array( 0, PHP_OUTPUT_HANDLER_REMOVABLE ),
+ '[junk-0]FILE-DATA',
+ array( 'Output buffer "default output handler" cannot be removed or cleaned, so 8 bytes of earlier output will be sent before the downloaded file.' ),
+ ),
+ );
+ }
+
+ /**
+ * Create a download source holding the given contents.
+ *
+ * @param string $contents File contents.
+ * @param bool $unknown_size Whether to use a data: URL, which has no size, instead of a temporary file.
+ * @return string File path or URL.
+ */
+ private function create_download_source( string $contents, bool $unknown_size ): string {
+ if ( $unknown_size ) {
+ return 'data://application/octet-stream;base64,' . base64_encode( $contents ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.obfuscation_base64_encode
+ }
+
+ $file = wp_tempnam( 'wc-download-handler-streaming' );
+ file_put_contents( $file, $contents ); // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_operations_file_put_contents -- Test fixture written to the temp directory.
+
+ return $file;
+ }
+
+ /**
+ * Delete a download source created by create_download_source().
+ *
+ * @param string $source File path or URL.
+ */
+ private function delete_download_source( string $source ): void {
+ if ( is_file( $source ) ) {
+ wp_delete_file( $source );
+ }
+ }
+
/**
* @testdox The Content-Type fallback to the resolved filename should apply to remote files only.
*/