#61719 closed defect (bug) (fixed)
WP_HTML_Tag_Processor doesn't allow to set a valid image src
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
This ticket was mentioned in Slack in #core-test by ankit-k-gupta. View the logs.
7 months ago
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
@
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
@
6 months ago
- Owner set to dmsnell
- Resolution set to fixed
- Status changed from new to closed
In 58844:
#11
@
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
@
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
@
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
This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.
6 months ago
#17
@
6 months ago
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 returnfalse
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?
Thanks for the report, @ivanzhuck.
This is a result of bringing in
esc_url()
toset_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.