Basic contact form

Question

This is for commercial purposes. It's not going to a database, but will just be emailed to a person via phpMailer.

I am not good at PHP (truly awful). I'd like to know whether this is good enough for going production or not.

<?php
session_start(); //allows use of session variables

if ($_SERVER["REQUEST_METHOD"] == "POST" && !empty($_SESSION['token']) && $_SESSION['token'] == $_POST['token']) {

      $token = md5(uniqid(mt_rand(), true));
      $_SESSION['token'] = $token;
      $_SESSION['token_time'] = time();


   if (empty($_POST["first-name"])) {
     $firstNameErr = "First name is required";
   } else {
     $first_name = test_input($_POST["first-name"]);
   }

    if (empty($_POST["last-name"])) {
     $lastNameErr = "Last name is required";
   } else {
     $last_name = test_input($_POST["last-name"]);
   }

   if (empty($_POST["email"])) {
     $emailErr = "Email is required";
   } else {
     $email = test_input($_POST["email"]);
   }

   if (empty($_POST["message"])) {
     $messageErr = "Message is required";
   } else {
     $message = test_input($_POST["message"]);
   }


   if(isset($first_name) && isset($last_name) && isset($email) && isset($message))
   {
     $_SESSION['first_name'] = $first_name;
     $_SESSION['last_name'] = $last_name;
     $_SESSION['email'] = $email;
     $_SESSION['message'] = $message;
     header("Location: contact9SessionsCRSF2.php");
     exit; 
   }
}
else {
  $token = md5(uniqid(mt_rand(), true));
  $_SESSION['token'] = $token;
  $_SESSION['token_time'] = time();
}

function test_input($data) {
   $data = trim($data);
   $data = stripslashes($data);
   $data = htmlspecialchars($data);
   return $data;
}
?>

  <nav class="navbar navbar-default navbar-fixed-top">
  <div class="container-fluid">
    <!-- Brand and toggle get grouped for better mobile display -->
    <div class="navbar-header">
      <button type="button" class="navbar-toggle collapsed" data-toggle="collapse" data-target="#bs-example-navbar-collapse-1" aria-expanded="false">
        <span class="sr-only">Toggle navigation</span>
        <span class="icon-bar"></span>
        <span class="icon-bar"></span>
        <span class="icon-bar"></span>
      </button>
      <a class="navbar-brand" href="index.html">New Daimaru Hotel<small id="address_of_hotel">345 E 1st Street Los Angeles, CA 90012</small></a>
    </div>

    <!-- Collect the nav links, forms, and other content for toggling -->
    <div class="collapse navbar-collapse" id="bs-example-navbar-collapse-1">
      <ul class="nav navbar-nav navbar-right">
        <li><a href="about.html">About</a></li>
        <li><a href="rooms.html">Rooms & information</a></li>
        <li><a href="reserve.php">Book to Reserve</a></li>
        <li><a href="location.html">Location Info</a></li>
        <li><a href="additional.html">Additional Info</a></li>
        <li><a href="contact.php">Contact</a></li>
      </ul>
    </div><!-- /.navbar-collapse -->
    <ul class="nav navbar-nav navbar-right">
        <p class="pull-right language_choice_english"><a href="contact.php">English</a></p>
        <p class="pull-right language_choice_japanese"><a href="contact-ja.php">Japanese</a></p>
    </ul>
  </div><!-- /.container-fluid -->
