Opened 7 years ago

Closed 7 years ago

#7513 closed Bug (fixed)

Invalid markup in STYLE make CK crash

Reported by: Dagfinn Bergsager Owned by: Frederico Caldeira Knabben
Priority: Normal Milestone: CKEditor 3.5.4
Component: Core : Parser Version: 3.1
Keywords: Cc:

Description

If i click the SOURCE-botton and insert <p style="color: red;>Red</p> (missing the last ") and then click SOURCE again to go back to normal edit-view; CK will go grey and stop working.

Attachments (2)

7513.patch (529 bytes) - added by Garry Yao 7 years ago.
7513_2.patch (562 bytes) - added by Frederico Caldeira Knabben 7 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 7 years ago by Jakub Ś

Status: newpending

Could you tell me which version of CKEditor are you using and in what browser does the issue occur?

I'm asking because I have checked all browsers and every time I've pasted your code, it was removed (was not parsed) and browser did not hang. CKEditor version I have used is the latest stable - 3.5.2.

Can you still reproduce the issue in latest demo version ? http://nightly.ckeditor.com/demo Perhaps this problem has already been fixed? If it has been fix than my advise is that you should upgrade to newer version. If not please provide me with more detailed information on how to reproduce this bug.

comment:2 Changed 7 years ago by Dagfinn Bergsager

I can reproduce it on http://nightly.ckeditor.com/6640/_samples/fullpage.html

And I pasted <p style="color: red;>Red</p> after <body>

Now I see that this only bug in Firefox4 and Chrome10 (tested on Win7 and Mac). IE and Opera change/remove the markup.

This might though be a bug in chrome and firefox4's edit components...

comment:3 Changed 7 years ago by Jakub Ś

Status: pendingconfirmed
Version: 3.1

If you paste the following code:

<p style="color: red;>Red</p>

before the sample text (E.g. in replaceByCode or fullPage samples) so that it looks like this:

<p style="color: red;>Red</p>
<p>This is some <strong>sample text</strong>. You are using <a href="http://ckeditor.com/">CKEditor</a>.</p>

Than Firefox4 and Chrome 10.0.648.204 will hang.

The error from fireBug is: "regular expression too complex while ( ( parts = this._.htmlPartsRegex.exec( html ) ) ) " htmlparser.js line:127

comment:4 Changed 7 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.5.4
Priority: NormalHigh

comment:5 Changed 7 years ago by Sa'ar Zac Elias

On other browsers:

<p ckeditor.com="" href="http://ckeditor.com/" http:="" style="color: red;">
	CKEditor.</p>

Still better than a crash.

Changed 7 years ago by Garry Yao

Attachment: 7513.patch added

comment:6 Changed 7 years ago by Garry Yao

Component: GeneralCore : Parser
Owner: set to Garry Yao
Priority: HighNormal
Status: confirmedreview

Propose for simplified html parser tokenized, reduce the complexity of attributes matcher (which consist of nested wildcards which are considered harmful for regexp engine), as the attributes parsing will be later handled by "attribsRegex".

comment:7 Changed 7 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

I was working to figure out why we have the attributes matching on the regex (there must be a reason). In fact, it is there to handle inputs like this:

<p title="a < b > c" class="test">Sample</p>

After applying the patch, the above HTML will not be handled properly on IE and Opera.

I have introduced a test for this case with [6742]:
http://ckeditor.t/dt/core/htmlparser/htmlparser.html

I've found a minor simplification to the regex that seems to work for this ticket. I'll ask for review on it.

Changed 7 years ago by Frederico Caldeira Knabben

Attachment: 7513_2.patch added

comment:8 Changed 7 years ago by Frederico Caldeira Knabben

Owner: changed from Garry Yao to Frederico Caldeira Knabben
Status: review_failedreview

comment:9 Changed 7 years ago by Garry Yao

Status: reviewreview_failed

While fixing your inputted TC, the patch will not parse the following source, degrade it as a single text node:

<span style=""">text</span>

comment:10 in reply to:  9 Changed 7 years ago by Frederico Caldeira Knabben

Status: review_failedreview

Replying to garry.yao:

While fixing your inputted TC, the patch will not parse the following source, degrade it as a single text node:

<span style=""">text</span>

This also happens with the current code. It's another edge case, which is not related to this ticket. At least no data loss happens here.

If you feel it's worth, please have a ticket for it. I would not waste my time on that though, without having some outsider confirming this as a valid issue.

comment:11 Changed 7 years ago by Garry Yao

Status: reviewreview_passed

It's just far more frequently occurred than your inputted TC, anyway R+ current, #7693 filed.

comment:12 Changed 7 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: review_passedclosed

Fixed with [6761].

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