HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpModerate

Confirming safety of SQL injection

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
sql-injectioncodereviewphpstackoverflowmysqlpdo
sqlconfirmingsafetyinjection

Problem

I believe with everyone's help on Stack Overflow, I got my code safe guarded from SQL injection. I'm trying to confirm that is correct, just in case I misinterpreted the help and advice I received.


xxxx

prepare("SELECT * FROM xxxx WHERE xxxx LIKE :search");
$stmt->execute(array(':search' => '%'.$var.'%'));

$result = $stmt->fetchAll();

if ( count($result) ) {
foreach($result as $row) {
echo "" 
. $row['xxxx'] .
""
. $row['xxxx'] .
""
. $row['xxxx'] .
""
. $row['xxxx'] .
"" .
"http://www.xxx.com/" . $row['image'] . "

      

";  
}  
} 
else {
echo "No results found.";
}
} 
catch(PDOException $e) {
echo 'ERROR: ' . $e->getMessage();
}
?>

Solution


  • You should not mix PHP logic and HTML. All logic should be placed on top of the document and HTML (+ some minor PHP loops/echos/ifs) below it;



  • $var = $_GET['q']; is unneeded and you loose memory;



  • never use the shorttag (



-
You forgot to set the errormode for PDO, that means it doesn't throw exceptions. To set it, use PDO::setAttribute:

$pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);


  • I do not recommend to use * in your queries. The query will be faster and easier to use if you specify which columns you want;



-
You should specify the fetch style of PDOStatement::fetchAll. Otherwise it defaults to PDO::FETCH_BOTH, which means your array get to big and your script slower. You can specify it with a parameter in the function:

$stmt->fetchAll(PDO::FETCH_ASSOC);


Or specify it for the PDOstatement:

$stmt->setFetchMode(PDO::FETCH_ASSOC);


  • A small tip: Indent your code within each block with a tab or 4 spaces, that makes your code much easier to read. Also, choose a coding standard and use it everywhere. Consistent scripts are a pleasure to write, read and use!



Some none PDO/PHP things:

  • The element is outdated. You should do this with CSS;



  • You should not use inline CSS (like style=, align=, width=), put it in a CSS stylesheet instead;



  • When you are using 2


` next to eachother, you almost always know you do something wrong and you should take a look at how to solve it with CSS margins/paddings.

Code Snippets

$pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
$stmt->fetchAll(PDO::FETCH_ASSOC);
$stmt->setFetchMode(PDO::FETCH_ASSOC);

Context

StackExchange Code Review Q#24296, answer score: 10

Revisions (0)

No revisions yet.