Database Handler Class

Question

I've written this DB Handler class. Please review it and suggest any code edits or point out mistakes and security loop holes. Please also suggest a better way to handle things, if there is a security loop hole in there.

class db {

protected $dbHost;
protected $dbUser;
protected $dbPass;
protected $dbName;
protected $dbLink;

public function __construct ( $host=NULL, $user=NULL, $pass=NULL, $db=NULL ){

    $this->dbHost = $host? $host : 'localhost';
    $this->dbUser = $user? $user : 'root';
    $this->dbPass = $pass? $pass : '';
    $this->dbName = $db? $db : 'my_db_name';

    if( !($this->dbLink = mysqli_connect( $this->dbHost, $this->dbUser, $this->dbPass, $this->dbName) ) ){
        echo 'Error While Connection to DB: '.mysqli_error( $this->dbLink );
    }
}

public function __destruct(){
    if( ! mysqli_close($this->dbLink) ){
        echo 'Error While Closing Connection to DB: '.mysqli_error( $this->dbLink );
    }
}





/********************************************************************************

    1) This function is used to fetch data

    @param: $tableName [required]
    @param: $requiredColumns. This is an array like 'tableColumn', 'tableColumn', 'tableColumn'
    @param: $conditionsArray. This is an associative array like 'tableColumn' => 'value to check'
    @param: $orderBy. This is an array like 'tableColumn', 'ASC'

*********************************************************************************/
public function select( $tableName, $requiredColumns = NULL, $conditionsArray = NULL, $orderBy = NULL ){
    if( $requiredColumns == NULL || count($requiredColumns) <= 0 ){
        $col = '*';
    }
    else{
        $col = implode(', ', $requiredColumns);
    }

    $query = 'SELECT '.$col.' FROM `'.$tableName.'`';
    if( $conditionsArray != NULL ){
        $args = array();
        foreach( $conditionsArray as $key => $value ){
            array_push( $args , '`'.$key.'` = "'.$value.'"' );
        }
        //var_dump( $args );
        $where = implode( ' AND ' , $args );
        $query .= ' WHERE ' . $where;
    }
    if( $orderBy != NULL ){
        $order = implode(' ', $orderBy);
        $query .= 'ORDER BY '.$order;
    }
    //echo $query;
    return $this->execute( $query , 'resource' );
}





/********************************************************************************

    2) This function is used to insert record

    @param: $tableName [required]
    @param: $data [required] This is an associative array like 'tableColumn' => 'value to insert'

*********************************************************************************/
public function insert( $tableName, $data ){
    if( $data != NULL && count($data) > 0 ){
        $cols = ''; $vals = '';
        foreach( $data as $key => $val ){
            $cols .= $key.', ';
            $vals .= "'".$val."', ";
        }
        $cols = rtrim( $cols , ", ");
        $vals = rtrim( $vals , ", ");
        $query = "INSERT INTO `".$tableName."` (".$cols.") VALUES (".$vals.")";
        return $this->execute( $query, true ) ;
    }
}





/********************************************************************************

    3) This function is used to Update records

    @param: $tableName [required]
    @param: $data [required] This is an associative array like 'tableColumn' => 'value to insert'
    @param: $conditionsArray. This is an associative array like 'tableColumn' => 'value to check'

*********************************************************************************/
public function update( $tableName, $data, $conditionsArray ){
    if( $data != NULL && count($data) > 0 ){
        $args = array();
        foreach( $data as $key => $value ){
            array_push( $args , '`'.$key.'` = "'.$value.'"' );
        }
        $dataString = implode( ', ' , $args );
        $arr = array();
        foreach( $conditionsArray as $key => $value ){
            array_push( $arr , '`'.$key.'` = "'.$value.'"' );
        }
        $where = implode( ' AND ' , $arr );
        $query = 'UPDATE `'.$tableName.'` SET '.$dataString.' WHERE '.$where;
        return $this->execute( $query );
    }
}





/********************************************************************************

    4) This function is used to check number of records

    @param: $tableName [required]
    @param: $conditionsArray. This is an associative array like 'tableColumn' => 'value to check'

*********************************************************************************/
public function numRows( $tableName, $conditionsArray = NULL ){
    if( $conditionsArray == NULL ){
        $query = 'SELECT 1 FROM '.$tableName;
    }
    else{
        $args = array();
        foreach( $conditionsArray as $key => $value ){
            array_push( $args , '`'.$key.'` = "'.$value.'"' );
        }
        //var_dump( $args );
        $where = implode( ' AND ' , $args );
        $query = 'SELECT 1 FROM `'.$tableName.'` WHERE ' . $where;
    }
    //echo $query;
    $resource = $this->execute( $query , 'resource' );
    return mysqli_num_rows( $resource );
}





/********************************************************************************

    5) This function is used to DELETE

    @param: $tableName [required]
    @param: $conditionsArray [required] This is an associative array like 'tableColumn' => 'value to check'

*********************************************************************************/
public function delete( $tableName, $conditionsArray ){
    if( $conditionsArray != NULL ){
        $args = array();
        foreach( $conditionsArray as $key => $value ){
            array_push( $args , '`'.$key.'` = "'.$value.'"' );
        }
        //var_dump( $args );
        $where = implode( ' AND ' , $args );
        $query = 'DELETE FROM `'.$tableName.'` WHERE ' . $where;
    }
    return $this->execute( $query );
}





/********************************************************************************

    6) This function is used to login

    @param: $tableName [required]
    @param: $conditionsArray. This is an associative array like 'tableColumn' => 'value to check'

*********************************************************************************/
public function login( $user, $pass ){
    $user = mysqli_real_escape_string( $this->dbLink, $user );
    $pass = mysqli_real_escape_string( $this->dbLink, $pass );
    $data = array( 'username' => $user, 'password' => md5($pass) );
    if( $this->numRows( 'users' , $data ) > 0 ){
        $result = $this->select( 'users' , NULL , $data );
        return mysqli_fetch_assoc( $result );
    }
    else{
        return NULL;
    }
}





/********************************************************************************

    7) This function is used to perform custom query

    @param: $query [required]

*********************************************************************************/
public function query( $query ){
    return $this->execute( $query );
}





/********************************************************************************

    8) This function is used to execute the queries

    @param: $query [required]
    @param: $return_type = 'bool' , 'resource' , 'id'

*********************************************************************************/
private function execute( $query , $return_type = 'bool' ){ 
    if( !empty($query) ){
        switch( $return_type ){
            case 'id' :
                if( mysqli_query( $this->dbLink , $query ) ){
                    return mysqli_insert_id( $this->dbLink );
                }
                else{
                    die(mysqli_error( $this->dbLink ));
                    return 0;
                }
                break;

            case 'resource' :
                if( $rs = mysqli_query( $this->dbLink , $query ) ){
                    return $rs;
                }
                else{
                    die(mysqli_error( $this->dbLink ));
                    return false;
                }
                break;

            case 'bool' :
                if( mysqli_query( $this->dbLink , $query ) ){
                    return true;
                }
                else{
                    die(mysqli_error( $this->dbLink ));
                    return false;
                }
                break;

            default :
                echo 'Invalid 2nd parameter (return format)';
                break;
        }
    }
    else{
        echo 'Empty query not allowed.';
    }
}

}

