Merge pull request #11201 from creative-commoners/pulls/5/fix-wysiwyg…

…-sanitisation Fix wysiwyg sanitisation
silverstripe · Apr 18, 2024 · c2199ff · c2199ff
2 parents 3e1b5e6 + 72692f9
commit c2199ff
Show file tree

Hide file tree

Showing 5 changed files with 98 additions and 19 deletions.
diff --git a/src/Forms/HTMLEditor/HTMLEditorField.php b/src/Forms/HTMLEditor/HTMLEditorField.php
@@ -138,7 +138,8 @@ public function saveInto(DataObjectInterface $record)
         // Sanitise if requested
         $htmlValue = HTMLValue::create($this->Value());
         if (HTMLEditorField::config()->sanitise_server_side) {
-            $santiser = HTMLEditorSanitiser::create(HTMLEditorConfig::get_active());
+            $config = $this->getEditorConfig();
+            $santiser = HTMLEditorSanitiser::create($config);
             $santiser->sanitise($htmlValue);
         }
 

diff --git a/src/Forms/HTMLEditor/HTMLEditorSanitiser.php b/src/Forms/HTMLEditor/HTMLEditorSanitiser.php
@@ -287,10 +287,6 @@ protected function attributeMatchesRule($attr, $rule = null)
      */
     public function sanitise(HTMLValue $html)
     {
-        if (!$this->elements && !$this->elementPatterns) {
-            return;
-        }
-
         $linkRelValue = $this->config()->get('link_rel_value');
         $doc = $html->getDocument();
 

diff --git a/src/Forms/HTMLEditor/TinyMCEConfig.php b/src/Forms/HTMLEditor/TinyMCEConfig.php
@@ -250,13 +250,11 @@ class TinyMCEConfig extends HTMLEditorConfig implements i18nEntityProvider
     private static $image_size_presets = [ ];
 
     /**
-     * TinyMCE JS settings
+     * Default TinyMCE JS options which apply to all new configurations.
      *
      * @link https://www.tiny.cloud/docs/tinymce/6/tinydrive-getting-started/#configure-the-required-tinymce-options
-     *
-     * @var array
      */
-    protected $settings = [
+    private static array $default_options = [
         'fix_list_elements' => true, // https://www.tiny.cloud/docs/tinymce/6/content-filtering/#fix_list_elements
         'formats' => [
             'alignleft' => [
@@ -311,8 +309,24 @@ class TinyMCEConfig extends HTMLEditorConfig implements i18nEntityProvider
         'promotion' => false,
         'upload_folder_id' => null, // Set folder ID for insert media dialog
         'link_default_target' => '_blank', // https://www.tiny.cloud/docs/tinymce/6/autolink/#example-using-link_default_target
+        // Default set of valid_elements which apply for all new configurations
+        'valid_elements' => "@[id|class|style|title],a[id|rel|rev|dir|tabindex|accesskey|type|name|href|target|title"
+        . "|class],-strong/-b[class],-em/-i[class],-strike[class],-u[class],#p[id|dir|class|align|style],-ol[class],"
+        . "-ul[class],-li[class],br,img[id|dir|longdesc|usemap|class|src|border|alt=|title|hspace|vspace|width|height|align|name|data*],"
+        . "-sub[class],-sup[class],-blockquote[dir|class],-cite[dir|class|id|title],"
+        . "-table[cellspacing|cellpadding|width|height|class|align|summary|dir|id|style],"
+        . "-tr[id|dir|class|rowspan|width|height|align|valign|bgcolor|background|bordercolor|style],"
+        . "tbody[id|class|style],thead[id|class|style],tfoot[id|class|style],"
+        . "#td[id|dir|class|colspan|rowspan|width|height|align|valign|scope|style],"
+        . "-th[id|dir|class|colspan|rowspan|width|height|align|valign|scope|style],caption[id|dir|class],"
+        . "-div[id|dir|class|align|style],-span[class|align|style],-pre[class|align],address[class|align],"
+        . "-h1[id|dir|class|align|style],-h2[id|dir|class|align|style],-h3[id|dir|class|align|style],"
+        . "-h4[id|dir|class|align|style],-h5[id|dir|class|align|style],-h6[id|dir|class|align|style],hr[class],"
+        . "dd[id|class|title|dir],dl[id|class|title|dir],dt[id|class|title|dir],"
     ];
 
+    protected $settings = [];
+
     /**
      * Holder list of enabled plugins
      *
@@ -337,6 +351,11 @@ class TinyMCEConfig extends HTMLEditorConfig implements i18nEntityProvider
      */
     protected $theme = 'silver';
 
+    public function __construct()
+    {
+        $this->settings = static::config()->get('default_options');
+    }
+
     /**
      * Get the theme
      *

diff --git a/tests/php/Forms/HTMLEditor/HTMLEditorFieldTest.php b/tests/php/Forms/HTMLEditor/HTMLEditorFieldTest.php
@@ -12,6 +12,7 @@
 use SilverStripe\Core\Config\Config;
 use SilverStripe\Dev\CSSContentParser;
 use SilverStripe\Dev\FunctionalTest;
+use SilverStripe\Forms\HTMLEditor\HTMLEditorConfig;
 use SilverStripe\Forms\HTMLEditor\HTMLEditorField;
 use SilverStripe\Forms\HTMLEditor\TinyMCEConfig;
 use SilverStripe\Forms\HTMLReadonlyField;
@@ -278,4 +279,41 @@ public function testGetAttributes()
         $this->assertEquals("auto", $data_config->height, 'Config height is not set');
         $this->assertEquals("60px", $data_config->row_height, 'Config row_height is not set');
     }
+
+    public function testFieldConfigSanitization()
+    {
+        $obj = TestObject::create();
+        $editor = HTMLEditorField::create('Content');
+        $defaultValidElements = [
+            '@[id|class|style|title|data*]',
+            'a[id|rel|dir|tabindex|accesskey|type|name|href|target|title|class]',
+            '-strong/-b[class]',
+            '-em/-i[class]',
+            '-ol[class]',
+            '#p[id|dir|class|align|style]',
+            '-li[class]',
+            'br',
+            '-span[class|align|style]',
+            '-ul[class]',
+            '-h3[id|dir|class|align|style]',
+            '-h2[id|dir|class|align|style]',
+            'hr[class]',
+        ];
+        $restrictedConfig = HTMLEditorConfig::get('restricted');
+        $restrictedConfig->setOption('valid_elements', implode(',', $defaultValidElements));
+        $editor->setEditorConfig($restrictedConfig);
+
+        $expectedHtmlString = '<p>standard text</p>Header';
+        $htmlValue = '<p>standard text</p><table><th><tr><td>Header</td></tr></th><tbody></tbody></table>';
+        $editor->setValue($htmlValue);
+        $editor->saveInto($obj);
+        $this->assertEquals($expectedHtmlString, $obj->Content, 'Table is not removed');
+
+        $defaultConfig = HTMLEditorConfig::get('default');
+        $editor->setEditorConfig($defaultConfig);
+
+        $editor->setValue($htmlValue);
+        $editor->saveInto($obj);
+        $this->assertEquals($htmlValue, $obj->Content, 'Table is removed');
+    }
 }
diff --git a/tests/php/Forms/HTMLEditor/HTMLEditorSanitiserTest.php b/tests/php/Forms/HTMLEditor/HTMLEditorSanitiserTest.php
@@ -11,9 +11,9 @@
 class HTMLEditorSanitiserTest extends FunctionalTest
 {
 
-    public function testSanitisation()
+    public function provideSanitise(): array
     {
-        $tests = [
+        return [
             [
                 'p,strong',
                 '<p>Leave Alone</p><div>Strip parent<strong>But keep children</strong> in order</div>',
@@ -129,13 +129,20 @@ public function testSanitisation()
                 'XSS vulnerable attributes starting with on or style are removed via configuration'
             ],
         ];
+    }
 
-        $config = HTMLEditorConfig::get('htmleditorsanitisertest');
-
-        foreach ($tests as $test) {
-            list($validElements, $input, $output, $desc) = $test;
-
-            $config->setOptions(['valid_elements' => $validElements]);
+    /**
+     * @dataProvider provideSanitise
+     */
+    public function testSanitisation(string $validElements, string $input, string $output, string $desc): void
+    {
+        foreach (['valid_elements', 'extended_valid_elements'] as $configType) {
+            $config = HTMLEditorConfig::get('htmleditorsanitisertest_' . $configType);
+            $config->setOptions([$configType => $validElements]);
+            // Remove default valid elements if we're testing extended valid elements
+            if ($configType !== 'valid_elements') {
+                $config->setOptions(['valid_elements' => '']);
+            }
             $sanitiser = new HtmlEditorSanitiser($config);
 
             $value = 'noopener noreferrer';
@@ -144,12 +151,30 @@ public function testSanitisation()
             } elseif (strpos($desc ?? '', 'link_rel_value is null') !== false) {
                 $value = null;
             }
-            Config::inst()->set(HTMLEditorSanitiser::class, 'link_rel_value', $value);
+
+            HTMLEditorSanitiser::config()->set('link_rel_value', $value);
 
             $htmlValue = HTMLValue::create($input);
             $sanitiser->sanitise($htmlValue);
 
-            $this->assertEquals($output, $htmlValue->getContent(), $desc);
+            $this->assertEquals($output, $htmlValue->getContent(), "{$desc} - using config type: {$configType}");
         }
     }
+
+    /**
+     * Ensure that when there are no valid elements at all for a configuration set,
+     * nothing is allowed.
+     */
+    public function testSanitiseNoValidElements(): void
+    {
+        $config = HTMLEditorConfig::get('htmleditorsanitisertest');
+        $config->setOptions(['valid_elements' => '']);
+        $config->setOptions(['extended_valid_elements' => '']);
+        $sanitiser = new HtmlEditorSanitiser($config);
+
+        $htmlValue = HTMLValue::create('<p>standard text</p><table><tbody><tr><th><a href="some-link">text</a></th></tr><tr><td>Header</td></tr></tbody></table>');
+        $sanitiser->sanitise($htmlValue);
+
+        $this->assertEquals('standard texttextHeader', $htmlValue->getContent());
+    }
 }