PHP contact form using PHPMailer and Google Recaptcha

Question

My company is designing a new site, and because I'm currently on bench I've been tasked with writing any PHP required. Pretty simple stuff, just a contact form and a resume upload page, both using a form to email the details to management. No database, no SQL, no business logic. The designer had already built the bulk of the form, so most of the bootstrap was existing when I started.

After some research, I decided on using PHPMailer and Google Recaptcha. The code works as expected, however, as a junior C#/.net dev, PHP is somewhat new to me. I've trawled through many similar code-review posts, and made improvements where I could, but still have some questions (as would I appreciate an overall review!):

1. Is it standard to have simple scripts such as this in the same file as the HTML? I've seen it both ways.
2. I've read that using globals is usually not best practice. However, I'm unsure how to set the errors from within the validate_form function otherwise. Is there a better way?
3. I stole the construct_error_html function from the old site, but I haven't seen similar examples of it. Is this an adequate way to build error messages? Or should I just print the error message within the HTML like I did with the recaptcha error? (<?="<p class='text-danger'>$recaptcha_error</p>";?>) These messages should rarely show, as I'm using HTML5 validation as well.
4. My code (especially the HTML/PHP mix) seems really messy. I tried to standardize formatting, but I'm still not completely happy. Any suggestions for making it look cleaner?
5. Security - I've followed along with several security-minded posts, any glaring mistakes?
6. I looked into AJAX, but it seems to be overkill for such a small form as this. Opinions?

I currently have all of the code in a single file, with the header and footer added through include statements.

contact.php

<?php

require 'assets/library/PHPMailer-master/PHPMailerAutoload.php';
require 'assets/library/recaptcha-master/src/autoload.php';

$overview_error = '';
$recaptcha_error = '';
$email_error = '';
$name_error = '';
$subject_error = '';
$message_error = '';
$email_sent = false;
$siteKey = 'SITE KEY'; // https://github.com/google/recaptcha/blob/master/examples/example-captcha.php

if (isset($_POST["submit"])) 
{
    if (!validate_form())
    {
        $overview_error = 'There are errors on the form.';
    }
    if (!isRecaptchaValid()) 
    {
        $overview_error = 'There are errors on the form.';
        $recaptcha_error = 'Please submit recaptcha!';
    } 
    else 
    {
        $subject = strip_tags($_POST["subject"]);
        $name = strip_tags($_POST["name"]);
        $email = strip_tags($_POST["email"]);
        $message = strip_tags($_POST["message"]);

        $mail = new PHPMailer; // https://github.com/PHPMailer/PHPMailer
        // Tell PHPMailer to use SMTP
        // Commented out for now
        //$mail->isSMTP();
        // Enable SMTP debugging and HTML-friendly debug output
        $mail->SMTPDebug = 2;
        $mail->Debugoutput = 'html';

        $mail->Host = "host";
        $mail->Port = 587; // Likely 25, 465, or 587
        $mail->SMTPAuth = true; // not sure what the auth details are for this
        $mail->Username = "email username";
        $mail->Password = "email password";

        $mail->setFrom('email', 'Company Contact Us Form');
        $mail->addReplyTo($email);
        $mail->addAddress('send to email', 'Company Administration');
        $mail->Subject = 'Contact us email from ' . $name . ' concerning ' . $subject;
        $mail->Body = $name . " has contacted us from the website leaving the following information: \nSubject: " . $subject . "\nName: " . $name . "\nEmail: " . $email . "\n\n" . $message;

        // Send the message, check for errors
        if(!$mail->send())
        {
            echo ("Message could not be sent.");
            echo ("Mailer Error: " . $mail->ErrorInfo);
            exit;
        }
        $email_sent = true;
    }
}

function validate_form()
{
    $error_count = 0;

    if (empty($_POST["email"])) {
        global $email_error;
        $email_error = "Please enter your email.";
        $error_count ++;
    } 
    elseif (!filter_var($_POST["email"], FILTER_VALIDATE_EMAIL))
    {
        global $email_error;
        $email_error = "Invalid email.";
        $error_count ++;
    }
    if (empty($_POST["name"])) {
        global $name_error;
        $name_error = "Please enter your name.";
        $error_count ++;
    }
    if (!isset($_POST["subject"])) {
        global $subject;
        $subject_error = "Please select a subject.";
        $error_count ++;
    }
    if (empty($_POST["message"])) {
        global $message_error;
        $message_error = "Please enter your comments.";
        $error_count ++;
    }
    return $error_count == 0 ? true : false;
}

function isRecaptchaValid()
{
    $siteSecret = 'SITE SECRET'; // www.google.com/recaptcha/admin
    $recaptcha = new \ReCaptcha\ReCaptcha($siteSecret);
    $response = $recaptcha->verify($_POST['g-recaptcha-response'], $_SERVER['REMOTE_ADDR']);
    if ($response->isSuccess()) {
        return true;
    } else {
        return false;
    }
}

