-5
\$\begingroup\$

I have created a php contact form and have tried to make it relatively secure. I've added a honeypot field that should be left blank, code to ensure all required fields are filled out and code to prevent email injection. I realise there are endless ways to tighten up on security but I am trying to keep this script lightweight whilst covering most of the basic security issues.

Are there any simple modifications that would improve the security of this script? Here is my current code:-

Edit: added " as suggested.

<?php 
 if (empty($_POST['honeypot'])){ 

$youremail = 'michael@example.com';
$body = "This is the result of your contact form:     

First Name: $_POST[firstname]
Last Name: $_POST[lastname]
E-Mail: $_POST[email]
Telephone: $_POST[telephone]
Message: $_POST[message]"; 

if( $_POST['email'] && !preg_match( "/[\r\n]/", $_POST['email']) ) {
$headers = "From: $_POST[email]";     
 } else {
 $headers = "From: $youremail"; 
 }

 if($_POST["message"] == "" || $_POST["firstname"] == "" ||     $_POST["lastname"] == "" || $_POST["telephone"] == "" || $_POST["message"] ==     ""){
echo "<p>Please fill in all fields</p>";
} else {
// finally, send the message     
mail($youremail, 'Contact Form', $body, $headers ); } 
header('Location: http://www.example.com/thanks/');
exit();
?>
\$\endgroup\$
1
  • \$\begingroup\$ Welcome to Code Review. Please fully test your script before asking for a review. \$\endgroup\$ Jul 3, 2015 at 18:16

1 Answer 1

3
\$\begingroup\$

I would improve this by using filter_var for validating the email address instead of your preg_match:

if( $_POST['email'] && filter_var($_POST['email'], FILTER_VALIDATE_EMAIL) ) {
    $headers = "From: ";     
} else {
    $headers = "From: $youremail"; 
}

Codewise, there are a couple of improvements I would make, mostly in the realm of verifying all your variables and string before doing anything with them. I usually go by creating an array with successes and one with errors, this way you can provide more useful feedback to the user as well, reporting what went wrong.

It looks more complicated, but it is more robust as well. By assigning and checking existence of variables, you are playing it safe. Now, you could also assign this to actual variables and use those, but then you have to do a check in the end again to find out which were right and which were wrong. I prefer this long-winded route of safety.

<?php 
if (empty($_POST['honeypot'])){ 

// Set a global $errors variable.
$errors = array();
$success = array();

$youremail = 'michael@example.com';
// It is not very advised to use this string interpolation. Safer to string it together. It is less error prone and your variables are more visually obvious.
// You also need to check that they are not empty, as empty inputs don't get posted.
$body  = 'This is the result of your contact form: \n';

!empty($_POST['firstname']) 
    ? $success['firstname'] = $_POST['firstname'] 
    : $errors[] = 'firstname';
$body .= !empty($success['firstname'])
    ? 'First Name: ' . $success['firstname'] . ' \n'
    : '';

!empty($_POST['lastname'])
    ? $success['lastname'] = $_POST['lastname']
    : $errors[] = 'lastname';
$body .=  !empty($success['lastname']) 
    ? 'Last Name: ' . $success['lastname'] . ' \n'
    : '';

// We will check if the email address is valid here.
!empty($_POST['email']) && filter_var($_POST['email'], FILTER_VALIDATE_EMAIL)
    ? $success['email'] = $_POST['email']
    : $errors[] = 'email';
$body .= !empty($success['email'])
    ? 'E-Mail: ' . $success['email'] . ' \n'
    : '';

!empty($_POST['telephone'])
    ? $success['telephone'] = $_POST['telephone']
    : $errors[] = 'telephone';
$body .= !empty($success['telephone'])
    ? 'Telephone: ' . $success['telephone'] . ' \n'
    : '';

// Use filter_var to sanitise this user string input.
!empty($_POST['message']) && filter_var($_POST['message'], FILTER_SANITIZE_STRING) 
    ? $success['message'] = $_POST['message']
    : $errors[] = 'message';
$body .= !empty($success['message'])
    ? 'Message: ' . $success['message'] . ' \n'
    : '';

if(!empty($success['email'])) {
    $headers = 'From: ' . $success['email'];     
} else {
    $headers = 'From: ' . $youremail; 
}

if()

if(!empty($errors)){
    echo "<p>The following errors have occured: " . implode(",",$errors) . "</p>";
} else {
// finally, send the message     
    mail($youremail, 'Contact Form', $body, $headers ); } 
    header('Location: http://www.example.com/thanks/');
}
// Always close your blocks - it's just common sense. Even though it won;t fail here, it makes your code look bad. Also, always indent.
// exit does not need the `()` is you are not passing it anything.
exit;
?>
\$\endgroup\$
8
  • \$\begingroup\$ @somethinghere..wow what a helpful response. Thank you. Someone else advised me to use the filter_var function. Will this act the same as my !preg_match to prevent malicious email injection like CC or BCC etc within that field? \$\endgroup\$
    – Michael_G
    Jul 3, 2015 at 16:23
  • \$\begingroup\$ Filter_var returns false (aka no matches) when it doesn't fit the bill, so yeah. Have a search for php.net's page on sanitising filters. Useful, built-in stuff. (I've just left an easy googling space) good luck. \$\endgroup\$ Jul 3, 2015 at 16:25
  • \$\begingroup\$ Also..most form codes just looks for fields to be filled in, and if so the form is sent. Obviosuly my honeypot field is the exception as the intended behaviour is for it to be left empty otherwise the form shouldn't be sent. Is my coding for this at the top correct - and it won't conflict with any of the other fields? \$\endgroup\$
    – Michael_G
    Jul 3, 2015 at 16:30
  • \$\begingroup\$ This should work. There is another solution, but as Fermat would have said, 'this comment box is too small to describe it in' :) \$\endgroup\$ Jul 3, 2015 at 16:33
  • 1
    \$\begingroup\$ @ismaelmiguel yeah, i read a stackoverflow post on it and it does seems complicated but FILTER_VALIDATE_EMAIL seems quite robust. This accepted answer is enlightening tbh: stackoverflow.com/questions/19220158/… \$\endgroup\$ Jul 3, 2015 at 17:03

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