[aur-dev] Couple of patches

elij

11 May 2011 11 May '11

4:01 a.m.

Here are a few patches. The first patch removes the 'submitter' field from the web-ui package results. The second patch fixes several instances where db_query's result was not tested before performing mysql queries upon it. This patch might be a tad messy due to some unfortunate 'vim retab!' foolery. A result of forgetting to set noexpandtab before editing and writing out some changes. Mostly just a silly reformatting of some document strings. If the rest of the patch is wanted, I can rebase and re-edit this particular patch if desired. The third patch fixes an odd case I ran into where a user didn't exist, but a query was being performed anyway -- and relying on the sql failure to handle the lack of account. The final patch was just fixing an inconsistency I ran across, where depending on the error state, either a zero or an empty string was returned. php evaluates both to false, so it doesn't _really_ matter, but consistency is nice. :)

Show replies by date

elij

11 May 11 May

4:01 a.m.

New subject: [aur-dev] [PATCH 1/4] remove submitter from package data

--- web/html/pkgsubmit.php | 3 +-- web/lib/pkgfuncs.inc | 10 +--------- web/template/pkg_details.php | 11 ----------- 3 files changed, 2 insertions(+), 22 deletions(-) diff --git a/web/html/pkgsubmit.php b/web/html/pkgsubmit.php index 26608ea..4f0c076 100644 --- a/web/html/pkgsubmit.php +++ b/web/html/pkgsubmit.php @@ -339,7 +339,7 @@ if ($uid): } else { # This is a brand new package - $q = sprintf("INSERT INTO Packages (Name, License, Version, CategoryID, Description, URL, SubmittedTS, ModifiedTS, SubmitterUID, MaintainerUID) VALUES ('%s', '%s', '%s-%s', %d, '%s', '%s', UNIX_TIMESTAMP(), UNIX_TIMESTAMP(), %d, %d)", + $q = sprintf("INSERT INTO Packages (Name, License, Version, CategoryID, Description, URL, SubmittedTS, ModifiedTS, MaintainerUID) VALUES ('%s', '%s', '%s-%s', %d, '%s', '%s', UNIX_TIMESTAMP(), UNIX_TIMESTAMP(), %d)", mysql_real_escape_string($new_pkgbuild['pkgname']), mysql_real_escape_string($new_pkgbuild['license']), mysql_real_escape_string($new_pkgbuild['pkgver']), @@ -347,7 +347,6 @@ if ($uid): mysql_real_escape_string($_REQUEST['category']), mysql_real_escape_string($new_pkgbuild['pkgdesc']), mysql_real_escape_string($new_pkgbuild['url']), - $uid, $uid); db_query($q, $dbh); diff --git a/web/lib/pkgfuncs.inc b/web/lib/pkgfuncs.inc index 7b43e45..c32037e 100644 --- a/web/lib/pkgfuncs.inc +++ b/web/lib/pkgfuncs.inc @@ -40,7 +40,7 @@ function canDeleteCommentArray($comment, $atype="", $uid=0) { # see if this Users.ID can manage the package # -function canManagePackage($uid=0,$AURMUID=0, $MUID=0, $SUID=0, $managed=0) { +function canManagePackage($uid=0,$AURMUID=0, $MUID=0, $managed=0) { if (!$uid) {return 0;} # The uid of the TU/Dev that manages the package @@ -51,10 +51,6 @@ function canManagePackage($uid=0,$AURMUID=0, $MUID=0, $SUID=0, $managed=0) { # if ($uid == $MUID && !$managed) {return 1;} - # If the package isn't maintained by a TU/Dev, is this the user-submitter? - # - if ($uid == $SUID && !$managed) {return 1;} - # otherwise, no right to manage this package # return 0; @@ -438,10 +434,6 @@ function pkg_search_page($SID="") { if (isset($_GET["SeB"]) && $_GET["SeB"] == "m") { $q_where .= "AND Users.Username = '".$_GET['K']."' "; } - # Search by submitter - elseif (isset($_GET["SeB"]) && $_GET["SeB"] == "s") { - $q_where .= "AND SubmitterUID = ".uid_from_username($_GET['K'])." "; - } # Search by name elseif (isset($_GET["SeB"]) && $_GET["SeB"] == "n") { $q_where .= "AND (Name LIKE '%".$_GET['K']."%') "; diff --git a/web/template/pkg_details.php b/web/template/pkg_details.php index 0658063..6fe0ec3 100644 --- a/web/template/pkg_details.php +++ b/web/template/pkg_details.php @@ -26,16 +26,6 @@ else { $edit_cat = "Category: " . $row['Category'] . ""; } -if ($row["SubmitterUID"]) { - $submitter = username_from_id($row["SubmitterUID"]); - if ($SID) { - $submitter = '<a href="account.php?Action=AccountInfo&ID=' . htmlspecialchars($row['SubmitterUID'], ENT_QUOTES) . '">' . htmlspecialchars($submitter) . '</a>'; - } - -} else { - $submitter = "None"; -} - if ($row["MaintainerUID"]) { $maintainer = username_from_id($row["MaintainerUID"]); if ($SID) { @@ -74,7 +64,6 @@ $out_of_date_time = ($row["OutOfDateTS"] == 0) ? $msg : gmdate("r", intval($row[ <?php echo $edit_cat ?> - <?php echo __('Submitter') .': ' . $submitter ?> <?php echo __('Maintainer') .': ' . $maintainer ?> <?php echo $votes ?> -- 1.7.2.5

Lukas Fleischer

2:11 p.m.

New subject: [aur-dev] [PATCH 1/4] remove submitter from package data

On Tue, May 10, 2011 at 09:01:27PM -0700, elij wrote:

...

--- web/html/pkgsubmit.php | 3 +-- web/lib/pkgfuncs.inc | 10 +--------- web/template/pkg_details.php | 11 ----------- 3 files changed, 2 insertions(+), 22 deletions(-)

The submitter field proved to be useful in some cases where a package was moved from the official repos to the AUR and either turned out to be incomplete or wasn't properly removed from the official repos. What's the motivation of removing this one?

elij

6:48 p.m.

New subject: [aur-dev] [PATCH 1/4] remove submitter from package data

On Wed, May 11, 2011 at 7:11 AM, Lukas Fleischer wrote:

...

On Tue, May 10, 2011 at 09:01:27PM -0700, elij wrote:

...
--- web/html/pkgsubmit.php | 3 +-- web/lib/pkgfuncs.inc | 10 +--------- web/template/pkg_details.php | 11 ----------- 3 files changed, 2 insertions(+), 22 deletions(-)

The submitter field proved to be useful in some cases where a package was moved from the official repos to the AUR and either turned out to be incomplete or wasn't properly removed from the official repos.

I guess I don't see what benefit the submitter field would have in such an instance. If someone moved it from the official repos to the aur, would they not be the submitter and also the maintainer?

...

What's the motivation of removing this one?

There was a discussion in another thread about adding submitter to the api return results, and I mentioned that I felt that submitter field wasn't very relevant or useful. This patch stemmed from that discussion.

Dan McGee

6:51 p.m.

New subject: [aur-dev] [PATCH 1/4] remove submitter from package data

On Wed, May 11, 2011 at 1:48 PM, elij wrote:

...

On Wed, May 11, 2011 at 7:11 AM, Lukas Fleischer wrote:

...
On Tue, May 10, 2011 at 09:01:27PM -0700, elij wrote:

...
--- web/html/pkgsubmit.php | 3 +-- web/lib/pkgfuncs.inc | 10 +--------- web/template/pkg_details.php | 11 ----------- 3 files changed, 2 insertions(+), 22 deletions(-)

The submitter field proved to be useful in some cases where a package was moved from the official repos to the AUR and either turned out to be incomplete or wasn't properly removed from the official repos.

I guess I don't see what benefit the submitter field would have in such an instance. If someone moved it from the official repos to the aur, would they not be the submitter and also the maintainer? Initially, yes. And then we all usually orphan the junk because we don't want it, we just put it there for postarity, so you've immediately lost information.

I think it has a lot less usefulness on the web page itself (at least for the general public), so I wouldn't be against culling it there, but as far as a point of reference when trying to look at the packages, it makes since to keep around. It can be far different than what the maintainer field tells you. -Dan

elij

7 p.m.

New subject: [aur-dev] [PATCH 1/4] remove submitter from package data

On Wed, May 11, 2011 at 11:51 AM, Dan McGee wrote:

...

On Wed, May 11, 2011 at 1:48 PM, elij wrote:

...
On Wed, May 11, 2011 at 7:11 AM, Lukas Fleischer wrote:

...
On Tue, May 10, 2011 at 09:01:27PM -0700, elij wrote:

...
--- web/html/pkgsubmit.php | 3 +-- web/lib/pkgfuncs.inc | 10 +--------- web/template/pkg_details.php | 11 ----------- 3 files changed, 2 insertions(+), 22 deletions(-)

The submitter field proved to be useful in some cases where a package was moved from the official repos to the AUR and either turned out to be incomplete or wasn't properly removed from the official repos.

I guess I don't see what benefit the submitter field would have in such an instance. If someone moved it from the official repos to the aur, would they not be the submitter and also the maintainer? Initially, yes. And then we all usually orphan the junk because we don't want it, we just put it there for postarity, so you've immediately lost information.

I think it has a lot less usefulness on the web page itself (at least for the general public), so I wouldn't be against culling it there, but as far as a point of reference when trying to look at the packages, it makes since to keep around. It can be far different than what the maintainer field tells you.

Hmm. So keeping it but maybe only showing it to TU or Developer class users may be more appropriate. Alternatively, it almost sounds like maintainer and submitter could be merged into an 'owner' value, and track owner history somehow (record each change of ownership in a join table). That might add the ability to track users that upload lots of packages, then disown them too. And track packages with high owner turnover (may tell whether a package is painful or burdensome to maintain). If such a thing were implemented though, I think it should be displayed only to TU or Developer class users. I can't see general users finding much use for it, but I could be wrong.

Lukas Fleischer

12 May 12 May

12:20 a.m.

New subject: [aur-dev] [PATCH 1/4] remove submitter from package data

On Wed, May 11, 2011 at 12:00:50PM -0700, elij wrote:

...

On Wed, May 11, 2011 at 11:51 AM, Dan McGee wrote:

...
On Wed, May 11, 2011 at 1:48 PM, elij wrote:

...
On Wed, May 11, 2011 at 7:11 AM, Lukas Fleischer wrote:

...
On Tue, May 10, 2011 at 09:01:27PM -0700, elij wrote:

...
--- web/html/pkgsubmit.php | 3 +-- web/lib/pkgfuncs.inc | 10 +--------- web/template/pkg_details.php | 11 ----------- 3 files changed, 2 insertions(+), 22 deletions(-)

The submitter field proved to be useful in some cases where a package was moved from the official repos to the AUR and either turned out to be incomplete or wasn't properly removed from the official repos.

I guess I don't see what benefit the submitter field would have in such an instance. If someone moved it from the official repos to the aur, would they not be the submitter and also the maintainer? Initially, yes. And then we all usually orphan the junk because we don't want it, we just put it there for postarity, so you've immediately lost information.

I think it has a lot less usefulness on the web page itself (at least for the general public), so I wouldn't be against culling it there, but as far as a point of reference when trying to look at the packages, it makes since to keep around. It can be far different than what the maintainer field tells you.

Hmm. So keeping it but maybe only showing it to TU or Developer class users may be more appropriate.

Agreed.

...

Alternatively, it almost sounds like maintainer and submitter could be merged into an 'owner' value, and track owner history somehow (record each change of ownership in a join table). That might add the ability to track users that upload lots of packages, then disown them too. And track packages with high owner turnover (may tell whether a package is painful or burdensome to maintain).

Hm, sounds cool, but I'm not sure if it's worth implementing. I don't see any real benefit from the feature itself or from the statistics that could be created using this yet.

...

If such a thing were implemented though, I think it should be displayed only to TU or Developer class users. I can't see general users finding much use for it, but I could be wrong.

elij

11 May 11 May

4:01 a.m.

New subject: [aur-dev] [PATCH 2/4] test return value from db_query before assuming it is valid

make the sql query form consistent in usage by cleaning up instances where db_query's result was not inspected before attempting to fetch row data from the handle --- web/html/addvote.php | 16 +++++- web/html/tu.php | 17 +++++- web/lib/acctfuncs.inc | 59 +++++++++++-------- web/lib/aur.inc | 8 ++- web/lib/pkgfuncs.inc | 116 +++++++++++++++++++++---------------- web/template/actions_form.php | 52 ++++++++++-------- web/template/pkg_search_form.php | 2 +- web/template/tu_list.php | 8 ++- 8 files changed, 172 insertions(+), 106 deletions(-) diff --git a/web/html/addvote.php b/web/html/addvote.php index 5936d56..a459610 100644 --- a/web/html/addvote.php +++ b/web/html/addvote.php @@ -21,14 +21,26 @@ if ($atype == "Trusted User" OR $atype == "Developer") { if (!empty($_POST['user'])) { $qcheck = "SELECT * FROM Users WHERE Username = '" . mysql_real_escape_string($_POST['user']) . "'"; - $check = mysql_num_rows(db_query($qcheck, $dbh)); + $result = db_query($qcheck, $dbh); + if ($result) { + $check = mysql_num_rows($result); + } + else { + $check = 0; + } if ($check == 0) { $error.= __("Username does not exist."); } else { $qcheck = "SELECT * FROM TU_VoteInfo WHERE User = '" . mysql_real_escape_string($_POST['user']) . "'"; $qcheck.= " AND End > UNIX_TIMESTAMP()"; - $check = mysql_num_rows(db_query($qcheck, $dbh)); + $result = db_query($qcheck, $dbh); + if ($result) { + $check = mysql_num_rows($result); + } + else { + $check = 0; + } if ($check != 0) { $error.= __("%s already has proposal running for them.", htmlentities($_POST['user'])); diff --git a/web/html/tu.php b/web/html/tu.php index c5cc36b..6ab8ae9 100644 --- a/web/html/tu.php +++ b/web/html/tu.php @@ -36,7 +36,13 @@ if ($atype == "Trusted User" OR $atype == "Developer") { $qvoted = "SELECT * FROM TU_Votes WHERE "; $qvoted.= "VoteID = " . $row['ID'] . " AND "; $qvoted.= "UserID = " . uid_from_sid($_COOKIE["AURSID"]); - $hasvoted = mysql_num_rows(db_query($qvoted, $dbh)); + $result = db_query($qvoted, $dbh); + if ($result) { + $hasvoted = mysql_num_rows($result); + } + else { + $hasvoted = 0; + } # List voters of a proposal. $qwhoVoted = "SELECT tv.UserID,U.Username @@ -85,10 +91,15 @@ if ($atype == "Trusted User" OR $atype == "Developer") { $canvote = 0; $errorvote = __("You've already voted for this proposal."); # Update if they voted - $hasvoted = mysql_num_rows(db_query($qvoted, $dbh)); + $result = db_query($qvoted, $dbh); + if ($result) { + $hasvoted = mysql_num_rows($result); + } $results = db_query($q, $dbh); - $row = mysql_fetch_assoc($results); + if ($results) { + $row = mysql_fetch_assoc($results); + } } } include("tu_details.php"); diff --git a/web/lib/acctfuncs.inc b/web/lib/acctfuncs.inc index 8ffa2f7..5bcff8b 100644 --- a/web/lib/acctfuncs.inc +++ b/web/lib/acctfuncs.inc @@ -197,7 +197,7 @@ function process_account_form($UTYPE,$TYPE,$A,$U="",$T="",$S="",$E="", } if (!$error && !valid_username($U) && !user_is_privileged($editor_user)) - $error = __("The username is invalid.") . "<ul>\n" + $error = __("The username is invalid.") . "<ul>\n" ."<li>" . __("It must be between %s and %s characters long", USERNAME_MIN_LEN, USERNAME_MAX_LEN ) . "</li>" @@ -718,11 +718,11 @@ function valid_user( $user ) $q = "SELECT ID FROM Users WHERE Username = '" . mysql_real_escape_string($user). "'"; - $result = mysql_fetch_row(db_query($q, $dbh)); - + $result = db_query($q, $dbh); # Is the username in the database? - if ($result[0]) { - return $result[0]; + if ($result) { + $row = mysql_fetch_row($result); + return $row[0]; } } return; @@ -751,25 +751,30 @@ function valid_passwd( $userID, $passwd ) $passwd_q = "SELECT ID FROM Users" . " WHERE ID = " . $userID . " AND Passwd = '" . salted_hash($passwd, $salt) . "'"; - $passwd_result = mysql_fetch_row(db_query($passwd_q, $dbh)); - if ($passwd_result[0]) { - return true; + $result = db_query($passwd_q, $dbh); + if ($result) { + $passwd_result = mysql_fetch_row($result); + if ($passwd_result[0]) { + return true; + } } } else { # check without salt $nosalt_q = "SELECT ID FROM Users". " WHERE ID = " . $userID . " AND Passwd = '" . md5($passwd) . "'"; - $nosalt_result = mysql_fetch_row(db_query($nosalt_q, $dbh)); - if ($nosalt_result[0]) { - # password correct, but salt it first - if (!save_salt($userID, $passwd)) { - trigger_error("Unable to salt user's password;" . - " ID " . $userID, E_USER_WARNING); - return false; + $result = db_query($nosalt_q, $dbh); + if ($result) { + $nosalt_row = mysql_fetch_row($result); + if ($nosalt_row[0]) { + # password correct, but salt it first + if (!save_salt($userID, $passwd)) { + trigger_error("Unable to salt user's password;" . + " ID " . $userID, E_USER_WARNING); + return false; + } + return true; } - - return true; } } } @@ -783,9 +788,12 @@ function user_suspended( $id ) { $dbh = db_connect(); $q = "SELECT Suspended FROM Users WHERE ID = " . $id; - $result = mysql_fetch_row(db_query($q, $dbh)); - if ($result[0] == 1 ) { - return true; + $result = db_query($q, $dbh); + if ($result) { + $row = mysql_fetch_row($result); + if ($result[0] == 1 ) { + return true; + } } return false; } @@ -797,7 +805,7 @@ function user_delete( $id ) { $dbh = db_connect(); $q = "DELETE FROM Users WHERE ID = " . $id; - $result = mysql_fetch_row(db_query($q, $dbh)); + db_query($q, $dbh); return; } @@ -809,9 +817,12 @@ function user_is_privileged( $id ) { $dbh = db_connect(); $q = "SELECT AccountTypeID FROM Users WHERE ID = " . $id; - $result = mysql_fetch_row(db_query($q, $dbh)); - if( $result[0] > 1) { - return $result[0]; + $result = db_query($q, $dbh); + if ($result) { + $row = mysql_fetch_row($result); + if( $result[0] > 1) { + return $result[0]; + } } return 0; diff --git a/web/lib/aur.inc b/web/lib/aur.inc index 5eed8e7..fb267af 100644 --- a/web/lib/aur.inc +++ b/web/lib/aur.inc @@ -491,8 +491,12 @@ function get_salt($user_id) { $dbh = db_connect(); $salt_q = "SELECT Salt FROM Users WHERE ID = " . $user_id; - $salt_result = mysql_fetch_row(db_query($salt_q, $dbh)); - return $salt_result[0]; + $result = db_query($salt_q, $dbh); + if ($result) { + $salt_row = mysql_fetch_row($result); + return $salt_row[0]; + } + return; } function save_salt($user_id, $passwd) diff --git a/web/lib/pkgfuncs.inc b/web/lib/pkgfuncs.inc index c32037e..f04ebff 100644 --- a/web/lib/pkgfuncs.inc +++ b/web/lib/pkgfuncs.inc @@ -316,45 +316,44 @@ function package_details($id=0, $SID="") { * outputs the body of search/search results page * * parameters: - * SID - current Session ID + * SID - current Session ID * preconditions: - * package search page has been accessed - * request variables have not been sanitized + * package search page has been accessed + * request variables have not been sanitized * - * request vars: - * O - starting result number - * PP - number of search hits per page - * C - package category ID number - * K - package search string - * SO - search hit sort order: - * values: a - ascending - * d - descending - * SB - sort search hits by: - * values: c - package category - * n - package name - * v - number of votes - * m - maintainer username - * SeB- property that search string (K) represents - * values: n - package name - * nd - package name & description - * x - package name (exact match) - * m - package maintainer's username - * s - package submitter's username - * do_Orphans - boolean. whether to search packages - * without a maintainer + * request vars: + * O - starting result number + * PP - number of search hits per page + * C - package category ID number + * K - package search string + * SO - search hit sort order: + * values: a - ascending + * d - descending + * SB - sort search hits by: + * values: c - package category + * n - package name + * v - number of votes + * m - maintainer username + * SeB- property that search string (K) represents + * values: n - package name + * nd - package name & description + * x - package name (exact match) + * m - package maintainer's username + * do_Orphans - boolean. whether to search packages + * without a maintainer * * - * These two are actually handled in packages.php. + * These two are actually handled in packages.php. * - * IDs- integer array of ticked packages' IDs - * action - action to be taken on ticked packages - * values: do_Flag - Flag out-of-date - * do_UnFlag - Remove out-of-date flag - * do_Adopt - Adopt - * do_Disown - Disown - * do_Delete - Delete (requires confirm_Delete to be set) - * do_Notify - Enable notification - * do_UnNotify - Disable notification + * IDs- integer array of ticked packages' IDs + * action - action to be taken on ticked packages + * values: do_Flag - Flag out-of-date + * do_UnFlag - Remove out-of-date flag + * do_Adopt - Adopt + * do_Disown - Disown + * do_Delete - Delete (requires confirm_Delete to be set) + * do_Notify - Enable notification + * do_UnNotify - Disable notification */ function pkg_search_page($SID="") { // establish a db connection @@ -391,15 +390,15 @@ function pkg_search_page($SID="") { } // FIXME: pull out DB-related code. all of it. - // this one's worth a choco-chip cookie, - // one of those nice big soft ones + // this one's worth a choco-chip cookie, + // one of those nice big soft ones // build the package search query // $q_select = "SELECT "; if ($SID) { $q_select .= "CommentNotify.UserID AS Notify, - PackageVotes.UsersID AS Voted, "; + PackageVotes.UsersID AS Voted, "; } $q_select .= "Users.Username AS Maintainer, PackageCategories.Category, @@ -422,7 +421,7 @@ function pkg_search_page($SID="") { $q_where = "WHERE 1 = 1 "; // TODO: possibly do string matching on category - // to make request variable values more sensible + // to make request variable values more sensible if (isset($_GET["C"]) && intval($_GET["C"])) { $q_where .= "AND Packages.CategoryID = ".intval($_GET["C"])." "; } @@ -499,7 +498,13 @@ function pkg_search_page($SID="") { $q_total = "SELECT COUNT(*) " . $q_from . $q_where; $result = db_query($q, $dbh); - $total = mysql_result(db_query($q_total, $dbh), 0); + $result_t = db_query($q_total, $dbh); + if ($result_t) { + $total = mysql_result($result_t, 0); + } + else { + $total = 0; + } if ($result && $total > 0) { if (isset($_GET["SO"]) && $_GET["SO"] == "d"){ @@ -851,7 +856,13 @@ function pkg_notify ($atype, $ids, $action = True) { # format in which it's sent requires this. foreach ($ids as $pid) { $q = "SELECT Name FROM Packages WHERE ID = $pid"; - $pkgname = mysql_result(db_query($q, $dbh), 0); + $result = db_query($q, $dbh); + if ($result) { + $pkgname = mysql_result($result , 0); + } + else { + $pkgname = ''; + } if ($first) $first = False; @@ -864,7 +875,8 @@ function pkg_notify ($atype, $ids, $action = True) { $q .= " AND PkgID = $pid"; # Notification already added. Don't add again. - if (!mysql_num_rows(db_query($q, $dbh))) { + $result = db_query($q, $dbh); + if (!mysql_num_rows($result)) { $q = "INSERT INTO CommentNotify (PkgID, UserID) VALUES ($pid, $uid)"; db_query($q, $dbh); } @@ -913,14 +925,14 @@ function pkg_delete_comment($atype) { $uid = uid_from_sid($_COOKIE["AURSID"]); if (canDeleteComment($comment_id, $atype, $uid)) { - $dbh = db_connect(); - $q = "UPDATE PackageComments "; - $q.= "SET DelUsersID = ".$uid." "; - $q.= "WHERE ID = ".intval($comment_id); - db_query($q, $dbh); - return __("Comment has been deleted."); + $dbh = db_connect(); + $q = "UPDATE PackageComments "; + $q.= "SET DelUsersID = ".$uid." "; + $q.= "WHERE ID = ".intval($comment_id); + db_query($q, $dbh); + return __("Comment has been deleted."); } else { - return __("You are not allowed to delete this comment."); + return __("You are not allowed to delete this comment."); } } @@ -959,8 +971,12 @@ function pkg_change_category($atype) { $q.= "FROM Packages "; $q.= "WHERE Packages.ID = ".$pid; $result = db_query($q, $dbh); - echo mysql_error(); - $pkg = mysql_fetch_assoc($result); + if ($result) { + $pkg = mysql_fetch_assoc($result); + } + else { + return __("You are not allowed to change this package category."); + } $uid = uid_from_sid($_COOKIE["AURSID"]); if ($uid == $pkg["MaintainerUID"] or diff --git a/web/template/actions_form.php b/web/template/actions_form.php index 45bc09b..058002f 100644 --- a/web/template/actions_form.php +++ b/web/template/actions_form.php @@ -8,39 +8,45 @@ # $q = "SELECT * FROM PackageVotes WHERE UsersID = ". $uid; $q.= " AND PackageID = ".$row["ID"]; - if (!mysql_num_rows(db_query($q, $dbh))) { - echo " "; - } else { - echo " "; + $result = db_query($q, $dbh); + if ($result) { + if (!mysql_num_rows($result)) { + echo " "; + } else { + echo " "; + } } # Comment Notify Button # $q = "SELECT * FROM CommentNotify WHERE UserID = ". $uid; $q.= " AND PkgID = ".$row["ID"]; - if (!mysql_num_rows(db_query($q, $dbh))) { - echo " "; - } else { - echo " "; + $result = db_query($q, $dbh); + if ($result) { + if (!mysql_num_rows($result)) { + echo " "; + } else { + echo " "; + } } -if ($row["OutOfDateTS"] === NULL) { - echo "\n"; -} else { - echo "\n"; + if ($row["OutOfDateTS"] === NULL) { + echo "\n"; + } else { + echo "\n"; } -if ($row["MaintainerUID"] === NULL) { - echo "\n"; -} else if ($uid == $row["MaintainerUID"] || - $atype == "Trusted User" || $atype == "Developer") { + if ($row["MaintainerUID"] === NULL) { + echo "\n"; + } else if ($uid == $row["MaintainerUID"] || + $atype == "Trusted User" || $atype == "Developer") { echo "\n"; } diff --git a/web/template/pkg_search_form.php b/web/template/pkg_search_form.php index 281cdc3..e25bdfd 100644 --- a/web/template/pkg_search_form.php +++ b/web/template/pkg_search_form.php @@ -38,7 +38,7 @@ <label><?php print __("Search by"); ?></label> <select name='SeB'> <?php - $searchby = array('nd' => __('Name, Description'), 'n' => __('Name Only'), 'x' => __('Exact name'), 'm' => __('Maintainer'), 's' => __('Submitter')); + $searchby = array('nd' => __('Name, Description'), 'n' => __('Name Only'), 'x' => __('Exact name'), 'm' => __('Maintainer')); foreach ($searchby as $k => $v): if (isset($_REQUEST['SeB']) && $_REQUEST['SeB'] == $k): ?> diff --git a/web/template/tu_list.php b/web/template/tu_list.php index 3a927d9..75d9414 100644 --- a/web/template/tu_list.php +++ b/web/template/tu_list.php @@ -40,7 +40,13 @@ <td class='<?php print $c ?>'> <?php $q = "SELECT * FROM TU_Votes WHERE VoteID = " . $row['ID'] . " AND UserID = " . uid_from_sid($_COOKIE["AURSID"]); - $hasvoted = mysql_num_rows(db_query($q, $dbh)); + $result_tulist = db_query($q, $dbh); + if ($result_tulist) { + $hasvoted = mysql_num_rows($result_tulist); + } + else { + $hasvoted = 0; + } ?> <?php if ($hasvoted == 0) { ?> -- 1.7.2.5

