Skip to content

KSES: Add wp_kses_sanitize_srcset() for image candidate lists - #14811

Open
adamsilverstein wants to merge 3 commits into
WordPress:trunkfrom
adamsilverstein:ticket/29807-srcset-parser
Open

adamsilverstein wants to merge 3 commits into
WordPress:trunkfrom
adamsilverstein:ticket/29807-srcset-parser

Conversation

@adamsilverstein

@adamsilverstein adamsilverstein commented Oct 24, 2026 •

Copy link
Copy Markdown
Member

Trac ticket: https://core.trac.wordpress.org/ticket/29807

First of a small stack splitting up #6184 along the lines @dmsnell suggested there: the srcset parser on its own, then wiring it into KSES by tag and attribute, then the allowed-HTML additions for picture, source and srcset.

This PR only adds wp_kses_sanitize_srcset( string $srcset, array $allowed_protocols ): string plus tests. Nothing calls it yet.

What it does:

  • Splits the value into image candidates following https://html.spec.whatwg.org/multipage/images.html#parsing-a-srcset-attribute - a URL is a run of non-whitespace characters, and a comma only separates candidates when it ends the URL or the descriptors, so commas inside CDN resizer URLs stay put.
  • Checks each URL with wp_kses_bad_protocol() and drops the whole candidate, descriptors included, when that check would change it. Descriptors are skipped, not validated.
  • Takes decoded attribute text and returns decoded text. No entity handling inside; callers re-encode.
  • Returns the input string itself when every candidate is allowed. When a candidate is dropped, the remaining candidates are rejoined with , and trailing whitespace trimmed.

Changes from the version in #6184, all following the review feedback there: no "multi-URI attributes" concept or filter, no , /   lookahead, no wp_kses_no_null() or whitespace tolerance inside schemes, and no validation of descriptors. Parentheses in descriptors are tracked only to find where a candidate ends, as the spec does, so a comma inside them does not split the candidate. The a.jpg 2q,javascript:... case that was documented as a known limitation is now handled, since the comma after an invalid descriptor ends the candidate the same way it does in the browser.

Naming: I went with wp_kses_sanitize_srcset() since srcset (and imagesrcset on link) is the only attribute with this syntax. Happy to rename if something like image_candidates reads better.

Testing

npm run test:php -- --group kses - the new tests are in tests/phpunit/tests/kses/wpKsesSanitizeSrcset.php. PHPStan at level 10 is clean on the new code.

AI Use

Claude Code drafted the code and this description from the review thread on #6184, and I steered. I will review and test.

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
@github-actions

github-actions Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

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:

Props adamsilverstein, westonruter.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.


Props Bot commands

You can trigger Props Bot actions by commenting on this PR:

  • @propsbot add <username> to give props to a GitHub user. Requires write access.
  • @propsbot remove <username> to leave a user out of the props. Requires write access.
  • @propsbot refresh to refresh this list. Adding the props-bot label works too.

@github-actions

Copy link
Copy Markdown

Test using WordPress Playground

The 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

  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

@westonruter westonruter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple nits. Also sending up a Claude review momentarily.

Comment thread tests/phpunit/tests/kses/wpKsesSanitizeSrcset.php Outdated
Comment thread tests/phpunit/tests/kses/wpKsesSanitizeSrcset.php Outdated
Comment thread tests/phpunit/tests/kses/wpKsesSanitizeSrcset.php Outdated

@westonruter westonruter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Comment by Claude Opus 5.5

A medium-effort review of wp_kses_sanitize_srcset() turned up one tokenizer mismatch with the HTML spec that lets a disallowed URL through, plus some simplification opportunities. Each was reproduced against this branch:

'a.jpg 1x(,)/?,javascript:alert(1) 2x'  → returned unchanged
'a.jpg 1x(,)z,javascript:alert(1) 2x'   → 'a.jpg 1x('
'a.jpg 1x(,)z, b.jpg 2x'                → unchanged (correct)

The practical impact is limited, since browsers do not execute javascript: URLs in image candidates, but the sanitizer and the browser end up seeing different URLs, which defeats the function's contract. Details are in the inline comments.

Comment thread src/wp-includes/kses.php Outdated
Comment thread src/wp-includes/kses.php Outdated
Comment thread src/wp-includes/kses.php Outdated
Comment thread src/wp-includes/kses.php Outdated
Comment thread src/wp-includes/kses.php Outdated
Comment thread tests/phpunit/tests/kses/wpKsesSanitizeSrcset.php Outdated
Declare return types on the test methods and data providers and a type
on the allowed protocols property, as suggested in review.

See #29807.
The HTML srcset parser treats a comma between `(` and `)` in the
descriptors as part of the descriptor, so ending candidates at the first
comma let the sanitizer and the browser disagree on the URLs: a value
like `a.jpg 1x(,)/?,javascript:alert(1) 2x` came back unchanged, and
rebuilding could truncate allowed candidates.

Track parentheses while scanning descriptors, and replace the restart
and rebuild pass with a single pass that collects kept candidates.

See #29807.
@adamsilverstein

Copy link
Copy Markdown
Member Author

Thanks @westonruter, the parens one was a real miss. Pushed fixes in 144732e and the typing nits in 77ce137.

Claude worked through the review items, notes below:

Commas inside descriptor parentheses - fixed. The descriptor scan now treats a comma between ( and ) as part of the descriptor, and an unclosed ( runs to the end of the value, matching the spec's "in parens" state. All three suggested cases are now tests, plus a.jpg 1x(, javascript:alert(1) 2x (kept, one candidate) and javascript:alert(1) 1x(, a.jpg 2x (removed, nothing left).

Single pass - done as suggested: the loop collects $kept, flags $dropped, and returns $dropped ? implode( ', ', $kept ) : $srcset. That removes the restart, the $checked_until bookkeeping and the double check of the failing candidate. An all-allowed value still comes back as the input string.

Strict scheme test instead of wp_kses_bad_protocol() - left as is. The /? heuristic is only a problem when the tokenizer hands it the wrong string, and with the parens fix the URLs it sees are the ones the browser sees. A browser scheme can only hold [a-z0-9+.-] before the colon, so a / before the first colon means no scheme to the browser either. Keeping the shared check also keeps srcset consistent with how KSES treats src and href, including the control character stripping, which a ^[a-z][a-z0-9+.-]*: test would have to reproduce to avoid letting a leading control character slip a javascript: past it. Happy to revisit if you see a case where they still disagree.

Types - applied the native return and property types. PHPStan at level 10 on kses.php and the test file reports nothing on the new lines, and --group kses passes locally (562 tests).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants