Disallow non-templates as interpolations in Polymer.html #5023

justinfagnani · 2018-01-07T18:49:57Z

This is to prevent XSS vulnerabilities from accidentally interpolating unsafe user-provided values into templates that would be interpreted as HTML.

With this change, the only values that can be used to construct templates are JS template literals and other HTML templates. We're not checking that HTML templates are created from Polymer.html, but if an app is letting user-controlled values into an HTML template as HTML, it already has an XSS.

cc @arthurevans and @katejeffreys for documenting this change.

kevinpschaaf

LGTM

graynorton · 2018-01-08T20:22:56Z

lib/utils/html-tag.html

-     * they are not intended as a replacement for Polymer data-binding.
+     * Templates can be composed by interpolating `HTMLTemplateElement`s in
+     * expressions in the JavaScript template literal. The nested template's
+     * `innerHTML` is inluced in the containing template.


'inluced' => 'included'

sorvell

Let's add a Polymer.htmlLiteral (bikeshed) so users have a way of getting a, say, imported string into a template.

Polymer.html`${Polymer.htmlLiteral(foo)}`

justinfagnani · 2018-01-23T21:11:07Z

@sorvell sounds good.

I would want to offer this as another template tag so that it can only be used on literals, not on string variables, and it should disallow interpretations of plain strings.

For a function that can consume any string, I would want unsafe in the name, ie Polymer.unsafeHTML().

justinfagnani · 2018-01-29T18:20:30Z

Given that 2.4 got tagged already, I think we should merge this as-is for a quick 2.4.1. We can always add more supported data types without breaking changes.

TimvdLippe

We talked about this in our meeting. I am okay with this change, given that the unsafeHTML function will be provided soon too.

P.S. build fails because you need to update the TypeScript types (this sadly also takes into account docs)

Allows composition of strings into html templates constructed with `Polymer.html`. This is useful for composing values into styles or other places where an html template would not compose. ``` const css = Polymer.htmlLiteral`background: red;` Polymer.html`<style>:host { ${css} }</style>` ```

TimvdLippe · 2018-01-29T19:45:19Z

lib/utils/perf-tester.js

