Make WordPress Core

Opened 5 years ago

Closed 4 years ago

#49558 closed task (blessed) (fixed)

Remove noreferrer from wp_targeted_link_rel and other uses

Reported by: joostdevalk's profile joostdevalk Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.6 Priority: normal
Severity: normal Version:
Component: General Keywords: has-unit-tests has-patch commit
Focuses: Cc:

Description (last modified by SergeyBiryukov)

When we added noopener noreferrer in #37941, the noreferrer bit was added specifically because at the time, Firefox didn't support noopener. Since it does now and has for a while, see here, I think we should remove it, as it does have nasty side effects: it breaks cross-site analytics.

We should remove it everywhere, as links in the admin don't send a referrer anyway after [41741] and as such there's no security risk to removing it.

Attachments (3)

49558.diff (31.4 KB) - added by audrasjb 4 years ago.
General: Remove noreferrer value from wp_targeted_link_rel and other uses
49558.2.diff (31.4 KB) - added by Mista-Flo 4 years ago.
Refresh patch against trunk
49558.3.diff (32.2 KB) - added by Mista-Flo 4 years ago.
Fix unit test

Download all attachments as: .zip

Change History (22)

#1 @SergeyBiryukov
5 years ago

  • Milestone changed from Awaiting Review to 5.5
  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

#2 @jonoaldersonwp
5 years ago

Seconded.

#3 @peterwilsoncc
5 years ago

  • Keywords needs-patch needs-unit-tests late added

Yes, I very much think this of benefit to the WP and its users.

However, the WP browser support policy still includes Edge 18 as it has over 1% usage at the time of writing.

Let's keep an eye on the Can I Use browser usage stats and gleefully commit this once the EdgeHTML Edge browser use drops below the WP support levels.

My hunch is that it will still make the 5.5 milestone, so I am keeping milestone unchanged.

#4 follow-up: @SergeyBiryukov
5 years ago

  • Description modified (diff)
  • Milestone changed from 5.5 to 5.6

It looks like Edge 18 still has 1.49% usage, so this cannot be addressed in time for 5.5, moving to 5.6.

#5 in reply to: ↑ 4 @elgameel
5 years ago

Replying to SergeyBiryukov:

It looks like Edge 18 still has 1.49% usage, so this cannot be addressed in time for 5.5, moving to 5.6.

I've been waiting for removing the tag and I think it should be fixed asap, not delaying it!

It is causing problems with affiliate tracking and making WordPress users lose earnings.

And in order to remove it, we need to switch each block with a link to HTML view and manually remove the tag.

@audrasjb
4 years ago

General: Remove noreferrer value from wp_targeted_link_rel and other uses

#6 @audrasjb
4 years ago

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

Given Edge is less than 1% market share now (0.77% - September 1, 2020), let's get rid of this attribute value.

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


4 years ago

#8 @Mista-Flo
4 years ago

  • Keywords needs-refresh added

Failed to apply patch on trunk, one unit test file affected.

@Mista-Flo
4 years ago

Refresh patch against trunk

@Mista-Flo
4 years ago

Fix unit test

#9 @Mista-Flo
4 years ago

  • Keywords needs-refresh removed

Well done. The patch seems pretty solid, I have tested it and there were some failing unit tests that I fixed in the last patch

#10 @audrasjb
4 years ago

  • Keywords commit added

Thanks @Mista-Flo !
Patch looks good to me as well. Marking for commit.

#11 @SergeyBiryukov
4 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 49215:

General: Remove noreferrer from wp_targeted_link_rel() and other uses.

When noopener noreferrer was originally added in #37941 and related tickets, the noreferrer bit was specifically included due to Firefox not supporting noopener at the time.

Since noopener has been supported by all major browsers for a while, it should now be safe to remove the noreferrer attribute from core.

Props Mista-Flo, audrasjb, joostdevalk, jonoaldersonwp, peterwilsoncc, elgameel.
Fixes #49558.

