HTML API: Ensure correct encoding of modified class names

[sirreal]

Ensure the class name modifications preserve correct HTML encoding when calling add_class and get_updated_html repeatedly.

There was an inconsistency in this branching where $existing_class would get a correctly decoded attribute value if there was a value returned from get_enqueued_attribute_value() but would otherwise get raw, unparsed HTML if there was a class attribute in the HTML:

wordpress-develop/src/wp-includes/html-api/class-wp-html-tag-processor.php

Lines 2339 to 2350 in 3a87241

	$existing_class = $this->get_enqueued_attribute_value( 'class' );
	if ( null === $existing_class || true === $existing_class ) {
	$existing_class = '';
	}
	if ( false === $existing_class && isset( $this->attributes['class'] ) ) {
	$existing_class = substr(
	$this->html,
	$this->attributes['class']->value_starts_at,
	$this->attributes['class']->value_length
	);
	}

Note that ::get_enqueued_attribute_value() returns a decoded value:

wordpress-develop/src/wp-includes/html-api/class-wp-html-tag-processor.php

Line 2734 in 3a87241

return WP_HTML_Decoder::decode_attribute( $enqueued_value );

This inconsistent behavior was hidden by the use of esc_attr() which avoids double-escaping character references, so when ::set_attribute() was later called it would only encoded character references that did not appear to already be encoded.

wordpress-develop/src/wp-includes/html-api/class-wp-html-tag-processor.php

Line 2479 in 3a87241

$this->set_attribute( 'class', $class );

When :set_attribute() was changed in r60919 (4892d46) to always escape attribute values, the double-escaping manifested.

WP_HTML_Tag_Processor and WP_HTML_Processor may incorrectly encode class names containing the characters &, <, >, ", or ' when modifying them via class methods like ::add_class() and calling ::get_updated_html().

For example:

$p = new WP_HTML_Tag_Processor('<div></div>');
$p->next_tag();
$p->add_class('&');
echo $p->get_updated_html() . "\n";
$p->add_class('OK');
echo $p->get_updated_html() . "\n";

Will print:

<div class="&amp;"></div>
<div class="&amp;amp; OK"></div>

Notice that the first pass is correct, & has been correctly encoded in the class attribute as &. However, after calling ::add_class() and ::get_updated_html() again, the & has incorrectly been double-encoded as &.

The same code in WordPress 6.8 would print:

<div class="&amp;"></div>
<div class="&amp; OK"></div>

This is related to r60919 that was released in WordPress 6.9. The double-encoding behavior was present before, but it was "corrected" in this case by the use of esc_attr() that avoids any double-encoding. When esc_attr() usage was removed in r60919, the double-escaping behavior manifests causing this issue.

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

Originally reported in WordPress/gutenberg#73713.

---

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

[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

• The Plugin and Theme Directories cannot be accessed within Playground.
• 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.

[sirreal]

These tests seem to have been ensuring incorrect behavior.

In both cases updated here an input class like poetry> (decoded value poetry>) was changed to poetry> (decoded value poetry>).

They were updated in #10143. This change restores their original (correct) assertion.

[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 jonsurrell, dmsnell.

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

[dmsnell]

I’ve been staring at this for a bit now and have no idea how we missed it. Surely we didn’t overlook this since 6.2 in the original introduction?

but you know what? we did!

php > $p = new WP_HTML_Tag_Processor( '<div class="&amp; &#x61;bc">' );
php > $p->next_tag();
php > $p->add_class( '&' );
php > $p->add_class( 'abc' );
php > echo $p->get_updated_html();
<div class="&amp; &#x61;bc &amp; abc">
php > $p->remove_class( 'abc' );
php > echo $p->get_updated_html();
<div class="&amp; &#x61;bc &amp;">
php > $p->remove_class( '&' );
php > echo $p->get_updated_html();
<div class="&amp; &#x61;bc &amp;">

now I don’t remember why; maybe it was performance-related or maybe it was a choice to lean on already-existing bugs, but I suppose it’s proper here to fix it regardless.

[sirreal]

Yes, there's been a latent issue for a while that was covered up by the behavior of esc_html(). It's become very clear with the change in 6.9 to use more rigorous attribute encoding.

[sirreal]

@ktmn this is a proposed fix for WordPress/gutenberg#73713.

[dmsnell]

It’d be nice to have performance evaluations of this, but that could be hard. It’s not directly in the hot path, and only happens when flushing updates. Either way, I prefer correctness than raw performance, and WP_HTML_Decoder::decode_attribute() is already checking for the presence of & so in most calls will be little more than a noop.

I tested and confirmed the broken behavior, that it originally appeared in 6.2 with the inception of the Tag Processor, and that the test is corrected. The theory checks out, so this should be solid.

[github-actions]

A commit was made that fixes the Trac ticket referenced in the description of this pull request.

SVN changeset: 61346
GitHub commit: 74b60c2

This PR will be closed, but please confirm the accuracy of this and reopen if there is more work to be done.