diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 0000000..01c5a21 --- /dev/null +++ b/.gitattributes @@ -0,0 +1,14 @@ +# Path-based git attributes +# https://www.kernel.org/pub/software/scm/git/docs/gitattributes.html + +# Ignore all test and documentation with "export-ignore". +/POET/Docs export-ignore +/POET/Tests export-ignore +/.gitattributes export-ignore +/.gitignore export-ignore +/.travis.yml export-ignore +/CHANGELOG.md export-ignore +/CONTRIBUTING.md export-ignore +/phpunit.xml.dist export-ignore +/POET.md export-ignore +/README.md export-ignore \ No newline at end of file diff --git a/.gitignore b/.gitignore index 2c360b3..63f3d5a 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,4 @@ -vendor/ +/vendor/ .patches/ .idea/ nbproject/ @@ -8,5 +8,5 @@ nbproject/ .buildpath .cache composer.phar +composer.lock phpunit.xml -moodle-local_codechecker \ No newline at end of file diff --git a/.travis.yml b/.travis.yml new file mode 100644 index 0000000..15fb3ed --- /dev/null +++ b/.travis.yml @@ -0,0 +1,13 @@ +language: php + +sudo: false + +install: + - phpenv config-rm xdebug.ini + - composer install --prefer-source + +php: + - 5.4 + - 7.0 + +script: ./vendor/bin/phpunit \ No newline at end of file diff --git a/POET.md b/POET.md new file mode 100644 index 0000000..ff5a475 --- /dev/null +++ b/POET.md @@ -0,0 +1,389 @@ +# POET Coding Standard + + +## Looping Over Slow Functions +Usage of slow functions from within loops could lead to performance problems. Often times, database queries in + loops can be removed from the loop and rewritten to grab the data in a single query. +## Built In Database Methods +Moodle comes with its own database layer. The base class is *moodle_database* + and it is accessed by the *$DB* global variable. This layer takes care of supporting + various database backends. All interactions with the database must go through this class. +## Database Table Prefix +SQL written for Moodle should not include the table prefix. The table name should just be passed into the + *moodle_database* class method or if using raw SQL, the table name should be surrounded by curly brackets. + + + + + + + + + +
Valid: Not using the prefix.Invalid: Using the prefix.
+ + $DB->get_records('user', ['id' => 1]); + $DB->get_records_sql('SELECT * FROM {user} WHERE id = ?', [1]); + + + + $DB->get_records('mdl_user', ['id' => 1]); + $DB->get_records_sql('SELECT * FROM mdl_user WHERE id = ?', [1]); + +
+## Request Variables +Some request variables are not reliable or have been removed in later versions of PHP. Usage of them should + be avoided. +## Deprecated Parameter Constants +The constants prefixed with *PARAM_* within Moodle are used for cleaning parameters. Deprecated constants should not be used. + Always try to use the most specific constant possible, EG: PARAM_TEXT instead of PARAM_CLEAN. +## Modifying PHP Configuration Settings +Cannot use the *ini_set* PHP function. This can cause unexpected behavior. + + + + + + + + + +
Valid: Use Moodle method to modify PHP settings.Invalid: Directly calling ini_set.
+ + raise_memory_limit(MEMORY_HUGE); + + + + ini_set('memory_limit', '1G'); + +
+## Superglobals +Not allowed to read values from PHP superglobals like *$_GET*, *$_POST*, etc. + + + + + + + + + +
Valid: Use Moodle methods to access request data.Invalid: Use of $_GET.
+ + $id = required_param('id', PARAM_INT); + $action = optional_param('action', 'default', PARAM_ALPHANUMEXT); + + + + $id = $_GET['id']; + $action = !empty($_GET['action']) ? $_GET['action'] : 'default'; + +
+## Warn About Raw SQL Functions +Moodle provides a number of helper functions for accessing the database, including some functions that allow + the use of raw SQL. This can be problematic if the SQL is complext, inefficient, or includes parameters + correctly. For this reason, this sniff warns about the use of these functions for further investigation. + + + + + + + + + +
Valid: Use placeholders for parameters.Invalid: Using string concatenation for parameters.
+ + $DB->get_records_sql('SELECT * FROM {course} WHERE shortname = ?', [$get]); + + + + $DB->get_records_sql('SELECT * FROM {course} WHERE shortname = '.$get); + +
+ + + + + + + + + +
Valid: Make use of indexes when dealing with large data sets.Invalid: There is no index on just username.
+ + $DB->get_records_sql(' + SELECT c.* + FROM {user_enrolments} ue + JOIN {enrol} e ON e.id = ue.enrolid + JOIN {course} c ON e.courseid = c.id + JOIN {user} u ON ue.userid = u.id + WHERE u.username = ? + AND u.mnethostid = ? + ', [$USER->username, $CFG->mnet_localhost_id]); + + + + $DB->get_records_sql(' + SELECT c.* + FROM {user_enrolments} ue + JOIN {enrol} e ON e.id = ue.enrolid + JOIN {course} c ON e.courseid = c.id + JOIN {user} u ON ue.userid = u.id + WHERE u.username = ? + ', [$USER->username]); + +
+## Manual Inclusion of jQuery +Including jQuery and associated libraries manually can cause issues. The versions bundled with Moodle should be used. + + + + + + + + + +
Valid: Use Moodle's JQuery library.Invalid: Using your own JQuery library.
+ + $PAGE->requires->jquery(); + + + + $PAGE->requires->js('/mod/foo/jquery.js'); + +
+## Unconditional If Statements +If statements that are always evaluated should not be used. + + + + + + + + + +
Valid: An if statement that only executes conditionally.Invalid: An if statement that is always performed.
+ + if ($test) { + $var = 1; + } + + + + if (true) { + $var = 1; + } + +
+ + + + + + + + + +
Valid: An if statement that only executes conditionally.Invalid: An if statement that is never performed.
+ + if ($test) { + $var = 1; + } + + + + if (false) { + $var = 1; + } + +
+## Todo Comments +FIXME Statements should be taken care of. + + + + + + + + + +
Valid: A comment without a fixme.Invalid: A fixme comment.
+ + // Handle strange case + if ($test) { + $var = 1; + } + + + + // FIXME: This needs to be fixed! + if ($test) { + $var = 1; + } + +
+## Todo Comments +TODO Statements should be taken care of. + + + + + + + + + +
Valid: A comment without a todo.Invalid: A todo comment.
+ + // Handle strange case + if ($test) { + $var = 1; + } + + + + // TODO: This needs to be fixed! + if ($test) { + $var = 1; + } + +
+## Byte Order Marks +Byte Order Marks that may corrupt your application should not be used. These include 0xefbbbf (UTF-8), 0xfeff (UTF-16 BE) and 0xfffe (UTF-16 LE). +## Multiple Statements On a Single Line +Multiple statements are not allowed on a single line. + + + + + + + + + +
Valid: Two statements are spread out on two separate lines.Invalid: Two statements are combined onto one line.
+ + $foo = 1; + $bar = 2; + + + + $foo = 1; $bar = 2; + +
+## Space After Casts +Spaces are not allowed after casting operators. + + + + + + + + + +
Valid: A cast operator is immediately before its value.Invalid: A cast operator is followed by whitespace.
+ + $foo = (string)1; + + + + $foo = (string) 1; + +
+## Lowercase Keywords +All PHP keywords should be lowercase. + + + + + + + + + +
Valid: Lowercase array keyword used.Invalid: Non-lowercase array keyword used.
+ + $foo = array(); + + + + $foo = Array(); + +
+## Line Endings +Unix-style line endings are preferred ("\n" instead of "\r\n"). +## Deprecated Functions +Deprecated functions should not be used. + + + + + + + + + +
Valid: A non-deprecated function is used.Invalid: A deprecated function is used.
+ + $foo = explode('a', $bar); + + + + $foo = split('a', $bar); + +
+## PHP Code Tags +Always use <?php ?> to delimit PHP code, not the <? ?> shorthand. This is the most portable way to include PHP code on differing operating systems and setups. +## Silenced Errors +Suppressing Errors is not allowed. + + + + + + + + + +
Valid: isset() is used to verify that a variable exists before trying to use it.Invalid: Errors are suppressed.
+ + if (isset($foo) && $foo) { + echo "Hello\n"; + } + + + + if (@$foo) { + echo "Hello\n"; + } + +
+## Closing PHP Tags +Files should not have closing php tags. + + + + + + + + +
Valid: No closing tag at the end of the file.Invalid: A closing php tag is included at the end of the file.
+ + + + + + +
+Documentation generated on Fri, 10 Jun 2016 12:59:10 -0700 by [PHP_CodeSniffer 2.6.0](https://github.com/squizlabs/PHP_CodeSniffer) \ No newline at end of file diff --git a/POET/Docs/Performance/LoopedFunctionStandard.xml b/POET/Docs/Performance/LoopedFunctionStandard.xml new file mode 100644 index 0000000..1dd71df --- /dev/null +++ b/POET/Docs/Performance/LoopedFunctionStandard.xml @@ -0,0 +1,9 @@ + + + + + + \ No newline at end of file diff --git a/POET/Docs/Portability/DatabaseFunctionStandard.xml b/POET/Docs/Portability/DatabaseFunctionStandard.xml new file mode 100644 index 0000000..39440b8 --- /dev/null +++ b/POET/Docs/Portability/DatabaseFunctionStandard.xml @@ -0,0 +1,10 @@ + + + + moodle_database + and it is accessed by the $DB global variable. This layer takes care of supporting + various database backends. All interactions with the database must go through this class. + ]]> + + \ No newline at end of file diff --git a/POET/Docs/Portability/DatabasePrefixStandard.xml b/POET/Docs/Portability/DatabasePrefixStandard.xml new file mode 100644 index 0000000..c765924 --- /dev/null +++ b/POET/Docs/Portability/DatabasePrefixStandard.xml @@ -0,0 +1,23 @@ + + + + moodle_database class method or if using raw SQL, the table name should be surrounded by curly brackets. + ]]> + + + + get_records('user', ['id' => 1]); +$DB->get_records_sql('SELECT * FROM {user} WHERE id = ?', [1]); + ]]> + + + get_records('mdl_user', ['id' => 1]); +$DB->get_records_sql('SELECT * FROM mdl_user WHERE id = ?', [1]); + ]]> + + + \ No newline at end of file diff --git a/POET/Docs/Security/BadRequestStandard.xml b/POET/Docs/Security/BadRequestStandard.xml new file mode 100644 index 0000000..b84510b --- /dev/null +++ b/POET/Docs/Security/BadRequestStandard.xml @@ -0,0 +1,9 @@ + + + + + + \ No newline at end of file diff --git a/POET/Docs/Security/DisallowParamConstantsStandard.xml b/POET/Docs/Security/DisallowParamConstantsStandard.xml new file mode 100644 index 0000000..7ab900e --- /dev/null +++ b/POET/Docs/Security/DisallowParamConstantsStandard.xml @@ -0,0 +1,16 @@ + + + + + PARAM_ within Moodle are used for cleaning parameters. Deprecated constants should not be used. + Always try to use the most specific constant possible, EG: PARAM_TEXT instead of PARAM_CLEAN. + ]]> + + \ No newline at end of file diff --git a/POET/Docs/Security/IniSetStandard.xml b/POET/Docs/Security/IniSetStandard.xml new file mode 100644 index 0000000..d690142 --- /dev/null +++ b/POET/Docs/Security/IniSetStandard.xml @@ -0,0 +1,20 @@ + + + + ini_set PHP function. This can cause unexpected behavior. + ]]> + + + + + + + + + + \ No newline at end of file diff --git a/POET/Docs/Security/SuperglobalStandard.xml b/POET/Docs/Security/SuperglobalStandard.xml new file mode 100644 index 0000000..48d5f11 --- /dev/null +++ b/POET/Docs/Security/SuperglobalStandard.xml @@ -0,0 +1,22 @@ + + + + $_GET, $_POST, etc. + ]]> + + + + + + + + + + \ No newline at end of file diff --git a/POET/Docs/Security/WarnSqlFunctionsStandard.xml b/POET/Docs/Security/WarnSqlFunctionsStandard.xml new file mode 100644 index 0000000..2849b3a --- /dev/null +++ b/POET/Docs/Security/WarnSqlFunctionsStandard.xml @@ -0,0 +1,56 @@ + + + + + + + + + get_records_sql('SELECT * FROM {course} WHERE shortname = ?', [$get]); + ]]> + + + get_records_sql('SELECT * FROM {course} WHERE shortname = '.$get); + ]]> + + + + + get_records_sql(' + SELECT c.* + FROM {user_enrolments} ue + JOIN {enrol} e ON e.id = ue.enrolid + JOIN {course} c ON e.courseid = c.id + JOIN {user} u ON ue.userid = u.id + WHERE u.username = ? + AND u.mnethostid = ? +', [$USER->username, $CFG->mnet_localhost_id]); + ]]> + + + get_records_sql(' + SELECT c.* + FROM {user_enrolments} ue + JOIN {enrol} e ON e.id = ue.enrolid + JOIN {course} c ON e.courseid = c.id + JOIN {user} u ON ue.userid = u.id + WHERE u.username = ? +', [$USER->username]); + ]]> + + + diff --git a/POET/Docs/Vendor/DisallowJqueryStandard.xml b/POET/Docs/Vendor/DisallowJqueryStandard.xml new file mode 100755 index 0000000..9a88c2e --- /dev/null +++ b/POET/Docs/Vendor/DisallowJqueryStandard.xml @@ -0,0 +1,27 @@ + + + + + + + + + requires->jquery(); + ]]> + + + requires->js('/mod/foo/jquery.js'); + ]]> + + + diff --git a/POET/Sniffs/Performance/LoopedFunctionSniff.php b/POET/Sniffs/Performance/LoopedFunctionSniff.php index 325f6f4..979da1c 100644 --- a/POET/Sniffs/Performance/LoopedFunctionSniff.php +++ b/POET/Sniffs/Performance/LoopedFunctionSniff.php @@ -45,7 +45,48 @@ protected function processTokenWithinScope(PHP_CodeSniffer_File $phpcsFile, $sta $tokens = $phpcsFile->getTokens(); $name = $tokens[$stackPtr]['content']; // These Moodle function should not be called inside of a loop. - $slowfunctions = array('get_records', 'event_trigger'); + $slowfunctions = array( + 'execute', + 'get_recordset', + 'get_recordset_list', + 'get_recordset_select', + 'get_recordset_sql', + 'export_table_recordset', + 'get_records', + 'get_records_list', + 'get_records_select', + 'get_records_sql', + 'get_records_menu', + 'get_records_select_menu', + 'get_records_sql_menu', + 'get_record', + 'get_record_select', + 'get_record_sql', + 'get_field', + 'get_field_select', + 'get_field_sql', + 'get_fieldset_select', + 'get_fieldset_sql', + 'insert_record_raw', + 'insert_record', + 'insert_records', + 'import_record', + 'update_record_raw', + 'update_record', + 'set_field', + 'set_field_select', + 'count_records', + 'count_records_select', + 'count_records_sql', + 'record_exists', + 'record_exists_select', + 'record_exists_sql', + 'delete_records', + 'delete_records_list', + 'delete_records_select', + 'replace_all_text', + 'event_trigger', + ); foreach ($slowfunctions as $function) { if ($name === $function) { diff --git a/POET/Sniffs/Security/DisallowParamConstantsSniff.php b/POET/Sniffs/Security/DisallowParamConstantsSniff.php new file mode 100644 index 0000000..4f4a4cb --- /dev/null +++ b/POET/Sniffs/Security/DisallowParamConstantsSniff.php @@ -0,0 +1,88 @@ + + * @copyright 2015 Blackboard Inc. + * @license https://www.gnu.org/copyleft/gpl.html GPLv3 + */ + +/** + * A Sniff to warn about using deprecated PARAM_* constants in Moodle. + * + * @category PHP + * @author Corey Wallis + * @copyright 2015 Blackboard Inc. + * @license https://www.gnu.org/copyleft/gpl.html GPLv3 + */ +class POET_Sniffs_Security_DisallowParamConstantsSniff implements PHP_CodeSniffer_Sniff +{ + /** + * A list of parameter constants and their alternatives. + * + * @var array + */ + private $_constants = [ + 'PARAM_RAW' => '', + 'PARAM_CLEAN' => '', + 'PARAM_INTEGER' => 'PARAM_INT', + 'PARAM_NUMBER' => 'PARAM_FLOAT', + 'PARAM_ACTION' => 'PARAM_ALPHANUMEXT', + 'PARAM_FORMAT' => 'PARAM_ALPHANUMEXT', + 'PARAM_MULTILANG' => 'PARAM_TEXT', + 'PARAM_CLEANFILE' => 'PARAM_FILE', + ]; + + /** + * Return the token types that this Sniff is interested in + * + * @return array + */ + public function register() + { + return [T_STRING]; + } + + /** + * Processes the tokens that this sniff is interested in. + * + * @param PHP_CodeSniffer_File $phpcsFile The file where the token was found. + * @param int $stackPtr The position in the stack where + * the token was found. + * + * @return void + */ + public function process(PHP_CodeSniffer_File $phpcsFile, $stackPtr) + { + $tokens = $phpcsFile->getTokens(); + + $name = trim($tokens[$stackPtr]['content']); + + if (isset($this->_constants[$name]) === true) { + $this->addError($phpcsFile, $stackPtr, $name); + } + } + + /** + * Generates the error or warning for this sniff. + * + * @param PHP_CodeSniffer_File $phpcsFile The file being scanned. + * @param int $stackPtr The position of the forbidden function + * in the token array. + * @param string $param The name of the forbidden param. + * + * @return void + */ + protected function addError($phpcsFile, $stackPtr, $param) + { + $data = [$param]; + $error = 'The use of the constant %s is strongly discouraged.'; + + if (empty($this->_constants[$param]) === false) { + $error .= " Use '".$this->_constants[$param]."' instead."; + $phpcsFile->addError($error, $stackPtr, 'Discouraged', $data); + } else { + $phpcsFile->addWarning($error, $stackPtr, 'Discouraged', $data); + } + } +} \ No newline at end of file diff --git a/POET/Sniffs/Security/RawParamSniff.php b/POET/Sniffs/Security/RawParamSniff.php deleted file mode 100644 index e808f77..0000000 --- a/POET/Sniffs/Security/RawParamSniff.php +++ /dev/null @@ -1,51 +0,0 @@ - - * @copyright 2015 Remote-Learner, Inc. - * @license http://www.gnu.org/licenses/gpl-3.0.txt GNU GPLv3 - * @link http://git.remote-learner.net/private.cgi?p=codelibrary_scripts.git - */ - -/** - * POET_Sniffs_Security_RawParamSniff. - * - * PARAM_RAW performs no security checking or parameter cleansing at all on the data it collects. - * Generally speaking this is a bad idea and should only be used in cases where the data can be - * guaranteed to be harmless. That rarely happens, so PARAM_RAW should not be used in almost all - * cases. - * - * @author Tyler Bannister - * @copyright 2015 Remote-Learner, Inc. - * @license http://www.gnu.org/licenses/gpl-3.0.txt GNU GPLv3 - * @link http://git.remote-learner.net/private.cgi?p=codelibrary_scripts.git - */ -class POET_Sniffs_Security_RawParamSniff implements PHP_CodeSniffer_Sniff { - /** - * Returns an array of tokens this test wants to listen for. - * - * @return array The array of tokens to run this sniff on. - */ - public function register() { - return array(T_STRING); - } - - /** - * Check if this string is PARAM_RAW. - * - * @param PHP_CodeSniffer_File $phpcsFile The file being scanned. - * @param int $stackPtr The position of the current token in the stack passed in $tokens. - */ - public function process(PHP_CodeSniffer_File $phpcsFile, $stackPtr) { - $tokens = $phpcsFile->getTokens(); - $content = $tokens[$stackPtr]['content']; - - // If it's a superglobal, it's a potential security hole. - if ($content === 'PARAM_RAW') { - $error = "Potential security issue: %s detected."; - $data = array($content); - $phpcsFile->addWarning($error, $stackPtr, 'Superglobal', $data); - } - } -} diff --git a/POET/Sniffs/Security/VersionSniff.php b/POET/Sniffs/Security/VersionSniff.php new file mode 100755 index 0000000..ed8b71c --- /dev/null +++ b/POET/Sniffs/Security/VersionSniff.php @@ -0,0 +1,53 @@ + + * @copyright 2016 Remote-Learner, Inc. + * @license http://www.gnu.org/licenses/gpl-3.0.txt GNU GPLv3 + * @link http://git.remote-learner.net/private.cgi?p=codelibrary_scripts.git + */ + +if (class_exists('PHP_CodeSniffer_Standards_AbstractVariableSniff', true) === false) { + throw new PHP_CodeSniffer_Exception('Class PHP_CodeSniffer_Standards_AbstractVariableSniff not found'); +} + +/** + * POET_Sniffs_Security_VersionSniff. + * + * Checks the version file to ensure $module not being used as its deprecated. + * + * @author Derek Henderson + * @copyright 2016 Remote-Learner, Inc. + * @license http://www.gnu.org/licenses/gpl-3.0.txt GNU GPLv3 + * @link https://github.com/POETGroup/poet-coding-standard + */ +class POET_Sniffs_Security_VersionSniff implements PHP_CodeSniffer_Sniff { + + /** + * Returns an array of tokens this test wants to listen for. + * + * @return array The array of tokens to run this sniff on. + */ + public function register() { + return array(T_VARIABLE); + } + + /** + * Check if $module used in version.php. + * + * @param PHP_CodeSniffer_File $phpcsFile The file being scanned. + * @param int $stackPtr The position of the current token in the stack passed in $tokens. + */ + public function process(PHP_CodeSniffer_File $phpcsFile, $stackPtr) { + $fn = $phpcsFile->getFilename(); + if (strpos($fn,'version.php')){ + $tokens = $phpcsFile->getTokens(); + $content = $tokens[$stackPtr]['content']; + if ($content === '$module') { + $error = 'Use of "$module" detected in version.php. Module was dropped in M3.0 and should not be used.'; + $phpcsFile->addWarning($error, $stackPtr, 'MODULE'); + } + } + } +} \ No newline at end of file diff --git a/POET/Sniffs/Security/WarnSqlFunctionsSniff.php b/POET/Sniffs/Security/WarnSqlFunctionsSniff.php new file mode 100644 index 0000000..bfce400 --- /dev/null +++ b/POET/Sniffs/Security/WarnSqlFunctionsSniff.php @@ -0,0 +1,95 @@ + + * @copyright 2015 Blackboard Inc. + * @license https://www.gnu.org/copyleft/gpl.html GPLv3 + */ + +/** + * A Sniff to warn about the use of the _sql database functions + * + * @category PHP + * @author Corey Wallis + * @copyright 2015 Blackboard Inc. + * @license https://www.gnu.org/copyleft/gpl.html GPLv3 + */ +class POET_Sniffs_Security_WarnSqlFunctionsSniff implements PHP_CodeSniffer_Sniff +{ + /** + * Returns an array of tokens this test wants to listen for. + * + * @return array + */ + public function register() + { + return [T_VARIABLE]; + } + + /** + * Processes this sniff, when one of its tokens is encountered. + * + * @param PHP_CodeSniffer_File $phpcsFile The file being scanned. + * @param int $stackPtr The position of the current token in + * the stack passed in $tokens. + * + * @return void + */ + public function process(PHP_CodeSniffer_File $phpcsFile, $stackPtr) + { + $tokens = $phpcsFile->getTokens(); + + $content = $tokens[$stackPtr]['content']; + + // Only check for functions using the $DB variable. + if ($content !== '$DB') { + return; + } + + // Find the name of the function. + $functionPtr = $phpcsFile->findNext(T_STRING, $stackPtr, null, false, null, true); + + if ($functionPtr === false) { + return; // Failed to find the function call. + } + + // Add a warning for functions that take raw sql. + $function = $tokens[$functionPtr]['content']; + + $sqlPos = strrpos($function, '_sql'); + if ($sqlPos !== false && ($sqlPos + 4) === strlen($function)) { + $this->_addWarning($function, $phpcsFile, $stackPtr); + return; + } + + $menuPos = strrpos($function, '_sql_menu'); + if ($menuPos !== false && ($menuPos + 9) === strlen($function)) { + $this->_addWarning($function, $phpcsFile, $stackPtr); + return; + } + + if ($function === 'execute') { + $this->_addWarning($function, $phpcsFile, $stackPtr); + return; + } + } + + /** + * Add a warning for the found function + * + * @param string $function The name of the function found + * @param PHP_CodeSniffer_File $phpcsFile The file being scanned + * @param int $stackPtr The pointer to the element in the stack + * + * @return void + */ + private function _addWarning($function, $phpcsFile, $stackPtr) + { + $type = 'SqlFunctionUsed'; + $error = 'Use of the %s database function found. '; + $error .= 'Check the SQL for parameter injection / complexity.'; + $data = [$function]; + $phpcsFile->addWarning($error, $stackPtr, $type, $data); + } +} diff --git a/POET/Sniffs/Vendor/DisallowJquerySniff.php b/POET/Sniffs/Vendor/DisallowJquerySniff.php new file mode 100755 index 0000000..f081245 --- /dev/null +++ b/POET/Sniffs/Vendor/DisallowJquerySniff.php @@ -0,0 +1,70 @@ + + * @copyright 2015 Blackboard Inc. + * @license https://www.gnu.org/copyleft/gpl.html GPLv3 + */ + +/** + * A Sniff to warn about manually including the jQuery libraries + * as opposed to using the ones bundled with Moodle core. + * + * @author Corey Wallis + * @copyright 2015 Blackboard Inc. + * @license https://www.gnu.org/copyleft/gpl.html GPLv3 + */ +class POET_Sniffs_Vendor_DisallowJquerySniff implements PHP_CodeSniffer_Sniff +{ + /** + * Returns an array of tokens this test wants to listen for. + * + * @return array + */ + public function register() + { + return array( + T_CONSTANT_ENCAPSED_STRING, + T_DOUBLE_QUOTED_STRING, + ); + + } + + /** + * Processes this sniff, when one of its tokens is encountered. + * + * @param PHP_CodeSniffer_File $phpcsFile The file being scanned. + * @param int $stackPtr The position of the current token in + * the stack passed in $tokens. + * + * @return void + */ + public function process(PHP_CodeSniffer_File $phpcsFile, $stackPtr) + { + $tokens = $phpcsFile->getTokens(); + + $content = trim($tokens[$stackPtr]['content'], '\'"'); + + if (preg_match('/jquery(\.|-).+js$/i', $content) === 1) { + $this->_addError($phpcsFile, $stackPtr); + } + } + + /** + * Add an error for the found path + * + * @param PHP_CodeSniffer_File $phpcsFile The file being scanned + * @param int $stackPtr The pointer to the element in the stack + * + * @return void + */ + private function _addError($phpcsFile, $stackPtr) + { + $type = 'jQueryComponents'; + $error = 'Including jQuery et al. directly is strongly discouraged.'; + $data = array(); + $phpcsFile->addError($error, $stackPtr, $type, $data); + + } +} diff --git a/POET/Tests/Performance/LoopedFunctionUnitTest.inc b/POET/Tests/Performance/LoopedFunctionUnitTest.inc new file mode 100644 index 0000000..213ad72 --- /dev/null +++ b/POET/Tests/Performance/LoopedFunctionUnitTest.inc @@ -0,0 +1,66 @@ +get_records('testing'); + +foreach([1, 2, 3] as $value) { + // This line should cause a warning. + $DB->get_records('testing', ['id' => $value]); +} + +while (false) { + // This line should cause a warning. + $DB->get_records('testing'); +} + +for ($i = 0; $i < 1; $i++) { + // This line should cause a warning. + $DB->get_records('testing', ['id' => $i]); +} + +do { + // This line should cause a warning. + $DB->get_records('testing'); +} while (false); + +while (false) { + // All of these should cause a warning. + $DB->execute(); + $DB->get_recordset(); + $DB->get_recordset_list(); + $DB->get_recordset_select(); + $DB->get_recordset_sql(); + $DB->export_table_recordset(); + $DB->get_records_list(); + $DB->get_records_select(); + $DB->get_records_sql(); + $DB->get_records_menu(); + $DB->get_records_select_menu(); + $DB->get_records_sql_menu(); + $DB->get_record(); + $DB->get_record_select(); + $DB->get_record_sql(); + $DB->get_field(); + $DB->get_field_select(); + $DB->get_field_sql(); + $DB->get_fieldset_select(); + $DB->get_fieldset_sql(); + $DB->insert_record_raw(); + $DB->insert_record(); + $DB->insert_records(); + $DB->import_record(); + $DB->update_record_raw(); + $DB->update_record(); + $DB->set_field(); + $DB->set_field_select(); + $DB->count_records(); + $DB->count_records_select(); + $DB->count_records_sql(); + $DB->record_exists(); + $DB->record_exists_select(); + $DB->record_exists_sql(); + $DB->delete_records(); + $DB->delete_records_list(); + $DB->delete_records_select(); + $DB->replace_all_text(); +} \ No newline at end of file diff --git a/POET/Tests/Performance/LoopedFunctionUnitTest.php b/POET/Tests/Performance/LoopedFunctionUnitTest.php new file mode 100644 index 0000000..a2f4e84 --- /dev/null +++ b/POET/Tests/Performance/LoopedFunctionUnitTest.php @@ -0,0 +1,51 @@ + int) + */ + protected function getErrorList() + { + return []; + } + + /** + * Returns the lines where warnings should occur. + * + * The key of the array should represent the line number and the value + * should represent the number of warnings that should occur on that line. + * + * @return array(int => int) + */ + protected function getWarningList() + { + $warnings = [ + 8 => 1, + 13 => 1, + 18 => 1, + 23 => 1, + ]; + + // Warnings for the long list of function calls at the end of the .inc file. + for ($i = 28; $i <= 65; $i++) { + $warnings[$i] = 1; + } + + return $warnings; + } +} \ No newline at end of file diff --git a/POET/Tests/Security/DisallowParamConstantsUnitTest.inc b/POET/Tests/Security/DisallowParamConstantsUnitTest.inc new file mode 100644 index 0000000..d02d21c --- /dev/null +++ b/POET/Tests/Security/DisallowParamConstantsUnitTest.inc @@ -0,0 +1,36 @@ +_form; + +$mform->addElement('text', 'name', get_string('name', 'mymod'), ['size' => '64']); +$mform->setType('name', PARAM_RAW); + +$mform->addElement('text', 'name', get_string('name', 'mymod'), ['size' => '64']); +$mform->setType('name', PARAM_CLEAN); + +$mform->addElement('text', 'name', get_string('name', 'mymod'), ['size' => '64']); +$mform->setType('name', PARAM_INTEGER); + +$mform->addElement('text', 'name', get_string('name', 'mymod'), ['size' => '64']); +$mform->setType('name', PARAM_NUMBER); + +$mform->addElement('text', 'name', get_string('name', 'mymod'), ['size' => '64']); +$mform->setType('name', PARAM_ACTION); + +$mform->addElement('text', 'name', get_string('name', 'mymod'), ['size' => '64']); +$mform->setType('name', PARAM_FORMAT); + +$mform->addElement('text', 'name', get_string('name', 'mymod'), ['size' => '64']); +$mform->setType('name', PARAM_MULTILANG); + +$mform->addElement('text', 'name', get_string('name', 'mymod'), ['size' => '64']); +$mform->setType('name', PARAM_CLEANFILE); + +$id = optional_param('id', '', PARAM_TEXT); +$id = optional_param('id', '', PARAM_RAW); +$id = optional_param('id', '', PARAM_CLEAN); +$id = optional_param('id', '', PARAM_INTEGER); +$id = optional_param('id', '', PARAM_NUMBER); +$id = optional_param('id', '', PARAM_ACTION); +$id = optional_param('id', '', PARAM_FORMAT); +$id = optional_param('id', '', PARAM_MULTILANG); +$id = optional_param('id', '', PARAM_CLEANFILE); \ No newline at end of file diff --git a/POET/Tests/Security/DisallowParamConstantsUnitTest.php b/POET/Tests/Security/DisallowParamConstantsUnitTest.php new file mode 100644 index 0000000..8e4609e --- /dev/null +++ b/POET/Tests/Security/DisallowParamConstantsUnitTest.php @@ -0,0 +1,63 @@ + + * @copyright 2015 Blackboard Inc. + * @license https://www.gnu.org/copyleft/gpl.html GPLv3 + */ + +/** + * A unit test for the DisallowParamConstantsSniff + * + * @author Corey Wallis + * @copyright 2015 Blackboard Inc. + * @license https://www.gnu.org/copyleft/gpl.html GPLv3 + * @group poet + */ +class POET_Tests_Security_DisallowParamConstantsUnitTest extends AbstractSniffUnitTest +{ + /** + * Returns the lines where errors should occur. + * + * The key of the array should represent the line number and the value + * should represent the number of errors that should occur on that line. + * + * @return array + */ + public function getErrorList() + { + return [ + 11 => 1, + 14 => 1, + 17 => 1, + 20 => 1, + 23 => 1, + 26 => 1, + 31 => 1, + 32 => 1, + 33 => 1, + 34 => 1, + 35 => 1, + 36 => 1, + ]; + } + + /** + * Returns the lines where warnings should occur. + * + * The key of the array should represent the line number and the value + * should represent the number of warnings that should occur on that line. + * + * @return array + */ + public function getWarningList() + { + return [ + 5 => 1, + 8 => 1, + 29 => 1, + 30 => 1, + ]; + } +} \ No newline at end of file diff --git a/POET/Tests/Security/WarnSqlFunctionsUnitTest.inc b/POET/Tests/Security/WarnSqlFunctionsUnitTest.inc new file mode 100644 index 0000000..09ac813 --- /dev/null +++ b/POET/Tests/Security/WarnSqlFunctionsUnitTest.inc @@ -0,0 +1,24 @@ +get_record('user', array('id'=>'1')); +$user = $DB->get_record_sql('SELECT * FROM {user} WHERE id = ?', array(1)); + +/// Question mark placeholders: +$DB->get_record_sql('SELECT * FROM {user} WHERE firstname = ? AND lastname = ?', + array('James', 'Bond')); + +/// Named placeholders: +$DB->get_record_sql('SELECT * FROM {user} WHERE firstname = :firstname AND lastname = :lastname', + array('firstname'=>'James', 'lastname'=>'Bond')); + +$DB->get_records_sql($sql, $params, $limitfrom, $limitnum); +$DB->get_records_sql_menu($sql, $params, $limitfrom, $limitnum); +$DB->count_records_sql($sql, $params); +$DB->record_exists_sql($sql, $params); +$DB->get_field_sql($sql, $params, $strictness); +$DB->get_fieldset_sql($sql, $params); +$DB->execute($sql, $parms); +$DB->get_recordset_sql($sql, $params, $limitfrom, $limitnum); + +// Should not cause a warning. +$DB->set_field('table', 'new_field', 'new_value'); +$DB->get_field('table', 'field'); diff --git a/POET/Tests/Security/WarnSqlFunctionsUnitTest.php b/POET/Tests/Security/WarnSqlFunctionsUnitTest.php new file mode 100644 index 0000000..745c136 --- /dev/null +++ b/POET/Tests/Security/WarnSqlFunctionsUnitTest.php @@ -0,0 +1,57 @@ + + * @copyright 2015 Blackboard Inc. + * @license https://www.gnu.org/copyleft/gpl.html GPLv3 + */ + +/** + * A unit test for the WarnSqlFunctionsSniff + * + * @author Corey Wallis + * @copyright 2015 Blackboard Inc. + * @license https://www.gnu.org/copyleft/gpl.html GPLv3 + * @group poet + */ +class POET_Tests_Security_WarnSqlFunctionsUnitTest extends AbstractSniffUnitTest +{ + /** + * Returns the lines where errors should occur. + * + * The key of the array should represent the line number and the value + * should represent the number of errors that should occur on that line. + * + * @return array + */ + public function getErrorList() + { + return array(); + } + + /** + * Returns the lines where warnings should occur. + * + * The key of the array should represent the line number and the value + * should represent the number of warnings that should occur on that line. + * + * @return array + */ + public function getWarningList() + { + return array( + 3 => 1, + 6 => 1, + 10 => 1, + 13 => 1, + 14 => 1, + 15 => 1, + 16 => 1, + 17 => 1, + 18 => 1, + 19 => 1, + 20 => 1, + ); + } +} diff --git a/POET/Tests/Vendor/DisallowJqueryUnitTest.inc b/POET/Tests/Vendor/DisallowJqueryUnitTest.inc new file mode 100755 index 0000000..81470e4 --- /dev/null +++ b/POET/Tests/Vendor/DisallowJqueryUnitTest.inc @@ -0,0 +1,17 @@ +wwwroot . '/js/jquery-2.1.4.min.js'; +$path = "{$CFG->wwwroot}/jquery-2.1.4.min.js"; diff --git a/POET/Tests/Vendor/DisallowJqueryUnitTest.php b/POET/Tests/Vendor/DisallowJqueryUnitTest.php new file mode 100755 index 0000000..2000ab0 --- /dev/null +++ b/POET/Tests/Vendor/DisallowJqueryUnitTest.php @@ -0,0 +1,52 @@ + + * @copyright 2015 Blackboard Inc. + * @license https://www.gnu.org/copyleft/gpl.html GPLv3 + */ + +/** + * A unit test for the DisallowJquerySniff + * + * @author Corey Wallis + * @copyright 2015 Blackboard Inc. + * @license https://www.gnu.org/copyleft/gpl.html GPLv3 + * @group poet + */ +class POET_Tests_Vendor_DisallowJqueryUnitTest extends AbstractSniffUnitTest +{ + /** + * Returns the lines where errors should occur. + * + * The key of the array should represent the line number and the value + * should represent the number of errors that should occur on that line. + * + * @return array + */ + public function getErrorList() + { + return array( + 3 => 1, + 4 => 1, + 5 => 1, + 10 => 1, + 11 => 1, + 12 => 1, + 16 => 1, + 17 => 1, + ); + } + + /** + * Returns the lines where warnings should occur. + * + * The key of the array should represent the line number and the value + * should represent the number of warnings that should occur on that line. + * + * @return array + */ + public function getWarningList() + { + return array(); + } +} diff --git a/POET/ruleset.xml b/POET/ruleset.xml index da314f2..f2fd93e 100644 --- a/POET/ruleset.xml +++ b/POET/ruleset.xml @@ -3,19 +3,38 @@ The POET plugin verification standard. + + warning + - + + warning + + + warning + + + + - - - - + + warning + + + + + + + + + + + - - - + + \ No newline at end of file diff --git a/README.md b/README.md index 251a8ce..64d9271 100644 --- a/README.md +++ b/README.md @@ -1,36 +1,9 @@ # Introduction -This project is simply a way to package up and distribute the POET Coding Standard which is defined in the -following project: [poet-coding-standard](https://github.com/poetgroup/poet-coding-standard) - -This project does not attempt to do anything else. If there are problems with the standard, then the problems should -be addressed in the `poet-coding-standard` project. Once the problem is fixed, it can be synced from -`poet-coding-standard` into this project. - -The sniffs check for the following: - - the presence of ini_set in PHP files; - - unconditional if statements; - - FixMe or ToDo in code; - - Ensuring the file ends with a newline; - - byte order marks that may corrupt application work; - - ensure each if statement is on a line by itself; - - checks for no space after cast tokens; - - all php keywords are lower case; - - displays a message when any code prefixed with an ampersand is encountered; - - checks to ensure that a file that declares new symbols does not cause any side effects; - - arrays conform to the coding standard; - - checks for the logical operators 'and' and 'or'; - - checks for alias and discouraged functions that are kept in php for compatability with older versions; - - checks for eval; - - looks for code that can never be executed; - - looks for double quotes; - - checks for slow functions in a loop; - - checks for database portability; - - looks for the use of RAWPARAM; - - looks for the use of Superglobals; - - +This is the POET Group's coding standard. This standard is primarily focused on assisting with code reviews. For +example, warning the code reviewer about potential security or performance problems. +Please visit the [coding standard documentation](POET.md) for further details. # Install @@ -49,10 +22,27 @@ the CodeSniffer command and the path to the `moodle` directory of this project: vendor/bin/phpcs --standard=POET /path/to/moodle/plugin ``` -# Credits +# Testing + +In order to run the unit tests, you must ensure that you install from source otherwise, testing code from +PHP_CodeSniffer would be missing. Here is an example of re-installing dependencies and running tests: + +``` +rm -rf vendor/ +composer install --prefer-source +vendor/bin/phpunit +``` -All praise should go to the contributors of -[poet-coding-standard](https://github.com/poetgroup/poet-coding-standard) +Please also know that any **new** tests added need the `@group poet` annotation added. This ensures that only tests +from the `POET` standard are run. + +# Documenting + +To update the standard's documentation, use the following command: + +``` +vendor/bin/phpcs --standard=POET --generator=markdown > POET.md +``` # License diff --git a/composer.json b/composer.json index 510f16d..389c51f 100644 --- a/composer.json +++ b/composer.json @@ -10,5 +10,24 @@ "require": { "php": ">=5.4.0", "squizlabs/php_codesniffer": "^2" + }, + "require-dev": { + "phpunit/phpunit": "^4.7" + }, + "scripts": { + "config-phpcs": [ + "vendor/bin/phpcs --config-set installed_paths ../../..", + "vendor/bin/phpcs --config-set default_standard POET", + "vendor/bin/phpcs --config-set show_progress 1", + "vendor/bin/phpcs --config-set colors 1", + "vendor/bin/phpcs --config-set report_width auto", + "vendor/bin/phpcs --config-set encoding utf-8" + ], + "post-install-cmd": [ + "@config-phpcs" + ], + "post-update-cmd": [ + "@config-phpcs" + ] } } diff --git a/phpunit.xml.dist b/phpunit.xml.dist new file mode 100644 index 0000000..7e15135 --- /dev/null +++ b/phpunit.xml.dist @@ -0,0 +1,21 @@ + + + + + ./vendor/squizlabs/php_codesniffer/tests/AllTests.php + + + + + POET + + POET/Tests + + + + + + poet + + + \ No newline at end of file