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.