From 494f1ec30e684f23d2edd6cf5d974defe212cb12 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B8rn=20A=2E=20Andersen?= Date: Sat, 25 Jul 2026 21:34:53 +0200 Subject: [PATCH 1/6] fix: resolve img/source srcset URLs and add a src fallback Resolve each srcset URL against the document base and drop disallowed schemes, since replace_urls() only handles single-URL attributes. On , copy the smallest entry into src when src is empty or a placeholder. --- src/Sanitize.php | 144 +++++++++++++++++++++++- tests/SanitizeSrcsetTest.php | 209 +++++++++++++++++++++++++++++++++++ 2 files changed, 351 insertions(+), 2 deletions(-) create mode 100644 tests/SanitizeSrcsetTest.php diff --git a/src/Sanitize.php b/src/Sanitize.php index 819071391..5575bf771 100644 --- a/src/Sanitize.php +++ b/src/Sanitize.php @@ -521,6 +521,10 @@ public function sanitize(string $data, int $type, string $base = '') } } + // Set the base before processing allowed nodes so `srcset` URLs + // can be absolutised, and keep only allowed HTML elements and + // attributes (this also rewrites ``/`` `srcset`). + $this->base = $base; if (!empty($this->allowed_html_elements_with_attributes)) { $this->enforce_allowed_html_nodes($document, $this->allow_data_attr, $this->allow_aria_attr); } @@ -547,7 +551,6 @@ public function sanitize(string $data, int $type, string $base = '') } // Replace relative URLs and blocks disallowed URI schemes (protocols) - $this->base = $base; foreach ($this->replace_url_attributes as $element => $attributes) { $this->replace_urls($document, $element, $attributes); } @@ -710,7 +713,8 @@ public function do_strip_htmltags(array $match) } /** - * Keep only allowed HTML elements (tags) and their allowed attributes. + * Keep only allowed HTML elements (tags) and their allowed attributes, + * and rewrite `srcset` URLs on `` and ``. */ protected function enforce_allowed_html_nodes(\DOMNode $element, bool $allow_data_attr = true, bool $allow_aria_attr = true): void { @@ -755,6 +759,10 @@ protected function enforce_allowed_html_nodes(\DOMNode $element, bool $allow_dat $element->removeAttributeNode($element->attributes[$i]); } } + + if (in_array($tag, ['img', 'source'], true) && $element->hasAttribute('srcset')) { + $this->rewrite_img_srcset($element); + } } if ($element instanceof \DOMElement || $element instanceof \DOMDocument) { for ($i = $element->childNodes->length - 1; $i >= 0; $i--) { @@ -1004,6 +1012,138 @@ private function get_http_client(): Client return $this->http_client; } + + /** + * Absolutise each URL in `srcset` on `` and `` against the + * document base, dropping entries with a disallowed URI scheme (this + * runs after `replace_urls`, so entries written into `src` below would + * otherwise bypass that scheme check). For `` only, when `src` is + * empty or a recognised placeholder (lazy-loading pattern), write the + * smallest `Nw` entry (or, failing that, the lowest-density `Nx` entry) + * as a fallback. The browser uses `srcset` for actual selection; `src` + * only loads when `srcset` can't be honoured (legacy browsers, + * non-browser API consumers), so the smallest is the safest fallback + * by bandwidth and is never larger than what `srcset` would have + * picked. + */ + private function rewrite_img_srcset(\DOMElement $element): void + { + $entries = $this->parse_srcset($element->getAttribute('srcset')); + if ($entries === []) { + return; + } + $absolutised = []; + foreach ($entries as $e) { + $abs = $this->registry->call(Misc::class, 'absolutize_url', [$e['url'], $this->base]); + if (!is_string($abs) || $abs === '' || !$this->is_allowed_scheme($abs)) { + continue; + } + $absolutised[] = ['url' => $abs, 'descriptor' => $e['descriptor'], 'w' => $e['w'], 'x' => $e['x']]; + } + if ($absolutised === []) { + $element->removeAttribute('srcset'); + return; + } + $element->setAttribute('srcset', implode(', ', array_map( + static fn (array $e): string => $e['descriptor'] === '' ? $e['url'] : $e['url'] . ' ' . $e['descriptor'], + $absolutised + ))); + + // `` (inside ``) has no `src` attribute; only `` + // needs a fallback `src` rewrite when its current value is a placeholder. + if ($element->tagName !== 'img') { + return; + } + $current = $element->getAttribute('src'); + if (!$this->is_placeholder_src($current)) { + return; + } + $candidates = array_values(array_filter($absolutised, static fn (array $e): bool => $e['w'] > 0)); + if ($candidates !== []) { + usort($candidates, static fn (array $a, array $b): int => $a['w'] <=> $b['w']); + } else { + $candidates = array_values(array_filter($absolutised, static fn (array $e): bool => $e['x'] > 0.0)); + usort($candidates, static fn (array $a, array $b): int => $a['x'] <=> $b['x']); + } + if ($candidates === []) { + return; + } + $element->setAttribute('src', $candidates[0]['url']); + } + + /** + * A `src` value is treated as a lazy-load placeholder if it is empty, + * matches the de-facto universal 1x1 transparent GIF marker (base64 + * encoding of the GIF89a header for a 1x1 transparent image), or is + * any other `data:` URI under 128 characters. The threshold sits + * between common placeholder sizes (~70-120 chars) and the smallest + * useful inline rasters (~200+ chars). + */ + private function is_placeholder_src(string $src): bool + { + if ($src === '') { + return true; + } + return str_starts_with($src, 'data:') && + (strlen($src) < 128 || str_starts_with($src, 'data:image/gif;base64,R0lGODlh')); + } + + /** + * Follows the WHATWG srcset parsing algorithm rather than splitting on + * every comma: a URL is a run of non-whitespace, so unencoded commas + * inside URLs (Cloudinary, WordPress image CDNs) do not break entries + * apart. A comma only separates entries when it ends a URL or follows + * a descriptor. + * + * @return list + * `descriptor` is `''` when the source entry has no descriptor + * (browser implies `1x`, so `x` is 1.0); `w` is 0 and `x` is + * 0.0 when the descriptor is not of that kind. Both are used + * when picking a fallback `src`. + */ + private function parse_srcset(string $srcset): array + { + $out = []; + $len = strlen($srcset); + $pos = 0; + while ($pos < $len) { + // Skip whitespace and separating commas + while ($pos < $len && (ctype_space($srcset[$pos]) || $srcset[$pos] === ',')) { + $pos++; + } + $start = $pos; + while ($pos < $len && !ctype_space($srcset[$pos])) { + $pos++; + } + $url = substr($srcset, $start, $pos - $start); + if ($url === '') { + break; + } + $descriptor = ''; + if (str_ends_with($url, ',')) { + // Trailing comma terminates the entry: URL without descriptor + $url = rtrim($url, ','); + } else { + $end = strpos($srcset, ',', $pos); + if ($end === false) { + $end = $len; + } + $descriptor = trim(substr($srcset, $pos, $end - $pos)); + $pos = $end; + } + if ($url === '') { + continue; + } + $w = (preg_match('/^(\d+)w$/', $descriptor, $m) === 1) ? (int)$m[1] : 0; + if ($descriptor === '') { + $x = 1.0; + } else { + $x = (preg_match('/^(\d+(?:\.\d+)?)x$/', $descriptor, $m) === 1) ? (float)$m[1] : 0.0; + } + $out[] = ['url' => $url, 'descriptor' => $descriptor, 'w' => $w, 'x' => $x]; + } + return $out; + } } class_alias('SimplePie\Sanitize', 'SimplePie_Sanitize'); diff --git a/tests/SanitizeSrcsetTest.php b/tests/SanitizeSrcsetTest.php new file mode 100644 index 000000000..2f2a80ce6 --- /dev/null +++ b/tests/SanitizeSrcsetTest.php @@ -0,0 +1,209 @@ +set_registry(new SimplePie_Registry()); + $sanitize->allowed_html_elements_with_attributes([ + 'picture' => [], + 'source' => ['type', 'src', 'srcset', 'sizes', 'media', 'height', 'width'], + 'img' => ['src', 'srcset', 'sizes', 'alt', 'width', 'height'], + ]); + + return $sanitize->sanitize($html, SIMPLEPIE_CONSTRUCT_HTML, $base); + } + + public function testPicksSmallestSrcsetWidthWhenSrcIsDataUri(): void + { + // `src` is a fallback for clients that can't honour `srcset`; the smallest + // entry is the safest by bandwidth and never larger than the browser pick. + $html = 'x'; + $out = $this->sanitize($html); + self::assertStringContainsString('src="https://example.com/s.jpg"', $out); + self::assertStringNotContainsString('data:image/gif', $out); + } + + public function testRecognisesGifPlaceholderRegardlessOfLength(): void + { + // The R0lGODlh prefix is the de-facto universal 1x1 transparent GIF marker; + // treat it as a placeholder even if padded past the 128-char threshold. + $gif = 'data:image/gif;base64,R0lGODlh' . str_repeat('A', 200); + $html = 'x'; + $out = $this->sanitize($html); + self::assertStringContainsString('src="https://example.com/real.jpg"', $out); + self::assertStringNotContainsString('R0lGODlh', $out); + } + + public function testRetainsSrcsetAttribute(): void + { + $html = 'x'; + $out = $this->sanitize($html); + self::assertStringContainsString('srcset=', $out); + self::assertStringContainsString('https://example.com/a.jpg 100w', $out); + self::assertStringContainsString('https://example.com/b.jpg 500w', $out); + } + + public function testRetainsSizesAttribute(): void + { + $html = 'x'; + $out = $this->sanitize($html); + self::assertStringContainsString('sizes="(max-width: 800px) 100vw, 800px"', $out); + } + + public function testAbsolutisesRelativeSrcsetUrls(): void + { + $html = 'x'; + $out = $this->sanitize($html, 'https://example.com/articles/page'); + self::assertStringContainsString('https://example.com/img/a.jpg 100w', $out); + self::assertStringContainsString('https://example.com/img/b.jpg 500w', $out); + self::assertStringNotContainsString('srcset="/img/', $out); + } + + public function testKeepsLegitimateImgSrc(): void + { + $html = 'x'; + $out = $this->sanitize($html); + self::assertStringContainsString('src="https://example.com/real.jpg"', $out); + } + + public function testFallsBackToLowestDensityWhenNoWidthEntries(): void + { + // No Nw entries: clients that only read src would keep the placeholder, + // so fall back to the lowest-density entry instead. + $html = 'x'; + $out = $this->sanitize($html); + self::assertStringContainsString('src="https://example.com/a.jpg"', $out); + } + + public function testPrefersWidthOverDensityForFallbackSrc(): void + { + $html = 'x'; + $out = $this->sanitize($html); + self::assertStringContainsString('src="https://example.com/w.jpg"', $out); + } + + public function testAbsolutisesDensityOnlySrcsetUrls(): void + { + $html = 'x'; + $out = $this->sanitize($html, 'https://example.com/page'); + self::assertStringContainsString('https://example.com/img/a.jpg 1x', $out); + self::assertStringContainsString('https://example.com/img/a@2x.jpg 2x', $out); + } + + public function testKeepsDensityEntriesWhenMixedWithWidthEntries(): void + { + $html = 'x'; + $out = $this->sanitize($html); + self::assertStringContainsString('https://example.com/a.jpg 100w', $out); + self::assertStringContainsString('https://example.com/b.jpg 500w', $out); + self::assertStringContainsString('https://example.com/c@2x.jpg 2x', $out); + } + + public function testDoesNotDoubleEncodeAmpersandFromSrcset(): void + { + $html = 'x'; + $out = $this->sanitize($html); + self::assertStringNotContainsString('&amp;', $out); + } + + public function testNoOpOnImgWithoutSrcset(): void + { + $html = 'x'; + $out = $this->sanitize($html); + self::assertStringContainsString('src="https://example.com/x.jpg"', $out); + } + + public function testPreservesBareSrcsetEntryWithoutDescriptor(): void + { + $html = 'x'; + $out = $this->sanitize($html); + self::assertStringContainsString('srcset="https://example.com/a.jpg"', $out); + self::assertStringNotContainsString('https://example.com/a.jpg 1x', $out); + } + + public function testRewritesSourceSrcsetInPicture(): void + { + $html = '' + . '' + . 'x' + . ''; + $out = $this->sanitize($html, 'https://example.com/page'); + self::assertStringContainsString(' has no src attribute even if the img placeholder logic considered it. + self::assertDoesNotMatchRegularExpression('/]*\ssrc=/', $out); + } + + public function testPreservesLongInlineBase64Src(): void + { + $big = 'data:image/png;base64,' . str_repeat('A', 300); + $html = 'x'; + $out = $this->sanitize($html); + self::assertStringContainsString('src="' . $big . '"', $out); + } + + public function testPreservesMidSizedInlineBase64Src(): void + { + // ~150 chars: comfortably larger than typical 1x1 placeholders but small + // enough that a loose threshold would have misclassified it. + $mid = 'data:image/png;base64,' . str_repeat('A', 150); + $html = 'x'; + $out = $this->sanitize($html); + self::assertStringContainsString('src="' . $mid . '"', $out); + } + + public function testKeepsCommasInsideSrcsetUrls(): void + { + // Cloudinary-style transformation URLs contain unencoded commas; + // splitting on every comma would corrupt them. + $html = 'x'; + $out = $this->sanitize($html); + self::assertStringContainsString('https://res.example.com/upload/w_300,c_scale/a.jpg 300w', $out); + self::assertStringContainsString('https://res.example.com/upload/w_600,c_scale/a.jpg 600w', $out); + self::assertStringContainsString('src="https://res.example.com/upload/w_300,c_scale/a.jpg"', $out); + } + + public function testParsesSrcsetEntriesSeparatedByCommaWithoutSpace(): void + { + $html = 'x'; + $out = $this->sanitize($html); + self::assertStringContainsString('https://example.com/a.jpg 100w', $out); + self::assertStringContainsString('https://example.com/b.jpg 500w', $out); + self::assertStringContainsString('src="https://example.com/a.jpg"', $out); + } + + public function testDropsDisallowedSchemesFromSrcset(): void + { + // The srcset rewrite runs after replace_urls, so a disallowed scheme must + // not survive in srcset nor be written into the src fallback. + $html = 'x'; + $out = $this->sanitize($html); + self::assertStringNotContainsString('javascript:', $out); + self::assertStringContainsString('src="https://example.com/a.jpg"', $out); + } + + public function testRemovesSrcsetWhenAllEntriesDisallowed(): void + { + $html = 'x'; + $out = $this->sanitize($html); + self::assertStringNotContainsString('srcset', $out); + self::assertStringContainsString('src="https://example.com/x.jpg"', $out); + } +} From 6f17958832c51d8dcbfe890878e330a2a6cd7124 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B8rn=20A=2E=20Andersen?= Date: Sat, 25 Jul 2026 22:01:38 +0200 Subject: [PATCH 2/6] test: move srcset tests to tests/Unit Match the namespaced location used by the current Sanitize tests instead of the legacy root tests/ directory. --- tests/{ => Unit}/SanitizeSrcsetTest.php | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) rename tests/{ => Unit}/SanitizeSrcsetTest.php (97%) diff --git a/tests/SanitizeSrcsetTest.php b/tests/Unit/SanitizeSrcsetTest.php similarity index 97% rename from tests/SanitizeSrcsetTest.php rename to tests/Unit/SanitizeSrcsetTest.php index 2f2a80ce6..03bee48c8 100644 --- a/tests/SanitizeSrcsetTest.php +++ b/tests/Unit/SanitizeSrcsetTest.php @@ -5,21 +5,25 @@ declare(strict_types=1); +namespace SimplePie\Tests\Unit; + use PHPUnit\Framework\TestCase; +use SimplePie\Registry; +use SimplePie\Sanitize; class SanitizeSrcsetTest extends TestCase { private function sanitize(string $html, string $base = 'https://example.com/'): string { - $sanitize = new SimplePie_Sanitize(); - $sanitize->set_registry(new SimplePie_Registry()); + $sanitize = new Sanitize(); + $sanitize->set_registry(new Registry()); $sanitize->allowed_html_elements_with_attributes([ 'picture' => [], 'source' => ['type', 'src', 'srcset', 'sizes', 'media', 'height', 'width'], 'img' => ['src', 'srcset', 'sizes', 'alt', 'width', 'height'], ]); - return $sanitize->sanitize($html, SIMPLEPIE_CONSTRUCT_HTML, $base); + return $sanitize->sanitize($html, \SimplePie\SimplePie::CONSTRUCT_HTML, $base); } public function testPicksSmallestSrcsetWidthWhenSrcIsDataUri(): void From b2988f03cb9ab5421b069c8f0d5673eb15a73362 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B8rn=20A=2E=20Andersen?= Date: Sat, 25 Jul 2026 22:10:32 +0200 Subject: [PATCH 3/6] test: cover empty src resolved against the base Documents why the allowed-node pass has to run before replace_urls(). --- tests/Unit/SanitizeSrcsetTest.php | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/Unit/SanitizeSrcsetTest.php b/tests/Unit/SanitizeSrcsetTest.php index 03bee48c8..a1405dbe6 100644 --- a/tests/Unit/SanitizeSrcsetTest.php +++ b/tests/Unit/SanitizeSrcsetTest.php @@ -75,6 +75,18 @@ public function testAbsolutisesRelativeSrcsetUrls(): void self::assertStringNotContainsString('srcset="/img/', $out); } + public function testEmptySrcWithBaseStillGetsSrcsetFallback(): void + { + // The allowed-node pass must run before `replace_urls()`. Otherwise an + // empty `src` is resolved to the document base first, the placeholder + // check no longer recognises it, and `src` is left pointing at the + // page the feed item came from instead of an image. + $html = 'x'; + $out = $this->sanitize($html, 'https://example.com/articles/page'); + self::assertStringContainsString('src="https://example.com/img/a.jpg"', $out); + self::assertStringNotContainsString('src="https://example.com/articles/page"', $out); + } + public function testKeepsLegitimateImgSrc(): void { $html = 'x'; From 3b2d1137ebc7d91711c84f95a0cd87f735fe9cba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B8rn=20A=2E=20Andersen?= Date: Sat, 25 Jul 2026 23:48:37 +0200 Subject: [PATCH 4/6] fix: use strpos in the data-/aria- attribute check str_starts_with is PHP 8.0+, but composer.json declares >=7.2 and CI tests it. No existing test called allowed_html_elements_with_attributes(), so this path was never reached on the old matrix entries. --- src/Sanitize.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Sanitize.php b/src/Sanitize.php index 5575bf771..3f5fbb682 100644 --- a/src/Sanitize.php +++ b/src/Sanitize.php @@ -750,8 +750,8 @@ protected function enforce_allowed_html_nodes(\DOMNode $element, bool $allow_dat for ($i = $element->attributes->length - 1; $i >= 0; $i--) { $attr = $element->attributes[$i]->nodeName; // Skip data-*, aria-* if allowed - if (($allow_data_attr && str_starts_with($attr, 'data-')) - || ($allow_aria_attr && str_starts_with($attr, 'aria-'))) { + if (($allow_data_attr && strpos($attr, 'data-') === 0) + || ($allow_aria_attr && strpos($attr, 'aria-') === 0)) { continue; } From 590032de912d4ea69221c0819451d69995d5f21d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B8rn=20A=2E=20Andersen?= Date: Sat, 25 Jul 2026 23:49:20 +0200 Subject: [PATCH 5/6] fix: keep srcset handling inside the PHP 7.2 baseline Arrow functions are 7.4+ and str_starts_with/str_ends_with are 8.0+, so Sanitize.php failed to parse on PHP 7.2 and 7.3. substr() returns string|false below 8.0, which broke the parse_srcset() return type under PHPStan on 7.4. assertDoesNotMatchRegularExpression is PHPUnit 9.1+ while composer.json still allows ^8. --- src/Sanitize.php | 30 ++++++++++++++++++++---------- tests/Unit/SanitizeSrcsetTest.php | 4 +++- 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/src/Sanitize.php b/src/Sanitize.php index 3f5fbb682..e14548ea7 100644 --- a/src/Sanitize.php +++ b/src/Sanitize.php @@ -1045,7 +1045,9 @@ private function rewrite_img_srcset(\DOMElement $element): void return; } $element->setAttribute('srcset', implode(', ', array_map( - static fn (array $e): string => $e['descriptor'] === '' ? $e['url'] : $e['url'] . ' ' . $e['descriptor'], + static function (array $e): string { + return $e['descriptor'] === '' ? $e['url'] : $e['url'] . ' ' . $e['descriptor']; + }, $absolutised ))); @@ -1058,12 +1060,20 @@ private function rewrite_img_srcset(\DOMElement $element): void if (!$this->is_placeholder_src($current)) { return; } - $candidates = array_values(array_filter($absolutised, static fn (array $e): bool => $e['w'] > 0)); + $candidates = array_values(array_filter($absolutised, static function (array $e): bool { + return $e['w'] > 0; + })); if ($candidates !== []) { - usort($candidates, static fn (array $a, array $b): int => $a['w'] <=> $b['w']); + usort($candidates, static function (array $a, array $b): int { + return $a['w'] <=> $b['w']; + }); } else { - $candidates = array_values(array_filter($absolutised, static fn (array $e): bool => $e['x'] > 0.0)); - usort($candidates, static fn (array $a, array $b): int => $a['x'] <=> $b['x']); + $candidates = array_values(array_filter($absolutised, static function (array $e): bool { + return $e['x'] > 0.0; + })); + usort($candidates, static function (array $a, array $b): int { + return $a['x'] <=> $b['x']; + }); } if ($candidates === []) { return; @@ -1084,8 +1094,8 @@ private function is_placeholder_src(string $src): bool if ($src === '') { return true; } - return str_starts_with($src, 'data:') && - (strlen($src) < 128 || str_starts_with($src, 'data:image/gif;base64,R0lGODlh')); + return strpos($src, 'data:') === 0 && + (strlen($src) < 128 || strpos($src, 'data:image/gif;base64,R0lGODlh') === 0); } /** @@ -1115,12 +1125,12 @@ private function parse_srcset(string $srcset): array while ($pos < $len && !ctype_space($srcset[$pos])) { $pos++; } - $url = substr($srcset, $start, $pos - $start); + $url = (string) substr($srcset, $start, $pos - $start); if ($url === '') { break; } $descriptor = ''; - if (str_ends_with($url, ',')) { + if (substr($url, -1) === ',') { // Trailing comma terminates the entry: URL without descriptor $url = rtrim($url, ','); } else { @@ -1128,7 +1138,7 @@ private function parse_srcset(string $srcset): array if ($end === false) { $end = $len; } - $descriptor = trim(substr($srcset, $pos, $end - $pos)); + $descriptor = trim((string) substr($srcset, $pos, $end - $pos)); $pos = $end; } if ($url === '') { diff --git a/tests/Unit/SanitizeSrcsetTest.php b/tests/Unit/SanitizeSrcsetTest.php index a1405dbe6..3a8248d7d 100644 --- a/tests/Unit/SanitizeSrcsetTest.php +++ b/tests/Unit/SanitizeSrcsetTest.php @@ -162,7 +162,9 @@ public function testRewritesSourceSrcsetInPicture(): void self::assertStringContainsString('https://example.com/img/b.jpg 1500w', $out); self::assertStringContainsString('sizes="100vw"', $out); // has no src attribute even if the img placeholder logic considered it. - self::assertDoesNotMatchRegularExpression('/]*\ssrc=/', $out); + // preg_match rather than a regex assertion: the assertion was renamed between + // the PHPUnit majors this package supports. + self::assertSame(0, preg_match('/]*\ssrc=/', $out)); } public function testPreservesLongInlineBase64Src(): void From a1c0195fa5f75c19e2ad3714664161410361386b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B8rn=20A=2E=20Andersen?= Date: Sat, 25 Jul 2026 23:49:39 +0200 Subject: [PATCH 6/6] fix: force HTTPS on srcset URLs as replace_urls does for src srcset entries were absolutised and scheme-checked but never passed through https_url(), so on a forced-HTTPS domain src was upgraded and srcset was not, leaving the browser to pick an http candidate. The scheme check now also mirrors the disallowed_uri_schemes guard from replace_urls(); a blocked entry is still dropped rather than prefixed with unsafe:, since a candidate the browser must never pick has no reason to stay in the list. The test helper now calls set_url_replacements() so replace_urls() actually runs, which is how SimplePie configures the sanitizer. --- src/Sanitize.php | 36 ++++++++++++++++++++----------- tests/Unit/SanitizeSrcsetTest.php | 34 ++++++++++++++++++++++++++--- 2 files changed, 54 insertions(+), 16 deletions(-) diff --git a/src/Sanitize.php b/src/Sanitize.php index e14548ea7..7e88cb541 100644 --- a/src/Sanitize.php +++ b/src/Sanitize.php @@ -1014,17 +1014,20 @@ private function get_http_client(): Client } /** - * Absolutise each URL in `srcset` on `` and `` against the - * document base, dropping entries with a disallowed URI scheme (this - * runs after `replace_urls`, so entries written into `src` below would - * otherwise bypass that scheme check). For `` only, when `src` is - * empty or a recognised placeholder (lazy-loading pattern), write the - * smallest `Nw` entry (or, failing that, the lowest-density `Nx` entry) - * as a fallback. The browser uses `srcset` for actual selection; `src` - * only loads when `srcset` can't be honoured (legacy browsers, - * non-browser API consumers), so the smallest is the safest fallback - * by bandwidth and is never larger than what `srcset` would have - * picked. + * Give every URL in `srcset` the same treatment `replace_urls()` gives a + * single-URL attribute: absolutise against the document base, block a + * disallowed URI scheme, force HTTPS where configured. `srcset` holds a + * list rather than one URL, so it cannot go in `replace_url_attributes` + * and nothing else in the pipeline touches it. + * + * For `` only, when `src` is empty or a recognised placeholder + * (lazy-loading pattern), write the smallest `Nw` entry (or, failing + * that, the lowest-density `Nx` entry) as a fallback. The browser uses + * `srcset` for actual selection; `src` only loads when `srcset` can't be + * honoured (legacy browsers, non-browser API consumers), so the smallest + * is the safest fallback by bandwidth and is never larger than what + * `srcset` would have picked. This runs before `replace_urls()`, so that + * `src` still goes through the normal single-URL pass afterwards. */ private function rewrite_img_srcset(\DOMElement $element): void { @@ -1035,10 +1038,17 @@ private function rewrite_img_srcset(\DOMElement $element): void $absolutised = []; foreach ($entries as $e) { $abs = $this->registry->call(Misc::class, 'absolutize_url', [$e['url'], $this->base]); - if (!is_string($abs) || $abs === '' || !$this->is_allowed_scheme($abs)) { + if (!is_string($abs) || $abs === '') { + continue; + } + // Same condition as replace_urls(), but a blocked entry is dropped + // instead of being kept with an `unsafe:` prefix: an entry the + // browser must never pick has no reason to stay in the candidate + // list, and unlike `src` the attribute tolerates being shorter. + if ($this->disallowed_uri_schemes !== [] && !$this->is_allowed_scheme($abs)) { continue; } - $absolutised[] = ['url' => $abs, 'descriptor' => $e['descriptor'], 'w' => $e['w'], 'x' => $e['x']]; + $absolutised[] = ['url' => $this->https_url($abs), 'descriptor' => $e['descriptor'], 'w' => $e['w'], 'x' => $e['x']]; } if ($absolutised === []) { $element->removeAttribute('srcset'); diff --git a/tests/Unit/SanitizeSrcsetTest.php b/tests/Unit/SanitizeSrcsetTest.php index 3a8248d7d..f3eff4521 100644 --- a/tests/Unit/SanitizeSrcsetTest.php +++ b/tests/Unit/SanitizeSrcsetTest.php @@ -13,10 +13,17 @@ class SanitizeSrcsetTest extends TestCase { - private function sanitize(string $html, string $base = 'https://example.com/'): string + /** + * @param array $https_domains + */ + private function sanitize(string $html, string $base = 'https://example.com/', array $https_domains = []): string { $sanitize = new Sanitize(); $sanitize->set_registry(new Registry()); + // Configure as SimplePie does, so srcset handling is exercised alongside + // the single-URL replace_urls() pass rather than in isolation. + $sanitize->set_url_replacements(); + $sanitize->set_https_domains($https_domains); $sanitize->allowed_html_elements_with_attributes([ 'picture' => [], 'source' => ['type', 'src', 'srcset', 'sizes', 'media', 'height', 'width'], @@ -209,14 +216,35 @@ public function testParsesSrcsetEntriesSeparatedByCommaWithoutSpace(): void public function testDropsDisallowedSchemesFromSrcset(): void { - // The srcset rewrite runs after replace_urls, so a disallowed scheme must - // not survive in srcset nor be written into the src fallback. + // srcset is never seen by replace_urls, so a disallowed scheme must be + // blocked here, and must not reach the src fallback either. $html = 'x'; $out = $this->sanitize($html); self::assertStringNotContainsString('javascript:', $out); self::assertStringContainsString('src="https://example.com/a.jpg"', $out); } + public function testForcesHttpsInSrcsetAsWellAsSrc(): void + { + // replace_urls() upgrades src on a forced-HTTPS domain. srcset has to get + // the same upgrade here, otherwise the browser picks a candidate and loads + // it over http: mixed content on exactly the images this rewrite targets. + $html = 'x'; + $out = $this->sanitize($html, 'https://example.com/', ['cdn.example.com']); + self::assertStringContainsString('src="https://cdn.example.com/p.jpg"', $out); + self::assertStringContainsString('https://cdn.example.com/a.jpg 100w', $out); + self::assertStringContainsString('https://cdn.example.com/b.jpg 500w', $out); + self::assertStringNotContainsString('http://cdn.example.com', $out); + } + + public function testLeavesHttpSrcsetAloneOffForcedHttpsDomain(): void + { + $html = 'x'; + $out = $this->sanitize($html, 'https://example.com/', ['cdn.example.com']); + self::assertStringContainsString('http://other.example.net/a.jpg 100w', $out); + } + public function testRemovesSrcsetWhenAllEntriesDisallowed(): void { $html = 'x';