Fix: infinite recursion in sanitizeHTML() when maxLength truncation doesn't converge (#9043)

* Fix: infinite recursion in sanitizeHTML() when maxLength truncation doesn't converge

Sanitizing can grow a truncated HTML fragment (e.g. an unclosed tag gets
auto-closed), so the previous recursive shrink-and-retry could hit a fixed
point and recurse forever, crashing on a stack overflow. Bound the retries
and fall back to a hard truncation that always terminates.

Add unit tests for FreshRSS_SimplePieCustom::sanitizeHTML(), the XSS
sanitization applied to all untrusted feed content, which had no coverage.

* Update app/Models/SimplePieCustom.php

Co-authored-by: Frans de Jonge <fransdejonge@gmail.com>

* Some refactoring
Avoid two mb_strcut in a row. More cases for fallback. Better fallback. More edge cases.

* Address trailing incomplete tag or entity

* Whitespace reduction

---------

Co-authored-by: Alexandre Alapetite <alexandre@alapetite.fr>
Co-authored-by: Frans de Jonge <fransdejonge@gmail.com>

app/Models/SimplePieCustom.php

@@ -286,9 +286,6 @@ final class FreshRSS_SimplePieCustom extends \SimplePie\SimplePie
 		if ($data === '' || ($maxLength !== null && $maxLength <= 0)) {
 			return '';
 		}
-		if ($maxLength !== null) {
-			$data = mb_strcut($data, 0, $maxLength, 'UTF-8');
-		}
 		/** @var FreshRSS_SimplePieCustom|null $simplePie */
 		static $simplePie = null;
 		if ($simplePie === null) {
@@ -296,16 +293,41 @@ final class FreshRSS_SimplePieCustom extends \SimplePie\SimplePie
 			$simplePie->enable_cache(false);
 			$simplePie->init();
 		}
+
+		// Sanitize and truncate, accounting for the fact that sanitizing can grow the string (e.g. auto-closing tags)
+		$data = html_only_entity_decode($data);
+		$truncated = $data;
+		$truncLength = $maxLength;
+		for ($attempt = 0; $attempt < 4; $attempt++) {
+			if ($maxLength !== null) {
+				$truncated = mb_strcut($truncated, 0, $truncLength, 'UTF-8');
+				$truncated = preg_replace('%(<[^>]{0,99}|&[^;]{0,32})$%', '', $truncated);	// Remove trailing incomplete tag or entity
+				if (!is_string($truncated)) {
+					break;
+				}
+			}
+			$sanitized = $simplePie->sanitize->sanitize($truncated, \SimplePie\SimplePie::CONSTRUCT_HTML, $base);
+			if (!is_string($sanitized)) {
+				break;
+			}
+			if ($maxLength === null) {
+				return $sanitized;
+			}
+			if (strlen($sanitized) <= $maxLength) {
+				if (trim(strip_tags($sanitized)) === '') {
+					break;	// Fallback
+				}
+				return $sanitized;
+			}
+			// Exponentially reduce the input length to account for the fact that sanitizing can grow the string
+			$overflow = strlen($sanitized) - $maxLength;
+			$truncLength = max(0, strlen($truncated) - $overflow - (2 ** $attempt));
+		}
+		// Our heuristic failed, so fallback to sanitize + strip tags + hard-truncate
 		$sanitized = $simplePie->sanitize->sanitize($data, \SimplePie\SimplePie::CONSTRUCT_HTML, $base);
 		if (!is_string($sanitized)) {
 			return '';
 		}
-		$result = html_only_entity_decode($sanitized);
-		if ($maxLength !== null && strlen($result) > $maxLength) {
-			//Sanitizing has made the result too long so try again shorter
-			$data = mb_strcut($result, 0, (2 * $maxLength) - strlen($result) - 2, 'UTF-8');
-			return self::sanitizeHTML($data, $base, $maxLength);
-		}
-		return $result;
+		return mb_strcut(strip_tags($sanitized), 0, $maxLength, 'UTF-8');
 	}
 }

tests/app/Models/SimplePieCustomTest.php

@@ -0,0 +1,93 @@
+<?php
+declare(strict_types=1);
+
+use PHPUnit\Framework\Attributes\DataProvider;
+
+/**
+ * FreshRSS_SimplePieCustom::sanitizeHTML() is the XSS defence applied to all untrusted feed
+ * content (entry content, entry/feed descriptions) before it is stored or displayed.
+ */
+final class SimplePieCustomTest extends \PHPUnit\Framework\TestCase {
+
+	#[\Override]
+	public static function setUpBeforeClass(): void {
+		FreshRSS_Context::initSystem();
+	}
+
+	public static function test_sanitizeHTML_whenEmptyString_returnsEmptyString(): void {
+		self::assertSame('', FreshRSS_SimplePieCustom::sanitizeHTML(''));
+	}
+
+	public static function test_sanitizeHTML_whenPlainText_returnsUnchanged(): void {
+		self::assertSame('plain text', FreshRSS_SimplePieCustom::sanitizeHTML('plain text'));
+	}
+
+	#[DataProvider('provideMaliciousHtml')]
+	public static function test_sanitizeHTML_whenMaliciousInput_stripsDangerousContent(string $input, string $mustNotContain): void {
+		$result = FreshRSS_SimplePieCustom::sanitizeHTML($input);
+		self::assertStringNotContainsString($mustNotContain, $result);
+	}
+
+	/** @return Traversable<string,array{string,string}> */
+	public static function provideMaliciousHtml(): Traversable {
+		yield 'script tag' => ['<script>alert(1)</script>Hello', '<script'];
+		yield 'inline event handler' => ['<img src="x" onerror="alert(1)">', 'onerror'];
+		yield 'JavaScript URL' => ['<a href="javascript:alert(1)">click</a>', 'href="javascript:'];
+		yield 'style tag' => ['<style>body{display:none}</style>Hello', '<style'];
+	}
+
+	public static function test_sanitizeHTML_whenSafeHtml_keepsAllowedTags(): void {
+		$result = FreshRSS_SimplePieCustom::sanitizeHTML('<p>Hello <b>world</b></p>');
+		self::assertSame('<p>Hello <b>world</b></p>', $result);
+	}
+
+	public static function test_sanitizeHTML_whenUnsafeAttributeIsRemoved_keepsAllowedTag(): void {
+		self::assertSame('Hello <br>', FreshRSS_SimplePieCustom::sanitizeHTML('Hello <br onclick="x">'));
+		self::assertSame('Hello <br>', FreshRSS_SimplePieCustom::sanitizeHTML('Hello <br onclick="x">', maxLength: 100));
+	}
+
+	public static function test_sanitizeHTML_whenMaxLengthIsZeroOrNegative_returnsEmptyString(): void {
+		self::assertSame('', FreshRSS_SimplePieCustom::sanitizeHTML('<p>Hello world</p>', maxLength: 0));
+		self::assertSame('', FreshRSS_SimplePieCustom::sanitizeHTML('<p>Hello world</p>', maxLength: -1));
+	}
+
+	public static function test_sanitizeHTML_whenResultFitsWithinMaxLength_isUnaffected(): void {
+		$result = FreshRSS_SimplePieCustom::sanitizeHTML('<p>Hello world</p>', maxLength: 100);
+		self::assertSame('<p>Hello world</p>', $result);
+	}
+
+	public static function test_sanitizeHTML_whenUnsafePrefixExceedsMaxLength_keepsSafeText(): void {
+		self::assertSame('text', FreshRSS_SimplePieCustom::sanitizeHTML('<script>NOK</script><p>text', maxLength: 5));
+	}
+
+	/**
+	 * Sanitizing can grow a truncated fragment (e.g. `<p>He` gets sanitized into `<p>He</p>`)
+	 */
+	#[DataProvider('provideMaxLengthInputs')]
+	public static function test_sanitizeHTML_whenMaxLengthForcesReSanitizing_terminatesWithinBound(string $input, int $maxLength): void {
+		$result = FreshRSS_SimplePieCustom::sanitizeHTML($input, maxLength: $maxLength);
+		self::assertLessThanOrEqual($maxLength, strlen($result));
+	}
+
+	/** @return Traversable<string,array{string,int}> */
+	public static function provideMaxLengthInputs(): Traversable {
+		yield 'unclosed tag' => ['<p>Hello world</p>', 5];
+		yield 'repeated short tags' => [str_repeat('<b>x</b> ', 50), 20];
+		yield 'single-character budget' => ['<p>Hello world</p>', 1];
+	}
+
+	#[DataProvider('provideIncompleteTagsOrEntities')]
+	public static function test_sanitizeHTML_Cases(string $input, int $maxLength, string $expected): void {
+		$result = FreshRSS_SimplePieCustom::sanitizeHTML($input, maxLength: $maxLength);
+		self::assertLessThanOrEqual($maxLength, strlen($result));
+		self::assertSame(trim($expected), trim($result));
+	}
+
+	/** @return Traversable<string,array{string,int,string}> */
+	public static function provideIncompleteTagsOrEntities(): Traversable {
+		yield 'unclosed tag not fitting' => ['<span>Hello</span> <span>World', 31, '<span>Hello</span>'];
+		yield 'unclosed entity' => ['Hello&#8230;', 9, 'Hello'];
+		yield 'double unclosed tag' => ['<b> <b>x', 10, 'x'];
+		yield 'triple unclosed tag' => [' <b><b><b>y', 20, 'y'];
+	}
+}