Add some admin bar methods to the printing functions

[johnbillion]

Passing unescaped data to the title property of WP_Admin_Bar::add_menu() is unsafe, because it's not escaped on output in the menu. Example:

add_action( 'admin_bar_menu', function( WP_Admin_Bar $wp_admin_bar ) {

    $title = 'Hello world<script>alert(document.cookie);</script>';
    $wp_admin_bar->add_menu( array(
        'id'    => 'wpcs-test',
        'title' => $title,
        'href'  => '#',
    ) );

} );

It would be nice if the following could be added as printing functions, so they trigger a warning when unescaped data is passed to them:

• $wp_admin_bar->add_menu
• $wp_admin_bar->add_node

I tried adding these to the WordPress_Sniff::$printingFunctions property array, but it didn't work, so this might not even be possible. The use of a specific variable name is weird, but it's a common one to use for the parameter passed to callback functions hooked onto the admin_bar_menu hook.

Any ideas about how this could be achieved?

[JDGrimes]

What we need to do is add some code that will check to see if the token before the function name is a T_OBJECT_OPERATOR or T_DOUBLE_COLON, and if so grab the variable name/class name. It would even be possible to map the value to the correct class name if the value was $this, self, parent, etc. I've thought of adding this feature before, I just haven't had a reason to do it yet.

---

This just reminded me, the add_menu_page() family of functions also does not escape the $menu_title (third parameter) before displaying it. We should add those to the printing functions as well.

[johnbillion]

This just reminded me, the add_menu_page() family of functions also does not escape the $menu_title (third parameter) before displaying it. We should add those to the printing functions as well.

Agreed.

[jrfnl]

Probably a silly question, but wouldn't it be more efficient to add an escaping function in the output loops for the add_menu_page() and admin bar family functions in core ?

[JDGrimes]

Probably a silly question, but wouldn't it be more efficient to add an escaping function in the output loops for the add_menu_page() and admin bar family functions in core ?

Yes, but I have doubts that such a patch would be committed, because of concerns with backward-compatibility. It is entirely possible that some plugins are actually using this "feature". You're welcome to try though.

[jrfnl]

Yes, but I have doubts that such a patch would be committed, because of concerns with backward-compatibility.

I'd have to check, but I seem to remember esc_html has protection against double escaping, so that shouldn't necessarily cause breaks. Alternatively wp_kses could possibly be used without breaking much. Or am I totally underestimating the potential impact ?

Wouldn't mind having a more detailed look (and send in a patch to core), but will be a while before I have the time.

[JDGrimes]

Right, esc_html() is idempotent. The issue would be that plugins might actually be adding their own HTML markup to the menu titles. wp_kses() would give us the chance of allowing some safe tags while still escaping the value, however it might still break some plugins that are doing it wrong. There is also the theory the wp_kses() has a negative performance impact, and should therefore not be applied on output unless absolutely necessary (though I'm not sure what the core team's current stance on this is).