0

Trying to bind an array (1st array bind) to prevent SQL injection

This is the working code:

if (isset($_POST['checkbox_selected']))
{       
    $valuesArr = array(); 
    foreach ($_POST['checkbox_selected'] as $key => $value) {
        //Retrieve Array ID to find Row number for CSVOption Column
        $findrow = array_search_partial($attributeid, $value);
        //attribure value is assigned to attribute id on form submit 
        $attribute = $value;            
        $csv = $csvcolumn[$findrow];        
        $valuesArr[] = "('$userid', '$feed_id', '$attribute', '$csv')";         
    }
        
    $sql = "INSERT INTO map (user_id, feed_id, attribute_id, csvcolumn) values ";
    $sql .= implode(',', $valuesArr);
    mysqli_query($conn,$sql);
}

I'm unable to bind the array, tried:

$sql = "INSERT INTO map (user_id, feed_id, attribute_id, csvcolumn) VALUES (?, ?, ? ,?)";
$stmt = $conn->prepare($sql);
$stmt->bind_param('iiii', implode(',', $valuesArr));
$stmt->execute();

echo implode(',', $valuesArr)
//('1', '1', '13', '9') //This is the the array which gets inserted into the SQL
//(user_id, feed_id, attribute_id, csvcolumn) //where these are the values assigned in the 1st statement
Dharman
  • 30,962
  • 25
  • 85
  • 135
just_testing_magento
  • 15
  • 5
  • I think you can do this with argument unpacking: https://www.php.net/manual/en/migration56.new-features.php#migration56.new-features.splat. Try: `$stmt->bind_param('iiii', ...$valuesArr);` to pass in the values of `$valuesArr` to `bind_param` – KompjoeFriek Aug 02 '23 at 19:32
  • 2
    Why so complicated? Leave the binding away and pass to execute directly. `$stmt->execute([$userid, $feed_id, $attribute, $csv]);`. – Markus Zeller Aug 02 '23 at 19:47

2 Answers2

4

You have two problems:

  1. You're not using the correct bind syntax.
  2. You're trying to insert multiple rows in a single prepared statement.
if (isset($_POST['checkbox_selected']))
{
    $sql = "INSERT INTO map (user_id, feed_id, attribute_id, csvcolumn) VALUES (?, ?, ?, ?);";
    // prepare only has to happen once
    $stmt = $conn->prepare($sql);

    $conn->begin_transaction();
    try {
        foreach ($_POST['checkbox_selected'] as $key => $value) {
            $findrow = array_search_partial($attributeid, $value);
            $attribute = $value;            
            $csv = $csvcolumn[$findrow];
            
            $stmt->bind_param('iiii', $userid, $feed_id, $attribute, $csv);
            $stmt->execute();
        }
        $conn->commit();
    } catch(mysqli_sql_exception $e) {
        $conn->rollback(); // immediately roll back changes
        throw $e; // re-throw exception
    }
}

The only scant benefit that you get from trying to pack multiple VALUES (), ... into a single query is that it gets wrapped into the implicit transaction that the query is wrapped in. Everything else about that approach is a downside. Explicitly opening the transaction wrapping the bind/execute loop gets you the same benefits [rollback on error, IO batching] while also leveraging the benefits of prepared statements. [single simple query parse, parameterization, etc]

Dharman
  • 30,962
  • 25
  • 85
  • 135
Sammitch
  • 30,782
  • 7
  • 50
  • 77
  • thanks, this works perfectly and definitely easier for me being relatively new to PHP to understand. Some new methods i havent come across before which i'll have to look into more as i progress ($conn->begin_transaction(), $conn->commit(); $conn->rollback(); ) – just_testing_magento Aug 03 '23 at 11:53
  • [This article](https://www.dbvis.com/thetable/database-transactions-101-the-essential-guide/) has a decent rundown of what a database transaction is. By default most DB APIs will wrap every query in its own transaction implicitly, but it's useful to start one manually in order to cover operations comprised of multiple statements. – Sammitch Aug 03 '23 at 18:13
0

Instead of

$stmt->bind_param('iiii', implode(',', $valuesArr));

You can use

$stmt->bind_param('iiii', $userid, $feed_id, $attribute, $csv);

This line

$valuesArr[] = "('$userid', '$feed_id', '$attribute', '$csv')";

Makes an array with one string in it with all the fields concatenated and I am not sure you meant to do that. The implode returns the first and only member of the array.

bguebert
  • 9
  • 3