Make WordPress Core

Opened 19 months ago

Last modified 8 weeks ago

#62545 new defect (bug)

Adminbar title not escaped

Reported by: kkmuffme's profile kkmuffme Owned by:
Milestone: Awaiting Review Priority: normal
Severity: minor Version:
Component: Toolbar Keywords: 2nd-opinion has-patch has-unit-tests
Focuses: Cc:

Description

class-wp-admin-bar.php

<?php
echo ">{$arrow}{$node->title}";

The title isn't getting escaped, there should be an esc_html() there?

Attachments (2)

0001-Coding-Standards-Escaping-adminbar-title-using-wp_ks.patch (1.1 KB) - added by yogeshbhutkar 19 months ago.
Patch to use wp_kses_post to escape the admin bar title instead of using esc_url or not escaping at all.
Screenshot 2024-11-25 at 11.32.22 AM.png (210.1 KB) - added by yogeshbhutkar 19 months ago.
Screenshot of using wp_kses_post() to escape the admin bar title.

Download all attachments as: .zip

Change History (11)

#1 @kkmuffme
19 months ago

e.g.

<?php
$wp_admin_bar->add_menu(
        array(
                'id'     => 'foo',
                'title'  => 'This & that',
                'href'   => 'https://foo.com',  
        )
);

The & will not be &amp; but literal

This ticket was mentioned in PR #7876 on WordPress/wordpress-develop by lgadzhev.


19 months ago
#2

  • Keywords has-patch added

@siliconforks commented on PR #7876:


19 months ago
#3

Doesn't this break the admin bar whenever HTML is used in a title (which is commonly done)?

I have attached a screenshot:

https://github.com/user-attachments/assets/bd8b405a-a480-4ed6-be71-8d91d4112c59

#4 @sabernhardt
19 months ago

  • Component changed from Security to Toolbar
  • Keywords close 2nd-opinion added; has-patch removed

It should not use esc_html() because that would break almost all links with an icon or image, including links—and the search form—from Core.

I'll leave the ticket open in case there is a better way to sanitize the node titles (or to skip creating a node if it finds something that does not belong).

@yogeshbhutkar
19 months ago

Patch to use wp_kses_post to escape the admin bar title instead of using esc_url or not escaping at all.

#5 @yogeshbhutkar
19 months ago

@sabernhardt, If escaping is required in this context, we can utilize wp_kses_post(). This function allows safe HTML to pass through while preventing potentially harmful elements, avoiding the side effects illustrated in the screenshot.

@yogeshbhutkar
19 months ago

Screenshot of using wp_kses_post() to escape the admin bar title.

#6 @sabernhardt
19 months ago

The wp_kses_post() function does not fit either. It would change & to &amp;, but it would also break the search node (the screenshot shows the front end without a search icon). The default KSES post array does not allow form or input elements, and the filter could remove other elements that plugins rely on.

#7 @sabernhardt
15 months ago

  • Keywords close removed

The title probably could normalize HTML entities:
wp_kses_normalize_entities( $node->title )

That is not escaping, but the function is part of esc_html().

@jonsurrell commented on PR #7876:


10 months ago
#8

The GitHub user that created this PR has been deleted. Closing the PR.

This ticket was mentioned in PR #11612 on WordPress/wordpress-develop by @dhrupo.


8 weeks ago
#9

  • Keywords has-patch has-unit-tests added

## Summary
Normalize HTML entities when rendering admin bar node titles without escaping valid markup used by core and plugins.

## Problem
WP_Admin_Bar::_render_item() outputs $node->title directly. This means text like This & that is rendered with a literal & instead of a normalized &amp;, while broad escaping approaches would break existing admin bar titles that intentionally include HTML.

## Solution
Use wp_kses_normalize_entities() when rendering the admin bar title. This normalizes entities in text content while preserving valid embedded markup.

## Testing
Added a PHPUnit regression test covering an admin bar title that includes both HTML markup and an ampersand.

Verified in the configured local wordpress-develop environment with:

  • node ./tools/local-env/scripts/docker.js exec --user wp_php php ./vendor/bin/phpunit --filter test_admin_bar_normalizes_title_entities_without_escaping_html tests/phpunit/tests/adminbar.php
  • node ./tools/local-env/scripts/docker.js exec --user wp_php php ./vendor/bin/phpunit tests/phpunit/tests/adminbar.php
Note: See TracTickets for help on using tickets.