Sanitising contact form input for PHPMailer

Question

I would like some insight on this function for sanitising user submitted data into a contact form. No HTML content or anything, just plain text.

Are there any vulnerabilities here that I'm missing? Am I ok to use the flag FILTER_FLAG_NO_ENCODE_QUOTES on the subject line? I'm not sure if quotes are safe to submit.

I'm looking to create a all-round function that will be safe to use site-wide.

Note: I'm using this with PHPMailer not mail().

// Sanitise headers
function Sanitise_Mail($h) {
    $h = filter_var($h,FILTER_SANITIZE_STRING,FILTER_FLAG_NO_ENCODE_QUOTES);

    // Taken from PEAR Mail
    $h = preg_replace('=((<CR>|<LF>|0x0A/%0A|0x0D/%0D|\\n|\\r)\S).*=i','',$h);

    return $h;
}

Site-wide functions (sample)... Feel free to point out any other problems here.

// Use instead of error_log to add some useful information to each log
function error_report($err) {
    global $user;
    error_log('log: ' . $err . ' | User: ' . $user->username . ' | User IP: ' . IP_ADDRESS);
}

// Find & test site / global email settings
function Email_Settings($default = false) {
    global $global,$setting;

    // Run once & save to $setting StdClass()
    if($default == false && !empty($setting->email_settings)) return $setting->email_settings;
    else {
        // Check for site email settings
        if($default == false && !empty($setting->smtp_host) && !empty($setting->smtp_username) && !empty($setting->smtp_password) && !empty($setting->smtp_key)) {
            if($smtp_pass = smtp_pass($setting->smtp_password,$setting->smtp_key)) {
                if(!empty($smtp_pass)) {
                    $e = new StdClass();

                    $e->host = $setting->smtp_host;
                    $e->username = $setting->smtp_username;
                    $e->password = $smtp_pass;
                    $e->receiver = !empty($setting->smtp_receiver) ? $setting->smtp_receiver : $setting->smtp_username;
                    $e->replyto = $setting->smtp_replyto;
                    $e->port = $setting->smtp_port;
                    $e->encryption = $setting->smtp_encryption;

                    $setting->email_settings = $e;

                    return $e;
                }
                else error_report('Email_Settings(): SMTP password was decrypted but is empty for site: ' . SITE_ID);
            }
            else error_report('Email_Settings(): SMTP password failed to decrypt for site: ' . SITE_ID);
        }

        // Fallback: Use default email settings
        else if(!empty($global->smtp_host) && !empty($global->smtp_username) && !empty($global->smtp_password) && !empty($global->smtp_key)) {
            if($smtp_pass = smtp_pass($global->smtp_password,$global->smtp_key)) {
                if(!empty($smtp_pass)) {
                    $e = new StdClass();

                    $e->host = $global->smtp_host;
                    $e->username = $global->smtp_username;
                    $e->password = $smtp_pass;
                    $e->receiver = OWNER_EMAIL !== NULL ? OWNER_EMAIL : FALLBACK_EMAIL;
                    $e->replyto = $global->smtp_replyto;
                    $e->port = $global->smtp_port;
                    $e->encryption = $global->smtp_encryption;

                    $setting->email_settings = $e;

                    return $e;
                }
                else error_report('Email_Settings(): Stored default password was decrypted but is empty.');
            }
            else error_report('Email_Settings(): Stored default SMTP password failed to decrypt.');
        }
        else error_report('Email_Settings(): Required email settings are missing.');
    }

    return false;
}

// Pass parameters to send emails
function Send_Email($subject,$body,$recipient,$sender = '',$sender_name = '',$html = true,$default = false) {
    if(Email_Settings($default)) {
        require_once ROOT . '/plugins/phpmailer/PHPMailerAutoload.php';

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

        $mail->CharSet = 'UTF-8';
        $mail->isSMTP();
        $mail->Host = Email_Settings($default)->host;
        $mail->SMTPAuth = true;
        $mail->Username = Email_Settings($default)->username;
        $mail->Password = Email_Settings($default)->password;
        if(!empty(Email_Settings($default)->encryption)) $mail->SMTPSecure = Email_Settings($default)->encryption;
        if(!empty(Email_Settings($default)->port)) $mail->Port = Email_Settings($default)->port;

        $mail->From = !empty(Email_Settings($default)->sender) ? Email_Settings($default)->sender : Email_Settings($default)->username;
        $mail->FromName = $sender_name;

        $mail->addAddress($recipient);
        if((bool)$html == true) $mail->isHTML(true);

        // Add reply-to header if available
        if(!empty($sender)) $replyto = $sender;
        else if(!empty(Email_Settings($default)->replyto)) $replyto = Email_Settings($default)->replyto;

        if(!empty($replyto)) $mail->AddReplyTo($replyto);

        $mail->Subject = $subject;
        $mail->Body = $body;

        if($mail->send()) return true;
        else {
            error_report('Send_Email() error: ' . $mail->ErrorInfo);
            return false;
        }
    }
    else error_report('Send_Email(): Email_Settings() returned false.');
}

This is how I'm currently using this on the contact page...

if(isset($_POST['contactus'])) {
    if(!empty($_POST['emailbody'])) {
        if(!empty($_POST['subjectfrom'])) {
            if(!empty($_POST['emailfrom'])) {
                if(!filter_var($_POST['emailfrom'],FILTER_VALIDATE_EMAIL)) {
                    $result['error'][] = @$lang->module['invlaidemail'];
                }
                else if(function_exists('Sanitise_Mail')) {
                    $_POST['namefrom'] = Sanitise_Mail($_POST['namefrom']);
                    $_POST['subjectfrom'] = Sanitise_Mail($_POST['subjectfrom']);
                    $_POST['phonefrom'] = Sanitise_Mail($_POST['phonefrom']);
                    $_POST['emailbody'] = Sanitise_Mail($_POST['emailbody']);

                    if(!empty($_POST['phonefrom'])) $phonenum = @$lang->module['phone'] . ' ' . $_POST['phonefrom'];
                    else $phonenum = '';

                    $body = $_POST['emailbody'] . "\r\n\r\n" . $phonenum;

                    // Check IP ban list
                    if(function_exists('check_ip')) check_ip();

                    if(empty($result['error'])) {
                        // Send email

                        if(Send_Email($_POST['subjectfrom'],$body,Email_Settings()->receiver,$_POST['emailfrom'],$_POST['namefrom'],0)
                        || Send_Email($_POST['subjectfrom'],$body,Email_Settings()->receiver,$_POST['emailfrom'],$_POST['namefrom'],0,1)) {
                            $result['success'][] = sprintf(@$lang->module['success'],$_POST['emailfrom']);

                            // Increment IP ban list
                            if(function_exists('ban_ip')) ban_ip(@$lang->module['ban_ip'],0,strtotime('+31 minutes'));

                            unset($_POST);
                        }
                        else $result['error'][] = @$lang->module['error'];
                    }
                    // Otherwise do nothing, echo errors in $result['error'] to browser later...
                }
                else {
                    $result['error'][] = @$lang->text['report_general_error'];
                    error_report('Function Sanitise_Mail() not found, cannot continue.');
                }
            }
            else $result['error'][] = @$lang->module['noemail'];
        }
        else $result['error'][] = @$lang->module['nosubject'];
    }
    else $result['error'][] = @$lang->module['nomessage'];
}

Mathieu Guindon · Accepted Answer · 2015-09-12 04:04:09Z

else if(function_exists('Sanitise_Mail')) {

If this condition is false, none of the inner scope executes, right? You could reduce nesting by getting it out of the way first.

Note that your code silently does nothing (no error, no success) if that condition fails: are you missing an else case? If there's no reason for an else case, then there's no reason to verify whether the function exists.

This could be a fun glitch -- for some values of "fun":

$result['error'][] = @$lang->module['invlaidemail'];

That's probably intended to be invalidemail.

Quill · Accepted Answer · 2015-09-12 09:37:14Z

A few things you can improve on:

function Sanitise_Mail($h) {
    $h = filter_var($h,FILTER_SANITIZE_STRING,FILTER_FLAG_NO_ENCODE_QUOTES);

    // Taken from PEAR Mail
    $h = preg_replace('=((<CR>|<LF>|0x0A/%0A|0x0D/%0D|\\n|\\r)\S).*=i','',$h);

    return $h;
}

You can just directly return $h in the preg_replace line.

Add spaces after your the commas between your parameters:

smtp_pass($global->smtp_password,$global->smtp_key)

becomes:

smtp_pass($global->smtp_password, $global->smtp_key)

The following two block are nearly identical, consider moving them into a format/structure that reduces the repetition:

// Check for site email settings
    if($default == false && !empty($setting->smtp_host) && !empty($setting->smtp_username) && !empty($setting->smtp_password) && !empty($setting->smtp_key)) {
        if($smtp_pass = smtp_pass($setting->smtp_password,$setting->smtp_key)) {
            if(!empty($smtp_pass)) {
                $e = new StdClass();

                $e->host = $setting->smtp_host;
                $e->username = $setting->smtp_username;
                $e->password = $smtp_pass;
                $e->receiver = !empty($setting->smtp_receiver) ? $setting->smtp_receiver : $setting->smtp_username;
                $e->replyto = $setting->smtp_replyto;
                $e->port = $setting->smtp_port;
                $e->encryption = $setting->smtp_encryption;

                $setting->email_settings = $e;

                return $e;
            }
            else error_report('Email_Settings(): SMTP password was decrypted but is empty for site: ' . SITE_ID);
        }
        else error_report('Email_Settings(): SMTP password failed to decrypt for site: ' . SITE_ID);
    }

Move the else statements in those nested blocks above the if statements, and just reverse the conditions.

This increases readability and clarity of code.

Wrap your brackets, you don't want a two measly brackets to give you a huge bug (See Apple SSL bug)

if(!empty($replyto)) $mail->AddReplyTo($replyto);

You probably knew this already, but omitting a direct comparison in an if statement tests the boolean; so, the following is redundant:

if((bool)$html == true)

In fact, $html should probably go into an empty check, not a boolean type-cast.

You do not want nestled statements beyond two or three, five is lunacy:

if(isset($_POST['contactus'])) {
    if(!empty($_POST['emailbody'])) {
        if(!empty($_POST['subjectfrom'])) {
            if(!empty($_POST['emailfrom'])) {

Consider:

if(!isset($_POST['contactus'])) {
    //do stuff
}
if(empty($_POST['emailbody'])) {
    //do stuff
}
if(empty($_POST['subjectfrom'])) {
    //do stuff
}
if(empty($_POST['emailfrom'])) {
    //do stuff
}

_{Notice also that I reversed the conditions there.}

This method means that the code is more readable, and you can solely avoid getting buried in layers and layers of conditionals.

You could even turn those checks into an object, where the $_POST variable is the key, and the error message the value.

This really is my favourite time of day; ternary time!

By using a ternary statement, you can simplify the following:

if(!empty($_POST['phonefrom'])) $phonenum = @$lang->module['phone'] . ' ' . $_POST['phonefrom'];
else $phonenum = '';

$phonenum = empty($_POST['phonefrom']) ? '' : @$lang->module['phone'] . ' ' . $_POST['phonefrom'];

No.

if(Send_Email($_POST['subjectfrom'],$body,Email_Settings()->receiver,$_POST['emailfrom'],$_POST['namefrom'],0)
||Send_Email($_POST['subjectfrom'],$body,Email_Settings()->receiver,$_POST['emailfrom'],$_POST['namefrom'],0,1)) {

This is not best practice, or even good practice for that matter.

there's two magic numbers (numbers without a clear definition)
there's no whitespace between parameters
the $_POST[''] variables should be defined additionally

This whole block needs more variable assignment:

$subject_from = $_POST['subjectfrom'];
$email_from   = $_POST['emailfrom'];
$name_from    = $_POST['namefrom'];
$reciever     = Email_Settings()->receiver;
$option       = 0;
$option2      = 1;
$sent_email_one = Send_Email($subject_from, $body, $reciever, $email_from, $name_from, $option);
$sent_email_two = Send_Email($subject_from, $body, $reciever, $email_from, $name_from, $option, $option2);
if($sent_email_one || $sent_email_two) {

Or at least something similar.

Thanks for the input, I will look at re-structuring where I can. I do have a habit of nesting unnecessarily. I wan't really happy with the Send_Email() mess myself, it was originally two separate conditionals that got combined while making changes. I can't believe I missed that ternary statement opportunity, so obvious. — PlasmaDan, Commented Sep 12, 2015 at 11:50
Yes, PLEASE handle error cases separately like if (badThing) { /* Error case: Bail out with non-0 return code, etc. */ } /* Normal case: no else or indent; just type the rest of the code */. Anything else just gets diabolical to read - as has been demonstrated. — underscore_d, Commented Sep 12, 2015 at 13:34

PlasmaDan · Accepted Answer · 2015-09-14 14:51:24Z

I've added a parameter to sanitise headers using the PEAR Mail snippet, so the function can be used for both headers and the mail body without stripping-out line-breaks.

// Sanitise mail
function Sanitise_Mail($h,$sanitise_headers = false) {
    $h = filter_var($h,FILTER_SANITIZE_STRING,FILTER_FLAG_NO_ENCODE_QUOTES);

    // Taken from PEAR Mail
    $h = !empty($sanitise_headers) ? preg_replace('=((<CR>|<LF>|0x0A/%0A|0x0D/%0D|\\n|\\r)\S).*=i','',$h) : $h;

    return $h;
}

I have also restructured the contact form include based on the feedback from the original question. I kept the SendEmail() functions together and just made it easier to read with more variable assignments.

<?php

// Submitted?
if(!isset($_POST['contact_submit'])) return;

$dependencies = array(
    'Sanitise_Mail',
    'Email_Settings',
    'Send_Email',
    'filter_var'
);

// Check dependencies
foreach($dependencies as $d) {
    if(!function_exists($d)) {
        $result['error'][] = @$lang->text['report_general_error'];
        error_report("Function $d() not defined, cannot continue.");
    }
}
if(!empty($result['error'])) return;

// Check IP ban list
if(function_exists('check_ip')) check_ip();
if(!empty($result['error'])) return;

// Check CAPTCHA
if(function_exists('check_captcha')) check_captcha();
if(!empty($result['error'])) return;

// Validate $_POST
if(empty($_POST['contact_name'])) $result['error'][] = @$lang->module['noname'];
if(empty($_POST['contact_email'])) $result['error'][] = @$lang->module['noemail'];
else if(!filter_var($_POST['contact_email'], FILTER_VALIDATE_EMAIL)) {
    $result['error'][] = @$lang->module['invalidemail'];
}
if(empty($_POST['contact_message'])) $result['error'][] = @$lang->module['nomessage'];

if(!empty($result['error'])) return;

// Send email
$mail_from_name = Sanitise_Mail($_POST['contact_name'], true);
$mail_subject = Sanitise_Mail($_POST['contact_subject'], true);
$mail_phone = Sanitise_Mail($_POST['contact_phone']);
$mail_body = Sanitise_Mail($_POST['contact_message']);
$mail_body .= !empty($mail_phone) ? "\r\n\r\n" . @$lang->module['phone'] . ' ' . $mail_phone : '';

$mail_to = Email_Settings()->receiver;
$mail_from = $_POST['contact_email'];
$mail_html = false;
$mail_failover = true;

if(Send_Email($mail_subject, $mail_body, $mail_to, $mail_from, $mail_from_name, $mail_html)
|| Send_Email($mail_subject, $mail_body, $mail_to, $mail_from, $mail_from_name, $mail_html, $mail_failover)) {
    // Success
    $result['success'][] = sprintf(@$lang->module['success'], $mail_from);
    unset($_POST);

    // Increment IP ban list
    if(function_exists('ban_ip')) {
        $ban_reason = @$lang->module['ban_ip'];
        $ban_ip = NULL;
        $ban_expire = strtotime('+31 minutes');

        ban_ip($ban_reason, $ban_ip, $ban_expire);
    }
}
else {
    $result['error'][] = sprintf(@$lang->module['error'], $mail_to);
    error_report('Send_Email() failed to send email.');
}

?>

Thanks for the help, any further comments welcome.

Stack Exchange Network

Sanitising contact form input for PHPMailer

3 Answers 3

No.

Your Answer

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

Hot Network Questions

Sanitising contact form input for PHPMailer

3 Answers 3

No.

Your Answer

Sign up or log in

Post as a guest

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

Related

Hot Network Questions

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