Add new rule `no-html-safe` #1079

patocallaghan · 2021-01-28T23:29:44Z

Introduces a no-html-safe rule to prevent the usage of the htmlSafe utility.

htmlSafe marks a string as safe for unescaped output with Ember templates so you can render it as HTML. While useful this can inadvertently open you up to Cross-site Scripting (XSS) vulnerabilities, especially if the string was generated from user input or some other untrusted source.

patocallaghan · 2021-01-28T23:31:34Z

lib/rules/no-html-safe.js

@@ -0,0 +1,37 @@
+'use strict';
+
+const ERROR_MESSAGE = 'Do not use htmlSafe.';


Error message is a bit dry. Would it better to do something like the following?

Do not use htmlSafe. It can open you up to XSS vulnerabilities.

I normally like to use the format Use X instead of Y for error messages, but there doesn't seem to be one single preferred fix for this violation.

Yeah I like doing similar but not sure we could fit any actionable advice in the error. How about linking to rule .md file?

We should not link to the documentation in the error message because the rule already includes the documentation link in its meta data and many IDEs will already provide a link to the documentation directly on the highlighted rule violation.

docs/rules/no-html-safe.md

patocallaghan · 2021-01-28T23:53:30Z

docs/rules/no-html-safe.md

+import { htmlSafe } from '@ember/string';
+```
+
+## Alternatives


There are no exact alternatives to using htmlSafe so I put together this Alternatives section which outlines some ways you can minimise your usage of htmlSafe. Happy for feedback whether this is suitable or not.

bmish · 2021-01-29T00:31:00Z

lib/rules/no-html-safe.js

+          (node.source.value === '@ember/string' || node.source.value === '@ember/template') &&
+          node.specifiers.some((s) => s.local.name === 'htmlSafe')
+        ) {
+          context.report(node, ERROR_MESSAGE);


Need to report the specific specifier htmlSafe instead of the entire import statement. We don't want to flag other specifiers that might be imported in the same statement.

bmish · 2021-01-29T00:37:09Z

docs/rules/no-html-safe.md

+<h1>{{@userContent}}</h1>
+```
+
+### Alternative 2: Define your own trusted sanitizing helper


Regarding alternatives 2 and 3, I'm unclear why it would be better to define a custom sanitizing helper. In fact, encouraging custom sanitizing helpers sound like a dangerous anti-pattern, as they would be much more likely to have bugs / security vulnerabilities versus using a published/trusted helper like htmlSafe from Ember.

So htmlSafe doesn't do any sanitization which is why it can easily become a XSS vector. With a name like htmlSafe I've been caught out and reviewed many PRs where people assume it is doing sanitization. All it does is takes the string with the HTML in it and says "yep you can render this string in a template as html", even if the string has XSSs in it.

React has a similar helper to htmlSafe but it's called dangerouslySetInnerHTML which is a lot more obvious you shouldn't be using it. They even have a similar lint rule.

For 2 and 3, I'm advocating if you have to use htmlSafe always sanitize the content first that you are going to mark safe. I'd recommend a sanitizer like DOMPurify but wasn't sure whether I'd call it out.

Happy to get further feedback though. Even happy to cut back the section altogether if you think it will just add to confusion.

I've tweaked the opening paragraph mentioning that htmlSafe doesn't perform sanitization, and also added updated alternative to mention DOMPurify.

Ah my bad. I knew htmlSafe doesn't do any sanitizing but it's easy get confused about it due to its poor name as you mention.

This plan sounds good:

Updating the doc to help people avoid that confusion

Suggesting something like DOMPurify so people don't think they need to write a custom sanitizer with their own string parsing

Ya, the name really is unfortunate. It is trying to be the developer saying "hey this is html safe", but most people read it as "hey please make this html safe for me".

docs/rules/no-html-safe.md

tests/lib/rules/no-html-safe.js

bmish · 2021-01-29T00:42:04Z

lib/rules/no-html-safe.js

@@ -0,0 +1,37 @@
+'use strict';
+
+const ERROR_MESSAGE = 'Do not use htmlSafe.';


I normally like to use the format Use X instead of Y for error messages, but there doesn't seem to be one single preferred fix for this violation.

lib/rules/no-html-safe.js

bmish · 2021-01-29T22:44:05Z

lib/rules/no-html-safe.js

+  meta: {
+    type: 'suggestion',
+    docs: {
+      description: 'disallow the use of htmlSafe',


Can you put htmlSafe in backticks to make it clear it's a function name?

Looks like this isn't done yet. You will need to run yarn update after updating the message.

Fix in: #1080

patocallaghan · 2021-01-29T23:01:31Z

Thanks @bmish 🙇‍♂️ I've updated the error message with backticks and just squashed all my commits.

rwjblue · 2021-01-30T03:32:40Z

docs/rules/no-html-safe.md

+Examples of **incorrect** code for this rule:
+
+```js
+import { htmlSafe } from '@ember/template';


I would vaguely prefer to fail at the usage site, not the import site. 🤔

I agree with this so I am updating the location where the violation is flagged in a follow-up PR: #1080

patocallaghan · 2021-01-30T20:09:05Z

Thanks for all the help folks 🙇‍♂️

patocallaghan commented Jan 28, 2021

View reviewed changes

docs/rules/no-html-safe.md Show resolved Hide resolved

patocallaghan force-pushed the patoc/no-html-safe branch from 1ba1ddf to 8ca8858 Compare January 28, 2021 23:50

patocallaghan commented Jan 28, 2021

View reviewed changes

bmish reviewed Jan 29, 2021

View reviewed changes

bmish requested a review from rwjblue January 29, 2021 00:43

bmish added the enhancement label Jan 29, 2021

bmish changed the title ~~Add no-html-safe rule~~ Add new rule no-html-safe Jan 29, 2021

bmish reviewed Jan 29, 2021

View reviewed changes

lib/rules/no-html-safe.js Outdated Show resolved Hide resolved

bmish reviewed Jan 29, 2021

View reviewed changes

bmish approved these changes Jan 29, 2021

View reviewed changes

patocallaghan force-pushed the patoc/no-html-safe branch from 0a8c1ac to 8b98d82 Compare January 29, 2021 22:55

Add no-html-safe rule

a77650e

patocallaghan force-pushed the patoc/no-html-safe branch from 8b98d82 to a77650e Compare January 29, 2021 23:00

rwjblue reviewed Jan 30, 2021

View reviewed changes

bmish merged commit e9459ea into ember-cli:master Jan 30, 2021

bmish mentioned this pull request Jan 30, 2021

Update no-html-safe rule to flag call instead of import #1080

Merged

patocallaghan deleted the patoc/no-html-safe branch January 30, 2021 20:08

This was referenced Mar 11, 2021

Bump eslint-plugin-ember from 9.6.0 to 10.2.0 alexlafroscia/ember-concurrency-wrap-in-task#127

Merged

chore(deps-dev): bump eslint-plugin-ember from 9.6.0 to 10.2.0 alexlafroscia/ember-esbuild#20

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new rule `no-html-safe` #1079

Add new rule `no-html-safe` #1079

patocallaghan commented Jan 28, 2021

patocallaghan Jan 28, 2021 •

edited

Loading

bmish Jan 29, 2021 •

edited

Loading

patocallaghan Jan 29, 2021

bmish Jan 29, 2021

patocallaghan Jan 28, 2021

bmish Jan 29, 2021

bmish Jan 29, 2021

patocallaghan Jan 29, 2021 •

edited

Loading

patocallaghan Jan 29, 2021

bmish Jan 29, 2021

rwjblue Jan 30, 2021

bmish Jan 29, 2021 •

edited

Loading

bmish Jan 29, 2021

bmish Jan 29, 2021

bmish Jan 30, 2021

patocallaghan commented Jan 29, 2021

rwjblue Jan 30, 2021

bmish Jan 30, 2021

patocallaghan commented Jan 30, 2021

		@@ -0,0 +1,37 @@
		'use strict';

		const ERROR_MESSAGE = 'Do not use htmlSafe.';

Add new rule no-html-safe #1079

Add new rule no-html-safe #1079

Conversation

patocallaghan commented Jan 28, 2021

patocallaghan Jan 28, 2021 • edited Loading

Choose a reason for hiding this comment

bmish Jan 29, 2021 • edited Loading

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

patocallaghan Jan 29, 2021 • edited Loading

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

bmish Jan 29, 2021 • edited Loading

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

patocallaghan commented Jan 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patocallaghan commented Jan 30, 2021

Add new rule `no-html-safe` #1079

Add new rule `no-html-safe` #1079

patocallaghan Jan 28, 2021 •

edited

Loading

bmish Jan 29, 2021 •

edited

Loading

patocallaghan Jan 29, 2021 •

edited

Loading

bmish Jan 29, 2021 •

edited

Loading