Dom: New allowHostRelativeUrls option

getkirby · Nov 6, 2023 · d4d0d22 · d4d0d22
1 parent e821214
commit d4d0d22
Show file tree

Hide file tree

Showing 33 changed files with 270 additions and 57 deletions.
diff --git a/src/Sane/DomHandler.php b/src/Sane/DomHandler.php
@@ -16,6 +16,8 @@
  * @link      https://getkirby.com
  * @copyright Bastian Allgeier
  * @license   https://opensource.org/licenses/MIT
+ *
+ * @SuppressWarnings(PHPMD.LongVariable)
  */
 class DomHandler extends Handler
 {
@@ -43,6 +45,13 @@ class DomHandler extends Handler
 	 */
 	public static array|bool $allowedDomains = true;
 
+	/**
+	 * Whether URLs that begin with `/` should be allowed even if the
+	 * site index URL is in a subfolder (useful when using the HTML
+	 * `<base>` element where the sanitized code will be rendered)
+	 */
+	public static bool $allowHostRelativeUrls = true;
+
 	/**
 	 * Names of allowed XML processing instructions
 	 */
@@ -57,25 +66,31 @@ class DomHandler extends Handler
 	/**
 	 * Sanitizes the given string
 	 *
+	 * @param bool $isExternal Whether the string is from an external file
+	 *                         that may be accessed directly
+	 *
 	 * @throws \Kirby\Exception\InvalidArgumentException If the file couldn't be parsed
 	 */
-	public static function sanitize(string $string): string
+	public static function sanitize(string $string, bool $isExternal = false): string
 	{
 		$dom = static::parse($string);
-		$dom->sanitize(static::options());
+		$dom->sanitize(static::options($isExternal));
 		return $dom->toString();
 	}
 
 	/**
 	 * Validates file contents
 	 *
+	 * @param bool $isExternal Whether the string is from an external file
+	 *                         that may be accessed directly
+	 *
 	 * @throws \Kirby\Exception\InvalidArgumentException If the file couldn't be parsed
 	 * @throws \Kirby\Exception\InvalidArgumentException If the file didn't pass validation
 	 */
-	public static function validate(string $string): void
+	public static function validate(string $string, bool $isExternal = false): void
 	{
 		$dom    = static::parse($string);
-		$errors = $dom->sanitize(static::options());
+		$errors = $dom->sanitize(static::options($isExternal));
 
 		// there may be multiple errors, we can only throw one of them at a time
 		if (count($errors) > 0) {
@@ -119,17 +134,29 @@ public static function validateDoctype(DOMDocumentType $doctype, array $options)
 	/**
 	 * Returns the sanitization options for the handler
 	 * (to be extended in child classes)
+	 *
+	 * @param bool $isExternal Whether the string is from an external file
+	 *                         that may be accessed directly
 	 */
-	protected static function options(): array
+	protected static function options(bool $isExternal): array
 	{
-		return [
-			'allowedDataUris' => static::$allowedDataUris,
-			'allowedDomains'  => static::$allowedDomains,
-			'allowedPIs'      => static::$allowedPIs,
-			'attrCallback'    => [static::class, 'sanitizeAttr'],
-			'doctypeCallback' => [static::class, 'validateDoctype'],
-			'elementCallback' => [static::class, 'sanitizeElement'],
+		$options = [
+			'allowedDataUris'       => static::$allowedDataUris,
+			'allowedDomains'        => static::$allowedDomains,
+			'allowHostRelativeUrls' => static::$allowHostRelativeUrls,
+			'allowedPIs'            => static::$allowedPIs,
+			'attrCallback'          => [static::class, 'sanitizeAttr'],
+			'doctypeCallback'       => [static::class, 'validateDoctype'],
+			'elementCallback'       => [static::class, 'sanitizeElement'],
 		];
+
+		// never allow host-relative URLs in external files as we
+		// cannot set a `<base>` element for them when accessed directly
+		if ($isExternal === true) {
+			$options['allowHostRelativeUrls'] = false;
+		}
+
+		return $options;
 	}
 
 	/**

diff --git a/src/Sane/Handler.php b/src/Sane/Handler.php
@@ -21,8 +21,11 @@ abstract class Handler
 {
 	/**
 	 * Sanitizes the given string
+	 *
+	 * @param bool $isExternal Whether the string is from an external file
+	 *                         that may be accessed directly
 	 */
-	abstract public static function sanitize(string $string): string;
+	abstract public static function sanitize(string $string, bool $isExternal = false): string;
 
 	/**
 	 * Sanitizes the contents of a file by overwriting
@@ -34,17 +37,20 @@ abstract public static function sanitize(string $string): string;
 	public static function sanitizeFile(string $file): void
 	{
 		$content   = static::readFile($file);
-		$sanitized = static::sanitize($content);
+		$sanitized = static::sanitize($content, isExternal: true);
 		F::write($file, $sanitized);
 	}
 
 	/**
 	 * Validates file contents
 	 *
+	 * @param bool $isExternal Whether the string is from an external file
+	 *                         that may be accessed directly
+	 *
 	 * @throws \Kirby\Exception\InvalidArgumentException If the file didn't pass validation
 	 * @throws \Kirby\Exception\Exception On other errors
 	 */
-	abstract public static function validate(string $string): void;
+	abstract public static function validate(string $string, bool $isExternal = false): void;
 
 	/**
 	 * Validates the contents of a file
@@ -56,7 +62,7 @@ abstract public static function validate(string $string): void;
 	public static function validateFile(string $file): void
 	{
 		$content = static::readFile($file);
-		static::validate($content);
+		static::validate($content, isExternal: true);
 	}
 
 	/**

diff --git a/src/Sane/Html.php b/src/Sane/Html.php
@@ -107,10 +107,13 @@ class Html extends DomHandler
 
 	/**
 	 * Returns the sanitization options for the handler
+	 *
+	 * @param bool $isExternal Whether the string is from an external file
+	 *                         that may be accessed directly
 	 */
-	protected static function options(): array
+	protected static function options(bool $isExternal): array
 	{
-		return array_merge(parent::options(), [
+		return array_merge(parent::options($isExternal), [
 			'allowedAttrPrefixes' => static::$allowedAttrPrefixes,
 			'allowedAttrs'        => static::$allowedAttrs,
 			'allowedNamespaces'   => [],

diff --git a/src/Sane/Sane.php b/src/Sane/Sane.php
@@ -77,10 +77,13 @@ public static function handler(
 	/**
 	 * Sanitizes the given string with the specified handler
 	 * @since 3.6.0
+	 *
+	 * @param bool $isExternal Whether the string is from an external file
+	 *                         that may be accessed directly
 	 */
-	public static function sanitize(string $string, string $type): string
+	public static function sanitize(string $string, string $type, bool $isExternal = false): string
 	{
-		return static::handler($type)->sanitize($string);
+		return static::handler($type)->sanitize($string, $isExternal);
 	}
 
 	/**
@@ -131,13 +134,16 @@ public static function sanitizeFile(
 	/**
 	 * Validates file contents with the specified handler
 	 *
+	 * @param bool $isExternal Whether the string is from an external file
+	 *                         that may be accessed directly
+	 *
 	 * @throws \Kirby\Exception\InvalidArgumentException If the file didn't pass validation
 	 * @throws \Kirby\Exception\NotFoundException If the handler was not found
 	 * @throws \Kirby\Exception\Exception On other errors
 	 */
-	public static function validate(string $string, string $type): void
+	public static function validate(string $string, string $type, bool $isExternal = false): void
 	{
-		static::handler($type)->validate($string);
+		static::handler($type)->validate($string, $isExternal);
 	}
 
 	/**

diff --git a/src/Sane/Svg.php b/src/Sane/Svg.php
@@ -465,10 +465,13 @@ public static function validateDoctype(DOMDocumentType $doctype, array $options)
 
 	/**
 	 * Returns the sanitization options for the handler
+	 *
+	 * @param bool $isExternal Whether the string is from an external file
+	 *                         that may be accessed directly
 	 */
-	protected static function options(): array
+	protected static function options(bool $isExternal): array
 	{
-		return array_merge(parent::options(), [
+		return array_merge(parent::options($isExternal), [
 			'allowedAttrPrefixes' => static::$allowedAttrPrefixes,
 			'allowedAttrs'        => static::$allowedAttrs,
 			'allowedNamespaces'   => static::$allowedNamespaces,

diff --git a/src/Sane/Svgz.php b/src/Sane/Svgz.php
@@ -19,12 +19,15 @@ class Svgz extends Svg
 	/**
 	 * Sanitizes the given string
 	 *
+	 * @param bool $isExternal Whether the string is from an external file
+	 *                         that may be accessed directly
+	 *
 	 * @throws \Kirby\Exception\InvalidArgumentException If the file couldn't be parsed or recompressed
 	 */
-	public static function sanitize(string $string): string
+	public static function sanitize(string $string, bool $isExternal = false): string
 	{
 		$string = static::uncompress($string);
-		$string = parent::sanitize($string);
+		$string = parent::sanitize($string, $isExternal);
 		$string = @gzencode($string);
 
 		if (is_string($string) !== true) {
@@ -37,13 +40,16 @@ public static function sanitize(string $string): string
 	/**
 	 * Validates file contents
 	 *
+	 * @param bool $isExternal Whether the string is from an external file
+	 *                         that may be accessed directly
+	 *
 	 * @throws \Kirby\Exception\InvalidArgumentException If the file couldn't be parsed
 	 * @throws \Kirby\Exception\InvalidArgumentException If the file didn't pass validation
 	 */
-	public static function validate(string $string): void
+	public static function validate(string $string, bool $isExternal = false): void
 	{
 		$string = static::uncompress($string);
-		parent::validate($string);
+		parent::validate($string, $isExternal);
 	}
 
 	/**

diff --git a/src/Toolkit/Dom.php b/src/Toolkit/Dom.php
@@ -280,7 +280,7 @@ public static function isAllowedUrl(
 
 		// allow site-internal URLs that didn't match the
 		// protocol-relative check above
-		if (mb_substr($url, 0, 1) === '/') {
+		if (mb_substr($url, 0, 1) === '/' && $options['allowHostRelativeUrls'] !== true) {
 			// if a CMS instance is active, only allow the URL
 			// if it doesn't point outside of the index URL
 			if ($kirby = App::instance(null, true)) {
@@ -525,6 +525,9 @@ public function query(
 	 *                       or `true` for any
 	 *                       - `allowedDomains`: Allowed hostnames for HTTP(S) URLs in `urlAttrs`
 	 *                       and inside `url()` wrappers or `true` for any
+	 *                       - `allowHostRelativeUrls`: Whether URLs that begin with `/` should be
+	 *                       allowed even if the site index URL is in a subfolder (useful when using
+	 *                       the HTML `<base>` element where the sanitized code will be rendered)
 	 *                       - `allowedNamespaces`: Associative array of all allowed namespace URIs;
 	 *                       the array keys are reference names that can be referred to from the
 	 *                       `allowedAttrPrefixes`, `allowedAttrs`, `allowedTags`, `disallowedTags`
@@ -708,18 +711,19 @@ protected static function normalizeSanitizeOptions(array $options): array
 		}
 
 		$options = array_merge([
-			'allowedAttrPrefixes' => [],
-			'allowedAttrs'        => true,
-			'allowedDataUris'     => true,
-			'allowedDomains'      => true,
-			'allowedNamespaces'   => true,
-			'allowedPIs'          => true,
-			'allowedTags'         => true,
-			'attrCallback'        => null,
-			'disallowedTags'      => [],
-			'doctypeCallback'     => null,
-			'elementCallback'     => null,
-			'urlAttrs'            => ['href', 'src', 'xlink:href'],
+			'allowedAttrPrefixes'   => [],
+			'allowedAttrs'          => true,
+			'allowedDataUris'       => true,
+			'allowedDomains'        => true,
+			'allowHostRelativeUrls' => true,
+			'allowedNamespaces'     => true,
+			'allowedPIs'            => true,
+			'allowedTags'           => true,
+			'attrCallback'          => null,
+			'disallowedTags'        => [],
+			'doctypeCallback'       => null,
+			'elementCallback'       => null,
+			'urlAttrs'              => ['href', 'src', 'xlink:href'],
 		], $options);
 
 		$options['_normalized'] = true;

diff --git a/tests/Sane/DomHandlerTest.php b/tests/Sane/DomHandlerTest.php
@@ -25,11 +25,16 @@ public function testSanitize()
 		$fixture   = '<?xml version="1.0" standalone="no"?><xml><test>Hello world</test></xml>';
 		$sanitized = "<?xml version=\"1.0\" encoding=\"UTF-8\" standalone=\"no\"?>\n<xml><test>Hello world</test></xml>";
 		$this->assertSame($sanitized, DomHandler::sanitize($fixture));
+
+		$string = '<xml><a xlink:href="/another-folder">Very malicious</a></xml>';
+		$this->assertSame($string, DomHandler::sanitize($string)); // not external by default
+		$this->assertSame('<xml><a>Very malicious</a></xml>', DomHandler::sanitize($string, isExternal: true));
 	}
 
 	public function testValidate()
 	{
 		$this->assertNull(DomHandler::validate('<!DOCTYPE xml><xml><test attr="value">Hello world</test></xml>'));
+		$this->assertNull(DomHandler::validate('<xml><a xlink:href="/another-folder">Very malicious</a></xml>'));
 	}
 
 	public function testValidateException1()
@@ -47,4 +52,12 @@ public function testValidateException2()
 
 		DomHandler::validate("<!DOCTYPE xml SYSTEM \"https://malicious.com/something.dtd\">\n<xml>\n<a href='javascript:alert(1)'></a>\n</xml>");
 	}
+
+	public function testValidateException3()
+	{
+		$this->expectException(InvalidArgumentException::class);
+		$this->expectExceptionMessage('The URL points outside of the site index URL');
+
+		DomHandler::validate('<xml><a xlink:href="/another-folder">Very malicious</a></xml>', isExternal: true);
+	}
 }
diff --git a/tests/Sane/HandlerTest.php b/tests/Sane/HandlerTest.php
@@ -28,6 +28,11 @@ public function testSanitizeFile()
 		$tmp      = $this->fixture('external-source-1.svg', true);
 		$this->assertNull(CustomHandler::sanitizeFile($tmp));
 		$this->assertFileEquals($expected, $tmp);
+
+		$expected = $this->fixture('xlink-subfolder.sanitized.svg');
+		$tmp      = $this->fixture('xlink-subfolder.svg', true);
+		$this->assertNull(CustomHandler::sanitizeFile($tmp));
+		$this->assertFileEquals($expected, $tmp);
 	}
 
 	/**
@@ -65,6 +70,18 @@ public function testValidateFileError()
 		CustomHandler::validateFile($this->fixture('external-source-1.svg'));
 	}
 
+	/**
+	 * @covers ::validateFile
+	 * @covers ::readFile
+	 */
+	public function testValidateFileErrorExternalFile()
+	{
+		$this->expectException(InvalidArgumentException::class);
+		$this->expectExceptionMessage('The URL points outside of the site index URL');
+
+		CustomHandler::validateFile($this->fixture('xlink-subfolder.svg'));
+	}
+
 	/**
 	 * @covers ::validateFile
 	 * @covers ::readFile

diff --git a/tests/Sane/HtmlTest.php b/tests/Sane/HtmlTest.php
@@ -2,6 +2,8 @@
 
 namespace Kirby\Sane;
 
+use Kirby\Exception\InvalidArgumentException;
+
 /**
  * @covers \Kirby\Sane\Html
  * @todo Add more tests from DOMPurify and the other test classes
@@ -27,4 +29,17 @@ public function allowedProvider()
 	{
 		return $this->fixtureList('allowed', 'html');
 	}
+
+	public function testDisallowedExternalFile()
+	{
+		$fixture   = $this->fixture('disallowed/link-subfolder.html');
+		$sanitized = $this->fixture('sanitized/link-subfolder.html');
+
+		$this->assertStringEqualsFile($fixture, Html::sanitize(file_get_contents($fixture)));
+		$this->assertStringEqualsFile($sanitized, Html::sanitize(file_get_contents($fixture), isExternal: true));
+
+		$this->expectException(InvalidArgumentException::class);
+		$this->expectExceptionMessage('The URL points outside of the site index URL');
+		Html::validateFile($fixture);
+	}
 }