Ticket #810 (closed defect: fixed)

Opened 6 years ago

Last modified 3 years ago

Filter on username can't work for registered users

Reported by: JavaWoman Owned by: BrianKoontz
Priority: normal Milestone: 1.3.1
Component: unspecified Version: 1.1.6.5
Severity: normal Keywords: trunk-ported
Cc:

Description

Both LoadRecentComments() and LoadRecentlyCommented() have an option to filter on comments by a specified user (by name) only. For admins, this works, for registered user it can't, although this is clearly intended to work. (I think this bug has been around since 1.1.6.4.)

The reason is that these functions expect a username as parameter, but the check for a registered user compares this to the user array in the session. That comparison will always fail, so a registered user will not be able to specify to see only their own comments.

The root cause is a sloppy use of parameter names: sometimes $user is used to signify the whole array of user data (as stored in the DB and the session of a logged-in user), and sometimes to signify just a username. The best defense against this is to always use '$username' when a function (etc.) actually wants just a name, and always use '$user' when a function expects the whole data array.

The fix in these two functions is to:

  1. use $username as the parameter name
  2. use the following construct like this to filter on a specific user only:
    	$curuser = $this->GetUser();
    	if (!empty($username) && ($username == $curuser['name'] || $this->IsAdmin()))
    	{
    		$where = "WHERE `user` = '".mysql_real_escape_string($username)."' ";
    	}
    

Change History

Changed 6 years ago by BrianKoontz

The intent of these two functions is not clear to me. It appears that we restrict users that are not admins to only those comments that belong to them. Is there some security issue involved with allowing users to view other users' comments? It's not as if they can't view the same comments on the actual page if they already have appropriate "comment read" access.

Changed 6 years ago by BrianKoontz

I should add that non-admin users can already display all comments (including those not belonging to them) by simply not specifying a GET "user" param.

Changed 6 years ago by BrianKoontz

  • owner changed from unassigned to BrianKoontz
  • status changed from new to assigned

Changed 6 years ago by BrianKoontz

  • status changed from assigned to testing

Changed 6 years ago by BrianKoontz

(In [1248]) Enabled the use of the "user" GET param for LoadRecentComments() and LoadRecentlyCommented() for logged-in users. Removed restriction that logged-in users can filter only their own comments (as this makes no sense, since they can get a list of all comments by default). Refs #810.

Changed 5 years ago by DarTar

  • keywords 1.1.7-unported added

Changed 5 years ago by DarTar

  • status changed from testing to assigned

this does work for me as an admin and regular registered users, it doesn't work as an anonymous user (tested calling wikka.php?wakka=RecentComments&user=DarTar)

To make this useful I think we should add an extra optional parameter to {{recentcomments}} and {{recentlycommented}} (and possibly {{recentchanges}}).

We should also document this, if enabled in 1.1.6.6 , see #845

Changed 5 years ago by BrianKoontz

this does work for me as an admin and regular registered users, it doesn't work as an anonymous user (tested calling wikka.php?wakka=RecentComments&user=DarTar)

This is by design (see description for [1248]).

Changed 5 years ago by BrianKoontz

  • status changed from assigned to closed
  • resolution set to invalid

Changed 5 years ago by BrianKoontz

  • keywords trunk-unported added; 1.1.7-unported removed

Changed 5 years ago by BrianKoontz

(In [1339]) Deprecated 'tag' arg in RecentlyCommented() and RecentComments(); added new 'user' arg to bring trunk up-to-date with 1.1.6.6; modified comment actions to support new arg. Refs #810.

Changed 5 years ago by BrianKoontz

  • keywords trunk-ported added; trunk-unported removed
  • status changed from closed to reopened
  • resolution invalid deleted
  • milestone changed from 1.1.6.6 to 1.3

Changed 5 years ago by BrianKoontz

  • status changed from reopened to assigned

Changed 5 years ago by BrianKoontz

  • status changed from assigned to testing

Changed 4 years ago by DotMG

  • status changed from testing to commit

Changed 4 years ago by BrianKoontz

  • status changed from commit to closed
  • resolution set to fixed

Changed 3 years ago by BrianKoontz

  • milestone changed from 1.3 to 1.3.1

Changed milestone to 1.3.1

Note: See TracTickets for help on using tickets.