From 87df8ebbf25b097b731696a8cbb9c7beda9cbd8f Mon Sep 17 00:00:00 2001 From: selul Date: Tue, 1 Sep 2026 13:07:18 +0300 Subject: [PATCH 1/4] fix: process page HTML outside PHP's output-buffer display handler Running replace_content() as the ob_start() display handler meant any output-buffering call from third-party code hooked into our filters was a fatal error, and any real fatal during processing (e.g. memory exhaustion) was masked as "Cannot use output buffering in output buffering display handlers" with a misleading crash location. The buffer is now a plain capture: close_buffer() flushes third-party buffers stacked above ours, captures our own by its recorded nesting level (never popping someone else's buffer), processes the HTML in normal execution context and re-arms the capture so late shutdown output is still handled. The attached handler remains only as a fallback that keeps the previous behavior when third-party code flushes our buffer mid-request. Also replaces the per-URL full-page preg_replace() loop with chunked single-pass replacement to reduce peak memory on large pages, the likely trigger of the masked production fatals. Fixes #1126 Co-Authored-By: Claude Fable 5 --- inc/manager.php | 163 ++++++++++++++++++++++++++--- tests/test-zz-buffer.php | 217 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 364 insertions(+), 16 deletions(-) create mode 100644 tests/test-zz-buffer.php diff --git a/inc/manager.php b/inc/manager.php index ffb713c7..04d9203b 100644 --- a/inc/manager.php +++ b/inc/manager.php @@ -122,6 +122,23 @@ final class Optml_Manager { * @var boolean Buffer state. */ private static $ob_started = false; + /** + * The output-buffer nesting level of our capture buffer. + * + * Used to make sure we only ever capture or close our own buffer and not + * one started by a third party. + * + * @var int Buffer nesting level, 0 when no capture buffer is armed. + */ + private static $ob_level = 0; + /** + * Whether the captured buffer was already processed at shutdown. + * + * When true, the fallback output handler passes content through untouched. + * + * @var boolean Processed state. + */ + private static $ob_processed = false; /** * Class instance method. @@ -408,6 +425,7 @@ public function register_hooks() { add_action( 'template_redirect', [ $this, 'register_after_setup' ] ); add_action( 'rest_api_init', [ $this, 'process_template_redirect_content' ], PHP_INT_MIN ); add_action( 'shutdown', [ $this, 'close_buffer' ], PHP_INT_MIN ); + add_action( 'shutdown', [ $this, 'close_final_buffer' ], PHP_INT_MAX ); foreach ( self::$loaded_compatibilities as $registered_compatibility ) { $registered_compatibility->register(); } @@ -780,8 +798,27 @@ function ( $url ) use ( $upload_resource ) { $urls ); - foreach ( $urls as $origin => $replace ) { - $html = preg_replace( '/(? $replace ) { + $quoted[] = preg_quote( $origin, '/' ); + } + $result = preg_replace_callback( + '/(?start_capture_buffer(); + } + + /** + * Start an output buffer that captures the page HTML. + * + * On normal requests the buffer is captured and processed by close_buffer() + * at shutdown, outside of PHP's display-handler context, so callbacks hooked + * into our filters are free to use output buffering themselves and fatal + * errors raised during processing keep their real message instead of being + * masked by "Cannot use output buffering in output buffering display handlers". + * + * The attached handler is only a fallback for third-party code that flushes + * our buffer before shutdown (streaming via ob_flush(), force-flush loops): + * in that case it processes the flushed chunk in handler context, matching + * the previous behavior. + * + * @return void + */ + private function start_capture_buffer() { + self::$ob_processed = false; ob_start( function ( $content ) { - /* - * Wrap the call to replace_content() so that PHP’s output-buffering system - * does not pass its own second argument ($phase bitmask) to our method. - * - * replace_content() expects the second parameter to be a boolean $partial, - * indicating whether the content is a partial replacement (e.g. for - * viewport lazy-load) or a full page. If PHP’s $phase integer is passed - * directly, it would be misinterpreted as $partial and break the logic. - * - * This closure filters the call, forwarding only the captured HTML buffer. - */ - return $this->replace_content( $content, self::is_ajax_request() ); + /* + * The closure also shields replace_content() from PHP's second + * display-handler argument ($phase bitmask), which would be + * misinterpreted as the boolean $partial parameter. + */ + if ( self::$ob_processed || $content === '' ) { + return $content; + } + try { + return $this->replace_content( $content, self::is_ajax_request() ); + } catch ( Throwable $t ) { + // Never break the page from inside a display handler. + do_action( 'optml_log', 'replace_content failed inside the output handler: ' . $t->getMessage() ); + return $content; + } } ); + self::$ob_level = ob_get_level(); } /** * Close the buffer and flush the content. */ public function close_buffer() { - if ( self::$ob_started && ob_get_length() ) { - ob_end_flush(); + if ( ! self::$ob_started ) { + return; + } + + /** + * Filters whether the captured page is processed at shutdown, outside of + * PHP's display-handler context. Return false to restore the legacy + * behavior of processing inside the output-buffer handler. + * + * @param bool $capture_at_shutdown Whether to process the buffer at shutdown. + */ + if ( apply_filters( 'optml_capture_at_shutdown', true ) === false ) { + if ( ob_get_length() ) { + ob_end_flush(); + } + return; + } + + /* + * Flush the buffers other plugins stacked on top of ours so their + * handlers still transform the page before we process it, preserving + * the same order as a full top-down flush at request shutdown. + */ + while ( ob_get_level() > self::$ob_level ) { + // phpcs:ignore WordPress.PHP.NoSilencedErrors.Discouraged -- a non-flushable buffer must not raise a notice; we stop on failure. + if ( ! @ob_end_flush() ) { + break; + } + } + + if ( ! $this->capture_and_process_buffer() ) { + do_action( 'optml_log', 'Optimole buffer was closed earlier by third-party code.' ); + return; } + + /* + * Re-arm the capture so output echoed by later shutdown callbacks is + * still processed and unguarded third-party flush calls find a buffer + * to close instead of raising a notice. + */ + $this->start_capture_buffer(); + } + + /** + * Close the re-armed buffer at the very end of shutdown. + * + * @return void + */ + public function close_final_buffer() { + if ( ! self::$ob_started ) { + return; + } + $this->capture_and_process_buffer(); + } + + /** + * Capture our buffer, process it outside the display-handler context and echo the result. + * + * @return bool Whether our buffer was found and consumed. + */ + private function capture_and_process_buffer() { + if ( self::$ob_level === 0 || ob_get_level() !== self::$ob_level ) { + return false; + } + $html = ob_get_contents(); + // Set before ob_end_clean() so our handler no-ops during buffer cleanup. + self::$ob_processed = true; + ob_end_clean(); + if ( $html !== false && $html !== '' ) { + echo $this->replace_content( $html, self::is_ajax_request() ); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped -- full page HTML, escaping would break the page. + } + return true; } /** * Throw error on object clone diff --git a/tests/test-zz-buffer.php b/tests/test-zz-buffer.php new file mode 100644 index 00000000..d38715d9 --- /dev/null +++ b/tests/test-zz-buffer.php @@ -0,0 +1,217 @@ +Test '; + + /** + * The output-buffer nesting level before each test. + * + * @var int + */ + private $base_level = 0; + + public function setUp(): void { + parent::setUp(); + $settings = new Optml_Settings(); + $settings->update( 'service_data', [ + 'cdn_key' => 'test123', + 'cdn_secret' => '12345', + 'whitelist' => [ 'example.com', 'example.org' ], + ] ); + $settings->update( 'lazyload', 'disabled' ); + $settings->update( 'cdn', 'enabled' ); + Optml_Url_Replacer::instance()->init(); + Optml_Tag_Replacer::instance()->init(); + Optml_Manager::instance()->init(); + + $this->reset_buffer_state(); + $this->base_level = ob_get_level(); + } + + public function tearDown(): void { + // Make any leftover capture handler a pass-through before cleaning up. + $this->reset_buffer_state( true ); + while ( ob_get_level() > $this->base_level ) { + // phpcs:ignore Generic.PHP.NoSilencedErrors.Discouraged + if ( ! @ob_end_clean() ) { + break; + } + } + $this->reset_buffer_state(); + parent::tearDown(); + } + + /** + * Reset Optml_Manager buffer statics between tests. + * + * @param bool $processed Value for the processed flag. + */ + private function reset_buffer_state( $processed = false ) { + $reflection = new ReflectionClass( Optml_Manager::class ); + foreach ( [ 'ob_started' => false, 'ob_level' => 0, 'ob_processed' => $processed ] as $property => $value ) { + $prop = $reflection->getProperty( $property ); + $prop->setAccessible( true ); + $prop->setValue( null, $value ); + } + } + + /** + * Callbacks on our filters may use output buffering without fataling. + * + * Before processing moved outside the display handler, the nested + * ob_start() below crashed with "Cannot use output buffering in output + * buffering display handlers". + */ + public function test_filter_callbacks_can_use_output_buffering() { + $manager = Optml_Manager::instance(); + $probed = 0; + add_filter( + 'optml_url_pre_process', + function ( $html ) use ( &$probed ) { + ob_start(); + echo 'probe'; + ob_get_clean(); + $probed ++; + return $html; + } + ); + ob_start(); + $manager->process_template_redirect_content(); + echo self::IMG_TAGS; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped + $manager->close_buffer(); + $manager->close_final_buffer(); + $out = ob_get_clean(); + + $this->assertSame( 1, $probed ); + $this->assertStringContainsString( 'i.optimole.com', $out ); + $this->assertSame( $this->base_level, ob_get_level() ); + } + + /** + * A buffer another plugin stacks on top of ours is flushed through its own + * handler first, and we process its transformed output — never swallow it. + */ + public function test_foreign_buffer_above_is_flushed_first() { + $manager = Optml_Manager::instance(); + ob_start(); + $manager->process_template_redirect_content(); + // Third-party handler started after ours, e.g. a minifier. + ob_start( + function ( $content ) { + return $content . ''; + } + ); + echo self::IMG_TAGS; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped + $manager->close_buffer(); + $manager->close_final_buffer(); + $out = ob_get_clean(); + + // Both the page image and the one appended by the foreign handler are optimized. + $this->assertSame( 2, substr_count( $out, 'i.optimole.com' ) ); + $this->assertStringNotContainsString( '"http://example.org/wp-content/uploads/foreign.jpg', $out ); + $this->assertSame( $this->base_level, ob_get_level() ); + } + + /** + * When third-party code force-flushes our buffer before shutdown, the + * fallback handler processes the content — matching the legacy behavior — + * and close_buffer() detects the loss without side effects. + */ + public function test_third_party_flush_falls_back_to_handler_processing() { + $manager = Optml_Manager::instance(); + ob_start(); + $manager->process_template_redirect_content(); + echo self::IMG_TAGS; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped + ob_end_flush(); // Third-party force flush of our buffer. + $this->assertSame( $this->base_level + 1, ob_get_level() ); + $manager->close_buffer(); + $manager->close_final_buffer(); + $out = ob_get_clean(); + + $this->assertSame( 1, substr_count( $out, 'i.optimole.com' ) ); + $this->assertSame( $this->base_level, ob_get_level() ); + } + + /** + * Calling process_template_redirect_content() twice must not stack a + * second buffer, and the page is processed exactly once. + */ + public function test_buffer_started_once() { + $manager = Optml_Manager::instance(); + ob_start(); + $manager->process_template_redirect_content(); + $level = ob_get_level(); + $manager->process_template_redirect_content(); + $this->assertSame( $level, ob_get_level() ); + echo self::IMG_TAGS; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped + $manager->close_buffer(); + $manager->close_final_buffer(); + $out = ob_get_clean(); + + $this->assertSame( 1, substr_count( $out, 'i.optimole.com' ) ); + } + + /** + * An empty buffer closes without output or errors. + */ + public function test_empty_buffer_no_output() { + $manager = Optml_Manager::instance(); + ob_start(); + $manager->process_template_redirect_content(); + $manager->close_buffer(); + $manager->close_final_buffer(); + + $this->assertSame( '', ob_get_clean() ); + $this->assertSame( $this->base_level, ob_get_level() ); + } + + /** + * Output echoed by shutdown callbacks running after close_buffer() is + * captured by the re-armed buffer and still processed. + */ + public function test_late_shutdown_output_is_processed() { + $manager = Optml_Manager::instance(); + ob_start(); + $manager->process_template_redirect_content(); + echo self::IMG_TAGS; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped + $manager->close_buffer(); + echo self::IMG_TAGS; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped + $manager->close_final_buffer(); + $out = ob_get_clean(); + + $this->assertSame( 2, substr_count( $out, 'i.optimole.com' ) ); + $this->assertSame( $this->base_level, ob_get_level() ); + } + + /** + * The optml_capture_at_shutdown filter restores the legacy in-handler flow. + */ + public function test_legacy_in_handler_mode() { + add_filter( 'optml_capture_at_shutdown', '__return_false' ); + $manager = Optml_Manager::instance(); + ob_start(); + $manager->process_template_redirect_content(); + echo self::IMG_TAGS; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped + $manager->close_buffer(); + $manager->close_final_buffer(); + $out = ob_get_clean(); + + $this->assertSame( 1, substr_count( $out, 'i.optimole.com' ) ); + $this->assertSame( $this->base_level, ob_get_level() ); + } +} From 1900dd377448fc900ab266766cd9cbabdc0879e0 Mon Sep 17 00:00:00 2001 From: selul Date: Tue, 1 Sep 2026 13:19:40 +0300 Subject: [PATCH 2/4] fix: bound URL replacement chunks by pattern size, not only count A chunk of 200 very long URLs (e.g. signed CDN URLs with kilobyte-sized query strings) could exceed PCRE's ~64KB compiled-pattern limit, failing the whole chunk and leaving those URLs unreplaced. Chunks now flush when the accumulated quoted pattern reaches 24KB, so compilation always succeeds regardless of URL length, and a failed chunk is logged via optml_log instead of being silently skipped. Co-Authored-By: Claude Fable 5 --- inc/manager.php | 33 +++++++++++++++++++++++++++------ tests/test-zz-buffer.php | 25 +++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 6 deletions(-) diff --git a/inc/manager.php b/inc/manager.php index 04d9203b..31207d2d 100644 --- a/inc/manager.php +++ b/inc/manager.php @@ -802,14 +802,33 @@ function ( $url ) use ( $upload_resource ) { * Replace all URLs in a single pass per chunk instead of one full-page * preg_replace() per URL, which allocated a new copy of the whole page * for every replaced URL and could exhaust memory on large pages. - * Chunking keeps the compiled pattern size bounded. + * Chunks are bounded by pattern size, not only count, so the compiled + * regex stays within PCRE's ~64KB limit even for very long URLs + * (e.g. signed CDN URLs with kilobyte-sized query strings). */ - foreach ( array_chunk( $urls, 200, true ) as $chunk ) { - $quoted = []; - foreach ( $chunk as $origin => $replace ) { - $quoted[] = preg_quote( $origin, '/' ); + $chunks = []; + $chunk = []; + $quoted = []; + $quoted_size = 0; + foreach ( $urls as $origin => $replace ) { + $quoted_origin = preg_quote( $origin, '/' ); + if ( ! empty( $chunk ) && ( count( $chunk ) >= 200 || $quoted_size + strlen( $quoted_origin ) > 24000 ) ) { + $chunks[] = [ $chunk, $quoted ]; + $chunk = []; + $quoted = []; + $quoted_size = 0; } - $result = preg_replace_callback( + $chunk[ $origin ] = $replace; + $quoted[] = $quoted_origin; + $quoted_size += strlen( $quoted_origin ) + 1; + } + if ( ! empty( $chunk ) ) { + $chunks[] = [ $chunk, $quoted ]; + } + + foreach ( $chunks as $pair ) { + list( $chunk, $quoted ) = $pair; + $result = preg_replace_callback( '/(?assertSame( $this->base_level, ob_get_level() ); } + /** + * Very long URLs (e.g. signed CDN URLs) must not push a replacement + * chunk's compiled pattern over PCRE's size limit. + */ + public function test_long_url_replacement_stays_within_pcre_limits() { + $manager = Optml_Manager::instance(); + add_filter( + 'optml_content_url', + function ( $url ) { + return 'https://replaced.test/marker'; + } + ); + $urls = []; + $html = ''; + for ( $i = 0; $i < 250; $i ++ ) { + $url = 'https://example.org/image-' . $i . '.jpg?X-Signature=' . str_repeat( 'a1b2c3d4', 180 ) . '&i=' . $i; + $urls[] = $url; + $html .= ''; + } + $out = $manager->do_url_replacement( $html, $urls ); + + $this->assertSame( 250, substr_count( $out, 'https://replaced.test/marker' ) ); + $this->assertStringNotContainsString( 'X-Signature', $out ); + } + /** * The optml_capture_at_shutdown filter restores the legacy in-handler flow. */ From 8892a33760afbb524f96a026b37cb07d64b1b9d7 Mon Sep 17 00:00:00 2001 From: selul Date: Tue, 1 Sep 2026 13:33:51 +0300 Subject: [PATCH 3/4] perf: apply URL replacement chunks as they fill Building every chunk's bookkeeping up front held all origin/replacement maps in memory at once, which cost about 1MB extra on pages with thousands of URLs. Each chunk is now applied as soon as it fills, so only one chunk's bookkeeping exists at a time; peak memory is now at or below the old per-URL loop at every scale. Co-Authored-By: Claude Fable 5 --- inc/manager.php | 56 ++++++++++++++++++++++++++++--------------------- 1 file changed, 32 insertions(+), 24 deletions(-) diff --git a/inc/manager.php b/inc/manager.php index 31207d2d..10ce310a 100644 --- a/inc/manager.php +++ b/inc/manager.php @@ -800,20 +800,20 @@ function ( $url ) use ( $upload_resource ) { /* * Replace all URLs in a single pass per chunk instead of one full-page - * preg_replace() per URL, which allocated a new copy of the whole page - * for every replaced URL and could exhaust memory on large pages. - * Chunks are bounded by pattern size, not only count, so the compiled - * regex stays within PCRE's ~64KB limit even for very long URLs - * (e.g. signed CDN URLs with kilobyte-sized query strings). + * preg_replace() per URL, which scanned and rebuilt the whole page for + * every replaced URL. Chunks are bounded by pattern size, not only + * count, so the compiled regex stays within PCRE's ~64KB limit even + * for very long URLs (e.g. signed CDN URLs with kilobyte-sized query + * strings). Each chunk is applied as soon as it fills, so only one + * chunk's bookkeeping is in memory at a time. */ - $chunks = []; $chunk = []; $quoted = []; $quoted_size = 0; foreach ( $urls as $origin => $replace ) { $quoted_origin = preg_quote( $origin, '/' ); if ( ! empty( $chunk ) && ( count( $chunk ) >= 200 || $quoted_size + strlen( $quoted_origin ) > 24000 ) ) { - $chunks[] = [ $chunk, $quoted ]; + $html = $this->replace_urls_chunk( $html, $chunk, $quoted ); $chunk = []; $quoted = []; $quoted_size = 0; @@ -823,28 +823,36 @@ function ( $url ) use ( $upload_resource ) { $quoted_size += strlen( $quoted_origin ) + 1; } if ( ! empty( $chunk ) ) { - $chunks[] = [ $chunk, $quoted ]; - } - - foreach ( $chunks as $pair ) { - list( $chunk, $quoted ) = $pair; - $result = preg_replace_callback( - '/(?replace_urls_chunk( $html, $chunk, $quoted ); } return $html; } + /** + * Replace one chunk of URLs in the content with a single combined pattern. + * + * @param string $html Content to process. + * @param array $chunk Map of origin => replacement URLs. + * @param string[] $quoted The preg_quote()d origins, in the same order. + * + * @return string Processed content, unchanged when the pattern fails. + */ + private function replace_urls_chunk( $html, $chunk, $quoted ) { + $result = preg_replace_callback( + '/(? Date: Tue, 1 Sep 2026 16:48:27 +0300 Subject: [PATCH 4/4] =?UTF-8?q?fix:=20address=20review=20=E2=80=94=20no=20?= =?UTF-8?q?in-handler=20processing,=20real=20buffer=20ownership?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P1: the fallback handler no longer runs replace_content() when a third party flushes our buffer early. The ob-in-handler fatal is an engine E_ERROR that catch (Throwable) cannot intercept, so processing there reintroduced the crash this rework removes; early-flushed content is now passed through unprocessed and logged. Only the explicit legacy mode (optml_capture_at_shutdown false) keeps in-handler processing. P2: buffer ownership is now verified by handler identity, not nesting level alone. The capture buffer uses a named method handler so ob_get_status()['name'] reports Optml_Manager::handle_buffer_fallback, and capture_and_process_buffer() refuses any buffer that does not carry it — a foreign buffer at our recorded level is never consumed. Co-Authored-By: Claude Fable 5 --- inc/manager.php | 76 ++++++++++++++++++++++++++++------------ tests/test-zz-buffer.php | 59 ++++++++++++++++++++++++++++--- 2 files changed, 108 insertions(+), 27 deletions(-) diff --git a/inc/manager.php b/inc/manager.php index 10ce310a..0e9a34ce 100644 --- a/inc/manager.php +++ b/inc/manager.php @@ -877,37 +877,59 @@ public function process_template_redirect_content() { * errors raised during processing keep their real message instead of being * masked by "Cannot use output buffering in output buffering display handlers". * - * The attached handler is only a fallback for third-party code that flushes - * our buffer before shutdown (streaming via ob_flush(), force-flush loops): - * in that case it processes the flushed chunk in handler context, matching - * the previous behavior. + * The attached handler is only a fallback for buffers flushed outside of + * close_buffer() — third-party force-flush loops, ob_flush() streaming, or + * core's wp_ob_end_flush_all() reaching the re-armed buffer. A named method + * is used instead of a closure so the buffer can be identified as ours via + * ob_get_status()['name']. * * @return void */ private function start_capture_buffer() { self::$ob_processed = false; - ob_start( - function ( $content ) { - /* - * The closure also shields replace_content() from PHP's second - * display-handler argument ($phase bitmask), which would be - * misinterpreted as the boolean $partial parameter. - */ - if ( self::$ob_processed || $content === '' ) { - return $content; - } - try { - return $this->replace_content( $content, self::is_ajax_request() ); - } catch ( Throwable $t ) { - // Never break the page from inside a display handler. - do_action( 'optml_log', 'replace_content failed inside the output handler: ' . $t->getMessage() ); - return $content; - } - } - ); + ob_start( [ $this, 'handle_buffer_fallback' ] ); self::$ob_level = ob_get_level(); } + /** + * The handler name PHP reports for our capture buffer in ob_get_status(). + */ + const OB_HANDLER_NAME = 'Optml_Manager::handle_buffer_fallback'; + + /** + * Output-buffer handler attached to our capture buffer. + * + * Runs only when the buffer is flushed outside of close_buffer(). Content is + * passed through UNPROCESSED here: running the replacement filter graph + * inside a PHP display handler would turn any third-party ob_*() call into + * an uncatchable fatal ("Cannot use output buffering in output buffering + * display handlers") — the very crash this rework removes. The only + * exception is the legacy mode selected via the optml_capture_at_shutdown + * filter, which explicitly restores the previous in-handler processing. + * + * @param string $content The buffered content. + * @param int $phase PHP's output-handler phase bitmask (unused; keeps replace_content()'s $partial parameter shielded from it). + * + * @return string The content to output. + */ + public function handle_buffer_fallback( $content, $phase = 0 ) { + if ( self::$ob_processed || $content === '' ) { + return $content; + } + if ( apply_filters( 'optml_capture_at_shutdown', true ) === false ) { + try { + return $this->replace_content( $content, self::is_ajax_request() ); + } catch ( Throwable $t ) { + // Never break the page from inside a display handler. + do_action( 'optml_log', 'replace_content failed inside the output handler: ' . $t->getMessage() ); + return $content; + } + } + do_action( 'optml_log', 'Optimole buffer was flushed outside close_buffer(); content passed through unprocessed.' ); + + return $content; + } + /** * Close the buffer and flush the content. */ @@ -970,12 +992,20 @@ public function close_final_buffer() { /** * Capture our buffer, process it outside the display-handler context and echo the result. * + * Ownership is verified by both nesting level and handler identity, so a + * buffer another plugin opened at the same level after ours was closed is + * never captured or closed by us. + * * @return bool Whether our buffer was found and consumed. */ private function capture_and_process_buffer() { if ( self::$ob_level === 0 || ob_get_level() !== self::$ob_level ) { return false; } + $status = ob_get_status(); + if ( ( $status['name'] ?? '' ) !== self::OB_HANDLER_NAME ) { + return false; + } $html = ob_get_contents(); // Set before ob_end_clean() so our handler no-ops during buffer cleanup. self::$ob_processed = true; diff --git a/tests/test-zz-buffer.php b/tests/test-zz-buffer.php index 43edebc4..15425d5c 100644 --- a/tests/test-zz-buffer.php +++ b/tests/test-zz-buffer.php @@ -129,10 +129,11 @@ function ( $content ) { /** * When third-party code force-flushes our buffer before shutdown, the - * fallback handler processes the content — matching the legacy behavior — - * and close_buffer() detects the loss without side effects. + * content is passed through UNPROCESSED: running the filter graph inside a + * display handler would make any third-party ob_*() call an uncatchable + * fatal. close_buffer() detects the loss without side effects. */ - public function test_third_party_flush_falls_back_to_handler_processing() { + public function test_third_party_flush_passes_content_through() { $manager = Optml_Manager::instance(); ob_start(); $manager->process_template_redirect_content(); @@ -143,10 +144,60 @@ public function test_third_party_flush_falls_back_to_handler_processing() { $manager->close_final_buffer(); $out = ob_get_clean(); - $this->assertSame( 1, substr_count( $out, 'i.optimole.com' ) ); + $this->assertStringNotContainsString( 'i.optimole.com', $out ); + $this->assertStringContainsString( 'themes/twentyseventeen/assets/images/header.jpg', $out ); + $this->assertSame( $this->base_level, ob_get_level() ); + } + + /** + * A third-party flush combined with an output-buffering filter callback + * must not fatal. Processing inside the handler would terminate PHP with + * "Cannot use output buffering in output buffering display handlers", + * which catch ( Throwable ) cannot intercept. + */ + public function test_third_party_flush_with_ob_filter_does_not_fatal() { + $manager = Optml_Manager::instance(); + add_filter( + 'optml_url_pre_process', + function ( $html ) { + ob_start(); + echo 'probe'; + ob_get_clean(); + return $html; + } + ); + ob_start(); + $manager->process_template_redirect_content(); + echo self::IMG_TAGS; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped + ob_end_flush(); // Would exit(255) if the handler ran the filter graph. + $manager->close_buffer(); + $manager->close_final_buffer(); + $out = ob_get_clean(); + + $this->assertStringContainsString( 'themes/twentyseventeen/assets/images/header.jpg', $out ); $this->assertSame( $this->base_level, ob_get_level() ); } + /** + * A foreign buffer that ends up at our recorded nesting level is never + * captured or closed: ownership requires our handler identity, not just + * the level. + */ + public function test_foreign_buffer_at_same_level_is_not_consumed() { + $manager = Optml_Manager::instance(); + $manager->process_template_redirect_content(); + ob_end_clean(); // Third party discards our buffer... + ob_start(); // ...and opens its own at the same level. + echo 'FOREIGN'; + $manager->close_buffer(); + $manager->close_final_buffer(); + + $this->assertSame( $this->base_level + 1, ob_get_level() ); + $this->assertSame( 'default output handler', ob_get_status()['name'] ); + $this->assertStringContainsString( 'FOREIGN', ob_get_contents() ); + ob_end_clean(); + } + /** * Calling process_template_redirect_content() twice must not stack a * second buffer, and the page is processed exactly once.