Lukas Fleischer

2:19 p.m.

New subject: [aur-dev] [PATCH 2/4] test return value from db_query before assuming it is valid

On Tue, May 10, 2011 at 09:01:28PM -0700, elij wrote:

...

make the sql query form consistent in usage by cleaning up instances where db_query's result was not inspected before attempting to fetch row data from the handle --- web/html/addvote.php | 16 +++++- web/html/tu.php | 17 +++++- web/lib/acctfuncs.inc | 59 +++++++++++-------- web/lib/aur.inc | 8 ++- web/lib/pkgfuncs.inc | 116 +++++++++++++++++++++---------------- web/template/actions_form.php | 52 ++++++++++-------- web/template/pkg_search_form.php | 2 +- web/template/tu_list.php | 8 ++- 8 files changed, 172 insertions(+), 106 deletions(-)

Yeah, cool. I guess there's some more places that need to get fixed but that looks like a good start. Could you please resend that one without the formatting changes? They make it kinda hard to spot the actual changes.

elij

11:17 p.m.

New subject: [aur-dev] [PATCH 2/4] test return value from db_query before assuming it is valid

make the sql query form consistent in usage by cleaning up instances where db_query's result was not inspected before attempting to fetch row data from the handle --- web/html/addvote.php | 16 +++++++++- web/html/tu.php | 17 +++++++++-- web/lib/acctfuncs.inc | 59 ++++++++++++++++++++++++---------------- web/lib/aur.inc | 8 ++++- web/lib/pkgfuncs.inc | 43 +++++++++++++++++++++--------- web/template/actions_form.php | 52 ++++++++++++++++++++---------------- web/template/tu_list.php | 8 +++++- 7 files changed, 135 insertions(+), 68 deletions(-) diff --git a/web/html/addvote.php b/web/html/addvote.php index 5936d56..a459610 100644 --- a/web/html/addvote.php +++ b/web/html/addvote.php @@ -21,14 +21,26 @@ if ($atype == "Trusted User" OR $atype == "Developer") { if (!empty($_POST['user'])) { $qcheck = "SELECT * FROM Users WHERE Username = '" . mysql_real_escape_string($_POST['user']) . "'"; - $check = mysql_num_rows(db_query($qcheck, $dbh)); + $result = db_query($qcheck, $dbh); + if ($result) { + $check = mysql_num_rows($result); + } + else { + $check = 0; + } if ($check == 0) { $error.= __("Username does not exist."); } else { $qcheck = "SELECT * FROM TU_VoteInfo WHERE User = '" . mysql_real_escape_string($_POST['user']) . "'"; $qcheck.= " AND End > UNIX_TIMESTAMP()"; - $check = mysql_num_rows(db_query($qcheck, $dbh)); + $result = db_query($qcheck, $dbh); + if ($result) { + $check = mysql_num_rows($result); + } + else { + $check = 0; + } if ($check != 0) { $error.= __("%s already has proposal running for them.", htmlentities($_POST['user'])); diff --git a/web/html/tu.php b/web/html/tu.php index c5cc36b..6ab8ae9 100644 --- a/web/html/tu.php +++ b/web/html/tu.php @@ -36,7 +36,13 @@ if ($atype == "Trusted User" OR $atype == "Developer") { $qvoted = "SELECT * FROM TU_Votes WHERE "; $qvoted.= "VoteID = " . $row['ID'] . " AND "; $qvoted.= "UserID = " . uid_from_sid($_COOKIE["AURSID"]); - $hasvoted = mysql_num_rows(db_query($qvoted, $dbh)); + $result = db_query($qvoted, $dbh); + if ($result) { + $hasvoted = mysql_num_rows($result); + } + else { + $hasvoted = 0; + } # List voters of a proposal. $qwhoVoted = "SELECT tv.UserID,U.Username @@ -85,10 +91,15 @@ if ($atype == "Trusted User" OR $atype == "Developer") { $canvote = 0; $errorvote = __("You've already voted for this proposal."); # Update if they voted - $hasvoted = mysql_num_rows(db_query($qvoted, $dbh)); + $result = db_query($qvoted, $dbh); + if ($result) { + $hasvoted = mysql_num_rows($result); + } $results = db_query($q, $dbh); - $row = mysql_fetch_assoc($results); + if ($results) { + $row = mysql_fetch_assoc($results); + } } } include("tu_details.php"); diff --git a/web/lib/acctfuncs.inc b/web/lib/acctfuncs.inc index 8ffa2f7..5bcff8b 100644 --- a/web/lib/acctfuncs.inc +++ b/web/lib/acctfuncs.inc @@ -197,7 +197,7 @@ function process_account_form($UTYPE,$TYPE,$A,$U="",$T="",$S="",$E="", } if (!$error && !valid_username($U) && !user_is_privileged($editor_user)) - $error = __("The username is invalid.") . "<ul>\n" + $error = __("The username is invalid.") . "<ul>\n" ."<li>" . __("It must be between %s and %s characters long", USERNAME_MIN_LEN, USERNAME_MAX_LEN ) . "</li>" @@ -718,11 +718,11 @@ function valid_user( $user ) $q = "SELECT ID FROM Users WHERE Username = '" . mysql_real_escape_string($user). "'"; - $result = mysql_fetch_row(db_query($q, $dbh)); - + $result = db_query($q, $dbh); # Is the username in the database? - if ($result[0]) { - return $result[0]; + if ($result) { + $row = mysql_fetch_row($result); + return $row[0]; } } return; @@ -751,25 +751,30 @@ function valid_passwd( $userID, $passwd ) $passwd_q = "SELECT ID FROM Users" . " WHERE ID = " . $userID . " AND Passwd = '" . salted_hash($passwd, $salt) . "'"; - $passwd_result = mysql_fetch_row(db_query($passwd_q, $dbh)); - if ($passwd_result[0]) { - return true; + $result = db_query($passwd_q, $dbh); + if ($result) { + $passwd_result = mysql_fetch_row($result); + if ($passwd_result[0]) { + return true; + } } } else { # check without salt $nosalt_q = "SELECT ID FROM Users". " WHERE ID = " . $userID . " AND Passwd = '" . md5($passwd) . "'"; - $nosalt_result = mysql_fetch_row(db_query($nosalt_q, $dbh)); - if ($nosalt_result[0]) { - # password correct, but salt it first - if (!save_salt($userID, $passwd)) { - trigger_error("Unable to salt user's password;" . - " ID " . $userID, E_USER_WARNING); - return false; + $result = db_query($nosalt_q, $dbh); + if ($result) { + $nosalt_row = mysql_fetch_row($result); + if ($nosalt_row[0]) { + # password correct, but salt it first + if (!save_salt($userID, $passwd)) { + trigger_error("Unable to salt user's password;" . + " ID " . $userID, E_USER_WARNING); + return false; + } + return true; } - - return true; } } } @@ -783,9 +788,12 @@ function user_suspended( $id ) { $dbh = db_connect(); $q = "SELECT Suspended FROM Users WHERE ID = " . $id; - $result = mysql_fetch_row(db_query($q, $dbh)); - if ($result[0] == 1 ) { - return true; + $result = db_query($q, $dbh); + if ($result) { + $row = mysql_fetch_row($result); + if ($result[0] == 1 ) { + return true; + } } return false; } @@ -797,7 +805,7 @@ function user_delete( $id ) { $dbh = db_connect(); $q = "DELETE FROM Users WHERE ID = " . $id; - $result = mysql_fetch_row(db_query($q, $dbh)); + db_query($q, $dbh); return; } @@ -809,9 +817,12 @@ function user_is_privileged( $id ) { $dbh = db_connect(); $q = "SELECT AccountTypeID FROM Users WHERE ID = " . $id; - $result = mysql_fetch_row(db_query($q, $dbh)); - if( $result[0] > 1) { - return $result[0]; + $result = db_query($q, $dbh); + if ($result) { + $row = mysql_fetch_row($result); + if( $result[0] > 1) { + return $result[0]; + } } return 0; diff --git a/web/lib/aur.inc b/web/lib/aur.inc index 5eed8e7..fb267af 100644 --- a/web/lib/aur.inc +++ b/web/lib/aur.inc @@ -491,8 +491,12 @@ function get_salt($user_id) { $dbh = db_connect(); $salt_q = "SELECT Salt FROM Users WHERE ID = " . $user_id; - $salt_result = mysql_fetch_row(db_query($salt_q, $dbh)); - return $salt_result[0]; + $result = db_query($salt_q, $dbh); + if ($result) { + $salt_row = mysql_fetch_row($result); + return $salt_row[0]; + } + return; } function save_salt($user_id, $passwd) diff --git a/web/lib/pkgfuncs.inc b/web/lib/pkgfuncs.inc index c32037e..df8aa96 100644 --- a/web/lib/pkgfuncs.inc +++ b/web/lib/pkgfuncs.inc @@ -399,7 +399,7 @@ function pkg_search_page($SID="") { $q_select = "SELECT "; if ($SID) { $q_select .= "CommentNotify.UserID AS Notify, - PackageVotes.UsersID AS Voted, "; + PackageVotes.UsersID AS Voted, "; } $q_select .= "Users.Username AS Maintainer, PackageCategories.Category, @@ -499,7 +499,13 @@ function pkg_search_page($SID="") { $q_total = "SELECT COUNT(*) " . $q_from . $q_where; $result = db_query($q, $dbh); - $total = mysql_result(db_query($q_total, $dbh), 0); + $result_t = db_query($q_total, $dbh); + if ($result_t) { + $total = mysql_result($result_t, 0); + } + else { + $total = 0; + } if ($result && $total > 0) { if (isset($_GET["SO"]) && $_GET["SO"] == "d"){ @@ -851,7 +857,13 @@ function pkg_notify ($atype, $ids, $action = True) { # format in which it's sent requires this. foreach ($ids as $pid) { $q = "SELECT Name FROM Packages WHERE ID = $pid"; - $pkgname = mysql_result(db_query($q, $dbh), 0); + $result = db_query($q, $dbh); + if ($result) { + $pkgname = mysql_result($result , 0); + } + else { + $pkgname = ''; + } if ($first) $first = False; @@ -864,7 +876,8 @@ function pkg_notify ($atype, $ids, $action = True) { $q .= " AND PkgID = $pid"; # Notification already added. Don't add again. - if (!mysql_num_rows(db_query($q, $dbh))) { + $result = db_query($q, $dbh); + if (!mysql_num_rows($result)) { $q = "INSERT INTO CommentNotify (PkgID, UserID) VALUES ($pid, $uid)"; db_query($q, $dbh); } @@ -913,14 +926,14 @@ function pkg_delete_comment($atype) { $uid = uid_from_sid($_COOKIE["AURSID"]); if (canDeleteComment($comment_id, $atype, $uid)) { - $dbh = db_connect(); - $q = "UPDATE PackageComments "; - $q.= "SET DelUsersID = ".$uid." "; - $q.= "WHERE ID = ".intval($comment_id); - db_query($q, $dbh); - return __("Comment has been deleted."); + $dbh = db_connect(); + $q = "UPDATE PackageComments "; + $q.= "SET DelUsersID = ".$uid." "; + $q.= "WHERE ID = ".intval($comment_id); + db_query($q, $dbh); + return __("Comment has been deleted."); } else { - return __("You are not allowed to delete this comment."); + return __("You are not allowed to delete this comment."); } } @@ -959,8 +972,12 @@ function pkg_change_category($atype) { $q.= "FROM Packages "; $q.= "WHERE Packages.ID = ".$pid; $result = db_query($q, $dbh); - echo mysql_error(); - $pkg = mysql_fetch_assoc($result); + if ($result) { + $pkg = mysql_fetch_assoc($result); + } + else { + return __("You are not allowed to change this package category."); + } $uid = uid_from_sid($_COOKIE["AURSID"]); if ($uid == $pkg["MaintainerUID"] or diff --git a/web/template/actions_form.php b/web/template/actions_form.php index 45bc09b..058002f 100644 --- a/web/template/actions_form.php +++ b/web/template/actions_form.php @@ -8,39 +8,45 @@ # $q = "SELECT * FROM PackageVotes WHERE UsersID = ". $uid; $q.= " AND PackageID = ".$row["ID"]; - if (!mysql_num_rows(db_query($q, $dbh))) { - echo " "; - } else { - echo " "; + $result = db_query($q, $dbh); + if ($result) { + if (!mysql_num_rows($result)) { + echo " "; + } else { + echo " "; + } } # Comment Notify Button # $q = "SELECT * FROM CommentNotify WHERE UserID = ". $uid; $q.= " AND PkgID = ".$row["ID"]; - if (!mysql_num_rows(db_query($q, $dbh))) { - echo " "; - } else { - echo " "; + $result = db_query($q, $dbh); + if ($result) { + if (!mysql_num_rows($result)) { + echo " "; + } else { + echo " "; + } } -if ($row["OutOfDateTS"] === NULL) { - echo "\n"; -} else { - echo "\n"; + if ($row["OutOfDateTS"] === NULL) { + echo "\n"; + } else { + echo "\n"; } -if ($row["MaintainerUID"] === NULL) { - echo "\n"; -} else if ($uid == $row["MaintainerUID"] || - $atype == "Trusted User" || $atype == "Developer") { + if ($row["MaintainerUID"] === NULL) { + echo "\n"; + } else if ($uid == $row["MaintainerUID"] || + $atype == "Trusted User" || $atype == "Developer") { echo "\n"; } diff --git a/web/template/tu_list.php b/web/template/tu_list.php index 3a927d9..75d9414 100644 --- a/web/template/tu_list.php +++ b/web/template/tu_list.php @@ -40,7 +40,13 @@ <td class='<?php print $c ?>'> <?php $q = "SELECT * FROM TU_Votes WHERE VoteID = " . $row['ID'] . " AND UserID = " . uid_from_sid($_COOKIE["AURSID"]); - $hasvoted = mysql_num_rows(db_query($q, $dbh)); + $result_tulist = db_query($q, $dbh); + if ($result_tulist) { + $hasvoted = mysql_num_rows($result_tulist); + } + else { + $hasvoted = 0; + } ?> <?php if ($hasvoted == 0) { ?> -- 1.7.2.5

elij

11:25 p.m.

New subject: [aur-dev] [PATCH 2/4] test return value from db_query before assuming it is valid

hmm. looks like there were still a couple lines of formatting junk in this patch.. hilariously the ones I missed are converting spaces to tabs to be more consistent. &_& the count is low though. Skimming the patch file it looks like only 3 or 4 lines. I got the big ones pruned out of the patch though.

Lukas Fleischer

12 May 12 May

12:09 a.m.

New subject: [aur-dev] [PATCH 2/4] test return value from db_query before assuming it is valid

On Wed, May 11, 2011 at 04:17:12PM -0700, elij wrote:

...

make the sql query form consistent in usage by cleaning up instances where db_query's result was not inspected before attempting to fetch row data from the handle --- web/html/addvote.php | 16 +++++++++- web/html/tu.php | 17 +++++++++-- web/lib/acctfuncs.inc | 59 ++++++++++++++++++++++++---------------- web/lib/aur.inc | 8 ++++- web/lib/pkgfuncs.inc | 43 +++++++++++++++++++++--------- web/template/actions_form.php | 52 ++++++++++++++++++++---------------- web/template/tu_list.php | 8 +++++- 7 files changed, 135 insertions(+), 68 deletions(-)

Looks quite ok now. Which method did you use to spot these inconsistencies? Skimming through the source code it seems that there are some more query results that should be validated, like the "SELECT" one in "web/html/pkgsubmit.php".

