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