PHP MySQL insert with $_REQUEST

Question

I am trying to insert this $_REQUEST data into my MySQL database. The code is shown below:

foreach ($_REQUEST as $value)
{

 $sql = "INSERT INTO beer (b_beer) VALUES ('$value');";

 if(mysqli_query($connection, $sql))
  {
  echo "Records added successfully.";
  } 

   else
   {
   echo "ERROR: Could not able to execute $sql. " . mysqli_error($connection);
   }
}

The MySQL successfully inserts into my 'beer' table, but also creates 2 empty records. Is there a reason why this is happening?

yeah, the foreach is doing this. The query should be outside of it. That, and/or you've an empty key. — Funk Forty Niner, Dec 07 '17 at 15:03
Change your foreach to have `as $key=>$value)` and add the key value to your database as well `('$key = $value')`. Will show what fields it's trying to add. — Nigel Ren, Dec 07 '17 at 15:05
In `REQUEST` there are 3 values, you have 3 `inputs` with `name` attribute in your form. PHP don't know that you want to save just one of them. Use `if` condition or forget to `foreach` loop here, really don't need it. — pavel, Dec 07 '17 at 15:06
Your code is vulnerable to [**SQL injection**](https://en.wikipedia.org/wiki/SQL_injection) attacks. You should use prepared statements with bound parameters, via either the [**mysqli**](https://secure.php.net/manual/en/mysqli.prepare.php) or [**PDO**](https://secure.php.net/manual/en/pdo.prepared-statements.php) driver. [**This post**](https://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php) has some good examples. — Alex Howansky, Dec 07 '17 at 15:10
Check what $_REQUEST array consists of before inserting data to database. use print_r($_REQUEST) — Ravinder Reddy, Dec 07 '17 at 15:10
@AlexHowansky: Yes, its vulnerable SQL injection - but perhaps you could give an example of how this could be exploited here? 1: I cannot see how this can be exploited to subvert the logic of the application shown 2: we do ask people to provide a *minimal* verifiable example — symcbean, Dec 07 '17 at 15:56
@symcbean Er wait... are you suggesting that the burden of providing a minimal verifiable example falls on the *commenters*? — Alex Howansky, Dec 07 '17 at 16:14
@Alex Howansky: No, I was looking for an example of how it could be exploited. There's nothing in your example which couldn't be achieved if the code wasn't vulnerable to SQL injection. — symcbean, Dec 07 '17 at 22:34
@symcbean Oh ok, I see what you mean. How about this as a payload: `'),((select password from mysql.user where user = 'root' limit 1)),('` — Alex Howansky, Dec 07 '17 at 22:54
@symcbean Or better, if b_beer has a unique constraint, we can wipe out any known value: `existing_value') on duplicate key update b_beer = '` — Alex Howansky, Dec 07 '17 at 23:01
@Alex Howansky: Your first exploit only puts the hashed password into the table. Even if you get it out its not a lot of help to compromise the system. Agreed, the second might constitute a vulnerability. But that just brings me back to the point that critiquing code which should be written to demonstrate a specific issue as not suitable for deployment in an application is bit silly. — symcbean, Dec 07 '17 at 23:13

score 1 · Answer 1 · answered Dec 07 '17 at 16:17

Because $_REQUEST is associative array that contains $_GET, $_POST and $_COOKIE, some of them may be empty, but are present with keys anyway. That's why you're getting three records.
I would add a condition if ($value!='') { ...insert... }, but obviously you're still vulnerable to what $_GET contains, in case user added anything in the URL.

DDeMartini · Accepted Answer · 2017-12-07T15:36:06.707

-1

Give this a run. You'd still be vulnerable to possible SQL Injections but at least you'll trim your values and only try to write those that actually contain something, plus with addslashes() you'll help ensure that any characters that would possibly break SQL are escaped.

foreach ($_REQUEST as $key => $value) {

    if(trim($key) == 'beer') {
        if($beerid = trim($value)) {
            $sql = sprintf('INSERT INTO beer (b_beer) VALUES ("%s")',addslashes($beerid));

            if(mysqli_query($connection, $sql)){
                echo "Records added successfully.";
            } else {
                printf("ERROR: Could not able to execute %s -- %s\n",$sql,mysqli_error($connection));
            }
        }
    }
}

OR

Since you are only processing one request, and you know the name of the field as 'beer' you can simply this way:

if(isset($_REQUEST['beer']) {

   if($beerid = trim($_REQUEST['beer'])) {
        $sql = sprintf('INSERT INTO beer (b_beer) VALUES ("%s")',addslashes($beerid));

        if(mysqli_query($connection, $sql)){
            echo "Records added successfully.";
        } else {
            printf("ERROR: Could not able to execute %s -- %s\n",$sql,mysqli_error($connection));
        }
    }
}

edited Dec 07 '17 at 15:36

answered Dec 07 '17 at 15:13

DDeMartini

339
1
6

DDeMartini thank you, this works, but it creates two separate records. I tried using: print_r($_REQUEST) and I got: Array ( [beer] => Battery Brown Ale [b_beer] => ) – John Clemente Dec 07 '17 at 15:28
trim the value and test for empty at the same time, it's a common design pattern if the assignment is not null (false) then $beerid contains something you might want to use. If the value was empty, trim will return FALSE/empty and the the assignment "$beerid =" will pass that through, and the outer evaluation "if()" will be false. Tried and true. – DDeMartini Dec 07 '17 at 15:29
_trim will return FALSE/empty_ not true, trim will return `string` – B001ᛦ Dec 07 '17 at 15:30
if you know you are only using one of the records from Request, then test for that key. I'll revise the comment, assuming 'beer' contains the value you want. – DDeMartini Dec 07 '17 at 15:30
@B001 it still evals false, it works, used it about 1000000 times and never had an issue. Remember, we're dealing with PHP here which is very loosely typed... so.. frankly, in this case I don't feel distinction matters... – DDeMartini Dec 07 '17 at 15:36
@B001 agreed, it's dirty; which is just fine for PHP - it's a decent language considering it's a resume writing tool ;) – DDeMartini Dec 07 '17 at 15:42
'adslashes' is not a reliable method of preventing SQL injection (which I presume is the reason you have added it here) – symcbean Dec 07 '17 at 15:58
@symcbean exactly.. which is why I mentioned that it is still vulnerable, I added it because it's a good practice for exception prevention, nothing more did you even read the first paragraph? – DDeMartini Dec 07 '17 at 16:00

PHP MySQL insert with $_REQUEST

2 Answers2