Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
adamsilverstein wants to merge 1 commit into
base: trunk
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 91 additions & 0 deletions src/wp-includes/kses.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3200,6 +3200,97 @@ function wp_kses_bad_protocol_once2( $scheme, $allowed_protocols ) {
}
}

/**
* Removes image candidates with a disallowed URL protocol from a `srcset` value.
*
* A `srcset` attribute holds a comma-separated list of image candidate strings,
* each a URL optionally followed by whitespace and width or density descriptors,
* for example `small.jpg 480w, large.jpg 1024w`. The value is split into candidates
* following the HTML specification's srcset parsing algorithm, so a comma only
* separates candidates when it ends a URL or its descriptors; a comma inside a URL,
* as produced by image CDN resizers, belongs to the URL.
*
* Each URL is checked with {@see wp_kses_bad_protocol()}. A candidate whose URL that
* check would change is removed in full, descriptors included, because stripping a
* scheme and keeping the rest would rewrite the URL rather than remove it, and
* descriptors left behind would be read as the next candidate's URL. As in
* {@see esc_url()}, a URL starting with `/` cannot carry a scheme and is kept as is.
*
* The value must be decoded attribute text, as returned by
* {@see WP_HTML_Tag_Processor::get_attribute()}, not raw HTML containing character
* references. The returned value is decoded text too; callers writing it back into
* HTML must escape it. When every candidate is allowed, the input is returned
* unchanged. Otherwise the remaining candidates are rejoined with `, `.
*
* @since 7.2.0
*
* @link https://html.spec.whatwg.org/multipage/images.html#parsing-a-srcset-attribute
*
* @param string $srcset Decoded `srcset` attribute value.
* @param string[] $allowed_protocols Array of allowed URL protocols.
* @return string The value without candidates whose URL uses a disallowed protocol.
*/
function wp_kses_sanitize_srcset( string $srcset, array $allowed_protocols ): string {
$whitespace = " \t\f\r\n";
$length = strlen( $srcset );
$at = 0;
$result = '';

/*
* Candidates are only copied once one has to be removed. Until then the
* scan allocates nothing but the URLs it checks, and an all-allowed value
* is returned as the very string that was passed in.
*
* On the first removal the scan restarts to rebuild the output; candidates
* before `$checked_until` passed their check in the first pass and are not
* checked again.
*/
$rebuild = false;

@westonruter westonruter Oct 25, 2026

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

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.

$checked_until = 0;

while ( $at < $length ) {
$at += strspn( $srcset, "{$whitespace},", $at );
if ( $at >= $length ) {
break;
}

// The URL is the run of non-whitespace characters, less any commas ending it.
$candidate_at = $at;
$url_length = strcspn( $srcset, $whitespace, $at );
$url = rtrim( substr( $srcset, $at, $url_length ), ',' );
$at += $url_length;

// 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 );

@westonruter westonruter Oct 25, 2026

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

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.

$candidate_length = $at - $candidate_at;
} else {
$candidate_length = strlen( $url );
}

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

@westonruter westonruter Oct 25, 2026

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

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.

}

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

@westonruter westonruter Oct 25, 2026

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

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.

}
continue;
}

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

@westonruter westonruter Oct 25, 2026

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

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.

}
}

return $rebuild ? $result : $srcset;
}

/**
* Converts and fixes HTML entities.
*
Expand Down
153 changes: 153 additions & 0 deletions tests/phpunit/tests/kses/wpKsesSanitizeSrcset.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,153 @@
<?php
/**
* Test wp_kses_sanitize_srcset() function.
*
* @group kses
*
* @covers ::wp_kses_sanitize_srcset
*/
class Tests_Kses_WpKsesSanitizeSrcset extends WP_UnitTestCase {

/**
* Allowed protocols used by every test.
*
* @var string[]
*/
protected $allowed_protocols;

@westonruter westonruter Oct 25, 2026

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.

Suggested change
protected $allowed_protocols;
protected array $allowed_protocols;


public function set_up() {
parent::set_up();
$this->allowed_protocols = wp_allowed_protocols();
}

/**
* @ticket 29807
*
* @dataProvider data_allowed_values_are_returned_unchanged
*
* @param string $srcset Value whose candidates are all allowed.
*/
public function test_allowed_values_are_returned_unchanged( string $srcset ) {
$this->assertSame( $srcset, wp_kses_sanitize_srcset( $srcset, $this->allowed_protocols ) );
}

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

@westonruter westonruter Oct 25, 2026

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.

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

return array(
'empty value' => array( '' ),
'whitespace only' => array( " \t\n" ),
'single URL, no descriptor' => array( 'image.jpg' ),
'relative URLs' => array( 'path/to/test.png 1x, path/to/test-2x.png 2x' ),
'root-relative URLs' => array( '/test.png 1x, /test-2x.png 2x' ),
'protocol-relative URLs' => array( '//cdn.example.com/img-300.jpg 300w, //cdn.example.com/img-600.jpg 600w' ),
'absolute URLs with width descriptors' => array( 'https://example.com/small.jpg 480w, https://example.com/medium.jpg 800w, https://example.com/large.jpg 1200w' ),
'uppercase scheme' => array( 'HTTPS://example.com/a.jpg 1x, Http://example.com/b.jpg 2x' ),
'decimal density descriptors' => array( 'image-1x.jpg 1x, image-1.5x.jpg 1.5x, image-2x.jpg 2x' ),
'no descriptor on the last candidate' => array( 'small.jpg 300w, medium.jpg 600w, large.jpg' ),
'no whitespace after the comma' => array( 'image1.jpg 1x,image2.jpg 2x' ),
'extra whitespace is preserved' => array( ' image1.jpg 1x , image2.jpg 2x ' ),
'newlines are preserved' => array( "image1.jpg 1x,\nimage2.jpg 2x" ),
'commas inside a URL belong to the URL' => array( 'http://localhost/test.png,big 1x, http://localhost/test.png,medium 2x' ),
'CDN resizer URLs with commas' => array( 'https://resizer.example/cdn-cgi/image/format=auto,onerror=redirect,quality=80,width=412,height=275,dpr=1,fit=crop,gravity=0.5x0.5/https://bucket.example/photo.jpg 412w, https://resizer.example/cdn-cgi/image/format=auto,quality=80,width=824,height=550/https://bucket.example/photo.jpg 824w' ),
'CDN resizer URL without descriptor' => array( 'https://cdn.example/image/format=auto,quality=80/img.jpg 400w, https://cdn.example/image/format=auto,quality=80/img-large.jpg' ),
'root-relative URL containing a colon' => array( '/images/a:b.jpg 1x' ),
);
}

