Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#54277 closed defect (bug) (fixed)

values within loop should be escaped properly before echo `wp-admin/theme-install.php`

Reported by: sabbirshouvo's profile sabbirshouvo Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.9 Priority: normal
Severity: normal Version:
Component: Themes Keywords: has-patch
Focuses: coding-standards Cc:

Description

In wp-admin/theme-install.php line 232 $feature_name is not escaped properly before echo the value. It should be escaped. In a similar scenario in file wp-admin/includes/theme-install.php same variable is escaped with esc_html( )

Attachments (3)

54277.diff (635 bytes) - added by sabbirshouvo 3 years ago.
escape $feature_name using esc_html()
Screenshot at Oct 17 11-49-59.png (153.7 KB) - added by sabbirshouvo 3 years ago.
@sabernhardt I agree with you. Check the image. I think it's more readable this way.
54277.2.diff (1.1 KB) - added by sabbirshouvo 3 years ago.
improved code readability & fix variable escaping.

Download all attachments as: .zip

Change History (12)

@sabbirshouvo
3 years ago

escape $feature_name using esc_html()

#1 follow-up: @afragen
3 years ago

Since it’s a variable, shouldn’t that be esc_attr()?

#2 in reply to: ↑ 1 @sabbirshouvo
3 years ago

Replying to afragen:

Since it’s a variable, shouldn’t that be esc_attr()?

I've used wp-admin/includes/theme-install.php line 149 as a reference.

#3 @sabernhardt
3 years ago

  • Version trunk deleted

Using esc_html() would be appropriate for the label text; the category name's variable is escaped the same way for the legend tag on line 226.

Side note: I got confused by both variables named $feature_name because the first foreach loop refers to the feature category name. Could we change that variable to $category_name (or something similar)?

@sabbirshouvo
3 years ago

@sabernhardt I agree with you. Check the image. I think it's more readable this way.

@sabbirshouvo
3 years ago

improved code readability & fix variable escaping.

#4 @mukesh27
3 years ago

Can we use escaped value directly instead of set variable for the escape value then use for print?

$feature_category = esc_html( $feature_category );
echo '<legend>' . $feature_category . '</legend>';

Should be

echo '<legend>' . esc_html( $feature_category ) . '</legend>';

#5 @sabbirshouvo
3 years ago

Actually, my original patch is for the next two variables. The variable $feature_name was not properly escaped. And your suggestion looks good too. Should I make changes to my patch? @mukesh27

#6 @mukesh27
3 years ago

Let's wait for other dev feedback on this.

#7 @SergeyBiryukov
3 years ago

  • Milestone changed from Awaiting Review to 5.9

#8 @SergeyBiryukov
3 years ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 51923:

Coding Standards: Improve escaping in wp-admin/theme-install.php.

  • Rename a duplicate $feature_name variable to $feature_group for clarity.
  • Escape the remaining $feature_name variable.

Follow-up to [27636], [35273].

Props sabbirshouvo, sabernhardt, mukesh27, afragen.
Fixes #54277.

This ticket was mentioned in Slack in #core by sergey. View the logs.


3 years ago

Note: See TracTickets for help on using tickets.