Skip to content

binding safe HTML to a template #57

Open
jmesserly opened this Issue Apr 24, 2013 · 40 comments
@jmesserly

I was wondering the recommended way of doing something like this:

<template bind>
  member name: {{memberName}}
  member api docs:
  <div>{{memberDocs}}</div>
</template>

Here, memberName is a string, but memberDocs is an HTML fragment generated from Markdown.

It would be nice if there was a way to bind this through templates. To avoid XSS, you want some assurance that the HTML fragment was either fully controlled by the application or validated. What we did was introduce a type like "SafeHtml" which is constructed something like "SafeHtml.unsafe" (or via a validator, if you have one). The idea there is to give security reviews a nice way to find interesting spots in the application, and makes accidental mistakes a lot less likely.

@sigmundch @ebidel

@rafaelw
polymer member
rafaelw commented Jun 20, 2013

So one way I'm thinking that binding markup to be parsed can work is binding to the 'content' of a . e.g.

<template bind>
  I am parsed: <template bind content="{{ boundMarkup }}"></template>
</template>
template.model = { boundMarkup: '<h1>I am a heading </h1>' };

To my mind this approach has a few nice qualities:

1) It's isn't super lightweight. I think binding to markup should generally be discouraged and it should be clear that when you are doing it, it is to be taken seriously.

2) It provides a clear demarkation that "markup can be inserted here"

3) It allows for safe interpretation of the markup. This can work like this:

-The template element implements bind('content', model, path) to be one-way binding to it's content document fragment.
-When the bound value changes, it parses the result to it's content. Because the produced markup is in an inert document, it cannot have side-effects, so any unsafe HTML isn't harmful (script can't run, etc...)
-The template element then "sanitizes" the produced markup (probably removing any script tags or javascript urls, etc...), and then produces an instance in the document.

@jmesserly

+1, this is really nice! fwiw, I don't think it has to be super lightweight.

-When the bound value changes, it parses the result to it's content. Because the produced markup is in an inert document, it cannot have side-effects, so any unsafe HTML isn't harmful (script can't run, etc...)

we'd have to be careful in polyfills for <template> but if I recall correctly, @blois and @arv found a way to parse fragments safely on browsers without template... (maybe template_element.js already implements "content" this way?)

@rafaelw
polymer member
rafaelw commented Jun 20, 2013

Yes. The polyfills are implemented this way. The content DocFrag is owned by a document created via document.implemenation.createHTMLDocument().

@sorvell
polymer member
@IgorMinar

Soy and Closure also have this new-ish thing called strict contextual autoescaping. The idea is that instead of trying to secure the potentially dangerous sinks (e.g. interpolation points in a template) in the app. You secure the sources and allow only data from trusted sources to enter these sinks.

In practice that means that a templating engine can only perform potentially dangerous innerHTML or similar operation, if the input data is of a trusted type "safe html" in this case. This data structure is immutable. You then focus on auditing the places that certify data as "safe html". If an interpolation with safe html is attempted, the raw value is retrieved and used for interpolation. However if regular string or a trusted wrapper of different type (e.g. "safe url" or "safe script") is provided as the input, the templating engine aborts the operation.

I like these ideas quite a bit, but I don't like that they are framework/library specific, which makes interoperability a big hassle. For that reason, I'd really like to see something in this spirit built into the platform so that all libraries use the same mechanism and data structures to provide this security feature across the board.

@xtofian
@blois
blois commented Jun 25, 2013

The approach being proposed is primarily validation rather than escaping- the results of a binding are validated to be safe then applied within the context which they are used.

Examples:

<div>{{value}}</div>

Normal text is always applied to element.textContent after the fragment has been parsed- there's no real sanitization here and the escaping is implicitly done by the browser.

<script>{{value}}</script>

Not allowed (though not quite sure at what level this would be blocked).

<img src='{{user.profileImage}}'/>
<img src='http://example.com/{{user.profileImage}}'/>

Here the binding system knows that src is being bound and needs to call a sanitizer to validate that the resulting URL is acceptable. This sanitizer will do things like validating that the URL is a same-origin URL, or similar checks. There's not really any auto-escaping but the sanitization is definitely contextual. The sanitization system needs to know the entire final value- not just the contents being replaced.

<span style="color:{{USER_COLOR}};">

The attribute sanitizer needs to handle this case as well, but I believe that the default system will not include CSS sanitization as this would require a CSS parser. In this case, the entire attribute would be removed (use CSS classes instead).

We're working on integrating sanitization into the core of Dart so innerHTML and similar APIs would be sanitized by default, using a system which is compatible with MDV sanitization.

@warpech
warpech commented Jul 8, 2013

any progress with this? I tried to hack through my own changes to MDV but no luck so far...

@rafaelw do you also consider making the following possible?

    <template id="tpl" bind>
      I am parsed: <template repeat="{{ people }}" content="{{ boundMarkup }}"></template>
    </template>
    document.getElementById('tpl').model = { boundMarkup: '<h1>I am {{ name }}</h1>', people: [
      {name: "John"},
      {name: "Alice"}
    ] };

This would add a whole dimension of flexibility by using template variables as partials.

@IgorMinar
@rafaelw
polymer member
rafaelw commented Jul 9, 2013

@warpech @IgorMinar I'm less worried about Spaghetti code than I am about XSS in this case. I think it's a requirement to allow binding to markup for certain use cases. It's less clear to me that it's a requirement to allow that markup itself to contain bindings.

@warpech. I agree with Igor that it's worth thinking through your use case to see if there's a more structured way to approach it. Perhaps if you say more, we can offer suggestions.

@blois
blois commented Jul 9, 2013
@hpoul
hpoul commented Oct 18, 2013

not sure if this is the right place to share this, but wanted to add an additional usecase:
i have a rather long list of items which are filtered by a simple text input given by a user - updated in real time has he types, with each line in the list highlighting the query string.. ie. this would require a lightweight solution to embed HTML content inside a repeat-template. currently i'm using a "result" element for each row.. which is really slow with > 50 items.. so i think a solution for embedding pre-rendered html content should be as lightweight as possible, such as a SafeHtml-like facility.

@akhileshgupta

One more usecase where I wish Polymer supports such templating structure:
I have a rich text editor based input screen which generates an HTML structure underneath. Now I want to render this HTML structure as is using Polymer template binding since this html markup is already sanitized during code generation.
Handlebar templates allow such binding by use of "triple-stash", which has been very useful.

@SBhojani

Is there a way (some sample code, perhaps) to make (any variation of) this work at the moment?

@andoband

@SBhojani If you're just looking to display some safe html you can set the innerHTML property of an element. AFAIK none of the ideas above for data-binding have been implemented in polymer yet.

@SBhojani

@andoband Yes, ended up doing that. Thanks.

@warpech
warpech commented Jan 22, 2014

I created my own custom element to insert safe HTML into a template: https://github.com/puppetjs/x-html

Would that be useful to you? I update it regularly because it tends to break with changes in Canary and Polymer but so far I was able to keep it working and will continue to do so (until there is a solution built into Polymer)

@psyrendust

Has there been any updates on this or a plan to add this into a future milestone?

@jmesserly

So, looking at this again, I'm wondering if using a Custom Element like https://github.com/Juicy/juicy-html is the right way to go. We could have a similar version that does more strict validation of the resulting "content", without needing to add a lot of code (and code size) to the core TemplateBinding package. @rafaelw thoughts?

@rafaelw rafaelw was assigned by jmesserly Aug 7, 2014
@jmesserly

fyi, just dup'd a few more bugs with this one. I think https://github.com/Juicy/juicy-html is currently the best solution.

@ebidel
polymer member
ebidel commented Sep 3, 2014

Just want to point out to folks to the new injectBoundHTML() method (see docs bug), which is useful for the "inject html into my template" case.

Could Polymer provide a safe filter that wraps this call?

<div>{{ content | safe }}</div>
@jmesserly

Could Polymer provide a safe filter that wraps this call?

Right now we don't have the hooks to write that as a filter. I think at the least, you'd need a binding delegate that can change the binding from 'textContent' to 'innerHTML' and validate that it's actually a SafeHTML object or apply a sanitization policy.

@arv
arv commented Sep 3, 2014

One idea we had early on was to allow a filter to return a DocumentFragment here and if so just replace the content with that instead of using textContent. It would require some special casing for textContent though.

@jmesserly

would that run through a sanitizer, or does it just add the raw nodes?

For raw nodes it's not so much a technical challenge IMO, as trying to make it feasible to do a security audit. Ideally, not every {{ binding }} needs to be security reviewed, only the ones that potentially inject HTML.

@jmesserly

It looks like Polymer has injectBoundHtml now. Does that resolve the issue? https://github.com/Polymer/polymer-dev/blob/3acc3c6a9e1db01cf403b80a210e9aee92a20b03/src/instance/utils.js#L96

@egyptianbman

One problem I'm having is I can't bind HTML inside a repeat.
example:

<template repeat="{{comment in comments}}">
  <div>{{comment.text | nl2br}}</div>
</template>
<script>
Polymer({
  nl2br: function(txt) {
    return txt.replace(/\n/, '<br />');
  }
});

injectBoundHTML is defined as function(html, element), meaning I have to have an instance of the element that I want to bind. The problem here is that the nl2br filter doesn't receive which div made the call, so I have no way of successfully calling injectBoundHTML.

@egyptianbman

Thank you @jmesserly, that worked perfectly, I was able to do:

<template is="juicy-html" content="{{comment.text | nl2br}}"></template>
@Jagatheesan

Juicy Html doesn't work for me on IE, anybody has got it working on IE?

@synthet1c

this works pretty well, haven't really tested it on browers other than chrome, but its simple js so should be cross browser.

<polymer-element name="some-thing" attributes="inner-html">
    <template>
        <div id="content"></div>
    </template>
    <script>
        Polymer('some-thing', {
            ready: function(){
                this.$.content.innerHTML = this['inner-html'];
            }
        });
    </script>
</polymer-element>
<!-- eg -->
<template repeat="{{ thing in things }}">
    <some-thing inner-html="thing"></some-thing>
</template>
@horacioibrahim

Nice @andrewFountai. I added an observer in the attribute/property inner-html. This allows content updates!

Change from:

<polymer-element name="some-thing" attributes="inner-html">
    <template>
        <div id="content"></div>
    </template>
    <script>
        Polymer('some-thing', {
            ready: function(){
                this.$.content.innerHTML = this['inner-html'];
            }
        });
    </script>
</polymer-element>

To:

polymer-element name="some-thing" attributes="innerHTML">
    <template>
        <div id="content"></div>
    </template>
    <script>
        Polymer('some-thing', {
            ready: function(){
                this.$.content.innerHTML = this['innerHTML'];
            },
            innerHTMLChanged: function(){
                this.$.content.innerHTML = this['innerHTML'];
            }
        });
    </script>
</polymer-element>
@perdona
perdona commented Oct 22, 2015

Juicy html doesnt work on Safari and Firefox. Does above solutions solve this case?

@egyptianbman

Excellent timing, I just ran into this problem as well, today.

@ranjeet-choudhary

this works for me <element inner-h-t-m-l="{{prop}}"></element>

@cedric-marcone

@ranjeet-choudhary You cannot be serious ! I laughed so much :D
Clever, but so risky ...you made my day even if never would do such a thing in one of my apps ;)

@davidgiven

@ranjeet-choudhary That's awesome, and so easy.

Apart from being an abomination, are there any real reasons why I couldn't just use that?

@ranjeet-choudhary

@davidgiven There is risk of XSS attack, if user is allowed to modify the html. Noting specific to inner-h-t-m-l=""

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.