#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)
Change History (22)
#1
@
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
@
5 years ago
- Keywords needs-patch added
- Owner set to audrasjb
- Status changed from new to assigned
#4
@
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.
#6
@
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.
#8
@
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
@
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
@
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, thenrel='noopener noreferrer'
has single quotes too. - If
target=_blank
does not have quotes (as in the example above), quotes of therel
attribute are checked as a fallback.
#11
follow-up:
↓ 12
@
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
@
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:
↓ 16
@
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>" );
This ticket was mentioned in Slack in #forums by yui. View the logs.
5 years ago
#16
in reply to:
↑ 13
;
follow-up:
↓ 17
@
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
@
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.
It's not just JS that's broken by this, but also JSON, see #46316.