WordPress.org

Make WordPress Core

Opened 7 weeks ago

Closed 5 weeks ago

#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)

comment-on-post-with-markup-in-title.jpg (53.8 KB) - added by joyously 7 weeks ago.
Using esc_html() makes the markup show.
49190.diff (426 bytes) - added by kjellr 6 weeks ago.
49190.2.diff (923 bytes) - added by peterwilsoncc 6 weeks ago.

Download all attachments as: .zip

Change History (11)

#1 @joyously
7 weeks 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.

@joyously
7 weeks ago

Using esc_html() makes the markup show.

#2 follow-up: @kjellr
6 weeks 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. 😕

Last edited 6 weeks ago by kjellr (previous) (diff)

#3 in reply to: ↑ 2 @peterwilsoncc
6 weeks 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.

@kjellr
6 weeks ago

#4 follow-up: @kjellr
6 weeks 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.

Last edited 6 weeks ago by kjellr (previous) (diff)

#5 in reply to: ↑ 4 @SergeyBiryukov
6 weeks 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 @SergeyBiryukov
6 weeks ago

  • Summary changed from Consider escaping get_the_title() in default themes to Twenty Twenty: Remove unnecessary escaping for get_the_title()

#7 @peterwilsoncc
6 weeks ago

In 49190.2.diff: As above but with the same change on line 37 of the same file.

#8 @SergeyBiryukov
5 weeks ago

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

In 47110:

Twenty Twenty: Remove unnecessary escaping of get_the_title() in comments.php.

For users with the unfiltered_html capability, titles can contain legitimate markup.

The title is filtered on saving, so the content in the database is considered safe.

Props kjellr, joyously, peterwilsoncc.
Fixes #49190.

Note: See TracTickets for help on using tickets.