Repository navigation
KSES: Allow picture, source and responsive img attributes in post content - #6184
adamsilverstein wants to merge 3 commits into
Conversation
Test using WordPress PlaygroundThe 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
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for the HTML5 <picture> element and srcset attribute on <img> tags to WordPress's KSES filtering system. This enables proper handling of responsive images in content filtering.
- Adds
<picture>and<source>elements to allowed post tags - Adds
srcsetandsizesattributes to<img>and<source>elements - Refactors URI sanitization logic to handle multi-URI attributes like
srcset
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/wp-includes/kses.php | Adds picture/source elements to allowed tags, adds srcset/sizes attributes, and creates new wp_kses_sanitize_uris function to handle multi-URI attributes |
| tests/phpunit/tests/kses.php | Adds comprehensive test coverage for img srcset attributes, srcset sanitization, and picture element filtering |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@adamsilverstein Can you resolve conflicts on this PR, as the testing team has to apply this patch to QA it? Thanks! |
yes, I will work on that. |
c402114 to
f9e002c
Compare
|
@adamsilverstein thanks for working on this ticket! |
I have resolved the merge conflicts and am looking at adding additional tests. I'll also review feedback on the Trac ticket. Also fine punting this to 7.1 early. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. Props Bot commandsYou can trigger Props Bot actions by commenting on this PR:
|
|
I added some fixes and additional tests. |
|
Refreshed against current trunk (clean merge, CI green). Given 7.0 RC1 has passed, now targeting 7.1. |
- Rename variables for clarity: $attr_name, $attr_value, $uri_attrs, $multi_uri_attrs, $matches. - Add native type declarations for parameters and return value. - Replace the indexed for loop with foreach. - Use the null coalescing operator for the optional descriptor match. - Restore alphabetical ordering of the img, source, and wp_kses_uri_attributes() attribute lists. Addresses review feedback from @westonruter on PR WordPress#6184. See #29807.
|
I've refreshed the branch against current trunk and pushed three commits: fc99930 - Review feedback. Addresses all of @westonruter's notes: alphabetized the 5ea4632 - Sanitize descriptor-less srcset entries individually. While addressing the feedback I found that the splitter only recognized 215b942 - HTML API: preserve srcset in set_attribute(). Registering Local runs: kses suite 412 tests green, HTML API suite 1,535 tests green, PHPCS clean on all touched files. |
|
Pushed two follow-up commits: a86d336 - Adds 6d3d20a - Documents a known, accepted parsing limitation: a comma attached to an invalid descriptor (e.g. All three affected suites pass locally (kses: 421, html-api tag processor: 507, wpKsesHair: 67) and PHPCS is clean. |
swissspidy
left a comment
swissspidy
left a comment
There was a problem hiding this comment.
I'm not super knowledgeable of KSES, so best if someone else also reviews, but the changes look reasonable to me.
dmsnell
left a comment
dmsnell
left a comment
There was a problem hiding this comment.
I’ve left some early thoughts. at the same time I’ve been working on a wp_kses() replacement using the HTML API. that’s not pushed yet, so there’s nothing you need to do, but I wanted you to be aware of it.
one way we could work more-independently here is by creating a new function whose purpose is to sanitize a comma-separated list of candidate image strings. given the conflation of HTML’s concept of that, and what I think is a new concept in WordPress of “multi URI attributes” (something HTML doesn’t talk about), and how we are attempting to specifically parse the one while being generic in naming, I think a separate method we call outside of wp_kses() could be a good start.
| * | ||
| * @return string[] HTML attribute names whose value contains a list of URLs. | ||
| */ | ||
| function wp_kses_multi_uri_attributes() { |
There was a problem hiding this comment.
HTML only defines a few global attributes, so while functions like esc_url() are stuck in legacy interfaces that prevent determining the type of an HTML attribute, we have the opportunity in new code to do better.
- the attributes table shows the type of
srcsetonimgandsourceelements, but on any other element it won’t be interpreted in the same way by the browser. - the srcset definition explains how to parse it.
- the
imagesrcsetis just like it but exists on thelinkelement.
to this point, I think the naming is a bit misleading. srcset is not a list of URLs or URIs, but a comma-separated list of image candidate strings. some attributes have space-separated lists, in contrast, and as you note, may contain width and pixel-density descriptors. this is not like ping on A or AREA elements, which only accept space-separated non-empty URLs.
I like the direction this is reaching for, and have wanted to improve Core’s handling of URL attributes myself, but I think we should be careful about trapping ourselves in a corner the way we have with esc_url().
@westonruter and I spoke about making semantic functions in the HTML API to handle each attribute byte, but obviously this requires substantial work to handle the varied types of those attributes.
There was a problem hiding this comment.
the sanitization spec mentions detecting javascript: URIs, which I think is our main concern. it would be interesting to see if we can align with this relatively new part of the HTML spec so that our behaviors mirror the browsers’
| * rejected here is also rejected as a scheme by browsers. | ||
| */ | ||
| $prefix = preg_replace( '/(�*58(?![;0-9])|�*3a(?![;a-f0-9]))/i', '$1;', $url ); | ||
| $prefix = preg_split( '/:|�*58;|�*3a;|:/i', $prefix, 2 ); |
There was a problem hiding this comment.
there are dangers in writing these decodes ad-hoc because of complicated rules for unexpected formatting. coming from wp_kses_hair(), these should actually be decoded already except for the big five syntax character <&>'"
however, we have WP_HTML_Decoder::decode_attribute() for this, as well as WP_HTML_Decoder::attribute_starts_with()
dmsnell
left a comment
dmsnell
left a comment
There was a problem hiding this comment.
we are still passing around the terminology of “multi-URI attributes” and I don’t want that to make it more difficult to properly handle and sanitize as we expand support.
everything surrounding wp_kses_uri_attributes() exhibits what’s a bit sludgy here:
- which HTML element the attribute is on matters, but isn’t used to determine if the attribute contains URLs
- the filter replaces the fixed set of URL-containing attributes rather than augments it, making it possible to remove one that a browser will still interpret as a URL
we’re also conflating HTML attribute sanitization and attribute value parsing, which makes it hard to ensure consistency for either of them. I think there’s a case to be made where we have a separate function or set of functions that split these activities and for which we can test and assert and improve their behavior.
switch ( $attr_name ) {
case 'imagesrcset':
case 'srcset':
$urls = wp_html_attribute_get_comma_separated_tokens( $value );
break;
case 'href':
case 'src':
$urls = array( wp_html_attribute_get_whitespace_surrounded_token( $value ) );
break;
}and so on. the naming and location of these functions is still murky, which is why they still aren’t in place. we should examine what the browsers do to verify them against the spec. for example, is a “comma-separated list of image candidate strings” the same as a comma-separated list of tokens, whose tokens are image candidate strings? or do browsers parse this one specially?
we might even consider adding a new kind of dispatcher function which returns an attribute’s type, and then have another function which is like wp_get_semantic_attribute_value( $tag_name, $attribute_name, $namespace = 'html' ): mixed. I think this is not the right time or PR for this, but it’s probably the right direction to move towards.
from a performance perspective I think we can do more to analyze the URLs in place without allocating so much. It seems like all this does is check for “bad protocols” and yet we’re creating all sorts of chunks and copies of strings repeatedly, and then string-building the result.
one approach I like to use is to track a $was_at value which represents the last part of the input that was copied into the output. if we reach the end and $was_at is 0, we can return the input directly and that will be reference-equal at the call-site.
otherwise, we can now only extract and allocate the scheme parts, and if those come back fine then we advance $was_at but avoid appending content into the $result.
it’s only once we encounter a changed value that we need to flush
$result .= substr( $attr_value, $was_at, $item_at - $was_at );
$result .= $url;
$result .= substr( $attr_value, $descriptors_at, $descriptor_length );
$was_at = $at;some details are omitted for brevity, but generally this will avoid allocations, avoid memory copies, and preserve the reference-equality of strings and interned strings by never copying unless necessary.
| * @since 7.2.0 | ||
| * | ||
| * @param string $attr_name The attribute name to test. | ||
| * @param string $attr_value The attribute value to sanitize. |
There was a problem hiding this comment.
there is no indication on whether these values are decoded or raw, and that could be incredibly important.
ideally, we should require already-decoded attribute values here so that this function can focus on the actual logic of URL parsing and sanitization, and not jumble in HTML-decoding logic as well.
| while ( $at < $length ) { | ||
| // The whitespace and commas separating this candidate from the previous one. | ||
| $separator_length = strspn( $attr_value, "{$whitespace},", $at ); | ||
| $separator = substr( $attr_value, $at, $separator_length ); |
There was a problem hiding this comment.
especially with kses it should not be essential to preserve the separator, and in fact, ideal to normalize it to a single space. that should mean we can skip right past the whitespace, which is common in HTML API code.
$at += strspn( $attr_value, " \t\f\r\n" );| if ( ! $dropped ) { | ||
| $result .= $separator; | ||
| } | ||
| break; |
There was a problem hiding this comment.
it looks like we’re intentionally preserving trailing whitespace here, but does it make more sense to trim off the final whitespace by unconditionally breaking here without appending the separator?
| ++$at; | ||
| } | ||
| } | ||
| $descriptors = substr( $attr_value, $descriptor_start, $at - $descriptor_start ); |
There was a problem hiding this comment.
note that if all we care about sanitizing is the URL, then we can skip the descriptors if we first segment the comma-separated list, which we need to do anyway. we can ignore the descriptors here, or if we elect, apply additional constraints (but that raises new questions, such as what to do when the syntax is right but the semantics are wrong)
| * rejected here is also rejected as a scheme by browsers. | ||
| */ | ||
| $prefix = preg_replace( '/(�*58(?![;0-9])|�*3a(?![;a-f0-9]))/i', '$1;', $url ); | ||
| $prefix = preg_split( '/:|�*58;|�*3a;|:/i', $prefix, 2 ); |
There was a problem hiding this comment.
these are patterns we want to avoid entirely.
this goes back into asking what kind of content this function expects. by translating or even recognizing these here, we are implicitly declaring that this function expects raw and possibly-encoded HTML text
a problem with that, however, is that we are basing our parsing of the comma-separated image candidate strings based on raw bytes. the code is internally incoherent because there is no way to properly pass values into this function.
I recommend that we require callers to pass already-decoded HTML text here, but if we go the other route, we have to instruct callers to re-encode HTML syntax characters before sending inputs here, and then we have to decode the entire attribute value at the start, not mid-way through.
every other option presents mis-parsing issues that will bypass sanitization and cause other corruption.
| if ( isset( $prefix[1] ) ) { | ||
| $scheme = wp_kses_decode_entities( $prefix[0] ); | ||
| $scheme = preg_replace( '/\s/', '', $scheme ); | ||
| $scheme = wp_kses_no_null( $scheme ); |
There was a problem hiding this comment.
we should really really avoid calling this function. it is far better to replace unwanted or scary bytes with the Unicode replacement character (U+FFFD �) than it is to remove them.
needless to say, if we require decoded inputs we can skip all of these.
IMO we should also avoid things like skipping the whitespace. for one, it should not be possible to reach this point in the code and find whitespace. for two, there is no plausible reason to accept http s://wordpress.org. if something isn’t normative and has no definitive resolution, we can reject it entirely.
there are, after all, untrusted inputs and the srcset is not that important that we need to over-stress to try and safely resolve invalid inputs.
| break; | ||
| } | ||
|
|
||
| // The URL is the next run of non-whitespace characters… |
There was a problem hiding this comment.
I found Parsing a srcset attribute and it answered my question: is this the same as a comma-separated list of tokens — it’s not. I see that the code here follows that algorithm and link to the spec is already above.
one thing remains though, this parsing is applied to all $multi_uri_attrs even though the parsing spec only applies to a srcset attribute, meaning that without additional code to guard against it, we are inviting mis-parses for other attributes people might add to the multi-url attributes list
|
@dmsnell Thanks for all of the detailed feedback. With all of the progress you are making on https://make.wordpress.org/core/2026/10/07/progress-report-wp_kses/ I'm hesitant to make any changes here. Besides being nervous about touching kses in general, I expect your approach to be vastly more accurate/precise and much less likely to introduce unintended side effects. What do you think, should I still try to land this change? |
|
@adamsilverstein you’re right; touching but I think more broadly this is (a) really good to pursue, and (b) best to try and build gradually towards. I think the terminology matters significantly if we want to avoid the troubles of the past. that is, we don’t need to ship perfect code or cover every future possibility, but we should ensure that the designs support the right things and that our interfaces support knowable needs. for instance, sadly, as your work taught me, at a minimum we are going to need to pass the tag name, and optionally the namespace, into the sanitization function (and any corresponding filter, unlike I think it’s also worth considering that we only allow extending the attribute set, and never remove those which are fixed by the HTML spec or browsers. because I’m long-winded, I’m going to try and summarize There may be multiple things in this PR that could be addressed separately:
as a final note, I think for some of these things, especially within the purview of KSES, it’s probably okay to hard-code them without new filters as a first step, because this is sensitive and if people don’t need to extend them, then it’s fine leaving this spec-oriented. hope this makes sense. |
|
makes sense @dmsnell - I'll work on this again soon, thanks for the feedback! I'll see if I can split up the code into the parts you mention can be addressed separately, maybe in the form of several stacked PRs. |
A srcset value is a comma-separated list of image candidate strings, not a single URL, so passing it through wp_kses_bad_protocol() or esc_url() either misses candidates or corrupts the list. This function segments the value following the HTML specification's srcset parsing algorithm and removes candidates whose URL uses a disallowed protocol. It takes decoded attribute text and returns decoded text, leaving HTML decoding and encoding to the caller, and returns the input string itself when every candidate is allowed. Nothing calls it yet; wiring it into KSES by tag and attribute follows separately. See https://core.trac.wordpress.org/ticket/29807
Both KSES parsers decide how to sanitize a URL attribute from the attribute name alone, which cannot express that srcset on img and source, and imagesrcset on link, hold a list of image candidates rather than one URL. Route URL attributes through a private dispatcher that takes the element name as well, sending those attributes to wp_kses_sanitize_srcset() and everything in wp_kses_uri_attributes() to wp_kses_bad_protocol() as before. wp_kses_hair() gains an optional $element parameter so wp_kses_attr() can pass the element through; existing callers without one keep the attribute-name-only behavior. wp_kses_one_attr() decodes the value before sanitizing and re-encodes it only when sanitization changed it, so encoded separators cannot hide a candidate. No allowed-HTML change yet, so srcset is still stripped from post content; that follows separately. See https://core.trac.wordpress.org/ticket/29807
Post content has had no way to carry responsive images: wp_kses_post() strips srcset and sizes from img, and removes picture and source entirely, so authors without unfiltered_html lose the markup the editor and themes generate. Now that srcset is sanitized per candidate by element, allow picture, source and the img attributes srcset, sizes, decoding and fetchpriority in the post context. source also allows src, width and height so video and audio sources survive as more than a bare element. Fixes https://core.trac.wordpress.org/ticket/29807
2351a91 to
6090c9f
Compare
|
First piece of the split is up: #14811 adds |
adamsilverstein
changed the title
Trac ticket: https://core.trac.wordpress.org/ticket/29807
Third and last PR in the stack, rebased down from the earlier version of this branch (the old history is tagged locally if anyone needs it). It builds on #14811 (the srcset parser) and #14812 (wiring it into both KSES parsers by element), so the diff shows all three commits until those land. The change in this PR is only the last one.
With per-candidate srcset sanitization in place, this allows the responsive image markup in post content:
picture(no attributes)sourcewithheight,media,sizes,src,srcset,type,width-srcis there sovideoandaudiosources keep their URL rather than surviving as a bare elementimggainsdecoding,fetchpriority,sizesandsrcset, and the list is alphabetizedEverything the review on the previous version asked for that was about the parser or the "multi-URI" abstraction is handled in the first two PRs. Dropped entirely since trunk moved: the Tag Processor
set_attribute()change (srcset was never added towp_kses_uri_attributes(), so there is nothing to work around),wp_kses_multi_uri_attributes()and its filter.Not in this stack, on purpose: a tag/namespace-aware filter for extending URL sanitization (dmsnell's third item - worth its own ticket once the hard-coded version has settled), and swapping
wp_kses_bad_protocol()for the HTML spec's sanitization algorithmjavascript:detection, which would be a change confined towp_kses_sanitize_srcset()later.imagesrcsetonlinkis supported by the dispatcher butlinkis not allowed in post content, so nothing changes there.Testing
npm run test:php -- --group kses. The newtest_responsive_image_markup_in_post_contentruns each case throughwp_kses_post()under both parsers. For a manual check, as a Contributor paste into a post:The
javascript:candidate should be gone and thepicturestructure intact. One behavior worth knowing: a srcset whose candidates are all disallowed comes out assrcset="", matching what an emptiedhreforsrcdoes today, rather than the attribute being dropped.AI Use
The rebase, the tests and this description were done with 🤖 Claude Code, steered by me. I will review and test.