Answer 1

protected $dbHost;
protected $dbUser;
protected $dbPass;
protected $dbName;
protected $dbLink;

Why protected? You except subclasses? Even with subclasses, why they could need to work with this fields?

The name db is sooo ugly. Database is better (even if too generic, but hey...)

$this->dbHost = $host? $host : 'localhost';
$this->dbUser = $user? $user : 'root';
$this->dbPass = $pass? $pass : '';
$this->dbName = $db? $db : 'my_db_name';

They are optional, why not put it directly in the constructor line?

public function __construct ( $host=NULL, $user=NULL, $pass=NULL, $db=NULL ){

will be

public function __construct ( $host="localhost", $user="root", $pass="", $db="my_db_name"){

Here

if( !($this->dbLink = mysqli_connect( $this->dbHost, $this->dbUser, $this->dbPass, $this->dbName) ) ){
    echo 'Error While Connection to DB: '.mysqli_error( $this->dbLink );
}

Why echo the error when you can throw the error?

throw new LogicException("Unable to connect to Database, error: " . mysqli_error ( $this->dbLink ));

Are you sure $this->dbLink will contains a reference to MySQLi and not FALSE? Reading your code i think it will be FALSE.

The same for __destruct.

/********************************************************************************

1) This function is used to fetch data

@param: $tableName [required]
@param: $requiredColumns. This is an array like 'tableColumn', 'tableColumn', 'tableColumn'
@param: $conditionsArray. This is an associative array like 'tableColumn' => 'value to check'
@param: $orderBy. This is an array like 'tableColumn', 'ASC'

*****************************************************************************/ Why so much ********** this just looks ugly.

Here

$query = 'SELECT '.$col.' FROM `'.$tableName.'`';

you can't use ? but you should filter your $col to be sure that it will not include any special char which could do any sql injection. The same for $tableName, in a project i used a an array where i saved all the table names which i allow to use, it the table is not in this list i throw an Exception. You could do something similar maybe?

The same for arguments, i suppose it's dynamic i was in a situations like yours but to avoid sql injection i still used mysqli_prepare (even if it doesn't allows you to use arrays)

It looks like this

public function commit($connection) {
    $finalParams = array ();

    $query = "UPDATE " . $this->table . " SET ";
    $paramTypes = "";

    if (count($this->mapChanges) == 0) {
        return 0;
    }

    foreach($this->mapChanges as $value) {
        $paramTypes .= $this->convertType($value);
    }

    // it will be the ID field
    $paramTypes .= "i";

    $finalParams[] = &$paramTypes;

    foreach($this->mapChanges as $column => &$value) {
        $query .= $column . " = ?,";
        $finalParams[] = &$value;
    }
    unset($value);

    //l'id da aggiornare
    $finalParams[] = &$this->id;

    // ultima "," sostituita con uno spazio vuoto
    $query[strlen($query) - 1] = " ";

    $query .= " WHERE id = ?";

    $stmt = $connection->prepare($query);

    if ($stmt === false) {
        return -1;
    }

    call_user_func_array(array($stmt, 'bind_param'), $finalParams);
    $stmt->execute();
    $rows = $stmt->affected_rows;
    $stmt->close();

    return $rows;
}

private function convertType($value) {
    $type = gettype($value);

    switch ($type) {
        case "boolean":
        case "integer":
            return "i";
        case "double":
            return "d";
        case "string":
            return "s";
        default:
            return "b";
    }
}

It's ugly, but does the job. It allows you to use dynamic arguments. Edit it if you want.

You could refactor your execute and avoid all this if blocks, something like

private function execute( $query , $return_type = 'bool' ){ 
    if (empty($query)) {
        echo "Empty query not allowed.";
        return false;
    }

    switch( $return_type ){
        case 'id' :
            if( mysqli_query( $this->dbLink , $query ) ){
                return mysqli_insert_id( $this->dbLink );
            }
            else{
                die(mysqli_error( $this->dbLink ));
                return 0;
            }
            break;

        case 'resource' :
            if( $rs = mysqli_query( $this->dbLink , $query ) ){
                return $rs;
            }
            else{
                die(mysqli_error( $this->dbLink ));
                return false;
            }
            break;

        case 'bool' :
            if( mysqli_query( $this->dbLink , $query ) ){
                return true;
            }
            else{
                die(mysqli_error( $this->dbLink ));
                return false;
            }
            break;

        default :
            echo 'Invalid 2nd parameter (return format)';
            break;
    }

}

Again, don't use echo to print errors but exception or specific return values.

die(mysqli_error( $this->dbLink ));
return false;

You die, the execution will stop. Inside an API, why you let my script die? I will do the die if i think i need to die. And stop echo's your errors. Noone wants to see your errors messages in the HTML page.

Answer 2

Security

This code is not secure.

SQL Injection

You are trusting the user input completely (except in the login function where you do use mysqli_real_escape_string, which is not secure enough). Use prepared statements instead. You should also read up on SQL injections in general.

Other

This is all minor in comparison to the SQL injection.

• don't hardcode your password in the PHP code, store it in an external configuration file (outside the web root as otherwise your passwords will be exposed).
• don't echo complete errors to the user. Use a custom error string instead.
• md5 is not a good enough hashing function, use something like bcrypt instead. (see for example here and here)

Code

query function

The query function just renames the execute function, and is thus useless. Also, you are not even using it. I would just remove it (and make execute public if you need to).

I think that you should fix the SQL injection issue before the rest of the code is reviewed, as it will change in a lot of places.