Opened 5 years ago

Closed 5 years ago

#7513 closed Bug (fixed)

Invalid markup in STYLE make CK crash

Reported by: bergsager Owned by: fredck
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 5 years ago.
7513_2.patch (562 bytes) - added by fredck 5 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 5 years ago by j.swiderski

  • Status changed from new to pending

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 5 years ago by 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 5 years ago by j.swiderski

  • Status changed from pending to confirmed
  • Version set to 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 5 years ago by fredck

  • Milestone set to CKEditor 3.5.4
  • Priority changed from Normal to High

comment:5 Changed 5 years ago by Saare

On other browsers:

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

Still better than a crash.

Changed 5 years ago by garry.yao

comment:6 Changed 5 years ago by garry.yao

  • Component changed from General to Core : Parser
  • Owner set to garry.yao
  • Priority changed from High to Normal
  • Status changed from confirmed to review

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 5 years ago by fredck

  • Status changed from review to review_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 5 years ago by fredck

comment:8 Changed 5 years ago by fredck

  • Owner changed from garry.yao to fredck
  • Status changed from review_failed to review

comment:9 follow-up: Changed 5 years ago by garry.yao

  • Status changed from review to review_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 5 years ago by fredck

  • Status changed from review_failed to review

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 5 years ago by garry.yao

  • Status changed from review to review_passed

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

comment:12 Changed 5 years ago by fredck

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

Fixed with [6761].

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