</nav>
<div id="main-container" class="col-xs-12">

  <form class="form-horizontal"  method="post" action="<?php echo htmlspecialchars($_SERVER["PHP_SELF"]);?>">

  <input type="hidden" name="token" value="<?php echo $token; ?>" />
    <div class="form-group">
      <label class="col-sm-2 control-label">First Name</label>
      <div class="col-sm-10">
      <input class="form-control" name="first-name" id="first-name" placeholder="First Name" type="text" value="<?php if(isset($first_name)) { echo $first_name; }?>">
       <?php if(isset($firstNameErr)) print ('<span class="error">* ' . $firstNameErr . '</span>'); ?>
      </div>
    </div>
    <div class="form-group">
      <label class="col-sm-2 control-label">Last Name</label>
      <div class="col-sm-10">
      <input class="form-control" name="last-name" id="last-name" placeholder="Last Name" type="text" value="<?php if(isset($last_name)) { echo $last_name; }?>">
       <?php if(isset($lastNameErr)) print ('<span class="error">* ' . $lastNameErr . '</span>'); ?>
     </div>
    </div>
      <div class="form-group">
      <label class="col-sm-2 control-label">Email</label>
      <div class="col-sm-10">
      <input class="form-control" name="email" id="email" placeholder="Email" type="email" value="<?php if(isset($email)) { echo $email; }?>">
       <?php if(isset($emailErr)) print ('<span class="error">* ' . $emailErr . '</span>'); ?>
      </div>
    </div>
    <div class="form-group">
      <label class="col-sm-2 control-label">Message</label>
      <div class="col-sm-10">
      <textarea class="form-control" rows="2" placeholder="Please type in your message" name="message" id="message" value="<?php if(isset($message)) { echo $message; }?>"></textarea>
       <?php if(isset($messageErr)) print ('<span class="error">* ' . $messageErr . '</span>'); ?>
      </div>
    </div>

    <div class="form-group">
    <div class="col-sm-offset-2 col-sm-10">
      <button type="submit" class="btn btn-default">Submit</button>
    </div>
  </div>
</form>

Here's the page that does the actual mailing:

<?php

session_start();

$first_name = $_SESSION['first_name'];
$last_name = $_SESSION['last_name'];
$email = $_SESSION['email'];
$message = nl2br($_SESSION['message']);

require 'PHPMailerAutoload.php';

$mail = new PHPMailer;

//$mail->SMTPDebug = 3;                               // Enable verbose debug output

$mail->isSMTP();                                      // Set mailer to use SMTP
$mail->Host = 'hosting_info';  // Specify main and backup SMTP servers
$mail->SMTPAuth = true;                               // Enable SMTP authentication
$mail->Username = '[email protected]';                 // SMTP username
$mail->Password = 'ugh';                           // SMTP password
$mail->SMTPSecure = 'tls';                            // Enable TLS encryption, `ssl` also accepted
$mail->Port = 587; 

$mail->addReplyTo( $email, $first_name );
$mail->addAddress( $email, $first_name );
$mail->addAddress( 'my_email_address', 'Staff' );
$mail->From = 'my_email_address';
$mail->FromName = 'Staff';


$mail->isHTML(true);                                  // Set email format to HTML

$mail->Subject = 'Contact';
$mail->Body    = "<h1>First Name: $first_name</h1>"
                                    ."<h1>Last Name: $last_name</h1>"
                                    ."<h1>Email: $email</h1>"
                                    ."<h1>Message $message</h1>"; 

$mail->AltBody = 'To view the message, please use an HTML compatible email viewer!';

if(!$mail->send()) {
    header('location: URL/contactError.html');

} else {
    header('location: URL/contactResult2.html');

}

Contents of contactError.html are:

<h1>Apologies, it seems that the email didn't get sent.</h1>
<h3>Please refill the form, and try again.</h3>
<h3>Thank you!</h3>

Contents of contactResult2.html are:

  <h4 class="typl8-gamma centered">Contact Form Emailed</h4>
  <h1 class="typl8-delta centered">Thank you for contacting us.</h1>
  <h3 class="typl8-delta centered">You will hear from us shortly</h3>

Answer 1

Input Validation

The test_input function seems to be from here, and it's really not a good function to use (see here and here).

In short: it isn't recommended defense against anything (the recommendation for XSS are to HTML encode when outputting data, not when receiving it, and the function doesn't provide defense against anything else), and it makes your data dirty, leading to usability problems.

