[Bug]: Fix sanitized text being encoded before persisting to db (pimc…

…ore#15960) * sanitized text is encoded, need to decode it before persisting in db * apply to Wysiwyg as well * edit mode as well * apply decode to object wysiwyg * draft test * add test for translator * add test for translator * add test for translator * add the <> symbols * test with sanitizeFor textarea * test with sanitizeFor body? * test with sanitizeFor body? * Fix issue with some special chars e.g. < by encoding before passing to the sanitizer * Fix < encoding issue before passing to the sanitizer * fix tests * fix tests --------- Co-authored-by: Divesh Pahuja <[email protected]>
limenet · Sep 25, 2023 · cdec6ec · cdec6ec
1 parent 3fb0f90
commit cdec6ec
Show file tree

Hide file tree

Showing 6 changed files with 111 additions and 5 deletions.
diff --git a/.github/ci/files/config/packages/html_sanitizer.yaml b/.github/ci/files/config/packages/html_sanitizer.yaml
@@ -2,5 +2,55 @@ framework:
     html_sanitizer:
         sanitizers:
             pimcore.wysiwyg_sanitizer:
+                allow_attributes:
+                    pimcore_type: '*'
+                    pimcore_id: '*'
+                allow_relative_links: true
+                allow_relative_medias: true
                 allow_elements:
-                    br: ''
+                    span: [ 'class', 'style', 'id' ]
+                    div: [ 'class', 'style', 'id' ]
+                    p: [ 'class', 'style', 'id' ]
+                    strong: 'class'
+                    em: 'class'
+                    h1: [ 'class', 'id' ]
+                    table: [ 'class', 'style', 'cellspacing', 'cellpadding', 'border', 'width', 'height', 'id' ]
+                    colgroup: 'class'
+                    col: [ 'class', 'style', 'id' ]
+                    thead: [ 'class', 'id' ]
+                    tbody: [ 'class', 'id' ]
+                    tr: [ 'class', 'id' ]
+                    td: [ 'class', 'id' ]
+                    th: [ 'class', 'id', 'scope' ]
+                    ul: [ 'class', 'style', 'id' ]
+                    li: [ 'class', 'style', 'id' ]
+                    ol: [ 'class', 'style', 'id' ]
+                    u: [ 'class', 'id' ]
+                    i: [ 'class', 'id' ]
+                    b: [ 'class', 'id' ]
+                    caption: [ 'class', 'id' ]
+                    sub: [ 'class', 'id' ]
+                    sup: [ 'class', 'id' ]
+                    blockquote: [ 'class', 'id' ]
+                    s: [ 'class', 'id' ]
+                    iframe: [ 'frameborder', 'height', 'longdesc', 'name', 'sandbox', 'scrolling', 'src', 'title', 'width' ]
+                    br: ''
+                    img: [ 'alt', 'style', 'src' ]
+            pimcore.translation_sanitizer:
+                allow_attributes:
+                    pimcore_type: '*'
+                    pimcore_id: '*'
+                allow_relative_links: true
+                allow_elements:
+                    span: [ 'class', 'style', 'id' ]
+                    p: [ 'class', 'style', 'id' ]
+                    strong: 'class'
+                    em: 'class'
+                    h1: [ 'class', 'id' ]
+                    h2: [ 'class', 'id' ]
+                    h3: [ 'class', 'id' ]
+                    h4: [ 'class', 'id' ]
+                    h5: [ 'class', 'id' ]
+                    h6: [ 'class', 'id' ]
+                    a: [ 'class', 'id', 'href', 'target', 'title', 'rel' ]
+                    br: ''
diff --git a/models/DataObject/ClassDefinition/Data/Wysiwyg.php b/models/DataObject/ClassDefinition/Data/Wysiwyg.php
@@ -114,7 +114,9 @@ public function getDataForResource(mixed $data, DataObject\Concrete $object = nu
     public function getDataFromResource(mixed $data, DataObject\Concrete $object = null, array $params = []): ?string
     {
         if (is_string($data)) {
-            $data = self::getWysiwygSanitizer()->sanitize($data);
+            //fix for https://github.com/pimcore/pimcore/issues/15740
+            $data = preg_replace('/(?!<[a-zA-Z=\"\':; ]*[^ ]>|<\\/[a-zA-Z="\':; ]*>)(<)/', "&lt;", $data);
+            $data = html_entity_decode(self::getWysiwygSanitizer()->sanitizeFor('body', $data));
         }
 
         return Text::wysiwygText($data);

diff --git a/models/Document/Editable/Wysiwyg.php b/models/Document/Editable/Wysiwyg.php
@@ -90,7 +90,9 @@ public function frontend()
     public function setDataFromResource(mixed $data): static
     {
         $helper = self::getWysiwygSanitizer();
-        $this->text = $helper->sanitize($data);
+        //fix for https://github.com/pimcore/pimcore/issues/15740
+        $data = preg_replace('/(?!<[a-zA-Z=\"\':; ]*[^ ]>|<\\/[a-zA-Z="\':; ]*>)(<)/', "&lt;", $data);
+        $this->text = html_entity_decode($helper->sanitizeFor('body', $data));
 
         return $this;
     }
@@ -101,7 +103,9 @@ public function setDataFromResource(mixed $data): static
     public function setDataFromEditmode(mixed $data): static
     {
         $helper = self::getWysiwygSanitizer();
-        $this->text = $helper->sanitize($data);
+        //fix for https://github.com/pimcore/pimcore/issues/15740
+        $data = preg_replace('/(?!<[a-zA-Z=\"\':; ]*[^ ]>|<\\/[a-zA-Z="\':; ]*>)(<)/', "&lt;", $data);
+        $this->text = html_entity_decode($helper->sanitizeFor('body', $data));
 
         return $this;
     }

diff --git a/models/Translation/Dao.php b/models/Translation/Dao.php
@@ -101,11 +101,13 @@ public function save(): void
                         continue;
                     }
 
+                    //fix for https://github.com/pimcore/pimcore/issues/15740
+                    $text = preg_replace('/(?!<[a-zA-Z=\"\':; ]*[^ ]>|<\\/[a-zA-Z="\':; ]*>)(<)/', "&lt;", $text);
                     $data = [
                         'key' => $this->model->getKey(),
                         'type' => $this->model->getType(),
                         'language' => $language,
-                        'text' => $sanitizer->sanitize($text),
+                        'text' => html_entity_decode($sanitizer->sanitizeFor('body', $text)),
                         'modificationDate' => $this->model->getModificationDate(),
                         'creationDate' => $this->model->getCreationDate(),
                         'userOwner' => $this->model->getUserOwner(),

diff --git a/tests/Model/DataObject/ObjectTest.php b/tests/Model/DataObject/ObjectTest.php
@@ -353,4 +353,27 @@ public function testEmptyValuesAsNullBackend(): void
         $value = $dataType->getDataFromEditmode($iqv, $object);
         $this->assertNull($value);
     }
+
+    public function testSanitization(): void
+    {
+        $db = Db::get();
+
+        $object = TestHelper::createEmptyObject();
+        $object->setWysiwyg('!@#$%^abc\'"<script>console.log("ops");</script> 测试< edf > "');
+        $object->save();
+
+        //reload from db
+        $object = DataObject::getById($object->getId(), ['force' => true]);
+
+        $this->assertEquals('!@#$%^abc\'" 测试< edf > "', $object->getWysiwyg(),'Asseting setter/getter value is sanitized');
+
+        $dbQueryValue = $db->fetchOne(
+            sprintf(
+                'SELECT `wysiwyg` FROM object_query_%s WHERE oo_id = %d',
+                $object->getClassName(),
+                $object->getId()
+            )
+        );
+        $this->assertEquals('!@#$%^abc\'" 测试< edf > "', $dbQueryValue, 'Asserting object_query table value is persisted as sanitized');
+    }
 }
diff --git a/tests/Unit/Translation/TranslatorTest.php b/tests/Unit/Translation/TranslatorTest.php
@@ -17,6 +17,7 @@
 
 namespace Pimcore\Tests\Unit\Translation;
 
+use Pimcore\Db;
 use Pimcore\Model\Translation;
 use Pimcore\Tests\Support\Test\TestCase;
 use Pimcore\Translation\Translator;
@@ -245,4 +246,28 @@ public function testCacheGetsInvalidatedOnSave(): void
 
         $this->assertCount(count($beforeAdd) + 1, $afterAdd);
     }
+
+    public function testSanitizedTranslation(): void
+    {
+        $translation = new Translation();
+        $key = 'sanitizerTest';
+        $translation->setDomain('messages');
+        $translation->setKey($key);
+        $translation->setTranslations(['en' => '!@#$%^abc\'"<script>console.log("ops");</script> 测试< edf > "']);
+        $translation->save();
+
+        $translation = Translation::getByKey($key);
+        $getter = $translation->getTranslation('en');
+        $this->assertEquals('!@#$%^abc\'" 测试< edf > "', $getter, 'Asserting translation is properly sanitized');
+
+        $db = Db::get();
+        $dbValue = $db->fetchOne(
+            sprintf(
+                'SELECT `text` FROM translations_messages WHERE `key` = %s AND `language` = %s',
+                $db->quote($key),
+                $db->quote('en')
+            )
+        );
+        $this->assertEquals('!@#$%^abc\'" 测试< edf > "', $dbValue, 'Asserting translation is persisted as sanitized');
+    }
 }