Make WordPress Core

Opened 11 months ago

Last modified 11 months ago

#58303 new enhancement

Escape $columns_css variable in dashboard widget

Reported by: mahamudur78's profile mahamudur78 Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-patch 2nd-opinion
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 (8)

This ticket was mentioned in PR #4451 on WordPress/wordpress-develop by @nazmulhudadev.


11 months ago
#1

  • Keywords has-patch added

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

#2 @nazmulhudadev
11 months ago

Here, I found a similar things at #57133

Last edited 11 months ago by SergeyBiryukov (previous) (diff)

#3 @SergeyBiryukov
11 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 @sabernhardt
11 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

#6 follow-up: @hbhalodia
11 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 @SergeyBiryukov
11 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:


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

Note: See TracTickets for help on using tickets.