Opened 5 years ago

Closed 5 years ago

Last modified 5 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 5 years ago.
issue can no longer be reproduced

Download all attachments as: .zip

Change History (21)

comment:1 Changed 5 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 5 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 5 years ago by Piotrek Koszuliński

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

comment:4 Changed 5 years ago by Marek Lewandowski

Owner: set to Marek Lewandowski
Status: confirmedassigned

comment:5 Changed 5 years ago by Marek Lewandowski

Status: assignedreview

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.

Version 0, edited 5 years ago by Marek Lewandowski (next)

comment:7 Changed 5 years ago by Marek Lewandowski

Status: review_failedreview

extra clousure removed

comment:8 Changed 5 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 5 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 5 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 5 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 5 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 5 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 5 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 5 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 5 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 5 years ago by Marek Lewandowski

Attachment: strinsert.mp4 added

issue can no longer be reproduced

comment:17 Changed 5 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 5 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 5 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 5 years ago by spuymore

Done, see #11701, thanks.

Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy