r102038 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r102037‎ | r102038 | r102039 >
Date:20:14, 4 November 2011
Author:foxtrott
Status:deferred (Comments)
Tags:
Comment:
bugfix (output escaped twice)
Modified paths:
  • /trunk/extensions/SemanticForms/includes/SF_ParserFunctions.php (modified) (history)

Diff [purge]

Index: trunk/extensions/SemanticForms/includes/SF_ParserFunctions.php
——@@ -236,11 +236,11 @@
237237 }
238238 if ( $inLinkType == 'button' ) {
239239 $str = "<form action=\"$link_url\" method=\"get\" class=\"$classStr\">";
240 - $str .= Xml::element( 'input', array( 'type' => 'submit', 'value' => $inLinkStr ) );
 240+ $str .= Xml::tags( 'button', array( 'type' => 'submit', 'value' => $inLinkStr ), $inLinkStr );
241241 $str .= "$hidden_inputs</form>";
242242 } elseif ( $inLinkType == 'post button' ) {
243243 $str = "<form action=\"$link_url\" method=\"post\" class=\"$classStr\">";
244 - $str .= Xml::element( 'input', array( 'type' => 'submit', 'value' => $inLinkStr ) );
 244+ $str .= Xml::tags( 'button', array( 'type' => 'submit', 'value' => $inLinkStr ), $inLinkStr );
245245 $str .= "$hidden_inputs</form>";
246246 } else {
247247 // If a target page has been specified but it doesn't
——@@ -251,7 +251,7 @@
252252 $classStr .= " new";
253253 }
254254 }
255 - $str = Xml::element( 'a', array( 'href' => $link_url, 'class' => $classStr, 'title' => $inTitle ), $inLinkStr );
 255+ $str = Xml::tags( 'a', array( 'href' => $link_url, 'class' => $classStr, 'title' => $inTitle ), $inLinkStr );
256256 }
257257 // hack to remove newline from beginning of output, thanks to
258258 // http://jimbojw.com/wiki/index.php?title=Raw_HTML_Output_from_a_MediaWiki_Parser_Function

Follow-up revisions

RevisionCommit summaryAuthorDate
r102049followup 102038: finer-grained escapingfoxtrott21:16, 4 November 2011
r102107followup r102038: use tags instead of open/closeElements; ensure parameter sa...foxtrott17:03, 5 November 2011

Comments

#Comment by Nikerabbit (talk | contribs)   14:22, 5 November 2011

This looks wrong. Where is the other (remaining) layer of escaping there? Also, it makes not sense currently, because $inLinkStr is also used in the parameter value, where it it still escaped. Anyway, buttons don't even have a tag content, so why are they given that?

#Comment by F.trott (talk | contribs)   14:49, 5 November 2011

This is a parser function handler. AFAIR, the parameters are parsed by the MW parser before they are handed to the handler. They may not be suitable for usage as a attribute value, though, which is why they are still escaped there.

#Comment by Nikerabbit (talk | contribs)   15:18, 5 November 2011

I just tested setFunctionHook (what this is too). The input is provided as-is - no parsing or escaping is done to it. Also if you use insertStripItem to add output, no escaping or parsing is done for the output either. Please point where the escaping is done, because otherwise this is arbitrary html injection.

#Comment by F.trott (talk | contribs)   15:59, 5 November 2011

You are right, it is indeed passed as-is. The parsing (and thus sanitizing) is done on line 173 of the file:

$value = trim( $parser->recursiveTagParse( $elements[1] ) );

#Comment by Nikerabbit (talk | contribs)   16:38, 5 November 2011

It seems to be safe then, although I don't yet understand why are you parsing them. Is it to allow templates and stuff there? Maybe there is a way to make the code both safe and clear that it is safe in the same time.

#Comment by F.trott (talk | contribs)   16:52, 5 November 2011

Yes, it is to allow passing wikitext. I rather like it as it is although I admit, that it is not really obvious. But it has the advantage that each input parameter is sanitized in a systematic manner and none is forgotten. I guess I will make that line more prominent and add a comment.

#Comment by F.trott (talk | contribs)   17:04, 5 November 2011

Turns out there actually was a way to get past sanitizing. Fixed it in followup.

Status & tagging log