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.';
 }
}
}

• cHao: The big thing that jumped out at me: Why are you storing the connection info, when you use it right away in the constructor and then never touch it again? You're just begging for a misplaced/forgotten var_dump to leak your database credentials.
• Syntax Error: You mean to say this line here: $this->dbLink = mysqli_connect( $this->dbHost, $this->dbUser, $this->dbPass, $this->dbName) I'm storing the connection info in dbLink and using this variable in almost every method below sir.
• cHao: No, i mean like $this->dbHost = $host ? $host : 'localhost';, and the same for username, password, and DB name. Once you've connected, you no longer need that info; you only needed it in the first place to build $this->dbLink.
• Syntax Error: You are kind of right :| I think I should make local variable in constructor for this purpose.
• cHao: You hardly even need to do that; you already have four of them -- the parameters to the constructor -- which you already give nonsensical default values to. You could instead say public function __construct($host='localhost', $user='root', $pass='', $db='my_db_name'). (Though frankly, it'd be better to not have defaults at all. That's per-app configuration info, and belongs in a config file. And with default values, that opens up the possibility of only specifying half the settings, which wouldn't make sense.)

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.

• tim: You mention it in your comment, but your code doesn't reflect this, so just for emphasis: the table is also an attack vector for sql injection if it is user supplied.
• Syntax Error: Thank you for your time to read and provide a lot of feedback. I will take my time to read your feedback in detail and edit my code as per your suggestion, then I'll post back with questions, if any.
• Syntax Error: I took your advice: changed protected to private. put the db credentials in the constructor arguments as defaults. Are you sure $this->dbLink will contains a reference to MySQLi and not FALSE? Reading your code i think it will be FALSE. you are right, if the connection fails then i have false in dbLink and it causes so much fatal errors. how to cope with this situation?
• Syntax Error: @marco aciemo ... your advice please?
• Marco Acierno: Try to use mysqli_connect_errno, mysqli_connect_error according to PHP Dcoumentation.

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.

• Syntax Error: My bad I forgot to mention that the data will be sent here by a controller which will check for SQL injections. But please do suggest whether i should handle SQL injections here or they are better off being handled in a separate controller? Nice suggestion about hasing and execute function.
• tim: You should deal with sql injections here, not in the calling code (you cannot be sure that the caller always remembers this, and this is the logical place to do it). Also, here is what the owasp says about filtering vs prepared statements: this methodology [escaping user input] is frail compared to using parameterized queries (it goes on like this; escaping user input is NOT secure enough).
• Syntax Error: hmmmm right. I'll read this in detail and will post back for question. Thanks amigo :)