3
\$\begingroup\$

Ok, so I have been looking for resources on how to make a registration form work with input validation. I want the error message show up under the table of the registration form and not on top of the website. Here is what I have working so far, but I am still learning and I would like to improve:

<?php

$host = 'localhost';
$user = 'root';
$pass = '';
$db = 'restaurantdb';

try {
$db_conn = new mysqli("$host", "$user", "$pass", "$db");
$db_conn->set_charset("utf8");
} catch (Exception $e) {
echo "Connection failed: " . $e->getMessage();
}

if (isset($_POST['register'])) {


session_start();
$forename = filter_input(INPUT_POST, 'forename', FILTER_SANITIZE_STRING);
$surname = filter_input(INPUT_POST, 'surname', FILTER_SANITIZE_STRING);
$username = filter_input(INPUT_POST, 'username', FILTER_SANITIZE_STRING);   
$password = filter_input(INPUT_POST, 'password', FILTER_SANITIZE_STRING);
$age = filter_input(INPUT_POST, 'age', FILTER_SANITIZE_STRING);
$email = filter_input(INPUT_POST, 'email', FILTER_SANITIZE_STRING);
$password = password_hash($password, PASSWORD_BCRYPT);

if(FindErrors($username, $password)) {
$query = "INSERT INTO users(firstname, lastname, username, password, age, email) 
VALUES('$forename', '$surname', '$username', '$password', '$age', '$email')";
$result = $db_conn->query($query);
if(!empty($result)) {
    $error_message = "";
    $success_message = "Registration successful";
    unset($_POST);
} else die($db_conn->error);
}
}


$forename = $surname = $username = $password = $age = $email = "";

function FindErrors($username, $password){


if (empty($forename)) {
    echo "First name is required\n";
} else {
    $forename = fix_string($_POST["forename"]); 
}

if (empty($surname)) {
    echo "Last name is required\n";
} else {
    $surname = fix_string($_POST["surename"]);
}

if (empty($username)) {
    echo "Username is required\n";
    return false;
} else {
    $password = fix_string($_POST["username"]);
}

if (empty($password)) {
    echo "Password is required\n";
    return false;
} else {
    $password = fix_string($_POST["password"]);
}

if (empty($age)) {
    echo "Age is required\n";
} else {
    $age = fix_string($_POST["age"]);
}

if (empty($email)) {
    echo "Email is required\n";
} else {
    $email = fix_string($_POST["email"]);
}
}

$fail = validate_forename($forename);
$fail .= validate_surname($surname);
$fail .= validate_username($username);
$fail .= validate_password($password);
$fail .= validate_age($age);
$fail .= validate_email($email);

echo "<!DOCTYPE html\n<html><head><title>An Example Form</title>";

if ($fail == "")
{
echo "</head><body>Form data successfully validated:
$forename, $surname, $username, $password, $age, $email.</body></html>";

// This is where you would enter the posted fields into a database.
// Preferably using hash encryption for the password.
exit;
}

echo <<<END_OF_BLOCK

<!--THIS IS HTML/JAVASCRIPT SECTION -->

<style>
    .signup {
        border: 1px solid #999999;
        font: normal 14px helvetica;
        color: #444444
    }
</style>

<script>
    function validate(form) {
        fail = validateForename(form.forename.value)
        fail += validateSurname(form.surname.value)
        fail += validateUsername(form.username.value)
        fail += validatePassword(form.password.value)
        fail += validateAge(form.age.value)
        fail += validateEmail(form.email.value)

        if (fail == "") return true
        else { alert(fail); return false }
    }

    function validateForename(field) {
        return (field == "") ? "No forename was entered.\n" : ""
    }

    function validateSurname(field) {
        return (field == "") ? "No surname was entered.\n" : ""
    }

    function validateUsername(field) {
        if (field == "") return "No Username was entered.\n"
        else if (field.length < 5)
            return "Usernames must be at least 5 characters.\n"
        else if (/[^a-zA-Z0-9_-]/.test(field))
            return "Only a-z, A-Z, 0-9, - and _ allowed in Usernames.\n"
        return ""
    }

    function validatePassword(field) {
        if (field == "") return "No password was entered.\n"
        else if (field.length < 6)
            return "Passwords must be at least 6 characters.\n"
        else if (!/[a-z]/.test(field)) || !/[A-Z]/.test(field) || !/[0-9]/.test(field))
        return "Passwords require one each of a-z, A-Z and 0-9.\n"
        return ""
    }

    function validateAge(field) {
        if (field == "" || isNaN(field)) return "No age was entered.\n"
        else if (field < 18 || field > 110)
            return "Age must be between 18 and 110.\n"
        return ""
    }

    function validateEmail(field) {
        if (field == "") return "No email was entered.\n"
        else if (!((field.indexOf(".") > 0) && (field.indexOf("@") > 0) || /[^a-zA-Z0-9.@_-]/.test(field)))
            return "The Email address is invalid.\n"
        return ""

    }

