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.

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