Opened 10 years ago

Closed 10 years ago

#3407 closed Bug (fixed)

scripts aren't protected while loading content

Reported by: Alfonso Martínez de Lizarrondo Owned by: Frederico Caldeira Knabben
Priority: Normal Milestone: CKEditor 3.0
Component: General Version: SVN (CKEditor) - OLD
Keywords: Confirmed Review+ 3.0RC Cc:

Description

Load this in source mode:

<script>alert(0)</script>

when you switch to design view the alert will show, so any script will be executed instead of being protected.

Attachments (7)

3407.patch (12.1 KB) - added by Artur Formella 10 years ago.
3407_2.patch (12.4 KB) - added by Artur Formella 10 years ago.
code.gif (80 bytes) - added by Artur Formella 10 years ago.
example image. Copy to _source/plugins/protectedsource/images
3407_3.patch (9.5 KB) - added by Artur Formella 10 years ago.
3407_4.patch (4.7 KB) - added by Artur Formella 10 years ago.
3407_5.patch (4.3 KB) - added by Frederico Caldeira Knabben 10 years ago.
3407_TC.patch (2.7 KB) - added by Garry Yao 10 years ago.

Download all attachments as: .zip

Change History (29)

comment:1 Changed 10 years ago by Artur Formella

Owner: set to Artur Formella
Status: newassigned

comment:2 Changed 10 years ago by Artur Formella

Keywords: Discussion added

It could be a new feature based on fakeElement:

Plugin to protect custom elements and provide ability to edit content and attibutes in dialog.

Something like that:

protectedElements:[
{
	tagName : "script",			// since 3.0
	className : "cke_script",	// since 3.0
	editable : false			// default in 3.0
},
{
	tagName : "mytag",
	className : "cke_mytag",
	editable : true,			// since 3.x
	check : function( element )	// since 3.x
	{
		return ( element.getAttribute( "yes" ) == "no" );
	}
},
{
	tagName : "gallery",		//for mediawiki
	className : "cke_mv_gallery",
	editable : true
}
]

comment:3 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Confirmed added; Discussion removed

Yes arczi we could have a feature for it, but this should not be our focus right now. At this moment we must implement the protection system, just like we have in V2.

comment:4 Changed 10 years ago by Alfonso Martínez de Lizarrondo

As Fredck said, the focus should be at least to reproduce the V2 behaviour: I've mentioned only scripts as they are easy to test, but remember that php code, asp... must be also preserved before the browsers parses the html and tries to convert it into DOM nodes breaking it.

Changed 10 years ago by Artur Formella

Attachment: 3407.patch added

comment:5 Changed 10 years ago by Artur Formella

Keywords: Review? added

comment:6 Changed 10 years ago by Artur Formella

Keywords: Review- added; Review? removed

JS error in the following case:

<!--
<?xml-stylesheet href="http://www.w3.org/StyleSheets/Core/Steely" type="text/css"?>
<?xml-stylesheet href="http://www.w3.org/StyleSheets/Core/Chocolate" type="text/css"?>
<?xml-stylesheet href="http://www.w3.org/StyleSheets/Core/Midnight" type="text/css"?>
<?xml-stylesheet href="http://www.w3.org/StyleSheets/Core/Modernist" type="text/css"?>
-->

Changed 10 years ago by Artur Formella

Attachment: 3407_2.patch added

Changed 10 years ago by Artur Formella

Attachment: code.gif added

example image. Copy to _source/plugins/protectedsource/images

comment:7 Changed 10 years ago by Artur Formella

Keywords: Review? added; Review- removed

Idea of this patch: Protect content using regexp before anything and convert it into comments or fake objects.

Usage:

{
	regExp : /regexp/g,
	editable : true/false	//use fakeElement / protectedElement, default false
	name : "mycode",	// used by fakeElement to show in elementpath

	replaceWith : myfunc // custom replace function
	className : "myclass"	// used by fakeElement, default "cke_code"
},

and explanation:

comment.js:  because filter.onComment can return object (not only the comment content) it should use CKEDITOR.htmlParser.fragment(). 

element.js: filter.onElement() can return text node (i.e. content of the fakeElement) so writer.text( element.value ) should be used.

filter.js: New private array: regExp : [].

text.js: new attribute in constructor (protect), default null. If true: don't use  value.replace( spacesRegex, ' ' ) and filter.onText( text )


fakeobjects/plugin.js: 
	In CKEDITOR.editor.prototype.createFakeElement():
		Added new attribute (protectContent), default null. Used in htmlFilterRules.
		typeof realElement can be a string

		Now we can protect element not only in wysiwyg mode:
		var document = this.document || CKEDITOR.document;

	CKEDITOR.editor.prototype.createFakeParserElement
		Added new attibute (protectContent), default null. Added for the compatibility with createFakeElement().

	elements : $ : function( element ):

		if _cke_protect then don't do anything - just return.


htmlDataProcessor/plugin.js: 
	Added protectSource(). It is called before anything to protect HTML using filter._.regExp

Tests (works of course):

<p>
	<script>bhfty<strong>gybf
6t ;()jio)(*&^
%$#sample text

</script> <script  language="JavaScript" a=b><!--bhfty<strong>gybf
6t ;()jio)(*&^
%$#sample text
//-->
</script><style><script  language="JavaScript"><!--bhfty<strong>gybf
6t ;()jio)(*&^
%$#sample text
//-->
</script></style><?<style><script><!--bhfty<strong>gybf
6t ;()jio)(*&^
%$#sample text
//-->
</script></style>?><?
<style><script><!--bhfty<strong>gybf
6t ;()jio)(*&^</script>
%$#sample text
//-->
</style>
?></p>

Known issues:

-Due to method:

<script>
alert("<script><\script>")
</script>

is parsed wrong. Workaround:

<script language="JavaScript">
<!--
alert("<script></script>")
//-->
</script>
or
<script>
alert("<script><\/script>")
</script>

-"code.gif" could be more thoughtful :)

