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

target=_blank rel=noreferrer implies noopener #2022

Closed
developit opened this issue Oct 22, 2018 · 18 comments · Fixed by #2043
Closed

target=_blank rel=noreferrer implies noopener #2022

developit opened this issue Oct 22, 2018 · 18 comments · Fixed by #2043

Comments

@developit
Copy link

As it turns out, <a target="_blank" rel="noreferrer"> actually implies noopener behavior.
See spec note here: https://html.spec.whatwg.org/multipage/links.html#link-type-noreferrer

This means adding/enforcing noopener here is redundant:
https://github.com/yannickcr/eslint-plugin-react/blob/72a71b302c5f36ee3dd440da43737afc21cc311e/lib/rules/jsx-no-target-blank.js#L38

@ljharb
Copy link
Member

ljharb commented Oct 22, 2018

That’s the latest spec; for which browsers is this true/are there any browsers for which this is not true? It seems safer to specify both.

@developit
Copy link
Author

Discussion is ongoing. Due to the fact that noreferrer predates noopener, there are cases where noopener is ignored (because the behavior is not implemented).

I'll update here if there's a conclusive result to the discussion.

@seancrater
Copy link
Contributor

seancrater commented Nov 11, 2018

@developit @yannickcr @ljharb It seems the Typescript linting issue related to this above was resolved by removing noopener as part of the a element linting. Any update on this? I can take this on if we'd like to do it

@seancrater
Copy link
Contributor

I believe this might be enough evidence to go through with this:

image

As per: https://html.spec.whatwg.org/multipage/links.html#link-type-noopener

@ljharb
Copy link
Member

ljharb commented Nov 12, 2018

@seancrater I don't; the spec is often aspirational, and doesn't describe historical (noncompliant) engines.

In general, if any version of any browser ever required both, then we should use both for the foreseeable future.

@feross
Copy link

feross commented Aug 15, 2019

This is sort-of related to the current issue, but apologies if it should have been a new issue.

The point of this rule is to prevent window.opener from working, right?

From @mathiasbynens's blog post About rel=noopener:

To prevent pages from abusing window.opener, use rel=noopener. This ensures window.opener is null in Chrome 49 & Opera 36, Firefox 52, Desktop Safari 10.1+, and iOS Safari 10.3+.

I only support modern browsers (and I think many others do as well) so for my use case it's enough to simply specify rel=noopener without rel=noreferrer. I'd like it if this rule treated rel=noopener alone as safe, or at least offered an option for this. The only browsers that don't support this are quite old at this point.

@Hypnosphi
Copy link
Contributor

Hypnosphi commented Aug 25, 2019

if any version of any browser ever required both

Actually, I found a counter-example: Firefox 33–35 removes opener with rel="noreferrer" but doesn't with rel="noreferrer noopener" (probably because it encounters an unsupported rel value). If you have a Browserstack account, you can verify it by opening http://noreferrer.hypnosphi.de (source) in FF 33–35 and clicking the links there.

@feross
Copy link

feross commented Aug 28, 2019

@Hypnosphi Interesting find. However, Firefox 33 was released in 2014-06-09 and Firefox has autoupdates enabled by default. Surely, we don't have to worry about these users anymore?

@Hypnosphi
Copy link
Contributor

Hypnosphi commented Aug 28, 2019

@feross sure, but @ljharb was talking about any version of any browser ever

@ljharb
Copy link
Member

ljharb commented Aug 29, 2019

Both Firefox and Chrome have autoupdates enabled by default; both have many outdated versions still in significant use on major websites.

@Hypnosphi
Copy link
Contributor

Hypnosphi commented Aug 29, 2019

@ljharb So WDYT about changing the rule to expect just rel="noreferrer" for wider browser support?

@ljharb
Copy link
Member

ljharb commented Aug 29, 2019

It seems like the issue is that there are these buckets of browsers:

  1. both are required
  2. both breaks it; only noreferrer is allowed (FF 33-35)
  3. both is fine but only noreferrer is required

