Make WordPress Core

Opened 10 days ago

Last modified 7 days ago

#62619 new defect (bug)

Remove `wp_kses_post()` filtering from admin notices

Reported by: azaozz's profile azaozz Owned by:
Milestone: 6.8 Priority: normal
Severity: normal Version: 6.4
Component: Administration Keywords: has-patch
Focuses: Cc:

Description

Follow-up to #57791.

There are several reasons why KSES filtering is not appropriate/not needed for admin notices. As far as I see the top three are:

  1. KSES is designed to run only when filtering HTML on saving to the database. It is not suitable for use when displaying content as it is slow and cumbersome.
  2. The wp_kses_post() function is designed specifically to be used when post content is saved to the database. It is not suitable for anything else.
  3. The content of the admin notices is provided (hard-coded) by WordPress or by plugins. It doesn't make sense to limit the use of HTML in these notices.

Change History (5)

#1 @azaozz
10 days ago

Can see that several bugs resulting from the filtering of the admin notices HTML were reported on the original ticket: #57791. Unfortunately the fixes for them were added as workarounds/exceptions instead of removing the actual cause. There are more reports of broken functionality, see #62606.

Unfortunately I cannot see why and how it was decided to filter the admin notices with wp_kses_post(), and what is being fixed by that. This doesn't seem to have been discussed there?

In any case I think that any type of filtering of text or HTML that is hard-coded and provided directly by WP core or plugins is not a goop idea. It doesn't serve any purpose, slows things down, limits use cases, and in this case breaks backwards compatibility.

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


8 days ago
#2

  • Keywords has-patch added; needs-patch removed

Trac ticket:

#3 follow-up: @yogeshbhutkar
8 days ago

Hi @azaozz,

Thank you for raising the ticket! I've added an initial patch to address the removal of wp_kses_post in the wp_admin_notice function as outlined.

As expected, this change impacts some test cases since the security checks previously handled by wp_kses_post are now less restrictive. Specifically, the following test cases in the wpAdminNotice.php file will fail and may need to be removed:

  1. Notices with unsafe types.
  2. Notices with unsafe IDs.
  3. Notices with unsafe class additions.
  4. Notices with invalid additional attributes.
  5. Notices with multiple attributes, including "role," invalid, "data-*," numeric, and boolean.

These cases were previously managed by wp_kses_post, and as noted in the ticket, "It doesn't make sense to limit the use of HTML in these notices." This means such scenarios will now need to be handled manually going forward.

On the bright side, this change resolves the issue described in ticket #62606, which is a positive step forward.

Could you confirm if it’s acceptable to address the failing test cases (primarily removing them) as part of this PR? I’m happy to proceed accordingly.

Looking forward to your feedback!

#4 in reply to: ↑ 3 ; follow-up: @azaozz
7 days ago

Replying to yogeshbhutkar:

As expected, this change impacts some test cases since the security checks previously handled by wp_kses_post are now less restrictive

Yea, several of the tests in Tests_Functions_WpAdminNotice won't pass after reverting that change. I wouldn't call these "security" tests. They just confirm how the HTML is restricted in admin notices.

Could you confirm if it’s acceptable to address the failing test cases (primarily removing them) as part of this PR?

Yes, thinking these should eventually be removed.

There may be more to it though. I'd really want to hear from the contributors that worked to implement #57791 why these restrictions were added in the first place. Also any links to any discussions regarding the restricting of HTML added by core and plugins would be very nice.

Frankly I don't see why plugins should be forbidden to do:

wp_admin_notice( '<script>anything goes here</script>' );

when they can so easily do something like:

add_action(
    'admin_notices', // 'in_admin_header', 'admin_head', or any other hook that fires while HTML is outputted.
    function() {
        echo '<script>anything goes here</script>';
    }
);
Last edited 7 days ago by azaozz (previous) (diff)

#5 in reply to: ↑ 4 @yogeshbhutkar
7 days ago

Replying to azaozz:

Also any links to any discussions regarding the restricting of HTML added by core and plugins would be very nice.

Hello @azaozz,

Thank you for your response. I came across a discussion regarding the use of wp_kses_post in the pull request (https://github.com/WordPress/wordpress-develop/pull/4119/files#r1272979830) that might be of interest.

Note: See TracTickets for help on using tickets.