From 910b34ac7a29e509b93969e817b94f682ee329eb Mon Sep 17 00:00:00 2001
From: bnjmnm <benm@umich.edu>
Date: Wed, 23 Feb 2022 05:53:44 -0500
Subject: [PATCH] Issue #3259493 by Wim Leers, lauriii, larowlan: [GHS] Unable
to limit attribute values: ::allowedElementsStringToHtmlSupportConfig() does
not generate configuration that CKEditor 5 expects
---
.../ckeditor5/src/HTMLRestrictions.php | 20 +-
.../SourceEditingTest.php | 205 ++++++++++++++++++
.../tests/src/Traits/CKEditor5TestTrait.php | 14 +-
.../tests/src/Unit/HTMLRestrictionsTest.php | 15 +-
4 files changed, 247 insertions(+), 7 deletions(-)
create mode 100644 core/modules/ckeditor5/tests/src/FunctionalJavascript/SourceEditingTest.php
diff --git a/core/modules/ckeditor5/src/HTMLRestrictions.php b/core/modules/ckeditor5/src/HTMLRestrictions.php
index 9870153281b2..d6c47cbd282c 100644
--- a/core/modules/ckeditor5/src/HTMLRestrictions.php
+++ b/core/modules/ckeditor5/src/HTMLRestrictions.php
@@ -807,8 +807,26 @@ public function toGeneralHtmlSupportConfig(): array {
if (is_array($value)) {
$value = array_keys($value);
}
+ // Drupal never allows style attributes due to security concerns.
+ // @see \Drupal\Component\Utility\Xss
+ if ($name === 'style') {
+ continue;
+ }
assert($value === TRUE || Inspector::assertAllStrings($value));
- $to_allow['attributes'][$name] = $value;
+ if ($name === 'class') {
+ $to_allow['classes'] = $value;
+ continue;
+ }
+ // If a single attribute value is allowed, it must be TRUE (see the
+ // assertion above). Otherwise, it must be an array of strings (see
+ // the assertion above), which lists all allowed attribute values. To
+ // be able to configure GHS to a range of values, we need to use a
+ // regular expression.
+ // @todo Expand to support partial wildcards in
+ // https://www.drupal.org/project/drupal/issues/3260853.
+ $to_allow['attributes'][$name] = is_array($value)
+ ? ['regexp' => ['pattern' => '/^(' . implode('|', $value) . ')$/']]
+ : $value;
}
}
$allowed[] = $to_allow;
diff --git a/core/modules/ckeditor5/tests/src/FunctionalJavascript/SourceEditingTest.php b/core/modules/ckeditor5/tests/src/FunctionalJavascript/SourceEditingTest.php
new file mode 100644
index 000000000000..d9a057fcb67c
--- /dev/null
+++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/SourceEditingTest.php
@@ -0,0 +1,205 @@
+<?php
+
+namespace Drupal\Tests\ckeditor5\FunctionalJavascript;
+
+use Drupal\ckeditor5\HTMLRestrictions;
+use Drupal\editor\Entity\Editor;
+use Drupal\filter\Entity\FilterFormat;
+use Drupal\Tests\ckeditor5\Traits\CKEditor5TestTrait;
+use Drupal\ckeditor5\Plugin\Editor\CKEditor5;
+use Symfony\Component\Validator\ConstraintViolation;
+
+/**
+ * @coversDefaultClass \Drupal\ckeditor5\Plugin\CKEditor5Plugin\SourceEditing
+ * @group ckeditor5
+ * @internal
+ */
+class SourceEditingTest extends CKEditor5TestBase {
+
+ use CKEditor5TestTrait;
+
+ /**
+ * The user to use during testing.
+ *
+ * @var \Drupal\user\UserInterface
+ */
+ protected $adminUser;
+
+ /**
+ * A host entity with a body field whose text to edit with CKEditor 5.
+ *
+ * @var \Drupal\node\NodeInterface
+ */
+ protected $host;
+
+ /**
+ * {@inheritdoc}
+ */
+ protected static $modules = [
+ 'ckeditor5',
+ 'node',
+ 'text',
+ ];
+
+ /**
+ * {@inheritdoc}
+ */
+ protected $defaultTheme = 'stark';
+
+ /**
+ * {@inheritdoc}
+ */
+ protected function setUp(): void {
+ parent::setUp();
+
+ FilterFormat::create([
+ 'format' => 'test_format',
+ 'name' => 'Test format',
+ 'filters' => [
+ 'filter_html' => [
+ 'status' => TRUE,
+ 'settings' => [
+ 'allowed_html' => '<p> <br> <a href>',
+ ],
+ ],
+ 'filter_align' => ['status' => TRUE],
+ 'filter_caption' => ['status' => TRUE],
+ ],
+ ])->save();
+ Editor::create([
+ 'editor' => 'ckeditor5',
+ 'format' => 'test_format',
+ 'settings' => [
+ 'toolbar' => [
+ 'items' => [
+ 'sourceEditing',
+ 'link',
+ ],
+ ],
+ 'plugins' => [
+ 'ckeditor5_sourceEditing' => [
+ 'allowed_tags' => [],
+ ],
+ ],
+ ],
+ 'image_upload' => [
+ 'status' => FALSE,
+ ],
+ ])->save();
+ $this->assertSame([], array_map(
+ function (ConstraintViolation $v) {
+ return (string) $v->getMessage();
+ },
+ iterator_to_array(CKEditor5::validatePair(
+ Editor::load('test_format'),
+ FilterFormat::load('test_format')
+ ))
+ ));
+ $this->adminUser = $this->drupalCreateUser([
+ 'use text format test_format',
+ 'bypass node access',
+ ]);
+
+ // Create a sample host entity to test CKEditor 5.
+ $this->host = $this->createNode([
+ 'type' => 'page',
+ 'title' => 'Animals with strange names',
+ 'body' => [
+ 'value' => '<p>The <a href="https://example.com/pirate" class="button" data-grammar="subject">pirate</a> is <a href="https://example.com/irate" class="use-ajax" data-grammar="adjective">irate</a>.</p>',
+ 'format' => 'test_format',
+ ],
+ ]);
+ $this->host->save();
+
+ $this->drupalLogin($this->adminUser);
+ }
+
+ /**
+ * Tests allowing extra attributes on already supported tags using GHS.
+ *
+ * @dataProvider providerAllowingExtraAttributes
+ */
+ public function testAllowingExtraAttributes(string $expected_markup, ?string $allowed_elements_string = NULL) {
+ if ($allowed_elements_string) {
+ // Allow creating additional HTML using SourceEditing.
+ $text_editor = Editor::load('test_format');
+ $settings = $text_editor->getSettings();
+ $settings['plugins']['ckeditor5_sourceEditing']['allowed_tags'][] = $allowed_elements_string;
+ $text_editor->setSettings($settings);
+
+ // Keep the allowed HTML tags in sync.
+ $text_format = FilterFormat::load('test_format');
+ $allowed_elements = HTMLRestrictions::fromTextFormat($text_format);
+ $updated_allowed_tags = $allowed_elements->merge(HTMLRestrictions::fromString($allowed_elements_string));
+ $filter_html_config = $text_format->filters('filter_html')
+ ->getConfiguration();
+ $filter_html_config['settings']['allowed_html'] = $updated_allowed_tags->toFilterHtmlAllowedTagsString();
+ $text_format->setFilterConfig('filter_html', $filter_html_config);
+
+ // Verify the text format and editor are still a valid pair.
+ $this->assertSame([], array_map(
+ function (ConstraintViolation $v) {
+ return (string) $v->getMessage();
+ },
+ iterator_to_array(CKEditor5::validatePair(
+ $text_editor,
+ $text_format
+ ))
+ ));
+
+ // If valid, save both.
+ $text_format->save();
+ $text_editor->save();
+ }
+
+ $this->drupalGet($this->host->toUrl('edit-form'));
+ $this->waitForEditor();
+ $this->assertSame($expected_markup, $this->getEditorDataAsHtmlString());
+ }
+
+ /**
+ * Data provider for ::testAllowingExtraAttributes().
+ *
+ * @return array
+ * The test cases.
+ */
+ public function providerAllowingExtraAttributes(): array {
+ return [
+ 'no extra attributes allowed' => [
+ '<p>The <a href="https://example.com/pirate">pirate</a> is <a href="https://example.com/irate">irate</a>.</p>',
+ ],
+
+ // Common case: any attribute that is not `style` or `class`.
+ '<a data-grammar="subject">' => [
+ '<p>The <a href="https://example.com/pirate" data-grammar="subject">pirate</a> is <a href="https://example.com/irate">irate</a>.</p>',
+ '<a data-grammar="subject">',
+ ],
+ '<a data-grammar="adjective">' => [
+ '<p>The <a href="https://example.com/pirate">pirate</a> is <a href="https://example.com/irate" data-grammar="adjective">irate</a>.</p>',
+ '<a data-grammar="adjective">',
+ ],
+ '<a data-grammar>' => [
+ '<p>The <a href="https://example.com/pirate" data-grammar="subject">pirate</a> is <a href="https://example.com/irate" data-grammar="adjective">irate</a>.</p>',
+ '<a data-grammar>',
+ ],
+
+ // Edge case: `class`.
+ '<a class="button">' => [
+ '<p>The <a class="button" href="https://example.com/pirate">pirate</a> is <a href="https://example.com/irate">irate</a>.</p>',
+ '<a class="button">',
+ ],
+ '<a class="use-ajax">' => [
+ '<p>The <a href="https://example.com/pirate">pirate</a> is <a class="use-ajax" href="https://example.com/irate">irate</a>.</p>',
+ '<a class="use-ajax">',
+ ],
+ '<a class>' => [
+ '<p>The <a class="button" href="https://example.com/pirate">pirate</a> is <a class="use-ajax" href="https://example.com/irate">irate</a>.</p>',
+ '<a class>',
+ ],
+
+ // Edge case: `style`.
+ // @todo https://www.drupal.org/project/drupal/issues/3260857
+ ];
+ }
+
+}
diff --git a/core/modules/ckeditor5/tests/src/Traits/CKEditor5TestTrait.php b/core/modules/ckeditor5/tests/src/Traits/CKEditor5TestTrait.php
index 2510d3a58fcc..1b2adbb59985 100644
--- a/core/modules/ckeditor5/tests/src/Traits/CKEditor5TestTrait.php
+++ b/core/modules/ckeditor5/tests/src/Traits/CKEditor5TestTrait.php
@@ -19,10 +19,20 @@ trait CKEditor5TestTrait {
*
* @return \DOMDocument
* The result of parsing CKEditor 5's data into a PHP DOMDocument.
+ */
+ protected function getEditorDataAsDom(): \DOMDocument {
+ return Html::load($this->getEditorDataAsHtmlString());
+ }
+
+ /**
+ * Gets CKEditor 5 instance data as a HTML string.
+ *
+ * @return string
+ * The result of retrieving CKEditor 5's data.
*
* @see https://ckeditor.com/docs/ckeditor5/latest/api/module_editor-classic_classiceditor-ClassicEditor.html#function-getData
*/
- protected function getEditorDataAsDom(): \DOMDocument {
+ protected function getEditorDataAsHtmlString(): string {
// We cannot trust on CKEditor updating the textarea every time model
// changes. Therefore, the most reliable way to get downcasted data is to
// use the CKEditor API.
@@ -31,7 +41,7 @@ protected function getEditorDataAsDom(): \DOMDocument {
return Drupal.CKEditor5Instances.get(Drupal.CKEditor5Instances.keys().next().value).getData();
})();
JS;
- return Html::load($this->getSession()->evaluateScript($javascript));
+ return $this->getSession()->evaluateScript($javascript);
}
/**
diff --git a/core/modules/ckeditor5/tests/src/Unit/HTMLRestrictionsTest.php b/core/modules/ckeditor5/tests/src/Unit/HTMLRestrictionsTest.php
index 4a8779420281..840cf083ad30 100644
--- a/core/modules/ckeditor5/tests/src/Unit/HTMLRestrictionsTest.php
+++ b/core/modules/ckeditor5/tests/src/Unit/HTMLRestrictionsTest.php
@@ -442,15 +442,19 @@ public function providerRepresentations(): \Generator {
];
yield 'realistic' => [
- new HTMLRestrictions(['a' => ['href' => TRUE, 'hreflang' => ['en' => TRUE, 'fr' => TRUE]], 'p' => ['data-*' => TRUE], 'br' => FALSE]),
- ['<a href hreflang="en fr">', '<p data-*>', '<br>'],
- '<a href hreflang="en fr"> <p data-*> <br>',
+ new HTMLRestrictions(['a' => ['href' => TRUE, 'hreflang' => ['en' => TRUE, 'fr' => TRUE]], 'p' => ['data-*' => TRUE, 'class' => ['block' => TRUE]], 'br' => FALSE]),
+ ['<a href hreflang="en fr">', '<p data-* class="block">', '<br>'],
+ '<a href hreflang="en fr"> <p data-* class="block"> <br>',
[
[
'name' => 'a',
'attributes' => [
'href' => TRUE,
- 'hreflang' => ['en', 'fr'],
+ 'hreflang' => [
+ 'regexp' => [
+ 'pattern' => '/^(en|fr)$/',
+ ],
+ ],
],
],
[
@@ -458,6 +462,9 @@ public function providerRepresentations(): \Generator {
'attributes' => [
'data-*' => TRUE,
],
+ 'classes' => [
+ 'block',
+ ],
],
['name' => 'br'],
],
--
GitLab