On Wed, May 11, 2011 at 11:54:34AM -0700, elij wrote:
> 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.";
> }
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.
