From 80e40d75bbeceb79ad1443f98891a6dcdbe0e18e Mon Sep 17 00:00:00 2001 From: catch Date: Fri, 21 Oct 2022 13:44:27 +0100 Subject: [PATCH] Issue #3314478 by Wim Leers, pooja saraah, smustgrave, lauriii, alexpott: Follow-up for #3231334: global attributes should result in HTMLRestrictions becoming simplified (cherry picked from commit e7943531b354ba3b8ff435b639a0118e59d14450) --- .../ckeditor5/src/HTMLRestrictions.php | 170 ++++++++++++++++++ .../src/Kernel/SmartDefaultSettingsTest.php | 8 +- .../tests/src/Kernel/ValidatorsTest.php | 1 + .../tests/src/Unit/HTMLRestrictionsTest.php | 117 ++++++++++++ 4 files changed, 295 insertions(+), 1 deletion(-) diff --git a/core/modules/ckeditor5/src/HTMLRestrictions.php b/core/modules/ckeditor5/src/HTMLRestrictions.php index f7be01a33d5..68cd0589e53 100644 --- a/core/modules/ckeditor5/src/HTMLRestrictions.php +++ b/core/modules/ckeditor5/src/HTMLRestrictions.php @@ -84,7 +84,54 @@ final class HTMLRestrictions { self::validateAllowedRestrictionsPhase2($elements); self::validateAllowedRestrictionsPhase3($elements); self::validateAllowedRestrictionsPhase4($elements); + self::validateAllowedRestrictionsPhase5($elements); $this->elements = $elements; + + // Simplify based on the global attributes: + // - `

<* dir>` must become `

<* dir>` + // - `

<* foo="a b">` must become `

<* foo="a b">` + // - `

<* foo="a b">` must become `

<* foo="a b">` + // In other words: the restrictions on `<*>` remain untouched, but the + // attributes and attribute values allowed by `<*>` should be omitted from + // all other tags. + // Note: `<*>` also allows specifying disallowed attributes, but no other + // tags are allowed to do this. Consequently, simplification is only needed + // if >=1 allowed attribute is present on `<*>`. + if (count($elements) >= 2 && array_key_exists('*', $elements) && array_filter($elements['*'])) { + // @see \Drupal\ckeditor5\HTMLRestrictions::validateAllowedRestrictionsPhase4() + $globally_allowed_attribute_restrictions = array_filter($elements['*']); + + // Prepare to compare the restrictions of all tags with those on the + // global attribute tag `<*>`. + $original = []; + $global = []; + foreach ($elements as $tag => $restrictions) { + // `<*>`'s attribute restrictions do not need to be compared. + if ($tag === '*') { + continue; + } + $original[$tag] = $restrictions; + $global[$tag] = $globally_allowed_attribute_restrictions; + } + + // The subset of attribute restrictions after diffing with those on `<*>`. + $net_global_attribute_restrictions = (new self($original)) + ->doDiff(new self($global)) + ->getAllowedElements(FALSE); + + // Update each tag's attribute restrictions to the subset. + foreach ($elements as $tag => $restrictions) { + // `<*>` remains untouched. + if ($tag === '*') { + continue; + } + $this->elements[$tag] = $net_global_attribute_restrictions[$tag] + // If the tag is absent from the subset, then its attribute + // restrictions were a strict subset of `<*>`: just allowing the tag + // without allowing attributes is then sufficient. + ?? FALSE; + } + } } /** @@ -119,6 +166,7 @@ final class HTMLRestrictions { // @see https://html.spec.whatwg.org/multipage/dom.html#global-attributes // @see validateAllowedRestrictionsPhase2() // @see validateAllowedRestrictionsPhase4() + // @see validateAllowedRestrictionsPhase5() if ($html_tag_name === '*') { continue; } @@ -148,6 +196,7 @@ final class HTMLRestrictions { // of a text format. // @see https://html.spec.whatwg.org/multipage/dom.html#global-attributes // @see validateAllowedRestrictionsPhase4() + // @see validateAllowedRestrictionsPhase5() if ($html_tag_name === '*' && !is_array($html_tag_restrictions)) { throw new \InvalidArgumentException(sprintf('The value for the special "*" global attribute HTML tag must be an array of attribute restrictions.')); } @@ -226,6 +275,7 @@ final class HTMLRestrictions { // of a text format. // @see https://html.spec.whatwg.org/multipage/dom.html#global-attributes // @see validateAllowedRestrictionsPhase2() + // @see validateAllowedRestrictionsPhase5() if ($html_tag_name === '*' && $html_tag_attribute_restrictions === FALSE) { continue; } @@ -246,6 +296,99 @@ final class HTMLRestrictions { } } + /** + * Validates allowed elements — phase 5: disallowed attribute overrides. + * + * Explicit overrides of globally disallowed attributes are considered errors. + * For example: `

