WP_HTML_Tag_Processor overview issue

[adamziel]

• Part of Block API #41236.

The follow-up work once the #42485 is merged:

• Handle HTML entities for the purposes of get_attribute and set_attribute. Define what should happen in cases such as $p->set_attribute( 'id', 'My‐id' ); and $p->get_attribute( 'id' ) when id="My‐id".
  • Add/html character reference decoder #47040
  • Tag Processor: Document and test XSS prevention in set_attribute #44447
  • Tag Processor: throw when supplied unacceptible attribute names. #44431
  • Decide whether on* attributes should be banned in set_attribute( ) to make adding inline JavaScript harder (see @peterwilsoncc's comment and the reasoning behind not banning them).
• Improve the documentation
  • A handbook page
  • New HTML Tag Processor - An in-depth tutorial developer-blog-content#75
  • Review the documentation blocks.
  • Improve description for keys in test data providers that use numeric values (example).
  • Tag processor: update @since version tags to 6.2.0 #44432
  • Expand class-level documentation for WP_HTML_Tag_Processor #44478
• New APIs
  • WIP: Introduce class for sourcing block attributes from HTML #46345
  • Tag Processor: Add bookmark invalidation logic #47559
  • WP_HTML_Processor: Add set_content_inside_balanced_tags #47036
  • Explore HTML parsing and Adoption Agency Algorithm adamziel/wordpress-develop#1
  • WP_HTML_Tag_Processor: Allow non-attribute lexical updates #47068
  • Explore adding a has_next_tag method
  • Tag Processor: Add bookmark system for tracking semantic locations in document #46018
• Basic API
  • Tag Processor: Remove the shorthand next_tag( $tag_name ) syntax #45082
  • Tag Processor: Add get_updated_html as a non-toString method of stringifying the markup #44597
  • Add a has_class method to the public WP_HTML_Tag_Processor API #46232 (rationale)
    (Try adding layout classnames to inner block wrapper #44600 (comment)))
• HTML Tag Processor: Add WP 6.3 compat layer #47933
• Find a canonical way of stringifying the processor. One that's different from (string) $w (related comment). Done: @w->get_updated_html()

Code is located in two folders:

• https://github.com/WordPress/gutenberg/tree/trunk/lib/experimental/html
• https://github.com/WordPress/gutenberg/tree/trunk/phpunit/html

cc @gziolo @dmsnell @georgeh

[azaozz]

Frankly I think #42485 was not ready for merging. Seems it lacks any security features, references, docs, how-to's, etc. Even doesn't mention what security and escaping requirements it has. It seems open to exploits in its current state.

This issue mentions some of that, but misses few things. For example validity of new attribute names. Also, it doesn't mention the needed security related inline docs and examples which are crucial for new APIs.

As I commented on #42485 (comment) the security side of this API still awaits a decision. Imho it needs to implement some restrictions that will make is much more robust and problems-free in the future, and now is the time for these restrictions. If this is not done (in time) there is a risk that WP will end up with another exploits-prone API just like the shortcodes. If you don't believe me please see the huge, slow, cumbersome regex-based shortcodes sanitization functions that had to be added in order to try to secure badly written, exploitable code in plugins. Please consider restricting the illegal chars in attribute names and html-entity encoding the htmlspecialchars in attribute values!

Also see #42485 (comment).

[adamziel]

@azaozz – #42485 was merged to continue iterating in smaller PRs, not because every kink was worked out. I agree with your points about html-entity encoding and restricting illegal chars. The former is now merged, the latter is in motion. The list of todos also reflects updating the docs.

About this point:

Decide whether on* attributes should be banned in set_attribute( ) to make adding inline JavaScript harder (see @peterwilsoncc's comment).

I don't think there's an effective way to do that. Say we restrict onclick. The developer can just work around it like this:

$p->set_attribute( '__onclick', 'alert()' );
$markup = str_replace( '__onclick', 'onclick', $p );

Notably, the processor does not allow injecting new tags, so there's no risk of injecting a rogue <script> tag. I'll mark that todo as completed for now – if anyone disagrees please speak out.

[adamziel]

Find a canonical way of stringifying the processor. One that's different from (string) $w.

A getter like $p->getUpdatedHTML() or a magic property like $p->updatedHTML could do the trick here. I mean that in addition to having __toString(), not as a replacement.

[adamziel]

Wild idea: Tag Processor could use seekable PHP streams instead of strings and internal pointers:

php > $fp = fopen('php://memory', 'w+');
php > fwrite($fp, '<a href="');
php > fwrite($fp, 'http://wordpress.org/');
php > fwrite($fp, '">Link</a>');
php > fseek($fp, 0);
php > echo fread($fp, 1024);
<a href="http://wordpress.org/">Link</a>

Potential benefits: Simpler, faster code that uses less memory (if we can get rid of $updated_html). This needs more investigation.

[adamziel]

6.2 dev note draft below. @dmsnell @gziolo @ockham feel free to edit this comment and adjust as you see fit:

---

WordPress 6.2 ships with WP_HTML_Tag_Processor – a tool to adjust HTML tag attributes in the block markup:

$p = new WP_HTML_Tag_Processor( '<a href="#"></a>' );
$p->next_tag( 'a' );
$p->remove_attribute( 'href' );
echo $p->get_updated_html();
// <a></a>

Before WordPress 6.2, the block markup was typically updated using regular expressions or the DOMDocument class. Both have downsides. The former is tedious and prone to security issues, while the latter uses libxml2 which does not support HTML5.

WP_HTML_Tag_Processor is safe and efficient. Unlike full-fledged HTML parsers, the processor avoids handling malformed markup, semantic problems, and building a document tree. Any problems that are present on the input are passed on to the browser. The processor doesn’t fix HTML just as it won’t break HTML.

The tradeoff is that it only offers a simplified API to modify HTML attributes. If you want to replace an img tag with a full-fledged figure layout, this API won’t offer that functionality. Similarly, the processor won’t help you replace all the child nodes of a particular div with a completely new markup. This system is focused on finding specific HTML tags and adding, removing, or updating the attributes on those tags.

Here's how to use WP_HTML_Tag_Processor:

Remove the href attribute from an anchor tag:

$p = new WP_HTML_Tag_Processor( $html );
$p->next_tag( 'a' );
$p->remove_attribute( 'href' );

Add a style attribute to the first tag in the document:

$p = new WP_HTML_Tag_Processor( $html );
$p->next_tag();
$p->set_attribute( 'style', 'display: none' );

Add a CSS class to the first tag having the wp-block-media-text__content class:

$p = new WP_HTML_Tag_Processor( $html );
$p->next_tag( array(
    'class_name' => 'wp-block-media-text__content'
) );
$p->add_class( 'wp-foo-bar' );

Add the srcset attribute to all image tags:

$p = new WP_HTML_Tag_Processor( $html );
while ( $p->next_tag() ) {
    if (
        isset( $p->get_attribute( 'src' ) ) &&
        ! isset( $p->get_attribute( 'srcset' )
    ) {
        $srcset = build_srcset( $p->get_attribute( 'src' ) );
        $p->set_attribute( 'srcset', $srcset );
    }
}

For additional documentation, refer to GitHub Pull Request. If you want to learn more about the motivation for this new API, check the post that proposed A new system for simply and reliably updating HTML attributes.

[gziolo]

I reviewed the listed todo items, and I have an impression that we can close this issue after landing HTML Tag Process in WordPress core. If there are any remaining issues, we can open issues that target individual use cases.

Find a canonical way of stringifying the processor. One that's different from (string) $w (#44410 (comment)).

There is get_updated_html method that fulfills the requirement.

Review the documentation blocks.

This happened as part of the WP core merge.

Improve description for keys in test data providers that use numeric values (example).

Fixed in https://github.com/WordPress/wordpress-develop/blob/39bfc2580d9b0ea7e6ff45b8d904408d925cbc4b/tests/phpunit/tests/html/wpHtmlTagProcessor.php

Explore adding a has_next_tag method (#44600 (comment))

Is that still necessary? Could we use the bookmarking system instead?

[adamziel]

@gziolo I refreshed the list of related issues – there's still some work related to the character decoding, the documentation, and the bookmarks API.

[gziolo]

@gziolo I refreshed the list of related issues – there's still some work related to the character decoding, the documentation, the bookmarks API.

Thank you for updating the description with the recently started tasks.

A handbook page

Documentation is going to be automatically generated from PHPDoc comments for all classes and methods. It will be exposed in the Code Reference section of Official WordPress Developer Resources, similar to for example WP_Block_Type class. What type of handbook page you would like to include in addition to that? Is that in the Block Editor Handbook?

In the codebase, there is the following comment included:

### Possible future direction for this module
 *
 *  - Prune the whitespace when removing classes/attributes: e.g. "a b c" -> "c" not " c".
 *    This would increase the size of the changes for some operations but leave more
 *    natural-looking output HTML.
 *  - Decode HTML character references within class names when matching. E.g. match having
 *    class `1<"2` needs to recognize `class="1&lt;&quot;2"`. Currently the Tag Processor
 *    will fail to find the right tag if the class name is encoded as such.
 *  - Properly decode HTML character references in `get_attribute()`. PHP's
 *    `html_entity_decode()` is wrong in a couple ways: it doesn't account for the
 *    no-ambiguous-ampersand rule, and it improperly handles the way semicolons may
 *    or may not terminate a character reference.

Does #47040 from the list cover points 2 and 3? It looks like we still need an issue for point 1 about removing whitespaces.

---

There is also an unlisted PR that introduces a new class for sourcing block attributes that gets referenced in the PR from the section with new APIs:

#46345

I see that some listed tasks reference or extend that work, they explore opening modifications to chunks of HTML:

• WP_HTML_Processor: Add set_content_inside_balanced_tags #47036
• WP_HTML_Tag_Processor: Allow non-attribute lexical updates #47068
• Tag Processor: Add bookmark invalidation logic #47559

What do you all see as a top priority and how people can get involved to help with efforts?

[adamziel]

Documentation is going to be automatically generated from PHPDoc comments for all classes and methods. It will be exposed in the Code Reference section of Official WordPress Developer Resources, similar to for example WP_Block_Type class. What type of handbook page you would like to include in addition to that? Is that in the Block Editor Handbook?

I'm thinking of a proper guide on how to use WP_HTML_Tag_Processor. API docs don't quite cut it.

Does #47040 from the list cover points 2 and 3?

Point 3 – yes.
Point 2 – potentially? I'm not sure – it requires a deeper look on how CSS classes are read from the attributes. @dmsnell may know off the top of his head.

There is also an unlisted PR that introduces a new class for sourcing block attributes that gets referenced in the PR from the section with new APIs:

Good spot 👍 Added to the list

What do you all see as a top priority and how people can get involved to help with efforts?

I think the two open PRs may be blockers for the draft PR. Can you confirm @ockham ? If yes, then those would be the priority to me. Then the set_content one and the CSS selector one.

[dmsnell]

Does #47040 from the list cover points 2 and 3?

Yes, completely. There's an enhancement possible noted in the decoder class which is adding a method like "decoded_strpos" or "decoded_has_substring" but when I was testing the performance wasn't impacted by calling decode on every call, since this only does something when character references are present, and in those cases (which are uncommon) we have to start decoding anyway.

[ockham]

I see that some listed tasks reference or extend that work, they explore opening modifications to chunks of HTML:

• WP_HTML_Processor: Add set_content_inside_balanced_tags #47036
• WP_HTML_Tag_Processor: Allow non-attribute lexical updates #47068
• Tag Processor: Add bookmark invalidation logic #47559

What do you all see as a top priority and how people can get involved to help with efforts?

I think the two open PRs may be blockers for the draft PR. Can you confirm @ockham ? If yes, then those would be the priority to me. Then the set_content one and the CSS selector one.

Yes, that's correct. Plus the non-attribute updates one depends on the bookmark invalidation one. So currently, it's a cascade of dependencies, including one more PR that I filed only recently. They are, in order:

1. HTML Tag Processor: Add WP 6.3 compat layer #47933
2. Tag Processor: Add bookmark invalidation logic #47559
3. WP_HTML_Tag_Processor: Allow non-attribute lexical updates #47068
4. WP_HTML_Processor: Add set_content_inside_balanced_tags #47036

[adamziel]

Surfacing my explorations of parsing HTML into the correct tree structure, e.g. <p><p>Lorem -> <p></p><p>Lorem</p>

adamziel/wordpress-develop#1

[gziolo]

We can close this issue as done. The work continues in WordPress Trac:

HTML API: Introduce HTML Processor, a higher-level partner to the Tag Processor

The list of open tickets is grouped under HTML API component.

There still are two open PRs:

• Add/html character reference decoder #47040 – should we create a ticket in Trac for this one?
• WIP: Introduce class for sourcing block attributes from HTML #46345 – I believe this one requires the full HTML API, does it deserve a Trac ticket, too?

Explore adding a has_next_tag method

This item is pretty vague, and it seems to be nice to have at this stage.

• New HTML Tag Processor - An in-depth tutorial developer-blog-content#75 is tracked elsewhere.

[dmsnell]

Thanks @gziolo.

The two open PRs are good to stay open here. I don't know if we need a Trac ticket or not because I expect them to remain open for a while. #46345 will likely never merge but only serve as a guide to what we end up merging (which is one of the reasons I keep these open).

has_next_tag doesn't make sense to me at the moment. I'm not even sure where that came from. With bookmarks we don't need it, and has_next_tag implies peeking ahead of the parser, which we don't currently do.

[strarsis]

Can the existing WP_HTML_Tag_Processor API be used to replace an HTML element with another one?
Can an additional WP_HTML_Tag_Processor instance be created from additional HTML and then be used to replace a tag from another WP_HTML_Tag_Processor instance?

[dmsnell]

@strarsis the Tag Processor doesn't support replacing any tags, but the HTML Processor (which is being built on top of it) will support that. because of the complexities of the semantic rules involved in replacing an element in HTML though, this is coming at a slower pace.