Charles is supporting a PHP based application. One feature of the application is a standard “Contact Us” form. I’ll let Charles take on the introduction:
While it looks fine on the outside, the code is a complete mess. The entire site is built with bad practices, redundant variables, poor validation, insecure cookie checks, and zero focus on maintainability or security. Even the core parts of the platform are a nightmare
We’re going to take this one in chunks, because it’s big and ugly.
try {
if (isset($_POST)) {
$name = $_POST['objName'];
$lst_name = $_POST['objLstName'];
$email = $_POST['objEmail'];
$phone = $_POST['objGsm'];
$message = $_POST['objMsg'];
$verifycode = $_POST['objVerifyCode'];
$objCmpT = $_POST['objCmpT'];
$objCmpS = $_POST['objCmpS'];
$objCountry = $_POST['objCountry'];
$objCity = $_POST['objCity'];
$objName2 = $_POST['objName2'];
$objLstName2 = $_POST['objLstName2'];
$objQuality = $_POST['objQuality'];
$objEmail = $_POST['objEmail'];
$objMsg2 = $_POST['objMsg2'];
$objVerifyCode2 = $_POST['objVerifyCode2'];
I don’t love that there’s no structure or class here, to organize these fields, but this isn’t bad, per se. We have a bunch of form fields, and we jam them into a bunch of variables. I am going to, with no small degree of willpower, not comment on the hungarian notation present in the field names. Look at me not commenting on it. I’m definitely not commenting on it. Look at me not commenting that some, but not all, of the variables also get the same hungarian prefix.
What’s the point of hungarian notation when everything just gets the same thing anyway; like hungarian is always bad, but this is just USELESS
Ahem.
Let’s continue with the code.
$ok = 0;
$ok2 = 0;
$sendTo = "[email protected]";
$golableMSG = '
-First Name & Last Name :' . $name . ' ' . $lst_name . '
-email :' . $email . '
-Phone Number : 0' . $phone . '
-Message : ' . $message;
$globaleMSG2 = '
-First Name & Last Name :' . $objName2 . ' ' . $objLstName2 . '
-Email :' . $objEmail . '
-Type of company : ' . $objCmpT . '
-Sector of activity : ' . $objCmpS . '
-Country : ' . $objCountry . '
-City : ' . $objCity . '
-Your position within the company : ' . $objQuality . '
-Message : ' . $objMsg2;
We munge all those form fields into strings. These are clearly going to be the bodies of our messages. Only now I’m noticing that the user had to supply two different names- $name and $objName2. Extra points here, as I believe they meant to name both of these message variables globaleMSG but misspelled the first one, golableMSG.
Well, let’s continue.
if (!$name) {
$data['msg1'] = '*';
} else {
$ok++;
$data['msg1'] = '';
}
if (!$lst_name) {
$data['msg2'] = '*';
} else {
$ok++;
$data['msg2'] = '';
}
if (!$email) {
$data['msg3'] = '*';
} else {
$ok++;
$data['msg3'] = '';
}
if ($phone <= 0) {
$data['msg4'] = '*';
} else {
$ok++;
$data['msg4'] = '';
}
if (!$message) {
$data['msg5'] = '*';
} else {
$ok++;
$data['msg5'] = '';
}
if (!$verifycode) {
$data['msg6'] = '*';
} else {
$ok++;
$data['msg6'] = '';
}
if (!$objCmpS) {
$data['msg7'] = '*';
} else {
$ok2++;
$data['msg7'] = '';
}
if (!$objCountry) {
$data['msg8'] = '*';
} else {
$ok2++;
$data['msg8'] = '';
}
if (!$objCity) {
$data['msg9'] = '*';
} else {
$ok2++;
$data['msg9'] = '';
}
if (!$objName2) {
$data['msg10'] = '*';
} else {
$ok2++;
$data['msg10'] = '';
}
if (!$objLstName2) {
$data['msg11'] = '*';
} else {
$ok2++;
$data['msg11'] = '';
}
if (!$objQuality) {
$data['msg12'] = '*';
} else {
$ok2++;
$data['msg12'] = '';
}
if (!$objMsg2) {
$data['msg13'] = '*';
} else {
$ok2++;
$data['msg13'] = '';
}
if (!$objVerifyCode2) {
$data['msg14'] = '*';
} else {
$ok2++;
$data['msg14'] = '';
}
What… what are we doing here? I worry that what I’m looking at here is some sort of preamble to verification code. But why is it like this? Why?
if ($ok == 6) {
if (preg_match("/^[ a-z,.+!:;()-]+$/", $name)) {
$data['msg1_1'] = '';
if (preg_match("/^[ a-z,.+!:;()-]+$/", $lst_name)) {
$data['msg2_2'] = '';
$subject = $name . " " . $lst_name;
if (filter_var($email, FILTER_VALIDATE_EMAIL)) {
$data['msg3_3'] = '';
$from = $email;
if (preg_match("/^[6-9][0-9]{8}$/", $phone)) {
$data['msg4_4'] = '';
if (intval($verifycode) == intval($_COOKIE['nmbr1']) + intval($_COOKIE['nmbr2'])) {
$data['msg6_6'] = '';
$headers = 'From: ' . $from . "\r\n" .
'Reply-To: ' . $sendTo . "\r\n" .
'X-Mailer: PHP/' . phpversion();
mail($sendTo, $subject, $golableMSG, $headers);
$data['msgfinal'] = 'Votre Messsage est bien envoyer';
} else {
$data['msg6_6'] = 'votre resultat est incorrect';
}
} else {
$data['msg4_4'] = 'Votre Numéro est incorrect';
}
} else {
$data['msg3_3'] = 'Votre Email est incorrect';
}
} else {
$data['msg2_2'] = 'Votre Prénom est Incorrect';
}
} else {
$data['msg1_1'] = 'Votre Nom est Incorrect';
}
}
Oh look, it is validation code. Their verification code system, presumably to prevent spamming messages, is not particularly secure or useful. The real thing I see here, though, is the namespaced keys. Earlier, we set $data['msg1'], and now we’re setting $data['msg1_1'] which is a code stench that could kill from a hundred yards.
And don’t worry, we do the same thing for the other message we send:
if ($ok2 == 8) {
if (preg_match("/^[ a-z,.+!:;()-]+$/", $objName2)) {
$data['msg10_10'] = '';
if (preg_match("/^[ a-z,.+!:;()-]+$/", $objLstName2)) {
$data['msg11_11'] = '';
$subject2 = $objName2 . " " . $objLstName2;
if (intval($objVerifyCode2) == intval($_COOKIE['nmbr3']) + intval($_COOKIE['nmbr4'])) {
$from2 = $objEmail;
$data['msg14_14'] = '';
$headers2 = 'From: ' . $from2 . "\r\n" .
'Reply-To: ' . $sendTo . "\r\n" .
'X-Mailer: PHP/' . phpversion();
mail($sendTo, $subject2, $globaleMSG2, $headers2);
$data['msgfinal'] = 'Votre Messsage est bien envoyer';
} else {
$data['msg14_14'] = 'votre resultat est incorrect';
}
} else {
$data['msg11_11'] = 'Votre Prénom est Incorrect';
}
} else {
$data['msg10_10'] = 'Votre Nom est Incorrect';
}
}
Phew. Hey, remember way back at the top, when we checked to see if the $_POST variable were set? Well, we do have an else clause for that.
} else {
throw new \Exception($mot[86]);
}
Who doesn’t love throwing messages by hard-coded array indexes in your array of possible error messages? Couldn’t be bothered with a constant, could we? Nope, message 86 it is.
But don’t worry about that exception going uncaught. Remember, this whole thing was inside of a try:
} catch (\Exception $e) {
$data['msgfinal'] = "Votre Messsage n'est pas bien envoyer";
}
Yeah, it didn’t matter what message we picked, because we just catch the exception and hard-code out an error message.
Also, I don’t speak French, but is “message” supposed to have an extra “s” in it?
Charles writes:
It’s crazy to see such sloppy work on a platform that seems okay at first glance. Honestly, this platform is the holy grail of messy code—it could have its own course on how not to code because of how bad and error-prone it is. There are also even worse scenarios of bad code, but it’s too long to share, and honestly, they’re too deep and fundamentally ingrained in the system to even begin explaining.
Oh, I’m sure we could explain it. The explanation may be “there was a severe and fatal lack of oxygen in the office, and this is what hypoxia looks like in code,” but I’m certain there’d be an explanation.
Your journey to .NET 9 is more than just one decision.Avoid migration migraines with the advice in this free guide. Download Free Guide Now!