Opened 8 years ago

Closed 7 years ago

#4683 closed Bug (fixed)

CKEDITOR.dom.element attribute containing " (double quotation mark) breaks basicWriter HTML output

Reported by: Peter Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.2
Component: Core : Output Data Version: SVN (CKEditor) - OLD
Keywords: Review+ Cc:

Description

Example:

element.setAttribute( 'onclick', 'document.location="/products/fa-3819";' );

Expected HTML output:

onclick="document.location="/products/fa-3819";"

Generated HTML output:

onclick="document.location="/products/fa-3819";"

(I use textarea replacement mode in my setup.)

DOM handling functions like setAttribute/getAttribute are expected to handle values as literals. All the required string processing should be made by the underlying framework.

I think the problem affects only CKEDITOR.htmlParser.basicWriter. CKEDITOR.dom.element and its setAttribute implementation has nothing to do with the problem.

See: /core/htmlparser/basicwriter.js Line 61-64:

		/**
		 * Writes an attribute. This function should be called after opening the
		 * tag with {@link #openTagClose}.
		 * @param {String} attName The attribute name.
		 * @param {String} attValue The attribute value.
		 * @example
		 * // Writes ' class="MyClass"'.
		 * writer.attribute( 'class', 'MyClass' );
		 */
		attribute : function( attName, attValue )
		{
			this._.output.push( ' ', attName, '="', attValue, '"' );
		},

Please add proper escaping for attributes. (CKEDITOR.tools.htmlEncode is not applicable since it leaves double quotes untouched in Firefox)

http://www.w3.org/TR/html4/charset.html#h-5.3.2
"Some authors use the character entity reference """ to encode instances of the double quote mark (") since that character may be used to delimit attribute values."

Attachments (1)

4683.patch (1.1 KB) - added by Garry Yao 7 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 8 years ago by Peter

Milestone: CKEditor 3.x

comment:2 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: basicwriter attribute setAttribute removed
Milestone: CKEditor 3.2
Priority: HighNormal

Changed 7 years ago by Garry Yao

Attachment: 4683.patch added

comment:3 Changed 7 years ago by Garry Yao

Keywords: Review? added
Owner: set to Garry Yao
Status: newassigned

comment:4 Changed 7 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review+ added; Review? removed

comment:5 Changed 7 years ago by Garry Yao

Resolution: fixed
Status: assignedclosed
Version: 3.0.1SVN (CKEditor)

Fixed with [5007].

comment:6 Changed 7 years ago by Garry Yao

Resolution: fixed
Status: closedreopened

[5007] has brought a regression:

Reproducing Procedures

  1. Open any page sample with the following source codes:
    <a name="anchor">anchor</a>
    
  • Actual Result: JavaScript error thrown;

comment:7 Changed 7 years ago by Garry Yao

Resolution: fixed
Status: reopenedclosed

Regression fixed with [5028].

comment:8 Changed 7 years ago by Alfonso Martínez de Lizarrondo

Resolution: fixed
Status: closedreopened

The change in the htmlwriter plugin isn't right.

On one hand, it doesn't seems to fail at that code with the testcase, so we could avoid that check. But if we prefer to be cautious and protect it, then the previous statement should be covered also by the same check as it could also fail :

			if ( typeof attValue == 'string' )
			{
				if ( this.forceSimpleAmpersand )
					attValue = attValue.replace( /&amp;/, '&' );

				// Browsers don't always escape quote in attribute values. (#4683)
				attValue = attValue.replace( /"/g, '&quot;' )
			}

comment:9 Changed 7 years ago by Garry Yao

On one hand, it doesn't seems to fail at that code with the testcase.

It's the basicWriter (input) change in [5007] that exactly failed the above TC here, while there's a huge possibility that same kind of problem could also affect htmlWriter (output) as well.

But if we prefer to be cautious and protect it, then the previous statement should be covered also by the same check as it could also fail.

You're right, but that's not part of this regression.

comment:10 Changed 7 years ago by Garry Yao

Resolution: fixed
Status: reopenedclosed

Nice catch though, I've opened #5096 for it.

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