From 1a395595dc3c96355b7152dc7afae9b2b2888b29 Mon Sep 17 00:00:00 2001 From: Kevin Israel Date: Sat, 16 Nov 2013 05:58:01 -0500 Subject: [PATCH] SECURITY: Always quote attribute values in Html ... even if $wgWellFormedXml is false. Bug: 55548 Change-Id: Iaa83d6c0784cfdbbe0f3fc716088b70b6d1457e9 --- includes/Html.php | 23 +---- tests/phpunit/includes/HtmlTest.php | 200 ++++++++++++++++++------------------ 2 files changed, 102 insertions(+), 121 deletions(-) diff --git a/includes/Html.php b/includes/Html.php index 932f753..7a6b44a 100644 --- a/includes/Html.php +++ b/includes/Html.php @@ -383,8 +383,7 @@ class Html { * 'http://www.mediawiki.org/' ) becomes something like * ' href="http://www.mediawiki.org"'. Again, this is like * Xml::expandAttributes(), but it implements some HTML-specific logic. - * For instance, it will omit quotation marks if $wgWellFormedXml is false, - * and will treat boolean attributes specially. + * For instance, it will treat boolean attributes specially. * * Attributes that should contain space-separated lists (such as 'class') array * values are allowed as well, which will automagically be normalized @@ -502,24 +501,6 @@ class Html { $value = implode( ' ', array_unique( $value ) ); } - // See the "Attributes" section in the HTML syntax part of HTML5, - // 9.1.2.3 as of 2009-08-10. Most attributes can have quotation - // marks omitted, but not all. (Although a literal " is not - // permitted, we don't check for that, since it will be escaped - // anyway.) - # - // See also research done on further characters that need to be - // escaped: http://code.google.com/p/html5lib/issues/detail?id=93 - $badChars = "\\x00- '=<>`/\x{00a0}\x{1680}\x{180e}\x{180F}\x{2000}\x{2001}" - . "\x{2002}\x{2003}\x{2004}\x{2005}\x{2006}\x{2007}\x{2008}\x{2009}" - . "\x{200A}\x{2028}\x{2029}\x{202F}\x{205F}\x{3000}"; - if ( $wgWellFormedXml || $value === '' - || preg_match( "![$badChars]!u", $value ) ) { - $quote = '"'; - } else { - $quote = ''; - } - if ( in_array( $key, self::$boolAttribs ) ) { // In HTML5, we can leave the value empty. If we don't need // well-formed XML, we can omit the = entirely. @@ -552,7 +533,7 @@ class Html { // @todo FIXME: Is this really true? $map['<'] = '<'; } - $ret .= " $key=$quote" . strtr( $value, $map ) . $quote; + $ret .= " $key=\"" . strtr( $value, $map ) . '"'; } } return $ret; diff --git a/tests/phpunit/includes/HtmlTest.php b/tests/phpunit/includes/HtmlTest.php index 21372a0..15b7291 100644 --- a/tests/phpunit/includes/HtmlTest.php +++ b/tests/phpunit/includes/HtmlTest.php @@ -163,22 +163,22 @@ class HtmlTest extends MediaWikiTestCase { $this->assertEquals( ' empty_string=""', Html::expandAttributes( array( 'empty_string' => '' ) ), - 'Empty string is always quoted' + 'Attribute values are always quoted: Empty string' ); $this->assertEquals( - ' key=value', + ' key="value"', Html::expandAttributes( array( 'key' => 'value' ) ), - 'Simple string value needs no quotes' + 'Attribute values are always quoted: Simple string' ); $this->assertEquals( - ' one=1', + ' one="1"', Html::expandAttributes( array( 'one' => 1 ) ), - 'Number 1 value needs no quotes' + 'Attribute values are always quoted: Number 1' ); $this->assertEquals( - ' zero=0', + ' zero="0"', Html::expandAttributes( array( 'zero' => 0 ) ), - 'Number 0 value needs no quotes' + 'Attribute values are always quoted: Number 0' ); $this->setMwGlobals( 'wgWellFormedXml', true ); @@ -304,48 +304,48 @@ class HtmlTest extends MediaWikiTestCase { */ public function testNamespaceSelector() { $this->assertEquals( - '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . '', Html::namespaceSelector(), 'Basic namespace selector without custom options' ); $this->assertEquals( - ' ' . - '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . '', Html::namespaceSelector( array( 'selected' => '2', 'all' => 'all', 'label' => 'Select a namespace:' ), @@ -355,24 +355,24 @@ class HtmlTest extends MediaWikiTestCase { ); $this->assertEquals( - ' ' . - '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . '', Html::namespaceSelector( array( 'label' => 'Select a namespace:' ) @@ -383,18 +383,18 @@ class HtmlTest extends MediaWikiTestCase { public function testCanFilterOutNamespaces() { $this->assertEquals( - '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . '', Html::namespaceSelector( array( 'exclude' => array( 0, 1, 3, 100, 101 ) ) @@ -405,23 +405,23 @@ class HtmlTest extends MediaWikiTestCase { public function testCanDisableANamespaces() { $this->assertEquals( - '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . + '' . "\n" . '', Html::namespaceSelector( array( 'disable' => array( 0, 1, 2, 3, 4 ) @@ -436,7 +436,7 @@ class HtmlTest extends MediaWikiTestCase { */ public function testHtmlElementAcceptsNewHtml5TypesInHtml5Mode( $HTML5InputType ) { $this->assertEquals( - '', + '', Html::element( 'input', array( 'type' => $HTML5InputType ) ), 'In HTML5, HTML::element() should accept type="' . $HTML5InputType . '"' ); @@ -490,10 +490,10 @@ class HtmlTest extends MediaWikiTestCase { 'area', array( 'shape' => 'rect' ) ); - $cases[] = array( '', + $cases[] = array( '', 'button', array( 'formaction' => 'GET' ) ); - $cases[] = array( '', + $cases[] = array( '', 'button', array( 'formenctype' => 'application/x-www-form-urlencoded' ) ); @@ -567,28 +567,28 @@ class HtmlTest extends MediaWikiTestCase { ); # specific handling - $cases[] = array( '', + $cases[] = array( '', 'input', array( 'type' => 'checkbox', 'value' => 'on' ), 'Default value "on" is stripped of checkboxes', ); - $cases[] = array( '', + $cases[] = array( '', 'input', array( 'type' => 'radio', 'value' => 'on' ), 'Default value "on" is stripped of radio buttons', ); - $cases[] = array( '', + $cases[] = array( '', 'input', array( 'type' => 'submit', 'value' => 'Submit' ), 'Default value "Submit" is kept on submit buttons (for possible l10n issues)', ); - $cases[] = array( '', + $cases[] = array( '', 'input', array( 'type' => 'color', 'value' => '' ), ); - $cases[] = array( '', + $cases[] = array( '', 'input', array( 'type' => 'range', 'value' => '' ), ); # ', + $cases[] = array( '', 'button', array( 'type' => 'submit' ), 'According to standard the default type is "submit". Depending on compatibility mode IE might use "button", instead.', ); @@ -644,7 +644,7 @@ class HtmlTest extends MediaWikiTestCase { 'Blacklist form validation attributes.' ); $this->assertEquals( - ' step=any', + ' step="any"', Html::expandAttributes( array( 'min' => 1, 'max' => 100, 'pattern' => 'abc', 'required' => true, 'step' => 'any' ) ), 'Allow special case "step=any".' ); -- 1.8.4.2