Help required to code an UPDATE query within the results of a SELECT query

[excelmaster]

Hello all,

 

Based on the suggestion of you wonderful folks here, I went away for a few days (to learn about PDO and Prepared Statements) in order to replace the MySQLi commands in my code. That's gone pretty well thus far...with me having learnt and successfully replaced most of my "bad" code with elegant, SQL-Injection-proof code (or so I hope).

 

The one-and-only problem I'm having (for now at least) is that I'm having trouble understanding how to execute an UPDATE query within the resultset of a SELECT query (using PDO and prepared statements, of course).

 

Let me explain (my scenario), and since a picture speaks a thousand words I've also inlcuded a screenshot to show you guys my setup:

 

In my table I have two columns (which are essentially flags i.e. Y/N), one for "items alreay purchased" and the other for "items to be purchased later". The first flag, if/when set ON (Y) will highlight row(s) in red...and the second flag will highlight row(s) in blue (when set ON).

 

I initially had four buttons, two each for setting the flags/columns to "Y", and another two to reverse the columns/flags to "N". That was when I had my delete functionality as a separate operation on a separate tab/list item, and that was fine.

 

Now that I've realized I can include both operations (update and delete) on just the one tab, I've also figured it would be better to pare down those four buttons (into just two), and set them up as a toggle feature i.e. if the value is currently "Y" then the button will set it to "N", and vice versa.

 

So, looking at my attached picture, if a person selects (using the checkboxes) the first four rows and clicks the first button (labeled "Toggle selected items as Purchased/Not Purchased") then the following must happen:

 

1. The purchased_flag for rows # 2 and 4 must be switched OFF (set to N)...so they will no longer be highlighted in red.

2. The purchased_flag for row # 3 must be switched ON (set to Y)...so that row will now be highlighted in red.

3. Nothing must be done to rows # 1 and 5 since: a) row 5 was not selected/checked to begin with, and b) row # 1 has its purchase_later_flag set ON (to Y), so it must be skipped over.

 

Looking at my code below, I'm guessing (and here's where I need the help) that there's something wrong in the code within the section that says "/*** loop through the results/collection of checked items ***/". I've probably made it more complex than it should be, and that's due to the fact that I have no idea what I'm doing (or rather, how I should be doing it), and this has driven me insane for the last 2 days...which prompted me to "throw in the towel" and seek the help of you very helpful and intellegent folks. BTW, I am a newbie at this, so if I could be provided the exact code, that would be most wonderful, and much highly appreciated.

 

Thanks to you folks, I'm feeling real good (with a great sense of achievement) after having come here and got the great advice to learn PDO and prepared statements.

 

Just this one nasty little hurdle is stopping me from getting to "end-of-job" on my very first WebApp. BTW, sorry about the long post...this is the best/only way I could clearly explaing my situation.

 

Cheers guys!

case "update-delete":
	if(isset($_POST['highlight-purchased'])) 
		{
			// ****** Setup customized query to obtain only items that are checked ******
			$sql = "SELECT * FROM shoplist WHERE";
					for($i=0; $i < count($_POST['checkboxes']); $i++)
						{
							$sql=$sql . " idnumber=" . $_POST['checkboxes'][$i] . " or";
						}
					$sql= rtrim($sql, "or");
											
			$statement = $conn->prepare($sql);
			$statement->execute();
																						
			// *** fetch results for all checked items (1st query) *** //
			$result = $statement->fetchAll();

			$statement->closeCursor();
											
			// Setup query that will change the purchased flag to "N", if it's currently set to "Y"
			$sqlSetToN = "UPDATE shoplist SET purchased = 'N' WHERE purchased = 'Y'";
											
			// Setup query that will change the purchased flag to "Y", if it's currently set to "N", "", or NULL
			$sqlSetToY = "UPDATE shoplist SET purchased = 'Y' WHERE purchased = 'N' OR purchased = '' OR purchased IS NULL";

			$statementSetToN = $conn->prepare($sqlSetToN);
			$statementSetToY = $conn->prepare($sqlSetToY);
											
			/*** loop through the results/collection of checked items ***/
			foreach($result as $row)
				{
					if ($row["purchased"] != "Y")
						{
							// *** fetch one row at a time pertaining to the 2nd query *** //
							$resultSetToY = $statementSetToY->fetch();
															
							foreach($resultSetToY as $row)
								{															
									$statementSetToY->execute();																
								}
						}
					else
						{
							// *** fetch one row at a time pertaining to the 2nd query *** //
							$resultSetToN = $statementSetToN->fetch();
															
							foreach($resultSetToN as $row)
								{															
									$statementSetToN->execute();																
								}
						}
				}
				break;
		}

 

 

