Make WordPress Core

Opened 4 years ago

Closed 5 months ago

Last modified 5 months ago

#53843 closed enhancement (fixed)

Remove adding of rel="noopener" to links with target="_blank"

Reported by: azaozz's profile azaozz Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 6.7 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch has-unit-tests changes-requested
Focuses: Cc:

Description

#43187 introduced adding of rel="noopener noreferrer" to links with target="_blank" as a security precaution. Later, in #49558 noreferrer was removed as no longer needed.

Since then most browsers were updated to imply noopener on links with target="_blank" making #43187 and similar changes not relevant any more. See:
https://html.spec.whatwg.org/multipage/links.html#following-hyperlinks (specs),
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a#browser_compatibility.

Adding this ticket now as a reminder to remove all noopener when all newer browsers implement the specs.

Attachments (4)

53843.diff (10.8 KB) - added by dhruval04 14 months ago.
noopener.png (32.9 KB) - added by azaozz 13 months ago.
rel=noopener - HTML_ HyperText Markup Language _ MDN.png (102.4 KB) - added by dhruval04 13 months ago.
noopner.png (51.9 KB) - added by dhruval04 9 months ago.

Download all attachments as: .zip

Change History (35)

#1 @sabernhardt
3 years ago

In case anyone wonders about the comment author links on the Comments admin screen (r52007), those would keep the rel attribute because they do not open in a new tab automatically.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


3 years ago

This ticket was mentioned in Slack in #core by presskopp. View the logs.


16 months ago

#6 @TobiasBg
14 months ago

  • Keywords needs-patch added
  • Milestone changed from Future Release to 6.5

The time has come, I think.

Quotes from https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a#browser_compatibility :

Note: Setting target="_blank" on <a> elements implicitly provides the same rel behavior as setting rel="noopener" which does not set window.opener.

[I]n newer browser versions setting target="_blank" implicitly provides the same protection as setting rel="noopener".

All major browsers have shown this behavior for more than 2-3 years now, meaning that all browser that are still supported by WordPress support this (unless I have overlooked something).

We should consider deprecating wp_targeted_link_rel() and related functions and stop adding it as a filter hook callback to a bunch of hooks.

noopener in static <a> tags in wp-admin can be removed, too. I don't think it's worth the risk to remove it from post content though.

[Brought to new attention by https://twitter.com/wesbos/status/1734619070127915021 .]

Last edited 14 months ago by TobiasBg (previous) (diff)

@dhruval04
14 months ago

#7 @dhruval04
14 months ago

  • Keywords has-patch added; needs-patch removed

@TobiasBg
https://core.trac.wordpress.org/ticket/53843
Removed "rel="noopener" tag from all files.

#8 follow-up: @audrasjb
13 months ago

Hi there,
I'd say 53843.diff is a good first step before eventually deprecating wp_targeted_link_rel() and related functions like wp_targeted_link_rel_callback().
I think this is ready to ship for 6.5, and we can target 6.6 to deprecate the functions in a follow-up ticket. What do you think @azaozz?

#9 in reply to: ↑ 8 @azaozz
13 months ago

  • Keywords 2nd-opinion added

Replying to audrasjb:

I think this is ready to ship for 6.5, and we can target 6.6 to deprecate the functions...

Yea, seems that rel="noopener" may not be needed any more. Thinking it would be good to confirm this 100%. As far as I see the current compatibility chart on MDN shows the Android WebView as not supporting this yet (this is the browser used by default on phones and also embedded in all kinds of devices like "smart" TVs, etc.).

Looking at the current patch it only removes the few rel="noopener" that are hard-coded in wp-admin. If that's not needed any more thinking we should stop adding it to the user content and deprecate the functions, see wp_targeted_link_rel(), wp_targeted_link_rel_callback(), wp_init_targeted_link_rel_filters(), wp_remove_targeted_link_rel_filters() starting here: https://github.com/WordPress/wordpress-develop/blob/6.4/src/wp-includes/formatting.php#L3304.

Last edited 13 months ago by azaozz (previous) (diff)

@azaozz
13 months ago

#10 @TobiasBg
13 months ago

Hhm, that's odd. I just found https://chromestatus.com/feature/6140064063029248 which seems to imply that this was also shipped in Webview in version 88, just like other Chromium-based browsers...

#11 @dhruval04
13 months ago

@azaozz @TobiasBg
I also found same on rel="noopener" here as well.https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/rel/noopener which support all browser including Webview android.

#12 follow-up: @TobiasBg
13 months ago

@dhruval04: Yes, rel="noopener" is supported by all modern browsers, but in this ticket we actually want to remove it :-) So the question is whether the note in the blue box is true for all modern browsers. There is some uncertainty regarding Android Webview here.

#13 in reply to: ↑ 12 @azaozz
13 months ago

Replying to TobiasBg:

Yep, the question here is whether Android Webview implies noopener when the target="_blank" attribute is present. Would be great if somebody can try to get more info on that, and which version(s) of Webview are affected.

Last edited 13 months ago by azaozz (previous) (diff)

#14 follow-up: @swissspidy
12 months ago

Yep, the question here is whether Android Webview implies noopener when the target="_blank" attribute is present. Would be great if somebody can try to get more info on that, and which version(s) of Webview are affected.

Well, caniuse.com still says no. And AFAIK Android WebView ≠ Chrome on Android, so https://chromestatus.com/feature/6140064063029248 does not affect Android WebView.

