User:Aaronw/Code review guidelines

Things to look for in code review:

• Improperly whitelisted or cleaned user input: $_GET, $_POST, $_REQUEST, and $_COOKIE are dangerous. All input should be obtained through the Mahara input API, and/or it should be sanitized in another appropriate way. If it's not sanitized, there should be a comment in the code explaining why it was not.
  • Check all input fields for injection vulnerabilities: At the least, try pasting <script type="text/javascript">alert('pwned');</script> into newly added input fields, or supplying it as a query parameter.
• Hard-coded language strings: There should be no language strings coded into the PHP itself. They should all be in lang files.
• Improperly pluralized language strings: Language strings of the form "N items" should use the fancy plural language string format, where the string is an array. See the language strings API for the details.
• Hard-coded HTML in the PHP: There should be no HTML in the PHP scripts or in the lang file (under most circumstances). It should almost always go in the dwoo templates.
• Uploading files to $wwwroot: Uploaded files must ALWAYS be stored under $dataroot, not $wwwroot. If you need to load them in the web browser, you must do so through a PHP script that reads the files and reprints it, like thumbnail.php.
• New content types: If the user is adding new things that can go on a page, check for this:
  • Can it be imported/exported to Leap2a format?
  • Can it be expored to HTML format (and imported, if appropriate?)
  • Does it respect the Page locking feature (when you submit a page to a group)?
  • If it includes file attachments, are they handled in a way consistent with other file attachments in Mahara (for example the way the Blog content type handles file attachments)
• New admin options
  • All new admin options should have a web UI, or should have a line (commented or uncommented) in config-defaults.php or config-dist.php.
  • Admin options that could be used for a privilege escalation should NOT have a web UI. They should be in config-defaults.php or config-dist.php. This is in acknowledgement that it is easier to steal an admin's web login through browser and OS vulnerabilities, than to gain filesystem access on the web server.
• Make sure the install matches the upgrade: Make sure that anything added to a db/upgrade.php script also gets done on a clean install, and vice versa. The db/upgrade.php script DOES NOT RUN during installation! New database structures on installation are created by parsing install.xml. DML for installation should be put in one of the pre- or post-installation functions in htdocs/lib/upgrade.php. Usually core_postinst() for config settings and database structure details that aren't supported by install.xml; and core_install_lastcoredata_defaults() to insert default records into new tables.
• Check that versions are incremented properly: Versions in version.php files should only be incremented if there are changes to the database (i.e. a new section in upgrade.php).
  • Patches to a stable branch (e.g. 1.7_STABLE) should increment the version number by 1. For example, $cfg->2008012400 becomes $cfg->2008012401
  • Patches to the master branch (which reflects the latest dev code that hasn't been released yet) should increment the date part of the version number, usually to the day the patch was commited, and set the numeric part back to 0. For example, $cfg->2008120411 becomes $cfg->2013072600
    • If you have multiple master patches on the same day, you can increment the numeric part of the patch.
  • See the Version Numbering Policy page for an explanation of why we do it this way.