Skip to content

Commit

Permalink
Prevent output buffering inside display handlers since causes fatal e…
Browse files Browse the repository at this point in the history 
…rror
  • Loading branch information
@westonruter
westonruter committed Apr 28, 2018
1 parent 8f35f13 commit ff5eaa5
Show file tree
Hide file tree
Showing 3 changed files with 131 additions and 42 deletions.
70 changes: 47 additions & 23 deletions includes/class-amp-theme-support.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -963,8 +963,21 @@ public static function dequeue_customize_preview_scripts() {
*
* @since 0.7
* @see AMP_Theme_Support::finish_output_buffering()
*
* @param array $args {
* Args.
*
* @type bool $cancelable Whether the output buffer can be flushed/removed/cleaned.
* }
*/
public static function start_output_buffering() {
public static function start_output_buffering( $args = array() ) {
$args = array_merge(
array(
'cancelable' => true, // Must be cancelable during unit tests.
),
$args
);

/*
* Disable the New Relic Browser agent on AMP responses.
* This prevents th New Relic from causing invalid AMP responses due the NREUM script it injects after the meta charset:
Expand All @@ -976,27 +989,32 @@ public static function start_output_buffering() {
newrelic_disable_autorum();
}

ob_start();
self::$initial_ob_level = ob_get_level();
$chunk_size = 0; // Default value.
if ( defined( 'PHP_OUTPUT_HANDLER_STDFLAGS' ) ) {
$erase_flags = $args['cancelable'] ? PHP_OUTPUT_HANDLER_STDFLAGS : 0; // phpcs:ignore PHPCompatibility.PHP.NewConstants.php_output_handler_stdflagsFound
} else {
$erase_flags = ! $args['cancelable'];
}
ob_start( array( __CLASS__, 'finish_output_buffering' ), $chunk_size, $erase_flags );

// Note that the following must be at 0 because wp_ob_end_flush_all() runs at shutdown:1.
add_action( 'shutdown', array( __CLASS__, 'finish_output_buffering' ), 0 );
/*
* Remove shutdown flushing which will cause PHP notice since buffer is not flushable.
* This was only needed in PHP 5.2 anyway, which this plugin does not support.
*/
remove_action( 'shutdown', 'wp_ob_end_flush_all', 1 );
}

/**
* Finish output buffering.
*
* @since 0.7
* @see AMP_Theme_Support::start_output_buffering()
*
* @param string $response Buffered Response.
* @return string Processed Response.
*/
public static function finish_output_buffering() {

// Flush output buffer stack until we get to the output buffer we started.
while ( ob_get_level() > self::$initial_ob_level ) {
ob_end_flush();
}

echo self::prepare_response( ob_get_clean() ); // WPCS: xss ok.
public static function finish_output_buffering( $response ) {
return self::prepare_response( $response );
}

/**
Expand Down Expand Up @@ -1129,18 +1147,24 @@ public static function prepare_response( $response, $args = array() ) {
}

/*
* Print additional AMP component scripts which have been discovered by the sanitizers, and inject into the head.
* Before printing the AMP scripts, make sure that no plugins will be manipulating the output to be invalid AMP
* since at this point the sanitizers have completed and won't check what is output here.
* Inject additional AMP component scripts which have been discovered by the sanitizers into the head.
* This is adapted from wp_scripts()->do_items(), but it runs only the bare minimum required to output
* the missing scripts, without allowing other filters to apply which may cause an invalid AMP response.
*/
ob_start();
$possible_hooks = array( 'wp_print_scripts', 'print_scripts_array', 'script_loader_src', 'clean_url', 'script_loader_tag', 'attribute_escape' );
foreach ( $possible_hooks as $possible_hook ) {
remove_all_filters( $possible_hook ); // An action and a filter are the same thing.
$script_tags = '';
foreach ( array_diff( array_keys( $amp_scripts ), wp_scripts()->done ) as $handle ) {
if ( ! wp_script_is( $handle, 'registered' ) ) {
continue;
}
$script_dep = wp_scripts()->registered[ $handle ];
$script_tags .= amp_filter_script_loader_tag(
sprintf(
"<script type='text/javascript' src='%s'></script>\n", // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedScript
esc_url( $script_dep->src )
),
$handle
);
}
add_filter( 'script_loader_tag', 'amp_filter_script_loader_tag', PHP_INT_MAX, 2 ); // Make sure this one required filter is present.
wp_scripts()->do_items( array_keys( $amp_scripts ) );
$script_tags = ob_get_clean();
if ( ! empty( $script_tags ) ) {
$response = preg_replace(
'#(?=</head>)#',
Expand Down
92 changes: 75 additions & 17 deletions includes/utils/class-amp-validation-utils.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,14 @@ class AMP_Validation_Utils {
*/
protected static $current_hook_source_stack = array();

/**
* Hook source stack.
*
* @since 0.7
* @var array[]
*/
protected static $hook_source_stack = array();

/**
* Add the actions.
*
Expand Down Expand Up @@ -819,6 +827,7 @@ public static function decorate_shortcode_source( $output, $tag ) {
/**
* Wraps output of a filter to add source stack comments.
*
* @todo Duplicate with AMP_Validation_Utils::wrap_buffer_with_source_comments()?
* @param string $value Value.
* @return string Value wrapped in source comments.
*/
Expand Down Expand Up @@ -925,6 +934,34 @@ public static function get_source( $callback ) {
return $source;
}

/**
* Check whether or not output buffering is currently possible.
*
* This is to guard against a fatal error: "ob_start(): Cannot use output buffering in output buffering display handlers".
*
* @return bool Whether output buffering is allowed.
*/
public static function can_output_buffer() {

// Abort when in shutdown since printing we know to be in buffering display handler, and no output allowed anyway.
if ( did_action( 'shutdown' ) ) {
return false;
}

// Check if any functions in call stack are output buffering display handlers.
$called_functions = array();
if ( defined( 'DEBUG_BACKTRACE_IGNORE_ARGS' ) ) {
$arg = DEBUG_BACKTRACE_IGNORE_ARGS; // phpcs:ignore PHPCompatibility.PHP.NewConstants.debug_backtrace_ignore_argsFound
} else {
$arg = false;
}
$backtrace = debug_backtrace( $arg ); // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_debug_backtrace -- Only way to find out if we are in a buffering display handler.
foreach ( $backtrace as $call_stack ) {
$called_functions[] = '{closure}' === $call_stack['function'] ? 'Closure::__invoke' : $call_stack['function'];
}
return 0 === count( array_intersect( ob_list_handlers(), $called_functions ) );
}

/**
* Wraps a callback in comments if it outputs markup.
*
Expand Down Expand Up @@ -958,20 +995,17 @@ public static function wrapped_callback( $callback ) {
$before_scripts_enqueued = $wp_scripts->queue;
}

/*
* Verifying that the current output buffer type is not PHP_OUTPUT_HANDLER_USER is required to prevent a fatal error:
* "ob_start(): Cannot use output buffering in output buffering display handlers"
*/
$ob_status = ob_get_status();
$can_buffer = ! ( isset( $ob_status['type'] ) && 1 /* PHP_OUTPUT_HANDLER_USER */ === $ob_status['type'] );
$output = null;
if ( $can_buffer ) {
ob_start();
// Wrap the markup output of (action) hooks in source comments.
$has_buffer_started = false;
self::$hook_source_stack[] = $callback['source'];
if ( self::can_output_buffer() ) {
$has_buffer_started = ob_start( array( __CLASS__, 'wrap_buffer_with_source_comments' ) );
}
$result = call_user_func_array( $function, array_slice( $args, 0, intval( $accepted_args ) ) );
if ( $can_buffer ) {
$output = ob_get_clean();
if ( $has_buffer_started ) {
@ob_end_flush(); // phpcs:ignore Generic.PHP.NoSilencedErrors.Discouraged
}
array_pop( self::$hook_source_stack );

// Keep track of which source enqueued the styles.
if ( isset( $wp_styles ) && isset( $wp_styles->queue ) ) {
Expand Down Expand Up @@ -999,16 +1033,40 @@ public static function wrapped_callback( $callback ) {
}
}

// Wrap output that contains HTML tags (as opposed to actions that trigger in HTML attributes).
if ( ! empty( $output ) && preg_match( '/<.+?>/s', $output ) ) {
echo AMP_Validation_Utils::get_source_comment( $callback['source'], true ); // WPCS: XSS ok.
echo $output; // WPCS: XSS ok.
echo AMP_Validation_Utils::get_source_comment( $callback['source'], false ); // WPCS: XSS ok.
}
return $result;
};
}

/**
* Wrap output buffer with source comments.
*
* A key reason for why this is a method and not a closure is so that
* the can_output_buffer method will be able to identify it by name.
*
* @since 0.7
* @todo Is duplicate of \AMP_Validation_Utils::decorate_filter_source()?
*
* @param string $output Output buffer.
* @return string Output buffer conditionally wrapped with source comments.
*/
public static function wrap_buffer_with_source_comments( $output ) {
if ( empty( self::$hook_source_stack ) ) {
return $output;
}

$source = self::$hook_source_stack[ count( self::$hook_source_stack ) - 1 ];

// Wrap output that contains HTML tags (as opposed to actions that trigger in HTML attributes).
if ( ! empty( $output ) && preg_match( '/<.+?>/s', $output ) ) {
$output = implode( '', array(
self::get_source_comment( $source, true ),
$output,
self::get_source_comment( $source, false ),
) );
}
return $output;
}

/**
* Output a removed set, each wrapped in <code></code>.
*
Expand Down
11 changes: 9 additions & 2 deletions tests/test-class-amp-theme-support.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -911,9 +911,14 @@ public function test_finish_output_buffering() {
AMP_Theme_Support::init();
AMP_Theme_Support::finish_init();

$status = ob_get_status();
$this->assertSame( 1, ob_get_level() );
$this->assertEquals( 'default output handler', $status['name'] );

// start first layer buffer.
ob_start();
AMP_Theme_Support::start_output_buffering();
$this->assertSame( 3, ob_get_level() );

echo '<img src="test.png"><script data-test>document.write(\'Illegal\');</script>';

Expand All @@ -925,9 +930,11 @@ public function test_finish_output_buffering() {
} );
echo 'bar';

AMP_Theme_Support::finish_output_buffering();
// get first layer buffer.
while ( ob_get_level() > 2 ) {
ob_end_flush();
}
$output = ob_get_clean();
$this->assertEquals( 1, ob_get_level() );

$this->assertContains( '<html amp', $output );
$this->assertContains( 'foo', $output );
Expand Down

0 comments on commit ff5eaa5

Please sign in to comment.