+
+  'use strict';
+
+  class PerfTester extends HTMLElement {


@sorvell Was this class intended to be committed in this PR? If so, we need some docs on what it actually does and how we can use it (assuming you used it to measure the performance of the changes in this PR).

argh, mistakenly committed; removing.

justinfagnani · 2018-01-29T20:07:48Z

lib/utils/html-tag.html

@@ -12,13 +12,31 @@
  (function() {
    'use strict';

+    class HTMLLiteral {


justinfagnani · 2018-01-29T20:08:14Z

lib/utils/html-tag.html

@@ -12,13 +12,31 @@
  (function() {
    'use strict';

+    class HTMLLiteral {
+      constructor(string) {
+        this.innerHTML = string;


I wouldn't name this innerHTML, this isn't an element. I'd go with value

It was an attempt to write less code, since both types can return their string value via innerHTML; fair that it's a bit short-cutty.

justinfagnani · 2018-01-29T20:26:26Z

lib/utils/html-tag.html

+     */
+    Polymer.htmlLiteral = function(strings, ...values) {
+      return new HTMLLiteral(values.reduce((acc, v, idx) =>
+        acc + literalValue(v) + strings[idx + 1], strings[0]));


style nit: 4 space indent for wrapped expressions

justinfagnani · 2018-01-29T20:28:02Z

lib/utils/html-tag.html

-      if (value instanceof HTMLTemplateElement) {
-        return /** @type {!HTMLTemplateElement} */(value).innerHTML;
+      if (value instanceof HTMLTemplateElement || value instanceof HTMLLiteral) {
+        return /** @type {!HTMLTemplateElement | !HTMLLiteral} */(value).innerHTML;
      } else {
        throw new Error(`non-template value passed to Polymer.html: ${value}`);


this message should probably be updated to allow for htmlLiteral too

justinfagnani · 2018-01-29T20:29:01Z

lib/utils/html-tag.html

+    class HTMLLiteral {
+      constructor(string) {
+        this.innerHTML = string;
+      }


Consider adding a toString() implementation with the literal value

dfreedm · 2018-01-29T20:48:20Z

lib/utils/html-tag.html

-     * they are not intended as a replacement for Polymer data-binding.
+     * Templates can be composed by interpolating `HTMLTemplateElement`s in
+     * expressions in the JavaScript template literal. The nested template's
+     * `innerHTML` is included in the containing template.


This should also include using htmlLiteral

Simpler for debugging

alchmy · 2018-05-10T17:45:02Z

It seems like a pretty major problem to not be able to separate the HTML templates into separate files. This is absolutely key to being able to write modular code. I would naturally think it would be a developers choice if they wanted to add these arbitrary files. It sort of seems like it's just a way to keep code within a Polymer environment(I don't want to be confrontational, I do hope I'm wrong, as @justinfagnani says). Perhaps I haven't fully understood what's involved, but if I can't use arbitrary HTML templates, it's actually a deal breaker for me in terms of Polymer vs Vanilla WC.

The Components I was using stopped working a few days ago because I was importing html strings into my code and using them to build templates. The end user doesn't build web components, so is it fair to say that the restriction and the bubble wrap only affects developers who should be checking for XSS anyway ?

miztroh-zz · 2018-05-10T17:50:13Z

@alchmy: You may want to check out the HTML Modules discussion.

WICG/webcomponents#645

alchmy · 2018-05-10T17:51:28Z

Ok Thanks. I'll have a read through it.

justinfagnani · 2018-05-11T15:57:25Z

@alchmy this isn't about not being able to break up templates, this is about not opening XSS vulnerabilities by blindly allowing user input to flow into privileged templates. You can compose templates, they just have be composed of values produced by the html tag.

Disallow non-templates as interpolations in Polymer.html

5e890fa

justinfagnani requested review from dfreedm and kevinpschaaf January 7, 2018 18:49

googlebot added the cla: yes label Jan 7, 2018

kevinpschaaf approved these changes Jan 8, 2018

View reviewed changes

justinfagnani mentioned this pull request Jan 8, 2018

Html helper Polymer/old-docs-site#2412

Merged

graynorton reviewed Jan 8, 2018

View reviewed changes

kevinpschaaf mentioned this pull request Jan 17, 2018

Alternative API for style modules? #4940

Closed

sorvell suggested changes Jan 23, 2018

View reviewed changes

Merge branch 'master' into html-tag

08a020f

justinfagnani requested a review from TimvdLippe as a code owner January 29, 2018 19:12

TimvdLippe approved these changes Jan 29, 2018

View reviewed changes

TimvdLippe reviewed Jan 29, 2018

View reviewed changes

justinfagnani commented Jan 29, 2018

View reviewed changes

dfreedm requested changes Jan 29, 2018

View reviewed changes

Steven Orvell and others added 6 commits January 29, 2018 16:18

lint fix and update types

4c5b739

Review updates.

8965cb3

update types.

a9ca5bf

[ci-skip] Add comment about accepting htmlLiteral values.

46c07f5

Update types.

99280df

Add a toString for output from Polymer.htmlLiteral

bbe4c53

Simpler for debugging

dfreedm approved these changes Jan 30, 2018

View reviewed changes

dfreedm merged commit eeb7160 into master Jan 31, 2018

dfreedm deleted the html-tag branch January 31, 2018 01:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disallow non-templates as interpolations in Polymer.html #5023

Disallow non-templates as interpolations in Polymer.html #5023

justinfagnani commented Jan 7, 2018

kevinpschaaf left a comment

graynorton Jan 8, 2018

sorvell left a comment

justinfagnani commented Jan 23, 2018

justinfagnani commented Jan 29, 2018

TimvdLippe left a comment

TimvdLippe Jan 29, 2018

sorvell Jan 30, 2018

justinfagnani Jan 29, 2018

justinfagnani Jan 29, 2018

kevinpschaaf Jan 30, 2018

justinfagnani Jan 29, 2018

justinfagnani Jan 29, 2018

justinfagnani Jan 29, 2018

dfreedm Jan 30, 2018

dfreedm Jan 29, 2018

alchmy commented May 10, 2018 •

edited

Loading

miztroh-zz commented May 10, 2018

alchmy commented May 10, 2018

justinfagnani commented May 11, 2018

Disallow non-templates as interpolations in Polymer.html #5023

Disallow non-templates as interpolations in Polymer.html #5023

Conversation

justinfagnani commented Jan 7, 2018

kevinpschaaf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorvell left a comment

Choose a reason for hiding this comment

justinfagnani commented Jan 23, 2018

justinfagnani commented Jan 29, 2018

TimvdLippe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alchmy commented May 10, 2018 • edited Loading

miztroh-zz commented May 10, 2018

alchmy commented May 10, 2018

justinfagnani commented May 11, 2018

alchmy commented May 10, 2018 •

edited

Loading