Help improving / reviewing some PHP code

Major_Disaster · 15 Jun 2010 at 14:14

Hello all,

A few months back i spent ages teaching myself some PHP and producing the contact form shown below. It was really my first proper go at using PHP and i was just wondering whether you Pro's could have a look over it and spot any glaring errors or security issues?

I was also wondering if someone could show me how to speed up parts of it (by looping?) becuase as you can see a lot of the code is repetitive.

Here the code:

PHP:

//Set blacklists
	$badwords = "/(naughty|words)/i";
	$exploits = "/(content-type|bcc:|cc:|document.cookie|onclick|onload|javascript)/i";
	$bots = "/(Indy|Blaiz|Java|libwww-perl|Python|OutfoxBot|User-Agent|PycURL|AlphaServer|T8Abot|Syntryx|WinHttp|WebBandit|nicebot)/i";

	//Check for any bots
	if(preg_match($bots, $_SERVER['HTTP_USER_AGENT'])) {
		die('<h1>Spam bots are not welcome.</h1>');
	}
	
	//Check if the user has sent a message in the last sixty seconds
	$timeLimit = $_SESSION['lastMailed'] + 60 < time();

	if (!$timeLimit) {
		die('<h1>Whoah, slow down there!</h1>
			<p><a href="javascript:history.go(-1)">Please go back and try sending your enquiry again.</a></p>
			');
	}
	
	//reCaptcha
	require_once('recaptchalib.php');
	$privatekey = "**********";
	$resp = recaptcha_check_answer ($privatekey,
		$_SERVER["REMOTE_ADDR"],
		$_POST["recaptcha_challenge_field"],
		$_POST["recaptcha_response_field"]);
	if (!$resp->is_valid) {
		die ('<h1>The reCAPTCHA text wasn\'t entered correctly</h1>
			 <p><a href="javascript:history.go(-1)">Please go back and try it again.</a></p>
			 ');
	}
	
	//Check to make sure that the First Name field is not empty, and that it does not contain badwords or exploits
	if(trim($_POST['firstName']) == '' ) {
		$error['firstName'] = "You didn't enter your <b>First Name</b>.";
	} else if (preg_match($badwords, trim($_POST['firstName'])) !== 0 || preg_match($exploits, trim($_POST['firstName'])) !== 0) {
		$error['firstName'] = "You entered a <b>First Name</b> which contains unacceptable or explicit words.";
	} else {
		$firstName = trim(stripslashes(strip_tags($_POST['firstName'])));
		unset($error['firstName']);
	}
	
	//Check to make sure that the Last Name field is not empty, and that it does not contain badwords or exploits
	if(trim($_POST['lastName']) == '' ) {
		$error['lastName'] = "You didn't enter your <b>Last Name</b>.";
	} else if (preg_match($badwords, trim($_POST['lastName'])) !== 0 || preg_match($exploits, trim($_POST['lastName'])) !== 0) {
		$error['lastName'] = "You entered a <b>Last Name</b> which contains unacceptable or explicit words.";
	} else {
		$lastName = trim(stripslashes(strip_tags($_POST['lastName'])));
		unset($error['lastName']);
	}

	//Check to make sure sure that a valid Email address is submitted
	if(trim($_POST['email']) == '') {
		$error['email'] = "You didn't enter your <b>Email</b> address.";
	} else if (!preg_match('/([a-z0-9])([-a-z0-9._])+([a-z0-9])\@([a-z0-9])([-a-z0-9_])+([a-z0-9])(\.([a-z0-9])([-a-z0-9_-])([a-z0-9])+)*/i', trim($_POST['email'])) || preg_match($badwords, trim($_POST['email'])) !== 0 || preg_match($exploits, trim($_POST['email'])) !== 0) {
		$error['email'] = "You didn't enter a valid <b>Email</b> address.";
	} else {
		$email = trim(stripslashes(strip_tags($_POST['email'])));
		unset($error['email']);
	}

	//Check to make sure that the Company Name field is not empty, and that it does not contain badwords or exploits
	if(trim($_POST['companyName']) == '') {
		$error['companyName'] = "You didn't enter your <b>Company Name</b>.";
	} else if (preg_match($badwords, trim($_POST['companyName'])) !== 0 || preg_match($exploits, trim($_POST['companyName'])) !== 0) {
		$error['companyName'] = "You entered a <b>Company Name</b> which contains unacceptable or explicit words.";
	} else {
		$companyName = trim(stripslashes(strip_tags($_POST['companyName'])));
		unset($error['companyName']);
	}
	
	//Check to ensure the Telephone Number field is valid
	if(preg_match($badwords, trim($_POST['telephone'])) !== 0 || preg_match($exploits, trim($_POST['telephone'])) !== 0) {
		$error['telephone'] = "You entered a <b>Telephone Number</b> which contains unacceptable or explicit characters.";
	} elseif (trim($_POST['telephone']) == '') {
		$telephone = "Not specified";
	} else {
		$telephone = trim(stripslashes(strip_tags($_POST['telephone'])));
		unset($error['telephone']);
	}
	
	//Check to ensure the Website field is valid
	if(preg_match($badwords, trim($_POST['website'])) !== 0 || preg_match($exploits, trim($_POST['website'])) !== 0) {
		$error['website'] = "You entered a <b>Website</b> address which contains unacceptable or explicit characters.";
	} elseif (trim($_POST['website']) == '') {
		$website = "Not specified";
	} else {
		$website = trim(stripslashes(strip_tags($_POST['website'])));
		unset($error['website']);
	}

	//Check to make sure a Message was entered
	if(trim($_POST['message']) == '') {
		$error['message'] = "You didn't enter a <b>Message</b>.";
	} else if (preg_match($badwords, trim($_POST['message'])) !== 0 || preg_match($exploits, trim($_POST['message'])) !== 0) {
		$error['message'] = "You entered a <b>Message</b> which contains unacceptable or explicit words.";
	} else {
		$message = trim(stripslashes(strip_tags($_POST['message'])));
		unset($error['message']);
	}
		
	//Get title and country
	$title = $_POST['title'];
	$country = $_POST['country'];
	
	//If there are any errors
	if(isset($error)) { 
		echo '<h1>Your Message Has Not Been Sent</h1>';
		echo '<p>Unfortunately your message was <b>not</b> sent as the following errors were detected:</p>';
		echo '<br />';
		echo '<ul>';
		if(isset($error['firstName'])){
			echo '<li> '.$error['firstName'].' </li>';
		}
		if(isset($error['lastName'])){
			echo '<li> '.$error['lastName'].' </li>';
		}
		if(isset($error['email'])){
			echo '<li> '.$error['email'].' </li>';
		}
		if(isset($error['companyName'])){
			echo '<li> '.$error['companyName'].' </li>';
		}
		if(isset($error['telephone'])){
			echo '<li> '.$error['telephone'].' </li>';
		}
		if(isset($error['website'])){
			echo '<li> '.$error['website'].' </li>';
		}
		if(isset($error['message'])){
			echo '<li> '.$error['message'].' </li>';
		}
		echo '</ul>';
		echo '<br />';
		echo '<p>Please <a href="javascript:history.go(-1)">go back and correct these errors.</a></p>';
	}
	
	//If there are no errors in the error array, send the email
	else if(!is_array($error)) {

And then the email is sent.

Cheers for the help

Major_Disaster · 15 Jun 2010 at 20:19

sldsmkd said:
Look at http://php.net/manual/en/book.filter.php

That looks really useful. Wish i would have seen that sooner! Are these recent additions to PHP? Will they become standard?

Major_Disaster · 20 Jun 2010 at 22:52

Im having a look at these filters, and was wondering if this is ok:

PHP:

$part_search = trim(filter_var($_POST['part-number'], FILTER_SANITIZE_STRING));

This is just the input for a part search, which searches a MySQL database. However this is my first play with PHP / MySQL so im getting worried about security.

Should i be worried about people entering URL's?

Any improvements?

Major_Disaster · 21 Jun 2010 at 00:09

Thanks for the reply.

Its my first time using these functions too (and dealing with MySQL in general). How would i go about escaping ', addslashes? Or anything better?

Major_Disaster · 21 Jun 2010 at 01:22

Thanks for all the replies.

Im getting really confused with escaping characters and best practices. Hows this:

PHP:

$part_search = trim(filter_var($_POST['part-number'], FILTER_SANITIZE_STRING));

PHP:

		$sanitized_part_search = mysql_real_escape_string($part_search);
		$query = mysql_query("SELECT * FROM `stock_search` WHERE `part_number` LIKE '%$sanitized_part_search%'") or die(mysql_error());

That seems to double escape characters, but if i add in stripslahes then mysql_real_escape_string doesnt seem to do anything?

Can someone show me a best practises template?

Major_Disaster · 21 Jun 2010 at 02:02

Thanks again for the help.

I cant seem to find the escape character option - do you mean one of the strip character flags? Ill definitely have a look at the MySQLi - i tried shoving an i on the end of everything but it just broke - so i will read up.

The data is part numbers, but they are quite varied. Letters, numbers, spaces, dashes, forward slashes, spaces and dots are all acceptable - hence why im having such a hard time!

Major_Disaster · 21 Jun 2010 at 17:37

Thanks for all the replies.

I think ive straightened out magic quotes, now im struggling with MySQLi and Prepared Statements. This is what i have:

PHP:

// Create a new mysqli object with database connection parameters
		$link = mysqli_connect('localhost', 'user', 'pass', 'db');
		
		if(mysqli_connect_errno()) {
			echo "Connection Failed: " . mysqli_connect_errno();
			echo mysqli_error();
		exit();
		}
		
		// Setup query
		$query = "SELECT * FROM `stock_search` WHERE `part_number` LIKE (?)";
		
		// Paramater to be bound
		$part_search = trim(filter_var($_POST['part-number'], FILTER_SANITIZE_STRING, FILTER_FLAG_NO_ENCODE_QUOTES));
		
		// Get instance of statement
		$stmt = mysqli_stmt_init($link);

		// Prepare Query
		if(mysqli_stmt_prepare($stmt, $query)){

			/* Bind parameters
			s - string, b - boolean, i - int, etc */
			 mysqli_stmt_bind_param($stmt, "s", $part_search);

			/* Execute it */
			mysqli_stmt_execute($stmt);

			/* Bind results */
			mysqli_stmt_bind_result($stmt, $part_number, $manufacturer, $stock);

			/* Fetch the value */
			 mysqli_stmt_fetch($stmt);
			 
			 $count = mysqli_num_rows($stmt);
			 echo $count;

			/* Close statement */
			 mysqli_stmt_close($stmt);
		}

		// Close Connection
		mysqli_close($link);

Im trying to find the number of rows, but getting the error:

Code:

Warning: mysqli_num_rows() expects parameter 1 to be mysqli_result, object given

Referencing " $count = mysqli_num_rows($stmt);"

Any ideas?