Last Comment Bug 507389 - [SECURITY] Users can see all products when editing bugs
: [SECURITY] Users can see all products when editing bugs
Status: RESOLVED FIXED
: regression
Product: Bugzilla
Classification: Server Software
Component: Creating/Changing Bugs (show other bugs)
: 3.3.4
: All All
: P1 critical (vote)
: Bugzilla 3.4
Assigned To: Frédéric Buclin
: default-qa
Mentors:
Depends on: 371995
Blocks: 507801 545695
  Show dependency treegraph
 
Reported: 2009-07-30 08:02 PDT by Sergej Pupykin
Modified: 2010-02-11 11:32 PST (History)
3 users (show)
mkanat: approval+
mkanat: approval3.4+
LpSolit: blocking3.4.1+
mkanat: testcase?
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch, v1 (2.28 KB, patch)
2009-07-30 11:40 PDT, Frédéric Buclin
mkanat: review-
Details | Diff | Review
patch, v2 (2.36 KB, patch)
2009-07-31 03:23 PDT, Frédéric Buclin
mkanat: review+
Details | Diff | Review
patch, v2.1 (3.03 KB, patch)
2009-08-01 04:41 PDT, Frédéric Buclin
mkanat: review+
Details | Diff | Review
full patch, v3 (3.08 KB, patch)
2009-08-01 06:22 PDT, Frédéric Buclin
mkanat: review+
Details | Diff | Review

Description Sergej Pupykin 2009-07-30 08:02:05 PDT
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.1) Gecko/20090718 Shiretoko/3.5.1
Build Identifier: bugzilla-3.4

User can see all products even he has no access to them.

Reproducible: Always

Steps to Reproduce:
1. create 2 products: prod1 and prod2
2. restrict user1 access only to prod1
3. try to edit own bug and see all products in combobox
Actual Results:  
user can see full product list

Expected Results:  
user can see allowed product list

product list page works ok.

problem affects only combo and listboxes on new chart and editbug pages
Comment 1 Frédéric Buclin 2009-07-30 10:05:09 PDT
I can reproduce on tip and 3.4. I cannot reproduce on 3.2.4 and 3.0.8, so this is a regression.

To be exact: a product must be totally hidden if group settings are Mandatory/Mandatory + the Entry bit is set.
Comment 2 Frédéric Buclin 2009-07-30 10:27:06 PDT
This is a regression due to bug 371995, which replaced bug.choices.product (which correctly calls $user->get_enterable_products) by Bugzilla::Product->get_all.

Bugzilla 3.3.4 and newer are affected.
Comment 3 Frédéric Buclin 2009-07-30 11:40:05 PDT
Created attachment 391655 [details] [diff] [review]
patch, v1

We need to pass the list of products to use.
Comment 4 Frédéric Buclin 2009-07-30 12:49:02 PDT
The problem related to charts is different, as charts do not see categories as products.
Comment 5 Max Kanat-Alexander 2009-07-30 15:10:49 PDT
Comment on attachment 391655 [details] [diff] [review]
patch, v1

>Index: template/en/default/bug/edit.html.tmpl
>     <tr>
>+       [% prod_list = user.get_enterable_products %]
>+       [% IF NOT user.can_enter_product(bug.product) %]
>+         [% prod_list.push(bug.product_obj) %]
>+         [% prod_list = prod_list.sort('name') %]

  You shouldn't need to sort them by name again--Perl will sort them differently than the database will, so it's best to just let the DB sort them.

>Index: template/en/default/bug/field.html.tmpl

>+  #   list (optional): The list of legal values, for select fields.

  Hmm. In other patches/customizations, I've called this "override_legal_values", which I think is better, because "list" could be a very common variable name.
