Amphtml: Sanitizer: Built-in AMP components in triple mustache

[IamAlta]

Additional info by @aghassemi:

When https://github.com/ampproject/amphtml/issues/14089 lands, we should be able to allow unscaped mustache content to include AMP components.

This issue was originally raised for allowing amp-img but I renamed to a generic version.

/cc @choumx @alabiaga

What's the issue?

I am trying to send content to amp-list/amp-mustache that contains amp-img. That is, the amp-img tag is part of the content.
The image does _not display in the content. NOTE: It does display on the html page indicating the code is okay.

How do we reproduce the issue?

<amp-list width="auto"
  height="1000"
  layout="fixed-height"
  src="https://example.com//amp-list.json">
  <template type="amp-mustache">
    {{{content}}}     
  <template>
</amp-list>

Returning json is something like

{
    "items": [{
        "content": "<amp-img src=\"/image1.jpg\" width=\"290\" height=\"435\" alt=\"Image 1\" layout=\"fixed\" ></amp-img>"
    }, {
        "content": "<amp-img src=\"/image2.jpg\" width=\"290\" height=\"435\" alt=\"Image 2\" layout=\"fixed\" ></amp-img>"
    }]
}

See here for working demo https://www.gardecor.com/store18/test-ajax-amp-img.php

What browsers are affected?

Tested only Firefox Developer recent one

Which AMP version is affected?

Powered by AMP ⚡ HTML – Version 1525461683159

[lannka]

@aghassemi can add more, but arbitrary innerHTML is not allowed here because that will be a way to bypass the AMP validation. In AMP, only a subset of HTML tags are allowed.

[aghassemi]

Hi @IamAlta, we only support a very limited set of HTML tags in {{{unscaped}}} (e.g. "triple-mustache") by design. Please see https://www.ampproject.org/docs/reference/components/amp-mustache#restrictions for the set. In short, if you need to have amp-components, they must be declared statically in the template itself and can not be part of data.

/to @choumx @alabiaga Would the work on better runtime validation allow us to remove this restriction?

[IamAlta]

Hi @aghassemi @iannka,

Any chance of making that work?

It would be a valuable tool to have that working - to send image data in content back. Not all content has the same structure so building the amp-img in the mustache would not work (tap:AMPsetState..." okay through json.

Get creative - I know you can do it.

[aghassemi]

@IamAlta @lannka Even a simple amp-img requires some validation. https://github.com/ampproject/amphtml/issues/14089 is the tracking issue for proper runtime validation in AMP, until that is launched, I don't see us being able to handle this correctly.

I am renaming the issue to a generic "Allow AMP components in unscaped mustache templates"

[lannka]

@IamAlta one thing to consider is to put all possible structures in one template (assuming you have a finite number of structures), and use CSS to select the correct one.

[IamAlta]

@lannka,
Thanks for the suggestion.
I made new pages for each content so that the images will show up.
This is complex and I don't want to add too much on a page.

@aghassemi,
Thank you for explaining. Maybe someday someone will wave a magic wand and make it so.

Have a wonderful day! I appreciate your time and good suggestions and responses.

[choumx]

If there aren't any security concerns, we could simply loosen the constraints and allow amp-* elements and accept user misconfiguration e.g. missing extension script, attribute typos etc. This wouldn't be any worse than canonical AMP or PWA is today.

[mdiblasio]

Any update on this FR? This would help solve some challenges around A/B testing / personalization by allowing business teams to use common A/B platforms to serve customized HTML to AMP pages. Sample use case: serve personalized promotional banners that use <amp-img>. These banners have different formats so could not be used with <amp-template>.

[hadidotj]

Hey Everyone!

I have been working with @mdiblasio to implement AMP pages for a client. Along with the A/B testing he mentioned, we also ran into an issue where we wanted to add "content" within a list of products on one of their pages.

So, for example, we may have a 3x3 grid of 9 products; however, they may want to have a special "promo" image link display in grid 3 (top right):

| | | |
|-----------|-----------|-------------------------|
| Product 1 | Product 2 | Clearance 20%-50% Off |
| Product 3 | Product 4 | Product 5 |
| Product 6 | Product 7 | Product 8 |

Further, assume the HTML markup looks something like this:

<a href="/clearance" title="Clearance">
  <amp-img src="/imgs/clearance.png" width="100" height="50"/>
</a>

Okay, this should be easy! 😄We were planing to use an <amp-list> to fetch the product "results" :

<amp-list src="/product/results" layout="responsive" width="300" height="150">
  <div placeholder>
    <div class="loading">Loading ...</div>
  </div>
  <template type="amp-mustache">
    <div class="product-grid">
      {{#products}}
        {{#content}}<div class="promo">{{{content}}}</div>{{/content}}
        {{^content}}
          <div class="product">
            <a href="/products/{{code}}">
              {{name}} <amp-img src="{{img}}" width="50" height="50"></amp-img>
            </a>
          </div>
        {{/content}}
      {{/products}}
    </div>
  </template>
</amp-list>

With the "JSON" response looking like:

{
   "items": [{
      "products": [
         {"code":"Hat1", "name":"Cool Hat", "img":"/img/hat-1.png"},
         {"code":"Shirt8", "name":"Great Shirt", "img":"/img/shirt-8.png"},
         {"content":"<a href=\"/clearance\" title=\"Clearance\"><amp-img src=\"/imgs/clearance.png\" width=\"100\" height=\"50\"/></a>"},
         {"code":"Pants2", "name":"Awesome Pants", "img":"/img/pants-2.png"},
         {"code":"Shoes5", "name":"Nice Shoes", "img":"/img/shoes-5.png"},
         // More product results
      ]
   }]
}

But, this sanitation killed that idea, as the <amp-img> is stripped. 😠

Have no fear! I have another idea 😏 I came up with a "work around" (sounds better than "hack :trollface:). In my JSON processing, replace any <amp-img> tags with a <span> with a background image :P

<a href="/clearance" title="Clearance">
  <span style="display:block;width:100px;height:50px;background-image:url('/imgs/clearance.png');background-size:100px 50px;">&nbsp;</span>
</a>

Obviously, this defeats the purpose of <amp-img> because now the image is loading, even if it is outside the viewport at the time! This would be an instance <amp-img> would be preferred.

Thanks!
Tyler

P.S. Please don't start stripping "background-image" in style attributes. I don't want to find another "work around" 😇

[nainar]

@choumx is this something that will benefit from a design discussion to talk about potential security issues here?

[choumx]

I don't think there are security implications here. It's more of a dev-x trade off between publisher flexibility and validator enforcement of required attributes, etc.

One potential concern is for AMP specs that have extension whitelists. E.g. amp4ads spec may not allow extension amp-foo, but allowing all amp-* tags in triple mustache would bypass this. This is not a problem yet since such tags should be inert without the extension script in the document head, but we should at least ensure to never dynamically load extension scripts.

A conservative start would be allowing built-in AMP components in triple mustache.

[mdiblasio]

<amp-img> would be a great start as it's likely the most common element that would be dynamically fetched and would satisfy all of the requirements commented in this thread.

[hadidotj]

Hi @choumx,

I saw a while go that this became prioritized and changed to "P2: Soon". Is there a timeline on when and/or can be injected dynamically/during runtime using "Triple Mustache?"

Thanks,
Tyler

[alabiaga]

@hadidotj I'll talk to @choumx on whether this needs to be prioritized. It's assigned to me but I have had other tasks that took priority. Thanks

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.