Thread Rating:
  • 0 Votes - 0 Average
  • 1
  • 2
  • 3
  • 4
  • 5
Userpages v1.0 Review

Please note that this is pretty much a negative criticism post, rather than a balanced review as mentioned in this thread. Also be aware that stuff posted here may be highly subjective.
Please feel free to criticise this post, however.

Plugin Reviewed: Userpages for MyBB
Plugin Version: 1.0 (last updated 7th June 2011)
Plugin Author: euantor
Author Message
ZiNgA BuRgA Offline
Fag
*******
Posts: 3,357
Joined: Jan 2008
Post: #1
Userpages v1.0 Review
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.

My Blog
06-13-2011 11:00 AM
Find all posts by this user Quote this message in a reply
euantor Offline
Junior Member
**
Posts: 14
Joined: Jan 2011
Post: #2
RE: Userpages v1.0 Review
Thanks very much Smile I'll make all the suggested fixes later today. I hadn't noticed the junk files till now - they're autosaves from gedit I believe.

Edit: Just looking through and I don't believe I should be using htmlspecialchars though, right? I already use the MyBB post parser and that should run htmlspecialchars should HTML be disabled, right? I also allow the use of HTML if the setting is on.
(This post was last modified: 06-15-2011 03:48 AM by euantor.)
06-14-2011 05:24 PM
Find all posts by this user Quote this message in a reply
ZiNgA BuRgA Offline
Fag
*******
Posts: 3,357
Joined: Jan 2008
Post: #3
RE: Userpages v1.0 Review
(06-14-2011 05:24 PM)euantor Wrote:  Edit: Just looking through and I don't believe I should be using htmlspecialchars though, right? I already use the MyBB post parser and that should run htmlspecialchars should HTML be disabled, right? I also allow the use of HTML if the setting is on.
Paste the following code in your userpage:

Code:
hi
</textarea>
<script type="text/javascript"> alert('hi'); </script>

Then go to edit your user page from the ModCP.

Also a quick look at your newer version (note, I'm not looking through everything), this is incorrect:

PHP Code:
1
2
3
4
5
6
7
8
9
	global $mybb, $db, $lang, $cache, $page, $templates, $theme, $headerinclude, $header, $footer, $usercpnav, $smilieinserter, $codebuttons, $currentuserpage, $templatelist;

	$lang->load('userpages');
	
	$usergroups_cache = $cache->read("usergroups");
	
	$templatelsit .= ",userpages_usercp_main";
	
	$templates->cache($db->escape_string($templatelist));

1) typo "templatelsit"
2) templates need to be cached from global_start (or earler)
3) caching one template will use a query, pointless, since the template->get call will automatically query anyway (though this will make the stats look a little better)
4) templates->cache overwrites all cached templates, so since you're pulling the entire templatelist, all dynamic modifications will be overwritten (plus it's unnecessary overhead)
5) I know MyBB does it, but $db->escape_string here is pointless and incorrect


My Blog
(This post was last modified: 06-16-2011 09:18 AM by ZiNgA BuRgA.)
06-16-2011 09:17 AM
Find all posts by this user Quote this message in a reply
euantor Offline
Junior Member
**
Posts: 14
Joined: Jan 2011
Post: #4
RE: Userpages v1.0 Review
Ok, thanks once again Yumi Smile I'll fix those issues too now. When I'm caching the templates at the global_start hook, do I just cache all of them?

Fixed the XSS vulnerability I think. Now to sort the other issues - shouldn't be too hard. Thanks for all the help so far!
(This post was last modified: 06-16-2011 05:30 PM by euantor.)
06-16-2011 05:19 PM
Find all posts by this user Quote this message in a reply
ZiNgA BuRgA Offline
Fag
*******
Posts: 3,357
Joined: Jan 2008
Post: #5
RE: Userpages v1.0 Review
(06-16-2011 05:19 PM)euantor Wrote:  When I'm caching the templates at the global_start hook, do I just cache all of them?
Up to you.  I personally would check $current_page and $mybb->input to determine which ones to add to the cache.

My Blog
06-17-2011 08:29 AM
Find all posts by this user Quote this message in a reply
euantor Offline
Junior Member
**
Posts: 14
Joined: Jan 2011
Post: #6
RE: Userpages v1.0 Review
I just cached all of them at the global_start - there aren't a great number of them to cache anyway. Thankyou so much for the review, it's helped a lot.
06-17-2011 07:57 PM
Find all posts by this user Quote this message in a reply


Forum Jump: