Crique my Code! PHP & MySQL

LazyD · Aug 8, 2007

Im doing a job for web development company that is serving as a "test" of my skills, its pretty basic, simply an HTML form that does the following:

Sends an Email to a specified Email Addy
Stores the Email in a database with the date it was submitted
Has a simple backend to choose date ranges and create XLS spreadsheets from that.

Before I submit the code to them, I just want your opinions on my coding, whether I have missed anything, need to add anything, etc. Pretty much, rip my code apart and tell me what you think...

EDIT**** Heres the pastebin URLs for the code - Easier to see there...
Frontend: PHP pastebin - collaborative debugging tool
Backend: PHP pastebin - collaborative debugging tool

Front End:

Code:

<?php
// Config File
require_once("Admin/config.php");

// If form has been submitted
if($_POST['submitform']) {
    mysql_connect($sqlServer, $sqlUsername, $sqlPassword);
    @mysql_select_db($sqlDatabase) or die("Cannot Select Database");

    // Addslashses Function
    function myAddSlashes( $string ) {
        if (get_magic_quotes_gpc()) {
            return ( $string );
            } else {
                return ( addslashes ( $string ) );
            }
        }

        foreach($_POST as $formInput) {
            myAddSlashes($formInput);
        }

        // POST VARS
        $Subject = $_POST['Subject'];
        $Name = $_POST['Name'];
        $Phone = $_POST['Phone'];
        $Email = $_POST['Email'];
        $Message = $_POST['messageText'];

        // Check to see if all forms are filled in
        $requiredVars = $_POST['requiredVars'];
        $requiredExplode = explode(",", $requiredVars);

        $requiredExplode = explode(",",$requiredVars);
        while(list($requiredCheck) = each($requiredExplode)) {
            if(!$$requiredExplode[$requiredCheck]) {
                die($requiredExplode[$requiredCheck]." is empty. Please go back and fill it in.");
            }
        }

        // Todays Date for Submission
        $todaysDate = date('Y-m-d');

        // Error Check Email Address Format
        if(!preg_match('/^[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]{2,4}$/i', $Email)) {
            echo "Email address does not appear to be in the correct format";

            } else { // If its good, continue

                // MySQL Query
                $query = "INSERT INTO `emails` (Id, Subject, Name, Phone, Email, Message, Date) VALUES (NULL, '$Subject, '$Name', '$Phone', '$Email', '$Message', '$todaysDate')";
                mysql_query($query);

                // If Query was Successful show thank you message, else, display error
                if(mysql_affected_rows() > 0) {
                    $emailSubject = $Subject;
                    $emailMessage = $Message;
                    $emailHeaders = 'From: test@lazydcreations.com' . "\r\n" .
                    'Reply-To: test@lazydcreations.com' . "\r\n" .
                    'X-Mailer: PHP/' . phpversion();

                    //mail($recipEmail, $emailSubject, $emailMessage, $emailHeaders);

                    // Thank you message
                    echo $thankYou;

                    } else {

                        //echo "There was a problem with your form submission";
                        echo "There was a problem with your form submission. Please go back and check that all fields are filled in correctly";
                    }

                }
                } else { // If form hasnt been submitted
                    ?>

                    <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
                    <html xmlns="http://www.w3.org/1999/xhtml">
                        <head>
                            <meta http-equiv="content-type" content="text/html;charset=UTF-8">
                            <title>Contact Form - LazyD</title>
                        </head>
                        <body>
                            <form action="#" method="post">
                                <input type="hidden" name="requiredVars" value="Subject,Name,Email,Phone,Message">
                                <table width="370" border="0" cellspacing="0" cellpadding="0">
                                    <tr>
                                        <td width="113"><div class="formText">Subject:</div></td>

                                        <td width="257"><input name="Subject" type="text" size="36" class="customTextField" /></td>
                                    </tr>
                                    <tr>
                                        <td height="6" colspan="2"></td>
                                    </tr>
                                    <tr>
                                        <td><div class="formText">Name:</div></td>
                                        <td><input name="Name" type="text" size="36" id="Name" class="customTextField" /></td>

                                    </tr>
                                    <tr>
                                        <td height="6" colspan="2"></td>
                                    </tr>
                                    <tr>
                                        <td><div class="formText">Phone:</div></td>
                                        <td width="257">
                                        <input name="Phone" type="text" size="36" id="Phone" class="customTextField" />    </td>

                                    </tr>
                                    <tr>
                                        <td height="6" colspan="2"></td>
                                    </tr>
                                    <tr>
                                        <td><div class="formText">Email Address: </div></td>
                                        <td><input  name="Email" type="text" size="36" id="Email" class="customTextField" /></td>
                                    </tr>

                                    <tr>
                                        <td height="31" colspan="2"></td>
                                    </tr>
                                    <tr>
                                        <td><div class="formText" style="text-align:left;">Message:</div></td>
                                    </tr>
                                    <tr>
                                        <td colspan="6"><textarea name="messageText" id="messageText" cols="68" rows="10"></textarea></td>

                                    </tr>
                                    <tr>
                                        <td height="16" colspan="2"></td>
                                    </tr>
                                    <tr>
                                        <td colspan="2"><input name="submitform" type="submit" id="Submit" value="Submit Form" class="customButtonFormat" /></td>
                                    </tr>
                                </table>
                            </form>
                        </body>
                    </html>
                    <?php } ?>