</script>
</head>
<body>
<header>
    <h1>Sun Sun Chinese Restuarant</h1>
</header>
<nav>
    <hr />
    <a href="index.html">Home</a>
    <a href="menu.html">Menu</a>
    <a href="contactus.html">Contact Us</a>
    <a href="validate.html">Sign Up</a>
    <hr />
</nav>

<table class="signup" border="0" cellpadding="2" cellspacing="5" bgcolor="#eeeeee"> 
    <th colspan="2" align="center">Signup Form</th>
    <form method="post" action="adduser.php" onsubmit="return validate(this)">
        <tr>
            <td>Forename</td>
            <td><input type="text" maxlength="30" name="forename" 
            value="" required></td>
        </tr>
        <tr>
            <td>Surname</td>
            <td><input type="text" maxlength="30" name="surname" 
            value="" required></td>
        </tr>
        <tr>
            <td>Username</td>
            <td><input type="text" maxlength="30" name="username" 
             value="" required></td>
        </tr>
        <tr>
            <td>Password</td>
            <td><input type="password" maxlength="30" name="password" 
            value="" required></td>
        </tr>
        <tr>
            <td>Age</td>
            <td><input type="text" maxlength="6" name="age" 
            value="" required></td>
        </tr>
        <tr>
            <td>Email</td>
            <td><input type="text" maxlength="60" name="email" 
            value="" required></td>
        </tr>

        <tr>
            <td colspan="2" align="center"><input type="submit" name="register"  value="Signup"></td>
        </tr>



    </form>

</table>
<!-----DISPLAY ERROR OR SUCCESS MESSAGE HERE------>


</body>

<footer>
<!-- The footer is a block at the bottom of the page. -->
<hr />
<center>
    <p>
        5301 Forest Lane<br />
        St. Louise, MS 35012<br />
        204-462-4296
    </p>
</center>
</footer>

</html>

END_OF_BLOCK;

//THE PHP Functions

function validate_forename($field)
{
return ($field == "") ? "No Forename was entered<br>": "";
}

function validate_surname($field)
{
    return ($field == "") ? "No Surname was entered<br>" : "";
}

function validate_username($field)
{
if ($field == "") return "No Username was entered<br>";
else if (strlen($field) < 5)
return "Usernames must be at least 5 characters<br>";
else if (preg_match("/[^a-zA-Z0-9_-]/", $field))
return "Only letters, numbers, numbers, - and _ in usernames<br>";
return "";
}

function validate_password($field)
{
if ($field == "") return "No Password was entered<br>";
else if (strlen($field) < 6)
return "Passwords must be at least 6 characters<br>";
else if (!preg_match("/[a-z]/", $field) || 
        !preg_match("/[A-Z]/", $field) ||
            !preg_match("/[0-9]/", $field))
            return "Passwords require 1 each of a-z, A-Z, and 0-9<br>";
            return "";
}

function validate_age($field)
{
    if ($field == "") return "No age was entered<br>";
else if ($field < 18 || $field > 110)
    return "Age must be between 18 and 110<br>";
return "";
}

function validate_email($field)
{
if ($field == "") return "No Email was entered<br>";
else if (!((strpos($field, ".") > 0) && 
(strpos($field, "@") > 0)) || 
preg_match("/[^a-zA-Z0-9.@_-]/", $field))
    return "The email address is invalid<br>";
return "";
}

function fix_string($string)
{
    if (get_magic_quotes_gpc()) $string = stripslashes($string);
return htmlentities ($string);
}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ FindErrors() will not work as you expect it to because of "function scope". For instance, $forename will always be empty because that variable does not exist inside the function. Your code is broken, you just don't realize it yet. Consider function FindErrors($username, $password){ to be interpretted as function FindErrors($param1, $param2){ ...it won't know what variable you are passing to it. \$\endgroup\$ May 5, 2019 at 16:06

2 Answers 2

1
\$\begingroup\$

There are many improvements you could make, but you're on the right track. In this answer I will only comment on some of the things I immediately notice.

Coding style

It is difficult to get an overview of your code. There's little organization. I would have split this code up into multiple files. For instance: There will be more PHP files on your server that need the database. Having a separate PHP file that opens the database, and can be included into this one, would help and you would only need to change the database access parameters in one place. Same is true for CSS and Javascript.

Use indentation to make your code more readable. See the code style guides.

Handling errors

If your database connection fails, you do catch the exception to echo a message, but then you just go on as if nothing has happened. What's the point of that? The same is true for the FindErrors() function. It echos after a form submission, but that's all it does. It just carries on and gets your code into real trouble.

User input

You filter user input at the beginning of your script, that is a good idea. However, it is still user input and should be treated with extra care. It is quite likely you will forget about this, now that you've dumped this input into normal PHP variables. Better put them in an array like $userInput or prefix the names like so: $input_forename. That way you remember to treat user input with care.

