XSS: Functions that already are escaped

WordPress / WordPress-Coding-Standards

PHP_CodeSniffer rules (sniffs) to enforce WordPress coding conventions

MIT License

2.56k stars 487 forks source link

XSS: Functions that already are escaped #747

Open grappler opened 7 years ago

grappler commented 7 years ago

Split off from #744

Expected next thing to be an escaping function (see Codex for 'Data Validation'), not 'get_bloginfo' Escaping of bloginfo via a filter.

get_bloginfo( 'name' ) should give an error but get_bloginfo( 'name', 'display' ) should not.

designseer commented 7 years ago

get_bloginfo( 'name', 'display' ) also gives the same error.

jrfnl commented 1 year ago

I've been looking at this, but this is not that straight-forward.

The function runs either of two filter hooks - bloginfo_url or bloginfo. https://github.com/WordPress/wordpress-develop/blob/bea4307daa21a3f97d159898b0ac676332e0e7de/src/wp-includes/general-template.php#L914-L936

For bloginfo, I can find an escaping function hooked into the filter in the default_filters.php file. For bloginfo_url, I cannot, for the life of me, find any functions hook in, let alone an escaping function.

If anyone can point to the code which hooks in an escaping function for bloginfo_url, this can be actioned, but until that time, I find this a risky change.

SergeyBiryukov commented 1 year ago

Some history here:

r2935 / #1545 added a conditional to avoid applying texturize filters to the get_bloginfo( 'url' ) value.
r3117 / #1890 added directory to the list.
r3681 / #2381 added home to the list.
r4168 / #2643 added the bloginfo_url filter.
r5998 / #4516 added the display context.

This was previously brought up in #26803 get_bloginfo() doesn't sanitize URLs, even when $filter is 'display', which is currently closed as wontfix due to backward compatibility concerns, see also the discussion in #16408:comment:1.

It might be worth revisiting that ticket and applying sanitize_url() to the bloginfo_url filter.

jrfnl commented 1 year ago

Thank you @SergeyBiryukov for providing that context!

Based on this info, I think bloginfo( $name, 'display' ) can not always be seen as escaped and would still need additional special casing based on $name and the sniff would need to maintain a list of which keys apply the bloginfo_url filter, which makes this a more complex change and more fiddly maintenance-wise.

jrfnl commented 1 year ago

As a side-note: yes, I think it would be good if this is revisited in Core.