PHP PDO MySQL count() prepared statement

Question

I'm on a Web development course. Working with PHP PDO MySQL they teach us on a workshop to do this:

function countUsers($search) {
    $and = '';
    if ($search != '') {
        $and = " AND user_name LIKE '%".$search."%'";    
    }
    $total = $this->db->query("SELECT COUNT(id) as rows FROM users WHERE valid = 1" . $and)->fetch(PDO::FETCH_OBJ);
    return $total->rows;
}

From my point of view this is totally wrong, the statement is not prepared and is passed directly from user input without any validation that can lead to SQL Injection, so I proposed this to the trainer (I know fetchColumn() would be more appropriate here but let's stick with this for the sake of the example):

function countUsers($search) {
    $and = '';
    $sqlSearch = "%$search%";

    if ($search != '') {
        $and = " AND user_name LIKE :username";  
    }

    $sql = "SELECT COUNT(id) as rows FROM users WHERE valid = 1" . $and;
    $sth = $this->db->prepare($sql);
    if ($search != '') {
        $sth->bindParam(':username', $sqlSearch, PDO::PARAM_STR);
    }
    $sth->execute();
    $total = $sth->fetch(PDO::FETCH_OBJ);
    return $total->rows;
}

Am I wrong? Are they wrong or we both wrong/right?

You should always bind untrusted parameters (which is pretty much all of them). — JimL
– JimL, Commented May 14, 2016 at 8:15
You are completely right. And even if input is trusted, it can still contain characters that break the sql so you should always use a prepared statement and bind the values to the placeholders. — jeroen
– jeroen, Commented May 14, 2016 at 8:15
I raised the problem in the class, they said I'm right but this is not an issue because this is just a working example and is not the purpose of the course to have 20 hours of web security. I think they are VERY wrong giving completely wrong and unsecure examples for students. They better used mysql_* or mysqli_* if not taking advantage of PDO's prepared statements. — bsteo
– bsteo, Commented May 14, 2016 at 8:33
Really? 20 hours to do it right from the start, that is, to prepare a statement and bind the values?! It seems your teacher is up for retirement... — jeroen
– jeroen, Commented May 14, 2016 at 8:40

Your Common Sense · Accepted Answer · 2016-05-14 08:43:31Z

4

Yes you are right.

However, your code is not optimal. In fact, prepared statements are intended to make your code cleaner, not more bloated.

function countUsers($search) {
    $sql = "SELECT COUNT(id) FROM users WHERE valid = 1 AND user_name LIKE ?";
    $sth = $this->db->prepare($sql);
    $sth->execute(["%$search%"]);
    return $sth->fetchColumn();
}

Part of the cleanup I did is a mere trick - as you can always search for LIKE '%%' and match all rows (excluding ones where user_name is null though).

But the rest is just a proper use of PDO features:

you can always use positional placeholders
you can always avoid bindParam() call
you should use appropriate fetch mode

edited May 14, 2016 at 8:43

answered May 14, 2016 at 8:41

Your Common Sense

158k42 gold badges226 silver badges374 bronze badges

Sign up to request clarification or add additional context in comments.

4 Comments

hjpotter92 Over a year ago

It should be an array parameter: ->execute("%$search%");

bsteo Over a year ago

Yes, I know, I just made my trainer's example less buggy.

bsteo Over a year ago

@Your Common Sense, your The only proper guide on PDO made me very critical about proper use of PDO :)

Your Common Sense Over a year ago

@bsteo I am mighty glad to hear! By the way, there is a usual flaw in your teacher's logic (excusable though, as it is shared by 95% of PHP users): the primary purpose of prepared statement is not security but a mere syntactical correctness. Add a quote to the search string, and the query will fail, even without any harmful intent. While security is rather a by-product.

Collectives™ on Stack Overflow

PHP PDO MySQL count() prepared statement

1 Answer 1

4 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

4 Comments

Your Answer

Sign up or log in

Post as a guest

Related