276605 - Ignore vote changes when usevotes = 0

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Frédéric Buclin]

Actually, you can still change your votes even if usevotes = 0 (via such an URL:
votes.cgi?bug_id=64&action=show_user). Consequently, you can still confirm bugs
if the following conditions are satistfied in CheckIfVotedConfirmed():

if ($votes >= $votestoconfirm && $status eq $::unconfirmedstate) {...}

We need to prevent that. I have a patch.

Comment 1

[Max Kanat-Alexander]

If you have a patch, let's see it. :-)

Does this also show up on the 2.18 branch? (If it does, it also needs to be
blocking2.18.)

Comment 2

[Max Kanat-Alexander]

Hey justdave, this looks like it might be a security bug. (Users can confirm
bugs when they shouldn't be able to.)

Comment 3

[Max Kanat-Alexander]

When I asked in the channel, justdave said that this isn't a security bug, just
a minor annoyance, since UNCONFIRMED will be disabled when usevotes = 0.

Comment 4

[Dave Miller [:justdave]]

I'll take this for both 2.18 and 2.20 if it's applicable to both, but I won't
hold release for it.

Comment 5

[Frédéric Buclin]

Attachment: patch, v1

forbid to edit votes if usevotes = 0 + fix a LOCK TABLE problem.

Comment 6

[Marc Schumann [:Wurblzap]]

Comment on attachment 170245 [details] [diff] [review]
patch, v1

It seems to me that what this bug addresses is basically covered by this:

-    record_votes();
+    record_votes() if Param('usevotes');
-    my $canedit = 1 if ($userid && $name eq Bugzilla->user->login);
+    my $canedit = (Param('usevotes') && $userid == $who) ? 1 : 0;

so this may be separated from the locking issue if it turns out to be
necessary.

I don't think what happens inside CheckIfVotedConfirmed should happen without
locking. More than that even: it should happen with the same lock that is
active during the UPDATE bugs SET votes part. You said that Dave agreed to move
CheckIfVotedConfirmed out of the locked part, but I can't see that.

This is my real issue. Let's address the minor ones, and the nits and questions
now :)

+    my $canedit = (Param('usevotes') && $userid == $who) ? 1 : 0;

The part beginning with '?' can go.

-	 my $v = FetchOneColumn();
-	 $v ||= 0;
+	 my $v = FetchOneColumn() || 0;

Unrelated to this bug, but fine with me.

+    print Bugzilla->cgi->header();

I can see that something along these lines is needed for CheckIfVotedConfirmed.
But in show_user it happens again. Am I missing something?

The header_done part is very hacky. It works, but I don't like it. Do you think
there may be a more elegant solution?

Comment 7

[Dave Miller [:justdave]]

(In reply to comment #6)
> You said that Dave agreed to move
> CheckIfVotedConfirmed out of the locked part, but I can't see that.

I said it sounded logical, but I hadn't actually looked at the code closely when
I said that.  I do know bbaetz was damn anal about proper table locking back in
the day, and most of those locks were there for a reason :)  Usually when one of
those "tables not locked" errors happen it means someone added a table to a
query somewhere and didn't add to the lock. 

If this patch can be split, I would much prefer it to be, because these really
are two separate issues.  I can't see this issue being a release blocker, but
the SQL error definitely is.  (That said, there's nothing stopping this from
going in before the release)

Comment 8

[Dave Miller [:justdave]]

er, I didn't mean to do that ;)

Comment 9

[Frédéric Buclin]

Comment on attachment 170245 [details] [diff] [review]
patch, v1

Asking bbaetz for review... in order to make sure if CheckIfVotedConfirmed()
should remain inside the LOCK... UNLOCK TABLE or not.

Bradley, any comments, suggestions are welcome.

Comment 10

[Marc Schumann [:Wurblzap]]

There's a big, bad LOCK TABLES statement in process_bug.cgi. I'm afraid we'll
need to adopt it or abstract it out as sub
lockAllFreakingTablesNeededForUpdatingBugs or something :/

Comment 11

[Frédéric Buclin]

Comment on attachment 170245 [details] [diff] [review]
patch, v1

I finally splitted my patch into two parts, to make things easier to review. So
this patch is now obsolete.

But I'm still interested in Bradley's opinion. ;)

Comment 12

[Frédéric Buclin]

Attachment: patch, v2

Now that bug 277013 is fixed, this patch only includes the remaining part of
the original patch.

Comment 13

[Marc Schumann [:Wurblzap]]

Comment on attachment 170830 [details] [diff] [review]
patch, v2

Ok, this does the trick.

+    my $canedit = (Param('usevotes') && $userid == $who) ? 1 : 0;

I still don't like that ?1:0, thinking that it's more readable without it, but
I'll leave it up to you whether you wish to put up a new patch without it. You
have r=wurblzap with or without it :)

Comment 14

[Marc Schumann [:Wurblzap]]

This does not apply cleanly to the branch; if you like, put up a branch patch,
set r+ on it and request approval2.18.

For the record, this...

(In reply to comment #6)
> +    print Bugzilla->cgi->header();
> 
> I can see that something along these lines is needed for CheckIfVotedConfirmed.
> But in show_user it happens again. Am I missing something?

... is all right (see https://bugzilla.mozilla.org/show_bug.cgi?id=277013#c3).

Comment 15

[Frédéric Buclin]

Attachment: patch for the 2.18 branch

I tested the critical case where the URL is /votes.cgi?action=show_user&user=
when the user is *not* logged in, because of the test

my $canedit = (Param('usevotes') && $userid == $who) ? 1 : 0;

In this case, neither the current user nor the user in the URL (empty actually)
are defined and I just tested if ($userid == $who) is true in this case, which
would be bad. Fortunately, DBNameToIdAndCheck() complains first on the trunk...
but an error message is displayed in the 2.18 branch!? I'm not sure why but I
suspect the _create() subroutine in User.pm to be the reason as this subroutine
changed between 2.18 and 2.19.

( I hope not to introduce another regression as I already did once :( )

Comment 16

[Shane H. W. Travis]

I'm not checking this in until the approved2.18? is either approved or goes 
away (in case you're wondering what the holdup is).

Comment 17

[Shane H. W. Travis]

2.18:
Checking in votes.cgi;
/cvsroot/mozilla/webtools/bugzilla/votes.cgi,v  <--  votes.cgi
new revision: 1.17.2.3; previous revision: 1.17.2.2
done
Checking in template/en/default/bug/votes/list-for-user.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/votes/list-for-
user.html.tmpl,v  <--  list-for-user.html.tmpl
new revision: 1.15.2.2; previous revision: 1.15.2.1
done

Tip:
Checking in votes.cgi;
/cvsroot/mozilla/webtools/bugzilla/votes.cgi,v  <--  votes.cgi
new revision: 1.22; previous revision: 1.21
done
Checking in template/en/default/bug/votes/list-for-user.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/votes/list-for-
user.html.tmpl,v  <--  list-for-user.html.tmpl
new revision: 1.17; previous revision: 1.16
done