r50709 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r50708‎ | r50709 | r50710 >
Date:00:23, 18 May 2009
Author:simetrical
Status:deferred (Comments)
Tags:
Comment:
Fix SQL injection in FCKeditor caused by r50683

Never, ever, ever try to use str_replace() for database-related
escaping. There are functions provided for this for good reason.

Not tested except for PHP syntax, since I don't have the extension
installed.
Modified paths:
  • /trunk/extensions/FCKeditor/FCKeditorSajax.body.php (modified) (history)

Diff [purge]

Index: trunk/extensions/FCKeditor/FCKeditorSajax.body.php
——@@ -199,10 +199,9 @@
200200 $ns = NS_CATEGORY;
201201 $dbr = wfGetDB( DB_SLAVE );
202202 /// @todo FIXME: should use Database class
203 - $m_root = str_replace( "'", "\'", $m_root );
204203 $sql = "SELECT tmpSelectCatPage.page_title AS title FROM ".$dbr->tableName('categorylinks')." AS tmpSelectCat ".
205204 "LEFT JOIN ".$dbr->tableName('page')." AS tmpSelectCatPage ON tmpSelectCat.cl_from = tmpSelectCatPage.page_id ".
206 - "WHERE tmpSelectCat.cl_to LIKE '$m_root' AND tmpSelectCatPage.page_namespace = $ns";
 205+ "WHERE tmpSelectCat.cl_to LIKE ".$dbr->addQuotes($m_root)." AND tmpSelectCatPage.page_namespace = $ns";
207206
208207 $res = $dbr->query( $sql, __METHOD__ );
209208 $ret = '';

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r50683FCKeditor: reverting one SQL change back to raw SQL - was getting database er...ashley12:25, 17 May 2009

Comments

#Comment by Jack Phoenix (talk | contribs)   12:59, 18 May 2009

Great, thank you for fixing this. It was a problem with the original code (see r50666) and I rewrote the query to use Database class in r50669, but there was something wrong with it and I couldn't get it working so I decided to revert it to the latest working version.

As my fixme note says, the whole query should be rewritten to use MediaWiki's Database class (to avoid issues such as this).

Status & tagging log