On Wed, May 11, 2011 at 7:22 AM, Lukas Fleischer <archli...@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).