r73453 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r73452‎ | r73453 | r73454 >
Date:09:05, 21 September 2010
Author:thomasv
Status:resolved (Comments)
Tags:
Comment:
use select wrapper (follow-up to r73216)
Modified paths:
  • /trunk/extensions/ProofreadPage/ProofreadPage_body.php (modified) (history)

Diff [purge]

Index: trunk/extensions/ProofreadPage/ProofreadPage_body.php
——@@ -783,11 +783,13 @@
784784 $pagelist = "'".implode( "', '", $pp)."'";
785785 $page_ns_index = MWNamespace::getCanonicalIndex( strtolower( $page_namespace ) );
786786 $dbr = wfGetDB( DB_SLAVE );
787 - $catlinks = $dbr->tableName( 'categorylinks' );
788 - $page = $dbr->tableName( 'page' );
789787 $cat = $dbr->strencode( str_replace( ' ' , '_' , wfMsgForContent( 'proofreadpage_quality0_category' ) ) );
790 - $query = "SELECT page_title FROM $page LEFT JOIN $catlinks on cl_from=page_id WHERE page_title in ( $pagelist ) AND cl_to='$cat' AND page_namespace=$page_ns_index;" ;
791 - $res = $dbr->query( $query , __METHOD__ );
 788+ $res = $dbr->select( array('page', 'categorylinks' ),
 789+ array("page_title"),
 790+ array("page_title IN ( $pagelist )", "cl_to='$cat'", "page_namespace=$page_ns_index" ),
 791+ __METHOD__, null,
 792+ array('categorylinks' => array ( 'LEFT JOIN','cl_from=page_id') ) );
 793+
792794 $q0_pages = array();
793795 if( $res ) {
794796 while( $o = $dbr->fetchObject( $res ) ) {

Follow-up revisions

RevisionCommit summaryAuthorDate
r73878follow up to r73453thomasv09:39, 28 September 2010
r73912follow up to r73453 : test if $pages is emptythomasv15:49, 28 September 2010

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r73216do not show page number and trailing newline for pages marked 'without text'thomasv16:44, 17 September 2010

Comments

#Comment by Brion VIBBER (talk | contribs)   04:46, 28 September 2010

Not using wrappers properly on the where clauses...

  • SECURITY ERROR: SQL injection in the manual creation of $pagelist: there's no escaping
    • if doing this manually, use $dbr->makeList()
    • no need to do it manually though! just use 'page_title' => $ppp
  • should be cl_to => $cat with the original unescaped value (let the wrappers do the escaping)
  • likewise, page_namespace => $page_ns_index

More generally I notice there's a lot of bugs in the code leading up to this; $page isn't always set, and there's some sloppy handling in page_index_text(). In cases where $page ends up empty (or unset) this leads to SQL errors (or exceptions thrown from Database::makeList() if it's done using the wrappers). Should probably check if the data set is empty and skip the query.

#Comment by Brion VIBBER (talk | contribs)   15:20, 28 September 2010

r73878 plus r73893 fix up the direct problems here, but note the underlying problems remain (though these should be tagged farther down)

Status & tagging log