0

This is the query I am preparing. This looks too much like a normal mysql_query and I am unsure if I am out of the safe boundaries of PDO. 

function opinionlist($orderby="dateposted desc",$page="0",$pagesize="10"){
  $dbh = new PDO(...);
  $s = $dbh->prepare("select * from fe_opinion 
                       order by :orderby limit :page,:pagesize");
  $s->bindParam(":orderby", $orderby);
  $s->bindParam(":page", $page);
  $s->bindParam(":pagesize, $pagesize");
  $s->execute();
  $opinionlist = $s->fetchAll(PDO::FETCH_ASSOC);
  echo json_encode($opinionlist);
}
  1. Can I safely build queries like this?
  2. Is getting a table name for an order by statement safe or should I ONLY get values from input?

currently I changed my code to

function opinionlist($orderby="dateposted desc",$page="0",$pagesize="10"){
  $orderbylist=array("dateposted desc","countcomment desc","countvote desc");
  $dbh = new PDO(...);
  if(!in_array($orderby, $orderbylist)){$orderby="dateposted desc";}
  $s = $dbh->prepare("select * from fe_opinion order by $orderby limit :page,:pagesize");
  $s->bindParam(":page", $page);
  $s->bindParam(":pagesize, $pagesize");
  $s->execute();
  $opinionlist = $s->fetchAll(PDO::FETCH_ASSOC);
  echo json_encode($opinionlist);
}
Uğur Gümüşhan
  • 2,455
  • 4
  • 34
  • 62
  • This might not actually work for the `LIMIT`. Parameters do not replace variables for concatenation. You could do `LIMIT :offset, :limit` but you can't replace the whole LIMIT clause with a single parameter. – Michael Berkowski Dec 08 '11 at 13:24
  • @Michael I changed the page parameters, my query is still getting table name for the order by statement. Is getting something other than a value injection safe? – Uğur Gümüşhan Dec 08 '11 at 13:28
  • 1
    You can't "bind" column names. That wouldn't be a prepared statement, but make it dynamic again. – mario Dec 08 '11 at 13:32
  • 1
    I checked the docs and didn't find specific info but this question seems to indicate a bound param won't work as an order by table either http://stackoverflow.com/questions/2542410/how-do-i-set-order-by-params-using-prepared-pdo-statement – Michael Berkowski Dec 08 '11 at 13:33
  • I would suggest splitting up the `$orderby` parameter into the column name and a boolean for ascending and descending. This will make it easier to validate. However, have you considering using an existing PHP ORM to construct queries? – Michael Mior Dec 08 '11 at 14:01
  • @MichaelMior Yes I made an array of accepted values for that parameter in second code block. and about ORM, no. I recently gave up on object oriented programming in favor of my precious programming time. I dont play legos anymore and I am totally fine with it. – Uğur Gümüşhan Dec 08 '11 at 14:03
  • @UğurGümüşhan Why does OOP take more time? – Michael Mior Dec 08 '11 at 15:14
  • @MichaelMior http://www.geocities.com/tablizer/oopbad.htm#gui – Uğur Gümüşhan Dec 09 '11 at 15:22

1 Answers1

1

It's good practice to validate the type and contents of your inputs.

You can sanitize $orderby with mysql_real_escape_string() or reject values for $orderby which don't consist of a single valid column name followed by an optional asc or desc (or even a comma separated list of these, if you wish). You could determine your list of valid columns either by hard coding or requesting a list of columns for that table (from INFORMATION_SCHEMA).

And of course you can use is_numeric() for $page and $pagesize.

With that in place, not only are you covering against sql injection, but also making your code more robust.

Small update: as you discovered, you can't use a parameter for $orderby.

Colin Pickard
  • 45,724
  • 13
  • 98
  • 148