Opened 7 years ago

Closed 7 years ago

#4683 closed Bug (fixed)

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

Reported by: psch 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 7 years ago by psch

  • Milestone CKEditor 3.x deleted

comment:2 Changed 7 years ago by fredck

  • Keywords basicwriter attribute setAttribute removed
  • Milestone set to CKEditor 3.2
  • Priority changed from High to Normal

Changed 7 years ago by garry.yao

comment:3 Changed 7 years ago by garry.yao

  • Keywords Review? added
  • Owner set to garry.yao
  • Status changed from new to assigned

comment:4 Changed 7 years ago by alfonsoml

  • Keywords Review+ added; Review? removed

comment:5 Changed 7 years ago by garry.yao

  • Resolution set to fixed
  • Status changed from assigned to closed
  • Version changed from 3.0.1 to SVN (CKEditor)

Fixed with [5007].

comment:6 Changed 7 years ago by garry.yao

  • Resolution fixed deleted
  • Status changed from closed to reopened

[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 set to fixed
  • Status changed from reopened to closed

Regression fixed with [5028].

comment:8 Changed 7 years ago by alfonsoml

  • Resolution fixed deleted
  • Status changed from closed to reopened

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 set to fixed
  • Status changed from reopened to closed

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

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