WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 4 months ago

#30724 closed defect (bug) (fixed)

Twenty Fifteen: Unnecessary use of esc_html()

Reported by: ocean90 Owned by: johnbillion
Milestone: 4.1 Priority: high
Severity: normal Version:
Component: Bundled Theme Keywords: has-patch commit fixed-major
Focuses: Cc:

Description

Since #30651 I had the attached patch on my disk but never created a ticket, until now. Will do a refresh in the next hours.

Attachments (2)

twentyfifteen.patch (23.1 KB) - added by ocean90 2 years ago.
30724-dates.patch (690 bytes) - added by ocean90 2 years ago.

Download all attachments as: .zip

Change History (26)

#1 @ocean90
2 years ago

  • Owner set to ocean90
  • Status changed from new to accepted

#2 @ocean90
2 years ago

In 30896:

Twenty Fifteen: Don't escape translated strings.

Replace every unnecessary instance of esc_html__() and esc_html_e() with just __() and _e().

see #30724.

#3 @ocean90
2 years ago

In 30897:

Twenty Fifteen: Remove esc_html() from blog description.

The blog description gets esc_html()'d *into* the DB. It's also escaped because the filter for get_bloginfo() is set to 'display'.

see #30724.

#4 @ocean90
2 years ago

In 30899:

Twenty Fifteen: Don't escape translated strings.

Replace every unnecessary instance of esc_html_x() with just _x().

see #30724.

#5 @ocean90
2 years ago

In 30901:

Twenty Fifteen: Remove esc_html() from get_the_author().

The display name is sanitized through sanitize_text_field, wp_filter_kses, and _wp_specialchars.

see #30724.

@ocean90
2 years ago

#6 follow-up: @ocean90
2 years ago

In 30905:

Twenty Fifteen: Remove unnecessary esc_html() from get_the_date() and get_the_modified_date().

see #30724.

#7 @ocean90
2 years ago

  • Keywords commit fixed-major added

#8 in reply to: ↑ 6 ; follow-up: @adamsilverstein
2 years ago

Replying to ocean90:

In 30905:

Twenty Fifteen: Remove unnecessary esc_html() from get_the_date() and get_the_modified_date().

see #30724.

@ocean90: I'm curious about your removal of escaping from translations - can you briefly explain/point me to the the logic? I am always suspicious of translations as a malicious script injection vector, is this not the case?

#9 @TomasM
2 years ago

Is there anywhere a clear guidance for the theme developers on when to use escaping?

I follow the development of _s and default WP themes to make my themes better and recently in Twenty Fifteen and _s there was escaping promoted. Now, when I applied those changes to my theme I will have to revert :-/

Is there any harm for having escaping in place?

#10 in reply to: ↑ 8 ; follow-up: @ocean90
2 years ago

Replying to adamsilverstein:

@ocean90: I'm curious about your removal of escaping from translations - can you briefly explain/point me to the the logic?

That's simple. If we don't trust translations anymore we should do it directly in the function. function __() { return esc_html(….

Replying to TomasM:

I follow the development of _s and default WP themes to make my themes better and recently in Twenty Fifteen and _s there was escaping promoted. Now, when I applied those changes to my theme I will have to revert :-/

That over-escaping seems to come from .com which is something I (and nacin, I talked with him before this ticket) don't want to support, especially not for a default theme where we have a type of review process for translations.

Is there any harm for having escaping in place?

No harm.


BTW: Twenty Fifteen wasn't even consistent with this.

#11 in reply to: ↑ 10 @adamsilverstein
2 years ago

Thanks for the clarification, much appreciated. [edited for brevity]

Last edited 2 years ago by lancewillett (previous) (diff)

#12 follow-up: @sboisvert
2 years ago

One problem with not escaping translations is that some plugins that filter translations will allow end users to push translations in the back end.
Depending on where these go they can break the code because characters are not escaped properly and the end users doing the translations won't understand what broke.
This doesn't even take under account that you may not be able to trust the end users doing the translations.

I feel that escaping protects against user error and potentially malicious users especially with translation plugins with little cost / negative repercussions.

Thanks!

#13 in reply to: ↑ 12 @johnbillion
2 years ago

Replying to sboisvert:

This doesn't even take under account that you may not be able to trust the end users doing the translations.

Translations are inherently trusted. The __() family of functions are used thousands of times and they don't escape output. If we're not trusting translations then we have a big problem.

#14 @johnbillion
2 years ago

  • Owner changed from ocean90 to johnbillion
  • Status changed from accepted to assigned

#15 @nacin
2 years ago

I support all changes here. What _s does has been dictated by Automattic's zealous escaping rules for VIPs and http://theme.wordpress.com/join/. It is not dictated by general best practices.

#16 @johnbillion
2 years ago

In 30924:

Twenty Fifteen: Don't escape translated strings.

Replace every unnecessary instance of esc_html__() and esc_html_e() with just __() and _e().

Merges [30896] to the 4.1 branch.

see #30724.

#17 @johnbillion
2 years ago

In 30925:

Twenty Fifteen: Remove esc_html() from blog description.

The blog description gets esc_html()'d *into* the DB. It's also escaped because the filter for get_bloginfo() is set to 'display'.

Merges [30897] to the 4.1 branch.

see #30724.

#18 @johnbillion
2 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 30926:

Twenty Fifteen: Remove various unnecessary esc_html() calls and replace various unnecessary esc_html_x() calls with _x().

Merges [30899], [30901], and [30905] into 4.1.

Props ocean90
Fixes #30724

This ticket was mentioned in Slack in #core-themes by ocean90. View the logs.


21 months ago

This ticket was mentioned in Slack in #core-themes by philip. View the logs.


20 months ago

This ticket was mentioned in Slack in #themereview by emiluzelac. View the logs.


17 months ago

This ticket was mentioned in Slack in #themereview by rabmalin. View the logs.


14 months ago

This ticket was mentioned in Slack in #themereview by rabmalin. View the logs.


8 months ago

This ticket was mentioned in Slack in #themereview by rabmalin. View the logs.


4 months ago

Note: See TracTickets for help on using tickets.