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

PHP form-to-email script

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

Problem

Someone says that my PHP code is vulnerable to XSS. I asked them what I should do to fix it, but now they don't want to help.

$action=$_REQUEST['action'];
                /* send the submitted data */
    {
    $name=$_REQUEST['name'];
    $email=$_REQUEST['email'];
    $message=$_REQUEST['message'];
    if (($name=="")||($email=="")||($message==""))
        {
        echo "All fields are required, please fill the form again.";
        }
    else{        
        $from="From: $name\r\nReturn-path: $email";
        $subject="Message sent using your contact form";
        mail("email@website.com", $subject, $message, $from);
        echo "Email sent!";
        }
    }  
?>

Solution

Sanitize the user input, but first and foremost: make sure there actually is any user input to sanitize:

$foo = $_POST['bar'];


might issue a notice, if that post variable doesn't exist, that's why it's considered good practice to use isset:

if (!isset($_POST['email']))
    exit('No email address provided');//don't use exit, redirect or something
$email = $_POST['email'];


Next, you sanitize and validate the input. For email addresses, that's easily done:

if (!filter_var($email, FILTER_VALIDATE_EMAIL))
    exit('Invalid email address given');//again, use redirect here


But all the other stuff has to be processed to avoid something you are wide open to:

Mail injection attacks

I've reviewed a couple of code snippets, in detail. Instead of copy-pasting my existing answers here, or typing them a second, third or fourth time, I'll just list a couple of links to some of these answers:

  • Critique sanitized user email PHP Script (Has most on mail injection, check the links in that answer, too)



  • Security of a "contact us" form Focusses more on email address validation



  • How can I make this code safer against XSS attacks? more general on XSS attacks, but make sure to check the links at the bottom.



Side-notes:

If a script contains nothing but PHP code, then the closing ?> tag is best omitted. This advice is given everywhere, including the official php.net site. Google as to why (whitespacing, parser tokens, ...)

I have the feeling you're not showing us the full code, because you have an opening and closing {} around everything, except the first statement.
If this code is the actual code, then please remove those redundant brackets: PHP isn't block-scoped, so they serve no purpouse, other than to confuse anyone else who might ever gaze at your code.

Code Snippets

$foo = $_POST['bar'];
if (!isset($_POST['email']))
    exit('No email address provided');//don't use exit, redirect or something
$email = $_POST['email'];
if (!filter_var($email, FILTER_VALIDATE_EMAIL))
    exit('Invalid email address given');//again, use redirect here

Context

StackExchange Code Review Q#49901, answer score: 3

Revisions (0)

No revisions yet.