diff --git a/includes/sanitizers/class-amp-base-sanitizer.php b/includes/sanitizers/class-amp-base-sanitizer.php index 8e7a936afb7..a0331d019e7 100644 --- a/includes/sanitizers/class-amp-base-sanitizer.php +++ b/includes/sanitizers/class-amp-base-sanitizer.php @@ -314,12 +314,12 @@ public function maybe_enforce_https_src( $src, $force_https = false ) { * @since 0.7 * * @param DOMElement $child The node to remove. - * @return void. + * @return void */ public function remove_invalid_child( $child ) { $child->parentNode->removeChild( $child ); if ( isset( $this->args['remove_invalid_callback'] ) ) { - call_user_func( $this->args['remove_invalid_callback'], $child, AMP_Validation_Utils::NODE_REMOVED ); + call_user_func( $this->args['remove_invalid_callback'], $child ); } } @@ -331,14 +331,23 @@ public function remove_invalid_child( $child ) { * * @since 0.7 * - * @param DOMElement $element The node for which to remove the attribute. - * @param string $attribute The attribute to remove from the node. - * @return void. + * @param DOMElement $element The node for which to remove the attribute. + * @param DOMAttr|string $attribute The attribute to remove from the element. + * @return void */ public function remove_invalid_attribute( $element, $attribute ) { - $element->removeAttribute( $attribute ); if ( isset( $this->args['remove_invalid_callback'] ) ) { - call_user_func( $this->args['remove_invalid_callback'], $element, AMP_Validation_Utils::ATTRIBUTE_REMOVED, $attribute ); + if ( is_string( $attribute ) ) { + $attribute = $element->getAttributeNode( $attribute ); + } + if ( $attribute ) { + $element->removeAttributeNode( $attribute ); + call_user_func( $this->args['remove_invalid_callback'], $attribute ); + } + } elseif ( is_string( $attribute ) ) { + $element->removeAttribute( $attribute ); + } else { + $element->removeAttributeNode( $attribute ); } } diff --git a/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php b/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php index b4c0faeee31..bdc9aa54e72 100644 --- a/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php +++ b/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php @@ -729,10 +729,7 @@ private function sanitize_disallowed_attributes_in_node( $node, $attr_spec_list } foreach ( $attrs_to_remove as $attr ) { - $node->removeAttributeNode( $attr ); - if ( isset( $this->args['remove_invalid_callback'], $attr->name ) ) { - call_user_func( $this->args['remove_invalid_callback'], $node, AMP_Validation_Utils::ATTRIBUTE_REMOVED, $attr->name ); - } + $this->remove_invalid_attribute( $node, $attr ); } } diff --git a/includes/utils/class-amp-validation-utils.php b/includes/utils/class-amp-validation-utils.php index c7450248f81..6b79504e7f8 100644 --- a/includes/utils/class-amp-validation-utils.php +++ b/includes/utils/class-amp-validation-utils.php @@ -12,20 +12,6 @@ */ class AMP_Validation_Utils { - /** - * The argument if an attribute was removed. - * - * @var array. - */ - const ATTRIBUTE_REMOVED = 'removed_attr'; - - /** - * The argument if a node was removed. - * - * @var array. - */ - const NODE_REMOVED = 'removed'; - /** * Key for the markup value in the REST API endpoint. * @@ -40,86 +26,31 @@ class AMP_Validation_Utils { */ const ERROR_KEY = 'has_error'; - /** - * Key of the AMP error query var. - * - * @var string. - */ - const ERROR_QUERY_KEY = 'amp_error'; - - /** - * Query arg value if there is an AMP error in the post content. - * - * @var string. - */ - const ERROR_QUERY_VALUE = '1'; - - /** - * Nonce name for the editor error message. - * - * @var string. - */ - const ERROR_NONCE = 'amp_nonce'; - - /** - * Nonce action for displaying the invalid AMP message. - * - * @var string. - */ - const ERROR_NONCE_ACTION = 'invalid_amp_message'; - - /** - * The attributes that the sanitizer removed. - * - * @var array. - */ - public static $removed_attributes; - /** * The nodes that the sanitizer removed. * - * @var array. + * @var DOMNode[] */ - public static $removed_nodes; + public static $removed_nodes = array(); /** * Add the actions. * - * @return void. + * @return void */ public static function init() { add_action( 'rest_api_init', array( __CLASS__, 'amp_rest_validation' ) ); - add_action( 'save_post', array( __CLASS__, 'validate_content' ), 10, 2 ); - add_action( 'edit_form_top', array( __CLASS__, 'display_error' ) ); - } - - /** - * Tracks when a sanitizer removes an attribute or node. - * - * @param DOMNode|DOMElement $node The node in which there was a removal. - * @param string $removal_type The removal: 'removed_attr' for an attribute, or 'removed' for a node or element. - * @param string $attr_name The name of the attribute removed (optional). - * @return void. - */ - public static function track_removed( $node, $removal_type, $attr_name = null ) { - if ( ( self::ATTRIBUTE_REMOVED === $removal_type ) && isset( $attr_name ) ) { // phpcs:ignore WordPress.NamingConventions.ValidVariableName.NotSnakeCaseMemberVar - self::$removed_attributes = self::increment_count( self::$removed_attributes, $attr_name ); - } elseif ( ( self::NODE_REMOVED === $removal_type ) && isset( $node->nodeName ) ) { - self::$removed_nodes = self::increment_count( self::$removed_nodes, $node->nodeName ); // phpcs:ignore WordPress.NamingConventions.ValidVariableName.NotSnakeCaseMemberVar - } + add_action( 'edit_form_top', array( __CLASS__, 'validate_content' ), 10, 2 ); } /** - * Tracks when a sanitizer removes an attribute or node. + * Tracks when a sanitizer removes an node (element or attribute). * - * @param array $histogram The count of attributes or nodes removed. - * @param string $key The attribute or node name removed. - * @return array $histogram The incremented histogram. + * @param DOMNode $node The node which was removed. + * @return void */ - public static function increment_count( $histogram, $key ) { - $current_value = isset( $histogram[ $key ] ) ? $histogram[ $key ] : 0; - $histogram[ $key ] = $current_value + 1; - return $histogram; + public static function track_removed( $node ) { + self::$removed_nodes[] = $node; } /** @@ -141,12 +72,19 @@ public static function was_node_removed() { * @return void. */ public static function process_markup( $markup ) { - $args = array( - 'content_max_width' => ! empty( $content_width ) ? $content_width : AMP_Post_Template::CONTENT_MAX_WIDTH, - ); - if ( self::is_authorized() ) { - $args['remove_invalid_callback'] = 'AMP_Validation_Utils::track_removed'; + 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, + 'remove_invalid_callback' => 'AMP_Validation_Utils::track_removed', + ); AMP_Content_Sanitizer::sanitize( $markup, amp_get_content_sanitizers(), $args ); } @@ -212,11 +150,34 @@ public static function get_response( $markup = null ) { self::process_markup( $markup ); $response['processed_markup'] = $markup; } - $response = array_merge( array( - self::ERROR_KEY => self::was_node_removed(), - 'removed_nodes' => self::$removed_nodes, - 'removed_attributes' => self::$removed_attributes, - ), $response ); + + $removed_elements = array(); + $removed_attributes = array(); + foreach ( self::$removed_nodes as $node ) { + if ( $node instanceof DOMAttr ) { + if ( ! isset( $removed_attributes[ $node->nodeName ] ) ) { + $removed_attributes[ $node->nodeName ] = 1; + } else { + $removed_attributes[ $node->nodeName ]++; + } + } elseif ( $node instanceof DOMElement ) { + if ( ! isset( $removed_elements[ $node->nodeName ] ) ) { + $removed_elements[ $node->nodeName ] = 1; + } else { + $removed_elements[ $node->nodeName ]++; + } + } + } + + $response = array_merge( + array( + self::ERROR_KEY => self::was_node_removed(), + ), + compact( + 'removed_elements', + 'removed_attributes' + ), + $response ); self::reset_removed(); return $response; @@ -232,8 +193,7 @@ public static function get_response( $markup = null ) { * @return void. */ public static function reset_removed() { - self::$removed_nodes = null; - self::$removed_attributes = null; + self::$removed_nodes = array(); } /** @@ -250,106 +210,66 @@ public static function validate_arg( $arg ) { } /** - * On updating a post, this checks the AMP validity of the content. + * Checks the AMP validity of the post content. * - * If it's not valid AMP, it adds a query arg to the redirect URL. - * This will cause an error message to appear above the 'Classic' editor. + * If it's not valid AMP, + * it displays an error message above the 'Classic' editor. * - * @param integer $post_id The ID of the updated post. - * @param WP_Post $post The updated post. + * @param WP_Post $post The updated post. * @return void. */ - public static function validate_content( $post_id, $post ) { - unset( $post_id ); - if ( ! post_supports_amp( $post ) || ! self::is_authorized() ) { + public static function validate_content( $post ) { + if ( ! post_supports_amp( $post ) || ! self::has_cap() ) { 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 ] ) ) { - add_filter( 'redirect_post_location', function( $location ) use ( $response ) { - $location = AMP_Validation_Utils::error_message( $location ); - $location = add_query_arg( - array( - 'removed_elements' => array_keys( $response['removed_nodes'] ), - 'removed_attributes' => array_keys( $response['removed_attributes'] ), - ), - $location - ); - return $location; - } ); + self::display_error( $response ); } } /** - * Whether the current user is authorized. + * Displays an error message on /wp-admin/post.php. * - * This checks that the user has a certain capability and the nonce is valid. - * It will only return true when updating the post on: - * wp-admin/post.php - * Avoids using check_admin_referer(). - * This function might be called in different places, - * and it can't cause it to die() if the nonce is invalid. - * - * @return boolean $is_valid True if the nonce is valid. - */ - public static function is_authorized() { - $nonce = isset( $_REQUEST['_wpnonce'] ) ? sanitize_text_field( wp_unslash( $_REQUEST['_wpnonce'] ) ) : ''; // WPCS: CSRF ok. - return ( self::has_cap() && ( false !== wp_verify_nonce( $nonce, 'update-post_' . get_the_ID() ) ) ); - } - - /** - * Adds an error message to the URL if it's not valid AMP. - * - * When redirecting after saving a post, the content was validated for AMP compliance. - * If it wasn't valid AMP, this will add a query arg to the URL. - * And an error message will display on /wp-admin/post.php. - * - * @param string $url The URL of the redirect. - * @return string $url The filtered URL, including the AMP error message query var. - */ - public static function error_message( $url ) { - $args = array( - self::ERROR_QUERY_KEY => self::ERROR_QUERY_VALUE, - self::ERROR_NONCE => wp_create_nonce( self::ERROR_NONCE_ACTION ), - ); - return add_query_arg( $args, $url ); - } - - /** - * Displays an error message on /wp-admin/post.php if the saved content is not valid AMP. - * - * Use $_GET, as get_query_var won't return the value. - * This displays at the top of the 'Classic' editor. + * Located at the top of the 'Classic' editor. + * States that the content is not valid AMP. * + * @param array $response The validation response, an associative array. * @return void. */ - public static function display_error() { - if ( ! isset( $_GET[ self::ERROR_QUERY_KEY ] ) ) { - return; + 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'] ) ) { + $removed_sets[] = array( + 'label' => __( 'Invalid elements:', 'amp' ), + 'names' => array_map( 'sanitize_key', $response['removed_elements'] ), + ); } - check_admin_referer( self::ERROR_NONCE_ACTION, self::ERROR_NONCE ); - $error = isset( $_GET[ self::ERROR_QUERY_KEY ] ) ? sanitize_text_field( wp_unslash( $_GET[ self::ERROR_QUERY_KEY ] ) ) : ''; // WPCS: CSRF ok. - if ( self::ERROR_QUERY_VALUE === $error ) { - echo '
'; - printf( '

%s

', esc_html__( 'Warning: There is content which fails AMP validation; it will be stripped when served as AMP.', 'amp' ) ); - if ( ! empty( $_GET['removed_elements'] ) && is_array( $_GET['removed_elements'] ) ) { - printf( - '

%s %s

', - esc_html__( 'Invalid elements:', 'amp' ), - '' . implode( ', ', array_map( 'esc_html', $_GET['removed_elements'] ) ) . '' - ); - } - if ( ! empty( $_GET['removed_attributes'] ) ) { - printf( - '

%s %s

', - esc_html__( 'Invalid attributes:', 'amp' ), - '' . implode( ', ', array_map( 'esc_html', $_GET['removed_attributes'] ) ) . '' - ); + if ( ! empty( $response['removed_attributes'] ) && is_array( $response['removed_attributes'] ) ) { + $removed_sets[] = array( + 'label' => __( 'Invalid attributes:', 'amp' ), + 'names' => array_map( 'sanitize_key', $response['removed_attributes'] ), + ); + } + foreach ( $removed_sets as $removed_set ) { + printf( '

%s ', esc_html( $removed_set['label'] ) ); + $items = array(); + foreach ( $removed_set['names'] as $name => $count ) { + if ( 1 === intval( $count ) ) { + $items[] = sprintf( '%s', esc_html( $name ) ); + } else { + $items[] = sprintf( '%s (%d)', esc_html( $name ), $count ); + } } - echo '

'; + echo implode( ', ', $items ); // WPCS: XSS OK. + echo '

'; } + echo '
'; } } diff --git a/tests/test-class-amp-base-sanitizer.php b/tests/test-class-amp-base-sanitizer.php index a7c6bcd8f08..b96bd95824c 100644 --- a/tests/test-class-amp-base-sanitizer.php +++ b/tests/test-class-amp-base-sanitizer.php @@ -245,26 +245,23 @@ public function test_enforce_sizes_attribute( $source_params, $expected_value, $ /** * Tests remove_child. * - * @see AMP_Base_Sanitizer::remove_invalid_child() + * @covers AMP_Base_Sanitizer::remove_invalid_child() */ public function test_remove_child() { $parent_tag_name = 'div'; - $child_tag_name = 'h1'; $dom_document = new DOMDocument( '1.0', 'utf-8' ); $parent = $dom_document->createElement( $parent_tag_name ); $child = $dom_document->createElement( 'h1' ); $parent->appendChild( $child ); - // To ignore WordPress.NamingConventions.ValidVariableName.NotSnakeCaseMemberVar. - // @codingStandardsIgnoreStart - $this->assertEquals( $child, $parent->firstChild ); $sanitizer = new AMP_Iframe_Sanitizer( $dom_document, array( 'remove_invalid_callback' => 'AMP_Validation_Utils::track_removed', ) ); $sanitizer->remove_invalid_child( $child ); $this->assertEquals( null, $parent->firstChild ); - $this->assertEquals( 1, AMP_Validation_Utils::$removed_nodes[ $child_tag_name ] ); + $this->assertCount( 1, AMP_Validation_Utils::$removed_nodes ); + $this->assertEquals( $child, AMP_Validation_Utils::$removed_nodes[0] ); $parent->appendChild( $child ); $this->assertEquals( $child, $parent->firstChild ); @@ -272,14 +269,13 @@ public function test_remove_child() { $this->assertEquals( null, $parent->firstChild ); $this->assertEquals( null, $child->parentNode ); - // @codingStandardsIgnoreEnd AMP_Validation_Utils::$removed_nodes = null; } /** * Tests remove_child. * - * @see AMP_Base_Sanitizer::remove_invalid_child() + * @covers AMP_Base_Sanitizer::remove_invalid_child() */ public function test_remove_attribute() { AMP_Validation_Utils::reset_removed(); @@ -288,20 +284,15 @@ public function test_remove_attribute() { $dom_document = new DOMDocument( '1.0', 'utf-8' ); $video = $dom_document->createElement( $video_name ); $video->setAttribute( $attribute, 'someFunction()' ); + $attr_node = $video->getAttributeNode( $attribute ); - // To ignore WordPress.NamingConventions.ValidVariableName.NotSnakeCaseMemberVar. - // @codingStandardsIgnoreStart - $args = array( + $args = array( 'remove_invalid_callback' => 'AMP_Validation_Utils::track_removed', ); - $expected_removed = array( - $attribute => 1, - ); $sanitizer = new AMP_Video_Sanitizer( $dom_document, $args ); $sanitizer->remove_invalid_attribute( $video, $attribute ); $this->assertEquals( null, $video->getAttribute( $attribute ) ); - $this->assertEquals( $expected_removed, AMP_Validation_Utils::$removed_attributes ); - // @codingStandardsIgnoreEnd + $this->assertEquals( array( $attr_node ), AMP_Validation_Utils::$removed_nodes ); AMP_Validation_Utils::reset_removed(); } diff --git a/tests/test-class-amp-theme-support.php b/tests/test-class-amp-theme-support.php index b71c4d41228..eacee79e26f 100644 --- a/tests/test-class-amp-theme-support.php +++ b/tests/test-class-amp-theme-support.php @@ -101,7 +101,7 @@ public function test_prepare_response() { > - + @@ -114,12 +114,19 @@ public function test_prepare_response() { data-aax_src="302"> + + function( $node ) use ( &$removed_nodes ) { + $removed_nodes[ $node->nodeName ] = $node; + }, + ) ); $this->assertContains( '', $sanitized_html ); $this->assertContains( '', $sanitized_html ); @@ -136,6 +143,11 @@ public function test_prepare_response() { $this->assertContains( '