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

[elij]

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.";
        }

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).