-2

I am trying to make a simple select query of a mysql database with PHP where two variables are set based on user input. One variable is a month and thus can only be one specific value at a time. The other is dynamic and can either be a specific value or all values in the table. The code looks similar to this:

$var1 = "-1";
if (isset($_GET['Month'])) {
   $var1 = $_GET['Month'];
}
$var2 = $_GET['Person'];
if ($var2 == "All") {
   $var2 ="('Person 1' or 'Person 2' or 'Person 3')";
}
$query= sprintf("Select * from Table where `Month` = %s and `Person` = %s", GetSQLValueString($var1, "text"), GetSQLValueString($var2, "text"));

When I input a specific person for $var2 I am able to get the correct results, however, when I input All for $var2, the code runs but I return no results.

Actual block of code:

$colname_chainsales = "-1";
if (isset($_GET['Period'])) {
  $colname_chainsales = $_GET['Period'];
}
$rep = $_GET['Rep'];
if ($rep == "All") {
    $rep = " 1=1 ";
} else {
    $rep = " `PSIP` = ".$_GET['Rep'];
}
mysql_select_db($database_PPSC, $PPSC);
$query_chainsales= sprintf("select `PSIP` as `Sales Rep`, `Month`, `chainsales`.`PPSC #` as `PPSC #`, `chainsales`.`Store Name` as `Store Name`, sum(`Sales Less Credits ($)`) as `Sales Less Credits ($)`, sum(`RX Sales`) as `RX Sales`, sum(`Brand RX`) as `Brand RX`,sum(`Brand Rx C2`) as `Brand Rx C2`,sum(`prnr sales`) as `prnr`,sum(`h2h sales`) as `h2h`,sum(`specialty sales`) as `Specialty`,sum(`Brand RX`)-sum(`Brand Rx C2`)- sum(`specialty sales`)-sum(`h2h sales`)  as `Net Brand`, sum(`Pro w/SG`) as `Pro w/SG` , sum(`Pro w/o SG`) as `rpro`, SUM(`Pro w/SG`) / (SUM(`RX Sales`)- SUM(`specialty sales`))* 100 AS `Compliance` from `chainsales` inner join `clients` on `chainsales`.`ppsc #`=`clients`.`ppsc #` WHERE `Month` = %s and %s Group by `PPSC #`, `Month` ORDER BY `PSIP`", GetSQLValueString($colname_chainsales, "text"), GetSQLValueString($rep, "text"));
$chainsales = mysql_query($query_chainsales, $PPSC) or die(mysql_error());
$row_chainsales= mysql_fetch_assoc($chainsales);
$totalRows_chainsales = mysql_num_rows($chainsales);
Frailtub
  • 1
  • 1
  • 1
    Great! Did you have any specific questions or? – GrumpyCrouton Aug 01 '17 at 13:45
  • 1
    You are wide open to [SQL Injections](http://php.net/manual/en/security.database.sql-injection.php) and should really use [Prepared Statements](http://php.net/manual/en/mysqli.quickstart.prepared-statements.php) instead of concatenating your queries. Not even the escape functions are 100% injection safe. – M. Eriksson Aug 01 '17 at 13:46
  • Column names should be surrounded by back ticks and string values should be surrounded by single quotes. – M. Eriksson Aug 01 '17 at 13:48
  • First of all, error in your query. `'Month', 'Person'`. Remove single quotes – Nana Partykar Aug 01 '17 at 13:49
  • Possible duplicate of [When to use single quotes, double quotes, and backticks in MySQL](https://stackoverflow.com/questions/11321491/when-to-use-single-quotes-double-quotes-and-backticks-in-mysql) – M. Eriksson Aug 01 '17 at 13:49
  • Sorry about not including the question, first post in all. When I input All for $var2, I should get getting a result displaying everything from Table, while instead I return no results. Also, I am not worried about SQL Injections at this time as this is only for internal use and can not be accessed by anyone outside the company. – Frailtub Aug 01 '17 at 13:51
  • `$_Get['Person']` PHP variables are case-sensitive; did you mean `$_GET['Person']`? For that matter, why have the else-case at all? – Chris Forrence Aug 01 '17 at 13:53
  • _"I am not worried about SQL Injections at this time"_ - The famous last words. Later on, you will copy/paste your functions/queries for another project or this app gets released in public and you will forget to fix it. And, if any value contains a single quote or ends with a backslash, your query will fail, either way. Plus,you wouldn't even need your `sprinf` and your `GetSQLValueString()` functions if you used Prepare Statements. – M. Eriksson Aug 01 '17 at 13:55
  • @Frailtub you should always design systems using proper code, such as protecting yourself against SQL Injection, there is no point in doing it **wrong** and then going back later to fix it as it's very easy to forget to do, and eventually the project may become too big to do so easily. Trust me on this one. – GrumpyCrouton Aug 01 '17 at 13:55
  • Made edited to the original post to fix typos in the code. It was not copy and pasted. This isn't a project I am starting but adding a functionality to a project that was created by someone else years ago. I would be rebuilding the entire thing from scratch to prevent SQL Injection. I removed the else statement as it was redundant. – Frailtub Aug 01 '17 at 14:02
  • If you want to fetch all if `$var2` contains "All", then just remove the `Person` statement from the query completely in that case. – M. Eriksson Aug 01 '17 at 14:04
  • Btw, isn't `GetSQLValueString()` an old DreamWeaver-generated function that escapes the inputs? Then you can't have quotes and stuff in those strings since they will be escaped. Do a `var_dump($sql)` and see what the query actually looks like when it's generated. – M. Eriksson Aug 01 '17 at 14:09

2 Answers2

0

Use the in operator in place of = in your query like so:

Select * from Table where 'Month' = %s and 'Person' in %s", GetSQLValueString($var1, "text"), GetSQLValueString($var2, "text")

Also, when you define $var2 as the list of people, change "or" to commas:

if ($var2 == "All") {
   $var2 ="('Person 1', 'Person 2' , 'Person 3')";
} else {
   $var2 ="('".$_Get['Person']."')" ;
}
Chris Forrence
  • 10,042
  • 11
  • 48
  • 64
Bibhudatta Sahoo
  • 4,808
  • 2
  • 27
  • 51
0

Changes

  1. Enclose column name using backtick instead of single quotes.
  2. Change $var2 value on basis of it's condition.

Edit-1

Suppose,

Case-1: $var2 = "All":: Query will append to IN operator.

case-2: $var2 = 1:: Query will append to = operator.

Updated Code

$var1 = "-1";
if (isset($_GET['Month'])) {
  $var1 = $_GET['Month'];
}

$var2 = $_GET['Person'];
if ($var2 == "All") {
  $var2 = " IN ('Person 1', 'Person 2', 'Person 3')";
} else {
  $var2 = " = ".$_GET['Person'];
}

$query= sprintf("SELECT * FROM `Table` WHERE `Month` = %s and `Person` %s", GetSQLValueString($var1, "text"), GetSQLValueString($var2, "text"));

Edit-2

Reference to OP Comment: When I input All for $var2, I should get getting a result displaying everything from Table,

$var1 = "-1";
if (isset($_GET['Month'])) {
  $var1 = $_GET['Month'];
}

$var2 = $_GET['Person'];
if ($var2 == "All") {
  $var2 = " 1 = 1 ";
} else {
  $var2 = " `Person` = ".$_GET['Person'];
}

$query= sprintf("SELECT * FROM `Table` WHERE `Month` = %s  AND %s", GetSQLValueString($var1, "text"), GetSQLValueString($var2, "text"));
Nana Partykar
  • 10,556
  • 10
  • 48
  • 77
  • Why not just remove the `Person`-statement completely in case all should be shown? – M. Eriksson Aug 01 '17 at 14:06
  • I can do that according to the requirement. Who knows, OP might have kept some restriction for other Persons too. Otherwise, we can remove that `IN ..` statement. @MagnusEriksson – Nana Partykar Aug 01 '17 at 14:08
  • It might been easy to miss since the OP only mentioned it in a comment: _"When I input All for $var2, I should get getting a result displaying everything from Table"_ – M. Eriksson Aug 01 '17 at 14:11
  • @MagnusEriksson: Done. I Didn't saw that. Thanks for pointing out. – Nana Partykar Aug 01 '17 at 14:18
  • This has worked in case 1 when the input is all, however, has broken the functionality of case 2 when a specific person is the input. – Frailtub Aug 01 '17 at 14:29
  • "*..has broken the functionality of case 2 when a specific person is the input.*" Broken in the sense? What error it displays @Frailtub – Nana Partykar Aug 01 '17 at 14:30
  • When a specific person is the input, it displays no results. – Frailtub Aug 01 '17 at 14:32
  • Have you checked the table name, column name? It should work, there is nothing where it will break @Frailtub. Which One you used Edit-1 or Edit-2? – Nana Partykar Aug 01 '17 at 14:35
  • I've updated my code. Please use my code once again. Modified `column name` in Edit-2 Section @Frailtub – Nana Partykar Aug 01 '17 at 14:37
  • Using Edit 2. I check everything. I can only assume that it is not passing the query correctly as the code executes, just does not return results. – Frailtub Aug 01 '17 at 14:43
  • Again check @Frailtub. Actually instead of `Person`, I used `person` in column name. Which I have corrected. – Nana Partykar Aug 01 '17 at 14:43
  • I checked. The code I submitted was a sample of what I was doing. Here is my actual code block. Added to original post. – Frailtub Aug 01 '17 at 14:49
  • I saw your actual code. What you can do is: `echo $query_chainsales; die;` Paste the output in your database table SQL and check what error is coming @Frailtub – Nana Partykar Aug 01 '17 at 14:55