get_the_title() should be escaped

[bmcculley]

I think get_the_title() should use esc_html in the comments.php file to prevent various possible malicious issues.

The two instances:

• Line 36
• Line 43

[denis-adobe]

can you explain what malicious issues you mean? that would mean every Automattic theme would have this issue https://github.com/Automattic/themes/blob/master/publication/comments.php#L31 https://github.com/Automattic/themes/blob/master/ixion/comments.php#L33 https://github.com/Automattic/themes/blob/master/radcliffe-2/comments.php#L35 https://github.com/Automattic/themes

[ylkyrg]

See comment from Tom J Nowell in the code reference:

Super admins and administrators have the ability to enter arbitrary HTML in the title field, but that doesn’t prevent problems from appearing, for example:

• A rogue administrator adds a script tag with malicious javscript
• A hacker manages to change the title via an exploit
• A compromised plugin uses a filter to change the title
• A broken plugin allows it to be changed
• A hacker has broken into Redis/APC/Memcached and modified the cache
• File based caches have been compromised

All of this is a non-issue with escaping, which makes sure what’s outputted is what you expected. That doesn’t mean you can’t let users put HTML in there, as long as you specify which tags are allowed

To display the title safely, do this: echo esc_html( get_the_title() );

And if you want the title to include HTML tags: echo wp_kses_post( get_the_title() );

[philiparthurmoore]

Got a patch or PR for this? It's something that needs to be fixed.

See https://themes.trac.wordpress.org/browser/twentytwenty/1.0/comments.php#L37 for an example.

[bmcculley]

@philiparthurmoore yes, I submitted pull request #1367 that patches this.

[Ismail-elkorchi]

Based on some tickets I found on WordPress Trac, escaping in those cases is not necessary and is not dictated by general best practices since the post title is escaped before it is saved to the database.

Moreover, Twenty Twenty which @philiparthurmoore suggested as an example removed the use of esc_html for get_the_title().

https://core.trac.wordpress.org/ticket/30724 https://core.trac.wordpress.org/ticket/49190

Update :

I had a chat with @carolinan on slack and she confirmed to me that escaping is still relevant here.

[samikeijonen]

This issue doesn't involve translations though. Or am I misreading this?

[Ismail-elkorchi]

@samikeijonen Of course it doesn't ! It only involve the title, which has nothing to do with translation. I don't know what I was thinking when I wrote this.

[Ismail-elkorchi]

This issue was fixed in https://github.com/Automattic/_s/pull/1367