Remove `wp_kses_post()` filtering from admin notices

[azaozz]

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.

[azaozz]

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.

Trac ticket:

[yogeshbhutkar]

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!

[azaozz]

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>';
    }
);

[yogeshbhutkar]

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.

[peterwilsoncc]

As the kses call was included when the function was introduced in [56408], I'm concerned that it's too late to remove it as third party developers may have assumed that it was safe to pass user input to the function as it escapes the output.

[azaozz]

Replying to yogeshbhutkar:

I came across a discussion regarding the use of wp_kses_post...

Thanks! Unfortunately it doesn't mention anything about why the text/html for admin notices has to be filtered and what is the reasoning for such decision.

[azaozz]

Replying to peterwilsoncc:

I'm concerned that it's too late to remove it as third party developers may have assumed that it was safe to pass user input to the function as it escapes the output.

Yea, good point. It seems it would be a really bad decision for a plugin to store and/or output any user input without sanitizing or escaping it, but that has been in core for some time and should stay in case a plugin would do such silly stuff :)

Seems to fix this the wp_admin_notice() function has to be deprecated and replaced by a new function that will work properly. It only echoes the output from wp_get_admin_notice() and runs an action that seems pretty useless as it repeats exactly the wp_admin_notice_markup filter. The name for the new function would probably be better as wp_print_admin_notice() or maybe wp_show_admin_notice().

[sabernhardt]

I do not think this would require creating a new function. Whenever the KSES is inappropriate, and the $message contents are properly sanitized, developers probably could use echo wp_get_admin_notice( $message ) instead. (I proposed that change for #62606.)

Other notes:

• At first, I thought about adding an argument for whether wp_admin_notice() sanitizes the content, with 'wp_kses_post' as the default. It could be even more elaborate with something like a $sanitize_callback argument, but even a simpler form of the argument seems unnecessary.
• A ​directory search of the `wp_admin_notice` action yielded zero results.

[joedolson]

Passing along some thoughts from @costdev, as the original author; I'll follow up with my own thoughts, after.

======

Whether the original intent of wp_kses_post() was only to sanitize before being sent to the database, ​Core, ​DevHub, ​WPCS, ​WPVIP all clearly either use or advise the use of wp_kses(_post)() when escaping output containing HTML. Original intent aside, the wide range of the above references clearly demonstrates a different understanding and usage in real-world scenarios.

It's not only user input to be escaped, but also error responses from external requests that may require escaping before output.

Requiring core and plugin authors to handle escaping themselves by default, rather than by exception, goes against the well-established practice of late escaping. It would also make filtering arguments and markup quite difficult.

The action isn't likely to appear in many public plugins. We always primarily intended it to be useful in cases such as logging fired admin notices.

There's ultimately no need to deprecate wp_admin_notice() or to change its established use of wp_kses_post() in line with well established convention in and out of core. If it's not suitable for a particular use by core or an extender, they can use wp_get_admin_notice(). At most, maybe a $sanitize_callback parameter could be added, but to my recollection, this is mostly seen in the REST API and would be a unicorn parameter in the context of admin notices/general HTML output.

[joedolson]

As I recall from discussion (and I can't find the discussion, so I can't be more detailed than that), the external error message case was the primary reason for wanting this; as much as most of the cases are known content, that is not the case 100% of the time. API requests are always going to be somewhat of an unknown.

Regarding the performance with wp_kses_post(), that's not a significant concern with short content, which is usually the case with admin notices. Zack Tollman did a ​review of `wp_kses()` performance comparing various types of content in (sheesh...2015) which showed that the performance hit was minor on short content. I would guess this is better now; but I'm not aware of a more recent study. Admin notices are also infrequent, so that performance hit is not something that will be executing a thousand times in a given screen view.

Ultimately, I'd probably want somebody from the security team to voice an opinion here.

[azaozz]

Replying to joedolson:

It's not only user input to be escaped, but also error responses from external requests that may require escaping before output.
...
Ultimately, I'd probably want somebody from the security team to voice an opinion here.

Sure, I am part of the security team :)
From purely security point of view the current code in wp_admin_notice() does not meet the requirements as it doesn't escape anything. The above is slightly incorrect. The code there just filters some of the HTML tags and allows others. From security point of view this is not good enough for untrusted content as the allowed tags can still be used to manipulate/deface the page where the user notice is shown.

Requiring core and plugin authors to handle escaping themselves by default, rather than by exception, goes against the well-established practice of late escaping.

What do you mean "escape by exception"? All plugins authors as well as all places in core are always required to escape untrusted content. This was the case with the user warnings for many years before wp_admin_notice() was introduced, right? Passing unescaped untrusted content to that function would still be considered a security risk as that that content is never really escaped (see above).

There's ultimately no need to deprecate wp_admin_notice() or to change its established use of wp_kses_post() in line with well established convention in and out of core.

Yea, perhaps another solution (apart from deprecation) can be found. However the use of wp_kses_post() in wp_admin_notice() is not inline with any WP conventions as it doesn't really provide anything useful. It does not provide adequate security for untrusted content. On top of that it is "dead code"/unneeded overhead in nearly all cases as the user warnings/messages text is hard-coded.

Edit: the "other solution" can probably be to introduce wp_admin_notice_unfiltered() then change wp_admin_notice() to use it. Something like:

function wp_admin_notice( $message, $args = array() ) {
	do_action( 'wp_admin_notice', $message, $args );

	$message = wp_kses_post( $message );

	return wp_admin_notice_unfiltered( $message, $args );
}

function wp_admin_notice_unfiltered( $message, $args = array() ) {
	do_action( 'wp_admin_notice_unfiltered', $message, $args );

	echo wp_get_admin_notice( $message, $args );
}

After that pretty much all admin notices in core can be changed to use the new function as the $message there is hard-coded and doesn't need any filtering.

[azaozz]

Replying to joedolson:

As I recall from discussion (and I can't find the discussion, so I can't be more detailed than that), the external error message case was the primary reason...

Sure, but was it the best solution to filter the HTML of countless hard-coded messages instead of escape (or filter if that was acceptable) the HTML only there?

Frankly the use of wp_kses_post() there does nothing. It is just some overhead that slows down WP a little bit. True, maybe that overhead is not big, but still some pointless code is run every time. This goes directly against the efforts of the performance team, and also changes the established way to show admin notices from before that function was introduced. You can see how many regressions were reported when it was committed, and how many individual edge case fixes had to be made.

I've been wondering: why is it that hard to see that a not-so-good decision was made at the time and just improve it? I really regret not being able to review this in time, but the code is not "set in stone" right? Anything and everything can be improved :)

[audrasjb]

As per today's bug scrub: given the lack of activity during the beta cycle of 6.8, I'm moving this ticket to the next milestone.