Need Some Quick Help..

[trg86]

Thanks again though EG, you have been helpful with several items on my project.

[Christian F.]

There are a number of issues with your code. Most are not too bad, but there are a few that are quite critical. However, all of them tells me that you do not know enough of the basic HTML, CSS, MySQL and PHP, which is causing all of your problems.

 

First and foremost, I'm pretty sure you've been told not to use mysql_result () before. If this isn't the case, consider yourself informed now. What you should be using is mysql_fetch_assoc () (or _array () or _row ()). mysql_result () should only be used when you want to retrieve a single field (or two at the most).

 

Secondly: htmlspecialchars () is not a protection against SQL injections, so you're using it completely wrong. Which leaves you open to SQL injections. For SQL queries you need to use mysql_real_escape_string () instead, or type casting (look it up in the manual, intval () is a good start).

htmlspecialchars () should only be used on content that's being sent to the browser, and then not applied permanently. Both it and mres () should only be used right before the content is added to the output (string), and not used to overwrite the original variable.

Like this, in other words:

// SQL-injection protection.
$query = 'INSERT INTO .... VALUES (%s, %s, %d....';
$query = sprintf ($query, mres ($name), mres ($email), $age....);

// HTML-injection (XSS, etc) protection.
echo '<p>Name: '.htmlspecialchars ($name).'</p>';

 

