Add JSXExpressionContainer as the href value type for target=_blank

[ksdmitrieva]

Right now the jsx-no-target-blank rule only fires if the href is a literal string. What about the cases when the value of the href comes from a JSX expression? For example:

<a target='_blank' href={this.props.url}></a>

This might be even more dangerous if the URL property is provided by the user or the third-party.

Would it make sense to add a function hasDynamicLink() like this one:
function hasDynamicLink(element) {
return element.attributes.some(attr => attr.name &&
attr.name.name === 'href' &&
attr.value.type === 'JSXExpressionContainer');
}

And then include it in the condition for the rule:

if (
isTargetBlank(node) &&
(hasExternalLink(node.parent) || hasDynamicLink(node.parent)) &&
!hasSecureRel(node.parent)
)
I tested it out locally. Will be happy to help with a PR.

[ljharb]

This seems reasonable; but what about the false positive case, where the dynamic link is internal?

I'd hate to force people to disable the entire rule in that case.

[ksdmitrieva]

That is a fair point. I guess, I'm looking at it from security review point of view. In this case, I'd rather know that the link is created dynamically, so I can trace where the dynamic value comes from and if it may contain untrusted input. But, yes, you're right this will generate some number of false positives.

…

On Fri, Mar 23, 2018 at 6:32 PM, Jordan Harband ***@***.***> wrote: This seems reasonable; but what about the false positive case, where the dynamic link is internal? I'd hate to force people to disable the entire rule in that case. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1737 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AF1Rz5cbDIaYywfUNpNbpklWcIq_NAD2ks5thXfogaJpZM4S3qJK> .

[ljharb]

This seems like it could be closed by #1784?