-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Comments: Fix content stripping between <
and >
when used for comparison
#9468
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments: Fix content stripping between <
and >
when used for comparison
#9468
Conversation
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN:
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
src/wp-includes/comment-template.php
Outdated
|
||
$comment_text = get_comment_text( $comment, $args ); | ||
|
||
if ( is_admin() ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be more appropriate to just check if the current user can view unfiltered html? There are some admins that might not be able to view unfiltered html, in which case this might double encode characters?
if ( is_admin() ) { | |
if ( current_user_can( 'unfiltered_html' ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's a good catch. I'll update it. Thank you!
// Encode < and > in a numeric comparisons, | ||
// to prevent them being parsed as HTML tags. | ||
$comment_text = preg_replace_callback( | ||
'/(<)(\s*\d+(?:\.\d+)?[^<>]*?)(>)(\s*\d+(?:\.\d+)?)/', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this may be a bit too restrictive. For example, the following math expression would NOT get matched by this regex:
5 < x > 3
I suspect there are just too many possible math expressions out there to craft a proper regex to capture them. Instead, perhaps we just run esc_html()
on the comment text?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the review! I agree it's hard to capture everything with just a regex. Initially, I though about escaping the characters using esc_html()
, but that results in escaping any anchor tags as well:
Before | After |
---|---|
![]() |
![]() |
I wasn't sure if it's ideal to have these elements escaped in the comment list or not. Please let me know if I can make this change.
@BugReportOnWeb welcome! thanks for submitting this proposed fix. Indeed, it looks like comments are being affected by this, but the problem does seem to stem from HTML parsing issues in I’m going to mark this as a duplicate because I think we want to be careful not to patch over a bug deeper in the system on the rendering path. That will add overhead and then leave some quirks around once the underlying issue is resolved. In #9270 we’re looking at relying on the HTML API to provide actual HTML parsing too, freeing us from having to discuss which specific instances of a parse failure we want to attempt to restore (without accidentally breaking other ones). |
This PR fixes the issue where mathematical comparison expressions in comments were causing comment content between
<
and>
to incorrectly strip when displayed in the WordPress admin comments list view.Testing Instructions:
Trac ticket: https://core.trac.wordpress.org/ticket/63810
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.