Skip to content

Sanitizer: resolve img/source srcset URLs and add a src fallback#87

Open
polybjorn wants to merge 6 commits into
FreshRSS:freshrssfrom
polybjorn:fix/img-srcset-placeholder
Open

Sanitizer: resolve img/source srcset URLs and add a src fallback#87
polybjorn wants to merge 6 commits into
FreshRSS:freshrssfrom
polybjorn:fix/img-srcset-placeholder

Conversation

@polybjorn

@polybjorn polybjorn commented Jul 25, 2026

Copy link
Copy Markdown

FreshRSS drops srcset unless it is whitelisted, and even when allowed the URLs inside are never resolved, because replace_urls() only rewrites single-URL attributes. Relative srcset URLs leak, and lazy-loaded images (placeholder in src, real images in srcset) show only the placeholder.

This handles srcset in the allowed-node pass of Sanitize:

  • Each srcset URL on <img> and <source> is resolved against the document base, and entries with a disallowed scheme are removed.
  • On <img>, when src is empty or a placeholder (1x1 transparent GIF, or a data: URI under 128 chars), the smallest entry is copied into src as a fallback.
  • Parsing follows the WHATWG srcset rules, so unencoded commas in URLs (Cloudinary, WordPress) do not split entries.

It only runs when a caller allows srcset through allowed_html_elements_with_attributes(), so default output is unchanged.

Ports the Sanitize change from FreshRSS/FreshRSS#8843, which currently patches the vendored copy of this file directly. Once this lands and the library is re-vendored, that PR keeps only its SimplePieCustom.php allowlist change.

Test plan

tests/Unit/SanitizeSrcsetTest.php adds 23 tests covering URL resolution, placeholder detection, fallback selection, comma-in-URL parsing, scheme blocking, HTTPS forcing, and <picture><source>. composer test, composer phpstan and composer cs pass, and the existing SanitizeTest is unchanged.

Resolve each srcset URL against the document base and drop disallowed
schemes, since replace_urls() only handles single-URL attributes. On <img>,
copy the smallest entry into src when src is empty or a placeholder.
polybjorn added a commit to polybjorn/FreshRSS that referenced this pull request Jul 25, 2026
An empty src was resolved to the document base before the placeholder
check ran, so src pointed at the article page instead of falling back to
a srcset entry. Restore the original node-pass ordering and set the base
ahead of it. Naming matches FreshRSS/simplepie#87 so the vendored file
stays in sync.
@polybjorn
polybjorn marked this pull request as ready for review July 25, 2026 19:58
Match the namespaced location used by the current Sanitize tests instead
of the legacy root tests/ directory.
Documents why the allowed-node pass has to run before replace_urls().
@Alkarex

Alkarex commented Jul 25, 2026

Copy link
Copy Markdown
Member

Please try to address the CI/CD failures if you can

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.
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.
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.
@polybjorn

Copy link
Copy Markdown
Author

Done. Three causes:

  • Arrow functions and str_starts_with need PHP 7.4 and 8.0, so Sanitize.php did not parse on 7.2 and 7.3.
  • substr() returns string|false below PHP 8.0, which broke the declared return type of parse_srcset().
  • assertDoesNotMatchRegularExpression needs PHPUnit 9.1, but composer.json allows 8, which is what PHP 7.2 installs.

Two things outside the CI fix:

  • enforce_allowed_html_nodes() already called str_starts_with, so it would fatal on PHP 7.2 and 7.3. No test reached it before, because none set allowed_html_elements_with_attributes(). Fixed here as well.
  • srcset URLs never went through https_url(). With set_https_domains() set, src came out HTTPS while srcset stayed HTTP, so the browser would load the image over HTTP. srcset now goes through the same steps as src.

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