Skip to content

Commit

Permalink
Merge pull request #2926 from ampproject/fix/child_tag_name_oneof
Browse files Browse the repository at this point in the history 
Fix handling of child_tag_name_oneof constraint in tag spec
  • Loading branch information
@westonruter
westonruter authored Aug 1, 2019
2 parents 5b5440b + cd47633 commit 66f5576
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 21 deletions.
10 changes: 4 additions & 6 deletions includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1850,7 +1850,7 @@ private function remove_disallowed_descendants( $node, $allowed_descendants ) {
}

/**
* Loop through node's children and remove the ones that are not whitelisted.
* Check whether the node validates the constraints for children.
*
* @param DOMNode $node Node.
* @param array $child_tags {
Expand Down Expand Up @@ -1878,24 +1878,22 @@ private function check_valid_children( $node, $child_tags ) {
}

// Verify that all of the child are among the set of allowed elements.
$removed_count = 0;
if ( isset( $child_tags['child_tag_name_oneof'] ) ) {
foreach ( $child_elements as $child_element ) {
if ( ! in_array( $child_element->nodeName, $child_tags['child_tag_name_oneof'], true ) ) {
$removed_count++;
$this->remove_invalid_child( $child_element );
return false;
}
}
}

// If there aren't the exact number of elements, then mark this $node as being invalid.
if ( isset( $child_tags['mandatory_num_child_tags'] ) ) {
return ( count( $child_elements ) - $removed_count ) === $child_tags['mandatory_num_child_tags'];
return count( $child_elements ) === $child_tags['mandatory_num_child_tags'];
}

// If there aren't enough elements, then mark this $node as being invalid.
if ( isset( $child_tags['mandatory_min_num_child_tags'] ) ) {
return ( count( $child_elements ) - $removed_count ) >= $child_tags['mandatory_min_num_child_tags'];
return count( $child_elements ) >= $child_tags['mandatory_min_num_child_tags'];
}

return true;
Expand Down
50 changes: 35 additions & 15 deletions tests/php/test-tag-and-attribute-sanitizer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,17 +72,23 @@ public function get_body_data() {
],

'amp-animation' => [
'<amp-animation layout="nodisplay"><span>bad</span><script type="application/json">{}</script><strong>very bad</strong></amp-animation>',
'<amp-animation layout="nodisplay"><script type="application/json">{}</script></amp-animation>',
null,
[ 'amp-animation' ],
],

'amp-call-tracking' => [
'<amp-call-tracking config="https://example.com/calltracking.json"><b>bad</b>--and not great: <a href="tel:123456789">+1 (23) 456-789</a><i>more bad</i>not great</amp-call-tracking>',
'<amp-call-tracking config="https://example.com/calltracking.json">--and not great: <a href="tel:123456789">+1 (23) 456-789</a>not great</amp-call-tracking>',
'amp-call-tracking-ok' => [
'<amp-call-tracking config="https://example.com/calltracking.json"><a href="tel:123456789">+1 (23) 456-789</a></amp-call-tracking>',
null,
[ 'amp-call-tracking' ],
],

'amp-call-tracking-bad-children' => [
'<amp-call-tracking config="https://example.com/calltracking.json"><b>bad</b>--and not great: <a href="tel:123456789">+1 (23) 456-789</a><i>more bad</i>not great</amp-call-tracking>',
'',
[],
],

'amp-call-tracking_blacklisted_config' => [
'<amp-call-tracking config="__amp_source_origin"><a href="tel:123456789">+1 (23) 456-789</a></amp-call-tracking>',
'',
Expand Down Expand Up @@ -278,9 +284,7 @@ static function () {
'',
'
<amp-story standalone live-story-disabled supports-landscape title="My Story" publisher="The AMP Team" publisher-logo-src="https://example.com/logo/1x1.png" poster-portrait-src="https://example.com/my-story/poster/3x4.jpg" poster-square-src="https://example.com/my-story/poster/1x1.jpg" poster-landscape-src="https://example.com/my-story/poster/4x3.jpg" background-audio="my.mp3">
<i>bad</i>
<amp-story-page id="my-first-page">
<i>bad</i>
<amp-story-grid-layer template="fill">
<amp-img id="object1" animate-in="rotate-in-left" src="https://example.ampproject.org/helloworld/bg1.jpg" width="900" height="1600">
</amp-img>
Expand All @@ -295,9 +299,7 @@ static function () {
</amp-story-grid-layer>
<amp-pixel src="https://example.com/tracker/foo" layout="nodisplay"></amp-pixel>
</amp-story-page>
<i>bad</i>
<amp-story-page id="my-second-page">
<i>bad</i>
<amp-analytics config="https://example.com/analytics.account.config.json"></amp-analytics>
<amp-story-grid-layer template="thirds">
<amp-img grid-area="bottom-third" src="https://example.ampproject.org/helloworld/bg2.gif" width="900" height="1600">
Expand Down Expand Up @@ -325,9 +327,7 @@ static function () {
</amp-twitter>
</amp-story-page-attachment>
</amp-story-page>
<i>bad</i>
<amp-story-bookend src="bookendv1.json" layout="nodisplay"></amp-story-bookend>
<i>bad</i>
<amp-analytics id="75a1fdc3143c" type="googleanalytics"><script type="application/json">{"vars":{"account":"UA-XXXXXX-1"},"triggers":{"trackPageview":{"on":"visible","request":"pageview"}}}</script></amp-analytics>
</amp-story>
'
Expand Down Expand Up @@ -568,7 +568,6 @@ static function() {
'
<amp-accordion class="sample" disable-session-states="">
ok
<p>bad</p>
<section expanded>
<h4>Section 1</h4>
<p>Bunch of awesome content.</p>
Expand All @@ -585,8 +584,6 @@ static function() {
</figure>
</section>
ok
<div>bad</div>
ok
</amp-accordion>
'
);
Expand Down Expand Up @@ -735,7 +732,7 @@ static function() {
],

'remove_node_with_disallowed_ancestor_and_disallowed_child_nodes' => [
'<amp-sidebar><amp-app-banner>This node is not allowed here.</amp-app-banner><nav><i>bad</i><ul><li>Hello</li></ul><ol><li>Hello</li></ol><i>bad</i></nav><amp-app-banner>This node is not allowed here.</amp-app-banner></amp-sidebar>',
'<amp-sidebar><amp-app-banner>This node is not allowed here.</amp-app-banner><nav><ul><li>Hello</li></ul><ol><li>Hello</li></ol></nav><amp-app-banner>This node is not allowed here.</amp-app-banner></amp-sidebar>',
'<amp-sidebar><nav><ul><li>Hello</li></ul><ol><li>Hello</li></ol></nav></amp-sidebar>',
[ 'amp-sidebar' ],
],
Expand Down Expand Up @@ -790,6 +787,23 @@ static function() {
[ 'amp-sidebar' ],
],

'amp_sidebar_with_div_inside_nav' => [
'
<amp-sidebar id="mobile-sidebar" layout="nodisplay" side="right">
<nav>
<ul>
<li><a href="https://example.com/"></a></li>
</ul>
<div class="some-div">
...
</div>
</nav>
</amp-sidebar>
',
null,
[ 'amp-sidebar' ],
],

'remove_node_without_mandatory_ancestor' => [
'<div>All I have is this div, when all you want is a noscript tag.<audio>Sweet tunes</audio></div>',
'<div>All I have is this div, when all you want is a noscript tag.</div>',
Expand Down Expand Up @@ -1328,8 +1342,8 @@ static function() {
],

'amp-image-slider' => [
'<amp-image-slider layout="responsive" width="100" height="200"><span>Not allowed</span><amp-img src="/green-apple.jpg" alt="A green apple"></amp-img><i>forbidden</i><amp-img src="/red-apple.jpg" alt="A red apple"></amp-img><div first>This apple is green</div><strong>not allowed</strong><div second>This apple is red</div><i>not</i> <span>ok</span></amp-image-slider>',
'<amp-image-slider layout="responsive" width="100" height="200"><amp-img src="/green-apple.jpg" alt="A green apple"></amp-img><amp-img src="/red-apple.jpg" alt="A red apple"></amp-img><div first>This apple is green</div><div second>This apple is red</div></amp-image-slider>',
null,
[ 'amp-image-slider' ],
],

Expand All @@ -1339,6 +1353,12 @@ static function() {
[],
],

'amp-image-slider-more-bad-children' => [
'<amp-image-slider layout="responsive" width="100" height="200"><span>Not allowed</span><amp-img src="/green-apple.jpg" alt="A green apple"></amp-img><i>forbidden</i><amp-img src="/red-apple.jpg" alt="A red apple"></amp-img><div first>This apple is green</div><strong>not allowed</strong><div second>This apple is red</div><i>not</i> <span>ok</span></amp-image-slider>',
'',
[],
],

'amp-fx-collection' => [
'<h1 amp-fx="parallax" data-parallax-factor="1.5">A title that moves faster than other content.</h1>',
null,
Expand Down

0 comments on commit 66f5576

Please sign in to comment.