bug #202 Fix double escaping in stimulus DTOs when using toArray() in…

… combination with form methods (jeroennoten) This PR was merged into the main branch. Discussion ---------- Fix double escaping in stimulus DTOs when using toArray() in combination with form methods This fixes a double-escaping bug when the `toArray()` methods are used in combination with the `form_` functions, as described in the README. `toArray()` should return an array with non-escaped attribute key-value pairs, because they will be escaped when printed. Therefore, I moved the escaping of the values to the `toString()` methods and I added some additional tests for the DTO classes to verify the change. This additionally fixes a missed escape for the `data-[controller]-[key]-class` attribute values. Commits ------- 0d40126 Fix double escaping in stimulus DTOs when using toArray() in combination with form methods
symfony · Jan 18, 2023 · 1862d71 · 1862d71
2 parents 52443ff + 0d40126
commit 1862d71
Show file tree

Hide file tree

Showing 8 changed files with 151 additions and 13 deletions.
diff --git a/src/Dto/AbstractStimulusDto.php b/src/Dto/AbstractStimulusDto.php
@@ -45,7 +45,7 @@ protected function getFormattedValue($value)
             $value = $value ? 'true' : 'false';
         }
 
-        return $this->escapeAsHtmlAttr($value);
+        return (string) $value;
     }
 
     protected function escapeAsHtmlAttr($value): string

diff --git a/src/Dto/StimulusActionsDto.php b/src/Dto/StimulusActionsDto.php
@@ -41,8 +41,8 @@ public function __toString(): string
             return '';
         }
 
