From 8778ecf1278e319f0366b2ead9a8fa8e9c497b58 Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Fri, 16 Feb 2018 22:20:04 -0600 Subject: [PATCH 01/93] Issue #842: Begin to validate plugins. If a . This uses the 'src' attribute to find the plugin or theme. --- .../sanitizers/class-amp-base-sanitizer.php | 2 +- .../sanitizers/class-amp-style-sanitizer.php | 3 + includes/utils/class-amp-validation-utils.php | 110 ++++++++++++- tests/test-class-amp-validation-utils.php | 152 +++++++++++++++--- 4 files changed, 244 insertions(+), 23 deletions(-) diff --git a/includes/sanitizers/class-amp-base-sanitizer.php b/includes/sanitizers/class-amp-base-sanitizer.php index a0331d019e7..38c0a167e10 100644 --- a/includes/sanitizers/class-amp-base-sanitizer.php +++ b/includes/sanitizers/class-amp-base-sanitizer.php @@ -342,7 +342,7 @@ public function remove_invalid_attribute( $element, $attribute ) { } if ( $attribute ) { $element->removeAttributeNode( $attribute ); - call_user_func( $this->args['remove_invalid_callback'], $attribute ); + call_user_func( $this->args['remove_invalid_callback'], $attribute, $element ); } } elseif ( is_string( $attribute ) ) { $element->removeAttribute( $attribute ); diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 92e9831d454..203894dc5fe 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -239,6 +239,9 @@ public function sanitize() { $this->dom->createComment( sprintf( 'Skipped including %s stylesheet since too large.', $skip ) ), $this->amp_custom_style_element->nextSibling ); + if ( isset( $this->args['remove_style_callback'] ) ) { + call_user_func( $this->args['remove_style_callback'], $skip ); + } } } } diff --git a/includes/utils/class-amp-validation-utils.php b/includes/utils/class-amp-validation-utils.php index 6b79504e7f8..b070ed02188 100644 --- a/includes/utils/class-amp-validation-utils.php +++ b/includes/utils/class-amp-validation-utils.php @@ -15,17 +15,24 @@ class AMP_Validation_Utils { /** * Key for the markup value in the REST API endpoint. * - * @var string. + * @var string */ const MARKUP_KEY = 'markup'; /** * Key for the error value in the response. * - * @var string. + * @var string */ const ERROR_KEY = 'has_error'; + /** + * Class of the style sanitizer. + * + * @var string + */ + const STYLE_SANITIZER = 'AMP_Style_Sanitizer'; + /** * The nodes that the sanitizer removed. * @@ -33,6 +40,13 @@ class AMP_Validation_Utils { */ public static $removed_nodes = array(); + /** + * The removed assets. + * + * @var array + */ + public static $removed_assets = array(); + /** * Add the actions. * @@ -46,11 +60,84 @@ public static function init() { /** * Tracks when a sanitizer removes an node (element or attribute). * - * @param DOMNode $node The node which was removed. + * @param DOMNode $node The node which was removed. + * @param DOMNode $parent_element The parent element of a removed attribute (optional). * @return void */ - public static function track_removed( $node ) { + public static function track_removed( $node, $parent_element = null ) { self::$removed_nodes[] = $node; + self::removed_script( $node, $parent_element ); + } + + /** + * Tracks when a script was removed. + * + * The $element argument is needed for a removed 'src' attribute. + * The attribute node has no information about that $element. + * If its parent $element is a '; // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedScript + /** + * The name of a tag that the sanitizer should strip. + * + * @var string + */ + public $disallowed_tag_name = 'script'; + + /** + * The name of an attribute that the sanitizer should strip. + * + * @var string + */ + public $disallowed_attribute_name = 'onload'; + + /** + * A mock plugin name that outputs invalid markup. + * + * @var string + */ + public $plugin_name = 'foo-bar'; + /** * A valid image that sanitizers should not alter. * @@ -607,9 +628,9 @@ public function test_register_post_type() { $this->assertEquals( AMP_Validation_Utils::POST_TYPE_SLUG, $amp_post_type->name ); $this->assertEquals( 'AMP Validation Errors', $amp_post_type->label ); $this->assertEquals( false, $amp_post_type->public ); - $this->assertFalse( $amp_post_type->show_ui ); - $this->assertFalse( $amp_post_type->show_in_menu ); - $this->assertFalse( $amp_post_type->show_in_admin_bar ); + $this->assertTrue( $amp_post_type->show_ui ); + $this->assertEquals( AMP_Options_Manager::OPTION_NAME, $amp_post_type->show_in_menu ); + $this->assertTrue( $amp_post_type->show_in_admin_bar ); } /** @@ -743,17 +764,113 @@ public function test_plugin_notice() { $this->assertEmpty( $output ); $pagenow = 'plugins.php'; // WPCS: global override ok. $_GET['activate'] = 'true'; - $plugin = 'foo-plugin'; + + $this->create_custom_post(); + ob_start(); + AMP_Validation_Utils::plugin_notice(); + $output = ob_get_clean(); + $this->assertContains( 'Warning: the following plugins are incompatible with AMP', $output ); + $this->assertContains( $this->plugin_name, $output ); + } + + /** + * Test for add_post_columns() + * + * @see AMP_Validation_Utils::add_post_columns() + */ + public function test_add_post_columns() { + $initial_columns = array( + 'cb' => '', + ); + $this->assertEquals( + array_merge( + $initial_columns, + array( + 'url' => 'URL', + AMP_Validation_Utils::REMOVED_ELEMENTS => 'Removed Elements', + AMP_Validation_Utils::REMOVED_ATTRIBUTES => 'Removed Attributes', + AMP_Validation_Utils::SOURCES_INVALID_OUTPUT => 'Incompatible Source', + ) + ), + AMP_Validation_Utils::add_post_columns( $initial_columns ) + ); + } + + /** + * Test for output_custom_column() + * + * @dataProvider get_custom_columns + * @see AMP_Validation_Utils::output_custom_column() + * @param string $column_name The name of the column. + * @param string $expected_value The value that is expected to be present in the column markup. + */ + public function test_output_custom_column( $column_name, $expected_value ) { + ob_start(); + AMP_Validation_Utils::output_custom_column( $column_name, $this->create_custom_post() ); + $this->assertContains( $expected_value, ob_get_clean() ); + } + + /** + * Gets the test data for test_output_custom_column(). + * + * @return array $columns + */ + public function get_custom_columns() { + return array( + 'url' => array( + 'url', + get_home_url(), + ), + 'removed_elements' => array( + AMP_Validation_Utils::REMOVED_ELEMENTS, + $this->disallowed_tag_name, + ), + 'removed_attributes' => array( + AMP_Validation_Utils::REMOVED_ATTRIBUTES, + $this->disallowed_attribute_name, + ), + 'sources_invalid_input' => array( + AMP_Validation_Utils::SOURCES_INVALID_OUTPUT, + $this->plugin_name, + ), + ); + } + + /** + * Test for add_recheck() + * + * @see AMP_Validation_Utils::add_recheck() + */ + public function test_add_recheck() { + $initial_actions = array( + 'trash' => 'Trash', + ); + $post = $this->factory()->post->create_and_get(); + $this->assertEquals( $initial_actions, AMP_Validation_Utils::add_recheck( $initial_actions, $post ) ); + + $custom_post_id = $this->create_custom_post(); + $actions = AMP_Validation_Utils::add_recheck( $initial_actions, get_post( $custom_post_id ) ); + $url = get_post_meta( $custom_post_id, AMP_Validation_Utils::AMP_URL_META, true ); + $this->assertContains( $url, $actions['recheck'] ); + $this->assertEquals( $initial_actions['trash'], $actions['trash'] ); + } + + /** + * Creates and inserts a custom post. + * + * @return int|WP_Error $error_post The ID of new custom post, or an error. + */ + public function create_custom_post() { $response = array( AMP_Validation_Utils::ERROR_KEY => true, - 'removed_elements' => array( - $this->disallowed_tag => 1, + AMP_Validation_Utils::REMOVED_ELEMENTS => array( + $this->disallowed_tag_name => 1, ), - 'removed_attributes' => array( - 'onload' => 1, + AMP_Validation_Utils::REMOVED_ATTRIBUTES => array( + $this->disallowed_attribute_name => 1, ), AMP_Validation_Utils::SOURCES_INVALID_OUTPUT => array( - 'plugin' => array( $plugin ), + 'plugin' => array( $this->plugin_name ), ), ); $content = wp_json_encode( $response ); @@ -764,15 +881,10 @@ public function test_plugin_notice() { 'post_content' => $content, 'post_status' => 'publish', ); - $error_post = wp_insert_post( wp_slash( $post_args ) ); - $url = home_url(); - add_post_meta( $error_post, AMP_Validation_Utils::AMP_URL_META, $url ); - - ob_start(); - AMP_Validation_Utils::plugin_notice(); - $output = ob_get_clean(); - $this->assertContains( 'Warning: the following plugins are incompatible with AMP', $output ); - $this->assertContains( $plugin, $output ); + $error_post = wp_insert_post( $post_args ); + $url = get_home_url(); + update_post_meta( $error_post, AMP_Validation_Utils::AMP_URL_META, $url ); + return $error_post; } } From 0728a1e0c054d9e183d44c49d470111781ad3e2d Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Thu, 1 Mar 2018 00:58:52 -0600 Subject: [PATCH 34/93] Issue #842: Correct Travis issue and use constant. Correct an issue in improper vertical alignment. And use constants in place of string literals. --- includes/utils/class-amp-validation-utils.php | 8 +-- tests/test-class-amp-validation-utils.php | 52 +++++++++---------- 2 files changed, 30 insertions(+), 30 deletions(-) diff --git a/includes/utils/class-amp-validation-utils.php b/includes/utils/class-amp-validation-utils.php index 916692398cc..6079167b5c1 100644 --- a/includes/utils/class-amp-validation-utils.php +++ b/includes/utils/class-amp-validation-utils.php @@ -485,16 +485,16 @@ public static function display_error( $response ) { echo '
'; printf( '

