Revamp comments handling for sandboxing levels

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.
@@ -1491,12 +1470,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,17 +1478,24 @@ 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
+		// performed.
+		AMP_Script_Sanitizer::class            => [],
+
 		AMP_Srcset_Sanitizer::class            => [],
 		AMP_Img_Sanitizer::class               => [
 			'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_Comments_Sanitizer::class          => [
-			'comments_live_list' => ! empty( $theme_support_args['comments_live_list'] ),
-		],
+		AMP_Form_Sanitizer::class              => [],
 		AMP_Video_Sanitizer::class             => [],
 		AMP_Audio_Sanitizer::class             => [],
 		AMP_Object_Sanitizer::class            => [],
@@ -1637,12 +1617,12 @@ 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. Also must come before the AMP_Form_Sanitizer.
 		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,
-		AMP_Comments_Sanitizer::class,
 		AMP_Video_Sanitizer::class,
 		AMP_Audio_Sanitizer::class,
 		AMP_Object_Sanitizer::class,

includes/class-amp-theme-support.php

@@ -898,12 +898,6 @@ 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 );
 		add_action(
@@ -913,12 +907,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 +1046,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 +1061,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      = [];
@@ -1122,14 +1113,15 @@ 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.
 	 * @param WP_Comment $comment The object of the comment being replied.
 	 * @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() ) {
@@ -1162,14 +1154,16 @@ 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.
 	 * @param string $text           Cancel comment reply link text.
 	 * @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' );
 		}
@@ -2159,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 );

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 ) {
@@ -493,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;
@@ -568,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 ] );