wp_kses: add support for picture element and srcset attribute on img tags - #6184
wp_kses: add support for picture element and srcset attribute on img tags#6184adamsilverstein wants to merge 47 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. |
Co-authored-by: Andrew Ozz <743931+azaozz@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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.
|
Refreshed against current trunk (clean merge, CI green). Given 7.0 RC1 has passed, now targeting 7.1. |
Extend the srcset entry boundary pattern to recognize descriptors like `1.5x` and `2.5x` so that per-entry protocol sanitization still applies when entries are separated by decimal descriptors. Previously a bad protocol following or preceding a decimal descriptor could leak through because the blob was treated as a single URL. Also extract the descriptor pattern into a local variable to avoid drift between the split regex and the delimiter/entry matchers. See #29807.
Add test coverage for: - Bad protocols in srcset entries separated by decimal descriptors. - Malformed srcset values (leading/trailing commas, whitespace only). - Uppercase and mixed-case attribute names on single- and multi-URI attributes. - Empty `$multi_uri` falling through to single-URI handling. - `wp_kses_one_attr()` with the `sizes` attribute. Also remove a dead `if ( $name === $value )` branch in `test_wp_filter_post_kses_img` and switch new assertions from `assertEquals()` to `assertSame()` to match the project convention. See #29807.
- 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.
The srcset entry splitter only recognized entry boundaries of the form "URL descriptor, URL". Descriptors are optional per the HTML spec, so a value like "safe.jpg, javascript:alert(1)" was treated as a single URL and the embedded bad protocol survived sanitization because wp_kses_bad_protocol() only inspects the start of the string. URLs in srcset must encode whitespace as %20, so a comma adjacent to whitespace can never be part of a URL and always separates two entries. Treat such commas as entry boundaries in addition to descriptor+comma boundaries. Commas with no adjacent whitespace (CDN image resizer URLs) remain part of the URL, matching browser srcset parsing. See #29807.
With srcset registered in wp_kses_uri_attributes(), set_attribute() would pass srcset values through esc_url(), which encodes the descriptor spaces as %20 and collapses the comma-separated URL list into a single broken URL. Give srcset the standard attribute escaping instead, matching its behavior before srcset became a URI attribute. Protocol sanitization of srcset entries is handled per-URL by KSES via wp_kses_sanitize_uris(). Also make the custom multi-URI attribute test exercise the multi-URI code path by registering the attribute via the wp_kses_uri_attributes filter. 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. |
…ruth. Replace the hardcoded 'srcset' checks in WP_HTML_Tag_Processor::set_attribute() and the wp_kses_sanitize_uris() default with a shared, filterable list so plugins can register additional srcset-like attributes (e.g. imagesrcset) without having them corrupted by esc_url() or missing per-entry protocol checks. Includes tests for the new default, the filter path, and the tag processor honoring the filter.
A comma attached to an invalid descriptor (e.g. "a.jpg 2q,javascript:...") is not an entry separator, so KSES protocol-checks the surrounding text as a single URL while browsers may parse the invalid srcset into more candidates. Explain in the parser comment why this asymmetry is safe (srcset candidates are only ever fetched as images, never navigated or executed) and pin the behavior with tests so future splitter changes are made consciously.
|
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.
The regex-based entry splitter diverged from how browsers tokenize srcset: a comma attached to an invalid descriptor was treated as part of a single URL while browsers start a new candidate there, letting a disallowed scheme ride through un-checked. It also trimmed trailing whitespace, mishandled newlines (the dot could not cross them), and cast a possible preg_split() failure to array, which would silently empty the whole value. Replace the splitter with a byte scanner implementing the specification's parse-a-srcset-attribute algorithm: URLs are runs of non-whitespace characters, trailing commas terminate a candidate, and descriptors end at the first comma outside parentheses. Only the URLs are protocol-checked; whitespace, separating commas, and descriptors are preserved byte for byte, so benign values now round-trip unchanged. No PCRE is involved in the splitting, removing the failure mode entirely. Additionally, a URL whose text before the first colon cannot be a URL scheme per RFC 3986 (an ASCII letter followed by letters, digits, plus, hyphen, and period) is left intact instead of being passed to wp_kses_bad_protocol(), which would otherwise rewrite the relative URL a.jpg,https://example.com/b.jpg into the cross-origin //example.com/b.jpg. The scheme detection mirrors the colon handling in wp_kses_bad_protocol_once() and normalizes a superset of the characters browsers strip from URLs, so nothing a browser could parse as a scheme skips the check. data: URIs remain stripped by default, consistent with src and href; the allowed-protocols list extends per candidate for sites that opt in. See #29807.
Requiring an attribute to appear in both wp_kses_uri_attributes() and wp_kses_multi_uri_attributes() before any sanitization applied created a silent failure mode: a plugin adding an attribute to only the multi-URI filter got no sanitization at all, with nothing to signal the missing second registration. Make membership in the multi-URI list sufficient on its own, so the worst outcome of a partial registration is per-URL sanitization rather than none (fail-safe). Registering the attribute in both lists is still recommended so code that consults only wp_kses_uri_attributes() recognizes it as a URI attribute. As a side effect, disabling srcset sanitization via filters now requires removing the attribute from both lists, and the srcset branch no longer consults the URI attributes list at all. See #29807.
wp_kses_hair() and wp_kses() are public functions dating to 1.0 whose $allowed_protocols parameter has long tolerated a string; the strict scalar types on the new wp_kses_sanitize_uris(), reached for every string attribute value, turned that into an unconditional TypeError on every PHP version, even for markup with no URI attributes at all. Drop the type declarations, matching the rest of kses.php, and cast the protocols list to an array as wp_kses_bad_protocol_once2() already does, so a string behaves as a single-protocol allowlist. Also restore the list hoisting wp_kses_hair() had before the refactor: fetch the URI and multi-URI attribute lists once per call and hand them to wp_kses_sanitize_uris() through new optional parameters, instead of re-running both filters for every attribute of every tag on the busiest kses path. See #29807.
Exempting multi-URI attributes such as srcset from esc_url() left them with only character escaping: set_attribute( 'srcset', 'javascript:alert(1) 1x' ) wrote the disallowed protocol out verbatim, and any attribute a plugin added to the wp_kses_multi_uri_attributes filter silently lost URL sanitization at every set_attribute() call site. Route multi-URI attribute values through wp_kses_sanitize_uris() instead, which strips disallowed protocols from each candidate URL individually while preserving the descriptors and spacing that esc_url() would corrupt, then apply the standard attribute escaping to the result. See #29807.
The scheme detection that decides whether a srcset candidate URL gets protocol-checked must mirror wp_kses_bad_protocol()'s entity handling in both directions: an entity-encoded colon still marks a scheme to strip, while an entity-encoded comma in the prefix must not be decoded into a character that disguises the prefix as sanitizable. These cases pin the two directions so future changes to either side keep them in sync. See #29807.
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()
Trac ticket: https://core.trac.wordpress.org/ticket/29807
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.