Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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 />";


}

?>
share|improve this question
add comment

1 Answer

up vote 1 down vote accepted
${$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.
share|improve this answer
    
I dont have the minimum rep required to vote your answer up. So +1 and thanks a lot. –  user1463541 Feb 7 '13 at 21:40
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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