`, `` are considered errors when the + * `style` and `on*` attributes are globally disallowed. + * + * Implicit overrides are not treated as errors: if all attributes are allowed + * on a tag, globally disallowed attributes still apply. + * For example: `

` allows all attributes on `

`, but still won't allow + * globally disallowed attributes. + * + * @param array $elements + * The allowed elements. + * + * @throws \InvalidArgumentException + */ + private static function validateAllowedRestrictionsPhase5(array $elements): void { + $conflict = self::findElementsOverridingGloballyDisallowedAttributes($elements); + if ($conflict) { + [$globally_disallowed_attribute_restrictions, $elements_overriding_globally_disallowed_attributes] = $conflict; + throw new \InvalidArgumentException(sprintf( + 'The attribute restrictions in "%s" are allowing attributes "%s" that are disallowed by the special "*" global attribute restrictions.', + implode(' ', (new self($elements_overriding_globally_disallowed_attributes))->toCKEditor5ElementsArray()), + implode('", "', array_keys($globally_disallowed_attribute_restrictions)) + )); + } + } + + /** + * Finds elements overriding globally disallowed attributes. + * + * @param array $elements + * The allowed elements. + * + * @return null|array + * NULL if no conflict is found, an array containing otherwise, containing: + * - the globally disallowed attribute restrictions + * - the elements overriding globally disallowed attributes + */ + private static function findElementsOverridingGloballyDisallowedAttributes(array $elements): ?array { + // Find the globally disallowed attributes. + // For example: `['*' => ['style' => FALSE, 'foo' => TRUE, 'bar' => FALSE]` + // has two globally disallowed attributes, the code below will extract + // `['*' => ['style' => FALSE, 'bar' => FALSE']]`. + $globally_disallowed_attribute_restrictions = !array_key_exists('*', $elements) + ? [] + : array_filter($elements['*'], function ($global_attribute_restrictions): bool { + return $global_attribute_restrictions === FALSE; + }); + if (empty($globally_disallowed_attribute_restrictions)) { + // No conflict possible. + return NULL; + } + + // The elements which could potentially have a conflicting override. + $elements_with_attribute_level_restrictions = array_filter($elements, function ($attribute_restrictions, string $attribute_name): bool { + return is_array($attribute_restrictions) && $attribute_name !== '*'; + }, ARRAY_FILTER_USE_BOTH); + if (empty($elements_with_attribute_level_restrictions)) { + // No conflict possible. + return NULL; + } + + // Construct a HTMLRestrictions object containing just the elements that are + // potentially overriding globally disallowed attributes. + // For example: `['p' => ['style' => TRUE]]`. + $potentially_overriding = new self($elements_with_attribute_level_restrictions); + + // Construct a HTMLRestrictions object that contains the globally disallowed + // attribute restrictions, but pretends they are allowed. This allows using + // ::intersect() to detect a conflict. + $conflicting_restrictions = new self(array_fill_keys( + array_keys($elements_with_attribute_level_restrictions), + // The disallowed attributes converted to allowed, to allow using the + // ::intersect() method to detect a conflict. + // In the example: `['style' => TRUE', 'bar' => TRUE]`. + array_fill_keys(array_keys($globally_disallowed_attribute_restrictions), TRUE) + )); + + // When there is a non-empty intersection at the attribute level, an + // override of a globally disallowed attribute was found. + $conflict = $potentially_overriding->intersect($conflicting_restrictions); + $elements_overriding_globally_disallowed_attributes = array_filter($conflict->getAllowedElements()); + + // No conflict found. + if (empty($elements_overriding_globally_disallowed_attributes)) { + return NULL; + } + + return [$globally_disallowed_attribute_restrictions, $elements_overriding_globally_disallowed_attributes]; + } + /** * Creates the empty set of HTML restrictions: nothing is allowed. * @@ -350,6 +493,33 @@ final class HTMLRestrictions { } } + // FilterHtml accepts configuration for `allowed_html` that it will not + // actually apply. In other words: it allows for meaningless configuration. + // HTMLRestrictions strictly forbids tags overriding globally disallowed + // attributes; it considers these conflicting statements. Since FilterHtml + // will not apply these anyway, remove them from $allowed prior to + // constructing a HTMLRestrictions object: + // - `` will become `` since the `style` attribute + // is globally disallowed by FilterHtml + // - `` will become `` since the `on*` attribute is + // globally disallowed by FilterHtml + // - `` will become `` since the `on*` attribute + // is globally disallowed by FilterHtml + // @see ::validateAllowedRestrictionsPhase5() + // @see \Drupal\filter\Plugin\Filter\FilterHtml::process() + // @see \Drupal\filter\Plugin\Filter\FilterHtml::getHTMLRestrictions() + $conflict = self::findElementsOverridingGloballyDisallowedAttributes($allowed); + if ($conflict) { + [, $elements_overriding_globally_disallowed_attributes] = $conflict; + foreach ($elements_overriding_globally_disallowed_attributes as $element => $attributes) { + foreach (array_keys($attributes) as $attribute_name) { + unset($allowed[$element][$attribute_name]); + } + if ($allowed[$element] === []) { + $allowed[$element] = FALSE; + } + } + } return new self($allowed); } diff --git a/core/modules/ckeditor5/tests/src/Kernel/SmartDefaultSettingsTest.php b/core/modules/ckeditor5/tests/src/Kernel/SmartDefaultSettingsTest.php index 071beb68651..203131afb71 100644 --- a/core/modules/ckeditor5/tests/src/Kernel/SmartDefaultSettingsTest.php +++ b/core/modules/ckeditor5/tests/src/Kernel/SmartDefaultSettingsTest.php @@ -97,7 +97,13 @@ class SmartDefaultSettingsTest extends KernelTestBase { 'filter_html' => [ 'status' => 1, 'settings' => [ - 'allowed_html' => '


', + // Misconfiguration aspects: + // 1. ``, not ``, while `DrupalLink` is enabled + // 2. `

` even though `style` is globally disallowed by + // filter_html + // 3. `` even though `on*` is globally disallowed by + // filter_html + 'allowed_html' => '


', ], ], ], diff --git a/core/modules/ckeditor5/tests/src/Kernel/ValidatorsTest.php b/core/modules/ckeditor5/tests/src/Kernel/ValidatorsTest.php index d27563a488f..6529b5df262 100644 --- a/core/modules/ckeditor5/tests/src/Kernel/ValidatorsTest.php +++ b/core/modules/ckeditor5/tests/src/Kernel/ValidatorsTest.php @@ -1193,6 +1193,7 @@ class ValidatorsTest extends KernelTestBase { ], ], 'violations' => [ + 'filters.filter_html' => 'The current CKEditor 5 build requires the following elements and attributes:
<br> <p onhover style> <* dir="ltr rtl" lang> <img on*> <blockquote style> <marquee> <a onclick="javascript:*"> <code style="foo: bar;">
The following elements are missing:
<p onhover style> <img on*> <blockquote style> <code style="foo: bar;">', 'settings.plugins.ckeditor5_sourceEditing.allowed_tags.0' => 'The following tag in the Source Editing "Manually editable HTML tags" field is a security risk: <p onhover>.', 'settings.plugins.ckeditor5_sourceEditing.allowed_tags.1' => 'The following tag in the Source Editing "Manually editable HTML tags" field is a security risk: <img on*>.', 'settings.plugins.ckeditor5_sourceEditing.allowed_tags.2' => 'The following tag in the Source Editing "Manually editable HTML tags" field is a security risk: <blockquote style>.', diff --git a/core/modules/ckeditor5/tests/src/Unit/HTMLRestrictionsTest.php b/core/modules/ckeditor5/tests/src/Unit/HTMLRestrictionsTest.php index 59b0f423504..ed15093e8d1 100644 --- a/core/modules/ckeditor5/tests/src/Unit/HTMLRestrictionsTest.php +++ b/core/modules/ckeditor5/tests/src/Unit/HTMLRestrictionsTest.php @@ -133,6 +133,24 @@ class HTMLRestrictionsTest extends UnitTestCase { ['*' => ['foo' => ['a' => FALSE, 'b' => FALSE]]], 'The "*" HTML tag has attribute restriction "foo", but it is not an array of key-value pairs, with HTML tag attribute values as keys and TRUE as values.', ]; + + // Invalid overrides of globally disallowed attributes. + yield 'INVALID: when "bar" is globally disallowed' => [ + ['foo' => ['bar' => TRUE], '*' => ['bar' => FALSE, 'baz' => TRUE]], + 'The attribute restrictions in "" are allowing attributes "bar" that are disallowed by the special "*" global attribute restrictions', + ]; + yield 'INVALID: when "style" is globally disallowed' => [ + ['foo' => ['style' => TRUE], '*' => ['bar' => FALSE, 'baz' => TRUE, 'style' => FALSE]], + 'The attribute restrictions in "" are allowing attributes "bar", "style" that are disallowed by the special "*" global attribute restrictions', + ]; + yield 'INVALID: when "on*" is globally disallowed' => [ + ['foo' => ['on*' => TRUE], '*' => ['bar' => FALSE, 'baz' => TRUE, 'style' => FALSE, 'on*' => FALSE]], + 'The attribute restrictions in "" are allowing attributes "bar", "style", "on*" that are disallowed by the special "*" global attribute restrictions', + ]; + yield 'INVALID: when "on" is globally disallowed' => [ + ['foo' => ['ontouch' => TRUE], '*' => ['bar' => FALSE, 'baz' => TRUE, 'style' => FALSE, 'on*' => FALSE]], + 'The attribute restrictions in "" are allowing attributes "bar", "style", "on*" that are disallowed by the special "*" global attribute restrictions', + ]; } /** @@ -507,6 +525,105 @@ class HTMLRestrictionsTest extends UnitTestCase { '

', ['h2' => ['id' => ['jump-*' => TRUE]]], ]; + + // Attribute restrictions that match the global attribute restrictions + // should be omitted from concrete tags. + yield '

<* foo>' => [ + '

<* foo>', + ['p' => FALSE, '*' => ['foo' => TRUE]], + ]; + yield '

<* foo> results in

getting simplified' => [ + '

<* foo>', + ['p' => FALSE, '*' => ['foo' => TRUE]], + ]; + yield '<* foo>

results in

getting simplified' => [ + '<* foo>

', + ['p' => FALSE, '*' => ['foo' => TRUE]], + ]; + yield '

<* foo> results in

getting simplified' => [ + '

<* foo>', + ['p' => ['bar' => TRUE], '*' => ['foo' => TRUE]], + ]; + yield '<* foo>

results in

getting simplified' => [ + '<* foo>

', + ['p' => ['bar' => TRUE], '*' => ['foo' => TRUE]], + ]; + yield '

+ <* foo="b a"> results in

getting simplified' => [ + '

<* foo="b a">', + ['p' => FALSE, '*' => ['foo' => ['b' => TRUE, 'a' => TRUE]]], + ]; + yield '<* foo="b a">

results in

getting simplified' => [ + '<* foo="b a">

', + ['p' => FALSE, '*' => ['foo' => ['b' => TRUE, 'a' => TRUE]]], + ]; + yield '

+ <* foo="b a"> results in

getting simplified' => [ + '

<* foo="b a">', + ['p' => ['bar' => TRUE], '*' => ['foo' => ['b' => TRUE, 'a' => TRUE]]], + ]; + yield '<* foo="b a">

results in

getting simplified' => [ + '<* foo="b a">

', + ['p' => ['bar' => TRUE], '*' => ['foo' => ['b' => TRUE, 'a' => TRUE]]], + ]; + yield '

+ <* foo="b a"> results in

getting simplified' => [ + '

<* foo="b a">', + ['p' => ['foo' => ['c' => TRUE]], '*' => ['foo' => ['b' => TRUE, 'a' => TRUE]]], + ]; + yield '<* foo="b a">

results in

getting simplified' => [ + '<* foo="b a">

', + ['p' => ['foo' => ['c' => TRUE]], '*' => ['foo' => ['b' => TRUE, 'a' => TRUE]]], + ]; + // Attribute restrictions that match the global attribute restrictions + // should be omitted from wildcard tags. + yield '

<$text-container foo> <* foo> results in <$text-container> getting simplified' => [ + '

<$text-container foo> <* foo>', + ['p' => FALSE, '*' => ['foo' => TRUE]], + ['p' => FALSE, '$text-container' => FALSE, '*' => ['foo' => TRUE]], + ]; + yield '<* foo>

results in <$text-container> getting stripped' => [ + '<* foo>

<$text-container foo>', + ['p' => FALSE, '*' => ['foo' => TRUE]], + ['p' => FALSE, '*' => ['foo' => TRUE], '$text-container' => FALSE], + ]; + yield '

<$text-container foo bar> <* foo> results in <$text-container> getting simplified' => [ + '

<$text-container foo bar> <* foo>', + ['p' => ['bar' => TRUE], '*' => ['foo' => TRUE]], + ['p' => FALSE, '$text-container' => ['bar' => TRUE], '*' => ['foo' => TRUE]], + ]; + yield '<* foo> <$text-container foo bar>

results in <$text-container> getting simplified' => [ + '<* foo> <$text-container foo bar>

', + ['p' => ['bar' => TRUE], '*' => ['foo' => TRUE]], + ['p' => FALSE, '*' => ['foo' => TRUE], '$text-container' => ['bar' => TRUE]], + ]; + yield '

<$text-container foo="a b"> + <* foo="b a"> results in <$text-container> getting simplified' => [ + '

<$text-container foo="a b"> <* foo="b a">', + ['p' => FALSE, '*' => ['foo' => ['b' => TRUE, 'a' => TRUE]]], + ['p' => FALSE, '$text-container' => FALSE, '*' => ['foo' => ['b' => TRUE, 'a' => TRUE]]], + ]; + yield '<* foo="b a">

<$text-container foo="a b"> results in <$text-container> getting simplified' => [ + '<* foo="b a">

<$text-container foo="a b">', + ['p' => FALSE, '*' => ['foo' => ['b' => TRUE, 'a' => TRUE]]], + ['p' => FALSE, '*' => ['foo' => ['b' => TRUE, 'a' => TRUE]], '$text-container' => FALSE], + ]; + yield '

<$text-container foo="a b" bar> + <* foo="b a"> results in <$text-container> getting simplified' => [ + '

<$text-container foo="a b" bar> <* foo="b a">', + ['p' => ['bar' => TRUE], '*' => ['foo' => ['b' => TRUE, 'a' => TRUE]]], + ['p' => FALSE, '$text-container' => ['bar' => TRUE], '*' => ['foo' => ['b' => TRUE, 'a' => TRUE]]], + ]; + yield '<* foo="b a">

<$text-container foo="a b" bar> results in <$text-container> getting simplified' => [ + '<* foo="b a">

<$text-container foo="a b" bar>', + ['p' => ['bar' => TRUE], '*' => ['foo' => ['b' => TRUE, 'a' => TRUE]]], + ['p' => FALSE, '*' => ['foo' => ['b' => TRUE, 'a' => TRUE]], '$text-container' => ['bar' => TRUE]], + ]; + yield '

<$text-container foo="a b c"> + <* foo="b a"> results in <$text-container> getting simplified' => [ + '

<$text-container foo="a b c"> <* foo="b a">', + ['p' => ['foo' => ['c' => TRUE]], '*' => ['foo' => ['b' => TRUE, 'a' => TRUE]]], + ['p' => FALSE, '$text-container' => ['foo' => ['c' => TRUE]], '*' => ['foo' => ['b' => TRUE, 'a' => TRUE]]], + ]; + yield '<* foo="b a">

<$text-container foo="a b c"> results in <$text-container> getting simplified' => [ + '<* foo="b a">

<$text-container foo="a b c">', + ['p' => ['foo' => ['c' => TRUE]], '*' => ['foo' => ['b' => TRUE, 'a' => TRUE]]], + ['p' => FALSE, '*' => ['foo' => ['b' => TRUE, 'a' => TRUE]], '$text-container' => ['foo' => ['c' => TRUE]]], + ]; } /**