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 40 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.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the 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. |
Tests currently fail, documenting three bugs: - CDN resizer URLs with commas in paths are incorrectly split by wp_kses_sanitize_uris() naive comma splitting - Original spacing around commas is not preserved - decoding and fetchpriority attrs stripped from img tags Also adds passing coverage tests for wp_kses_uri_attributes, wp_kses_one_attr srcset handling, source/picture element edge cases, and the URI attributes filter hook.
These performance attributes are commonly added by WordPress core (wp_img_tag_add_loading_optimization_attrs) and should not be stripped when content passes through KSES filtering.
CDN image resizers (e.g. Cloudflare) use commas in URL paths like cdn-cgi/image/format=auto,quality=80,width=412/... which were incorrectly split by the naive comma-based preg_split in wp_kses_sanitize_uris(). This rewrites the splitting to use the srcset descriptor pattern (e.g. 480w, 2x) as entry boundaries, preserving commas within URLs. Also preserves original whitespace around separators instead of normalizing with implode.
WordPress coding standards require aligned double arrows in multi-line arrays. Adjusts spacing so all arrows in the data provider align with the longest key.
|
I added some fixes and additional tests. |
Expand test coverage for ticket #29807 with edge cases: CDN URLs with gravity=0.5x0.5 parameters, decimal pixel density descriptors, picture element type fallbacks and art direction, protocol-relative URLs, and descriptor-less final srcset entries. Update @SInCE to 7.1.0 per milestone.
|
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. |
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.