[New] jsx-no-target-blank: Add warnOnSpreadAttributes option

Defaults to `false`. When set to `true`, treats spread attributes as dangerous unless explicitly overriden. e.g. the following is safe: <a {...dangerousObject} rel="noreferrer" target="_blank"></a> This change also extends target="_blank" detection to include conditional expressions whose alternate or consequent is the "_blank" string (case-insensitive). Fixes #2827
jsx-eslint · Oct 31, 2020 · c4ecce9 · c4ecce9
1 parent 66593e5
commit c4ecce9
Show file tree

Hide file tree

Showing 4 changed files with 140 additions and 52 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -11,6 +11,7 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel
 * [`jsx-no-constructed-context-values`]: add new rule which checks when the value passed to a Context Provider will cause needless rerenders ([#2763][] @dylanOshima)
 * [`jsx-wrap-multilines`]: fix crash with `declaration`s that are on a new line after `=` ([#2875][] @ljharb)
 * [`jsx-indent-props`]: add `ignoreTernaryOperator` option ([#2846][] @SebastianZimmer)
+* [`jsx-no-target-blank`]: Add `warnOnSpreadAttributes` option ([#2855][] @michael-yx-wu)
 
 ### Fixed
 * [`display-name`]/component detection: avoid a crash on anonymous components ([#2840][] @ljharb)
@@ -35,6 +36,7 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel
 [#2871]: https://github.com/yannickcr/eslint-plugin-react/issues/2871
 [#2870]: https://github.com/yannickcr/eslint-plugin-react/issues/2870
 [#2869]: https://github.com/yannickcr/eslint-plugin-react/issues/2869
+[#2855]: https://github.com/yannickcr/eslint-plugin-react/pull/2855
 [#2852]: https://github.com/yannickcr/eslint-plugin-react/pull/2852
 [#2851]: https://github.com/yannickcr/eslint-plugin-react/issues/2851
 [#2846]: https://github.com/yannickcr/eslint-plugin-react/pull/2846

diff --git a/docs/rules/jsx-no-target-blank.md b/docs/rules/jsx-no-target-blank.md
@@ -1,15 +1,11 @@
 # Prevent usage of unsafe `target='_blank'` (react/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'`, however, is a severe
-security vulnerability ([see here for more details](https://html.spec.whatwg.org/multipage/links.html#link-type-noopener))
+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'`, however, is a severe security vulnerability ([see here for more details](https://html.spec.whatwg.org/multipage/links.html#link-type-noopener))
 This rules requires that you accompany `target='_blank'` attributes with `rel='noreferrer'`.
 
 ## Rule Details
 
-This rule aims to prevent user generated links from creating security vulnerabilities by requiring
-`rel='noreferrer'` for external links, and optionally any dynamically generated links.
+This rule aims to prevent user generated links from creating security vulnerabilities by requiring `rel='noreferrer'` for external links, and optionally any dynamically generated links.
 
 ## Rule Options
 ```json
@@ -20,7 +16,8 @@ This rule aims to prevent user generated links from creating security vulnerabil
 
 * allow-referrer: optional boolean. If `true` does not require `noreferrer`. Defaults to `false`.
 * enabled: for enabling the rule. 0=off, 1=warn, 2=error. Defaults to 0.
-* enforce: optional string, 'always' or 'never'
+* enforceDynamicLinks: optional string, 'always' or 'never'
+* warnOnSpreadAttributes: optional boolean. Defaults to `false`.
 
 ### `enforceDynamicLinks`
 
@@ -56,6 +53,27 @@ Examples of **correct** code for this rule, when configured with `{ "enforceDyna
 var Hello = <a target='_blank' href={dynamicLink}></a>
 ```
 
+### `warnOnSpreadAttributes`
+
+Spread attributes are a handy way of passing programmatically-generated props to components, but may contain unsafe props e.g.
+
+```jsx
+const unsafeProps = {
+  href: "http://example.com",
+  target: "_blank",
+};
+
+<a {...unsafeProps}></a>
+```
+
+Defaults to false. If false, this rule will ignore all spread attributes. If true, this rule will treat all spread attributes as if they contain an unsafe combination of props, unless specifically overridden by props _after_ the last spread attribute prop e.g. the following would not be violations:
+
+```jsx
+<a {...unsafeProps} rel="noreferrer"></a>
+<a {...unsafeProps} target="_self"></a>
+<a {...unsafeProps} href="/some-page"></a>
+```
+
 ### Custom link components
 
 This rule supports the ability to use custom components for links, such as `<Link />` which is popular in libraries like `react-router`, `next.js` and `gatsby`. To enable this, define your custom link components in the global [shared settings](https://github.com/yannickcr/eslint-plugin-react/blob/master/README.md#configuration) under the `linkComponents` configuration area. Once configured, this rule will check those components as if they were `<a />` elements.
@@ -81,4 +99,4 @@ For links to a trusted host (e.g. internal links to your own site, or links to a
 
 ## When Not To Use It
 
-If you do not have any external links, you can disable this rule.
+If you do not have any external links, you can disable this rule.
diff --git a/lib/rules/jsx-no-target-blank.js b/lib/rules/jsx-no-target-blank.js
@@ -12,51 +12,77 @@ const linkComponentsUtil = require('../util/linkComponents');
 // Rule Definition
 // ------------------------------------------------------------------------------
 
-function isTargetBlank(attr) {
-  return attr.name
-    && attr.name.name === 'target'
-    && attr.value
-    && ((
-      attr.value.type === 'Literal'
-      && attr.value.value.toLowerCase() === '_blank'
-    ) || (
-      attr.value.type === 'JSXExpressionContainer'
-      && attr.value.expression
-      && attr.value.expression.value
-      && String(attr.value.expression.value).toLowerCase() === '_blank'
-    ));
+function lastIndexMatching(arr, condition) {
+  return arr.map(condition).lastIndexOf(true);
 }
 
-function hasExternalLink(element, linkAttribute) {
-  return element.attributes.some((attr) => attr.name
-      && attr.name.name === linkAttribute
-      && attr.value.type === 'Literal'
-      && /^(?:\w+:|\/\/)/.test(attr.value.value));
+function attributeValuePossiblyBlank(attribute) {
+  if (!attribute.value) {
+    return false;
+  }
+  const value = attribute.value;
+  if (value.type === 'Literal' && typeof value.value === 'string' && value.value.toLowerCase() === '_blank') {
+    return true;
+  }
+  if (value.type === 'JSXExpressionContainer') {
+    const expr = value.expression;
+    if (expr.type === 'Literal' && typeof expr.value === 'string' && expr.value.toLowerCase() === '_blank') {
+      return true;
+    }
+    if (expr.type === 'ConditionalExpression') {
+      if (expr.alternate.type === 'Literal' && expr.alternate.value && expr.alternate.value.toLowerCase() === '_blank') {
+        return true;
+      }
+      if (expr.consequent.type === 'Literal' && expr.consequent.value && expr.consequent.value.toLowerCase() === '_blank') {
+        return true;
+      }
+    }
+  }
+  return false;
 }
 
-function hasDynamicLink(element, linkAttribute) {
-  return element.attributes.some((attr) => attr.name
+function hasTargetBlank(node, warnOnSpreadAttributes, spreadAttributeIndex) {
+  const targetIndex = lastIndexMatching(node.attributes, (attr) => attr.name && attr.name.name === 'target');
+  const foundTargetBlank = targetIndex !== -1 && attributeValuePossiblyBlank(node.attributes[targetIndex]);
+  return foundTargetBlank || (warnOnSpreadAttributes && targetIndex < spreadAttributeIndex);
+}
+
+function hasExternalLink(node, linkAttribute, warnOnSpreadAttributes, spreadAttributeIndex) {
+  const linkIndex = lastIndexMatching(node.attributes, (attr) => attr.name && attr.name.name === linkAttribute);
+  const foundExternalLink = linkIndex !== -1 && ((attr) => attr.value.type === 'Literal' && /^(?:\w+:|\/\/)/.test(attr.value.value))(
+    node.attributes[linkIndex]);
+  return foundExternalLink || (warnOnSpreadAttributes && linkIndex < spreadAttributeIndex);
+}
+
+function hasDynamicLink(node, linkAttribute) {
+  const dynamicLinkIndex = lastIndexMatching(node.attributes, (attr) => attr.name
     && attr.name.name === linkAttribute
+    && attr.value
     && attr.value.type === 'JSXExpressionContainer');
+  if (dynamicLinkIndex !== -1) {
+    return true;
+  }
 }
 
-function hasSecureRel(element, allowReferrer) {
-  return element.attributes.find((attr) => {
-    if (attr.type === 'JSXAttribute' && attr.name.name === 'rel') {
-      const value = attr.value
-        && ((
-          attr.value.type === 'Literal'
-          && attr.value.value
-        ) || (
-          attr.value.type === 'JSXExpressionContainer'
-          && attr.value.expression
-          && attr.value.expression.value
-        ));
-      const tags = value && value.toLowerCase && value.toLowerCase().split(' ');
-      return tags && (allowReferrer ? tags.indexOf('noopener') >= 0 : tags.indexOf('noreferrer') >= 0);
-    }
+function hasSecureRel(node, allowReferrer, warnOnSpreadAttributes, spreadAttributeIndex) {
+  const relIndex = lastIndexMatching(node.attributes, (attr) => (attr.type === 'JSXAttribute' && attr.name.name === 'rel'));
+
+  if (relIndex === -1 || (warnOnSpreadAttributes && relIndex < spreadAttributeIndex)) {
     return false;
-  });
+  }
+
+  const relAttribute = node.attributes[relIndex];
+  const value = relAttribute.value
+    && ((
+      relAttribute.value.type === 'Literal'
+      && relAttribute.value.value
+    ) || (
+      relAttribute.value.type === 'JSXExpressionContainer'
+      && relAttribute.value.expression
+      && relAttribute.value.expression.value
+    ));
+  const tags = value && typeof value === 'string' && value.toLowerCase().split(' ');
+  return tags && (allowReferrer ? tags.indexOf('noopener') >= 0 : tags.indexOf('noreferrer') >= 0);
 }
 
 module.exports = {
@@ -75,6 +101,9 @@ module.exports = {
         },
         enforceDynamicLinks: {
           enum: ['always', 'never']
+        },
+        warnOnSpreadAttributes: {
+          type: 'boolean'
         }
       },
       additionalProperties: false
@@ -84,22 +113,25 @@ module.exports = {
   create(context) {
     const configuration = context.options[0] || {};
     const allowReferrer = configuration.allowReferrer || false;
+    const warnOnSpreadAttributes = configuration.warnOnSpreadAttributes || false;
     const enforceDynamicLinks = configuration.enforceDynamicLinks || 'always';
     const components = linkComponentsUtil.getLinkComponents(context);
 
     return {
-      JSXAttribute(node) {
-        if (
-          !components.has(node.parent.name.name)
-          || !isTargetBlank(node)
-          || hasSecureRel(node.parent, allowReferrer)
-        ) {
+      JSXOpeningElement(node) {
+        if (!components.has(node.name.name)) {
           return;
         }
 
-        const linkAttribute = components.get(node.parent.name.name);
+        const spreadAttributeIndex = lastIndexMatching(node.attributes, (attr) => (attr.type === 'JSXSpreadAttribute'));
+        if (!hasTargetBlank(node, warnOnSpreadAttributes, spreadAttributeIndex)) {
+          return;
+        }
 
-        if (hasExternalLink(node.parent, linkAttribute) || (enforceDynamicLinks === 'always' && hasDynamicLink(node.parent, linkAttribute))) {
+        const linkAttribute = components.get(node.name.name);
+        const hasDangerousLink = hasExternalLink(node, linkAttribute, warnOnSpreadAttributes, spreadAttributeIndex)
+          || (enforceDynamicLinks === 'always' && hasDynamicLink(node, linkAttribute));
+        if (hasDangerousLink && !hasSecureRel(node, allowReferrer, warnOnSpreadAttributes, spreadAttributeIndex)) {
           context.report({
             node,
             message: 'Using target="_blank" without rel="noreferrer" '

diff --git a/tests/lib/rules/jsx-no-target-blank.js b/tests/lib/rules/jsx-no-target-blank.js
@@ -64,6 +64,22 @@ ruleTester.run('jsx-no-target-blank', rule, {
     {code: '<a target={\'targetValue\'} href="/absolute/path"></a>'},
     {code: '<a target={"targetValue"} href="/absolute/path"></a>'},
     {code: '<a target={null} href="//example.com"></a>'},
+    {
+      code: '<a {...someObject} href="/absolute/path"></a>',
+      options: [{enforceDynamicLinks: 'always', warnOnSpreadAttributes: true}]
+    },
+    {
+      code: '<a {...someObject} rel="noreferrer"></a>',
+      options: [{enforceDynamicLinks: 'always', warnOnSpreadAttributes: true}]
+    },
+    {
+      code: '<a {...someObject} rel="noreferrer" target="_blank"></a>',
+      options: [{enforceDynamicLinks: 'always', warnOnSpreadAttributes: true}]
+    },
+    {
+      code: '<a {...someObject} href="foobar" target="_blank"></a>',
+      options: [{enforceDynamicLinks: 'always', warnOnSpreadAttributes: true}]
+    },
     {
       code: '<a target="_blank" href={ dynamicLink }></a>',
       options: [{enforceDynamicLinks: 'never'}]
@@ -143,6 +159,26 @@ ruleTester.run('jsx-no-target-blank', rule, {
     code: '<a target="_blank" href={ dynamicLink }></a>',
     options: [{enforceDynamicLinks: 'always'}],
     errors: defaultErrors
+  }, {
+    code: '<a {...someObject}></a>',
+    options: [{enforceDynamicLinks: 'always', warnOnSpreadAttributes: true}],
+    errors: defaultErrors
+  }, {
+    code: '<a {...someObject} target="_blank"></a>',
+    options: [{enforceDynamicLinks: 'always', warnOnSpreadAttributes: true}],
+    errors: defaultErrors
+  }, {
+    code: '<a href="foobar" {...someObject} target="_blank"></a>',
+    options: [{enforceDynamicLinks: 'always', warnOnSpreadAttributes: true}],
+    errors: defaultErrors
+  }, {
+    code: '<a href="foobar" target="_blank" {...someObject}></a>',
+    options: [{enforceDynamicLinks: 'always', warnOnSpreadAttributes: true}],
+    errors: defaultErrors
+  }, {
+    code: '<a href="foobar" target="_blank" rel="noreferrer" {...someObject}></a>',
+    options: [{enforceDynamicLinks: 'always', warnOnSpreadAttributes: true}],
+    errors: defaultErrors
   }, {
     code: '<Link target="_blank" href={ dynamicLink }></Link>',
     options: [{enforceDynamicLinks: 'always'}],