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

  1. Evaluate whether the string can be refactored to one of the formats outlined in this change record: https://www.drupal.org/node/2311123
  2. 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.
  3. 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:

  1. Clean install of Drupal 8.
  2. Open up the ckeditor toolbar editor (admin side) from admin/config/content/formats/manage/basic_html (ex.g.)
  3. Compare the output above in HEAD and with the patch applied. Confirm that there is no double-escaping.
  4. 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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chrisfromredfin’s picture

chrisfromredfin’s picture

Issue summary: View changes

Pass the image alternative string through SafeMarkup::format() when it's built, rather than using concatenation and calling SafeMarkup::set() later, in CKEditor.admin.inc.

chrisfromredfin’s picture

Status: Active » Needs review

Go testbot, go!

joelpittet’s picture

Assigned: chrisfromredfin » Unassigned
Status: Needs review » Needs work
FileSize
128.13 KB

Something happened to some of the buttons on here.

chrisfromredfin’s picture

Assigned: Unassigned » chrisfromredfin
Status: Needs work » Needs review
FileSize
4.74 KB

Aha! 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).

joelpittet’s picture

Assigned: chrisfromredfin » Unassigned
Status: Needs review » Reviewed & tested by the community
FileSize
155.86 KB
158.29 KB

That 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

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

This looks like a good solution, and great catch with the manual testing.

