HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Sanitising contact form input for PHPMailer

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
contactinputforformsanitisingphpmailer

Problem

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('=((||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;

Solution

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.

Code Snippets

else if(function_exists('Sanitise_Mail')) {
$result['error'][] = @$lang->module['invlaidemail'];

Context

StackExchange Code Review Q#104482, answer score: 5

Revisions (0)

No revisions yet.