Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't warn react/jsx-no-target-blank if "opener" or "referrer" are in the rel attribute #2054

Closed
Jessidhia opened this issue Nov 26, 2018 · 12 comments

Comments

@Jessidhia
Copy link
Contributor

Jessidhia commented Nov 26, 2018

In other words, these should all not warn:

<a href={...} target='_blank' rel='noopener noreferrer'/>
<a href={...} target='_blank' rel='noopener referrer'/>
<a href={...} target='_blank' rel='opener noreferrer'/>
<a href={...} target='_blank' rel='opener referrer'/>

However, if either (no)opener or (no)referer are missing, the rule should still warn.

@Jessidhia Jessidhia changed the title Don't warn react/jsx-no-target-blank if "opener" or "referer" are in the rel attribute Don't warn react/jsx-no-target-blank if "opener" or "referrer" are in the rel attribute Nov 26, 2018
@ljharb
Copy link
Member

ljharb commented Nov 26, 2018

I'm confused; the first one already doesn't warn: https://github.com/yannickcr/eslint-plugin-react/blob/master/tests/lib/rules/jsx-no-target-blank.js#L37

As for the others - see https://mathiasbynens.github.io/rel-noopener/. "opener" is never recommended, so I would expect those to warn as well.

The same logic seems to support not allowing "referrer" either.

@Jessidhia
Copy link
Contributor Author

Jessidhia commented Nov 26, 2018

The first one is currently the only case that doesn't warn, I'm just suggesting that explicitly saying you want referrer should be allowed.

For newer browsers, noreferrer also prevents sending the Referer header, which you might want to intentionally send.

I indeed haven't found any case where I'd want to use opener, but I found many instances where the Referer header should still be sent even if window.opener should be nulled.

@ljharb
Copy link
Member

ljharb commented Nov 26, 2018

If something is only safe on newer browsers, then it’s not safe, and shouldn’t be permitted.

What cases do you have where you want the referrer header sent?

@Jessidhia
Copy link
Contributor Author

Jessidhia commented Nov 27, 2018

target='_blank' links that open pages that we control, or from specific target sites for analytics.

We're using a module to generate URLs instead of hardcoding paths (a manually maintained reverse router, if you will), and that trips the plugin even for ultimately relative URLs with target='_blank'.

enforceDynamicLinks: 'never' would work but then it wouldn't catch cases where it's like user input URLs. The suggestion is to have a way to indicate it is intended that Referer is sent without just disabling the rule or making it strictly weaker by disabling dynamic links.

@ljharb
Copy link
Member

ljharb commented Nov 27, 2018

hmm. so it's not even about external URLs, it's about certain special cases of dynamic URLs where you'd prefer to not even have to specify opener/referrer?

Considering that force-opening links in a new tab is inherently a hostile UX interaction, and even more so for links on the same site, I'm not inclined to let the default behavior allow either opener or referrer.

Is there a rule option you could think of that would be simpler than eslint override comments or removing the blank target, that would support your use case?

@davidje13
Copy link

davidje13 commented Feb 22, 2019

I have encountered this issue for a particular link to an external site where it is desirable to keep the Referer heading working.

I agree that opener is always questionable at best. It's a strong indication of a security issue for any link (internal or external) to keep a reference to the opening page.

Referrer is a bit different though; unless there's a particularly sensitive piece of information in the URL, it is not a security concern. It is also sent as standard practice for any link which isn't target="_blank". It might be a privacy concern in some situations, but that's a separate issue. Bundling the two together in a single lint rule is perhaps questionable in itself (opener really is specific to _blank, but referrer has no real connection to it).

I think the proposed solution of allowing developers to explicitly specify that referrer is OK for a particular link is a good one. If I could mark the link as "noopener referrer" and have the lint warning disappear I'd be happy with that. I don't agree with allowing "opener" though; if somebody wants to do that they should have to explicitly disable the rule, because that indicates a genuine design flaw.

Right now my only recourse is to surround the block with eslint-disable lines, which is not ideal since it means any other links in the same section will be ignored.

Rule options are not useful to me, because I want to be able to apply this to a specific link, not a bunch of links with something in common. I want the rule to apply as normal everywhere else.

@ljharb
Copy link
Member

ljharb commented Feb 22, 2019

Allowing an explicit referer seems reasonable. The common case will be a link with neither attribute, and it won’t open the door for opener.

@peterbe
Copy link

peterbe commented Apr 18, 2019

If I could mark the link as "noopener referrer" and have the lint warning disappear I'd be happy with that.

I totally agree with that. The referer bit can leak privacy but being able to set noopener referer seems exactly what we need. And I still like that it warns on noopner (alone) because it forces you to think about what it might mean to leak the referer.

@robations
Copy link

Just to add the Lighthouse audit for this issue recommends that one of rel=noopener or rel=noreferrer be set, rather than require both:

https://developers.google.com/web/tools/lighthouse/audits/noopener

@Vadorequest
Copy link

This creates noises. Most of my links have noopener attached on it (for security purpose), but for SEO purpose I want the referrer to be allowed.

I'd like an option to avoid warnings when one of them is used (instead of both). I also believe it should become the default behaviour.

wu-lee pushed a commit to TransitionbyDesign/homemaker that referenced this issue Nov 6, 2020
> warning  Using target="_blank" without rel="noreferrer" is a security risk: see
https://html.spec.whatwg.org/multipage/links.html#link-type-noopener
react/jsx-no-target-blank

This seems to be valid. See jsx-eslint/eslint-plugin-react#2054
wu-lee pushed a commit to TransitionbyDesign/homemaker that referenced this issue Nov 6, 2020
> warning  Using target="_blank" without rel="noreferrer" is a security risk: see
https://html.spec.whatwg.org/multipage/links.html#link-type-noopener
react/jsx-no-target-blank

This seems to be valid. See jsx-eslint/eslint-plugin-react#2054
@aaronmgdr
Copy link

just ran into an issue where i had

target="blank" rel="noreferrer" and it kept warning me that Using target="_blank" without rel="noreferrer" is a security risk

finally figured out i needed to add noopener

if it's possible for the warning to be more clear about what needs to be changed I think that would be a great improvement.

@edemaine
Copy link
Contributor

edemaine commented Feb 22, 2021

@aaronmgdr Merged PR #2925 already fixes this particular issue (#2924); once there's a release, there won't be an error anymore with rel="noreferrer".

@ljharb ljharb closed this as completed Feb 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

8 participants