Ticket #3869 (closed Bug: fixed)

Opened 5 years ago

Last modified 4 years ago

Script with comment

Reported by: arczi Owned by: arczi
Priority: Normal Milestone: CKEditor 3.0
Component: Core : Output Data Version:
Keywords: Review+ Cc:

Description

Paste the following content in source mode, switch to WYSIWYG and back to source.

	<script language="Javascript" type="text/javascript">
	<!--
	var factive_color = '#F9F9F0';
	var faonmouse_color = '#DEE3E7';
	var faonmouse2_color = '#EFEFEF';
	var cname = 'bb038dfef17';

	//-->
	</script>

Result:

<script language="Javascript" type="text/javascript">
	<!--{cke_protected}%3C!%2D%2D%0A%09var%20factive_color%20%3D%20'%23F9F9F0'%3B%0A%09var%20faonmouse_color%20%3D%20'%23DEE3E7'%3B%0A%09var%20faonmouse2_color%20%3D%20'%23EFEFEF'%3B%0A%09var%20cname%20%3D%20'bb038dfef17'%3B%0A%0A%09%09%2F%2F%2D%2D%3E-->
	</script>

Expected result: No change

Attachments

3869.patch (3.1 KB) - added by arczi 5 years ago.
3869_2.patch (4.2 KB) - added by arczi 5 years ago.
3869_3.patch (4.8 KB) - added by arczi 5 years ago.

Change History

comment:1 Changed 5 years ago by arczi

  • Status changed from new to assigned
  • Owner set to arczi

Changed 5 years ago by arczi

comment:2 Changed 5 years ago by arczi

  • Keywords Review? added

comment:3 follow-up: ↓ 4 Changed 5 years ago by garry.yao

  • Keywords Review- added; Review? removed

You're always sensitive to various ways of comments ;), maybe the implementation could be further simplified by eliminate the encoding of comments inside script tag at first place instead of restore it later.

var regexes =
	[
		// Script tags will also be forced to be protected, otherwise
		// IE will execute them.
		/<script[\s\S]*?<\/script>/gi,

		// <noscript> tags (get lost in IE and messed up in FF).
		/<noscript[\s\S]*?<\/noscript>/gi,

		// We must protect all comments
		// to avoid loosing them (of course, IE related).
		/<!--(:?!{cke_protected})[\s\S]*?-->/g,

	]

comment:4 in reply to: ↑ 3 Changed 5 years ago by arczi

Replying to garry.yao:

You're always sensitive to various ways of comments ;), maybe the implementation could be further simplified by eliminate the encoding of comments inside script tag at first place instead of restore it later.

No :) This bug is not only with comments but also with all protectedSource.

Changing order in regexes would fix only few cases.

More buggy content:

<?
echo 'aaa<script>bbb</script>bbbb';
?>
<!-- <? echo $code ?> -->
<? echo "<noscript>aaa</noscript>" ?>

and any combination of protectedSource inside of other protectedSource.

This content works now, but won't work with your regexes

<!--
	<script language="Javascript" type="text/javascript">
	var factive_color = '#F9F9F0';
</script>//-->

Changed 5 years ago by arczi

comment:5 Changed 5 years ago by arczi

  • Keywords Review? added; Review- removed

comment:6 Changed 5 years ago by garry.yao

  • Keywords Review- added; Review? removed

I see rational behind this now, yes, double checking the replaced string is inevitable at least for now, I still see two optimzie points:

  1. Instead of encode and decode twice, we can defer the encode after the marking is done, which means the first item we try to mark all pairs pending protected, second time we catch all of them and perform the encoding;
  2. We may further turning the sequence of the regexps list order by appearing frequency in desc;

comment:7 Changed 5 years ago by arczi

  1. It would increase complexity because we would need to scan twice to protect everything, event with depth=1.

How to mark it?
I think protection is a good mark because we don't need to scan twice :)

We don't even know if it is in pairs, i.e (depth=3, order in config: comments, script, php):

aaa
<?
echo 'bbb<script>
<!-- 
ccc();
//--></script>
ddd';
?>
eee
  1. How to learn the correct order?

The same problem was in V2 and was fixed in similar way but at restoring. In V3 we have different parser and it is more elegant to do it during protection.

Changed 5 years ago by arczi

comment:8 Changed 5 years ago by arczi

  • Keywords Review? added; Review- removed

After talk with Garry I've made a new patch. Usage of encode/decodeURIComponent was reduced.

comment:9 Changed 5 years ago by garry.yao

  • Keywords Review+ added; Review? removed

comment:10 Changed 5 years ago by arczi

  • Status changed from assigned to closed
  • Resolution set to fixed

Fixed with [3844]

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