Sanitize invalid children of amp-story and amp-story-page elements to prevent white story of death by westonruter · Pull Request #3336 · ampproject/amp-wp

westonruter · 2019-09-24T00:04:59Z

A compatibility issue was discovered in #3321 with the Reading Time WP plugin, but it is likely going to happen with other plugins as well. The Reading Time WP plugin filters the_content to inject this at the beginning:

<span class="rt-reading-time" style="display: block;"><span class="rt-label">Reading Time: </span> <span class="rt-time">1</span> <span class="rt-label rt-postfix">minute</span></span>

This results in invalid amp-story which restricts its children to elements like amp-story-page. When this span is a direct child and the child_tag_name_oneof constraint is violated, the result is the entire amp-story being invalid and a white story of death (where the body has no children). The validation error is not helpful at all:

This problem was actually “prophesied” in #2926:

👉 A side-effect of the change here is the sanitization of AMP components with invalid children will be more draconian. For example, if an amp-story has an invalid child element, then the entire amp-story element will be removed, as opposed to the invalid child alone being removed. Nevertheless, since only AMP components have such validation constraints for children, it should not so common for this to occur in WordPress sites that are just outputting normal HTML. It only will become an issue when starting to use AMP components, and this makes #1420 much more important as it will be needed to explain why the element was removed.

So this PR fixes the problem by extending the AMP_Story_Sanitizer to preemptively remove AMP Story elements under amp-story and amp-story-page which are invalid. These are the two elements which have the child_tag_name_oneof constraint. This special case sanitizer is especially important for AMP Stories since all of the markup for a story is in post_content and is prone to be mutated with the_content filters to add elements like word counts, sharing buttons, and related posts. This PR prevents such elements from being seen by the tag-and-attribute sanitizer, thus preventing the amp-story and amp-story-page as a whole from being removed.

In the case of the span which the Reading Time WP plugin adds to the_content, the validation error now becomes much more helpful:

And no white story of death occurs.

Fixes #3321.

schlessera

Minor nitpicks only.

schlessera · 2019-09-24T10:54:01Z