elij

3:05 a.m.

New subject: [aur-dev] [PATCH 2/4] test return value from db_query before assuming it is valid

On Wed, May 11, 2011 at 5:09 PM, Lukas Fleischer wrote:

...

On Wed, May 11, 2011 at 04:17:12PM -0700, elij wrote:

...
make the sql query form consistent in usage by cleaning up instances where db_query's result was not inspected before attempting to fetch row data from the handle --- web/html/addvote.php | 16 +++++++++- web/html/tu.php | 17 +++++++++-- web/lib/acctfuncs.inc | 59 ++++++++++++++++++++++++---------------- web/lib/aur.inc | 8 ++++- web/lib/pkgfuncs.inc | 43 +++++++++++++++++++++--------- web/template/actions_form.php | 52 ++++++++++++++++++++---------------- web/template/tu_list.php | 8 +++++- 7 files changed, 135 insertions(+), 68 deletions(-)

Looks quite ok now. Which method did you use to spot these inconsistencies?

After I found and noticed one (was working on some code that triggered a php warning due to the issue, I did the following grep to find more instances. I was primarily looking for instances where db_query was being _directly_ passed to mysql_fetch_* functions. grep -R db_query /path/to/web/ | grep mysql

...

Skimming through the source code it seems that there are some more query results that should be validated, like the "SELECT" one in "web/html/pkgsubmit.php".

Yeah, I didn't originally look for these, but I found some more with this.. grep -R -n -A1 db_query * | grep -B1 mysql That find instances where db_query is directly followed (next line) by a mysql_ function. Looking at these a few are cases of 'not testing return value of db_query before eval'. output of above command: # looks like it needs to be fixed html/pkgsubmit.php:303: $result = db_query($q, $dbh); html/pkgsubmit.php-304- $pdata = mysql_fetch_assoc($result); # this one look relatively ok. just an insert. html/pkgsubmit.php:352: db_query($q, $dbh); html/pkgsubmit.php-353- $packageID = mysql_insert_id($dbh); # looks like it needs to be fixed html/account.php:51: $result = db_query($q, $dbh); html/account.php-52- if (!mysql_num_rows($result)) { # looks like it needs to be fixed html/account.php:78: $result = db_query($q, $dbh); html/account.php-79- if (!mysql_num_rows($result)) { # looks like it needs to be fixed html/account.php:115: $result = db_query($q, $dbh); html/account.php-116- if (!mysql_num_rows($result)) { # looks like it needs to be fixed html/tu.php:28: $results = db_query($q, $dbh); html/tu.php-29- $row = mysql_fetch_assoc($results); # looks like it needs to be fixed html/tu.php:53: $result = db_query($qwhoVoted,$dbh); html/tu.php-54- if (mysql_num_rows($result) > 0) { # this one is probably ok. it uses the handle to get a 'changed row count' html/passreset.php:45: $result = db_query($q, $dbh); html/passreset.php-46- if (!mysql_affected_rows($dbh)) { # looks like it needs to be fixed lib/pkgfuncs.inc:254: $result = db_query($q, $dbh); lib/pkgfuncs.inc-255- if (mysql_num_rows($result) > 0) { # looks like it needs to be fixed lib/pkgfuncs.inc:637: $result = db_query($q, $dbh); lib/pkgfuncs.inc-638- if (mysql_num_rows($result)) { # looks like it needs to be fixed lib/pkgfuncs.inc:879: $result = db_query($q, $dbh); lib/pkgfuncs.inc-880- if (!mysql_num_rows($result)) { # looks like it needs to be fixed lib/stats.inc:24: $result = db_query($dbq, $dbh); lib/stats.inc-25- $row = mysql_fetch_row($result); # looks like it needs to be fixed lib/aur.inc:30: $result = db_query($q, $dbh); lib/aur.inc-31- if (mysql_num_rows($result) == 0) { # looks like it needs to be fixed lib/aur.inc:348: $result = db_query($q, $dbh); lib/aur.inc-349- if (mysql_num_rows($result) == 0) {return 1;} # looks like it needs to be fixed template/pkg_comment_form.php:31: $result = db_query($q, $dbh); template/pkg_comment_form.php-32- $row = mysql_fetch_assoc($result); Some of this could probably be cleaned up through judicious use of a few wrapper functions, with the added benefit of abstracting the sql away like we had previously talked about... Still, you eat an elephant with small bites, so I think fixing these directly for now would probably be the quickest path to resolution. This would also ease abstraction later, because it would add sensible checks at the same location where abstraction would need to test for query results anyway.

elij

11 May 11 May

4:01 a.m.

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

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); -- 1.7.2.5

