WordPress.org

Make WordPress Core

Opened 6 weeks ago

Closed 5 weeks ago

Last modified 5 weeks ago

#47244 closed defect (bug) (fixed)

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

Reported by: fierevere Owned by: audrasjb
Milestone: 5.2.1 Priority: high
Severity: major Version: 5.2
Component: Widgets Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

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

Attachments (3)

47244.diff (780 bytes) - added by audrasjb 6 weeks ago.
47244-2.diff (1.6 KB) - added by birgire 6 weeks ago.
47244.3.diff (2.2 KB) - added by SergeyBiryukov 6 weeks ago.

Download all attachments as: .zip

Change History (22)

#1 @SergeyBiryukov
6 weeks ago

  • Milestone changed from Awaiting Review to 5.2.1
  • Priority changed from normal to high
  • Severity changed from normal to major

#2 @audrasjb
6 weeks ago

  • Keywords needs-patch added
  • Owner set to audrasjb
  • Status changed from new to assigned

#3 @TobiasBg
6 weeks ago

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

@audrasjb
6 weeks ago

#4 @audrasjb
6 weeks ago

  • Keywords has-patch added; needs-patch removed

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

#5 @fierevere
6 weeks ago

Attachment 47244.diff​ added

works as a charm for example counter code.

#6 @SergeyBiryukov
6 weeks ago

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.

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

#7 @SergeyBiryukov
6 weeks ago

  • Keywords needs-unit-tests added

@birgire
6 weeks ago

#8 @birgire
6 weeks ago

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

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.

#9 @SergeyBiryukov
6 weeks ago

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

#10 @SergeyBiryukov
6 weeks ago

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.
Last edited 6 weeks ago by SergeyBiryukov (previous) (diff)

#11 follow-up: @audrasjb
6 weeks ago

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.

#12 in reply to: ↑ 11 @SergeyBiryukov
6 weeks ago

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! :)

#13 follow-up: @birgire
6 weeks ago

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>" );

#14 @fierevere
6 weeks ago

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

This ticket was mentioned in Slack in #forums by yui. View the logs.


6 weeks ago

#16 in reply to: ↑ 13 ; follow-up: @SergeyBiryukov
6 weeks ago

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?

#17 in reply to: ↑ 16 @audrasjb
5 weeks ago

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.

#18 @SergeyBiryukov
5 weeks ago

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

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.

#19 @SergeyBiryukov
5 weeks ago

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.

Note: See TracTickets for help on using tickets.