4
\$\begingroup\$

I am a beginner, and I'm trying to secure a sign-login system on my website. Is my code good/enough to prevent SQL injection?

THIS IS THE SIGN FILES

This is the index.php that takes user input:

<?php

require('some_path\autoload_class.php');

$pass = $_POST['passwordd'];
$card = $_POST['cardd'];

$hashed_pass = password_hash($pass, PASSWORD_DEFAULT);
$hashed_card = password_hash($card, PASSWORD_DEFAULT);

$user = new User($_POST['emaill'], $hashed_pass, $hashed_card);
$user->insert();

echo("Your <b>Email</b> is: {$_POST['emaill']}<br>");
echo("Your <b>Password</b> is: {$pass}<br>");
echo("Your <b>Card's number</b> is: {$card}");
?>

This is User.php Class:

<?php

class User {
    //User and database info
    private $email;
    private $password;
    private $card;
    private $host = "localhost";
    private $user = "root";
    private $dbpassword = "x";
    private $dbName = "database";

    public function __construct(string $user_email, string $user_password, string $user_card) {
        $this->email = $user_email;
        $this->password = $user_password;
        $this->card = $user_card;
    }

    public function insert(){
        try{

            //The connection to the Database

            $dsn = "mysql:host={$this->host};dbname={$this->dbName}";
            $pdo = new PDO($dsn, $this->user, $this->dbpassword);
            $pdo->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
            $pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
            echo("Connected successfully to database!<br>");

            //The code to insert user input into database (table user)

            $sql = "INSERT INTO user(email, password, card) VALUES(?, ?, ?)";
            $prepared_statement = $pdo->prepare($sql);
            $prepared_statement->execute([$this->email, $this->password, $this->card]);
            echo("User input stored with success!<br>");

            //I did read that this is good practice for closing the connection

            $pdo = null;
            $prepared_statement = null;

            //if there's an error, it will be printed out

        }catch (PDOException $e) {
            echo("Connection failed!<br>");
            echo($e->getMessage());
        }
    }
}

THIS IS THE LOGIN FILES

This is index.php that takes user input and confirm if password is correct:

<?php

require('some_path\autoload_class.php');

$user = new login($_POST['emaill'], $_POST['passwordd']);
$user->fetch();

This is Login.php class:

<?php

class login {
    private $email;
    private $password;
    private $host = "localhost";
    private $user = "root";
    private $dbpassword = "x";
    private $dbName = "database";

    public function __construct(string $user_email, string $user_password) {
        $this->email = $user_email;
        $this->password = $user_password;
    }

    public function fetch(){
        try{

            //The connection to the database

            $dsn = "mysql:host={$this->host};dbname={$this->dbName}";
            $pdo = new PDO($dsn, $this->user, $this->dbpassword);
            $pdo->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
            $pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
            $pdo->setAttribute(PDO::ATTR_DEFAULT_FETCH_MODE, PDO::FETCH_ASSOC);
            echo("Connected successfully to database!<br>");

            //The query
            $sql = "SELECT * FROM user WHERE email = ?";
            $prepared_statement = $pdo->prepare($sql);
            $prepared_statement->execute([$this->email]);
            $user = $prepared_statement->fetchAll();

            //Because $user is an assocc array, I use password_verify inside this foreach
            foreach ($user as $key) {
                //if user password == to hashed password in database where email == user email
                if (password_verify($this->password, $key['password'])) {
                    echo("User Indentity Confirmed with Success!<br>");
                    echo("<h2>INFO</h2><hr><hr>");
                    echo("<b>User Id:</b> {$key['user_id']}<br>");
                    echo("<b>User Email:</b> {$key['email']}<br>");
                    echo("<b>User Password:</b> {$key['password']}<br>");
                    echo("<b>User Card number:</b> {$key['card']}");
                }else{
                    echo("<h2>User Indentity Not Confirmed.</h2>");
                }
            }
            $pdo = null;
            $prepared_statement = null;

        }catch (PDOException $e) {
            echo("Oops...Something Went Wrong!<br>");
            echo($e->getMessage());
        }
    }
}

In the sign files, I'm trying to get user input in the most secure way I possible, hashing password and credit card number, using pdo with prepared statements, and then inserting the user input into database.

In the login files, I'm trying to use the email provided by user, find the row in user table where email = user email provided, and then comparing the hashed password with password provided.

\$\endgroup\$
3
  • \$\begingroup\$ echo is not a function. Adding parentheses is very confusing \$\endgroup\$ – Dharman Apr 28 at 14:13
  • \$\begingroup\$ Your PDO connection needs charset. Don't rely on the default one. Set it properly yourself to be utf8mb4 \$\endgroup\$ – Dharman Apr 28 at 14:14
  • \$\begingroup\$ @Dharman thank you, sorry I'm late to reply. You're always there! I will google for more information about charset. And I'm just used to use echo as a function because it looks like printf(). \$\endgroup\$ – irtexas19 May 2 at 20:45
4
\$\begingroup\$

Your code is good enough at preventing sql injection. But it sucks (pardon me) in everything else.

Don't print database error to the user.

Don't put echoes into your logic.

Don't double last letters of post fields names.

Don't repeat yourself by integrating your database connection to every class that has nothing to do with db. Prefer composition. User should not insert itself into db. Login should not fetch itself from db. How are you going to connect to a different db without changing every class in your project?

EDIT: To clarify the last point, your code should look more like this:

// definitions 
function connect() {
  return new \PDO(/*maybe take it from environemt variables rather then hardcoding it*/);
}

// services instantiation (your bootstrap, DI container or so...)
$pdo = connect();
$users = new UsersService($pdo);


// create new user
$users->createUser($userCreateRequestData);

// to login
$user = $users->login($userName, $password);
...

Or in other words, there should be separate objects to represent a user (its data) and to manipulate user objects (ie, creating new users, verifying their password, obtaining their data, updating their data, removing them, etc...). User data dont need to know how, where and when they are persisted. And the persister dont need to carry arund data of a specific user.

You may actually want to create a separate class for authentication as that's really a bit different layer then the persistance of user data - which is db related, but auth processes dont really need to know the details. On other hand, it may need to access session which is not really a concern of the data access layer. So they should be separate - Auth layer knows session and DA layer but it doesnt care for the actual connection, DA layer knows about the connection but it doesnt care if session or auth layer exists at all.

Actually creating a user should also be a separate class. Because creating a user often involved other things then just insert a row to database (ie send an email) and those things are again no concern of DA layer.

Eventually you might end up having more and more one purpose classes (which is good).

// bootstrap
$pdo = connect();
$users = new UsersRepository($pdo);

// create user
$userFactory = new UserFactory($users, $mailService);
$userFactory->createUser($userCreateRequestData);

// login user
$auth = new Authenticator($users, $sessionService);
$user = $auth->login($userName, $password);
```
\$\endgroup\$
3
  • \$\begingroup\$ Thanks for the answer, i really understood everything except: "User class should not insert() itself... and Login class should not fetch() itself". What does it mean? \$\endgroup\$ – irtexas19 Apr 26 at 7:05
  • \$\begingroup\$ @irtexas19 I have edited my answer with some more detail and code samples, have a look... \$\endgroup\$ – slepic Apr 26 at 9:08
  • \$\begingroup\$ thank you for all, I'll read up! \$\endgroup\$ – irtexas19 Apr 26 at 9:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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