HTML API: Core parsing improvements for 7.2

[dmsnell]

This tracking ticket is for refactors to apply the HTML API in places where WordPress currently relies on PCRE-based or other ad-hoc parsers.

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.

Trac ticket: Core-65984

Merge after #13273, which accounts for three of the failing tests.

• [ ] self-closing non-HTML elements
• [ ] remove opening tag when required attributes are missing, and closing tag
• [ ] replace C0 controls with their escapes, rather than stripping them away
• [ ] handle incomplete parsing, including closing all open elements

[dmsnell]

In 63380:

HTML API: Refactor wp_get_admin_notice().

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.

Developed in: ​https://github.com/WordPress/wordpress-develop/pull/13273
Discussed in: https://core.trac.wordpress.org/ticket/65984

Props dmsnell, joedolson, johnbillion, jorbin.
See #65984.

Merged in ​[63380]
​https://github.com/wordpress/wordpress-develop/commit/25a91cb9724e0fbda50a0d281c8f84e34aa738a8

Trac ticket: Core-65984.

Rewrites the logic of wp_strip_all_tags() to rely on the HTML API rather than on strip_tags(), due to the challenges strip_tags() has with properly parsing HTML. The new version of the function returns renderable text nodes, normalized and re-escaped for more predictable inputs into downstream parsers.

### Notable changes

• The contents of SCRIPT and STYLE tags used to be lifted out of the elements, which led to data corruption, as these contents are not intended to be renderable with the rest of the text nodes. These are no longer included in the function outputs; the same is true for the “special atomic elements” defined by the HTML API (IFRAME, TEXTAREA, TITLE, etc…)
• Text contents are decoded and then re-encoded to normalize the use of character references.

Trac ticket: Core-65984

## Status

• [ ] Add some tests to demonstrate behavior.

## Description

Characters in TITLE and TEXTAREA aren’t required to be escaped, but doing so might prevent downstream parsers from mis-parsing content inside of these elements as markup, when in fact they are plaintext.

This patch escapes the content by default to prevent such a scenario.

[dmsnell]

In 63418:

HTML API: Escape syntax characters in RCDATA.

Characters in TITLE and TEXTAREA aren’t required to be escaped, but doing so might prevent downstream parsers from mis-parsing content inside of these elements as markup, when in fact they are plaintext.

This patch escapes the content by default to prevent such a scenario.

Developed in: ​https://github.com/WordPress/wordpress-develop/pull/13327
Discussed in: https://core.trac.wordpress.org/ticket/65984

Props dmsnell, jonsurrell, westonruter.
See #65984.

Merged in [63418]
​https://github.com/wordpress/wordpress-develop/commit/049287aeda8855d6c29af747839377d7669a9fe6

I'm supportive of this direction overall. I'll leave some feedback on different points.

I started this review at 0b1535e0f5974562a364fc6298a3fb87b5a9bb5e but it moved while I was reviewing, so some of this review may be stale by the time I post.

---

General things to debug:

echo wp_kses( '<template>foo</template>', [ 'template' => true ] );
// Prints: </template>

echo wp_kses( '<div style>x</div>', [ 'div' => ['style' => true] ] );
// Prints: <div style="1">x</div>

echo wp_kses( '<div style="col&#x01;or:red">x</div>', [ 'div' => [ 'style' => true ] ] );
// Before (trunk): <div>x</div>
// After         : <div style="color:red">x</div>

---

This is an interesting one, the block delimiter is preserved (improvement) but for some reason wp:image becomes wp:core/image:

echo wp_kses( '', [] );
// Before: <!-- wp:image {"alt":"x"} -->
// After:

---

_Incomplete tokens_ are interesting.

KSES would leave something like a trailing <b this is lost as HTML text (it would escape <). Browsers do not do that and this PR does not do that, the <b… is dropped.

That means that partial tokens like the unclosed <b above but also "atomic elements" are stripped entirely:

echo wp_kses( '<p>keep me</p><b this is lost', ['p'=>[],'b'=>[]] ) . "\n";
echo wp_kses( '<p>keep me</p><title>this is lost', ['p'=>[],'title'=>[]] ) . "\n";
echo wp_kses( '<p>keep me</p><style>*{content:"this is lost";}', ['p'=>[],'title'=>[]] ) . "\n";
echo wp_kses( '<p>keep me</p><style>*{content:"this is lost";}', ['p'=>[],'style'=>[]] ) . "\n";
echo wp_kses( '<p>keep me', ['p'=>[]] ) . "\n";

