HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

PDO class and security

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
andsecurityclasspdo

Problem

I have started a project in PHP with PDO and I'm almost done, but I've read somewhere that PDO escape alone is not secure and we have to consider some settings of PDP. I am a little confused about my PDO class and usage.

public function __construct(){
    //set DNS
    $dns = 'mysql:host=' . $this->host . ';dbname=' . $this->dbname.';charset=utf8';
    //set options
    $options = array(PDO::ATTR_PERSISTENT => true, PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION, PDO::MYSQL_ATTR_INIT_COMMAND => "SET NAMES utf8",PDO::ATTR_EMULATE_PREPARES => false );
    //create a new PDO instance
    try{
        $this->dbh = new PDO($dns, $this->user, $this->pass, $options);
    } //catch any errors
    catch(PDOException $e){
        $this->error = $e->getMessage();
    }
}

public function query($query){
    $this->stmt = $this->dbh->prepare($query);
}

public function bind($param, $value, $type = null){
    if(is_null($type)){
        switch(true){
            case is_int($value):
                $type = PDO::PARAM_INT;
                break;
            case is_bool($value):
                $type = PDO::PARAM_BOOL;
                break;
            case is_null($value):
                $type = PDO::PARAM_NULL;
                break;
            default:
                $type = PDO::PARAM_STR;
        }
    }
    $this->stmt->bindValue($param, $value, $type);
}

public function execute(){
    return $this->stmt->execute();
}

public function resultset(){
    $this->execute();
    return $this->stmt->fetchAll(PDO::FETCH_OBJ);
}

public function single(){
    $this->execute();
    return $this->stmt->fetch(PDO::FETCH_OBJ);
}

public function rowCount(){
    return $this->stmt->rowCount();
}


Queries:

$db->query("SELECT * FROM XYZ WHERE some=:some");
$db->bind(':some', $somevar);
$result= $db->resultset();   ///for select

$result= $db->single();  ///if we want single row


For inserting and updating:

