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.

• user1463541: I dont have the minimum rep required to vote your answer up. So +1 and thanks a lot.