WordPress.org

Make WordPress Core

Opened 14 months ago

Closed 5 months ago

#49558 closed task (blessed) (fixed)

Remove noreferrer from wp_targeted_link_rel and other uses

Reported by: joostdevalk Owned by: 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 8 months ago.
General: Remove noreferrer value from wp_targeted_link_rel and other uses
49558.2.diff (31.4 KB) - added by Mista-Flo 6 months ago.
Refresh patch against trunk
49558.3.diff (32.2 KB) - added by Mista-Flo 6 months ago.
Fix unit test

Download all attachments as: .zip

Change History (22)

#1 @SergeyBiryukov
14 months ago

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

#3 @peterwilsoncc
14 months 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
9 months 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
9 months 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
8 months ago

General: Remove noreferrer value from wp_targeted_link_rel and other uses

#6 @audrasjb
8 months 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.


6 months ago

#8 @Mista-Flo
6 months ago

  • Keywords needs-refresh added

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

@Mista-Flo
6 months ago

Refresh patch against trunk

@Mista-Flo
6 months ago

Fix unit test

#9 @Mista-Flo
6 months 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
6 months ago

  • Keywords commit added

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

#11 @SergeyBiryukov
6 months 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
6 months 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
6 months 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
6 months 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
6 months 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
5 months 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
5 months ago

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

#18 in reply to: ↑ 16 @Mista-Flo
5 months 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
5 months 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.