https://bugzilla.wikimedia.org/show_bug.cgi?id=20275
--- Comment #7 from Aryeh Gregor <[email protected]> 2009-08-26 01:36:41 UTC --- Okay, it seems like escapeLike() does strencode() as well. So you've got like() doing no escaping at all. I think this is potentially quite surprising; I'd prefer to see it doing addQuotes() on the input. Generally speaking, APIs that escape more than expected are better than those that escape less than expected -- the former leads to double-escaping, the latter leads to SQL injection (or XSS, etc.). escapeLike() clearly can't be used in that case, of course, some new and confusingly similar method would have to be made that only does the replacement of _ and % and \ and nothing else. When reviewing your patch, I got seriously confused about escaping. I had to rewrite the whole thing twice as I figured out what your method did and what escapeLike() did and how LIKE works (I didn't realize \\\\ was necessary for a single slash in LIKE). As far as I understand it, the correct way to do things currently is 'LIKE ' . $db->escapeLike( $foo ) . '%', while with your patch the correct way would be $db->like( $db->escapeLike( $foo ) . '%' ), and I'd like to see $db->like( $db->someNewName( $foo ) . '%' ). If I'm wrong, then most of my review comments here are nonsense. :D - $likeprefix = str_replace( '_', '\\_', $prefix); ... - AND pl_title LIKE '$likeprefix:%'"; + AND pl_title " . $wgDatabase->like( $wgDatabase->escapeLike( $name ) . ':%' ); ... - $likeprefix = str_replace( '_', '\\_', $prefix); ... - AND {$page}_title LIKE '$likeprefix:%'"; + AND {$page}_title " . $this->db->like( $this->db->escapeLike( $name ) . ':%' ); Don't these add double-escaping of some kind? (Or was $likeprefix not already escaped, and this would previously have been SQL injection if it weren't from a maintenance script?) - array( 'el_index LIKE ' . $dbr->addQuotes( $like ) ), __METHOD__ ); + array( 'el_index ' . $dbr->like( $like ) ), __METHOD__ ); // LinkFilter::makeLike already escapes everything ... - array( 'el_index LIKE ' . $dbr->addQuotes( $like ) ), __METHOD__ ); + array( 'el_index ' . $dbr->like( $like ) ), __METHOD__ ); // LinkFilter::makeLike already escapes everything So these were double-escaping before? + $this->mQueryConds = array( 'LOWER(img_name) ' $dbr->like( '%' . strtolower( $nt->getDBkey() ) . '%' ); Syntax error? A dot is missing. - $m = $dbr->strencode( strtolower( $nt->getDBkey() ) ); - $m = str_replace( "%", "\\%", $m ); - $m = str_replace( "_", "\\_", $m ); - $this->mQueryConds = array( "LOWER(img_name) LIKE '%{$m}%'" ); + $this->mQueryConds = array( 'LOWER(img_name) ' $dbr->like( '%' . strtolower( $nt->getDBkey() ) . '%' ); ... - $encSearch = $dbr->addQuotes( $stripped ); + $like = $dbr->like( $stripped ); $encSQL = ''; if ( isset ($this->mNs) && !$wgMiserMode )@@ -144,7 +144,7 @@ $externallinks $use_index WHERE page_id=el_from... - AND $clause LIKE $encSearch + AND $clause $like ... - $encRange = $dbr->addQuotes( "$range%" ); $encAddr = $dbr->addQuotes( $iaddr ); $conds[] = "(ipb_address = $encIp) OR - (ipb_range_start LIKE $encRange AND + (ipb_range_start " . $dbr->like( "$range%" ) . " AND ... - $this->addWhere('el_index LIKE ' . $db->addQuotes( $likeQuery )); + $this->addWhere('el_index ' . $db->like( $likeQuery )); } else if(!is_null($protocol)) - $this->addWhere('el_index LIKE ' . $db->addQuotes( "$protocol%" )); + $this->addWhere('el_index ' . $db->like( "$protocol%" )); These all remove addQuotes()/strencode(). Are you adding SQL injection here? Even if theoretically you don't think the variables should contain characters that need to be escaped, it's better to call addQuotes()/strencode() to be on the safe side, and the closer to the point of use, the better. + $db = $this->mDb; ... - $this->mConds[] = $this->mDb->bitAnd('log_deleted', LogPage::DELETED_ACTION) . ' = 0'; + $this->mConds[] = $db->bitAnd('log_deleted', LogPage::DELETED_ACTION) . ' = 0'; } else if( !$wgUser->isAllowed( 'suppressrevision' ) ) { - $this->mConds[] = $this->mDb->bitAnd('log_deleted', LogPage::SUPPRESSED_ACTION) . + $this->mConds[] = $db->bitAnd('log_deleted', LogPage::SUPPRESSED_ACTION) . Try to avoid making unrelated changes like this, it makes patches harder to review. /** + * LIKE statement wrapper + * @param s$ String: pattern for LIKE to match. Must already be escaped with escapeLike() where needed. + * @ return String: fully built LIKE statement + */ It's better if you make sure to use the normal spacing here, with an extra space before the * starting each line of the comment after the first, so the *'s on the left all line up. It would be good if the comment were more verbose, but that's no problem, I can expand it a bit in a later commit. + return "LIKE '" . $s . "'"; I would add an extra space before and after the return value (also for the SQLite modified version). It can't hurt, and it might avoid SQL syntax errors from stuff like "tablename" . $db->like( $foo ). I can't comment on the tests, since I've never looked at the PHPUnit that's checked in. We don't really use tests except for the parser. -- Configure bugmail: https://bugzilla.wikimedia.org/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are the assignee for the bug. You are on the CC list for the bug. _______________________________________________ Wikibugs-l mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
