Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I've this function to list products from database by category. I'm using prepared statements, but wouldn't this make it pretty pointless to use them?

function getProducts($category,$search_param,$pdo){
      $query = "SELECT * FROM renkaat 
        INNER JOIN ajoneuvotyypit ON tyyppiID = ajoneuvotyyppiID 
        INNER JOIN vuodenajat ON vuodenaikaID = renkaat.vuodenaika
        INNER JOIN valmistajat ON renkaat.valmistaja = valmistajat.valmistajaID
        WHERE renkaat.$category= ?
    $smt = $pdo->prepare($query);

      //do stuff

And the function is simply called like this, with no escaping done:


Should I escape the $category?

share|improve this question
up vote 10 down vote accepted

Yes, $category in your query string, coming directly from $_GET is a problem.

You should validate $category, and that's not exactly the same as escaping. I assume you have a limited set of valid categories in your database, so validation should be something like:

$valid_categories = array("sector", "industry", "company");
if (!in_array($category, $valid_categories)) {
    // no no

As for the other part of your question, paraphrased:

Is it pointless to use prepared statements if there is a variable part in the query?

It's not pointless, because every time you run the query for the same $category value, it can benefit from the previously prepared statement.

share|improve this answer
I meant pointless from a security point of view, not performance, but thanks for the heads up! Luckily this little snippet was on the administration side and not the live site... – user22886 Apr 19 '14 at 18:19

Yes, this code is vulnerable to SQL injection, just like any dynamically composed SQL that is not properly quoted. The quoting mechanism for identifiers is database dependent, though.

A bigger concern is how this situation came about in the first place. If you ever need to programmatically choose a column like that, your schema is probably poorly designed. There should be a small number of columns in the schema, all with fixed names. The fact that you have many columns, all with compatible types such that you can apply the same query over them, indicates that those attributes should all be upgraded to an Attribute entity.

share|improve this answer
Yeah, I wasn't the one creating this system in the first place. I'm just "adding features" here, but seems like that I've to go trough the whole code to check for these kinds of incidents. – user22886 Apr 19 '14 at 18:17

Yes, it is a problem, the following will give you all data from all categories:

$category = 'validcategory="searchparam" OR 1 = 1 OR validcategory'

You should check if the $category exists first, maybe dynamic like this is better than defining an array in code:

share|improve this answer

More than validating or quoting your queries you should parameterize them. Here's a different SO question which covers the topic.

share|improve this answer
It is parameterized? – user22886 Apr 21 '14 at 17:08

Your Answer


By posting your answer, you agree to the privacy policy and terms of service.