Preventing email injection - Part 2

A day ago I have asked a question on here about Preventing email injection. I had some feedback and worked on it, and below is the latest update.

Could anyone please share their opinion? Is it secure enough? Is there a way to short-cut the code?

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!";
            break;
                default:
                    $to = "";
                    $subject = "New Message From: $name";
                    $message = "Name: $name
                                   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";

                    if(mail($to, $subject, $message, $headers)){
                        $response = "Success! Your submission has been sent.";
                    }else{
                        $response = "Error! There was a problem with sending.";
                    }
            break;
               }
            }
         }
         else {
            $response = "Error";
         }
      ob_flush();
   }
?>

• Due to the nested and long conditions the code becomes hard to follow

at some points. Early returns prevent nesting of conditions.

• Some code is duplicate (step 9)

• Possibly some values could be overwritten by url-forging (depending on the server-configuration). (step 5)

I'll do a step-by-step refactoring here with some explanations.

-
Move everything in a function. This eases early-return.

function sendMail() {
    // your code here. 
}

-
While not necessary, it's I like to invert the conditions so the static expression stands on the left side. This avoids accidental assignment in conditions. Avoids tricky to detect bugs (though most IDEs and codesniffer emit a warning).

-
Using the === operator for comparison avoids bugs to unforeseen casting

function sendMail() {
    if ('POST' !== $_SEVER['REQUEST_METHOD']) {
        return '';
    }

    if ($_SESSION['token'] !== $_POST['token']) {
        return '0';
    }
    // remaining code
}

-
What's the purpose of your output-cachting? I can't see any outputs being made? Trying to catch any errors? Hiding them is a bad idea, catch and handle them. If simply removed them.

• You are using $_REQUEST instead of $_POST. Sure you do check if the request type. But this doesn't prevent anyone from submitting both, GET and POST values. And here lies a dangerous problem: If I send you a link to form.php?message=something this does get send to the server as well when you submit the form. Now GET and POST both having the key name collide. Depending on the variables_order the GET value I set overwrites your message. If you use $_POST to access your variables instead this issue can be prevented.

-
I have extracted a simple accessor function to check for existence of a $_POST variable and return else:

function getPostValue($key) {
    return isset($_POST[$key]) ? $_POST[$key] : null;
}

function sendMail() {
    if ('POST' !== $_SEVER['REQUEST_METHOD']) {
        return '';
    }

    if ($_SESSION['token'] !== $_POST['token']) {
        return '0';
    }

    $_SESSION['token'] = '';

    $name = getPostValue('name');
    $email = getPostValue('email');
    $message = getPostValue('message');
    $number = getPostValue('number');
    $date = getPostValue('date');
    $select = getPostValue('select');
    $radio = getPostValue('radio');
    $checkbox = getPostValue('checkbox');

    if (!isset($name, $email, $message, $number, $date, $select, $radio, $checkbox)) {
        $response = "Error";
    }
}

-
All those input validations go with early return again (snippet only):

$spam_pattern = "/[\r\n]|Content-Type:|Bcc:|Cc:/i";
    if (!preg_match($spam_pattern, $name) 
        || !preg_match($spam_pattern, $number)
        || !preg_match($spam_pattern, $date)
        || !preg_match($spam_pattern, $radio)
        || !preg_match($spam_pattern, $checkbox)
        || !preg_match($spam_pattern, $select)
        || !preg_match($spam_pattern, $message)) {
        return 'Invalid Request!';
    }

-
The $to variables and others you use as parameters for mail are only used once. Furthermore the $message variable is overwritten, removing the original content. Variable reuse makes stuff hard to track though. I just do this all as arguments in the mail function (snippet):

$sendResult = mail(
        'support@loaidesign.co.uk', 
        'New Message From: ' . $name, 
        "Name: 
           number: $number
           date: $date
           select: $select
           radio: $radio
           checkbox: $checkbox
           Email: $email
           Message: $message", 
        "MIME-Version: 1.0 r\n Content-type: text/html; charset=utf-8 \r\n From: '.$email . '\r\n"
    );

-
All those returns have their html-markup in common. We sure can extract this into a common helper:

function getResponse($message, $isError) {
    return '' . $message . '';
}

-
Final remarks & further points of improvement:

• Indention of three characters is unusual. Most times its two or four.

• This can of course be further refactored: Especially creating local variables from $_POST is a point of repetation. Maybe it could be handled as an array. That'd make mass-checking (preg_match) easier: just loop over all input variables.

• If desired, this could of course be moved into a dedicated mailer class

• The functions depend on the global state ($_POST). A next step would be to pass the required input variables as argument (maybe an array :)). This would make this function reusable as well.

• Returning formated strings limits reusability to HTML output. Throwing expections or error codes puts this into the responsibility of the caller, not the callee.

My final result:

```
function getPostValue($key) {
return isset($_POST[$key]) ? $_POST[$key] : null;
}

function getResponse($message, $isError) {
return '' . $message . '';
}

function sendMail() {
if ('POST' !== $_SEVER['REQUEST_METHOD']) {

StackExchange Code Review Q#42411, answer score: 4