I have a dynamic form that users can add extra fields too. The group of fields are added and the input name stays the same but gets a incremental number attached to it. So for my insert php file I just check to see if the next value is set and if so it inserts all the required info. But this code doesn't look very 'streamlined' to me. Is there a better way?

This allows you to do the logic for the insertion once, and then use a loop to do all of the fields.

The (pseudo) code would be like:

$products = (isset($_POST['products']) && is_array($_POST['products'])) ? $_POST['products'] : array();
//Note that I've assumed the id column is an int, and the type column is a string.
//In real schema, I would expect type to also be an int, but I want to illustrate proper escaping.
//(There is more on this in a later section)
$query = "INSERT INTO products (id, type) VALUES (%d, '%s')";
foreach ($products as $product) {
//Ensure that $product contains a valid entry, and if it does not, store errors somewhere
if (valid entry) {
$res = mysql_query(sprintf($query, (int) $product['id'], mysql_real_escape_string($product['type'])));
if (!$res) {
//Log an error about the query failing
}
}
}

Put all the validation on the client side you want, but at the end of the day, it's bypassable. In fact, a user doesn't even have to use a browser, much less use JavaScript. A user can send your server anything he wants. After all, at the end of the day, HTTP is a text based protocol.

Also, properly escaping isn't just about security. It's also about correctness.

Not escaping SQL is the same concept as this. This isn't a security issue; it's just wrong. Escaping SQL is the same concept as doing:

$string = "The other day, someone said, \"Hello my name is Fred!\"";

The escaping is necessary so that the SQL server can parse the query correctly in the same way that escaping in PHP is necessary so that the PHP engine can parse the script correctly.

The reason that SQL injection is a security concern is user input. If you don't properly escape, users can change your code.

It's hard to make a brief PHP-injection example, but hopefully the concept is there. An attempt at an example:

eval("\$x = 5 + {$_POST['input']};");

That's all good and fine as long as $_POST['input'] === "5"; or some other kind of innocent value. However, consider if $_POST['input'] === "5; evilFunction()". Suddenly the eval is:

eval("\$x = 5 + 5; evilFunction();");

This is the broad concept of SQL injection. (Various articles on the internet will explain it much better than me though :p.)

(eval should very rarely [arguably, never] actually be used -- this is just to illustrate a point)

Anyway, the solution is very simple. Anything that is going to be a string needs to be run through mysql_real_escape_string before being interpolated into a query. Likewise, anything that is not a string should be handled as the proper type:

I would advise everyone to move away from the old mysql extension. The mysql_* functions are a very thin wrapper around the C API, and though there is not anything particularly wrong with them, PDO is just better.

Different RDBMS can be changed out relatively easily (switching from mysql_* to Postresql is extremely painful. Switching from PDO with MySQL to PDO with Postresql isn't pleasant, but is care is taken to stay as dialect-agnostic as possible, it's reasonably easy)

The second point is the important one. Since the server is putting the paramters into place, it's impossbile for something to be misinterpretted.

An example will be a lot more useful:

//create the connection
$db = new PDO("mysql:host=localhost;dbname=database_name", "username", "password");
//I like for failed queries to throw exceptions.
//If you're not comfortable with exception handling, you would want to omit the following line.
$db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
$products = (isset($_POST['products']) && is_array($_POST['products'])) ? $_POST['products'] : array();
//Create the prepared statement -- in a way this is like having SQL create a temporary function
$insertStmt = $db->prepare("INSERT INTO products (id, type) VALUES (:id, :type)");
foreach ($products as $product) {
//Ensure that $product contains a valid entry, and if it does not, store errors somewhere
if (valid entry) {
//Execute the statement -- this is similar to calling the temporary function
//Note that a new array is created in case $product contains an extra key. For example,
//if there were a $product['extra'] field, then using $insertStmt->execute($product) would
//cause an error since there would be an extra paramater that MySQL would not know what
//to do with.
$insertStmt->execute(array(
'id' => $product['id'],
'type' => $product['type']
));
}
}

Don't assume user input is there

Never assume anything from the client side. What I mean in this particular instance is to never assume a key exists in $_POST, $_GET, $_COOKIE, etcetra.

Just because isset($_POST['productid2']) is true does not mean that the type2 key for $_POST is set. This goes back to the note earlier about people being able to send your client whatever they want.

I could make your script issue all kinds of index undefined and mysql_query() errors if I wanted by making a form like the following:

My approach is to be paranoid on user provided values. Something intersting to note is that $_GET/$_POST/$_COOKIE/$_REQUEST values will always be either a string or an array (unless somewhere in your script you have something like: $_POST['int'] = 5;). This means that if you're not expecting an array, you should make sure you're getting a string.

If you want to be a bit more flexible, you could use is_scalar instead of is_string.

The broad idea is that you never want to access an array by key $k unless you are certain that array_key_exists($k, $arr) is true. When $arr is something user provided, you can never be certain of that unless you explicitly check it.

I think there are some browsers that do not support form arrays, so I'd be careful of this. Otherwise good review +1! I'll see if I can't find that link somewhere and post it back here for verification.
–
mseancoleJun 18 '12 at 16:25

@showerhead It's PHP that makes it an array, not the browser. All that needs to happen on the browser side is that the form data needs to be correctly created. Considering it would be harder for a browser to get it wrong than to get it right, I would imagine that all browsers do it correctly. But I'm not sure if it's specified behavior by the HTML/HTTP standards. (I do know for sure though that IE 7+, Chrome, FF 3+ all handle it fine -- not sure about older versions or Opera/Safari.)
–
CorbinJun 18 '12 at 17:07