Problem/Motivation

If an asterisk is added to an allowed html tag in the filter_html configuration an error is thrown due to code added in #3228334: Refactor HTMLRestrictionsUtilities to a HtmlRestrictions value object

See https://www.drupal.org/project/drupal/issues/3228334#comment-14440797 for more information.

Steps to reproduce

Install standard profile
Go to /admin/config/content/formats/manage/basic_html and add * to the list of allowed attributes on one of the tags (e.g <img * src...>
Save the format
Try to edit the format again

Error thrown:

Error: Unsupported operand types in Drupal\filter\Plugin\Filter\FilterHtml->filterAttributes() (line 122 of core/modules/filter/src/Plugin/Filter/FilterHtml.php). 

Proposed resolution

Remaining tasks

CommentFileSizeAuthor
#43 core-3268983-43-10.x.patch4.62 KBnod_
#42 core-3268983-42.patch4.73 KBnod_
#42 interdiff-40-42.txt869 bytesnod_
#40 core-3268983-40.patch3.75 KBnod_
#40 interdiff-36-40.txt1.35 KBnod_
#37 interdiff-35-36.txt820 bytesnod_
#37 core-3268983-36.patch2.25 KBnod_
#35 core-3268983-35.patch1.59 KBnod_
#35 interdiff-33-35.txt2.96 KBnod_
#33 interdiff-31-33.txt3.68 KBnod_
#33 core-3268983-33.patch3.49 KBnod_
#31 3268983-31.patch2.78 KBscott_euser
#25 3268983-25.patch3.8 KBWim Leers
#20 drupal-filter_html_fix-3268983-20.patch897 bytesiSoLate
#19 drupal-filter_html_fix-3268983-19.patch884 bytesiSoLate
#18 drupal-filter_html_fix-3268983-18.patch864 bytesiSoLate
#9 drupal-filter_html_fix-3268983-9.patch854 bytesplach
#8 drupal-filter_html_fix-3268983-8.patch832 bytesplach

Issue fork drupal-3268983

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

acbramley created an issue. See original summary.

larowlan made their first commit to this issue’s fork.

larowlan’s picture

https://git.drupalcode.org/issue/drupal-3268983/-/blob/9.4.x/core/module... seems to suggest we had coverage for that case, clearly we don't though.

Its worth noting that this impacts sites not using Ckeditor5

acbramley’s picture

Status: Active » Needs review

Failing test committed to the MR.

nod_’s picture

Just ran into that one too

Wim Leers’s picture

plach’s picture

plach’s picture

Rerolled.

The attached patch can be used to mitigate the issue until a proper fix is merged. Do not review it, please, it is not supposed to be merged, work should continue in the MR.

vistree’s picture

Patch from #9 solves the problem on Drupal core 9.3.7 and 9.3.8. Before 9.3.7 I did not get the error!

Status: Needs review » Needs work

The last submitted patch, 9: drupal-filter_html_fix-3268983-9.patch, failed testing. View results

plach’s picture

mizage@gmail.com’s picture

#9 works for me.

jcontreras’s picture

#9 worked for me. =)

Wim Leers’s picture

Wim Leers’s picture

Title: FilterHtml throws Unsupported operand types error when * used in tag attribute » [regression] FilterHtml throws Unsupported operand types error when * used in tag attribute
Wim Leers’s picture

Version: 9.4.x-dev » 9.3.x-dev
Assigned: Unassigned » Wim Leers

I'll start pushing this forward in the next few days, since I am responsible for regressing this in #3228334: Refactor HTMLRestrictionsUtilities to a HtmlRestrictions value object 😬

The reason this was possible: because filter_html has almost no test coverage. Understandable, given that it dates back to the early Drupal days!

And because this is a regression, moving this to 9.3.x, because it's affecting real-world sites out there.

iSoLate’s picture

Problem is that your php version does not know what to do with the code.

iSoLate’s picture

Wrong patch.

iSoLate’s picture

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

dieuwe’s picture

Version: 9.4.x-dev » 9.3.x-dev

I think this needs to be set back to 9.3.x as the regression still appears for me in an upgrade to 9.3.16.

Patch from #9 still applies as a workaround.

The problem has nothing to do with PHP version so #18-20 are not relevant, $tag_attributes is still going to end up as TRUE in certain situations causing the illegal array + boolean operand types error.

Wim Leers’s picture

#22 Agreed! Will get started on this today.

Wim Leers’s picture

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
3.8 KB

Analysis

#3228334: Refactor HTMLRestrictionsUtilities to a HtmlRestrictions value object made a change to FilterHtml:

        // This tag has a notation like "<foo *>", to indicate all attributes
        // are allowed.
        if ($node->hasAttribute($star_protector)) {
          $restrictions['allowed'][$tag] = TRUE;
          continue;
        }

This introduced the crashed mentioned above, but only for configurations of the HTML filter that never were actually supported: those that allow all attributes on a particular tag.

We needed this for certain test cases … but that's not really a good enough reason. Besides, there was a very good reason to not allow <foo *> for FilterHtml! Quoting myself from #2549077-78: Allow the "Limit allowed HTML tags" filter to also restrict HTML attributes, and only allow a small whitelist of attributes by default from nearly 7 years ago:

Is there any way to wildcard all attributes of a tag?

No, that'd largely defeat the purpose: it's likely that most sites would then end up allowing all attributes on some or all tags. That'd then make this entire patch pointless.

🙈

And clearly, some sites were trying to implement this very bad idea! (Of course, the validation logic didn't inform the user that this was not supported, but at least it did not actually work. Yes, validation logic for config forms in Drupal core and providing helpful messages is saddeningly absent from Drupal core 😔 — but we need not fix that here.)

Conclusion

We should revert the one change that #3228334: Refactor HTMLRestrictionsUtilities to a HtmlRestrictions value object made to FilterHtml, to fully undo this regression with no risky side effects such as suddenly allowing more attributes to make it to the end user than have been the case until now. (That risk has never occurred, because this exception happens specifically because \Drupal\filter\Plugin\Filter\FilterHtml::filterAttributes() does not support $tag_attributes === TRUE! 👍)

So this patch:

  1. undoes #3228334: Refactor HTMLRestrictionsUtilities to a HtmlRestrictions value object
  2. tweaks the \Drupal\Tests\ckeditor5\Unit\HTMLRestrictionsTest test coverage to not rely on <foo *>
  3. but also updates \Drupal\Tests\ckeditor5\Unit\HTMLRestrictionsTest to ensure we still have the same amount of test coverage
Wim Leers’s picture

Self-review for context:

  1. +++ b/core/modules/ckeditor5/tests/src/Unit/HTMLRestrictionsTest.php
    @@ -214,9 +214,9 @@ public function providerConvenienceConstructors(): \Generator {
    -    yield 'tag with wildcard attribute' => [
    +    yield 'tag with wildcard attribute: no string representation possible, because strongly discouraged' => [
           '<a *>',
    -      ['a' => TRUE],
    +      ['a' => ['*' => TRUE]],
         ];
    

    This tweaks the existing test coverage — this was only testing a convenience constructor anyway!

  2. +++ b/core/modules/ckeditor5/tests/src/Unit/HTMLRestrictionsTest.php
    @@ -374,23 +374,6 @@ public function providerConvenienceConstructors(): \Generator {
    -    yield '$block + one concrete tag to resolve into that already allows all attributes: concrete more permissive than wildcard' => [
    -      '<p *> <$block class="text-align-left text-align-center text-align-right text-align-justify">',
    -      [
    -        'p' => TRUE,
    -      ],
    -      [
    -        'p' => TRUE,
    -        '$block' => [
    -          'class' => [
    -            'text-align-left' => TRUE,
    -            'text-align-center' => TRUE,
    -            'text-align-right' => TRUE,
    -            'text-align-justify' => TRUE,
    -          ],
    -        ],
    -      ],
    -    ];
     
         // @todo Test `data-*` attribute: https://www.drupal.org/project/drupal/issues/3260853
       }
    @@ -809,6 +792,20 @@ public function providerOperands(): \Generator {
    
    @@ -809,6 +792,20 @@ public function providerOperands(): \Generator {
           'intersection' => new HTMLRestrictions(['$block' => ['class' => TRUE], 'p' => ['class' => TRUE]]),
           'union' => 'b',
         ];
    +    yield 'wildcard + matching tag: wildcard resolves into matching tag, but matching tag already supports all attributes' => [
    +      'a' => new HTMLRestrictions(['p' => TRUE]),
    +      'b' => new HTMLRestrictions(['$block' => ['class' => ['foo' => TRUE, 'bar' => TRUE]]]),
    +      'diff' => 'a',
    +      'intersection' => HTMLRestrictions::emptySet(),
    +      'union' => new HTMLRestrictions(['p' => TRUE, '$block' => ['class' => ['foo' => TRUE, 'bar' => TRUE]]]),
    +    ];
    +    yield 'wildcard + matching tag: wildcard resolves into matching tag, but matching tag already supports all attributes — vice versa' => [
    +      'a' => new HTMLRestrictions(['$block' => ['class' => ['foo' => TRUE, 'bar' => TRUE]]]),
    +      'b' => new HTMLRestrictions(['p' => TRUE]),
    +      'diff' => 'a',
    +      'intersection' => HTMLRestrictions::emptySet(),
    +      'union' => new HTMLRestrictions(['p' => TRUE, '$block' => ['class' => ['foo' => TRUE, 'bar' => TRUE]]]),
    +    ];
     
    

    This moves the test coverage out of the convenience constructor test coverage and into an explicit test for the merging behavior.

    You can see that the merging behavior is exactly the same.

  3. +++ b/core/modules/filter/src/Plugin/Filter/FilterHtml.php
    @@ -267,13 +267,6 @@ public function getHTMLRestrictions() {
    -        // This tag has a notation like "<foo *>", to indicate all attributes
    -        // are allowed.
    -        if ($node->hasAttribute($star_protector)) {
    -          $restrictions['allowed'][$tag] = TRUE;
    -          continue;
    -        }
    -
    

    This is the revert.

anup.singh’s picture

Patch #25 works for me.

capysara’s picture

Is the patch in #25 ready for testing? CI says it Patch Failed to Apply. Also, I tried (and failed) to apply it to 9.3.18-dev, 9.4.2-dev, and 9.5.0-dev before I noticed that.

lauriii’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

The patch needs a reroll because it doesn't apply anymore.

anup.singh’s picture

Yup, Not getting applied anymore.
But also not able to replicate issue anymore without patch.

scott_euser’s picture

I managed to replicate the issue in another site, here's a reroll. I otherwise didn't change anything else, but I think still at needs work right?

xjm’s picture

Issue tags: +Drupal 10 beta blocker
nod_’s picture

Status: Needs work » Needs review
FileSize
3.49 KB
3.68 KB

there is already a test for all attributes,

    yield 'INVALID: keys valid, values invalid attribute restrictions due to broad wildcard instead of prefix/infix/suffix wildcard attribute name' => [
      ['foo' => ['*' => TRUE]],
      'The "foo" HTML tag has an attribute restriction "*". This implies all attributes are allowed. Remove the attribute restriction instead, or use a prefix (`*-foo`), infix (`*-foo-*`) or suffix (`foo-*`) wildcard restriction instead.',
    ];

so removing the change to the <a> test case.

Moved the new tests in a more appropriate place, instead of in the middle of the <a> tests.

Status: Needs review » Needs work

The last submitted patch, 33: core-3268983-33.patch, failed testing. View results

nod_’s picture

Status: Needs work » Needs review
FileSize
2.96 KB
1.59 KB

Alternate fix, instead of messing with the tests I fixed the method that transforms a string into a HtmlRestriction object expected by CKE5 code.

This fixes the tests but when trying to change the editor in the text format to CKE5, there is a PHP error and seems like nothing happens for the user because the ajax request crashed.

nod_’s picture

Issue tags: -Needs reroll
nod_’s picture

this update fix the error happening in the UI when switching from CKE4 config to CKE5 config when there is a <a *> config.

catch’s picture

Version: 9.3.x-dev » 9.4.x-dev
Status: Needs review » Needs work
Issue tags: +Needs tests

There was test coverage in previous versions of the patch, but now none - looks like the steps to reproduce in the issue summary should still be a valid test case?

nod_’s picture

The test coverage exists already, what wim introduced was in his words "This moves the test coverage out of the convenience constructor test coverage and into an explicit test for the merging behavior."

The changes to the test were to account for the fact that before the patch <a *> was transformed directly to ['a' => TRUE] and after the patch what CKE5 got was ['a' => ['*' => TRUE]] instead. But this wouldn't have worked since there is a explicit check in HtmlRestrictions to disallow this kind of configuration (see test failures in #33)

nod_’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
1.35 KB
3.75 KB

umm I guess we can copy paste tests they're fast so shouldn't matter much to test twice

catch’s picture

+++ b/core/modules/filter/src/Plugin/Filter/FilterHtml.php
@@ -267,13 +267,6 @@ public function getHTMLRestrictions() {
       $tag = $node->tagName;
       if ($node->hasAttributes()) {
-        // This tag has a notation like "<foo *>", to indicate all attributes
-        // are allowed.
-        if ($node->hasAttribute($star_protector)) {
-          $restrictions['allowed'][$tag] = TRUE;
-          continue;
-        }
-

But isn't this the bug? And shouldn't it be testable in filter module?

nod_’s picture

FilterHtml is tests are pretty lacking, so we're using CKE5 as proxy to test it kind of. That piece of code was introduced in #3228334: Refactor HTMLRestrictionsUtilities to a HtmlRestrictions value object and that issue changed the result of FilterHtml::getHTMLRestrictions for a config of <img *> in the "Limit allowed HTML tags and correct faulty HTML" textarea in the following way:

// Before issue #3228334
[
  "allowed" => [
    "img" => ["*" => TRUE]
  ]
]

// After issue #3228334
[
  "allowed" => [
    "img" => TRUE
  ]
]

This patch is putting back the initial behavior and makes the adjustments in the CKE5 modules directly instead of changing that code for everyone. So it makes sense to test in CKE5 that <code *> is evaluated as ["code" => TRUE].

Added a change to the FilterHtml test to make sure we don't "eat" that * attribute anymore.

Sorry I realize I'm kind of rambling saying the same stuff again, I'm sure you got it the first time, was making it explicit for me to make sure I thought through your request :)

nod_’s picture

  • catch committed 2115c9b on 9.4.x
    Issue #3268983 by nod_, iSoLate, plach, Wim Leers, acbramley, larowlan,...
  • catch committed afc9c24 on 9.5.x
    Issue #3268983 by nod_, iSoLate, plach, Wim Leers, acbramley, larowlan,...

  • catch committed 3fa258d on 9.4.x
    Revert "Issue #3268983 by nod_, iSoLate, plach, Wim Leers, acbramley,...
  • catch committed c719149 on 9.5.x
    Revert "Issue #3268983 by nod_, iSoLate, plach, Wim Leers, acbramley,...
catch’s picture

Status: Needs review » Reviewed & tested by the community

OK that makes sense, just better to have testing at the lowest level possible when we can.

Started committing this one, then realised it was still CNR, so just moving to RTBC...

lauriii’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/ckeditor5/tests/src/Unit/HTMLRestrictionsTest.php
    @@ -816,6 +816,20 @@ public function providerOperands(): \Generator {
    +    yield 'wildcard + matching tag: wildcard resolves into matching tag, but matching tag already supports all attributes' => [
    

    I'm not sure these are strictly required for this patch. They are simply additional test coverage for something that is already working. If we decide to keep these here, I think these should be moved to the "Wildcard tag + matching tag cases." group.

  2. +++ b/core/modules/ckeditor5/src/HTMLRestrictions.php
    @@ -340,6 +340,14 @@ private static function fromObjectWithHtmlRestrictions(object $object): HTMLRest
    +    // When allowing all tags on an attribute, transform FilterHtml output from
    +    // ['tag' => ['*'=> TRUE]] to ['tag' => TRUE]
    +    foreach ($allowed as $element => $attributes) {
    +      if (is_array($attributes) && isset($attributes['*']) && $attributes['*'] === TRUE) {
    +        $allowed[$element] = TRUE;
    +      }
    +    }
    
    @@ -390,6 +398,14 @@ public static function fromString(string $elements_string): HTMLRestrictions {
    +    // When allowing all tags on an attribute, transform FilterHtml output from
    +    // ['tag' => ['*'=> TRUE]] to ['tag' => TRUE]
    +    foreach ($allowed_elements as $element => $attributes) {
    +      if (is_array($attributes) && isset($attributes['*']) && $attributes['*'] === TRUE) {
    +        $allowed_elements[$element] = TRUE;
    +      }
    +    }
    

    Why do we need these changes? We don't have a failing test case that would prove these are needed. In fact, we have test coverage in \Drupal\Tests\ckeditor5\Unit\HTMLRestrictionsTest::testConvenienceConstructors which I believe proves that these are unnecessary.

lauriii’s picture

Status: Needs work » Needs review

Just saw that there were test failures on #33 from \Drupal\Tests\ckeditor5\Unit\SmartDefaultSettingsTest. Researching that.

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

I ran \Drupal\Tests\ckeditor5\Unit\HTMLRestrictionsTest::testConvenienceConstructors again locally and it's also failing on the expected test cases with the partial fix. That addresses #47.2. I'll address #47.1 on commit.

  • lauriii committed c78d1a8 on 10.1.x
    Issue #3268983 by nod_, iSoLate, plach, Wim Leers, acbramley, larowlan,...

  • lauriii committed 5aecb66 on 10.0.x
    Issue #3268983 by nod_, iSoLate, plach, Wim Leers, acbramley, larowlan,...

  • lauriii committed 6750f9f on 9.5.x
    Issue #3268983 by nod_, iSoLate, plach, Wim Leers, acbramley, larowlan,...

  • lauriii committed 9a0ab60 on 9.4.x
    Issue #3268983 by nod_, iSoLate, plach, Wim Leers, acbramley, larowlan,...
lauriii’s picture

Status: Reviewed & tested by the community » Fixed

Committed #43 c78d1a8 and pushed to 10.1.x and cherry-picked to 10.0.x. Committed #42 to 9.5.x and cherry-picked to 9.4.x. Thanks!

Opened #3295935: Follow-up to #3268983: Move test case to correct group to address #47.1.

lauriii’s picture

Saving credits because they were lost for some reason.

  • alexpott committed 07a1601 on 10.1.x
    Issue #3295935 by lauriii: Follow-up to #3268983: Move test case to...

  • alexpott committed 1a8b586 on 10.0.x
    Issue #3295935 by lauriii: Follow-up to #3268983: Move test case to...

  • alexpott committed 3242627 on 9.5.x
    Issue #3295935 by lauriii: Follow-up to #3268983: Move test case to...

  • alexpott committed 172f53b on 9.4.x
    Issue #3295935 by lauriii: Follow-up to #3268983: Move test case to...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.