Richcombo breaks when given double quotes

[Marcus Bointon]

I'm using the ​strinsert plugin to inject HTML snippets into my editor. It works fine for plain-text content, but fails for HTML (and some other markup) content because it doesn't escape it. I traced this to the add function of the richcombo element:

add: function(a, b, c) {
    this._.items[a] = c || a;
    this._.list.add(a, b, c)
},

The problem here is that a can contain a string which breaks the HTML. Here's a minimal plugin that demonstrates the problem:

CKEDITOR.plugins.add('mytags',
{
    requires : ['richcombo'],
    init : function( editor )
    {
        editor.ui.addRichCombo('mytags',
        {
            label: 'My tags',
            title: 'My tags',
            voiceLabel: 'My tags',
            className: 'cke_format',
            multiSelect:false,
            panel:
            {
                css: [ editor.config.contentsCss, CKEDITOR.skin.getPath('editor') ],
                voiceLabel: editor.lang.panelVoiceLabel
            },
        
            init: function()
            {
                this.add('<span class="test">test</span>', 'Test', 'Test');
                this.add('"test2"', 'Test2', 'Test2');
                this.add('test3', 'Test3', 'Test3');
           },
            onClick: function( value )
            {
                editor.focus();
                editor.fire( 'saveSnapshot' );
                editor.insertText(value);
                editor.fire( 'saveSnapshot' );
            }
        });
    }
});

The onClick handler can be ignored as the problem arises before it is used; whether you use insertText or insertHTML makes no difference.

The first item in the menu contains

test')" onclick="CKEDITOR.tools.callFunction(215,'…

instead of the expected Test, and selecting the malformed menu item this creates results in a Syntax error: Unexpected EOF on the console. The second item appears correctly in the menu, but clicking it results in the same syntax error. It appears to be the double quotes that are causing it problems; Removing the double quotes works fine, as demonstrated by the third item.

I can't think of any circumstance where you would want this behaviour; there needs to be some form of escaping in the add function to prevent this from happening. This may also apply to other components which add items in a similar way.

Since this breaks out of escaping, it might also represent an opportunity for XSS injection.

[Marcus Bointon]

I've done some more experiments: it still breaks if I encode the quotes. Adding these examples to the above plugin:

this.add('<span class=\x22test\x22>test</span>', 'Test4', 'Test4'); //JS encoded chars
this.add('<span class=&quot;test&quot;>test</span>', 'Test5', 'Test5'); //HTML entities
this.add('<span class=&amp;quot;test&amp;quot;>test</span>', 'Test6', 'Test6'); //Double-encoded HTML entities

These all give this error: TypeError: 'null' is not an object (evaluating 'this.element.getDocument().getById(this._.items[a]).hasClass').

[Piotrek Koszuliński]

The value passed to add method is not encoded before being used in this template:

' {onclick}="CKEDITOR.tools.callFunction({clickFn},\'{val}\'); return false;"'

Therefore both characters - ' and "" will break that HTML. It is of course a bug.

Simple workaround for that is using only alphanumeric characters as values and then grab the final value from hash:

editor.insertText( textsHash[ value ] );

where textsHash is e.g.:

{ foo: '<p>...</p>', bar: '<h1>...</h1>' }

[Marek Lewandowski]

pushed to ​t/11101 ddev and t/11101 tests

I was wondering if escapeSingleQuotes should make its way into CKEDITOR.tools, but i didn't moved it there yet - the best moment will be if it will be needed in other place.

[Piotrek Koszuliński]

​https://github.com/cksource/ckeditor-dev/commit/11b68d7be5da66ce5b72932e8adb2793b747695a#diff-45e7e3f6eb9e3ea54e6e802757d9a6c1R21

That closure is unnecessary.

[Marek Lewandowski]

extra clousure removed

[Piotrek Koszuliński]

I pushed additional commits to tests. Please squash all dev and all tests commits to single ones before merging branches to master.

[Marek Lewandowski]

fixed with ​git:6aca6feebb at dev, merged to master in tests

[spuymore]

I don't get how this fixes the issue? It seems to fix something that has nothing to do with richcombo. The actual issue still exists, please reopen!

Replying to m.lewandowski:

fixed with ​git:6aca6feebb at dev, merged to master in tests

[Piotrek Koszuliński]

Richcombo uses listblock and the issue was in the listblock plugin.

Do you have a TC which still does not work?

[spuymore]

Yes, the actual issue describes strinsert that uses RichCombo as well. As soon as you provide e.g. a piece of HTML you get the javascript errors as described in comment:1.

[spuymore]

By the way, I assumed the fix is available in CKeditor 4.3.3, is it? If not, then I guess I should wait for next version?

[Marek Lewandowski]

You're correct to assume that version 4.3.3 does not have this issue.

We've added html to both value, and label in conjuction with strinsert and none of them caused any exception.

	strings.push(['<b style="font-style: italic;">foo</b>', 'fancy bold', 'fancy bold']);
	strings.push(['lorem ipsum', 'fancy <i>label</i>', 'label']);

Please, provide a code listing which is causing the issue.

[spuymore]

Install CKeditor with the strinsert addon from ​https://github.com/57u/custom-dropdown-ckeditor4/commit/32bb75a5fc5129722ddd32637e8639be2735cbb5 (​http://ckeditor.com/addon/strinsert download is not updated), configure for an editor with e.g.:

config.extraPlugins = 'strinsert';
config.strinsert_strings = [{'name': 'test', 'value': '<b>some html</b>'}];

and select test from the Insert dropdown in the editor toolbar and you'll get the JavaScript errors mentioned in comment:1.

[spuymore]

I currently have to work around the issue by setting values URL encoded like:

config.strinsert_strings = [{'name': 'test', 'value': encodeURIComponent('<b>some html</b>')}];

and decoding them in the strinsert plugin.js as follows on line 101:

editor.insertHtml(decodeURIComponent(value));

[Marek Lewandowski]

issue can no longer be reproduced

[Marek Lewandowski]

Issues can not be reproduced in 4.3.3 - we downloaded given plugin, changed config in the way as in your comment.

Please see ​attached video. Make sure that your copy uses CKEditor at version 4.3.3.

[spuymore]

Hello, you're right, sorry about that, thought it wouldn't, here's an example that will fail really:

config.strinsert_strings = [{'name': 'test', 'value': 'html entities like <, &gt and & wont work'}];

May be it's more down to HTML entities?

[Marek Lewandowski]

No problem, but in that case, please create a new issue with the description of the issue and code/steps how to reproduce it.

[spuymore]

Done, see #11701, thanks.