%s

', esc_html__( 'Warning: There is content which fails AMP validation; it will be stripped when served as AMP.', 'amp' ) ); $removed_sets = array(); - if ( ! empty( $response['removed_elements'] ) && is_array( $response['removed_elements'] ) ) { + if ( ! empty( $response[ self::REMOVED_ELEMENTS ] ) && is_array( $response[ self::REMOVED_ELEMENTS ] ) ) { $removed_sets[] = array( 'label' => __( 'Invalid elements:', 'amp' ), - 'names' => array_map( 'sanitize_key', $response['removed_elements'] ), + 'names' => array_map( 'sanitize_key', $response[ self::REMOVED_ELEMENTS ] ), ); } - if ( ! empty( $response['removed_attributes'] ) && is_array( $response['removed_attributes'] ) ) { + if ( ! empty( $response[ self::REMOVED_ATTRIBUTES ] ) && is_array( $response[ self::REMOVED_ATTRIBUTES ] ) ) { $removed_sets[] = array( 'label' => __( 'Invalid attributes:', 'amp' ), - 'names' => array_map( 'sanitize_key', $response['removed_attributes'] ), + 'names' => array_map( 'sanitize_key', $response[ self::REMOVED_ATTRIBUTES ] ), ); } foreach ( $removed_sets as $removed_set ) { diff --git a/tests/test-class-amp-validation-utils.php b/tests/test-class-amp-validation-utils.php index 0278cc6a784..c51196d039e 100644 --- a/tests/test-class-amp-validation-utils.php +++ b/tests/test-class-amp-validation-utils.php @@ -236,13 +236,13 @@ public function test_validate_markup() { ) ) ); $response = AMP_Validation_Utils::validate_markup( $request ); $expected_response = array( - AMP_Validation_Utils::ERROR_KEY => true, - 'removed_elements' => array( + AMP_Validation_Utils::ERROR_KEY => true, + AMP_Validation_Utils::REMOVED_ELEMENTS => array( 'script' => 1, ), - 'removed_attributes' => array(), - 'processed_markup' => $markup, - 'sources_with_invalid_output' => array(), + AMP_Validation_Utils::REMOVED_ATTRIBUTES => array(), + 'processed_markup' => $markup, + 'sources_with_invalid_output' => array(), ); $this->assertEquals( $expected_response, $response ); @@ -251,11 +251,11 @@ public function test_validate_markup() { ) ) ); $response = AMP_Validation_Utils::validate_markup( $request ); $expected_response = array( - AMP_Validation_Utils::ERROR_KEY => false, - 'removed_elements' => array(), - 'removed_attributes' => array(), - 'processed_markup' => $this->valid_amp_img, - 'sources_with_invalid_output' => array(), + AMP_Validation_Utils::ERROR_KEY => false, + AMP_Validation_Utils::REMOVED_ELEMENTS => array(), + AMP_Validation_Utils::REMOVED_ATTRIBUTES => array(), + 'processed_markup' => $this->valid_amp_img, + 'sources_with_invalid_output' => array(), ); $this->assertEquals( $expected_response, $response ); } @@ -271,13 +271,13 @@ public function test_get_response() { $this->set_capability(); $response = AMP_Validation_Utils::get_response( $this->disallowed_tag ); $expected_response = array( - AMP_Validation_Utils::ERROR_KEY => true, - 'removed_elements' => array( + AMP_Validation_Utils::ERROR_KEY => true, + AMP_Validation_Utils::REMOVED_ELEMENTS => array( 'script' => 1, ), - 'removed_attributes' => array(), - 'processed_markup' => $this->disallowed_tag, - 'sources_with_invalid_output' => array(), + AMP_Validation_Utils::REMOVED_ATTRIBUTES => array(), + 'processed_markup' => $this->disallowed_tag, + 'sources_with_invalid_output' => array(), ); $this->assertEquals( $expected_response, $response ); } @@ -503,11 +503,11 @@ public function test_display_error() { $removed_element = 'script'; $removed_attribute = 'onload'; $response = array( - AMP_Validation_Utils::ERROR_KEY => true, - 'removed_elements' => array( + AMP_Validation_Utils::ERROR_KEY => true, + AMP_Validation_Utils::REMOVED_ELEMENTS => array( $removed_element => 1, ), - 'removed_attributes' => array( + AMP_Validation_Utils::REMOVED_ATTRIBUTES => array( $removed_attribute => 1, ), ); @@ -666,9 +666,9 @@ public function test_store_validation_errors() { // This should create a new post for the errors. $this->assertEquals( AMP_Validation_Utils::POST_TYPE_SLUG, $custom_post->post_type ); - $this->assertEquals( $expected_removed_elements, $validation['removed_elements'] ); + $this->assertEquals( $expected_removed_elements, $validation[ AMP_Validation_Utils::REMOVED_ELEMENTS ] ); $this->assertEquals( true, $validation[ AMP_Validation_Utils::ERROR_KEY ] ); - $this->assertEquals( array(), $validation['removed_attributes'] ); + $this->assertEquals( array(), $validation[ AMP_Validation_Utils::REMOVED_ATTRIBUTES ] ); $this->assertEquals( array( 'foo' ), $validation[ AMP_Validation_Utils::SOURCES_INVALID_OUTPUT ]['plugin'] ); $meta = get_post_meta( $post_id, AMP_Validation_Utils::AMP_URL_META, true ); $this->assertEquals( $url, $meta ); @@ -707,7 +707,7 @@ public function test_store_validation_errors() { ); // A post already exists for this URL, so it should be updated. - $this->assertEquals( $expected_removed_elements, $validation['removed_elements'] ); + $this->assertEquals( $expected_removed_elements, $validation[ AMP_Validation_Utils::REMOVED_ELEMENTS ] ); $this->assertTrue( $validation[ AMP_Validation_Utils::ERROR_KEY ] ); $this->assertEquals( array( 'foo' ), $validation[ AMP_Validation_Utils::SOURCES_INVALID_OUTPUT ]['plugin'] ); $this->assertEquals( $expected_url, $url ); @@ -821,7 +821,7 @@ public function get_custom_columns() { 'url', get_home_url(), ), - 'removed_elements' => array( + 'removed_element' => array( AMP_Validation_Utils::REMOVED_ELEMENTS, $this->disallowed_tag_name, ), @@ -861,7 +861,7 @@ public function test_add_recheck() { * @return int|WP_Error $error_post The ID of new custom post, or an error. */ public function create_custom_post() { - $response = array( + $response = array( AMP_Validation_Utils::ERROR_KEY => true, AMP_Validation_Utils::REMOVED_ELEMENTS => array( $this->disallowed_tag_name => 1, @@ -873,9 +873,9 @@ public function create_custom_post() { 'plugin' => array( $this->plugin_name ), ), ); - $content = wp_json_encode( $response ); - $encoded_errors = md5( $content ); - $post_args = array( + $content = wp_json_encode( $response ); + $encoded_errors = md5( $content ); + $post_args = array( 'post_type' => AMP_Validation_Utils::POST_TYPE_SLUG, 'post_name' => $encoded_errors, 'post_content' => $content, From 2228520f40634ffefeab73593714c4494adf5c61 Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Thu, 1 Mar 2018 01:11:19 -0600 Subject: [PATCH 35/93] Issue #842: Improve post actions, prevent muliple plugins. Only remove some items from post actions. Keep the rest of the actions in place. Like 'Restore' when in the trash. Also, prevent displaying multiple plugins on plugins.php. --- includes/utils/class-amp-validation-utils.php | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/includes/utils/class-amp-validation-utils.php b/includes/utils/class-amp-validation-utils.php index 6079167b5c1..e360e657898 100644 --- a/includes/utils/class-amp-validation-utils.php +++ b/includes/utils/class-amp-validation-utils.php @@ -695,7 +695,7 @@ public static function plugin_notice() { return; } $errors = json_decode( $error_post->post_content, true ); - $invalid_plugins = isset( $errors[ self::SOURCES_INVALID_OUTPUT ]['plugin'] ) ? $errors[ self::SOURCES_INVALID_OUTPUT ]['plugin'] : null; + $invalid_plugins = isset( $errors[ self::SOURCES_INVALID_OUTPUT ]['plugin'] ) ? array_unique( $errors[ self::SOURCES_INVALID_OUTPUT ]['plugin'] ) : null; if ( isset( $invalid_plugins ) ) { echo '

'; $reported_plugins = array(); @@ -781,17 +781,14 @@ public static function add_recheck( $actions, $post ) { if ( self::POST_TYPE_SLUG !== $post->post_type ) { return $actions; } - $trash = isset( $actions['trash'] ) ? $actions['trash'] : null; - $url = get_post_meta( $post->ID, self::AMP_URL_META, true ); - $actions = array(); + unset( $actions['edit'] ); + unset( $actions['inline hide-if-no-js'] ); + $url = get_post_meta( $post->ID, self::AMP_URL_META, true ); // @todo: $url needs to recheck the AMP validation of the page, and reload the edit.php page. if ( ! empty( $url ) ) { $actions['recheck'] = sprintf( '%s', esc_url( $url ), esc_html__( 'Re-check', 'amp' ) ); } - if ( isset( $trash ) ) { - $actions['trash'] = $trash; - } return $actions; } From c2664ca9a746c0d0ad8c99d003934287dc00de1f Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Thu, 1 Mar 2018 01:40:20 -0600 Subject: [PATCH 36/93] Issue #842: Add a bulk action 're-check' linkon edit.php. This displays on the 'AMP Validation Errors' page. @todo: implement this bulk action on the back-end. --- includes/utils/class-amp-validation-utils.php | 17 +++++++++++++++-- tests/test-class-amp-validation-utils.php | 18 ++++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/includes/utils/class-amp-validation-utils.php b/includes/utils/class-amp-validation-utils.php index e360e657898..03fbeaf9f82 100644 --- a/includes/utils/class-amp-validation-utils.php +++ b/includes/utils/class-amp-validation-utils.php @@ -116,9 +116,10 @@ public static function init() { } } ); add_action( 'all_admin_notices', array( __CLASS__, 'plugin_notice' ) ); - add_action( 'manage_' . self::POST_TYPE_SLUG . '_posts_columns', array( __CLASS__, 'add_post_columns' ) ); + add_filter( 'manage_' . self::POST_TYPE_SLUG . '_posts_columns', array( __CLASS__, 'add_post_columns' ) ); add_action( 'manage_posts_custom_column', array( __CLASS__, 'output_custom_column' ), 10, 2 ); - add_action( 'post_row_actions', array( __CLASS__, 'add_recheck' ), 10, 2 ); + add_filter( 'post_row_actions', array( __CLASS__, 'add_recheck' ), 10, 2 ); + add_filter( 'bulk_actions-edit-' . self::POST_TYPE_SLUG, array( __CLASS__, 'add_bulk_action' ), 10, 2 ); } /** @@ -792,4 +793,16 @@ public static function add_recheck( $actions, $post ) { return $actions; } + /** + * Adds a 'Re-check' bulk action to the edit.php page. + * + * @param array $actions The bulk actions in the edit.php page. + * @return array $actions The filtered bulk actions. + */ + public static function add_bulk_action( $actions ) { + unset( $actions['edit'] ); + $actions['recheck'] = esc_html__( 'Re-check', 'amp' ); + return $actions; + } + } diff --git a/tests/test-class-amp-validation-utils.php b/tests/test-class-amp-validation-utils.php index c51196d039e..e74586e39ce 100644 --- a/tests/test-class-amp-validation-utils.php +++ b/tests/test-class-amp-validation-utils.php @@ -102,6 +102,10 @@ public function test_init() { $this->assertEquals( 10, has_filter( 'amp_content_sanitizers', self::TESTED_CLASS . '::add_validation_callback' ) ); $this->assertEquals( 10, has_action( 'init', self::TESTED_CLASS . '::register_post_type' ) ); $this->assertEquals( 10, has_action( 'all_admin_notices', self::TESTED_CLASS . '::plugin_notice' ) ); + $this->assertEquals( 10, has_filter( 'manage_' . AMP_Validation_Utils::POST_TYPE_SLUG . '_posts_columns', self::TESTED_CLASS . '::add_post_columns' ) ); + $this->assertEquals( 10, has_action( 'manage_posts_custom_column', self::TESTED_CLASS . '::output_custom_column' ) ); + $this->assertEquals( 10, has_filter( 'post_row_actions', self::TESTED_CLASS . '::add_recheck' ) ); + $this->assertEquals( 10, has_filter( 'bulk_actions-edit-' . AMP_Validation_Utils::POST_TYPE_SLUG, self::TESTED_CLASS . '::add_bulk_action' ) ); } /** @@ -855,6 +859,20 @@ public function test_add_recheck() { $this->assertEquals( $initial_actions['trash'], $actions['trash'] ); } + /** + * Test for add_bulk_action() + * + * @see AMP_Validation_Utils::add_bulk_action() + */ + public function test_add_bulk_action() { + $initial_action = array( + 'edit' => 'Edit', + ); + $actions = AMP_Validation_Utils::add_bulk_action( $initial_action ); + $this->assertFalse( isset( $action['edit'] ) ); + $this->assertEquals( 'Re-check', $actions['recheck'] ); + } + /** * Creates and inserts a custom post. * From d2b033cd6d9d3643b09a9d697a7248e2be69f93c Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Thu, 1 Mar 2018 01:50:04 -0600 Subject: [PATCH 37/93] Issue #842: Prevent showing the same source multiple times. On the edit.php page, there's a column for 'Incompatible Source'. The way the sources are stored enables duplicated. So use array_unique to prevent displaying more than one. --- includes/utils/class-amp-validation-utils.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/includes/utils/class-amp-validation-utils.php b/includes/utils/class-amp-validation-utils.php index 03fbeaf9f82..27cf8e77865 100644 --- a/includes/utils/class-amp-validation-utils.php +++ b/includes/utils/class-amp-validation-utils.php @@ -761,7 +761,7 @@ public static function output_custom_column( $column_name, $post_id ) { if ( isset( $errors[ self::SOURCES_INVALID_OUTPUT ] ) ) { $sources = array(); foreach ( $errors[ self::SOURCES_INVALID_OUTPUT ] as $type => $names ) { - foreach ( $names as $name ) { + foreach ( array_unique( $names ) as $name ) { $sources[] = sprintf( '%s: %s', esc_html( $type ), esc_html( $name ) ); } } From 816b77d2084ff0410f5496a8b9010dc0caffbde1 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 1 Mar 2018 14:22:12 -0800 Subject: [PATCH 38/93] Add validation reporting of invalid shortcodes --- includes/utils/class-amp-validation-utils.php | 57 +++++++++++++++---- 1 file changed, 45 insertions(+), 12 deletions(-) diff --git a/includes/utils/class-amp-validation-utils.php b/includes/utils/class-amp-validation-utils.php index 27cf8e77865..c1dbf960b5c 100644 --- a/includes/utils/class-amp-validation-utils.php +++ b/includes/utils/class-amp-validation-utils.php @@ -107,19 +107,23 @@ class AMP_Validation_Utils { public static function init() { add_action( 'rest_api_init', array( __CLASS__, 'amp_rest_validation' ) ); add_action( 'edit_form_top', array( __CLASS__, 'validate_content' ), 10, 2 ); + add_action( 'init', array( __CLASS__, 'register_post_type' ) ); + add_action( 'all_admin_notices', array( __CLASS__, 'plugin_notice' ) ); + add_filter( 'manage_' . self::POST_TYPE_SLUG . '_posts_columns', array( __CLASS__, 'add_post_columns' ) ); + add_action( 'manage_posts_custom_column', array( __CLASS__, 'output_custom_column' ), 10, 2 ); + add_filter( 'post_row_actions', array( __CLASS__, 'add_recheck' ), 10, 2 ); + add_filter( 'bulk_actions-edit-' . self::POST_TYPE_SLUG, array( __CLASS__, 'add_bulk_action' ), 10, 2 ); + + // @todo There is more than just node removal that needs to be reported. There is also script enqueues, external stylesheets, cdata length, etc. + // Actions and filters involved in validation. add_action( 'wp', array( __CLASS__, 'callback_wrappers' ) ); + add_filter( 'do_shortcode_tag', array( __CLASS__, 'decorate_shortcode_source' ), -1, 2 ); add_filter( 'amp_content_sanitizers', array( __CLASS__, 'add_validation_callback' ) ); - add_action( 'init', array( __CLASS__, 'register_post_type' ) ); add_action( 'activate_plugin', function() { if ( ! has_action( 'shutdown', array( __CLASS__, 'validate_home' ) ) ) { add_action( 'shutdown', array( __CLASS__, 'validate_home' ) ); // Shutdown so all plugins will have been activated. } } ); - add_action( 'all_admin_notices', array( __CLASS__, 'plugin_notice' ) ); - add_filter( 'manage_' . self::POST_TYPE_SLUG . '_posts_columns', array( __CLASS__, 'add_post_columns' ) ); - add_action( 'manage_posts_custom_column', array( __CLASS__, 'output_custom_column' ), 10, 2 ); - add_filter( 'post_row_actions', array( __CLASS__, 'add_recheck' ), 10, 2 ); - add_filter( 'bulk_actions-edit-' . self::POST_TYPE_SLUG, array( __CLASS__, 'add_bulk_action' ), 10, 2 ); } /** @@ -335,19 +339,18 @@ public static function validate_content( $post ) { } /** - * Wraps callbacks in comments, to indicate to the sanitizer which plugin added them. + * Wraps callbacks in comments to indicate to the sanitizer which extension added them. * - * Iterates through all of the registered callbacks. - * If a callback is from a plugin and outputs markup, - * this wraps the markup in comments. - * Later, the sanitizer can identify which plugin any illegal markup is from. + * Iterates through all of the registered callbacks for actions and filters. + * If a callback is from a plugin and outputs markup, this wraps the markup in comments. + * Later, the sanitizer can identify which theme or plugin the illegal markup is from. * * @global array $wp_filter * @return void */ public static function callback_wrappers() { global $wp_filter; - if ( ! self::should_validate_front_end() ) { + if ( ! self::should_validate_front_end() ) { // @todo Better to just conditionally call callback_wrappers() based on whether should_validate_front_end(). Active request rather than passive. return; } $pending_wrap_callbacks = array(); @@ -376,6 +379,36 @@ public static function callback_wrappers() { } } + /** + * Filters the output created by a shortcode callback. + * + * @since 0.7 + * + * @param string $output Shortcode output. + * @param string $tag Shortcode name. + * @return string Output. + * @global array $shortcode_tags + */ + public static function decorate_shortcode_source( $output, $tag ) { + global $shortcode_tags; + if ( ! self::should_validate_front_end() ) { // @todo Better to just conditionally call callback_wrappers() based on whether should_validate_front_end(). Active request rather than passive. + return $output; + } + if ( ! isset( $shortcode_tags[ $tag ] ) ) { + return $output; + } + $source = self::get_source( $shortcode_tags[ $tag ] ); + if ( empty( $source ) ) { + return $output; + } + $output = implode( '', array( + sprintf( '', esc_attr( $source['type'] ), esc_attr( $source['name'] ) ), // @todo Need to also include the offending shortcode $tag. + $output, + sprintf( '', esc_attr( $source['type'] ), esc_attr( $source['name'] ) ), + ) ); + return $output; + } + /** * Gets the plugin or theme of the callback, if one exists. * From 2715cf1f093c869a4135d441e9c2e702bb0a2bbe Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Thu, 1 Mar 2018 23:50:37 -0600 Subject: [PATCH 39/93] Implement 'Re-check' bulk action. Add a handler for this: handle_bulk_action(). Validate each of the URLs in the action. And check if errors remain. Display a message, stating whether there are still errors in the rechecked URLs. @todo: implement inline 'Re-check' action. --- includes/utils/class-amp-validation-utils.php | 106 ++++++++++++++++-- tests/test-class-amp-validation-utils.php | 78 +++++++++++-- 2 files changed, 170 insertions(+), 14 deletions(-) diff --git a/includes/utils/class-amp-validation-utils.php b/includes/utils/class-amp-validation-utils.php index c1dbf960b5c..30015773eb7 100644 --- a/includes/utils/class-amp-validation-utils.php +++ b/includes/utils/class-amp-validation-utils.php @@ -89,6 +89,27 @@ class AMP_Validation_Utils { */ const REMOVED_SOURCES = 'removed_sources'; + /** + * The action to recheck URLs for AMP validity. + * + * @var string + */ + const RECHECK_ACTION = 'amp-recheck'; + + /** + * The query arg for whether there are remaining errors after rechecking URLs. + * + * @var string + */ + const REMAINING_ERRORS = 'amp_remaining_errors'; + + /** + * The query value for if there are remaining errors. + * + * @var string + */ + const REMAINING_ERROR_VALUE = '1'; + /** * The nodes that the sanitizer removed. * @@ -113,6 +134,8 @@ public static function init() { add_action( 'manage_posts_custom_column', array( __CLASS__, 'output_custom_column' ), 10, 2 ); add_filter( 'post_row_actions', array( __CLASS__, 'add_recheck' ), 10, 2 ); add_filter( 'bulk_actions-edit-' . self::POST_TYPE_SLUG, array( __CLASS__, 'add_bulk_action' ), 10, 2 ); + add_filter( 'handle_bulk_actions-edit-' . self::POST_TYPE_SLUG, array( __CLASS__, 'handle_bulk_action' ), 10, 3 ); + add_action( 'admin_notices', array( __CLASS__, 'remaining_error_notice' ) ); // @todo There is more than just node removal that needs to be reported. There is also script enqueues, external stylesheets, cdata length, etc. // Actions and filters involved in validation. @@ -696,17 +719,27 @@ public static function existing_post( $url ) { } /** - * Validate the home page. + * Validates the home page. * - * @return WP_Error|array The response array, or WP_Error. + * @return array|WP_Error The response array, or WP_Error. */ public static function validate_home() { - $url = add_query_arg( + return self::validate_url( home_url() ); + } + + /** + * Validates a given URL, and stores its errors in a custom post. + * + * @param string $url The URL to validate. + * @return array|WP_Error The response array, or WP_Error. + */ + public static function validate_url( $url ) { + $validation_url = add_query_arg( self::VALIDATION_QUERY_VAR, 1, - home_url() + $url ); - return wp_remote_get( $url, array( + return wp_remote_get( $validation_url, array( 'cookies' => wp_unslash( $_COOKIE ), 'sslverify' => false, ) ); @@ -821,7 +854,7 @@ public static function add_recheck( $actions, $post ) { // @todo: $url needs to recheck the AMP validation of the page, and reload the edit.php page. if ( ! empty( $url ) ) { - $actions['recheck'] = sprintf( '%s', esc_url( $url ), esc_html__( 'Re-check', 'amp' ) ); + $actions[ self::RECHECK_ACTION ] = sprintf( '%s', esc_url( $url ), esc_html__( 'Re-check', 'amp' ) ); } return $actions; } @@ -834,8 +867,67 @@ public static function add_recheck( $actions, $post ) { */ public static function add_bulk_action( $actions ) { unset( $actions['edit'] ); - $actions['recheck'] = esc_html__( 'Re-check', 'amp' ); + $actions[ self::RECHECK_ACTION ] = esc_html__( 'Re-check', 'amp' ); return $actions; } + /** + * Handles the 'Re-check' bulk action on the edit.php page. + * + * @param string $redirect The URL of the redirect. + * @param string $action The action. + * @param array $items The items on which to take the action. + * @return string $redirect The filtered URL of the redirect. + */ + public static function handle_bulk_action( $redirect, $action, $items ) { + if ( self::RECHECK_ACTION !== $action ) { + return $redirect; + } + $urls = array(); + foreach ( $items as $item ) { + $url = get_post_meta( $item, self::AMP_URL_META, true ); + if ( ! empty( $url ) ) { + $urls[] = $url; + self::validate_url( $url ); + } + } + + // Get the URLs that still have errors after re-checking. + $remaining_error = false; + foreach ( $urls as $url ) { + $error_post_id = self::existing_post( $url ); + if ( isset( $error_post_id ) ) { + $remaining_error = true; + } + } + + return add_query_arg( + self::REMAINING_ERRORS, + $remaining_error ? self::REMAINING_ERROR_VALUE : '0', + $redirect + ); + } + + /** + * Outputs an admin notice after rechecking URL(s) on the custom post page. + * + * @return void + */ + public static function remaining_error_notice() { + if ( ! isset( $_GET['post_type'], $_GET[ self::REMAINING_ERRORS ] ) || ( self::POST_TYPE_SLUG !== sanitize_text_field( wp_unslash( $_GET['post_type'] ) ) ) ) { // WPCS: CSRF ok. + return; + } + + $is_error = ( isset( $_GET[ self::REMAINING_ERRORS ] ) && ( self::REMAINING_ERROR_VALUE === sanitize_text_field( wp_unslash( $_GET[ self::REMAINING_ERRORS ] ) ) ) ); // WPCS: CSRF ok. + $class = $is_error ? 'notice-warning' : 'updated'; + $message = $is_error ? __( 'Warning: the rechecked URLs still have validation errors', 'amp' ) : __( 'The rechecked URLs have no validation error', 'amp' ); + + printf( + '

%s

', + esc_attr( $class ), + esc_html( $message ), + esc_html__( 'Dismiss this notice.', 'amp' ) + ); + } + } diff --git a/tests/test-class-amp-validation-utils.php b/tests/test-class-amp-validation-utils.php index e74586e39ce..9a2cc8be087 100644 --- a/tests/test-class-amp-validation-utils.php +++ b/tests/test-class-amp-validation-utils.php @@ -96,16 +96,17 @@ public function test_init() { AMP_Validation_Utils::init(); $this->assertEquals( 10, has_action( 'rest_api_init', self::TESTED_CLASS . '::amp_rest_validation' ) ); $this->assertEquals( 10, has_action( 'edit_form_top', self::TESTED_CLASS . '::validate_content' ) ); - $this->assertEquals( 10, has_action( 'wp', self::TESTED_CLASS . '::callback_wrappers' ) ); - $this->assertEquals( 10, has_action( 'amp_content_sanitizers', self::TESTED_CLASS . '::add_validation_callback' ) ); - $this->assertEquals( 10, has_action( 'init', self::TESTED_CLASS . '::register_post_type' ) ); - $this->assertEquals( 10, has_filter( 'amp_content_sanitizers', self::TESTED_CLASS . '::add_validation_callback' ) ); $this->assertEquals( 10, has_action( 'init', self::TESTED_CLASS . '::register_post_type' ) ); $this->assertEquals( 10, has_action( 'all_admin_notices', self::TESTED_CLASS . '::plugin_notice' ) ); $this->assertEquals( 10, has_filter( 'manage_' . AMP_Validation_Utils::POST_TYPE_SLUG . '_posts_columns', self::TESTED_CLASS . '::add_post_columns' ) ); $this->assertEquals( 10, has_action( 'manage_posts_custom_column', self::TESTED_CLASS . '::output_custom_column' ) ); $this->assertEquals( 10, has_filter( 'post_row_actions', self::TESTED_CLASS . '::add_recheck' ) ); $this->assertEquals( 10, has_filter( 'bulk_actions-edit-' . AMP_Validation_Utils::POST_TYPE_SLUG, self::TESTED_CLASS . '::add_bulk_action' ) ); + $this->assertEquals( 10, has_filter( 'handle_bulk_actions-edit-' . AMP_Validation_Utils::POST_TYPE_SLUG, self::TESTED_CLASS . '::handle_bulk_action' ) ); + $this->assertEquals( 10, has_action( 'admin_notices', self::TESTED_CLASS . '::remaining_error_notice' ) ); + $this->assertEquals( 10, has_action( 'wp', self::TESTED_CLASS . '::callback_wrappers' ) ); + $this->assertEquals( -1, has_filter( 'do_shortcode_tag', self::TESTED_CLASS . '::decorate_shortcode_source' ) ); + $this->assertEquals( 10, has_action( 'amp_content_sanitizers', self::TESTED_CLASS . '::add_validation_callback' ) ); } /** @@ -751,7 +752,16 @@ public function test_existing_post() { * @see AMP_Validation_Utils::validate_home() */ public function test_validate_home() { - $response = AMP_Validation_Utils::validate_home(); + $this->assertTrue( is_array( AMP_Validation_Utils::validate_home() ) ); + } + + /** + * Test for validate_url() + * + * @see AMP_Validation_Utils::validate_url() + */ + public function test_validate_url() { + $response = AMP_Validation_Utils::validate_url( get_permalink( $this->factory()->post->create() ) ); $this->assertTrue( is_array( $response ) ); } @@ -855,7 +865,7 @@ public function test_add_recheck() { $custom_post_id = $this->create_custom_post(); $actions = AMP_Validation_Utils::add_recheck( $initial_actions, get_post( $custom_post_id ) ); $url = get_post_meta( $custom_post_id, AMP_Validation_Utils::AMP_URL_META, true ); - $this->assertContains( $url, $actions['recheck'] ); + $this->assertContains( $url, $actions[ AMP_Validation_Utils::RECHECK_ACTION ] ); $this->assertEquals( $initial_actions['trash'], $actions['trash'] ); } @@ -870,7 +880,61 @@ public function test_add_bulk_action() { ); $actions = AMP_Validation_Utils::add_bulk_action( $initial_action ); $this->assertFalse( isset( $action['edit'] ) ); - $this->assertEquals( 'Re-check', $actions['recheck'] ); + $this->assertEquals( 'Re-check', $actions[ AMP_Validation_Utils::RECHECK_ACTION ] ); + } + + /** + * Test for handle_bulk_action() + * + * @see AMP_Validation_Utils::handle_bulk_action() + */ + public function test_handle_bulk_action() { + $initial_redirect = admin_url( 'plugins.php' ); + $items = array( $this->create_custom_post(), $this->create_custom_post() ); + + // The action isn't 'amp-recheck,' so the callback should return the URL unchanged. + $this->assertEquals( $initial_redirect, AMP_Validation_Utils::handle_bulk_action( $initial_redirect, 'trash', $items ) ); + $this->assertEquals( + add_query_arg( + AMP_Validation_Utils::REMAINING_ERRORS, + AMP_Validation_Utils::REMAINING_ERROR_VALUE, + $initial_redirect + ), + AMP_Validation_Utils::handle_bulk_action( $initial_redirect, AMP_Validation_Utils::RECHECK_ACTION, $items ) + ); + } + + /** + * Test for remaining_error_notice() + * + * @see AMP_Validation_Utils::remaining_error_notice() + */ + public function test_remaining_error_notice() { + ob_start(); + AMP_Validation_Utils::remaining_error_notice(); + $this->assertEmpty( ob_get_clean() ); + + $_GET['post_type'] = 'post'; + $_GET[ AMP_Validation_Utils::REMAINING_ERRORS ] = AMP_Validation_Utils::REMAINING_ERROR_VALUE; + ob_start(); + AMP_Validation_Utils::remaining_error_notice(); + $this->assertEmpty( ob_get_clean() ); + + $_GET['post_type'] = AMP_Validation_Utils::POST_TYPE_SLUG; + ob_start(); + AMP_Validation_Utils::remaining_error_notice(); + $output = ob_get_clean(); + $this->assertContains( 'div class="notice is-dismissible', $output ); + $this->assertContains( 'notice-warning', $output ); + $this->assertContains( 'Warning: the rechecked URLs still have validation errors', $output ); + + $_GET[ AMP_Validation_Utils::REMAINING_ERRORS ] = 0; + ob_start(); + AMP_Validation_Utils::remaining_error_notice(); + $output = ob_get_clean(); + $this->assertContains( 'div class="notice is-dismissible', $output ); + $this->assertContains( 'updated', $output ); + $this->assertContains( 'The rechecked URLs have no validation error', $output ); } /** From 76e2be5d6fe6223a4c5f4958e8954932e5338667 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 1 Mar 2018 22:55:14 -0800 Subject: [PATCH 40/93] Refactor how sources are determined for validation * Allow sources to be determined when calling remove_invalid_child() from any sanitizers (not just whitelist). * Include more context in sources, including the shortcode tag and hook name. * Run removal callback before node is removed from document. * Add amp_disable_invalid_removal arg to prevent invalid elements and attributes from being removed (for debugging). * Prevent calling reset_removed() inside get_response() so that multiple copies of response can be obtained. Reduce side effects. * Remove use of constant for 'remove_invalid_callback' key since adds class dependency from AMP_Base_Sanitizer to AMP_Validation_Utils. * Prevent wrapping action output if it does not contain HTML tags, to prevent comments from corrupting actions triggered in HTML attributes. * Rename get_response() to get_validation_results() to prevent confusion with returning WP_REST_Response. * Add comment to end of body that contains the validation results when validation is being performed. * Check should_validate_front_end before calling store_validation_errors, not the other way around. * Remove redundant calls to has_cap(). * Add reporting for removal of amp-keyframe element. * Allow reporting of external stylesheet links. * Prevent applying the_content filters twice when calling validate_content. --- includes/class-amp-theme-support.php | 25 ++- .../sanitizers/class-amp-base-sanitizer.php | 65 ++---- .../sanitizers/class-amp-style-sanitizer.php | 6 +- .../class-amp-tag-and-attribute-sanitizer.php | 7 +- includes/utils/class-amp-validation-utils.php | 203 ++++++++++++------ phpcs.xml | 2 +- tests/test-class-amp-base-sanitizer.php | 44 ---- tests/test-class-amp-theme-support.php | 2 +- tests/test-class-amp-validation-utils.php | 190 ++++++++++------ 9 files changed, 306 insertions(+), 238 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 077d7bdf343..140da2d1da5 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -216,6 +216,10 @@ public static function register_hooks() { add_action( 'comment_form', array( __CLASS__, 'add_amp_comment_form_templates' ), 100 ); remove_action( 'comment_form', 'wp_comment_form_unfiltered_html_nonce' ); + if ( AMP_Validation_Utils::should_validate_front_end() ) { + AMP_Validation_Utils::add_validation_hooks(); + } + // @todo Add character conversion. } @@ -955,11 +959,12 @@ public static function prepare_response( $response, $args = array() ) { $args = array_merge( array( - 'content_max_width' => ! empty( $content_width ) ? $content_width : AMP_Post_Template::CONTENT_MAX_WIDTH, // Back-compat. - 'use_document_element' => true, - AMP_Validation_Utils::CALLBACK_KEY => null, - '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. + 'content_max_width' => ! empty( $content_width ) ? $content_width : AMP_Post_Template::CONTENT_MAX_WIDTH, // Back-compat. + 'use_document_element' => true, + 'remove_invalid_callback' => null, + '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. + 'disable_invalid_removal' => ! empty( $_REQUEST['amp_disable_invalid_removal'] ), // WPCS: csrf ok. ), $args ); @@ -994,7 +999,15 @@ public static function prepare_response( $response, $args = array() ) { trigger_error( esc_html( sprintf( __( 'The database has the %s encoding when it needs to be utf-8 to work with AMP.', 'amp' ), get_bloginfo( 'charset' ) ) ), E_USER_WARNING ); // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_trigger_error } - AMP_Validation_Utils::store_validation_errors(); + if ( AMP_Validation_Utils::should_validate_front_end() ) { + AMP_Validation_Utils::store_validation_errors(); + $comment = $dom->createComment( "\nValidation Status:\n" . wp_json_encode( AMP_Validation_Utils::get_validation_results() ) ); + $body = $dom->getElementsByTagName( 'body' )->item( 0 ); + if ( $body ) { + $body->appendChild( $comment ); + } + } + $response = "\n"; $response .= AMP_DOM_Utils::get_content_from_dom_node( $dom, $dom->documentElement ); diff --git a/includes/sanitizers/class-amp-base-sanitizer.php b/includes/sanitizers/class-amp-base-sanitizer.php index c0fcb94d67a..50683330394 100644 --- a/includes/sanitizers/class-amp-base-sanitizer.php +++ b/includes/sanitizers/class-amp-base-sanitizer.php @@ -54,6 +54,7 @@ abstract class AMP_Base_Sanitizer { * @type array $amp_bind_placeholder_prefix * @type bool $allow_dirty_styles * @type bool $allow_dirty_scripts + * @type bool $disable_invalid_removal * @type callable $remove_invalid_callback * } */ @@ -76,15 +77,6 @@ abstract class AMP_Base_Sanitizer { */ protected $root_element; - /** - * Stack of the plugin or theme that is outputting markup, if any. - * - * Each item in the stack is an array containing the 'type' (theme, plugin, mu-plugin) and the 'name' of the extension. - * - * @var array[][] - */ - public $current_sources = array(); - /** * AMP_Base_Sanitizer constructor. * @@ -334,16 +326,17 @@ public function maybe_enforce_https_src( $src, $force_https = false ) { */ public function remove_invalid_child( $child ) { $parent = $child->parentNode; - $child->parentNode->removeChild( $child ); - if ( isset( $this->args[ AMP_Validation_Utils::CALLBACK_KEY ] ) ) { - call_user_func( $this->args[ AMP_Validation_Utils::CALLBACK_KEY ], + if ( isset( $this->args['remove_invalid_callback'] ) ) { + call_user_func( $this->args['remove_invalid_callback'], array( - 'node' => $child, - 'parent' => $parent, - 'sources' => $this->current_sources, + 'node' => $child, + 'parent' => $parent, ) ); } + if ( empty( $this->args['disable_invalid_removal'] ) ) { + $child->parentNode->removeChild( $child ); + } } /** @@ -359,43 +352,27 @@ public function remove_invalid_child( $child ) { * @return void */ public function remove_invalid_attribute( $element, $attribute ) { - if ( isset( $this->args[ AMP_Validation_Utils::CALLBACK_KEY ] ) ) { + if ( isset( $this->args['remove_invalid_callback'] ) ) { if ( is_string( $attribute ) ) { $attribute = $element->getAttributeNode( $attribute ); } if ( $attribute ) { - $element->removeAttributeNode( $attribute ); - call_user_func( $this->args[ AMP_Validation_Utils::CALLBACK_KEY ], + call_user_func( $this->args['remove_invalid_callback'], array( - 'node' => $attribute, - 'parent' => $element, - 'sources' => $this->current_sources, + 'node' => $attribute, + 'parent' => $element, ) ); + if ( empty( $this->args['disable_invalid_removal'] ) ) { + $element->removeAttributeNode( $attribute ); + } + } + } elseif ( empty( $this->args['disable_invalid_removal'] ) ) { + if ( is_string( $attribute ) ) { + $element->removeAttribute( $attribute ); + } else { + $element->removeAttributeNode( $attribute ); } - } elseif ( is_string( $attribute ) ) { - $element->removeAttribute( $attribute ); - } else { - $element->removeAttributeNode( $attribute ); - } - } - - /** - * Updates the captured current plugin that is outputting markup. - * - * @since 0.7 - * - * @param DOMComment $node The node to check for the presence of a plugin in a comment. - * @return void - */ - public function capture_current_source( $node ) { - if ( ! preg_match( '#(?P/)?(?Ptheme|plugin|mu-plugin):(?P.*)#s', $node->nodeValue, $matches ) ) { - return; - } - if ( ! empty( $matches['closing'] ) ) { - $this->current_sources[] = wp_array_slice_assoc( $matches, array( 'type', 'name' ) ); - } else { - array_pop( $this->current_sources ); } } } diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index f085144cf87..e1b18b9cef4 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -237,7 +237,7 @@ public function sanitize() { } $this->amp_custom_style_element->appendChild( $this->dom->createTextNode( $css ) ); - // @todo This would be a candidate for sanitization reporting. + // @todo This would be a candidate for sanitization reporting, when ! empty( $skipped ) || $total_size > $this->custom_max_size. // Add comments to indicate which sheets were not included. foreach ( array_reverse( $skipped ) as $skip ) { $this->amp_custom_style_element->parentNode->insertBefore( @@ -306,7 +306,7 @@ private function process_style_element( DOMElement $element ) { if ( 'body' === $element->parentNode->nodeName && $element->hasAttribute( 'amp-keyframes' ) ) { $validity = $this->validate_amp_keyframe( $element ); if ( true !== $validity ) { - $element->parentNode->removeChild( $element ); // @todo Add reporting. + $this->remove_invalid_child( $element ); } return; } @@ -344,7 +344,7 @@ private function process_link_element( DOMElement $element ) { $css_file_path = $this->get_validated_css_file_path( $href ); if ( is_wp_error( $css_file_path ) ) { - $element->parentNode->removeChild( $element ); // @todo Report removal. Show HTML comment? + $this->remove_invalid_child( $element ); return; } diff --git a/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php b/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php index 99cba3d694b..030c2be8d3b 100644 --- a/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php +++ b/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php @@ -283,10 +283,7 @@ public function sanitize() { private function process_node( $node ) { // Don't process text or comment nodes. - if ( XML_COMMENT_NODE === $node->nodeType ) { - $this->capture_current_source( $node ); - return; - } elseif ( XML_TEXT_NODE === $node->nodeType || XML_CDATA_SECTION_NODE === $node->nodeType ) { + if ( XML_TEXT_NODE === $node->nodeType || XML_COMMENT_NODE === $node->nodeType || XML_CDATA_SECTION_NODE === $node->nodeType ) { return; } @@ -1562,7 +1559,7 @@ private function remove_node( $node ) { $node = $parent; $parent = $parent->parentNode; if ( $parent ) { - $this->remove_invalid_child( $node ); + $parent->removeChild( $node ); } } } diff --git a/includes/utils/class-amp-validation-utils.php b/includes/utils/class-amp-validation-utils.php index 30015773eb7..26087715077 100644 --- a/includes/utils/class-amp-validation-utils.php +++ b/includes/utils/class-amp-validation-utils.php @@ -139,9 +139,6 @@ public static function init() { // @todo There is more than just node removal that needs to be reported. There is also script enqueues, external stylesheets, cdata length, etc. // Actions and filters involved in validation. - add_action( 'wp', array( __CLASS__, 'callback_wrappers' ) ); - add_filter( 'do_shortcode_tag', array( __CLASS__, 'decorate_shortcode_source' ), -1, 2 ); - add_filter( 'amp_content_sanitizers', array( __CLASS__, 'add_validation_callback' ) ); add_action( 'activate_plugin', function() { if ( ! has_action( 'shutdown', array( __CLASS__, 'validate_home' ) ) ) { add_action( 'shutdown', array( __CLASS__, 'validate_home' ) ); // Shutdown so all plugins will have been activated. @@ -149,6 +146,15 @@ public static function init() { } ); } + /** + * Add hooks for doing validation during preprocessing/sanitizing. + */ + public static function add_validation_hooks() { + add_action( 'wp', array( __CLASS__, 'callback_wrappers' ) ); + add_filter( 'do_shortcode_tag', array( __CLASS__, 'decorate_shortcode_source' ), -1, 2 ); + add_filter( 'amp_content_sanitizers', array( __CLASS__, 'add_validation_callback' ) ); + } + /** * Tracks when a sanitizer removes a node (element or attribute). * @@ -157,16 +163,11 @@ public static function init() { * * @type DOMElement|DOMNode $node The removed node. * @type DOMElement|DOMNode $parent The parent of the removed node. - * @type array[][] $sources { - * The data of the removed sources (theme, plugin, or mu-plugin). - * - * @type string $name The name of the source. - * @type string $type The type of the source. - * } * } * @return void */ public static function track_removed( $removed ) { + $removed['sources'] = self::locate_sources( $removed['node'] ); self::$removed_nodes[] = $removed; } @@ -186,23 +187,20 @@ public static function was_node_removed() { * Also passes a 'remove_invalid_callback' to keep track of stripped attributes and nodes. * * @param string $markup The markup to process. - * @return void + * @return string Sanitized markup. */ public static function process_markup( $markup ) { - if ( ! self::has_cap() ) { - return; - } - AMP_Theme_Support::register_content_embed_handlers(); - remove_filter( 'the_content', 'wpautop' ); /** This filter is documented in wp-includes/post-template.php */ $markup = apply_filters( 'the_content', $markup ); $args = array( - 'content_max_width' => ! empty( $content_width ) ? $content_width : AMP_Post_Template::CONTENT_MAX_WIDTH, - self::CALLBACK_KEY => 'AMP_Validation_Utils::track_removed', + 'content_max_width' => ! empty( $content_width ) ? $content_width : AMP_Post_Template::CONTENT_MAX_WIDTH, + 'remove_invalid_callback' => 'AMP_Validation_Utils::track_removed', ); - AMP_Content_Sanitizer::sanitize( $markup, amp_get_content_sanitizers(), $args ); + + $results = AMP_Content_Sanitizer::sanitize( $markup, amp_get_content_sanitizers(), $args ); + return $results[0]; } /** @@ -238,6 +236,8 @@ public static function has_cap() { /** * Validate the markup passed to the REST API. * + * @todo Rename to handle_validate_request or something. + * * @param WP_REST_Request $request The REST request. * @return array|WP_Error. */ @@ -249,28 +249,25 @@ public static function validate_markup( WP_REST_Request $request ) { ) ); } - return self::get_response( $json[ self::MARKUP_KEY ] ); + // @todo Add request param to indicate whether the supplied content is raw (and needs the_content filters applied). + $processed = self::process_markup( $json[ self::MARKUP_KEY ] ); + $response = self::get_validation_results(); + self::reset_removed(); + $response['processed_markup'] = $processed; + return $response; } /** * Gets the AMP validation response. * - * If $markup isn't passed, - * It will return the validation errors the sanitizers found in rendering the page. + * Returns the current validation errors the sanitizers found in rendering the page. + * + * @todo Refactor this to return list of each element/attribute removed along with their sources. No need to normalize since information is list. * - * @param string $markup To validate for AMP compatibility (optional). - * @return array $response The AMP validity of the markup. + * @return array The AMP validity of the markup. */ - public static function get_response( $markup = null ) { - global $wp; - $response = array(); - if ( isset( $markup ) ) { - self::process_markup( $markup ); - $response['processed_markup'] = $markup; - } elseif ( isset( $wp ) ) { - $response['url'] = add_query_arg( $wp->query_string, '', home_url( $wp->request ) ); - } - + public static function get_validation_results() { + $results = array(); $removed_elements = array(); $removed_attributes = array(); $invalid_sources = array(); @@ -298,19 +295,18 @@ public static function get_response( $markup = null ) { } } - $response = array_merge( + $results = array_merge( array( - self::ERROR_KEY => self::was_node_removed(), + self::ERROR_KEY => self::was_node_removed(), // @todo This should probably be eliminated. self::SOURCES_INVALID_OUTPUT => $invalid_sources, ), compact( 'removed_elements', 'removed_attributes' ), - $response ); - self::reset_removed(); + $results ); - return $response; + return $results; } /** @@ -320,6 +316,8 @@ public static function get_response( $markup = null ) { * these static values will remain. * So reset them in case another test is needed. * + * @todo Rename to reset_validation_results(). + * * @return void */ public static function reset_removed() { @@ -349,16 +347,87 @@ public static function validate_arg( $arg ) { * @return void */ public static function validate_content( $post ) { - if ( ! post_supports_amp( $post ) || ! self::has_cap() ) { + if ( ! post_supports_amp( $post ) ) { return; } - AMP_Theme_Support::register_content_embed_handlers(); - /** This filter is documented in wp-includes/post-template.php */ - $filtered_content = apply_filters( 'the_content', $post->post_content, $post->ID ); - $response = self::get_response( $filtered_content ); - if ( isset( $response[ self::ERROR_KEY ] ) && ( true === $response[ self::ERROR_KEY ] ) ) { - self::display_error( $response ); + + self::process_markup( $post->post_content ); + $results = self::get_validation_results(); + self::reset_removed(); + if ( isset( $results[ self::ERROR_KEY ] ) && ( true === $results[ self::ERROR_KEY ] ) ) { // @todo Is not error implied by $results not being empty? ERROR_KEY seems redundant. + self::display_error( $results ); + } + } + + /** + * Get source start comment. + * + * @param string $type Extension type. + * @param string $name Extension name. + * @param array $args Args. + * @return string HTML Comment. + */ + public static function get_source_comment_start( $type, $name, $args = array() ) { + return sprintf( '', $type, $name, str_replace( '--', '', wp_json_encode( $args ) ) ); + } + + /** + * Get source end comment. + * + * @param string $type Extension type. + * @param string $name Extension name. + * @return string HTML Comment. + */ + public static function get_source_comment_end( $type, $name ) { + return sprintf( '', $type, $name ); + } + + /** + * Parse source comment. + * + * @param DOMComment $comment Comment. + * @return array|null Source info or null if not a source comment. + */ + public static function parse_source_comment( DOMComment $comment ) { + if ( ! preg_match( '#^\s*(?P/)?amp-source-stack:(?Ptheme|plugin|mu-plugin):(?P\S+)(?: (?P{.+}))?\s*$#s', $comment->nodeValue, $matches ) ) { + return null; + } + $source = wp_array_slice_assoc( $matches, array( 'type', 'name' ) ); + + $source['closing'] = ! empty( $matches['closing'] ); + if ( isset( $matches['args'] ) ) { + $source['args'] = json_decode( $matches['args'], true ); } + return $source; + } + + /** + * Walk back tree to find the open sources. + * + * @param DOMNode $node Node to look for. + * @return array[][] { + * The data of the removed sources (theme, plugin, or mu-plugin). + * + * @type string $name The name of the source. + * @type string $type The type of the source. + * } + */ + public static function locate_sources( DOMNode $node ) { + $xpath = new DOMXPath( $node->ownerDocument ); + $comments = $xpath->query( 'preceding::comment()[ contains( ., "amp-source-stack:" ) ]', $node ); + $sources = array(); + foreach ( $comments as $comment ) { + $source = self::parse_source_comment( $comment ); + if ( $source ) { + if ( $source['closing'] ) { + array_pop( $sources ); + } else { + unset( $source['closing'] ); + $sources[] = $source; + } + } + } + return $sources; } /** @@ -373,9 +442,6 @@ public static function validate_content( $post ) { */ public static function callback_wrappers() { global $wp_filter; - if ( ! self::should_validate_front_end() ) { // @todo Better to just conditionally call callback_wrappers() based on whether should_validate_front_end(). Active request rather than passive. - return; - } $pending_wrap_callbacks = array(); foreach ( $wp_filter as $filter_tag => $wp_hook ) { foreach ( $wp_hook->callbacks as $priority => $callbacks ) { @@ -385,6 +451,9 @@ public static function callback_wrappers() { $pending_wrap_callbacks[ $filter_tag ][] = array_merge( $callback, $source_data, + array( + 'hook' => $filter_tag, + ), compact( 'priority' ) ); } @@ -414,9 +483,6 @@ public static function callback_wrappers() { */ public static function decorate_shortcode_source( $output, $tag ) { global $shortcode_tags; - if ( ! self::should_validate_front_end() ) { // @todo Better to just conditionally call callback_wrappers() based on whether should_validate_front_end(). Active request rather than passive. - return $output; - } if ( ! isset( $shortcode_tags[ $tag ] ) ) { return $output; } @@ -425,9 +491,9 @@ public static function decorate_shortcode_source( $output, $tag ) { return $output; } $output = implode( '', array( - sprintf( '', esc_attr( $source['type'] ), esc_attr( $source['name'] ) ), // @todo Need to also include the offending shortcode $tag. + self::get_source_comment_start( $source['type'], $source['name'], array( 'shortcode' => $tag ) ), $output, - sprintf( '', esc_attr( $source['type'] ), esc_attr( $source['name'] ) ), + self::get_source_comment_end( $source['type'], $source['name'] ), ) ); return $output; } @@ -501,6 +567,7 @@ public static function get_source( $callback ) { * @type int $accepted_args * @type string $type * @type string $source + * @type string $hook * } * @return closure $wrapped_callback The callback, wrapped in comments. */ @@ -516,14 +583,15 @@ public static function wrapped_callback( $callback ) { } elseif ( $accepted_args >= func_num_args() ) { $result = call_user_func_array( $function, $args ); } else { - $result = call_user_func_array( $function, array_slice( $args, 0, intval( $accepted_args ) ) ); + $result = call_user_func_array( $function, array_slice( $args, 0, intval( $accepted_args ) ) ); // @todo Why not only do this? } $output = ob_get_clean(); - if ( ! empty( $output ) ) { - printf( '', esc_attr( $callback['type'] ), esc_attr( $callback['name'] ) ); + // Wrap output that contains HTML tags (as opposed to actions that trigger in HTML attributes). + if ( ! empty( $output ) && preg_match( '/<.+?>/', $output ) ) { + echo self::get_source_comment_start( $callback['type'], $callback['name'], array( 'hook' => $callback['hook'] ) ); // WPCS: XSS ok. echo $output; // WPCS: XSS ok. - printf( '', esc_attr( $callback['type'] ), esc_attr( $callback['name'] ) ); + echo self::get_source_comment_end( $callback['type'], $callback['name'] ); // WPCS: XSS ok. } return $result; }; @@ -601,11 +669,13 @@ public static function should_validate_front_end() { * @return array $sanitizers The filtered AMP sanitizers. */ public static function add_validation_callback( $sanitizers ) { - if ( self::should_validate_front_end() ) { - foreach ( $sanitizers as $sanitizer => $args ) { - $args[ self::CALLBACK_KEY ] = __CLASS__ . '::track_removed'; - $sanitizers[ $sanitizer ] = $args; - } + foreach ( $sanitizers as $sanitizer => $args ) { + $sanitizers[ $sanitizer ] = array_merge( + $args, + array( + 'remove_invalid_callback' => __CLASS__ . '::track_removed', + ) + ); } return $sanitizers; } @@ -638,14 +708,13 @@ public static function register_post_type() { * If there's already an error post for the URL, but there's no error anymore, it deletes it. * * @return int|null $post_id The post ID of the custom post type used, or null. + * @global WP $wp */ public static function store_validation_errors() { - if ( ! self::should_validate_front_end() ) { - return null; - } + global $wp; - $response = self::get_response(); - $url = isset( $response['url'] ) ? $response['url'] : null; // @todo If url is not defined then that should be an error. + $response = self::get_validation_results(); + $url = add_query_arg( $wp->query_string, '', home_url( $wp->request ) ); unset( $response['url'] ); $encoded_errors = wp_json_encode( $response ); $post_name = md5( $encoded_errors ); diff --git a/phpcs.xml b/phpcs.xml index 312fc87d2aa..16c6f31e59f 100644 --- a/phpcs.xml +++ b/phpcs.xml @@ -28,7 +28,7 @@ - + diff --git a/tests/test-class-amp-base-sanitizer.php b/tests/test-class-amp-base-sanitizer.php index 2269f28d3f9..f2208ccbf73 100644 --- a/tests/test-class-amp-base-sanitizer.php +++ b/tests/test-class-amp-base-sanitizer.php @@ -344,48 +344,4 @@ public function test_remove_attribute() { ); AMP_Validation_Utils::reset_removed(); } - - /** - * Test capture_current_source. - * - * @covers AMP_Base_Sanitizer::capture_current_source() - */ - public function test_capture_current_source() { - $dom = new DOMDocument(); - $node = $dom->createComment( '/plugin:amp' ); - $sanitizer = new AMP_Tag_And_Attribute_Sanitizer( $dom ); - $this->assertEquals( array(), $sanitizer->current_sources ); - $sanitizer->capture_current_source( $node ); - $this->assertCount( 1, $sanitizer->current_sources ); - - $amp_source = array( - 'type' => 'plugin', - 'name' => 'amp', - ); - $foo_source = array( - 'type' => 'theme', - 'name' => 'foo', - ); - - $this->assertEquals( - $amp_source, - $sanitizer->current_sources[0] - ); - - $node = $dom->createComment( '/theme:foo' ); - $sanitizer->capture_current_source( $node ); - $this->assertEquals( - array( - $amp_source, - $foo_source, - ), - $sanitizer->current_sources - ); - - $sanitizer->capture_current_source( $dom->createComment( 'theme:foo' ) ); - $this->assertEquals( array( $amp_source ), $sanitizer->current_sources ); - $sanitizer->capture_current_source( $dom->createComment( 'plugin:amp' ) ); - $this->assertEquals( array(), $sanitizer->current_sources ); - } - } diff --git a/tests/test-class-amp-theme-support.php b/tests/test-class-amp-theme-support.php index 7f1bf2af09f..9a97ff9fedd 100644 --- a/tests/test-class-amp-theme-support.php +++ b/tests/test-class-amp-theme-support.php @@ -149,7 +149,7 @@ public function test_prepare_response() { $this->assertContains( '