Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#4351 closed Bug (fixed)

Dashes cannot be used in attribute names

Reported by: Niek Kouwenberg Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.1
Component: General Version: 3.0
Keywords: Confirmed HasPatch Review+ Cc:

Description

The CKEditor htmlParser uses a regular expression to check for valid attributes. This regex however, does not allow dashes to be used in the attribute name.

If I'm correct, dashes are allowed as character in an attribute name, and the "\w" set, does not include the dash. Therefore, the dash should be added separately (like the colon).

htmlparser.js:

- 21. var attribsRegex = /([\w:]+)...
+ 21. var attribsRegex = /([\w:\-]+)...

Attachments (2)

4351.patch (658 bytes) - added by Garry Yao 8 years ago.
4351_2.patch (645 bytes) - added by Garry Yao 8 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 8 years ago by Tobiasz Cudnik

Keywords: Confirmed HasPatch added
Milestone: CKEditor 3.xCKEditor 3.1

You are right, one example of such attribute would be http-equiv.

Thanks for report and the fix.

comment:2 Changed 8 years ago by Garry Yao

Owner: set to Garry Yao
Status: newassigned

Changed 8 years ago by Garry Yao

Attachment: 4351.patch added

comment:3 Changed 8 years ago by Garry Yao

Keywords: Review? added

comment:5 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

Looking at the W3C specs, we can see that attribute names accept a complex range of chars. But, let's keep it simple here, stickying with the common cases. So, let's just add "-" and "." to it ("_" is already inside \w).

Changed 8 years ago by Garry Yao

Attachment: 4351_2.patch added

comment:6 Changed 8 years ago by Garry Yao

Keywords: Review? added; Review- removed

comment:7 Changed 8 years ago by Niek Kouwenberg

Please note: In "4351_2.patch", the "." (period) needs to be escaped, it now matches any character!

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

Keywords: Review+ added; Review? removed

Replying to niek:

Please note: In "4351_2.patch", the "." (period) needs to be escaped, it now matches any character!

Yes! Be sure to have it escaped when committing.

comment:9 Changed 8 years ago by Garry Yao

Resolution: fixed
Status: assignedclosed

Fixed with [4349].

comment:10 in reply to:  9 ; Changed 8 years ago by Niek Kouwenberg

Resolution: fixed
Status: closedreopened

Replying to garry.yao:

Fixed with [4349].

You still haven't escaped the dot.

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

Resolution: fixed
Status: reopenedclosed

Replying to niek:

You still haven't escaped the dot.

After some internal talk and more research, we confirmed that the dot doesn't need to be escaped inside a character class.

comment:12 Changed 8 years ago by Niek Kouwenberg

Indeed, you are right! My apologies.

Small suggestion; add a comment to related tickets when discussion these things internally. This way you'll prevent confusion and possible new bug reports (since your last comment stated 'be sure to have it escaped', and the diff doesn't show the escape).

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

Replying to niek:

Small suggestion; add a comment to related tickets when discussion these things internally. This way you'll prevent confusion and possible new bug reports (since your last comment stated 'be sure to have it escaped', and the diff doesn't show the escape).

You're right. The committer should have added a small comment about it. Sorry for this small mis-observation, and thanks for the suggestion ;)

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