search

Automattic / _s

Hi. I'm a starter theme called _s, or underscores, if you like. I'm a theme meant for hacking so don't use me as a Parent Theme. Instead try turning me into the next, most awesome, WordPress theme out there. That's what I'm here for.
http://underscores.me/
GNU General Public License v2.0
10.94k stars 3.12k forks source link

escape get_the_title #1367

Closed bmcculley closed 4 years ago

bmcculley commented 5 years ago

use esc_html function on get_the_title to prevent potential malicious issues.

Changes proposed in this Pull Request:

Updated comments.php added esc_html function on get_the_title to prevent potential malicious issues.

Related issue(s):

issue #1366

Ismail-elkorchi commented 4 years ago

@bmcculley Thank you for your pull request.

In addition to comments.php, inc/template-tags.php, template-parts/content-page.php and template-parts/content.php also contain instances of the get_the_title() function which needs to be escaped.

Could you please update your pull request to escape those too?

bmcculley commented 4 years ago

@Ismail-elkorchi Thank you, I have updated the other files as well.

bmcculley commented 4 years ago

@tomjn should I take this as the final word and change it now or wait for more input?

IanDelMar commented 4 years ago

You could consider using the 'the_title' hook instead of re-writing all the files. I know it's not late escaping but it makes it much more easier to change the then default escaping function sitewide.

Ismail-elkorchi commented 4 years ago

@bmcculley see https://github.com/Automattic/_s/issues/1366#issuecomment-605448941

Ismail-elkorchi commented 4 years ago

@bmcculley the recommendation @tomjn gave you is right on point. Replacing esc_html() with wp_kses_post() is more appropriate in this case, as the title can contain some basic tags.

After making these changes, can you please squash your commits into one ? Here is how to do it. The PR then should be good to go.

bmcculley commented 4 years ago

@Ismail-elkorchi, thank you. I have made and committed the changes.

Ismail-elkorchi commented 4 years ago

@bmcculley unfortunately some unrelated changes got included in the commit because your fork has gone out of sync. You should rebase your branch atop of the latest version of master.

bmcculley commented 4 years ago

@Ismail-elkorchi, sorry about that, rebased with master.