Question is: do we specifically support this "browser"?

#15 in reply to: ↑ 14 @TobiasBg
12 months ago

Replying to swissspidy:

so https://chromestatus.com/feature/6140064063029248 does not affect Android WebView.

But Webview is specifically mentioned, with "Shipping: 88"?

Can we somehow check the source code on this?

#16 @swissspidy
12 months ago

But Webview is specifically mentioned, with "Shipping: 88"?

As I said, AFAIK the Android WebView (the built-in system widget) is different from "Chrome for Android, in a WebView".

The Chrome commit changed /chrome/browser in the Chromium repo, but Android WebView is maintained in /android_webview and was not modified.

#17 @sabernhardt
12 months ago

  • Milestone changed from 6.5 to Future Release

#18 @dhruval04
9 months ago

Hi @azaozz @audrasjb @swissspidy

As i see the current compatibility chart on MDN https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a#browser_compatibility shows that Android WebView is now supporting noopener when the target="_blank" attribute is present.

@dhruval04
9 months ago

#19 @TobiasBg
9 months ago

That MDN change was done in this pull request.

The decision was to trust https://chromestatus.com/feature/6140064063029248 which was also mentioned above.

The Chromium ticket seems to be https://issues.chromium.org/issues/40599708 with the commit at https://chromium.googlesource.com/chromium/src.git/+/a8a460cbc25a2474b48b844756503d749c584738 .

The question still is what "Webview" really is, as in @swissspidy's comment.

#20 @swissspidy
9 months ago

  • Keywords 2nd-opinion removed

I just asked about this internally and from what I understood it should work in Android WebView just as well.

So I think we're good to proceed here.

#21 @TobiasBg
9 months ago

  • Keywords needs-refresh needs-patch added; has-patch removed
  • Milestone changed from Future Release to 6.7

Thanks for checking on this, @swissspidy!

As the beta phase for the 6.6 release cycle has already started, this enhancement would probably need to go into 6.7, unless we want to get in the removal of noopener from static <a> tags now already. (53843.diff might need a refresh and double-check though, given that it's five months old.)

In 6.7, we could deprecate wp_targeted_link_rel() and related functions and stop adding it as a filter hook callback to a bunch of hooks. This would still need a patch.

I'm setting milestone and keywords accordingly.

This ticket was mentioned in PR #6814 on WordPress/wordpress-develop by @DorZki.


8 months ago
#22

  • Keywords has-patch has-unit-tests added; needs-refresh needs-patch removed

#23 @DorZki
8 months ago

I'm on it :)

This ticket was mentioned in Slack in #core by dorzki. View the logs.


8 months ago

#25 @neo2k23
8 months ago

Sorry to hop into this as mentioned in here https://core.trac.wordpress.org/ticket/61482 I think the code needs patching anyway as it does not only work on target="_blank"

If i add

<a target="_blank" href="https://myforum.com">Go to the Forum</a>
<a target="_self" href="https://myforum.com">Go to the Forum</a>
<a target="_parent" href="https://myforum.com">Go to the Forum</a>
<a target="_top" href="https://myforum.com">Go to the Forum</a>

they all get the rel="noopener" added to the a-tag

<a target="_blank" href="https://myforum.com" rel="noopener">Go to the Forum</a>
<a target="_self" href="https://myforum.com" rel="noopener">Go to the Forum</a>
<a target="_parent" href="https://myforum.com" rel="noopener">Go to the Forum</a>
<a target="_top" href="https://myforum.com" rel="noopener">Go to the Forum</a>

#26 @peterwilsoncc
6 months ago

  • Keywords changes-requested added

The linked pull request is very close, I've left a couple of notes to relocation a retain functions as no-op calls. @DorZki I'm happy to push to your branch if you don't have bandwidth to work on the patch.

#27 @DorZki
6 months ago

Hi @peterwilsoncc,
Sorry for the late reply, been busy :)

I will go over the comments and fix whatever needs to be fixed :)

This ticket was mentioned in Slack in #core by stoyangeorgiev. View the logs.


5 months ago

#29 @peterwilsoncc
5 months ago

  • Owner set to peterwilsoncc
  • Resolution set to fixed
  • Status changed from new to closed

In 59120:

General: Remove noopener from links opening in a new tab.

Removes the automatic addition of rel="noopener noreferrer" from links targeting a new tab or window, target='_blank'. Since this was introduced, supported browsers have changed their security policies and no longer allow the opened link to have JavaScript access to the previous tab.

Deprecates:

  • wp_targeted_link_rel()
  • wp_targeted_link_rel_callback()
  • wp_init_targeted_link_rel_filters(): converted to a noop function
  • wp_remove_targeted_link_rel_filters(): converted to a noop function

The deprecated functions are retained in formatting.php as in SHORTINIT mode the file is included while deprecated.php is not.

This also removes the noopener from links hard coded within the WordPress dashboard linking to documentation and other resources.

Props audrasjb, azaozz, dhruval04, dorzki, neo2k23, presskopp, sabernhardt, swissspidy, tobiasbg.
Fixes #53843.

#31 @peterwilsoncc
5 months ago

In 59121:

General: Delete tests/phpunit/tests/formatting/wpTargetedLinkRel.php.

Really delete tests/phpunit/tests/formatting/wpTargetedLinkRel.php this time rather than leaving it hanging around as an empty file.

Follow up to [59120].

Props noisysocks.
See #53843

Note: See TracTickets for help on using tickets.