```
$db->query("INSERT INTO XYZ (x,y,z) VALUES (:x,:y,:z)");
$d

Solution

There are a few things which I consider required. First of all I think your code does a good job. Below I have written some of the things I would also implement.

First of all. Be careful when echoing exception message directly. Especially database exceptions has a tendency to contain connection credentials. Imagine an error happens in your application and a malicious user sees this message. He could then log-in onto your database and wreck havoc! Try to keep the amount of sensitive information out of exception messages users are able to see.

From what I can see your class wraps a PDO instance to make certain tasks easier. When you wrap your PDO instance inside a class your lose some of PDO capabilities. As far as I can see from the code I have no way to perform a transaction. This may not be useful to you now due to an older MySql version, but when you sometimes in the future upgrade (I hope you do :D) this might be desired. I would provide a way to interact with the PDO instance from outside the class. A simple pdo() method would solve that problem.

/**
 * Provides access to the application PDO instance.
 *
 * @return \PDO
 */
public function pdo() {
    return $this->dbh;
}


When looking at your execute, resultset and single methods I see three flaws. The first being a potential double execution of your query. Imagine you sometime in the future call the execute method. Then you follow that by calling the resultset or single method. Now you have executed the query twice. This may not cause bugs when you are fetching data, but if the query is an insert statement, which populates an unique column, you will get an SQL error. I would remove the calls to the PDO execute method inside the resultset and single methods. This would effectively eliminate all possible bugs of this kind. Keep in mind you should also perform some error checking to see if the current statement has been executed or you would otherwise get an error. This could be solved by setting a flag indicating if the statement has been executed.

private $executed = false;

public function execute() {
    $this->stmt->execute();
    $this->executed = true; // Set the flag.
}


The second flaw is that you are unable to close the cursor for the current statement. By closing the cursor you gain the ability to execute the statement once more with different parameters. You would also gain some (small) performance increase on huge queries. Take a look at the documentation for the closeCursor method.

public function closeCursor() {
    $this->stmt->closeCursor();
    $this->executed = false; // Reset the execution flag.
}


The third flaw which I really think is required regards your resultset and single methods once more. By default they will fetch the result-set into an object. This is a good practice when writing OOP code, but often you will find yourself fetching result sets into specific classes. Consider passing the data directly into an User class or passing an object of StdClass into the user class, so the user class can fetch the data. By passing directly into the class you save some performance and make the code more readable. I would also change the code, so you are able to fetch the result set as an array. Sometimes this may be preferred and you cannot predict the future. You could change the method signature of resultset (and single too) to something like the following:

/**
 * Fetch entire result-set of the current query.
 *
 * @param integer     $mode  A PDO constant declaring the fetch mode.
 * @param string|null $class A qualified class name to fetch the result into.
 * @param array       $args  Constructor arguments for the custom class.
 *
 * @return mixed Returns the result set according to the specified fetch mode. 
 *               The default mode is to fetch an object of type StdClass.
 */
public function resultset($mode = PDO::FETCH_OBJ, $class = null, array $args = []) {

    if(!is_null($class) && in_array($mode, [PDO::FETCH_CLASS, PDO::FETCH_OBJ])) {

        return $this->stmt->fetchAll(PDO::FETCH_CLASS; $class, $args);

    }

    return $this->stmt->fetchAll($mode);

}

/**
 * Fetch one row of the current query.
 *
 * @param integer     $mode  A PDO constant declaring the fetch mode.
 * @param string|null $class A qualified class name to fetch the result into.
 * @param array       $args  Constructor arguments for the custom class.
 *
 * @return mixed Returns the result set according to the specified fetch mode. 
 *               The default mode is to fetch an object of type StdClass.
 */
public function single($mode = PDO::FETCH_OBJ, $class = null, array $args = []) {

    if(!is_null($class) && in_array($mode, [PDO::FETCH_CLASS, PDO::FETCH_OBJ])) {

        return $this->stmt->fetchObject($class, $args);

    }

    return $this->stmt->fetch($mode);

}


There is also another thing which I think is important. You are creating your PDO connection in the cons

Code Snippets

/**
 * Provides access to the application PDO instance.
 *
 * @return \PDO
 */
public function pdo() {
    return $this->dbh;
}
private $executed = false;

public function execute() {
    $this->stmt->execute();
    $this->executed = true; // Set the flag.
}
public function closeCursor() {
    $this->stmt->closeCursor();
    $this->executed = false; // Reset the execution flag.
}
/**
 * Fetch entire result-set of the current query.
 *
 * @param integer     $mode  A PDO constant declaring the fetch mode.
 * @param string|null $class A qualified class name to fetch the result into.
 * @param array       $args  Constructor arguments for the custom class.
 *
 * @return mixed Returns the result set according to the specified fetch mode. 
 *               The default mode is to fetch an object of type StdClass.
 */
public function resultset($mode = PDO::FETCH_OBJ, $class = null, array $args = []) {

    if(!is_null($class) && in_array($mode, [PDO::FETCH_CLASS, PDO::FETCH_OBJ])) {

        return $this->stmt->fetchAll(PDO::FETCH_CLASS; $class, $args);

    }

    return $this->stmt->fetchAll($mode);

}

/**
 * Fetch one row of the current query.
 *
 * @param integer     $mode  A PDO constant declaring the fetch mode.
 * @param string|null $class A qualified class name to fetch the result into.
 * @param array       $args  Constructor arguments for the custom class.
 *
 * @return mixed Returns the result set according to the specified fetch mode. 
 *               The default mode is to fetch an object of type StdClass.
 */
public function single($mode = PDO::FETCH_OBJ, $class = null, array $args = []) {

    if(!is_null($class) && in_array($mode, [PDO::FETCH_CLASS, PDO::FETCH_OBJ])) {

        return $this->stmt->fetchObject($class, $args);

    }

    return $this->stmt->fetch($mode);

}
public function __construct(PDOConnection $connection) {

    $this->dbh = $connection->getConnection();

}

Context

StackExchange Code Review Q#82454, answer score: 3

Revisions (0)

No revisions yet.