HTML API: Refactor wp_get_admin_notice()

[dmsnell]

Trac ticket: Core-65984

Refactors wp_get_admin_notice() to rely on the HTML API for HTML generation. This provides more reliable parsing and hardening for and against the provided arguments, but introduces behavioral changes that should only affect already-broken cases.

Namely, the function previously generated its output and then mandated that calling code run it through wp_kses(). This meant that certain kinds of corruption were possible which would break boundaries within the generated HTML and result in mangled output through wp_kses(). Now, the function preserves all boundaries when generating the output, meaning that wp_kses() will receive already-normalized content. As part of this change, the $message parameter runs through wp_kses() in advance. This may lead to some existing static message values changing from what they previously generated.

[github-actions]

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

• All changes will be lost when closing a tab with a Playground instance.
• All changes will be lost when refreshing the page.
• A fresh instance is created each time the link below is clicked.
• Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
  it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props dmsnell, joedolson, johnbillion, jorbin.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[joedolson]

The biggest back compat risk here is in stripping form elements. It's relatively common to pass form data into a notice if an extender is looking to have the user take an action as part of the notice.

This removes a capability that previously existed by using a custom wp_kses filter on the echoed results from wp_get_admin_notice().

This can be gotten around, but makes it more difficult to limit the scope for when these elements are allowed.

[dmsnell]

thanks @joedolson for noting this. I left e9e6b6f as a separate commit because I was reluctant on this point.

@johnbillion raised a question about normalizing without sanitizing, which is the goal here. we could replace this with WP_HTML_Processor::normalize( $message ) and that would leave the existing sanitization domain while improving wp_kses() ability to parse the message.

[dmsnell]

I think that for now I will revert this wrapping change, which still leaves the function more reliable than it was before, but doesn’t go as far as it could.

[johnbillion]

I've just noticed that wp_admin_notice() passes the notice through wp_kses_post(). Does that need to be removed if KSES is moving into wp_get_admin_notice()?

[aaronjorbin]

Generally, this looks good to me. Left a few notes, none are blockers.

[aaronjorbin]

Aside: This feels like it could be a good helper function. grep -rnE --include='*.php' "str_contains\(.*,[[:space:]]*' '[[:space:]]*\)" . found 5 additional places where we do str_contains on an empty string.

[dmsnell]

it’s an abstraction that has been sitting in the wing, so to speak. one thing we discovered was that we have to be attentive to where we apply the \f form feed character. in HTML-sourced strings it’s part of whitespace, but in CSS content it’s not.

this means it’s different for class names going into HTML vs. class names going into a block attribute as JSON.

wp_contains_html_whitespace() and wp_contains_css_whitespace() could do it, but then we also have wp_contains_linear_whitespace() from RFC specs…and I just don’t know if the abstractions are actually worth it, which is why I haven’t pushed any.

this is good thought; my comment here is mostly to share the context of what has kept it from being born yet.

[dmsnell]

@aaronjorbin some context we have from the HTML API development, which I mostly maintain as practice these days, is that abstracting low-level text operations like this is a hefty operation which showed up surprisingly well in performance benchmarking for the HTML API. it’s not likely to be in the hot-path here, so the risk is lower, but there’s an outsized impact on abstracting these calls compared to languages whose compilers inline them, or languages with cheap function calls (PHP’s remain fairly expensive, for reasons I don’t understand)

[aaronjorbin]

I assume that add_class won't have an issue if an empty string is passed to it? Before it would just mean extra white space.

[dmsnell]

yeah that’s a good point to consider inside the HTML API. we could call _doing_it_wrong() like we do with set_attribute(), but the operation is benign.

in fact, if no existing classes are on the item, it won’t even add the class attribute when an empty class name is passed.

php > require __DIR__ . '/src/wp-load.php';
php > $p = new WP_HTML_Tag_Processor( '<div>' );
php > $p->next_token();
php > var_dump( $p->add_class( '' ) );
bool(true)
php > var_dump( $p->get_updated_html() );
string(5) "<div>"
php > var_dump( $p->add_class( 'bar' ) );
bool(true)
php > var_dump( $p->get_updated_html() );
string(17) "<div class="bar">"
php > var_dump( $p->add_class( null ) );
bool(true)
php > var_dump( $p->get_updated_html() );
string(18) "<div class="bar ">"
php > var_dump( $p->add_class( [] ) );
PHP Warning:  Uncaught TypeError: Cannot access offset of type array on array in /Users/dmsnell/code/WordPress-develop/src/wp-includes/html-api/class-wp-html-tag-processor.php:4824
Stack trace:
#0 php shell code(1): WP_HTML_Tag_Processor->add_class(Array)
#1 {main}
  thrown in /Users/dmsnell/code/WordPress-develop/src/wp-includes/html-api/class-wp-html-tag-processor.php on line 4824

Warning: Uncaught TypeError: Cannot access offset of type array on array in /Users/dmsnell/code/WordPress-develop/src/wp-includes/html-api/class-wp-html-tag-processor.php:4824
Stack trace:
#0 php shell code(1): WP_HTML_Tag_Processor->add_class(Array)
#1 {main}
  thrown in /Users/dmsnell/code/WordPress-develop/src/wp-includes/html-api/class-wp-html-tag-processor.php on line 4824

some of these deep internals do pass non-strings at times when we expect them. we might add hardening, though I think that in this case we have a similar behavior as the legacy sprintf() had

[dmsnell]

Does that need to be removed if KSES is moving into wp_get_admin_notice()?

@johnbillion theoretically wp_kses() should be idempotent and running an input through it twice should yield the same result. Obviously that’s not the case today, but when mixed with #13271 I think it will be. It’s mostly true today at least, and especially so for normative well-formed markup.

[dmsnell]

because of what @joedolson brought up, and because I was already unsure of whether we really want to run the message through wp_kses() I have removed the commit where that was taking place. this leaves the $message behavior the same as it previously was, but improves the reliability of the attribute handling.

[dmsnell]

Merged in [63380]
25a91cb