Improve escaping in /wp-admin/includes/template.php

[McGuive7]

It was brought to my attention that various output in /wp-admin/includes/template.php is missing proper escaping. This includes titles for settings sections and fields, inline Thickbox JS, and various translatable strings (the translations for which might accidentally or intentionally include problematic content).

This patch adds various escaping functions where needed.

[McGuive7]

Patch to fix missing escaping functions from /wp-admin/includes/template.php

[SergeyBiryukov]

esc_html_e() should only be used for translatable strings, not for PHP variables.

Translations should be considered safe, we generally only escape them in attributes or in <option> tags.

[McGuive7]

Aha, I see that I was incorrectly using esc_html_e() for variables, when it is indeed a gettext function. I can fix that for sure.

On another point, I'm confused. You say that esc_html_e() should only be used for translatable strings, and then you go on to say translations should be considered safe - which to me means that they don't need escaping. If they are safe, and never need escaping, then why does a function like esc_html__() or esc_html_e() exist? Can you clarify on this one?

I've actually had quite a bit of general confusion on what to escape and what not to escape. Up until recently, I had assumed that there was no need to escape translations, as the translator should ensure that they are safe, right? But then I recently received a PR on one of my projects stating the following as a reason for escaping translations:

In this case there might be code injections in translation files. There are some plugins that allow you to translate in-plugin text directly from WP admin and so on. You can't rely on what might happen and what new ways of translation or code injections might come up.

Additionally, when I dig into WP core, there are plenty of examples of escaping translatable strings using esc_html_e() and esc_attr_e(). That said, I've also come a cross many instances in which translatable strings aren't escaped - here are a few examples:

/wp-admin/custom-header.php (extra odd because it includes markup)

echo __( '<strong>Random:</strong> Show a different image on each page.' );

/wp-admin/network/users.php

echo __( 'Attribute all content to:' )

/wp-admin/includes/class-wp-posts-list-table.php`

echo __( '–OR–' );

Additionally, this thread on the _s repo seems to indicate that translatable strings are not always safe, as plugins can allow users to include potentially malicious translations: ​https://github.com/Automattic/_s/issues/231

Anyhow, thanks for the feedback - any further clarification would be awesome. Thanks!

[ocean90]

Replying to McGuive7:

On another point, I'm confused. You say that esc_html_e() should only be used for translatable strings, and then you go on to say translations should be considered safe

That was just for the wrong use of esc_html_e() for esc_attr_e( $bytes ) in your patch.

Core translations are considered as safe because we have a review process for them. See also the comments on #30724.

Side note: All occurrences of echo __() should be replaced with _e().

[marsjaninzmarsa]

Replying to ocean90:

Side note: All occurrences of echo __() should be replaced with _e().

Fixed in #32239.