Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#47244 closed defect (bug) (fixed)

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

Reported by: fierevere's profile fierevere Owned by: audrasjb's profile 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 5 years ago.
47244-2.diff (1.6 KB) - added by birgire 5 years ago.
47244.3.diff (2.2 KB) - added by SergeyBiryukov 5 years ago.

Download all attachments as: .zip

Change History (22)

#1 @SergeyBiryukov
5 years ago

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

#2 @audrasjb
5 years ago

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

#3 @TobiasBg
5 years ago

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

@audrasjb
5 years ago

#4 @audrasjb
5 years 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
5 years ago

Attachment 47244.diff​ added

works as a charm for example counter code.

#6 @SergeyBiryukov
5 years 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 5 years ago by SergeyBiryukov (previous) (diff)

#7 @SergeyBiryukov
5 years ago

  • Keywords needs-unit-tests added

@birgire
5 years ago

#8 @birgire
5 years 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
5 years 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
5 years 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 rel attribute are checked as a fallback.
Version 0, edited 5 years ago by SergeyBiryukov (next)

#11 follow-up: @audrasjb
5 years 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
5 years 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
5 years 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
5 years 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.


5 years ago

#16 in reply to: ↑ 13 ; follow-up: @SergeyBiryukov
5 years 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 years 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 years 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 years 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.