5

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?

3

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');
| improve this answer | |
  • Ah, that's awesome. Thanks a lot! - I'll accept the answer as soon as I have tested this. – Marcel Jul 27 '16 at 16:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.