Opened 11 years ago

Closed 11 years ago

#3441 closed Bug (fixed)

CKEDITOR changes script tag content

Reported by: Artur Formella Owned by: Frederico Caldeira Knabben
Priority: Normal Milestone: CKEditor 3.0
Component: Core : Output Data Version:
Keywords: Confirmed Review+ Cc:

Description

Paste in source mode

<p>
	<script>alert('sample text. You are using <a href="http://www.fckeditor.net/">FCKeditor</a>');</script></p>

switch to WYSIWYG and back to Source. Result:

<p>
	<script>alert(&#39;sample text. You are using </script><a href="http://www.fckeditor.net/"><script>FCKeditor</script></a><script>&#39;);</script></p>

expected result: nothing changed

Attachments (7)

3441.patch (428 bytes) - added by Artur Formella 11 years ago.
3441_2.patch (2.0 KB) - added by Garry Yao 11 years ago.
3441_3.patch (2.7 KB) - added by Garry Yao 11 years ago.
3441_4.patch (5.7 KB) - added by Garry Yao 11 years ago.
3441_5.patch (5.0 KB) - added by Garry Yao 11 years ago.
3441_6.patch (6.9 KB) - added by Garry Yao 11 years ago.
3441_7.patch (6.2 KB) - added by Frederico Caldeira Knabben 11 years ago.

Download all attachments as: .zip

Change History (30)

Changed 11 years ago by Artur Formella

Attachment: 3441.patch added

comment:1 Changed 11 years ago by Artur Formella

Keywords: Review? added

comment:2 Changed 11 years ago by Garry Yao

Keywords: Review- added; Review? removed
Owner: set to Garry Yao

The patch doesn't work, I'm attaching a new patch here.

Changed 11 years ago by Garry Yao

Attachment: 3441_2.patch added

comment:3 Changed 11 years ago by Garry Yao

Keywords: Review? added; Review- removed

comment:4 Changed 11 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

Instead of having changes to the current event calls, it would be much nicer if we could have the parser throwing the "onCData" event, which would work for the contents of <script> and <style> elements.

Changed 11 years ago by Garry Yao

Attachment: 3441_3.patch added

comment:5 Changed 11 years ago by Garry Yao

Keywords: Review? added; Review- removed

comment:6 Changed 11 years ago by Garry Yao

Keywords: Confirmed added
Status: newassigned

comment:7 Changed 11 years ago by Artur Formella

What was wrong?

comment:8 in reply to:  7 Changed 11 years ago by Artur Formella

Replying to arczi:

What was wrong?

Ok, sorry. Now I know.

comment:9 Changed 11 years ago by Artur Formella

Keywords: Review- added; Review? removed

Bug is somewhere else. Source code:

<p>
	<script>alert('sample text. You are using <a href="http://www.fckeditor.net/">FCKeditor</a>');</script></p>

is replaced with

<p>
	<script><![CDATA[alert('sample text. You are using ]]></script><a href="http://www.fckeditor.net/"><script><![CDATA[FCKeditor]]></script></a><script><![CDATA[');]]></script></p>

It means that editor "think" that <a href ...> in a HTML tag instead of text inside <script>.

Actual behavior:

<p>
	<script>--------------TEXT----------------</script><a href="http://www.fckeditor.net/"><script>---------</script></a><script>---</script></p>

Expected behavior:

<p>
	<script>--------------------------------TEXT--------------------------------------------------</script></p>

comment:10 in reply to:  9 Changed 11 years ago by Artur Formella

Replying to arczi:

It means that editor "think" that <a href ...> in a HTML tag instead of text inside <script>.

It means that editor "think" that <a href ...> is a HTML tag instead of text inside <script>.

Changed 11 years ago by Garry Yao

Attachment: 3441_4.patch added

comment:11 Changed 11 years ago by Garry Yao

Keywords: Review? added; Review- removed

Nice catch Artur! while the ideal fix would be some changes to the current html parser regexp to ignore those tags within pseudo CDATA section, inside 'script' and 'style', but...the expression would be quite hard to construct without touching the current lines of the parser logics. But don't hesitate to have a try :)

Matching any html tag parts which is not inside either <script> or <style>

comment:12 Changed 11 years ago by Garry Yao

Another doable approach would be two times parsing:

  1. first time extracting all pseudo CDATA section content and get stored;
  2. second time perform the normal parsing and compensate the CDATA section into desired tag.

comment:13 Changed 11 years ago by Artur Formella

Garry, could you look at #3407? It can solve this bug.

comment:14 Changed 11 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

The approach is wrong here. The parser should not treat CDATA as a special kind of text node. It's a different node at all, so we have element, text, comment and CDATA. Te parser fires onCDATA with the raw CDATA value. The writer will simply write the CDATA with no special manipulations to the data, and no CDATA[[ ]] is to be added to the output.

comment:15 in reply to:  13 Changed 11 years ago by Garry Yao

Replying to arczi:

Garry, could you look at #3407? It can solve this bug.

Thanks for the tip here, I've discussed with Fred on this few days ago, we found the CDATA parsing is still necessary since the '<style>' tag could be protected as '<script>' we will have in #3047, so the fix should be some how different than that.

comment:16 in reply to:  14 ; Changed 11 years ago by Garry Yao

Replying to fredck:

The approach is wrong here. The parser should not treat CDATA as a special kind of text node.

Please correct me: Since CDATA is only a special kind of text node from w3c spec:

  1. DOM Level1 said so
  2. check node type of the only child of script element, it's shown as text.

Do we have real benefit of creating a 'cdata' type node presentation?

The writer will simply write the CDATA with no special manipulations to the data, and no CDATA ? is to be added to the output.

I can see rational of this.

Changed 11 years ago by Garry Yao

Attachment: 3441_5.patch added

comment:17 Changed 11 years ago by Garry Yao

Keywords: Review? added; Review- removed

comment:18 in reply to:  16 ; Changed 11 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

Replying to garry.yao:

Please correct me: Since CDATA is only a special kind of text node from w3c spec:

No, CDATA is not a special kind of text node. It's programmatic interface simply inherits the text interface, but they are different. They are parsed and rendered differently.

Do we have real benefit of creating a 'cdata' type node presentation?

In any case, we are not trying to reproduce the W3C definition here. We need to fit it to our needs, were text nodes needs special processing (like entities replacement), while CDATA is written as is.


In any case, I've noticed that the patch is still trying to propose using the "text" class to represent the CDATA. We must instead have a dedicated class for CDATA under CKEDITOR.htmlParser.

I wanted to take the ticket ownership to come with the final solution, but I've tried applying this patch and it's not working. Can you please update your local copy an provide a new patch.

Changed 11 years ago by Garry Yao

Attachment: 3441_6.patch added

comment:19 in reply to:  18 Changed 11 years ago by Garry Yao

Keywords: Review? added; Review- removed

Replying to fredck: I've provided an updated patch accordingly, the patch works, and you may take it over if you're intending to change the way CDATA is parsed within html parser.

comment:20 Changed 11 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed
Owner: changed from Garry Yao to Frederico Caldeira Knabben
Status: assignednew

I'll be attaching a new patch with some minor cleanup and modifications.

Changed 11 years ago by Frederico Caldeira Knabben

Attachment: 3441_7.patch added

comment:21 Changed 11 years ago by Frederico Caldeira Knabben

Keywords: Review? added; Review- removed

comment:22 Changed 11 years ago by Garry Yao

Keywords: Review+ added; Review? removed

comment:23 Changed 11 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: newclosed

Fixed with [3496].

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