Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#4719 closed Bug (fixed)

IE does not escape attribute values properly

Reported by: Dirk Owned by: Minh Nguyen
Priority: Normal Milestone: CKEditor 3.3
Component: General Version: 3.0.1
Keywords: Confirmed IE Review+ Cc:

Description

reproduce: (in WYSIWYG mode)

editorObj.setData('<a href="#" x="&lt;a b=&quot;c&quot;/&gt;">a</a>')

In IE you will see: " _cke_saved_href="#">a

source code:

<a href="http://localhost/t/backend/#" x="<a b=">&quot; _cke_saved_href=&quot;#&quot;&gt;a</a>

instead of: a

source code:

<a href="#" x="&lt;a b=&quot;c&quot;/&gt;">a</a>

Attachments (5)

4719.patch (780 bytes) - added by Minh Nguyen 8 years ago.
This bug happen in IE when we have quote character in value of attribute.
4719_2.patch (1.8 KB) - added by Minh Nguyen 8 years ago.
4719_3.patch (1.8 KB) - added by Minh Nguyen 8 years ago.
4719_4.patch (1.6 KB) - added by Minh Nguyen 7 years ago.
4719_5.patch (2.2 KB) - added by Minh Nguyen 7 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: Confirmed IE added
Milestone: CKEditor 3.2
Priority: HighNormal

Changed 8 years ago by Minh Nguyen

Attachment: 4719.patch added

This bug happen in IE when we have quote character in value of attribute.

comment:2 Changed 8 years ago by Minh Nguyen

Keywords: Review? added
Owner: set to Minh Nguyen
Status: newassigned

comment:3 Changed 8 years ago by Garry Yao

Keywords: Review- added; Review? removed

The "quote" is already fixed in #4683 and we should be looking to fix other entities here.

Changed 8 years ago by Minh Nguyen

Attachment: 4719_2.patch added

comment:4 Changed 8 years ago by Minh Nguyen

Keywords: Review? added; Review- removed

Changed 8 years ago by Minh Nguyen

Attachment: 4719_3.patch added

comment:5 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed
Milestone: CKEditor 3.2CKEditor 3.3

The patch works well, and I was about to give it R+. In the last minute, I've decided also testing the performance of it by loading a log document and checking the output generation performance. It's almost 40% slower after the patch.

We need also some performance optimization at this point. If we want to you CKEDITOR.tools.htmlEncode, we need to rewrite it, much probably avoiding having a function call chain there. Otherwise we can think about optimizing it in the writter functions directly.

I had some doubt about the real need of encoding other chars inside attributes, but this is definitely a need according to the XML specs, which is inherited into XHTML, but not in HTML.

As the breaking issue here has been already fixed with [5007], we don't have much urgency here, so I'm moving this ticket to the 3.3.

comment:6 Changed 8 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.3CKEditor 3.2

Ops, moving back to the 3.2 as this patch is also fixing another important issue: pasting is not working on some cases. For example, it's not possible to past this entire page (all browsers): http://www.w3.org/TR/html4/

comment:7 Changed 8 years ago by Garry Yao

  1. 4719_3.patch breaks test case dt/core/tools::test_htmlEncode2, specifically character quote should not be always escaped, it's only feasible when considering an attribute value context, the php htmlspecialchars will be a good API ref for it.


  1. The current implementation of CKEDITOR.tools.htmlEncode is over complicated, it's safely enough to just escape manually five characters there, by a RegExp, without bothering DOM.

comment:8 in reply to:  6 ; Changed 8 years ago by Garry Yao

Replying to fredck:

Ops, moving back to the 3.2 as this patch is also fixing another important issue...

The problem has been fixed with [5028] on #4683.

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

Replying to garry.yao:

  1. 4719_3.patch breaks test case dt/core/tools::test_htmlEncode2, specifically character quote should not be always escaped, it's only feasible when considering an attribute value context, the php htmlspecialchars will be a good API ref for it.

Well, not a big issue. It may avoid us having a dedicated htmlAttEncode function.

  1. The current implementation of CKEDITOR.tools.htmlEncode is over complicated, it's safely enough to just escape manually five characters there, by a RegExp, without bothering DOM.

The DOM trick is a nice attempt to make it work as fast as possible in all browsers. It's a nice solution, but we may change it so we avoid the functions chain.

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

Milestone: CKEditor 3.2CKEditor 3.3

Replying to garry.yao:

The problem has been fixed with [5028] on #4683.

At this point, let's get back to this one later.

Changed 7 years ago by Minh Nguyen

Attachment: 4719_4.patch added

comment:11 Changed 7 years ago by Minh Nguyen

Keywords: Review? added; Review- removed

Changed 7 years ago by Minh Nguyen

Attachment: 4719_5.patch added

comment:12 Changed 7 years ago by Garry Yao

Keywords: Review+ added; Review? removed

Please make the documentation of 'htmlEncodeAttr' better by emphasizing the escape happen in the context of a html attribute value.

comment:13 Changed 7 years ago by Minh Nguyen

Resolution: fixed
Status: assignedclosed

Fixed with [5271].

comment:14 Changed 7 years ago by Minh Nguyen

and [5272].

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