Repository: yannickcr/eslint-plugin-react
Pull request #582 (2016-05-02 23:10:14)

New Rule: jsx-no-target-blank
New rule to protect against a pretty serious security vulnerability: https://mathiasbynens.github.io/rel-noopener

Using `target='_blank'` to open a link in a new tab is inherently unsafe and should always be accompanied by `rel='noopener noreferrer'`

coveralls: 
[![Coverage Status](https://coveralls.io/builds/6020435/badge)](https://coveralls.io/builds/6020435)

Coverage increased (+0.02%) to 96.817% when pulling **10c727ac38e100df74746dd0424ee97ff9a8c62a on Gasparila:master** into **63d33cb7f0e46484636fe178f8c8ffaaeaa024e9 on yannickcr:master**.


Patch:
diff --git a/README.md b/README.md
index 724a77f7..cc71502b 100644
--- a/README.md
+++ b/README.md
@@ -112,6 +112,7 @@ The plugin has a [recommended configuration](#user-content-recommended-configura
 * [jsx-no-bind](docs/rules/jsx-no-bind.md): Prevent usage of `.bind()` and arrow functions in JSX props
 * [jsx-no-duplicate-props](docs/rules/jsx-no-duplicate-props.md): Prevent duplicate props in JSX
 * [jsx-no-literals](docs/rules/jsx-no-literals.md): Prevent usage of unwrapped JSX strings
+* [jsx-no-target-blank](docs/rules/jsx-no-target-blank.md): Prevent usage of unsafe `target='\_blank'`
 * [jsx-no-undef](docs/rules/jsx-no-undef.md): Disallow undeclared variables in JSX
 * [jsx-pascal-case](docs/rules/jsx-pascal-case.md): Enforce PascalCase for user-defined JSX components
 * [jsx-sort-props](docs/rules/jsx-sort-props.md): Enforce props alphabetical sorting
diff --git a/docs/rules/jsx-no-target-blank.md b/docs/rules/jsx-no-target-blank.md
new file mode 100644
index 00000000..96f15dbe
--- /dev/null
+++ b/docs/rules/jsx-no-target-blank.md
@@ -0,0 +1,27 @@
+# Prevent usage of unsafe target='_blank' (jsx-no-target-blank)
+
+When creating a JSX element that has an a tag, it is often desired to have
+the link open in a new tab using the target='_blank' attribute. Using this
+attribute unaccompanied by rel='noreferrer noopener', however, is a severe
+security vulnerability ([see here for more details](https://mathiasbynens.github.io/rel-noopener))
+This rules requires that you accompany all target='_blank' attributes with rel='noreferrer noopener'.
+
+## Rule Details
+
+The following patterns are considered errors:
+
+```javascript
+var Hello = <a target='_blank'></a>
+```
+
+The following patterns are not considered erros:
+
+```javascript
+var Hello = <p target='_blank'></p>
+var Hello = <a target='_blank' rel='noopener noreferrer'></a>
+var Hello = <a></a>
+```
+
+## When Not To Use It
+
+If you do not have any external links, you can disable this rule
diff --git a/index.js b/index.js
index 78eebaf9..19693475 100644
--- a/index.js
+++ b/index.js
@@ -43,7 +43,8 @@ module.exports = {
     'no-string-refs': require('./lib/rules/no-string-refs'),
     'prefer-stateless-function': require('./lib/rules/prefer-stateless-function'),
     'require-render-return': require('./lib/rules/require-render-return'),
-    'jsx-first-prop-new-line': require('./lib/rules/jsx-first-prop-new-line')
+    'jsx-first-prop-new-line': require('./lib/rules/jsx-first-prop-new-line'),
+    'jsx-no-target-blank': require('./lib/rules/jsx-no-target-blank')
   },
   configs: {
     recommended: {
diff --git a/lib/rules/forbid-prop-types.js b/lib/rules/forbid-prop-types.js
index 16269430..19b10452 100644
--- a/lib/rules/forbid-prop-types.js
+++ b/lib/rules/forbid-prop-types.js
@@ -53,6 +53,9 @@ module.exports = function(context) {
    */
   function checkForbidden(declarations) {
     declarations.forEach(function(declaration) {
+      if (declaration.type !== 'Property') {
+        return;
+      }
       var target;
       var value = declaration.value;
       if (
diff --git a/lib/rules/jsx-curly-spacing.js b/lib/rules/jsx-curly-spacing.js
index 50f63889..a580dd90 100644
--- a/lib/rules/jsx-curly-spacing.js
+++ b/lib/rules/jsx-curly-spacing.js
@@ -34,16 +34,6 @@ module.exports = function(context) {
     return left.loc.start.line !== right.loc.start.line;
   }
 
-  /**
-   * Determines whether two adjacent tokens have whitespace between them.
-   * @param {Object} left - The left token object.
-   * @param {Object} right - The right token object.
-   * @returns {boolean} Whether or not there is space between the tokens.
-   */
-  function isSpaced(left, right) {
-    return left.range[1] < right.range[0];
-  }
-
   /**
   * Reports that there shouldn't be a newline after the first token
   * @param {ASTNode} node - The node to report in the event of an error.
@@ -161,13 +151,13 @@ module.exports = function(context) {
    */
   function validateBraceSpacing(node, first, second, penultimate, last) {
     if (spaced) {
-      if (!isSpaced(first, second)) {
+      if (!sourceCode.isSpaceBetweenTokens(first, second)) {
         reportRequiredBeginningSpace(node, first);
       } else if (!multiline && isMultiline(first, second)) {
         reportNoBeginningNewline(node, first);
       }
 
-      if (!isSpaced(penultimate, last)) {
+      if (!sourceCode.isSpaceBetweenTokens(penultimate, last)) {
         reportRequiredEndingSpace(node, last);
       } else if (!multiline && isMultiline(penultimate, last)) {
         reportNoEndingNewline(node, last);
@@ -177,11 +167,11 @@ module.exports = function(context) {
     }
 
     // "never" setting if we get here.
-    if (isSpaced(first, second) && !(multiline && isMultiline(first, second))) {
+    if (sourceCode.isSpaceBetweenTokens(first, second) && !(multiline && isMultiline(first, second))) {
       reportNoBeginningSpace(node, first);
     }
 
-    if (isSpaced(penultimate, last) && !(multiline && isMultiline(penultimate, last))) {
+    if (sourceCode.isSpaceBetweenTokens(penultimate, last) && !(multiline && isMultiline(penultimate, last))) {
       reportNoEndingSpace(node, last);
     }
   }
@@ -193,13 +183,9 @@ module.exports = function(context) {
   return {
     JSXExpressionContainer: function(node) {
       var first = context.getFirstToken(node);
-      var second = context.getFirstToken(node, 1);
-      var penultimate = sourceCode.getLastToken(node, 1);
       var last = sourceCode.getLastToken(node);
-
-      if (first === penultimate && second === last) {
-        return;
-      }
+      var second = context.getTokenAfter(first);
+      var penultimate = sourceCode.getTokenBefore(last);
 
       validateBraceSpacing(node, first, second, penultimate, last);
     }
diff --git a/lib/rules/jsx-no-target-blank.js b/lib/rules/jsx-no-target-blank.js
new file mode 100644
index 00000000..8f1d7839
--- /dev/null
+++ b/lib/rules/jsx-no-target-blank.js
@@ -0,0 +1,35 @@
+/**
+ * @fileoverview Forbid target='_blank' attribute
+ * @author Kevin Miller
+ */
+'use strict';
+
+// ------------------------------------------------------------------------------
+// Rule Definition
+// ------------------------------------------------------------------------------
+
+module.exports = function(context) {
+  return {
+    JSXAttribute: function(node) {
+      if (node.name.name === 'target' && node.value.value === '_blank') {
+        var relFound = false;
+        var attrs = node.parent.attributes;
+        for (var idx in attrs) {
+          if (attrs[idx].name.name === 'rel') {
+            var tags = attrs[idx].value.value.split(' ');
+            if (tags.indexOf('noopener') >= 0 && tags.indexOf('noreferrer') >= 0) {
+              relFound = true;
+              break;
+            }
+          }
+        }
+        if (!relFound) {
+          context.report(node, 'Using target="_blank" without rel="noopener noreferrer" ' +
+          'is a security risk: see https://mathiasbynens.github.io/rel-noopener');
+        }
+      }
+    }
+  };
+};
+
+module.exports.schema = [];
diff --git a/lib/rules/require-render-return.js b/lib/rules/require-render-return.js
index 1c82cb90..a94ac44f 100644
--- a/lib/rules/require-render-return.js
+++ b/lib/rules/require-render-return.js
@@ -22,6 +22,54 @@ module.exports = Components.detect(function(context, components, utils) {
     });
   }
 
+  /**
+   * Get properties for a given AST node
+   * @param {ASTNode} node The AST node being checked.
+   * @returns {Array} Properties array.
+   */
+  function getComponentProperties(node) {
+    switch (node.type) {
+      case 'ClassDeclaration':
+        return node.body.body;
+      case 'ObjectExpression':
+        return node.properties;
+      default:
+        return [];
+    }
+  }
+
+  /**
+   * Get properties name
+   * @param {Object} node - Property.
+   * @returns {String} Property name.
+   */
+  function getPropertyName(node) {
+    // Special case for class properties
+    // (babel-eslint does not expose property name so we have to rely on tokens)
+    if (node.type === 'ClassProperty') {
+      var tokens = context.getFirstTokens(node, 2);
+      return tokens[1] && tokens[1].type === 'Identifier' ? tokens[1].value : tokens[0].value;
+    }
+
+    return node.key.name;
+  }
+
+  /**
+   * Check if a given AST node has a render method
+   * @param {ASTNode} node The AST node being checked.
+   * @returns {Boolean} True if there is a render method, false if not
+   */
+  function hasRenderMethod(node) {
+    var properties = getComponentProperties(node);
+    for (var i = 0, j = properties.length; i < j; i++) {
+      if (getPropertyName(properties[i]) !== 'render') {
+        continue;
+      }
+      return /FunctionExpression$/.test(properties[i].value.type);
+    }
+    return false;
+  }
+
   return {
     ReturnStatement: function(node) {
       var ancestors = context.getAncestors(node).reverse();
@@ -31,8 +79,8 @@ module.exports = Components.detect(function(context, components, utils) {
           depth++;
         }
         if (
-          (ancestors[i].type !== 'Property' && ancestors[i].type !== 'MethodDefinition') ||
-          ancestors[i].key.name !== 'render' ||
+          !/(MethodDefinition|(Class)?Property)$/.test(ancestors[i].type) ||
+          getPropertyName(ancestors[i]) !== 'render' ||
           depth > 1
         ) {
           continue;
@@ -41,11 +89,19 @@ module.exports = Components.detect(function(context, components, utils) {
       }
     },
 
+    ArrowFunctionExpression: function(node) {
+      if (node.expression === false || getPropertyName(node.parent) !== 'render') {
+        return;
+      }
+      markReturnStatementPresent(node);
+    },
+
     'Program:exit': function() {
       var list = components.list();
       for (var component in list) {
         if (
           !list.hasOwnProperty(component) ||
+          !hasRenderMethod(list[component].node) ||
           list[component].hasReturnStatement ||
           (!utils.isES5Component(list[component].node) && !utils.isES6Component(list[component].node))
         ) {
diff --git a/lib/util/Components.js b/lib/util/Components.js
index 039ab554..a213c293 100644
--- a/lib/util/Components.js
+++ b/lib/util/Components.js
@@ -424,11 +424,11 @@ function componentRule(rule, context) {
     },
 
     ThisExpression: function(node) {
-      node = utils.getParentComponent();
-      if (!node || !/Function/.test(node.type)) {
+      var component = utils.getParentComponent();
+      if (!component || !/Function/.test(component.type) || !node.parent.property) {
         return;
       }
-      // Ban functions with a ThisExpression
+      // Ban functions accessing a property on a ThisExpression
       components.add(node, 0);
     },
 


Test patch:
diff --git a/tests/lib/rules/forbid-prop-types.js b/tests/lib/rules/forbid-prop-types.js
index bf277c94..b265baf6 100644
--- a/tests/lib/rules/forbid-prop-types.js
+++ b/tests/lib/rules/forbid-prop-types.js
@@ -188,6 +188,17 @@ ruleTester.run('forbid-prop-types', rule, {
       '});'
     ].join('\n'),
     parserOptions: parserOptions
+  }, {
+    // Proptypes declared with a spread property
+    code: [
+      'class Test extends react.component {',
+      '  static propTypes = {',
+      '    intl: React.propTypes.number,',
+      '    ...propTypes',
+      '  };',
+      '}'
+    ].join('\n'),
+    parser: 'babel-eslint'
   }],
 
   invalid: [{
diff --git a/tests/lib/rules/jsx-curly-spacing.js b/tests/lib/rules/jsx-curly-spacing.js
index 8341e65b..0c6d1a7c 100644
--- a/tests/lib/rules/jsx-curly-spacing.js
+++ b/tests/lib/rules/jsx-curly-spacing.js
@@ -78,6 +78,11 @@ ruleTester.run('jsx-curly-spacing', rule, {
     ].join('\n'),
     options: ['never'],
     parserOptions: parserOptions
+  }, {
+    code: '<App foo={bar/* comment */} />;',
+    options: ['never'],
+    parserOptions: parserOptions,
+    parser: 'babel-eslint'
   }],
 
   invalid: [{
diff --git a/tests/lib/rules/jsx-no-target-blank.js b/tests/lib/rules/jsx-no-target-blank.js
new file mode 100644
index 00000000..2b624e29
--- /dev/null
+++ b/tests/lib/rules/jsx-no-target-blank.js
@@ -0,0 +1,43 @@
+/**
+ * @fileoverview Forbid target='_blank' attribute
+ * @author Kevin Miller
+ */
+'use strict';
+
+// ------------------------------------------------------------------------------
+// Requirements
+// ------------------------------------------------------------------------------
+
+var rule = require('../../../lib/rules/jsx-no-target-blank');
+var RuleTester = require('eslint').RuleTester;
+
+var parserOptions = {
+  ecmaVersion: 6,
+  ecmaFeatures: {
+    jsx: true
+  }
+};
+
+// ------------------------------------------------------------------------------
+// Tests
+// ------------------------------------------------------------------------------
+
+var ruleTester = new RuleTester();
+ruleTester.run('jsx-no-target-blank', rule, {
+  valid: [
+    {code: '<a href="foobar"></a>', parserOptions: parserOptions},
+    {code: '<a randomTag></a>', parserOptions: parserOptions},
+    {code: '<a href="foobar" target="_blank" rel="noopener noreferrer"></a>', parserOptions: parserOptions}
+  ],
+  invalid: [
+    {code: '<a target="_blank"></a>', parserOptions: parserOptions,
+     errors: [{message: 'Using target="_blank" without rel="noopener noreferrer" is a security risk:' +
+      ' see https://mathiasbynens.github.io/rel-noopener'}]},
+    {code: '<a target="_blank" rel=""></a>', parserOptions: parserOptions,
+     errors: [{message: 'Using target="_blank" without rel="noopener noreferrer" is a security risk:' +
+      ' see https://mathiasbynens.github.io/rel-noopener'}]},
+    {code: '<a target="_blank" rel="noopenernoreferrer"></a>', parserOptions: parserOptions,
+     errors: [{message: 'Using target="_blank" without rel="noopener noreferrer" is a security risk:' +
+      ' see https://mathiasbynens.github.io/rel-noopener'}]}
+  ]
+});
diff --git a/tests/lib/rules/no-multi-comp.js b/tests/lib/rules/no-multi-comp.js
index 745b208a..bbf2f32d 100644
--- a/tests/lib/rules/no-multi-comp.js
+++ b/tests/lib/rules/no-multi-comp.js
@@ -61,20 +61,6 @@ ruleTester.run('no-multi-comp', rule, {
       '});'
     ].join('\r'),
     parserOptions: parserOptions
-  }, {
-    code: [
-      'export default {',
-      '  renderHello() {',
-      '    let {name} = this.props;',
-      '    return <div>{name}</div>;',
-      '  },',
-      '  renderHello2() {',
-      '    let {name2} = this.props;',
-      '    return <div>{name2}</div>;',
-      '  }',
-      '};'
-    ].join('\n'),
-    parser: 'babel-eslint'
   }, {
     code: [
       'function Hello(props) {',
@@ -179,5 +165,23 @@ ruleTester.run('no-multi-comp', rule, {
       message: 'Declare only one React component per file',
       line: 4
     }]
+  }, {
+    code: [
+      'export default {',
+      '  renderHello(props) {',
+      '    let {name} = props;',
+      '    return <div>{name}</div>;',
+      '  },',
+      '  renderHello2(props) {',
+      '    let {name} = props;',
+      '    return <div>{name}</div>;',
+      '  }',
+      '};'
+    ].join('\n'),
+    parser: 'babel-eslint',
+    errors: [{
+      message: 'Declare only one React component per file',
+      line: 6
+    }]
   }]
 });
diff --git a/tests/lib/rules/require-render-return.js b/tests/lib/rules/require-render-return.js
index d29960b2..8da8fbff 100644
--- a/tests/lib/rules/require-render-return.js
+++ b/tests/lib/rules/require-render-return.js
@@ -37,6 +37,28 @@ ruleTester.run('require-render-return', rule, {
       '}'
     ].join('\n'),
     parserOptions: parserOptions
+  }, {
+    // ES6 class with render property
+    code: [
+      'class Hello extends React.Component {',
+      '  render = () => {',
+      '    return <div>Hello {this.props.name}</div>;',
+      '  }',
+      '}'
+    ].join('\n'),
+    parser: 'babel-eslint',
+    parserOptions: parserOptions
+  }, {
+    // ES6 class with render property (implicit return)
+    code: [
+      'class Hello extends React.Component {',
+      '  render = () => (',
+      '    <div>Hello {this.props.name}</div>',
+      '  )',
+      '}'
+    ].join('\n'),
+    parser: 'babel-eslint',
+    parserOptions: parserOptions
   }, {
     // ES5 class
     code: [
@@ -93,6 +115,23 @@ ruleTester.run('require-render-return', rule, {
       '}'
     ].join('\n'),
     parserOptions: parserOptions
+  }, {
+    // ES6 class without a render method
+    code: 'class Hello extends React.Component {}',
+    parserOptions: parserOptions
+  }, {
+    // ES5 class without a render method
+    code: 'var Hello = React.createClass({});',
+    parserOptions: parserOptions
+  }, {
+    // ES5 class with an imported render method
+    code: [
+      'var render = require(\'./render\');',
+      'var Hello = React.createClass({',
+      '  render',
+      '});'
+    ].join('\n'),
+    parserOptions: parserOptions
   }],
 
   invalid: [{
@@ -133,5 +172,19 @@ ruleTester.run('require-render-return', rule, {
     errors: [{
       message: 'Your render method should have return statement'
     }]
+  }, {
+    // Missing return ES6 class render property
+    code: [
+      'class Hello extends React.Component {',
+      '  render = () => {',
+      '    <div>Hello {this.props.name}</div>',
+      '  }',
+      '}'
+    ].join('\n'),
+    parser: 'babel-eslint',
+    parserOptions: parserOptions,
+    errors: [{
+      message: 'Your render method should have return statement'
+    }]
   }
 ]});