Amp-wp: AMP_Allowed_Tags_Generated class is stripping amp-date-display {{iso}} format from amp-timeago component

[paraleadogg]

When trying to convert a timestamp to an iso datetime format using an amp-date-display component with amp-mustache template the {{iso}} format is being stripped from the amp-timeago component by the datetime regex value in the AMP_Allowed_Tags_Generated class on line 5417

It would be good if the below could be allowed to work as it would give a degree of flexibility with regards to the type of datetime we can pass to amp-timeago (sometimes it's not possible to change the format of the date being consumed via an external api call etc)

<amp-date-display timestamp-seconds="1567062005" layout="fixed" width="360" height="20">
    <template type="amp-mustache">
        <amp-timeago class="m1"
        width="160"
        height="20"
        datetime="{{iso}}">&nbsp;</amp-timeago>
    </template>
</amp-date-display>

[chrissjhitc]

I'd second this...

This issue affects the amp-date-display 'timestamp-seconds' attribute too. The attribute is checked against value_regex: "\d+" - meaning you can't use a mustache var such as {{timestamp}} when drawing the components inside an amp-list without the attribute being stripped during sanitization.

[westonruter]

Thanks a lot for reporting this. We'll fix this bug.

[westonruter]

As for how to implement this, apparently we need to skip validating the contents of any attribute value that begins with {{ if the element is inside of a template[type="amp-mustache"]. It would be good to check the validator JS code in AMP to confirm this is what it is doing.

[schlessera]

I did some browsing through the reference validator to find out how it works.

There are 3 concepts that are important here:

1. The validator generally lets template tags through, but they depend on the templating engine extension, of course. So, for Mustache, this is {{}}, but it might be something else for a different templating engine. There seems to be official support for Mustache only right now, though ( <template type="amp-mustache">).

2. There are a few conditions that make these template tags invalid. So, for example, you cannot easily nest templates within templates, and you cannot have the template tags modify the sanitized markup itself, like <{{tagName}}> or <div {{attributeName}}="value">.

3. AMP will run sanitization on the output after the template was rendered, to ensure everything remains secure. For {{{}}} (unescaped output), it limits the rendered output to a whitelist of HTML tags.

I suggest we proceed as follows:

1. We should start with implementing this via the Tag & Attribute Sanitizer, basically side-stepping any attribute or content requirements when we encounter {{}} placeholders that are wrapped at some level within <template> tags. To stay safe we should probably already provide a mapping between type and placeholders, otherwise the logic might end up being hard to extend to additional template languages later on. I don't think we can reliably reason about partial attributes/content (i.e. href="https://example.com/pages/{{target-page}}"), so as soon as the placeholders are encounter, we should just generally ignore the entire attribute/content.

2. This should go into a separate template sanitizer. However, I think this can be postponed to a later step, as I don't see this to be high-priority. We're mostly sanitizing output we've generated ourselves, so this is probably not something users will encounter a lot anyway.

3. This can be ignored for now, it will only run on the client-side anyway.

[westonruter]

OK, so to summarize…

We'll skip validating an attribute value if:

1. The element is inside of a template, and
2. The attribute value contains {{ and }} anywhere in the string.

These changes will be applied to the AMP_Tag_And_Attribute_Sanitizer.

Correct?

[schlessera]

Yes, correct. This is not a perfect validation, but it is a start to fix the bug and allow actual usage of templates.

Any further issues should then be solved in separate issues/PRs to tighten it back down again as much as we want/need.

[westonruter]

Agreed. This should solve the vast majority of cases, perhaps 100% of the cases. It seems the logic you've outlined here is the same as the AMP validator uses. If {{x}} is in an attribute value, then no validation is performed, even when the result will clearly never be valid. For example, the amp-timeago[datetime] attribute expects an ISO date string. And yet the validator doesn't complain if it contains extra stuff which will clearly make it not valid:

If the template variable is removed, then it becomes invalid.

The AMP plugin is only concerned with the same level of validation checking. It just needs to make sure that the initial page is marked as valid so that it passes validation at https://validator.ampproject.org/ as well.

[westonruter]

Testing Instructions

1. Create a new Custom HTML block and provide it with the following:

<amp-date-display timestamp-seconds="1567062005" layout="fixed" width="360" height="20">
    <template type="amp-mustache">
        <amp-timeago class="m1"
        width="160"
        height="20"
        datetime="{{iso}}">&nbsp;</amp-timeago>
    </template>
</amp-date-display>

1. Click Save Draft and there should be no validation error reported for the block. Before, without this patch, the user would have seen:

[kienstra]

Looks Good

Hi @westonruter,
Thanks for your detailed testing steps.

Like you mentioned, the validation error no longer appears with #3243:

Before

After