Merge pull request #944 from Automattic/fix/valid-element-removal

Prevent removal of valid elements

includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php

@@ -494,13 +494,20 @@ private function validate_tag_spec_for_node( $node, $tag_spec ) {
 	 * @param DOMNode $node           Node.
 	 * @param array[] $attr_spec_list Attribute Spec list.
 	 *
-	 * @return int Number of times the attribute spec list matched. If there was a mismatch, then 0 is returned.
+	 * @return float Number of times the attribute spec list matched. If there was a mismatch, then 0 is returned. 0.5 is returned if there is an implicit match.
 	 */
 	private function validate_attr_spec_list_for_node( $node, $attr_spec_list ) {
-
-		// If node has no attributes there is no point in continuing.
+		/*
+		 * If node has no attributes there is no point in continuing, but if none of attributes
+		 * in the spec list are mandatory, then we give this a score.
+		 */
 		if ( ! $node->hasAttributes() ) {
-			return 0;
+			foreach ( $attr_spec_list as $attr_name => $attr_spec_rule ) {
+				if ( isset( $attr_spec_rule[ AMP_Rule_Spec::MANDATORY ] ) ) {
+					return 0;
+				}
+			}
+			return 0.5;
 		}
 
 		foreach ( $node->attributes as $attr_name => $attr_node ) {
@@ -522,6 +529,13 @@ private function validate_attr_spec_list_for_node( $node, $attr_spec_list ) {
 
 		$score = 0;
 
+		/*
+		 * Keep track of how many of the attribute spec rules are mandatory,
+		 * because if none are mandatory, then we will let this rule have a
+		 * score since all the invalid attributes can just be removed.
+		 */
+		$mandatory_count = 0;
+
 		/*
 		 * Iterate through each attribute rule in this attr spec list and run
 		 * the series of tests. Each filter is given a `$node`, an `$attr_name`,
@@ -530,8 +544,15 @@ private function validate_attr_spec_list_for_node( $node, $attr_spec_list ) {
 		 */
 		foreach ( $attr_spec_list as $attr_name => $attr_spec_rule ) {
 
+			// If attr spec rule is empty, then it allows anything.
+			if ( empty( $attr_spec_rule ) && $node->hasAttribute( $attr_name ) ) {
+				$score++;
+				continue;
+			}
+
 			// If a mandatory attribute is required, and attribute exists, pass.
 			if ( isset( $attr_spec_rule[ AMP_Rule_Spec::MANDATORY ] ) ) {
+				$mandatory_count++;
 				$result = $this->check_attr_spec_rule_mandatory( $node, $attr_name, $attr_spec_rule );
 				if ( AMP_Rule_Spec::PASS === $result ) {
 					$score++;
@@ -671,6 +692,11 @@ private function validate_attr_spec_list_for_node( $node, $attr_spec_list ) {
 			}
 		}
 
+		// Give the spec a score if it doesn't have any mandatory attributes.
+		if ( 0 === $mandatory_count && 0 === $score ) {
+			$score = 0.5;
+		}
+
 		return $score;
 	}
 

tests/amp-tag-and-attribute-sanitizer-private-methods-tests.php

@@ -881,15 +881,15 @@ public function get_validate_attr_spec_list_for_node_data() {
 					'node_tag_name' => 'div',
 					'attr_spec_list' => array(),
 				),
-				0,
+				0.5, // Because there are no mandatory attributes.
 			),
 			'attributes_no_spec' => array(
 				array(
 					'source' => '<div attribute1 attribute2 attribute3></div>',
 					'node_tag_name' => 'div',
 					'attr_spec_list' => array(),
 				),
-				0,
+				0.5, // Because there are no mandatory attributes.
 			),
 			'attributes_alternative_names' => array(
 				array(
@@ -905,7 +905,7 @@ public function get_validate_attr_spec_list_for_node_data() {
 						),
 					),
 				),
-				0,
+				0.5, // Because there are no mandatory attributes.
 			),
 			'attributes_mandatory' => array(
 				array(

tests/test-amp-form-sanitizer.php

@@ -1,10 +1,12 @@
 <?php
 /**
- * Tests for form sanitisation.
+ * Tests for form sanitization.
  *
  * @package AMP
  */
 
+// phpcs:disable WordPress.Arrays.MultipleStatementAlignment.DoubleArrowNotAligned
+
 /**
  * Class AMP_Form_Sanitizer_Test
  *
@@ -60,6 +62,10 @@ public function get_data() {
 				'<form method="post" action="https://example.org/" target="some_other_target"></form>',
 				'<form method="post" target="_blank" action-xhr="https://example.org/?_wp_amp_action_xhr_converted=1"><div submit-error=""><template type="amp-mustache">{{{error}}}</template></div></form>',
 			),
+			'jetpack_contact_form' => array(
+				'<form action="https://src.wordpress-develop.test/contact/#contact-form-9" method="post" class="contact-form commentsblock"><div class="element-has-attributes">hello</div><div><label for="g9-favoritenumber" class="grunion-field-label text">Favorite number</label><input type="text" name="g9-favoritenumber" id="g9-favoritenumber" value="" class="text"></div><p class="contact-submit"><input type="submit" value="Submit" class="pushbutton-wide"><input type="hidden" id="_wpnonce" name="_wpnonce" value="640996fb1e"><input type="hidden" name="_wp_http_referer" value="/contact/"><input type="hidden" name="contact-form-id" value="9"><input type="hidden" name="action" value="grunion-contact-form"><input type="hidden" name="contact-form-hash" value="df9f9136763f5eb819f433e4fe4af3447534e8cc"></p></form>',
+				'<form method="post" class="contact-form commentsblock" action-xhr="https://src.wordpress-develop.test/contact/?_wp_amp_action_xhr_converted=1#contact-form-9" target="_top"><div class="element-has-attributes">hello</div><div><label for="g9-favoritenumber" class="grunion-field-label text">Favorite number</label><input type="text" name="g9-favoritenumber" id="g9-favoritenumber" value="" class="text"></div><p class="contact-submit"><input type="submit" value="Submit" class="pushbutton-wide"><input type="hidden" id="_wpnonce" name="_wpnonce" value="640996fb1e"><input type="hidden" name="_wp_http_referer" value="/contact/"><input type="hidden" name="contact-form-id" value="9"><input type="hidden" name="action" value="grunion-contact-form"><input type="hidden" name="contact-form-hash" value="df9f9136763f5eb819f433e4fe4af3447534e8cc"></p><div submit-error=""><template type="amp-mustache">{{{error}}}</template></div></form>',
+			),
 		);
 	}
 
@@ -83,6 +89,8 @@ public function test_converter( $source, $expected = null ) {
 		$whitelist_sanitizer->sanitize();
 
 		$content = AMP_DOM_Utils::get_content_from_dom( $dom );
+		$content = preg_replace( '/(?<=>)\s+(?=<)/', '', $content );
+
 		$this->assertEquals( $expected, $content );
 	}
 

tests/test-tag-and-attribute-sanitizer.php

@@ -755,11 +755,11 @@ public function get_html_data() {
 			),
 			'bad_meta_ua_compatible' => array(
 				'<html amp><head><meta charset="utf-8"><meta http-equiv="X-UA-Compatible" content="IE=9,chrome=1"></head><body></body></html>',
-				'<html amp><head><meta charset="utf-8"></head><body></body></html>',
+				'<html amp><head><meta charset="utf-8"><meta content="IE=9,chrome=1"></head><body></body></html>', // Note the http-equiv is removed because the content violates its attribute spec.
 			),
 			'bad_meta_charset' => array(
 				'<html amp><head><meta charset="latin-1"><title>Mojibake?</title></head><body></body></html>',
-				'<html amp><head><title>Mojibake?</title></head><body></body></html>',
+				'<html amp><head><meta><title>Mojibake?</title></head><body></body></html>', // Note the charset attribute is removed because it violates the attribute spec, but the entire element is not removed because charset is not mandatory.
 			),
 			'bad_meta_viewport' => array(
 				'<html amp><head><meta charset="utf-8"><meta name="viewport" content="width=device-width"></head><body></body></html>',
@@ -773,6 +773,10 @@ public function get_html_data() {
 				'<html amp><head><meta charset="utf-8"><meta name="viewport" content="width=device-width,height=device-height,initial-scale=2,maximum-scale=3,minimum-scale=1.0,shrink-to-fit=yes,user-scalable=yes,viewport-fit=cover"></head><body></body></html>',
 				null, // No change.
 			),
+			'meta_og_property' => array(
+				'<html amp><head><meta charset="utf-8"><meta property="og:site_name" content="AMP Site"></head><body></body></html>',
+				null, // No change.
+			),
 		);
 
 		// Also include the body tests.