patternphpMinor
PHP contact form
Viewed 0 times
contactphpform
Problem
I recently had my site hacked, with the main index file being overwritten (nothing else was touched). I'm assuming they hijacked my form input.
Are there any vulnerabilities in the code or does this look to be safe now?
Are there any vulnerabilities in the code or does this look to be safe now?
Solution
I don't see vulnerabilities here:
But I see some dangerous practices.
You create
At this point all the
I don't know if this is intentional,
for example because you want to preserve the original inputs instead of using the sanitized ones.
This is a bit confusing.
The script uses
It would be better to create cleaned copies early on,
and work with the cleaned versions,
for example:
I find it odd that you clear the values of the
This seems to suggest that you don't trust the rest of the program might use these values by accident.
It would be better to reorganize your code to render such accidental uses impossible,
something you can trust, so you don't need to write paranoid code.
For example it would be better to reorganize your code to functions,
and have a better control of the flow of the program and the actions taken.
A common practice is to use a
so that you can have a main flow like this:
- The script is not executing shell commands
- The script is not executing database queries
- The only "external" call is to
mail(...), the arguments are reasonably sanitized, and in any case I would expect themail(...)function to do its own sanitization, and not erase the hard disk if something really evil is embedded$message
But I see some dangerous practices.
You create
$message from input that's not yet validated:$name = $_POST['name'];
$email = $_POST['email'];
$comments = $_POST['comments'];
$feedback = "";
$message = <<<EMAIL
Name: $name
Email: $email
Message:
$comments
EMAIL;At this point all the
$_POST fields are not validated yet.I don't know if this is intentional,
for example because you want to preserve the original inputs instead of using the sanitized ones.
This is a bit confusing.
The script uses
$_POST["something"] repeatedly.It would be better to create cleaned copies early on,
and work with the cleaned versions,
for example:
$cleaned_name = clean_input($_POST['name']);
$cleaned_email = clean_input($_POST['email']);
$cleaned_comments = clean_input($_POST['comments']);
function clean_input($data) {
$data = trim($data);
$data = stripslashes($data);
$data = htmlspecialchars($data);
return $data;
}test_input was not a good name for this purpose, so I renamed it to clean_input.I find it odd that you clear the values of the
$_POST:mail($myemail,$subject,$message);
$feedback = "Thanks for contacting us ! We'll be in touch soon.";
$_POST['name'] = "";
$_POST['email'] = "";
$_POST['comments'] = "";This seems to suggest that you don't trust the rest of the program might use these values by accident.
It would be better to reorganize your code to render such accidental uses impossible,
something you can trust, so you don't need to write paranoid code.
For example it would be better to reorganize your code to functions,
and have a better control of the flow of the program and the actions taken.
A common practice is to use a
Form class,so that you can have a main flow like this:
$form = new Form;
if ($form->is_valid()) {
// send email, taking values from $form->cleaned_data
} else {
// print errors, taking values from $form->errors
}Code Snippets
$name = $_POST['name'];
$email = $_POST['email'];
$comments = $_POST['comments'];
$feedback = "";
$message = <<<EMAIL
Name: $name
Email: $email
Message:
$comments
EMAIL;$cleaned_name = clean_input($_POST['name']);
$cleaned_email = clean_input($_POST['email']);
$cleaned_comments = clean_input($_POST['comments']);
function clean_input($data) {
$data = trim($data);
$data = stripslashes($data);
$data = htmlspecialchars($data);
return $data;
}mail($myemail,$subject,$message);
$feedback = "Thanks for contacting us ! We'll be in touch soon.";
$_POST['name'] = "";
$_POST['email'] = "";
$_POST['comments'] = "";$form = new Form;
if ($form->is_valid()) {
// send email, taking values from $form->cleaned_data
} else {
// print errors, taking values from $form->errors
}Context
StackExchange Code Review Q#82799, answer score: 3
Revisions (0)
No revisions yet.