Repository navigation
KSES: Sanitize srcset by element and attribute in both parsers - #14812
Open
adamsilverstein wants to merge 2 commits into
Open
adamsilverstein wants to merge 2 commits into
adamsilverstein wants to merge 2 commits into
Conversation
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
|
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:
|
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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Trac ticket: https://core.trac.wordpress.org/ticket/29807
Second PR in the stack splitting up #6184. Builds on #14811, so the diff here includes that commit until it lands; the change in this PR is the second commit only.
Both KSES parsers currently pick the URL sanitizer from the attribute name alone, so there is no way to say that
srcsetonimg/source(andimagesrcsetonlink) is a list of image candidates rather than a single URL. This PR adds a small private dispatcher,_wp_kses_sanitize_url_attribute( $element, $attribute, $value, $allowed_protocols, $uri_attributes ), that takes the element name as well and routes those attributes towp_kses_sanitize_srcset(); anything inwp_kses_uri_attributes()still goes throughwp_kses_bad_protocol()exactly as before, and everything else is untouched. Per @dmsnell's note on #6184 this is hard-coded with no new filter for now.It is wired into three places:
wp_sanitize_html_kses(), where the value is already decoded and re-encoded by the processor.wp_kses_hair(), which gains an optional third$elementparameter thatwp_kses_attr()now passes. Existing callers without it (embed.php, formatting.php) keep the attribute-name-only behavior, with srcset still sanitized as srcset.wp_kses_one_attr(), which now decodes the value before sanitizing and re-encodes it only when sanitization changed it, so /,separators cannot hide a candidate.Nothing user-facing changes yet:
srcsetis not in the allowed post HTML, so it is still stripped. That lands in the third PR. The tests here pass their own$allowed_htmland run every case through both parsers, togglingwp_kses_force_legacy_parser.One thing worth a look:
srcseton an element browsers do not read it on (thespancase in the tests) is intentionally left alone rather than parsed. That follows the "dispatch by tag" feedback, but if folks would rather sanitize it anyway as belt and braces, that is a one-line change in the dispatcher.Testing
npm run test:php -- --group kses. New cases are intests/phpunit/tests/kses.php(test_srcset_is_sanitized_by_element,test_one_attr_sanitizes_srcset) andtests/phpunit/tests/kses/wpKsesHair.php. Same caveat as the first PR: I could not run PHPUnit locally today, so CI is the first full run.AI Use
Code and description both came out of a conversation with 🤖 Claude Code, working from the review thread on #6184. I will review and test.