Userpages v1.0 Review

[ZiNgA BuRgA]

This is a plugin "review" requested by euantor.

Overall, the plugin is fairly solid - with a small XSS vulnerability with the userpage editor.


Anyway, here's my points:

• The install function:
  
  PHP Code:

  function userpages_install() { global $mybb, $db, $cache; $db->query("ALTER TABLE `".TABLE_PREFIX."users` ADD `userpage` TEXT"); $db->write_query("ALTER TABLE `".TABLE_PREFIX."usergroups` ADD `canuserpage` INT(1) NOT NULL DEFAULT '0', ADD `canuserpageedit` INT(1) NOT NULL DEFAULT '0', ADD `canuserpagemod` INT(1) NOT NULL DEFAULT '0';");

  ---

  I'm not sure about the mix of query() and write_query() there.  Ultimately, I don't think anyone has a setup on which there's any difference between the two, so I don't really care, but IMO, one should at least be consistent.
  Also, the 'userpage' column doesn't seem to be using NULL values, so probably should be declared as NOT NULL.
  On a related note, the check in userpages_main()
  

  PHP Code:

  if ($userpage['userpage'] !== "") {

  ---

  will evaluate to true if the value is null (ie, user never set a page) or if $userpage itself is null (invalid uid supplied in the URL).  The latter case isn't so much of an issue here, though it probably should error out if an invalid uid is supplied, rather than display a blank user page.  The former case will display a user page link even if none has been set.

• Ideally, the setting inserts' title/descriptions should be run through $db->escape_string, although probably not an issue in most cases.
  I'd personally have settings and template insertion done in the install/uninstall routines, and the template modification left in the activate/deactivate routines, but I guess I'll leave that decision to the author.

• PHP Code:

  1 2 3 4 5 6 7 8

  function userpages_edit_group_do() { global $updated_group, $mybb; $updated_group['canuserpage'] = $mybb->input['canuserpage']; $updated_group['canuserpageedit'] = $mybb->input['canuserpageedit']; $updated_group['canuserpagemod'] = $mybb->input['canuserpagemod']; }

  ---

  Although this is in the AdminCP, I'd probably still like to see sanitisation here.  A simple intval should be suffice.

• PHP Code:

  1 2 3 4 5 6 7

  function userpages_usercpmenu() { global $db, $mybb, $templates, $theme, $usercpmenu, $lang, $collapsed, $collapsedimg, $lang, $cache; require_once MYBB_ROOT."/inc/class_datacache.php"; $cache = new datacache; $cache->cache();

  ---

  I don't know why the $cache is being reconstructed here - one really shouldn't be doing this.

• PHP Code:

  $currentuserpage = $db->fetch_field($db->simple_select("users", "userpage", "uid = ".$mybb->user['uid']), "userpage"); eval("\$page = \"".$templates->get('userpages_usercp_main')."\";");

  ---

  $currentuserpage needs to be run through htmlspecialchars before being displayed.  Same thing with the ModCP editor.

• PHP Code:

  1 2 3 4 5 6 7 8 9 10 11

  function userpages_main() { global $mybb, $db, $memprofile, $lang, $cache, $userpage_parser, $templates, $theme, $headerinclude, $header, $footer, $page, $parser, $userpagelink; $lang->load('userpages'); $usergroups_cache = $cache->read("usergroups"); if (!$usergroups_cache[$mybb->user['usergroup']]['canuserpage']) { error_no_permission(); }

  ---

  Careful!  The action isn't checked before the permission checking.  In other words, if the user doesn't have permission to view user pages, they also won't be able to view profiles.

• PHP Code:

  $query = $db->simple_select("users", "uid, username, usergroup", "userpage != ''", array( 'limit' => (($page-1)*10).", 10"));

  ---

  MyBB's simple_select() limit has two components, 'limit' and 'limit_start', which probably should be used instead of the comma separation here.

• There's also a few places where & is used instead of & in links, such as the following:
  
  PHP Code:

  $user['edituserpagelink'] = $mybb->settings['bburl']."/modcp.php?action=userpages_edit&uid=".$user['uid'];

  ---

• One would suggest stating compatibility with MyBB 1.6 only in the plugin file itself (not mentioned in the readme either).
• Finally, none of the added template calls are cached, meaning that the $template->get() calls each add an additional query.
• Also, the junk files ending with ~ probably should be removed from the package.
  

The plugin is otherwise implemented quite nicely and permission checking done reasonably well IMO.