LazyD · Aug 8, 2007

Backend:

Code:

<?php
// Config File
require_once("config.php");

// Get Variables
//$startDate = $_GET['startdate'];
//$endDate = $_GET['enddate'];

if($_POST['submitform']) {
    // Connect to DB
    mysql_connect($sqlServer, $sqlUsername, $sqlPassword);
    @mysql_select_db($sqlDatabase) or die("Cannot Select Database");

    $startDate = $_POST['startDate'];
    $endDate = $_POST['endDate'];

    //mySQL Query
    $query = "SELECT Date,Subject,Name,Phone,Email,Message FROM `emails` WHERE Date BETWEEN '$startDate' AND '$endDate'";
    $result = mysql_query($query);
    $count = mysql_num_rows($result);

    if($count == 0) {
        echo "No Records Found";
        } else {

            $fields = mysql_num_fields($result);

            for ($i = 0; $i < $fields; $i++) {
                $header .= mysql_field_name($result, $i) . "\t";
            }

            while($row = mysql_fetch_row($result)) {
                $line = '';
                foreach($row as $value) {
                    if ((!isset($value)) OR ($value == "")) {
                        $value = "\t";
                        } else {
                            $value = str_replace('"', '""', $value);
                            $value = '"' . $value . '"' . "\t";
                        }
                        $line .= $value;
                    }
                    $data .= trim($line)."\n";
                }
                $data = str_replace("\r","",$data);

                header("Content-type: application/x-msdownload");
                header("Content-Disposition: attachment; filename=report.xls");
                header("Pragma: no-cache");
                header("Expires: 0");
                print "$header\n$data";

            }

            } else {
                ?>

                <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
                <html xmlns="http://www.w3.org/1999/xhtml">
                    <head>
                        <meta http-equiv="content-type" content="text/html;charset=UTF-8">
                        <title>Contact Form Admin Area - LazyD</title>
                        <script type="text/javascript" src="calendarDateInput.js"></script>
                    </head>
                    <body>
                        Select a Start Date and End Date to view submitted items during that time period<br /><br />
                        <form action="" method="post">
                            Report Start Date:<br />
                            <script>DateInput('startDate', true, 'YYYY-MM-DD')</script><br />
                            Report End Date:<br />
                            <script>DateInput('endDate', true, 'YYYY-MM-DD')</script><br />
                            <input type="submit" name="submitform" value="Get Report" />
                        </form>
                    </body>
                </html>

                <?php } ?>

DomainRealty · Aug 8, 2007

// POST VARS
$Subject = $_POST['Subject'];
$Name = $_POST['Name'];
$Phone = $_POST['Phone'];
$Email = $_POST['Email'];
$Message = $_POST['messageText'];

I don't even know what to say here.

PARSE YOUR FUCKING INPUT ONLY AFTER YOU CLEAN IT!

Here...

PHP:

function isclean($var) {
     if (!preg_match("/^[a-zA-Z0-9\-\_\@\.]+$/",$var)) return 0;
     else return 1;
}

if (isclean($_POST['Subject'])) $Subject = $_POST['Subject']; else die();
..
..
..

Jason

wesley · Aug 8, 2007

Here are my thoughts:

- use if (!empty($_POST['submitform'])) instead of if ($_POST['submitform'])

- Do not use addslashes... Use mysql_real_escape_string($var);

- Only add the mysql_real_escape_string/addslashes once you create the SQL query to execute.

- What the heck is this? You get the required fields from a $_POST variable? Someone can easily tamper with this.

- Using die() when validation fails is not very good for usability. Redirect to the form page, make sure all form fields are filled in with the user variablese, and show an error message.

This is also the reason why you don't want to do mysql_real_escape_string beforehand, etc. Also I doubt you can validate some fields if they are already escaped.

