From e39376a59a7437aea572ca1b3306ae0531d8ffca Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 8 Feb 2018 12:36:31 -0800 Subject: [PATCH] Prevent skipping elements that don't explicitly match an attribute spec list when missing any optional attributes --- .../class-amp-tag-and-attribute-sanitizer.php | 28 ++++++++++++++++--- ...ribute-sanitizer-private-methods-tests.php | 6 ++-- tests/test-amp-form-sanitizer.php | 10 ++++++- tests/test-tag-and-attribute-sanitizer.php | 2 +- 4 files changed, 37 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 32d243a41f8..353e95931c4 100644 --- a/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php +++ b/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`, @@ -538,6 +552,7 @@ private function validate_attr_spec_list_for_node( $node, $attr_spec_list ) { // 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++; @@ -677,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; } diff --git a/tests/amp-tag-and-attribute-sanitizer-private-methods-tests.php b/tests/amp-tag-and-attribute-sanitizer-private-methods-tests.php index dc48a7fe371..a4746c4b743 100644 --- a/tests/amp-tag-and-attribute-sanitizer-private-methods-tests.php +++ b/tests/amp-tag-and-attribute-sanitizer-private-methods-tests.php @@ -881,7 +881,7 @@ 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( @@ -889,7 +889,7 @@ 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_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( diff --git a/tests/test-amp-form-sanitizer.php b/tests/test-amp-form-sanitizer.php index 071931eeaee..9d1690d3620 100644 --- a/tests/test-amp-form-sanitizer.php +++ b/tests/test-amp-form-sanitizer.php @@ -1,10 +1,12 @@ ', '
', ), + 'jetpack_contact_form' => array( + '
hello

', + '
hello

', + ), ); } @@ -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 ); } diff --git a/tests/test-tag-and-attribute-sanitizer.php b/tests/test-tag-and-attribute-sanitizer.php index 5bb515f41d9..1549a5c23bd 100644 --- a/tests/test-tag-and-attribute-sanitizer.php +++ b/tests/test-tag-and-attribute-sanitizer.php @@ -759,7 +759,7 @@ public function get_html_data() { ), 'bad_meta_charset' => array( 'Mojibake?', - 'Mojibake?', + 'Mojibake?', // 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( '',