includes/sanitizers/class-amp-story-sanitizer.php

 	public function sanitize() {
-		$nodes     = $this->dom->getElementsByTagName( self::$tag );
-		$num_nodes = $nodes->length;
+		$this->amp_story_tag_spec      = AMP_Allowed_Tags_Generated::get_allowed_tag( 'amp-story' )[0];


get_allowed_tag() can potentially return null and this will then throw a notice on PHP 7.4+: https://3v4l.org/pnjnl

However, I assume we fully control the allowed tags here and those we check for here can't be filtered away?

That's correct. If we update the Validator spec and it results in these tag specs being null then we'd catch it in the unit test.

@schlessera I hardened this in 685cfa1.

schlessera · 2019-09-24T10:58:22Z

includes/sanitizers/class-amp-story-sanitizer.php

-			return;
+		$amp_story_element = $this->dom->getElementsByTagName( 'amp-story' )->item( 0 );
+		if ( $amp_story_element instanceof DOMElement ) {
+			$this->sanitize_story_element( $amp_story_element );


The way this flows seems counterintuitive to me, it makes it look like sanitizing the story element is an edge case.

I would prefer it for the condition to be inversed and add an early return. Then have the sanitize_story_element() as the default next step.

schlessera · 2019-09-24T11:00:09Z

includes/sanitizers/class-amp-story-sanitizer.php

+		$node = $element->firstChild;
+		while ( $node ) {
+			$next_node = $node->nextSibling;
+			if ( $node instanceof DOMElement ) {


Same logic inversion here, I would prefer an early return (continue in this case) instead of making the main logic look like an edge case.

True, but the reason why I did it this way was because of $node = $next_node needing to run below. Otherwise, I'd have added:

if ( ! $node instanceof DOMElement ) { $node = $next_node; continue; }

But that seems worse because the logic is duplicated.

How about something like this:

$node = $element->firstChild; do { $next_node = $node->nextSibling; if ( ! $node instanceof DOMElement ) { continue; } if ( 'amp-story-page' === $node->nodeName ) { $page_number++; $this->sanitize_story_page_element( $node, $page_number ); } elseif ( ! in_array( $node->nodeName, $this->amp_story_tag_spec['tag_spec']['child_tags']['child_tag_name_oneof'], true ) ) { $this->remove_invalid_child( $node ); } } while ( $node = $next_node );

Note: This is mostly just preference here. I'll approve the changes and let you decide whether you want to make changes or not.

Thanks, I like that. However, I tried it and then there is a PHPCS compliant: WordPress.CodeAnalysis.AssignmentInCondition.FoundInWhileCondition. We can revisit later.

schlessera · 2019-09-24T11:05:26Z

includes/sanitizers/class-amp-story-sanitizer.php

+		$node = $element->firstChild;
+		while ( $node ) {
+			$next_node = $node->nextSibling;
+			if ( $node instanceof DOMElement ) {


I would also prefer an early return/continue here instead.

See reasoning above.

schlessera · 2019-09-24T11:08:19Z

tests/php/test-amp-story-sanitizer.php

+				'<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>',
+			],
 		];


Test for CTA removal is missing...?

That should be covered above by story_with_cta_on_first_page and story_with_multiple_cta_on_second_page.

westonruter · 2019-09-24T17:41:41Z

includes/sanitizers/class-amp-story-sanitizer.php

+			if ( ! isset( $rule_specs ) ) {
+				continue;
+			}
+			foreach ( $rule_specs as $rule_spec ) {


Note that the $rule_specs array only has one item in it.

… 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

* tag '1.3.0': (318 commits) Bump 1.3.0 Add inline styles for custom fonts (#3345) Limit deeply-nesting test to 200 to fix Xdebug error (#3341) Bump 1.3-RC2 (#3335) Sanitize invalid children of amp-story and amp-story-page elements to prevent white story of death (#3336) Remove unused Travis deploy stage (#3340) Implement automated accessibility testing using Axe (#3294) Only add all Google Font style rules in editor context Prevent adding AMP query var to Story URLs in Compatibility Tool Prevent attempting to redirect Stories with rejected validation errors Ensure all AMP scripts (including v0.js) get moved to the head Make sure that media picker is background types are filter correctly. Normalize style[type] attribute quote style after r46164 in WP core Fix phpunit covers tags Bump version to 1.3-RC1 Strip 100% width/height from layout=fill elements Fix issue with cut (#3246) Remove unused Google Fonts SVGs (#3289) Fix resize for non-fit text box (#3259) Use template_dir consistently as signal for transitional mode ...

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

7178db5

westonruter added the AMP Stories label Sep 24, 2019

westonruter added this to the v1.3 milestone Sep 24, 2019

googlebot added the cla: yes Signed the Google CLA label Sep 24, 2019

westonruter mentioned this pull request Sep 24, 2019

Markup prepended via the_content filter causes amp-story to be removed from page #3321

Closed

westonruter requested a review from schlessera September 24, 2019 01:01

schlessera requested changes Sep 24, 2019

View reviewed changes

schlessera reviewed Sep 24, 2019

View reviewed changes

swissspidy added the Sanitizers label Sep 24, 2019

schlessera approved these changes Sep 24, 2019

View reviewed changes

westonruter mentioned this pull request Sep 24, 2019

Bump 1.3-RC2 #3335

Merged

7 tasks

Harden logic for gathering allowed children for AMP Stories

685cfa1

westonruter commented Sep 24, 2019

View reviewed changes

westonruter merged commit 559e9a8 into develop Sep 24, 2019

westonruter deleted the fix/story-sanitization branch September 24, 2019 18:01

westonruter mentioned this pull request Oct 1, 2019

Components for amp-story and amp-story-page are removed if invalid children are rejected from sanitization #3393

Closed

Conversation

westonruter commented Sep 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

schlessera left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

westonruter commented Sep 24, 2019 •

edited

Loading