#49190 closed defect (bug) (fixed)
Twenty Twenty: Remove unnecessary escaping for get_the_title()
Reported by: | kjellr | Owned by: | SergeyBiryukov |
---|---|---|---|
Milestone: | 5.4 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Bundled Theme | Keywords: | has-patch commit |
Focuses: | Cc: |
Description
Twenty Twenty escapes get_the_title()
using esc_html()
in its comments.php
file:
https://themes.trac.wordpress.org/browser/twentytwenty/1.0/comments.php#L49
echo sprintf( /* translators: 1: number of comments, 2: post title */ _nx( '%1$s reply on “%2$s”', '%1$s replies on “%2$s”', $comments_number, 'comments title', 'twentytwenty' ), number_format_i18n( $comments_number ), esc_html( get_the_title() ) );
Elsewhere, the theme escapes get_the_title()
using wp_kses_post()
:
https://themes.trac.wordpress.org/browser/twentytwenty/1.0/template-parts/navigation.php#L37
<span class="title"><span class="title-inner"><?php echo wp_kses_post( get_the_title( $prev_post->ID ) ); ?></span></span>
Escaping get_the_title
one way or another seems like generally good practice, however none of the other default themes seem to do this regularly. For example:
Twenty Nineteen:
https://core.trac.wordpress.org/browser/tags/5.0/src/wp-content/themes/twentynineteen/comments.php#L52
Twenty Seventeen:
https://themes.trac.wordpress.org/browser/twentyseventeen/2.2/comments.php#L49
Twenty Sixteen:
https://themes.trac.wordpress.org/browser/twentysixteen/2.0/comments.php#L43
Twenty Fifteen:
https://themes.trac.wordpress.org/browser/twentyfifteen/2.5/comments.php#L43
Twenty Fourteen:
https://themes.trac.wordpress.org/browser/twentyfourteen/2.7/comments.php#L42
Twenty Thirteen:
https://themes.trac.wordpress.org/browser/twentythirteen/2.9/comments.php#L37
Twenty Twelve:
https://themes.trac.wordpress.org/browser/twentytwelve/2.9/comments.php#L36
Twenty Eleven:
https://themes.trac.wordpress.org/browser/twentyeleven/3.3/comments.php#L38
Twenty Ten:
https://themes.trac.wordpress.org/browser/twentyten/2.9/comments.php#L41
Should those themes follow Twenty Twenty's lead and escaping get_the_title()
?
Additional relevant discussion in the _s repository, which many of these themes utilize: https://github.com/Automattic/_s/issues/1366/
Attachments (3)
Change History (12)
#2
follow-up:
↓ 3
@
5 years ago
It shouldn't be using esc_html() on the title, because titles can contain markup, so you've identified a bug in the comment.php.
Makes sense! Would wp_kses_post()
be a suitable replacement in these cases? It seems like we wouldn't want to eliminate those em
or bold
tags entirely, and something like strip_tags()
or esc_attr()
would do that. But on the other hand, there are probably a number of HTML tags that are allowed in posts that wouldn't make sense to display in a post title. 😕
#3
in reply to:
↑ 2
@
5 years ago
Replying to kjellr:
Would
wp_kses_post()
be a suitable replacement in these cases?
WordPress core runs KSES on the post's title as it's saved so the content in the database is considered safe. Therefore there is no need to run wp_kses_post()
on title. It's the same process that makes escaping the_content()
unnecessary.
The bug is in twentytwenty rather than the other themes.
#4
follow-up:
↓ 5
@
5 years ago
WordPress core runs KSES on the post's title as it's saved so the content in the database is considered safe. Therefore there is no need to run wp_kses_post() on title. It's the same process that makes escaping the_content() unnecessary.
That was my initial assumption, before seeing the linked suggestion in the _s
repository. There's some conflicting information on the code reference page that should be clarified:
https://developer.wordpress.org/reference/functions/get_the_title/#comment-2150
Searching around the web seems to result in similar confusion. Just as one example, this CSS Tricks article initially suggested that get_the_title()
was escaped by default, only to reverse that via a post update:
https://css-tricks.com/introduction-to-wordpress-front-end-security-escaping-the-things/
In any case, if get_the_title()
does not need to be escaped, 49190.diff should correct the use of esc_html()
in Twenty Twenty.
#5
in reply to:
↑ 4
@
5 years ago
- Keywords has-patch commit added
- Milestone changed from Awaiting Review to 5.4
Replying to kjellr:
That was my initial assumption, before seeing the linked suggestion in the
_s
repository.
Just noting that _s
tends to over-escape things, apparently per WordPress.com VIP guidelines, see the discussion in comment:10:ticket:30724.
49190.diff looks good.
#6
@
5 years ago
- Summary changed from Consider escaping get_the_title() in default themes to Twenty Twenty: Remove unnecessary escaping for get_the_title()
#7
@
5 years ago
In 49190.2.diff: As above but with the same change on line 37 of the same file.
#8
@
5 years ago
- Owner set to SergeyBiryukov
- Resolution set to fixed
- Status changed from new to closed
In 47110:
It shouldn't be using
esc_html()
on the title, because titles can contain markup, so you've identified a bug in the comment.php.