From 35b8d4f54c5aa0b1ff3a2ecfb85e9d96f246fe64 Mon Sep 17 00:00:00 2001 From: catch Date: Mon, 2 May 2022 17:20:08 +0100 Subject: [PATCH] Issue #3272516 by Wim Leers, yogeshmpawar, bnjmnm, catch: Deprecate FilterInterface::getHTMLRestrictions()' forbidden_tags functionality --- .../ckeditor5/src/HTMLRestrictions.php | 9 --- .../FundamentalCompatibilityConstraint.php | 7 --- ...mentalCompatibilityConstraintValidator.php | 56 ------------------- .../tests/src/Kernel/ValidatorsTest.php | 27 --------- .../editor/src/EditorXssFilter/Standard.php | 40 +------------ .../filter/src/Entity/FilterFormat.php | 39 +++---------- .../filter/src/FilterFormatInterface.php | 6 +- .../filter/src/Plugin/FilterInterface.php | 14 +---- .../config/schema/filter_test.schema.yml | 6 -- .../FilterTestRestrictTagsAndAttributes.php | 5 -- .../filter/tests/src/Kernel/FilterAPITest.php | 7 ++- 11 files changed, 20 insertions(+), 196 deletions(-) diff --git a/core/modules/ckeditor5/src/HTMLRestrictions.php b/core/modules/ckeditor5/src/HTMLRestrictions.php index af7cdeb178d..e5b0d14e9c5 100644 --- a/core/modules/ckeditor5/src/HTMLRestrictions.php +++ b/core/modules/ckeditor5/src/HTMLRestrictions.php @@ -37,9 +37,6 @@ use Drupal\filter\Plugin\FilterInterface; * * @see ::WILDCARD_ELEMENT_METHODS * - * NOTE: Currently only supports the 'allowed' portion. - * @todo Add support for "forbidden" tags in https://www.drupal.org/project/drupal/issues/3231336 - * * @internal */ final class HTMLRestrictions { @@ -341,12 +338,6 @@ final class HTMLRestrictions { } $restrictions = $object->getHTMLRestrictions(); - if (!isset($restrictions['allowed'])) { - // @todo Handle HTML restrictor filters that only set forbidden_tags - // https://www.drupal.org/project/ckeditor5/issues/3231336. - throw new \DomainException('text formats with only filters that forbid tags rather than allowing tags are not yet supported.'); - } - $allowed = $restrictions['allowed']; return new self($allowed); diff --git a/core/modules/ckeditor5/src/Plugin/Validation/Constraint/FundamentalCompatibilityConstraint.php b/core/modules/ckeditor5/src/Plugin/Validation/Constraint/FundamentalCompatibilityConstraint.php index 6932fe8ae4f..6487dcd3faf 100644 --- a/core/modules/ckeditor5/src/Plugin/Validation/Constraint/FundamentalCompatibilityConstraint.php +++ b/core/modules/ckeditor5/src/Plugin/Validation/Constraint/FundamentalCompatibilityConstraint.php @@ -25,13 +25,6 @@ class FundamentalCompatibilityConstraint extends Constraint { */ public $noMarkupFiltersMessage = 'CKEditor 5 only works with HTML-based text formats. The "%filter_label" (%filter_plugin_id) filter implies this text format is not HTML anymore.'; - /** - * The violation message when fundamental HTML elements are forbidden. - * - * @var string - */ - public $forbiddenElementsMessage = 'CKEditor 5 needs at least the <p> and <br> tags to be allowed to be able to function. They are forbidden by the "%filter_label" (%filter_plugin_id) filter.'; - /** * The violation message when fundamental HTML elements are not allowed. * diff --git a/core/modules/ckeditor5/src/Plugin/Validation/Constraint/FundamentalCompatibilityConstraintValidator.php b/core/modules/ckeditor5/src/Plugin/Validation/Constraint/FundamentalCompatibilityConstraintValidator.php index b401acb1a1e..fba1985842b 100644 --- a/core/modules/ckeditor5/src/Plugin/Validation/Constraint/FundamentalCompatibilityConstraintValidator.php +++ b/core/modules/ckeditor5/src/Plugin/Validation/Constraint/FundamentalCompatibilityConstraintValidator.php @@ -114,22 +114,7 @@ class FundamentalCompatibilityConstraintValidator extends ConstraintValidator im */ private function checkHtmlRestrictionsAreCompatible(FilterFormatInterface $text_format, FundamentalCompatibilityConstraint $constraint): void { $fundamental = new HTMLRestrictions($this->pluginManager->getProvidedElements(self::FUNDAMENTAL_CKEDITOR5_PLUGINS)); - - // @todo Remove in favor of HTMLRestrictions::diff() in https://www.drupal.org/project/drupal/issues/3231336 $html_restrictions = $text_format->getHtmlRestrictions(); - $minimum_tags = array_keys($fundamental->getAllowedElements()); - $forbidden_minimum_tags = isset($html_restrictions['forbidden_tags']) - ? array_diff($minimum_tags, $html_restrictions['forbidden_tags']) - : []; - if (!empty($forbidden_minimum_tags)) { - $offending_filter = static::findHtmlRestrictorFilterForbiddingTags($text_format, $minimum_tags); - $this->context->buildViolation($constraint->forbiddenElementsMessage) - ->setParameter('%filter_label', (string) $offending_filter->getLabel()) - ->setParameter('%filter_plugin_id', $offending_filter->getPluginId()) - ->addViolation(); - } - - // @todo Remove early return in https://www.drupal.org/project/drupal/issues/3231336 if (!isset($html_restrictions['allowed'])) { return; } @@ -212,47 +197,6 @@ class FundamentalCompatibilityConstraintValidator extends ConstraintValidator im } } - /** - * Analyzes a text format to find the filter not allowing required tags. - * - * @param \Drupal\filter\FilterFormatInterface $text_format - * A text format whose filters to check for compatibility. - * @param string[] $required_tags - * A list of HTML tags that are required. - * - * @return \Drupal\filter\Plugin\FilterInterface - * The filter plugin instance not allowing the required tags. - * - * @throws \InvalidArgumentException - */ - private static function findHtmlRestrictorFilterForbiddingTags(FilterFormatInterface $text_format, array $required_tags): FilterInterface { - // Get HTML restrictor filters that actually restrict HTML. - $filters = static::getFiltersInFormatOfType( - $text_format, - FilterInterface::TYPE_HTML_RESTRICTOR, - function (FilterInterface $filter) { - return $filter->getHTMLRestrictions() !== FALSE; - } - ); - - foreach ($filters as $filter) { - $restrictions = $filter->getHTMLRestrictions(); - - // @todo Fix - // \Drupal\filter_test\Plugin\Filter\FilterTestRestrictTagsAndAttributes::getHTMLRestrictions(), - // whose computed value for forbidden_tags does not comply with the API - // https://www.drupal.org/project/drupal/issues/3231331. - if (array_keys($restrictions['forbidden_tags']) != range(0, count($restrictions['forbidden_tags']))) { - $restrictions['forbidden_tags'] = array_keys($restrictions['forbidden_tags']); - } - if (isset($restrictions['forbidden_tags']) && !empty(array_intersect($required_tags, $restrictions['forbidden_tags']))) { - return $filter; - } - } - - throw new \InvalidArgumentException('This text format does not have a "tags forbidden" restriction that includes the required tags.'); - } - /** * Analyzes a text format to find the filter not allowing required tags. * diff --git a/core/modules/ckeditor5/tests/src/Kernel/ValidatorsTest.php b/core/modules/ckeditor5/tests/src/Kernel/ValidatorsTest.php index 52a6c460a92..ce060a68a4d 100644 --- a/core/modules/ckeditor5/tests/src/Kernel/ValidatorsTest.php +++ b/core/modules/ckeditor5/tests/src/Kernel/ValidatorsTest.php @@ -462,33 +462,6 @@ class ValidatorsTest extends KernelTestBase { ], 'violations' => [], ]; - $data['INVALID: forbidden tags'] = [ - 'settings' => [ - 'toolbar' => [ - 'items' => [], - ], - 'plugins' => [], - ], - 'image_upload' => [ - 'status' => FALSE, - ], - 'filters' => [ - 'filter_test_restrict_tags_and_attributes' => [ - 'id' => 'filter_test_restrict_tags_and_attributes', - 'provider' => 'filter_test', - 'status' => TRUE, - 'weight' => 0, - 'settings' => [ - 'restrictions' => [ - 'forbidden_tags' => ['p' => FALSE], - ], - ], - ], - ], - 'violations' => [ - '' => 'CKEditor 5 needs at least the <p> and <br> tags to be allowed to be able to function. They are forbidden by the "Tag and attribute restricting filter" (filter_test_restrict_tags_and_attributes) filter.', - ], - ]; $restricted_html_format_filters = Yaml::parseFile(__DIR__ . '/../../../../../profiles/standard/config/install/filter.format.restricted_html.yml')['filters']; $data['INVALID: the default restricted_html text format'] = [ 'settings' => [ diff --git a/core/modules/editor/src/EditorXssFilter/Standard.php b/core/modules/editor/src/EditorXssFilter/Standard.php index 754a014ca62..24a27df44e9 100644 --- a/core/modules/editor/src/EditorXssFilter/Standard.php +++ b/core/modules/editor/src/EditorXssFilter/Standard.php @@ -42,9 +42,9 @@ class Standard extends Xss implements EditorXssFilterInterface { // , for example? Then we would strip that tag, even though it is // allowed, thereby causing data loss! // Therefore, we want to be smarter still. We want to take into account - // which HTML tags are allowed and forbidden by the text format we're - // filtering for, and if we're switching from another text format, we want - // to take that format's allowed and forbidden tags into account as well. + // which HTML tags are allowed by the text format we're filtering for, and + // if we're switching from another text format, we want to take that + // format's allowed tags into account as well. // In other words: we only expect markup allowed in both the original and // the new format to continue to exist. $format_restrictions = $format->getHtmlRestrictions(); @@ -52,17 +52,6 @@ class Standard extends Xss implements EditorXssFilterInterface { $original_format_restrictions = $original_format->getHtmlRestrictions(); } - // Any tags that are explicitly blacklisted by the text format must be - // appended to the list of default dangerous tags: if they're explicitly - // forbidden, then we must respect that configuration. - // When switching from another text format, we must use the union of - // forbidden tags: if either text format is more restrictive, then the - // safety expectations of *both* text formats apply. - $forbidden_tags = self::getForbiddenTags($format_restrictions); - if ($original_format !== NULL) { - $forbidden_tags = array_merge($forbidden_tags, self::getForbiddenTags($original_format_restrictions)); - } - // Any tags that are explicitly whitelisted by the text format must be // removed from the list of default dangerous tags: if they're explicitly // allowed, then we must respect that configuration. @@ -78,9 +67,6 @@ class Standard extends Xss implements EditorXssFilterInterface { // formats. $blacklisted_tags = array_diff($dangerous_tags, $allowed_tags); - // Also blacklist tags that are explicitly forbidden in either text format. - $blacklisted_tags = array_merge($blacklisted_tags, $forbidden_tags); - $output = static::filter($html, $blacklisted_tags); // Since data-attributes can contain encoded HTML markup that could be @@ -140,26 +126,6 @@ class Standard extends Xss implements EditorXssFilterInterface { return $allowed_tags; } - /** - * Get all forbidden tags from a restrictions data structure. - * - * @param array|false $restrictions - * Restrictions as returned by FilterInterface::getHTMLRestrictions(). - * - * @return array - * An array of forbidden HTML tags. - * - * @see \Drupal\filter\Plugin\Filter\FilterInterface::getHTMLRestrictions() - */ - protected static function getForbiddenTags($restrictions) { - if ($restrictions === FALSE || !isset($restrictions['forbidden_tags'])) { - return []; - } - else { - return $restrictions['forbidden_tags']; - } - } - /** * {@inheritdoc} */ diff --git a/core/modules/filter/src/Entity/FilterFormat.php b/core/modules/filter/src/Entity/FilterFormat.php index 697d9441002..cf03af72a79 100644 --- a/core/modules/filter/src/Entity/FilterFormat.php +++ b/core/modules/filter/src/Entity/FilterFormat.php @@ -305,16 +305,6 @@ class FilterFormat extends ConfigEntityBase implements FilterFormatInterface, En // with the existing set, to ensure we only end up with the tags that are // allowed by *all* filters with an "allowed html" setting. else { - // Track the union of forbidden tags. - if (isset($new_restrictions['forbidden_tags'])) { - if (!isset($restrictions['forbidden_tags'])) { - $restrictions['forbidden_tags'] = $new_restrictions['forbidden_tags']; - } - else { - $restrictions['forbidden_tags'] = array_unique(array_merge($restrictions['forbidden_tags'], $new_restrictions['forbidden_tags'])); - } - } - // Track the intersection of allowed tags. if (isset($restrictions['allowed'])) { $intersection = $restrictions['allowed']; @@ -372,32 +362,17 @@ class FilterFormat extends ConfigEntityBase implements FilterFormatInterface, En $restrictions['allowed'] = $intersection; } + // Simplification: if the only remaining allowed tag is the asterisk + // (which contains attribute restrictions that apply to all tags), + // then effectively nothing is allowed. + if (count($restrictions['allowed']) === 1 && array_key_exists('*', $restrictions['allowed'])) { + $restrictions['allowed'] = []; + } + return $restrictions; } }, NULL); - // Simplification: if we have both allowed (intersected) and forbidden - // (unioned) tags, then remove any allowed tags that are also forbidden. - // Once complete, the list of allowed tags expresses all tag-level - // restrictions, and the list of forbidden tags can be removed. - if (isset($restrictions['allowed']) && isset($restrictions['forbidden_tags'])) { - foreach ($restrictions['forbidden_tags'] as $tag) { - if (isset($restrictions['allowed'][$tag])) { - unset($restrictions['allowed'][$tag]); - } - } - unset($restrictions['forbidden_tags']); - } - - // Simplification: if the only remaining allowed tag is the asterisk - // (which contains attribute restrictions that apply to all tags), and - // there are no forbidden tags, then effectively nothing is allowed. - if (isset($restrictions['allowed'])) { - if (count($restrictions['allowed']) === 1 && array_key_exists('*', $restrictions['allowed']) && !isset($restrictions['forbidden_tags'])) { - $restrictions['allowed'] = []; - } - } - return $restrictions; } } diff --git a/core/modules/filter/src/FilterFormatInterface.php b/core/modules/filter/src/FilterFormatInterface.php index 707776b39af..62a704a6fcb 100644 --- a/core/modules/filter/src/FilterFormatInterface.php +++ b/core/modules/filter/src/FilterFormatInterface.php @@ -72,10 +72,8 @@ interface FilterFormatInterface extends ConfigEntityInterface { * * @return array|false * A structured array as returned by FilterInterface::getHTMLRestrictions(), - * but with the intersection of all filters in this text format. The - * restrictions will either forbid or allow a list of tags. In the latter - * case, it's possible that restrictions on attributes are also stored. - * FALSE means there are no HTML restrictions. + * but with the intersection of all filters in this text format. FALSE means + * there are no HTML restrictions. */ public function getHtmlRestrictions(); diff --git a/core/modules/filter/src/Plugin/FilterInterface.php b/core/modules/filter/src/Plugin/FilterInterface.php index e82a682a622..3b591251ce1 100644 --- a/core/modules/filter/src/Plugin/FilterInterface.php +++ b/core/modules/filter/src/Plugin/FilterInterface.php @@ -184,9 +184,9 @@ interface FilterInterface extends ConfigurableInterface, DependentPluginInterfac * format. * * @return array|false - * A nested array with *either* of the following keys: - * - 'allowed': (optional) the allowed tags as keys, and for each of those - * tags (keys) either of the following values: + * A nested array with the following structure: + * - 'allowed': the allowed tags as keys, and for each of those tags + * (keys) either of the following values: * - TRUE to indicate any attribute is allowed * - FALSE to indicate no attributes are allowed * - an array to convey attribute restrictions: the keys must be @@ -198,7 +198,6 @@ interface FilterInterface extends ConfigurableInterface, DependentPluginInterfac * be attribute values (which may use a wildcard, e.g. "xsd:*"), * the possible values are TRUE or FALSE: to mark the attribute * value as allowed or forbidden, respectively - * - 'forbidden_tags': (optional) the forbidden tags * * There is one special case: the "wildcard tag", "*": any attribute * restrictions on that pseudotag apply to all tags. @@ -243,13 +242,6 @@ interface FilterInterface extends ConfigurableInterface, DependentPluginInterfac * ) * @endcode * - * A simpler example, for a very coarse filter: - * @code - * array( - * 'forbidden_tags' => array('iframe', 'script') - * ) - * @endcode - * * The simplest example possible: a filter that doesn't allow any HTML: * @code * array( diff --git a/core/modules/filter/tests/filter_test/config/schema/filter_test.schema.yml b/core/modules/filter/tests/filter_test/config/schema/filter_test.schema.yml index 92e107f4e91..f54502c16df 100644 --- a/core/modules/filter/tests/filter_test/config/schema/filter_test.schema.yml +++ b/core/modules/filter/tests/filter_test/config/schema/filter_test.schema.yml @@ -14,9 +14,3 @@ filter_settings.filter_test_restrict_tags_and_attributes: sequence: type: ignore label: 'Tag and optionally list of attributes' - forbidden_tags: - type: sequence - label: 'Forbidden tags' - sequence: - type: boolean - label: 'Tag' diff --git a/core/modules/filter/tests/filter_test/src/Plugin/Filter/FilterTestRestrictTagsAndAttributes.php b/core/modules/filter/tests/filter_test/src/Plugin/Filter/FilterTestRestrictTagsAndAttributes.php index c6f62009cdc..13a9145110d 100644 --- a/core/modules/filter/tests/filter_test/src/Plugin/Filter/FilterTestRestrictTagsAndAttributes.php +++ b/core/modules/filter/tests/filter_test/src/Plugin/Filter/FilterTestRestrictTagsAndAttributes.php @@ -54,11 +54,6 @@ class FilterTestRestrictTagsAndAttributes extends FilterBase { } } } - if (isset($restrictions['forbidden_tags'])) { - foreach ($restrictions['forbidden_tags'] as $tag => $bool) { - $restrictions['forbidden_tags'][$tag] = (bool) $bool; - } - } return $restrictions; } diff --git a/core/modules/filter/tests/src/Kernel/FilterAPITest.php b/core/modules/filter/tests/src/Kernel/FilterAPITest.php index 34a2ed13f40..78a9977d1f8 100644 --- a/core/modules/filter/tests/src/Kernel/FilterAPITest.php +++ b/core/modules/filter/tests/src/Kernel/FilterAPITest.php @@ -137,8 +137,11 @@ class FilterAPITest extends EntityKernelTestBase { $stupid_filtered_html_format->save(); $this->assertSame( $stupid_filtered_html_format->getHtmlRestrictions(), - // No tag is allowed. - ['allowed' => []], + [ + 'allowed' => [ + '*' => ['style' => FALSE, 'on*' => FALSE, 'lang' => TRUE, 'dir' => ['ltr' => TRUE, 'rtl' => TRUE]], + ], + ], 'FilterFormatInterface::getHtmlRestrictions() works as expected for the stupid_filtered_html format.' ); $this->assertSame(