[aur-dev][PATCH 1/3] fix broken SQL query that always failed
Due to missing whitespace at the end of strings during joining, we ended up with the query fragment "DelTS IS NULLAND NOT PinnedTS" which should be "DelTS IS NULL AND NOT PinnedTS" So the check for pinned comments > 5 likely always failed. In php 7, a completely broken query that raises exceptions in the database engine was silently ignored... in php 8, it raises Uncaught PDOException: SQLSTATE[HY000]: General error: 1 near "PinnedTS": syntax error in <file> and aborts the page building. End result: users with permission to pin comments cannot see any comments, or indeed page content below the first comment header Signed-off-by: Eli Schwartz <eschwartz@archlinux.org> --- web/lib/pkgbasefuncs.inc.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/lib/pkgbasefuncs.inc.php b/web/lib/pkgbasefuncs.inc.php index a4925891..4c8abba7 100644 --- a/web/lib/pkgbasefuncs.inc.php +++ b/web/lib/pkgbasefuncs.inc.php @@ -21,7 +21,7 @@ function pkgbase_comments_count($base_id, $include_deleted, $only_pinned=false) $q = "SELECT COUNT(*) FROM PackageComments "; $q.= "WHERE PackageBaseID = " . $base_id . " "; if (!$include_deleted) { - $q.= "AND DelTS IS NULL"; + $q.= "AND DelTS IS NULL "; } if ($only_pinned) { $q.= "AND NOT PinnedTS = 0"; -- 2.30.1
We usually guard such queries and have both mysql and sqlite branches. But I have not implemented the sqlite branch. Given sqlite is typically used for local dev setups, the fact that "users with more than the configured max simultaneous logins" can avoid getting some logins annulled is probably not a huge risk. And this always *used* to fail on sqlite, silently. Now, in php 8, it raises PDOException, which prevents running the test server Document this as a FIXME for now, until someone reimplements the query for sqlite. Signed-off-by: Eli Schwartz <eschwartz@archlinux.org> --- web/lib/acctfuncs.inc.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/web/lib/acctfuncs.inc.php b/web/lib/acctfuncs.inc.php index d238c0e0..30c4cfe0 100644 --- a/web/lib/acctfuncs.inc.php +++ b/web/lib/acctfuncs.inc.php @@ -597,7 +597,9 @@ function try_login() { /* Generate a session ID and store it. */ while (!$logged_in && $num_tries < 5) { $session_limit = config_get_int('options', 'max_sessions_per_user'); - if ($session_limit) { + # FIXME: this does not work for sqlite (JOIN in a DELETE clause) + # hence non-prod instances can have a naughty amount of simultaneous logins + if ($backend == "mysql" && $session_limit) { /* * Delete all user sessions except the * last ($session_limit - 1). -- 2.30.1
Signed-off-by: Eli Schwartz <eschwartz@archlinux.org> --- I don't know nearly enough SQL to know any kind of tradeoffs between one or the other, but this one should theoretically *work* everywhere, and certainly runs to success here on my sqlite development environment. web/lib/acctfuncs.inc.php | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/web/lib/acctfuncs.inc.php b/web/lib/acctfuncs.inc.php index 30c4cfe0..752abe97 100644 --- a/web/lib/acctfuncs.inc.php +++ b/web/lib/acctfuncs.inc.php @@ -597,21 +597,17 @@ function try_login() { /* Generate a session ID and store it. */ while (!$logged_in && $num_tries < 5) { $session_limit = config_get_int('options', 'max_sessions_per_user'); - # FIXME: this does not work for sqlite (JOIN in a DELETE clause) - # hence non-prod instances can have a naughty amount of simultaneous logins - if ($backend == "mysql" && $session_limit) { + if ($session_limit) { /* * Delete all user sessions except the * last ($session_limit - 1). */ - $q = "DELETE s.* FROM Sessions s "; - $q.= "LEFT JOIN (SELECT SessionID FROM Sessions "; + $q = "DELETE FROM Sessions "; $q.= "WHERE UsersId = " . $userID . " "; + $q.= "AND SessionID NOT IN (SELECT SessionID FROM Sessions "; + $q.= "WHERE UsersID = " . $userID . " "; $q.= "ORDER BY LastUpdateTS DESC "; - $q.= "LIMIT " . ($session_limit - 1) . ") q "; - $q.= "ON s.SessionID = q.SessionID "; - $q.= "WHERE s.UsersId = " . $userID . " "; - $q.= "AND q.SessionID IS NULL;"; + $q.= "LIMIT " . ($session_limit - 1) . ")"; $dbh->query($q); } -- 2.30.1
Merged all three patches (and closed merge request on GitLab). Thanks!
On 20/02/2021 17:33, Lukas Fleischer via aur-dev wrote:
Merged all three patches (and closed merge request on GitLab). Thanks!
Thanks for merging, will this be available in the live branch soon? Thanks in advance, Jelle
participants (3)
-
Eli Schwartz
-
Jelle van der Waa
-
Lukas Fleischer