Target blank dynamic link should fail

[kenearley]

Made from the suggestion in @ksdmitrieva's issue, I believe this is the right approach even though you would get false positives since it cannot check for relative paths in dynamic links. However, I think that would be rare since it's not best practice to use target='_blank' on internal links anyway.

[ljharb]

I think that since it can generate false positives, it should be behind an option.

[kenearley]

Makes sense to me. I’ll update the pull request.

…

On Sat, May 12, 2018 at 1:09 PM Jordan Harband ***@***.***> wrote: I think that since it can generate false positives, it should be behind an option. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1784 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHxOftjxo6OG3Av3nU9MQ22TbUS1azTks5txyVIgaJpZM4T8FR9> .

[kenearley]

@ljharb I've added the option and set "always" as the default since it seems the safer state.

One thing I noticed is that you still would not get the error if you assigned the href using the spread operator to assign the href prop. Should that be covered in another option?

[ljharb]

I think if you can expand the error so it applies to an inline spread-"href", that would be generally useful?

[ljharb]

LGTM

[kenearley]

To be fair, if you set ALL your props within an object (eg. target, rel, etc) and then use the spread, it would bypass all these checks. Am I right?

[ljharb]

Yes, that's generally true - but spreading props is an antipattern anyways, so it also might be OK to allow the checks to be bypassed in that case.

[ksdmitrieva]

Thanks, @kenearley, for moving this along. The whole idea of the vulnerability is when untrusted link is added without "rel=noopener". When the link is hard-coded, it's most likely trusted. Otherwise, why would you add untrusted links to your application. Untrusted links will most likely come from user input. Anyway, I'm glad this is added.
Interesting edge case with the spread operator. If it's an antipattern, I wonder if there is a rule that highlights that usage.