patternphpMinor
PHPmailer sanitazion/validation a second oppinion.
Viewed 0 times
oppinionvalidationsecondsanitazionphpmailer
Problem
I wonder if someone could give me some pointers to my PHPmailer Process file and see if they find any misstakes or things that I have missed in the validation/sanitation of the data.
Any input would be most wellcome!
```
$value) {
$entries[$key] = sanitize($value);
}
// Get a set of variable variables for easier use
foreach ($entries as $key => $value) {
${$key} = $value;
}
//validation
//contact_Name
if (empty($contact_Name)) {
$errors['contact_Name'] = 'Detta fält måste fyllas i.';
}
elseif (mb_strlen($contact_Name, 'UTF-8') > 50) {
$errors['contact_Name'] = 'Detta namn är för långt, får inte vara över 50 tecken.';
}
//contact_Foretag.
if (mb_strlen($contact_Foretag, 'UTF-8') > 50) {
$errors['contact_Foretag'] = 'Detta företagsnamn är för långt, får inte vara över 50 tecken';
}
//contact_Email
if (empty($contact_Email)) {
$errors['contact_Email'] = 'Detta fält måste fyllas i.';
}
elseif (!(filter_var($contact_Email, FILTER_VALIDATE_EMAIL))) {
$errors['contact_Email'] = 'Fyll i en giltig E-post address.';
}
//contact_Subject
if (empty($contact_Subject)) {
$errors['contact_Subject'] = 'Detta fält måste fyllas i.';
}
elseif (mb_strlen($contact_Subject, 'UTF-8') > 50) {
$errors['contact_Subject'] = 'Detta ämne är för långt, får inte vara över 50 tecken.';
}
//contact_Message
if (empty($contact_Message)) {
$errors['contact_Message'] = 'Detta fält måste fyllas i.';
}
elseif (mb_strlen($contact_Message, 'UTF-8') > 5000) {
$errors['contact_Message'] = 'Detta meddeoande är för långt, får inte vara över 5000 tecken.';
}
//If no errors
if (empty($errors)) {
$formOK = true;
$mail = new PHPMailer(true);
try {
$mail->IsSMTP();
$mail->SMTPDebug = 0;
$mail->SMTPAuth = true;
$mail->SMTPSecure = "ssl";
$mail->Host = MAIL_HOST;
$mail->Port = MAIL_PORT;
$mail->Username = MAIL_USER;
$mail->Password = MAIL_PASS;
$mail->SetFrom($contact_Email, $contact_Name);
$mail->Subject = "$contact_Subject";
$mail->Body = "Your received the following message from
Any input would be most wellcome!
```
$value) {
$entries[$key] = sanitize($value);
}
// Get a set of variable variables for easier use
foreach ($entries as $key => $value) {
${$key} = $value;
}
//validation
//contact_Name
if (empty($contact_Name)) {
$errors['contact_Name'] = 'Detta fält måste fyllas i.';
}
elseif (mb_strlen($contact_Name, 'UTF-8') > 50) {
$errors['contact_Name'] = 'Detta namn är för långt, får inte vara över 50 tecken.';
}
//contact_Foretag.
if (mb_strlen($contact_Foretag, 'UTF-8') > 50) {
$errors['contact_Foretag'] = 'Detta företagsnamn är för långt, får inte vara över 50 tecken';
}
//contact_Email
if (empty($contact_Email)) {
$errors['contact_Email'] = 'Detta fält måste fyllas i.';
}
elseif (!(filter_var($contact_Email, FILTER_VALIDATE_EMAIL))) {
$errors['contact_Email'] = 'Fyll i en giltig E-post address.';
}
//contact_Subject
if (empty($contact_Subject)) {
$errors['contact_Subject'] = 'Detta fält måste fyllas i.';
}
elseif (mb_strlen($contact_Subject, 'UTF-8') > 50) {
$errors['contact_Subject'] = 'Detta ämne är för långt, får inte vara över 50 tecken.';
}
//contact_Message
if (empty($contact_Message)) {
$errors['contact_Message'] = 'Detta fält måste fyllas i.';
}
elseif (mb_strlen($contact_Message, 'UTF-8') > 5000) {
$errors['contact_Message'] = 'Detta meddeoande är för långt, får inte vara över 5000 tecken.';
}
//If no errors
if (empty($errors)) {
$formOK = true;
$mail = new PHPMailer(true);
try {
$mail->IsSMTP();
$mail->SMTPDebug = 0;
$mail->SMTPAuth = true;
$mail->SMTPSecure = "ssl";
$mail->Host = MAIL_HOST;
$mail->Port = MAIL_PORT;
$mail->Username = MAIL_USER;
$mail->Password = MAIL_PASS;
$mail->SetFrom($contact_Email, $contact_Name);
$mail->Subject = "$contact_Subject";
$mail->Body = "Your received the following message from
Solution
and welcome to CodeReview!
Here's a few things:
Here's a few things:
- You have a lot of pointless comments
- ("includes": The includes are the next line. We see them; no need to tell us.)
- Indent block content.
- Whenever you enter an if block, indent it.
- Same for while/for/functions/classes/methods/etc
- Your variable-variable loop is equivalent to
extract.
extracttends to be a bad idea (the manual has various warnings)
- It pollutes the scope
- You no longer know what variables are and are not defined unless you strictly control what is inside of the array (which you don't for $_POST)
- Users can clobber your values, meaning there are certain security implications.
- Can be mitigated with options to extract, but still a problem
- In short, there are a few situations where extract is convenient and a decent idea; this is not one of them.
- use
$_POST[...]or$entries[...]directly.
- Variable variables are also a bad idea.
- Variable variables scream "What I really need is an array!"
- You already have an array :)
- Don't sanitize content until you're actually ready to put it into the context for which you're sanitizing it.
- For example, & is a special character in HTML (in some contexts). DBs do not care about it. Nor do plain text emails. Or email subjects. Or... infinite things.
- Rule of thumb: sanitize at the last possible moment.
- You can also take the raw value and process it, but you can't always do the reverse.
- Be careful with empty() on strings
- empty($var) === (isset($var) && $var)
- Implicit bool rules are very odd in PHP though. In particular, the string "0" can get you.
- I doubt
0should be a valid email content, but just throwing it out there in case it gets you in the future.
- Invert your conditions and you'll see that you can bail early:
- if (...) { /* huge block / } else { / redirect / exit; }
- Instead you could do if (!...) { / redirect */ exit; }
- Now your entire file doesn't have to be indented one step.
- This is similar to the concept of returning early, though it tends to be less applicable to files (redirection is one of the few justified uses of
exitin web site PHP)
- You have a typo of
$contact_Mamerather than$contact_Namein your email content
- Be careful with 500 errors. IE has a minimum length limit where if your response is less than some number of bytes, IE 'helpfully' replaces your content with its own.
- Also, I'm torn on how I feel about this being a 500 response. It's not really an internal error so much as just an exception.
- Internal errors implies a major mess up to me like Apache is pissed off, or a E_FATAL PHP error.
- You should probably log the email failing to send. You could just use something like
trigger_errorif you want to go the simple route.
$formOKis gaurenteed to be$formOK === empty($errors). This means you could just useempty($errors)in it's place.
- I tend to use only strict equalities with strings these days. If you're comparing something to a string, you're sure 99.9% of the time that it's a string. If you're sure it should be a string, might as well use a strict check.
- There are situations where loose string comparisons are still quite handy though.
- I would use a GET or POST variable to signify AJAX rather than checking a header.
- Headers can be spoofed.
- Testing becomes easier.
- No potential browser/JS library compatibility issues
$mail->Subject = "$contact_Subject";is (functionally) equivalent to$mail->Subject = (string) $contact_Subject;
- You know it's already a string, so no need for the cast.
- Just do
$mail->Subject = $contact_Subject.
- Also, I wouldn't use
"$var"as a cast. I would stick with explict casts ((string) $var).
Context
StackExchange Code Review Q#25182, answer score: 2
Revisions (0)
No revisions yet.