4
\$\begingroup\$

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)");
$db->bind(':x', $x);
$db->bind(':y', $y);
$db->bind(':z', $z); 
$db->execute();

Is there anything wrong which will affect me in future? I have searched many things for PDO and have updated my class accordingly, but I want to be sure about my project.

\$\endgroup\$

3 Answers 3

3
\$\begingroup\$

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 constructor effectively making the parameters for the connection hardcoded (or so it looks from $this->user due to lack of a constructor argument). I would extract the connection functionality into a separate class. This will also allow you to use different connections with the same database class. I would write something like the following. First I would alter the constructor in your current class.

public function __construct(PDOConnection $connection) {

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

}

Then I would write the PDOConnection class.

class PDOConnection {

    private $instance;

    private $dsn;
    private $username;
    private $password;
    private $options = [];

    public function __construct($dsn, $username, $password, array $options = []) {
        $this->dsn      = $dsn;
        $this->username = $username;
        $this->password = $password;
        $this->options  = $options;
    }

    public function setAttribute($name, $value) {

        if(!$this->instance instanceof PDO) {
            throw new LogicException('Cannot set PDO attribute. Please make sure you are connected using the connect() method.');
        }

        if($this->instance->setAttribute($name, $value) === false) {
            throw new LogicException('Could not set PDO attribute: ' . $name);
        }

    }

    public function setOption($name, $value) {
        $this->options[$name] = $value;
    }

    public function getConnection() {

        if(!$this->instance instanceof PDO) {
            throw new LogicException('No database connection established.');
        }
        return $this->instance;

    }

    public function connect() {

        try {

            $this->instance = new PDO($this->dsn, $this->username, $this->password, $this->options);

        }catch(PDOException $exception) {

            throw new ErrorException('Could not connect to the database!', null, $exception);

        }

    }

    public function disconnect() {
        $this->instance = null;
    }

}

Keep in mind this code has no error checking and not been tested, so there might be an error.

The usage of these classes would be like the following. I have named your class Database since I couldn't see the original name from your post.

$dsn  = 'mysql:host=127.0.0.1;dbname=AWESOMENESS;charset=utf8';
$user = 'username';
$pass = 'password';

$connection = new PDOConnection($dsn, $user, $pass, [
    PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
    PDO::ATTR_CURSOR  => PDO::CURSOR_FWDONLY, // Scrollable cursors may be unavailable or very expensive on MySql
    PDO::ATTR_EMULATE_PREPARES => false,
    PDO::ATTR_PERSISTENT => false
]);

$connection->connect();

$database = new Database($connection);

I hope this can help guide you in the right direction, happy coding!

\$\endgroup\$
0
\$\begingroup\$

I don't see anything wrong with your code as long as you are using PHP 5.3.6 or newer. See the information referenced here for more information about what settings of PDO are insecure in previous versions of PHP. Specifically, your "SET NAMES" call will be insecure because of problems in PDO and will open you up to SQL injection based on character set issues.

\$\endgroup\$
2
  • \$\begingroup\$ but i heard that charset=utf8 in character encoding in the DSN string is not supported by some version.so what is the safest method ? i have done that also see my class. \$\endgroup\$
    – dabhi4ever
    Feb 24, 2015 at 7:13
  • \$\begingroup\$ DSN is not supported in the versions of php that SET NAMES causes problems for. That is the problem. For PHP versions <5.3.6 you need to use the suggestions in my link ie Never use SET NAMES, always specify the character encoding in the DSN. On PHP < 5.3.6, there is no safe way to change the character encoding on client side. If you need another encoding than latin1, change the MySQL configuration on the server. Set PDO::ATTR_EMULATE_PREPARES to false, so that you'll get actual prepared statements instead of "emulated" ones, which can be vulnerable to escaping issues \$\endgroup\$ Feb 24, 2015 at 8:33
0
\$\begingroup\$

I haven't got a lot of time ATM, so for now, I'll just share my initial thoughts on your code. I'll be updating and expanding on this answer later today.

Your worries:

"I've read somewhere that PDO escape alone is not secure"

Well, if by "PDO escape" you mean PDO's prepared statements (both emulated and non-emulated), there are some caveats to take into consideration. Yes, prepared statements are the easiest way to prevent most types of injection attacks, but they're not magic.
Injection vulnerabilities using prepared statements have been well documented before. see this post for example, it explains how these attacks work, and how to prevent them.

The bottom line is: use the charset=utf8 (or some other collation) in your DSN string you pass to the PDO constructor. If you're working on an outdated PHP version, you'll have to use PDO::MYSQL_ATTR_INIT_COMMAND in the options array instead. But it's better to upgrade your PHP version anyway.

"I'm a little confused about my PDO class and usage"

I can understand that. That's probably because your class isn't a PDO class as such. Your class looks like it's something of a wrapper around PDO, but at the same time, you're using it as a wrapper around the resultsets (which are represented by PDOStatement instances). Effectively, you're combining 2 classes into a wrapper class, that pretends to wrap just one class. That's bad practice, seeing as it's a violation of the Single Responsability Principle (SRP).

The idea behind the SRP is simply this: a class should have just one job. In the case of PDO, the PDO instance represents a connection to the database. a PDOStatement instance represents a query in some form or another (either a prepared statement - yet to be executed) or a query's result set. Your class seemingly does both. Worst of all, it assigns each query to the same property ($this->stmt), so your class is incapable of handling more than one query at a time. Here's a scenario, which is easily tackled using PDO as-is:

$db = new PDO($dsn, $usr, $pass, $options);
try {
    $db->beginTransaction();
    $stmt = $db->prepare('INSERT INTO tbl VALUES (:foo, :bar)');
    $refStmt = $db->prepare('UPDATE pending SET inserted = 1 WHERE foo = :foo AND bar = :bar');
    foreach ($data as $bind) {
        $stmt->execute($bind);
        $stmt->closeCursor();
        $refStmt->execute($bind);
        $refStmt->closeCursor();
    }
    $db->commit();//persist to db
} catch (PDOException $e) {
    if ($db instanceof PDO and $db->inTransaction()) {
        $db->rollBack();//undo changes
    }
    throw new RuntimeException(
        'Transaction failed',
        0,
        $e
    );
}
$db = null;//disconnect
echo 'Transaction OK';

This type of work is impossible to do using your class. It's just too limiting.

Tl;Tr Don't wrap/extend PDO

I've been very vocal about this on numerous occasions. Read one of my older reviews here where I explain why a PDO wrapper/child class is not-done.

Other issues:

  • As always: please adhere to the coding standards as much as possible. The opening { of a method, for example, belongs on the next line. It may seem silly, but standards Matter, trust me.
  • A constructor that creates a PDO instance should take arguments. Your constructor uses $this->host etc... to create the DB connection, without the user (the person creating the instance) being able to choose what DB to connect to.
  • An excpetion should be caught by code that can handle it. You just catch a PDOException in your constructor, set the $this->error property, and assume the caller will think to check the value of this property. Don't assume things like that. The exception was raised, and so the caller must be notified. Don't catch the exception, let it fly.
  • Seeing as your constructor catches the exception, it's very possible that the user will call methods, like query on the instance, without $this->dbh being set. That's messy. Just check, and make sure the properties you use are set, and that their values are what you expect them to be. Consider the following:

Assuming your class is called Database:

$db = new Database();
//$db->error is set, PDO failed to connect
//but I forget to check
$db->query('SELECT id FROM table');//error, I shouldn't be allowed to get this far

or worse still:

$db = new Database();
//all goes well
$db->execute();//$this->stmt is not an instance of PDOStatement!

To be continued...

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.