Why we review Magento modules before installation

Why we review Magento modules before installation

Modules occasionally contain backdoors and vulnerabilities. Here is why you need to have every one reviewed before it is installed on your Magento site.

Scroll Down for more info
Summary:
  • Always review a Magento module before installing it.
  • Never assume that just because the module vendor's website says it is good that it is.
  • If you don't know how to review a Magento module or are not a developer, contact a Magento specialized agency to handle the module review for you.
  • A bad module/extension could be an open door to the world.

Technical:

Last week, I purchased a module on behalf of one of our clients. I was excited to find the module as it seemed to be fully featured and perhaps a better solution than ones we had used in the past.

While the module appears to have excellent functionality, we found the code quality was seriously lacking. This article details some of the issues that we found and highlights how important code reviews are before installing a module.

We specialize in custom Magento development. We do many Magento customizations for our clients, often even when using a prebuilt module such as this one. The module described in this article received a “F” grade, and we want to give you a couple of reasons behind that. The quality simply is not up to the standards we set for ourselves at SwiftOtter (even if we lower the bar), and the code violates many best practices.


Consider several examples:

class SomeMerchant_SomeNamespace_Model_Observer_SomeObserverClass extends SomeMerchant_SomeNamespace_Model_Observer
{
const USE_MEASUREMENT_PROTOTOCOL = false; 

/**
* @param Varien_Event_Observer $observer
*/
public function beforePlaceOrder(Varien_Event_Observer $observer)
{
    if (self::USE_MEASUREMENT_PROTOTOCOL && $observer->getOrder()->getId())
        {
            die('here');

            Mage::getModel(‘a/model’)->purchase($observer->getOrder());
        }
    }
}

This code will obviously never be run in production, as the condition will always be false. Why ship code that obviously you're mandating not be run? This is likely due to no code review and would be considered a major red flag. However, in the case that this code could run, upon entering that “if” statement, the entire program would halt execution, without a warning, explanation, or notice to the customer.

Here’s another example, from a block:


protected function _construct()
{
    if ($this->use)
    {
        set_time_limit(360);

        set_include_path(get_include_path() . PATH_SEPARATOR . '/lib/SomeDir’);

        /**
         * Set custom template
         */
        $this->setTemplate(‘path/to/some/template/auth.phtml');
    }
}

The lib folder is already part of the include path, and resetting the include path is an unnecessary operation that slows down the application. After seeing this, I’m concerned about how well the module vendor’s team knows the Magento platform. While Magento is a monolithic platform, and there are aspects to it that are easy to miss, the autoloader and include path are pretty basic knowledge, and something like this reveals perhaps a lack of expertise.

Yet another example:


class SomeVendor_SomeModule_Block_Notify_Split extends Mage_Adminhtml_Block_Template
{
public function getMessage()
{
    if ('' !== (string) Mage::getStoreConfig('module/config/code') && 0 == (int) Mage::getStoreConfig('ec/config/code_split'))
    {
        return true;
    }

        return null;
    }
}

While this short class looks fairly benign, the conditional statement is bizarre in nature, and again shows a lack of knowledge about Magento, as well as PHP, that is concerning to me. PHP performs automatic type coercion in equality checks that doesn’t necessitate casting the value, as I see here. Also, the second part of the statement casts the value to an integer, and then uses a loosely-typed equality check—to compare it to another integer! This simply makes no sense. Finally, you could merely use the Mage::getStoreConfigFlag() function for both of these, thus eliminating the need for type conversions in the first place.

And, one final example:


if (!(int)@Mage::getStoreConfig('module/definitions/dimensions'))
{
return array();
}

Quite frankly, this example (from a helper class), just doesn’t make sense. Let’s walk through the issues with the conditional statement—I can identify at least three.

  • First of all, the config value shouldn’t be converted to an integer—that’s just unnecessary to check for a false value.
  • Second, after converting to an integer, the ! operator is used, which will convert the integer into a boolean. We’ve now performed a second type conversion—which will reverse the effects of the first one—that could simply have been handled by the Magento framework with Mage::getStoreConfigFlag().
  • The third issue, and the most concerning, is the error suppression operator (@) in front of the Mage::getStoreConfig call. I have multiple Magento certifications and have been involved in the development of at least 10 different sites on the platform and the development or integration of over 70 modules. I never heard of an issue where a call to Mage::getStoreConfig threw an exception. But, I still wanted to give it a chance, so I decided to look through the Magento code that retrieves store configuration values. As I suspected, at the point in the store execution flow, it is virtually impossible for an exception to be thrown. Furthermore, if an exception were thrown at this point, it would be due to a critical failure—either the inability to identify the current store scope, or perhaps an issue with the Magento XML processing logic—in which case, the error would require immediate attention and should at the very least be logged, instead of merely swept under the rug with the @ operator. What I believe is more likely in this case is that a developer on this project was encountering an unrelated issue and just started throwing whatever they could at it, and then failed to go back through and remove the remnants of their troubleshooting once they had resolved whatever prompted it.

There are many, many other examples across the codebase—these are just a few.

While this module appears to function, it isn’t something that we can deliver to our customers. When we include a module in a project, we put our name behind it, and agree to maintain it should there be any issues. I am unwilling to identify our company with this module, and I’m concerned about the negative impact that maintaining it could have on our bottom-line.

We found large amounts of obfuscated (encrypted) code. This is a red flag in that they are trying to conceal an action they are taking. This could mean they are sending information back to their servers for “licensing” purposes. They could be sending back sales statistics and your database credentials.

Another red flag we found was the introduction of a controller with obfuscated (encrypted) code. The controller is what makes a page appear on the frontend (like viewing a product). This is even more of a concern as they could be scouting the internet looking for license compliance: them and hackers who know of specific vulnerabilities.


Verdict of the code review? Failed.

There’s no reason to put our client through the pain of having to deal with a module that’s sub-standard. Unfortunately, there are more poorly written modules in the Magento world than excellent ones, and we often have to throw out modules due to inferior code quality.

If you are a developer who works with the Magento platform, you’ve likely been nodding along, agreeing with the examples given. But, if you’re a store owner, you’re probably wondering “what difference does it make to me?”

It makes a tremendous difference to your bottom line, especially if you want any customizations made to the module. Troubleshooting or customizing a module that’s inferior in quality often takes at least twice as long as an excellently-developed one, and leads to frustration and hiccups for the end-user, whether they are members of your internal team or customers on your website.


In closing:

In the end, a commitment to excellence is something that’s easy to talk about, but harder to put into practice, especially when there seems to be an easier way around problems. However, at SwiftOtter, we believe passionately in the pursuit of excellence in all we do, and we are willing to do whatever it takes to see our customers successful.

This is the type of process and approach that we take. While there can be more investment up front, we have seen the results pay off (as in this case, modifying or maintaining such code can be very difficult).

SwiftOtter, Inc.
It relates to Search Engine Optimization.
Tyler Schade - developer at Swift Otter

Formerly a developer at SwiftOtter.