contact.php file

Question

This is my contact.php file which has only 2 functionalities:

1. Response with the default page if it is requested by the get method
2. Process the message posted by the client if it is requested by the post method

It didn't designed to process any ajax or any other request, just the "get the page" and "post this message" requests.

I'm using functions and variables that is all defined in that file.

This is the file as it is in it's "native habitat", which was just copied from Notepad++ and pasted it here.

<?php // contact page
require_once 'data/included.php';
$pageup = gettembleteup();
$pagetitle = "Contact";
$pagereadyscript="";
$pagestyle="div#contact{background-color:rgb(210,210,210);max-width:370px;border-radius: 30px;padding:15px;} td.frmtext{text-align:left;vertical-align:top;width:80px;}
td.inputs{text-align:center;vertical-align:top;width:250px;} td input,textarea,select {width:100%}";
$pageup = str_replace("pagetitleplaceholder",$pagetitle,$pageup);
$pageup = str_replace("pagereadyscriptplaceholder",$pagereadyscript,$pageup);
$pageup = str_replace("pagestyleplaceholder",$pagestyle,$pageup);
$pageup = str_replace("pagebodyplaceholder","",$pageup);
echo $pageup;
$pagedown = gettembletedown("");
$message = "";
$invalidinput = false;
// posting a contact message
if($_SERVER['REQUEST_METHOD'] == "POST"){rqdata("contact-001","submit query",0);
 $name = isset($_POST['name']) ? $_POST['name'] : "";
 $phone = isset($_POST['phone']) ? $_POST['phone'] : "";
 $email = isset($_POST['email']) ? $_POST['email'] : "";
 $subject = isset($_POST['subject']) ? $_POST['subject'] : "";
 $title = isset($_POST['title']) ? $_POST['title'] : "";
 $contactingmessage = isset($_POST['message']) ? $_POST['message'] : "";
 //checking values
 if (strlen($contactingmessage) == 0){$invalidinput = true;lg("contact-301","embty message",1,1);
 $message = "We didn't receive your query. you didn't enter a message!";
 $message = str_replace("placeholder",$message,$errormessage);
 };
 if(!$invalidinput){
 if (strlen($name) > 45){$invalidinput = true;lg("contact-302","long name",1,2);
 $message = "We didn't receive your query. Name must be less than 45";
 $message = str_replace("placeholder",$message,$errormessage);
 };
 }; 
 if(!$invalidinput){
 if(strlen((string)$phone) > 0 && preg_match("/[^0-9+ ]/u",(string)$phone)){$invalidinput = true;lg("contact-303","invalid phone",1,1);
 $message = "We didn't receive your query. Phone number must contains only numbers , or + sign";
 $message = str_replace("placeholder",$message,$errormessage);
 };
 };
 if(!$invalidinput){
 if($email !== ""){
 if(filter_var($email, FILTER_VALIDATE_EMAIL) === false){$invalidinput = true;lg("contact-304","invalid email",1,1);
 $message = "We didn't receive your query. The Email is not correct.";
 $message = str_replace("placeholder",$message,$errormessage);
 }
 };
 };
 if(!$invalidinput){
 if($title !== ""){
 if(strlen((string)$title) > 75){$invalidinput = true;lg("contact-305","long title",1,2);
 $message = "We didn't receive your query. Title must be less than 75";
 $message = str_replace("placeholder",$message,$errormessage);
 }
 };
 };
 if(!$invalidinput){
 if(strlen((string)$contactingmessage) > 1000){$invalidinput = true;lg("contact-305","long msg",1,2);
 $message = "We didn't receive your query. Message must be less than 1000 length";
 $message = str_replace("placeholder",$message,$errormessage);
 }
 };
 // finished validation
 if($message === ""){
 $name = $websiteconn->real_escape_string($name);
 $title = $websiteconn->real_escape_string($title);
 $contactingmessage = $websiteconn->real_escape_string($contactingmessage);
 $subject = $websiteconn->real_escape_string($subject);
 if($session['id'] == ""){$session['id'] ="null";}else{$session['id'] = "'".$session['id']."'";};
 $query = "insert into message(message_name,message_phone,message_email,message_subject,message_title,message_message,session_id)
 values('$name','$phone','$email','$subject','$title','$contactingmessage',".$session['id'].");";
 $result = $websiteconn->query($query); if(!$result){$erid = lg("contact-306",sqlerror($query,$websiteconn->error));mdie("Error-id:$erid");};
 if($email == "" && $phone == "" && ($session['username'] =='guest' || $session['id'] == "null")){
 $message = "We have recieved your query successifully. and we are proccessing it.<br>
 We can't replay to you as you didn't provide a phone number or email and you are not signed in!";
 $message = str_replace("placeholder",$message,$warningmessage);
 }else{
 $message = "We have received your query successifully. and we are proccessing it.<br>
 Your contact information that we have is : ".$session['phonenumber'].", ".$session['email'].", ".$email.", ".$phone.".";
 $message = str_replace("placeholder",$message,$successmessage);
 };
 };
 echo $message;
};
?>
<?php
 //getting the page(not posting a contact message)
 //$_GET['subject'] is a paramter to load the page with a sleected subject from the subjects select box
 if($_SERVER['REQUEST_METHOD'] == "GET"){rqdata("contact-002","contact page",0);};
 if($_SERVER['REQUEST_METHOD'] == "GET" || $invalidinput){
 $getsubject = isset($_GET['subject']) ? $_GET['subject'] : "";
 $firstoption="<option>accounting question</option>";
 if($getsubject){
 switch ($getsubject){
 case "reporterror":$firstoption="<option>error/bug reporting</option>";
 break;
 default:$firstoption="<option>other</option>";
 };
 };
 $subjectoptions = "<option>accounting question</option><option>programming question</option>
 <option>complaint/complement</option><option>I want to buy an application</option><option>I want to try an application for free</option><option>co-operation/partnership</option>
 <option>website feedback</option><option>error/bug reporting</option><option>other</option>";
 $subjectoptions = str_replace($firstoption,"",$subjectoptions);
 $firstoption .= $subjectoptions; $subjectoptions = $firstoption;
 echo "
 <img src='media/smileface3.png' style='height:100px;width:100px;'>
 <div id='contact' style='color:black;display:inline-block;vertical-align:top;text-align:left;'><h2 style='display:inline'>Contact Me</h2><br><br>
 <form method='post' action='contact.php'>
 <table>
 <tr><td class='frmtext'>Name : </td><td class='inputs'><input type='text' name='name'></input></td></tr>
 <tr><td class='frmtext'>Phone: </td><td class='inputs'><input type='text' name='phone'></input></td></tr>
 <tr><td class='frmtext'>Email: </td><td class='inputs'><input type='text' name='email'></input></td></tr>
 <tr height='10'><td></td></tr>
 <tr><td class='frmtext'>Subject: </td><td class='inputs'><select name='subject'>$subjectoptions
 </select></td></tr>
 <tr><td class='frmtext'>Title: </td><td class='inputs'><input type='text' name='title' value='";
 if($_SERVER['REQUEST_METHOD'] == "POST" && $invalidinput){echo $title;};
 echo"'></input></td></tr>
 <tr><td class='frmtext'>Message: <span style='color:red'>*</span></td><td class='inputs'><textarea maxlength='1000' name='message'>";
 if($_SERVER['REQUEST_METHOD'] == "POST" && $invalidinput){echo $contactingmessage;};
 echo"</textarea></td></tr><tr><td></td><td class='inputs'><input type='submit' value='submit' style='width:120px'></input></td></tr>
 </table></form><span style='font-size:90%'><i><b>Phone, Email:</b> without any of them we will not be able to contact you back</i></span>
 <hr><b>Or Send An Email</div>";
 };
 echo $pagedown;
