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
Comment | File | Size | Author |
---|---|---|---|
#43 | core-3268983-43-10.x.patch | 4.62 KB | nod_ |
| |||
#42 | core-3268983-42.patch | 4.73 KB | nod_ |
#42 | interdiff-40-42.txt | 869 bytes | nod_ |
#40 | core-3268983-40.patch | 3.75 KB | nod_ |
#40 | interdiff-36-40.txt | 1.35 KB | nod_ |
Issue fork drupal-3268983
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:
Comments
Comment #3
larowlanhttps://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
Comment #5
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedFailing test committed to the MR.
Comment #6
nod_Just ran into that one too
Comment #7
Wim LeersComment #8
plach-
Comment #9
plachRerolled.
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.
Comment #10
vistree CreditAttribution: vistree commentedPatch 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!
Comment #12
plachComment #13
mizage@gmail.com CreditAttribution: mizage@gmail.com commented#9 works for me.
Comment #14
jcontreras CreditAttribution: jcontreras at Texas Creative commented#9 worked for me. =)
Comment #15
Wim LeersComment #16
Wim LeersComment #17
Wim LeersI'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.Comment #18
iSoLate CreditAttribution: iSoLate at Randstad Digital commentedProblem is that your php version does not know what to do with the code.
Comment #19
iSoLate CreditAttribution: iSoLate at Randstad Digital commentedWrong patch.
Comment #20
iSoLate CreditAttribution: iSoLate at Randstad Digital commentedagain wrong...
Comment #22
dieuweI 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.Comment #23
Wim Leers#22 Agreed! Will get started on this today.
Comment #24
Wim LeersUgh, had to divert attention to #3291744: Ensure Editor config entities using CKEditor 4 only store plugins settings for actually enabled plugins instead today. Will tackle tomorrow.
Comment #25
Wim LeersAnalysis
#3228334: Refactor HTMLRestrictionsUtilities to a HtmlRestrictions value object made a change to
FilterHtml
: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 *>
forFilterHtml
! 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:🙈
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:
\Drupal\Tests\ckeditor5\Unit\HTMLRestrictionsTest
test coverage to not rely on<foo *>
\Drupal\Tests\ckeditor5\Unit\HTMLRestrictionsTest
to ensure we still have the same amount of test coverageComment #26
Wim LeersSelf-review for context:
This tweaks the existing test coverage — this was only testing a convenience constructor anyway!
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.
This is the revert.
Comment #27
anup.singh CreditAttribution: anup.singh as a volunteer and for Faichi Solutions Pvt Ltd commentedPatch #25 works for me.
Comment #28
capysara CreditAttribution: capysara at Bounteous commentedIs 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.
Comment #29
lauriiiThe patch needs a reroll because it doesn't apply anymore.
Comment #30
anup.singh CreditAttribution: anup.singh as a volunteer and for Faichi Solutions Pvt Ltd commentedYup, Not getting applied anymore.
But also not able to replicate issue anymore without patch.
Comment #31
scott_euser CreditAttribution: scott_euser at Soapbox Communications Ltd commentedI 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?
Comment #32
xjmComment #33
nod_there is already a test for all attributes,
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.Comment #35
nod_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.
Comment #36
nod_Comment #37
nod_this update fix the error happening in the UI when switching from CKE4 config to CKE5 config when there is a
<a *>
config.Comment #38
catchThere 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?
Comment #39
nod_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)Comment #40
nod_umm I guess we can copy paste tests they're fast so shouldn't matter much to test twice
Comment #41
catchBut isn't this the bug? And shouldn't it be testable in filter module?
Comment #42
nod_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: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 :)
Comment #43
nod_Comment #46
catchOK 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...
Comment #47
lauriiiI'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.
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.Comment #48
lauriiiJust saw that there were test failures on #33 from
\Drupal\Tests\ckeditor5\Unit\SmartDefaultSettingsTest
. Researching that.Comment #49
lauriiiI 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.Comment #54
lauriiiCommitted #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.
Comment #55
lauriiiSaving credits because they were lost for some reason.