From bdd1be9b67a4fd2136763e5ea614aab09918ac46 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 27 Aug 2021 23:43:17 -0700 Subject: [PATCH 01/16] Initial success with comment reply links --- includes/class-amp-theme-support.php | 21 +- .../class-amp-comments-sanitizer.php | 205 ++++++++++++++---- .../sanitizers/class-amp-script-sanitizer.php | 9 +- src/SandboxingLevels.php | 7 +- 4 files changed, 190 insertions(+), 52 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 234697c9e65..15e44beb69d 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -900,10 +900,10 @@ static function( $html ) { // Commenting hooks. // @todo When custom scripts appear on the page, this logic should be skipped. To do so, this would require moving the logic to the AMP_Comments_Sanitizer. - add_filter( 'comment_form_defaults', [ __CLASS__, 'filter_comment_form_defaults' ], PHP_INT_MAX ); - add_filter( 'comment_reply_link', [ __CLASS__, 'filter_comment_reply_link' ], 10, 4 ); - add_filter( 'cancel_comment_reply_link', [ __CLASS__, 'filter_cancel_comment_reply_link' ], 10, 3 ); - remove_action( 'comment_form', 'wp_comment_form_unfiltered_html_nonce' ); +// add_filter( 'comment_form_defaults', [ __CLASS__, 'filter_comment_form_defaults' ], PHP_INT_MAX ); +// add_filter( 'comment_reply_link', [ __CLASS__, 'filter_comment_reply_link' ], 10, 4 ); +// add_filter( 'cancel_comment_reply_link', [ __CLASS__, 'filter_cancel_comment_reply_link' ], 10, 3 ); +// remove_action( 'comment_form', 'wp_comment_form_unfiltered_html_nonce' ); add_filter( 'wp_kses_allowed_html', [ __CLASS__, 'include_layout_in_wp_kses_allowed_html' ], 10 ); add_filter( 'get_header_image_tag', [ __CLASS__, 'amend_header_image_with_video_header' ], PHP_INT_MAX ); add_action( @@ -913,12 +913,12 @@ static function() { }, 0 ); - add_action( - 'wp_enqueue_scripts', - static function() { - wp_dequeue_script( 'comment-reply' ); // Handled largely by AMP_Comments_Sanitizer and *reply* methods in this class. - } - ); +// add_action( +// 'wp_enqueue_scripts', +// static function() { +// wp_dequeue_script( 'comment-reply' ); // Handled largely by AMP_Comments_Sanitizer and *reply* methods in this class. +// } +// ); } /** @@ -1097,6 +1097,7 @@ public static function filter_comment_form_defaults( $default_args ) { } } + // @todo See . $state_id = self::get_comment_form_state_id( get_the_ID() ); $text_binding = sprintf( '%s.replyToName ? %s : %s', diff --git a/includes/sanitizers/class-amp-comments-sanitizer.php b/includes/sanitizers/class-amp-comments-sanitizer.php index 2c4bda37f62..480b099b750 100644 --- a/includes/sanitizers/class-amp-comments-sanitizer.php +++ b/includes/sanitizers/class-amp-comments-sanitizer.php @@ -6,6 +6,9 @@ */ use AmpProject\Amp; +use AmpProject\Attribute; +use AmpProject\Extension; +use AmpProject\Tag; /** * Class AMP_Comments_Sanitizer @@ -24,30 +27,34 @@ class AMP_Comments_Sanitizer extends AMP_Base_Sanitizer { * @var array */ protected $DEFAULT_ARGS = [ - 'comment_live_list' => false, + 'comment_live_list' => false, // @todo See . + 'comment_reply_js_printed' => false, ]; /** * Pre-process the comment form and comment list for AMP. * + * @todo Fix https://github.com/ampproject/amp-wp/issues/6231 + * * @since 0.7 */ public function sanitize() { - foreach ( $this->dom->getElementsByTagName( 'form' ) as $comment_form ) { + // @todo Check if the comment-reply script was printed. + // @todo Add defer to comment-reply if the script _is_ on the page (and there were no dependencies). + + foreach ( $this->dom->getElementsByTagName( Tag::FORM ) as $comment_form ) { // Skip processing comment forms which have opted-out of conversion to amp-form. // Note that AMP_Form_Sanitizer runs before AMP_Comments_Sanitizer according to amp_get_content_sanitizers(). - if ( $comment_form->hasAttribute( 'action' ) ) { - continue; - } +// if ( $comment_form->hasAttribute( 'action' ) ) { +// continue; +// } - /** - * Comment form. - * - * @var DOMElement $comment_form - */ - $action = $comment_form->getAttribute( 'action-xhr' ); + $action = $comment_form->getAttribute( Attribute::ACTION_XHR ); +// if ( ! $action ) { +// $action = $comment_form->getAttribute( Attribute::ACTION ); +// } $action_path = wp_parse_url( $action, PHP_URL_PATH ); - if ( 'wp-comments-post.php' === basename( $action_path ) ) { + if ( $action_path && 'wp-comments-post.php' === basename( $action_path ) ) { $this->process_comment_form( $comment_form ); } } @@ -61,6 +68,16 @@ public function sanitize() { } } + /** + * Get the ID for the amp-state. + * + * @param int $post_id Post ID. + * @return string ID for amp-state. + */ + protected function get_comment_form_state_id( $post_id ) { + return sprintf( 'commentform_post_%d', $post_id ); + } + /** * Comment form. * @@ -70,16 +87,17 @@ public function sanitize() { */ protected function process_comment_form( $comment_form ) { $form_fields = []; - foreach ( $comment_form->getElementsByTagName( 'input' ) as $element ) { + foreach ( $comment_form->getElementsByTagName( Tag::INPUT ) as $element ) { /** @var DOMElement $element */ - $name = $element->getAttribute( 'name' ); + $name = $element->getAttribute( Attribute::NAME ); if ( $name ) { $form_fields[ $name ][] = $element; } } - foreach ( $comment_form->getElementsByTagName( 'textarea' ) as $element ) { + + foreach ( $comment_form->getElementsByTagName( Tag::TEXTAREA ) as $element ) { /** @var DOMElement $element */ - $name = $element->getAttribute( 'name' ); + $name = $element->getAttribute( Attribute::NAME ); if ( $name ) { $form_fields[ $name ][] = $element; } @@ -93,22 +111,25 @@ protected function process_comment_form( $comment_form ) { if ( empty( $form_fields['comment_post_ID'] ) ) { return; } - $post_id = (int) $form_fields['comment_post_ID'][0]->getAttribute( 'value' ); - $state_id = AMP_Theme_Support::get_comment_form_state_id( $post_id ); + $post_id = (int) $form_fields['comment_post_ID'][0]->getAttribute( Attribute::VALUE ); + $state_id = $this->get_comment_form_state_id( $post_id ); $form_state = [ - 'values' => [], - 'submitting' => false, - 'replyToName' => '', + 'values' => [], + 'submitting' => false, + 'replyTo' => '', // @todo This should rather be just replyToText. ]; + $comment_parent_id = null; if ( ! empty( $form_fields['comment_parent'] ) ) { - $comment_id = (int) $form_fields['comment_parent'][0]->getAttribute( 'value' ); + $comment_parent_id = (int) $form_fields['comment_parent'][0]->getAttribute( Attribute::VALUE ); + + // @todo Obtain replyTo? if ( $comment_id ) { - $reply_comment = get_comment( $comment_id ); - if ( $reply_comment ) { - $form_state['replyToName'] = $reply_comment->comment_author; - } +// $reply_comment = get_comment( $comment_id ); +// if ( $reply_comment ) { +// $form_state['replyToName'] = $reply_comment->comment_author; +// } } } @@ -117,23 +138,23 @@ protected function process_comment_form( $comment_form ) { foreach ( $form_field as $element ) { // @todo Radio and checkbox inputs are not supported yet. - if ( in_array( strtolower( $element->getAttribute( 'type' ) ), [ 'checkbox', 'radio' ], true ) ) { + if ( in_array( strtolower( $element->getAttribute( Attribute::TYPE ) ), [ 'checkbox', 'radio' ], true ) ) { continue; } - $element->setAttribute( sprintf( $amp_bind_attr_format, 'disabled' ), "$state_id.submitting" ); + $element->setAttribute( sprintf( $amp_bind_attr_format, Attribute::DISABLED ), "$state_id.submitting" ); - if ( 'textarea' === strtolower( $element->nodeName ) ) { + if ( Tag::TEXTAREA === strtolower( $element->nodeName ) ) { $form_state['values'][ $name ] = $element->textContent; $element->setAttribute( sprintf( $amp_bind_attr_format, 'text' ), "$state_id.values.$name" ); } else { - $form_state['values'][ $name ] = $element->hasAttribute( 'value' ) ? $element->getAttribute( 'value' ) : ''; - $element->setAttribute( sprintf( $amp_bind_attr_format, 'value' ), "$state_id.values.$name" ); + $form_state['values'][ $name ] = $element->hasAttribute( Attribute::VALUE ) ? $element->getAttribute( Attribute::VALUE ) : ''; + $element->setAttribute( sprintf( $amp_bind_attr_format, Attribute::VALUE ), "$state_id.values.$name" ); } // Update the state in response to changing the input. $element->setAttribute( - 'on', + Attribute::ON, sprintf( 'change:AMP.setState( { %s: { values: { %s: event.value } } } )', $state_id, @@ -144,12 +165,10 @@ protected function process_comment_form( $comment_form ) { } // Add amp-state to the document. - $amp_state = $this->dom->createElement( 'amp-state' ); - $amp_state->setAttribute( 'id', $state_id ); - $script = $this->dom->createElement( 'script' ); - $script->setAttribute( 'type', 'application/json' ); - $amp_state->appendChild( $script ); - $script->appendChild( $this->dom->createTextNode( wp_json_encode( $form_state, JSON_UNESCAPED_UNICODE ) ) ); + $amp_state = $this->dom->createElement( Extension::STATE ); + $amp_state->setAttribute( Attribute::ID, $state_id ); + $script = $this->dom->createElement( Tag::SCRIPT ); + $script->setAttribute( Attribute::TYPE, 'application/json' ); $comment_form->insertBefore( $amp_state, $comment_form->firstChild ); // Update state when submitting form. @@ -177,7 +196,115 @@ protected function process_comment_form( $comment_form ) { wp_json_encode( $form_reset_state, JSON_UNESCAPED_UNICODE ) ), ]; - $comment_form->setAttribute( 'on', implode( ';', $on ) ); + $comment_form->setAttribute( Attribute::ON, implode( ';', $on ) ); + + // @todo DO the filter_comment_form_defaults. + $reply_heading_element = $this->dom->getElementById( 'reply-title' ); + $reply_heading_text_node = null; + $reply_link_to_parent = null; + if ( $reply_heading_element && $reply_heading_element->firstChild instanceof DOMText ) { + $reply_heading_text_node = $reply_heading_element->firstChild; + } + if ( $reply_heading_text_node && $reply_heading_text_node->nextSibling instanceof DOMElement ) { + $reply_link_to_parent = $reply_heading_text_node->nextSibling; + } + if ( $reply_heading_text_node && $reply_link_to_parent ) { + $reply_heading_text_span = $this->dom->createElement( Tag::SPAN ); + $reply_heading_element->replaceChild( $reply_heading_text_span, $reply_heading_text_node ); + $reply_heading_text_span->appendChild( $reply_heading_text_node ); + + // Move whitespace after the node. + $reply_heading_text_node->nodeValue = rtrim( $reply_heading_text_node->nodeValue ); + $reply_heading_element->insertBefore( + $this->dom->createTextNode( ' ' ), + $reply_heading_text_span->nextSibling + ); + + // @todo Consider replytocom. $comment_parent_id + $text_binding = sprintf( + '%1$s.replyTo ? %1$s.replyTo : %2$s', + $state_id, +// str_replace( +// '%s', +// sprintf( '" + %s.replyToName + "', $state_id ), +// wp_json_encode( $default_args['title_reply_to'], JSON_UNESCAPED_UNICODE ) +// ), + wp_json_encode( $reply_heading_text_node->nodeValue, JSON_UNESCAPED_UNICODE ) + ); + + $reply_heading_text_span->setAttribute( + Amp::BIND_DATA_ATTR_PREFIX . 'text', + $text_binding + ); + } + + // Populate amp-state. + $amp_state->appendChild( $script ); + $script->appendChild( $this->dom->createTextNode( wp_json_encode( $form_state, JSON_UNESCAPED_UNICODE ) ) ); + + $comment_reply_links = $this->dom->xpath->query( '//a[ @data-commentid and @data-postid and @data-replyto and @data-respondelement and contains( @class, "comment-reply-link" ) ]' ); + foreach ( $comment_reply_links as $comment_reply_link ) { + /** @var DOMElement $comment_reply_link */ + + $comment_reply_state = [ + 'replyTo' => $comment_reply_link->getAttribute( 'data-replyto' ), + 'values' => [ + 'comment_parent' => $comment_reply_link->getAttribute( 'data-commentid' ), + ], + ]; + + $comment_reply_link->setAttribute( + Attribute::HREF, + '#' . $comment_reply_link->getAttribute( 'data-respondelement' ) + ); + + $comment_reply_link->setAttribute( + Attribute::ON, + sprintf( + 'tap:AMP.setState({%s:%s})', + wp_json_encode( $state_id ), + wp_json_encode( $comment_reply_state, JSON_UNESCAPED_UNICODE ) + ) + ); + } + + $cancel_comment_reply_link = $this->dom->getElementById( 'cancel-comment-reply-link' ); + if ( $cancel_comment_reply_link instanceof DOMElement ) { + $cancel_comment_reply_link->removeAttribute( Attribute::STYLE ); + + if ( ! $comment_parent_id ) { + $cancel_comment_reply_link->setAttributeNode( $this->dom->createAttribute( Attribute::HIDDEN ) ); + } + + $tap_state = [ + $state_id => [ + 'replyTo' => '', + 'values' => [ + 'comment_parent' => '0', + ], + ], + ]; + $cancel_comment_reply_link->setAttribute( + Attribute::ON, + sprintf( 'tap:AMP.setState( %s )', wp_json_encode( $tap_state, JSON_UNESCAPED_UNICODE ) ) + ); + + $cancel_comment_reply_link->setAttribute( + Amp::BIND_DATA_ATTR_PREFIX . Attribute::HIDDEN, + sprintf( '%s.values.comment_parent == "0"', $state_id ) + ); + } + + +// $reply_heading_text_node = +// if ( $reply_heading_element ) { +// +// +// var replyHeadingElement = getElementById( config.commentReplyTitleId ); +// var replyHeadingTextNode = replyHeadingElement && replyHeadingElement.firstChild; +// var replyLinkToParent = replyHeadingTextNode && replyHeadingTextNode.nextSibling; +// +// } } /** diff --git a/includes/sanitizers/class-amp-script-sanitizer.php b/includes/sanitizers/class-amp-script-sanitizer.php index 9f59965653c..01b6648f35e 100644 --- a/includes/sanitizers/class-amp-script-sanitizer.php +++ b/includes/sanitizers/class-amp-script-sanitizer.php @@ -54,8 +54,10 @@ class AMP_Script_Sanitizer extends AMP_Base_Sanitizer { * @var array */ protected $DEFAULT_ARGS = [ - 'unwrap_noscripts' => true, - 'sanitize_js_scripts' => false, + 'unwrap_noscripts' => true, + 'sanitize_js_scripts' => false, + 'allow_comment_reply_js' => false, // @todo Support this. + // @todo Have a callback which indicates whether scripts are bento-friendly. The comment-reply is one such script which is allowed in moderate. ]; /** @@ -104,6 +106,7 @@ public function init( $sanitizers ) { */ public function sanitize() { if ( ! empty( $this->args['sanitize_js_scripts'] ) ) { + // @todo This should automatically add defer to script#comment-reply-js if no other scripts had it as a dependency. $this->sanitize_js_script_elements(); } @@ -118,8 +121,10 @@ public function sanitize() { // associated style rules. // @todo There should be an attribute on script tags that opt-in to keeping tree shaking and/or to indicate what class names need to be included. // @todo Depending on the size of the underlying stylesheets, this may need to retain the use of external styles to prevent inlining excessive CSS. This may involve writing minified CSS to disk, or skipping style processing altogether if no selector conversions are needed. + // @todo Prevent inclusion of comment-reply.js alone from causing this. if ( $this->kept_script_count > 0 ) { $sanitizer_arg_updates = [ + // @todo If comment-reply.js was on the page (likely best detected via wp_script_is('comment-reply', 'done')), then AMP_Comment_Sanitizer should know. AMP_Style_Sanitizer::class => [ 'skip_tree_shaking' => true ], AMP_Img_Sanitizer::class => [ 'native_img_used' => true ], AMP_Video_Sanitizer::class => [ 'native_video_used' => true ], diff --git a/src/SandboxingLevels.php b/src/SandboxingLevels.php index c2630298903..caa89b3d2ff 100644 --- a/src/SandboxingLevels.php +++ b/src/SandboxingLevels.php @@ -142,8 +142,13 @@ public function add_hooks() { // Opt-in to the new script sanitization logic in the script sanitizer. add_filter( 'amp_content_sanitizers', - static function ( $sanitizers ) { + static function ( $sanitizers ) use ( $sandboxing_level ) { $sanitizers[ AMP_Script_Sanitizer::class ]['sanitize_js_scripts'] = true; + + // @todo This: + // if ( $sandboxing_level < 3 ) { + // $sanitizers[ AMP_Script_Sanitizer::class ]['allow_comment_reply_js'] = true; + // } return $sanitizers; } ); From d8a41b1a5218fcb239aaa55e82fbb078d8e89b7b Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Sat, 28 Aug 2021 00:10:50 -0700 Subject: [PATCH 02/16] Simplify state --- includes/sanitizers/class-amp-comments-sanitizer.php | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/includes/sanitizers/class-amp-comments-sanitizer.php b/includes/sanitizers/class-amp-comments-sanitizer.php index 480b099b750..b2f6c0eb599 100644 --- a/includes/sanitizers/class-amp-comments-sanitizer.php +++ b/includes/sanitizers/class-amp-comments-sanitizer.php @@ -247,9 +247,11 @@ protected function process_comment_form( $comment_form ) { /** @var DOMElement $comment_reply_link */ $comment_reply_state = [ - 'replyTo' => $comment_reply_link->getAttribute( 'data-replyto' ), - 'values' => [ - 'comment_parent' => $comment_reply_link->getAttribute( 'data-commentid' ), + $state_id => [ + 'replyTo' => $comment_reply_link->getAttribute( 'data-replyto' ), + 'values' => [ + 'comment_parent' => $comment_reply_link->getAttribute( 'data-commentid' ), + ], ], ]; @@ -261,8 +263,7 @@ protected function process_comment_form( $comment_form ) { $comment_reply_link->setAttribute( Attribute::ON, sprintf( - 'tap:AMP.setState({%s:%s})', - wp_json_encode( $state_id ), + 'tap:AMP.setState(%s)', wp_json_encode( $comment_reply_state, JSON_UNESCAPED_UNICODE ) ) ); From 7be0ee6d7bac09ce8714a8001f77a5b216b93a6a Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Sat, 28 Aug 2021 00:13:38 -0700 Subject: [PATCH 03/16] Focus on comment textarea when clicking on reply --- includes/sanitizers/class-amp-comments-sanitizer.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/includes/sanitizers/class-amp-comments-sanitizer.php b/includes/sanitizers/class-amp-comments-sanitizer.php index b2f6c0eb599..6b498b701e6 100644 --- a/includes/sanitizers/class-amp-comments-sanitizer.php +++ b/includes/sanitizers/class-amp-comments-sanitizer.php @@ -263,7 +263,7 @@ protected function process_comment_form( $comment_form ) { $comment_reply_link->setAttribute( Attribute::ON, sprintf( - 'tap:AMP.setState(%s)', + 'tap:AMP.setState(%s),comment.focus', wp_json_encode( $comment_reply_state, JSON_UNESCAPED_UNICODE ) ) ); From 78f58853b5d108b6364d4f4f7a66227509866240 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Sat, 28 Aug 2021 00:34:32 -0700 Subject: [PATCH 04/16] Remove commented-out code and add deprecations --- includes/class-amp-theme-support.php | 25 ++++++--------- .../class-amp-comments-sanitizer.php | 32 +++---------------- tests/php/test-class-amp-theme-support.php | 10 ++++-- 3 files changed, 22 insertions(+), 45 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 15e44beb69d..482c66ec8ab 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -898,14 +898,9 @@ static function( $html ) { $priority = defined( 'PHP_INT_MIN' ) ? PHP_INT_MIN : ~PHP_INT_MAX; // phpcs:ignore PHPCompatibility.Constants.NewConstants.php_int_minFound add_action( 'template_redirect', [ __CLASS__, 'start_output_buffering' ], $priority ); - // Commenting hooks. - // @todo When custom scripts appear on the page, this logic should be skipped. To do so, this would require moving the logic to the AMP_Comments_Sanitizer. -// add_filter( 'comment_form_defaults', [ __CLASS__, 'filter_comment_form_defaults' ], PHP_INT_MAX ); -// add_filter( 'comment_reply_link', [ __CLASS__, 'filter_comment_reply_link' ], 10, 4 ); -// add_filter( 'cancel_comment_reply_link', [ __CLASS__, 'filter_cancel_comment_reply_link' ], 10, 3 ); -// remove_action( 'comment_form', 'wp_comment_form_unfiltered_html_nonce' ); add_filter( 'wp_kses_allowed_html', [ __CLASS__, 'include_layout_in_wp_kses_allowed_html' ], 10 ); add_filter( 'get_header_image_tag', [ __CLASS__, 'amend_header_image_with_video_header' ], PHP_INT_MAX ); + remove_action( 'comment_form', 'wp_comment_form_unfiltered_html_nonce' ); // @todo The AMP_Comments_Sanitizer should add data-ampdevmode since user would be logged-in. add_action( 'wp_print_footer_scripts', static function() { @@ -913,12 +908,6 @@ static function() { }, 0 ); -// add_action( -// 'wp_enqueue_scripts', -// static function() { -// wp_dequeue_script( 'comment-reply' ); // Handled largely by AMP_Comments_Sanitizer and *reply* methods in this class. -// } -// ); } /** @@ -1058,11 +1047,13 @@ public static function get_current_canonical_url() { * Get the ID for the amp-state. * * @since 0.7 + * @deprecated Logic moved to AMP_Comments_Sanitizer. * * @param int $post_id Post ID. * @return string ID for amp-state. */ public static function get_comment_form_state_id( $post_id ) { + _deprecated_function( __METHOD__, '2.2' ); return sprintf( 'commentform_post_%d', $post_id ); } @@ -1071,12 +1062,13 @@ public static function get_comment_form_state_id( $post_id ) { * * @since 0.7 * @see comment_form() - * @todo When custom scripts appear on the page, this logic should be skipped. To do so, this would require moving the logic to the AMP_Comments_Sanitizer. + * @deprecated Logic moved to AMP_Comments_Sanitizer. * * @param array $default_args Comment form arg defaults. * @return array Filtered comment form args. */ public static function filter_comment_form_defaults( $default_args ) { + _deprecated_function( __METHOD__, '2.2' ); // Obtain the actual args provided to the comment_form() function since it is not available in the filter. $args = []; @@ -1123,7 +1115,7 @@ public static function filter_comment_form_defaults( $default_args ) { * * @since 0.7 * @see get_comment_reply_link() - * @todo When custom scripts appear on the page, this logic should be skipped. To do so, this would require moving the logic to the AMP_Comments_Sanitizer. + * @deprecated Logic moved to AMP_Comments_Sanitizer. * * @param string $link The HTML markup for the comment reply link. * @param array $args An array of arguments overriding the defaults. @@ -1131,6 +1123,7 @@ public static function filter_comment_form_defaults( $default_args ) { * @return string Comment reply link. */ public static function filter_comment_reply_link( $link, $args, $comment ) { + _deprecated_function( __METHOD__, '2.2' ); // Continue to show default link to wp-login when user is not logged-in. if ( get_option( 'comment_registration' ) && ! is_user_logged_in() ) { @@ -1163,7 +1156,7 @@ public static function filter_comment_reply_link( $link, $args, $comment ) { * * @since 0.7 * @see get_cancel_comment_reply_link() - * @todo When custom scripts appear on the page, this logic should be skipped. To do so, this would require moving the logic to the AMP_Comments_Sanitizer. + * @deprecated Logic moved to AMP_Comments_Sanitizer. * * @param string $formatted_link The HTML-formatted cancel comment reply link. * @param string $link Cancel comment reply link URL. @@ -1171,6 +1164,8 @@ public static function filter_comment_reply_link( $link, $args, $comment ) { * @return string Cancel reply link. */ public static function filter_cancel_comment_reply_link( $formatted_link, $link, $text ) { + _deprecated_function( __METHOD__, '2.2' ); + if ( empty( $text ) ) { $text = __( 'Click here to cancel reply.', 'default' ); } diff --git a/includes/sanitizers/class-amp-comments-sanitizer.php b/includes/sanitizers/class-amp-comments-sanitizer.php index 6b498b701e6..a8fca91d27e 100644 --- a/includes/sanitizers/class-amp-comments-sanitizer.php +++ b/includes/sanitizers/class-amp-comments-sanitizer.php @@ -15,6 +15,7 @@ * * Strips and corrects attributes in forms. * + * @todo When comment-reply is on the page, this should be skipped. * @internal */ class AMP_Comments_Sanitizer extends AMP_Base_Sanitizer { @@ -117,20 +118,12 @@ protected function process_comment_form( $comment_form ) { $form_state = [ 'values' => [], 'submitting' => false, - 'replyTo' => '', // @todo This should rather be just replyToText. + 'replyTo' => '', ]; $comment_parent_id = null; if ( ! empty( $form_fields['comment_parent'] ) ) { $comment_parent_id = (int) $form_fields['comment_parent'][0]->getAttribute( Attribute::VALUE ); - - // @todo Obtain replyTo? - if ( $comment_id ) { -// $reply_comment = get_comment( $comment_id ); -// if ( $reply_comment ) { -// $form_state['replyToName'] = $reply_comment->comment_author; -// } - } } $amp_bind_attr_format = Amp::BIND_DATA_ATTR_PREFIX . '%s'; @@ -198,7 +191,6 @@ protected function process_comment_form( $comment_form ) { ]; $comment_form->setAttribute( Attribute::ON, implode( ';', $on ) ); - // @todo DO the filter_comment_form_defaults. $reply_heading_element = $this->dom->getElementById( 'reply-title' ); $reply_heading_text_node = null; $reply_link_to_parent = null; @@ -220,15 +212,12 @@ protected function process_comment_form( $comment_form ) { $reply_heading_text_span->nextSibling ); - // @todo Consider replytocom. $comment_parent_id + // Note: if the replytocom query parameter was set, then the existing value will already be a replyTo value. + // Nevertheless, the link will have the replytocom arg removed, so clicking on the link will cause + // navigation to a page that has the nameless heading text. $text_binding = sprintf( '%1$s.replyTo ? %1$s.replyTo : %2$s', $state_id, -// str_replace( -// '%s', -// sprintf( '" + %s.replyToName + "', $state_id ), -// wp_json_encode( $default_args['title_reply_to'], JSON_UNESCAPED_UNICODE ) -// ), wp_json_encode( $reply_heading_text_node->nodeValue, JSON_UNESCAPED_UNICODE ) ); @@ -295,17 +284,6 @@ protected function process_comment_form( $comment_form ) { sprintf( '%s.values.comment_parent == "0"', $state_id ) ); } - - -// $reply_heading_text_node = -// if ( $reply_heading_element ) { -// -// -// var replyHeadingElement = getElementById( config.commentReplyTitleId ); -// var replyHeadingTextNode = replyHeadingElement && replyHeadingElement.firstChild; -// var replyLinkToParent = replyHeadingTextNode && replyHeadingTextNode.nextSibling; -// -// } } /** diff --git a/tests/php/test-class-amp-theme-support.php b/tests/php/test-class-amp-theme-support.php index a518b4c3725..9ad3d4631f0 100644 --- a/tests/php/test-class-amp-theme-support.php +++ b/tests/php/test-class-amp-theme-support.php @@ -779,9 +779,6 @@ public function test_add_hooks() { $priority = defined( 'PHP_INT_MIN' ) ? PHP_INT_MIN : ~PHP_INT_MAX; // phpcs:ignore PHPCompatibility.Constants.NewConstants.php_int_minFound $this->assertEquals( $priority, has_action( 'template_redirect', [ self::TESTED_CLASS, 'start_output_buffering' ] ) ); - $this->assertEquals( PHP_INT_MAX, has_filter( 'comment_form_defaults', [ self::TESTED_CLASS, 'filter_comment_form_defaults' ] ) ); - $this->assertEquals( 10, has_filter( 'comment_reply_link', [ self::TESTED_CLASS, 'filter_comment_reply_link' ] ) ); - $this->assertEquals( 10, has_filter( 'cancel_comment_reply_link', [ self::TESTED_CLASS, 'filter_cancel_comment_reply_link' ] ) ); $this->assertFalse( has_action( 'comment_form', 'wp_comment_form_unfiltered_html_nonce' ) ); $this->assertEquals( PHP_INT_MAX, has_filter( 'get_header_image_tag', [ self::TESTED_CLASS, 'amend_header_image_with_video_header' ] ) ); } @@ -881,6 +878,7 @@ public function test_get_current_canonical_url_in_paired_amp() { * Test get_comment_form_state_id. * * @covers AMP_Theme_Support::get_comment_form_state_id() + * @expectedDeprecated AMP_Theme_Support::get_comment_form_state_id */ public function test_get_comment_form_state_id() { $post_id = 54; @@ -893,6 +891,8 @@ public function test_get_comment_form_state_id() { * Test filter_comment_form_defaults. * * @covers AMP_Theme_Support::filter_comment_form_defaults() + * @expectedDeprecated AMP_Theme_Support::filter_comment_form_defaults + * @expectedDeprecated AMP_Theme_Support::get_comment_form_state_id */ public function test_filter_comment_form_defaults() { global $post; @@ -914,6 +914,8 @@ public function test_filter_comment_form_defaults() { * Test filter_comment_reply_link. * * @covers AMP_Theme_Support::filter_comment_reply_link() + * @expectedDeprecated AMP_Theme_Support::filter_comment_reply_link + * @expectedDeprecated AMP_Theme_Support::get_comment_form_state_id */ public function test_filter_comment_reply_link() { global $post; @@ -949,6 +951,8 @@ public function test_filter_comment_reply_link() { * Test filter_cancel_comment_reply_link. * * @covers AMP_Theme_Support::filter_cancel_comment_reply_link() + * @expectedDeprecated AMP_Theme_Support::filter_cancel_comment_reply_link + * @expectedDeprecated AMP_Theme_Support::get_comment_form_state_id */ public function test_filter_cancel_comment_reply_link() { global $post; From a89fcd62b44faaedee1cc4d33b687a4373c81792 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Sat, 28 Aug 2021 22:34:53 -0700 Subject: [PATCH 05/16] Update comment --- includes/class-amp-theme-support.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 482c66ec8ab..04c59114a17 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -900,7 +900,7 @@ static function( $html ) { add_filter( 'wp_kses_allowed_html', [ __CLASS__, 'include_layout_in_wp_kses_allowed_html' ], 10 ); add_filter( 'get_header_image_tag', [ __CLASS__, 'amend_header_image_with_video_header' ], PHP_INT_MAX ); - remove_action( 'comment_form', 'wp_comment_form_unfiltered_html_nonce' ); // @todo The AMP_Comments_Sanitizer should add data-ampdevmode since user would be logged-in. + remove_action( 'comment_form', 'wp_comment_form_unfiltered_html_nonce' ); // @todo The AMP_Comments_Sanitizer should add data-ampdevmode since user would be logged-in, or else the this script should be allowed whenever comment-reply is also allowed. add_action( 'wp_print_footer_scripts', static function() { @@ -1089,7 +1089,6 @@ public static function filter_comment_form_defaults( $default_args ) { } } - // @todo See . $state_id = self::get_comment_form_state_id( get_the_ID() ); $text_binding = sprintf( '%s.replyToName ? %s : %s', From d20d39f1fbbdb6797c6dc4c67cc0ac13dd8fa603 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 1 Sep 2021 14:25:58 -0700 Subject: [PATCH 06/16] Mark script output from wp_comment_form_unfiltered_html_nonce() as being in dev mode --- includes/amp-helper-functions.php | 5 +++++ includes/class-amp-theme-support.php | 1 - tests/php/test-class-amp-theme-support.php | 1 - 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index 4fbea71e314..0647024bc1a 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -1595,6 +1595,11 @@ function amp_get_content_sanitizers( $post = null ) { */ $dev_mode_xpaths = (array) apply_filters( 'amp_dev_mode_element_xpaths', [] ); + // Prevent removal of script output by wp_comment_form_unfiltered_html_nonce(). + if ( current_user_can( 'unfiltered_html' ) ) { + $dev_mode_xpaths[] = '//script[ preceding-sibling::input[ @name = "_wp_unfiltered_html_comment_disabled" ] and contains( text(), "_wp_unfiltered_html_comment_disabled" ) ]'; + } + if ( is_admin_bar_showing() ) { $dev_mode_xpaths[] = '//*[ @id = "wpadminbar" ]'; $dev_mode_xpaths[] = '//*[ @id = "wpadminbar" ]//*'; diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 04c59114a17..733762b28df 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -900,7 +900,6 @@ static function( $html ) { add_filter( 'wp_kses_allowed_html', [ __CLASS__, 'include_layout_in_wp_kses_allowed_html' ], 10 ); add_filter( 'get_header_image_tag', [ __CLASS__, 'amend_header_image_with_video_header' ], PHP_INT_MAX ); - remove_action( 'comment_form', 'wp_comment_form_unfiltered_html_nonce' ); // @todo The AMP_Comments_Sanitizer should add data-ampdevmode since user would be logged-in, or else the this script should be allowed whenever comment-reply is also allowed. add_action( 'wp_print_footer_scripts', static function() { diff --git a/tests/php/test-class-amp-theme-support.php b/tests/php/test-class-amp-theme-support.php index 9ad3d4631f0..93bb72e8842 100644 --- a/tests/php/test-class-amp-theme-support.php +++ b/tests/php/test-class-amp-theme-support.php @@ -779,7 +779,6 @@ public function test_add_hooks() { $priority = defined( 'PHP_INT_MIN' ) ? PHP_INT_MIN : ~PHP_INT_MAX; // phpcs:ignore PHPCompatibility.Constants.NewConstants.php_int_minFound $this->assertEquals( $priority, has_action( 'template_redirect', [ self::TESTED_CLASS, 'start_output_buffering' ] ) ); - $this->assertFalse( has_action( 'comment_form', 'wp_comment_form_unfiltered_html_nonce' ) ); $this->assertEquals( PHP_INT_MAX, has_filter( 'get_header_image_tag', [ self::TESTED_CLASS, 'amend_header_image_with_video_header' ] ) ); } From 7c37319f445147d5ec24e9ca8e085158da70d654 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 1 Sep 2021 17:56:41 -0700 Subject: [PATCH 07/16] Run core theme sanitizer before script sanitizer --- includes/amp-helper-functions.php | 15 ++++++++------- tests/php/test-amp-helper-functions.php | 4 ++++ 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index 0647024bc1a..8c4ea9e8184 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -1491,12 +1491,6 @@ function amp_get_content_sanitizers( $post = null ) { AMP_O2_Player_Sanitizer::class => [], AMP_Playbuzz_Sanitizer::class => [], - // The AMP_Script_Sanitizer runs here because based on whether it allows custom scripts - // to be kept, it may impact the behavior of other sanitizers. For example, if custom - // scripts are kept then this is a signal that tree shaking in AMP_Style_Sanitizer cannot be - // performed. - AMP_Script_Sanitizer::class => [], - AMP_Core_Theme_Sanitizer::class => [ 'template' => get_template(), 'stylesheet' => get_stylesheet(), @@ -1505,6 +1499,13 @@ function amp_get_content_sanitizers( $post = null ) { ], 'native_img_used' => $native_img_used, ], + + // The AMP_Script_Sanitizer runs here because based on whether it allows custom scripts + // to be kept, it may impact the behavior of other sanitizers. For example, if custom + // scripts are kept then this is a signal that tree shaking in AMP_Style_Sanitizer cannot be + // performed. + AMP_Script_Sanitizer::class => [], + AMP_Srcset_Sanitizer::class => [], AMP_Img_Sanitizer::class => [ 'align_wide_support' => current_theme_supports( 'align-wide' ), @@ -1642,8 +1643,8 @@ function amp_get_content_sanitizers( $post = null ) { // Force core essential sanitizers to appear at the end at the end, with non-essential and third-party sanitizers appearing before. $expected_final_sanitizer_order = [ + AMP_Core_Theme_Sanitizer::class, // Must come before script sanitizer since onclick attributes are removed. AMP_Script_Sanitizer::class, // Must come before sanitizers for image, video, audio, form, and style. - AMP_Core_Theme_Sanitizer::class, AMP_Srcset_Sanitizer::class, AMP_Img_Sanitizer::class, AMP_Form_Sanitizer::class, diff --git a/tests/php/test-amp-helper-functions.php b/tests/php/test-amp-helper-functions.php index 0aa9b168350..3ef3ef37d41 100644 --- a/tests/php/test-amp-helper-functions.php +++ b/tests/php/test-amp-helper-functions.php @@ -1770,6 +1770,10 @@ static function( $classes ) { array_search( 'Even_After_Validating_Sanitizer', $ordered_sanitizers, true ), array_search( AMP_Script_Sanitizer::class, $ordered_sanitizers, true ) ); + $this->assertGreaterThan( + array_search( AMP_Core_Theme_Sanitizer::class, $ordered_sanitizers, true ), + array_search( AMP_Script_Sanitizer::class, $ordered_sanitizers, true ) + ); $this->assertEquals( AMP_Layout_Sanitizer::class, $ordered_sanitizers[ count( $ordered_sanitizers ) - 4 ] ); $this->assertEquals( AMP_Style_Sanitizer::class, $ordered_sanitizers[ count( $ordered_sanitizers ) - 3 ] ); $this->assertEquals( AMP_Meta_Sanitizer::class, $ordered_sanitizers[ count( $ordered_sanitizers ) - 2 ] ); From bdefef0cace3699c22d22d8dc26ea948a0ae793d Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 1 Sep 2021 20:23:01 -0700 Subject: [PATCH 08/16] Introduce transform_important_qualifiers arg to style sanitizer --- .../sanitizers/class-amp-style-sanitizer.php | 102 +++++++++++------- tests/php/test-amp-style-sanitizer.php | 74 +++++++++++++ 2 files changed, 135 insertions(+), 41 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 0cd1d3a15d4..93857e5d8d2 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -122,15 +122,16 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer { * Array of flags used to control sanitization. * * @var array { - * @type string[] $dynamic_element_selectors Selectors for elements (or their ancestors) which contain dynamic content; selectors containing these will not be filtered. - * @type bool $use_document_element Whether the root of the document should be used rather than the body. - * @type bool $require_https_src Require HTTPS URLs. - * @type callable $validation_error_callback Function to call when a validation error is encountered. - * @type bool $should_locate_sources Whether to locate the sources when reporting validation errors. - * @type string $parsed_cache_variant Additional value by which to vary parsed cache. - * @type string[] $focus_within_classes Class names in selectors that should be replaced with :focus-within pseudo classes. - * @type string[] $low_priority_plugins Plugin slugs of the plugins to deprioritize when hitting the CSS limit. - * @type bool $allow_transient_caching Whether to allow caching parsed CSS in transients. This may need to be disabled when there is highly-variable CSS content. + * @type string[] $dynamic_element_selectors Selectors for elements (or their ancestors) which contain dynamic content; selectors containing these will not be filtered. + * @type bool $use_document_element Whether the root of the document should be used rather than the body. + * @type bool $require_https_src Require HTTPS URLs. + * @type callable $validation_error_callback Function to call when a validation error is encountered. + * @type bool $should_locate_sources Whether to locate the sources when reporting validation errors. + * @type string $parsed_cache_variant Additional value by which to vary parsed cache. + * @type string[] $focus_within_classes Class names in selectors that should be replaced with :focus-within pseudo classes. + * @type string[] $low_priority_plugins Plugin slugs of the plugins to deprioritize when hitting the CSS limit. + * @type bool $allow_transient_caching Whether to allow caching parsed CSS in transients. This may need to be disabled when there is highly-variable CSS content. + * @type bool $transform_important_qualifiers Whether !important rules should be transformed. This also necessarily transform inline style attributes. * } */ protected $args; @@ -141,19 +142,20 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer { * @var array */ protected $DEFAULT_ARGS = [ - 'dynamic_element_selectors' => [ + 'dynamic_element_selectors' => [ 'amp-list', 'amp-live-list', '[submit-error]', '[submit-success]', 'amp-script', ], - 'should_locate_sources' => false, - 'parsed_cache_variant' => null, - 'focus_within_classes' => [ 'focus' ], - 'low_priority_plugins' => [ 'query-monitor' ], - 'allow_transient_caching' => true, - 'skip_tree_shaking' => false, + 'should_locate_sources' => false, + 'parsed_cache_variant' => null, + 'focus_within_classes' => [ 'focus' ], + 'low_priority_plugins' => [ 'query-monitor' ], + 'allow_transient_caching' => true, + 'skip_tree_shaking' => false, + 'transform_important_qualifiers' => true, ]; /** @@ -955,12 +957,14 @@ public function sanitize() { } } - $elements = []; - foreach ( $this->dom->xpath->query( "//*[ @style $dev_mode_predicate ]" ) as $element ) { - $elements[] = $element; - } - foreach ( $elements as $element ) { - $this->collect_inline_styles( $element ); + if ( $this->args['transform_important_qualifiers'] ) { + $elements = []; + foreach ( $this->dom->xpath->query( "//*[ @style $dev_mode_predicate ]" ) as $element ) { + $elements[] = $element; + } + foreach ( $elements as $element ) { + $this->collect_inline_styles( $element ); + } } $this->finalize_styles(); @@ -1570,11 +1574,21 @@ private function get_parsed_stylesheet( $stylesheet, $options = [] ) { $cache_impacting_options = array_merge( wp_array_slice_assoc( $options, - [ 'property_allowlist', 'property_denylist', 'stylesheet_url', 'allowed_at_rules' ] + [ + 'property_allowlist', + 'property_denylist', + 'stylesheet_url', + 'allowed_at_rules', + ] ), wp_array_slice_assoc( $this->args, - [ 'should_locate_sources', 'parsed_cache_variant', 'dynamic_element_selectors' ] + [ + 'should_locate_sources', + 'parsed_cache_variant', + 'dynamic_element_selectors', + 'transform_important_qualifiers', + ] ), [ 'language' => get_bloginfo( 'language' ), // Used to tree-shake html[lang] selectors. @@ -2382,10 +2396,12 @@ private function process_css_declaration_block( RuleSet $ruleset, CSSList $css_l $this->process_font_face_at_rule( $ruleset, $options ); } - $results = array_merge( - $results, - $this->transform_important_qualifiers( $ruleset, $css_list, $options ) - ); + if ( $this->args['transform_important_qualifiers'] ) { + $results = array_merge( + $results, + $this->transform_important_qualifiers( $ruleset, $css_list, $options ) + ); + } // Remove the ruleset if it is now empty. if ( 0 === count( $ruleset->getRules() ) ) { @@ -2605,10 +2621,12 @@ private function process_css_keyframes( KeyFrame $css_list, $options ) { continue; } - $results = array_merge( - $results, - $this->transform_important_qualifiers( $rules, $css_list, $options ) - ); + if ( $this->args['transform_important_qualifiers'] ) { + $results = array_merge( + $results, + $this->transform_important_qualifiers( $rules, $css_list, $options ) + ); + } $properties = $rules->getRules(); foreach ( $properties as $property ) { @@ -3203,15 +3221,17 @@ static function ( $matches ) { } // Replace the somewhat-meta [style] attribute selectors with attribute selector using the data attribute the original styles are copied into. - $selector = preg_replace( - '/(?<=\[)style(?=([*$~]?=.*?)?])/is', - self::ORIGINAL_STYLE_ATTRIBUTE_NAME, - $selector, - -1, - $count - ); - if ( $count > 0 ) { - $has_changed_selectors = true; + if ( $this->args['transform_important_qualifiers'] ) { + $selector = preg_replace( + '/(?<=\[)style(?=([*$~]?=.*?)?])/is', + self::ORIGINAL_STYLE_ATTRIBUTE_NAME, + $selector, + - 1, + $count + ); + if ( $count > 0 ) { + $has_changed_selectors = true; + } } /* diff --git a/tests/php/test-amp-style-sanitizer.php b/tests/php/test-amp-style-sanitizer.php index e4e0984595c..7fbde898c08 100644 --- a/tests/php/test-amp-style-sanitizer.php +++ b/tests/php/test-amp-style-sanitizer.php @@ -975,6 +975,80 @@ public function test_tree_shaking_disabled() { $this->assertGreaterThan( 75000, strlen( implode( '', $actual_stylesheets ) ) ); } + /** @return array */ + public function get_data_to_test_transform_important_qualifiers_arg() { + return [ + 'transform' => [ + true, + [ + ':root:not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_) .foo{color:red}', + ':root:not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_) .foo[data-amp-original-style*="blue"]{outline:solid 2px green}', + ':root:not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_) .amp-wp-9605c4d{background:blue}', + ], + null, + ], + 'no_transform' => [ + false, + [ + '.foo{color:red !important}', + '.foo[style*="blue"]{outline:solid 2px green !important}', + ], + 'background: blue !important', + ], + ]; + } + + /** + * Test that transformation of !important qualifiers (and processing of style attributes) can be turned off. + * + * @dataProvider get_data_to_test_transform_important_qualifiers_arg + * @param bool $transform_important_qualifiers Sanitizer args. + * @param array $expected_stylesheets Expected stylesheets. + * @param string|null $expected_style_attr Inline style attribute value, or null if not expected. + */ + public function test_transform_important_qualifiers_arg( $transform_important_qualifiers, $expected_stylesheets, $expected_style_attr ) { + $dom = Document::fromHtml( + ' + + + + + + +
+ + + ', + Options::DEFAULTS + ); + + $args = [ + 'use_document_element' => true, + ]; + + $sanitizer = new AMP_Style_Sanitizer( $dom, array_merge( $args, compact( 'transform_important_qualifiers' ) ) ); + $sanitizer->sanitize(); + + $validating_sanitizer = new AMP_Tag_And_Attribute_Sanitizer( $dom, $args ); + $validating_sanitizer->sanitize(); + + $this->assertEquals( + $expected_stylesheets, + array_values( array_filter( $sanitizer->get_stylesheets() ) ) + ); + + $style_attr = $dom->xpath->query( '//*/@style' )->item( 0 ); + if ( $style_attr instanceof DOMAttr ) { + $style_attr = $style_attr->nodeValue; + } + + $this->assertEquals( $expected_style_attr, $style_attr ); + } + /** * Data for testing AMP selector conversion. * From aa06a75bd4565bd5e86706fbc77c0e1f73bdcacc Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 1 Sep 2021 21:22:16 -0700 Subject: [PATCH 09/16] Greatly simplify thread_comments handling in comment sanitizer --- includes/amp-helper-functions.php | 12 +- .../sanitizers/class-amp-base-sanitizer.php | 13 + .../class-amp-comments-sanitizer.php | 262 +++++++++--------- .../sanitizers/class-amp-script-sanitizer.php | 97 +++++-- src/SandboxingLevels.php | 8 +- .../php/test-class-amp-comments-sanitizer.php | 45 ++- 6 files changed, 242 insertions(+), 195 deletions(-) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index 8c4ea9e8184..04e1c5f4d3d 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -1500,6 +1500,11 @@ function amp_get_content_sanitizers( $post = null ) { 'native_img_used' => $native_img_used, ], + AMP_Comments_Sanitizer::class => [ + 'thread_comments' => (bool) get_option( 'thread_comments' ), + 'comments_live_list' => ! empty( $theme_support_args['comments_live_list'] ), + ], + // The AMP_Script_Sanitizer runs here because based on whether it allows custom scripts // to be kept, it may impact the behavior of other sanitizers. For example, if custom // scripts are kept then this is a signal that tree shaking in AMP_Style_Sanitizer cannot be @@ -1514,9 +1519,6 @@ function amp_get_content_sanitizers( $post = null ) { AMP_Form_Sanitizer::class => [ 'native_post_forms_allowed' => $native_post_forms_allowed, ], - AMP_Comments_Sanitizer::class => [ - 'comments_live_list' => ! empty( $theme_support_args['comments_live_list'] ), - ], AMP_Video_Sanitizer::class => [], AMP_Audio_Sanitizer::class => [], AMP_Object_Sanitizer::class => [], @@ -1598,7 +1600,7 @@ function amp_get_content_sanitizers( $post = null ) { // Prevent removal of script output by wp_comment_form_unfiltered_html_nonce(). if ( current_user_can( 'unfiltered_html' ) ) { - $dev_mode_xpaths[] = '//script[ preceding-sibling::input[ @name = "_wp_unfiltered_html_comment_disabled" ] and contains( text(), "_wp_unfiltered_html_comment_disabled" ) ]'; + $dev_mode_xpaths[] = AMP_Comments_Sanitizer::UNFILTERED_HTML_COMMENT_SCRIPT_XPATH; } if ( is_admin_bar_showing() ) { @@ -1644,11 +1646,11 @@ function amp_get_content_sanitizers( $post = null ) { // Force core essential sanitizers to appear at the end at the end, with non-essential and third-party sanitizers appearing before. $expected_final_sanitizer_order = [ AMP_Core_Theme_Sanitizer::class, // Must come before script sanitizer since onclick attributes are removed. + AMP_Comments_Sanitizer::class, // Must come before AMP_Script_Sanitizer since it removes comment-rely.js. AMP_Script_Sanitizer::class, // Must come before sanitizers for image, video, audio, form, and style. AMP_Srcset_Sanitizer::class, AMP_Img_Sanitizer::class, AMP_Form_Sanitizer::class, - AMP_Comments_Sanitizer::class, AMP_Video_Sanitizer::class, AMP_Audio_Sanitizer::class, AMP_Object_Sanitizer::class, diff --git a/includes/sanitizers/class-amp-base-sanitizer.php b/includes/sanitizers/class-amp-base-sanitizer.php index 76a471eefbb..bb28997f85b 100644 --- a/includes/sanitizers/class-amp-base-sanitizer.php +++ b/includes/sanitizers/class-amp-base-sanitizer.php @@ -140,11 +140,24 @@ public function get_selector_conversion_mapping() { return []; } + /** + * Get args. + * + * @since 2.2 + * + * @return array Args. + */ + public function get_args() { + return $this->args; + } + /** * Update args. * * Merges the supplied args with the existing args. * + * @since 2.2 + * * @param array $args Args. */ public function update_args( $args ) { diff --git a/includes/sanitizers/class-amp-comments-sanitizer.php b/includes/sanitizers/class-amp-comments-sanitizer.php index a8fca91d27e..b9b1b925163 100644 --- a/includes/sanitizers/class-amp-comments-sanitizer.php +++ b/includes/sanitizers/class-amp-comments-sanitizer.php @@ -7,6 +7,7 @@ use AmpProject\Amp; use AmpProject\Attribute; +use AmpProject\Dom\Element; use AmpProject\Extension; use AmpProject\Tag; @@ -15,11 +16,24 @@ * * Strips and corrects attributes in forms. * - * @todo When comment-reply is on the page, this should be skipped. * @internal */ class AMP_Comments_Sanitizer extends AMP_Base_Sanitizer { + /** + * XPath expression for script output to allow users with unfiltered_html to use that capability in top-level window. + * + * @see wp_comment_form_unfiltered_html_nonce() + * @var string + */ + const UNFILTERED_HTML_COMMENT_SCRIPT_XPATH = ' + //script[ + preceding-sibling::input[ @name = "_wp_unfiltered_html_comment_disabled" ] + and + contains( text(), "_wp_unfiltered_html_comment_disabled" ) + ] + '; + /** * Default args. * @@ -29,9 +43,44 @@ class AMP_Comments_Sanitizer extends AMP_Base_Sanitizer { */ protected $DEFAULT_ARGS = [ 'comment_live_list' => false, // @todo See . - 'comment_reply_js_printed' => false, + 'thread_comments' => false, // By default maps to thread_comments option. + 'allow_commenting_scripts' => false, // @todo Change this to allow_native_comments which also adds px-verified to comment form instead of validation error. ]; + /** + * Init. + * + * @param AMP_Base_Sanitizer[] $sanitizers Sanitizers. + */ + public function init( $sanitizers ) { + parent::init( $sanitizers ); + + // When initializing with comment scripting allowed, let the script sanitizer know that the commenting scripts + // have are PX-verified so that they won't get removed nor reported as validation errors. + // @todo Or rather should this be accomplished via the amp_validation_error_default_sanitized filter? + if ( + ! empty( $this->args['allow_commenting_scripts'] ) + && + isset( $sanitizers[ AMP_Script_Sanitizer::class ] ) + && + $sanitizers[ AMP_Script_Sanitizer::class ] instanceof AMP_Script_Sanitizer + ) { + $script_sanitizer = $sanitizers[ AMP_Script_Sanitizer::class ]; + + // @todo Why go the trouble of this? Add attributes below instead. + // @todo Add data-ampdevmode attribute to UNFILTERED_HTML_COMMENT_SCRIPT_XPATH here. + $script_args = $script_sanitizer->get_args(); + if ( $this->args['thread_comments'] && wp_script_is( 'comment-reply', 'done' ) ) { + $script_args['px_verified_node_xpaths'][] = '//script[ @id = "comment-reply-js" ]'; + } + if ( current_user_can( 'unfiltered_html' ) ) { + $script_args['px_verified_node_xpaths'][] = self::UNFILTERED_HTML_COMMENT_SCRIPT_XPATH; + } + + $script_sanitizer->update_args( $script_args ); + } + } + /** * Pre-process the comment form and comment list for AMP. * @@ -40,20 +89,11 @@ class AMP_Comments_Sanitizer extends AMP_Base_Sanitizer { * @since 0.7 */ public function sanitize() { - // @todo Check if the comment-reply script was printed. - // @todo Add defer to comment-reply if the script _is_ on the page (and there were no dependencies). - foreach ( $this->dom->getElementsByTagName( Tag::FORM ) as $comment_form ) { - // Skip processing comment forms which have opted-out of conversion to amp-form. - // Note that AMP_Form_Sanitizer runs before AMP_Comments_Sanitizer according to amp_get_content_sanitizers(). -// if ( $comment_form->hasAttribute( 'action' ) ) { -// continue; -// } - $action = $comment_form->getAttribute( Attribute::ACTION_XHR ); -// if ( ! $action ) { -// $action = $comment_form->getAttribute( Attribute::ACTION ); -// } + if ( ! $action ) { + $action = $comment_form->getAttribute( Attribute::ACTION ); + } $action_path = wp_parse_url( $action, PHP_URL_PATH ); if ( $action_path && 'wp-comments-post.php' === basename( $action_path ) ) { $this->process_comment_form( $comment_form ); @@ -70,127 +110,87 @@ public function sanitize() { } /** - * Get the ID for the amp-state. + * Process comment form. + * + * @since 0.7 * - * @param int $post_id Post ID. - * @return string ID for amp-state. + * @param Element $comment_form Comment form. */ - protected function get_comment_form_state_id( $post_id ) { - return sprintf( 'commentform_post_%d', $post_id ); + protected function process_comment_form( Element $comment_form ) { + $this->ampify_threaded_comments( $comment_form ); } /** - * Comment form. + * Ampify threaded comments by utilizing amp-bind to implement comment reply functionality. * - * @since 0.7 + * The logic here is only needed if: + * 1. Threaded comments is enabled, and + * 2. The comment-reply script was not added to the page. * - * @param DOMElement $comment_form Comment form. + * @param Element $comment_form Comment form. */ - protected function process_comment_form( $comment_form ) { - $form_fields = []; - foreach ( $comment_form->getElementsByTagName( Tag::INPUT ) as $element ) { - /** @var DOMElement $element */ - $name = $element->getAttribute( Attribute::NAME ); - if ( $name ) { - $form_fields[ $name ][] = $element; - } + protected function ampify_threaded_comments( Element $comment_form ) { + // Do nothing if comment threading is not enabled. + if ( ! $this->args['thread_comments'] ) { + return; } - foreach ( $comment_form->getElementsByTagName( Tag::TEXTAREA ) as $element ) { - /** @var DOMElement $element */ - $name = $element->getAttribute( Attribute::NAME ); - if ( $name ) { - $form_fields[ $name ][] = $element; - } - } + // If comment-reply is on the page, then prevent adding amp-bind implementation as well. + $comment_reply_script = $this->dom->getElementById( 'comment-reply-js' ); + if ( $comment_reply_script instanceof Element && $this->args['allow_commenting_scripts'] ) { + + // @todo This should add data-px-verified-tag attribute! + // Improve performance by deferring comment-reply. + $comment_reply_script->setAttributeNode( $this->dom->createAttribute( 'defer' ) ); - /** - * Named input elements. - * - * @var DOMElement[][] $form_fields - */ - if ( empty( $form_fields['comment_post_ID'] ) ) { return; } - $post_id = (int) $form_fields['comment_post_ID'][0]->getAttribute( Attribute::VALUE ); - $state_id = $this->get_comment_form_state_id( $post_id ); - $form_state = [ - 'values' => [], - 'submitting' => false, - 'replyTo' => '', + // Remove comment-reply.js since it will be implemented using amp-bind below. + if ( $comment_reply_script instanceof Element ) { + $comment_reply_script->parentNode->removeChild( $comment_reply_script ); + } + + // Create reply state. + $amp_state = $this->dom->createElement( Extension::STATE ); + $comment_form->insertBefore( $amp_state, $comment_form->firstChild ); + $state_id = 'ampCommentThreading'; + $amp_state->setAttribute( Attribute::ID, $state_id ); + $state = [ + 'replyTo' => '', + 'commentParent' => '0', // @todo What if page accessed with replytocom? Then this should be $comment_parent_id below. ]; - $comment_parent_id = null; - if ( ! empty( $form_fields['comment_parent'] ) ) { - $comment_parent_id = (int) $form_fields['comment_parent'][0]->getAttribute( Attribute::VALUE ); - } + $comment_parent_id = 0; + $comment_parent_input = $this->dom->getElementById( 'comment_parent' ); + if ( $comment_parent_input instanceof Element ) { + $comment_parent_id = (int) $comment_parent_input->getAttribute( Attribute::VALUE ); - $amp_bind_attr_format = Amp::BIND_DATA_ATTR_PREFIX . '%s'; - foreach ( $form_fields as $name => $form_field ) { - foreach ( $form_field as $element ) { - - // @todo Radio and checkbox inputs are not supported yet. - if ( in_array( strtolower( $element->getAttribute( Attribute::TYPE ) ), [ 'checkbox', 'radio' ], true ) ) { - continue; - } - - $element->setAttribute( sprintf( $amp_bind_attr_format, Attribute::DISABLED ), "$state_id.submitting" ); - - if ( Tag::TEXTAREA === strtolower( $element->nodeName ) ) { - $form_state['values'][ $name ] = $element->textContent; - $element->setAttribute( sprintf( $amp_bind_attr_format, 'text' ), "$state_id.values.$name" ); - } else { - $form_state['values'][ $name ] = $element->hasAttribute( Attribute::VALUE ) ? $element->getAttribute( Attribute::VALUE ) : ''; - $element->setAttribute( sprintf( $amp_bind_attr_format, Attribute::VALUE ), "$state_id.values.$name" ); - } - - // Update the state in response to changing the input. - $element->setAttribute( - Attribute::ON, - sprintf( - 'change:AMP.setState( { %s: { values: { %s: event.value } } } )', - $state_id, - wp_json_encode( $name ) - ) - ); - } + $comment_parent_input->setAttribute( + Amp::BIND_DATA_ATTR_PREFIX . 'value', + sprintf( '%s.commentParent', $state_id ) + ); } // Add amp-state to the document. - $amp_state = $this->dom->createElement( Extension::STATE ); - $amp_state->setAttribute( Attribute::ID, $state_id ); $script = $this->dom->createElement( Tag::SCRIPT ); $script->setAttribute( Attribute::TYPE, 'application/json' ); - $comment_form->insertBefore( $amp_state, $comment_form->firstChild ); + $script->appendChild( $this->dom->createTextNode( wp_json_encode( $state, JSON_UNESCAPED_UNICODE ) ) ); + $amp_state->appendChild( $script ); - // Update state when submitting form. - $form_reset_state = $form_state; - unset( - $form_reset_state['values']['author'], - $form_reset_state['values']['email'], - $form_reset_state['values']['url'] - ); - $on = [ - // Disable the form when submitting. - sprintf( - 'submit:AMP.setState( { %s: { submitting: true } } )', - wp_json_encode( $state_id ) - ), - // Re-enable the form fields when the submission fails. - sprintf( - 'submit-error:AMP.setState( { %s: { submitting: false } } )', - wp_json_encode( $state_id ) - ), - // Reset the form to its initial state (with enabled form fields), except for the author, email, and url. + + // Reset state when submitting form. + $comment_form->addAmpAction( + 'submit-success', sprintf( - 'submit-success:AMP.setState( { %s: %s } )', + '%s.clear,AMP.setState({%s: %s})', + $this->dom->getElementId( $comment_form ), $state_id, - wp_json_encode( $form_reset_state, JSON_UNESCAPED_UNICODE ) - ), - ]; - $comment_form->setAttribute( Attribute::ON, implode( ';', $on ) ); + wp_json_encode( $state ) + ) + ); + // Prepare the comment form for replies. The logic here corresponds to what is found in comment-reply.js. $reply_heading_element = $this->dom->getElementById( 'reply-title' ); $reply_heading_text_node = null; $reply_link_to_parent = null; @@ -227,20 +227,15 @@ protected function process_comment_form( $comment_form ) { ); } - // Populate amp-state. - $amp_state->appendChild( $script ); - $script->appendChild( $this->dom->createTextNode( wp_json_encode( $form_state, JSON_UNESCAPED_UNICODE ) ) ); - + // Update comment reply links to set the reply state. $comment_reply_links = $this->dom->xpath->query( '//a[ @data-commentid and @data-postid and @data-replyto and @data-respondelement and contains( @class, "comment-reply-link" ) ]' ); foreach ( $comment_reply_links as $comment_reply_link ) { - /** @var DOMElement $comment_reply_link */ + /** @var Element $comment_reply_link */ $comment_reply_state = [ $state_id => [ - 'replyTo' => $comment_reply_link->getAttribute( 'data-replyto' ), - 'values' => [ - 'comment_parent' => $comment_reply_link->getAttribute( 'data-commentid' ), - ], + 'replyTo' => $comment_reply_link->getAttribute( 'data-replyto' ), + 'commentParent' => $comment_reply_link->getAttribute( 'data-commentid' ), ], ]; @@ -249,39 +244,32 @@ protected function process_comment_form( $comment_form ) { '#' . $comment_reply_link->getAttribute( 'data-respondelement' ) ); - $comment_reply_link->setAttribute( - Attribute::ON, + $comment_reply_link->addAmpAction( + 'tap', sprintf( - 'tap:AMP.setState(%s),comment.focus', + 'AMP.setState(%s),comment.focus', wp_json_encode( $comment_reply_state, JSON_UNESCAPED_UNICODE ) ) ); } $cancel_comment_reply_link = $this->dom->getElementById( 'cancel-comment-reply-link' ); - if ( $cancel_comment_reply_link instanceof DOMElement ) { - $cancel_comment_reply_link->removeAttribute( Attribute::STYLE ); + if ( $cancel_comment_reply_link instanceof Element ) { + // Use hidden attribute to hide/show cancel reply link when commentParent is zero. + $cancel_comment_reply_link->removeAttribute( Attribute::STYLE ); if ( ! $comment_parent_id ) { $cancel_comment_reply_link->setAttributeNode( $this->dom->createAttribute( Attribute::HIDDEN ) ); } - - $tap_state = [ - $state_id => [ - 'replyTo' => '', - 'values' => [ - 'comment_parent' => '0', - ], - ], - ]; $cancel_comment_reply_link->setAttribute( - Attribute::ON, - sprintf( 'tap:AMP.setState( %s )', wp_json_encode( $tap_state, JSON_UNESCAPED_UNICODE ) ) + Amp::BIND_DATA_ATTR_PREFIX . Attribute::HIDDEN, + sprintf( '%s.commentParent == "0"', $state_id ) ); - $cancel_comment_reply_link->setAttribute( - Amp::BIND_DATA_ATTR_PREFIX . Attribute::HIDDEN, - sprintf( '%s.values.comment_parent == "0"', $state_id ) + // Reset state when clicking cancel. + $cancel_comment_reply_link->addAmpAction( + 'tap', + sprintf( 'AMP.setState({%s: %s})', $state_id, wp_json_encode( $state, JSON_UNESCAPED_UNICODE ) ) ); } } diff --git a/includes/sanitizers/class-amp-script-sanitizer.php b/includes/sanitizers/class-amp-script-sanitizer.php index 01b6648f35e..6b930d4e7c6 100644 --- a/includes/sanitizers/class-amp-script-sanitizer.php +++ b/includes/sanitizers/class-amp-script-sanitizer.php @@ -54,10 +54,9 @@ class AMP_Script_Sanitizer extends AMP_Base_Sanitizer { * @var array */ protected $DEFAULT_ARGS = [ - 'unwrap_noscripts' => true, - 'sanitize_js_scripts' => false, - 'allow_comment_reply_js' => false, // @todo Support this. - // @todo Have a callback which indicates whether scripts are bento-friendly. The comment-reply is one such script which is allowed in moderate. + 'unwrap_noscripts' => true, + 'sanitize_js_scripts' => false, + 'px_verified_node_xpaths' => [], ]; /** @@ -79,6 +78,13 @@ class AMP_Script_Sanitizer extends AMP_Base_Sanitizer { */ protected $kept_script_count = 0; + /** + * Number of kept nodes which were PX-verified. + * + * @var int + */ + protected $px_verified_kept_node_count = 0; + /** * Sanitizers. * @@ -106,7 +112,6 @@ public function init( $sanitizers ) { */ public function sanitize() { if ( ! empty( $this->args['sanitize_js_scripts'] ) ) { - // @todo This should automatically add defer to script#comment-reply-js if no other scripts had it as a dependency. $this->sanitize_js_script_elements(); } @@ -114,29 +119,34 @@ public function sanitize() { // unwrapped or else this could result in the JS and no-JS fallback experiences both being present on the page. // So unwrapping is only done when no custom scripts were retained (and the sanitizer arg opts-in to unwrap). if ( 0 === $this->kept_script_count && ! empty( $this->args['unwrap_noscripts'] ) ) { + // @todo Also only do this if px_verified_kept_node_count is 0? $this->unwrap_noscript_elements(); } - // When there are kept custom scripts, skip tree shaking since it's likely JS will toggle classes that have - // associated style rules. - // @todo There should be an attribute on script tags that opt-in to keeping tree shaking and/or to indicate what class names need to be included. + $sanitizer_arg_updates = []; + + // When there are kept custom scripts, skip tree shaking since it's likely JS will toggle classes that have associated style rules. + // @todo There should be an attribute on script tags that opt-in to keeping tree shaking and/or to indicate what class names need to be included. (Update: This is in part implemented by px_verified_node_xpaths.) // @todo Depending on the size of the underlying stylesheets, this may need to retain the use of external styles to prevent inlining excessive CSS. This may involve writing minified CSS to disk, or skipping style processing altogether if no selector conversions are needed. - // @todo Prevent inclusion of comment-reply.js alone from causing this. if ( $this->kept_script_count > 0 ) { - $sanitizer_arg_updates = [ - // @todo If comment-reply.js was on the page (likely best detected via wp_script_is('comment-reply', 'done')), then AMP_Comment_Sanitizer should know. - AMP_Style_Sanitizer::class => [ 'skip_tree_shaking' => true ], - AMP_Img_Sanitizer::class => [ 'native_img_used' => true ], - AMP_Video_Sanitizer::class => [ 'native_video_used' => true ], - AMP_Audio_Sanitizer::class => [ 'native_audio_used' => true ], - AMP_Iframe_Sanitizer::class => [ 'native_iframe_used' => true ], - AMP_Form_Sanitizer::class => [ 'native_post_forms_allowed' => true ], - AMP_Tag_And_Attribute_Sanitizer::class => [ 'prefer_bento' => true ], - ]; - foreach ( $sanitizer_arg_updates as $sanitizer_class => $sanitizer_args ) { - if ( array_key_exists( $sanitizer_class, $this->sanitizers ) ) { - $this->sanitizers[ $sanitizer_class ]->update_args( $sanitizer_args ); - } + $sanitizer_arg_updates[ AMP_Style_Sanitizer::class ]['skip_tree_shaking'] = true; + } + + // When custom scripts are on the page, use Bento AMP components whenever possible and turn off some CSS + // processing is unnecessary for a valid AMP page and which can break custom scripts. + if ( $this->px_verified_kept_node_count > 0 || $this->kept_script_count > 0 ) { + $sanitizer_arg_updates[ AMP_Tag_And_Attribute_Sanitizer::class ]['prefer_bento'] = true; + $sanitizer_arg_updates[ AMP_Style_Sanitizer::class ]['transform_important_qualifiers'] = false; + $sanitizer_arg_updates[ AMP_Img_Sanitizer::class ]['native_img_used'] = true; + $sanitizer_arg_updates[ AMP_Video_Sanitizer::class ]['native_video_used'] = true; + $sanitizer_arg_updates[ AMP_Audio_Sanitizer::class ]['native_audio_used'] = true; + $sanitizer_arg_updates[ AMP_Iframe_Sanitizer::class ]['native_iframe_used'] = true; + $sanitizer_arg_updates[ AMP_Form_Sanitizer::class ]['native_post_forms_allowed'] = true; + } + + foreach ( $sanitizer_arg_updates as $sanitizer_class => $sanitizer_args ) { + if ( array_key_exists( $sanitizer_class, $this->sanitizers ) ) { + $this->sanitizers[ $sanitizer_class ]->update_args( $sanitizer_args ); } } } @@ -219,12 +229,34 @@ protected function sanitize_js_script_elements() { ]' ); + $px_verified_nodes = []; + if ( ! empty( $this->args['px_verified_node_xpaths'] ) ) { + foreach ( $this->args['px_verified_node_xpaths'] as $script_xpath ) { + $xpath_query = $this->dom->xpath->query( $script_xpath ); + foreach ( $xpath_query as $px_verified_node ) { + $px_verified_nodes[] = $px_verified_node; + } + } + } + /** @var Element $script */ foreach ( $scripts as $script ) { if ( DevMode::hasExemptionForNode( $script ) ) { continue; } + if ( in_array( $script, $px_verified_nodes, true ) ) { + $script->setAttributeNode( + $this->dom->createAttribute( AMP_Validation_Manager::AMP_UNVALIDATED_TAG_ATTRIBUTE ) + ); + $this->dom->documentElement->setAttributeNode( + $this->dom->createAttribute( AMP_Validation_Manager::AMP_NON_VALID_DOC_ATTRIBUTE ) + ); + $this->px_verified_kept_node_count++; + // @todo Consider forcing any PX-verified script to have async/defer if not module. For inline scripts, hack via data: URL? + continue; + } + if ( $script->hasAttribute( Attribute::SRC ) ) { // Skip external AMP CDN scripts. if ( 0 === strpos( $script->getAttribute( Attribute::SRC ), 'https://cdn.ampproject.org/' ) ) { @@ -285,6 +317,25 @@ protected function sanitize_js_script_elements() { continue; } + // Since the attribute has been PX-verified, mark it as an unvalidated attribute and move along. + if ( in_array( $event_handler_attribute, $px_verified_nodes, true ) ) { + $attr_value = $element->getAttribute( AMP_Validation_Manager::AMP_UNVALIDATED_ATTRS_ATTRIBUTE ); + if ( $attr_value ) { + $attr_value .= ' ' . $event_handler_attribute->nodeName; + } else { + $attr_value = $event_handler_attribute->nodeName; + } + $element->setAttribute( AMP_Validation_Manager::AMP_UNVALIDATED_ATTRS_ATTRIBUTE, $attr_value ); + $this->dom->documentElement->setAttributeNode( + $this->dom->createAttribute( AMP_Validation_Manager::AMP_NON_VALID_DOC_ATTRIBUTE ) + ); + $this->dom->documentElement->setAttributeNode( + $this->dom->createAttribute( AMP_Validation_Manager::AMP_NON_VALID_DOC_ATTRIBUTE ) + ); + $this->px_verified_kept_node_count++; + continue; + } + $removed = $this->remove_invalid_attribute( $element, $event_handler_attribute, diff --git a/src/SandboxingLevels.php b/src/SandboxingLevels.php index caa89b3d2ff..2c899d29c3c 100644 --- a/src/SandboxingLevels.php +++ b/src/SandboxingLevels.php @@ -12,6 +12,7 @@ use AmpProject\AmpWP\Infrastructure\Conditional; use AmpProject\AmpWP\Infrastructure\Service; use AMP_Form_Sanitizer; +use AMP_Comments_Sanitizer; use AMP_Script_Sanitizer; /** @@ -145,10 +146,9 @@ public function add_hooks() { static function ( $sanitizers ) use ( $sandboxing_level ) { $sanitizers[ AMP_Script_Sanitizer::class ]['sanitize_js_scripts'] = true; - // @todo This: - // if ( $sandboxing_level < 3 ) { - // $sanitizers[ AMP_Script_Sanitizer::class ]['allow_comment_reply_js'] = true; - // } + if ( $sandboxing_level < 3 ) { + $sanitizers[ AMP_Comments_Sanitizer::class ]['allow_commenting_scripts'] = true; + } return $sanitizers; } ); diff --git a/tests/php/test-class-amp-comments-sanitizer.php b/tests/php/test-class-amp-comments-sanitizer.php index e4d907ccb10..2ad985dafb5 100644 --- a/tests/php/test-class-amp-comments-sanitizer.php +++ b/tests/php/test-class-amp-comments-sanitizer.php @@ -60,20 +60,22 @@ public function test_sanitize_incorrect_action() { * @covers AMP_Comments_Sanitizer::sanitize() */ public function test_sanitize_allowed_action_xhr() { + update_option( 'thread_comments', '1' ); $form_sanitizer = new AMP_Form_Sanitizer( $this->dom ); - $comments_sanitizer = new AMP_Comments_Sanitizer( $this->dom ); + $comments_sanitizer = new AMP_Comments_Sanitizer( + $this->dom, + [ 'thread_comments' => '1' ] + ); // Use an allowed action. $form = $this->create_form( '/wp-comments-post.php' ); $form_sanitizer->sanitize(); $comments_sanitizer->sanitize(); - $on = $form->getAttribute( 'on' ); - $this->assertStringContainsString( 'submit:AMP.setState(', $on ); - $this->assertStringContainsString( 'submit-error:AMP.setState(', $on ); - foreach ( $this->get_form_element_names() as $name ) { - $this->assertStringContainsString( $name, $on ); - } + $this->assertEquals( + 'submit-success:i-amp-0.clear,AMP.setState({ampCommentThreading: {"replyTo":"","commentParent":"0"}})', + $form->getAttribute( 'on' ) + ); } /** @@ -96,7 +98,8 @@ public function test_sanitize_native_post_form() { * @covers AMP_Comments_Sanitizer::process_comment_form() */ public function test_process_comment_form() { - $instance = new AMP_Comments_Sanitizer( $this->dom ); + update_option( 'thread_comments', '1' ); + $instance = new AMP_Comments_Sanitizer( $this->dom, [ 'thread_comments' => true ] ); $form = $this->create_form( '/wp-comments-post.php' ); $this->call_private_method( $instance, 'process_comment_form', [ $form ] ); @@ -104,26 +107,16 @@ public function test_process_comment_form() { $on = $form->getAttribute( 'on' ); $amp_state = $this->dom->getElementsByTagName( 'amp-state' )->item( 0 ); - $this->assertStringContainsString( 'submit:AMP.setState(', $on ); - $this->assertStringContainsString( 'submit-error:AMP.setState(', $on ); - $this->assertStringContainsString( 'submit-success:AMP.setState(', $on ); - $this->assertStringContainsString( strval( $GLOBALS['post']->ID ), $on ); + $this->assertEquals( + $on, + 'submit-success:i-amp-0.clear,AMP.setState({ampCommentThreading: {"replyTo":"","commentParent":"0"}})' + ); $this->assertEquals( 'script', $amp_state->firstChild->nodeName ); - foreach ( $this->get_form_element_names() as $name ) { - $this->assertStringContainsString( $name, $on ); - $this->assertStringContainsString( $name, $amp_state->nodeValue ); - } - foreach ( $form->getElementsByTagName( 'input' ) as $input ) { - /** - * Input. - * - * @var DOMElement $input - */ - $on = $input->getAttribute( 'on' ); - $this->assertStringContainsString( 'change:AMP.setState(', $on ); - $this->assertStringContainsString( strval( $GLOBALS['post']->ID ), $on ); - } + $this->assertEquals( + '{"replyTo":"","commentParent":"0"}', + $amp_state->nodeValue + ); } /** From 696f1e9f29acdec9a9276d6bb753a9703e6c5a5a Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 1 Sep 2021 22:46:39 -0700 Subject: [PATCH 10/16] Only unwrap noscripts when no PX-verified scripts present either --- includes/amp-helper-functions.php | 7 +-- .../sanitizers/class-amp-base-sanitizer.php | 22 +++++++ .../class-amp-comments-sanitizer.php | 62 ++++++++----------- .../sanitizers/class-amp-script-sanitizer.php | 53 +++++----------- .../sanitizers/class-amp-style-sanitizer.php | 1 + .../class-amp-validation-manager.php | 17 +++++ 6 files changed, 81 insertions(+), 81 deletions(-) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index 04e1c5f4d3d..74d89c80eed 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -1598,11 +1598,6 @@ function amp_get_content_sanitizers( $post = null ) { */ $dev_mode_xpaths = (array) apply_filters( 'amp_dev_mode_element_xpaths', [] ); - // Prevent removal of script output by wp_comment_form_unfiltered_html_nonce(). - if ( current_user_can( 'unfiltered_html' ) ) { - $dev_mode_xpaths[] = AMP_Comments_Sanitizer::UNFILTERED_HTML_COMMENT_SCRIPT_XPATH; - } - if ( is_admin_bar_showing() ) { $dev_mode_xpaths[] = '//*[ @id = "wpadminbar" ]'; $dev_mode_xpaths[] = '//*[ @id = "wpadminbar" ]//*'; @@ -1646,7 +1641,7 @@ function amp_get_content_sanitizers( $post = null ) { // Force core essential sanitizers to appear at the end at the end, with non-essential and third-party sanitizers appearing before. $expected_final_sanitizer_order = [ AMP_Core_Theme_Sanitizer::class, // Must come before script sanitizer since onclick attributes are removed. - AMP_Comments_Sanitizer::class, // Must come before AMP_Script_Sanitizer since it removes comment-rely.js. + AMP_Comments_Sanitizer::class, // Must come before AMP_Script_Sanitizer since it either removes comment-rely.js or marks it as PX-verified. AMP_Script_Sanitizer::class, // Must come before sanitizers for image, video, audio, form, and style. AMP_Srcset_Sanitizer::class, AMP_Img_Sanitizer::class, diff --git a/includes/sanitizers/class-amp-base-sanitizer.php b/includes/sanitizers/class-amp-base-sanitizer.php index bb28997f85b..de0f9bd0483 100644 --- a/includes/sanitizers/class-amp-base-sanitizer.php +++ b/includes/sanitizers/class-amp-base-sanitizer.php @@ -506,6 +506,15 @@ public function remove_invalid_child( $node, $validation_error = [] ) { return false; } + // Prevent removing a tag which was verified for PX. + if ( + $node instanceof DOMElement + && + $node->hasAttribute( AMP_Validation_Manager::PX_VERIFIED_TAG_ATTRIBUTE ) + ) { + return false; + } + // Prevent double-reporting nodes that are rejected for sanitization. if ( isset( $this->nodes_to_keep[ $node->nodeName ] ) && in_array( $node, $this->nodes_to_keep[ $node->nodeName ], true ) ) { return false; @@ -581,6 +590,19 @@ public function remove_invalid_attribute( $element, $attribute, $validation_erro return false; } + // Prevent removing an attribute which was verified for PX. + if ( + $element->hasAttribute( AMP_Validation_Manager::PX_VERIFIED_ATTRS_ATTRIBUTE ) + && + in_array( + $node->nodeName, + preg_split( '/\s+/', $element->getAttribute( AMP_Validation_Manager::PX_VERIFIED_ATTRS_ATTRIBUTE ) ), + true + ) + ) { + return false; + } + $should_remove = $this->should_sanitize_validation_error( $validation_error, compact( 'node' ) ); if ( $should_remove ) { $allow_empty = ! empty( $attr_spec[ AMP_Rule_Spec::VALUE_URL ][ AMP_Rule_Spec::ALLOW_EMPTY ] ); diff --git a/includes/sanitizers/class-amp-comments-sanitizer.php b/includes/sanitizers/class-amp-comments-sanitizer.php index b9b1b925163..67cd53d9255 100644 --- a/includes/sanitizers/class-amp-comments-sanitizer.php +++ b/includes/sanitizers/class-amp-comments-sanitizer.php @@ -10,6 +10,7 @@ use AmpProject\Dom\Element; use AmpProject\Extension; use AmpProject\Tag; +use AmpProject\DevMode; /** * Class AMP_Comments_Sanitizer @@ -47,40 +48,6 @@ class AMP_Comments_Sanitizer extends AMP_Base_Sanitizer { 'allow_commenting_scripts' => false, // @todo Change this to allow_native_comments which also adds px-verified to comment form instead of validation error. ]; - /** - * Init. - * - * @param AMP_Base_Sanitizer[] $sanitizers Sanitizers. - */ - public function init( $sanitizers ) { - parent::init( $sanitizers ); - - // When initializing with comment scripting allowed, let the script sanitizer know that the commenting scripts - // have are PX-verified so that they won't get removed nor reported as validation errors. - // @todo Or rather should this be accomplished via the amp_validation_error_default_sanitized filter? - if ( - ! empty( $this->args['allow_commenting_scripts'] ) - && - isset( $sanitizers[ AMP_Script_Sanitizer::class ] ) - && - $sanitizers[ AMP_Script_Sanitizer::class ] instanceof AMP_Script_Sanitizer - ) { - $script_sanitizer = $sanitizers[ AMP_Script_Sanitizer::class ]; - - // @todo Why go the trouble of this? Add attributes below instead. - // @todo Add data-ampdevmode attribute to UNFILTERED_HTML_COMMENT_SCRIPT_XPATH here. - $script_args = $script_sanitizer->get_args(); - if ( $this->args['thread_comments'] && wp_script_is( 'comment-reply', 'done' ) ) { - $script_args['px_verified_node_xpaths'][] = '//script[ @id = "comment-reply-js" ]'; - } - if ( current_user_can( 'unfiltered_html' ) ) { - $script_args['px_verified_node_xpaths'][] = self::UNFILTERED_HTML_COMMENT_SCRIPT_XPATH; - } - - $script_sanitizer->update_args( $script_args ); - } - } - /** * Pre-process the comment form and comment list for AMP. * @@ -135,14 +102,35 @@ protected function ampify_threaded_comments( Element $comment_form ) { return; } - // If comment-reply is on the page, then prevent adding amp-bind implementation as well. + // Flag the unfiltered_html comment script as being in AMP Dev Mode at the tag level since it is only ever in + // the page when the user is logged-in which is when Dev Mode will be enabled anyway. + if ( current_user_can( 'unfiltered_html' ) ) { + $unfiltered_html_comment_script = $this->dom->xpath->query( self::UNFILTERED_HTML_COMMENT_SCRIPT_XPATH )->item( 0 ); + if ( $unfiltered_html_comment_script instanceof Element ) { + $unfiltered_html_comment_script->setAttributeNode( $this->dom->createAttribute( DevMode::DEV_MODE_ATTRIBUTE ) ); + + // Also indicate that that this element is PX-verified so that if by chance AMP Dev Mode is disabled, + // it will not be considered a custom script that will require turning off CSS tree shaking, etc. + if ( $this->args['allow_commenting_scripts'] ) { + $unfiltered_html_comment_script->setAttributeNode( + $this->dom->createAttribute( AMP_Validation_Manager::PX_VERIFIED_TAG_ATTRIBUTE ) + ); + } + } + } + + // If comment-reply is on the page and commenting scripts are allowed, mark it as being PX-verified and improve + // performance with defer. Then short-circuit since the amp-bind implementation won't be needed. $comment_reply_script = $this->dom->getElementById( 'comment-reply-js' ); if ( $comment_reply_script instanceof Element && $this->args['allow_commenting_scripts'] ) { - // @todo This should add data-px-verified-tag attribute! + // Prevent the script from being sanitized by the script sanitizer and prevent it from triggering the loose sandboxing level. + $comment_reply_script->setAttributeNode( + $this->dom->createAttribute( AMP_Validation_Manager::PX_VERIFIED_TAG_ATTRIBUTE ) + ); + // Improve performance by deferring comment-reply. $comment_reply_script->setAttributeNode( $this->dom->createAttribute( 'defer' ) ); - return; } diff --git a/includes/sanitizers/class-amp-script-sanitizer.php b/includes/sanitizers/class-amp-script-sanitizer.php index 6b930d4e7c6..7ec02c016e6 100644 --- a/includes/sanitizers/class-amp-script-sanitizer.php +++ b/includes/sanitizers/class-amp-script-sanitizer.php @@ -54,9 +54,8 @@ class AMP_Script_Sanitizer extends AMP_Base_Sanitizer { * @var array */ protected $DEFAULT_ARGS = [ - 'unwrap_noscripts' => true, - 'sanitize_js_scripts' => false, - 'px_verified_node_xpaths' => [], + 'unwrap_noscripts' => true, + 'sanitize_js_scripts' => false, ]; /** @@ -118,15 +117,14 @@ public function sanitize() { // If custom scripts were kept (after sanitize_js_script_elements() ran) it's important that noscripts not be // unwrapped or else this could result in the JS and no-JS fallback experiences both being present on the page. // So unwrapping is only done when no custom scripts were retained (and the sanitizer arg opts-in to unwrap). - if ( 0 === $this->kept_script_count && ! empty( $this->args['unwrap_noscripts'] ) ) { - // @todo Also only do this if px_verified_kept_node_count is 0? + if ( 0 === $this->kept_script_count && 0 === $this->px_verified_kept_node_count && ! empty( $this->args['unwrap_noscripts'] ) ) { $this->unwrap_noscript_elements(); } $sanitizer_arg_updates = []; // When there are kept custom scripts, skip tree shaking since it's likely JS will toggle classes that have associated style rules. - // @todo There should be an attribute on script tags that opt-in to keeping tree shaking and/or to indicate what class names need to be included. (Update: This is in part implemented by px_verified_node_xpaths.) + // @todo There should be an attribute on script tags that opt-in to keeping tree shaking and/or to indicate what class names need to be included. // @todo Depending on the size of the underlying stylesheets, this may need to retain the use of external styles to prevent inlining excessive CSS. This may involve writing minified CSS to disk, or skipping style processing altogether if no selector conversions are needed. if ( $this->kept_script_count > 0 ) { $sanitizer_arg_updates[ AMP_Style_Sanitizer::class ]['skip_tree_shaking'] = true; @@ -229,29 +227,13 @@ protected function sanitize_js_script_elements() { ]' ); - $px_verified_nodes = []; - if ( ! empty( $this->args['px_verified_node_xpaths'] ) ) { - foreach ( $this->args['px_verified_node_xpaths'] as $script_xpath ) { - $xpath_query = $this->dom->xpath->query( $script_xpath ); - foreach ( $xpath_query as $px_verified_node ) { - $px_verified_nodes[] = $px_verified_node; - } - } - } - /** @var Element $script */ foreach ( $scripts as $script ) { if ( DevMode::hasExemptionForNode( $script ) ) { continue; } - if ( in_array( $script, $px_verified_nodes, true ) ) { - $script->setAttributeNode( - $this->dom->createAttribute( AMP_Validation_Manager::AMP_UNVALIDATED_TAG_ATTRIBUTE ) - ); - $this->dom->documentElement->setAttributeNode( - $this->dom->createAttribute( AMP_Validation_Manager::AMP_NON_VALID_DOC_ATTRIBUTE ) - ); + if ( $script->hasAttribute( AMP_Validation_Manager::PX_VERIFIED_TAG_ATTRIBUTE ) ) { $this->px_verified_kept_node_count++; // @todo Consider forcing any PX-verified script to have async/defer if not module. For inline scripts, hack via data: URL? continue; @@ -317,21 +299,16 @@ protected function sanitize_js_script_elements() { continue; } - // Since the attribute has been PX-verified, mark it as an unvalidated attribute and move along. - if ( in_array( $event_handler_attribute, $px_verified_nodes, true ) ) { - $attr_value = $element->getAttribute( AMP_Validation_Manager::AMP_UNVALIDATED_ATTRS_ATTRIBUTE ); - if ( $attr_value ) { - $attr_value .= ' ' . $event_handler_attribute->nodeName; - } else { - $attr_value = $event_handler_attribute->nodeName; - } - $element->setAttribute( AMP_Validation_Manager::AMP_UNVALIDATED_ATTRS_ATTRIBUTE, $attr_value ); - $this->dom->documentElement->setAttributeNode( - $this->dom->createAttribute( AMP_Validation_Manager::AMP_NON_VALID_DOC_ATTRIBUTE ) - ); - $this->dom->documentElement->setAttributeNode( - $this->dom->createAttribute( AMP_Validation_Manager::AMP_NON_VALID_DOC_ATTRIBUTE ) - ); + // Since the attribute has been PX-verified, move along. + if ( + $element->hasAttribute( AMP_Validation_Manager::PX_VERIFIED_ATTRS_ATTRIBUTE ) + && + in_array( + $event_handler_attribute->nodeName, + preg_split( '/\s+/', $element->getAttribute( AMP_Validation_Manager::PX_VERIFIED_ATTRS_ATTRIBUTE ) ), + true + ) + ) { $this->px_verified_kept_node_count++; continue; } diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 93857e5d8d2..3d649e25c9f 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -901,6 +901,7 @@ public function sanitize() { * in document order. DOMNode::compareDocumentPosition() is not yet implemented. */ + // @todo Also consider skipping the processing of link and style elements that have data-px-verified-tag. $dev_mode_predicate = ''; if ( DevMode::isActiveForDocument( $this->dom ) ) { $dev_mode_predicate = sprintf( ' and not ( @%s )', AMP_Rule_Spec::DEV_MODE_ATTRIBUTE ); diff --git a/includes/validation/class-amp-validation-manager.php b/includes/validation/class-amp-validation-manager.php index 2dc2cd89fc0..3c49dc2dddb 100644 --- a/includes/validation/class-amp-validation-manager.php +++ b/includes/validation/class-amp-validation-manager.php @@ -84,6 +84,16 @@ class AMP_Validation_Manager { */ const AMP_UNVALIDATED_TAG_ATTRIBUTE = 'data-amp-unvalidated-tag'; + /** + * HTML attribute to indicate an tag/element has been verified for PX. + * + * The difference here with `data-amp-unvalidated-tag` is that the PX-verified means that the tag will work + * properly Bento components and CSS tree shaking. + * + * @var string + */ + const PX_VERIFIED_TAG_ATTRIBUTE = 'data-px-verified-tag'; + /** * HTML attribute to indicate one or more attributes are exempted from AMP validation. * @@ -93,6 +103,13 @@ class AMP_Validation_Manager { */ const AMP_UNVALIDATED_ATTRS_ATTRIBUTE = 'data-amp-unvalidated-attrs'; + /** + * HTML attribute to indicate one or more attributes have been verified for PX from AMP validation. + * + * @var string + */ + const PX_VERIFIED_ATTRS_ATTRIBUTE = 'data-px-verified-attrs'; + /** * The errors encountered when validating. * From 35485acc307bcc4ca079ec83d57a4020183f566b Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 1 Sep 2021 23:00:55 -0700 Subject: [PATCH 11/16] Prevent removal of PX-validated custom tags --- .../class-amp-tag-and-attribute-sanitizer.php | 52 +++++++++++++++---- 1 file changed, 43 insertions(+), 9 deletions(-) diff --git a/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php b/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php index ea0599befad..7ae69ea8514 100644 --- a/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php +++ b/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php @@ -10,6 +10,7 @@ use AmpProject\Amp; use AmpProject\Attribute; use AmpProject\CssLength; +use AmpProject\DevMode; use AmpProject\Dom\Document; use AmpProject\Layout; use AmpProject\Extension; @@ -420,7 +421,7 @@ private function process_node( DOMElement $node ) { // If it's not an allowed tag, replace the node with it's children. $this->replace_node_with_children( $node ); - // Return early since this node no longer exists. + // Return early since we don't know anything about this node to validate it. return null; } @@ -2558,14 +2559,40 @@ private function get_ancestor_with_matching_spec_name( DOMElement $node, $ancest * @see https://github.com/ampproject/amp-wp/issues/1100 * * @param DOMElement $node Node. + * @return bool Whether replacement was done. */ private function replace_node_with_children( DOMElement $node ) { + if ( DevMode::isExemptFromValidation( $node ) ) { + return false; + } + + // Prevent removing a tag which was exempted from validation. + if ( + $node instanceof DOMElement + && + $node->hasAttribute( AMP_Validation_Manager::AMP_UNVALIDATED_TAG_ATTRIBUTE ) + && + $node->ownerDocument + && + $node->ownerDocument->documentElement->hasAttribute( AMP_Validation_Manager::AMP_NON_VALID_DOC_ATTRIBUTE ) + ) { + return false; + } + + // Prevent removing a tag which was verified for PX. + if ( + $node instanceof DOMElement + && + $node->hasAttribute( AMP_Validation_Manager::PX_VERIFIED_TAG_ATTRIBUTE ) + ) { + return false; + } // If node has no children or no parent, just remove the node. if ( ! $node->hasChildNodes() || ! $node->parentNode ) { - $this->remove_node( $node ); - - return; + if ( $this->remove_node( $node ) ) { + return true; + } } /* @@ -2584,16 +2611,18 @@ private function replace_node_with_children( DOMElement $node ) { // Prevent double-reporting nodes that are rejected for sanitization. if ( isset( $this->should_not_replace_nodes[ $node->nodeName ] ) && in_array( $node, $this->should_not_replace_nodes[ $node->nodeName ], true ) ) { - return; + return false; } // Replace node with fragment. $should_replace = $this->should_sanitize_validation_error( [], compact( 'node' ) ); - if ( $should_replace ) { - $node->parentNode->replaceChild( $fragment, $node ); - } else { + if ( ! $should_replace ) { $this->should_not_replace_nodes[ $node->nodeName ][] = $node; + return false; } + $node->parentNode->replaceChild( $fragment, $node ); + return true; + } /** @@ -2605,6 +2634,7 @@ private function replace_node_with_children( DOMElement $node ) { * @since 0.5 * * @param DOMElement $node Node. + * @return bool Whether removal was done. */ private function remove_node( DOMElement $node ) { /** @@ -2614,7 +2644,9 @@ private function remove_node( DOMElement $node ) { */ $parent = $node->parentNode; if ( $node && $parent ) { - $this->remove_invalid_child( $node ); + if ( ! $this->remove_invalid_child( $node ) ) { + return false; + } } // @todo Does this parent removal even make sense anymore? Perhaps limit to

only. @@ -2625,6 +2657,8 @@ private function remove_node( DOMElement $node ) { $parent->removeChild( $node ); } } + + return true; } /** From 9e24f79326450eb603557425e11664cd3ae38588 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 1 Sep 2021 23:01:19 -0700 Subject: [PATCH 12/16] Remove amp attribute from pages containing PX-verified attrs --- includes/class-amp-theme-support.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 733762b28df..ac3b387ae06 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -2153,6 +2153,9 @@ static function( Optimizer\Error $error ) { $dom->documentElement->hasAttribute( AMP_Validation_Manager::AMP_NON_VALID_DOC_ATTRIBUTE ) || ( $dom->documentElement->hasAttribute( DevMode::DEV_MODE_ATTRIBUTE ) && ! is_user_logged_in() ) + || + // @todo It would be preferable if we didn't have to do this query since we've already encountered an attribute. + $dom->xpath->query( sprintf( '//*/@%s | //*/@%s', AMP_Validation_Manager::PX_VERIFIED_TAG_ATTRIBUTE, AMP_Validation_Manager::PX_VERIFIED_ATTRS_ATTRIBUTE ) )->length > 0 ) { $dom->documentElement->removeAttribute( Attribute::AMP ); $dom->documentElement->removeAttribute( Attribute::AMP_EMOJI ); From ee1c3eaf40eba7872679a9b424a01a133067403c Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 2 Sep 2021 11:53:23 -0700 Subject: [PATCH 13/16] Rework form sanitizer to always leave POST forms alone or convert * Raise error when `action-xhr` appears when native POST forms are used. * Eliminate redundant `amp_is_native_post_form_allowed()`. * Refine side effects of keeping non-AMP scripts vs PX-verified ones. --- includes/amp-helper-functions.php | 29 ++---------- .../sanitizers/class-amp-form-sanitizer.php | 45 +++++++++---------- .../sanitizers/class-amp-script-sanitizer.php | 21 ++++++--- .../class-amp-validation-error-taxonomy.php | 4 +- src/SandboxingLevels.php | 35 ++++++--------- tests/php/test-amp-form-sanitizer.php | 36 +++++---------- tests/php/test-amp-helper-functions.php | 15 ------- tests/php/test-amp-script-sanitizer.php | 2 +- tests/php/test-class-amp-theme-support.php | 15 +++---- 9 files changed, 71 insertions(+), 131 deletions(-) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index 74d89c80eed..8e59e089383 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -1416,26 +1416,6 @@ function amp_is_native_img_used() { return (bool) apply_filters( 'amp_native_img_used', false ); } -/** - * Determine whether to allow native `POST` forms without conversion to use the `action-xhr` attribute and use the amp-form component. - * - * @since 2.2 - * @link https://github.com/ampproject/amphtml/issues/27638 - * - * @return bool Whether to allow native `POST` forms. - */ -function amp_is_native_post_form_allowed() { - /** - * Filters whether to allow native `POST` forms without conversion to use the `action-xhr` attribute and use the amp-form component. - * - * @since 2.2 - * @link https://github.com/ampproject/amphtml/issues/27638 - * - * @param bool $use_native Whether to allow native `POST` forms. - */ - return (bool) apply_filters( 'amp_native_post_form_allowed', false ); -} - /** * Get content sanitizers. * @@ -1480,8 +1460,7 @@ function amp_get_content_sanitizers( $post = null ) { AMP_Theme_Support::TRANSITIONAL_MODE_SLUG === AMP_Options_Manager::get_option( Option::THEME_SUPPORT ) ); - $native_img_used = amp_is_native_img_used(); - $native_post_forms_allowed = amp_is_native_post_form_allowed(); + $native_img_used = amp_is_native_img_used(); $sanitizers = [ // Embed sanitization must come first because it strips out custom scripts associated with embeds. @@ -1516,9 +1495,7 @@ function amp_get_content_sanitizers( $post = null ) { 'align_wide_support' => current_theme_supports( 'align-wide' ), 'native_img_used' => $native_img_used, ], - AMP_Form_Sanitizer::class => [ - 'native_post_forms_allowed' => $native_post_forms_allowed, - ], + AMP_Form_Sanitizer::class => [], AMP_Video_Sanitizer::class => [], AMP_Audio_Sanitizer::class => [], AMP_Object_Sanitizer::class => [], @@ -1641,7 +1618,7 @@ function amp_get_content_sanitizers( $post = null ) { // Force core essential sanitizers to appear at the end at the end, with non-essential and third-party sanitizers appearing before. $expected_final_sanitizer_order = [ AMP_Core_Theme_Sanitizer::class, // Must come before script sanitizer since onclick attributes are removed. - AMP_Comments_Sanitizer::class, // Must come before AMP_Script_Sanitizer since it either removes comment-rely.js or marks it as PX-verified. + AMP_Comments_Sanitizer::class, // Must come before AMP_Script_Sanitizer since it either removes comment-rely.js or marks it as PX-verified. Also must come before the AMP_Form_Sanitizer. AMP_Script_Sanitizer::class, // Must come before sanitizers for image, video, audio, form, and style. AMP_Srcset_Sanitizer::class, AMP_Img_Sanitizer::class, diff --git a/includes/sanitizers/class-amp-form-sanitizer.php b/includes/sanitizers/class-amp-form-sanitizer.php index 92c01e89892..b2188c45fd0 100644 --- a/includes/sanitizers/class-amp-form-sanitizer.php +++ b/includes/sanitizers/class-amp-form-sanitizer.php @@ -21,11 +21,11 @@ class AMP_Form_Sanitizer extends AMP_Base_Sanitizer { /** - * Validation error code emitted when native POST forms are opted-into and one is encountered. + * Validation error code emitted when there is a POST form with action-xhr but only native POST forms are used. * * @var string */ - const FORM_HAS_POST_METHOD_WITHOUT_ACTION_XHR_ATTR = 'FORM_HAS_POST_METHOD_WITHOUT_ACTION_XHR_ATTR'; + const POST_FORM_HAS_ACTION_XHR_WHEN_NATIVE_USED = 'POST_FORM_HAS_ACTION_XHR_WHEN_NATIVE_USED'; /** * Placeholder for default args, to be set in child classes. @@ -33,14 +33,14 @@ class AMP_Form_Sanitizer extends AMP_Base_Sanitizer { * @var array */ protected $DEFAULT_ARGS = [ - 'native_post_forms_allowed' => false, + 'native_post_forms_used' => false, ]; /** * Array of flags used to control sanitization. * * @var array { - * @type bool $native_post_forms_allowed When true, a user can decide via validation error status to convert to an XHR form. + * @type bool $native_post_forms_used When true, the amp-form extension will be omitted and any form[method=post][action] elements will not be converted into [action-xhr]. * } */ protected $args; @@ -108,28 +108,27 @@ public function sanitize() { $node->setAttribute( 'action', $action_url ); } } elseif ( 'post' === $method ) { - // If native POST forms are allowed, raise an error that gives the user the option to convert to an - // AMP-valid form with action-xhr. If the status of the validation error is 'removed', then the - // conversion will be performed. Otherwise, if the status is 'kept' then the POST form will be retained - // in the page by marking it with AMP dev mode, and the amp-form extension will be omitted from being - // added to the page. - if ( ! $xhr_action && $this->args['native_post_forms_allowed'] ) { - $validation_error = [ - 'code' => self::FORM_HAS_POST_METHOD_WITHOUT_ACTION_XHR_ATTR, - ]; - if ( ! $this->should_sanitize_validation_error( $validation_error, compact( 'node' ) ) ) { - // Opt-in to dev mode to prevent raising validation errors for an intentionally invalid . - // It doesn't make sense to raise a validation error to allow the user to decide whether to convert from - // to since the native_img_used arg is the opt-in to not do any such conversion. - // @todo Remove once https://github.com/ampproject/amphtml/issues/30442 lands. - $node->setAttributeNode( - $this->dom->createAttribute( AMP_Validation_Manager::AMP_UNVALIDATED_TAG_ATTRIBUTE ) - ); - $this->dom->documentElement->setAttributeNode( - $this->dom->createAttribute( AMP_Validation_Manager::AMP_NON_VALID_DOC_ATTRIBUTE ) + // If native post forms are used, then mark any POST forms as being unvalidated for AMP. Note that it is + // an all or nothing proposition with forms, where there cannot be some POST forms with [action] and + // others with [action-xhr]. The former is incompatible with the amp-form extension but the latter + // fundamentally depends on it. So it's one or the other. + if ( $this->args['native_post_forms_used'] ) { + if ( $xhr_action ) { + // @todo Consider rewriting action-xhr to action? Or include a shim which implements the amp-form functionality? + $this->remove_invalid_child( + $node, + [ 'code' => self::POST_FORM_HAS_ACTION_XHR_WHEN_NATIVE_USED ] ); continue; } + + $node->setAttributeNode( + $this->dom->createAttribute( AMP_Validation_Manager::AMP_UNVALIDATED_TAG_ATTRIBUTE ) + ); + $this->dom->documentElement->setAttributeNode( + $this->dom->createAttribute( AMP_Validation_Manager::AMP_NON_VALID_DOC_ATTRIBUTE ) + ); + continue; } $node->removeAttribute( 'action' ); diff --git a/includes/sanitizers/class-amp-script-sanitizer.php b/includes/sanitizers/class-amp-script-sanitizer.php index 7ec02c016e6..11916f5f5aa 100644 --- a/includes/sanitizers/class-amp-script-sanitizer.php +++ b/includes/sanitizers/class-amp-script-sanitizer.php @@ -123,11 +123,23 @@ public function sanitize() { $sanitizer_arg_updates = []; - // When there are kept custom scripts, skip tree shaking since it's likely JS will toggle classes that have associated style rules. + // When there are kept custom scripts, turn off conversion to AMP components since scripts may be attempting to + // query for them directly, and skip tree shaking since it's likely JS will toggle classes that have associated + // style rules. // @todo There should be an attribute on script tags that opt-in to keeping tree shaking and/or to indicate what class names need to be included. // @todo Depending on the size of the underlying stylesheets, this may need to retain the use of external styles to prevent inlining excessive CSS. This may involve writing minified CSS to disk, or skipping style processing altogether if no selector conversions are needed. if ( $this->kept_script_count > 0 ) { - $sanitizer_arg_updates[ AMP_Style_Sanitizer::class ]['skip_tree_shaking'] = true; + $sanitizer_arg_updates[ AMP_Style_Sanitizer::class ]['skip_tree_shaking'] = true; + $sanitizer_arg_updates[ AMP_Form_Sanitizer::class ]['native_post_forms_used'] = true; + $sanitizer_arg_updates[ AMP_Video_Sanitizer::class ]['native_video_used'] = true; + $sanitizer_arg_updates[ AMP_Audio_Sanitizer::class ]['native_audio_used'] = true; + $sanitizer_arg_updates[ AMP_Iframe_Sanitizer::class ]['native_iframe_used'] = true; + + // Once amp-img is deprecated, these won't be needed and an won't prevent strict sandboxing level for valid AMP. + // Note that AMP_Core_Theme_Sanitizer would have already run, so we can't update it here. Nevertheless, + // the native_img_used flag was already enabled by the SandboxingLevels service. + $sanitizer_arg_updates[ AMP_Gallery_Block_Sanitizer::class ]['native_img_used'] = true; + $sanitizer_arg_updates[ AMP_Img_Sanitizer::class ]['native_img_used'] = true; } // When custom scripts are on the page, use Bento AMP components whenever possible and turn off some CSS @@ -135,11 +147,6 @@ public function sanitize() { if ( $this->px_verified_kept_node_count > 0 || $this->kept_script_count > 0 ) { $sanitizer_arg_updates[ AMP_Tag_And_Attribute_Sanitizer::class ]['prefer_bento'] = true; $sanitizer_arg_updates[ AMP_Style_Sanitizer::class ]['transform_important_qualifiers'] = false; - $sanitizer_arg_updates[ AMP_Img_Sanitizer::class ]['native_img_used'] = true; - $sanitizer_arg_updates[ AMP_Video_Sanitizer::class ]['native_video_used'] = true; - $sanitizer_arg_updates[ AMP_Audio_Sanitizer::class ]['native_audio_used'] = true; - $sanitizer_arg_updates[ AMP_Iframe_Sanitizer::class ]['native_iframe_used'] = true; - $sanitizer_arg_updates[ AMP_Form_Sanitizer::class ]['native_post_forms_allowed'] = true; } foreach ( $sanitizer_arg_updates as $sanitizer_class => $sanitizer_args ) { diff --git a/includes/validation/class-amp-validation-error-taxonomy.php b/includes/validation/class-amp-validation-error-taxonomy.php index d8c5e96a69a..6060b6dc78e 100644 --- a/includes/validation/class-amp-validation-error-taxonomy.php +++ b/includes/validation/class-amp-validation-error-taxonomy.php @@ -3341,10 +3341,10 @@ static function ( $attribute_name ) { '' . esc_html( $validation_error['node_name'] ) . '' ); - case AMP_Form_Sanitizer::FORM_HAS_POST_METHOD_WITHOUT_ACTION_XHR_ATTR: + case AMP_Form_Sanitizer::POST_FORM_HAS_ACTION_XHR_WHEN_NATIVE_USED: return sprintf( /* translators: %1$s is 'POST', %2$s is 'action-xhr' */ - esc_html__( 'Form has %1$s method without %2$s attribute. (Mark as kept to prevent conversion to AMP.)', 'amp' ), + esc_html__( 'Native %1$s form has %2$s attribute.', 'amp' ), 'POST', 'action-xhr' ); diff --git a/src/SandboxingLevels.php b/src/SandboxingLevels.php index 2c899d29c3c..4b841dc8250 100644 --- a/src/SandboxingLevels.php +++ b/src/SandboxingLevels.php @@ -7,13 +7,16 @@ namespace AmpProject\AmpWP; +use AMP_Comments_Sanitizer; +use AMP_Core_Theme_Sanitizer; +use AMP_Form_Sanitizer; +use AMP_Gallery_Block_Sanitizer; +use AMP_Img_Sanitizer; use AMP_Options_Manager; -use AmpProject\AmpWP\Infrastructure\Registerable; +use AMP_Script_Sanitizer; use AmpProject\AmpWP\Infrastructure\Conditional; +use AmpProject\AmpWP\Infrastructure\Registerable; use AmpProject\AmpWP\Infrastructure\Service; -use AMP_Form_Sanitizer; -use AMP_Comments_Sanitizer; -use AMP_Script_Sanitizer; /** * Experimental service to facilitate flexible AMP. @@ -137,9 +140,6 @@ public function add_hooks() { $sandboxing_level = AMP_Options_Manager::get_option( self::OPTION_SANDBOXING_LEVEL ); - // Allow native POST forms, but they won't be converted by default (unless on level 3, per below). - add_filter( 'amp_native_post_form_allowed', '__return_true' ); - // Opt-in to the new script sanitization logic in the script sanitizer. add_filter( 'amp_content_sanitizers', @@ -147,7 +147,13 @@ static function ( $sanitizers ) use ( $sandboxing_level ) { $sanitizers[ AMP_Script_Sanitizer::class ]['sanitize_js_scripts'] = true; if ( $sandboxing_level < 3 ) { + $sanitizers[ AMP_Form_Sanitizer::class ]['native_post_forms_used'] = true; $sanitizers[ AMP_Comments_Sanitizer::class ]['allow_commenting_scripts'] = true; + + // Once amp-img is deprecated, these won't be needed and an won't prevent strict sandboxing level for valid AMP. + $sanitizers[ AMP_Img_Sanitizer::class ]['native_img_used'] = true; + $sanitizers[ AMP_Core_Theme_Sanitizer::class ]['native_img_used'] = true; + $sanitizers[ AMP_Gallery_Block_Sanitizer::class ]['native_img_used'] = true; } return $sanitizers; } @@ -158,21 +164,6 @@ static function ( $sanitizers ) use ( $sandboxing_level ) { add_filter( 'amp_validation_error_default_sanitized', '__return_false' ); } - if ( $sandboxing_level < 3 ) { - // Prevent conversion of POST forms to use action-xhr by default. - add_filter( - 'amp_validation_error_default_sanitized', - static function ( $sanitized, $error ) { - if ( isset( $error['code'] ) && AMP_Form_Sanitizer::FORM_HAS_POST_METHOD_WITHOUT_ACTION_XHR_ATTR === $error['code'] ) { - $sanitized = false; - } - return $sanitized; - }, - 20, - 2 - ); - } - // To facilitate testing, vary the errors by the sandboxing level. add_filter( 'amp_validation_error', diff --git a/tests/php/test-amp-form-sanitizer.php b/tests/php/test-amp-form-sanitizer.php index f47cdd23e20..34c3a074d94 100644 --- a/tests/php/test-amp-form-sanitizer.php +++ b/tests/php/test-amp-form-sanitizer.php @@ -194,33 +194,23 @@ public function get_data() { 'add_dev_mode' => true, ], ], - 'form_with_post_action_converted' => [ - '

