[Validator] Template attribute values in data-* attributes not validated

[sidkshatriya]

The following template attribute values correctly generate validation errors:

<template type="amp-mustache">
  <!-- validation error: UNESCAPED_TEMPLATE_IN_ATTR_VALUE -->
  <div class="{{{ hello }}}"></div>
  <!-- validation error: TEMPLATE_PARTIAL_IN_ATTR_VALUE -->
  <p class="{{ >testing }}}"></p>
</template>

These errors are triggered by the validateAttrValueBelowTemplateTag()

Interestingly, the following will not trigger any validation error:

<template type="amp-mustache">
  <!-- no validation error -->
  <div data-message="{{{ hello }}}"></div>
  <!-- no validation error -->
  <p data-message="{{ >testing }}}"></p>
</template>

This is because all data-* attribute values are not validated at all. (data-* attribute names are validated though). (see validateAttrNotFoundInSpec())

Is this behaviour correct? Should we not check data-* values also whether they have a correct value if they are underneath template tags?

This seems to contradict the comment "We disallow these in attribute values" (see below)

static valueHasUnescapedTemplateSyntax(value) {
    // Mustache (https://mustache.github.io/mustache.5.html), our template
    // system, supports {{{unescaped}}} or {{{&unescaped}}} and there can
    // be whitespace after the 2nd '{'. We disallow these in attribute Values.
    const unescapedOpenTag = new RegExp('{{\\s*[&{]');
    return unescapedOpenTag.test(value);
  }

Another contradiction: "We disallow partials in attribute values" as partials are allowed in data-* (see below)

static valueHasPartialsTemplateSyntax(value) {
    // Mustache (https://mustache.github.io/mustache.5.html), our template
    // system, supports 'partials' which include other Mustache templates
    // in the format of {{>partial}} and there can be whitespace after the {{.
    // We disallow partials in attribute values.
    const partialsTag = new RegExp('{{\\s*>');
    return partialsTag.test(value);
  }

[dvoytenko]

/to @Gregable

I'd indeed expect the same partial/unescaping rules to apply to data attributes as well.

That being said, the validation of templates is mainly aimed to "inform" of potential pitfalls when using templates. The results of template execution are still pushed via HTML sanitizer as well.

[Gregable]

This is a legitimate bug, the data-* attributes take a slightly different code path through the validator due to being 'wildcard' attribute names, and aren't getting run through the template validation.

As Dima said though, our primary goal in validation is to make sure that the pre-mustache-rendered document doesn't break amp validation rules. The post-mustache-rendered doc will be validated separately (currently via a runtime sanitizer) and also needs to be valid amp. The unescaped/partials logic is there to provide developer hints.

[ericlindley-g]

@Gregable Slotting in backlog, but feel free to correct/update the milestone as needed

[Gregable]

Resolved. This will be deployed to cdn.ampproject.org next week.