Lukas Fleischer

2:22 p.m.

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

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.

elij

6:54 p.m.

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

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

Lukas Fleischer

12 May 12 May

12:11 a.m.

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

On Wed, May 11, 2011 at 11:54:34AM -0700, elij wrote:

...

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

elij

11 May 11 May

4:01 a.m.

New subject: [aur-dev] [PATCH 4/4] make return value consistent for this function

--- web/lib/aur.inc | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/web/lib/aur.inc b/web/lib/aur.inc index fb267af..f8fa715 100644 --- a/web/lib/aur.inc +++ b/web/lib/aur.inc @@ -196,7 +196,7 @@ function uid_from_sid($sid="") { $q.= "AND Sessions.SessionID = '" . mysql_real_escape_string($sid) . "'"; $result = db_query($q, $dbh); if (!$result) { - return 0; + return ""; } $row = mysql_fetch_row($result); -- 1.7.2.5

Lukas Fleischer

2:31 p.m.

New subject: [aur-dev] [PATCH 4/4] make return value consistent for this function

On Tue, May 10, 2011 at 09:01:30PM -0700, elij wrote:

...

--- web/lib/aur.inc | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/web/lib/aur.inc b/web/lib/aur.inc index fb267af..f8fa715 100644 --- a/web/lib/aur.inc +++ b/web/lib/aur.inc @@ -196,7 +196,7 @@ function uid_from_sid($sid="") { $q.= "AND Sessions.SessionID = '" . mysql_real_escape_string($sid) . "'"; $result = db_query($q, $dbh); if (!$result) { - return 0; + return ""; } $row = mysql_fetch_row($result);

Yes, that is one of the code parts that really suck. Returning zero or an empty string in a function that is called uid_from_sid() is just nonsense. I started refactoring this, replacing all zeros and empty strings in functions that are designed to return IDs by "null" but didn't ever finish that since I didn't have enough time to check and fix all invocations, also (some of them compare to "0" and stuff). It would be cool to have someone looking into that :)

4740

Age (days ago)

4741

Last active (days ago)

List overview

Download

18 comments

3 participants

participants (3)

Dan McGee
elij
Lukas Fleischer