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

Preventing email injection

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

Problem

I have asked a question on Stack Overflow and one of the guys actually managed to hack my contact form and inject a fake email into the $header of the PHP!

So after talking to him, he sent me a few articles, and I aimed to update the PHP to secure this gap and prevent the spam.

Here is my old PHP:


                        number: $number
                        date: $date
                        select: $select
                        radio: $radio
                        checkbox: $checkbox
                        Email: $email
                        Message: $message";

            $headers  = 'MIME-Version: 1.0' . "\r\n";
            $headers .= 'Content-type: text/html; charset=utf-8' . "\r\n";
            $headers .= 'From: '.$email . "\r\n";
            $mailed = (mail($to, $subject, $message, $headers));

            if( isset($_REQUEST['ajax']))$response = ($mailed) ? "1" : "0";
            else $response = ($mailed) ? "Success!" : "Error! There was a problem with sending.";
            echo $response;
         }

      } else {
        echo "Form data error!";
      }
      ob_flush();
      die();
   }
?>


and here is the updated version:

```
Invalid email address!";
break;

case !preg_match($spam_pattern, $name):
case !preg_match($spam_pattern, $number):
case !preg_match($spam_pattern, $date):
case !preg_match($spam_pattern, $select):
case !preg_match($spam_pattern, $radio):
case !preg_match($spam_pattern, $checkbox):
case !preg_match($spam_pattern, $message):
$response = "Invalid request made!";
break;

default:
$to = "";
$subject = "New Message From: $name";
$message = "Name: $name
number: $number
date: $date
select: $select
radio: $radio

Solution

@James is correct, PHP has function to validate so you don't have to (and they're better).

Your switch (true) is bothering me. I think an if condition could work there instead, as that's not why switches were built.

$mailed = (mail($to, $subject, $message, $headers));


You don't need the parentheses in this line. In fact, you could just shorten it all a bit and use:

if (mail($to, $subject, $message, $headers)) {
    $response = "Success!";
} else {
    $response = "Error! There was a problem with sending.";
}

Code Snippets

$mailed = (mail($to, $subject, $message, $headers));
if (mail($to, $subject, $message, $headers)) {
    $response = "<h2 style='color: green'>Success!</h2>";
} else {
    $response = "<h2 style='color: blue'>Error! There was a problem with sending.</h2>";
}

Context

StackExchange Code Review Q#42313, answer score: 5

Revisions (0)

No revisions yet.