+++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/Internal.php
@@ -127,7 +128,8 @@ public function getConfig(Editor $editor) {
-      return '<a href="#" class="cke-icon-only cke_' . $direction . '" role="button" title="' . $name . '" aria-label="' . $name . '"><span class="cke_button_icon cke_button__' . str_replace(' ', '', $name) . '_icon">' . $name . '</span></a>';
+      $clean_name = str_replace(' ', '', $name);
+      return SafeMarkup::format('<a href="#" class="cke-icon-only cke_@direction" role="button" title="@name" aria-label="@name"><span class="cke_button_icon cke_button__@cleanname_icon">@name</span></a>', ['@direction' => $direction, '@name' => $name, '@cleanname' => $clean_name]);

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!

chrisfromredfin’s picture

Still working on the tests, but I'm pitching this right now...

// In the markup below, we mostly use the name (which may include spaces),
// but in one spot we use it as a CSS class, so strip spaces.
$class_name = str_replace(' ', '', $name);
return SafeMarkup::format('<a href="#" class="cke-icon-only cke_@direction" role="button" title="@name" aria-label="@name"><span class="cke_button_icon cke_button__@classname_icon">@name</span></a>', ['@direction' => $direction, '@name' => $name, '@classname' => $class_name]);

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.

chrisfromredfin’s picture

Status: Needs work » Needs review
FileSize
7.58 KB

OK, 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.

joelpittet’s picture

@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.

joelpittet’s picture

+++ b/core/modules/ckeditor/src/Tests/CKEditorAdminTest.php
@@ -164,6 +164,19 @@ function testExistingFormat() {
+    $this->assertRaw('\u003Cli data-drupal-ckeditor-button-name=\u0022-\u0022 class=\u0022ckeditor-button-separator ckeditor-multiple-button\u0022 data-drupal-ckeditor-type=\u0022separator\u0022\u003E\u003Ca href=\u0022#\u0022 role=\u0022button\u0022 aria-label=\u0022Button separator\u0022 class=\u0022ckeditor-separator\u0022\u003E\u003C\/a\u003E\u003C\/li\u003E');

That 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?

chrisfromredfin’s picture

Status: Needs review » Needs work

Still on the fence about Html::getClass() - but agree about json_encode() so setting back to NW.

joelpittet’s picture

@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.

chrisfromredfin’s picture

Html::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.

aneek’s picture

@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!

joelpittet’s picture

@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.

aneek’s picture

@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!

chx’s picture

So where's this issue a month later :) ? Who needs to do what?

Wim Leers’s picture

aneek’s picture

alexpott’s picture

  1. +++ b/core/modules/ckeditor/ckeditor.admin.inc
    @@ -7,7 +7,6 @@
    @@ -66,10 +65,10 @@ function template_preprocess_ckeditor_settings_toolbar(&$variables) {
    
    @@ -66,10 +65,10 @@ function template_preprocess_ckeditor_settings_toolbar(&$variables) {
    +      $value = $button['image_alternative' . $rtl];
    ...
    +      $value = $button['image_alternative'];
         }
         elseif (isset($button['image'])) {
           $value = array(
    

    Given 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.

  2. +++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/Internal.php
    @@ -127,7 +128,10 @@ public function getConfig(Editor $editor) {
    -      return '<a href="#" class="cke-icon-only cke_' . $direction . '" role="button" title="' . $name . '" aria-label="' . $name . '"><span class="cke_button_icon cke_button__' . str_replace(' ', '', $name) . '_icon">' . $name . '</span></a>';
    +      // In the markup below, we mostly use the name (which may include spaces),
    +      // but in one spot we use it as a CSS class, so strip spaces.
    +      $class_name = str_replace(' ', '', $name);
    +      return SafeMarkup::format('<a href="#" class="cke-icon-only cke_@direction" role="button" title="@name" aria-label="@name"><span class="cke_button_icon cke_button__@classname_icon">@name</span></a>', ['@direction' => $direction, '@name' => $name, '@classname' => $class_name]);
    
    @@ -256,7 +260,7 @@ public function getButtons() {
    -        'image_alternative' => '<a href="#" role="button" aria-label="' . t('Format') . '"><span class="ckeditor-button-dropdown">' . t('Format') . '<span class="ckeditor-button-arrow"></span></span></a>',
    +        'image_alternative' => SafeMarkup::format('<a href="#" role="button" aria-label="@format_text"><span class="ckeditor-button-dropdown">@format_text<span class="ckeditor-button-arrow"></span></span></a>', ['@format_text' => t('Format')]),
    
    @@ -282,7 +286,7 @@ public function getButtons() {
    -        'image_alternative' => '<a href="#" role="button" aria-label="' . t('Button separator') . '" class="ckeditor-separator"></a>',
    +        'image_alternative' => SafeMarkup::format('<a href="#" role="button" aria-label="@button_separator_text" class="ckeditor-separator"></a>', ['@button_separator_text' => t('Button separator')]),
    
    +++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/StylesCombo.php
    @@ -59,7 +60,7 @@ public function getButtons() {
    -        'image_alternative' => '<a href="#" role="button" aria-label="' . t('Styles') . '"><span class="ckeditor-button-dropdown">' . t('Styles') . '<span class="ckeditor-button-arrow"></span></span></a>',
    +        'image_alternative' => SafeMarkup::format('<a href="#" role="button" aria-label="@styles_text"><span class="ckeditor-button-dropdown">@styles_text<span class="ckeditor-button-arrow"></span></span></a>', ['@styles_text' => t('Styles')]),
    

    Change these to render arrays rendering links.

joelpittet’s picture

+++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/StylesCombo.php
@@ -59,7 +60,7 @@ public function getButtons() {
-        'image_alternative' => '<a href="#" role="button" aria-label="' . t('Styles') . '"><span class="ckeditor-button-dropdown">' . t('Styles') . '<span class="ckeditor-button-arrow"></span></span></a>',
+        'image_alternative' => SafeMarkup::format('<a href="#" role="button" aria-label="@styles_text"><span class="ckeditor-button-dropdown">@styles_text<span class="ckeditor-button-arrow"></span></span></a>', ['@styles_text' => t('Styles')]),

Try making these render arrays...

alexpott’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
9.42 KB
7.53 KB

To 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.

Wim Leers’s picture

Besides 2 questions, this is RTBC. #23's interdiff looks amazing.

  1. +++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/Internal.php
    @@ -131,7 +130,15 @@
    -      return SafeMarkup::format('<a href="#" class="cke-icon-only cke_@direction" role="button" title="@name" aria-label="@name"><span class="cke_button_icon cke_button__@classname_icon">@name</span></a>', ['@direction' => $direction, '@name' => $name, '@classname' => $class_name]);
    +      return [
    +        '#type' => 'inline_template',
    +        '#template' => '<a href="#" class="cke-icon-only cke_{{ direction }}" role="button" title="{{ name }}" aria-label="{{ name }}"><span class="cke_button_icon cke_button__{{ classname }}_icon">{{ name }}</span></a>',
    +        '#context' => [
    +          'direction' => $direction,
    +          'name' => $name,
    +          'classname' => $class_name,
    +        ],
    +      ];
    

    <3

  2. +++ b/core/modules/ckeditor/src/Tests/CKEditorAdminTest.php
    @@ -176,6 +176,19 @@ function testExistingFormat() {
    +    $this->verbose($this->content);
    

    Do we want to keep this when committing?

  3. +++ b/core/modules/ckeditor/src/Tests/CKEditorAdminTest.php
    @@ -176,6 +176,19 @@ function testExistingFormat() {
    +    $this->assertRaw('\u003Cli data-drupal-ckeditor-button-name=\u0022-\u0022 class=\u0022ckeditor-button-separator ckeditor-multiple-button\u0022 data-drupal-ckeditor-type=\u0022separator\u0022\u003E\u003Ca href=\u0022#\u0022 role=\u0022button\u0022 aria-label=\u0022Button separator\u0022 class=\u0022ckeditor-separator\u0022\u003E\u003C\/a\u003E\u003C\/li\u003E');
    

    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()…)

alexpott’s picture

FileSize
4.48 KB

Here's a less amazing interdiff.

alexpott’s picture

Addressed #24.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Much more legible, thanks!

catch’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/Internal.php
@@ -127,7 +127,18 @@ public function getConfig(Editor $editor) {
+      $class_name = str_replace(' ', '', $name);

Why str_replace() and not Html::cleanCssIdentifier()? If there's a good reason, it could use a comment.

Otherwise looks good.

alexpott’s picture

@catch that's not changed by this issue. I think it is out-of-scope. Shall I file a followup?

alexpott’s picture

Wim Leers’s picture

Indeed. Thanks, Alex. Patch posted on the other issue.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Fair enough.

Committed/pushed to 8.0.x, thanks!

  • catch committed db37224 on 8.0.x
    Issue #2501711 by cwells, alexpott: Remove or document SafeMarkup::set...

Status: Fixed » Closed (fixed)

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