patternphpMinor
PHP form-to-email script
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:
might issue a notice, if that post variable doesn't exist, that's why it's considered good practice to use
Next, you sanitize and validate the input. For email addresses, that's easily done:
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:
Side-notes:
If a script contains nothing but PHP code, then the closing
I have the feeling you're not showing us the full code, because you have an opening and closing
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.
$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 hereBut 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 hereContext
StackExchange Code Review Q#49901, answer score: 3
Revisions (0)
No revisions yet.