Your HTML code is a mess too, filled with outdated tags and properties. Remove all the use of center, font, and all of the properties that define how elements look. (bgcolor, align, width, border, etc. Same with the br tags you've tossed in there to create whitespace, use CSS margins instead.

The biggest issue with your HTML code, however, lies inside the loop of your main page: You're closing the page for each iteration.

 

Which brings us back to the MySQL part. As someone already slightly hinted towards, you do not need to use mysql_close () in your scripts, in fact I highly recommend not to use it. PHP closes the connections on its own, whenever the script finishes parsing. You don't have to do anything, and you avoid creating issues like one of the issues you've had in this thread.

You should also specify the fields you want to insert data into, which does 2 things:

1. You won't experience the last issue you had in this thread again, as long as you make sure the number of fields match up with the data.
   
2. You don't need to add empty or default values for fields, which your database will handle automatically. Making your queries shorter.

 

Primarily my advice to you would be to read up on the languages you're using. The manuals are out there to be used, as they tell you exactly what the different commands do and how to use them. I understand that you're learning, but you cannot learn properly without using the documentation to verify your assumptions.

Semantic HTML is also something you should look up on, as it'll help you write proper (and shorter) HTML code. Which again will help reduce complexity, thus increasing readability and reducing the chance of problems.

Last, but not least, you also need to read up on input validation. A subject I haven't touched in this post, but you are also lacking completely in your processor script.

Edited October 29, 2012 by Christian F.

[trg86]

Christian, thank you for your input. I'm not taking offense to the constructive criticism, but I know HTML and CSS like the back of my hand, I've been doing it for years. During development I tend to get into bad habits of using lengthy old font tags and line-breaks just to see how things will look, before I finalize them (in CSS), you will also notice that I do have some permanent css classes already coded in as well.

 

As for the validation of data in the form processor, I beg to differ sir, as I have a separate validation class already programmed, I have just not implemented it into this system yet.

 

But, I will take your advice with the PHP and the SQL, those are the areas I am newer in.

Edited October 29, 2012 by trg86

[Muddy_Funster]

Here's another little bit of advice on the SQL front - always name your fields when doing a query (either a SELECT or an INSERT), and don't INSERT anything into an auto_inc field.

 

So instead of

SELECT * FROM table1

USE

SELECT `field1`, `field2`, `field3`, `field4` FROM table1

 

and

INSERT INTO table1 VALUES('', 'value1', 'value2', 'value3')

Becomes

INSERT INTO table1 (`field2`, `field3`, `field4`) VALUES ('value1', 'value2', 'value3')

[trg86]

Here's another little bit of advice on the SQL front - always name your fields when doing a query (either a SELECT or an INSERT), and don't INSERT anything into an auto_inc field.

 

So instead of

SELECT * FROM table1

USE

SELECT `field1`, `field2`, `field3`, `field4` FROM table1

 

and

INSERT INTO table1 VALUES('', 'value1', 'value2', 'value3')

Becomes

INSERT INTO table1 (`field2`, `field3`, `field4`) VALUES ('value1', 'value2', 'value3')

 

Okay, advice taken.

 

Quick question though, even though my current setup of the code works, what is the benefit/difference of doing it this way?

[Barand]

Benefits

 

INSERTS:

You only need to provide values for those fields you need to (others just take the default values.

 

If you later add a new column to the table the current inserts don't fail.

 

 

SELECTS:

Query performance is proportional to the amount of data returned - only get what you need.

 

If you have table joins, using * always gives duplicated information.

 

 

In both query types it provides information about what the query is actually doing.

 

 

[trg86]

Okay, I can definitely understand that. Which again will limit the problem I had last night, by having a column in the database table that wasn't in use...it through off the whole thing...

[trg86]

Also, I made some changes, referencing Christian's advice, does this look a little better as far as security/injection protection? Compared to my original code, which can be seen in this thread.

 

//
//Gathered Data From The Users Form Entires
//
$name = ($_POST['name']);
$email = ($_POST['email']);
$age = ($_POST['age']);
$gender = ($_POST['gender']);
$location = ($_POST['location']);
$homephone = ($_POST['homephone']);
$referrer = ($_POST['referrer']);

//
//HTML XSS Injection Protection
//
$name = stripslashes($name);
$email = stripslashes($email);
$age = stripslashes($age);
$gender = stripslashes($gender);
$location = stripslashes($location);
$homephone = stripslashes($homephone);
$referrer = stripslashes($referrer);

//
//SQL Injection Protection
//
$name = mysql_real_escape_string($name);
$email = mysql_real_escape_string($email);
$age = mysql_real_escape_string($age);
$gender = mysql_real_escape_string($gender);
$location = mysql_real_escape_string($location);
$homephone = mysql_real_escape_string($homephone);
$referrer = mysql_real_escape_string($referrer);

[trg86]

Does this look a little better?

 

$query = "INSERT INTO `leads` (`name`,`email`,`age`,`gender`,`location`,`homephone`,`referrer`) VALUES ('', '$name','$email','$age','$gender','$location','$homephone','$referrer','0','')"; //Insert Form Values To Database

Edited October 29, 2012 by trg86

[MDCode]

Also, I made some changes, referencing Christian's advice, does this look a little better as far as security/injection protection? Compared to my original code, which can be seen in this thread.

 

//
//Gathered Data From The Users Form Entires
//
$name = ($_POST['name']);
$email = ($_POST['email']);
$age = ($_POST['age']);
$gender = ($_POST['gender']);
$location = ($_POST['location']);
$homephone = ($_POST['homephone']);
$referrer = ($_POST['referrer']);

//
//HTML XSS Injection Protection
//
$name = stripslashes($name);
$email = stripslashes($email);
$age = stripslashes($age);
$gender = stripslashes($gender);
$location = stripslashes($location);
$homephone = stripslashes($homephone);
$referrer = stripslashes($referrer);

//
//SQL Injection Protection
//
$name = mysql_real_escape_string($name);
$email = mysql_real_escape_string($email);
$age = mysql_real_escape_string($age);
$gender = mysql_real_escape_string($gender);
$location = mysql_real_escape_string($location);
$homephone = mysql_real_escape_string($homephone);
$referrer = mysql_real_escape_string($referrer);

 

You don't need the parentheses around $_POST['variable'] Also, in case you are ever going to display the information anywhere you want to add htmlentities to your sanitizing

[trg86]

You don't need the parentheses around $_POST['variable'] Also, in case you are ever going to display the information anywhere you want to add htmlentities to your sanitizing

 

The data is displayed on my main_interface.php page.

 

I've updated my code, feel free to review and comment on any better ways for me to do something, thanks!

 

<?php
//***************************************
//Database Login Information & Connection
//***************************************
$username="REMOVED_FOR_THIS_POST"; //Database username
$password="REMOVED_FOR_THIS_POST"; //Database password
$database="REMOVED_FOR_THIS_POST"; //Database name
mysql_connect("REMOVED_FOR_THIS_POST",$username,$password); //Connection to Database
@mysql_select_db($database) or die( "Error, the requested database was not found!"); //Database Selection

//*****************************************
//Gathered Data From The Users Form Entires
//*****************************************
$name = htmlentities $_POST['name'];
$email = htmlentities $_POST['email'];
$age = htmlentities $_POST['age'];
$gender = htmlentities $_POST['gender'];
$location = htmlentities $_POST['location'];
$homephone = htmlentities $_POST['homephone'];
$referrer = htmlentities $_POST['referrer'];

//*************************
//HTML Injection Protection
//*************************
$name = stripslashes($name);
$email = stripslashes($email);
$age = stripslashes($age);
$gender = stripslashes($gender);
$location = stripslashes($location);
$homephone = stripslashes($homephone);
$referrer = stripslashes($referrer);

//************************
//SQL Injection Protection
//************************
$name = mysql_real_escape_string($name);
$email = mysql_real_escape_string($email);
$age = mysql_real_escape_string($age);
$gender = mysql_real_escape_string($gender);
$location = mysql_real_escape_string($location);
$homephone = mysql_real_escape_string($homephone);
$referrer = mysql_real_escape_string($referrer);

//*******************************
//Insertion Of Data Into Database
//*******************************
$query = "INSERT INTO `leads` VALUES ('', '$name','$email','$age','$gender','$location','$homephone','$referrer','0','')";
mysql_query($query);
echo mysql_error();
?>

Edited October 29, 2012 by trg86

[Christian F.]

//*************************
//HTML Injection Protection
//*************************
$name = stripslashes($name);
$email = stripslashes($email);
$age = stripslashes($age);
$gender = stripslashes($gender);
$location = stripslashes($location);
$homephone = stripslashes($homephone);
$referrer = stripslashes($referrer);

This isn't HTML injection prevention, or any kind of injection prevention. If anything it can lead to more attack vectors, unless you have magic_quotes_gpc turned on. Using it today, and especially in that manner, is a fallacy; A remnant from the old PHP 4.X days.

 

When it comes to htmlentities () it should only be used right before you send the data to the browser, just like mysql_real_escape_string () for SQL queries. As I stated in my previous post, and I even gave examples. Not to mention the fact that you need to treat strings and numbers differently.

The use of MRES () can be acceptable, if (and only if) you're not going to use the variables for anything else later on. However, the golden rule of output escaping is that you should never overwrite the original variables (as mentioned above).

 

So, the addition of MRES(), addition of some comments, and the removal of mysql_close () are all good things. Besides that, almost every single point in my previous posts stands.

As for your HTML and CSS knowledge, as well as what you have or don't have in the ways of validation; I'm afraid that I can only comment upon what you've posted, and my criticism stands quite firm based upon that. If you want us to comment upon what you have, then you have to show it to us.

 

I must confess that I'm a bit confused as to why you want to make all that extra work for yourself, by using the work flow you do when creating the HTML code, but.. If it works for you, then all the more power to you. Just be prepared to get criticism on it, if you post messy code again.

[trg86]

Christian, thank you for your comments again, it is appreciated. As you can see, this is a better example of how clean my code normally is...this project has some sloppy points, but all will be cleaned up..

 

As for:

 

//*************************
//HTML Injection Protection
//*************************
$name = stripslashes($name);
$email = stripslashes($email);
$age = stripslashes($age);
$gender = stripslashes($gender);
$location = stripslashes($location);
$homephone = stripslashes($homephone);
$referrer = stripslashes($referrer);

 

you say it is pretty much incorrect, and I am definitely trying to get this 100% protected, what would you recommend for this?

 

 

Additionally, referencing:

 

//*****************************************
//Gathered Data From The Users Form Entires
//*****************************************
$name = htmlentities $_POST['name'];
$email = htmlentities $_POST['email'];
$age = htmlentities $_POST['age'];
$gender = htmlentities $_POST['gender'];
$location = htmlentities $_POST['location'];
$homephone = htmlentities $_POST['homephone'];
$referrer = htmlentities $_POST['referrer'];

 

is this technically incorrect as well?

[Muddy_Funster]

Just a note here, mysql_real_escape_string() does nothing to integer values, such as an age. The way your are using your htmlentities() would be better suited to using sanitizer and validator filters - I persoanly always use FILTER_VALIDATE_EMAIL() for all email fields. I'd also never apply htmlentities() to an email field.

[Christian F.]

All stripslashes () does, is to unquote a quoted string. Or to put it in another way, from the PHP manual:

Returns a string with backslashes stripped off. (\' becomes ' and so on.) Double backslashes (\\) are made into a single backslash (\).

 

Backslashes does nothing in a HTML documents, but they are an important meta-character in PHP strings. So unless you have magic quotes on, you do not want to use this function.

 

As I've mentioned 2 times already, HTML escaping should only be done right before you send the output to the HTML output. Not before you save it to the database, or anywhere else. Only right before you echo or concatenate the contents of the variables to the HTML, and without overwriting the original values (preferably).

 

To give you an example of what I'm trying to say, let's use the contact form code I use in my own framework.

As you notice I've used some custom functions in there, most of which should be easy to guess what they do. The two more difficult ones, I suspect, would be the validate () function and the template engine.

validate () returns the unmodified user input, from the POST array, but only if it matches the validation expressions (first parameter). If not, then it uses the template engine and htmlspecialchars () to prefill the input elements of the form, and builds upon the error message.

The template engine uses the $Template->_FORM_FIELD_{name} properties to prefill the values of the input fields, so that the user doesn't have to write everything anew.

 

Notice where I've used htmlspecialchars () in that code, and where the result is sent.

 

If I had a SQL query in there, instead of the Send_Email () call, this is approximately how it'd look:

$Query = "INSERT INTO $Table ($Fields) VALUES (%s, %s, %s)";
$DB->Query (sprintf ($Query, Quote_Smart ($Name), Quote_Smart ($Email), Quote_Smart ($Message));

Where Quote_Smart () is a wrapper around mysql_real_escape_string (), taken from the PHP manual and slightly modified.

 

I think that you might perhaps benefit from reading up a bit more in the PHP manual, to really familiarize yourself with what the different functions does, and (more importantly) why they do it. Once you've done that, you can better reflect upon their use and whether or not what they do is what you want in that specific case.

Programming is kind of like a game of chess, as the more steps you plan ahead the better your game (code) will be.

 

Muddy_Funster: If anything the e-mail address is perhaps one of the most important things to escape, as (if you go by the RFC) it is perfectly legal to put HTML tags in an e-mail address!

Never trust the user, and don't underestimate the dark side: Escape everything that you don't have 100% control over yourself.

Edited October 30, 2012 by Christian F.

[Muddy_Funster]

Actualy...I keep it simple with the sanitize and validate functions I linked to previously. I'm pretty sure that the validate uses the same match as the sanitize (can't evidence that though). I should have stipulated that I use both, but figured it would be taken as granted.

FILTER_SANITIZE_EMAIL "email" Remove all characters except letters, digits and !#$%&'*+-/=?^_`{|}~@.[].

 

So that's not going to play nice with html tags, yeah it allows the ampersand, but strips semi-colons, so that's not a big issue either.

 

oh....and I trust no one