4
\$\begingroup\$

I have just started messing around with PHP/SQL and came up with a simple sign-in application that checks if your sign-in information is valid based on database information. Here is my code:

<!DOCTYPE HTML>
<html>
<head>
<title>Sample Database Page</title>
<style>
    body {
        font-family: sans-serif;
    }
    .error {
        display: none;
        margin: 10px;
        align-content: center;
        justify-content: flex-start;
        background-color: red;
        border-radius: 5px;
    }
    .error > p {
        display: block;
        width: auto;
    }
</style>
</head>
<body>
<form method="post" action="<?php echo htmlspecialchars($_SERVER["PHP_SELF"]);?>" id="signInForm">
    Username:
    <input type="text" name="username" maxlength="15"/>
    Password:
    <input type="text" name="password" maxlength="15"/>
    <input type="submit"/>
</form>
<div class="errorContainer">
    <div class="error" id="usernameError1">
        <img src="exclamation_mark.png" width="35px"/>
        <p>Your username contains invalid characters</p>
    </div>
    <div class="error" id="passwordError1">
        <img src="exclamation_mark.png" width="35px"/>
        <p>Your password contains invalid characters</p>
    </div>
    <div class="error" id="usernameError2">
        <img src="exclamation_mark.png" width="35px"/>
        <p>The username you enter could not be found. Please try again.</p>
    </div>
    <div class="error" id="passwordError2">
        <img src="exclamation_mark.png" width="35px"/>
        <p>The password is incorrect. Please try again.</p>
    </div>
</div>

<?php
    require __DIR__ . "/helper_functions.php";
    # Contains 7 functions
    # openConnection: opens the database connection
    # closeConnection: closes the database connection
    # isValidUsername: checks user input against this regex: /^[A-Za-z0-9_]+$/
    # isValidPassword: checks user input against this regex: /^[A-Za-z0-9!@$#%_]+$/
    # escapeSymbols: escapes $ (\$)
    # displayError: uses Javascript to set the display of the selected error message to flex
    # displayUsername: inserts the provided username back into it's input field using Javascript if an error has occurred

    if ($_SERVER["REQUEST_METHOD"] == "POST") {
        $conn = openConnection();
        if ($conn->connect_error) {
            die("Connection failed: " . $conn->connect_error);
        }

        $return = false;
        if (!isValidUsername($_POST["username"])) {
            displayError("usernameError1");
            $return = true;
        }
        if (!isValidPassword($_POST["password"])) {
            displayError("passwordError1");
            $return = true;
        }
        if ($return) {
            displayUsername($_POST["username"]);
            closeConnection($conn);
            return;
        }

        $username = $_POST["username"];
        $password = escapeSymbols($_POST["password"]);
        $sql = "SELECT * FROM `customer_data` WHERE username = '" . $username . "'";
        $result = mysqli_query($conn, $sql);
        if (mysqli_num_rows($result) == 0) {
            displayError("usernameError2");
            closeConnection($conn);
            return;
        }
        $row = $result->fetch_assoc();
        $correctPassword = $row["password"];
        if ($password == $correctPassword) {
            echo "Access granted!";
            closeConnection($conn);
        } else {
            displayError("passwordError2");
            displayUsername($_POST["username"]);
            closeConnection($conn);
        }
    }
?>
</body>
</html>

When you submit the form, a few things happen:

  1. A connection to the database is set up. If the database cannot be found, the application dies.
  2. The application checks if the submitted fields don't contain invalid characters. If one does, an error is displayed for that field, and if both do, error messages are displayed for each. The submitted username is displayed again in it's input field. This process stops.
  3. The username is then checked for in the database. If it is not found, an error message is displayed, the username submitted by the user is displayed in it's input field, and this process is stopped. Otherwise, it checks if the password (with $ escaped) is the users. If it is, the program displays "Access granted!" and the input is valid. Otherwise, an error message is displayed, the previously submitted username is displayed in it's input field, and this process ends.

What can I do to improve this program?


UPDATE:

Here is the code for helper_functions.php as requested:

<?php
    function openConnection() {
        $dbhost = "localhost";
        $dbuser = "root";
        $dbpass = "";
        $db = "online_store_database";
        $conn = new mysqli($dbhost, $dbuser, $dbpass, $db) or die("Connect failed:  %s\n" . $conn -> error);

        return $conn;
    }
    function closeConnection($conn) {
        $conn -> close();
    }
    function isValidUsername($data) {
        return preg_match('/^[A-Za-z0-9_]+$/', $data);
    }
    function isValidPassword($data) {
        return preg_match('/^[A-Za-z0-9!@$#%_]+$/', $data);
    }
    function escapeSymbols($string) {
        return str_replace("$", "\$", $string); 
    }
    function displayError($errorName) {
        echo "<script type='text/javascript'>document.getElementById('" . $errorName . "').style.display = 'flex';</script>";
    }
    function displayUsername($username) {
        echo "<script async type='text/javascript'>document.getElementsByName('username')[0].value = '" . $username . "';</script>";
    }
?>
\$\endgroup\$
  • \$\begingroup\$ Just some quick points - you should be using prepared statements and also not store plain text passwords (look into password_hash. Try not to mix the procedural and OO versions of the mysqli api (mysqli_num_rows and then ->fetch_assoc) \$\endgroup\$ – Nigel Ren Nov 16 '19 at 20:55
  • \$\begingroup\$ It may be helpful to include the source code for helper_functions.php as some of these helper functions seem to serve little real purpose. \$\endgroup\$ – Nigel Ren Nov 16 '19 at 20:58
  • \$\begingroup\$ @NigelRen Can you explain what you're saying in regards to prepared statements and procedural/object-oriented versions of MySQLi in your first comment? I just started with PHP. \$\endgroup\$ – Brendon Shaw Nov 16 '19 at 21:09
  • \$\begingroup\$ @Nigel please do not post review points as comments. \$\endgroup\$ – mickmackusa Nov 16 '19 at 21:15
2
\$\begingroup\$
  • openConnection() is written to die() internally if there is a connection error, there is no need for you to write:

    if ($conn->connect_error) {
        die("Connection failed: " . $conn->connect_error);
    }
    

    I do not recommend that you provide the precise mysql error when something goes wrong. It is better practice to provide a vague explanation to the end user when this occurs. They don't need the specificity -- just tell them that it wasn't their fault and that you intend to fix it / they should try again later / they should contact you (if you are not actively monitoring the site).

    Nor do I recommend that you litter your script with die() calls. They will break an otherwise valid markup. (die() will prevent the closing tags from being printed at the bottom of your document.)

  • \w = [A-Za-z0-9_] for this reason you can simplify your code as:

    function isValidUsername($data) {
        return preg_match('/^\w+$/', $data);
    }
    

    and

    function isValidPassword($data) {
        return preg_match('/^[\w!@$#%]+$/', $data);
    }
    

    However, I must urge you not to limit the valid characters in a password entry -- this is simply bad practice because it will lighten the workload for someone who endeavors to brute force attack your login system. You should only be checking that it has length, and if you want to indicate password strength, implement some rules about expected characters and minimum password length.

  • Throw escapeSymbols() away entirely. You should not be escaping or adjusting a user's password -- ever. If they typed it, it should be saved verbatim.

  • Never store unencrypted passwords into your database. The guidance you require on this topic is far to vast to write into a single post here. Encryption is a subject that professionals specialize in, so the volume of understanding is yours to decide. At the very least, please start but don't finish reading here: Secure hash and salt for PHP passwords

  • Prepared statements are a must for helping to keep your site secure and stable. Please start but don't finish reading here: How to create a secure mysql prepared statement in php?

  • In alignment with the previous item, you must not trust user input to be safe to inject into your content either. You should implement a sanitizing layer before allowing any of their data to make it to your display. Please start but don't finish reading here: How can I sanitize user input with PHP?

  • Unless your main form script is being called by a function, I don't understand why you have return written -- there will be no place to return to, just remove them

  • Do not mix object-oriented with procedural mysqli syntax. Pick one and stick with it. I recommend OO because it is more succinct. You start with $conn->connect_error which is OO, so just use the same syntax onward. (in case you don't follow my meaning, mysqli_query() is procedural syntax $conn->query() is OO; but again, look into prepared statements.)

  • Don't bother with closing your database connection. PHP will do it for you when your script completes.

  • Try to structure your battery of conditions into a considerate condition block with "failed" outcomes written first, then the successful outcome last while avoiding any exit()/die() calls to interrupt the flow -- if this is a consistent design throughout your project, your project will be easier to read and maintain.

\$\endgroup\$
  • 1
    \$\begingroup\$ Thank you for accepting @Brendon, but it can be an advantageous strategy to withhold the green tick until adequate time passes to allow the submission of multiple reviews. Some people will not review a page that already has a green tick (I am not one of those people). If you would like to remove the green tick to "keep your question in the game", that is just fine with me. You will stand to receive more insights this way. \$\endgroup\$ – mickmackusa Nov 16 '19 at 21:59
2
\$\begingroup\$

Just a few things to add to mickmackusa's excellent answer...

  • Consider using the <label> HTML tag

    In things like

    Username:
    <input type="text" name="username" maxlength="15"/>
    

    When you click on a label, focus is sent to the input associated with it. This helps improve accessibility as well as other benefits - Why use <label>?

    <label for="username">Username:</label>
    <input type="text" id="username" name="username" maxlength="15"/>
    
  • A general point about SQL

    $sql = "SELECT * FROM `customer_data` WHERE username = '" . $username . "'";
    

    Normally I would suggest only selecting the columns which you actually use. So in this case you would only use (including the addition of prepared statements)

    $sql = "SELECT `password` FROM `customer_data` WHERE `username` = ?";
    

    (for me) It is also worth sticking to using backticks round column and table names. If you always use them it can help as reserved words in table or column names can randomly become very useful (order tables can be common) and then SQL starts to complain unless it's in backticks.

  • In the next line, you don't check if the command actually worked...

    $result = mysqli_query($conn, $sql);
    

    You can either use something like

    if ($result = mysqli_query($conn, $sql)) {
       // process successful query
    }
    

    or use

    mysqli_report(MYSQLI_REPORT_STRICT);
    

    which makes PHP throw an exception when any errors occur (explained here)

\$\endgroup\$
1
\$\begingroup\$

I am more a man of practice than of advise.

Maybe because I feel the responsibility for the advise I give out. Some advises are much simpler to be given than implemented. I need to make sure that my advise won't raise more questions than it does answer - otherwise it will be anything but help. It is simple to say "use prepared statements" but how exactly one should use it? It is simple to say "do not provide the precise mysql error" but how to do that?

That's why I prefer a complete working solution to a wordy sermon. And, being obsessed with the knowledge re-use, I am writing articles that provide the full story.

Here is one on the error reporting. It basically explains that your database interaction code should (almost) never handle its errors - it's just none of its business. So just leave database errors alone. You'll be to deal with them later, when you'd feel the urge to (of course it is explained in the above article how to do that) but for the moment you can put it aside.
Instead, just configure your PHP to handle errors uniformly, according to the server role.

Here is one about mysqli_connect(). The connection code is not as simple as just a single line. There are many options to pay attention for.

Here is one about running SELECT queries with mysqli in general and checking login and password against a database in particular.

Other issues in your code are:

  • of course, passwords must be hashed.
  • after the successful login, you have to store the user credentials in a session. and then relocate a user to some other page.
  • there is not much point in strictly validating the input. It makes sense for the registration but here you aren't going to record anything, so you can put aside most validations
  • therefore, there is no point in the whole return business. The only error message you would have is one that says login or password is incorrect.
  • there is a rule of thumb says all PHP code must go first and HTML later. It makes sense as your PHP code may want to send an HTTP header first
    • besides, it is always a good idea to separate HTML from PHP.
  • escapeSymbols() is completely useless if not harmful
  • mysqli_num_rows() is also a useless function, it could be skipped seven days in a week
  • close connection is... also that. If your script is not supposed to work from the point where this function is called (which happens most of time) there is no point in closing the connection manually, PHP will do it for you
  • the empty form action attribute effectively says "sand the form to the current URL", so it's much simpler and less error prone to leave it empty
  • the php closing tag (?>) is also useless if it's the last thing in the file

Given all the above here is your code refactored

helper_functions.php

<?php

error_reporting(E_ALL);
ini_set('display_errors', 1); // change it to 0 on production
ini_set('log_errors', 1);

require __DIR__ . "/mysqli.php";

function isValidUsername($data) {
    return preg_match('/^[A-Za-z0-9_]+$/', $data);
}
function displayError($errorName) {
    echo "<script type='text/javascript'>document.getElementById('" . $errorName . "').style.display = 'flex';</script>";
}

mysqli.php

<?php
$host = '127.0.0.1';
$db   = 'online_store_database';
$user = 'root';
$pass = '';
$charset = 'utf8mb4';

mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);
try {
    $mysqli = new mysqli($host, $user, $pass, $db);
    $mysqli->set_charset($charset);
} catch (\mysqli_sql_exception $e) {
     throw new \mysqli_sql_exception($e->getMessage(), $e->getCode());
}
unset($host, $db, $user, $pass, $charset); // we don't need them anymore

the main PHP file

<?php
if ($_SERVER["REQUEST_METHOD"] == "POST") {
    require __DIR__ . "/helper_functions.php";

    $stmt = $conn->prepare("SELECT * FROM `customer_data` WHERE username = ?");
    $stmt->bind_param("s", $_POST["username"]);
    $stmt->execute();
    $result = $stmt->get_result();
    $row = $result->fetch_assoc();
    if ($row && password_verify($_POST["password"], $row["password"]))
    {
        session_start();
        $_SESSION['user'] = $row;
        header("Location: /");
        exit;
    } else {
        $error = 'passwordError2';
    }
}
include 'form.php';

form.php

<!DOCTYPE HTML>
<html>
<head>
<title>Sample Database Page</title>
<style>
    body {
        font-family: sans-serif;
    }
    .error {
        display: none;
        margin: 10px;
        align-content: center;
        justify-content: flex-start;
        background-color: red;
        border-radius: 5px;
    }
    .error > p {
        display: block;
        width: auto;
    }
</style>
</head>
<body>
<form method="post" action="" id="signInForm">
    Username:
    <input type="text" name="username" maxlength="15"/>
    Password:
    <input type="text" name="password" maxlength="15"/>
    <input type="submit"/>
</form>
<div class="errorContainer">
    <div class="error" id="usernameError1">
        <img src="exclamation_mark.png" width="35px"/>
        <p>Your username contains invalid characters</p>
    </div>
    <div class="error" id="passwordError1">
        <img src="exclamation_mark.png" width="35px"/>
        <p>Your password contains invalid characters</p>
    </div>
    <div class="error" id="usernameError2">
        <img src="exclamation_mark.png" width="35px"/>
        <p>The username you enter could not be found. Please try again.</p>
    </div>
    <div class="error" id="passwordError2">
        <img src="exclamation_mark.png" width="35px"/>
        <p>The password is incorrect. Please try again.</p>
    </div>
</div>
<?php if (!empty($error)) displayError("passwordError2"); ?>
</body>
</html>
\$\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.