Array based approach
Instead of appending a number to the field, use an array based approach:
<form action="" method="POST">
<b>Product 1</b><br
<label>Product Id <input type="text" name="products[0][id]"></label>
<br>
<label>Product Type <input type="text" name="products[0][type]"></label>
<br>
<b>Product 2</b><br>
<label>Product Id <input type="text" name="products[1][id]"></label>
<br>
<label>Product Type <input type="text" name="products[1][type]"></label>
</form>
Now if you were to submit this to PHP and do print_r($_POST['products']);, you would get something like the following:
array(
0 => array(
'id' => '...',
'type' => '...',
),
1 => array(
'id' => '...',
'type' => '...',
)
)
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
}
}
}
SQL Injection
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.
$string = "The other day, someone said, "Hello my name is Fred!""; //Syntax error!
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:
$query = "INSERT INTO tbl (stringField, intField) VALUES ('%s', '%d')";
$qry = sprintf($query,
mysql_real_escape_string($someString), //make sure the string field is properly escaped
(int) $someInt //make sure the int field is an int
);
PDO
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.
In particular, PDO:
- Offers a cleaner API
- Offers better error handling (the ability to have failed queries throw exceptions is wonderful)
- Handles transactions SQL-dialect agnostically (for the most part)
- 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)
- Prepared statements
Prepared statements deserve their own sublist:
- 100% safe from SQL injection (if used correctly, of course)
- Better performance for batched queries
- Various other minor things
Prepared statements work vaguely like:
* Create a parameterized statement
* Let the SQL server handle putting the variables into place
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:
<form action="http://yoursite.tld/somepage.php" method="POST">
<input type="text" name="productid2" value="let there be errors!">
<input type="submit">
</form>
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.
$type = (isset($_POST['type']) && is_string($_POST['type'])) ? $_POST['type'] : null;
if ($type === null) {
echo 'Please provide a type';
} else {
//do some other validations on $type
}
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.
mysqlextension at all would also be a good idea. \$\endgroup\$