patternphpModerate
Database Handler Class
Viewed 0 times
databasehandlerclass
Problem
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.
``
}
//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' );
``
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) $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' );
Solution
Security
This code is not secure.
SQL Injection
You are trusting the user input completely (except in the
Other
This is all minor in comparison to the SQL injection.
Code
The
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.
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.
md5is not a good enough hashing function, use something likebcryptinstead. (see for example here and here)
Code
query functionThe
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.
Context
StackExchange Code Review Q#61231, answer score: 11
Revisions (0)
No revisions yet.