r101844 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r101843‎ | r101844 | r101845 >
Date:18:25, 3 November 2011
Author:kipcool
Status:ok (Comments)
Tags:
Comment:
"and" missing
Modified paths:
  • /trunk/extensions/Wikidata/OmegaWiki/WikiDataAPI.php (modified) (history)

Diff [purge]

Index: trunk/extensions/Wikidata/OmegaWiki/WikiDataAPI.php
——@@ -206,7 +206,7 @@
207207 $dbr = wfGetDB( DB_SLAVE );
208208 $queryResult = $dbr->query( "SELECT syntrans_sid FROM {$dc}_syntrans " .
209209 "WHERE defined_meaning_id=$definedMeaningId AND expression_id=$expressionId " .
210 - getLatestTransactionRestriction( "{$dc}_syntrans" ) . " LIMIT 1" );
 210+ ' AND ' . getLatestTransactionRestriction( "{$dc}_syntrans" ) . " LIMIT 1" );
211211
212212 if ( $synonym = $dbr->fetchObject( $queryResult ) )
213213 return $synonym->syntrans_sid;

Comments

#Comment by Nikerabbit (talk | contribs)   11:01, 4 November 2011

This code is btw not demonstrably secure from SQL injections.

#Comment by P858snake (talk | contribs)   12:10, 4 November 2011

How is this OK if its not secure from SQL injections?

#Comment by Catrope (talk | contribs)   12:16, 4 November 2011

Niklas said "not demonstrably secure" which is not the same thing as "not secure". Code should be written in such a way that the reviewer can immediately tell that it's secure ("demonstrably secure"). This code is not written that way, which requires a review to dig into the code and figure out where the various variables come from and what the getLatestTransactionRestriction function can return, and determine if any of those can lead to security issues. This is bad because it's harder to tell if something is secure. It's still possible to write secure code this way, it's just hard to review and easy for a developer to do it wrong or to break security later.

#Comment by Nikerabbit (talk | contribs)   12:37, 4 November 2011

What Roan said + that it is not fair to require someone to write code they didn't necessarily write.

#Comment by Kipmaster (talk | contribs)   12:17, 4 November 2011

Please explain!

I did not write that code, I just repaired a bug.

Status & tagging log