Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support template validation #892

Closed
dvoytenko opened this issue Nov 10, 2015 · 6 comments
Closed

Support template validation #892

dvoytenko opened this issue Nov 10, 2015 · 6 comments
Assignees

Comments

@dvoytenko
Copy link
Contributor

As part of supporting dynamic content we will be rolling out <template> support and we need it to validate.

The template element is described in some detail here: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/template

Our first target is Mustache.js: https://github.com/janl/mustache.js/, but there could be any number of template languages. They are all fairly similar. One critical aspect of use of template languages in AMP, however, is that we require a valid DOM. template element gives us this assurance, but validator has to confirm.

Here's the nuances of supporting templates:

  1. template element must have a type attribute prefixed with "amp-". This type should be declared by an async script with custom-template attribute. For instance:
<script custom-template="amp-mustache" async src="..."></script>
...
<template type="amp-mustache">
Bonjour, {{monde}}!
</template>
  1. As mentioned above, the content within template element is a well-formed HTML. This, more or less, follows from using template element, but the validator needs to make it doubly sure that, although valid in Mustache, we will not allow:
    2.1. Expressions on element names are not allowed, e.g. <{{tagName}}></{{tagName}}> is invalid.
    2.2. Expressions on element attribute names are not allowed, e.g. <div {{#noborder}}noborder{{/noborder}}> is invalid. Notice that attribute value expressions are perfectly valid.
  2. Mostly the rules for validation are the same we already employ in AMP, such as:
  3. <script>, <style> tags not allowed
  4. style attribute is not allowed
  5. on* attributes are not allowed with a sole exception of on attribute
  6. and so on
  7. Unlike normal AMP, validation of some attributes within template tag will have to relaxed. E.g. it's possible to specify <a href="{{href}}">. So, as a general rule we will allow {{}} values with template in place of fixed values or other controlled values in AMP elsewhere. Where security is concerned, the final validation will be performed by the client-side sanitizer (not part of this task). E.g. client-side sanitizer will ensure that a tag created by a template will not have "javascript:" in href and so on. See Caja-based sanitizer for AMP templates #890 for more info.
  8. One exception is that we may need to extend grammar to support no-value attributes. Since we do not allow calculated attribute-name expressions such as {{#noborder}} noborder {{/#noborder}}, we may have to support an alternative. I'm exploring an option with ?, e.g. noborder?={{noborder}}. I'd like feedback if this is easy to enable in the validator.

For more on the choice of template element, see: https://docs.google.com/document/d/1q-5MPQHnOHLF_uL7lQsGZdzuBgrPTkCy2PdRP-YCbOw/edit

/cc @cramforce
/cc @NataliaLKB

Related to #657.

@Gregable
Copy link
Member

Interesting.

How will we prevent layout reflows with templates? Even simple text replacements will have their width determined by the actual text string. Will the rendering engine block on the loading of the json blob?

@dvoytenko
Copy link
Contributor Author

Templates do not render themselves. The target AMP elements call templates to render some part of their content, so it depends on each element. For instance, a carousel can call JSON service and template to render slides. But no relayout is needed there since all the slides a positioned behind each other in the fixed space. Some other elements might need to be more creative with space. We also have "requestHeight" system that should help.

@Gregable
Copy link
Member

Gregable commented Dec 3, 2015

Looking at this in more detail now...

For the "type", is "amp-mustache" the only currently allowed value? Will make it quick to add new ones, I just want to be sure that this is the only one for now.
Guess: Yes, "amp-mustache" is the only currently allowed type.

For amp-mustache, what is the src= value that we are looking for in the script tag? Clearly we can't allow any value there as it'll be executed as j/s, but it's not provided in the issue anywhere that I can see.

Are there any attribute values that we don't want to allow as mustache variable? The only things I can thing of would be maybe the layout and related attribute as well as maybe class?
Guess: any attribute value can be a mustache variable for now

Are <amp-*> elements allowed inside template tags?
Guess: yes? I can't think of an example.

Mozilla doc specifies that the <template> must be a child of <body>, <frameset>, <head> or <colgroup>. Do we want the same constraint? Notably we only have a mechanism at the moment to denote a single parent constraint, so if we could limit to be only allowed as a child of <body> or not limit it's parent at all, those would be slightly easier to implement, though not a big deal.
Guess: <body> restriction is fine for now.

Mustache doc has two types of variables, {{foo}} and {{{foo}}}, the latter is unescaped content which can also be implemented with {{&foo}}. Do we want to allow the unescaped form?
Guess: We do not want unescaped forms.

Mustache has this set delimiter syntax that allows the parsing to switch to a different delimeter from {{ and }}. That would be a beast to implement validator for. OK to disallow for now? Note that without it, no templates can actually have "{{" or "}}" as a literal string inside them.
Guess: We do not want to allow set delimiter syntax.

Mustache supports partials which in turn seem to import a template from somewhere else. I don't see how this would be used with your implementation. Is it something we want to disallow for now?
Guess: We do not want to allow partials.

FYI: Mustache supports function calls in the template. It's impossible for the validator to know that {{foo}} will eventually be used in conjunction with the j/s object:
{ "foo": function() { return "foo"; } },
so this will have to be up to runtime for safety.

To your question 8, I'm not sure how difficult this would be. It's not trivial, but it seems possible. The part that would get very tricky is if this interacts with the attributes in the layout rules which have more complex interactions than elsewhere.

@dvoytenko
Copy link
Contributor Author

For the "type", is "amp-mustache" the only currently allowed value? Will make it quick to add new ones, I just want to be sure that this is the only one for now.

Yes. Special-case all the way.

For amp-mustache, what is the src= value that we are looking for in the script tag? Clearly we can't allow any value there as it'll be executed as j/s, but it's not provided in the issue anywhere that I can see.

This is the same spec as for elements - so whatever we do, e.g. for amp-iframe elements.

Are there any attribute values that we don't want to allow as mustache variable? The only things I can thing of would be maybe the layout and related attribute as well as maybe class?

By default any value.

Are <amp-*> elements allowed inside template tags?

See examples here: https://github.com/ampproject/amphtml/blob/master/extensions/amp-list/amp-list.md

Mozilla doc specifies that the <template> must be a child of <body>, <frameset>, <head> or <colgroup>.

I think this is wrong. According to https://html.spec.whatwg.org/multipage/scripting.html#the-template-element it's allowed anywhere where a phrasing or script content is allowed. For us this basically means that it's allowed anywhere, except inside <span> and <p>. But normally we do not enforce this kind of stuff.

Mustache doc has two types of variables, {{foo}} and {{{foo}}}, the latter is unescaped content which can also be implemented with {{&foo}}. Do we want to allow the unescaped form?

Yes, allow unescaped values. We sanitize it. The sanitizer is currently in the security review.

Mustache has this set delimiter syntax that allows the parsing to switch to a different delimeter from {{ and }}.

Fine to disallow.

Mustache supports partials which in turn seem to import a template from somewhere else. I don't see how this would be used with your implementation. Is it something we want to disallow for now?

No need to disallow. If we decide to implement them - they should just work.

FYI: Mustache supports function calls in the template. It's impossible for the validator to know that {{foo}} will eventually be used in conjunction with the j/s object

We will keep it possible as well. Currently we do not allow functions in the payload, so it's impossible to make any calls. But we may allow them in the future.

To your question 8, I'm not sure how difficult this would be. It's not trivial, but it seems possible. The part that would get very tricky

Let's ignore this for now.

@dvoytenko
Copy link
Contributor Author

Tested on all browsers, we can skip <{{tag}}> validation - <template> element guarantees well-formed HTML for us here.

But we have two new rules:

  1. Unescaped-mustache is NOT allowed in attribute value.
  2. '{' and '}' are not allowed in "data-" attribute names

@Gregable
Copy link
Member

Gregable commented Jan 9, 2016

The validation changes in the CL above are now live everywhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants