2
\$\begingroup\$

I've written some PHP code that connects to a database and saves input entered by the user at our signup page. I would like some feedback before I present this code to my colleagues, as I intend on using this code for our website:

  • Security
  • Use of PDO vs other connection methods
  • Variable Naming
  • PHP Conventions
  • HTML Conventions
  • Anything else

sign_up.html

<!DOCTYPE html>
<html lang="en-US">
    <head>
        <meta charset="UTF-8">
        <title>Sign Up</title>
        <link rel="stylesheet" type="text/css" href="styles/main.css">
        <link rel="stylesheet" type="text/css" href="styles/dropdown.css">
        <link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/font-awesome/4.7.0/css/font-awesome.min.css">
        <script src="scripts/empties.js"></script>
    </head>
    <body>

        <!-- Nav Bar -->
        <div class="navbar">
            <a href="index.html"><i class="fa fa-fw fa-home" style="font-size: 40px"></i></a>
        </div>

        <!-- Signup Area -->
        <div id="sign_up_area">
            <form action="sign_up.php" method="post">
                <input id="form_input" 
                       type="text" 
                       placeholder="First Name" 
                       name="firstname"><br />
                <input id="form_input" 
                       type="text" 
                       placeholder="Last Name"
                       name="lastname"><br />
                <input id="form_input"
                       type="text" 
                       placeholder="Age"
                       name="age"><br />
                <input id="form_input" 
                       type="text" 
                       placeholder="Email"
                       name="email"><br />
                <input id="form_input" 
                       type="password" 
                       placeholder="Password"
                       name="password"><br />
                <button id="signup_button" type="submit" disabled>Sign Up</button>
            </form>
        </div>
    </body>
</html>

sign_up.php

<?php

include 'config/database.php';

try {

    $conn = new PDO("mysql:host=$DB_HOST;dbname=Database", $DB_USER, $DB_PASSWORD);
    $conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

    $firstname = filter_var($_POST["firstname"], FILTER_SANITIZE_STRING);
    $lastname = filter_var($_POST["lastname"], FILTER_SANITIZE_STRING);
    $email = filter_var($_POST["email"], FILTER_SANITIZE_EMAIL);
    $password = filter_var($_POST["password"], FILTER_SANITIZE_STRING);

    #No need to hash name and email, can be used to check duplicates
    $password_hash = password_hash($password, PASSWORD_DEFAULT);

    $stmt = $conn->prepare("INSERT INTO Users (FirstName, LastName, Email, Password) VALUES (:firstname, :lastname, :email, :password)");
    $stmt->bindParam(':firstname', $firstname);
    $stmt->bindParam(':lastname', $lastname);
    $stmt->bindParam(':email', $email);
    $stmt->bindParam(':password', $password_hash);
    $stmt->execute();

    header("Location: signed_up.html");

} catch(PDOException $e) {

    echo "Connection failed: " . $e->getMessage();

}

?>
\$\endgroup\$
  • \$\begingroup\$ Echoing the error message right away is a bad practice. See my PHP error reporting basics. Also, filter_var is mostly misused \$\endgroup\$ – Your Common Sense Aug 10 '19 at 16:04
  • 1
    \$\begingroup\$ @YourCommonSense Please don't answer in comments. Any issues you see with the code should be mentioned in an answer. \$\endgroup\$ – 200_success Aug 10 '19 at 16:29
1
\$\begingroup\$
  • I see multiple instances of id="form_input". These attributes need to change to class or each id attribute value should be altered to be unique.

  • Instead of using <br> tags to style your input fields on separate lines, use css. By setting all of the elements in the form as display:block;, you can eliminate the excess markup -- a cleaner alternative. CSS should be the first technique to create spacing and layout stylings; when you find yourself creating spaces with the spacebar or with <br>, try to achieve the same effect with display, margin, padding, etc.

  • 11 reasons why placeholders are problematic -- I can appreciate that you intend to design a clean, minimalist layout, but I don't think I support non-labeled fields and your project should maintain a consistent theme considering that your project may eventually need to employ more forms.

  • In addition to the previous bullet point about UX, I will urge you to add some client-side validation to your form. These character limitations will be up to you. I assume empties.js is doing something that enables the submit button, but I can't see it. If empties.js is as functionally narrow a file as it's name suggests, you may want to build more robust processes in there.

  • Rather than sanitizing server-side and unconditionally inserting into the database, you should be catching overtly "bad" submissions and denying access.

  • If a submitted password contains html markup, that is no concern of yours (no need to sanitize) because you will never, ever be printing that value -- ever. More clearly: never alter someone's submitted password characters.

  • You must never, ever present the raw error message to the end user. See @YCS's hyperlink. Especially with registration forms, you need to find the correct balance between informing the registrant why the submission was denied, yet not compromising the security of your project.

  • Finally, login/registration systems are widely available now. If you are doing this as an educational exercise, then carry on. If you don't want to reinvent the wheel, just use a pre-existing script from a security-minded author.

\$\endgroup\$

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.