Form validation script

Question

I have a simple form validation script. Could you tell me how I can improve this script (mainly regarding security)?

HTML page

<form name="input_form" action="process_form.php" method="post">

    <p>
    <label for="name">Name</label>
    <input type="text" name="name" id="name" maxlength="25" />
    </p>

    <p>
    <label for="password">Password</label>
    <input type="password" name="password" id="password" maxlength="25" />
    </p>

    <p>
    <label for="email">Email</label>
    <input type="text" name="email" id="email" maxlength="100" />
    </p>


    <p>
    <label for="website">Website</label>
    <input type="text" name="website" id="website" maxlength="100" />
    </p>

    <p>
    <label for="feedback">Feedback</label>
    <textarea name="feedback"></textarea>
    </p>

    <p>
    <label for="sex">Sex</label>
    <input type="radio" name="sex" value="male" />Male
    <input type="radio" name="sex" value="female" />Female
    </p>

    <p>
    <label for="hobbies">Hobbies</label>
    <input type="checkbox" name="hobbies[]" value="reading" />Reading
    <input type="checkbox" name="hobbies[]" value="basketball" />Basketball
    <input type="checkbox" name="hobbies[]" value="football" />Football
    </p>


    <p>
    <label for="country">Country</label>
    <select name="country">
        <option value="0">Select</option>
        <option value="india">India</option>
        <option value="england">England</option>
        <option value="usa">USA</option>
    </select>
    </p>

    <p>
    <label for="education">Education</label>
    <select multiple="multiple" name="education[]">
        <option value="high_school">High School</option>
        <option value="degree">Degree</option>
        <option value="phd">Phd</option>
    </select>
    </p>

    <p>
        <input type="submit" name="submit" id="submit" value="Submit" />
        <input type="reset" name="reset" id="reset" value="Reset" />
    </p>

</form>

process_form.php

<?php

if(isset($_POST['submit']))
{
    // white list
    $expected_elements = array('name','password','email','website','feedback','country');


    // create dynamic variables with same name as the form elements
    foreach ($expected_elements as $key) 
    {
        ${$key} = $_POST[$key];
    }

    $error = "";

    /*
    * This array will store the names of those form elements whose value shouldn't be empty
    */
    $check_empty_vars = array('name','password','email','website');

    /*
    * This array will store the names the form elements and the corresponding max length allowed
    */
    $max_allowed_length = array(
            "name" => 25,
            "password" => 25,
            "email" => 100
        );


    foreach($check_empty_vars as $var_name)
    {
        if(${$var_name} == "")     
        {
            $error .= ucfirst($var_name) . " cannot be empty <br />";
        }
    }

    foreach ($max_allowed_length as $key => $value) 
    {
        if(strlen(${$key}) > $value)
        {
            $error .= ucfirst($key) . " cannot be greater than $value characters <br />";
        }
    }

    // letters and spaces only
    if( ! preg_match('/^[A-Za-z\s ]+$/', $name))
    {
        $error .= "Name should contain only letters and spaces <br />";
    }

    if($email != "")
    {
        if( ! filter_var($email, FILTER_VALIDATE_EMAIL))
        {
            $error .= "Enter a valid email <br />";
        }   
    }

    if($website != "")
    {
        if( ! filter_var($website, FILTER_VALIDATE_URL))
        {
            $error .= "Enter a valid website address <br />";
        }
    }

    if( ! isset($_POST['sex']))
    {           
        $error .= "Select Sex <br />";
    }

    else
    {
        switch($_POST['sex'])
        {
            case 'male':
            case 'female':
            $sex = $_POST['sex']; 
            break;
        }
    }

    if( ! isset($_POST['hobbies']))
    {
        $error .= "Select Hobbies <br />";
    }

    else
    {
        switch($_POST['hobbies'])
        {
            case 'reading':
            case 'basketball':
            case 'football':
            $hobbies = $_POST['hobbies']; 
            break;
        }
    }   

    if($country == "0")
    {
        $error .= "Select country <br />";
    }   

    if( ! isset($_POST['education']))
    {
        $error .= "Select education <br />";
    }

    else
    {
        switch($_POST['education'])
        {
            case 'high_school':
            case 'degree':
            case 'phd':
            $education = $_POST['education']; 
            break;
        }
    }

    echo $error. "<br />";


}

?>

Answer 1

${$key}

This makes me somewhat nervous, but I guess it's acceptable in this context since it's whitelisted.

---

foreach($check_empty_vars as $var_name)
{
    if(${$var_name} == "")     
    {
        $error .= ucfirst($var_name) . " cannot be empty <br />";
    }
}

I'd recommend moving this right after the $check_empty_vars to reduce the variable live time. This would have the pleasant side-effect of also moving the $max_allowed_length declaration close to its foreach loop.

---

foreach ($expected_elements as $key) 
{
    ${$key} = $_POST[$key];
}

What happens if the user submits a form without a value of $key? Depending on your warning level, you may get a Notice: Undefined index. You may want to add isset or array_key_exists to that loop.

---

In [A-Za-z\s ], \s already includes whitespace. Also, your name filtering choices are incomplete. What if my name includes other characters?

Some people may report their names as having a ', or á, í, etc.

---

You have your validating logic and your display logic in a single script. This might be problematic in terms of maintenance. See separation of concerns.

A comment in the official PHP documentation page for filter_var claim that http://example.com/"><script>alert(document.cookie)</script> is accepted as a valid URL. Beware.

---

switch($_POST['sex'])
{
    case 'male':
    case 'female':
    $sex = $_POST['sex']; 
    break;
}

What if I send a POST request with a sex of robot? Add an appropriate error message.

---

switch($_POST['hobbies'])
{
    case 'reading':
    case 'basketball':
    case 'football':
    $hobbies = $_POST['hobbies']; 
    break;
}

Isn't hobbies supposed to be an array? (Not a single variable) What I said about invalid input in $_POST['sex'] also applies here.

---

Education has the same array-related problem.

---

You should validate country too. I send a bogus POST request saying I'm from "The Moon" and your code won't complain.

---

Is there a particular reason to giving the form and input reset elements a name?

---

I can't tell if it's safe without seeing the PHP code that is executed later in that same page. The usual PHP security recommendations apply, though:

• Use prepared statements for database queries (and preferably isolate it in its own layer) to prevent SQL injections
• Properly escape your fields before echoing them to prevent XSS injections.
• If this is a registration form, you might want to add some protection against bots (such as displaying a CAPTCHA if too many requests are submitted).
• If this is a form that's displayed after the user has logged in, make sure you're protected against CSRF attacks.