Make WordPress Core

Opened 18 months ago

Closed 4 months ago

Last modified 4 months ago

#58303 closed enhancement (wontfix)

Escape $columns_css variable in dashboard widget

Reported by: mahamudur78's profile 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

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
18 months ago

Here, I found a similar things at https://core.trac.wordpress.org/ticket/57133

Version 0, edited 18 months ago by nazmulhudadev (next)

#3 follow-up: @SergeyBiryukov
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 @sabernhardt
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

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

#10 @desrosj
4 months ago

  • Keywords 2nd-opinion close removed
  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

I agree with closing this one out. @mahamudur78 thanks for opening this, though!

@desrosj commented on PR #4451:


4 months ago
#11

This ticket was closed as wontfix.

Note: See TracTickets for help on using tickets.