Class to create dynamic php + mysql queries (safely)

Question

I've started to learn PHP classes, interfaces, extends and things related to those.

Before this i've worked only with functions and it started to be quite a mess with hundreads of separate functions.

What i'm trying achieve is the following:

1. Html table, which has 25 products listed. Each product has its quantity and price fields. Some fields allow price to be input and some allows only amount.

2. Session cart where the products are updated as the user changes values in html table. (Jquery + AJAX)

3. Calculations class, which would be used to create different calculation formulas based on product (Some are calculated per person price * amount and some has base prices and other calculations)

4. Database tools class, which would be used to retrieve needed prices from database.

That would give the basic idea of what i'm doing. The actual code is related to the Database classes, which are used to handle database queries and connections.

Database Connection Class

class DatabaseConnection {

    protected $db_user = '';
    protected $db_pass = '';    
    protected $db_dsn = 'mysql:host=localhost;dbname=';
    protected $options = array(
        PDO::MYSQL_ATTR_INIT_COMMAND => 'SET NAMES utf8',
        PDO::ATTR_ERRMODE => PDO::ERRMODE_WARNING,
    );  

    public function dbconnect(){
        try {
            $db = new PDO($this->db_dsn, $this->db_user, $this->db_pass, $this->options);
        }
        catch (PDOException $e) {
            echo 'Connection failed: ' . $e->getMessage();
            exit;
        }
            return $db;
        }
    }

Database tools class

class dbTools {

    private $db;
    private $verifiedTable;
    private $verifiedColumn;
    private $verifiedItemId;

    function __construct() {
        $this->db = new DatabaseConnection();
        $this->db = $this->db->dbconnect();
    }

    public function validateValues($table, $column, $itemId) {
        // Verify that table exists
        $verifiedTable = $this->verifyTable($table);
        $this->verifiedTable = $verifiedTable;

        // Verify that column exists
        $verifiedColumn = $this->verifyColumn($column, $this->verifiedTable);       
        $this->verifiedColumn = $verifiedColumn;

        // Verify that id exists
        $verifiedItemId = $this->verifyItemId($this->verifiedTable, $itemId);
        $this->verifiedItemId = $verifiedItemId;    
    }

    private function verifyTable($table) {
        $result = $this->db->query("show tables");
        $final = $result->fetchAll(PDO::FETCH_COLUMN);

        if(in_array($table, $final)) {
            $verifiedTable = $table;
        } else {
            throw new exception('Table doesn´t exists');
        }
        return $verifiedTable;
    }       

    private function verifyColumn($column, $verifiedTable) {
        $result = $this->db->query('SHOW COLUMNS FROM '.$verifiedTable.'');
        $columns = $result->fetchAll(PDO::FETCH_COLUMN);

        if(in_array($column, $columns)) {
            $verifiedColumn = $column;
        } else {
            throw new exception('Column doesn´t exist');
        }
        return $verifiedColumn;     
    }

    private function verifyItemId($verifiedTable, $itemId) {
        $result = $this->db->query('SELECT id FROM '.$verifiedTable.'');
        $ids = $result->fetchAll(PDO::FETCH_COLUMN);        

        if(in_array($itemId, $ids)) {
            $verifiedItemId = $itemId;
        } else {
            throw new exception('Id doesn´t exist');
        }
        return $verifiedItemId;
    }   

    public function getItem() { 
        $statement = 'SELECT '.$this->verifiedColumn.' FROM '.$this->verifiedTable.' WHERE id = :id';
        $query = $this->db->prepare($statement);
        $query->bindParam(':id', $this->verifiedItemId);        
        $query->execute();
        $row = $query->fetchColumn();
        return $row;
    }
}

Usage

$dbTools = new dbTools();
$dbTools->validateValues('table','column', 'id');
echo $dbTools->getItem();

I've tried to protect my queries from sql injections. For example. with verifyTable() method i'm first retrieving tables from database and creating array from those. Then i'm testing the unknown value against that array. If the value is found = the unknown value is safe.

Is this correct approach and is the above class safe from sql injections?

I would appreciate all help and comments. Thanks

Answer 1

Is this correct approach and is the above class safe from sql injections?

Yes, it's safe (against SQL injection; if you give users complete control over table and column names, they may read data you do not want them to read).

But I would worry about performance a bit. For each SELECT query you want to perform, you actually have to perform 4 queries now.

The verifyItemId looks to be the most expensive - it fetches all items in the table -, but is luckily also completely unnecessary. You use prepared statements for the id, which is enough. If you do want an additional layer of security, you could additionally filter your input (for example, you know that ids can only be integers). You could use an input class like this for example.

For the table and column check, this is a bit more difficult. Ideally, tables and columns should not be user supplied. But I have seen both in practice, so as defense in depth, it's not a bad idea to filter this at this level. I could think of two possible performance improvements:

• hardcode the table and column names, and check against that. This does mean that whenever you update your database schema, you need to update those hardcoded values as well.
• perform the lookup once, and save it. That way, you only need to perform the additional query once per request instead of once per query.

I would go with the first approach. It also gives you an additional level of security as you can filter more strictly. For example, you may not want a user to be able to read out the user table. Or if you do want to allow that, you definitely do not want them to be able to read out the password column (although this filtering likely has to take place at a higher level).

Misc

• don't echo or die in classes, it makes it hard to recover or show a custom error message. Just throw the exception.
• pass the database connection to the dbTools class instead of creating it there. This way, you could pass different connections, eg when testing.
• final isn't a great name. tables would be better.
• class names should start with an upper-case character.
• you don't need one-time variables. Eg instead of assigning $verifiedTable = $this->verifyTable($table);, just assign $this->verifiedTable = $this->verifyTable($table); directly.

You also use this structure quite often:

        if(in_array($table, $final)) {
            $verifiedTable = $table;
        } else {
            throw new exception('Table doesn´t exists');
        }
        return $verifiedTable;

You can simplify this:

        if(in_array($table, $final)) {
            return $table;
        } else {
            throw new exception('Table doesn´t exists');
        }