Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
template_preprocess_ckeditor_settings_toolbar calls SafeMarkup::set() which is meant to be for internal use only.
Proposed resolution
- Remove the call by refactoring the code.
If refactoring is not possible, thoroughly document where the string is coming from and why it is safe, and why SafeMarkup::set() is required.
Remaining tasks
Evaluate whether the string can be refactored to one of the formats outlined in this change record: https://www.drupal.org/node/2311123- Identify whether there is existing automated test coverage for the sanitization of the string. If there is, list the test in the issue summary. If there isn't, add an automated test for it.
If the string cannot be refactored, the SafeMarkup::set() usage needs to be thoroughly audited and documented.
Manual testing steps (for XSS and double escaping)
Do these steps both with HEAD and with the patch applied:
- Clean install of Drupal 8.
- Open up the ckeditor toolbar editor (admin side) from admin/config/content/formats/manage/basic_html (ex.g.)
- Compare the output above in HEAD and with the patch applied. Confirm that there is no double-escaping.
If there is any user or calling code input in the string, submit
alert('XSS');and ensure that it is sanitized.
User interface changes
N/A
API changes
N/A
Comment | File | Size | Author |
---|---|---|---|
#26 | 2501711.26.patch | 7.37 KB | alexpott |
#26 | 23-26-interdiff.txt | 3.76 KB | alexpott |
#25 | 9-23-interdiff.txt | 4.48 KB | alexpott |
#23 | 2501711.23.patch | 7.53 KB | alexpott |
#23 | 9-23-interdiff.txt | 9.42 KB | alexpott |
Comments
Comment #1
chrisfromredfinComment #2
chrisfromredfinPass the image alternative string through SafeMarkup::format() when it's built, rather than using concatenation and calling SafeMarkup::set() later, in CKEditor.admin.inc.
Comment #3
chrisfromredfinGo testbot, go!
Comment #4
joelpittetSomething happened to some of the buttons on here.
Comment #5
chrisfromredfinAha! No clue how I missed seeing that, but those three are given directly and not passed through that anonymous function. Had to adjust them on their own, one was in another file (Styles).
Comment #6
joelpittetThat did it thank you! and I double checked the files to see if there were any left and you got them all @cwells!
Before
After
Comment #7
xjmThis looks like a good solution, and great catch with the manual testing.
So this is minor, but can we rename the variable from
$clean_name
to$machine_name
or something and add an inline comment above its declaration? I had to squint at this for several minutes before I figured out what it was for (composing a CSS class name).Also, based on the fact that we introduced a regression and no tests failed, I think we potentially need to add test coverage here. Thanks!
Comment #8
chrisfromredfinStill working on the tests, but I'm pitching this right now...
With that said, is there an equivalent D8 method to drupal_html_class(), and if so, should we use that? Not sure if it's worth the function call overhead, but on the other hand, it lets people more flexibly change the name (ex.g. adding capitalization or something for the 'image alternative').
Let me know what you think about both of the above.
Comment #9
chrisfromredfinOK, I'm pretty new to writing tests, and because the CKEditor is so JS dependent, I couldn't think of any other way than to just expect raw JS in drupalSettings. If there are any other / better ideas for this test, I'm all ears.
Comment #10
joelpittet@cwells was thinking the same thing. The equivalent is Html::getClass()
Here's the change record https://www.drupal.org/node/2388737
It may do more than just remove the space and I'm not sure how important the class names are at this point. But good question regardless.
Comment #11
joelpittetThat is cool that passes. Not an expert at tests either possible improvement suggestion:
I see a bunch of json_encode calls in that test class, maybe would be better to write the real Markup and json_encode it so it's easier to read?
Comment #12
chrisfromredfinStill on the fence about Html::getClass() - but agree about json_encode() so setting back to NW.
Comment #13
joelpittet@cwells maybe just give it a try and see if there is any test coverage for
Html::getClass()
:)Or if the results are the same there will be no problem or if there are no CSS classes being used also no problem I think because it would be more consistency with other generated classes in core.
Comment #14
chrisfromredfinHtml::getClass is a bad idea. :) It breaks a lot of things, but most importantly, it converts spaces to dashes, and then the classnames don't match what's expected in core/assets/vendor/ckeditor/skins/*/*.css. Since that's a vendor library, it makes sense in this case to leave the code as it is there.
Still working on seeing if I can make Json::encode() work. Likely, except many many things about Simpletest are failing on my laptop environment, so will try on my Mac where things work better.
Comment #15
aneek CreditAttribution: aneek as a volunteer commented@joelpittet & @cwells, I think way back we had a similar issue. @joelpittet if you can remember of this #2306515: SafeMarkup in CKEditor toolbar configuration UI — fix by not generating markup in PHP but using Twig Macros.
Can we implement that one if that is not performance killer? Yes, I know #2306515: SafeMarkup in CKEditor toolbar configuration UI — fix by not generating markup in PHP but using Twig Macros has a fix that is quite big compared to this but at least we will have a proper macro implementation in Drupal if that one is used.
@cwells you can find some tests in #2306515: SafeMarkup in CKEditor toolbar configuration UI — fix by not generating markup in PHP but using Twig Macros to get the runtime generated toolbar JS settings.
So what do you guys think? If this one seems more convincing then we can close the previous one.
Thanks!
Comment #16
joelpittet@aneek oh yeah! Thanks for reminding me! So that one has tests, which is great. I wonder how much we can merge, I don't want to scope creep but the other issue handles much more the way we'd like to get it in the template.
@cwells have a peek at the work done there, it's a bit bigger scope but may be the direction we should head in.
Comment #17
aneek CreditAttribution: aneek as a volunteer commented@joelpittet, also remember the CKEditor had an issue with rtl image rendering in the toolbar. We solved that one in the previous issue I mentioned. So that has to be merged if this patch doesn't solve that.
Thanks!
Comment #18
chx CreditAttribution: chx commentedSo where's this issue a month later :) ? Who needs to do what?
Comment #19
Wim LeersComment #20
aneek CreditAttribution: aneek as a volunteer commentedAny updates on this one? Should we adapt this one or the other #2306515: SafeMarkup in CKEditor toolbar configuration UI — fix by not generating markup in PHP but using Twig Macros?
Comment #21
alexpottGiven that $value can be a render array, it is in the elseif below, I think we might be able to construct the links as render arrays instead of strings in the plugins.
Change these to render arrays rendering links.
Comment #22
joelpittetTry making these render arrays...
Comment #23
alexpottTo me this is a perfect usecase for inline templates - a themer should never play with this markup as it is completely dependent on what ckeditor expects. The tests added by @cwells shows that this is behaving as expected.
Comment #24
Wim LeersBesides 2 questions, this is RTBC. #23's interdiff looks amazing.
<3
Do we want to keep this when committing?
Ideally we'd just write normal HTML and let PHP escape this. But … I don't think we can get PHP to create these Unicode escape sequences? (Except using
str_replace()
…)Comment #25
alexpottHere's a less amazing interdiff.
Comment #26
alexpottAddressed #24.
Comment #27
Wim LeersMuch more legible, thanks!
Comment #28
catchWhy str_replace() and not Html::cleanCssIdentifier()? If there's a good reason, it could use a comment.
Otherwise looks good.
Comment #29
alexpott@catch that's not changed by this issue. I think it is out-of-scope. Shall I file a followup?
Comment #30
alexpottCreated #2548723: Comment on or replace class name cleaning in CKEditorPlugin/Internal::getButtons()
Comment #31
Wim LeersIndeed. Thanks, Alex. Patch posted on the other issue.
Comment #32
catchFair enough.
Committed/pushed to 8.0.x, thanks!