Last Comment Bug 303183 - Bugzilla fails to warn when a product change would force the removal of an optional group
: Bugzilla fails to warn when a product change would force the removal of an op...
Status: RESOLVED FIXED
[sg:want][wanted-bmo][blocker will fix]
: selenium
Product: Bugzilla
Classification: Server Software
Component: Creating/Changing Bugs (show other bugs)
: 2.19.1
: All All
: -- major (vote)
: Bugzilla 3.0
Assigned To: Frédéric Buclin
: default-qa
:
Mentors:
Depends on: 347213
Blocks: 271023 348704
  Show dependency treegraph
 
Reported: 2005-08-02 19:49 PDT by Jesse Ruderman
Modified: 2007-08-16 21:53 PDT (History)
8 users (show)
LpSolit: approval3.0+
mkanat: blocking3.0.1-
mkanat: blocking2.22-
LpSolit: testcase+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch for 3.0, v1 (7.22 KB, patch)
2007-07-10 15:09 PDT, Frédéric Buclin
no flags Details | Diff | Splinter Review
patch for 3.0, v1.1 (8.44 KB, patch)
2007-07-10 15:18 PDT, Frédéric Buclin
wicked: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2005-08-02 19:49:36 PDT
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] (justdave@bugzilla.org) 2005-08-02 22:15:13 PDT
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 2006-01-09 04:49:54 PST
This shouldn't be too hard to fix IMO.
Comment 3 Max Kanat-Alexander 2006-01-11 17:07:18 PST
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] 2006-05-30 00:20:51 PDT
(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] 2006-06-21 01:26:11 PDT
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 2006-11-01 07:50:51 PST
Not a security bug -> 2.22
Comment 7 Gervase Markham [:gerv] 2007-02-20 03:15:18 PST
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 2007-06-03 09:23:58 PDT
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 2007-06-19 11:31:53 PDT
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 2007-07-07 06:25:17 PDT
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 2007-07-07 14:31:33 PDT
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] (use needinfo?) 2007-07-07 14:47:37 PDT
(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 2007-07-10 15:09:49 PDT
Created attachment 271741 [details] [diff] [review]
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 2007-07-10 15:18:35 PDT
Created attachment 271747 [details] [diff] [review]
patch for 3.0, v1.1

Remove an unused variable + update a comment.
Comment 15 Teemu Mannermaa (:wicked) 2007-07-26 06:14:54 PDT
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 2007-07-26 07:02:12 PDT
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

Note You need to log in before you can comment on or make changes to this bug.