Repository: ember-template-lint/ember-template-lint
Pull request #591 (2019-01-06 20:00:44)

feat: update 'style-concatenation' rule to catch an unsafe usage of the 'concat' helper.
This change includes:
* Updates existing rule to catch an additional unsafe case
* General rule/doc/test cleanup/improvement


Patch:
diff --git a/docs/rule/style-concatenation.md b/docs/rule/style-concatenation.md
index 253b262f..59a54017 100644
--- a/docs/rule/style-concatenation.md
+++ b/docs/rule/style-concatenation.md
@@ -4,35 +4,49 @@ Ember has a runtime warning that says:
 
 > Binding style attributes may introduce cross-site scripting vulnerabilities; please ensure that values being bound are properly escaped.
 
-This warning can only be avoided by always marking the bound value with `Ember.String.htmlSafe`. While we can't detect statically if you're always providing a safe string, we can detect common cases where it's impossible that you're doing so.
+This warning can only be avoided by marking the bound value with `Ember.String.htmlSafe`. While we can't always detect statically if you're providing a safe string, we can detect and forbid common cases where it's impossible that you're doing so.
 
-For example, implied string concatenation does not propagate `htmlSafe`, so any use of quotes is therefore forbidden.
+Common cases which do not propagate `htmlSafe` include:
+
+* Implied string concatenation using quotes
+* The `concat` helper
 
 This rule **forbids** the following:
 
 ```hbs
-<div style="background-style: url({{url}})">
+<div style="background-image: url({{url}})">
+```
+
+```hbs
+<div style="{{background-image url}}">
 ```
 
 ```hbs
-<div style="{{make-background url}}">
+<div style={{concat knownSafeStyle1 ";" knownSafeStyle2}}>
 ```
 
 This rule **allows** the following:
 
 ```hbs
-<div style={{make-background url}}>
+<div style={{html-safe (concat "background-image: url(" url ")"}}>
+```
+
+```hbs
+<div style={{background-image url}}>
+
+{{! Presumably, `background-image` is a helper which returns an `htmlSafe` style string. }}
 ```
 
 ```hbs
-<div style={{html-safe (concat knownSafeStyle1 knownSafeStyle2)}}>
+<div style={{html-safe (concat knownSafeStyle1 ";" knownSafeStyle2)}}>
 ```
 
 ### References
 
 * See the [Binding Style Attributes](https://emberjs.com/deprecations/v1.x/#toc_binding-style-attributes) Ember deprecation documentation
 * See the [documentation](https://www.emberjs.com/api/ember/release/functions/@ember%2Ftemplate/htmlSafe) for Ember's `htmlSafe` function
-* See the [documentation](https://github.com/romulomachado/ember-cli-string-helpers#html-safe) for the `html-safe` handlebars helper from the `ember-cli-string-helpers` addon
+* See the [documentation](https://www.emberjs.com/api/ember/release/classes/Ember.Templates.helpers/methods/concat?anchor=concat) for Ember's `concat` handlebars template helper
+* See the [documentation](https://github.com/romulomachado/ember-cli-string-helpers#html-safe) for the `html-safe` handlebars template helper from the `ember-cli-string-helpers` addon
 
 ### Related Rules
 
diff --git a/lib/helpers/ast-node-info.js b/lib/helpers/ast-node-info.js
index ecc24c85..e43f497a 100644
--- a/lib/helpers/ast-node-info.js
+++ b/lib/helpers/ast-node-info.js
@@ -16,10 +16,18 @@ function isCommentStatement(node) {
   return node.type === 'CommentStatement';
 }
 
+function isConcatStatement(node) {
+  return node.type === 'ConcatStatement';
+}
+
 function isMustacheCommentStatement(node) {
   return node.type === 'MustacheCommentStatement';
 }
 
+function isPathExpression(node) {
+  return node.type === 'PathExpression';
+}
+
 function isElementNode(node) {
   return node && node.type === 'ElementNode';
 }
@@ -88,7 +96,9 @@ module.exports = {
   hasChildren: hasChildren,
   isBlockStatement: isBlockStatement,
   isCommentStatement: isCommentStatement,
+  isConcatStatement: isConcatStatement,
   isMustacheCommentStatement: isMustacheCommentStatement,
+  isPathExpression: isPathExpression,
   isComponentNode: isComponentNode,
   isConfigurationHtmlComment: isConfigurationHtmlComment,
   isElementNode: isElementNode,
diff --git a/lib/rules/lint-style-concatenation.js b/lib/rules/lint-style-concatenation.js
index 84c1a27d..bca8f8d6 100644
--- a/lib/rules/lint-style-concatenation.js
+++ b/lib/rules/lint-style-concatenation.js
@@ -3,14 +3,21 @@
 const AstNodeInfo = require('../helpers/ast-node-info');
 const Rule = require('./base');
 
+const ERROR_MESSAGE = 'Concatenated styles must be marked as `htmlSafe`.';
+
 module.exports = class StyleConcat extends Rule {
   visitor() {
     return {
       ElementNode(node) {
         let style = AstNodeInfo.findAttribute(node, 'style');
-        if (style && style.value.type === 'ConcatStatement') {
+        if (
+          style && (
+            AstNodeInfo.isConcatStatement(style.value) ||
+            (AstNodeInfo.isMustacheStatement(style.value) && isConcatHelper(style.value.path))
+          )
+        ) {
           this.log({
-            message: 'You may not use string concatenation to build up styles',
+            message: ERROR_MESSAGE,
             line: style.loc && style.loc.start.line,
             column: style.loc && style.loc.start.column,
             source: this.sourceForNode(style)
@@ -20,3 +27,9 @@ module.exports = class StyleConcat extends Rule {
     };
   }
 };
+
+function isConcatHelper(node) {
+  return node && AstNodeInfo.isPathExpression(node) && node.original === 'concat';
+}
+
+module.exports.ERROR_MESSAGE = ERROR_MESSAGE;


Test patch:
diff --git a/test/unit/rules/lint-style-concatenation-test.js b/test/unit/rules/lint-style-concatenation-test.js
index a84d57c7..030d74f0 100644
--- a/test/unit/rules/lint-style-concatenation-test.js
+++ b/test/unit/rules/lint-style-concatenation-test.js
@@ -1,6 +1,7 @@
 'use strict';
 
 const generateRuleTests = require('../../helpers/rule-test-harness');
+const ERROR_MESSAGE = require('../../../lib/rules/lint-style-concatenation').ERROR_MESSAGE;
 
 generateRuleTests({
   name: 'style-concatenation',
@@ -8,16 +9,31 @@ generateRuleTests({
   config: true,
 
   good: [
+    '<img>',
+    '<img style={{myStyle}}>',
     '<img style={{background-image url}}>',
-    '<img style="background-image: url(/foo.png)"}}>'
+    '<img style="background-image: url(/foo.png)"}}>',
+    '<img style={{html-safe (concat "background-image: url(" url ")")}}>',
+    '<img style={{html-safe (concat knownSafeStyle1 ";" knownSafeStyle2)}}>'
   ],
 
   bad: [
+    {
+      template: '<img style="{{myStyle}}">',
+
+      result: {
+        message: ERROR_MESSAGE,
+        moduleId: 'layout.hbs',
+        source: 'style="{{myStyle}}"',
+        line: 1,
+        column: 5
+      }
+    },
     {
       template: '<img style="background-image: {{url}}">',
 
       result: {
-        message: 'You may not use string concatenation to build up styles',
+        message: ERROR_MESSAGE,
         moduleId: 'layout.hbs',
         source: 'style="background-image: {{url}}"',
         line: 1,
@@ -28,12 +44,23 @@ generateRuleTests({
       template: '<img style="{{background-image url}}">',
 
       result: {
-        message: 'You may not use string concatenation to build up styles',
+        message: ERROR_MESSAGE,
         moduleId: 'layout.hbs',
         source: 'style="{{background-image url}}"',
         line: 1,
         column: 5
       }
+    },
+    {
+      template: '<img style={{concat knownSafeStyle1 ";" knownSafeStyle2}}>',
+
+      result: {
+        message: ERROR_MESSAGE,
+        moduleId: 'layout.hbs',
+        source: 'style={{concat knownSafeStyle1 ";" knownSafeStyle2}}',
+        line: 1,
+        column: 5
+      }
     }
   ]
 });