2
\$\begingroup\$

This code processes a basic contact form with a name, email and message. It also asks for a code to be typed in. The data is then transformed to avoid malicious placing of HTML script on the server, and if the name and email are valid, sent to my email.

Is this code really secure against the attempt to place malicious HTML code?

<!DOCTYPE html>
<html>
    <head>
        <title>Contact Form</title>
        <meta charset="utf-8">
        <link rel="stylesheet" href="css/site.css" >
        <link rel="stylesheet" href="css/font-awesome-4.4.0/css/font-awesome.min.css" >
    </head>

    <body>
        <?php
            $nameErr = $emailErr = $messageErr = $errors = "";
            $name = $email = $message = "";
            $myemail = 'myemail@email.com';

            if (empty($_POST["name"])) {
                $nameErr = " (required)";
            } else {
                $name = test_input($_POST["name"]);
                if (!preg_match("/^[a-zA-Z ]*$/",$name)) {
                    $nameErr = " only letters and white space allowed"; 
                }
            }
            if (empty($_POST["email"])) {
                $emailErr = " (required)";
            } else {
                $email = test_input($_POST["email"]);
                if (!filter_var($email, FILTER_VALIDATE_EMAIL)) {
                    $emailErr = " invalid email format"; 
                }
            }
            if (empty($_POST["message"])) {
                $messageErr = " (required)";
            } else {
                $message = test_input($_POST["message"]);
            }

            if(empty($nameErr) && empty($emailErr) && empty($messageErr))
            {
                if (strtolower($_POST['code']) != 'mycode') {die('Wrong access code');}
                $to = $myemail;
                $email_subject = "Contact form submission - $name";
                $email_body = "Name: $name \n " . "Email: $email\n\n $message";
                $headers = "From: $myemail\n";
                $headers .= "Reply-To: $email";
                mail($to,$email_subject,$email_body,$headers);
                $message = "Thank you for your message! :)";
                $name = " ";
                $email = " ";
            }

            function test_input($data) {
                $data = trim($data);
                $data = stripslashes($data);
                $data = htmlspecialchars($data);
                return $data;
            }
        ?>


            <form method="post" action="<?php echo htmlspecialchars($_SERVER["PHP_SELF"]);?>">
                <div>
                    <label for="name">Name:</label>
                    <input type="text" name="name" <?php echo "value = " . $name;?>>
                    <span><?php echo $nameErr;?></span>
                </div>
                <div>
                    <label for="mail">E-mail:</label>
                    <input type="email" name="email" <?php echo "value = " . $email;?>>
                    <span><?php echo $emailErr;?></span>
                </div>
                <div>
                    <label for="msg">Message:</label>
                    <textarea name="message"><?php echo $message;?></textarea>
                    <span><?php echo $messageErr;?></span>
                </div>
                <div>
                    <label for="code">Access code:</label>
                    <input type="text" name="code">
                    <i class="fa fa-arrow-left"></i> type <span class="em4">mycode</span> here
                </div>
                <div class="button">
                    <button type="submit">Send</button>
                </div>
            </form>

            <br>
            <footer id="foot01"></footer>
            <script src="css/script.js"></script>
        </div>
    </body>
</html>
| improve this question | | | | |
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! To help reviewers give you better answers, please add sufficient context to your question. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. See also this meta question. \$\endgroup\$ – SuperBiasedMan Oct 22 '15 at 14:57
  • \$\begingroup\$ Thanks, I've added some more context. I hope this gets us there :) \$\endgroup\$ – bakunin Oct 22 '15 at 15:00
2
\$\begingroup\$

test_input

I see this function quite often, and I would not recommend using it. It just applies some rather random functions to the input which may or may not be appropriate.

I went into this a little more in-depth here, but the gist of is is this:

  • stripslashes only makes sense if magic quotes are enabled, so you should check for that first (and only apply stripslashes in some bootstrap file to GET/POST). If you apply it even when magic quotes is not enabled - the default, hopefully - , you will get usability issues (eg with email addresses like jane\'smith@gmail.com).
  • htmlspecialchars should be applied when outputing data, not when reading user input into variables. If you don't follow this, it will be really difficult to know which values that are echoed are actually safe and which are not.

Security

Your code is open to reflected XSS. As an example, use this as email address:

foo autofocus onfocus=alert(1) foo=foo.com

The problem is that you do not have quotes around your values.

I'm not quite sure if you were thinking of XSS when you said avoid malicious placing of html script on the server, if not: cleaning the Email of HTML is not really the job of your Email send script, but of the script that later reads and displays the Email (if that is not secure, I could just bypass your filters of the Email content by sending you Emails directly rather than using your Email send script).

Misc

  • You are not consistent with your placement of curly brackets
  • Multiple assignments on a single line are hard to read. Also, you don't need to assign variables the empty string. So something like $name = $email = $message = ""; can just be removed.
  • some of your for attributes do not match the input fields.
  • you have one </div> too many.
| improve this answer | | | | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.