| Message ID | 20180810212628.21458-1-eschwartz@archlinux.org |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Fri, 10 Aug 2018 at 23:26:28, Eli Schwartz wrote: > The notify script expects to see the userid followed by additional > arguments like the pkgbase id, however, these were getting sent swapped > around (presumably due to the similarity with the db connection which > expects them in the other order when processing SQL statements). > > As a result, some random userid with the same id as the pkgbase, got sent a > notification regarding some package with the same id as the real user's id. > > Signed-off-by: Eli Schwartz <eschwartz@archlinux.org> > --- > aurweb/git/serve.py | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > [...] Wow, good catch! The patch is totally correct but the suspected cause is not. I think thus is a regression introduced by f3b4c5c (Refactor the notification script, 2018-05-17) where some of the parameters where accidentally pushed around. It is a shame that our test suite did not catch this but looking at the tests... "$NOTIFY" adopt 1 1 && [...] "$NOTIFY" comaintainer-add 1 1 && [...] "$NOTIFY" comaintainer-remove 1 1 && ... it is not much of a surprise either. Maybe we should try to make each parameter unique? I also realized that we don't seem to test disown notifications at all! I will add a reference to f3b4c5c (Refactor the notification script, 2018-05-17) to the commit message, merge to pu and patch our live setup in a minute. Thanks! Best regards, Lukas
On 8/11/18 4:25 AM, Lukas Fleischer wrote: > On Fri, 10 Aug 2018 at 23:26:28, Eli Schwartz wrote: >> The notify script expects to see the userid followed by additional >> arguments like the pkgbase id, however, these were getting sent swapped >> around (presumably due to the similarity with the db connection which >> expects them in the other order when processing SQL statements). >> >> As a result, some random userid with the same id as the pkgbase, got sent a >> notification regarding some package with the same id as the real user's id. >> >> Signed-off-by: Eli Schwartz <eschwartz@archlinux.org> >> --- >> aurweb/git/serve.py | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> [...] > > Wow, good catch! > > The patch is totally correct but the suspected cause is not. I think > thus is a regression introduced by f3b4c5c (Refactor the notification > script, 2018-05-17) where some of the parameters where accidentally > pushed around. > > It is a shame that our test suite did not catch this but looking at the > tests... > > "$NOTIFY" adopt 1 1 && > [...] > "$NOTIFY" comaintainer-add 1 1 && > [...] > "$NOTIFY" comaintainer-remove 1 1 && > > ... it is not much of a surprise either. Maybe we should try to make > each parameter unique? I also realized that we don't seem to test disown > notifications at all! > > I will add a reference to f3b4c5c (Refactor the notification script, > 2018-05-17) to the commit message, merge to pu and patch our live setup > in a minute. Looks good to me. Minor nit: s/where/were/ in the commit message.
On 8/11/18 4:25 AM, Lukas Fleischer wrote: > Wow, good catch! > > The patch is totally correct but the suspected cause is not. I think > thus is a regression introduced by f3b4c5c (Refactor the notification > script, 2018-05-17) where some of the parameters where accidentally > pushed around. We're still getting a small handful of reports about this, and I'm not sure why. When was this deployed to aur.archlinux.org?
diff --git a/aurweb/git/serve.py b/aurweb/git/serve.py index 93ff34c..2882780 100755 --- a/aurweb/git/serve.py +++ b/aurweb/git/serve.py @@ -107,7 +107,7 @@ def pkgbase_adopt(pkgbase, user, privileged): [pkgbase_id, userid]) conn.commit() - subprocess.Popen((notify_cmd, 'adopt', str(pkgbase_id), str(userid))) + subprocess.Popen((notify_cmd, 'adopt', str(userid), str(pkgbase_id))) conn.close() @@ -165,8 +165,8 @@ def pkgbase_set_comaintainers(pkgbase, userlist, user, privileged): cur = conn.execute("INSERT INTO PackageComaintainers " + "(PackageBaseID, UsersID, Priority) " + "VALUES (?, ?, ?)", [pkgbase_id, userid, i]) - subprocess.Popen((notify_cmd, 'comaintainer-add', str(pkgbase_id), - str(userid))) + subprocess.Popen((notify_cmd, 'comaintainer-add', str(userid), + str(pkgbase_id))) else: cur = conn.execute("UPDATE PackageComaintainers " + "SET Priority = ? " + @@ -179,7 +179,7 @@ def pkgbase_set_comaintainers(pkgbase, userlist, user, privileged): "WHERE PackageBaseID = ? AND UsersID = ?", [pkgbase_id, userid]) subprocess.Popen((notify_cmd, 'comaintainer-remove', - str(pkgbase_id), str(userid))) + str(userid), str(pkgbase_id))) conn.commit() conn.close() @@ -266,7 +266,7 @@ def pkgbase_disown(pkgbase, user, privileged): if userid == 0: raise aurweb.exceptions.InvalidUserException(user) - subprocess.Popen((notify_cmd, 'disown', str(pkgbase_id), str(userid))) + subprocess.Popen((notify_cmd, 'disown', str(userid), str(pkgbase_id))) conn.close()
The notify script expects to see the userid followed by additional arguments like the pkgbase id, however, these were getting sent swapped around (presumably due to the similarity with the db connection which expects them in the other order when processing SQL statements). As a result, some random userid with the same id as the pkgbase, got sent a notification regarding some package with the same id as the real user's id. Signed-off-by: Eli Schwartz <eschwartz@archlinux.org> --- aurweb/git/serve.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)