comment:8 in reply to:  7 Changed 10 years ago by Alfonso Martínez de Lizarrondo

Replying to arczi:

Known issues:

-Due to method:

<script>
alert("<script><\script>")
</script>

is parsed wrong. Workaround:

I guess that you mean alert("<script></script>"), but putting that inside a script tag is invalid html, the html parser will close the script at that point (and the javascript parser will say that it's broken) and try to parse the ")</script> as another html block.

Changed 10 years ago by Artur Formella

Attachment: 3407_3.patch added

comment:9 Changed 10 years ago by Artur Formella

New patch in attachment.

Changes:

-simplified implementation and config

-"editable" parameter was removed

-changes from #3441 are used.

comment:10 Changed 10 years ago by Garry Yao

The patch works in the expected way, while it's arguable to the html filter for protected other languages for the following reason:

  1. The pre-processing of other embeded XML processor instructions, like '<?php>' should not handled by html filter which is responsible for (X)HTML only.The new adding 'regExp' rule for this purpose might get confused with the other rules.
  2. It's more reasonable if the restoring of these instruction tags could be defer until the (X)HTML has completed and act as some sort of post-processing with encoded contents inside the comments.
  3. Perhaps we should make distinguish between <script>, <style> and instruction tags, <script> is protected by default, <style> not been protected(for good of customized inline style), and <%php%> been protected depend on configuration.

comment:11 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

The problem here, and the thing to keep in mind, is that the protect source feature must leave the original source "perfectly intact". So, we should not use the parser for it... for input processing at least.

This feature is supposed to be much simple. It should be implemented almost entirely in the htmlprocessor plugin:

  • On input, the original data is processed, simply running the protection replacement regexes, inserting comments in place of protected text (just like V2).
  • On output, a filter for comments would replace them with the relative original string (CDATA).

It should look like V2 in many ways, which is actually a quite simple feature. We don't need a dedicated plugin or too much code for that.

Also, just like stated Garry, some protection is to be done by default, like comments and scripts (just like V2). Others, come from configuration.

The "element" protection idea is interesting, but it would have much few usage. We need it for <script> actually, but we're already doing that internally in the code, so we can come with the proper regex for that.

Also, I don't see the point of having a name for each config entry. So, the config should look like this:

CKEDITOR.config.protectedSource =
    [
        /<%[\s\S]*?%>/g,    // ASP Code
        /(<asp:[^\>]+>[\s|\S]*?<\/asp:[^\>]+>)|(<asp:[^\>]+\/>)/gi,    // ASP.Net Tags
        /<\?[\s\S]*?\?>/g   // PHP Code
    ];

To summarize, we should keep it simple here, not missing the experience we have made with V2.

comment:12 Changed 10 years ago by Artur Formella

Ok.

We had problems in v2 with the implementation of protected source. In drupal and mediawiki we need protect new tags ( source, gallery, ref, reference, imagemap,nowiki,... ), templates:

{{}}

Protected source from v2 was simply useless in most cases. My idea was to provide by the way a feature to handle add those requirements. Simple way to add GALLERY tag (maybe in future):

{
	name : 'gallery'	// for elementpatch  and alt for fakeobject
	regExp : /<gallery[^>]*>(.|\n|\r)*?<\/gallery[^>]*>/g 
	className : "cke_mv_gallery",
	editable : true,  // use fakeobject
	editby : "somedialog",
}

We need to protect comments because of

<!-- <? ?> -->

Changed 10 years ago by Artur Formella

Attachment: 3407_4.patch added

comment:13 Changed 10 years ago by Artur Formella

Keywords: Review? added; Review- removed

comment:14 Changed 10 years ago by Artur Formella

Owner: Artur Formella deleted
Status: assignednew

#3407 has been marked as DUP.

comment:15 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed
Owner: set to Frederico Caldeira Knabben
Status: newassigned

Ok... let's solve things in the proper order, one by one. Otherwise we risk having too many unrelated features to be verified that are just blocking the simplest one, which would fix the issue.

I'm coming with a new patch that will be strictly related to this issue.

Changed 10 years ago by Frederico Caldeira Knabben

Attachment: 3407_5.patch added

comment:16 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review? added; Review- removed

comment:17 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: 3.0RC added

comment:18 Changed 10 years ago by Garry Yao

Keywords: Review- added; Review? removed

The patch is generally good, but I believe we've in some trouble with this case in IE:

<script>alert('')</script>
<p>content</p>

After the <script> has been protected into comments, it's get stripped on after we call IE to help fixing it.See the attached TC for detail.

Changed 10 years ago by Garry Yao

Attachment: 3407_TC.patch added

comment:19 Changed 10 years ago by Martin Kou

A similar approach to #3710 should be useful here: before calling IE to fix the HTML via innerHTML, the <script> tag can be converted to a temporary tag containing the original tag's HTML in URL encoded format. After IE has fixed the HTML, revert the transformation.

comment:20 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review? added; Review- removed

This is not a problem with <script> only. It would break any kind of code that gets protected.

This is an old and known issue with IE. If we have a comment at the beginning of the document, it get lost (it happens today without the patch). We have a workaround for it in V2. We need a new ticket for it, but this issue should not block this ticket.

comment:21 Changed 10 years ago by Garry Yao

Keywords: Review+ added; Review? removed

Please include the TC before submitting if you feel NP with it, a dedicate ticket regarding the 'head comment lost' issue will be opened ASA this ticket get fixed.

comment:22 Changed 10 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: assignedclosed

Fixed with [3735]. The TC is failing on IE due to #3801.

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