Fix handling of Mustache templates by kienstra · Pull Request #4583 · ampproject/amp-wp

kienstra · 2020-04-14T20:14:21Z

Summary

Skip validating attributes for layout (e.g. width, height, and sizes) when elements are inside of Mustache templates (in both template elements and script elements).
Skip validating attributes containing Mustache template variables when the element is inside of a Mustache template.
Ensure that form sanitizer recognizes Mustache templates located in script[type=text/plain][template=amp-template] elements in addition to template elements.
Skip processing style attributes that contain Mustache template variables inside of script[type=text/plain][template=amp-template] elements in addition to template elements.
Ensure that URL attributes in Mustache templates do not have Mustache template variables URL-encoded.

For tags that are direct descendants of <template type="amp-mustache"> or <script type="text/plain" template="amp-mustache">, this prevents an error from a dynamic width or height variable, like <amp-img width="{{my_width}}">.

Fixes https://wordpress.org/support/topic/binding-expressions-flagged-as-invalid-markup/#post-12673829

Fixes #4446.

Steps to reproduce

Copy this into a Custom HTML block:

<amp-list
  width="auto"
  height="100"
  layout="fixed-height"
  src="/static/inline-examples/data/amp-list-urls.json"
>
    <template type="amp-mustache">
        <amp-img srcset="{{srcset}}" src="https://sdf.com" width="{{width}}" height="{{height}}" layout="responsive" alt="Episode thumbnail for {{title}}"></amp-img>
    </template>
</amp-list>

Save the post
Expected: there's no validation error
Actual: theres an error:

With this PR, there isn't an error.

Checklist

My pull request is addressing an open issue (please create one otherwise).
My code is tested and passes existing tests.
My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

kienstra · 2020-04-14T20:15:25Z

includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php

 		}

+		if ( $this->is_inside_mustache_template( $node ) ) {
+			return true;


Maybe this goes too far in not validating the layout at all if it's inside a <template type="amp-mustache"> or <script type="text/plain" template="amp-mustache>"

Let's use the AMP validator as the guide:

If any of the layout attributes (layout, width, height, heights, or sizes) have a Mustache template variable in them, then the validation is skipped.

This is the logic that we need to replicate: https://github.com/ampproject/amphtml/blob/c083d2c6120a251dcc9b0beb33c0336c7d3ca5a8/validator/engine/validator.js#L4038-L4054

Example AMP page with samples:

<!doctype html> <html ⚡> <head> <meta charset="utf-8"> <link rel="canonical" href="self.html" /> <meta name="viewport" content="width=device-width,minimum-scale=1"> <style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript> <script async src="https://cdn.ampproject.org/v0.js"></script> <script async custom-template="amp-mustache" src="https://cdn.ampproject.org/v0/amp-mustache-0.2.js"></script> </head> <body> <template type="amp-mustache"><div><div><div><amp-img src="/img1.png" layout="bad" width="bad" height="bad"></amp-img></div></div></div></template> <template type="amp-mustache"><div><div><div><amp-img src="/img1.png" layout="{{layout}}" width="bad" height="bad"></amp-img></div></div></div></template> <template type="amp-mustache"><div><div><div><amp-img src="/img1.png" layout="bad" width="{{width}}" height="bad"></amp-img></div></div></div></template> <template type="amp-mustache"><div><div><div><amp-img src="/img1.png" layout="bad" width="bad" height="{{height}}"></amp-img></div></div></div></template> <template type="amp-mustache"><div><div><div><amp-img src="/img1.png" layout="bad" width="bad" height="bad" heights="{{height}}"></amp-img></div></div></div></template> <template type="amp-mustache"><div><div><div><amp-img src="/img1.png" layout="bad" width="bad" height="bad" heights="bad" sizes="{{sizes}}"></amp-img></div></div></div></template> </body> </html>

Sounds good, I'll make a commit for that if you'd like.

Augment the test cases with the samples above.

Sure, 3d994a9 applies this suggestion.

I'll add some more test cases.

includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php

westonruter · 2020-04-14T21:19:01Z

includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php

 		}

+		if ( $this->is_inside_mustache_template( $node ) ) {
+			return true;


Let's use the AMP validator as the guide:

If any of the layout attributes (layout, width, height, heights, or sizes) have a Mustache template variable in them, then the validation is skipped.

This is the logic that we need to replicate: https://github.com/ampproject/amphtml/blob/c083d2c6120a251dcc9b0beb33c0336c7d3ca5a8/validator/engine/validator.js#L4038-L4054

Example AMP page with samples:

<!doctype html> <html ⚡> <head> <meta charset="utf-8"> <link rel="canonical" href="self.html" /> <meta name="viewport" content="width=device-width,minimum-scale=1"> <style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript> <script async src="https://cdn.ampproject.org/v0.js"></script> <script async custom-template="amp-mustache" src="https://cdn.ampproject.org/v0/amp-mustache-0.2.js"></script> </head> <body> <template type="amp-mustache"><div><div><div><amp-img src="/img1.png" layout="bad" width="bad" height="bad"></amp-img></div></div></div></template> <template type="amp-mustache"><div><div><div><amp-img src="/img1.png" layout="{{layout}}" width="bad" height="bad"></amp-img></div></div></div></template> <template type="amp-mustache"><div><div><div><amp-img src="/img1.png" layout="bad" width="{{width}}" height="bad"></amp-img></div></div></div></template> <template type="amp-mustache"><div><div><div><amp-img src="/img1.png" layout="bad" width="bad" height="{{height}}"></amp-img></div></div></div></template> <template type="amp-mustache"><div><div><div><amp-img src="/img1.png" layout="bad" width="bad" height="bad" heights="{{height}}"></amp-img></div></div></div></template> <template type="amp-mustache"><div><div><div><amp-img src="/img1.png" layout="bad" width="bad" height="bad" heights="bad" sizes="{{sizes}}"></amp-img></div></div></div></template> </body> </html>

kienstra · 2020-04-14T22:48:56Z

I'm going to push a commit for the very minor PHPCS issue if that's alright.

westonruter · 2020-04-14T23:13:26Z

I just noticed this PR is targeting the 1.5 branch when it should be develop. I'm going to rebase.

When there's a mustache template that has an element like <amp-img width="{{width}}">, there was an issue with invalid width. But this proabably doesn't need to be validated.

…g attributes Fixes #4446

Add 2 spaces, in response to a Travis issue.

westonruter · 2020-04-14T23:16:11Z

I'll squash merge this PR into develop and then cherry-pick the squashed commit into 1.5.

… }} syntax Similar to the one in the AMP validator, as Weston pointed out.

includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php

…izer Co-Authored-By: Weston Ruter <westonruter@google.com>

westonruter · 2020-04-14T23:36:24Z

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

+			],
+
+			'inside_amp_mustache_template_sizes'          => [
+				'<template type="amp-mustache"><div><div><div><amp-img src="https://example.com/image.jpg" sizes="{{sizes}}" layout="responsive" width="200" height="400" alt="Lake vacation"></amp-img></div></div></div></template>',


I suggest having multiple amp-img elements in here somewhat like:

<amp-img src="/img1.png" layout="bad" width="bad" height="bad"></amp-img> <amp-img src="/img1.png" layout="{{layout}}" width="bad" height="bad"></amp-img> <amp-img src="/img1.png" layout="bad" width="{{width}}" height="bad"></amp-img> <amp-img src="/img1.png" layout="bad" width="bad" height="{{height}}"></amp-img> <amp-img src="/img1.png" layout="bad" width="bad" height="bad" heights="{{height}}"></amp-img> <amp-img src="/img1.png" layout="bad" width="bad" height="bad" heights="bad" sizes="{{sizes}}"></amp-img>

Where the first one is invalid but the others confirm all at once that the various attributes are accounted for.

669285e adds something similar to this.

@kienstra Does testing those actually make the most sense in the AMP_Layout_Sanitizer_Test? Wouldn't it make better sense to test

I realize that the above sample should probably rather (or in addition) be added to the tag-and-attribute tests because this is where the mustache template vars are being checked, right? When doing so, it shows a difference of behavior from the AMP Validator. See 891c108.

Now, it could be argued that the AMP plugin is actually doing something better because it is removing the attributes that could never be valid even when Mustache template handling is being performed. If that is the case, we should codify the behavior in this mustache_templates_with_variable_attrs test case to show how it behaves.

Yeah, that test above isn't very useful. It's hard to get a failing test there, as the layout sanitizer mainly deals with layout="fill" and layout="fixed-height".

With your 891c108, I could revert the test above if you'd like.

Like Attribute::LAYOUT in place of 'layout'.

includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php

westonruter · 2020-04-14T23:53:44Z

includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php

+		if ( ! empty( $this->open_elements[ Tag::TEMPLATE ] ) ) {
+			while ( $node->parentNode instanceof DOMElement ) {
+				$node = $node->parentNode;
+				if ( Tag::TEMPLATE === $node->nodeName && Extension::MUSTACHE === $node->getAttribute( Attribute::TYPE ) ) {
+					return true;
+				}
+			}
+		} elseif ( ! empty( $this->open_elements[ Tag::SCRIPT ] ) ) {
+			while ( $node->parentNode instanceof DOMElement ) {
+				$node = $node->parentNode;
+				if ( Tag::SCRIPT === $node->nodeName && Extension::MUSTACHE === $node->getAttribute( Attribute::TEMPLATE ) && Attribute::TYPE_TEXT_PLAIN === $node->getAttribute( Attribute::TYPE ) ) {
+					return true;
+				}
+			}
+		}


Note the style sanitizer is currently accomplishing this via:

return 0 !== $this->dom->xpath->query( '//template[ @type="amp-mustache" ]//.|//script[ @template="amp-mustache" and @type="text/plain" ]//.', $node )->length;

We can do one better here because we have open_elements which the style sanitizer has.

I don't think we necessarily need to switch to using XPath.

kienstra · 2020-04-14T23:59:38Z

lib/common/src/Dom/Document.php

+     *
+     * @var string
+     */
+    const XPATH_MUSTACHE_TEMPLATE_ELEMENTS_QUERY = './/self::template[ @type = "amp-mustache" ]|//self::script[ @type = "text/plain" and @template = "amp-mustache" ]';


Good idea to add this constant

kienstra · 2020-04-15T00:09:23Z

Here's another snippet that looks good in a Custom HTML block now:

<amp-list class="amp-2col-list" width="auto" height="530" layout="fixed-height" load-more="auto" src="https://example.com/something">
  <div placeholder>Loading ...</div>
  <div fallback>Failed to load Healthy Software Developer Episodes.</div>
  <template type="amp-mustache">
    <div class="list-item post">
      <a href="{{url}}">Link Here</a>
    </div>
    <amp-img src="{{src}}" srcset="{{srcset}}" width="{{width}}" height="{{height}}" layout="responsive" alt="Episode Thumbnail Image for {{title}}"></amp-img>
  </template>
</amp-list>

It's very close to the one reported in https://wordpress.org/support/topic/binding-expressions-flagged-as-invalid-markup/#post-12673757

westonruter · 2020-04-15T00:15:20Z

tests/php/test-tag-and-attribute-sanitizer.php

+					<!--✅--><amp-img src="/img2.png" layout="{{layout}}" width="bad" height="bad"></amp-img>
+					<!--✅--><amp-img src="/img3.png" layout="bad" width="{{width}}" height="bad"></amp-img>
+					<!--✅--><amp-img src="/img4.png" layout="bad" width="bad" height="{{height}}"></amp-img>
+					<!--✅--><amp-img src="/img5.png" layout="bad" width="bad" height="bad" heights="{{height}}"></amp-img>
+					<!--✅--><amp-img src="/img6.png" layout="bad" width="bad" height="bad" heights="bad" sizes="{{sizes}}"></amp-img>


These are all valid in the AMP validator. But the the sanitizer is removing some of the attributes here.

See comment: #4583 (comment)

I realize that the above sample should probably rather (or in addition) be added to the tag-and-attribute tests because this is where the mustache template vars are being checked, right? When doing so, it shows a difference of behavior from the AMP Validator. See 891c108.

Now, it could be argued that the AMP plugin is actually doing something better because it is removing the attributes that could never be valid even when Mustache template handling is being performed. If that is the case, we should codify the behavior in this mustache_templates_with_variable_attrs test case to show how it behaves.

It's currently getting sanitized as:

<amp-img src="/img2.png" layout="{{layout}}" width="bad" height="bad"></amp-img> <amp-img src="/img3.png" width="{{width}}" height="bad"></amp-img> <amp-img src="/img4.png" width="bad" height="{{height}}"></amp-img> <amp-img src="/img5.png" width="bad" height="bad" heights="{{height}}"></amp-img> <amp-img src="/img6.png" width="bad" height="bad" heights="bad" sizes="{{sizes}}"></amp-img>

…_with_variable_attrs

westonruter · 2020-04-15T00:30:03Z

tests/php/test-tag-and-attribute-sanitizer.php

+					<amp-img src="/img3.png" width="{{width}}" height="bad" heights="bad" sizes="bad"></amp-img>
+					<amp-img src="/img4.png" width="bad" height="{{height}}" heights="bad" sizes="bad"></amp-img>
+					<amp-img src="/img5.png" width="bad" height="bad" heights="{{heights}}" sizes="bad"></amp-img>
+					<amp-img src="/img6.png" width="bad" height="bad" heights="bad" sizes="{{sizes}}"></amp-img>


For some reason only the layout=bad attribute is getting removed. Why?

The value would not match the list of supported layouts:

amp-wp/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php

Lines 656 to 660 in 214d9e6

if ( isset( $tag_spec['amp_layout']['supported_layouts'] ) ) {

$layouts = wp_array_slice_assoc( AMP_Rule_Spec::$layout_enum, $tag_spec['amp_layout']['supported_layouts'] );

$merged_attr_spec_list['layout'][ AMP_Rule_Spec::VALUE_REGEX_CASEI ] = '(' . implode( '|', $layouts ) . ')';

}

westonruter · 2020-04-15T00:31:22Z

tests/php/test-tag-and-attribute-sanitizer.php

+					<amp-img src="/img2.png" layout="{{layout}}" width="bad" height="bad" heights="bad" sizes="bad"></amp-img>
+					<amp-img src="/img3.png" layout="bad" width="{{width}}" height="bad" heights="bad" sizes="bad"></amp-img>
+					<amp-img src="/img4.png" layout="bad" width="bad" height="{{height}}" heights="bad" sizes="bad"></amp-img>
+					<amp-img src="/img5.png" layout="bad" width="bad" height="bad" heights="{{heights}}" sizes="bad"></amp-img>
+					<amp-img src="/img6.png" layout="bad" width="bad" height="bad" heights="bad" sizes="{{sizes}}"></amp-img>


These are allowed by the AMP validator, though we can argue they shouldn't be since they will never be valid.

pierlon · 2020-04-15T11:45:52Z

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

+				<form id="myform" role="search" class="search-form amp-wp-f2a1aff" method="get" action="https://example.com/" target="_top" data-amp-original-style="color: blue">
+					<amp-autocomplete filter="substring" items="." filter-value="title" max-entries="6" min-characters="2" submit-on-enter="" src="https://example.com/autocomplete/">
+						<script template="amp-mustache" type="text/plain" id="amp-template-custom">
+							<div class="city-item amp-wp-d4ea4c7" data-value="{{city}}, {{state}}" data-amp-original-style="outline: solid 1px black;">


Should the style attribute be renamed to data-amp-original-style here? I thought it would skip doing so with:

amp-wp/includes/sanitizers/class-amp-style-sanitizer.php

Lines 2667 to 2673 in 82bcaed

// Skip processing stylesheets that contain mustache template variables if the element is inside of a mustache template.

if (

preg_match( '/{{[^}]+?}}/', $value ) &&

0 !== $this->dom->xpath->query( '//template[ @type="amp-mustache" ]//.|//script[ @template="amp-mustache" and @type="text/plain" ]//.', $element )->length

) {

return;

}

No, it is correct to be replaced here. It's only style attributes that contain Mustache template variables that get skipped.

pierlon · 2020-04-15T12:14:59Z

tests/php/test-tag-and-attribute-sanitizer.php

+					<amp-img src="/img3.png" width="{{width}}" height="bad" heights="bad" sizes="bad"></amp-img>
+					<amp-img src="/img4.png" width="bad" height="{{height}}" heights="bad" sizes="bad"></amp-img>
+					<amp-img src="/img5.png" width="bad" height="bad" heights="{{heights}}" sizes="bad"></amp-img>
+					<amp-img src="/img6.png" width="bad" height="bad" heights="bad" sizes="{{sizes}}"></amp-img>


The value would not match the list of supported layouts:

amp-wp/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php

Lines 656 to 660 in 214d9e6

if ( isset( $tag_spec['amp_layout']['supported_layouts'] ) ) {

$layouts = wp_array_slice_assoc( AMP_Rule_Spec::$layout_enum, $tag_spec['amp_layout']['supported_layouts'] );

$merged_attr_spec_list['layout'][ AMP_Rule_Spec::VALUE_REGEX_CASEI ] = '(' . implode( '|', $layouts ) . ')';

}

pierlon · 2020-04-15T12:54:27Z

Aside from this PR, I noticed the mustache placeholder {{/ is defined twice in \AmpProject\Dom\Document::getMustacheTagPlaceholders:

amp-wp/lib/common/src/Dom/Document.php

Lines 1255 to 1265 in 93175ab

    
           // Note: The order of these tokens is important, as it determines the order of the order of the replacements. 
        
           $tokens = [ 
        
               '{{{', 
        
               '}}}', 
        
               '{{#', 
        
               '{{^', 
        
               '{{/', 
        
               '{{/', 
        
               '{{', 
        
               '}}', 
        
           ];

Any reason as to why, or is it a typo?

schlessera · 2020-04-15T13:46:44Z

I noticed the mustache placeholder {{/ is defined twice [...] Any reason as to why, or is it a typo?

That seems to be a typo and can be removed.

westonruter · 2020-04-15T16:52:55Z

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

+
+			'inside_amp_mustache_template_simple_text'    => [
+				'<template type="amp-mustache"><span>Hello, {{first_name}}!</span></template>',
+			],
+
+			'inside_amp_mustache_template_tag'            => [
+				'<template type="amp-mustache"><amp-img src="{{url}}" layout="intrinsic" srcset="{{foo}}" width="{{example_width}}" height="{{example_height}}" alt="This is for {{title}}"></amp-img></template>',
+			],
+
+			'inside_amp_mustache_script_tag'              => [
+				'<script template="amp-mustache" type="text/plain"><amp-img src="{{url}}" layout="intrinsic" srcset="{{foo}}" width="{{example_width}}" height="{{example_height}}" alt="This is for {{title}}"></amp-img></script>',
+			],
+
+			'inside_amp_mustache_template_responsive'     => [
+				'<template type="amp-mustache"><div><div><div><amp-img src="{{url}}" layout="responsive" srcset="{{foo}}" width="{{example_width}}" height="{{example_height}}" alt="This is for {{title}}"></amp-img></div></div></div></template>',
+			],
+
+			'inside_amp_mustache_template_bad_attrs'      => [
+				'<template type="amp-mustache"><div><div><div>
+					<amp-img src="/img1.png" data-amp-layout="bad" width="bad" height="bad"></amp-img>
+					<amp-img src="/img1.png" data-amp-layout="{{layout}}" width="bad" height="bad"></amp-img>
+					<amp-img src="/img1.png" width="{{width}}" height="bad"></amp-img>
+					<amp-img src="/img1.png" data-amp-layout="bad" width="{{width}}" height="bad"></amp-img>
+					<amp-img src="/img1.png" data-amp-layout="bad" width="bad" height="{{height}}"></amp-img>
+					<amp-img src="/img1.png" data-amp-layout="bad" width="bad" height="bad" heights="{{height}}"></amp-img>
+					<amp-img src="/img1.png" data-amp-layout="bad" width="bad" height="bad" heights="bad" sizes="{{sizes}}"></amp-img>
+				</div></div></div></template>',
+				'<template type="amp-mustache"><div><div><div>
+					<amp-img src="/img1.png" width="bad" height="bad" layout="bad"></amp-img>
+					<amp-img src="/img1.png" width="bad" height="bad" layout="{{layout}}"></amp-img>
+					<amp-img src="/img1.png" width="{{width}}" height="bad"></amp-img>
+					<amp-img src="/img1.png" width="{{width}}" height="bad" layout="bad"></amp-img>
+					<amp-img src="/img1.png" width="bad" height="{{height}}" layout="bad"></amp-img>
+					<amp-img src="/img1.png" width="bad" height="bad" heights="{{height}}" layout="bad"></amp-img>
+					<amp-img src="/img1.png" width="bad" height="bad" heights="bad" sizes="{{sizes}}" layout="bad"></amp-img>
+				</div></div></div></template>',
+			],
+
+			'inside_amp_mustache_template_fill'           => [
+				'<template type="amp-mustache"><div><div><amp-img width="100%" height="10" data-amp-layout="fill" src="{{src}}"></amp-img></div></div></template>',
+				'<template type="amp-mustache"><div><div><amp-img width="auto" height="10" src="{{src}}" layout="fixed-height"></amp-img></div></div></template>',
+			],


I'm not exactly sure what these are testing since no changes were made to the layout sanitizer. Is it just to ensure the layout sanitizer passes through attributes containing Mustache template variables in the same way as if they are not using template variables?

Yeah, these can probably be deleted. I think they're from when I thought the change needed to be in the layout sanitizer.

But the layout sanitizer mainly deals with styles, layout="fill", and layout="fixed-height", which these don't test.

These are removed in 6e34e77

kienstra · 2020-04-15T17:06:29Z

Here's a build of this branch cherry-picked onto the 1.5 branch:

amp.zip

(Updated, now has amp-stories-*.css files removed)

schlessera

Build failures look unrelated.

…ustache-width-attribute * 'develop' of github.com:ampproject/amp-wp: Stub request based on test scenario (#4588)

* Fix an issue with a width attribute in a mustache template When there's a mustache template that has an element like <amp-img width="{{width}}">, there was an issue with invalid width. But this proabably doesn't need to be validated. * Align => in arrays, in response to failed Travis build * Add failing test for when element is not a direct descendant of template * Account for nodes in nested elements inside mustache templates * Reuse is_inside_mustache_template to decide whether to skip validating attributes Fixes #4446 * Address PHPCS issue Add 2 spaces, in response to a Travis issue. * Account for Mustache script templates in forms * Add a helper method for whether a node has a layout attribute with {{ }} syntax Similar to the one in the AMP validator, as Weston pointed out. * Skip processing style attributes in Mustache scripts * Commit Weston's suggestion to use the same regex from the style sanitizer Co-Authored-By: Weston Ruter <westonruter@google.com> * Use the Attribute constants in place of string literals Like Attribute::LAYOUT in place of 'layout'. * Make use of constants in is_inside_mustache_template * Also use Tag constants in is_inside_mustache_template * Add more <amp-img> to the test 'inside_amp_mustache_template_sizes' * Add failing test for differing behavior from Validator * Add heights and sizes attributes to all amp-img in mustache_templates_with_variable_attrs * Fix test by removing invalid layout attributes * Remove duplicated item in getMustacheTagPlaceholders * Revert new test in AMP_Layout_Sanitizer_Test Co-authored-by: Weston Ruter <westonruter@google.com>

* tag '1.5.3': Bump 1.5.3 Bump version to 1.5.3-RC1 Fix handling of Mustache templates (#4583) Stub request based on test scenario (#4588) Update tests after block-library/style.css changes in Gutenberg 7.9 (#4579) Restrict doing plugin upgrade routine when not in admin (#4538) Add new accessibility sanitizer (#4535) Fix unit tests (#4564) Add button into Site Health to reenable CSS transient caching (#4522) Restore unification of multi-page post content in Reader mode (#4547) Prevent styles from being removed when in Customizer preview with Standard mode (#4553) Omit Jetpack from being activated during PHPUnit test runs (#4474) Mock Facebook embed tests (#4474) Mock Imgur embed tests (#4474) Use title case for Paired Browsing link in edit post screen (#4540) Ensure that validation query vars persist through redirects (#4544) Add requirements to plugin file header (#4543) Force status code of validation responses to be 200 (#4533) Update optimizer test specs (#4527) Bump 1.5.3-alpha

…phtml-2004041903580 * 'develop' of github.com:ampproject/amp-wp: (48 commits) Bump https-proxy-agent from 2.2.2 to 2.2.4 (#4596) Update dependency babel-jest to v25.3.0 (#4550) Update dependency core-js to v3.6.5 (#4558) For the 'Preview AMP' button, use a title instead of a tooltip (#4601) Bump stable tag to 1.5.3 Fix handling of Mustache templates (#4583) Stub request based on test scenario (#4588) Return early instead of storing eventual return value in variable Improve phpdoc and logic conditions Update links in pull request template Update contributing.md with link to wiki Remove engineering.md now that it is on the wiki Remove project-management.md since only applicable to Stories Add conditions for comment feed, trackback, robots, and favicon Fix typo in global phpdoc Update tests after block-library/style.css changes in Gutenberg 7.9 (#4579) Remove special conditions for Reader mode; remove need for $exit condition in redirects Fix translators comment Add comment explaining short-circuit behavior when query var is present Update version for _doing_it_wrong() from 1.5.3 to 1.6.0 ...

…filter-list-table-row-actions * 'develop' of github.com:ampproject/amp-wp: (56 commits) Bump https-proxy-agent from 2.2.2 to 2.2.4 (#4596) Update dependency babel-jest to v25.3.0 (#4550) Update dependency core-js to v3.6.5 (#4558) For the 'Preview AMP' button, use a title instead of a tooltip (#4601) Update pull request template based on new workflow Bump stable tag to 1.5.3 Fix handling of Mustache templates (#4583) Stub request based on test scenario (#4588) Return early instead of storing eventual return value in variable Improve phpdoc and logic conditions Update links in pull request template Update contributing.md with link to wiki Remove engineering.md now that it is on the wiki Remove project-management.md since only applicable to Stories Add conditions for comment feed, trackback, robots, and favicon Fix typo in global phpdoc Update tests after block-library/style.css changes in Gutenberg 7.9 (#4579) Remove special conditions for Reader mode; remove need for $exit condition in redirects Fix translators comment Add comment explaining short-circuit behavior when query var is present ...

…widgets-registration * 'develop' of github.com:ampproject/amp-wp: (88 commits) Fix grammar typo Bump CSS cache version Update composer.lock Use patch file instead of diff Update patch: Fix parsing CSS selectors which contain commas Update php-css-parser to dev-master#bc6ec74; remove patches/php-css-parser-138-extended.patch Add test to demonstrate failure to parse class names containing escaped fractions Restrict metaboxes which appear on the validated URL screen Update hook priority in test_add_admin_hooks Restrict row actions for taxonomy terms Add test for disable-inline-width on amp-img Exclude data-ampdevmode attribute exclusion rule Update spec to 2004142326360 to remove container layout from amp-list Bump https-proxy-agent from 2.2.2 to 2.2.4 (#4596) Update dependency babel-jest to v25.3.0 (#4550) Update dependency core-js to v3.6.5 (#4558) For the 'Preview AMP' button, use a title instead of a tooltip (#4601) Update pull request template based on new workflow Bump stable tag to 1.5.3 Fix handling of Mustache templates (#4583) ...

googlebot added the cla: yes Signed the Google CLA label Apr 14, 2020

kienstra commented Apr 14, 2020

View reviewed changes

westonruter requested changes Apr 14, 2020

View reviewed changes

includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php Outdated Show resolved Hide resolved

westonruter requested changes Apr 14, 2020

View reviewed changes

westonruter added Bug Something isn't working Validation labels Apr 14, 2020

westonruter mentioned this pull request Apr 14, 2020

Ensure that Mustache templates in text/plain scripts are accounted for the same as those in template tags #4446

Closed

westonruter changed the base branch from 1.5 to develop April 14, 2020 23:13

kienstra and others added 7 commits April 14, 2020 16:15

Fix an issue with a width attribute in a mustache template

2991392

When there's a mustache template that has an element like <amp-img width="{{width}}">, there was an issue with invalid width. But this proabably doesn't need to be validated.

Align => in arrays, in response to failed Travis build

d594076

Add failing test for when element is not a direct descendant of template

89127b8

Account for nodes in nested elements inside mustache templates

3019c76

Reuse is_inside_mustache_template to decide whether to skip validatin…

60992de

…g attributes Fixes #4446

Address PHPCS issue

1b1dafa

Add 2 spaces, in response to a Travis issue.

Account for Mustache script templates in forms

93175ab

westonruter force-pushed the add/amp-mustache-width-attribute branch from 6eb41a1 to 93175ab Compare April 14, 2020 23:15

kienstra and others added 2 commits April 14, 2020 18:30

Add a helper method for whether a node has a layout attribute with {{…

3d994a9

… }} syntax Similar to the one in the AMP validator, as Weston pointed out.

Skip processing style attributes in Mustache scripts

234a493

westonruter reviewed Apr 14, 2020

View reviewed changes

includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php Outdated Show resolved Hide resolved

westonruter reviewed Apr 14, 2020

View reviewed changes

includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php Outdated Show resolved Hide resolved

Commit Weston's suggestion to use the same regex from the style sanit…

1a0acb7

…izer Co-Authored-By: Weston Ruter <westonruter@google.com>

westonruter reviewed Apr 14, 2020

View reviewed changes

Use the Attribute constants in place of string literals

a5c46f4

Like Attribute::LAYOUT in place of 'layout'.

westonruter reviewed Apr 14, 2020

View reviewed changes

includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php Outdated Show resolved Hide resolved

westonruter added 2 commits April 14, 2020 16:46

Make use of constants in is_inside_mustache_template

719cab9

Also use Tag constants in is_inside_mustache_template

214d9e6

westonruter reviewed Apr 14, 2020

View reviewed changes

westonruter requested review from pierlon and schlessera April 14, 2020 23:56

kienstra commented Apr 14, 2020

View reviewed changes

Add failing test for differing behavior from Validator

891c108

westonruter reviewed Apr 15, 2020

View reviewed changes

westonruter added 2 commits April 14, 2020 17:22

Add heights and sizes attributes to all amp-img in mustache_templates…

82bcaed

…_with_variable_attrs

Fix test by removing invalid layout attributes

8579cfe

westonruter reviewed Apr 15, 2020

View reviewed changes

pierlon reviewed Apr 15, 2020

View reviewed changes

Remove duplicated item in getMustacheTagPlaceholders

c368c24

pierlon approved these changes Apr 15, 2020

View reviewed changes

westonruter reviewed Apr 15, 2020

View reviewed changes

westonruter approved these changes Apr 15, 2020

View reviewed changes

Revert new test in AMP_Layout_Sanitizer_Test

6e34e77

schlessera approved these changes Apr 15, 2020

View reviewed changes

Merge branch 'develop' of github.com:ampproject/amp-wp into add/amp-m…

fc96b3a

…ustache-width-attribute * 'develop' of github.com:ampproject/amp-wp: Stub request based on test scenario (#4588)

westonruter changed the title ~~Fix an issue with a width attribute in a mustache template~~ Fix handling of Mustache templates Apr 15, 2020

westonruter merged commit 0a87d54 into develop Apr 15, 2020

westonruter deleted the add/amp-mustache-width-attribute branch April 15, 2020 19:15

kmyram added this to the v1.5.3 milestone May 28, 2020

	if ( isset( $tag_spec['amp_layout']['supported_layouts'] ) ) {
	$layouts = wp_array_slice_assoc( AMP_Rule_Spec::$layout_enum, $tag_spec['amp_layout']['supported_layouts'] );

	$merged_attr_spec_list['layout'][ AMP_Rule_Spec::VALUE_REGEX_CASEI ] = '(' . implode( '\|', $layouts ) . ')';
	}

	// Skip processing stylesheets that contain mustache template variables if the element is inside of a mustache template.
	if (
	preg_match( '/{{[^}]+?}}/', $value ) &&
	0 !== $this->dom->xpath->query( '//template[ @type="amp-mustache" ]//.\|//script[ @template="amp-mustache" and @type="text/plain" ]//.', $element )->length
	) {
	return;
	}

Conversation

kienstra commented Apr 14, 2020 • edited by westonruter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Steps to reproduce

Checklist

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kienstra commented Apr 14, 2020

Uh oh!

westonruter commented Apr 14, 2020

Uh oh!

westonruter commented Apr 14, 2020

Uh oh!

Uh oh!

Uh oh!

westonruter Apr 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

westonruter Apr 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kienstra commented Apr 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

pierlon commented Apr 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

schlessera commented Apr 15, 2020

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!

kienstra commented Apr 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

kienstra commented Apr 14, 2020 •

edited by westonruter

Loading

westonruter Apr 14, 2020 •

edited

Loading

westonruter Apr 15, 2020 •

edited

Loading

kienstra commented Apr 15, 2020 •

edited

Loading

pierlon commented Apr 15, 2020 •

edited

Loading

kienstra commented Apr 15, 2020 •

edited

Loading