Magento ECG Coding Standard – Why Many PHP Functions Are Disallowed

code analysiscoding-standardsdocumentationmagento-ecg

The Magento ECG Coding Standard seems to be (at least kind of) official as standard for Magento 1 extensions:

https://github.com/magento-ecg/coding-standard

But I don't understand the reasoning behind all rules, and the code sniffer rules with their messages alone do not help much. Is there any detailed documentation about the standard? I know the common best practices and the developers guide but cannot find anything specific about these coding standards.

What troubles me most is the strictness about not using PHP functions.

For example: Why is every single file system related PHP function forbidden?

I guess, you are supposed to use Varien_Io_File, Varien_File_Object etc. but even the core developers are not aware of all the Varien classes and you often find things like in Mage_ImportExport_Model_Import_Adapter_Csv:

    $this->_fileHandler = fopen($this->_source, 'r');

So, the core is not the best example, as often.

Other IMHO questionable forbidden functions:

  • mb_parse_str
  • parse_str
  • parse_url

  • base64_decode

    • yeah, it's used in backdoors but banning eval should be enough and there are legit use cases, like encoding binary data. And other than json_decode (which is not forbidden) there is no core helper available for this.

Source: https://github.com/magento-ecg/coding-standard/blob/master/Sniffs/Security/ForbiddenFunctionSniff.php

Essentially, my question boils down to: Where is this standard documented? And/or is there a list of "things to use instead of these native PHP functions"?

Best Answer

Got an unofficial answer from the ECG team on that:

First of all, the flagged functions are not necessarily disallowed - they should trigger a manual review on usage to ensure legitimate use. It's a review helper tool, not good/bad code scoring tool.

Second, the assumption was it is better to use Magento wrappers (functions/classes) if they exists as they might offer additional functionality or protection.

Third, as for specific calls, base64_decode is used often for malicious injected code, and the rest like parse_str might be vulnerable, especially handling unknown or user provided input - see for example http://php-security.org/2010/05/31/mops-2010-049-php-parse_str-interruption-memory-corruption-vulnerability/

Again, this is flagging it for review not rejecting the code as bad.

Hope it helps.

Related Topic