- Your email address regex looks simplistic. Here is mine: '/^([a-z0-9\!\#\$\%\&\'\*\+\-\/\=\?\^\_\`\{\|\}\~]+(\.[a-z0-9\!\#\$\%\&\'\*\+\-\/\=\?\^\_\`\{\|\}\~]+)*)@(((([-a-z0-9]*[a-z0-9])?)|(#[0-9]+)|(\[((([01]?[0-9]{0,2})|(2(([0-4][0-9])|(5[0-5]))))\.){3}(([01]?[0-9]{0,2})|(2(([0-4][0-9])|(5[0-5]))))\]))\.)*((([-a-z0-9]*[a-z0-9])?)|(#[0-9]+)|(\[((([01]?[0-9]{0,2})|(2(([0-4][0-9])|(5[0-5]))))\.){3}(([01]?[0-9]{0,2})|(2(([0-4][0-9])|(5[0-5]))))\]))$/i'

Actually, it's from phorum.

- No checks when mysql_query() fails. Should be mysql_query($sql) or log_error($sql, mysql_error()); Same for mysql_connect and mysql_select_db.
function log_error($sql, $error) {
//save to txt
//redirect user
}

- Thank you message is again just a simple line on an entire page? Not good usability.

- Your so called frontend seems to me has backend code, it sends the email. You need to separate your actions from your views.

- $sqlServer, $sqlUsername, etc, would be better defined as constants since they don't change anyway.

Backend
-------

$startDate and $endDate are not even escaped.. SQL injection possible.

wesley · Aug 8, 2007

Oh btw have they asked for a PHP5 or PHP4 example? What you have now is very procedural..

DomainRealty · Aug 8, 2007

My die() call was just to make an example...I used it in lieu of stopTheDamnedApplicationandTellTheUserNotToBotherWithTheBullshit().

Jason

LazyD · Aug 9, 2007

Nice to see some life in this thread, I just assumed I wasnt gonna get dick for a response, but considering how badly my programming skills suck apparently, you guys had alot to say....

I went through your opinions/thoughts and came up with this. Still far from perfect...

PHP pastebin - collaborative debugging tool

There not really that worried about the error or success messages.

wesley · Aug 9, 2007

Better, though you don't need to do or logError on mysql_real_escape_string.

I also don't think it's necessary to do mysql_affected_rows() after an insert since if there is an error it would go to the logError() function and execution would stop.

Also, using @funcname is a bad practice to me, since it can slow down your site. Just disable error reporting or set it to something not warning level.

And you do foreach ($_POST as $key => $val) { } but then you use $_POST[$key] inside that foreach instead of just $val? You already have that right there..

As the script is right now if someone submits a fake page with no POST variables your script will presumably still send an email, you do your validation in foreach but what if there is no $_POST['email'], the validation doesn't get called because it doesn't exist in the array and you cannot loop over it.

And if you output variables always htmlspecialchars() them: "The " . htmlspecialchars($finput) . " .. "

Edit: I don't see it very good because of the weird indentation but do you only do mysql_real_escape_string when it's not email address or phone number? Do it with every variable.

LazyD · Aug 9, 2007

Wesley, thank you...

As the script is right now if someone submits a fake page with no POST variables your script will presumably still send an email, you do your validation in foreach but what if there is no $_POST['email'], the validation doesn't get called because it doesn't exist in the array and you cannot loop over it.

Im not sure I understand that, your saying if someone was using something like cURL, without any POST vars it would still send an email? I guess that makes sense but how do I protect against that?

Ive also made changes based on your suggestions as well as DomainRealty's

wesley · Aug 9, 2007

Don't do the validation in the foreach.

Then afterwards you can either do foreach to apply mysql_real_escape_string or just do it when you are setting up the query:

mysql_query('BLA " . mysql_real_escape_string($_POST['email']) . '" BLA');

LazyD · Aug 9, 2007

So, I uploaded it to there server, the only change I had to make was that they wanted an html file that posted the form - Ok, no big deal, I can handle that...

For some reason, the foreach loop is clearing out the $_POST variables, I dont understand it...

My code is as follows: PHP pastebin - collaborative debugging tool

DISREGARDDDDDD ^^^^^^^^^^^^^^^^^^^^^^

LazyD · Aug 9, 2007

Wesley, since there using an htm file with the form that POSTs the form to my PHP script which processes it, what is the best way to return any message to the user, such as a success message, error message, etc.

If the contact page were in PHP I would have many more options like using SESSIONS to store message information, etc. but at this point im kind of stuck.

Search

Search

Crique my Code! PHP & MySQL

LazyD

$monies = false;

LazyD

$monies = false;

DomainRealty

I'm a Coder

wesley

New member

wesley

New member

DomainRealty

I'm a Coder

LazyD

$monies = false;

wesley

New member

LazyD

$monies = false;

wesley

New member

LazyD

$monies = false;

LazyD

$monies = false;