[excelmaster]

Forgot to mention the problem I'm having with my current code!

 

So right now, the correct number of rows are being returned for the first query i.e. the one that extracts the selected/checked rows...but when I click the first/red-text button, nothing happens to any of the rows i.e. none of the flags/column values are being changed

[Strider64]

If you know the items to be unchecked or checked then why don't you just update them? I'm assuming that each item has an unique id, thus you could just update the selected id rows.

 

Interestingly I have built something similar and all I do is :

$id = $_POST['id'];
if ( isset($_POST['checked'])) {
	
	$checked = $_POST['checked'];
	
	$query = 'UPDATE thelist SET checked=:checked, dateAdded=NOW() WHERE id=:id';
	
	$stmt = $pdo->prepare($query);
	
	$result = $stmt->execute(array(':checked' => $checked, ':id' => $id));
	
}

I do this real time using AJAX, but this could be modified to something that you are doing. I am guessing that it will entail using array and a loop.

 

To see a working example of what I'm talking about : http://www.thegrocerylist.us/

 

I would also break down what you do in sections (methods/functions), for example add function, update function(s) and delete functions. You'll be surprise that you will have about the same amount of code ( maybe even less).

Edited May 13, 2014 by Strider64

[Ch0cu3r]

You have way over complicated this. Everything can be handled by just a single update query. 

 

So upon the clicking the red button you are setting the value for the  purchased  column in the table to Y   where the records purchase_later_flag flag is not N and the record id matches the id contained in the checkbox value. The operation is then reversed for the blue button (Setting purchased to N and purchaed_later_flag to Y).

 

Example code

if(isset($_POST['highlight-purchased'])) 
{
    // ****** toggle purchased flag ******
    $sql = "UPDATE shoplist SET purchased = 'Y' WHERE purchase_later_flag != 'N' AND idnumber IN(' .implode(',', array_map('intval', $_POST['checkboxes'])). ')';
    $conn->query($sql);
}

// reveres the operation for purchase later
if(isset($_POST['highlight-purchase_later']))
{
    // ****** toggle purchased later flag ******
    $sql = "UPDATE shoplist SET purchased_later_flag = 'Y' WHERE purchased != 'Y' AND idnumber IN(' .implode(',', array_map('intval', $_POST['checkboxes'])). ')';
    $conn->query($sql);
​}

Also you are using prepare statements completely incorrectly, the values used in your prepared queries should be replaced with placeholders. Whereby then you bind the values to the query when you goto execute it. The database will then replace the placeholders with the escaped values. Currently your code is still open to SQL injection.

[Psycho]

EDIT: Ch0cu3r beat me to it and covered most of what I state below. But, I think he missed the point that the values need to be toggled. If I am reading the above query correctly it only changes the value from 'Y' to 'N'. But, the OP also needs the 'N' values changed to 'Y'.

 

 

 I've probably made it more complex than it should be, . . .

 

I would agree with that statement. I think the core of the problem is how the UI workflow is built. As a user I would be thoroughly confused on how to use that page. I think you are approaching the problem from a "programatic" perspective. You should think about the problem that the user is trying to solve and the workflow that would make sense to them. But, I don't know enough about your application or the users in order to provide any suggestions, so we'll deal with what you have.

 

Regarding the two columns for already purchased and items to purchase later you could combine those into a single column with three possible values to simplify things. Or, at the very least, it would be better to change the current possible values to 0 and 1, because those can be interpreted as True/False.

 

