5
\$\begingroup\$

I have created this MySQLI Class. I wanted to know if this class is secure enough to be used on the production websites or more improvements can be done to this class.

<?php
// Make local variables of the mySQL variables from the Configuration file
        $db_host = DB_HOST;
        $db_user = DB_USER;
        $db_pass = DB_PASSWORD;
        $db_name = DB_NAME;

class database{
    protected $link, $results, $num_rows;   

    public function __construct($db_host, $db_user, $db_pass, $db_name){
        $this->link = mysqli_connect($db_host, $db_user, $db_pass);
        mysqli_select_db($this->link, $db_name);
    }

/* RUN ANY QUERY AGAINST THE DATABASE */
    public function QUERY($sql){
        $this->results = mysqli_query($this->link, $sql);
        $this->num_rows = mysqli_num_rows($this->results);
    }

/* RUN ONLY SELECT QUERY */
    public function SELECT($sql){
        $this->results = mysqli_query($this->link, $sql);
        $this->num_rows = mysqli_num_rows($this->results);
    }

/* RUN ONLY INSERT QUERY */
    public function INSERT($sql){
        return $this->results = mysqli_query($this->link, $sql);
    }

/* RUN ONLY UPDATE QUERY */
    public function UPDATE($sql){
        return $this->results = mysqli_query($this->link, $sql);        
    }

/* RUN ONLY DELETE QUERY */
    public function DELETE($sql){
        return $this->results = mysqli_query($this->link, $sql);        
    }

/* RETURNS COUNT OF RESULTS FROM DATABASE */
    public function NUM_ROWS(){
        return $this->num_rows;
    }

/* RETURNS RESULTS IN OBJECT FROM DATABASE */   
    public function FETCH_OBJECT(){
        $rows = array();
        for ($x = 0; $x < $this->num_rows(); $x++){
            $rows[] = mysqli_fetch_object($this->results);
        }
        return $rows;
    }

/* RETURNS RESULTS IN ASSOC FROM DATABASE */
    public function FETCH_ARRAY(){
        $rows = array();
        for ($x = 0; $x < $this->num_rows(); $x++){
            $rows[] = mysqli_fetch_array($this->results);
        }
        return $rows;
    }

/* RETURNS RESULTS IN ASSOC FROM DATABASE */
    public function FETCH_ASSOC(){
        $rows = array();
        for ($x = 0; $x < $this->num_rows(); $x++){
            $rows[] = mysqli_fetch_assoc($this->results);
        }
        return $rows;
    }

/* RETURNS RESULTS IN ROW FROM DATABASE */
    public function FETCH_ROW(){
        $rows = array();
        for ($x = 0; $x < $this->num_rows(); $x++){
            $rows[] = mysqli_fetch_row($this->results);
        }
        return $rows;
    }

/* RETURNS RESULTS IN ROW COUNT FROM DATABASE */    
    public function ROW_COUNT(){
        $rows = array();
        for ($x = 0; $x < $this->num_rows(); $x++){
            $row[] = mysqli_fetch_row($this->results);
        }
        return $row;
    }

/* RETURNS ID OF THE LAST QUERY FROM THE INSERT QUERY */    
    public function INSERT_ID(){
     return $this->results = mysqli_insert_id($this->link);
    }

/* ESCAPES DATA BEFORE INSERTING INTO DATABASE TO PREVENT INJECTIONS */
    public function ESCAPE($field){
        $field = secure_xss($field);
        return mysqli_real_escape_string($this->link,$field);
    }   
}

    function get_table( $table_name = "" ){
        global $table_prefix;
        return $table_prefix.$table_name;
    }

/* Instantiating a class */
    $db = new database($db_host, $db_user,$db_pass, $db_name);

?>
\$\endgroup\$

2 Answers 2

7
\$\begingroup\$

This is going to be harsh, continue reading at your own risk!

A few points I want to make here, that are definitely missing for this to be anywhere near production:

Security

Security in this class is nonexistant. You get a query from somewhere and run it. no checks no nothing. While in itself this could possibly even work, you definitely shouldn't do this. For one, it's open wide to SQL-Injections.

Even though you expose a function ESCAPE you never force any query to go through it. That makes it utterly useless, you could drop it directly.

Design

You have these overridden methods: SELECT, INSERT, [...] Why are you not checking what the SQL you get does?? Every single method of these is almost exactly the same. The only difference is, that SELECT and QUERY methods are doing a single thing more and not return anything.

Additionally the results you get back are simply placed into $this->results which is... slightly useless, given that the same happens when using mysqli_*-functions. If you'd want to provide extra functionality, you should really do so and not create an additional layer of clutter and boilerplate code around your database interactions.

In summary your class does too little. It might be preferrable to just drop this class and code directly against the mysqli_* functions.

Conventions

SHOUTCASE method names are ... "unconventional" (read: wrong) in php Shoutcase is usually reserved for SQL and sometimes for a class or constants. Function names are either snake_case (e.g. mysqli_insert_id) or alllowercase (e.g. strnatcasecmp). I'd also have accepted camelCase, but shoutcase is unacceptable.

Usually class-names are at least PascalCase (as seen in frameworks like Symphony, Zend, [...]), yours is lowercase :(

Also you don't support a no-args constructor or constructor overloads, anyone instantiating your database will have to call it:

$db = new database(DB_HOST, DB_USER, DB_PASSWORD, DB_NAME);

This makes the whole point of having default values for this moot.

Summa summarum:

Your class lacks a real use-case, it's insecure and violates conventions almost everywhere.

\$\endgroup\$
0
5
\$\begingroup\$

@Vogel612 already provided you with very good feedback, I just have a couple of smaller points in addition to that.

  • FETCH_ROW and ROW_COUNT do exactly the same thing, so they shouldn't both exist.
  • you shouldn't secure against XSS when inserting data into a database (XSS happens when data is printed to the user, and that is where it should be prevented).
  • if you use mysqli_real_escape_string instead of prepared statements, you have to remember to use single quotes around all variables to prevent SQL injection (and this can be harder than it seems). So I would recommend to use prepared statements (you can do this with mysqli or you can switch to PDO).
  • use consistent indentation (same amount of spaces, the deeper nested the code, the more indented, etc).

In summary, I would agree with Vogel612, your class doesn't really serve a purpose. Either use mysqli directly, use PDO, or think about what extra functionality you could really use in a class like this.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for your feedback, learnt something new about security. \$\endgroup\$
    – Red Virus
    Commented Oct 22, 2014 at 9:29
  • 1
    \$\begingroup\$ About FETCH_ROW and ROW_COUNT, I just noticed that they don't do exactly the same. I think ROW_COUNT is bugged. It initializes the array $rows, but adds to and returns $row \$\endgroup\$ Commented Oct 22, 2014 at 10:45

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.