trustedHtml and trusted-html

[GavinJoyce]

Rendered

[snewcomer]

I love this idea 👍. Also, what if trusted-html was extensible (or some other name/invocation of it). For example, if one could whitelist certain HTML tags while the rest is escaped.

[GavinJoyce]

For example, if one could whitelist certain HTML tags while the rest is escaped

Whitelisting a subset of HTML is a surprisingly hard problem, see this list of XSS attack vectors for an example of the kinds of thing that you have to deal with. I'd think that responsibility would be better served by an external sanitisation library.

[dgeb]

Thanks @GavinJoyce - this LGTM overall 👍

I definitely prefer trustedHtml to htmlSafe. Using "trusted" makes it clear that it's a descriptor of the argument rather than a function that will perform sanitization upon it.

I am not as sure about deprecating {{{, which is a handlebars feature that is probably fairly widely used in templates. It seems sufficient (to me) to allow folks to lint against allowing it in their templates.

[GavinJoyce]

I am not as sure about deprecating {{{, which is a handlebars feature that is probably fairly widely used in templates. It seems sufficient (to me) to allow folks to lint against allowing it in their templates.

Thanks @dgeb, I've added the following unresolved question to the bottom of the RFC:

Instead of deprecating {{{, perhaps we could just help developers to lint against it?

To my mind, the fact that {{{ is widely used isn't necessarily a good reason not to change it. I suspect that there are many existing uses of {{{ in people's applications that in reality are unknown security issues - I've seen PRs in applications that I've worked on where new uses of {{{ would have introduced security issues if not noticed at review time. This was before we linted against it, I agree that linting will help prevent this. I'd prefer if this wasn't opt-in though.

I'm interested in what the consensus on deprecating {{{ is. The other changes in this RFC would be valuable on their own if we were to decide to drop it of course.

[paddyobrien]

I definitely prefer trustedHtml to htmlSafe. Using "trusted" makes it clear that it's a descriptor of the argument rather than a function that will perform sanitization upon it.

It seems unusual for a function to be named after its argument, so I think it's still pretty likely that someone would read this function as something which does the job of sanitization. i.e. "I give this function HTML and it gives me 'trusted' HTML, which must be safe.".

I think the best thing to do here would be to have the function name contain a scary warning sign. e.g. dangerousHtml unsafeHtml

[topaxi]

What about bypassSanitization or similar? Which doesn't describe what it is but what it does.

[chancancode]

I may have more substantial thoughts about the other aspects, but since a big part of this is that there is not one type of "safe" string, I think we should answer these questions:

1. Should <div style={{trusted-html @someString}}> a) Just Work™ b) raise the warning we have today c) error d) somehow "escape" it (not sure what that would mean)? Personally I think it should be c, but it would require providing a way to mark it as safe for that position

2. Similarly (but not the same!), should <div style="...; thing: {{trusted-html @someString}}; ..."> a) Just Work™ b) raise the warning we have today c) error d) somehow "escape" it (not sure what that would mean)? Personally I think it should be c, but it would require providing a way to mark it as safe for that position

If you agree with me, I think this calls for a slightly more generic solution, like {{trusted this.someString for="html"}} and {{trusted this.someString for="style"}} (it also narrowly avoids the camel vs dash problem, which is slightly awkward right this moment, because of the uncertainty around template imports).

If we go down this rabbit hole, there is more to unpack/design here – like whether 1 & 2 are really the same thing, and if for="style" good enough for both, or do we need something more context-specific. Personally, I think we can probably start with treating 1 & 2 as the same thing for now and deprecate our way out of it if we decide to start parsing CSS at build time.

But bottomline is, I don't think doing nothing is really an option here, allowing trusted-html in CSS positions is really wrong given the spirit of this proposal.

[22a]

It seems unusual for a function to be named after its argument, so I think it's still pretty likely that someone would read this function as something which does the job of sanitization. i.e. "I give this function HTML and it gives me 'trusted' HTML, which must be safe.".

This resonates with me too and is my one concern about trustedHtml+trusted-html. Perhaps we run the risk of leaking implementation detail here but how about something like markStringAsTrustedHtml so that it's clear the function call is merely setting a flag instead of sanitising? It's a very unwieldy name (and the accompanying helper would be too - mark-string-as-trusted-html) but perhaps this inconvenience is justified in order to disambiguate.

What about bypassSanitization or similar? Which doesn't describe what it is but what it does.

If I understand correctly ember doesn't provide any default sanitisation so I'm not sure that bypasses works here
Edit: Sorry my understanding here was incorrect, I was still in the mindset of {{{. I think

{{bypass-sanitization myString}}

is great for communicating bypassing the escaping provided by {{ but I wonder if the mental model holds in JS contexts where we're modifying some existing value to later use it in a template:

myTrustedHtmlString: computed('myString', function() {
  return bypassSanitization(this.myString);
}

[GavinJoyce]

It seems unusual for a function to be named after its argument, so I think it's still pretty likely that someone would read this function as something which does the job of sanitization. i.e. "I give this function HTML and it gives me 'trusted' HTML, which must be safe.".

Agreed @paddyobrien, I think trust-this-html or treat-this-string-as-trusted-html are more accurate names but I think that they both suffer from being a little longwinded and unusually structured.

I think the best thing to do here would be to have the function name contain a scary warning sign. e.g. dangerousHtml unsafeHtml

While these names will indeed be scary warnings, I think they suffer from similar issues to htmlSafe. I suspect that it would be easy for someone to think that it's fine to pass dangerous HTML to dangerousHtml or unsafe HTML to unsafeHtml. In reality, we don't want dangerous or unsafe html to come anywhere near our applications. I'll add them to the list of alternative names in the bottom of the RFC so that the community can consider them when reading the RFC.

What about bypassSanitization or similar? Which doesn't describe what it is but what it does.

@topaxi, I'll add that to the list of alternative names too. I don't think Ember is doing any sanitization so that name might not exactly fit. bypassHtmlEscaping might be a little more accurate, I'll add that too.

[GavinJoyce]

1. Should <div style={{trusted-html @someString}}> a) Just Work™ b) raise the warning we have today c) error d) somehow "escape" it (not sure what that would mean)? Personally I think it should be c, but it would require providing a way to mark it as safe for that position

@chancancode interesting. When creating this RFC I was assuming that trusted-html would behave in a similar fashion (b) to how {{{ currently behaves, it would raise a warning when used in style={{trusted-html someCss}}. EDIT: I'm not fully correct here, see this comment for the current behaviour

I hadn't considered your second case nor a new third case which is when used as an element helper (<div {{trusted-html @someString}}>) - perhaps this one should just be disallowed? Either way, I should detail how we treat these three cases in this RFC.

{{trusted this.someString for="html"}} and {{trusted this.someString for="style"}} are interesting, you're right that trusted HTML and trusted style are not the same thing. I wonder do we need to treat them differently when invoking the helper though? If we were to end up doing some build-time or run-time checking to the trusted style content, perhaps we would have enough information on which type of trusted string it is from where the helper is invoked? Perhaps this would be enough?

<div>{{trust this.someTrustedHtml}}</div>
<div style={{trust this.someTrustedStyle}}></div>
<div style="font-size: 2em; {{trust this.someTrustedStyle}}"></div>

(it also narrowly avoids the camel vs dash problem, which is slightly awkward right this moment, because of the uncertainty around template imports).

If there is uncertainty around naming, perhaps this RFC should stay in draft form until this has been resolved? We could continue to work on the draft, just leave the exact naming until after the other RFC has merged (unless, of course, we agree on a name that is unaffected by other RFCs).

Personally, I think we can probably start with treating 1 & 2 as the same thing for now and deprecate our way out of it if we decide to start parsing CSS at build time.

I wonder how useful parsing CSS style at build time would be? Most of the uses of dynamic style string that I've seen in Intercom have included run-time dynamic values. If the style is static at build time, shouldn't it be defined in CSS or in a <style> tag in HBS?

Possibly beyond the scope of this RFC, but it's probably useful to chat about the likely future direction of this warning. Hopefully we can do better over time:

WARNING: Binding style attributes may introduce cross-site scripting vulnerabilities; please ensure that values being bound are properly escaped. For more information, including how to disable this warning, see https://emberjs.com/deprecations/v1.x/#toc_binding-style-attributes.

[sheriffderek]

I can only add my limited use cases to this discussion. I mostly find myself using safeString for 2 things:

1. for backgroundImages populated by a CMS (usually computed based on screen context/size) - and

2. for inline computed styles like an animated menu transform or width like this progress bar

This default 'no-inline-styles' rule for templates / says that doing those things isn't OK. But I'm not really sure how I would go about those tasks otherwise. https://github.com/ember-template-lint/ember-template-lint/blob/master/docs/rule/no-inline-styles.md

export default Component.extend({
  // Passed in
  percent: 0,
  // Width
  percentageStyles: computed('percent', function() {
    return htmlSafe(`width: ${this.percent}%;`);
  }),
  ...

<div
  class='bar'
  style={{percentageStyles}}
  >
</div>

<ProgressBar @percent={{percentComplete}} />

I guess I don't really understand how any of my use-cases could be dangerous... So, I'm naive in that regard - but I'm just putting in a vote for something - not so short that scares everyone away - like (mut - and not too long style={{trustedString this.someString for="style"}} - because the more obscure stuff - the more people we'll scare off. I can use Vue in a codepen - and that's a pretty low barrier of entry / and it doesn't tell inside-jokes or yell at me for using logical bindings.

[GavinJoyce]

I tested a bunch of uses of {{{ with a css style string in Ember 3.7 development mode:

1. <div style={{{trustedStyle}}} />

{{#let "background-color: yellow;" as |trustedStyle|}}
  <div style={{{trustedStyle}}}>this should have a yellow background</div>
{{/let}}

2. <div style="{{{trustedStyle}}}" />

{{#let "background-color: yellow;" as |trustedStyle|}}
  <div style="{{{trustedStyle}}}">this should have a yellow background</div>
{{/let}}

3. <div style="color: blue; {{{trustedStyle}}}" />

{{#let "background-color: yellow;" as |trustedStyle|}}
  <div style="color: blue; {{{trustedStyle}}}">this should have a yellow background</div>
{{/let}}

4. <div foo="color: blue; {{{trustedStyle}}}" />

{{#let "background-color: yellow;" as |trustedStyle|}}
  <div foo="color: blue; {{{trustedStyle}}}">this should not have a yellow background</div>
{{/let}}

5. <div {{{trustedStyle}}} />

{{#let "background-color: yellow;" as |trustedStyle|}}
  <div {{{trustedStyle}}}>this should not have a yellow background</div>
{{/let}}

The results:

1 and 2 rendered correctly without warning.

3 rendered correctly with the following warning:

WARNING: Binding style attributes may introduce cross-site scripting vulnerabilities; please ensure that values being bound are properly escaped. For more information, including how to disable this warning, see https://emberjs.com/deprecations/v1.x/#toc_binding-style-attributes. Style affected: "color: blue; background-color: yellow;"

4 rendered the following html:

<div foo="color: blue; background-color: yellow;">this should not have a yellow background</div>

5 resulted in the following template compilation error:

Assertion Failed: Cannot invoke the trustedStyle modifier because it was shadowed by a local variable (i.e. a block param) with the same name. Please rename the local variable to resolve the conflict.

Some questions:

• Why does using {{{ in some style positions result in a warning when others do not. Shouldn't this be consistent?
• Why do we warn about {{{ being used for style and not for HTML? Isn't using it for HTML potentially more dangerous? Am I correct in saying that the danger of using {{{ in style positions is that a malicious user could inject CSS rules into the element and the danger of using {{{ in HTML positions is that a malicious user could perform a XSS attack?
• If we are to move to a better named helper (such as {{trust}}), perhaps we should remove these warnings altogether and instead provide clear documentation for the {{trust}} helper?
• Am I correct in noticing that the instructions given in the deprecation details are not fully accurate, especially: Handlebars only escapes HTML, not CSS, so this may introduce a potential XSS vulnerability into your application if a malicious user is able to provide data used in the myStyle property.?
• Should we assert that {{{ (or the new {{trust}} helper) should not be used as an element modifier?

^ /cc @chancancode as your initial comment touched upon these points

[GavinJoyce]

@sheriffderek thanks for providing that example, I think it's a pretty common use case to want to quickly construct a dynamic string of css style from dynamic values that you already trust. I think we have an opportunity to improve developer ergonomics starting with this RFC, but likely continuing with an addon which helps construct trusted dynamic css strings safely - perhaps one already exists?

We use an internal fmtStyle computed property macro in our app, it's simple but perhaps covers 90% of our use cases. It simply allows nulls, words and numbers:

Computed.fmtStyle('width: %@%; background-color: #%@', 'percent', 'colour'),

[sandstrom]

Very good proposal! 💯

I think it would make sense to try to deprecate Handlebars.SafeString and replace it with Handlebars.TrustedString while we're at it.

As others have mentioned, I think making it easy to lint against {{{ may be an okay alternative to deprecating it.

[wagenet]

It seems that there's been a lot of healthy discussion around this RFC so I'm going to try to get it moving again.