[aur-dev] [PATCH 3/4] fix case where user does not exist

[Lukas Fleischer]

On Wed, May 11, 2011 at 11:54:34AM -0700, elij wrote:
> On Wed, May 11, 2011 at 7:22 AM, Lukas Fleischer
> <archlinux at cryptocrack.de> wrote:
> > On Tue, May 10, 2011 at 09:01:29PM -0700, elij wrote:
> >> the query was being performed when $id was not set, resulting in an
> >> invalid sql query being performed.
> >> ---
> >>  web/lib/acctfuncs.inc |    3 +++
> >>  1 files changed, 3 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/web/lib/acctfuncs.inc b/web/lib/acctfuncs.inc
> >> index 5bcff8b..b2f0548 100644
> >> --- a/web/lib/acctfuncs.inc
> >> +++ b/web/lib/acctfuncs.inc
> >> @@ -786,6 +786,9 @@ function valid_passwd( $userID, $passwd )
> >>   */
> >>  function user_suspended( $id )
> >>  {
> >> +     if (!$id) {
> >> +             return false;
> >> +     }
> >>       $dbh = db_connect();
> >>       $q = "SELECT Suspended FROM Users WHERE ID = " . $id;
> >>       $result = db_query($q, $dbh);
> >
> > Looks ok, but I'd rather say we should locate the code path that led to
> > the unset parameter and add some additional validation there to avoid
> > further unexpected behaviour.
> 
> The source is in try_login (also in lib/acctfuncs.inc):
> 
>     if ( isset($_REQUEST['user']) || isset($_REQUEST['passwd']) ) {
>         $userID = valid_user($_REQUEST['user']);
>         if ( user_suspended( $userID ) ) {
>             $login_error = "Account Suspended.";
>         }

Thanks. I will look into that.

> valid_user (also in the same file) can return (no value) in some cases.
> that large if/elseif block in try_login could probably have some more
> conditions added to test for existence of $userID before calling
> user_suspended on it. Still.. it might make sense to have a guard (the
> test added with this patch) in the function itself in case usage of it
> down the road changes (another code path or something).

Yeah, will be pushed.