Make WordPress Core

Opened 4 weeks ago

Last modified 4 days ago

#48117 assigned defect (bug)

onclick attribute is not properly escaped in the _render_item method of WP_Admin_Bar class.

Reported by: tmatsuur Owned by: whyisjake
Milestone: Future Release Priority: normal
Severity: normal Version: 5.2.3
Component: Toolbar Keywords: needs-patch
Focuses: Cc:
PR Number:

Description (last modified by whyisjake)

I noticed that when the onclick attribute value is specified with the meta key of the add_menu method of $wp_admin_bar, the onclick attribute is output twice.

$wp_admin_bar->add_menu( array(
    'id'     => 'mylink',
    'title'  => 'mylink',
    'href'   => 'https://wordpress.org/',
    'meta'   => array(
        'onclick'  => 'alert( "wp" )',
) );

The rendered HTML source looks like this:

<li id='wp-admin-bar-mylink'><a class='ab-item' href='https://wordpress.org/' onclick="alert( &quot;wp&quot; )" onclick='alert( &quot;wp&quot; )'>mylink</a></li>

As a result of investigating this cause, if the href attribute was set in the _render_item method, the onclick attribute was output after being escaped with the esc_js function, and then was output after being escaped with another esc_attr function.

If the href attribute is not set, the onclick attribute is output after being escaped by the esc_attr function.

The vulnerable source code is as follows: class-wp-admin-bar.php: 536-551

if ( $has_link ) {
    $attributes = array( 'onclick', 'target', 'title', 'rel', 'lang', 'dir' );
    echo "<a class='ab-item'$aria_attributes href='" . esc_url( $node->href ) . "'";
    if ( ! empty( $node->meta['onclick'] ) ) {
        echo ' onclick="' . esc_js( $node->meta['onclick'] ) . '"';
} else {
    $attributes = array( 'onclick', 'target', 'title', 'rel', 'lang', 'dir' );
    echo '<div class="ab-item ab-empty-item"' . $aria_attributes;

foreach ( $attributes as $attribute ) {
    if ( ! empty( $node->meta[ $attribute ] ) ) {
        echo " $attribute='" . esc_attr( $node->meta[ $attribute ] ) . "'";

The variable $attributes always contains onclick, so it escapes with the esc_attr function and outputs the attribute value.

I hope that the onclick attribute will be output properly.

Change History (5)

#1 @dd32
4 weeks ago

  • Component changed from General to Toolbar

#2 @whyisjake
4 weeks ago

Thanks for adding this ticket @tmatsuur

#3 follow-up: @whyisjake
4 weeks ago

  • Description modified (diff)

#4 in reply to: ↑ 3 @tmatsuur
4 weeks ago

Replying to whyisjake:

Thanks @whyisjake

#5 @SergeyBiryukov
4 days ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release
Note: See TracTickets for help on using tickets.