Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#48117 closed defect (bug) (fixed)

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

Reported by: tmatsuur's profile tmatsuur Owned by: whyisjake's profile whyisjake
Milestone: 5.4 Priority: normal
Severity: normal Version: 5.2.3
Component: Toolbar Keywords: has-patch commit
Focuses: Cc:

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'   => '',
    'meta'   => array(
        'onclick'  => 'alert( "wp" )',
) );

The rendered HTML source looks like this:

<li id='wp-admin-bar-mylink'><a class='ab-item' href='' 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.

Attachments (1)

48117.diff (1.1 KB) - added by dinhtungdu 5 years ago.

Download all attachments as: .zip

Change History (10)

#1 @dd32
5 years ago

  • Component changed from General to Toolbar

#2 @whyisjake
5 years ago

Thanks for adding this ticket @tmatsuur

#3 follow-up: @whyisjake
5 years ago

  • Description modified (diff)

#4 in reply to: ↑ 3 @tmatsuur
5 years ago

Replying to whyisjake:

Thanks @whyisjake

#5 @SergeyBiryukov
5 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

5 years ago

#6 @dinhtungdu
5 years ago

  • Keywords has-patch added; needs-patch removed

48117.diff escapes onclick attribute properly and prevents duplicated rendering of onclick when the href attribute presents.

#7 @whyisjake
5 years ago

  • Keywords commit added
  • Milestone changed from Future Release to 5.4

#8 @whyisjake
5 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 46734:

Toolbar: Properly escape the onclick attribute.

The onclick attribute was being escaped twice, once with esc_js and again with esc_attr.

Fixes #48117.
Props tmatsuur, dinhtungdu.

Note: See TracTickets for help on using tickets.