Cleaner - Professional Code

Hi Guys,

I have self taught myself PHP / MYSQL over the past year, learning just enough and what I need to for each job that comes along.

Can I please get some constructive criticism on the following code?

Am I doing things correctly / securely enough, the least time consuming way ???

// check if the form was submitted
	if(isset($_POST['submitted']))
   	{


// Show errors, if any
  	ini_set ('display_errors', 1);  
  	error_reporting (E_ALL & ~E_NOTICE);

include 'db-connect.php';

// filter posted variables to guard against header injection	
function cleaner($data)
    {
        if(is_array($data))
        {
            $ret = array();
            foreach($data as $key=>$value)
            {
                $ret[$key] = cleaner($value);
            }
            return $ret;
        }
        else
        {
            if(!is_numeric($data))
            {
                if(get_magic_quotes_gpc())
                {
                    $data = stripslashes($data);
                }
                $data = mysql_real_escape_string($data);
            }
            return $data;
        }
    }    
    
	$clean = cleaner($_POST);
	// we then need to access each POSTED variable like this:  $clean['name'] 


// declare the variables that werent required (by the form) and or declare integars that didnt need to be cleaned
	$security = $_POST['security'];
	$job_description = $clean['job_description'];
	$vet_reg_num = $clean['vet_reg_num'];
	$fax_number = (int) $_POST['fax_number'];
	$type = $clean['type'];
	


// format the checkbox data - seperate with commas
foreach($clean['services'] as &$value) {
    	$value="".$value."";
	} 
	$services=implode(", ", $clean['services']);



// Initialize error array.
	$errors = array();


if(empty($clean['username'])){
	$errors[] = 'A username is required and must be alpha-numeric.';
	}


// validate the username
	$username = $clean['username'];
	if (!preg_match('/^[a-z\d_]{4,28}$/i', $username)) {
	$errors[] = 'A username is required and must be alpha-numeric.';
	}



// Make sure the username is available.
	include 'db-connect.php';

		$query = "SELECT username FROM Members WHERE username='$username'";		
		$result= mysql_query($query) or die(mysql_error());
		
		if (mysql_num_rows($result)) {
		$errors[] = 'Sorry, that username is already in use.';
		}
	mysql_close($db);
	


// Check for a password and match against the confirmed password.
	if (eregi ("^[[:alnum:]]{4,20}$", stripslashes(trim($clean['password'])))) {
	
		if ($clean['password'] == $clean['password_confirm']) {
			$password = $clean['password'];
		} else {
			$errors[] = 'Your password did not match the confirmed password!';
		}
		} else {
			$errors[] = 'Your password must be alpha-numeric.';
		}


if(empty($clean['password_confirm'])){
		$errors[] = 'Please confirm your password.';
	} else{ 
		$password_confirm = $clean['password_confirm'];
	}


if(empty($clean['title'])){
		$errors[] = 'Please enter your title.';
	} else{ 
		$title = $clean['title'];
	}
	

if(empty($clean['first_name'])){
		$errors[] = 'Please enter your first name.';
	} else{ 
		$first_name = $clean['first_name'];
	}


if(empty($clean['last_name'])){
		$errors[] = 'Please enter your last name.';
	} else{ 
		$last_name = $clean['last_name'];
	}


if(empty($clean['occupation'])){
		$errors[] = 'Please enter your occupation.';
	} else{ 
		$occupation = $clean['occupation'];
	}

	
if(empty($clean['prac_name'])){
		$errors[] = 'Please enter your practice name.';
	} else{ 
		$prac_name = $clean['prac_name'];
	}

	
if(empty($clean['prac_type'])){
		$errors[] = 'Please enter your practice type.';
	} else{ 
		$prac_type = $clean['prac_type'];
	}
	


// Check for an email address & make sure there are no errors.
	$email_address = $clean['email_address'];
	if (empty($email_address))
	{
		if (!eregi("^.+@.+\\..+$", $email_address))
		{
		$errors[] = 'Your email address contains errors.';
		}
		} 


// Make sure the email address is available.
	include 'db-connect.php';

		$query = "SELECT email FROM Members WHERE email='$email_address'";		
		$result= mysql_query($query) or die(mysql_error());
		
		if (mysql_num_rows($result)) {
		$errors[] = 'Sorry, that email address is already in use.';
		}
	mysql_close($db);



if(empty($clean['phone_number']) && ($clean['mobile_number'])){
	$errors[] = 'Please enter a phone number';
	} else{ 
		$phone_number = $clean['phone_number'];
		$mobile_number = $clean['mobile_number'];
	}


if(empty($clean['street'])){
		$errors[] = 'Please enter a street name.';
	} else{ 
		$street = $clean['street'];
	}
	
	
if(empty($clean['city'])){
		$errors[] = 'Please enter a city.';
	} else{ 
		$city = $clean['city'];
	}

	
if(empty($clean['state'])){
		$errors[] = 'Please select a state.';
	} else{ 
		$state = $clean['state'];
	}
	

if(empty($clean['postcode'])){
		$errors[] = 'Please enter your postcode.';
	} else{ 
		$postcode = $clean['postcode'];
	}

	
if(empty($clean['country'])){
		$errors[] = 'Please enter your country.';
	} else{ 
		$country = $clean['country'];
	}


// Check the security field has been answered correctly
	if(($security) !== "2"){
		$errors[] = "You answered the security question wrongly. 1 + 1 = 2)";
	}

// If everything went okay and there were no errors, continue.	
	if (empty($errors)) { 
	
	
	
// format the date & time
	$now = time();
	$thisYear = date("Y-m-d H:i:s", $now);	

	
// Create the activation code
	$a = md5(uniqid(rand(), true));



require 'db-connect.php';


   $sql = "INSERT into Members (
	username,
	pass,
	title,
	first_name,
	last_name,
	occupation,
	job_description,
	vet_reg_num,
	email,
	prac_name,
	street,
	city,
	state,
	postcode,
	country,
	phone,
	mobile,
	fax,
	type,
	services,
	activation_code,
	date_reg) 
   VALUES (
	
	'$username',
	SHA('$password'), 
	'$title',
	'$first_name',
	'$last_name',
	'$occupation',
	'$job_description',
	'$vet_reg_num',
	'$email_address',
	'$prac_name',
	'$street',
	'$city',
	'$state',
	'$postcode',
	'$country',
	'$phone_number',
	'$mobile_number',
	'$fax_number',
	'$prac_type',
	'$services',
	'$a',
	'$thisYear') ";

       mysql_query($sql) or die(mysql_error());

	echo 'data added!';
	
		mysql_close($db);
			exit();


// if errors array contains a value		
	} else {
	echo '<table align="center"><tr><td><b>The following error(s) occurred:</b><br /><blockquote>';
		foreach ($errors as $msg) {
		echo "* $msg<br />\n";
		}
	echo '</font></blockquote></td></tr></table><br /><br />';
	}	




