r55433 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r55432‎ | r55433 | r55434 >
Date:20:39, 21 August 2009
Author:simetrical
Status:ok (Comments)
Tags:
Comment:
Switch from Xml:: to Html:: in a few places

These should all theoretically be covered by the parser tests. All
tests pass, the only change needed was to account for less overescaping
in Html::expandAttributes(). There's no reason to escape <>' in
"-quoted attributes, unless I'm mistaken and have just added some XSS.
Modified paths:
  • /trunk/phase3/includes/Linker.php (modified) (history)
  • /trunk/phase3/includes/parser/DateFormatter.php (modified) (history)
  • /trunk/phase3/maintenance/parserTests.txt (modified) (history)

Diff [purge]

Index: trunk/phase3/maintenance/parserTests.txt
——@@ -1467,7 +1467,7 @@
14681468 !! input
14691469 [[%3c%23]][[%3e%23]]
14701470 !! result
1471 -<p>[[%3c%23]]<a href="https://www.mediawiki.org/index.php?title=%3E&amp;action=edit&amp;redlink=1" class="new" title="&gt; (page does not exist)">&gt;#</a>
 1471+<p>[[%3c%23]]<a href="https://www.mediawiki.org/index.php?title=%3E&amp;action=edit&amp;redlink=1" class="new" title="> (page does not exist)">&gt;#</a>
14721472 </p>
14731473 !! end
14741474
——@@ -3502,7 +3502,7 @@
35033503 !! input
35043504 [[:Category:MediaWiki User's Guide]]
35053505 !! result
3506 -<p><a href="https://www.mediawiki.org/wiki/Category:MediaWiki_User%27s_Guide" title="Category:MediaWiki User&#39;s Guide">Category:MediaWiki User's Guide</a>
 3506+<p><a href="https://www.mediawiki.org/wiki/Category:MediaWiki_User%27s_Guide" title="Category:MediaWiki User's Guide">Category:MediaWiki User's Guide</a>
35073507 </p>
35083508 !! end
35093509
——@@ -3513,7 +3513,7 @@
35143514 !! input
35153515 [[Category:MediaWiki User's Guide]]
35163516 !! result
3517 -<a href="https://www.mediawiki.org/wiki/Category:MediaWiki_User%27s_Guide" title="Category:MediaWiki User&#039;s Guide">MediaWiki User's Guide</a>
 3517+<a href="https://www.mediawiki.org/wiki/Category:MediaWiki_User%27s_Guide" title="Category:MediaWiki User's Guide">MediaWiki User's Guide</a>
35183518 !! end
35193519
35203520 !! test
——@@ -6820,7 +6820,7 @@
68216821 !! input
68226822 [[:Category:МедиаWики Усер'с Гуиде]]
68236823 !! result
6824 -<a href="https://www.mediawiki.org/wiki/Category:MediaWiki_User%27s_Guide" title="Category:MediaWiki User&#039;s Guide">MediaWiki User's Guide</a>
 6824+<a href="https://www.mediawiki.org/wiki/Category:MediaWiki_User%27s_Guide" title="Category:MediaWiki User's Guide">MediaWiki User's Guide</a>
68256825 !! end
68266826
68276827
Index: trunk/phase3/includes/parser/DateFormatter.php
——@@ -268,7 +268,7 @@
269269 $isoDate = implode( '-', $isoBits );;
270270
271271 // Output is not strictly HTML (it's wikitext), but <span> is whitelisted.
272 - $text = Xml::tags( 'span',
 272+ $text = Html::rawElement( 'span',
273273 array( 'class' => 'mw-formatted-date', 'title' => $isoDate ), $text );
274274
275275 return $text;
Index: trunk/phase3/includes/Linker.php
——@@ -206,7 +206,7 @@
207207
208208 $ret = null;
209209 if( wfRunHooks( 'LinkEnd', array( $this, $target, $options, &$text, &$attribs, &$ret ) ) ) {
210 - $ret = Xml::openElement( 'a', $attribs ) . $text . Xml::closeElement( 'a' );
 210+ $ret = Html::rawElement( 'a', $attribs, $text );
211211 }
212212
213213 wfProfileOut( __METHOD__ );
——@@ -382,7 +382,7 @@
383383 wfDebug("Hook LinkerMakeExternalImage changed the output of external image with url {$url} and alt text {$alt} to {$img}\n", true);
384384 return $img;
385385 }
386 - return Xml::element( 'img',
 386+ return Html::element( 'img',
387387 array(
388388 'src' => $url,
389389 'alt' => $alt ) );
——@@ -753,7 +753,7 @@
754754 return $link;
755755 }
756756 if ( $attribs ) {
757 - $attribsText .= Xml::expandAttributes( $attribs );
 757+ $attribsText .= Html::expandAttributes( $attribs );
758758 }
759759 return '<a href="'.$url.'"'.$attribsText.'>'.$text.'</a>';
760760 }

Comments

#Comment by Brion VIBBER (talk | contribs)   00:08, 10 September 2009

This actually might be a problem depending on the order of expansion of markup; we go to some effort to ensure that wiki-markup in your attributes isn't going to get expanded in the middle of something with these pre-escapes. Can you add a test case to confirm there's no problem with such a case?

#Comment by Simetrical (talk | contribs)   16:23, 10 September 2009

Could you clarify what kind of markup might be a problem? I'm not understanding your explanation.

#Comment by Brion VIBBER (talk | contribs)   19:28, 30 September 2009

Multiple passes of various expansion in the parser. We want to make sure anything we output at an intermediate stage is armored against further parsing, so meta-chars like ', [], <>, {}, etc should be escaped for safety. See examples in Sanitizer::safeEncodeAttribute...

But looking in more detail it looks like we already weren't using that from Xml:: class (just from things we normalize from within wikitext) so if it was safe before it's as safe now. Removing fixme marker.

Status & tagging log