PHP Developers Network

A community of PHP developers offering assistance, advice, discussion, and friendship.
It is currently Tue Dec 01, 2020 6:56 am

All times are UTC - 5 hours

Post new topic Reply to topic  [ 4 posts ] 
Author Message
PostPosted: Mon Sep 12, 2011 11:42 pm 
Forum Newbie

Joined: Wed Jan 19, 2011 11:04 pm
Posts: 6

I have some good understanding of PHP mysql sanitization but really want to know if what im currently doing is the best possible security.

Data is coming from a form then passed via ajax to a php file for uploading to mysql. Currently zero sanitization.

Syntax: [ Download ] [ Hide ]

      $data1 = $_GET["data1"];

      $data = array();
      $data = explode("|", $data1);
        foreach ($data as $value) {            
            $data2 = explode(":", $value);
            $data[$data2[0]] = $data2[1];        
        $query = "INSERT INTO transactions SET
        mysql_query($query) or die (mysql_error());

What would you do?

PostPosted: Tue Sep 13, 2011 3:00 am 
Forum Regular
User avatar

Joined: Tue Sep 28, 2010 11:41 am
Posts: 984
Location: Columbus, Ohio
Here is an example of something that will work:
Syntax: [ Download ] [ Hide ]

        // Make sure the passed information looks properly formatted
        if (preg_match('/^([a-z]+:[^|]+\|)+$/i',$_GET['data1'].'|')) {
                $data = array_fill_keys(array('Firstname','Lastname','Address','Suburb','City','Phone','Email','Amount'),'');
                $pieces = explode('|', $_GET['data1']);
                foreach($pieces as $pair) {
                        list($key,$val) = explode(':',$pair,2);
                        if (array_key_exists($data,$key)) {
                                $data[$key] = $val;

                foreach($data as $key=>$val) {
                        if ($val=='') {
                                die ('[ERR:'.__LINE__.'] Missing value for '.$key);

                $query = 'INSET INTO `transactions` SET ';
                foreach($data as $key=>$val) {
                        $query .= '`'.strtolower($key).'` = "'. mysql_real_escape_string($val).'",';
                $query .= '`date`=NOW()';

                mysql_query($query) or die ('[ERR:'.__LINE__.'] ' . mysql_error());
        else {
                die ('[ERR:'.__LINE__.'] Invalid parameters passed');


Here it is spelled out...

1. Make sure that we are get a properly passed string, of key:value|key:value|key:value. Note that the regular expression looks for a | after each set, so for the check, I add an extra one to the end to match.

2. Create an array $data that contain all the fields, set to an empty string to start with.

3. Break the string to groups like you did, then loop through those groups doing the following:

3a. Makes sure you only explode to two elements, so that you can actually have a colon in the data values

3b. Only if the key already exists in $data assign it, otherwise it gets skipped

4. This part was optional, however if all values are supposed to be required, loop through $data to make sure all are set to something (otherwise error with message)

5. Build up the query. Normally I would do INSERT INTO `table` (`field1`,`field2`,`field3`) VALUES ("value1","value2","value3) To be honest, I have been doing SQL so long, I didn't know they they adapted to allow the INSERT INTO `table` SET `field1`="value1", `field2`="value2", `field3`="value3". Learn something new every day even after 11 years programming php/mysql

5a. Since all the fields are in $data, we just loop through it to build up pairs. Note that each loop places a comma at the end of the string, see next step. Note also that I am properly escaping the value so that SQL injection doesn't happen.

5b. I add the DATE field here, instead of the beginning. I do this since the query at this point is already ending in a comma, ready for another set. Otherwise I would have had to trim the ending comma off.

6. execute the query, if failed, error out.

7. The else for the initial preg_match, if it hits here, the data wasn't passed correctly, so give the correct error.

Hope this helps, and again, thank you for opening my eyes to the alternative INSERT method. It makes the code more readable. Using the way I always did it before, I would have done:

Syntax: [ Download ] [ Hide ]
                $query1 = 'INSET INTO `transactions` (';
                $query2 = ') VALUES (';
                foreach($data as $key=>$val) {
                        $query1 .= '`'.strtolower($key).'`,';
                        $query2 .= '"'. mysql_real_escape_string($val).'",';
                $query = $query1 . '`date`' . $query2 . 'NOW() )';

PostPosted: Tue Sep 13, 2011 4:13 am 
DevNet Resident
User avatar

Joined: Sun Sep 03, 2006 5:19 am
Posts: 1579
Location: Sofia, Bulgaria
1. Use trigger_error instead of die(), it will do the right thing both on your developer setup and on the production server
2. Check if $_GET['data1'] exists and is a string

There are things that can go wrong depending on the db setup here, if one of the strings is larger than the field size in the DB, but they would hardly make a difference security-wise with this table. It can be dangerous if it happens on the login table, though so you must be aware of that. Otherwise it looks ok.

PostPosted: Tue Sep 13, 2011 7:05 pm 
Forum Newbie

Joined: Wed Jan 19, 2011 11:04 pm
Posts: 6
WOw thats fantastic effort twinedev.

Cheers for that and yes ill be implementing that!

ps: what exactly is prepared statements?

Display posts from previous:  Sort by  
Post new topic Reply to topic  [ 4 posts ] 

All times are UTC - 5 hours

Who is online

Users browsing this forum: No registered users and 4 guests

You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot post attachments in this forum

Jump to:  
Powered by phpBB® Forum Software © phpBB Group