[BUGFIX] Improved argument validation in TagBuilder (#937) (#969)

The current approach to sanitize HTML tag attributes by
TagBuilder can be improved to only allow certain characters.
As TagBuilder is a low-level API to create arbitrary HTML tags,
the goal is not to prevent any JavaScript from being executed
because this might be a desired behavior in some use cases.

We would like to prevent the following:

```php
$unsafeInput = "onclick='alert(1)'";
$tagBuilder->addAttribute($unsafeInput, 'some value');
```

While still allowing the following:

```php
$tagBuilder->addAttribute('onclick', 'doSomething()');
```

Thus, this patch applies a strict allow-list to argument names
and throws an exception if any other characters are used.
The characters are limited to ASCII characters except for
select characters that are problematic in HTML attributes,
such as single and double quotes, equal signs, less/greater than,
forward slashes, ampersants and white space.

Developers are advised to be very careful when using user input
as attribute names as this change cannot prevent all accidential
and thus potentially malicious JavaScript execution.

Thanks to @GatekeeperBuster for making us aware of this.

src/Core/ViewHelper/TagBuilder.php

@@ -7,6 +7,8 @@
 
 namespace TYPO3Fluid\Fluid\Core\ViewHelper;
 
+use InvalidArgumentException;
+
 /**
  * Tag builder. Can be easily accessed in AbstractTagBasedViewHelper
  *
@@ -180,7 +182,8 @@ public function ignoreEmptyAttributes($ignoreEmptyAttributes)
     /**
      * Adds an attribute to the $attributes-collection
      *
-     * @param string $attributeName name of the attribute to be added to the tag
+     * @param string $attributeName name of the attribute to be added to the tag. Be extremely
+     *                              careful if this value is user-provided input!
      * @param string|\Traversable|array|null $attributeValue attribute value, can only be array or traversable
      *                                                       if the attribute name is either "data" or "area". In
      *                                                       that special case, multiple attributes will be created
@@ -190,9 +193,25 @@ public function ignoreEmptyAttributes($ignoreEmptyAttributes)
      */
     public function addAttribute($attributeName, $attributeValue, $escapeSpecialCharacters = true)
     {
-        if ($escapeSpecialCharacters) {
-            $attributeName = htmlspecialchars($attributeName);
+        // Limit attribute names to ASCII characters to keep validation reasonably simple
+        // The regular expression lists all printable ASCII characters (0x20 to 0x7F) more or
+        // less in the order they are defined in the standard.
+        // The following characters are excluded and thus not allowed in attribute names to prevent
+        // certain XSS security issues:
+        // - Space and Delete character
+        // - Single (') and double quotes (")
+        // - Less than (<) and greater than (>)
+        // - Equals sign (=)
+        // - Forward slash (/)
+        // - Ampersand (&)
+        // Please note that we cannot fully prevent XSS here because browsers interpret the
+        // value of certain attributes prefixed with "on" (e. g. "onclick") as JavaScript,
+        // which might even be desired functionality.
+        // Please be extremely careful when using user-provided content as attribute name!
+        if (preg_match('/[^0-9A-Za-z!#\$%()*+,\.:;?@\\[\]\^_`{|}~-]/', $attributeName)) {
+            throw new InvalidArgumentException('Invalid attribute name provided: ' . $attributeName, 1721982367);
         }
+
         if (is_array($attributeValue) || $attributeValue instanceof \Traversable) {
             if (!in_array($attributeName, ['data', 'aria'], true)) {
                 throw new \InvalidArgumentException(

tests/Functional/Cases/Escaping/ViewHelperEscapingTest.php

@@ -9,6 +9,9 @@
 
 namespace TYPO3Fluid\Fluid\Tests\Functional\Cases\Escaping;
 
+use InvalidArgumentException;
+use PHPUnit\Framework\Attributes\DataProvider;
+use PHPUnit\Framework\Attributes\Test;
 use TYPO3Fluid\Fluid\Core\Parser\Configuration;
 use TYPO3Fluid\Fluid\Core\Parser\Interceptor\Escape;
 use TYPO3Fluid\Fluid\Core\Rendering\RenderingContext;
@@ -320,8 +323,24 @@ public function testTagBasedViewHelperEscapesAttributes(): void
         self::assertSame('<div class="' . self::ESCAPED . '" />', $this->renderCode($viewHelper, '<test:test class="{value}" />'), 'Tag attribute is escaped');
         self::assertSame('<div data-foo="' . self::ESCAPED . '" />', $this->renderCode($viewHelper, '<test:test data="{foo: value}" />'), 'Tag data attribute values are escaped');
         self::assertSame('<div foo="' . self::ESCAPED . '" />', $this->renderCode($viewHelper, '<test:test additionalAttributes="{foo: value}" />'), 'Tag additional attributes values are escaped');
-        self::assertSame('<div data-&gt;' . self::ESCAPED . '&lt;="1" />', $this->renderCode($viewHelper, '<test:test data=\'{"><script>alert(1)</script><": 1}\' />'), 'Tag data attribute keys are escaped');
-        self::assertSame('<div &gt;' . self::ESCAPED . '&lt;="1" />', $this->renderCode($viewHelper, '<test:test additionalAttributes=\'{"><script>alert(1)</script><": 1}\' />'), 'Tag additional attributes keys are escaped');
         self::assertSame('<div data-foo="' . self::ESCAPED . '" />', $this->renderCode($viewHelper, '<test:test data-foo="{value}" />'), 'Tag unregistered data attribute is escaped');
     }
+
+    public static function tagBasedViewHelperValidatesAttributeNamesDataProvider(): array
+    {
+        return [
+            ['<test:test data=\'{"><script>alert(1)</script><": 1}\' />'],
+            ['<test:test additionalAttributes=\'{"><script>alert(1)</script><": 1}\' />'],
+        ];
+    }
+
+    #[Test]
+    #[DataProvider('tagBasedViewHelperValidatesAttributeNamesDataProvider')]
+    public function tagBasedViewHelperValidatesAttributeNames(string $template): void
+    {
+        self::expectException(InvalidArgumentException::class);
+        self::expectExceptionCode(1721982367);
+        $viewHelper = new TagBasedTestViewHelper();
+        $this->renderCode($viewHelper, $template);
+    }
 }

tests/Unit/Core/ViewHelper/TagBuilderTest.php

@@ -9,6 +9,8 @@
 
 namespace TYPO3Fluid\Fluid\Tests\Unit\Core\ViewHelper;
 
+use InvalidArgumentException;
+use PHPUnit\Framework\Attributes\DataProvider;
 use PHPUnit\Framework\Attributes\Test;
 use PHPUnit\Framework\TestCase;
 use TYPO3Fluid\Fluid\Core\ViewHelper\TagBuilder;
@@ -96,8 +98,8 @@ public function attributesAreProperlyRendered(): void
         $tagBuilder = new TagBuilder('tag');
         $tagBuilder->addAttribute('attribute1', 'attribute1value');
         $tagBuilder->addAttribute('attribute2', 'attribute2value');
-        $tagBuilder->addAttribute('attribute3', 'attribute3value');
-        self::assertEquals('<tag attribute1="attribute1value" attribute2="attribute2value" attribute3="attribute3value" />', $tagBuilder->render());
+        $tagBuilder->addAttribute(':at-tr$ib.ut_e3#', 'attribute3value');
+        self::assertEquals('<tag attribute1="attribute1value" attribute2="attribute2value" :at-tr$ib.ut_e3#="attribute3value" />', $tagBuilder->render());
     }
 
     #[Test]
@@ -119,6 +121,29 @@ public function customArrayAttributesThrowException(): void
         $tagBuilder->addAttribute('custom', ['attribute1' => 'data1', 'attribute2' => 'data2']);
     }
 
+    public static function attributeNamesAreProperlyValidatedDataProvider(): array
+    {
+        return [
+            ['onclick="alert(1)"'],
+            ['onblur=\'alert(1)\''],
+            ['onchange=alert(1)'],
+            ['on \t\n\rchange'],
+            ['on&change'],
+            ['foo/>'],
+        ];
+    }
+
+    #[Test]
+    #[DataProvider('attributeNamesAreProperlyValidatedDataProvider')]
+    public function attributeNamesAreProperlyValidated(string $attributeName): void
+    {
+        self::expectException(InvalidArgumentException::class);
+        self::expectExceptionCode(1721982367);
+
+        $tagBuilder = new TagBuilder('tag');
+        $tagBuilder->addAttribute($attributeName, '');
+    }
+
     #[Test]
     public function attributeValuesAreEscapedByDefault(): void
     {