From 99e965b5d7b3a90ce6be0b1233c3e9828c9a79b7 Mon Sep 17 00:00:00 2001 From: Bernie Hamlin Date: Mon, 16 Oct 2023 12:39:18 +1300 Subject: [PATCH 1/5] FIX Use field editorconfig when sanitising content --- src/Forms/HTMLEditor/HTMLEditorField.php | 3 +- .../Forms/HTMLEditor/HTMLEditorFieldTest.php | 38 +++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/src/Forms/HTMLEditor/HTMLEditorField.php b/src/Forms/HTMLEditor/HTMLEditorField.php index db8d08a05..3edd623f7 100644 --- a/src/Forms/HTMLEditor/HTMLEditorField.php +++ b/src/Forms/HTMLEditor/HTMLEditorField.php @@ -138,7 +138,8 @@ class HTMLEditorField extends TextareaField // 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/tests/php/Forms/HTMLEditor/HTMLEditorFieldTest.php b/tests/php/Forms/HTMLEditor/HTMLEditorFieldTest.php index 2abe550aa..b046a1fd2 100644 --- a/tests/php/Forms/HTMLEditor/HTMLEditorFieldTest.php +++ b/tests/php/Forms/HTMLEditor/HTMLEditorFieldTest.php @@ -12,6 +12,7 @@ use SilverStripe\Control\Director; 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 @@ EOS $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 = '

standard text

Header'; + $htmlValue = '

standard text

Header
'; + $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'); + } } From 584968e80c31e4f95773fec3d99a2167d0adaf32 Mon Sep 17 00:00:00 2001 From: Guy Sartorelli Date: Thu, 18 Apr 2024 14:27:25 +1200 Subject: [PATCH 2/5] MNT Update tests to use a dataprovider Also explicitly test both valid_elements and extended_valid_elements --- .../HTMLEditor/HTMLEditorSanitiserTest.php | 28 ++++++++++++------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/tests/php/Forms/HTMLEditor/HTMLEditorSanitiserTest.php b/tests/php/Forms/HTMLEditor/HTMLEditorSanitiserTest.php index 3d5c3d5c6..d43dd9a5f 100644 --- a/tests/php/Forms/HTMLEditor/HTMLEditorSanitiserTest.php +++ b/tests/php/Forms/HTMLEditor/HTMLEditorSanitiserTest.php @@ -11,9 +11,9 @@ use SilverStripe\View\Parsers\HTMLValue; class HTMLEditorSanitiserTest extends FunctionalTest { - public function testSanitisation() + public function provideSanitise(): array { - $tests = [ + return [ [ 'p,strong', '

Leave Alone

Strip parentBut keep children in order
', @@ -129,13 +129,20 @@ class HTMLEditorSanitiserTest extends FunctionalTest '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,13 @@ class HTMLEditorSanitiserTest extends FunctionalTest } 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}"); } } } From a4adad60e96cdcb8ebc87ea9f935cce47ac06a3a Mon Sep 17 00:00:00 2001 From: Guy Sartorelli Date: Thu, 18 Apr 2024 14:28:02 +1200 Subject: [PATCH 3/5] FIX Don't skip sanitisation when no valid elements are defined --- src/Forms/HTMLEditor/HTMLEditorSanitiser.php | 4 ---- .../HTMLEditor/HTMLEditorSanitiserTest.php | 17 +++++++++++++++++ 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/src/Forms/HTMLEditor/HTMLEditorSanitiser.php b/src/Forms/HTMLEditor/HTMLEditorSanitiser.php index a075d98fa..fa23c476b 100644 --- a/src/Forms/HTMLEditor/HTMLEditorSanitiser.php +++ b/src/Forms/HTMLEditor/HTMLEditorSanitiser.php @@ -287,10 +287,6 @@ class HTMLEditorSanitiser */ public function sanitise(HTMLValue $html) { - if (!$this->elements && !$this->elementPatterns) { - return; - } - $linkRelValue = $this->config()->get('link_rel_value'); $doc = $html->getDocument(); diff --git a/tests/php/Forms/HTMLEditor/HTMLEditorSanitiserTest.php b/tests/php/Forms/HTMLEditor/HTMLEditorSanitiserTest.php index d43dd9a5f..6e68b39d6 100644 --- a/tests/php/Forms/HTMLEditor/HTMLEditorSanitiserTest.php +++ b/tests/php/Forms/HTMLEditor/HTMLEditorSanitiserTest.php @@ -160,4 +160,21 @@ class HTMLEditorSanitiserTest extends FunctionalTest $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('

standard text

text
Header
'); + $sanitiser->sanitise($htmlValue); + + $this->assertEquals('standard texttextHeader', $htmlValue->getContent()); + } } From 2bdc24c86a4474106a85f9e648379290408f057e Mon Sep 17 00:00:00 2001 From: Guy Sartorelli Date: Thu, 18 Apr 2024 14:38:02 +1200 Subject: [PATCH 4/5] ENH Set default valid_elements for new TinyMCE config --- src/Forms/HTMLEditor/TinyMCEConfig.php | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/Forms/HTMLEditor/TinyMCEConfig.php b/src/Forms/HTMLEditor/TinyMCEConfig.php index 80621abd0..bb940a1b0 100644 --- a/src/Forms/HTMLEditor/TinyMCEConfig.php +++ b/src/Forms/HTMLEditor/TinyMCEConfig.php @@ -311,6 +311,20 @@ 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]," ]; /** From 72692f9f1017c932dde5fac77970bdfbe9de2f46 Mon Sep 17 00:00:00 2001 From: Guy Sartorelli Date: Thu, 18 Apr 2024 14:38:26 +1200 Subject: [PATCH 5/5] NEW Make default TinyMCE settings configurable --- src/Forms/HTMLEditor/TinyMCEConfig.php | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/Forms/HTMLEditor/TinyMCEConfig.php b/src/Forms/HTMLEditor/TinyMCEConfig.php index bb940a1b0..8b5076f39 100644 --- 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' => [ @@ -327,6 +325,8 @@ class TinyMCEConfig extends HTMLEditorConfig implements i18nEntityProvider . "dd[id|class|title|dir],dl[id|class|title|dir],dt[id|class|title|dir]," ]; + protected $settings = []; + /** * Holder list of enabled plugins * @@ -351,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 *