Magento getCollection , SQL injection

Question

I installed an extension for a customer of ours and I'm just learning about SQL and SQL injection, and this extension uses all kind of AJAX calls. However, I checked the code and some of the AJAX parameters are used directly in queries. I wonder, is this code for example SQL injection safe?

    $collection = Mage::getModel('blogn/user')->getCollection();
    $collection->getSelect()->joinLeft($table, 'main_table.user_id = ' . $userId. ','profile_id');

So, the $table variable is hardset just above the code. However, the $userId directly comes from an AJAX POST.

This is just an example, but a little down further there is also a getSelect()->where part, where also the $userId is direclty inserted.

Is this safe? To me it seems not. Should the query be changed to bind variables? Does Magento support this? For example, can I make the query something like this:

$collection->getSelect()->joinLeft($table, 'main_table.user_id = ?,'profile_id',array($userId));

Assuming that binding this variable would be safer because it can be prepared statement then?

Answer 1

Definitely not safe if anyone can change that parameter. Making an SQL injection with such code is easy as.

Magento totally supports binding here's how you should do it:

$adapter = $collection->getConnection();
$binds = $adapter->quoteInto("main_table.user_id = ?", $userId);
$collection->getSelect()->joinLeft($table, $binds, 'profile_id');