function construct_error_html($error)
{
    if(isset($error))
    {
        if(strlen(trim($error)) > 0)
        {
            $error_open = "<span class='help-inline text-danger'>  ";
            $error_close = "</span>";
            return $error_open.$error.$error_close;
        }
    }
}
?>

<!DOCTYPE html>
<html lang="en">
<head>
    <?php include("assets/includes/head.php"); ?>
</head>
<body>

<?php include("assets/includes/header.php"); ?>

<div id="pageheaderwrap" class="header_con">
    <div class="container">
    </div>
</div>

<div id="hero">
    <div class="container inner">
        <div class="row">
            <div class="col-md-8 col-sm-9 col-md-offset-2">
                <header>
                    <?php
                    if ($email_sent)
                    { ?>
                        <h1>Thank You</h1>
                        <p>Thank you for contacting us, we'll get back to you as soon as possible.</p>
                    <?php
                    } else {
                    ?>
                    <h1>Contact</h1>
                    <p>Let us put our expertise to work for you.</p>
                    <?php }?>
                </header>
            </div>
        </div>
    </div>
</div>

<div class="container w">
    <div class="row">
        <div class="col-md-8 col-md-offset-2">
            <div class="section">
                <div id="content-area">
                    <p> We'd love to hear from you.  You can call us at (phone) or use the contact form below to send us an email.</p>
                </div>
            </div>
        </div>
    </div>
</div>
<?php if (!$email_sent) { ?>
<div class="container gap">
    <div class="row">
        <div class="col-md-8">
            <div class="well well-sm">
                <form  role="form" method="post" action="emailtest.php">
                    <div class="row">
                            <?="<p class='text-danger'>$overview_error</p>";?>  
                    </div>
                    <div class="row">
                        <div class="col-md-6">
                            <div class="form-group">
                                <label for="name">Name</label><?php print construct_error_html($name_error); ?>
                                <input type="text" class="form-control" id="name" name="name" placeholder="Enter name"  
                                    value="<?= isset($_POST['name']) ? htmlspecialchars($_POST['name']) : '' ;?>" />
                            </div>
                            <div class="form-group">
                                <label for="email">Email Address</label><?php print construct_error_html($email_error); ?>
                                <div class="input-group">
                                <span class="input-group-addon"><span class="glyphicon glyphicon-envelope"></span>
                                </span>
                                    <input type="email" class="form-control" id="email" name="email" placeholder="Enter email" 
                                        value="<?= isset($_POST['email']) ? htmlspecialchars($_POST['email']) : '' ;?>" /></div>
                            </div>
                            <div class="form-group">
                                <label for="subject">Subject</label><?php print construct_error_html($subject_error); ?>
                                <select id="subject" name="subject" class="form-control">
                                    <option value="na" selected="">Choose One:</option>
                                    <option value="service"
                                        <?php if(isset($_POST['subject']) && $_POST['subject'] == 'service')
                                            echo ' selected="selected"'; ?> 
                                        >General Customer Service</option>
                                    <option value="suggestions"
                                        <?php if(isset($_POST['subject']) && $_POST['subject'] == 'suggestions')
                                            echo ' selected="selected"'; ?> 
                                        >Suggestions</option>
                                    <option value="product"
                                        <?php if(isset($_POST['subject']) && $_POST['subject'] == 'product')
                                            echo ' selected="selected"'; ?> 
                                        >Product Support</option>
                                </select>
                            </div>
                            <div class="form-group">
                                <div class="g-recaptcha" data-sitekey="<?= $siteKey; ?>"></div>
                                <?="<p class='text-danger'>$recaptcha_error</p>";?>
                            </div>
                        </div>
                        <div class="col-md-6">
                            <div class="form-group">
                                <label for="message">
                                    Message</label><?php print construct_error_html($message_error); ?>
                                <textarea name="message" id="message" class="form-control" rows="10" cols="25"
                                    placeholder="Message"><?= isset($_POST['message']) ? htmlspecialchars($_POST['message']) : '' ; ?></textarea>
                            </div>
                        </div>
                        <div class="col-md-12">
                            <button type="submit" name="submit" class="btn btn-primary pull-right" id="btnContactUs">
                                Send Message</button>
                        </div>
                    </div>
                </form>
            </div>
        </div>
    </div>
</div>
<?php } ?>

<?php include("assets/includes/footer.php"); ?>

</body>
</html>

Answer 1

<?php

require 'assets/library/PHPMailer-master/PHPMailerAutoload.php';
require 'assets/library/recaptcha-master/src/autoload.php';

I am guessing that this script is in a web directory. If so, you should strongly consider moving your PHP libraries out of the web directory altogether rather than being in child directories of web root. There is no reason to ever potentially expose these files to the world, or to have to spent the overhead of putting in redirection/authentication logic to protect these resources.

---

$overview_error = '';
$recaptcha_error = '';
$email_error = '';
$name_error = '';
$subject_error = '';
$message_error = '';

I have comment below about alternate an alternate approach you might want to consider for form validation. Even if you don't want to make that change, consider generalizing your validation function so that it is re-usable across different forms. You could then pass form-specific configuration to that function and have it execute validation on what type of data you are validating.

This could be as simple as an array you pass into the function containing validation and sanitization filters you want to apply to the passed parameters. Perhaps something like this:

$fields = (object) [
    'name' => [
        'filters' => [
            FILTER_SANITIZE_STRING
        ],
        'required' => true
    ],
    'email' => 'overview' => [
        'filters' => [
            FILTER_VALIDATE_EMAIL
        ],
        'required' => true
    ],
    // etc.
];              

With your validation function now looking similar to this:

function validate_post_input($fields) {
    // have some structures to provide easily
    $return = new stdClass();
    $return->field_values = new stdClass();
    $return->validation_results = new stdClass();
    $return->validation_failed = false;
    foreach($fields as $param => $data) {
        // determine presence of data
        if(empty($_POST[$param])) {
            if($data['required']) {
                // set value to false for this item
                // to indicate validation failure
                $return->field_values->{$param} = null;
                $return->validation_results->{$param} = false;
                $return->validation_failed = true;
            } else {
                // not required, so we set value to empty string
                $return->field_values->{$param} = '';
                $return->validation_results->{$param} = true;
            }
            // no reason to do anything else with this parameter
            continue;
        }

        $value = $_POST[$param];
        // iterate through filter
        foreach($data->filters AS $filter) {
            $value = filter_var($value, $filter);
            if ($value === false) {
                $return->field_values->{$param} = null;
                $return->validation_results->{$param} = false;
                $return->validation_failed = true;
                break;
            }
            $return->field_values->{$param} = $value;
            $return->validation_results->{$param} = true;
        }
    }

    return $return;
} 

// usage
$fields = ...;
$validation = validate_post_input($fields);
// get all validated/sanitized field results
$form_values = $validation->field_values;
// get validation results on a per field basis
$validation_results = $validation->validation_results;
// get overall validation pass/fail
$validation_failed = $validation->validation_failed;

---

$siteKey = 'SITE KEY'; // https://github.com/google/recaptcha/blob/master/examples/example-captcha.php

Ideally, this should be defined in application configuration somewhere.

---

if (isset($_POST["submit"])) 
{
    if (!validate_form())

Consider replacing this validate_form() with filter_input_array() - http://php.net/manual/en/function.filter-input-array.php - or have this function used within validate_form operating against a configuration passed into the function as parameter. This function allows for both validation and sanitization depending on what filters you apply to each passed parameter.

This a more reusable approach across various pages with forms in that it allows you to avoid having to have several different validation functions for different forms. Instead you have a single function which can perform your POST validation and you have form-specific configurations which you can pass to that function.

---

        $subject = strip_tags($_POST["subject"]);
        $name = strip_tags($_POST["name"]);
        $email = strip_tags($_POST["email"]);
        $message = strip_tags($_POST["message"]);

Is sanitize filter better for this purpose?

---

        $mail->Port = 587; // Likely 25, 465, or 587
        $mail->SMTPAuth = true; // not sure what the auth details are for this

Don't use comments at end of line. They are hard to read and can make like too long

---

        $mail->Subject = 'Contact us email from ' . $name . ' concerning ' . $subject;
        $mail->Body = $name . " has contacted us from the website leaving the following information: \nSubject: " . $subject . "\nName:

" . $name . "\nEmail: " . $email . "\n\n" . $message;

Lines of code in this section are too long. try to kep them under 80 characters per line. Break across lines to do this.

---

        // Send the message, check for errors
        if(!$mail->send())
        {
            echo ("Message could not be sent.");
            echo ("Mailer Error: " . $mail->ErrorInfo);
            exit;
        }

Don't echo out code-level error messages to the end user. Log the errors, but give meaningful display message to user in HTML.

---

function validate_form()

If you look at other comments, changing to more generalized validation function will help you get away from using globals. Bottom line is this function should not set end user messaging. It should do one thing - perform the validation functionality, leaving the caller to determine messaging based on validation results.

---

    $siteSecret = 'SITE SECRET'; // www.google.com/recaptcha/admin

Better in app config.

    $recaptcha = new \ReCaptcha\ReCaptcha($siteSecret);
    $response = $recaptcha->verify($_POST['g-recaptcha-response'], $_SERVER['REMOTE_ADDR']);

Consider validating these inputs before passing them to this verify method.

---

function construct_error_html($error)
{
    if(isset($error))

validate against empty() and fail out of function if non-zero length string is passed. No need to nest all the rest of your code inside a conditional.

For example

      if(empty($error)) {
          throw new InvalidArgumentException(
              'Non-zero length string expected'
          );
      }
      // rest of code, no longer nested in conditional

---

        if(strlen(trim($error)) > 0)

Why is this necessary? If you validate properly you get away form this conditional. If there any reason to expect that the error messaging, which this applicaiotn controls would ever have extra whitespace anyway?

---

        {
            $error_open = "<span class='help-inline text-danger'>  ";
            $error_close = "</span>";
            return $error_open.$error.$error_close;
        }

Agree with comment in other answer about templating out at least using HEREDOC as rudimentary templating.

---

<head>
    <?php include("assets/includes/head.php"); ?>

Consider moving includes out of web directory. Typical thorughout.

---

            <div class="col-md-8 col-sm-9 col-md-offset-2">
                <header>
                    <?php
                    if ($email_sent)
                    { ?>
                        <h1>Thank You</h1>
                        <p>Thank you for contacting us, we'll get back to you as soon as possible.</p>
                    <?php
                    } else {
                    ?>
                    <h1>Contact</h1>
                    <p>Let us put our expertise to work for you.</p>
                    <?php }?>
                </header>
            </div>

Maybe a stylistic choice, but if working without a templated approach and mixing HTML and PHP. I would prefer to have blocks of PHP code not indented with HTML for easier visual separation. Inline PHP variables output I would keep in-line with HTML though.

I might code this section like:

            <div class="col-md-8 col-sm-9 col-md-offset-2">
                <header>
<?php
// use whatever your current PHP nesting level is here
if ($email_sent) {
?>
                        <h1>Thank You</h1>
                        <p>Thank you for contacting us, we'll get back to you as soon as possible.</p>
<?php
} else {
?>
                       <h1>Contact</h1>
                       <p>Let us put our expertise to work for you.</p>
<?php
}
?>
                </header>
            </div>

<?php if (!$email_sent) { ?>

Really hard to see this conditional mixed into HTML like this and with all PHP and open/close tags there on one line

---

                <form  role="form" method="post" action="emailtest.php">
                    <div class="row">
                            <?="<p class='text-danger'>$overview_error</p>";?>  
                    </div>

Should this this whole row only be shown conditionally based on whether there is error info to display?

---

                            <div class="form-group">
                                <label for="name">Name</label><?php print construct_error_html($name_error);

?> value="" />

Typical comments for all fields - - Hard to read this code. Lines are too long. Too much output manipulation happening here. - Should you even call construct_error_html() if there is no error for this field? - Why have isset() check on value to be populated here? By this point in the code you should already know exactly what values you are will be populated into these inputs, whether they be blank values or values derived from POST input. You should also have already prepared the strings for output.

---

<?php } ?>

<?php include("assets/includes/footer.php"); ?>

Don't drop in and out of PHP unnecessarily. This can cause spurious output if you are not careful.

---

Your code does nothing to protect against cross-site-request forgery (CSRF) attacks. Typically this would take form of variable stored in session compared against token passed as hidden input in the form. Without this, your script can easily be hijacked to spam mail recipients.

---

Well composed application tpyically use a combination of server side validations and client side (javascript) validations. You might consider adding the javascript validation at some point to this or other forms. IN such cases it might be even more useful to take the configuration-based approach to your form fields, as you could easily output validation configurations from PHP into javascript to configure both server and client-side validation logic from a single place. It probably wouldn't even be much more of a stretch to output the whole form dynamically based on this configuration, though this may be overkill for a single simple form like this. I mention this more as food for thought for the future. There are form-builder libraries out there that do this exact thing.

Answer 2

Huge problem

You have 1 HUGE problem in your code:

If you enter correct captcha - the form is sent. Doesn't matter if it passed validation or not.

It is a logic error in your post processing. You check if form is valid - if it is not - you display error. Then you check if captcha is valid - if it is - you send email.

Bad practice for modern environment

Also, an incoveniece and bad form of web programming is not redirecting the page after the form was submitted.

Take into consideration that some mobile devices automatically refresh pages in browsers in certain situations. So, if you submit form and it sends an email - you can leave the browser, come back later - and it will get sent again.

Bad practice 2

Email spam prevention. Nothing is done to prevent numerous messages to be sent through your form - you don't check that there were say less than 10 messages sent from that IP address or anything like that.

Also, I guess you rely on PHPMailer to avoid someone hijacking fields in your form to add recipients, something like 'subject: header:Message-To: [email protected]' might or might not be catched.

Also, someone could insert a fake link into your form field and it would be delivered to the recipient of the email.

---

More than the syntax and other beauty enhancements you should first ensure that a) your code works and b) your code is safe.