Allow the validation_error_callback to return false to prevent saniti…

…zation where relevant * Move debug code into AMP_Validation_Utils class. * Remove disable_invalid_removal sanitizer arg in favor of validation_error_callback
ampproject · Apr 6, 2018 · 73f4373 · 73f4373
1 parent 7bd658d
commit 73f4373
Show file tree

Hide file tree

Showing 4 changed files with 154 additions and 82 deletions.
diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php
@@ -604,7 +604,7 @@ public static function amend_comment_form() {
 	 * @see get_query_template()
 	 *
 	 * @param array $templates Template hierarchy.
-	 * @returns array Templates.
+	 * @return array Templates.
 	 */
 	public static function filter_paired_template_hierarchy( $templates ) {
 		$support = get_theme_support( 'amp' );
@@ -991,15 +991,12 @@ public static function prepare_response( $response, $args = array() ) {
 			return $response;
 		}
 
-		$is_validation_debug_mode = ! empty( $_REQUEST[ AMP_Validation_Utils::DEBUG_QUERY_VAR ] ); // WPCS: csrf ok.
-
 		$args = array_merge(
 			array(
-				'content_max_width'       => ! empty( $content_width ) ? $content_width : AMP_Post_Template::CONTENT_MAX_WIDTH, // Back-compat.
-				'use_document_element'    => true,
-				'allow_dirty_styles'      => self::is_customize_preview_iframe(), // Dirty styles only needed when editing (e.g. for edit shortcodes).
-				'allow_dirty_scripts'     => is_customize_preview(), // Scripts are always needed to inject changeset UUID.
-				'disable_invalid_removal' => $is_validation_debug_mode,
+				'content_max_width'    => ! empty( $content_width ) ? $content_width : AMP_Post_Template::CONTENT_MAX_WIDTH, // Back-compat.
+				'use_document_element' => true,
+				'allow_dirty_styles'   => self::is_customize_preview_iframe(), // Dirty styles only needed when editing (e.g. for edit shortcodes).
+				'allow_dirty_scripts'  => is_customize_preview(), // Scripts are always needed to inject changeset UUID.
 			),
 			$args
 		);
@@ -1051,9 +1048,7 @@ public static function prepare_response( $response, $args = array() ) {
 		}
 
 		if ( AMP_Validation_Utils::should_validate_response() ) {
-			AMP_Validation_Utils::finalize_validation( $dom, array(
-				'remove_source_comments' => ! $is_validation_debug_mode,
-			) );
+			AMP_Validation_Utils::finalize_validation( $dom );
 		}
 
 		$response  = "<!DOCTYPE html>\n";

diff --git a/includes/sanitizers/class-amp-base-sanitizer.php b/includes/sanitizers/class-amp-base-sanitizer.php
@@ -54,7 +54,6 @@ abstract class AMP_Base_Sanitizer {
 	 *      @type array $amp_bind_placeholder_prefix
 	 *      @type bool $allow_dirty_styles
 	 *      @type bool $allow_dirty_scripts
-	 *      @type bool $disable_invalid_removal
 	 *      @type callable $validation_error_callback
 	 * }
 	 */
@@ -290,18 +289,22 @@ public function maybe_enforce_https_src( $src, $force_https = false ) {
 	 *
 	 * @param DOMNode|DOMElement $node The node to remove.
 	 * @param array              $args Additional args to pass to validation error callback.
-	 *
-	 * @return void
+	 * @return bool Whether the node should have been removed, that is, that the node was sanitized for validity.
 	 */
 	public function remove_invalid_child( $node, $args = array() ) {
+		$should_remove = true;
 		if ( isset( $this->args['validation_error_callback'] ) ) {
-			call_user_func( $this->args['validation_error_callback'],
+			$result = call_user_func( $this->args['validation_error_callback'],
 				array_merge( compact( 'node' ), $args )
 			);
+			if ( is_bool( $result ) ) {
+				$should_remove = $result;
+			}
 		}
-		if ( empty( $this->args['disable_invalid_removal'] ) ) {
+		if ( $should_remove ) {
 			$node->parentNode->removeChild( $node );
 		}
+		return $should_remove;
 	}
 
 	/**
@@ -315,32 +318,37 @@ public function remove_invalid_child( $node, $args = array() ) {
 	 * @param DOMElement     $element   The node for which to remove the attribute.
 	 * @param DOMAttr|string $attribute The attribute to remove from the element.
 	 * @param array          $args      Additional args to pass to validation error callback.
-	 * @return void
+	 * @return bool Whether the node should have been removed, that is, that the node was sanitized for validity.
 	 */
 	public function remove_invalid_attribute( $element, $attribute, $args = array() ) {
+		$should_remove = true;
 		if ( isset( $this->args['validation_error_callback'] ) ) {
 			if ( is_string( $attribute ) ) {
 				$attribute = $element->getAttributeNode( $attribute );
 			}
 			if ( $attribute ) {
-				call_user_func( $this->args['validation_error_callback'],
+				$result = call_user_func( $this->args['validation_error_callback'],
 					array_merge(
 						array(
 							'node' => $attribute,
 						),
 						$args
 					)
 				);
-				if ( empty( $this->args['disable_invalid_removal'] ) ) {
+				if ( is_bool( $result ) ) {
+					$should_remove = $result;
+				}
+				if ( $should_remove ) {
 					$element->removeAttributeNode( $attribute );
 				}
 			}
-		} elseif ( empty( $this->args['disable_invalid_removal'] ) ) {
+		} else {
 			if ( is_string( $attribute ) ) {
 				$element->removeAttribute( $attribute );
 			} else {
 				$element->removeAttributeNode( $attribute );
 			}
 		}
+		return $should_remove;
 	}
 }