Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix handling of child_tag_name_oneof constraint in tag spec #2926

Merged
merged 1 commit into from
Aug 1, 2019
Merged

Fix handling of child_tag_name_oneof constraint in tag spec #2926

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php
View file
Open in desktop
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
Open in desktop
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