Skip to content

Commit

Permalink
Merge pull request ampproject#1101 from Automattic/fix/output-buffer-…
Browse files Browse the repository at this point in the history 
…in-display-handlers

Fix fatal errors related to output buffering and excessive response header sizes
  • Loading branch information
@westonruter
westonruter authored Apr 30, 2018
2 parents 6ff75c9 + 3d27037 commit 8426594
Show file tree
Hide file tree
Showing 4 changed files with 191 additions and 108 deletions.
76 changes: 46 additions & 30 deletions includes/class-amp-theme-support.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,12 +77,12 @@ class AMP_Theme_Support {
public static $headers_sent = array();

/**
* Output buffering level when starting.
* Whether output buffering has started.
*
* @since 0.7
* @var int
* @var bool
*/
protected static $initial_ob_level = 0;
protected static $is_output_buffering = false;

/**
* Initialize.
Expand Down Expand Up @@ -261,7 +261,13 @@ public static function add_hooks() {
* Start output buffering at very low priority for sake of plugins and themes that use template_redirect
* instead of template_include.
*/
add_action( 'template_redirect', array( __CLASS__, 'start_output_buffering' ), 0 );
$priority = defined( 'PHP_INT_MIN' ) ? PHP_INT_MIN : ~PHP_INT_MAX; // phpcs:ignore PHPCompatibility.PHP.NewConstants.php_int_minFound
add_action( 'template_redirect', array( __CLASS__, 'start_output_buffering' ), $priority );

// Add validation hooks *after* output buffering has started for the response.
if ( AMP_Validation_Utils::should_validate_response() ) {
AMP_Validation_Utils::add_validation_hooks();
}

// Commenting hooks.
add_filter( 'wp_list_comments_args', array( __CLASS__, 'set_comments_walker' ), PHP_INT_MAX );
Expand All @@ -272,10 +278,6 @@ public static function add_hooks() {
remove_action( 'comment_form', 'wp_comment_form_unfiltered_html_nonce' );
add_filter( 'wp_kses_allowed_html', array( __CLASS__, 'whitelist_layout_in_wp_kses_allowed_html' ), 10 );

if ( AMP_Validation_Utils::should_validate_response() ) {
AMP_Validation_Utils::add_validation_hooks();
}

// @todo Add character conversion.
}

Expand Down Expand Up @@ -976,27 +978,35 @@ public static function start_output_buffering() {
newrelic_disable_autorum();
}

ob_start();
self::$initial_ob_level = ob_get_level();
ob_start( array( __CLASS__, 'finish_output_buffering' ) );
self::$is_output_buffering = true;
}

// 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 );
/**
* Determine whether output buffering has started.
*
* @since 0.7
* @see AMP_Theme_Support::start_output_buffering()
* @see AMP_Theme_Support::finish_output_buffering()
*
* @return bool Whether output buffering has started.
*/
public static function is_output_buffering() {
return self::$is_output_buffering;
}

/**
* 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 ) {
self::$is_output_buffering = false;
return self::prepare_response( $response );
}

/**
Expand Down Expand Up @@ -1129,18 +1139,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
134 changes: 92 additions & 42 deletions includes/utils/class-amp-validation-utils.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,13 +124,6 @@ class AMP_Validation_Utils {
*/
const NONCE_ACTION = 'amp_recheck_';

/**
* HTTP response header name containing JSON-serialized validation errors.
*
* @var string
*/
const VALIDATION_ERRORS_RESPONSE_HEADER_NAME = 'X-AMP-Validation-Errors';

/**
* Transient key to store validation errors when activating a plugin.
*
Expand Down Expand Up @@ -193,6 +186,16 @@ class AMP_Validation_Utils {
*/
protected static $current_hook_source_stack = array();

/**
* Hook source stack.
*
* This has to be public for the sake of PHP 5.3.
*
* @since 0.7
* @var array[]
*/
public static $hook_source_stack = array();

/**
* Add the actions.
*
Expand Down Expand Up @@ -819,6 +822,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 +929,39 @@ 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() {

// Output buffering for validation can only be done while overall output buffering is being done for the response.
if ( ! AMP_Theme_Support::is_output_buffering() ) {
return false;
}

// Abort when in shutdown since output has finished, when we're likely in the overall output buffering display handler.
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,9 +995,17 @@ public static function wrapped_callback( $callback ) {
$before_scripts_enqueued = $wp_scripts->queue;
}

ob_start();
// Wrap the markup output of (action) hooks in source comments.
AMP_Validation_Utils::$hook_source_stack[] = $callback['source'];
$has_buffer_started = false;
if ( AMP_Validation_Utils::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 ) ) );
$output = ob_get_clean();
if ( $has_buffer_started ) {
ob_end_flush();
}
array_pop( AMP_Validation_Utils::$hook_source_stack );

// Keep track of which source enqueued the styles.
if ( isset( $wp_styles ) && isset( $wp_styles->queue ) ) {
Expand Down Expand Up @@ -988,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 Expand Up @@ -1040,39 +1109,27 @@ public static function should_validate_response() {
* Args.
*
* @type bool $remove_source_comments Whether source comments should be removed. Defaults to true.
* @type bool $send_validation_errors_header Whether the X-AMP-Validation-Errors header should be sent. Defaults to true.
* @type bool $append_validation_status_comment Whether the validation errors should be appended as an HTML comment. Defaults to true.
* }
*/
public static function finalize_validation( DOMDocument $dom, $args = array() ) {
$args = array_merge(
array(
'send_validation_errors_header' => true,
'remove_source_comments' => true,
'append_validation_status_comment' => true,
),
$args
);

if ( $args['send_validation_errors_header'] && ! headers_sent() ) {
self::send_validation_errors_header();
}

if ( $args['remove_source_comments'] ) {
self::remove_source_comments( $dom );
}

if ( $args['append_validation_status_comment'] ) {
$report = "\n# Validation Status\n";
$report .= "\n## Summary\n";
$report .= wp_json_encode( self::summarize_validation_errors( self::$validation_errors ), 128 /* JSON_PRETTY_PRINT */ ) . "\n";
$report .= "\n## Details\n";
$report .= wp_json_encode( self::$validation_errors, 128 /* JSON_PRETTY_PRINT */ ) . "\n";
$comment = $dom->createComment( $report );
$body = $dom->getElementsByTagName( 'body' )->item( 0 );
if ( $body ) {
$body->appendChild( $comment );
}
$encoded = wp_json_encode( self::$validation_errors, 128 /* JSON_PRETTY_PRINT */ );
$encoded = str_replace( '--', '\u002d\u002d', $encoded ); // Prevent "--" in strings from breaking out of HTML comments.
$comment = $dom->createComment( 'AMP_VALIDATION_ERRORS:' . $encoded . "\n" );
$dom->documentElement->appendChild( $comment );
}
}

Expand Down Expand Up @@ -1122,13 +1179,6 @@ public static function register_post_type() {
$post_type->cap->create_posts = 'do_not_allow';
}

/**
* Send validation errors back in response header.
*/
public static function send_validation_errors_header() {
header( self::VALIDATION_ERRORS_RESPONSE_HEADER_NAME . ': ' . wp_json_encode( self::$validation_errors ) );
}

/**
* Stores the validation errors.
*
Expand Down Expand Up @@ -1271,11 +1321,11 @@ public static function validate_url( $url ) {
wp_remote_retrieve_response_message( $r )
);
}
$json = wp_remote_retrieve_header( $r, self::VALIDATION_ERRORS_RESPONSE_HEADER_NAME );
if ( ! $json ) {
return new WP_Error( 'response_header_absent' );
$response = wp_remote_retrieve_body( $r );
if ( ! preg_match( '#</body>.*?<!--\s*AMP_VALIDATION_ERRORS\s*:\s*(\[.*?\])\s*-->#s', $response, $matches ) ) {
return new WP_Error( 'response_comment_absent' );
}
$validation_errors = json_decode( $json, true );
$validation_errors = json_decode( $matches[1], true );
if ( ! is_array( $validation_errors ) ) {
return new WP_Error( 'malformed_json_validation_errors' );
}
Expand Down
Loading

0 comments on commit 8426594

Please sign in to comment.