/**
* @ticket 29807
*
* @dataProvider data_candidates_with_disallowed_protocols_are_removed
*
* @param string $srcset Value containing at least one disallowed candidate.
* @param string $expected Value with those candidates removed.
*/
public function test_candidates_with_disallowed_protocols_are_removed( string $srcset, string $expected ) {
$this->assertSame( $expected, wp_kses_sanitize_srcset( $srcset, $this->allowed_protocols ) );
}

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

@westonruter westonruter Oct 25, 2026

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.

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

@westonruter westonruter Oct 25, 2026

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

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.

return array(
'first candidate' => array(
'bad://localhost/test.png 1x, http://localhost/test-2x.png 2x',
'http://localhost/test-2x.png 2x',
),
'last candidate' => array(
'http://localhost/test.png 1x, bad://localhost/test-2x.png 2x',
'http://localhost/test.png 1x',
),
'middle candidate' => array(
'image-1x.jpg 1x, javascript:alert(1) 1.5x, image-2x.jpg 2x',
'image-1x.jpg 1x, image-2x.jpg 2x',
),
'only candidate' => array(
'javascript:alert(1) 1x',
'',
),
'every candidate' => array(
'javascript:alert(1) 1x, data:text/html,<script>alert(1)</script> 2x',
'',
),
'two in a row' => array(
'a.jpg 1x, javascript:alert(1) 2x, vbscript:msgbox(1) 3x, d.jpg 4x',
'a.jpg 1x, d.jpg 4x',
),
'data URL' => array(
'data:image/png;base64,iVBORw0KGgo= 1x, https://example.com/a.jpg 2x',
'https://example.com/a.jpg 2x',
),
'URL with commas and a disallowed scheme' => array(
'http://localhost/test.png,big 1x, bad://localhost/test.png,medium 2x',
'http://localhost/test.png,big 1x',
),
'descriptor-less candidate after a comma' => array(
'safe.jpg, javascript:alert(1)',
'safe.jpg',
),
'comma-joined URLs form one candidate' => array(
'safe.jpg,javascript:alert(1) 2x',
'',
),
'comma attached to an invalid descriptor' => array(
'a.jpg 2q,javascript:alert(1) 2x',
'a.jpg 2q',
),
'descriptors are not sanitized as URLs' => array(
'a.jpg javascript:alert(1), b.jpg 2x',
'a.jpg javascript:alert(1), b.jpg 2x',
),
'scheme that empties under wp_kses_bad_protocol' => array(
'a:b:c:d:e:f:g 100w, safe.jpg 200w',
'safe.jpg 200w',
),
'emptied scheme without descriptor' => array(
'a:b:c:d:e:f:g, safe.jpg 200w',
'safe.jpg 200w',
),
'separators are normalized once a candidate is removed' => array(
" a.jpg 1x ,\n\tjavascript:alert(1) 2x ,b.jpg 3x ",
'a.jpg 1x, b.jpg 3x',
),
'allowed scheme is kept in its original case' => array(
'HTTPS://example.com/a.jpg 1x, javascript:alert(1) 2x',
'HTTPS://example.com/a.jpg 1x',
),
);
}

/**
* @ticket 29807
*/
public function test_respects_the_given_allowed_protocols() {
$srcset = 'https://example.com/a.jpg 1x, ftp://example.com/b.jpg 2x';

$this->assertSame( $srcset, wp_kses_sanitize_srcset( $srcset, array( 'https', 'ftp' ) ) );
$this->assertSame( 'https://example.com/a.jpg 1x', wp_kses_sanitize_srcset( $srcset, array( 'https' ) ) );
$this->assertSame( '', wp_kses_sanitize_srcset( $srcset, array() ) );
}
}
Loading