From c4ecce98d07f3d865e0458efed73e4bc787ec439 Mon Sep 17 00:00:00 2001 From: Michael Wu Date: Fri, 30 Oct 2020 21:25:45 -0400 Subject: [PATCH] [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: This change also extends target="_blank" detection to include conditional expressions whose alternate or consequent is the "_blank" string (case-insensitive). Fixes #2827 --- CHANGELOG.md | 2 + docs/rules/jsx-no-target-blank.md | 34 +++++-- lib/rules/jsx-no-target-blank.js | 120 ++++++++++++++++--------- tests/lib/rules/jsx-no-target-blank.js | 36 ++++++++ 4 files changed, 140 insertions(+), 52 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c7dd4b2815..e7901e442b 100644 --- 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 index 5729cae891..0ca6129ee2 100644 --- 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 = ``` +### `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", +}; + + +``` + +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 + + + +``` + ### Custom link components This rule supports the ability to use custom components for links, such as `` 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 `` 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. \ No newline at end of file +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 index 5c0f5353d7..656cd3009f 100644 --- 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 index e4b813ade9..dbe9a0e528 100644 --- 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: ''}, {code: ''}, {code: ''}, + { + code: '', + options: [{enforceDynamicLinks: 'always', warnOnSpreadAttributes: true}] + }, + { + code: '', + options: [{enforceDynamicLinks: 'always', warnOnSpreadAttributes: true}] + }, + { + code: '', + options: [{enforceDynamicLinks: 'always', warnOnSpreadAttributes: true}] + }, + { + code: '', + options: [{enforceDynamicLinks: 'always', warnOnSpreadAttributes: true}] + }, { code: '', options: [{enforceDynamicLinks: 'never'}] @@ -143,6 +159,26 @@ ruleTester.run('jsx-no-target-blank', rule, { code: '', options: [{enforceDynamicLinks: 'always'}], errors: defaultErrors + }, { + code: '', + options: [{enforceDynamicLinks: 'always', warnOnSpreadAttributes: true}], + errors: defaultErrors + }, { + code: '', + options: [{enforceDynamicLinks: 'always', warnOnSpreadAttributes: true}], + errors: defaultErrors + }, { + code: '', + options: [{enforceDynamicLinks: 'always', warnOnSpreadAttributes: true}], + errors: defaultErrors + }, { + code: '', + options: [{enforceDynamicLinks: 'always', warnOnSpreadAttributes: true}], + errors: defaultErrors + }, { + code: '', + options: [{enforceDynamicLinks: 'always', warnOnSpreadAttributes: true}], + errors: defaultErrors }, { code: '', options: [{enforceDynamicLinks: 'always'}],