Bug fix for rel attributes in social-link.php

[niallhotfoot]

A security audit has picked up that social links linking to external tabs were at risk of tab nabbing.

Upon further investigation, I found that the rel attributes needed, should have been being added.

If you look in wp-includes/blocks/social-link.php on line 65, you need to wrap the attribute in a trim() so that if there are no additional rel attributes set, it won't start with a space as this won't work!

so the line should be:

        $processor->set_attribute( 'rel', trim(esc_attr( $rel ) . ' noopener nofollow' ));

<?php

[dmsnell]

Can you clarify here @niallhotfoot? I'm confused where you mention the leading whitespace. Is the problem that when there are no existing rel values, the new value is noopener nofollow and that fails in the browser?

It would be helpful to have a couple examples showing when this is right and when this is wrong and how the browser handles it. Based on the HTML specification I wouldn't expect extra spaces to cause any trouble, but I can definitely believe that they do; I'm not sure how to confirm and test it though.

Also noted, that esc_attr() doesn't belong there within the HTML API calls. Values are properly escaped by set_attribute(), but that's not related to this ticket.

[niallhotfoot]

That's right yeah. When I was testing the social link block, the noopener nofollow was not being added at all, which is why it got flagged in a securoty audit.

When testing, if i trimmed of the space so it was just passing 'noopener nofollow', it followed through to the view.

[dmsnell]

@niallhotfoot I performed some testing with the following file

<li><a href="noopener-child.html" target="tester">Open</a></li>
<li><a href="noopener-child.html" target="tester" rel="noopener">Open with noopener</a></li>
<li><a href="noopener-child.html" target="tester" rel=" noopener">Open with _noopener</a></li>

All three cases worked the way I expected, where the leading space didn't have an impact on the rel attribute. This matches the HTML specification, indicating that the value is a space-separated list of tokens.

the noopener nofollow was not being added at all

This might be a clue; I wonder if some other filter is running and looking for something naive, such as something looking for str_starts_with( $rel, "noopener" )

I searched Core's code and couldn't find anything, then tested wp_targeted_link_rel() and couldn't find a way to mess it up. Can you share the setup you have which led to the missing rel value? Did you have to perform any steps?

<?php
wp_targeted_link_rel( '<a href="https://page.com" target="testing">' );
// "<a href="https://page.com" target="testing" rel="noopener">"

wp_targeted_link_rel( '<a href="https://page.com" target="testing" rel="noopener">' );
// "<a href="https://page.com" target="testing" rel="noopener">"

wp_targeted_link_rel( '<a href="https://page.com" target="testing" rel=" noopener">' );
// "<a href="https://page.com" target="testing" rel="noopener">"

wp_targeted_link_rel( '<a href="https://page.com" target="testing" rel=" noopener nofollow">' );
// "<a href="https://page.com" target="testing" rel="noopener nofollow">"

wp_targeted_link_rel( '<a href="https://page.com" target="testing" rel=" ">' );
// "<a href="https://page.com" target="testing" rel="noopener">"

wp_targeted_link_rel( '<a href="https://page.com" target="testing" rel="">' );
// "<a href="https://page.com" target="testing" rel="noopener">"

[niallhotfoot]

Interesting! This is a site that I've inheritted so it is possible that something is hijacking it somewhere!

I've got a work around in place at the moment and will have a deeper dive into finding out why the rel attributes might be getting excluded on my end.

Thank you so much for taking the time to look into that, it's very much appreciated, and I apologise that I've incorrectly reported this as a bug.

[dmsnell]

@niallhotfoot to be clear I think your patch is great and there's no reason not to trim() the attribute, but I would like to get at the root of what's happening because we might have stumbled upon another problem that needs fixing, whereas this fix could hide that. let's update both 😄

[niallhotfoot]

I've just been doing some follow up testing for you and I'm afraid I'm none the wiser! I've scoured the code for any references or hooks into WP_HTML_Tag_processor itself but I've not found anything.

So I'll breifly go through the setup from the beginning!

I've got a site which has widget areas, in that widget are it is using the Social Icons block.

When rendered on the front end, it is going through wp-includes > blocks > social-link.php through the render_block_core_social_link function.

In this function, there is a section about opening in a new tab.

The line in question is this one:

$processor->set_attribute( 'rel', esc_attr( $rel ) . ' noopener nofollow' );

If I dump the $processor as it is, I cannot find the 'noopener nofollow' attributes. However, dumping $processor->getAttributes('rel') does get the attributes.

If I repeat the process with my workaround, so:

$processor->set_attribute( 'rel', trim(esc_attr( $rel ) . ' noopener nofollow' ));

And then dump the $processor, I am then able to find the 'noopener nofollow'

I'm really interested to see if it works fine for you using the Social Icons block in a widget!

[dmsnell]

if some other filter is running

pardon for my miscommunication; I know that the HTML Tag Processor isn't running this kind of filter over the content, but if some other code in some other WordPress plugin is running one, it could be intercepting links after the Tag Processor has rendered and then removing the rel attribute.

when you say "I dump the $processor are you running something like var_dump( $processor->get_updated_html() )`

to mkae this easier you can reach out to me on Slack and we can chat about it.

[niallhotfoot]

That's exactly what I'm doing yeah. Dumping straight after the line I'm referring to. This says to me that it's not a hook or something as it's at that point that it's diverging.

However, if you're unable to reproduce it then it's definitely something on this end!

I'm afraid I'm unable to get on slack at the moment.

[dmsnell]

Let's see if we can't hash this out in realtime when you have a chance. I'm unable to reproduce so I must be doing something wrong. There's nothing I can think of that should cause this behavior. Here is my test case:

<?php
// from inside a WordPress directory
require_once( __DIR__ . '/wp-load.php' );

$p = new WP_HTML_Tag_Processor( '<a href="https://wordpress.org">' );
$p->next_tag();

echo $p->get_updated_html();
// <a href="https://wordpress.org">

$p->set_attribute( 'rel', ' noopener nofollow' );
echo $p->get_updated_html();
// <a rel=" noopener nofollow" href="https://wp.com">

$p->set_attribute( 'rel', 'noopener nofollow' );
echo $p->get_updated_html();
// <a rel="noopener nofollow" href="https://wp.com">

[niallhotfoot]

Here you go man, I've broken it down into a quick copy and paste for you to see if it's happening on your end!

$rel = '';
$link = '<li><a href="" class=""></a></li>';
$processor = new WP_HTML_Tag_Processor( $link );
$processor->next_tag( 'a' );
$processor->set_attribute( 'rel', esc_attr( $rel ) . ' noopener nofollow' );
$processor->set_attribute( 'target', '_blank' );

var_dump($processor->get_updated_html());

I'm able to run that in a wordpress view file and get the same results, so it should be easier for you ot replicate with that!

My var_dump hasn't added in the rel attribute! And I bet your's will! Let me know how you get on!

[dmsnell]

I'm able to run that in a wordpress view file

@niallhotfoot I think this confirms that there's some other filtering going on. I pasted that code into my shell and got what I expected.

<li><a rel=" noopener nofollow" target="_blank" href="" class=""></a></li>

if you have any plugins installed can you try disabling those and see if you still lose the rel? I looked further in Core but I'm unable to find anything that matches ['"](?:noopener|nofollow) which I would expect, given the behavior you're reporting.

of note, having code run in a view function is different than running in isolation. there are going to be filters applied to the output of that view function, which is where I suspect that this rel is being removed.

[niallhotfoot]

That's great, thank you so much for your help!

I've simply hard coded the links on the site as editting wordpress core files isn't practical or reliable in the slightest!

Thanks so much for your help trying to get to the bottom of this!

[dmsnell]

I've simply hard coded the links on the site as editting wordpress core files isn't practical or reliable in the slightest!

I don't want you to edit any Core files; I only wanted you to try and disable any plugins and try your experiment again to see if any third-party plugins or theme code is modifying the output of that view function. if it is, we can ask the plugin author to update their code so it stops breaking peoples' sites.

[niallhotfoot]

I bloody found it!

You were indeed right about it being a plugin, but not as we thought!

I've just been through and disbled each plugin and checked the site, and there it was "Real-Time Find and Replace".

So I went in and checked what was setup in there, as you guessed it, the rel attribute! The reason my trim worked was because it was the whole rel attribute that was in the search and replace which included the space!

Ah the joys of inheritting sites!

Thank you so much again for taking the time to help me out diagnosing this. I should have looked into the plugins first!

[dmsnell]

no worries @niallhotfoot - we'll get the trim() in, because it's fine. in the meantime, hope you can fix it on your end and it will stop losing that attribute. thank you for your patience with this and for helping us verify that it isn't something in WordPress that's broken.

[dmsnell]

applying trim() in ​Gutenberg#55705 and this should be merged in during the next Core update that pulls in Gutenberg packages (after it merges).

I'm going to close this ticket since it's being taken care of in Gutenberg and since Core is not breaking the reported noopener directly.