// if the form wasn't submitted - display the reg form
		}

PHP:

 

if(!is_numeric($data))
Check that line on your cleaner function...I prefer using this

if(!ctype_digit($data)) //<---more secure

if(!ctype_digit($data))
{
  if(!get_magic_quotes_gpc())
    {
       $data = mysql_real_escape_string($data);
    }
return $data;

PHP:

 

If you're interested in why specifically you should use it, ctype_digit will make sure that all the characters in a string are integers - note that it doesn't allow decimal points (but before php 5.1 it does return true for empty strings). is_numeric is a bad function to use because it returns true for all sorts of mathematical number expressions.

With regards to your actual code however, I would suggest you verify that the given country is a real country (provide a dropdown box with the list of countries in, then check the submitted value is the same as one of them). There's no need to filter the input with the cleaner - simply give each country in the list an integer that is it's position in the list, then use the ctype_digit function and an if statement to check it is an integer from the list.

Simply enter that integer into the database as a tinyint datatype, and then match up the integer with the country when you are displaying the country. This will reduce the size (increasing speed, fractionally) of your database. With databases, repeat as little information in the database as possible, within reason - this is a good example of what is reasonable. Of course, things like birth dates aren't reasonable - they should be put in as dates (that is, as a single 8 digit string of #s), not with a number for every day of every year.

 

Here's another way of writing your cleaner function by using pass by reference.

function cleaner(&$value){     
   
   if(!get_magic_quotes_gpc()){      
            
         $value = mysql_real_escape_string($value);
                 
      }//end if    

}//end cleaner function

array_walk($_POST,'cleaner');   //<----check all the post array elements
 

$user_name =  $_POST['username']; //<-----post is clean to use now

PHP:

 

The way he did it was right, though:

if(get_magic_quotes_gpc())
{
    $data = stripslashes($data);
}

$data = mysql_real_escape_string($data);

PHP:

Your method relies on magic_quotes, which are not safe enough. If they're enabled, you should STRIP the slashes, and THEN use mysql_real_escape_string(). (Like he did!). Your method above also doesn't support arrays.

Also, I'd add a trim() to your function. All your empty() checks would be useless if the user entered empty spaces.

And I'm not sure what the point of this is:

$value="".$value."";

PHP:

...?

 

you also only need to include the require 'db-connect.php'; once, to make sure it is not included twice remove 1 of them or use require_once or include_once instead.

 

Thanks for the awesome replies guys

Any thoughts on how I could best consolidate all the 'if empty validations' as they are very time consuming to type out ?

 

Make an array with the NAMES of all required fields. Use array_filter() on the array the user submitted. After that, all empty fields will be gone, then you can use array_diff() and array_keys() to compare these two arrays, and if they're not equal, get the "difference" of the array and tell the user to fill out these fields.

 

Correct me if I'm wrong.

if(get_magic_quotes_gpc())  //<---when get_magic_qutoes_gpc is on, it will automatically escape the $data.
{
    $data = stripslashes($data);  //<---in here, you change back the $data to it's original value without escape.
}

$data = mysql_real_escape_string($data);//<---in here you escape the $data again, so I though if get_magic_quotes_gpc is on, then it automatically escape the $data but when it coming down to mysql_real_escape_string($data) then you double the escape of the data.

//That stipslashes is a waste of resources $data = stripslashes($data) if get_magic_quotes_gpc() is on.

PHP:

 

magic_quotes is the same as applying addslashes(), which doesn't escape all necessary characters. mysql_real_escape_string() is much safer than addslashes(). Even on php.net they suggest to strip the slashes first:

http://www.php.net/mysql_real_escape_string (Example #3)

 

I found this function from php.net http://www.php.net/mysql_real_escape_string

It's clarify why using of stripslashes. It seems like mysql_real_escape_string() is much secure and stronger than addslashes.

function mysql_prep($value){
    $magic_quotes_active = get_magic_quotes_gpc();
    $new_enough_php = function_exists("mysql_real_escape_string");
    // i.e PHP >= v4.3.0
    if($new_enough_php){
    //undo any magic quote effects so mysql_real_escape_string can do the work
    if($magic_quotes_active){
        $value = stripslashes($value);
    }
    $value = mysql_real_escape_string($value);
    }else{ // before PHP v4.3.0
        // if magic quotes aren't already on this add slashes manually
        if(!$magic_quotes_active){
            $value = addslashes($value);
        } //if magic quotes are avtive, then the slashes already exist
    }
    return $value;
} 

PHP: