Opened 14 years ago

Closed 14 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 14 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 14 years ago by Peter

Milestone: CKEditor 3.x

comment:2 Changed 14 years ago by Frederico Caldeira Knabben

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

Changed 14 years ago by Garry Yao

Attachment: 4683.patch added

comment:3 Changed 14 years ago by Garry Yao

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

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

Keywords: Review+ added; Review? removed

comment:5 Changed 14 years ago by Garry Yao

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

Fixed with [5007].

comment:6 Changed 14 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 14 years ago by Garry Yao

Resolution: fixed
Status: reopenedclosed

Regression fixed with [5028].

comment:8 Changed 14 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 14 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 14 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 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy