Website user registration script in PHP

Question

I have a registration system, but I really don't know if it is safe, because I don't understand security very well and I'm afraid of compromising my clients' data, I believe that the methods I created are good for security, but I still have doubts if it is safe

I created several variables using encapsulation, and then used them, is that right? Or should I create them within the methods

Is regular expression enough to protect against JavaScript attacks?

Is there a vulnerability in my code or an error?

Can we consider that I have created efficient methods for the security of my clients?

<?php
/**
 * Signup
 */

class SignUp {

    private $email;
    private $password;
    private $name;
    private $sql;
    private $result;
    private $conn;
    private $remote_addr;
    private $http_user_agent;
    private $http_client_ip;
    private $http_x_forwarded_for;
    private $check_name;
    private $line;

    public function __construct($host, $dbname, $user, $pass) {
        try {
            $this->conn = new PDO("mysql:host=$host;dbname=$dbname", $user, $pass);
            $this->conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
            return $this->conn;
        }catch(PDOException $e) {
            throw new Exception($e->getMessage());
        }
    }


    public function setEmail($e) {
        $this->email = $e;
    }

    public function getEmail() {
        return $this->email;
    }

    public function setName($n) {
        $this->name = $n;
    }

    public function getName() {
        return $this->name;
    }

    public function setPassword($password, $confirm) {
        if ($password === $confirm) {
            $this->password = password_hash($password, PASSWORD_BCRYPT);
        } else {
            // Handle input error here
            echo "Password does not match!";
        }
    }

    public function getPasswordHash() {
        return $this->password;
    }


    // public function CheckHashes() {
    //     echo $this->password;
    // }

    public function CheckName(){
        $this->check_name = $this->conn->prepare("SELECT name FROM users WHERE name = :name");
        $this->check_name->execute(array(
            ":name" => $this->getName()
        ));

        $this->line = $this->check_name->rowCount();

        if($this->line != 0){
            $_SESSION['msg'] = '<p style="color:red;">Name already exists!</p>';
            header("Location: signup.php");
            exit();
        }
    }

    public function CheckEmail(){
        $checkmail = $this->conn->prepare("SELECT email FROM users WHERE email = :email");
        $checkmail->execute(array(
            ":email" => $this->getEmail()
        ));

        $theline = $checkmail->rowCount();

        if($theline != 0){
            $_SESSION['msg'] = '<p style="color:red;">Email already exists!</p>';
            header("Location: signup.php");
            exit();
        }
    }

    public function CheckFields(){
        if(empty($this->password) || empty($this->name) || empty($this->email)){
            $_SESSION['msg'] = '<p style="color:red;">Please, fill out all the fields!</p>';
            header("Location: signup.php");
            exit();
        }
    }

    public function ValidateNameLength(){
        if(strlen($this->getName()) < 10){
            $_SESSION['msg'] = '<p style="color:red;">Can only use 10 or more characters as name!</p>';
            header("Location: signup.php");
            exit();
        }

        if(strlen($this->getName()) > 25){
            $_SESSION['msg'] = '<p style="color:red;">Can only use a maximum of 25 characters as name!</p>';
            header("Location: signup.php");
            exit();
        }
    }

    public function ValidateEmail(){
        if(!filter_var($this->getEmail(), FILTER_VALIDATE_EMAIL)){
            $_SESSION['msg'] = '<p style="color:red;">Incorrect email!</p>';
            header("Location: signup.php");
            exit();
        }
    }
    
    public function Insert() {
        if (preg_match("/^[A-Za-z0-9\s]*$/", $this->getName()) ) {
                $this->sql = "INSERT INTO users (name, email, pass, ip_address, http_client_ip, http_x_forwarded_for, http_user_agent) VALUES(:name, :email, :pass, :ip_address, :http_client_ip, :http_x_forwarded_for, :http_user_agent)";
                $this->result = $this->conn->prepare($this->sql);
                $this->remote_addr = $_SERVER['REMOTE_ADDR'];
                $this->http_client_ip = $_SERVER['HTTP_CLIENT_IP'];
                $this->http_x_forwarded_for = $_SERVER['HTTP_X_FORWARDED_FOR'];
                $this->http_user_agent = $_SERVER['HTTP_USER_AGENT'];
                $this->result->execute(array(
                    ":name" => $this->getName(),
                    ":email" => $this->getEmail(),
                    ":pass" => $this->getPasswordHash(),
                    ":ip_address" => $this->remote_addr,
                    ":http_client_ip" => $this->http_client_ip,
                    ":http_x_forwarded_for" => $this->http_x_forwarded_for,
                    ":http_user_agent" => $this->http_user_agent
                ));

                if($this->result == true){
                    $_SESSION['msg'] = '<p style="color:green;">Registered successfully!</p>';
                    header("Location: signin.php");
                    exit();
                }
        }

        else {
            $_SESSION['msg'] = '<p style="color:green;">You can only use letters and numbers as name!</p>';
            header("Location: signup.php");
            exit();
        }

    }
}

$obj = new SignUp('localhost', 'dbname', 'user', 'password');
$obj->setEmail('[email protected]');
$obj->setName('Anne');
$obj->setPassword('password', 'password');
$obj->CheckName();
$obj->CheckEmail();
$obj->CheckFields();
$obj->ValidateNameLength();
$obj->ValidateEmail();
$obj->Insert();
// $obj->getEmail();
// echo $obj->CheckHashes();
// echo "\n";

Answer 1

Security

Your main worry is security. I don't see any obvious security problems in your code. To judge whether your code is really safe, I would need to see the whole system, and you have only given us a part of it. This piece seems secure, and that's all I can say.

Coding style

Your code has some real style problems. It looks to me that you're new to OOP. I'll deal with the problems from the big ones to minor ones.

Code repetition

There's quite a lot of code repetition. Two obvious case are:

1: Where you set an error message, jump to signup.php, and exit(). There could be a method like this:

private function returnWithError($errorMessage)
{
    $_SESSION['error'] = $errorMessage;
    header('Location: ' . $this->signUpPage);
    exit();
} 

I would leave out the HTML tag in the error message. The HTML should be added by the code that outputs the error to the user. That makes more sense because it is the responsibility of the output code to format the error. I will come back to this further on.

2: The methods CheckName() and CheckEmail() are very similar. They could both use a method that would look something like this:

private function existingColumnValue($columnName, $value)
{                                                               
    $statement = $this->database->prepare("SELECT userId FROM users WHERE $columnName = :value");  
    $statement->execute([':value' => $value]);
    return $statement->rowCount() > 0;
}

This method simply checks whether a value exists in a column. It can be reused for all columns in the users table. Instead of redirecting, and exiting, it returns a boolean. Now you can rewrite CheckName() and CheckEmail() as:

public function existingName($name)
{            
  return existingColumnValue('name', $name);                                                   
}

public function existingEmail($email)
{            
  return existingColumnValue('email', $email);                                                   
}

Responsibility

Now let's dive a bit deeper into what this class is meant to do. The main focus is: Accepting new users into the database, or not. That is its responsibility, no more no less. So setting up a database connection, redirecting, or formatting output, shouldn't be part of this class. The code at the bottom of your page should ideally look something like this:

$database = new PDO(.....);
$users    = new Users($database);
if ($users->isValidNewUser($name, $email, $password)) {
    $userId = $users->addUser($name, $email, $password);
}
else {
    $_SESSION['error'] = $users->getLastError();
    header('Location: signup.php');
}

I've named the class after the database table it interacts with. The database handle is now created outside this class. Exactly how is up to you. I use two methods of the class: isValidNewUser() and addUser(). This also points to future extensions, of this class, with methods like getUser() and removeUser(). So all the Users class does now is interacting with the database. Of course you're wondering what isValidNewUser() and addUser() might look like:

private function setError($errorMessage)
{
   $this->errorMessage = $errorMessage;
   return FALSE;
}

public function getLastError()
{
   return $this->errorMessage;
}

public function isValidNewUser($name, $email, $password)
{
  if (empty($name)) return setError('Please supply a name');
  if (empty($email)) return setError('Please supply an email address');
  if (empty($password)) return setError('Please supply a password');
  if (!this->isValidName($name)) return setError('Invalid name. The name should be between 10 and 25 characters. You can only use letters and numbers.');
  if ($this->existingName($name)) return setError('This name is already taken, please choose another');
  if ($this->existingEmail($email)) return setError('This email address is already registered');
  return TRUE;
}

public function addUser($name, $email, $password)
{
  $statement = $this->database->prepare("INSERT INTO users (name, email, pass, ip_address, http_client_ip, http_x_forwarded_for, http_user_agent) 
                                         VALUES(:name, :email, :password_hash, :ip_address, :http_client_ip, :http_x_forwarded_for, :http_user_agent)");  
  $statement->execute([':name'                 => $name,
                       ':email'                => $email,
                       ':password_hash'        => password_hash($password, PASSWORD_BCRYPT),
                       ':ip_address'           => $_SERVER['REMOTE_ADDR'],
                       ':http_client_ip'       => $_SERVER['HTTP_CLIENT_IP'],
                       ':http_x_forwarded_for' => $_SERVER['HTTP_X_FORWARDED_FOR'],
                       ':http_user_agent'      => $_SERVER['HTTP_USER_AGENT']]);
  return $this->database->lastInsertID();
}

By using two methods you can now skip the validation if you want to. This could be useful if you want to import users from somewhere else and you know that list is error free. But even if you use the validation, it will not redirect, it only sets an error message. I haven't written out the isValidName() method, because this code example is already quite long.

Minor other issues

• You return $this->conn from __construct(). A constructor always returns the class it constructs, so a return of something else will not work.
• You abbreviate the name parameter to $n and the email parameter to $e. Why? Would $name and $email not be better?
• You use $this->password to store the password hash. It's not uncommon to do this, but it is confusing. What's in $this->password? Is it the password or a hash? Why not call it $this->passwordHash?
• You're using a lot of class level variables where they are not needed, for instance $this->check_name in CheckName(). Moreso, in CheckEmail() you don't do that.
• I think using Check in a method name is not very descriptive of what the method does.
• A short array syntax exists which replaces array() with []. I prefer it, but array() does exactly the same.

Ideas for the future

There's one problem I haven't addressed: By separating the validation of the user data from the insert routine, we run the risk that this will not work properly. It is very unlikely, but suppose two different people, with the same name, sign up at exactly the same time. Both could have their name checked against the database, it is not found, and then both are inserted. These very unlikely events tend to occur, once in a blue moon, on very busy sites. These type of errors are very hard to debug.

There are several ways to solve this problem. A classic way is to use transations. Another method is to use a conditional insert query: "If this name doesn't exist, then insert this data.".

Either way, I thought you should be aware of this minor problem.