Comment 6 Frédéric Buclin 2009-07-30 15:16:53 PDT
(In reply to comment #5)
>   You shouldn't need to sort them by name again--Perl will sort them
> differently than the database will, so it's best to just let the DB sort them.

I have to sort the list here because the added product shouldn't be at the end (unless we are fine with this, but this is confusing).
Comment 7 Max Kanat-Alexander 2009-07-30 21:13:37 PDT
Um, I'd be okay with having the extra product at the *beginning*, I think.
Comment 8 Frédéric Buclin 2009-07-31 02:24:04 PDT
(In reply to comment #7)
> Um, I'd be okay with having the extra product at the *beginning*, I think.

OK, sure, easy to do. Patch coming.
Comment 9 Frédéric Buclin 2009-07-31 03:23:06 PDT
Created attachment 391843 [details] [diff] [review]
patch, v2

I now only set the override_... list if the user can edit the product field, else we don't waste time generating the list as the field would be read-only.
Comment 10 Max Kanat-Alexander 2009-07-31 03:34:23 PDT
Comment on attachment 391843 [details] [diff] [review]
patch, v2

>Index: template/en/default/bug/edit.html.tmpl
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/edit.html.tmpl,v
>retrieving revision 1.160
>diff -3 -p -u -r1.160 edit.html.tmpl
>--- template/en/default/bug/edit.html.tmpl	23 Jul 2009 06:36:10 -0000	1.160
>+++ template/en/default/bug/edit.html.tmpl	31 Jul 2009 10:16:52 -0000
>@@ -375,8 +375,16 @@
>     [%#############%]
>     
>     <tr>
>+       [% IF bug.check_can_change_field('product', 0, 1) %]
>+         [% prod_list = user.get_enterable_products %]
>+         [% IF NOT user.can_enter_product(bug.product) %]
>+           [% prod_list.unshift(bug.product_obj) %]
>+         [% END %]
>+       [% END %]
>+
>        [% INCLUDE bug/field.html.tmpl
>             bug = bug, field = select_fields.product,
>+            override_legal_values = prod_list
>             desc_url = 'describecomponents.cgi', value = bug.product
>             editable = bug.check_can_change_field('product', 0, 1) %]
>     </tr>
>Index: template/en/default/bug/field.html.tmpl
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/field.html.tmpl,v
>retrieving revision 1.29
>diff -3 -p -u -r1.29 field.html.tmpl
>--- template/en/default/bug/field.html.tmpl	17 Jul 2009 22:40:15 -0000	1.29
>+++ template/en/default/bug/field.html.tmpl	31 Jul 2009 10:16:52 -0000
>@@ -23,6 +23,7 @@
> [%# INTERFACE:
>   #   field: a Bugzilla::Field object
>   #   value: The value of the field for this bug.
>+  #   override_legal_values (optional): The list of legal values, for select fields.
>   #   editable: Whether the field should be displayed as an editable
>   #             <input> or as just the plain text of its value.
>   #   allow_dont_change: display the --do_not_change-- option for select fields.
>@@ -130,7 +131,10 @@
>               [% dontchange FILTER html %]
>             </option>
>           [% END %]
>-          [% FOREACH legal_value = field.legal_values %]
>+          [% IF NOT override_legal_values %]
>+            [% override_legal_values = field.legal_values %]
>+          [% END %]
>+          [% FOREACH legal_value = override_legal_values %]
>             [% SET control_value = legal_value.visibility_value %]
>             [% SET control_field = field.value_field %]
>             <option value="[% legal_value.name FILTER html %]"
Comment 11 Max Kanat-Alexander 2009-07-31 03:34:45 PDT
We should release ASAP to get out a fix for this issue.
Comment 12 Frédéric Buclin 2009-08-01 04:41:14 PDT
Created attachment 392078 [details] [diff] [review]
patch, v2.1

I just realized my patch doesn't pass 008filter.t, because unshift() is not in the whitelist. Fixed.
Comment 13 Frédéric Buclin 2009-08-01 05:38:11 PDT
tip:

Checking in t/008filter.t;
/cvsroot/mozilla/webtools/bugzilla/t/008filter.t,v  <--  008filter.t
new revision: 1.30; previous revision: 1.29
done
Checking in template/en/default/bug/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.161; previous revision: 1.160
done
Checking in template/en/default/bug/field.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/field.html.tmpl,v  <--  field.html.tmpl
new revision: 1.30; previous revision: 1.29
done


3.4:

Checking in t/008filter.t;
/cvsroot/mozilla/webtools/bugzilla/t/008filter.t,v  <--  008filter.t
new revision: 1.29.2.1; previous revision: 1.29
done
Checking in template/en/default/bug/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.156.2.2; previous revision: 1.156.2.1
done
Checking in template/en/default/bug/field.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/field.html.tmpl,v  <--  field.html.tmpl
new revision: 1.24.2.4; previous revision: 1.24.2.3
done
Comment 14 Frédéric Buclin 2009-08-01 06:22:11 PDT
Created attachment 392086 [details] [diff] [review]
full patch, v3

The previous patch has a bug where values can be overriden by other fields. This is a full patch, with 2.1 being backed out.
Comment 15 Frédéric Buclin 2009-08-01 07:01:48 PDT
tip:

Checking in template/en/default/bug/field.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/field.html.tmpl,v  <--  field.html.tmpl
new revision: 1.31; previous revision: 1.30
done

3.4:

Checking in template/en/default/bug/field.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/field.html.tmpl,v  <--  field.html.tmpl
new revision: 1.24.2.5; previous revision: 1.24.2.4
done
Comment 16 Max Kanat-Alexander 2009-08-01 08:38:12 PDT
Security advisory sent, unlocking bug.

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