0

Do I need to do anything to protect the three variables, like using the escape string or binding them? I'm not sure if I did this correctly, people just recommended using prepared statements and so I am trying to figure them out.

$order = $_POST['order'];
$heading = $_POST['heading'];
$content = $_POST['content'];    
try {
$dbh = new PDO("mysql:host=$hostname;dbname=saintfiv_faq", $username, $password);
/*** echo a message saying we have connected ***/
echo 'Connected to database<br />';

/*** INSERT data ***/
$stmt = $dbh->prepare("INSERT INTO faq(`order`, `heading`, `content`) VALUES (:order, :heading, :content)");
$stmt->bindParam(':order', $order, PDO::PARAM_INT);
$stmt->bindParam(':heading', $heading, PDO::PARAM_STR, strlen($heading));
$stmt->bindParam(':content', $content, PDO::PARAM_STR, strlen($content));
/*** close the database connection ***/
$stmt->execute();
}
catch(PDOException $e)
{
echo $e->getMessage();
}
Michael St Clair
  • 5,937
  • 10
  • 49
  • 75

2 Answers2

2

You are not using prepared statements in your code. Prepared statements would look more like this:

$stmt = $db->prepare("INSERT INTO foo (bar, baz) VALUES (?, ?);");

$stmt->bindValue(1, "Fez");
$stmt->bindValue(2, "Hat");
$stmt->execute();

Your example code is potentially vulnerable to SQL injection because you are simply inserting the variables directly into the SQL string. You will want to either use prepared statements and bind the values (this is the preferred solution), or alternatively just make sure you escape all input to exec() correctly.

It might also be worth mentioning that exec() is fine for totally hardcoded statements - e.g., $db->exec("SELECT foo FROM bar;"); - since the SQL is hardcoded, there is no potential for SQL injection. I, however, like to always use prepare instead, as a matter of style.

To specifically execute the query in your code, you would do something like this:

$stmt = $db->prepare("INSERT INTO faq (`order`, `heading`, `content`) " .
    "VALUES (?, ?, ?);");

$stmt->bindValue(1, $order);
$stmt->bindValue(2, $heading);
$stmt->bindValue(3, $content);
$stmt->execute();

I would also recommend the official PHP documentation, as it shows some other ways of doing the same thing (namely, you can pass your parameters as an array to execute() instead of binding them individually): http://php.net/manual/en/pdo.prepare.php.

CmdrMoozy
  • 3,870
  • 3
  • 19
  • 31
  • so how would I put my variables into that – Michael St Clair Jun 27 '13 at 22:47
  • I updated my answer to execute the query you have in your OP. – CmdrMoozy Jun 27 '13 at 22:52
  • I just edited my code based off what I read in the manual, does that work? – Michael St Clair Jun 27 '13 at 23:00
  • I generally prefer to use `bindValue` instead of `bindParam`, unless I have a good reason (like re-using a query multiple times with different values). I also don't think you should be passing the string length to `bindParam` - it's unecessary, and generally your data will change when using `bindParam` so a fixed string length wouldn't work. Other than that, it looks good to me - if you've tested it, did you run into any problems? – CmdrMoozy Jun 28 '13 at 16:19
  • I tested it and it seems to be fine – Michael St Clair Jun 29 '13 at 02:14
1

You definitely DO need to protect them - otherwise, someone might put in a heading of "AMADANON's Heading" - and the apostrophe will look to the database as a close-quote. This is an inadvertant example, there will also be people trying to attack your database.

The recommended(1) way to do this is to use parameters. use VALUES(?,?,?), then when calling execute, pass the variables in there.

For more information, Read the PHP manual, check out the examples

I don't like bound variables, it's too difficult to see what happens where.

This also means you can prepare a cursor (with a SQL statement) once, then use it many times with different parameters.

Escape is acceptable, but I don't see where it adds any benefit, and parameters are clearer.

(1) by me

AMADANON Inc.
  • 5,753
  • 21
  • 31