Skip to content

Commit

Permalink
Ensure response cache is varied by validation error sanitization status
Browse files Browse the repository at this point in the history
  • Loading branch information
@westonruter
westonruter committed Jun 1, 2018
1 parent 70b81ae commit e2f7bc7
Show file tree
Hide file tree
Showing 3 changed files with 112 additions and 69 deletions.
37 changes: 29 additions & 8 deletions includes/class-amp-theme-support.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1058,7 +1058,7 @@ public static function prepare_response( $response, $args = array() ) {
'allow_dirty_styles' => self::is_customize_preview_iframe(), // Dirty styles only needed when editing (e.g. for edit shortcodes).
'allow_dirty_scripts' => is_customize_preview(), // Scripts are always needed to inject changeset UUID.
'enable_response_caching' => (
( ! defined( 'WP_DEBUG' ) || true !== WP_DEBUG )
! ( defined( 'WP_DEBUG' ) && WP_DEBUG )
&&
! AMP_Validation_Manager::should_validate_response()
),
Expand All @@ -1080,8 +1080,20 @@ public static function prepare_response( $response, $args = array() ) {

$response_cache = wp_cache_get( $response_cache_key, self::RESPONSE_CACHE_GROUP );

if ( ! empty( $response_cache ) ) {
return $response_cache;
// Make sure that all of the validation errors should be sanitized in the same way; if not, then the cached body should be discarded.
if ( isset( $response_cache['validation_results'] ) ) {
foreach ( $response_cache['validation_results'] as $validation_result ) {
$should_sanitize = AMP_Validation_Manager::is_validation_error_sanitized( $validation_result['error'] );
if ( $should_sanitize !== $validation_result['sanitized'] ) {
unset( $response_cache['body'] );
break;
}
}
}

// Short-circuit response with cached body.
if ( isset( $response_cache['body'] ) ) {
return $response_cache['body'];
}
}

Expand All @@ -1102,14 +1114,13 @@ public static function prepare_response( $response, $args = array() ) {
1
);
}
$dom = AMP_DOM_Utils::get_dom( $response );

$xpath = new DOMXPath( $dom );

$dom = AMP_DOM_Utils::get_dom( $response );
$head = $dom->getElementsByTagName( 'head' )->item( 0 );

// Make sure scripts from the body get moved to the head.
if ( isset( $head ) ) {
// Make sure scripts from the body get moved to the head.
$xpath = new DOMXPath( $dom );
foreach ( $xpath->query( '//body//script[ @custom-element or @custom-template ]' ) as $script ) {
$head->appendChild( $script );
}
Expand Down Expand Up @@ -1208,7 +1219,17 @@ public static function prepare_response( $response, $args = array() ) {

// Cache response if enabled.
if ( true === $args['enable_response_caching'] ) {
wp_cache_set( $response_cache_key, $response, self::RESPONSE_CACHE_GROUP, MONTH_IN_SECONDS );
$response_cache = array(
'body' => $response,
'validation_results' => array_map(
function( $result ) {
unset( $result['error']['sources'] );
return $result;
},
AMP_Validation_Manager::$validation_results
),
);
wp_cache_set( $response_cache_key, $response_cache, self::RESPONSE_CACHE_GROUP, MONTH_IN_SECONDS );
}

return $response;
Expand Down
74 changes: 43 additions & 31 deletions includes/validation/class-amp-validation-manager.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,48 @@ public static function has_cap() {
return current_user_can( 'edit_posts' );
}

/**
* Determine whether a validation error should be sanitized.
*
* @param array $error Validation error.
* @return bool Whether error should be sanitized.
*/
public static function is_validation_error_sanitized( $error ) {
$term_data = AMP_Validation_Error_Taxonomy::prepare_validation_error_taxonomy_term( $error );

$term = get_term_by( 'slug', $term_data['slug'], AMP_Validation_Error_Taxonomy::TAXONOMY_SLUG );
if ( isset( self::$validation_error_status_overrides[ $term_data['slug'] ] ) ) {
$sanitized = AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACCEPTED_STATUS === self::$validation_error_status_overrides[ $term_data['slug'] ];
} elseif ( ! empty( $term ) && AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACCEPTED_STATUS === $term->term_group ) {
$sanitized = true;
} else {
$sanitized = false;
}

/**
* Filters whether the validation error should be sanitized.
*
* Note that the $node is not passed here to ensure that the filter can be
* applied on validation errors that have been stored. Likewise, the $sources
* are also omitted because these are only available during an explicit
* validation request and so they are not suitable for plugins to vary
* sanitization by. Note that returning false this indicates that the
* validation error should not be considered a blocker to render AMP.
*
* @since 1.0
*
* @param bool $sanitized Whether sanitized.
* @param array $context {
* Context data for validation error sanitization.
*
* @type array $error Validation error being sanitized.
* }
*/
$sanitized = apply_filters( 'amp_validation_error_sanitized', $sanitized, compact( 'error' ) );

return $sanitized;
}

/**
* Add validation error.
*
Expand Down Expand Up @@ -421,37 +463,7 @@ public static function add_validation_error( array $error, array $data = array()
*/
$error = apply_filters( 'amp_validation_error', $error, compact( 'node' ) );

$term_data = AMP_Validation_Error_Taxonomy::prepare_validation_error_taxonomy_term( $error );

$term = get_term_by( 'slug', $term_data['slug'], AMP_Validation_Error_Taxonomy::TAXONOMY_SLUG );
if ( isset( self::$validation_error_status_overrides[ $term_data['slug'] ] ) ) {
$sanitized = AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACCEPTED_STATUS === self::$validation_error_status_overrides[ $term_data['slug'] ];
} elseif ( ! empty( $term ) && AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACCEPTED_STATUS === $term->term_group ) {
$sanitized = true;
} else {
$sanitized = false;
}

/**
* Filters whether the validation error should be sanitized.
*
* Note that the $node is not passed here to ensure that the filter can be
* applied on validation errors that have been stored. Likewise, the $sources
* are also omitted because these are only available during an explicit
* validation request and so they are not suitable for plugins to vary
* sanitization by. Note that returning false this indicates that the
* validation error should not be considered a blocker to render AMP.
*
* @since 1.0
*
* @param bool $sanitized Whether sanitized.
* @param array $context {
* Context data for validation error sanitization.
*
* @type array $error Validation error being sanitized.
* }
*/
$sanitized = apply_filters( 'amp_validation_error_sanitized', $sanitized, compact( 'error' ) );
$sanitized = self::is_validation_error_sanitized( $error );

// Add sources back into the $error for referencing later. @todo It may be cleaner to store sources separately to avoid having to re-remove later during storage.
$error = array_merge( $error, compact( 'sources' ) );
Expand Down
70 changes: 40 additions & 30 deletions tests/test-class-amp-theme-support.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1009,7 +1009,7 @@ public function test_prepare_response() {
ob_start();
?>
<!DOCTYPE html>
<html amp <?php language_attributes(); ?>>
<html amp>
<head>
<?php wp_head(); ?>
<script data-head>document.write('Illegal');</script>
Expand Down Expand Up @@ -1077,43 +1077,53 @@ public function test_prepare_response() {
$removed_nodes
);

$call_response = function() use ( $original_html ) {
return AMP_Theme_Support::prepare_response( $original_html, array(
'enable_response_caching' => true,
$prepare_response_args = array(
'enable_response_caching' => true,
);

$call_prepare_response = function() use ( $original_html, &$prepare_response_args ) {
AMP_Response_Headers::$headers_sent = array();
AMP_Validation_Manager::$validation_results = array();
return AMP_Theme_Support::prepare_response( $original_html, $prepare_response_args );
};

$get_server_timing_header_count = function() {
return count( array_filter(
AMP_Response_Headers::$headers_sent,
function( $header ) {
return 'Server-Timing' === $header['name'];
}
) );
};

// Test that first response isn't cached.
$first_response = $call_response();
$this->assertGreaterThan( 0, count( array_filter(
AMP_Response_Headers::$headers_sent,
function( $header ) {
return 'Server-Timing' === $header['name'];
}
) ) );
$first_response = $call_prepare_response();
$this->assertGreaterThan( 0, $get_server_timing_header_count() );
$this->assertContains( '<html amp>', $first_response ); // Note: AMP because sanitized validation errors.

// Test that response cache is return upon second call.
AMP_Response_Headers::$headers_sent = array();
$this->assertEquals( $first_response, $call_response() );
$this->assertSame( 0, count( array_filter(
AMP_Response_Headers::$headers_sent,
function( $header ) {
return 'Server-Timing' === $header['name'];
}
) ) );
$this->assertEquals( $first_response, $call_prepare_response() );
$this->assertSame( 0, $get_server_timing_header_count() );

// Test new cache upon argument change.
AMP_Response_Headers::$headers_sent = array();
AMP_Theme_Support::prepare_response( $original_html, array(
'enable_response_caching' => true,
'test_reset_by_arg' => true,
) );
$this->assertGreaterThan( 0, count( array_filter(
AMP_Response_Headers::$headers_sent,
function( $header ) {
return 'Server-Timing' === $header['name'];
}
) ) );
$prepare_response_args['test_reset_by_arg'] = true;
$call_prepare_response();
$this->assertGreaterThan( 0, $get_server_timing_header_count() );

// Test response is cached.
$call_prepare_response();
$this->assertSame( 0, $get_server_timing_header_count() );

// Test that response is no longer cached due to a change whether validation errors are sanitized.
remove_filter( 'amp_validation_error_sanitized', '__return_true' );
add_filter( 'amp_validation_error_sanitized', '__return_false' );
$prepared_html = $call_prepare_response();
$this->assertGreaterThan( 0, $get_server_timing_header_count() );
$this->assertContains( '<html>', $prepared_html ); // Note: no AMP because unsanitized validation error.

// And test response is cached.
$call_prepare_response();
$this->assertSame( 0, $get_server_timing_header_count() );
}

/**
Expand Down

0 comments on commit e2f7bc7

Please sign in to comment.