Misc

• Your indentation is off, and your bracket positioning is inconsistent, making your code harder to read.
• Using a new CSRF token for each request is good for security, but not that good for usability (breaks back button, etc). Evaluate if this is really necessary, and if not, use a token per session.
• Having file names such as contact9SessionsCRSF2 or contactResult2 is a sign that you have files in your code base which are not actually used, but kept "just in case". This can easily lead to confusion, think about deleting them and removing the 2 from the file names, and/or use version control.
• The token generation and setting is duplicated code, which is not a good idea as it makes it hard to change and clutters up your code. You could extract it into a generateAndSetToken function.

Answer 2

First your code looks rather clean, and easy to read: that's already an important point.

Then a possible improvement I immediately notice is that, because processing of the query parameters is quite similar for all of them, you can dramatically reduce your code, both in control part and in HTML part.

To do so, first simply register your query parameters:

  $params = [
    'first-name' => NULL,
    'last-name' => NULL,
    'email' => NULL,
    'message' => NULL,
  ];

Then the four successive occurrences of primary control part becomes:

  foreach ($params as $key => $value) {
    if (!empty(trim($_POST[$key]))) {
      $params[$key] = htmlspecialchars(stripslashes(trim($_POST[$key])));
    }
  }

Above you can see that:

• here we don't any more generate an error message: seems not useful to do it now, will be done in HTML part so we don't use supplemental variables for this.
• since this is now a function it can include what you're previously doing in test_input().
• but we added the trim() early to the !empty($_POST[$key]), because if the initial content was one or more space(s) it wouldn't be seen as empty!

Once each parameter is controlled, now we can also reduce the way we accept or not to use the whole input, like this:

  if (array_filter($params) == $params) {
    foreach ($params as $key => $value) {
      $_SESSION[$key] = $value;
    }
    header("Location: contact9SessionsCRSF2.php");
    exit; 
  }

Here we take advantage of how array_filter() works when no callback function is provided, simply dropping "not-true" items.

Last point regarding this control part, I don't understand why you generate token and token-time twice: once (always) just before controlling, and another time if control failed.
So I'm giving it up in this complete version of the control part (you can re-establish it if there is a good reason):

session_start(); //allows use of session variables

if ($_SERVER["REQUEST_METHOD"] == "POST"
&& !empty($_SESSION['token'])
&& $_SESSION['token'] == $_POST['token']) {

  $token = md5(uniqid(mt_rand(), true));
  $_SESSION['token'] = $token;
  $_SESSION['token_time'] = time();

  $params = [
    'first-name' => NULL,
    'last-name' => NULL,
    'email' => NULL,
    'message' => NULL,
  ];
  foreach ($params as $key => $value) {
    if (!empty(trim($_POST[$key]))) {
      $params[$key] = htmlspecialchars(stripslashes($_POST[$key]));
    }
  }
  if (array_filter($params) == $params) {
    foreach ($params as $key => $value) {
      $_SESSION[$key] = $value;
    }
    header("Location: contact9SessionsCRSF2.php");
    exit; 
  }
}

Then we may do something pretty equivalent for the HTML part (only the main-container content changes):

<div id="main-container" class="col-xs-12">
  <form class="form-horizontal"  method="post"
    action="<?php echo htmlspecialchars($_SERVER["PHP_SELF"]);?>">

    <input type="hidden" name="token" value="<?php echo $token; ?>" />
