wp_targeted_link_rel filter should not be applied to 'Custom HTML` widget

[fierevere]

Since WordPress 5.2 is using "Widgets to blocks" there is a breakage for user supplied custom HTML code in widgets, notably used for stats and counters.

Example of code:

<!--LiveInternet counter--><script type="text/javascript">
document.write("<a href='//www.liveinternet.ru/click' "+
"target=_blank><img src='//counter.yadro.ru/hit?t52.6;r"+
escape(document.referrer)+((typeof(screen)=="undefined")?"":
";s"+screen.width+"*"+screen.height+"*"+(screen.colorDepth?
screen.colorDepth:screen.pixelDepth))+";u"+escape(document.URL)+
";h"+escape(document.title.substring(0,150))+";"+Math.random()+
"' alt='' title='LiveInternet: number of pageviews and visitors"+
" for 24 hours is shown' "+
"border='0' width='88' height='31'><\/a>")
</script><!--/LiveInternet-->

the supplied code works for WP prior to 5.2 when used in Custom HTML widget, but does not after upgrade to 5.2

Note from @otto42 (from Slack):

Okay, the actual problem with that live internet guy’s code is that the wp_targeted_link_rel filter function is screwing with his JS code, because it sees a targeted link and so it adds the rel noopener code to it. Unfortunately, it uses the wrong kind of quote marks

[TobiasBg]

It's not just JS that's broken by this, but also JSON, see #46316.

[audrasjb]

@fierevere thanks for the ticket, you're absolutely right.

In 47244.diff: Remove wp_targeted_link_rel function from custom HTML widget since it should be totally customizable HTML.

[fierevere]

Attachment 47244.diff​ added

works as a charm for example counter code.

[SergeyBiryukov]

Running wp_targeted_link_rel() on custom HTML widget appears to be an intentional change in [45143].

I'm fine with reverting it for 5.2.1, but for 5.3 it makes more sense to fix the function so that it performs the replacement without breaking the code.

[birgire]

If we go for 47244.diff​ in 5.2.1, then 47244-2.diff​ adjusts the existing and relevant unit test.

It would be great to fix this through wp_targeted_link_rel(), but I guess one could create some difficult Javascript examples that would be very hard to handle correctly with a regex in wp_targeted_link_rel().

So reverting seems like a good workaround for now.

[SergeyBiryukov]

For reference, the broken code using the example from ticket description:

<!--LiveInternet counter--><script type="text/javascript">
document.write("<a href='//www.liveinternet.ru/click' "+
"target=_blank rel="noopener noreferrer"><img src='//counter.yadro.ru/hit?t52.6;r"+
escape(document.referrer)+((typeof(screen)=="undefined")?"":
";s"+screen.width+"*"+screen.height+"*"+(screen.colorDepth?
screen.colorDepth:screen.pixelDepth))+";u"+escape(document.URL)+
";h"+escape(document.title.substring(0,150))+";"+Math.random()+
"' alt='' title='LiveInternet: number of pageviews and visitors"+
" for 24 hours is shown' "+
"border='0' width='88' height='31'><\/a>")
</script><!--/LiveInternet-->

Note the incorrect quotes in rel="noopener noreferrer".

[SergeyBiryukov]

47244.3.diff​ builds on @birgire's patch from #46316 and fixes both tickets. Might need a refresh if #46316 is committed first.

The patch ensures that rel attribute is added with the correct quotes:

• if target='_blank' has single quotes, then rel='noopener noreferrer' has single quotes too.
• If target=_blank does not have quotes (as in the example above), quotes of the href attribute are checked as a fallback.

[audrasjb]

Not sure we should arbitrary add rel attributes to Custom HTML Widget, though (since it's customized code by nature).

In any case, I tested your patch and it works well on my side @SergeyBiryukov. I think that would be nice to commit that so it can be proof tested during RC.

[SergeyBiryukov]

Replying to audrasjb:

Not sure we should arbitrary add rel attributes to Custom HTML Widget, though (since it's customized code by nature).

Unless I'm missing something, adding rel="noopener noreferrer" to any custom content for additional security was the intention of #43187 (for posts, comments, term descriptions, etc.) and #46421 (for HTML and Text widgets).

Thanks for testing the patch! :)

[birgire]

Thanks for the improved patch @SergeyBiryukov

Just for reference, here are two examples that are currently not caught:

String is split up like this:

document.write( '<p>Links: <a href="/" ' + "target=_blank>No rel</a></p>" );

and it will become:

document.write( '<p>Links: <a href="/" ' + "target=_blank rel="noopener noreferrer">No rel</a></p>" );

And this one (target and href attributes without quotes):

document.write( "<p>Links: <a href=/ target=_blank>No rel</a></p>" );

that will become:

document.write( "<p>Links: <a href=/ target=_blank rel="noopener noreferrer">No rel</a></p>" );

[fierevere]

Can we still have widget that does not maim the custom HTML code?

[SergeyBiryukov]

Replying to birgire:

Just for reference, here are two examples that are currently not caught

Right, thanks. I think these would be acceptable edge cases for now. We can reconsider them later if there's any evidence of widespread usage, like with the LiveInternet counter from the ticket description.

Replying to fierevere:

Can we still have widget that does not maim the custom HTML code?

That would require a new ticket to discuss full revert of #46421. Given that per #43187 this filter is also used for other custom HTML content (posts, comments, term descriptions, etc., as noted above), excluding the HTML widget would seem a bit inconsistent and arbitrary. Shouldn't this widget be treated like any other custom content?

[audrasjb]

Replying to SergeyBiryukov:

Replying to birgire:

Just for reference, here are two examples that are currently not caught

Right, thanks. I think these would be acceptable edge cases for now. We can reconsider them later if there's any evidence of widespread usage, like with the LiveInternet counter from the ticket description.

Right, these are very acceptable edge cases :)

Concerning full revert, Your argumentation make sense to me. By the way, I still think it'd be interesting to discuss that later in another ticket.

[SergeyBiryukov]

In 45348:

Formatting: Adjust wp_targeted_link_rel() to ensure JSON format is preserved and correct quotes are used when adding the missing rel attribute.

Props birgire, TobiasBg, fierevere, audrasjb, SergeyBiryukov.
Fixes #46316, #47244.

[SergeyBiryukov]

In 45349:

Formatting: Adjust wp_targeted_link_rel() to ensure JSON format is preserved and correct quotes are used when adding the missing rel attribute.

Props birgire, TobiasBg, fierevere, audrasjb, SergeyBiryukov.
Merges [45348] to the 5.2 branch.
Fixes #46316, #47244.