Skip to content

Commit

Permalink
Sanitize invalid children of amp-story and amp-story-page elements to…
Browse files Browse the repository at this point in the history 
… prevent white story of death (#3336)

* Sanitize invalid children of amp-story and amp-story-page elements

* Harden logic for gathering allowed children for AMP Stories
  • Loading branch information
@westonruter
westonruter committed Sep 24, 2019
1 parent c1e5cb0 commit 93e1782
Show file tree
Hide file tree
Showing 2 changed files with 114 additions and 28 deletions.
129 changes: 101 additions & 28 deletions includes/sanitizers/class-amp-story-sanitizer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,50 +20,123 @@ class AMP_Story_Sanitizer extends AMP_Base_Sanitizer {
*/
public static $tag = 'amp-story-page';

/**
* Allowed children for given tags.
*
* @var array
*/
private $tag_allowed_children = [
'amp-story' => [],
'amp-story-page' => [],
];

/**
* Sanitize the AMP elements contained by <amp-story-page> element where necessary.
*
* @since 0.2
*/
public function sanitize() {
$nodes = $this->dom->getElementsByTagName( self::$tag );
$num_nodes = $nodes->length;

if ( 0 === $num_nodes ) {
return;
}

for ( $i = $num_nodes - 1; $i >= 0; $i-- ) {
$node = $nodes->item( $i );

if ( ! $node ) {
// Gather the allowed children for amp-story and amp-story-page elements.
foreach ( array_keys( $this->tag_allowed_children ) as $tag_name ) {
$rule_specs = AMP_Allowed_Tags_Generated::get_allowed_tag( $tag_name );
if ( ! isset( $rule_specs ) ) {
continue;
}
foreach ( $rule_specs as $rule_spec ) {
if ( ! isset( $rule_spec['tag_spec']['child_tags']['child_tag_name_oneof'] ) ) {
continue;
}
$this->tag_allowed_children[ $tag_name ] = array_merge(
$this->tag_allowed_children[ $tag_name ],
$rule_spec['tag_spec']['child_tags']['child_tag_name_oneof']
);
}
}

$cta_layers = $node->getElementsByTagName( 'amp-story-cta-layer' );
$num_cta_layers = $cta_layers->length;
$amp_story_element = $this->dom->getElementsByTagName( 'amp-story' )->item( 0 );
if ( $amp_story_element instanceof DOMElement ) {
$this->sanitize_story_element( $amp_story_element );
}
}

/**
* Sanitizes usage of Call-to-Action layers.
*
* Does not use the remove_invalid_child() method
* since the withCallToActionValidation HOC in the editor
* already warns the user about improper usage.
*/
for ( $j = $num_cta_layers - 1; $j >= 0; $j-- ) {
$cta_layer_node = $cta_layers->item( $j );
/**
* Sanitize the children of an amp-story element.
*
* @param DOMElement $element An amp-story element.
*/
private function sanitize_story_element( DOMElement $element ) {
$page_number = 0;

// The first page in a story must not have a CTA layer.
if ( 0 === $i ) {
$cta_layer_node->parentNode->removeChild( $cta_layer_node );
continue;
$node = $element->firstChild;
while ( $node ) {
$next_node = $node->nextSibling;
if ( $node instanceof DOMElement ) {
if ( 'amp-story-page' === $node->nodeName ) {
$page_number++;
$this->sanitize_story_page_element( $node, $page_number );
} else {
$this->maybe_remove_disallowed_child( $node );
}
}
$node = $next_node;
}
}

if ( $j > 0 ) {
// There can only be one CTA layer.
$cta_layer_node->parentNode->removeChild( $cta_layer_node );
/**
* Sanitize the children of an amp-story-page element.
*
* @param DOMElement $element An amp-story-page element.
* @param int $page_number Page number.
*/
private function sanitize_story_page_element( DOMElement $element, $page_number ) {
$cta_layer_count = 0;

$node = $element->firstChild;
while ( $node ) {
$next_node = $node->nextSibling;
if ( $node instanceof DOMElement ) {
if ( 'amp-story-cta-layer' === $node->nodeName ) {
/*
* Remove all erroneous Call-to-Action layers.
*
* Does not use the remove_invalid_child() method
* since the withCallToActionValidation HOC in the editor
* already warns the user about improper usage.
*/
$cta_layer_count ++;
if ( 1 === $page_number || $cta_layer_count > 1 ) {
$element->removeChild( $node );
}
} else {
$this->maybe_remove_disallowed_child( $node );
}
}
$node = $next_node;
}
}

/**
* Remove an element if it is not allowed by the parent.
*
* @param DOMElement $node Child element.
* @return bool Whether the child was removed.
*/
private function maybe_remove_disallowed_child( DOMElement $node ) {
if ( ! $node->parentNode ) {
return false;
}

$parent_node_name = $node->parentNode->nodeName;
if ( empty( $this->tag_allowed_children[ $parent_node_name ] ) ) {
return false;
}

if ( in_array( $node->nodeName, $this->tag_allowed_children[ $parent_node_name ], true ) ) {
return false;
}

$this->remove_invalid_child( $node );
return true;
}
}
13 changes: 13 additions & 0 deletions tests/php/test-amp-story-sanitizer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,14 @@ public function get_data() {
'<amp-story-page><amp-story-grid-layer><p>Lorem Ipsum Demet Delorit.</p></amp-story-grid-layer></amp-story-page><amp-story-page><amp-story-grid-layer></amp-story-grid-layer><amp-story-cta-layer><a href="">Foo</a></amp-story-cta-layer><amp-story-cta-layer><a href="">Foo</a></amp-story-cta-layer></amp-story-page>',
'<amp-story-page><amp-story-grid-layer><p>Lorem Ipsum Demet Delorit.</p></amp-story-grid-layer></amp-story-page><amp-story-page><amp-story-grid-layer></amp-story-grid-layer><amp-story-cta-layer><a href="">Foo</a></amp-story-cta-layer></amp-story-page>',
],
'story_with_invalid_root_elements' => [
'<p>Word count: 4</p><amp-story-page><amp-story-grid-layer><p>Lorem Ipsum Demet Delorit.</p></amp-story-grid-layer></amp-story-page><p>Related posts: <a href="https://example.com/"><strong>Example</strong></a></p>',
'<amp-story-page><amp-story-grid-layer><p>Lorem Ipsum Demet Delorit.</p></amp-story-grid-layer></amp-story-page>',
],
'story_with_invalid_layer_siblings' => [
'<amp-story-page><p>Before layer</p><amp-story-grid-layer><p>Lorem Ipsum Demet Delorit.</p></amp-story-grid-layer><p>After layer</p></amp-story-page</p>',
'<amp-story-page><amp-story-grid-layer><p>Lorem Ipsum Demet Delorit.</p></amp-story-grid-layer></amp-story-page>',
],
];
}

Expand All @@ -57,8 +65,13 @@ public function get_data() {
* @dataProvider get_data
*/
public function test_converter( $source, $expected = null ) {
$amp_story_element = '<amp-story standalone title="Test" publisher="Tester" publisher-logo-src="https://example.com/favicons/tester-228x228.png" poster-portrait-src="https://example.com/test.jpg">%s</amp-story>';

$source = sprintf( $amp_story_element, $source );
if ( is_null( $expected ) ) {
$expected = $source;
} else {
$expected = sprintf( $amp_story_element, $expected );
}
$dom = AMP_DOM_Utils::get_dom_from_content( $source );

Expand Down

0 comments on commit 93e1782

Please sign in to comment.