Before/after diff of output:

.txt

@@ -1 @@
-<p>keep me</p>&lt;b this is lost
-<p>keep me</p><title>this is lost
-<p>keep me</p>*{content:"this is lost";}
-<p>keep me</p><style>*{content:"this is lost";}
+<p>keep me</p>
+<p>keep me</p>
+<p>keep me</p>
+<p>keep me</p>
 <p>keep me

This is worth considering carefully. The HTML snippets KSES sees don't necessarily align with coherent boundaries. In particular echo wp_kses_post( 'a<b', [] ); is a<b on trunk (which seems reasonable) but on this branch its a.

We _could_ align with the previous behavior, assume that the incomplete tokens are HTML text, and escape them as such. There may be negative implications to that, it's worth some careful consideration.

---

Foreign content is difficult, is this a case of integration points bailing because they're risky?

echo wp_kses( '<math><mtext>x</mtext></math>', ['math'=>[],'mtext'=>[]] );
// (no output)

---

</br> produces warnings.

echo wp_kses( '</br>', [ 'br' => true ] );
// Warning: foreach() argument must be of type array|object, true given in /var/www/html/wp-includes/kses.php on line 1499
// Warning: foreach() argument must be of type array|object, null given in /var/www/html/wp-includes/kses.php on line 1536
// Prints: <br>

First warning is the allowed tags. I used true for the allowed tags value.

This change produces a warning where before it did not:
​https://github.com/WordPress/wordpress-develop/blob/0b1535e0f5974562a364fc6298a3fb87b5a9bb5e/src/wp-includes/kses.php#L1499

Maybe true is intentionally supported:

​https://github.com/WordPress/wordpress-develop/blob/049287aeda8855d6c29af747839377d7669a9fe6/src/wp-includes/kses.php#L1533-L1537

Either way, it's filterable and important to to follow whatever KSES did previously. I think if the value is true it should use an empty array (no attributes allowed).

Second warning is here:

​https://github.com/WordPress/wordpress-develop/blob/0b1535e0f5974562a364fc6298a3fb87b5a9bb5e/src/wp-includes/kses.php#L1536

As it is now, this needs a null guard. I had proposed a fix at the tag processor level in ​https://github.com/WordPress/wordpress-develop/pull/9657 that we could consider.

@sirreal this is great. thanks for the awesome review and findings.

• the TEMPLATE issue was silly; the code was decrementing the depth before processing the closing tags. I’ve resolved that by splitting the depth adjustment to before for openers and after for closers. TEMPLATE is still off by policy. we can open it up, but I haven’t done that yet, as I think it’s probably reasonable to not allow user-provided inputs to add them. for example, to supplant some TEMPLATE that might already be on the page otherwise.

• style="1" had to do with calling get_attribute() manually rather than relying on the normalized-to-string value. I’ve fixed that and also only run through style processing if the value is non-true. this also removes the C0 controls before processing.

• style="color;red" this seems like one of the many kinds of improvements occurring with this rewrite. what about this should be debugged?

• but for some reason wp:image becomes wp:core/image: that’s because it’s being re-serialized and the Block Processor returns a fully-qualified name. I wouldn’t call this a bug, but we could consider removing the core/, and potentially via a new method on the block processor like ->get_minimal_block_type(). this only happens on block delimiters whose JSON attributes are modified.

• for incomplete tokens I still consider this an improvement, and it goes back to our original rationale for pausing at incomplete tokens. in a browser, this stuff is even ignored. we can look at an example like a<b and think it makes sense to escape, but what about <!-- wp:para or <div class="wp-fullwidth attachment-id-15" data-wp-bi and such? those look far more like tags and comments that _should not_ be escaped and revealed on the page. in the case of the block delimiter, it would invite disclosure of something that was perhaps meant to be private on the server. thus, in the absence of any way to know what would have come next, truncating as a means of inevitable data loss feels preferable to showing syntax.

• the </br> case has been fixed. I’m not sure why it arose for that one in my testing and not for other tags. added if ( is_array() ) guards.

