From c54a02d6dc9f23025def8b0fde094d5c11815a84 Mon Sep 17 00:00:00 2001
From: Weston Ruter <weston@xwp.co>
Date: Thu, 8 Feb 2018 21:05:38 -0800
Subject: [PATCH 1/5] Refactor remove_invalid_callback to take single DOMNode
 arg, whether DOMElement or DOMAttr

---
 .../sanitizers/class-amp-base-sanitizer.php   | 23 +++--
 .../class-amp-tag-and-attribute-sanitizer.php |  5 +-
 includes/utils/class-amp-validation-utils.php | 94 ++++++++-----------
 tests/test-class-amp-base-sanitizer.php       | 21 ++---
 tests/test-class-amp-theme-support.php        | 16 +++-
 tests/test-class-amp-validation-utils.php     | 82 ++++++----------
 6 files changed, 102 insertions(+), 139 deletions(-)

diff --git a/includes/sanitizers/class-amp-base-sanitizer.php b/includes/sanitizers/class-amp-base-sanitizer.php
index 8e7a936afb7..a0331d019e7 100644
--- a/includes/sanitizers/class-amp-base-sanitizer.php
+++ b/includes/sanitizers/class-amp-base-sanitizer.php
@@ -314,12 +314,12 @@ public function maybe_enforce_https_src( $src, $force_https = false ) {
 	 * @since 0.7
 	 *
 	 * @param DOMElement $child The node to remove.
-	 * @return void.
+	 * @return void
 	 */
 	public function remove_invalid_child( $child ) {
 		$child->parentNode->removeChild( $child );
 		if ( isset( $this->args['remove_invalid_callback'] ) ) {
-			call_user_func( $this->args['remove_invalid_callback'], $child, AMP_Validation_Utils::NODE_REMOVED );
+			call_user_func( $this->args['remove_invalid_callback'], $child );
 		}
 	}
 
@@ -331,14 +331,23 @@ public function remove_invalid_child( $child ) {
 	 *
 	 * @since 0.7
 	 *
-	 * @param DOMElement $element   The node for which to remove the attribute.
-	 * @param string     $attribute The attribute to remove from the node.
-	 * @return void.
+	 * @param DOMElement     $element   The node for which to remove the attribute.
+	 * @param DOMAttr|string $attribute The attribute to remove from the element.
+	 * @return void
 	 */
 	public function remove_invalid_attribute( $element, $attribute ) {
-		$element->removeAttribute( $attribute );
 		if ( isset( $this->args['remove_invalid_callback'] ) ) {
-			call_user_func( $this->args['remove_invalid_callback'], $element, AMP_Validation_Utils::ATTRIBUTE_REMOVED, $attribute );
+			if ( is_string( $attribute ) ) {
+				$attribute = $element->getAttributeNode( $attribute );
+			}
+			if ( $attribute ) {
+				$element->removeAttributeNode( $attribute );
+				call_user_func( $this->args['remove_invalid_callback'], $attribute );
+			}
+		} elseif ( is_string( $attribute ) ) {
+			$element->removeAttribute( $attribute );
+		} else {
+			$element->removeAttributeNode( $attribute );
 		}
 	}
 
diff --git a/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php b/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php
index b4c0faeee31..bdc9aa54e72 100644
--- a/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php
+++ b/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php
@@ -729,10 +729,7 @@ private function sanitize_disallowed_attributes_in_node( $node, $attr_spec_list
 		}
 
 		foreach ( $attrs_to_remove as $attr ) {
-			$node->removeAttributeNode( $attr );
-			if ( isset( $this->args['remove_invalid_callback'], $attr->name ) ) {
-				call_user_func( $this->args['remove_invalid_callback'], $node, AMP_Validation_Utils::ATTRIBUTE_REMOVED, $attr->name );
-			}
+			$this->remove_invalid_attribute( $node, $attr );
 		}
 	}
 
diff --git a/includes/utils/class-amp-validation-utils.php b/includes/utils/class-amp-validation-utils.php
index c7450248f81..986665a4a4c 100644
--- a/includes/utils/class-amp-validation-utils.php
+++ b/includes/utils/class-amp-validation-utils.php
@@ -12,20 +12,6 @@
  */
 class AMP_Validation_Utils {
 
-	/**
-	 * The argument if an attribute was removed.
-	 *
-	 * @var array.
-	 */
-	const ATTRIBUTE_REMOVED = 'removed_attr';
-
-	/**
-	 * The argument if a node was removed.
-	 *
-	 * @var array.
-	 */
-	const NODE_REMOVED = 'removed';
-
 	/**
 	 * Key for the markup value in the REST API endpoint.
 	 *
@@ -68,24 +54,17 @@ class AMP_Validation_Utils {
 	 */
 	const ERROR_NONCE_ACTION = 'invalid_amp_message';
 
-	/**
-	 * The attributes that the sanitizer removed.
-	 *
-	 * @var array.
-	 */
-	public static $removed_attributes;
-
 	/**
 	 * The nodes that the sanitizer removed.
 	 *
-	 * @var array.
+	 * @var DOMNode[]
 	 */
-	public static $removed_nodes;
+	public static $removed_nodes = array();
 
 	/**
 	 * Add the actions.
 	 *
-	 * @return void.
+	 * @return void
 	 */
 	public static function init() {
 		add_action( 'rest_api_init', array( __CLASS__, 'amp_rest_validation' ) );
@@ -94,32 +73,13 @@ public static function init() {
 	}
 
 	/**
-	 * Tracks when a sanitizer removes an attribute or node.
+	 * Tracks when a sanitizer removes an node (element or attribute).
 	 *
-	 * @param DOMNode|DOMElement $node         The node in which there was a removal.
-	 * @param string             $removal_type The removal: 'removed_attr' for an attribute, or 'removed' for a node or element.
-	 * @param string             $attr_name    The name of the attribute removed (optional).
-	 * @return void.
-	 */
-	public static function track_removed( $node, $removal_type, $attr_name = null ) {
-		if ( ( self::ATTRIBUTE_REMOVED === $removal_type ) && isset( $attr_name ) ) { // phpcs:ignore WordPress.NamingConventions.ValidVariableName.NotSnakeCaseMemberVar
-			self::$removed_attributes = self::increment_count( self::$removed_attributes, $attr_name );
-		} elseif ( ( self::NODE_REMOVED === $removal_type ) && isset( $node->nodeName ) ) {
-			self::$removed_nodes = self::increment_count( self::$removed_nodes, $node->nodeName ); // phpcs:ignore WordPress.NamingConventions.ValidVariableName.NotSnakeCaseMemberVar
-		}
-	}
-
-	/**
-	 * Tracks when a sanitizer removes an attribute or node.
-	 *
-	 * @param array  $histogram The count of attributes or nodes removed.
-	 * @param string $key       The attribute or node name removed.
-	 * @return array $histogram The incremented histogram.
+	 * @param DOMNode $node The node which was removed.
+	 * @return void
 	 */
-	public static function increment_count( $histogram, $key ) {
-		$current_value     = isset( $histogram[ $key ] ) ? $histogram[ $key ] : 0;
-		$histogram[ $key ] = $current_value + 1;
-		return $histogram;
+	public static function track_removed( $node ) {
+		self::$removed_nodes[] = $node;
 	}
 
 	/**
@@ -212,11 +172,34 @@ public static function get_response( $markup = null ) {
 			self::process_markup( $markup );
 			$response['processed_markup'] = $markup;
 		}
-		$response = array_merge( array(
-			self::ERROR_KEY      => self::was_node_removed(),
-			'removed_nodes'      => self::$removed_nodes,
-			'removed_attributes' => self::$removed_attributes,
-		), $response );
+
+		$removed_elements   = array();
+		$removed_attributes = array();
+		foreach ( self::$removed_nodes as $node ) {
+			if ( $node instanceof DOMAttr ) {
+				if ( ! isset( $removed_attributes[ $node->nodeName ] ) ) {
+					$removed_attributes[ $node->nodeName ] = 1;
+				} else {
+					$removed_attributes[ $node->nodeName ]++;
+				}
+			} elseif ( $node instanceof DOMElement ) {
+				if ( ! isset( $removed_elements[ $node->nodeName ] ) ) {
+					$removed_elements[ $node->nodeName ] = 1;
+				} else {
+					$removed_elements[ $node->nodeName ]++;
+				}
+			}
+		}
+
+		$response = array_merge(
+			array(
+				self::ERROR_KEY => self::was_node_removed(),
+			),
+			compact(
+				'removed_elements',
+				'removed_attributes'
+			),
+		$response );
 		self::reset_removed();
 
 		return $response;
@@ -232,8 +215,7 @@ public static function get_response( $markup = null ) {
 	 * @return void.
 	 */
 	public static function reset_removed() {
-		self::$removed_nodes      = null;
-		self::$removed_attributes = null;
+		self::$removed_nodes = array();
 	}
 
 	/**
@@ -272,7 +254,7 @@ public static function validate_content( $post_id, $post ) {
 				$location = AMP_Validation_Utils::error_message( $location );
 				$location = add_query_arg(
 					array(
-						'removed_elements'   => array_keys( $response['removed_nodes'] ),
+						'removed_elements'   => array_keys( $response['removed_elements'] ),
 						'removed_attributes' => array_keys( $response['removed_attributes'] ),
 					),
 					$location
diff --git a/tests/test-class-amp-base-sanitizer.php b/tests/test-class-amp-base-sanitizer.php
index a7c6bcd8f08..ef4bab3beb5 100644
--- a/tests/test-class-amp-base-sanitizer.php
+++ b/tests/test-class-amp-base-sanitizer.php
@@ -245,26 +245,23 @@ public function test_enforce_sizes_attribute( $source_params, $expected_value, $
 	/**
 	 * Tests remove_child.
 	 *
-	 * @see AMP_Base_Sanitizer::remove_invalid_child()
+	 * @covers AMP_Base_Sanitizer::remove_invalid_child()
 	 */
 	public function test_remove_child() {
 		$parent_tag_name = 'div';
-		$child_tag_name  = 'h1';
 		$dom_document    = new DOMDocument( '1.0', 'utf-8' );
 		$parent          = $dom_document->createElement( $parent_tag_name );
 		$child           = $dom_document->createElement( 'h1' );
 		$parent->appendChild( $child );
 
-		// To ignore WordPress.NamingConventions.ValidVariableName.NotSnakeCaseMemberVar.
-		// @codingStandardsIgnoreStart
-
 		$this->assertEquals( $child, $parent->firstChild );
 		$sanitizer = new AMP_Iframe_Sanitizer( $dom_document, array(
 			'remove_invalid_callback' => 'AMP_Validation_Utils::track_removed',
 		) );
 		$sanitizer->remove_invalid_child( $child );
 		$this->assertEquals( null, $parent->firstChild );
-		$this->assertEquals( 1, AMP_Validation_Utils::$removed_nodes[ $child_tag_name ] );
+		$this->assertCount( 1, AMP_Validation_Utils::$removed_nodes );
+		$this->assertEquals( $child, AMP_Validation_Utils::$removed_nodes[0] );
 
 		$parent->appendChild( $child );
 		$this->assertEquals( $child, $parent->firstChild );
@@ -272,14 +269,13 @@ public function test_remove_child() {
 
 		$this->assertEquals( null, $parent->firstChild );
 		$this->assertEquals( null, $child->parentNode );
-		// @codingStandardsIgnoreEnd
 		AMP_Validation_Utils::$removed_nodes = null;
 	}
 
 	/**
 	 * Tests remove_child.
 	 *
-	 * @see AMP_Base_Sanitizer::remove_invalid_child()
+	 * @covers AMP_Base_Sanitizer::remove_invalid_child()
 	 */
 	public function test_remove_attribute() {
 		AMP_Validation_Utils::reset_removed();
@@ -288,20 +284,15 @@ public function test_remove_attribute() {
 		$dom_document = new DOMDocument( '1.0', 'utf-8' );
 		$video        = $dom_document->createElement( $video_name );
 		$video->setAttribute( $attribute, 'someFunction()' );
+		$attr_node = $video->getAttributeNode( $attribute );
 
-		// To ignore WordPress.NamingConventions.ValidVariableName.NotSnakeCaseMemberVar.
-		// @codingStandardsIgnoreStart
 		$args = array(
 			'remove_invalid_callback' => 'AMP_Validation_Utils::track_removed',
 		);
-		$expected_removed = array(
-			$attribute => 1,
-		);
 		$sanitizer = new AMP_Video_Sanitizer( $dom_document, $args );
 		$sanitizer->remove_invalid_attribute( $video, $attribute );
 		$this->assertEquals( null, $video->getAttribute( $attribute ) );
-		$this->assertEquals( $expected_removed, AMP_Validation_Utils::$removed_attributes );
-		// @codingStandardsIgnoreEnd
+		$this->assertEquals( array( $attr_node ), AMP_Validation_Utils::$removed_nodes );
 		AMP_Validation_Utils::reset_removed();
 	}
 
diff --git a/tests/test-class-amp-theme-support.php b/tests/test-class-amp-theme-support.php
index b71c4d41228..eacee79e26f 100644
--- a/tests/test-class-amp-theme-support.php
+++ b/tests/test-class-amp-theme-support.php
@@ -101,7 +101,7 @@ public function test_prepare_response() {
 		<html amp <?php language_attributes(); ?>>
 			<head>
 				<?php wp_head(); ?>
-				<script data-head>document.write('TODO: This needs to be sanitized as well once.');</script>
+				<script data-head>document.write('Illegal');</script>
 			</head>
 			<body>
 				<img width="100" height="100" src="https://example.com/test.png">
@@ -114,12 +114,19 @@ public function test_prepare_response() {
 					data-aax_src="302"></amp-ad>
 				<?php wp_footer(); ?>
 
+				<button onclick="alert('Illegal');">no-onclick</button>
+
 				<style>body { background: black; }</style>
 			</body>
 		</html>
 		<?php
 		$original_html  = trim( ob_get_clean() );
-		$sanitized_html = AMP_Theme_Support::prepare_response( $original_html );
+		$removed_nodes  = array();
+		$sanitized_html = AMP_Theme_Support::prepare_response( $original_html, array(
+			'remove_invalid_callback' => function( $node ) use ( &$removed_nodes ) {
+				$removed_nodes[ $node->nodeName ] = $node;
+			},
+		) );
 
 		$this->assertContains( '<meta charset="' . get_bloginfo( 'charset' ) . '">', $sanitized_html );
 		$this->assertContains( '<meta name="viewport" content="width=device-width,minimum-scale=1">', $sanitized_html );
@@ -136,6 +143,11 @@ public function test_prepare_response() {
 		$this->assertContains( '<script async custom-element="amp-audio"', $sanitized_html );
 
 		$this->assertContains( '<script async custom-element="amp-ad"', $sanitized_html );
+
+		$this->assertContains( '<button>no-onclick</button>', $sanitized_html );
+		$this->assertCount( 2, $removed_nodes );
+		$this->assertInstanceOf( 'DOMElement', $removed_nodes['script'] );
+		$this->assertInstanceOf( 'DOMAttr', $removed_nodes['onclick'] );
 	}
 
 	/**
diff --git a/tests/test-class-amp-validation-utils.php b/tests/test-class-amp-validation-utils.php
index 16f90515019..bdf086a33d9 100644
--- a/tests/test-class-amp-validation-utils.php
+++ b/tests/test-class-amp-validation-utils.php
@@ -83,36 +83,11 @@ public function test_init() {
 	 * @see AMP_Validation_Utils::track_removed()
 	 */
 	public function test_track_removed() {
-		$attr_name              = 'invalid-attr';
-		$expected_removed_attrs = array(
-			$attr_name => 1,
-		);
-		$expected_removed_nodes = array(
-			'img' => 1,
-		);
-		$this->assertEmpty( AMP_Validation_Utils::$removed_attributes );
 		$this->assertEmpty( AMP_Validation_Utils::$removed_nodes );
-		AMP_Validation_Utils::track_removed( $this->node, AMP_Validation_Utils::ATTRIBUTE_REMOVED, $attr_name );
-		$this->assertEquals( $expected_removed_attrs, AMP_Validation_Utils::$removed_attributes );
-		AMP_Validation_Utils::track_removed( $this->node, AMP_Validation_Utils::NODE_REMOVED );
-		$this->assertEquals( $expected_removed_nodes, AMP_Validation_Utils::$removed_nodes );
-	}
-
-	/**
-	 * Test increment_count.
-	 *
-	 * @see AMP_Validation_Utils::increment_count()
-	 */
-	public function test_increment_count() {
-		$attribute     = 'script';
-		$one_attribute = array(
-			$attribute => 1,
-		);
-		$expected      = array(
-			$attribute => 2,
-		);
-		$this->assertEquals( $one_attribute, AMP_Validation_Utils::increment_count( array(), $attribute ) );
-		$this->assertEquals( $expected, AMP_Validation_Utils::increment_count( $one_attribute, $attribute ) );
+		AMP_Validation_Utils::track_removed( $this->node );
+		AMP_Validation_Utils::track_removed( $this->node );
+		$this->assertEquals( array( $this->node, $this->node ), AMP_Validation_Utils::$removed_nodes );
+		AMP_Validation_Utils::reset_removed();
 	}
 
 	/**
@@ -121,9 +96,8 @@ public function test_increment_count() {
 	 * @see AMP_Validation_Utils::was_node_removed()
 	 */
 	public function test_was_node_removed() {
-		$attr_name = 'invalid-attr';
 		$this->assertFalse( AMP_Validation_Utils::was_node_removed() );
-		AMP_Validation_Utils::track_removed( $this->node, AMP_Validation_Utils::NODE_REMOVED );
+		AMP_Validation_Utils::track_removed( $this->node );
 		$this->assertTrue( AMP_Validation_Utils::was_node_removed() );
 	}
 
@@ -135,35 +109,36 @@ public function test_was_node_removed() {
 	public function test_process_markup() {
 		$this->set_authorized();
 		AMP_Validation_Utils::process_markup( $this->valid_amp_img );
-		$this->assertEquals( null, AMP_Validation_Utils::$removed_nodes );
-		$this->assertEquals( null, AMP_Validation_Utils::$removed_attributes );
+		$this->assertEquals( array(), AMP_Validation_Utils::$removed_nodes );
 
 		AMP_Validation_Utils::reset_removed();
 		$video = '<video src="https://example.com/video">';
-		AMP_Validation_Utils::process_markup( $this->valid_amp_img );
+		AMP_Validation_Utils::process_markup( $video );
 		// This isn't valid AMP, but the sanitizer should convert it to an <amp-video>, without stripping anything.
-		$this->assertEquals( null, AMP_Validation_Utils::$removed_nodes );
-		$this->assertEquals( null, AMP_Validation_Utils::$removed_attributes );
+		$this->assertEquals( array(), AMP_Validation_Utils::$removed_nodes );
 
 		AMP_Validation_Utils::reset_removed();
 
 		AMP_Validation_Utils::process_markup( $this->disallowed_tag );
-		$this->assertEquals( 1, AMP_Validation_Utils::$removed_nodes['script'] );
-		$this->assertEquals( null, AMP_Validation_Utils::$removed_attributes );
+		$this->assertCount( 1, AMP_Validation_Utils::$removed_nodes );
+		$this->assertEquals( 'script', AMP_Validation_Utils::$removed_nodes[0]->nodeName );
 
 		AMP_Validation_Utils::reset_removed();
 		$disallowed_style = '<div style="display:none"></div>';
 		AMP_Validation_Utils::process_markup( $disallowed_style );
-		$removed_attribute = AMP_Validation_Utils::$removed_attributes;
-		$this->assertEquals( null, AMP_Validation_Utils::$removed_nodes );
-		$this->assertEquals( null, $removed_attribute );
+		$this->assertEquals( array(), AMP_Validation_Utils::$removed_nodes );
 
 		AMP_Validation_Utils::reset_removed();
 		$invalid_video = '<video width="200" height="100"></video>';
 		AMP_Validation_Utils::process_markup( $invalid_video );
-		$removed_node = AMP_Validation_Utils::$removed_nodes;
-		$this->assertEquals( 1, AMP_Validation_Utils::$removed_nodes['video'] ); // phpcs:ignore WordPress.NamingConventions.ValidVariableName.NotSnakeCaseMemberVar.
-		$this->assertEquals( null, AMP_Validation_Utils::$removed_attributes );
+		$this->assertCount( 1, AMP_Validation_Utils::$removed_nodes );
+		$this->assertEquals( 'video', AMP_Validation_Utils::$removed_nodes[0]->nodeName );
+
+		AMP_Validation_Utils::reset_removed();
+		AMP_Validation_Utils::process_markup( '<button onclick="evil()">Do it</button>' );
+		$this->assertCount( 1, AMP_Validation_Utils::$removed_nodes );
+		$this->assertEquals( 'onclick', AMP_Validation_Utils::$removed_nodes[0]->nodeName );
+		AMP_Validation_Utils::reset_removed();
 	}
 
 	/**
@@ -221,10 +196,10 @@ public function test_validate_markup() {
 		$response          = AMP_Validation_Utils::validate_markup( $request );
 		$expected_response = array(
 			$this->error_key     => true,
-			'removed_nodes'      => array(
+			'removed_elements'   => array(
 				'script' => 1,
 			),
-			'removed_attributes' => null,
+			'removed_attributes' => array(),
 			'processed_markup'   => $this->disallowed_tag,
 		);
 		$this->assertEquals( $expected_response, $response );
@@ -235,8 +210,8 @@ public function test_validate_markup() {
 		$response          = AMP_Validation_Utils::validate_markup( $request );
 		$expected_response = array(
 			$this->error_key     => false,
-			'removed_nodes'      => null,
-			'removed_attributes' => null,
+			'removed_elements'   => array(),
+			'removed_attributes' => array(),
 			'processed_markup'   => $this->valid_amp_img,
 		);
 		$this->assertEquals( $expected_response, $response );
@@ -252,10 +227,10 @@ public function test_get_response() {
 		$response          = AMP_Validation_Utils::get_response( $this->disallowed_tag );
 		$expected_response = array(
 			$this->error_key     => true,
-			'removed_nodes'      => array(
+			'removed_elements'   => array(
 				'script' => 1,
 			),
-			'removed_attributes' => null,
+			'removed_attributes' => array(),
 			'processed_markup'   => $this->disallowed_tag,
 		);
 		$this->assertEquals( $expected_response, $response );
@@ -267,12 +242,9 @@ public function test_get_response() {
 	 * @see AMP_Validation_Utils::reset_removed()
 	 */
 	public function test_reset_removed() {
-		AMP_Validation_Utils::$removed_nodes[]    = $this->node;
-		AMP_Validation_Utils::$removed_attributes = array( 'onclick' );
+		AMP_Validation_Utils::$removed_nodes[] = $this->node;
 		AMP_Validation_Utils::reset_removed();
-
-		$this->assertEquals( null, AMP_Validation_Utils::$removed_nodes );
-		$this->assertEquals( null, AMP_Validation_Utils::$removed_attributes );
+		$this->assertEquals( array(), AMP_Validation_Utils::$removed_nodes );
 	}
 
 	/**

From fd561693b9810271c7af3123b4bd472d44882f09 Mon Sep 17 00:00:00 2001
From: Weston Ruter <weston@xwp.co>
Date: Fri, 9 Feb 2018 20:06:35 -0800
Subject: [PATCH 2/5] Indicate the counts for invalid elements and attributes

---
 includes/utils/class-amp-validation-utils.php | 34 +++++++++++++------
 tests/test-class-amp-base-sanitizer.php       |  2 +-
 2 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/includes/utils/class-amp-validation-utils.php b/includes/utils/class-amp-validation-utils.php
index 986665a4a4c..1847e0f29ae 100644
--- a/includes/utils/class-amp-validation-utils.php
+++ b/includes/utils/class-amp-validation-utils.php
@@ -254,8 +254,8 @@ public static function validate_content( $post_id, $post ) {
 				$location = AMP_Validation_Utils::error_message( $location );
 				$location = add_query_arg(
 					array(
-						'removed_elements'   => array_keys( $response['removed_elements'] ),
-						'removed_attributes' => array_keys( $response['removed_attributes'] ),
+						'removed_elements'   => $response['removed_elements'],
+						'removed_attributes' => $response['removed_attributes'],
 					),
 					$location
 				);
@@ -316,20 +316,32 @@ public static function display_error() {
 		if ( self::ERROR_QUERY_VALUE === $error ) {
 			echo '<div class="notice notice-warning">';
 			printf( '<p>%s</p>', esc_html__( 'Warning: There is content which fails AMP validation; it will be stripped when served as AMP.', 'amp' ) );
+			$removed_sets = array();
 			if ( ! empty( $_GET['removed_elements'] ) && is_array( $_GET['removed_elements'] ) ) {
-				printf(
-					'<p>%s %s</p>',
-					esc_html__( 'Invalid elements:', 'amp' ),
-					'<code>' . implode( '</code>, <code>', array_map( 'esc_html', $_GET['removed_elements'] ) ) . '</code>'
+				$removed_sets[] = array(
+					'label' => __( 'Invalid elements:', 'amp' ),
+					'names' => array_map( 'sanitize_key', $_GET['removed_elements'] ),
 				);
 			}
-			if ( ! empty( $_GET['removed_attributes'] ) ) {
-				printf(
-					'<p>%s %s</p>',
-					esc_html__( 'Invalid attributes:', 'amp' ),
-					'<code>' . implode( '</code>, <code>', array_map( 'esc_html', $_GET['removed_attributes'] ) ) . '</code>'
+			if ( ! empty( $_GET['removed_attributes'] ) && is_array( $_GET['removed_attributes'] ) ) {
+				$removed_sets[] = array(
+					'label' => __( 'Invalid attributes:', 'amp' ),
+					'names' => array_map( 'sanitize_key', $_GET['removed_attributes'] ),
 				);
 			}
+			foreach ( $removed_sets as $removed_set ) {
+				printf( '<p>%s ', esc_html( $removed_set['label'] ) );
+				$items = array();
+				foreach ( $removed_set['names'] as $name => $count ) {
+					if ( 1 === intval( $count ) ) {
+						$items[] = sprintf( '<code>%s</code>', esc_html( $name ) );
+					} else {
+						$items[] = sprintf( '<code>%s</code> (%d)', esc_html( $name ), $count );
+					}
+				}
+				echo implode( ', ', $items ); // WPCS: XSS OK.
+				echo '</p>';
+			}
 			echo '</div>';
 		}
 	}
diff --git a/tests/test-class-amp-base-sanitizer.php b/tests/test-class-amp-base-sanitizer.php
index ef4bab3beb5..b96bd95824c 100644
--- a/tests/test-class-amp-base-sanitizer.php
+++ b/tests/test-class-amp-base-sanitizer.php
@@ -286,7 +286,7 @@ public function test_remove_attribute() {
 		$video->setAttribute( $attribute, 'someFunction()' );
 		$attr_node = $video->getAttributeNode( $attribute );
 
-		$args = array(
+		$args      = array(
 			'remove_invalid_callback' => 'AMP_Validation_Utils::track_removed',
 		);
 		$sanitizer = new AMP_Video_Sanitizer( $dom_document, $args );

From 2305fcba6bb32e434af673e4abdac233aba5b587 Mon Sep 17 00:00:00 2001
From: Ryan Kienstra <ryan.kienstra@xwp.co>
Date: Tue, 13 Feb 2018 13:31:36 -0600
Subject: [PATCH 3/5] Issue #843: Move Sanitization reporting to
 'edit_form_top.'

Before, this was on the 'save_post' action.
But as Weston mentioned,
this could be in one place,
and this removes the need for a nonce.
Also, remove the function is_authorized().
This checked for a nonce.
Replace this with the existing has_cap().
---
 includes/utils/class-amp-validation-utils.php | 160 +++++-------------
 tests/test-class-amp-validation-utils.php     |  70 +++++---
 2 files changed, 81 insertions(+), 149 deletions(-)

diff --git a/includes/utils/class-amp-validation-utils.php b/includes/utils/class-amp-validation-utils.php
index 1847e0f29ae..b8d96e75cd8 100644
--- a/includes/utils/class-amp-validation-utils.php
+++ b/includes/utils/class-amp-validation-utils.php
@@ -26,34 +26,6 @@ class AMP_Validation_Utils {
 	 */
 	const ERROR_KEY = 'has_error';
 
-	/**
-	 * Key of the AMP error query var.
-	 *
-	 * @var string.
-	 */
-	const ERROR_QUERY_KEY = 'amp_error';
-
-	/**
-	 * Query arg value if there is an AMP error in the post content.
-	 *
-	 * @var string.
-	 */
-	const ERROR_QUERY_VALUE = '1';
-
-	/**
-	 * Nonce name for the editor error message.
-	 *
-	 * @var string.
-	 */
-	const ERROR_NONCE = 'amp_nonce';
-
-	/**
-	 * Nonce action for displaying the invalid AMP message.
-	 *
-	 * @var string.
-	 */
-	const ERROR_NONCE_ACTION = 'invalid_amp_message';
-
 	/**
 	 * The nodes that the sanitizer removed.
 	 *
@@ -68,8 +40,7 @@ class AMP_Validation_Utils {
 	 */
 	public static function init() {
 		add_action( 'rest_api_init', array( __CLASS__, 'amp_rest_validation' ) );
-		add_action( 'save_post', array( __CLASS__, 'validate_content' ), 10, 2 );
-		add_action( 'edit_form_top', array( __CLASS__, 'display_error' ) );
+		add_action( 'edit_form_top', array( __CLASS__, 'validate_content' ), 10, 2 );
 	}
 
 	/**
@@ -104,7 +75,7 @@ public static function process_markup( $markup ) {
 		$args = array(
 			'content_max_width' => ! empty( $content_width ) ? $content_width : AMP_Post_Template::CONTENT_MAX_WIDTH,
 		);
-		if ( self::is_authorized() ) {
+		if ( self::has_cap() ) {
 			$args['remove_invalid_callback'] = 'AMP_Validation_Utils::track_removed';
 		}
 		AMP_Content_Sanitizer::sanitize( $markup, amp_get_content_sanitizers(), $args );
@@ -232,118 +203,65 @@ public static function validate_arg( $arg ) {
 	}
 
 	/**
-	 * On updating a post, this checks the AMP validity of the content.
+	 * Checks the AMP validity of the post content.
 	 *
-	 * If it's not valid AMP, it adds a query arg to the redirect URL.
-	 * This will cause an error message to appear above the 'Classic' editor.
+	 * If it's not valid AMP,
+	 * it displays an error message above the 'Classic' editor.
 	 *
-	 * @param integer $post_id The ID of the updated post.
-	 * @param WP_Post $post    The updated post.
+	 * @param WP_Post $post The updated post.
 	 * @return void.
 	 */
-	public static function validate_content( $post_id, $post ) {
-		unset( $post_id );
-		if ( ! post_supports_amp( $post ) || ! self::is_authorized() ) {
+	public static function validate_content( $post ) {
+		if ( ! post_supports_amp( $post ) || ! self::has_cap() ) {
 			return;
 		}
 		/** This filter is documented in wp-includes/post-template.php */
 		$filtered_content = apply_filters( 'the_content', $post->post_content, $post->ID );
 		$response         = self::get_response( $filtered_content );
 		if ( isset( $response[ self::ERROR_KEY ] ) && ( true === $response[ self::ERROR_KEY ] ) ) {
-			add_filter( 'redirect_post_location', function( $location ) use ( $response ) {
-				$location = AMP_Validation_Utils::error_message( $location );
-				$location = add_query_arg(
-					array(
-						'removed_elements'   => $response['removed_elements'],
-						'removed_attributes' => $response['removed_attributes'],
-					),
-					$location
-				);
-				return $location;
-			} );
+			self::display_error( $response );
 		}
 	}
 
 	/**
-	 * Whether the current user is authorized.
+	 * Displays an error message on /wp-admin/post.php.
 	 *
-	 * This checks that the user has a certain capability and the nonce is valid.
-	 * It will only return true when updating the post on:
-	 * wp-admin/post.php
-	 * Avoids using check_admin_referer().
-	 * This function might be called in different places,
-	 * and it can't cause it to die() if the nonce is invalid.
-	 *
-	 * @return boolean $is_valid True if the nonce is valid.
-	 */
-	public static function is_authorized() {
-		$nonce = isset( $_REQUEST['_wpnonce'] ) ? sanitize_text_field( wp_unslash( $_REQUEST['_wpnonce'] ) ) : ''; // WPCS: CSRF ok.
-		return ( self::has_cap() && ( false !== wp_verify_nonce( $nonce, 'update-post_' . get_the_ID() ) ) );
-	}
-
-	/**
-	 * Adds an error message to the URL if it's not valid AMP.
-	 *
-	 * When redirecting after saving a post, the content was validated for AMP compliance.
-	 * If it wasn't valid AMP, this will add a query arg to the URL.
-	 * And an error message will display on /wp-admin/post.php.
-	 *
-	 * @param string $url The URL of the redirect.
-	 * @return string $url The filtered URL, including the AMP error message query var.
-	 */
-	public static function error_message( $url ) {
-		$args = array(
-			self::ERROR_QUERY_KEY => self::ERROR_QUERY_VALUE,
-			self::ERROR_NONCE     => wp_create_nonce( self::ERROR_NONCE_ACTION ),
-		);
-		return add_query_arg( $args, $url );
-	}
-
-	/**
-	 * Displays an error message on /wp-admin/post.php if the saved content is not valid AMP.
-	 *
-	 * Use $_GET, as get_query_var won't return the value.
-	 * This displays at the top of the 'Classic' editor.
+	 * Located at the top of the 'Classic' editor.
+	 * States that the content is not valid AMP.
 	 *
+	 * @param array $response The validation response, an associative array.
 	 * @return void.
 	 */
-	public static function display_error() {
-		if ( ! isset( $_GET[ self::ERROR_QUERY_KEY ] ) ) {
-			return;
+	public static function display_error( $response ) {
+		echo '<div class="notice notice-warning">';
+		printf( '<p>%s</p>', esc_html__( 'Warning: There is content which fails AMP validation; it will be stripped when served as AMP.', 'amp' ) );
+		$removed_sets = array();
+		if ( ! empty( $response['removed_elements'] ) && is_array( $response['removed_elements'] ) ) {
+			$removed_sets[] = array(
+				'label' => __( 'Invalid elements:', 'amp' ),
+				'names' => array_map( 'sanitize_key', $response['removed_elements'] ),
+			);
 		}
-		check_admin_referer( self::ERROR_NONCE_ACTION, self::ERROR_NONCE );
-		$error = isset( $_GET[ self::ERROR_QUERY_KEY ] ) ? sanitize_text_field( wp_unslash( $_GET[ self::ERROR_QUERY_KEY ] ) ) : ''; // WPCS: CSRF ok.
-		if ( self::ERROR_QUERY_VALUE === $error ) {
-			echo '<div class="notice notice-warning">';
-			printf( '<p>%s</p>', esc_html__( 'Warning: There is content which fails AMP validation; it will be stripped when served as AMP.', 'amp' ) );
-			$removed_sets = array();
-			if ( ! empty( $_GET['removed_elements'] ) && is_array( $_GET['removed_elements'] ) ) {
-				$removed_sets[] = array(
-					'label' => __( 'Invalid elements:', 'amp' ),
-					'names' => array_map( 'sanitize_key', $_GET['removed_elements'] ),
-				);
-			}
-			if ( ! empty( $_GET['removed_attributes'] ) && is_array( $_GET['removed_attributes'] ) ) {
-				$removed_sets[] = array(
-					'label' => __( 'Invalid attributes:', 'amp' ),
-					'names' => array_map( 'sanitize_key', $_GET['removed_attributes'] ),
-				);
-			}
-			foreach ( $removed_sets as $removed_set ) {
-				printf( '<p>%s ', esc_html( $removed_set['label'] ) );
-				$items = array();
-				foreach ( $removed_set['names'] as $name => $count ) {
-					if ( 1 === intval( $count ) ) {
-						$items[] = sprintf( '<code>%s</code>', esc_html( $name ) );
-					} else {
-						$items[] = sprintf( '<code>%s</code> (%d)', esc_html( $name ), $count );
-					}
+		if ( ! empty( $response['removed_attributes'] ) && is_array( $response['removed_attributes'] ) ) {
+			$removed_sets[] = array(
+				'label' => __( 'Invalid attributes:', 'amp' ),
+				'names' => array_map( 'sanitize_key', $response['removed_attributes'] ),
+			);
+		}
+		foreach ( $removed_sets as $removed_set ) {
+			printf( '<p>%s ', esc_html( $removed_set['label'] ) );
+			$items = array();
+			foreach ( $removed_set['names'] as $name => $count ) {
+				if ( 1 === intval( $count ) ) {
+					$items[] = sprintf( '<code>%s</code>', esc_html( $name ) );
+				} else {
+					$items[] = sprintf( '<code>%s</code> (%d)', esc_html( $name ), $count );
 				}
-				echo implode( ', ', $items ); // WPCS: XSS OK.
-				echo '</p>';
 			}
-			echo '</div>';
+			echo implode( ', ', $items ); // WPCS: XSS OK.
+			echo '</p>';
 		}
+		echo '</div>';
 	}
 
 }
diff --git a/tests/test-class-amp-validation-utils.php b/tests/test-class-amp-validation-utils.php
index bdf086a33d9..98ac9f54e43 100644
--- a/tests/test-class-amp-validation-utils.php
+++ b/tests/test-class-amp-validation-utils.php
@@ -73,8 +73,7 @@ public function setUp() {
 	 */
 	public function test_init() {
 		$this->assertEquals( 10, has_action( 'rest_api_init', 'AMP_Validation_Utils::amp_rest_validation' ) );
-		$this->assertEquals( 10, has_action( 'save_post', 'AMP_Validation_Utils::validate_content' ) );
-		$this->assertEquals( 10, has_action( 'edit_form_top', 'AMP_Validation_Utils::display_error' ) );
+		$this->assertEquals( 10, has_action( 'edit_form_top', 'AMP_Validation_Utils::validate_content' ) );
 	}
 
 	/**
@@ -262,26 +261,32 @@ public function test_validate_arg() {
 	}
 
 	/**
-	 * Test is_authorized
+	 * Test validate_content
 	 *
-	 * @see AMP_Validation_Utils::is_authorized()
+	 * @see AMP_Validation_Utils::validate_content()
 	 */
-	public function test_is_authorized() {
-		$this->assertFalse( AMP_Validation_Utils::is_authorized() );
-		$this->set_authorized();
-		$this->assertTrue( AMP_Validation_Utils::is_authorized() );
-	}
+	public function test_validate_content() {
+		wp_set_current_user( $this->factory()->user->create( array(
+			'role' => 'administrator',
+		) ) );
+		$post = $this->factory()->post->create_and_get();
+		ob_start();
+		AMP_Validation_Utils::validate_content( $post );
+		$output = ob_get_clean();
 
-	/**
-	 * Test error_message().
-	 *
-	 * @see AMP_Validation_Utils::error_message().
-	 */
-	public function test_error_message() {
-		$url            = 'https://example.org/am';
-		$with_query_arg = wp_parse_url( AMP_Validation_Utils::error_message( $url ) );
-		$this->assertContains( AMP_Validation_Utils::ERROR_QUERY_KEY, $with_query_arg['query'] );
-		$this->assertContains( AMP_Validation_Utils::ERROR_QUERY_VALUE, $with_query_arg['query'] );
+		$this->assertNotContains( 'notice notice-warning', $output );
+		$this->assertNotContains( 'Warning:', $output );
+
+		$post_id            = $this->factory()->post->create();
+		$post               = get_post( $post_id );
+		$post->post_content = $this->disallowed_tag;
+		ob_start();
+		AMP_Validation_Utils::validate_content( $post );
+		$output = ob_get_clean();
+
+		$this->assertContains( 'notice notice-warning', $output );
+		$this->assertContains( 'Warning:', $output );
+		$this->assertContains( '<code>script</code>', $output );
 	}
 
 	/**
@@ -290,24 +295,33 @@ public function test_error_message() {
 	 * @see AMP_Validation_Utils::display_error().
 	 */
 	public function test_display_error() {
-		wp_set_current_user( $this->factory()->user->create( array(
-			'role' => 'administrator',
-		) ) );
-		unset( $_GET[ AMP_Validation_Utils::ERROR_QUERY_KEY ] );
+		$response = array(
+			AMP_Validation_Utils::ERROR_KEY => false,
+		);
 		ob_start();
-		AMP_Validation_Utils::display_error();
+		AMP_Validation_Utils::display_error( $response );
 		$output = ob_get_clean();
 		$this->assertFalse( strpos( $output, 'notice notice-error' ) );
 		$this->assertFalse( strpos( $output, 'Notice: your post fails AMP validation' ) );
 
-		$_GET[ AMP_Validation_Utils::ERROR_QUERY_KEY ] = AMP_Validation_Utils::ERROR_QUERY_VALUE;
-		$_REQUEST[ AMP_Validation_Utils::ERROR_NONCE ] = wp_create_nonce( AMP_Validation_Utils::ERROR_NONCE_ACTION );
-		$_REQUEST['_wp_http_referer']                  = admin_url();
+		$removed_element   = 'script';
+		$removed_attribute = 'onload';
+		$response          = array(
+			AMP_Validation_Utils::ERROR_KEY => true,
+			'removed_elements'              => array(
+				$removed_element => 1,
+			),
+			'removed_attributes'            => array(
+				$removed_attribute => 1,
+			),
+		);
 		ob_start();
-		AMP_Validation_Utils::display_error();
+		AMP_Validation_Utils::display_error( $response );
 		$output = ob_get_clean();
 		$this->assertContains( 'notice notice-warning', $output );
 		$this->assertContains( 'Warning:', $output );
+		$this->assertContains( $removed_element, $output );
+		$this->assertContains( $removed_attribute, $output );
 	}
 
 	/**

From e7f896d88dc9a434ab9abde74ec38e4dee1b4c4f Mon Sep 17 00:00:00 2001
From: Ryan Kienstra <ryan.kienstra@xwp.co>
Date: Tue, 13 Feb 2018 14:41:38 -0600
Subject: [PATCH 4/5] Issue #843: Prevent displayling extra errors for <p>.

Because 'wpautop' runs on 'the_content',
process_markup() showed errors from removing <p> tags.
For example, it created this markup:
<p><script async src=https://example.com/script></script></p>
It seemed to have removed the <p> tag,
As it contained a disallowed element.
But it's not needed to report that the <p> is removed.
So remove 'wpautop' as a callback for 'the_content.'
Gutenberg also does this, unless the post has no block.
@see gutenberg_wpautop().
---
 includes/utils/class-amp-validation-utils.php |  8 +++++++-
 tests/test-class-amp-validation-utils.php     | 14 ++++++++++++--
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/includes/utils/class-amp-validation-utils.php b/includes/utils/class-amp-validation-utils.php
index b8d96e75cd8..dd539573ab6 100644
--- a/includes/utils/class-amp-validation-utils.php
+++ b/includes/utils/class-amp-validation-utils.php
@@ -72,7 +72,12 @@ public static function was_node_removed() {
 	 * @return void.
 	 */
 	public static function process_markup( $markup ) {
-		$args = array(
+		AMP_Theme_Support::register_content_embed_handlers();
+		remove_filter( 'the_content', 'wpautop' );
+
+		/** This filter is documented in wp-includes/post-template.php */
+		$markup = apply_filters( 'the_content', $markup );
+		$args   = array(
 			'content_max_width' => ! empty( $content_width ) ? $content_width : AMP_Post_Template::CONTENT_MAX_WIDTH,
 		);
 		if ( self::has_cap() ) {
@@ -215,6 +220,7 @@ public static function validate_content( $post ) {
 		if ( ! post_supports_amp( $post ) || ! self::has_cap() ) {
 			return;
 		}
+		AMP_Theme_Support::register_content_embed_handlers();
 		/** This filter is documented in wp-includes/post-template.php */
 		$filtered_content = apply_filters( 'the_content', $post->post_content, $post->ID );
 		$response         = self::get_response( $filtered_content );
diff --git a/tests/test-class-amp-validation-utils.php b/tests/test-class-amp-validation-utils.php
index 98ac9f54e43..0be1cbcf39f 100644
--- a/tests/test-class-amp-validation-utils.php
+++ b/tests/test-class-amp-validation-utils.php
@@ -277,8 +277,6 @@ public function test_validate_content() {
 		$this->assertNotContains( 'notice notice-warning', $output );
 		$this->assertNotContains( 'Warning:', $output );
 
-		$post_id            = $this->factory()->post->create();
-		$post               = get_post( $post_id );
 		$post->post_content = $this->disallowed_tag;
 		ob_start();
 		AMP_Validation_Utils::validate_content( $post );
@@ -287,6 +285,18 @@ public function test_validate_content() {
 		$this->assertContains( 'notice notice-warning', $output );
 		$this->assertContains( 'Warning:', $output );
 		$this->assertContains( '<code>script</code>', $output );
+		AMP_Validation_Utils::reset_removed();
+
+		$youtube            = 'https://www.youtube.com/watch?v=GGS-tKTXw4Y';
+		$post->post_content = $youtube;
+		ob_start();
+		AMP_Validation_Utils::validate_content( $post );
+		$output = ob_get_clean();
+
+		// The YouTube embed handler should convert the URL into a valid AMP element.
+		$this->assertNotContains( 'notice notice-warning', $output );
+		$this->assertNotContains( 'Warning:', $output );
+		AMP_Validation_Utils::reset_removed();
 	}
 
 	/**

From 18a877e369263884109a800cfe3075b71de6fb8d Mon Sep 17 00:00:00 2001
From: Ryan Kienstra <ryan.kienstra@xwp.co>
Date: Tue, 13 Feb 2018 14:48:38 -0600
Subject: [PATCH 5/5] Issue #843: Exit early if ! has_cap().

Instead of wrapping the 'invalid_callback' in this,
simply exist process_markup().
If that callback isn't added,
there's no need for the rest of the function.
---
 includes/utils/class-amp-validation-utils.php | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/includes/utils/class-amp-validation-utils.php b/includes/utils/class-amp-validation-utils.php
index dd539573ab6..6b79504e7f8 100644
--- a/includes/utils/class-amp-validation-utils.php
+++ b/includes/utils/class-amp-validation-utils.php
@@ -72,17 +72,19 @@ public static function was_node_removed() {
 	 * @return void.
 	 */
 	public static function process_markup( $markup ) {
+		if ( ! self::has_cap() ) {
+			return;
+		}
+
 		AMP_Theme_Support::register_content_embed_handlers();
 		remove_filter( 'the_content', 'wpautop' );
 
 		/** This filter is documented in wp-includes/post-template.php */
 		$markup = apply_filters( 'the_content', $markup );
 		$args   = array(
-			'content_max_width' => ! empty( $content_width ) ? $content_width : AMP_Post_Template::CONTENT_MAX_WIDTH,
+			'content_max_width'       => ! empty( $content_width ) ? $content_width : AMP_Post_Template::CONTENT_MAX_WIDTH,
+			'remove_invalid_callback' => 'AMP_Validation_Utils::track_removed',
 		);
-		if ( self::has_cap() ) {
-			$args['remove_invalid_callback'] = 'AMP_Validation_Utils::track_removed';
-		}
 		AMP_Content_Sanitizer::sanitize( $markup, amp_get_content_sanitizers(), $args );
 	}