Actions

User

Difference between revisions of "Aaronw/Code review guidelines"

From Mahara Wiki

< User:Aaronw
(Created page with "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 thro…")
 
Line 1: Line 1:
 
Things to look for in code review:
 
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.
+
* '''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 <tt>&lt;script type="text/javascript">alert('pwned');&lt;/script></tt> into newly added input fields, or supplying it as a query parameter.
+
** '''Check all input fields for injection vulnerabilities''': At the least, try pasting <tt>&lt;script type="text/javascript">alert('pwned');&lt;/script></tt> 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.
+
* '''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.
+
* '''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.
+
* '''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.
+
* '''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:
+
* '''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 imported/exported to Leap2a format?
** Can it be expored to HTML format (and imported, if appropriate?)
+
** 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)?
+
** 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)
+
** 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'''
+
* '''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.
+
** 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.
+
** 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 <tt>htdocs/lib/upgrade.php</tt>. Usually <tt>core_postinst()</tt> for config settings and database structure details that aren't supported by install.xml; and <tt>core_install_lastcoredata_defaults()</tt> to insert default records into new tables.
+
* '''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 <tt>htdocs/lib/upgrade.php</tt>. Usually <tt>core_postinst()</tt> for config settings and database structure details that aren't supported by install.xml; and <tt>core_install_lastcoredata_defaults()</tt> to insert default records into new tables.

Revision as of 13:44, 5 August 2013

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.