RAW queries and SQL injection

Instead of messing around with raw just do proper bindings: See

https://laracasts.com/discuss/channels/guides/getpdo-usage and https://laracasts.com/discuss/channels/eloquent/writing-all-queries-directly-vs-model-relations and https://laracasts.com/discuss/channels/laravel/sql-native-to-query-builder

@jlrdw thanks for the links, I'll check them out but my query is rather complex and I need to do it as RAW, there's no way to convert it to eloquent.

Read the replies. You will see.

Goodness an example that @Cronix gave isn't good enough

$bindings = [
    'product_type_id' => 1,
    'service_sub_type_id' => 2,
    'school_id' => 57,
    'status_id1' => 2,
    'status_id2' => 3
];

$users = DB::select('SELECT DISTINCT CONCAT(u.last_name, ", ", u.first_name ) AS full_name, ci.created_at AS date_purchase
FROM cart_items AS ci
LEFT JOIN products AS p ON ci.product_id = p.id
LEFT JOIN carts AS c ON ci.cart_id = c.id
LEFT JOIN `status` AS s ON ci.status_id = s.id

INNER JOIN users AS u ON c.user_id = u.id
INNER JOIN school_users AS su ON u.id = su.user_id

INNER JOIN
    (
    SELECT MAX(created_at) AS created_at1, cart_id
    FROM cart_items
    GROUP BY `cart_id`
    )p2 ON ci.cart_id = p2.cart_id AND ci.created_at = p2.created_at1

WHERE p.product_type_id = :product_type_id AND p.service_sub_type_id = :service_sub_type_id AND su.school_id = :school_id AND ci.status_id = :status_id1 OR ci.status_id = :status_id2
ORDER BY ci.created_at DESC', $bindings);

Yours is less complex.

With what I saw, you can do a QueryBuilder of this query.

@jlrdw As you can see from the question I am already passing the binding from an array yet the query gets executed without sanitization. I will post later the exact query and you tell me if it's possible to convert to eloquent without using RAW as I was not able.

@jlrdw @Vilfago

Ok this is the exact query, how do you convert to eloquent without using a DB:RAW?

the only data passed by the user is $id

    $models = DB::select(DB::raw("SELECT
                Count(collected_items.model_type_id) AS total_collected,
                       producers.producer,
                       item_models.id,
                       item_models.model_name,
                       lengths.mm,
                       item_models.picture,
                       item_models.color,
                       item_models.material,
                       collected_item_user.collected_item_hand,

                       Count(CASE item_types.id WHEN '1' THEN item_types.id ELSE NULL END) AS '1',
                       Count(CASE item_types.id WHEN '2' THEN item_types.id ELSE NULL END) AS '2',
                       Count(CASE item_types.id WHEN '3' THEN item_types.id ELSE NULL END) AS '3',
                       Count(CASE item_types.id WHEN '4' THEN item_types.id ELSE NULL END) AS '4',
                       Count(CASE item_types.id WHEN '5' THEN item_types.id ELSE NULL END) AS '5',
                       Count(CASE item_types.id WHEN '6' THEN item_types.id ELSE NULL END) AS '6',
                       Count(CASE item_types.id WHEN '7' THEN item_types.id ELSE NULL END) AS '7',
                       Count(CASE item_types.id WHEN '8' THEN item_types.id ELSE NULL END) AS '8',
                       Count(CASE item_types.id WHEN '9' THEN item_types.id ELSE NULL END) AS '9',
                       Count(CASE item_types.id WHEN '10' THEN item_types.id ELSE NULL END) AS '10',
                       Count(CASE item_types.id WHEN '11' THEN item_types.id ELSE NULL END) AS '11',
                       Count(CASE item_types.id WHEN '12' THEN item_types.id ELSE NULL END) AS '12',
                       Count(CASE item_types.id WHEN '13' THEN item_types.id ELSE NULL END) AS '13',
                       Count(CASE item_types.id WHEN '14' THEN item_types.id ELSE NULL END) AS '14',
                       Count(CASE item_types.id WHEN '15' THEN item_types.id ELSE NULL END) AS '15'
                FROM   collected_items
                       INNER JOIN collected_item_user
                               ON collected_items.id = collected_item_user.collected_item_id
                       INNER JOIN users
                               ON collected_item_user.user_id = users.id
                       INNER JOIN item_types
                               ON collected_items.model_type_id = item_types.id
                       INNER JOIN item_models
                               ON collected_items.model_id = item_models.id
                       INNER JOIN producers
                               ON collected_items.producer_id = producers.id
                       INNER JOIN lengths
                               ON collected_items.length_id = lengths.id
                                  AND lengths.id = item_models.length_id
                                  AND producers.id = item_models.producer_id
                WHERE  users.data_type_id = $user_data_type
                       AND users.item_type_id = $user_type
                       AND users.data_x_id = 1
                       AND collected_items.model_id = $id

                GROUP  BY collected_item_user.collected_item_hand"));

I would not use eloquent for such a query I would use regular PDO Which one of the links gives an example Of usage, that's just how I would do it.

And that's just my rule if I have to use the word raw in anyting I may as well use regular PDO.

If you read through the links It talks about limitations using eloquent.

I would imagine somehow you could come up with some large eloquent query That has regular and raw usage but these things quickly become a mess. Sometimes even a nightmare.

However, since the $id comes from this kind of request url:

http://myproject55.test/model/{id}?_method=get

Can't I just add a constraint to the route like this:

Route::get('model/{id}', 'Controller@request')->where('id', '[0-9]+')->middleware('auth');

This way id can only be numeric

@jlrdw Like this?

$params = array(':id' => $id);

$pdo->prepare('

//That long RAW query   

');

$pdo->execute($params);

Just practice the query using regular PDO outside of laravel first. With known correct results and work out any bugs.

@chriz74 you said you were already using bindings, but what you posted is not using bindings.

Try this:

$models = DB::select(DB::raw("SELECT
            Count(collected_items.model_type_id) AS total_collected,
                   producers.producer,
                   item_models.id,
                   item_models.model_name,
                   lengths.mm,
                   item_models.picture,
                   item_models.color,
                   item_models.material,
                   collected_item_user.collected_item_hand,

                   Count(CASE item_types.id WHEN '1' THEN item_types.id ELSE NULL END) AS '1',
                   Count(CASE item_types.id WHEN '2' THEN item_types.id ELSE NULL END) AS '2',
                   Count(CASE item_types.id WHEN '3' THEN item_types.id ELSE NULL END) AS '3',
                   Count(CASE item_types.id WHEN '4' THEN item_types.id ELSE NULL END) AS '4',
                   Count(CASE item_types.id WHEN '5' THEN item_types.id ELSE NULL END) AS '5',
                   Count(CASE item_types.id WHEN '6' THEN item_types.id ELSE NULL END) AS '6',
                   Count(CASE item_types.id WHEN '7' THEN item_types.id ELSE NULL END) AS '7',
                   Count(CASE item_types.id WHEN '8' THEN item_types.id ELSE NULL END) AS '8',
                   Count(CASE item_types.id WHEN '9' THEN item_types.id ELSE NULL END) AS '9',
                   Count(CASE item_types.id WHEN '10' THEN item_types.id ELSE NULL END) AS '10',
                   Count(CASE item_types.id WHEN '11' THEN item_types.id ELSE NULL END) AS '11',
                   Count(CASE item_types.id WHEN '12' THEN item_types.id ELSE NULL END) AS '12',
                   Count(CASE item_types.id WHEN '13' THEN item_types.id ELSE NULL END) AS '13',
                   Count(CASE item_types.id WHEN '14' THEN item_types.id ELSE NULL END) AS '14',
                   Count(CASE item_types.id WHEN '15' THEN item_types.id ELSE NULL END) AS '15'
            FROM   collected_items
                   INNER JOIN collected_item_user
                           ON collected_items.id = collected_item_user.collected_item_id
                   INNER JOIN users
                           ON collected_item_user.user_id = users.id
                   INNER JOIN item_types
                           ON collected_items.model_type_id = item_types.id
                   INNER JOIN item_models
                           ON collected_items.model_id = item_models.id
                   INNER JOIN producers
                           ON collected_items.producer_id = producers.id
                   INNER JOIN lengths
                           ON collected_items.length_id = lengths.id
                             AND lengths.id = item_models.length_id
                             AND producers.id = item_models.producer_id
            WHERE  users.data_type_id = :data_type
                   AND users.item_type_id = :user_type
                   AND users.data_x_id = 1
                   AND collected_items.model_id = :id

            GROUP  BY collected_item_user.collected_item_hand"), 
    [
    'data_type' => $user_data_type, 
    'user_type' => $user_type, 
    'id' =>$id
    ]);

@36864 ok, however the only data passed by the user is $id, those others are set inside the controller. by the way it's [ or ( ? ['data_type' => $user_data_type, 'user_type' => $user_type, 'id' =>$id) and am I not passing bindings like this:

        array(
                    
                    'input_id' => $id,

                    )

as in the original question

@36864 I tried running the query in the ide, I can still see it's passing something like $id OR 1=1 .. however the IDE is asking me to input the data so maybe it's not considering the bindings. I have to find a way to output the query in the browser.

edit: I added this to check the query in the browser:

                \DB::listen(function($sql) {
                    var_dump($sql);
                });

I can see $id OR 1=1 is still being passed. So what's the catch here?

For practice just have a form to pass the parameter, and yes passing an array of bindings is correct. Something like:

    public function getOne($dogid)
    {
        $sql = "SELECT * FROM " . PREFIX . "dogs WHERE dogid = :dogid and field2 = :param2";
        $sth = DB::getPdo()->prepare($sql);
        $params = [':dogid' => $dogid, ':field2' => $param2];  // array is fine
        $sth->execute($params);
        return $sth->fetch(\PDO::FETCH_OBJ);
    }

But look at the PHP manual as specific bindings can be passed as well. But generally an array like above is fine.

Or of course you could do similar using DB facade. Just to me it's easier using getPdo.

@jlrdw This works. I had to cast the result to an array like this:

$models = array($sth->fetch(\PDO::FETCH_OBJ)) ;

to be able to traverse it with my routines.

Now if I pass this in the browser:

http://myproject55.test/model/218?_method=get

or this:

http://myproject55.test/model/218%20OR%201=1?_method=get

I get the same results.

You can change FETCH_OBJ to FETCH_ASSOC

And for many results you need fetchAll instead of fetch.

'fetch' was just example of one result.

See http://php.net/manual/en/pdostatement.fetchall.php

http://php.net/manual/en/pdostatement.fetch.php

The important thing here is that the parameter values are combined with the compiled statement, not an SQL string. SQL injection works by tricking the script into including malicious strings when it creates SQL to send to the database. So by sending the actual SQL separately from the parameters, you limit the risk of ending up with something you didn't intend. Any parameters you send when using a prepared statement will just be treated as strings (although the database engine may do some optimization so parameters may end up as numbers too, of course). In the example above, if the $name variable contains 'Sarah'; DELETE FROM employees the result would simply be a search for the string "'Sarah'; DELETE FROM employees", and you will not end up with an empty table.

Very simple.

And also if you are new to using pdo, it wouldn't hurt to take a good PDO tutorial prior to using laravel.

It would really give you a better understanding of what's going on. As laravel uses PDO in the background.

Here is one https://phpdelusions.net/pdo

But you can search for others.

@rsvb thanks for the explanation however if the point is to pass a string to the query can't we just cast the parameters to string and keep the RAW query? What difference does it make?

You should to see :

AND data.id = "218 OR 1 = 1" so no injection but a search string.

In PDO you can do specific binding like:

$sth->bindParam(':something', $something, PDO::PARAM_INT);
$sth->bindParam(':something2', $something2, PDO::PARAM_STR, 12);

Which is very secure. But generally just passing the parameter array, mysql works out what is what, i.e., an INT vs a STRING.

Some databases require the passing of specific parameters like above.

But passing the parameter array in the execute statement is still binding.

You as a future developer you need to take some quality time and learn some of this stuff. Don't just trust laravel to do everything for you.

The guide on pdo I gave link to is a good one, with other links that further explain sql injection, read the guide.

the solution works if the query has one id to look for. But what if I need to search for multiple ids?

in the query it will be

AND item.id IN ($ids)

where for example

$ids = "35,36,69,73,98,218,219,234,242"

if I put ':ids' => $ids in the params and pass them as

AND item.id IN (:ids)

I get 0 results while there are results in the DB for id 218 and 219.

by dumping $sth I can see that the ids are not being passed. Here's what it passes: IN (:ids)\n

Then use eloquent and laravel collections, in PDO you'd select in a select.

You really need to take a good PDO tutorial, and that would answer any questions.

@jlrdw Thanks for your reply however I don't get it. If I dump $params this is the result:

array:1 [
  ":ids" => "35,36,69,73,98,218,219,234,242"
]

So :ids has the data, so why it's not passed correctly?

READ https://www.w3schools.com/sql/sql_in.asp

And take this tutorial https://www.w3schools.com/sql/

@jlrdw maybe I didn't make myself clear. The original query works perfectly as my $ids is a string I get like this:

        $plucked_ids = $models_ids->pluck('id')->toArray();

                $ids = join(",", $plucked_ids);

With that I get a string ( $ids) that I pass to the SQL query and it works. However if I try to pass the same string to the placeholder :ids the string is not passed. So the placeholder is not getting any data.

PDO doesn't play well with the IN clause, but from an old git gist:

$ids = array( 8, 9 );
$place_holders = implode( ',', array_fill( 0, count($ids), '?' ) );
$sth = $con->prepare("
  SELECT id
    FROM MyGuests
    WHERE id IN ( $place_holders )
");
foreach( $ids as $k => $v ){
    $sth->bindValue( $k + 1, $v, PDO::PARAM_INT );
}
$sth->execute( );
var_dump( $sth->fetchAll() );
var_dump( $sth->rowCount() );

It can be done.

I don't or never needed the in clause myself, but something between like:

        $start = 3;
        $finish = 9;
        $params = ['start' => $start, 'finish' => $finish];
        $sql = "SELECT * FROM dc_pets where petid > :start and petid < :finish "  . $pagingQuery;
        $sth = db::getPdo()->prepare($sql);
        $sth->execute($params);
        $pets = $sth->fetchAll(\PDO::FETCH_OBJ);

Just an example.

@jlrdw what is $con in $sth = $con->prepare ?

Tring this code I get $con undefined variable, modifying it with:

$sql=" //QUERY";
$sth = DB::getPdo()->prepare($sql);
foreach( $ids as $k => $v ){
                    $sth->bindValue( $k + 1, $v, PDO::PARAM_INT );
                }

                $sth->execute( );

I get: SQLSTATE[HY093]: Invalid parameter number: mixed named and positional parameters

edit:

I get it, I can't bind other parameters in the query like ":model" or something else or it will spit out that error.

I used positional parameters and modified the code like this to bind the other params:

foreach( $plucked_ids as $k => $v ){
                    $sth->bindValue( $k + 3, $v, PDO::PARAM_INT );
                }
                $sth->bindValue(1, $data_1, PDO::PARAM_INT);
                $sth->bindValue(2, $data_2, PDO::PARAM_INT);

It seems it's working.

Just remember this post started out talking about alternatives to the raw expression but ultimately use whatever technique works for you.

And whether you use eloquent or not it still would not hurt to take a good PDO tutorial.

Just remember many complex queries can be tricky to work out Some of it is trial and error until it works.