2
\$\begingroup\$

i finally have been moved to PDO (from MySQLi) and i m now trying to make simple login and registration page (for training purposes).

This code does work! But i want to know if it is secure?

Also witch way is better for prepared statements to pass values with ? or :username? Is the way i use PDO is simple or should i change something?

// Define variables and initialize with empty values
$username = $password = $confirm_password = "";
$username_err = $password_err = $confirm_password_err = "";

// Processing form data when form is submitted
if($_SERVER["REQUEST_METHOD"] == "POST"){

    // Validate username
    if(empty(trim($_POST["username"]))){
        $username_err = "Please enter a username.";
    } else{
        // Prepare a select statement
        $sql = "SELECT id FROM users WHERE username = :username";

        if($stmt = $pdo->prepare($sql)){
            // Bind variables to the prepared statement as parameters
            $stmt->bindParam(":username", $param_username, PDO::PARAM_STR);

            // Set parameters
            $param_username = trim($_POST["username"]);

            // Attempt to execute the prepared statement
            if($stmt->execute()){
                if($stmt->rowCount() == 1){
                    $username_err = "This username is already taken.";
                } else{
                    $username = trim($_POST["username"]);
                    $email = trim($_POST["email"]);
                }
            } else{
                echo "Oops! Something went wrong. Please try again later.";
            }
        }

        // Close statement
        unset($stmt);
    }

    // Validate password
    if(empty(trim($_POST["password"]))){
        $password_err = "Please enter a password.";     
    } elseif(strlen(trim($_POST["password"])) < 6){
        $password_err = "Password must have atleast 6 characters.";
    } else{
        $password = trim($_POST["password"]);
    }

    // Validate confirm password
    if(empty(trim($_POST["confirm_password"]))){
        $confirm_password_err = "Please confirm password.";     
    } else{
        $confirm_password = trim($_POST["confirm_password"]);
        if(empty($password_err) && ($password != $confirm_password)){
            $confirm_password_err = "Password did not match.";
        }
    }

    // Check input errors before inserting in database
    if(empty($username_err) && empty($password_err) && empty($confirm_password_err)){

        // Prepare an insert statement
        $sql = "INSERT INTO users (username, email,  password) VALUES (:username, :email, :password)";

        if($stmt = $pdo->prepare($sql)){
            // Bind variables to the prepared statement as parameters
            $stmt->bindParam(":username", $param_username, PDO::PARAM_STR);
            $stmt->bindParam(":email", $param_email, PDO::PARAM_STR);
            $stmt->bindParam(":password", $param_password, PDO::PARAM_STR);

            // Set parameters
            $param_username = $username;
            $param_email = $email;
            $param_password = password_hash($password, PASSWORD_DEFAULT); // Creates a password hash

            // Attempt to execute the prepared statement
            if($stmt->execute()){
                // Redirect to login page
                header("location: login");
                die();
            } else{
                echo "Something went wrong. Please try again later.";
            }
        }

        // Close statement
        unset($stmt);
    }

    // Close connection
    unset($pdo);
}

Thanks in advance!

\$\endgroup\$
2
\$\begingroup\$

I don't really like the kind of a review that is popular on this site which only states that your code has some problems but doesn't really offer any solution or improvement.

Given for some problems listed it's really hard to find a plausible solution, I would prefer an answer where author at least tries to provide a solution, so it will be sort of a sanity check for the problem stated.

Now to your code. It is pretty secure but not well coded. For me, the main problem here is the code repetition. For some reason each value is accessed and duplicated through many different variables. Why not to keep just one variable initialized at the top?

Also, empty() is never needed when a variable is deliberately set. You can always use a variable itself for the purpose.

The PDO code could be also improved, in two ways

  • Positional placeholders are less verbose and there is no real use fro the named placeholder of only one variable is going to be used.
  • Such a manual error handling (i.e. if($stmt = $pdo->prepare($sql))) is useless and harmful at the same time. An error message like "Oops! Something went wrong." would help noone. Please read my exhaustive explanation on the PHP error reporting for the details.

So here is the 1st tier refactoring, just to make your code tidy and meaningful:

<?php
// Processing form data when form is submitted
if($_SERVER["REQUEST_METHOD"] == "POST"){

    // why not to define a variable already with a value?
    $username = trim($_POST["username"]);
    $email = trim($_POST["email"]);
    $password = trim($_POST["password"]);
    $confirm_password = trim($_POST["confirm_password"]);

    // I would rather make errors an array than separate variables;
    $error = [];

    // Validate username
    if(!$username){
        $error['username'] = "Please enter a username.";
    } else{
        $sql = "SELECT id FROM users WHERE username = ?";
        $stmt = $pdo->prepare($sql);
        $stmt->execute([$username]);
        if($stmt->rowCount()){
            $error['username'] = "This username is already taken.";
        }
    }

    // Validate password
    if(!$password){
        $error['password'] = "Please enter a password.";
    } elseif(strlen($password) < 6) {
        $error['password'] = "Password must have at least 6 characters.";
    }

    // Validate confirm password
    if(!$confirm_password){
        $error['confirm_password'] = "Please enter confirm password.";
    } elseif ($password != $confirm_password){
        $error['confirm_password'] = "Password did not match.";
    }

    // Check input errors before inserting in database
    if(!$error){
        $param_password = password_hash($password, PASSWORD_DEFAULT); // Creates a password hash

        // Prepare an insert statement
        $sql = "INSERT INTO users (username, email,  password) VALUES (:username, :email, :password)";

        $stmt = $pdo->prepare($sql);
        // Bind variables to the prepared statement as parameters
        $stmt->bindParam(":username", $username);
        $stmt->bindParam(":email", $email);
        $stmt->bindParam(":password", $param_password);

        $stmt->execute();
        header("location: login");
        die;
    }
}

Just keep in mind that refactoring is an endless process, you can improve any code. but better to do it gradually, in order for you to understand every change as opposed to adding some cargo cult code mindlessly.

\$\endgroup\$
  • \$\begingroup\$ To be honest, I was hoping for your answer! Thanks for explaining it in details and for this code update! I think Kiko answer also is good but will accept yours as it provides visual example. Thanks! \$\endgroup\$ – Ingus Oct 21 at 11:54

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.