Validation before sending mail in PHP

Question

In my initial problem posted on SO, whenever anyone accessed the Mail.php on the server, it used to send an empty email to the $to. To avoid this I came up with a solution.

require 'PHPMailer/PHPMailerAutoload.php';
    
    $yourName = $_POST['yourName'];
    $sender  = $_POST['emailID'];
    $subject  = $_POST['subject'];
    $message  = $_POST['message'];
    $to = '[email protected]';

        if(empty($yourName) || empty($sender) || empty($subject) || empty($message) || empty($message))
        {
            echo "Fields are empty";
        }
        else
        {
                $mail = new PHPMailer;
                //$mail->SMTPDebug = 2;                               
                $mail->isSMTP();                                      
                $mail->Host = 'smtp.gmail.com';  
                $mail->SMTPAuth = true;                               
                $mail->Username = 'gmailUser';      
                $mail->Password = 'gmailPassword';                     
                $mail->SMTPSecure = 'tls';                            
                $mail->Port = 587;                                    

                $mail->setFrom($sender,$yourName);
                $mail->addAddress($to);     
                $mail->addReplyTo($sender);

                $mail->isHTML(true);                                  

                $mail->Subject = $subject;
                $mail->Body    = "<b>From: </b>". $sender. "<br>" ." <b>Name: </b>". $yourName. "<br>". "<b> Message Body </b>" .$message;
                $mail->AltBody = "<b>From: </b>". $sender. "<br>" ." <b>Name: </b>". $yourName. "<br>". "<b> Message Body </b>" .$message;

                if(!$mail->send()) 
                {
                    echo 'Message could not be sent.';
                    echo 'Mailer Error: ' . $mail->ErrorInfo;
                } 
                else 
                {
                    echo "Message has been sent....You're being redirected.....";
                }
        }

This fix, basically allows the user to interact with Mail.php, but Mail.php doesn't sends a null email to $to

Now hours later, I find out that this is a very bad approach.

I would like to know how a professional would solve this problem?

Any good approach which I could use to efficiently optimize the code?

Answer 1

Not a professional, but might help.

You are accessing elements from $_POST without checking if they exist in the array. If (say, message) is not in the array, php will give an "undefined index" error. This might not be an issue if you are perfectly sure your values will always come from forms, but then this code might have limited use (see below). The empty check you did later is too late to have any effect. This can easily be fixed by doing the empty checks first.

I would suggest an alternative approach to:

    if(empty($yourName) || empty($sender) || empty($subject) || empty($message) || empty($message))
    {
        echo "Fields are empty";
    }
    else
    {
        ...
    }

I think if code inside the else got larger and larger, and if you include many more if ... else statements around them, the code might become hard to read and debug. I would suggest doing something like this.

    if(empty($yourName) || empty($sender) || empty($subject) || empty($message) || empty($message))
    {
        echo "Fields are empty";
        return;
    }
    ...

Now one less missing } to worry about if your code explodes.

No need to do empty($message) twice.

I guess you are using empty to check if a field has no characters in it. Be aware that input such as "0" is also considered empty by php.

I would encourage you to make this code a little more general. That is, you can reuse this code by rapping it in a function. If your system wants to send different kinds of emails from different sources, it would be a pain to copy this code over and over again. I would do something like this:

function sendMail($name, $sender, $subject, $message, $to, $subject, $body) {
    // ...
}

You see, by putting it inside a function, your email code now belongs to one place and can be used by many other sources other than forms (E.G. system generated or data from files). This is why aside from checking if the value of a specific field has any characters, validating if a field exist is also very important.

Finally, I think any configuration such as port, host, username and password should belong to a configuration file and retrieved by the function. That way they would be easier to find and change.

Answer 2

Validate your fields individually so you can provide the client side with better error messages (assuming this is an AJAX script)...

$reqd_params = array('yourName', 'emailID', 'subject', 'message');
foreach($reqd_params as $param)
    !empty($_REQUEST[$param]) || die("$param parameter is required.");

The AltBody paramter is supposed to be the plaintext version of the regular body, yours has html in it. You need to do wrap it in striptags and nl2br like so... striptags(nl2br($my_html_message))

Avoid returning plain text from an AJAX script too. Set your properties in an array and return it in JSON so Javascript can read it easier.

for example:

header("content-type: application/json");
echo json_encode(array(
    "success" => true,
    "message" => "email_sent"
));

Edit.. I said nl2br but what u need is the opposite of that..