303183 - Bugzilla fails to warn when a product change would force the removal of an optional group

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Jesse Ruderman]

Moving a security-sensitive bug from Firefox to Toolkit makes it public.  I
discovered this with bug 303181.  I don't know if there's sometihng wrong with
bugizlla.mozilla.org's configuration, but it seems to me that even with an odd
configuration, Bugzilla should never make a bug public without at least
informing the person changing the bug.

Maybe Firefox and Toolkit both have a group called "security" and that's
confusing Bugzilla?

Comment 1

[Dave Miller [:justdave]]

The Toolkit product was set up so that the security group was not allowed to be
used in it.  Bugzilla did as it was told.  It has no way to tell the security
group apart from any other bug group, and often times each product has its own
mandatory groups which are automatically set.  I just fixed the group controls
for Toolbox so it shouldn't happen again.  

That said, I'm almost positive Bugzilla *is* supposed to warn you when that
happens.  Examining the source on process_bug.cgi, it appears that the warning
only gets triggered if the destination product has any groups which are set to
be "Default on" but not mandatory.

Bugzilla really should warn if a product change would force an *optional* group
to be removed.  This is sort of the same problem as bug 276553, but coming from
a different direction (trying to change the product rather than trying to
directly change groups).

Comment 2

[Frédéric Buclin]

This shouldn't be too hard to fix IMO.

Comment 3

[Max Kanat-Alexander]

This isn't exactly a regression, so I wouldn't hold up the release on it. However, I would like to see it fixed at some point, possibly also back on the 2.20 branch.

Comment 4

[Daniel Veditz [:dveditz]]

(In reply to comment #1)
> Bugzilla really should warn if a product change would force an *optional*
> group to be removed.

If the person changing the product does not have permission to remove the group then the product change should be blocked, or the group should be preserved even if the new product doesn't officially support it.

That last must be possible, someone recently filed a new bug in BMO and checked the security issue checkbox, but filed against a product that apparently didn't support that group. The security group was added successfully, and only after I accidentally removed it and found I couldn't replace it did we realize the product wasn't set up to support the security group.

Comment 5

[Daniel Veditz [:dveditz]]

This bug strikes again, security group lost when morgamic switched bug 338114 from Firefox to Update unintentionally leaving bug unprotected.

Comment 6

[Frédéric Buclin]

Not a security bug -> 2.22

Comment 7

[Gervase Markham [:gerv]]

This may not be a security bug in terms of people breaking into a Bugzilla machine, but it's definitely one in terms of people getting to see stuff they shouldn't be able to see. A privacy bug, if you like.

Gerv

Comment 8

[Frédéric Buclin]

My patch in bug 347213 will fix this bug, but I'm not a fan of backporting it on 2.22.

Comment 9

[Frédéric Buclin]

Bug 347213 will fix this issue in 3.2, but I will attach a minimal fix for 3.0. We won't take it for 2.22 though.

Comment 10

[Frédéric Buclin]

I can attach a fix for the 3.0.x branch in the coming days. I think it worths waiting for this fix before releasing 3.0.1 as some developers see this bug as security related.

Comment 11

[Max Kanat-Alexander]

We've lived with it for long enough that I don't consider it a blocker.

Of course, if you *do* come up with a patch in the next few days, it'd certainly be *nice* to have.

Comment 12

[Reed Loden [:reed]]

(In reply to comment #11)
> We've lived with it for long enough that I don't consider it a blocker.

Well, the friendly folks at b.m.o do consider it a blocker for some stuff we'd like to do. :)

Comment 13

[Frédéric Buclin]

Attachment: patch for 3.0, v1

I really need another pair of eyes on this patch, i.e. with heavy testing to be sure I'm not regressing anything. The behavior of process_bug.cgi in Bugzilla 3.2 is definitely better than that (but is too invasive for a backport, as already said).

Note that I don't let users have a better control on group restrictions, but display more accurate messages based on what process_bug.cgi really does.

Comment 14

[Frédéric Buclin]

Attachment: patch for 3.0, v1.1

Remove an unused variable + update a comment.

Comment 15

[Teemu Mannermaa (:wicked)]

Comment on attachment 271747 [details] [diff] [review]
patch for 3.0, v1.1

Fix following things before checkin and this should be ready to go. Group code is pretty complicated so I'm glad QA will happen before release. :)

>Index: template/en/default/bug/process/verify-new-product.html.tmpl
>===================================================================
> [%# INTERFACE:

Add old_groups to interface docs.

>+    <p>The following groups are not legal for the <b>[% cgi.param("product")
...
>+    <ul>
...
>+    </ul>
...
>+    </p>

An ul tag can't be inside p so the first ul tag implicitly ends the p tag. This causes validation error for the p end tag ending non-opened p tag. You need to add a p end tag before the ul start tag and a p start tag after the ul end tag.

>+      <input type="radio" id="add_yesifinold" name="addtonewgroup" value="yesifinold" checked="checked">
>+        <label for="add_yesifinold">yes, but only if the [% terms.bug %] was in any of

Nit: There's an extra space before this "yes, ..." but not before the "yes" and "no" radio button texts. This is nit only because this is an existing problem.

Comment 16

[Frédéric Buclin]

Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.351.2.5; previous revision: 1.351.2.4
done
Checking in template/en/default/bug/process/verify-new-product.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/process/verify-new-product.html.tmpl,v  <--  verify-new-product.html.tmpl
new revision: 1.20.2.2; previous revision: 1.20.2.1
done