0
\$\begingroup\$

I have written my own CMS for learning, and have included all of my core files below.

This is my first time playing with OOP and prepared statements, so I imagine there may be alot of changing. There is one problem I am having, when I try and check if a username/email already exists in the register function by using either num_rows or affected_rows, I get a non-object error for the two lines that are counting the rows.

dbc class (Database connection, functions, etc):

<?php
/**
* class dbc
* @package Core
*
*/

// Sanity check. Does this class already exist?
if(!class_exists('dbc')) {
class dbc {
    public function __construct() {
        // Make the constants class variables
        $this->host = host;
        $this->username = username;
        $this->password = password;
        $this->database = database;
        $this->charset = charset;

        $this->connection = new mysqli($this->host, $this->username, $this->password, $this->database);

        // Database connection error?
        if($this->checkConnection() == false) {
            die('Database connection error! <br />' . $this->connection->connect_error);
            return false;
        }

        $this->connection->set_charset(charset);
    }

    // Close the MySQLi connection
    public function __destruct() {
        if($this->connection) {
            $this->connection->close();
        }
    }

    public function select($columns = array(), $table, $variables = '', $order = '') {
        // Are the required variables empty or equals null?
        if(empty($columns) || empty($table)) {
            return false;
        }

        // Set $order and $variables to false
        if($order == '' || $order == null) {
            $order = false;
        }else if($variables == '' || $variables == null) {
            $variables = false;
        }

        // Check if $columns is an array
        if(!is_array($columns)) {
            return false;
        }

        // Set $fields as an array
        $fields = array();
        // Individualise the array
        foreach($columns as $field) {
            $fields[] = $field;
        }

        // Sepeate all individuals with commas
        $fields = implode(', ', $fields);

        // Finalise the MySQLi query
        if($variables == false && $order == false) {
            $query = "SELECT $fields FROM $table";
        }else if($variables != false && $order == false) {
            $query = "SELECT $fields FROM $table WHERE $variables";
        }else if($variables == false && $order != false) {
            $query = "SELECT $fields FROM $table ORDER BY $order";
        }else if($variables != false && $order != false) {
            $query = "SELECT $fields FROM $table WHERE $variables ORDER BY $order";
        }

        // Prepare the query for executing
        $stmt = $this->connection->prepare($query);

        // Execute the query
        $stmt->execute();

        // Get the results of the query
        $result = $stmt->get_result();

        // Set the results to a variable
        while($row = $result->fetch_assoc()) {
            $results[] = $row;
        }

        // Return the results as array 0
        return $results[0];
    }

    public function insert($table, $variables = array()) {
        // Are the required variables empty or equals null?
        if(empty($table) || empty($variables)) {
            return false;
        }

        // Set both variables to an array
        $fields = array();
        $values = array();
        // Divide each value as a singular value
        foreach($variables as $field => $value) {
            $fields[] = $field;
            $values[] = "'" . $value . "'";
        }

        // Split the fields & values with commas - to follow the mysqli query format
        $fields = '(' . implode(', ', $fields) . ')';
        $values = '(' . implode(', ', $values) . ')';

        // Finalised Query
        $query = " INSERT INTO $table $fields VALUES $values";

        // Prepare the query for executing
        $stmt = $this->connection->prepare($query);

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

        // Execute the prepared query
        $stmt->execute();
    }

    public function checkConnection() {
        if($this->connection->connect_errno) {
            // Connection Unsucessful
            return false;
        }else {
            // Connection Successful
            return true;
        }
    }

    public function affected() {
        return $this->connection->num_rows;
    }
}
}

?>

Config (Core website settings):

<?php

// Database Connection Information (removed for security)
define('host', '');
define('username', '');
define('password', '');
define('database', '');

// Show PHP/MySQLi errors?
define('DEBUG_MODE', true);

define('charset', 'UTF-8');

define('default_user_group', '1')

?>

User class (Register, login, etc for users):

<?php
/**
* @package Core
*/

if(!class_exists('user')) {
class user {
    public function __construct() {}

    public function register($username, $password, $confirmpassword, $email, $firstname, $lastname) {
        if(empty($username) || empty($password) || empty($confirmpassword) || empty($email) || empty($firstname) || empty($lastname)) {
            return false;
        }

        if($password === $confirmpassword) {
            $dbc = new dbc();

            $hash = '_HG.g2Sxa.';
            $encryptpassword = md5(md5($hash . $password . $hash));

            $status = default_user_group;
            if(!empty($_SERVER['HTTP_CLIENT_IP'])) {
                $ip = $_SERVER['HTTP_CLIENT_IP'];
            }else if(!empty($_SERVER['HTTP_X_FORWARDED_FOR'])) {
                $ip = $_SERVER['HTTP_X_FORWARDED_FOR'];
            }else {
                $ip = $_SERVER['REMOVE_ADDR'];
            }

            $ip = ip2long($ip);

            $userData = array(
                'username' => $username,
                'password' => $encryptpassword,
                'email' => $email,
                'firstname' => $firstname,
                'lastname' => $lastname,
                'status' => $status,
                'ip' => $ip
            );

            $createUser = $dbc->insert('users', $userData);
        }
    }
}
}
?>
\$\endgroup\$
0

1 Answer 1

6
\$\begingroup\$

Security

SQL Injection

You are completely open to SQL injection.

It is not enough to simply use prepare. The important part is that you should never put variables directly into the query. The query should only contain placeholders (either :key or ?). The variables are then later bound to the query.

Hashing

md5 is not good enough for password hashing, and it hasn't been for at least 15-20 years. It's broken, and it's too fast. Use bcrypt instead (PHP offers this via password_hash).

Additionally, a site-wide salt (also called pepper) is not enough. You want a per-user salt. Luckily, password_hash manages this for you.

Misc

  • if (cond) return false else return true can be written as return !cond.
  • don't die in classes or functions. It makes it impossible for the calling code to recover. Throw an exception instead.
  • nothing after die will be executed, so returning something afterwards doesn't make sense.
  • why is your pepper called hash? It's quite confusing.
  • passwords are hashed, not encrypted, so encryptpassword should be passwordHash.
  • either use snake_case or camelCase, but don't mix both, and don't use neither (multi-word variables written in all lowercase are hard to read).
  • if the password and the confirmpassword are unequal, you just silently fail.
  • your error handling in general could be improved. For example, you do not check the result of an insert.
  • things that are named the same should work the same. In your code, eg variables works very differently for select and insert, which will make using your code very difficult, especially as you have no documentation.
\$\endgroup\$

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.