These are obviously incompatible.

As a first step, what are the exact browsers and versions for each of these three buckets? Once that research is done and data gathered in one place, then we can decide what the best course of action is - assuming that there will need to be a way to configure "both" or "only noreferrer", given that there's no "works for everything" setting.

@Hypnosphi
Copy link
Contributor

Hypnosphi commented Aug 29, 2019

I ran a test suite over all the browser/platform combinations available on Browserstack: sources, report

Basically, browsers from bucket 1 should pass test 3 and not pass test 4. Apparently, there are none.

Bucket 2 passes test 4 but not 3. It is FF 33-35:
Screen Shot 2019-08-29 at 20 59 26

Bucket 3 passes both tests 4 and 3. There's also a bucket 4 which doesn't pass either. Those are legacy browsers without any way to remove window.opener from target="_blank" links.

Tests 1 and 2 serve as a check that the whole setup works. It fails in some browsers, so I had to check those by hand (open http://noreferrer.hypnosphi.de in Browserstack Live and click through the links). All of those browsers are either bucket 3 or 4.

The setup also didn't seem to work on Android devices at all, so I checked those by hand as well. All the browsers on the available platforms are bucket 4 except for UC Browser which opens all the links in the same tab as if they didn't have target="_blank".

@Hypnosphi
Copy link
Contributor

Hypnosphi commented May 4, 2020

@ljharb did you have a chance to take a look at the provided data?

@ljharb
Copy link
Member

ljharb commented May 4, 2020

@Hypnosphi sorry for the delay. So, to reiterate: in your testing, every browser fell into one of these categories?:

  1. legacy browsers that have no way to remove window.opener, we can't fix these
  2. browsers that remove it with just noreferrer, or with both

which means that the third category, "browsers that only remove it with both" is empty?

@Hypnosphi
Copy link
Contributor

That's correct

@ljharb
Copy link
Member

ljharb commented May 5, 2020

Given that, it seems we should modify the rule - by default, even - to no longer require both (but, to continue to allow both).

@Hypnosphi
Copy link
Contributor

Looks like #2043 does exactly that

ljharb pushed a commit to seancrater/eslint-plugin-react that referenced this issue May 7, 2020
ljharb pushed a commit to seancrater/eslint-plugin-react that referenced this issue May 7, 2020
ljharb pushed a commit to seancrater/eslint-plugin-react that referenced this issue May 7, 2020
@ljharb ljharb closed this as completed in b9d2eb5 May 7, 2020
chiabi added a commit to pedaling/class101-ui that referenced this issue Jul 2, 2020
- noreferrer의사용은 noreferrer noopener을 함께 사용하는 것과 같은 동작을 한다
- ref: jsx-eslint/eslint-plugin-react#2022
- ref: https://html.spec.whatwg.org/multipage/links.html#link-type-noreferrer
chiabi added a commit to pedaling/class101-ui that referenced this issue Jul 2, 2020
- noreferrer의사용은 noreferrer noopener을 함께 사용하는 것과 같은 동작을 한다
- ref: jsx-eslint/eslint-plugin-react#2022
- ref: https://html.spec.whatwg.org/multipage/links.html#link-type-noreferrer
chiabi added a commit to pedaling/class101-ui that referenced this issue Jul 2, 2020
- noreferrer의사용은 noreferrer noopener을 함께 사용하는 것과 같은 동작을 한다
- ref: jsx-eslint/eslint-plugin-react#2022
- ref: https://html.spec.whatwg.org/multipage/links.html#link-type-noreferrer
chiabi added a commit to pedaling/class101-ui that referenced this issue Jul 3, 2020
- noreferrer의사용은 noreferrer noopener을 함께 사용하는 것과 같은 동작을 한다
- ref: jsx-eslint/eslint-plugin-react#2022
- ref: https://html.spec.whatwg.org/multipage/links.html#link-type-noreferrer
jstcki added a commit to republik/plattform that referenced this issue Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

5 participants