I'm not going to comment on your current code, but rather provide what I think is the solution. First of all, you only want to run the query against the records that the user has selected (we'll address the first record later).The checkboxes should be named as an array with the value being the ID of the records such as this (Don't create your checkboxes with a name like 'ckeckboxes'! That's just stupid (sorry to be blunt). Give all variables, fields, etc. names that give you an idea of what they contain. In this case, the checkboxes should contain the ID of the products, so 'prodIDs' would make more sense):

<input type="checkbox" name="prodIDs[]" value="5" />

So, to ensure the query is only run against the selected products you would use an IN condition as part of the WHERE clause. Of course you would do some pre-processing on the passed IDs.

WHERE prod_id IN (1, 2, 3)

Second, since you don't want to run this for records where the "items to be purchased later" is true, you would add an exclusion to the where clause

WHERE prod_id IN (1, 2, 3)
  AND NOT purchase_later_flag -- I.e. value is false

Lastly, for the records that do match the where clause you want to toggle the current value. You can do that simply set the value based upon the current value. If the values are 0/1 this is VERY easy

SET purchase_now_flag = NOT purchase_now_flag

Everything put together:

//Force all passed IDs to be integers and filter out 0/empty values
$prodIDsAry = array_filter(array_map('intval', $_POST['prodIDs']));
if(count($prodIDsAry))
{
    //No valid IDs passed - add error condition
}
else
{
    //Create comma separated list of selected product IDs
    $prodIDList = implode(', ', $prodIDsAry);
    //Create query to update selected records
    $query = "UPDATE shoplist
              SET purchase_now_flag = NOT purchase_now_flag
              WHERE prod_id IN ({$prodIDList})
                AND NOT purchase_later_flag";
}

If you don't change the columns to use 0/1 and instead continue to use Y/N (which I would advise against) this query should work

 

  $query = "UPDATE shoplist
              SET purchase_now_flag = IF(purchase_now_flag='Y', 'N', 'Y')
              WHERE prod_id IN (1, 2, 3, 4)
                AND NOT purchase_later_flag <> 'Y'";

Edited May 13, 2014 by Psycho

[excelmaster]

@Ch0cu3r: Actually, the two flags (and therefore the two buttons) are independent of each other (with each serving a different/specific purpose).

 

The "already_purchased" flag is used during the time of shopping (on the weekend) right after an item is picked up (in the grocery store)...so we can easily see which items have been bought, and which are yet to be bought (red = bought, black = yet to be bought).

 

Now, some items we may already have at home (so we don't need to purchase them on the weekend) but will need to buy a few days after the weekend, and therefore we'd like to see those items highlighted in a different color (blue in this case)

 

So when we first head to the grocery store the list will only show rows in black and/or blue...and only the black items will be purchased that day (Saturday).

 

As we go along, picking up the items on the list, we'll click the "red-text button" to indicate that that items has been purchased, and the row will be highlighted in red text. So typically once the shopping is complete, all those black rows will show up as red (unless an item was out of stock).

 

The above being said, the code I provided was specific only to the red-text button i.e. "already_purchased" logic/functionality. I have an almost identical piece of code/logic that deals with the blue-text button ("to be purchased later" items), the only difference being the column-name. However, the logic for the red-text button must/does check to see if a selected row is highlighted in blue i.e. it's a "purchase_later" item then it must be skipped over (since we are clicking the red button, and only want to toggle between red and black or alread_purchased and not_purchased).

 

So, I understand the first "$sql...." statement that you provided, and I kow how to fix it as per my requirement...but I'm not sure about the second "$sql..." statement! As @psycho correctly pointed out, a reverse logic must be developed for the very same button, in order to toggle the value when clicked a second time.

 

Also, where exactly do I place this code?

 

>> Currently your code is still open to SQL injection.

Can you tell me which part(s) of my code is still open to SQL injection?

 

@Psycho: thanks for provide such a detailed explanation. I'm gonna need some time to first digest all of that, then understand it, and finally put it to use. I will most certainly have more questions and/or clarifications...but it may come later today or possibly even tomorrow.

[Psycho]

@Psycho: thanks for provide such a detailed explanation. I'm gonna need some time to first digest all of that, then understand it, and finally put it to use. I will most certainly have more questions and/or clarifications...but it may come later today or possibly even tomorrow.

 

You only need to determine which button the user selected. Then take the passed IDs from the checkboxes and run the query. The 'case' in you example code was for 'update-delete' which didn't seem logical for the specific process you were asking about. But, I would think all/most of your processes would be using the selected check boxes - so I would get those values before the switch statement.

 

I don't know what logic you have to determine the value used for the switch(), but here's a quick example of how you can do everythign pretty simply. I made up my own switch values for illustrative purposes

 

<?php
 
//Force all passed IDs to be integers and filter out 0/empty values
$prodIDsAry = array_filter(array_map('intval', $_POST['prodIDs']));
//Format list into a comma separated string
$prodIDList = implode(', ', $prodIDsAry);
 
//Use your logic to determine the value to be used for the switch statement
$switchValue = '??????';
 
//Detemine the process to run
switch($switchValue)
{
    case "toggle_purchased":
        //Toggle purchased flag for selected records where the
        //purchase_later_flag is NOT 'Y'
        if(!empty($prodIDList))
        {
            $query = "UPDATE shoplist
                      SET purchased_flag = IF(purchased_flag='Y', 'N', 'Y')
                      WHERE prod_id IN ({$prodIDsAry})
                        AND NOT purchase_later_flag <> 'Y'";
            $statement = $conn->exec($query);
        }
        break;
 
      case "toggle_purchase_later":
        //Toggle purchased flag for selected records where the
        //purchased_flag is NOT 'Y'
        if(!empty($prodIDList))
        {
            $query = "UPDATE shoplist
                      SET purchase_later_flag = IF(purchase_later_flag='Y', 'N', 'Y')
                      WHERE prod_id IN ({$prodIDsAry})
                        AND NOT purchase_later_flag <> 'Y'";
            $statement = $conn->exec($query);
        }
        break;
 
      case "delete_selected":
        //Delete only the selected records
        if(!empty($prodIDList))
        {
            $query = "DELETE FROM shoplist
                      WHERE prod_id IN ({$prodIDsAry})";
            $statement = $conn->exec($query);
        }
        break;
 
      case "delete_all":
        //Delete all the records
        $query = "DELETE FROM shoplist";
        $statement = $conn->exec($query);
        break;
}
 
?>

[Ch0cu3r]

 

>> Currently your code is still open to SQL injection.

Can you tell me which part(s) of my code is still open to SQL injection?

Yes because on this part of the code you are using raw $_POST data in your query

   // ****** Setup customized query to obtain only items that are checked ******
   $sql = "SELECT * FROM shoplist WHERE";
   for($i=0; $i < count($_POST['checkboxes']); $i++)
   {
      $sql=$sql . " idnumber=" . $_POST['checkboxes'][$i] . " or";
   }
   $sql= rtrim($sql, "or");
   $statement = $conn->prepare($sql);
   $statement->execute();

...;

And as I stated you are using a prepared queries incorrectly (read my comment at the bottom of post #4)

[excelmaster]

@Psycho: Thanks once again (a million times over) for providing what (at first glance) seems exactly "what the doctor ordered"! That is indeed some amazing code that you've provided me with.

 

The only processes that use the checkboxes are the update and delete processes...and that's why I decided to merge them into one. That being said, I'm sure my code for the "search" function is also overly complicated and unnecessarily long. Should I include that code as part of this post to have it looked over and potentially changed?

 

>> I don't know what logic you have to determine the value used for the switch().....

The answer to this question of yours is provided in the code block below: So the question I have is this: is that a safe way to do it...or is it still open to SQL injection? If it is indeed open to SQL injection then how do I fix/change it?

 

 

if(isset($_POST["action"])){
     switch($_POST["action"]){
           case "insert":
                  ....
                  ....
           case "update-delete":
                  ....
                  ....
           case "search":
                  ....
                  ....
       }
  }

 

@Ch0cu3r: I certainly wasn't aware that checkboxes could be used to as a mechanism to perform SQL injection. Likewise, I didn't think that a dropdown listbox (allowing only fixed/coded values) could be used as a means to perform SQL injection. I was under the impression that only free-format textboxes could be used for malicious purposes.

 

>> And as I stated you are using a prepared queries incorrectly (read my comment at the bottom of post #4)

I certainly did read your comment at the bottom of post #4, and that's what prompted me to ask the question ("Can you tell me which part(s) of my code is still open to SQL injection?")...because you did not state which part was still to prone to SQL injection, and as I said above, I wasn't aware that checkboxes could be use for SQL injection.

 

So...how do I deal with checkboxes and dropdown listboxes....to make them SQL-injection-proof?

 

Thanks once again to both of you for the amazing help and patience. Much appreciated! I'm learning so much, so soon...it's not even funny!

Cheers

[Psycho]

>> I don't know what logic you have to determine the value used for the switch().....

The answer to this question of yours is provided in the code block below: So the question I have is this: is that a safe way to do it...or is it still open to SQL injection? If it is indeed open to SQL injection then how do I fix/change it?

 

You should really take some time to understand what SQL injection is instead of just asking us if there is still a problem. Basically, you should never use data in a query that is provided externally (such as a user form post) without some means of sanitizing the data. You can do this through prepared statements (just using PDO doesn't do this) or in specifically sanitizing the data. In the code I provided, the first two line ensure that all the submitted values will be integers and non-integers will be ignored.

 

The value you are using for the switch() statement has nothing to do with the data used in the queries.

 

 

@Ch0cu3r: I certainly wasn't aware that checkboxes could be used to as a mechanism to perform SQL injection. Likewise, I didn't think that a dropdown listbox (allowing only fixed/coded values) could be used as a means to perform SQL injection. I was under the impression that only free-format textboxes could be used for malicious purposes.

 

So...how do I deal with checkboxes and dropdown listboxes....to make them SQL-injection-proof?

 

 

Any and ALL values coming from the user should be considered potentially malicious. Just because you can't modify a select field using direct keyboard/mouse input, it is very simple for someone to create their own form with the same field names but different values than you intended or to even hijack the response from your form and manipulate the values.

 

If you are using the values in a query, you can use prepared statements (go do some reading on how these work). You can also sanitize values before using in a query as I did. I didn't use a prepared statement because the number of values from the checkboxes is unknown ahead of time and it was more efficient to do as I did. But, it is not just SQL Injection that you need to worry about. There is also XSS vulnerabilities where potentially malicious JavaScript could be introduced. Before you use ANY data whether it is coming from the user, from your database, or anything - think about how you are going to use that data and what means you should use to protect it. There is no magic bullet and no one way to always protect the data. You might use htmlentities() on data being output tot he web page, but if you repurpose the data for a CSV file or an XML file you would want to use a different method.

[Jacques1]

Regarding SQL injections:

 

It's completely irrelevant whether you're dealing with checkboxes, text fields or whatever. Anybody can send any data to your server.

 

What you need to understand is that the client and the server communicate through HTTP messages. Any user interface built on top of this is just for convenience and has no bearing on the data. I could connect to your server right now and send any HTTP request I want, regardless of how your HTML forms may look like.

 

A form is really just a guide for the client. You're telling the user which kind of input you'd like to get. Whether or not the client follows your rules is completely up to them. They may send you the expected “on” for your checkbox parameter. But they might as well send you malicious SQL snippets.

 

So it's always the same rule: Never trust user input. This includes POST parameters, GET parameters, cookies, the URL, all HTTP headers and any other part of the HTTP request. All of this is under the user's control and can be used for an attack.

 

Learn how to use PDO correctly. The whole point of prepared statements is to not insert values into the query string.

[excelmaster]

@Psycho: The code you provided was almost a "drop-in replacement" for the code I had in my app. I only had to make a a few minor changes (here and there) and it (my app) is now working like a dream....and I can safely say that you've helped me get to end-of-job on this project.

 

One thing I will do thought (in the next few days) is to learn/read up more on PHP/MySQL security, SQL injection etc. and then re-visit this project to make it as water-tight as possible.

 

Thank you once again