117718 - Mass Change removes a bugs groupset if the bug was in the wrong product group

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect, P1)

Description

[Jacob Steenhagen]

I was just looking into bug 113975 and notice that $comma always seemed to be 
set for me even though I purposfully hadn't seleted anything to change on the 
mass form.  That lead me to discover that DoComma() was being called from line 
568 which should only be called if either $groupAdd or $groupDel was set.  As 
it tuned out, $groupDel was set to be the sum of every group I was in.  I 
tested a mass change of a secure bug, making sure that the "Don't change
this group restriction" button was selected, and sure enough, it removed the 
bug from the group.

Comment 1

[Jacob Steenhagen]

The "Do not change" radio button's value is "-1", but as of version 1.103 of 
process_bug.cgi, the bit fiddling code no longer looks for that value before 
adding that bitset to the $groupDel variable.  What confuses me, is that it 
does check for:

if (!$::FORM{"bit-$b"}) {

Which should evaluate to true if $::FORM{"bit-$b"} == -1.

Comment 2

[Jacob Steenhagen]

OK, looking a little deeper, this occurs if you have product groups for which 
the "Don't touch", "Add", and "Remove" radio buttons don't appear on the mass 
change page.  This also results in undefined warnings in the error log, but I 
suspect those same undefined warnings appear when a checkbox is unchecked on 
the bug form.

Comment 3

[Jacob Steenhagen]

Probably the easiest solution to this is to pass "-1" as a hidden for value for 
any groups that aren't going to appear on the bug form.

Comment 4

[Jacob Steenhagen]

Attachment: Include hidden form value

Comment 5

[Dave Miller [:justdave]]

I don't see what this is fixing....?   This only fixes if the bug is in a
product group for a product that's not represented on the buglist.  But if the
product isn't represented in the buglist, how did that bug get in that group? 
Bugs shouldn't be in groups for products the bugs aren't in....

I think this is a broader issue... we probably need a sanity check to make sure
bugs aren't in the wrong product groups, and also better protections in
process_bug.cgi to make sure you can't set bugs to the wrong product groups.

Comment 6

[Bradley Baetz (:bbaetz)]

I know this worked, because in the original patch for this (was that me - I
think it was) had a problem which justdave found, and I fixed. I think.

Comment 7

[Jacob Steenhagen]

The problem is as Dave described it:

> the bug is in a product group for a product that's not represented on the
> buglist

How I got it that way, I'm not really sure (that is got my bug into the wrong 
product group), but we shouldn't pull it out of a secure group and make it 
public just because the product group wasn't in the buglist (mine was a test 
bug, so it wasn't a real big deal, but I could see it being a problem 
elsewhere).

In talking with Dave on IRC, I think we shouldn't even show the group on the 
mass change page if it's a product group and there's more than one product 
listed on the mass change page.  We already have a precidence for this with 
components, etc. and groups are currently the only thing that defy this.  
Having the group widget on the mass change page this way is (I think) one 
possible way to get a bug into the wrong bug group.

Comment 8

[Matthew Tuck [:CodeMachine]]

Above precedent is something we will surely be removing at some stage (bug
#39120).

Comment 9

[Bradley Baetz (:bbaetz)]

I'm going to be pendantic and point out that this can happen if the bug was
changed after the page was displayed, but befor ethe new form was submitted. You
can't assume that noone has changed this in the meantime.

Comment 10

[Bradley Baetz (:bbaetz)]

the mass change page lists all groups you are a member of - is this a bug?
Should it check for product groups?

However, the processbug code shold probably just check that the form value is
defined before trying to fiddle with the bits, shouldn't it?

Comment 11

[Dave Miller [:justdave]]

I'm not going to make this a blocker because although certainly possible, this
isn't a very likely situation to run into.  Bumping severity to critical though
because we should try to get this in if we can, but I won't hold the release for
it.  (there is a patch here waiting for review)

Comment 12

[Bradley Baetz (:bbaetz)]

Comment on attachment 63246 [details] [diff] [review]
Include hidden form value

Yeah, but the real fix is in process_bug, else people could manipulate the raw
html directly. I'll try for a patch later tonight.

Comment 13

[Bradley Baetz (:bbaetz)]

Attachment: patch

This works with mass change and normal. I haven't tweaked the html to try this
on teh actual problem, though.

Comment 14

[Dave Miller [:justdave]]

Comment on attachment 78026 [details] [diff] [review]
patch

r= justdave x2

This compiles and passes the tests.  I don't have a way to actually test what
this was designed to fix, but it's pretty obvious this will handle it from the
code.

Comment 15

[Bradley Baetz (:bbaetz)]

-> me (patch author)

Comment 16

[Bradley Baetz (:bbaetz)]

Checked in