#12 follow-up: @antpb
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I'm suddenly getting failures running npm run test:php on my local that is up to date with trunk. They seem similar to the changes from this ticket. @SergeyBiryukov is this patch possibly causing it? I've shared a bit of the output below.

There were 22 failures:
1) Tests_Targeted_Link_Rel::test_add_to_links_with_target_blank
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'<p>Links: <a href="/" target="_blank" rel="noopener">No rel</a></p>'
+'<p>Links: <a href="/" target="_blank" rel="noopener noreferrer">No rel</a></p>'
/var/www/tests/phpunit/tests/formatting/WPTargetedLinkRel.php:12
2) Tests_Targeted_Link_Rel::test_add_to_links_with_target_foo
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'<p>Links: <a href="/" target="foo" rel="noopener">No rel</a></p>'
+'<p>Links: <a href="/" target="foo" rel="noopener noreferrer">No rel</a></p>'
/var/www/tests/phpunit/tests/formatting/WPTargetedLinkRel.php:18
3) Tests_Targeted_Link_Rel::test_target_as_first_attribute
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'<p>Links: <a target="_blank" href="#" rel="noopener">No rel</a></p>'
+'<p>Links: <a target="_blank" href="#" rel="noopener noreferrer">No rel</a></p>'
/var/www/tests/phpunit/tests/formatting/WPTargetedLinkRel.php:24
4) Tests_Targeted_Link_Rel::test_add_to_existing_rel
Failed asserting that two strings are identical.
--- Expected
+++ Actual

#13 @Mista-Flo
4 years ago

Weird @antpb I just set my git up to date with trunk and unit tests from this file are all passing. Do you have a special configuration?

#14 in reply to: ↑ 12 ; follow-up: @SergeyBiryukov
4 years ago

  • Type changed from enhancement to task (blessed)

Replying to antpb:

I'm suddenly getting failures running npm run test:php on my local that is up to date with trunk. They seem similar to the changes from this ticket. @SergeyBiryukov is this patch possibly causing it?

The tests are passing on Travis, could you run npm run build just to make sure the build directory is updated?

#15 in reply to: ↑ 14 @antpb
4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

Replying to SergeyBiryukov:

Replying to antpb:

I'm suddenly getting failures running npm run test:php on my local that is up to date with trunk. They seem similar to the changes from this ticket. @SergeyBiryukov is this patch possibly causing it?

The tests are passing on Travis, could you run npm run build just to make sure the build directory is updated?

Thanks @SergeyBiryukov. My build was stale when I noticed this. And this is why it's important to stop when you're tired. 😆

#16 follow-ups: @wpwebdevj
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I have tested version 5.6 and the block editor is still adding noreferrer to newly created links with target. Now that noreferrer has been removed from wp_targeted_link_rel, doesn't it make sense to also remove it from the block editor?

#17 @joostdevalk
4 years ago

@wpwebdevj yes. @SergeyBiryukov would this require opening a new ticket on the Gutenberg GitHub?

#18 in reply to: ↑ 16 @Mista-Flo
4 years ago

Replying to wpwebdevj:

I have tested version 5.6 and the block editor is still adding noreferrer to newly created links with target. Now that noreferrer has been removed from wp_targeted_link_rel, doesn't it make sense to also remove it from the block editor?

Hi, yes it definitely make sense I think, but the block editor project is handled by the Gutenberg team in its own Github repo, not in trac, so you need to open an issue on it, and link to this trac ticket for reference.

#19 in reply to: ↑ 16 @SergeyBiryukov
4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

Replying to wpwebdevj:

I have tested version 5.6 and the block editor is still adding noreferrer to newly created links with target. Now that noreferrer has been removed from wp_targeted_link_rel, doesn't it make sense to also remove it from the block editor?

Thanks for catching that!

Looks like there is already a Gutenberg issue for this: https://github.com/WordPress/gutenberg/issues/26914, so I'm closing the ticket as fixed for the core part.

Also related: #47202.

Note: See TracTickets for help on using tickets.