Safe and secure Object Oriented insertion with PDO

Question

Safe and secure Object Oriented insertion with PDO

is this code secure against SQL injection?, it uses prepared and parametrized statement. if not then what should i do because i only want to use it via Object Oriented procedure where i can insert column name and column values.

    <?php

        class CommunItY
        {
            const community_host = "localhost";
            const community_db = "DB";
            const db_username = "root";
            const db_password = "";
            private $conn = null;
            public $trace = "";

            public function insert($table ,$values = array())
            {
            try{
                foreach ($values as $field => $v)
                {
                    $ins[] = ':' . $field;
                }
                $ins = implode(',', $ins);
                $fields = implode(',', array_keys($values));
                $sql = "INSERT INTO $table ($fields) VALUES ($ins)";  
                $ready = $this->conn->prepare($sql);
                foreach ($values as $f => $v)
                {
                    $ready->bindValue(':' . $f, $v);
                }
                $ready->execute();
            }
            catch(Exception $e){
            $this->trace .= " • insertion error • ". $e->getMessage();
            }
            }//end of method

        public function __construct(){
        $connectionString = sprintf("mysql:host=%s; dbname=%s; charset=utf8", 
                                CommunItY::community_host, CommunItY::community_db);
            try {
                $this->conn = new PDO($connectionString, CommunItY::db_username, CommunItY::db_password);
                $this->conn->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);    
                $this->conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
            } //end of connection by PDO
            catch(PDOException $e){
            $this->trace .= " • ". $e->getMessage();
            }
        }//end of construct

        public function __destruct(){
        $this->conn = null; //close connection
        } //end of destruct

    }

calling...

    $call = new Contact()
    $call->insert(table_x, array('col1' => 'value1', 'col2' => 'value2'));

Answer 1

Your code is probably not affected by SQL Injection. But there is one flaw. You should not allow any kind of table names or field names. A better solution would be one class for each object that you want to represent. Let's say you have apples and bananas as important part of your application. Then you want to create a class Apple and a class Banana, that each represent one banana or apple entry in the database.

For the actual editing of the database you could create a class AppleEditor or BananaEditor respectively that is able to update an apple or a banana. That way you have a fixed set of fields and only the values are variable.

Coming back to your code: I would not use a const for the database credentials. Any script has thereby access to them. It's better to use them in a private static variable.

Then there is the issue with the class name. Down in the call section you have a class Contact but at the top your class is named CommunItY. To have a minimal code setup, I'd recomment the following:

class Contact
{
    private static $community_host = "localhost";
    private static $community_db = "DB";
    private static $db_username = "root";
    private static $db_password = "";

    /**
     * the database connection
     * @var \PDO
     */
    private $conn = null;
    private $trace = "";

    /**
     * name of the contact
     * @var string
     */
    private $name = '';

    /**
     * address of the contact
     * @var string
     */
    private $address = '';

    private $contactID = 0;

    public function __construct($id) {
        $this->connectToDatabase();
        $this->contactID = intval($id);
    }//end of construct

    /**
     * Updates a contact.
     * 
     * @param string $name
     * @param string $address
     */
    public function update($name, $address)
    {
        $name = trim($name);
        $address = trim($address);
        try{
            $sql = "UPDATE contact SET name = ?, address = ? WHERE contactID = ?";
            $ready = $this->conn->prepare($sql);
            $ready->bindValues(1, $name, \PDO::PARAM_STR);
            $ready->bindValue(2, $address, \PDO::PARAM_STR);
            $ready->bindValue(3, $this->contactID, \PDO::PARAM_INT);
            $ready->execute();
        }
        catch(\PDOException $e){
            $this->trace .= " • insertion error • ". $e->getMessage();
        }
    }//end of method



    public function __destruct(){
        $this->conn = null; //close connection
    } //end of destruct


    private function connectToDatabase() {
        $connectionString = sprintf("mysql:host=%s; dbname=%s; charset=utf8",
            self::$community_host, self::$community_db);
        try {
            $this->conn = new \PDO($connectionString, self::$db_username, self::$db_password);
            $this->conn->setAttribute(\PDO::ATTR_EMULATE_PREPARES, false);
            $this->conn->setAttribute(\PDO::ATTR_ERRMODE, \PDO::ERRMODE_EXCEPTION);
        } //end of connection by PDO
        catch(\PDOException $e){
            $this->trace .= " • ". $e->getMessage();
        }
    }
}

$call = new Contact(1);
$call->update('name', 'address');

This is of course not a perfect situation. But it shows how it could be handled.

Answer 2

Probably not. You have introduced injection in the field names. If you can guarantee the field names are always generated by your code and never from an outside source, that might be OK. But this is a public method. So any bit of code in the system might try to do something "smart" and end up opening up a hole by passing a parameter to your insert method.

You should escape your incoming field names by putting them in quotes (or if you are using MySQL and haven't enabled ANSI compatibility, use backtics). You also must escape any quotes within the name.

$fields = implode(',', array_map(function($name) {
    return '"' . str_replace('"', '""', $name) . '"' ;
}, array_keys($values)));

For the values, you should just use positional paramters (?) or make up your own names. I don't think PDO has a mechanism for escaping the :bind_param names.

Also note that because of PHP's terrible handling of characters outside of the 7-bit ASCII range, this may still not be 100% safe if anyone starts mucking around with the internal byte encoding of strings. The only way to be safe in that case is to first ensure the field names only contain expected characters or validate them against a well-known list of field names (perhaps by using the INFORMATION_SCHEMA to inspect the table columns).