Make WordPress Core

Opened 7 months ago

Closed 5 months ago

Last modified 4 months ago

#61719 closed defect (bug) (fixed)

WP_HTML_Tag_Processor doesn't allow to set a valid image src

Reported by: ivanzhuck's profile ivanzhuck Owned by: dmsnell's profile dmsnell
Milestone: 6.6.2 Priority: normal
Severity: normal Version: 6.6
Component: HTML API Keywords: has-patch fixed-major dev-reviewed commit
Focuses: Cc:

Description

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...">

Change History (23)

#1 @ivanzhuck
7 months ago

  • Component changed from General to HTML API

#2 @dmsnell
7 months ago

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.

This ticket was mentioned in Slack in #core-test by ankit-k-gupta. View the logs.


7 months ago

#4 @Ankit K Gupta
7 months ago

  • Keywords needs-patch added

This ticket was mentioned in PR #7114 on WordPress/wordpress-develop by @amitraj2203.


6 months ago
#5

  • Keywords has-patch added; needs-patch removed

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.

@amitraj2203 commented on PR #7114:


6 months ago
#6

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

#7 @dmsnell
6 months ago

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.

@amitraj2203 commented on PR #7114:


6 months ago
#8

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

#9 @dmsnell
6 months ago

  • Owner set to dmsnell
  • Resolution set to fixed
  • Status changed from new to closed

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.

#11 @dmsnell
6 months ago

  • Keywords dev-feedback added
  • Milestone changed from Awaiting Review to 6.6.2
  • Resolution fixed deleted
  • Status changed from closed to reopened

Re-opening for a backport into 6.6.2.

#12 @ivanzhuck
6 months ago

@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.

#13 @dmsnell
6 months ago

@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.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


6 months ago

#15 @hellofromTonya
6 months ago

  • Keywords fixed-major added

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


6 months ago

#17 @hellofromTonya
6 months ago

Discussed in today's 6.6.2 bug scrub. Summary:

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?

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


5 months ago

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


5 months ago

#20 @jorbin
5 months ago

  • Keywords dev-reviewed commit added; dev-feedback removed

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

#21 @hellofromTonya
5 months ago

Thanks @jorbin.

I'm in the process of backporting it now.

#22 @hellofromTonya
5 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

#23 @Hrohh
4 months ago

You can set

$processor->set_attribute( 'src', "//:0" );
Note: See TracTickets for help on using tickets.