Ticket #4719 (closed Bug: fixed)

Opened 4 years ago

Last modified 4 years ago

IE does not escape attribute values properly

Reported by: dirk Owned by: m.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

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

Change History

comment:1 Changed 4 years ago by fredck

  • Priority changed from High to Normal
  • Keywords Confirmed IE added
  • Milestone set to CKEditor 3.2

Changed 4 years ago by m.nguyen

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

comment:2 Changed 4 years ago by m.nguyen

  • Status changed from new to assigned
  • Keywords Review? added
  • Owner set to m.nguyen

comment:3 Changed 4 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 4 years ago by m.nguyen

comment:4 Changed 4 years ago by m.nguyen

  • Keywords Review? added; Review- removed

Changed 4 years ago by m.nguyen

comment:5 Changed 4 years ago by fredck

  • Keywords Review- added; Review? removed
  • Milestone changed from CKEditor 3.2 to CKEditor 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 follow-up: ↓ 8 Changed 4 years ago by fredck

  • Milestone changed from CKEditor 3.3 to CKEditor 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 follow-up: ↓ 9 Changed 4 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 ; follow-up: ↓ 10 Changed 4 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 4 years ago by fredck

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

  • Milestone changed from CKEditor 3.2 to CKEditor 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 4 years ago by m.nguyen

comment:11 Changed 4 years ago by m.nguyen

  • Keywords Review? added; Review- removed

Changed 4 years ago by m.nguyen

comment:12 Changed 4 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 4 years ago by m.nguyen

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

Fixed with [5271].

comment:14 Changed 4 years ago by m.nguyen

and [5272].

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