KSES: Add wp_kses_sanitize_srcset() for image candidate lists by adamsilverstein · Pull Request #14811 · WordPress/wordpress-develop

adamsilverstein · 2026-10-24T00:52:01Z

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. Only once a candidate is dropped does it build a new string, and then 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, no parentheses handling in descriptors. 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. Note: I could not run the PHPUnit suite locally today (Docker network pool exhausted), so I am relying on CI here; the cases were checked against the function with a small standalone harness.

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 · 2026-10-24T00:52:10Z

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.

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 · 2026-10-24T00:55:20Z

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

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

westonruter · 2026-10-25T03:31:08Z

+	/**
+	 * @return array<string, array{string}>
+	 */
+	public function data_allowed_values_are_returned_unchanged() {


Suggested change

public function data_allowed_values_are_returned_unchanged() {

public function data_allowed_values_are_returned_unchanged(): array {

westonruter · 2026-10-25T03:31:15Z

+	/**
+	 * @return array<string, array{string, string}>
+	 */
+	public function data_candidates_with_disallowed_protocols_are_removed() {


Suggested change

public function data_candidates_with_disallowed_protocols_are_removed() {

public function data_candidates_with_disallowed_protocols_are_removed(): array {

westonruter · 2026-10-25T03:35:09Z

+	 *
+	 * @var string[]
+	 */
+	protected $allowed_protocols;


Suggested change

protected $allowed_protocols;

protected array $allowed_protocols;

westonruter

🤖 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.

westonruter · 2026-10-25T03:40:54Z

+
+		// A URL ended by a comma has no descriptors; otherwise they run to the next comma.
+		if ( strlen( $url ) === $url_length ) {
+			$at              += strcspn( $srcset, ',', $at );


🤖 Comment by Claude Opus 5.5

The descriptor scan ends at the first comma, but the spec's descriptor tokenizer has an "in parens" state in which a comma between ( and ) is part of the descriptor. So the candidate boundaries here can differ from the browser's.

For a.jpg 1x(,)/?,javascript:alert(1) 2x, the browser reads the descriptor 1x(,)/? and then a separate URL javascript:alert(1). This loop instead takes )/?,javascript:alert(1) as the next URL, and since wp_kses_bad_protocol() treats anything with /? before the first colon as scheme-less, the value is returned unchanged.

Tracking parentheses while scanning descriptors (a comma only ends the candidate when not inside parens) would align this with the spec.

westonruter · 2026-10-25T03:40:54Z

+
+		if ( $rebuild ) {
+			if ( $is_allowed ) {
+				$result .= ( '' === $result ? '' : ', ' ) . rtrim( substr( $srcset, $candidate_at, $candidate_length ), $whitespace );


🤖 Comment by Claude Opus 5.5

Same root cause as the comma-in-parens issue above, but on the rebuild path it also corrupts allowed candidates: a.jpg 1x(,)z,javascript:alert(1) 2x comes back as a.jpg 1x(, and inputs like a.jpg 1x(,)z, bad://x 2x surface )z as a new image URL candidate. The output then selects different images than the original did, beyond the removed candidates. Fixing the parenthesis handling fixes this too.

westonruter · 2026-10-25T03:40:55Z

+		if ( $rebuild && $candidate_at < $checked_until ) {
+			$is_allowed = true;
+		} else {
+			$is_allowed = '/' === $url[0] || 0 === strcasecmp( wp_kses_bad_protocol( $url, $allowed_protocols ), $url );


🤖 Comment by Claude Opus 5.5

This inherits the /?-before-colon heuristic from wp_kses_bad_protocol(), which was designed for href values. Since each srcset URL is already isolated here, a strict scheme test (e.g. ^[a-z][a-z0-9+.-]*: compared case-insensitively against $allowed_protocols) would be more robust and would not depend on the tokenizer being exactly right. As it stands, any tokenizer mismatch combined with a /? prefix yields a bypass.

westonruter · 2026-10-25T03:40:55Z

+		if ( ! $is_allowed ) {
+			$rebuild       = true;
+			$checked_until = $candidate_at;
+			$at            = 0;


🤖 Comment by Claude Opus 5.5

After the first removal, the scan restarts at offset 0 and re-tokenizes the whole prefix. Also, since the skip condition is $candidate_at < $checked_until (strict), the candidate that just failed is passed through wp_kses_bad_protocol() a second time. Continuing from the failing candidate, or recording accepted candidate offsets during the first pass, would avoid the double work.

westonruter · 2026-10-25T03:40:55Z

+	 * before `$checked_until` passed their check in the first pass and are not
+	 * checked again.
+	 */
+	$rebuild       = false;


🤖 Comment by Claude Opus 5.5

The $rebuild / $checked_until / restart state machine could be replaced with a single pass: collect $kept[] = rtrim( $candidate, $whitespace ), set $dropped = true on a failure, and end with return $dropped ? implode( ', ', $kept ) : $srcset;. The cost is a small array on the all-allowed path, in exchange for removing the two-mode branches from the loop and the redundant re-scan noted below.

westonruter · 2026-10-25T03:40:55Z

+	/**
+	 * @return array<string, array{string, string}>
+	 */
+	public function data_candidates_with_disallowed_protocols_are_removed() {


🤖 Comment by Claude Opus 5.5

No case covers parenthesized descriptors containing commas, which is where the spec's tokenizer is special and where the implementation diverges. Suggested additions:

a.jpg 1x(,)/?,javascript:alert(1) 2x — the javascript: candidate should be removed.

a.jpg 1x(,)z,javascript:alert(1) 2x — the javascript: candidate should be removed without truncating the first candidate.

a.jpg 1x(,)z, b.jpg 2x — should be returned unchanged.

This was referenced Oct 24, 2026

KSES: Allow picture, source and responsive img attributes in post content #6184

Open

KSES: Sanitize srcset by element and attribute in both parsers #14812

Open

westonruter reviewed Oct 25, 2026

View reviewed changes

	public function data_allowed_values_are_returned_unchanged() {
	public function data_allowed_values_are_returned_unchanged(): array {

	public function data_candidates_with_disallowed_protocols_are_removed() {
	public function data_candidates_with_disallowed_protocols_are_removed(): array {

	protected $allowed_protocols;
	protected array $allowed_protocols;

Conversation

adamsilverstein commented Oct 24, 2026

Testing

AI Use

Uh oh!

github-actions Bot commented Oct 24, 2026

Uh oh!

github-actions Bot commented Oct 24, 2026

Test using WordPress Playground

Some things to be aware of

Uh oh!

westonruter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

westonruter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants