From 53a788875f00c4678809d35af269e6ae0b5d7090 Mon Sep 17 00:00:00 2001 From: Gavin Joyce Date: Sat, 9 Feb 2019 12:07:42 +0100 Subject: [PATCH 1/5] `trustedHtml` and `trusted-html` --- text/0000-template.md | 93 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 93 insertions(+) create mode 100644 text/0000-template.md diff --git a/text/0000-template.md b/text/0000-template.md new file mode 100644 index 0000000000..afa4eaceb4 --- /dev/null +++ b/text/0000-template.md @@ -0,0 +1,93 @@ +- Start Date: 2019-02-09 +- Relevant Teams: Ember.js, Learning +- RFC PR: https://github.com/emberjs/rfcs/pull/443 +- Tracking: (leave this empty) + +# `trustedHtml` and `trusted-html` + +## Summary + +This RFC supersedes [RFC 319](https://github.com/emberjs/rfcs/pull/319) and proposes that we: + + * Deprecate `htmlSafe` in favor of a new `trustedHtml` function. + * Deprecate `{{{` in favor of a new `trusted-html` handlebars helper. + +## Motivation + +When rendering, Ember escapes values by default. This helps prevent security attack vectors such as XSS when rendering untrusted content from end-users of the application. Sometimes a developer will have a need to bypass this default and render unescaped content directly, for example when rendering sanatized HTML returned from a server endpoint. + +Ember currently provides a couple of ways to render unescaped content, [`htmlSafe`](https://www.emberjs.com/api/ember/release/functions/@ember%2Ftemplate/htmlSafe) and the `{{{` handlebars syntax. + +There are a number of downsides to these: + + * `htmlSafe` is a confusing name, it's a common and understandable mistake to think that this function sanatizes markup to make it safe. + * `{{{` is very similar to `{{` and the slight different in syntax doesn't indicate the important difference in how these two syntaxes behave. + * `htmlSafe` and `{{{` have similar responsibilities but they have different names. + +Replacing `htmlSafe` and `{{{` with `trustedHtml` and `trusted-html` brings a number of benefits: + + * It helps make Ember easier to undertand and more consistent. + * It helps prevent developers from inadvertantly introducing serious security attack vectors by encouraging them to consider the trust aspects of rendering unescaped content. + * The `html-safe` helper can be used in sub-expressions and the result can be passed around in ways that `{{{` can not. + +## Detailed design + +### `htmlSafe` -> `trustedHtml` + +We'll create a new `trustedHtml` function which will be based on the [existing `htmlSafe` implementation](https://github.com/emberjs/ember.js/blob/dff3c621801999e06dc773ce50a35a97233a0eb5/packages/%40ember/-internals/glimmer/lib/utils/string.ts#L72-L85). + +Note: It might be wise to rename `Handlebars.SafeString` to `Handlebars.TrustedString` in future. In the interest of keeping things simple, we'll leave that outside the context of this RFC though. + +We'll modify `htmlSafe` to log a deprecation warning and then internally invoke `trustedHtml`. The deprecation warning will be something like: + +``` +Using `htmlSafe` is deprecated, please use `trustedHtml` instead. +``` + +### `{{{` -> `trusted-html` + +We'll create a new `trusted-html` handlebars helper: + +```js +import { helper } from '@ember/component/helper'; +import { trustedHtml } from '@ember/string'; + +export function trustedHtml([html]) { + return trustedHtml(html); +} +``` + +We'll create an AST transform in [`packages/ember-template-compiler`](https://github.com/emberjs/ember.js/tree/master/packages/ember-template-compiler) which will emit a deprecation warning for all uses of `{{{`. The deprecation warning will be something like: + +``` +Using `{{{` is deprecated, please use the `trusted-html` helper instead. +``` + +We'll create a codemod to help people automatically migrate their uses of `{{{` to `trusted-html`. + +## How we teach this + +The `trusted-html` template helper should be mentioned in the [API docs](https://emberjs.com/api/ember/release/classes/Ember.Templates.helpers). + +We should replace the current [`htmlSafe` API documentation](https://www.emberjs.com/api/ember/release/functions/@ember%2Ftemplate/htmlSafe) with similar documentation for `trustedHtml`. + +If we have a security section in the guides, we should mention how using `trusted-html` and `trustedHtml` requires the developer to understand the risks that rendering unescaped content can pose and that they are asserting that they do trust the content. + +## Drawbacks + +This RFC introduces some API changes which require some changes to existing applications. We'll need to teach Ember users the new function and helper. + +## Alternatives + +There was some discussion on the [original RFC](https://github.com/emberjs/rfcs/pull/319) for some alternative naming. These include: + + * `trust-this-html` + * `unescape(d)` + * `raw` + * `rawHtml` + * `dangerously-render-html` + * `dangerous-html` + +## Unresolved questions + +> Is `trusted-html` the best name? Are there additional suggestions that I should include in the section above? From 3543a291380588190fe1ca78756065be804112da Mon Sep 17 00:00:00 2001 From: Gavin Joyce Date: Mon, 11 Feb 2019 08:00:33 +0000 Subject: [PATCH 2/5] spelling --- text/0000-template.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/text/0000-template.md b/text/0000-template.md index afa4eaceb4..68ef20fc99 100644 --- a/text/0000-template.md +++ b/text/0000-template.md @@ -14,7 +14,7 @@ This RFC supersedes [RFC 319](https://github.com/emberjs/rfcs/pull/319) and prop ## Motivation -When rendering, Ember escapes values by default. This helps prevent security attack vectors such as XSS when rendering untrusted content from end-users of the application. Sometimes a developer will have a need to bypass this default and render unescaped content directly, for example when rendering sanatized HTML returned from a server endpoint. +When rendering, Ember escapes values by default. This helps prevent security attack vectors such as XSS when rendering untrusted content from end-users of the application. Sometimes a developer will have a need to bypass this default and render unescaped content directly, for example when rendering sanitized HTML returned from a server endpoint. Ember currently provides a couple of ways to render unescaped content, [`htmlSafe`](https://www.emberjs.com/api/ember/release/functions/@ember%2Ftemplate/htmlSafe) and the `{{{` handlebars syntax. @@ -27,7 +27,7 @@ There are a number of downsides to these: Replacing `htmlSafe` and `{{{` with `trustedHtml` and `trusted-html` brings a number of benefits: * It helps make Ember easier to undertand and more consistent. - * It helps prevent developers from inadvertantly introducing serious security attack vectors by encouraging them to consider the trust aspects of rendering unescaped content. + * It helps prevent developers from inadvertently introducing serious security attack vectors by encouraging them to consider the trust aspects of rendering unescaped content. * The `html-safe` helper can be used in sub-expressions and the result can be passed around in ways that `{{{` can not. ## Detailed design From bb6341a19e2032b6dc1bfcf356187ba88146e029 Mon Sep 17 00:00:00 2001 From: Gavin Joyce Date: Mon, 11 Feb 2019 08:35:01 +0000 Subject: [PATCH 3/5] add unresolved question about deprecating `{{{` --- text/0000-template.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/text/0000-template.md b/text/0000-template.md index 68ef20fc99..9de879d6de 100644 --- a/text/0000-template.md +++ b/text/0000-template.md @@ -90,4 +90,5 @@ There was some discussion on the [original RFC](https://github.com/emberjs/rfcs/ ## Unresolved questions -> Is `trusted-html` the best name? Are there additional suggestions that I should include in the section above? + * Is `trusted-html` the best name? Are there additional suggestions that we should include in the section above? + * Instead of deprecating `{{{`, perhaps we could just help developers to lint against it? From 66a741137eafb0c124cf62de482bdb16b9fb1e8a Mon Sep 17 00:00:00 2001 From: Gavin Joyce Date: Tue, 12 Feb 2019 10:23:12 +0000 Subject: [PATCH 4/5] add some additional suggestions --- text/0000-template.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/text/0000-template.md b/text/0000-template.md index 9de879d6de..0d2f31c7ce 100644 --- a/text/0000-template.md +++ b/text/0000-template.md @@ -73,6 +73,12 @@ We should replace the current [`htmlSafe` API documentation](https://www.emberjs If we have a security section in the guides, we should mention how using `trusted-html` and `trustedHtml` requires the developer to understand the risks that rendering unescaped content can pose and that they are asserting that they do trust the content. +We should revisit the binding style attributes warning message and the content that it links to: + +``` +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. +``` + ## Drawbacks This RFC introduces some API changes which require some changes to existing applications. We'll need to teach Ember users the new function and helper. @@ -87,6 +93,10 @@ There was some discussion on the [original RFC](https://github.com/emberjs/rfcs/ * `rawHtml` * `dangerously-render-html` * `dangerous-html` + * `unsafe-html` + * `bypass-sanitization` + * `bypass-html-escaping` + * `trusted` ## Unresolved questions From 11d782d3355d1b864e16e7bfee2b91a2dc577607 Mon Sep 17 00:00:00 2001 From: Gavin Joyce Date: Wed, 13 Feb 2019 13:19:15 +0000 Subject: [PATCH 5/5] added additional unresolved questions --- text/0000-template.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/text/0000-template.md b/text/0000-template.md index 0d2f31c7ce..3ed1dbb9eb 100644 --- a/text/0000-template.md +++ b/text/0000-template.md @@ -102,3 +102,5 @@ There was some discussion on the [original RFC](https://github.com/emberjs/rfcs/ * Is `trusted-html` the best name? Are there additional suggestions that we should include in the section above? * Instead of deprecating `{{{`, perhaps we could just help developers to lint against it? + * Should these helpers/functions differentiate between HTML and style trusted strings? See [this comment](https://github.com/emberjs/rfcs/pull/443#issuecomment-462282379) for details. + * Should we remove the current `Binding style attributes may introduce cross-site scripting vulnerabilities` warning? See [this comment](https://github.com/emberjs/rfcs/pull/443#issuecomment-463129055) for more details.