WP_HTML_Tag_Processor doesn't allow to set a valid image src

[ivanzhuck]

WP_HTML_Tag_Processor doesn't allow to set inline encoded SVG as a source for IMG tag. It appeared in WordPress version 6.6

How to reproduce:

$newSrc = "data:image/svg+xml,%0A%3Csvg width='100' height='100' viewBox='0 0 100 100' fill='none' xmlns='http://www.w3.org/2000/svg'%3E%3Crect width='100' height='100' fill='%23D9D9D9'/%3E%3C/svg%3E%0A";

$processor = new WP_HTML_Tag_Processor( '<img src="something">' );
$processor->next_tag('IMG');
$processor->set_attribute('src', $newSrc);
$outputHtml = $processor->get_updated_html();

// $outputHtml contains <img src=""> instead of <img src="data:image/svg+xml...">

[dmsnell]

Thanks for the report, @ivanzhuck.
This is a result of bringing in esc_url() to set_attribute() in [58472] and [58473].

Hopefully this will improve as we internalize the security guards, but in the meantime, it should return false if WordPress rejects the update. This could make for a quick patch if you want to propose one.

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

## What:
This pull request updates the set_attribute method in WP_HTML_Tag_Processor to properly handle inline data URIs, such as SVGs.

## Description
Previously, data URIs were altered or stripped due to the use of esc_url(), causing issues with attributes like src when using data URIs. This fix ensures data URIs are preserved as intended.

Hi @dmsnell Can you please review this PR.
Thanks!

[dmsnell]

Noting here since my comment from Github didn't come over. Thanks for the patch @amitraj2203.

I prefer that we reject this patch as proposed, which provides a way to avoid attribute escaping, and change it so that when WordPress rejects an attribute update, that the function returns false to indicate as much.

In time, I hope to remove all esc_ functions from the core HTML API with a much better design for sanitization, so for now I think it's best if we stay minimal and do all we can to avoid blurring parsing and sanitization.

Thanks for the review. I have made some changes. Could you please take another look?

[dmsnell]

In 58844:

HTML API: Indicate when WordPress rejects attribute updates.

When setting an an attribute value in the HTML API, WordPress may reject
an update based on rules in kses. In these cases, the return value from
an escaping function will be an empty string, and the HTML API should
reject the update. Unfortunately, it currently reports that it updates the
attribute but sets an empty string value, which is misleading.

In this patch, the HTML API will refuse the attribute update and return
false to indicate as much when WordPress rejects the updates.

Developed in ​https://github.com/wordpress/wordpress-develop/pull/7114
Discussed in https://core.trac.wordpress.org/ticket/61719

Follow-up to [58472].

Props: amitraj2203, dmsnell, mukesh27.
Fixes #61719.

Merged in [58844]
​https://github.com/wordpress/wordpress-develop/commit/0c46e2a0b4803a0a7e7c9b0abf1cbb7811019a1f

[dmsnell]

Re-opening for a backport into 6.6.2.

[ivanzhuck]

@dmsnel, thank you for the fix. But it doesn't solve the issue. The HTML API still doesn't allow to use data-encoded images as values for the src atribute. It's a valid HTML code, so it looks wrong that HTML API doesn't accept it anymore. It worked perfectly before the version WP 6.6.

[dmsnell]

@ivanzhuck that's correct, but as for now it's relying on WordPress' own sanitization. the plan is to improve that sanitization with time, but I believe that this is no worse than running this code, or am I missing something?

<?php

echo '<img src="' . esc_url( $data_uri ) . '">';

Eventually there will be a split between something like a HTML_Tag_Processor/HTML_Processor and WP_HTML_Tag_Processor/WP_HTML_Processor will become subclasses of those, which apply WordPress' own rules. or maybe we'll be able to fix kses to ensure safe data URIs.

The overarching goal leading to the delayed full support is ensuring that the default behaviors are safe. Today that's done by running the attributes through WordPress' sanitization.

[hellofromTonya]

​Discussed in today's 6.6.2 bug scrub. Summary:

• Does [58844] fix this issue?

No, but it is fixing a case where it needs to bail out (reject an update) as part of [58473] which was introduced in 6.6 cycle.

@dmsnell shared:

yeah it was a bug fix but for a different nuance than the report
more like ensuring that the HTML API is honest when it rejects an update
...
the HTML API doesn't promise to allow arbitrary changes, and was already designed to return false when a change wasn't accepted

[58844] makes sense to me as a follow-up fix.

As a confidence check, checking in with @jorbin, who contributed to [58473]. Do you have any concerns?

[jorbin]

I've tested and reviewed [58844] and it is good for backport to the 6.6 branch.

[hellofromTonya]

Thanks @jorbin.

I'm in the process of backporting it now.

[hellofromTonya]

In 58980:

HTML API: Indicate when WordPress rejects attribute updates.

When setting an an attribute value in the HTML API, WordPress may reject
an update based on rules in kses. In these cases, the return value from
an escaping function will be an empty string, and the HTML API should
reject the update. Unfortunately, it currently reports that it updates the
attribute but sets an empty string value, which is misleading.

In this changeset, the HTML API will refuse the attribute update and return
false to indicate as much when WordPress rejects the updates.

Reviewed by jorbin, hellofromTonya.
Merges [58844] to the 6.6 branch.

Follow-up to [58472].

Props amitraj2203, dmsnell, mukesh27.
Fixes #61719.

[Hrohh]

You can set

$processor->set_attribute( 'src', "//:0" );