Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#49190 closed defect (bug) (fixed)

Twenty Twenty: Remove unnecessary escaping for get_the_title()

Reported by: kjellr's profile kjellr Owned by: sergeybiryukov's profile 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 5 years ago.
Using esc_html() makes the markup show.
49190.diff (426 bytes) - added by kjellr 5 years ago.
49190.2.diff (923 bytes) - added by peterwilsoncc 5 years ago.

Download all attachments as: .zip

Change History (12)

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

@joyously
5 years ago

Using esc_html() makes the markup show.

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

Last edited 5 years ago by kjellr (previous) (diff)

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

@kjellr
5 years ago

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

Last edited 5 years ago by kjellr (previous) (diff)

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

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

#8 @SergeyBiryukov
5 years 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.

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


5 years ago

Note: See TracTickets for help on using tickets.