-        return rtrim('data-action="'.implode(' ', $this->actions).'" '.implode(' ', array_map(static function (string $attribute, string $value): string {
-            return $attribute.'="'.$value.'"';
+        return rtrim('data-action="'.implode(' ', $this->actions).'" '.implode(' ', array_map(function (string $attribute, string $value): string {
+            return $attribute.'="'.$this->escapeAsHtmlAttr($value).'"';
         }, array_keys($this->parameters), $this->parameters)));
     }
 

diff --git a/src/Dto/StimulusControllersDto.php b/src/Dto/StimulusControllersDto.php
@@ -48,11 +48,11 @@ public function __toString(): string
 
         return rtrim(
             'data-controller="'.implode(' ', $this->controllers).'" '.
-            implode(' ', array_map(static function (string $attribute, string $value): string {
-                return $attribute.'="'.$value.'"';
+            implode(' ', array_map(function (string $attribute, string $value): string {
+                return $attribute.'="'.$this->escapeAsHtmlAttr($value).'"';
             }, array_keys($this->values), $this->values)).' '.
-            implode(' ', array_map(static function (string $attribute, string $value): string {
-                return $attribute.'="'.$value.'"';
+            implode(' ', array_map(function (string $attribute, string $value): string {
+                return $attribute.'="'.$this->escapeAsHtmlAttr($value).'"';
             }, array_keys($this->classes), $this->classes))
         );
     }

diff --git a/src/Dto/StimulusTargetsDto.php b/src/Dto/StimulusTargetsDto.php
@@ -23,7 +23,7 @@ public function addTarget(string $controllerName, string $targetNames = null): v
     {
         $controllerName = $this->getFormattedControllerName($controllerName);
 
-        $this->targets['data-'.$controllerName.'-target'] = $this->escapeAsHtmlAttr($targetNames);
+        $this->targets['data-'.$controllerName.'-target'] = $targetNames;
     }
 
     public function __toString(): string
@@ -32,8 +32,8 @@ public function __toString(): string
             return '';
         }
 
-        return implode(' ', array_map(static function (string $attribute, string $value): string {
-            return $attribute.'="'.$value.'"';
+        return implode(' ', array_map(function (string $attribute, string $value): string {
+            return $attribute.'="'.$this->escapeAsHtmlAttr($value).'"';
         }, array_keys($this->targets), $this->targets));
     }
 

diff --git a/tests/Dto/StimulusActionsDtoTest.php b/tests/Dto/StimulusActionsDtoTest.php
@@ -0,0 +1,42 @@
+<?php
+
+/*
+ * This file is part of the Symfony WebpackEncoreBundle package.
+ * (c) Fabien Potencier <fabien@symfony.com>
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\WebpackEncoreBundle\Tests\Dto;
+
+use PHPUnit\Framework\TestCase;
+use Symfony\WebpackEncoreBundle\Dto\StimulusActionsDto;
+use Twig\Environment;
+use Twig\Loader\ArrayLoader;
+
+class StimulusActionsDtoTest extends TestCase
+{
+    /**
+     * @var StimulusActionsDto
+     */
+    private $stimulusActionsDto;
+
+    protected function setUp(): void
+    {
+        $this->stimulusActionsDto = new StimulusActionsDto(new Environment(new ArrayLoader()));
+    }
+
+    public function testToStringEscapingAttributeValues(): void
+    {
+        $this->stimulusActionsDto->addAction('foo', 'bar', 'baz', ['qux' => '"']);
+        $attributesHtml = (string) $this->stimulusActionsDto;
+        self::assertSame('data-action="baz->foo#bar" data-foo-qux-param="&quot;"', $attributesHtml);
+    }
+
+    public function testToArrayNoEscapingAttributeValues(): void
+    {
+        $this->stimulusActionsDto->addAction('foo', 'bar', 'baz', ['qux' => '"']);
+        $attributesArray = $this->stimulusActionsDto->toArray();
+        self::assertSame(['data-action' => 'baz->foo#bar', 'data-foo-qux-param' => '"'], $attributesArray);
+    }
+}
diff --git a/tests/Dto/StimulusControllersDtoTest.php b/tests/Dto/StimulusControllersDtoTest.php
@@ -0,0 +1,54 @@
+<?php
+
+/*
+ * This file is part of the Symfony WebpackEncoreBundle package.
+ * (c) Fabien Potencier <fabien@symfony.com>
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\WebpackEncoreBundle\Tests\Dto;
+
+use PHPUnit\Framework\TestCase;
+use Symfony\WebpackEncoreBundle\Dto\StimulusControllersDto;
+use Twig\Environment;
+use Twig\Loader\ArrayLoader;
+
+class StimulusControllersDtoTest extends TestCase
+{
+    /**
+     * @var StimulusControllersDto
+     */
+    private $stimulusControllersDto;
+
+    protected function setUp(): void
+    {
+        $this->stimulusControllersDto = new StimulusControllersDto(new Environment(new ArrayLoader()));
+    }
+
+    public function testToStringEscapingAttributeValues(): void
+    {
+        $this->stimulusControllersDto->addController('foo', ['bar' => '"'], ['baz' => '"']);
+        $attributesHtml = (string) $this->stimulusControllersDto;
+        self::assertSame(
+            'data-controller="foo" '.
+            'data-foo-bar-value="&quot;" '.
+            'data-foo-baz-class="&quot;"',
+            $attributesHtml
+        );
+    }
+
+    public function testToArrayNoEscapingAttributeValues(): void
+    {
+        $this->stimulusControllersDto->addController('foo', ['bar' => '"'], ['baz' => '"']);
+        $attributesArray = $this->stimulusControllersDto->toArray();
+        self::assertSame(
+            [
+                'data-controller' => 'foo',
+                'data-foo-bar-value' => '"',
+                'data-foo-baz-class' => '"',
+            ],
+            $attributesArray
+        );
+    }
+}
diff --git a/tests/Dto/StimulusTargetsDtoTest.php b/tests/Dto/StimulusTargetsDtoTest.php
@@ -0,0 +1,42 @@
+<?php
+
+/*
+ * This file is part of the Symfony WebpackEncoreBundle package.
+ * (c) Fabien Potencier <fabien@symfony.com>
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\WebpackEncoreBundle\Tests\Dto;
+
+use PHPUnit\Framework\TestCase;
+use Symfony\WebpackEncoreBundle\Dto\StimulusTargetsDto;
+use Twig\Environment;
+use Twig\Loader\ArrayLoader;
+
+class StimulusTargetsDtoTest extends TestCase
+{
+    /**
+     * @var StimulusTargetsDto
+     */
+    private $stimulusTargetsDto;
+
+    protected function setUp(): void
+    {
+        $this->stimulusTargetsDto = new StimulusTargetsDto(new Environment(new ArrayLoader()));
+    }
+
+    public function testToStringEscapingAttributeValues(): void
+    {
+        $this->stimulusTargetsDto->addTarget('foo', '"');
+        $attributesHtml = (string) $this->stimulusTargetsDto;
+        self::assertSame('data-foo-target="&quot;"', $attributesHtml);
+    }
+
+    public function testToArrayNoEscapingAttributeValues(): void
+    {
+        $this->stimulusTargetsDto->addTarget('foo', '"');
+        $attributesArray = $this->stimulusTargetsDto->toArray();
+        self::assertSame(['data-foo-target' => '"'], $attributesArray);
+    }
+}
diff --git a/tests/IntegrationTest.php b/tests/IntegrationTest.php
@@ -330,7 +330,7 @@ public function provideLegacyRenderMultipleStimulusControllers()
             ],
             'controllerValues' => [],
             'expectedString' => 'data-controller="my-controller" data-my-controller-my-value-value="&#x7B;&quot;nested&quot;&#x3A;&quot;array&quot;&#x7D;"',
-            'expectedArray' => ['data-controller' => 'my-controller', 'data-my-controller-my-value-value' => '&#x7B;&quot;nested&quot;&#x3A;&quot;array&quot;&#x7D;'],
+            'expectedArray' => ['data-controller' => 'my-controller', 'data-my-controller-my-value-value' => '{"nested":"array"}'],
         ];
 
         yield 'multiple-controllers-scalar-data' => [
@@ -344,7 +344,7 @@ public function provideLegacyRenderMultipleStimulusControllers()
             ],
             'controllerValues' => [],
             'expectedString' => 'data-controller="my-controller another-controller" data-my-controller-my-value-value="scalar-value" data-another-controller-another-value-value="scalar-value&#x20;2"',
-            'expectedArray' => ['data-controller' => 'my-controller another-controller', 'data-my-controller-my-value-value' => 'scalar-value', 'data-another-controller-another-value-value' => 'scalar-value&#x20;2'],
+            'expectedArray' => ['data-controller' => 'my-controller another-controller', 'data-my-controller-my-value-value' => 'scalar-value', 'data-another-controller-another-value-value' => 'scalar-value 2'],
         ];
 
         yield 'normalize-names' => [
@@ -582,7 +582,7 @@ public function testLegacyRenderMultipleStimulusTargets()
 
         $this->assertSame([
                 'data-my-controller-target' => 'myTarget',
-                'data-symfony--ux-dropzone--dropzone-target' => 'anotherTarget&#x20;fooTarget',
+                'data-symfony--ux-dropzone--dropzone-target' => 'anotherTarget fooTarget',
             ],
             $dto->toArray()
         );