EscapeOutputSniff should not flag echoing WP_Widget::get_field_id() and WP_Widget::get_field_name()

[westonruter]

There's no reason that the echo statements on these lines should be flagged:

<label for="<?php echo $this->get_field_id( 'title' ); ?>"><?php _e( 'Title:' ); ?></label>
<input class="widefat" id="<?php echo $this->get_field_id( 'title' ); ?>" 
       name="<?php echo $this->get_field_name( 'title' ); ?>"
       type="text"
       value="<?php echo esc_attr( $instance['title'] ); ?>">

[westonruter]

Work in issue-159-escape-output-sniff-whitelist-widget-methods branch.

[GaryJones]

Since get_field_id() and get_field_name() don't escape the values, would it not be sensible for your code to use esc_attr() to follow best practice about always escaping late in context?

[westonruter]

Technically, yes. However, I have never ever seen any string passed into $widget->get_field_id() which was anything other than a static string. I never see a variable passed in, and so there isn't a risk of it causing a problem. I would say the risk here is at the same level as not escaping the output of _e(), which is a potential attack vector if a malicious translation were installed—and so esc_html_e() and friends should need to be used.

[shadyvb]

Thou shalt never trust User Input :grinning: ( devs being users here )

[westonruter]

<input name="<?php echo $this->get_field_name( 'nope"><script>alert('PWNED');</script>' ) ?>">

[GaryJones]

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?

[westonruter]

Core doesn't consider _e() to be a significant risk. But if you feel that strongly about it, perhaps this should be turned off by default, and only activated via a sniff property in a project ruleset.

[GaryJones]

_e() is just a translating function - it can be used when escaping is not needed e.g.:

<h1><?php _e( 'My <em>Emphasis</em>', 'textdomain' ); ?></h1>

Here, using esc_html() would print the literal markup, which would be wrong.

It's also just a function - it can't change without an awful lot of hackery within WP. A method call though, such as $this->get_field_name() could have been overridden by a custom subclass, so your other methods can't assume that it's the parent or peer version that's being used.

perhaps this should be turned off by default, and only activated via a sniff property in a project ruleset

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.

[westonruter]

_e() is just a translating function - it can be used when escaping is not needed

Yes, but consider a malicious POT file:

msgid "Hello world"
msgstr "Hola mundo<script>alert('PWDNED')</script>"

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&lt;script&gt;alert('PWNED')&lt;/script&gt;</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.

[shadyvb]

@westonruter I think Nick's latest post on VIP lobby makes late-escaping such functions a requirement for VIP platform development.

[westonruter]

Wow. You're right! He even mostly used the same example of WP_Widget::get_field_name()

[GaryJones]

Don't suppose Nick's post could be copied somewhere please? I don't have access.

[westonruter]

@GaryJones sent it to you over email.

[nickdaugherty]

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.

[GaryJones]

Thanks @nickdaugherty .

So you don't allow _e()? Do you have a list of WP core functions that you don't allow?

[Viper007Bond]

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 esc_(html|attr)_e() instead of just _e(). It's just safer.

[shadyvb]

Closing since the main issue has been resolved.