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

[tw2113]

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.

[birgire]

Sounds good.

Isnt it possible to have duplication of noopener with patch?

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

[tw2113]

@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.

[birgire]

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 ☺

[tw2113]

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

[tw2113]

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.

[SergeyBiryukov]

Related: #37941, #53843.

[mukesh27]

Patch 53839.patch​ remove bottom additional target checking.

[SergeyBiryukov]

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/.

[costdev]

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();
}

2. Refresh the Dashboard.
3. Navigate to Links > Add New.
4. Create a link, assign it to a category (create a new category if none exist) and publish.
5. Under Target, select _blank.
6. On the frontend, open up DevTools (or right-click on the bookmark and click "Inspect").
7. 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.

[costdev]

Before / After 53839.patch

[costdev]

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

[hellofromTonya]

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

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

[hellofromTonya]

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?

[tw2113]

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.

[hellofromTonya]

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.

[hellofromTonya]

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.

Committed via changeset https://core.trac.wordpress.org/changeset/52061.

[hellofromTonya]

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

[david.binda]

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.

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

[hellofromTonya]

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.

[hellofromTonya]

Prepping commit.

[hellofromTonya]

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.

Committed via https://core.trac.wordpress.org/changeset/52396.