The default bug view has changed. See this FAQ.

[SECURITY] Users can see all products when editing bugs

RESOLVED FIXED in Bugzilla 3.4

Status

()

Bugzilla
Creating/Changing Bugs
P1
critical
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: Sergej Pupykin, Assigned: Frédéric Buclin)

Tracking

({regression})

3.3.4
Bugzilla 3.4
regression
Dependency tree / graph
Bug Flags:
approval +
approval3.4 +
blocking3.4.1 +
testcase ?

Details

Attachments

(1 attachment, 3 obsolete attachments)

3.08 KB, patch
Max Kanat-Alexander
: review+
Details | Diff | Splinter Review
(Reporter)

Description

8 years ago
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
(Assignee)

Comment 1

8 years ago
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.
Group: bugzilla-security
Severity: normal → critical
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking3.4.1+
Keywords: regression
OS: Linux → All
Priority: -- → P1
Hardware: x86 → All
Target Milestone: --- → Bugzilla 3.4
Version: unspecified → 3.4
(Assignee)

Updated

8 years ago
Severity: critical → major
(Assignee)

Comment 2

8 years ago
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.
Depends on: 371995
Version: 3.4 → 3.3.4
(Assignee)

Comment 3

8 years ago
Created attachment 391655 [details] [diff] [review]
patch, v1

We need to pass the list of products to use.
Assignee: create-and-change → LpSolit
Status: NEW → ASSIGNED
Attachment #391655 - Flags: review?(mkanat)
(Assignee)

Comment 4

8 years ago
The problem related to charts is different, as charts do not see categories as products.
Summary: user can see all products if he edits bug or creates new chart → [SECURITY] Users can see all products when editing bugs

Updated

8 years ago
Attachment #391655 - Flags: review?(mkanat) → review-

Comment 5

8 years ago
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.

Updated

8 years ago
Severity: major → critical
(Assignee)

Comment 6

8 years ago
(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

8 years ago
Um, I'd be okay with having the extra product at the *beginning*, I think.
(Assignee)

Comment 8

8 years ago
(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.
(Assignee)

Comment 9

8 years ago
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.
Attachment #391655 - Attachment is obsolete: true
Attachment #391843 - Flags: review?(mkanat)

Updated

8 years ago
Attachment #391843 - Flags: review?(mkanat) → review+

Comment 10

8 years ago
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

8 years ago
We should release ASAP to get out a fix for this issue.
Flags: approval?
Flags: approval3.4?

Updated

8 years ago
Blocks: 507801
(Assignee)

Comment 12

8 years ago
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.
Attachment #391843 - Attachment is obsolete: true
Attachment #392078 - Flags: review?(mkanat)

Updated

8 years ago
Attachment #392078 - Flags: review?(mkanat) → review+
(Assignee)

Updated

8 years ago
Flags: approval?
Flags: approval3.4?
Flags: approval3.4+
Flags: approval+
(Assignee)

Comment 13

8 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Updated

8 years ago
Flags: testcase?
Flags: approval?
Flags: approval3.4?
Flags: approval3.4+
Flags: approval+

Updated

8 years ago
Flags: approval?
Flags: approval3.4?
Flags: approval3.4+
Flags: approval+
(Assignee)

Comment 14

8 years ago
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.
Attachment #392078 - Attachment is obsolete: true
Attachment #392086 - Flags: review?(mkanat)

Updated

8 years ago
Attachment #392086 - Flags: review?(mkanat) → review+
(Assignee)

Comment 15

8 years ago
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

8 years ago
Security advisory sent, unlocking bug.
Group: bugzilla-security
Blocks: 545695
You need to log in before you can comment on or make changes to this bug.