Another problem in FindErrors() is that you directly access $_POST. What's the point of all the input filtering at the beginning? Don't do this, use the variables, or the array, you created at the beginning.

Do not filter_input(INPUT_POST, 'password', FILTER_SANITIZE_STRING); the password. It could remove things from the password that an user intended to have there. It's a password, the content can be anything and doesn't need filtering.

Don't repeat yourself

You've implemented the same validation functions in PHP and in Javascript. I can understand why, but it does feel like duplication of functionality. I don't have a quick solution at hand, that would require long code examples, but try to avoid this. See: DRY

Database

Do not store plain-text passwords in a database. See: Never store passwords in a database!. Store a hash of the password, see: password_hash().

An obvious security problem is that your database query is open to SQL injection attacks. Use prepared statements instead. This is by far the most common problem we see, and it is actively exploited (because many developers can't be bothered).

Finally

That's my list so far. This is not a complete code review. I haven't given proper advice on how the code could be rewritten. Nevertheless I hope there are some useful tips here for you.

\$\endgroup\$
4
  • \$\begingroup\$ To be honest, I do not know why I would want to use the duplicate of the same functionality of php in javascript, why would I want to do that? Is one language better than the other? Wouldn't I be better off removing php code and use the javascript instead? I was following an example, but I did not know why. \$\endgroup\$ Sep 8, 2019 at 15:15
  • \$\begingroup\$ @AlejandroH I never said you should duplicate functionality in various languages. I was talking about problems with the coding style, and I meant to say that the same solutions apply to other languages as well. More so, further on I talk about the DRY principle, which says: Do not repeat yourself. \$\endgroup\$ Sep 8, 2019 at 21:59
  • \$\begingroup\$ Basically, what I am trying to say is that I know I shouldnt repeat myself. Does that mean I would be fine removing javascript code? Shouldnt I just be coding in PHP only and if I am going to code in PHP, and if javascript just code in javascript? \$\endgroup\$ Sep 9, 2019 at 22:19
  • \$\begingroup\$ No, most of the time you would use a combination of PHP and Javascript. PHP runs on the server, it has direct access to databases and other server resources. Javascripts runs (normally) in the browser, it can respond quickly to user actions, and manipulate the DOM. Both have their strengths and you should take advantage of these. What you should try to avoid is implementing the same functionality in PHP and Javascript. That's just unnecessary and complicates any future work on the code. \$\endgroup\$ Sep 10, 2019 at 7:51
1
\$\begingroup\$

To add onto KIKO's answer, you are on the right track.

Ultimately there is quite a bit you can change and there's a ton I would fix myself. But I'm not going to re-write the whole thing for you. Hopefully these pointers will help you move in the right direction.

First things first lets answer your question:

Your Question

How do you put the message below the table?

Quite easy actually. You can re-open PHP below that table and echo a variable or message. Right now you have:

if (empty($forename)) {
    echo "First name is required\n";
} else {
    $forename = fix_string($_POST["forename"]); 
}

Instead what you can do is store that error message in a variable

if (empty($forename)) {
    $error = "First name is required\n";
    // You could also unset the variable here too- Not necessary but good for memory clean up
    unset($forename);
} else {
    $forename = fix_string($_POST["forename"]); 
}

then below your table

<?php
if($error){
 echo $error
}
?>

With that you can also switch from the empty function to an exclamation mark operator

if(!$forename){
// error
}else{
// Good to go
}

Now let's get a bit more advanced

Instead of storing each error in an individual variable, create an array for errors

$err = [];

Then inside each if statement you can add that message to that array

$err[] = "Some Error";

And below your table use an iterator to get all of the values from that array

foreach($err as $key){
  echo $key;
}

Of course before you do any iteration, check to see if that array even has anything, you can achieve this by using the count function.

$num = count($err);
if ($num > 0 ){
 // Iterate
}

Session Start

First thing you have to change is where you're calling session_start take that and put it right at the very very top. In a vast majority of cases you should have it at the top before anything else. Only when using say for example an login with AJAX. But lets not get to ahead of ourselves.

Password

Same thing that KIKO said, you don't need to filter your password, at the very least change the 3rd parameter in it to FILTER_UNSAFE_RAW or just remove it - because you're hashing it there is no chance for a user to do anything. I know you've been taught and told several times to filter ALL inputs, this is the one and only exception to that rule. Once a password is hashed there's nothing left that PHP will mistake for code.

Functions

Should be declared before you try to use them. I'm not sure why they are at the bottom of your document, they should be at the top with all the other PHP. Not sure if you just pasted those in from an include file at the bottom just so they were there? If its a separate file just start a new code block with the little tilde's ```

Finishing Up

I think I covered everything that you were mainly asking. Like I said there a lot I would personally change. Hopefully that helps though.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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