[wp-trac] [WordPress Trac] #48117: onclick attribute is not properly escaped in the _render_item method of WP_Admin_Bar class.

[WordPress Trac]

#48117: onclick attribute is not properly escaped in the _render_item method of
WP_Admin_Bar class.
--------------------------+------------------------------
 Reporter:  tmatsuur      |       Owner:  whyisjake
     Type:  defect (bug)  |      Status:  assigned
 Priority:  normal        |   Milestone:  Awaiting Review
Component:  Toolbar       |     Version:  5.2.3
 Severity:  normal        |  Resolution:
 Keywords:                |     Focuses:
--------------------------+------------------------------
Description changed by whyisjake:

Old description:

> 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( "wp" )"
> onclick='alert( "wp" )'>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.

New description:

 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( "wp" )"
 onclick='alert( "wp" )'>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.

--

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/48117#comment:3>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform