I'm developing a PHP contact form script for my first site, using PHPMailer. I'm mainly concerned about security issues since I'm rather new to PHP. I've learnt a lot from reading several code reviews here, which helped me improve my code and make it more secure but I would be surprised if there's nothing more to improve in that regard. Also, would you recommend me to filter my $subject and $message variables using filter_var like so?
filter_var($myVar, FILTER_SANITIZE_STRING);
Please share your thoughts, any feedbacks would be appreciated. Thanks.
Here's the form (contact.php):
<?php
session_start();
$firstName = $_SESSION['inputs']['firstName'];
$lastName = $_SESSION['inputs']['lastName'];
$email = $_SESSION['inputs']['email'];
$subject = $_SESSION['inputs']['subject'];
$message = $_SESSION['inputs']['message'];
?><!DOCTYPE html>
<!-- ... -->
<?php
// Display errors
if (array_key_exists('errors', $_SESSION)) {
echo
'<div class="alert alert-error">
<ul>
<li>'.implode('</li>
<li>', $_SESSION['errors']).'</li>
</ul>
</div>';
// Display 'success' message
} elseif (array_key_exists('success', $_SESSION)) {
echo
'<div class="alert alert-success">
<p>Your message has been successfully sent.</p>
</div>';
}
?>
<form class="form" action="process_form.php" method="POST">
<div class="form_details">
<input type="text" name="firstName" placeholder="First name" value="<?php echo htmlspecialchars($firstName, ENT_QUOTES, 'utf-8'); ?>"/>
<input type="text" name="lastName" placeholder="Last name" value="<?php echo htmlspecialchars($lastName, ENT_QUOTES, 'utf-8'); ?>"/>
<input type="email" name="email" placeholder="E-mail" value="<?php echo htmlspecialchars($email, ENT_QUOTES, 'utf-8'); ?>"/>
<input type="text" name="subject" maxlength="100" placeholder="Subject" value="<?php echo htmlspecialchars($subject, ENT_QUOTES, 'utf-8'); ?>"/>
</div>
<div class="form_message">
<textarea name="message" placeholder="Your message"><?php echo htmlspecialchars($message, ENT_QUOTES, 'utf-8'); ?></textarea>
</div>
<input type="submit" name="submit" value="Submit"/>
</form>
<!-- ... -->
And the script to process it (process_form.php):
<?php
function sanitize_input($input) {
$input = str_ireplace(array('\r', '\n', '%0a', '%0d', '0x0A'), '', $input);
$input = trim($input);
return $input;
}
# ---- DEFINE VARIABLES ---- #
$errors = [];
$firstName = $inputs['firstName'] = sanitize_input($_POST['firstName']);
$lastName = $inputs['lastName'] = sanitize_input($_POST['lastName']);
$email = $inputs['email'] = sanitize_input($_POST['email']);
$subject = $inputs['subject'] = sanitize_input($_POST['subject']);
$message = $inputs['message'] = sanitize_input($_POST['message']);
$name = [$firstName, $lastName];
# ---- PROCESS THE INPUTS AND GENERATE ERRORS ---- #
if ($firstName == '' && $lastName == '') {
$errors['name'] = 'Your name is required.';
} elseif ($firstName == '') {
$errors['name'] = 'Your first name is required.';
} elseif ($lastName == '') {
$errors['name'] = 'Your last name is required.';
} elseif (preg_grep("/^\p{L}*(?>[- ']\p{L}*)*$/u", $name, PREG_GREP_INVERT)) {
$errors['name'] = "Your name may only contain letters, whitespaces, - or '.";
}
if ($email == '') {
$errors['email'] = 'Your e-mail address is required.';
} elseif (!filter_var($email, FILTER_VALIDATE_EMAIL)) {
$errors['email'] = 'Please enter a valid e-mail address.';
}
if ($subject == '') {
$errors['subject'] = 'A subject is required.';
}
if ($message == '') {
$errors['message'] = 'A message is required.';
}
# ---- SEND ERRORS TO USER IF THERE IS ANY, OTHERWISE SEND MESSAGE ---- #
session_start();
if (!empty($errors)) {
$_SESSION['errors'] = $errors;
$_SESSION['inputs'] = $inputs;
header('Location: contact.php');
exit;
} else {
require_once('phpmailer/PHPMailerAutoload.php');
$config = include('../config.ini.php');
date_default_timezone_set($config['mail']['timezone']);
$mail = new PHPMailer();
$mail->isSMTP();
$mail->Host = $config['mail']['host'];
$mail->SMTPAuth = true;
$mail->Username = $config['mail']['username'];
$mail->Password = $config['mail']['password'];
$mail->SMTPDebug = 0;
$mail->SMTPSecure = $config['mail']['connection'];
$mail->Port = $config['mail']['port'];
$mail->AddAddress($config['mail']['mailAddress']);
$mail->AddReplyTo($email, $firstName.' '.$lastName);
$mail->FromName = $firstName.' '.$lastName;
$mail->Subject = $subject;
$mail->Body = $message;
if (!$mail->Send()) {
$errors['notSent'] = 'The message could not be sent.';
$errors['errorInfo'] = 'Error returned: '.'"'.$mail->ErrorInfo.'".';
$_SESSION['errors'] = $errors;
$_SESSION['inputs'] = $inputs;
header('Location: contact.php');
exit;
} else {
$_SESSION['success'] = 1;
header('Location: contact.php');
}
}
1 Answer 1
At a quick glance...
Validation
You are setting variables with the value of $_POST which may not have been set as a result you get errors, consider a format where you only set named variables when you know the validation is complete successfully, until then use raw $_POST.
You mention you are new to PHP, so make isset, empty and trim your friends when it comes to checking if data has been passed through by the user.
So,
if ($firstName == '' && $lastName == '') [...]
becomes,
if (!isset($firstName) || empty(trim($firstName)) && !isset($lastName) || empty(trim($lastName))) {}
You can also make use of the validatation filters provided for filter_var or the ctype_* functions.
Do you fields need to be a specific maximum/minimum length? If so, you have strlen at your disposal.
Sanitization
The fact that you are considering sanitization is already a good sign!
The rule of thumb is that you never have one function for everything when it comes to sanitization. Generally because it won't need all those cleaning functions. So instead, you create generic functions for the data types and if need be more specific functions for others fields such as usernames.
You have for integers intval and for floats floatval and for strings and bools you have strval and boolval.
Consider reading OWASP's in-depth article in regards to security in PHP.
Lastly (for now), it is generally considered good practice to use exit when you do a redirection to actually halt script execution.
-
\$\begingroup\$ Thank you very much for the feedback and suggestions. I'm going to read the links and make the changes you recommended. \$\endgroup\$Junitar– Junitar2017年07月03日 06:22:47 +00:00Commented Jul 3, 2017 at 6:22