wp:image becomes wp:core/image

I was confused here because I expected it to happen across core blocks, but I didn't observe the same behavior with the paragraph block. I may not have been testing appropriately.

<div style="col&#x01;or:red">x</div>

I called this out just to make sure we know what's different and understand the implications. Was this passed through safecss_filter_attr() before and is it now? What explains the different handling?

• <div style="col&#x01;or:red">not red</div> is not red, but I guess the control character removing KSES does makes it <div style="color:red">not red</div> which _is_ red and makes sense to keep if we're removing control characters. (Note, this has : in the declaration, your reply had a typo ; between property and value).
• <div style="col&#x01;or;red">x</div> (with the ; instead of :) seems objectively better. Before it became or;red, now it's color;red which makes more sense. I suspect the lingering problem that there's no valid CSS declaration is a problem at another layer we can eventually address with improved CSS handling.

for incomplete tokens I still consider this an improvement

I _mostly_ agree that removing it is the right thing to do. I think it would be appropriate to escape the incomplete token as HTML in sometimes and drop it others, but that would require a defining where the incomplete token parsing and a set of rules for how to deal with each case. Defining those rules will require some arbitrary decisions that are likely to be surprising as often as not.

One important change that's worth worth documenting clearly is that _some_ elements must be properly closed, while _other_ elements do not need to be closed. This is especially important since we've seen examples where the KSES input starts with end tags. That implies that other KSES input may ends with unclosed tags.

Again, I think on the whole these are probably _good_ to remove, but may still be _surprising_. An example that lists the "atomic elements" defined by the tag processor and a few examples should help with this, e.g. <div>Everything after this is removed:<title>gone</div>

Improve support for content appearing after BODY, HTML nodes where
possible. Some HTML may be representable by the HTML API without
introducing ordering problems.

When HTML moves to the "after-{body,html,…}" insertion modes, BODY and
HTML remain on the stack of open elements. Certain content is inserted
in place (comments), some content is inserted in BODY (white space
text), and most other content will switch back to the "in body"
insertion mode.

The difficulty with the HTML API is that the result in the tree may be
out-of-order compared to the positioning of the corresponding tokens in
the HTML input text. For example:

</body>FOO

Produces this tree where the ordering is the same even though a BODY
closer is reached:

[DOCUMENT]

└HTML

├HEAD
├BODY
│ └#text: FOO
└#comment: comment

Compare with this case:

</body>FOO

The tree is identical, the FOO text is _out of order_ in the tree
compared to the HTML string. This case continues to be invalid because
when the FOO text needs to be reached, the parser would have already
reached a BODY tag closer and content outside of the body:

[DOCUMENT]

└HTML

├HEAD
├BODY
│ └#text: FOO
└#comment: comment

This approach deviates from the explicit steps in the spec and will pop
the BODY and HTML tags from the stack of open elements when content
should appear outside of them. Flags are set for each "after-" mode to
prevent returning to BODY and then continuing to produce content
outside, which would produce the unsupported out-of-order behavior.

See ​https://html.spec.whatwg.org/#parsing-main-afterbody.

Trac ticket: https://core.trac.wordpress.org/ticket/65984

core/image vs. image

I’ve normalized by removing the core/ namespace prefix.

<template> is always removed, is that correct? The Interactivity API often relies on template elements. It may be unusual for those to come from untrusted sources, but I'm concerned about blocking it entirely.

I’ve re-gated this to only block when TEMPLATE isn’t configured. In the earlier drafts, it was qualitatively different, but now, we are tracking foreign content and template content separately from how we track skipping content.

A major concern is that MathML is extremely restricted, most MathML markup I've tested is stripped because it quickly encounters integration points. I think this needs a more permissive solution because as-is it disallows a lot of potentially valid and conforming content that would previously pass.

That’s something we can iterate on. Fundamentally it’s only about how much accounting we duplicate here. I’d rather start more-locked-down and relax the rules as we can.

HTML character references are normalized. Often this is fine, but it can produce shortcode syntax where HTML character references avoided it.

Seems reasonable enough to escape, but also, the appropriate way to “escape” shortcodes is with the double-bracket syntax, like [[gallery]].

wp_remove_unwanted_c0_controls() seems to run on text nodes but not the contents of atomic elements. Those are mostly the same category so it's worth considering whether it should be applied there or not.

I really don’t like this. It gets really confusing when we start applying it to special atomic elements, which have their own unique escaping rules. I still think this was probably the wrong layer at which to address the problem: if we can audit XML generation we should be free to remove C0-controls-removal as it was originally intended to prevent generating broken XML.

One thing I'd consider is to completely remove known void HTML elements that don't satisfy the required attributes. That seems to align with the intended behavior.

This is what it’s supposed to be doing already, and I think it does. Are you seeing it fail to do that?

The problem with the existing behaviors and tests is that OBJECT is _not_ a void element, and wp_kses() is looking to the errant self-closing flag as a proxy for that.

If you change your test/example code to a void or special atomic element, or a self-closing foreign element, then it should fully remove it.

This should inject newlines into some HTML elements

Do you think this should be an easy thing to resolve? Just ensure that the modifiable text always starts with a newline? Or is the problem that we’re pushing out the decoded text and what we should be doing is building a custom <pre>#text</pre> and then chopping off the <pre> and </pre>?

In both cases, I'd expect the block name to remain unchanged. This is likely limited to Core blocks.

This is technically true; the serialization changes, but the block name remains unchanged. I’ve added the aesthetic change, but they are semantically equivalent. I understand the desire for aesthetics, but I’m not scoping this to require byte-identical idempotence. wp_kses() is likely not even idempotent in many places this isn’t. This is a good goal, but I would not want us to stall the work simply because of neutral variations.

That's the situation right now on trunk

This is also the scope-level for merging this work. There are lots of things I don’t like about wp_kses() legacy behavior, but I don}t want to demand that we fix everything at first. After this change, these things _should_ be considerably easier to update than they have been, since we have a structural approach to take here.

---

pushing updates in bfedf65535 which address your points unless stated otherwise.

[johnbillion]

The no-JS fallback for displaying a newly generated application password has broken since this change.

Affected code: ​https://github.com/WordPress/wordpress-develop/blob/2c6ec190fed195492439cd75177223bdf0812016/src/wp-admin/authorize-application.php#L206-L221

To reproduce:

1. Disable JavaScript in your browser
2. Visit /wp-admin/authorize-application.php
3. Enter a name and click Approve
4. Observe that the password is not shown because the input field gets stripped out of the admin message

[dmsnell]

@johnbillion thanks for noting this. it’s probably worth creating a new Trac ticket for that bug, because I just double-confirmed that the reported behavior is present going back to WordPress 6.4.0 when the message was wrapped in wp_admin_notice().

I tried this by running the HTML through wp_admin_notice() on an interactive PHP session, and then when. It consistently printed without the INPUT for as long as wp_admin_notice() existed, I downloaded the release ZIP and setup a new site, forced true for wp_is_application_passwords_supported() and for wp_is_application_passwords_available() and tested in my browser. The input field with the password was present when JavaScript runs, but absent when disabled. It seems that this might have been broken since three years ago this upcoming Sunday.

This should inject newlines into some HTML elements

Do you think this should be an easy thing to resolve? Just ensure that the modifiable text always starts with a newline? Or is the problem that we’re pushing out the decoded text and what we should be doing is building a custom <pre>#text</pre> and then chopping off the <pre> and </pre>?

A few elements ignore a first newline: TEXTAREA, PRE, LISTING. TEXTAREA is already covered by set_modifiable_html():

​https://github.com/WordPress/wordpress-develop/blob/f27699dd07b561435c923fad7db61a5898200187/src/wp-includes/html-api/class-wp-html-tag-processor.php#L4191-L4200

It should be covered on get_modifiable_text() for these elements, so the leading newline be stripped:

​https://github.com/WordPress/wordpress-develop/blob/f27699dd07b561435c923fad7db61a5898200187/src/wp-includes/html-api/class-wp-html-tag-processor.php#L3867-L3879

I think the best thing to do here for PRE and LISTING is to ​always inject a leading newline after their open tag, something like this:

$output .= $tag_maker->get_updated_html();
if ( 'html' === $namespace && 'PRE' === $tag_name || 'LISTING' === $tag_name ) {
  $output .= "\n";
}

There are some behavioral changes around unclosed blocks. Before, KSES would close them (either as void or with a closing delimiter):

Input	Before	After
in	in	in