#58303 closed enhancement (wontfix)
Escape $columns_css variable in dashboard widget
Reported by: | mahamudur78 | Owned by: | |
---|---|---|---|
Milestone: | Priority: | normal | |
Severity: | normal | Version: | |
Component: | Administration | Keywords: | has-patch |
Focuses: | coding-standards | Cc: |
Description
While examining the \wp-admin\includes\dashboard.php file in WordPress, I discovered an escaping issue when echoing a dynamic value of an attribute (such as "class") in an HTML attribute. Specifically, the issue is located on line 269 of that file. Based on my observation, I believe that the dynamic value should be properly escaped to prevent potential syntax errors or security vulnerabilities.
Change History (11)
This ticket was mentioned in PR #4451 on WordPress/wordpress-develop by @nazmulhudadev.
18 months ago
#1
- Keywords has-patch added
#2
@
18 months ago
Here, I found a similar things at https://core.trac.wordpress.org/ticket/57133
#3
follow-up:
↓ 9
@
18 months ago
Hi there, welcome to WordPress Trac! Thanks for the ticket.
The $columns
variable goes through absint()
and is not user-editable, so it does not currently require escaping, though it might be preferable to add the escaping as a defensive coding measure.
This is indeed similar to [54857] / #57133, but there is also an ongoing discussion in comment:17:ticket:58251 on whether it is a good idea to preventively add escaping in cases like this.
#4
@
18 months ago
- Keywords 2nd-opinion added
- Summary changed from Found Escaping Issue While Echoing Attribute Dynamic Value in HTML Attribute. to Escape $columns_css variable in dashboard widget
- Type changed from defect (bug) to enhancement
This ticket was mentioned in PR #4482 on WordPress/wordpress-develop by @hbhalodia.
18 months ago
#5
Trac ticket: https://core.trac.wordpress.org/ticket/58303
#6
follow-up:
↓ 7
@
18 months ago
Hi @SergeyBiryukov @nazmulhudadev, I have added another patch for this which uses the core sanitize_html_class
https://developer.wordpress.org/reference/functions/sanitize_html_class/ function to properly sanitize the HTML class. I think we should use this function instead of any escaping function, as we are using it as a class. Hence, this would be a proper sanitization function being used in the context.
Thanks.
#7
in reply to:
↑ 6
@
18 months ago
Replying to hbhalodia:
I have added another patch for this which uses the core
sanitize_html_class
https://developer.wordpress.org/reference/functions/sanitize_html_class/ function
Thanks for the PR! However, as noted above, the $columns
variable goes through absint()
and is not user-editable, so it does not currently require sanitization.
It is also worth noting that per the security guiding principles, sanitizing should be done early and escaping should be as late as possible, so for the output itself, esc_attr()
would be the correct function to use.
@SergeyBiryukov commented on PR #4482:
18 months ago
#8
Thanks for the PR!
Closing this for now, as there is an existing PR in #4451, see the comment on the ticket for more details.
#9
in reply to:
↑ 3
@
5 months ago
- Keywords close added
Replying to SergeyBiryukov:
This is indeed similar to [54857] / #57133, but there is also an ongoing discussion in comment:17:ticket:58251 on whether it is a good idea to preventively add escaping in cases like this.
The discussion in #58251 culminated in the decision to NOT escape an HTML attribute value when that value is guaranteed to be safe. In this case, as Sergey explained, the attribute value is the concatenation of an explicit string and the result of an absint()
call. Hence the value is safe and does not need unnecessary escaping.
Hi there! Thanks for contributing to WordPress!
Yes, I think so that it should be escaped.
Trac ticket: https://core.trac.wordpress.org/ticket/58303