Make WordPress Core

Opened 6 months ago

Closed 5 weeks ago

Last modified 5 weeks ago

#53839 closed enhancement (fixed)

Add rel="noopener" to output of wp_list_bookmarks() when target is set to "_blank"

Reported by: tw2113 Owned by: hellofromTonya
Milestone: 5.9 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch has-unit-tests commit
Focuses: Cc:

Description

To help aid in hardening WordPress and the web as a whole, it became a best practice to add noopener to the rel attribute for links that have a target of _blank.

I have provided the following patch to add this noopener value to the rel attributes on the output of wp_list_bookmarks() instances, when a link has a _blank target value.

Attachments (6)

53839-noopener-on-blogroll-links.diff (658 bytes) - added by tw2113 6 months ago.
53839-noopener-on-blogroll-links.2.diff (726 bytes) - added by tw2113 6 months ago.
53839.patch (960 bytes) - added by mukesh27 6 months ago.
Before_After_Patch_53839.patch.jpg (64.5 KB) - added by costdev 3 months ago.
Before / After 53839.patch
After-53839.patch---additional-tests.jpg (142.6 KB) - added by costdev 3 months ago.
Patch 53839 - Additional manual tests. Successfully adds noopener to the rel attribute when Target is not set to _none.
53839-2.diff (1.6 KB) - added by david.binda 6 weeks ago.

Download all attachments as: .zip

Change History (34)

#1 @SergeyBiryukov
6 months ago

  • Milestone changed from Awaiting Review to 5.9

#2 @birgire
6 months ago

Sounds good.

Isnt it possible to have duplication of noopener with patch?

I just wonder about the edge case where rel is already noopener.

#3 follow-up: @tw2113
6 months ago

@birgire Definitely willing and able to add in an array_unique() or in_array() check.

Since you mentioned it, I think it is possible to key in your own rel values.

Correction, the field is readonly. So the only way someone would get to the rel tag would be through the wp_list_bookmarks filter which is after constructing the entire list.

Last edited 6 months ago by tw2113 (previous) (diff)

#4 in reply to: ↑ 3 @birgire
6 months ago

Replying to tw2113:

Thanks for checking, prev. I just had a quick look at the link insert function that allows to insert rel data

https://developer.wordpress.org/reference/functions/wp_insert_link/

since Im travelling with a mobile unable to dig deep ☺

#5 @tw2113
6 months ago

Will check that out and see what can be done. Thanks.

#6 @tw2113
6 months ago

I'm doubtful we're going to run in to too many issues with wp_insert_link() but that said, I have amended the patch to do an in_array() check first before inserting.

#7 @SergeyBiryukov
6 months ago

Related: #37941, #53843.

Last edited 6 months ago by SergeyBiryukov (previous) (diff)

@mukesh27
6 months ago

#8 @mukesh27
6 months ago

Patch 53839.patch remove bottom additional target checking.

#9 @SergeyBiryukov
6 months ago

Thanks for the patches!

I think 53839.patch would be preferable here, as it adds noopener to any link with a target attribute, rather than _blank specifically, which seems more consistent with wp_targeted_link_rel().

Some unit tests for this would be great. They could draw inspiration from the wp_targeted_link_rel() tests. They could be for either _walk_bookmarks() or wp_list_bookmarks() function, and be placed under tests/bookmarks/.

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


3 months ago

#11 @costdev
3 months ago

Test Report

Env

  • WordPress 5.9-alpha-20211102.234109
  • Chrome 95.0.4638.69
  • Windows 10
  • Theme: Twenty Twenty One
  • Gutenberg Editor
  • Plugins: None


Steps to test

  1. Enable the Link manager by adding this to your theme's functions.php file:
// Enable link manager.
add_filter( 'pre_option_link_manager_enabled', '__return_true' );

// Display bookmarks on frontend.
if ( ! is_admin() ) {
        wp_list_bookmarks();
}
  1. Refresh the Dashboard.
  2. Navigate to Links > Add New.
  3. Create a link, assign it to a category (create a new category if none exist) and publish.
  4. Under Target, select _blank.
  5. On the frontend, open up DevTools (or right-click on the bookmark and click "Inspect").
  6. You should see that the link should contain rel="noopener" and look similar to this:
    <a href="http://wordpress.org" rel="noopener" target="_blank">With _blank</a>
    

Results

  • Patch: 53839.patch
  • Before patch: The bookmark with target="_blank" did not have rel="noopener" in its attributes.
  • After patch: The bookmark with target="_blank" did have rel="noopener" in its attributes.
  • Result: Patch works as expected. No regressions identified.
Last edited 3 months ago by costdev (previous) (diff)

@costdev
3 months ago

Before / After 53839.patch

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


3 months ago

@costdev
3 months ago

Patch 53839 - Additional manual tests. Successfully adds noopener to the rel attribute when Target is not set to _none.

#13 @hellofromTonya
3 months ago

  • Keywords needs-unit-tests added

Thank you @costdev for manually testing it. Adding a note that you're also currently working on the unit tests for it too. Thanks!

This ticket was mentioned in PR #1834 on WordPress/wordpress-develop by costdev.


3 months ago

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

To help aid in hardening WordPress and the web as a whole, it became a best practice to add "noopener" to the rel attribute for links that have a target.

This updates wp_list_bookmarks() so that it now adds "noopener" to the rel attribute when the link's target is set.

Trac ticket: https://core.trac.wordpress.org/ticket/53839

#15 @hellofromTonya
3 months ago

  • Owner set to hellofromTonya
  • Status changed from new to reviewing

The PR 1834 is approved. Tests are done. The patch has been optimized to use str_contains() instead of converting to/from array. It's ready for commit.

Ticket #53843 seeks to remove 'noopener'. Should this PR/patch be committed knowing that that ticket will potentially remove it?

#16 @tw2113
3 months ago

I'm fine if it either gets removed later or this ticket doesn't get committed in the end. It looks like @azaozz opened his that afternoon, meanwhile I opened this one that morning. Both on the 30th of July.

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


3 months ago

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


3 months ago

#19 @hellofromTonya
3 months ago

  • Keywords commit added

After discussing during today's 5.9 feature scrub, consensus is to commit this patch. #53843 is opened to explore which browsers support it and how to deal with it.

#20 @hellofromTonya
3 months ago

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

In 52061:

General: Add "noopener" to wp_list_bookmarks() output.

In the bookmarks walker _walk_bookmarks(), add a 'noopener' to the bookmark's rel attribute when there's target attribute.

Adds a new test class for wp_list_bookmarks() and tests for this change.

Follow-up to [3880], [10712].

Props birgire, costdev, hellofromTonya, mukesh27 , sergeybiryukov, tw2113.
Fixes #53839.

#22 @hellofromTonya
3 months ago

Thank you everyone for your contributions! The patch has been committed and will ship in 5.9.

#23 @david.binda
6 weeks ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Sorry for re-opening the ticket, but I have found out that the PHPUnit tests accompanying the functional changeset are not testing all the details of the change, as there is a typo in the argument passed to the WP_UnitTestCase::factory()->bookmark->create method.

The code in wp_list_bookmarks works with the link_rel attribute (see https://core.trac.wordpress.org/browser/trunk/src/wp-includes/bookmark-template.php?rev=52061#L104), instead of the rel defined in the data providers (eg.: https://core.trac.wordpress.org/browser/trunk/tests/phpunit/tests/bookmark/wpListBookmarks.php?rev=52061#L44 )

Therefore, the conditional in https://core.trac.wordpress.org/browser/trunk/src/wp-includes/bookmark-template.php?rev=52061#L108 is never matched by the PHPUnit tests, and none of the tests are actually passing in any previously existing rel attribute value; the noopener, if expected, is the only value in rel="".

That said, I'm attaching a patch which fixes the typo in existing data providers, and adds one more test, which tests whether the noopener is actually added to the pre-existing value.

@david.binda
6 weeks ago

This ticket was mentioned in PR #2058 on WordPress/wordpress-develop by hellofromtonya.


6 weeks ago

Combines patch https://core.trac.wordpress.org/attachment/ticket/53839/53839-2.diff with existing tests by:

  • Changing rel to link_rel in args
  • Adding $expected string in the test_wp_list_bookmarks_adds_noopener() test (which replaces the new test_wp_list_bookmarks_adds_noopener_keep_original() test in the 53839-2.diff patch

Props @david.binda.

Trac ticket: https://core.trac.wordpress.org/ticket/53839

#25 @hellofromTonya
6 weeks ago

Hey @davidbinda, Thank you for finding the issue in the new test case and contributing the 53839-2.diff patch. The new test method added is a duplicate of the test in the data provider. I combined that new test with test_wp_list_bookmarks_adds_noopener() test by adding the expected rel="" string to each dataset.

Marking for commit.

#26 @hellofromTonya
5 weeks ago

Prepping commit.

#27 @hellofromTonya
5 weeks ago

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

In 52396:

General: Fix 'rel' argument in Tests_Functions_wpListBookmarks test datasets.

wp_list_bookmarks() arguments include 'link_rel', but not 'rel'. This commit fixes this argument in the test datasets. It also adds an expected 'rel=""' result check, i.e. instead of only checking for 'noopener'.

Follow-up to [52395].

Props davidbinda.
Fixes #53839.

Note: See TracTickets for help on using tickets.