', - '
' . $form_templates . '
', - [ - 'native_post_forms_allowed' => true, - ], - [ AMP_Form_Sanitizer::FORM_HAS_POST_METHOD_WITHOUT_ACTION_XHR_ATTR ], - ], - 'form_with_post_action_kept' => [ + 'native_form_with_post_action' => [ '
', sprintf( '
', AMP_Validation_Manager::AMP_UNVALIDATED_TAG_ATTRIBUTE ), [ - 'native_post_forms_allowed' => true, - 'keep_post_forms' => true, - 'expected_non_valid_doc' => true, + 'native_post_forms_used' => true, + 'expected_non_valid_doc' => true, ], - [ AMP_Form_Sanitizer::FORM_HAS_POST_METHOD_WITHOUT_ACTION_XHR_ATTR ], + [], ], - 'form_with_post_action-xhr_ok' => [ + 'native_form_with_post_action-xhr' => [ '
', - '
', + '', [ - 'native_post_forms_allowed' => true, - 'keep_post_forms' => true, - 'expected_non_valid_doc' => false, + 'native_post_forms_used' => true, + 'expected_non_valid_doc' => false, ], - [], + [ AMP_Form_Sanitizer::POST_FORM_HAS_ACTION_XHR_WHEN_NATIVE_USED ], ], ]; } @@ -243,13 +233,9 @@ public function test_converter( $source, $expected = null, $args = [], $expected $dom->documentElement->setAttribute( AMP_Rule_Spec::DEV_MODE_ATTRIBUTE, '' ); } - $actual_errors = []; - - $args['validation_error_callback'] = static function( $error ) use ( &$actual_errors, $args ) { + $actual_errors = []; + $args['validation_error_callback'] = static function( $error ) use ( &$actual_errors ) { $actual_errors[] = $error; - if ( AMP_Form_Sanitizer::FORM_HAS_POST_METHOD_WITHOUT_ACTION_XHR_ATTR === $error['code'] && ! empty( $args['keep_post_forms'] ) ) { - return false; - } return true; }; diff --git a/tests/php/test-amp-helper-functions.php b/tests/php/test-amp-helper-functions.php index 3ef3ef37d41..f57ccd73cf7 100644 --- a/tests/php/test-amp-helper-functions.php +++ b/tests/php/test-amp-helper-functions.php @@ -1701,21 +1701,6 @@ public function test_amp_is_native_img_used() { $this->assertFalse( amp_is_native_img_used() ); } - /** - * Test amp_is_native_post_form_allowed(). - * - * @covers ::amp_is_native_post_form_allowed() - */ - public function test_amp_is_native_post_form_allowed() { - $this->assertFalse( amp_is_native_post_form_allowed(), 'Expected to be disabled by default for now.' ); - - add_filter( 'amp_native_post_form_allowed', '__return_true' ); - $this->assertTrue( amp_is_native_post_form_allowed() ); - - add_filter( 'amp_native_post_form_allowed', '__return_false', 20 ); - $this->assertFalse( amp_is_native_post_form_allowed() ); - } - /** * Test deprecated $post param for amp_get_content_embed_handlers(). * diff --git a/tests/php/test-amp-script-sanitizer.php b/tests/php/test-amp-script-sanitizer.php index fd61018dee5..41968ab6dd0 100644 --- a/tests/php/test-amp-script-sanitizer.php +++ b/tests/php/test-amp-script-sanitizer.php @@ -434,7 +434,7 @@ public function test_cascading_sanitizer_argument_changes_with_custom_scripts( $ AMP_Form_Sanitizer::class => new AMP_Form_Sanitizer( $dom, [ - 'native_post_forms_allowed' => false, // Overridden by AMP_Script_Sanitizer when there is a kept script. + 'native_post_forms_used' => false, // Overridden by AMP_Script_Sanitizer when there is a kept script. 'validation_error_callback' => function () use ( $remove_custom_scripts ) { return $remove_custom_scripts; }, diff --git a/tests/php/test-class-amp-theme-support.php b/tests/php/test-class-amp-theme-support.php index 93bb72e8842..9c05e22c267 100644 --- a/tests/php/test-class-amp-theme-support.php +++ b/tests/php/test-class-amp-theme-support.php @@ -1870,19 +1870,14 @@ public function test_prepare_response_when_allowing_native_post_forms( $converte $this->set_template_mode( AMP_Theme_Support::STANDARD_MODE_SLUG ); add_filter( - 'amp_validation_error_default_sanitized', - static function ( $sanitized, $error ) use ( $converted ) { - if ( AMP_Form_Sanitizer::FORM_HAS_POST_METHOD_WITHOUT_ACTION_XHR_ATTR === $error['code'] ) { - $sanitized = $converted; - } - return $sanitized; - }, - 10, - 2 + 'amp_content_sanitizers', + static function ( $sanitizers ) use ( $converted ) { + $sanitizers[ AMP_Form_Sanitizer::class ]['native_post_forms_used'] = ! $converted; + return $sanitizers; + } ); wp(); - add_filter( 'amp_native_post_form_allowed', '__return_true' ); AMP_Theme_Support::init(); AMP_Theme_Support::finish_init(); ob_start(); From 5c3241e7ede47c6ac031355708120075120ce52b Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 2 Sep 2021 12:29:45 -0700 Subject: [PATCH 14/16] Test keeping non-AMP custom elements and prevent double reporting --- .../class-amp-tag-and-attribute-sanitizer.php | 23 ++++---- .../php/test-tag-and-attribute-sanitizer.php | 56 +++++++++++++++++-- 2 files changed, 64 insertions(+), 15 deletions(-) diff --git a/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php b/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php index 7ae69ea8514..fd9fb76de6a 100644 --- a/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php +++ b/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php @@ -2588,6 +2588,11 @@ private function replace_node_with_children( DOMElement $node ) { return false; } + // Prevent double-reporting nodes that are rejected for sanitization. + if ( isset( $this->should_not_replace_nodes[ $node->nodeName ] ) && in_array( $node, $this->should_not_replace_nodes[ $node->nodeName ], true ) ) { + return false; + } + // If node has no children or no parent, just remove the node. if ( ! $node->hasChildNodes() || ! $node->parentNode ) { if ( $this->remove_node( $node ) ) { @@ -2595,6 +2600,13 @@ private function replace_node_with_children( DOMElement $node ) { } } + // Replace node with fragment. + $should_replace = $this->should_sanitize_validation_error( [], compact( 'node' ) ); + if ( ! $should_replace ) { + $this->should_not_replace_nodes[ $node->nodeName ][] = $node; + return false; + } + /* * If node has children, replace it with them and push children onto stack * @@ -2609,17 +2621,6 @@ private function replace_node_with_children( DOMElement $node ) { $child = $node->firstChild; } - // Prevent double-reporting nodes that are rejected for sanitization. - if ( isset( $this->should_not_replace_nodes[ $node->nodeName ] ) && in_array( $node, $this->should_not_replace_nodes[ $node->nodeName ], true ) ) { - return false; - } - - // Replace node with fragment. - $should_replace = $this->should_sanitize_validation_error( [], compact( 'node' ) ); - if ( ! $should_replace ) { - $this->should_not_replace_nodes[ $node->nodeName ][] = $node; - return false; - } $node->parentNode->replaceChild( $fragment, $node ); return true; diff --git a/tests/php/test-tag-and-attribute-sanitizer.php b/tests/php/test-tag-and-attribute-sanitizer.php index 0ff1339e007..c3a0fb40c91 100644 --- a/tests/php/test-tag-and-attribute-sanitizer.php +++ b/tests/php/test-tag-and-attribute-sanitizer.php @@ -3993,6 +3993,54 @@ public function get_data_for_replace_node_with_children_validation_errors() { ], ], ], + + 'custom_elements-removed' => [ + 'Hello!', + 'Hello!', + [ + [ + 'node_name' => 'bar-baz', + 'parent_name' => 'foo-bar', + 'code' => AMP_Tag_And_Attribute_Sanitizer::DISALLOWED_TAG, + 'node_attributes' => [], + 'type' => AMP_Validation_Error_Taxonomy::HTML_ELEMENT_ERROR_TYPE, + 'node_type' => XML_ELEMENT_NODE, + ], + [ + 'node_name' => 'foo-bar', + 'parent_name' => 'body', + 'code' => AMP_Tag_And_Attribute_Sanitizer::DISALLOWED_TAG, + 'node_attributes' => [], + 'type' => AMP_Validation_Error_Taxonomy::HTML_ELEMENT_ERROR_TYPE, + 'node_type' => XML_ELEMENT_NODE, + ], + ], + true, + ], + + 'custom_elements-kept' => [ + 'Hello!', + 'Hello!', + [ + [ + 'node_name' => 'bar-baz', + 'parent_name' => 'foo-bar', + 'code' => AMP_Tag_And_Attribute_Sanitizer::DISALLOWED_TAG, + 'node_attributes' => [], + 'type' => AMP_Validation_Error_Taxonomy::HTML_ELEMENT_ERROR_TYPE, + 'node_type' => XML_ELEMENT_NODE, + ], + [ + 'node_name' => 'foo-bar', + 'parent_name' => 'body', + 'code' => AMP_Tag_And_Attribute_Sanitizer::DISALLOWED_TAG, + 'node_attributes' => [], + 'type' => AMP_Validation_Error_Taxonomy::HTML_ELEMENT_ERROR_TYPE, + 'node_type' => XML_ELEMENT_NODE, + ], + ], + false, + ], ]; } @@ -4005,20 +4053,20 @@ public function get_data_for_replace_node_with_children_validation_errors() { * @param string $source_content Source DOM content. * @param string $expected_content Expected content after sanitization. * @param array[] $expected_errors Expected errors. + * @param bool $sanitize Whether the invalid markup should be sanitized. */ - public function test_replace_node_with_children_validation_errors( $source_content, $expected_content, $expected_errors ) { + public function test_replace_node_with_children_validation_errors( $source_content, $expected_content, $expected_errors, $sanitize = true ) { $dom = AMP_DOM_Utils::get_dom_from_content( $source_content ); $sanitizer = new AMP_Tag_And_Attribute_Sanitizer( $dom, [ - 'validation_error_callback' => function( $error, $context ) use ( &$expected_errors ) { + 'validation_error_callback' => function( $error, $context ) use ( &$expected_errors, $sanitize ) { $expected = array_shift( $expected_errors ); $tag = $expected['node_name']; $this->assertEquals( $expected, $error ); $this->assertInstanceOf( 'DOMElement', $context['node'] ); $this->assertEquals( $tag, $context['node']->tagName ); - $this->assertEquals( $tag, $context['node']->nodeName ); - return true; + return $sanitize; }, ] ); From 68f19e064f48f837f2b593c0c4a3c88cae3d715c Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 2 Sep 2021 12:50:12 -0700 Subject: [PATCH 15/16] Remove completed todos --- includes/sanitizers/class-amp-comments-sanitizer.php | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/includes/sanitizers/class-amp-comments-sanitizer.php b/includes/sanitizers/class-amp-comments-sanitizer.php index 67cd53d9255..1bb446be87e 100644 --- a/includes/sanitizers/class-amp-comments-sanitizer.php +++ b/includes/sanitizers/class-amp-comments-sanitizer.php @@ -45,14 +45,12 @@ class AMP_Comments_Sanitizer extends AMP_Base_Sanitizer { protected $DEFAULT_ARGS = [ 'comment_live_list' => false, // @todo See . 'thread_comments' => false, // By default maps to thread_comments option. - 'allow_commenting_scripts' => false, // @todo Change this to allow_native_comments which also adds px-verified to comment form instead of validation error. + 'allow_commenting_scripts' => false, ]; /** * Pre-process the comment form and comment list for AMP. * - * @todo Fix https://github.com/ampproject/amp-wp/issues/6231 - * * @since 0.7 */ public function sanitize() { From 7048f65d5edb43d72a5b6e4322693f741b7c0ca8 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 2 Sep 2021 12:51:38 -0700 Subject: [PATCH 16/16] Fix typo in comments_live_list Fixes #4624 --- includes/sanitizers/class-amp-comments-sanitizer.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/includes/sanitizers/class-amp-comments-sanitizer.php b/includes/sanitizers/class-amp-comments-sanitizer.php index 1bb446be87e..840aefd0c24 100644 --- a/includes/sanitizers/class-amp-comments-sanitizer.php +++ b/includes/sanitizers/class-amp-comments-sanitizer.php @@ -43,7 +43,7 @@ class AMP_Comments_Sanitizer extends AMP_Base_Sanitizer { * @var array */ protected $DEFAULT_ARGS = [ - 'comment_live_list' => false, // @todo See . + 'comments_live_list' => false, 'thread_comments' => false, // By default maps to thread_comments option. 'allow_commenting_scripts' => false, ]; @@ -65,7 +65,7 @@ public function sanitize() { } } - if ( ! empty( $this->args['comments_live_list'] ) ) { + if ( $this->args['comments_live_list'] ) { $comments = $this->dom->xpath->query( '//amp-live-list/*[ @items ]/*[ starts-with( @id, "comment-" ) ]' ); foreach ( $comments as $comment ) {