New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
EscapeOutputSniff should not flag echoing WP_Widget::get_field_id() and WP_Widget::get_field_name() #159
Comments
Work in |
Since |
Technically, yes. However, I have never ever seen any string passed into |
|
<input name="<?php echo $this->get_field_name( 'nope"><script>alert('PWNED');</script>' ) ?>"> |
It's a small risk sure, but still a risk (risk is binary - it either exists or it doesn't). Why introduce inconsistencies in your own code about escaping, and subsequently adding exceptions to the sniffs? |
Core doesn't consider |
Here, using It's also just a function - it can't change without an awful lot of hackery within WP. A method call though, such as
I agree - keep everything consistent§, and only introduce custom exceptions via a project ruleset. § This escaping sniff overall gives me by far the most false positives in terms of unwanted HTML context escaping as per my example, but there's probably an existing ticket for that elsewhere. |
Yes, but consider a malicious POT file:
And then compare: <h1><?php _e( 'Hello world', 'textdomain' ); ?></h1> ↳ <h1>Hola mundo<script>alert('PWNED')</script></h1> With: <h1><?php esc_html_e( 'Hello world', 'textdomain' ); ?></h1> ↳ <h1>Hola mundo<script>alert('PWNED')</script></h1> Unlikely? Yes. Risk? Probably not. In any case, such edge cases should be covered by sniff properties so rulesets can fine tune the behavior. |
@westonruter I think Nick's latest post on VIP lobby makes late-escaping such functions a requirement for VIP platform development. |
Wow. You're right! He even mostly used the same example of |
Don't suppose Nick's post could be copied somewhere please? I don't have access. |
@GaryJones sent it to you over email. |
We published the post publicly here: http://vip.wordpress.com/2014/06/20/the-importance-of-escaping-all-the-things/ As mentioned in this thread, the risk with the widget functions is real, because any code on the site can sneakily inject itself in the way mentioned in the VIP post, in non-obvious ways. We also are requiring escaping of translations on WordPress.com VIP, for the same reasons outlined by @westonruter. |
Thanks @nickdaugherty . So you don't allow |
Unrelated to VIP, I tend to not trust translations personally in my plugins and other code. Unless my translation string specifically has HTML in it, then I prefer to use |
Closing since the main issue has been resolved. |
…additional-function NoAddAdminPages: add `add_links_page()` to the functions list
There's no reason that the
echo
statements on these lines should be flagged:The text was updated successfully, but these errors were encountered: