Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#11101 closed Bug (fixed)

Richcombo breaks when given double quotes

Reported by: Marcus Bointon Owned by: Marek Lewandowski
Priority: Normal Milestone: CKEditor 4.3.1
Component: General Version:
Keywords: Cc:

Description

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.

Attachments (1)

strinsert.mp4 (472.0 KB) - added by Marek Lewandowski 10 years ago.
issue can no longer be reproduced

Download all attachments as: .zip

Change History (21)

comment:1 Changed 10 years ago by 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').

comment:2 Changed 10 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.3.1
Status: newconfirmed
Version: 4.2.3 (GitHub - master)

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>' }

comment:3 Changed 10 years ago by Piotrek Koszuliński

Summary: richcombo breaks when given double quotesRichcombo breaks when given double quotes

comment:4 Changed 10 years ago by Marek Lewandowski

Owner: set to Marek Lewandowski
Status: confirmedassigned

comment:5 Changed 10 years ago by Marek Lewandowski

Status: assignedreview

pushed to t/11101 dev 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.

Last edited 10 years ago by Marek Lewandowski (previous) (diff)

comment:7 Changed 10 years ago by Marek Lewandowski

Status: review_failedreview

extra clousure removed

comment:8 Changed 10 years ago by Piotrek Koszuliński

Status: reviewreview_passed

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

comment:9 Changed 10 years ago by Marek Lewandowski

Resolution: fixed
Status: review_passedclosed

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

comment:10 in reply to:  9 Changed 10 years ago by 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

comment:11 Changed 10 years ago by Piotrek Koszuliński

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

Do you have a TC which still does not work?

comment:12 Changed 10 years ago by 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.

comment:13 Changed 10 years ago by 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?

comment:14 Changed 10 years ago by 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.

comment:15 Changed 10 years ago by 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.

comment:16 Changed 10 years ago by 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));

Changed 10 years ago by Marek Lewandowski

Attachment: strinsert.mp4 added

issue can no longer be reproduced

comment:17 Changed 10 years ago by 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.

comment:18 Changed 10 years ago by 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 &lt;, &gt and &amp; wont work'}];

May be it's more down to HTML entities?

comment:19 Changed 10 years ago by 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.

comment:20 Changed 10 years ago by spuymore

Done, see #11701, thanks.

Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy