4

I am using PDO prepared statements to insert data to database from external xml source, because I do not trust the source 100% I used bindValue on all variables including strings and integers, for example:

SQL:

INSERT INTO table (id, int1, int2, string1, string2)
VALUES (:id, :int1, :int2, :string1, :string2)

In my PDO function:

$sth->bindValue(":id", $id, PDO::PARAM_INT);
$sth->bindValue(":int1", $int1, PDO::PARAM_INT);
$sth->bindValue(":int2", $int2, PDO::PARAM_INT);
$sth->bindValue(":string1", $string1, PDO::PARAM_STR);
$sth->bindValue(":string2", $string2, PDO::PARAM_STR);

Now my question is, if I previousley used int casting to get the integer values, do I still need to use prepared statment for integer values:

$id= (int) $xml->node->attributes()->id

$id will always be an integer, even if the id in the xml file is not an integer the returned value when using (int) will be 0.

Is it safe in this case to just do:

INSERT INTO table (id, int1, int2, string1, string2)
VALUES ($id, $int1, $int2, :string1, :string2)

EDIT (Example of shorter code):

Binding all parameters:

$sql="INSERT INTO table (id, int1, int2, string1, string2)
     VALUES (:id, :int1, :int2, :string1, :string2)";

$pars = array(":id"=>$id,":int1"=>$int1,":int2"=>$int2,":string1"=>$string1,
              ":string2"=>$string2); 

$model->insert($sql,$pars);

Without Intergers Binding:

$sql="INSERT INTO table (id, int1, int2, string1, string2)
      VALUES ($id, $int1, $int2, :string1, :string2)";

$pars = array(":string1"=>$string1,":string2"=>$string2);

$model->insert($sql,$pars);

now imagine this code with 20+ parameters.

Cœur
  • 37,241
  • 25
  • 195
  • 267
DeepBlue
  • 684
  • 7
  • 23
  • JFYI, to handle 20+ parameters with one single variable *arrays* were specifically invented. – Your Common Sense Mar 25 '14 at 10:44
  • Yes that's why I used an array in the above code! ($pars array) – DeepBlue Mar 25 '14 at 10:51
  • You have only *two* variables in it. While it can contain all 20+. witout the need of building it manually – Your Common Sense Mar 25 '14 at 10:57
  • If I have $data array containing all the data then I still need to put vairables manually in the array.. so it is the same (ex: $data=array($param1,$param1...$param20). – DeepBlue Mar 25 '14 at 11:17
  • You don't. You are parsing XML, and parser is giving you an array already – Your Common Sense Mar 25 '14 at 11:24
  • I like your shorter version anyway, I just didn't want to bypass bindValue (although I know there is no big adventage of using it other than forcing specific datatypes) – DeepBlue Mar 25 '14 at 11:41
  • regarding the XML parsing it is not as simple as you think, there is a lot of formating needed, so there is no way to rely on direct array input.. thanks anyway I will go with your direct execute code I think it is much cleaner. – DeepBlue Mar 25 '14 at 11:53

2 Answers2

4

Theoretically it's safe. An int cannot contain malicious data, so there's hardly any chance of SQL injection.

However, this depends on you making no mistakes in your source code. The variables $int1 and $int2 may be guaranteed to be integers now, but as you'll be editing your code and move stuff around, they may not be in the future. Using parameter bindings when creating the SQL statement gives you 100% certainty that nothing can cause malformed syntax. This guarantee does not exist if you rely on other parts of the code doing the sanitisation.

deceze
  • 510,633
  • 85
  • 743
  • 889
3

This is wrong question to ask. For too many reasons.

Architectural is one of them.
When your app matures, you will separate your DB layer from input processing level. Means DB code will never know which variable intended to be int and which is not.

Means code something like this

$model = new Model($data);
$model->save();

See - there is no trace nor of PDO, nor of SQL. All DB interaction is done behind the scenes. And obviously this DB layer should be able to handle data properly regardless any prior validations. Which makes the latter just useless, if they done only for DB. Frankly, It's DB layer's business, how to format values, not programmer's

Overall sanity is another.
You can make your code as simple as

$sql = "INSERT INTO table VALUES (?, ?, ?, ?, ?)";
$pdo->prepare($sql)->execute($data);

if you have your values already in array
Why bother with inserting variables directly at all?

Another is efficiency.
You are apparently doing multiple inserts. With placeholders for all values you can prepare your query ONCE and then only execute it with new portions of data - so, actually use this neat side effect of prepared statements:

$stmt = $pdo->prepare("INSERT INTO table VALUES (?, ?, ?, ?, ?)");
foreach ($data as $row) {
    $stmt->execute($row);
}
Your Common Sense
  • 156,878
  • 40
  • 214
  • 345
  • Actually this will not affect my Architectural which will not change, the only thing that will change is the passed $data array. the thing is that I do not need prepared statments in all my app, so I pass both SQL string and $data array to my insert function which will take care of the passed parameters. – DeepBlue Mar 25 '14 at 09:54
  • for the sanity reason "why bother inseting variables directly?", good question, my reasons is that it makes my code shorter and I have a feeling it will make it faster (although I have no evidence on that) in some cases where I have a lot of parameters (20+) to bind. – DeepBlue Mar 25 '14 at 09:56
  • Shorter? Shorter than shown here? Care to prove such a statement with a code? – Your Common Sense Mar 25 '14 at 09:58
  • as of architectural reason, it is said "*When* your app matures..." – Your Common Sense Mar 25 '14 at 09:59
  • I have edited my orignal question and added an example of shorter code. – DeepBlue Mar 25 '14 at 10:41
  • 1. it is **artificial**, not a real code. 2. it is not shorter. – Your Common Sense Mar 25 '14 at 10:43
  • Do you care to explain how this is not shorter?! And according to you a simple PHP array is artificial and not real code! really? can you give a real example to prove your theory? – DeepBlue Mar 25 '14 at 10:46
  • Just see second example in my answer. For the 20 parameters only 13 question marks have to be added, keeping the rest of the code the same – Your Common Sense Mar 25 '14 at 10:50