?>

Please note that I've never seen a real working complete PHP application, and I'm seeking any comments and advice.

• Ron Beyer: You should avoid putting more than one statement on a line, typically. So this: $invalidinput = true;lg("contact-301","embty message",1,1); should have started a new line after the { and each have been on their own line.
• Mike Brant: I'll be honest. This code looks like it was pulled from a 10 year old PHP site. I could go through are review this, but I am going to give comments very similar to what you received on the linked code review. Mainly that is, that is is almost a non-starter to review for things like security concerns, alternate coding approaches, etc. because there are just SO many stylistic problems that I think you almost need to take a step back and try to implement some of what was surfaced on your other review before trying to worry about reviewing related code that has A LOT of the same problems.
• Mike Brant: I guess another thing that would be important to point out is to discard whatever sources you are using to learn PHP, as they are clearly both dated, and not focused on code quality. You need to seek out sources of good coding behavior - real world open source PHP applications or frameworks is a good start - recently updated PHP books with focus on OOP, design patterns, practical security, etc. And start using an actual IDE with full support for PHP as well as support for enforcing codiong style standards like PSR standards - php-fig.org/psr
• Mike Brant: @Accountantم W3 Schools is almost universally regarded in the development community as an awful source of information. It is a site that is great at SEO optimization, but really poor in terms of content. Google "top PHP frameworks" or similar and take a look at some of the more popular ones out there - Zend, CodeIgniter, Symfony, Yii, etc. These are not applications themselves, but they are heavily-used well know frameworks, each with their own take on how one might build an app. Try a few of them out. Strive to see how they do things like handling form posts, separate view logic, etc.
• Mike Brant: Consider reading phptherightway.com and amazon.com/gp/product/1491905018/… or

Answer 1

As with your previous question, there is a lot wrong here. I'll focus on formatting issues, as they are easiest to fix and thus provide the most cost/benefit right now.

Most of these are relatively easy to apply to your code, and you really should do them.

• You need to put each statement on its own line! Will your code get longer? Yes. But it will also get more readable (or readable in the first place, really). You can see a statement as anything that is ended with a ; or a {. You can use an IDE to do this for you.
• The same goes for your CSS and HTML code. For HTML code, each Tag should be on its own line.
• You need to change how you name variables. Typing variable names in all lowercase is completely unreadable. gettembleteup should be getTemplateUp, etc. You can easily rename variables using an IDE.
• You should put all your CSS code in a CSS file. This will improve readability of your code, and also increase the performance of your application.
• I would also suggest to put all the HTML code in its own file (eg by using a templating engine). But this will require some rewriting of your code.

Security

This time I actually checked, and you are indeed vulnerable to SQL injection and XSS:

The email parameter is vulnerable:

foo'[email protected]

An attacker can thus read data from your database, and possibly even get complete control of your server.

You are vulnerable to XSS via the title and contactingmessage parameter. An attacker can thus steal cookies from users, perform actions for them, etc.

You are also vulnerable to CSRF, but that's not that serious for an email form. Still, it makes the exploitation of the XSS issue possible, and I would assume that there is no CSRF protection in the rest of your code either.

Personally, I would very strongly suggest against using this code on any server connected to the internet. Even if you were to fix the problems above, the way the code is written, these will not be the only vulnerabilities, and it will be impossible to fix them all. I would suggest rewriting the entire application from scratch, and sticking to proper coding standards as well as security best practices (prepared statements, templating engine with default encoding, and so on).

Formatting Example

Here is your code with some formatting improvements, to give you an idea. It's still far from perfect, but it's a start:

<?php
// contact page
require_once 'data/included.php';
$pageup = gettembleteup();
$pagetitle = "Contact";
$pagereadyscript = "";
$pagestyle = "div#contact{
 background-color:rgb(210,210,210);
 max-width:370px;
 border-radius: 30px;
 padding:15px;
} 
td.frmtext{
 text-align:left;
 vertical-align:top;
 width:80px;
}
td.inputs{
 text-align:center;
 vertical-align:top;
 width:250px;
} 
td input,textarea,select {
 width:100%
}";
$pageup = str_replace("pagetitleplaceholder", $pagetitle, $pageup);
$pageup = str_replace("pagereadyscriptplaceholder", $pagereadyscript, $pageup);
$pageup = str_replace("pagestyleplaceholder", $pagestyle, $pageup);
$pageup = str_replace("pagebodyplaceholder", "", $pageup);
echo $pageup;
$pagedown = gettembletedown("");
$message = "";
$invalidinput = false;
// posting a contact message
if ($_SERVER['REQUEST_METHOD'] == "POST") {
 rqdata("contact-001", "submit query", 0);
 $name = isset($_POST['name']) ? $_POST['name'] : "";
 $phone = isset($_POST['phone']) ? $_POST['phone'] : "";
 $email = isset($_POST['email']) ? $_POST['email'] : "";
 $subject = isset($_POST['subject']) ? $_POST['subject'] : "";
 $title = isset($_POST['title']) ? $_POST['title'] : "";
 $contactingmessage = isset($_POST['message']) ? $_POST['message'] : "";
 //checking values
 if (strlen($contactingmessage) == 0) {
 $invalidinput = true;
 lg("contact-301", "embty message", 1, 1);
 $message = "We didn't receive your query. you didn't enter a message!";
 $message = str_replace("placeholder", $message, $errormessage);
 }
 if (!$invalidinput && strlen($name) > 45) {
 $invalidinput = true;
 lg("contact-302", "long name", 1, 2);
 $message = "We didn't receive your query. Name must be less than 45";
 $message = str_replace("placeholder", $message, $errormessage);
 }
 if (!$invalidinput && strlen((string) $phone) > 0 && preg_match("/[^0-9+ ]/u", (string) $phone)) {
 $invalidinput = true;
 lg("contact-303", "invalid phone", 1, 1);
 $message = "We didn't receive your query. Phone number must contains only numbers , or + sign";
 $message = str_replace("placeholder", $message, $errormessage);
 }
 if (!$invalidinput && $email !== "" && filter_var($email, FILTER_VALIDATE_EMAIL) === false) {
 $invalidinput = true;
 lg("contact-304", "invalid email", 1, 1);
 $message = "We didn't receive your query. The Email is not correct.";
 $message = str_replace("placeholder", $message, $errormessage);
 }
 if (!$invalidinput && $title !== "" && strlen((string) $title) > 75) {
 $invalidinput = true;
 lg("contact-305", "long title", 1, 2);
 $message = "We didn't receive your query. Title must be less than 75";
 $message = str_replace("placeholder", $message, $errormessage);
 }
 if (!$invalidinput && strlen((string) $contactingmessage) > 1000) {
 $invalidinput = true;
 lg("contact-305", "long msg", 1, 2);
 $message = "We didn't receive your query. Message must be less than 1000 length";
 $message = str_replace("placeholder", $message, $errormessage);
 }
 // finished validation
 if ($message === "") {
 $name = $websiteconn->real_escape_string($name);
 $title = $websiteconn->real_escape_string($title);
 $contactingmessage = $websiteconn->real_escape_string($contactingmessage);
 $subject = $websiteconn->real_escape_string($subject);
 if ($session['id'] == "") {
 $session['id'] = "null";
 } else {
 $session['id'] = "'" . $session['id'] . "'";
 }
 $query = "insert into message(message_name,message_phone,message_email,message_subject,message_title,message_message,session_id)
 values('$name','$phone','$email','$subject','$title','$contactingmessage'," . $session['id'] . ");";
 $result = $websiteconn->query($query);
 if (!$result) {
 $erid = lg("contact-306", sqlerror($query, $websiteconn->error));
 mdie("Error-id:$erid");
 }
 if ($email == "" && $phone == "" && ($session['username'] == 'guest' || $session['id'] == "null")) {
 $message = "We have recieved your query successifully. and we are proccessing it.<br>
 We can't replay to you as you didn't provide a phone number or email and you are not signed in!";
 $message = str_replace("placeholder", $message, $warningmessage);
 } else {
 $message = "We have received your query successifully. and we are proccessing it.<br>
 Your contact information that we have is : " . $session['phonenumber'] . ", " . $session['email'] . ", " . $email . ", " . $phone . ".";
 $message = str_replace("placeholder", $message, $successmessage);
 }
 }
 echo $message;
}
?>
<?php
//getting the page(not posting a contact message)
//$_GET['subject'] is a paramter to load the page with a sleected subject from the subjects select box
if ($_SERVER['REQUEST_METHOD'] == "GET") {
 rqdata("contact-002", "contact page", 0);
}
if ($_SERVER['REQUEST_METHOD'] == "GET" || $invalidinput) {
 $getsubject = isset($_GET['subject']) ? $_GET['subject'] : "";
 $firstoption = "<option>accounting question</option>";
 if ($getsubject) {
 switch ($getsubject) {
 case "reporterror":$firstoption = "<option>error/bug reporting</option>";
 break;
 default:$firstoption = "<option>other</option>";
 }
 }
 $subjectoptions = "<option>accounting question</option>
 <option>programming question</option>
 <option>complaint/complement</option>
 <option>I want to buy an application</option>
 <option>I want to try an application for free</option>
 <option>co-operation/partnership</option>
 <option>website feedback</option>
 <option>error/bug reporting</option>
 <option>other</option>";
 $subjectoptions = str_replace($firstoption, "", $subjectoptions);
 $firstoption .= $subjectoptions;
 $subjectoptions = $firstoption;
 echo "
 <img src='media/smileface3.png' style='height:100px;width:100px;'>
 <div id='contact' style='color:black;display:inline-block;vertical-align:top;text-align:left;'>
 <h2 style='display:inline'>Contact Me</h2>
 <br><br>
 <form method='post' action='contact.php'>
 <table>
 <tr>
 <td class='frmtext'>Name : </td>
 <td class='inputs'><input type='text' name='name'></input></td>
 </tr>
 <tr>
 <td class='frmtext'>Phone: </td>
 <td class='inputs'><input type='text' name='phone'></input></td>
 </tr>
 <tr>
 <td class='frmtext'>Email: </td>
 <td class='inputs'><input type='text' name='email'></input></td>
 </tr>
 <tr height='10'>
 <td></td>
 </tr>
 <tr>
 <td class='frmtext'>Subject: </td>
 <td class='inputs'><select name='subject'>$subjectoptions
 </select></td>
 </tr>
 <tr>
 <td class='frmtext'>Title: </td>
 <td class='inputs'><input type='text' name='title' value='";
 if ($_SERVER['REQUEST_METHOD'] == "POST" && $invalidinput) {
 echo $title;
 }
 echo "'></input></td>
 </tr>
 <tr>
 <td class='frmtext'>Message: <span style='color:red'>*</span></td>
 <td class='inputs'><textarea maxlength='1000' name='message'>";
 if ($_SERVER['REQUEST_METHOD'] == "POST" && $invalidinput) {
 echo $contactingmessage;
 }
 echo"</textarea></td>
 </tr>
 <tr>
 <td></td>
 <td class='inputs'><input type='submit' value='submit' style='width:120px'></input></td>
 </tr>
 </table>
 </form>
 <span style='font-size:90%'><i><b>Phone, Email:
 </b> without any of them we will not be able to contact you back</i>
 </span>
 <hr>
 <b>Or Send An Email
 </div>";
}
echo $pagedown;
?>

• Accountant م: I really don't know how to thank you for your time .1) I downloaded eclipse IDE and will try to leave notepad++... 2) right now I'm reading PSR standards- I never thought there such a thing called coding standards- ...3) I will try to put my css in .css files , and use element classes and Ids in my php to let the .css files select them (I will avoid inline styles) .... 4) I will separate the big HTML block (my template) into a lone php file , and will replace the place holders in it by the pages variables.....
• Accountant م: finally this word " I would suggest rewriting the entire application from scratch" just broke my heart. I CAN'T just select all thousands of lines and hit the delete button :(( I prefer to die rather than destroy that (those line are my life now). I know I started wrong and I really wish that I found you months ago, but what I can do now except try from now on to follow the standards ... I will try to apply the things that I told you now and re edit this contact.php and post a follow up question.
• Accountant م: please If you have time I need your help and your opinion here