<?php
$param_names = [
    'first-name' => 'First Name',
    'last-name' => 'Last name',
    'email' => 'email',
    'message' => 'message',
];
foreach ($param_names as $key => $value) {
?>
    <div class="form-group">
      <label class="col-sm-2 control-label"><?php echo $value; ?></label>
        <div class="col-sm-10">
            <input class="form-control" type="text"
              name="<?php echo $key; ?>" id="<?php echo $key; ?>"
              placeholder="<?php echo $value; ?>"
              value="<?php echo $params[$key]; ?>">
<?php
if(!$params[$key])
?>
        <span class="error">* <?php echo $value; ?> is required</span>
      </div>
    </div>

    <div class="form-group">
      <div class="col-sm-offset-2 col-sm-10">
        <button type="submit" class="btn btn-default">Submit</button>
      </div>
    </div>
  </form>
</div>

Answer 3

$mail->AltBody = 'To view the message, please use an HTML compatible em...

For basic information, why is an html message REQUIRED? Output the data the user needs in plain text and email.

Answer 4

Styling and readability

Your indentation is inconsistent, both for blocks on the same level (3 and 6 spaces) as blocks on different levels (2 or 3 spaces). Inconsistent whitespace can cause a developer to wrongly believe code belongs to a different block than it actually belongs. I recommend fixing all indentation.

Your code-style is inconsistent in some places. Between if and the curly brace is sometimes whitespace, sometimes not. You sometimes place else on a new line, you sometimes do not.

You sometimes use print and you sometimes use echo. While they do not work exactly the same, you use them in the same context. I recommend replacing one of the two with the other for clarity.

Logic errors

You test several parameters with a construction like if (empty($_POST["email"])) {, but use isset($email) in an if-statement that redirects the user. empty(...) is a shorthand for !isset($var) || $var == FALSE. You are not testing for the condition when $var == FALSE. This is a problem with, for example, the empty string. Since you do use trim(..) later to sanitize the data, you probably should use that here too.

Code duplication

The following block is duplicated in both the if and the else-clause. You can move this code above the if-statement, and omit the else statement completely.

$token = md5(uniqid(mt_rand(), true));
$_SESSION['token'] = $token;
$_SESSION['token_time'] = time();

You have several similar blocks to validate the input, like the one below. To avoid code duplication you can consider putting this code in a function.

if (empty($_POST["message"])) {
  $messageErr = "Message is required";
} else {
  $message = test_input($_POST["message"]);
}

Sanitizing

Your function named test_input does not do what it's name suggest. Instead, it does seem to make an attempt at sanitizing the input. I am unsure what stripslashes(..) is doing in there. Furtermore, your call to htmlspecialchars does not name an encoding.

You are not checking if the email-address actually is in the form of an email adddress. You might want to at least consider if it matches .+@.+\..+.

Html semantics

contactError.html and contactResult2.html use headers to output a message. I am assuming that you have the proper html-boilerplate around them. You are misusing headers to display text of various sizes. The text displayed is not actually a header, and should be displayed in a span-tag with custom class instead.

No guards in mail

Your mail script does not check if the content of the session variables is there. It does not even guard against reloading the page. If I extract the name of the script that does the mailing, I can go there directly and a bogus email is sent.

Edit: I was wrong about it not being guarded against reloading the page, because the page itself will do another redirect. A possible exploit with the current setup could be to have an url that looks like an image on a forum, which will actually redirect to yoursite.com/contact9SessionsCRSF2.php. What this will do is creating an army of innocent people sending you empty emails. A DDOS... without needing to actually hack any computers. You should check the validity of the used parameters in the script itself, not assume that a different script in a different request did that for you.

Answer 5

Limit length of incoming, outgoing data. Restrict incoming POST or other to only the character ranges needed. UTF8 has characters which you likely don't need to support; some are used for abusive purposes. Use regular expressions to check incoming POST or other data. Consider captcha's for limiting abuse. If possible do not support emails containing markup. Unset, clear or overwrite each variable when finished with it.

Place lines of code containing credentials for SMTP user account into a separate file without a PHP file extension. Use Apache or other server side restrictions to prevent any direct access. Use file system permissions to restrict access also. Then use require_once within the PHP script to then include this file.

Related but not PHP code effort: have email admin limit number of outgoing messages, IP address sources that can connect with this isolated SMTP account.