Contact Form Sanitized Inputs

Question

I have the following as my Ajax / PHP contact form on a site I am building. I have sanitized the inputs but just wanted to make sure I havent missed anything...

    // Only process POST reqeusts.
    if ($_SERVER["REQUEST_METHOD"] == "POST") {
        // Get the form fields and remove whitespace.
        $name = strip_tags(trim($_POST["name"]));
        $name = str_replace(array("\r","\n"),array(" "," "),$name);
        $phone = trim($_POST["phone"]);
        $phone = filter_var($phone, FILTER_SANITIZE_NUMBER_INT);
        $email = filter_var($_POST['email'], FILTER_SANITIZE_EMAIL);
        $subject = strip_tags(trim($_POST["subject"]));
        $subject = str_replace(array("\r","\n"),array(" "," "),$subject);
        $message = strip_tags(trim($_POST["message"]));
        $message = str_replace(array("\r","\n"),array(" "," "),$message);

        // Check that data was sent to the mailer.
        if ( empty($name) OR empty($phone)) {
            // Set a 400 (bad request) response code and exit.
            http_response_code(400);
            echo "Oops! There was a problem with your submission. Please complete the form and try again.";
            exit;
        }

        // Set the recipient email address.
        $recipient = "[email protected]";

        // Set the email subject.
        $emailsubject = "New contact from $name";

        // Build the email content.
        $email_content = "Name: $name\n";
        $email_content .= "Email: $email\n\n";
        $email_content .= "Phone:\n$phone\n";
        $email_content .= "Subject:\n$subject\n";
        $email_content .= "Message:\n$message\n";

        // Build the email headers.
        $email_headers = "From: $name <$email>";

        // Send the email.
        if (mail($recipient, $emailsubject, $email_content, $email_headers)) {
            // Set a 200 (okay) response code.
            http_response_code(200);
            echo 'Thank You! Your message has been sent.';
        } else {
            // Set a 500 (internal server error) response code.
            http_response_code(500);
            echo "Oops! Something went wrong and we couldn't send your message.";
        }

    } else {
        // Not a POST request, set a 403 (forbidden) response code.
        http_response_code(403);
        echo "There was a problem with your submission, please try again.";
    }

Answer 1

The good news is that your script doesn't look exploitable. In particular, stripping out line break characters protects against header-splitting attacks. Good job there.

The bad news is that it can mangle the text unnecessarily. For example, if a user submits the form with the subject line

Your <blink> tag is annoying

… it will come across as

Subject: Your tag is annoying

And for what gain? strip_tags() is meant as a feeble defense against inappropriate HTML tags, but here you are sending plain text mail — a problem that has absolutely nothing to do with HTML.

It also probably does not make sense to strip all line termination characters from the message.

---

A more general concern I have is your use of the term sanitize, as it leads to confusion. I recommend striking that word from your programming vocabulary (even if the PHP documentation uses it), to be replaced by three specific terms:

1. Canonicalization (or "Normalization"): transforming input from multiple representations of the same data into one preferred form.

   For example, if your form accepts a credit card number, then you should strip out all whitespace, because they are not a meaningful part of the data. Another example would be to lower-case an e-mail address.

2. Validation: rejecting input that violates your rules. Validation failure should cause the user to have to resubmit after fixing the errors.

   For example, if your form accepts a credit card number, then you should reject any submission that does not contain the right number of digits, the correct leading digits for the accepted card types, and a correct Luhn checksum.

   For an e-mail form, your validation rules might require a name to be non-empty, a subject line that is one line of a reasonable length, and a plausible-looking e-mail address.

3. Escaping: transforming strings so that one kind of string can be safely embedded inside another kind of string without being misinterpreted.

   Any time you compose a string that will be interpreted by another computer system, be it an e-mail message, HTML page, or SQL query, assume that you are vulnerable to some kind of injection attack. Every single one of these "human-friendly" languages (as opposed to binary formats such as JPEG images) will have delimiters of special significance. Header-splitting, HTML/JavaScript injection, and SQL injection attacks all have the same root cause: careless string concatenation or interpolation.

   Therefore, before concatenating or interpolating strings, stop and think: "What is the appropriate escaping mechanism that I should be using?" In the case of e-mail headers, RFC 2047 suggests that the relevant escaping mechanism is mb_encode_mimeheader(). Sometimes, the answer is that no escaping is required — the e-mail message body in this example is one of those rare occasions where that's true.

Canonicalization provides user-friendliness. Validation enforces your business logic. Escaping, not canonicalization or validation, upholds security. The term "sanitize" conflates the three mechanisms, leading you do write improperly engineered code.

---

Suggested solution

function canonicalize(&$params) {
    $params['email'] = filter_var($params['email'], FILTER_SANITIZE_EMAIL);
    $params['phone'] = filter_var($params['phone'], FILTER_SANITIZE_NUMBER_INT);
}

function validate(&$params) {
    $errors = array();
    if (empty($params['name'])) {
        $errors['name'] = 'A name is required';
    }
    if (empty($params['email'])) {
        $errors['email'] = 'Invalid e-mail address';
    }
    if (empty($params['phone'])) {
        $errors['phone'] = 'A phone number is required';
    }
    return $errors;
}

function recipient(&$params) {
    return "[email protected]";
}

function subject(&$params) {
    return mb_encode_mimeheader("New contact from " . $params['name'], 'UTF-8', 'Q');
}

function body(&$params) {
    // Don't bother escaping e-mail body; it's for human consumption.
    return sprintf(
        "Name: %s\n" .
        "Email: %s\n" .
        "Phone: %s\n" .
        "Subject: %s\n" .
        "Message:\n%s\n",
        $params['name'], $params['email'], $params['phone'],
        $params['subject'], $params['message']);
}

function headers(&$params) {
    return sprintf(
        "From: %s <%s>",
        mb_encode_mimeheader($params['name'], 'UTF-8', 'Q'),
        $params['email']
    );
}

// Only process POST reqeusts.
if ($_SERVER["REQUEST_METHOD"] != "POST") {
    // Not a POST request, set a 405 (Method Not Allowed) response code.
    http_response_code(405);
    echo "There was a problem with your submission, please try again.";
} else {
    canonicalize($_POST);

    if (($errors = validate($_POST))) {
        display_form($errors);
    } elseif (!mail(recipient($_POST), subject($_POST), body($_POST), headers($_POST))) {
        // Set a 500 (Internal Server Error) response code.
        http_response_code(500);
        echo "Oops! Something went wrong and we couldn't send your message.";
    } else {
        // Set a 200 (Success) response code.
        http_response_code(200);
        echo 'Thank You! Your message has been sent.';
    }
}

In the rewrite above, I also proposed:

• Breaking up the code into functions.
• Rearranging the flow such that all of the error handlers come first. (There are many ways to fail along the one true path to success.)
• Changed the HTTP status code for non-POST requests to 405.
• A stub (to be implemented) so that the user can get more informative feedback on validation failure.

Answer 2

I have sanitized the inputs but just wanted to make sure I havent missed anything

If I pass [email protected]\nTo: [email protected] the newline will not be filtered out. You should use \\n instead of \n, or just nl2br.

Misc

• return early to reduce nesting for your POST check the same way you do it for your $name check: if ($_SERVER["REQUEST_METHOD"] != "POST") { http_response_code(403); exit; }
• be consistent with your variable names. You use underscore for most of them, so use it for emailsubject as well.
• some of your comments aren't necessary, as they just repeat the code itself (such as // Only process POST reqeusts.).