Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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?

mysql_select_db("inventory", $con);

if( !isset($_POST['productid2']) ) {
$sql="INSERT INTO shipped (id, type, client, product, color, quantity)
VALUES ('$_POST[productid]','$_POST[type]','$_POST[client]','$_POST[product]','$_POST[color]','$_POST[quantity]')";
}

if( isset($_POST['productid2']) ) {
$sql="INSERT INTO shipped (id, type, client, product, color, quantity)
VALUES ('$_POST[productid]','$_POST[type]','$_POST[client]','$_POST[product]','$_POST[color]','$_POST[quantity]'),
('$_POST[productid2]','$_POST[type2]','$_POST[client2]','$_POST[product2]','$_POST[color2]','$_POST[quantity2]')";
}

if( isset($_POST['productid3']) ) {
$sql="INSERT INTO shipped (id, type, client, product, color, quantity)
VALUES ('$_POST[productid]','$_POST[type]','$_POST[client]','$_POST[product]','$_POST[color]','$_POST[quantity]'),
('$_POST[productid2]','$_POST[type2]','$_POST[client2]','$_POST[product2]','$_POST[color2]','$_POST[quantity2]'),
('$_POST[productid3]','$_POST[type3]','$_POST[client3]','$_POST[product3]','$_POST[color3]','$_POST[quantity3]')";
}

    // And I have like this 5 more times, the code gets longer each time!
share|improve this question
3  
Before you think about optimisation it would be a good idea to read up on SQL injection. Your code is wide open to attack at the moment –  Clive Jun 17 '12 at 2:05
    
@Clive the form fields are all select type and are validated before being posted. –  mcflause Jun 17 '12 at 2:16
    
That won't stop an attacker from posting to the form with harmful values –  Clive Jun 17 '12 at 2:24
    
Not using the mysql extension at all would also be a good idea. –  ThiefMaster Jun 17 '12 at 9:20

1 Answer 1

up vote 7 down vote accepted

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.

share|improve this answer
1  
Quite possibly the best answer I have ever received. I will be bookmarking this for future reading! Thanks –  mcflause Jun 17 '12 at 17:07
    
@mcflause Glad I could help :). –  Corbin Jun 17 '12 at 22:54
    
+1, exhaustive answer. –  haylem Jun 18 '12 at 5:07
    
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. –  mseancole Jun 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.) –  Corbin Jun 18 '12 at 17:07

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.