Last Comment Bug 653341 - Bug.create() fails to error out if an invalid group is passed
: Bug.create() fails to error out if an invalid group is passed
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: WebService (show other bugs)
: 4.0
: All All
: -- normal (vote)
: Bugzilla 4.0
Assigned To: Frédéric Buclin
: default-qa
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-27 21:03 PDT by Dave Miller [:justdave] (justdave@bugzilla.org)
Modified: 2011-09-23 17:14 PDT (History)
5 users (show)
mkanat: approval+
mkanat: approval4.0+
mkanat: blocking4.0.2+
LpSolit: testcase+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch, v1 (2.08 KB, patch)
2011-04-28 10:55 PDT, Frédéric Buclin
no flags Details | Diff | Splinter Review
patch, v1.1 (2.19 KB, patch)
2011-04-28 10:57 PDT, Frédéric Buclin
mkanat: review+
Details | Diff | Splinter Review
patch, v1.2 (4.36 KB, patch)
2011-04-29 04:34 PDT, Frédéric Buclin
no flags Details | Diff | Splinter Review
patch, v1.3 (4.78 KB, patch)
2011-04-29 04:41 PDT, Frédéric Buclin
mkanat: review+
Details | Diff | Splinter Review

Description Dave Miller [:justdave] (justdave@bugzilla.org) 2011-04-27 21:03:12 PDT
Like the summary says, Bug.create() fails to error out if an invalid group is passed.  As far as I can tell from the source, this was intentional, so as to not disclose group names.  However, it has the unfortunate side effect of causing automated tools which submit confidential bugs to suddenly start filing them in public when a group name changes (or in this case when the API changed to require group names instead of IDs), which caused an information disclosure on mozilla.org this morning.
Comment 1 Max Kanat-Alexander 2011-04-27 21:08:35 PDT
I suppose this is my "I told you so" moment. :-)

I don't think we need to keep this confidential, but I do agree that it's a high-priority issue that needs to be fixed.
Comment 2 Dave Miller [:justdave] (justdave@bugzilla.org) 2011-04-27 21:13:11 PDT
And yeah, I guess I can yell at the webdev guys for using an "undocumented" API (because groups weren't documented at all in 3.6, but it was there in the source), but the name change thing could still bite someone in the future.
Comment 3 Frédéric Buclin 2011-04-28 03:10:16 PDT
We should reject the bug submission, and the error message should be similar to what we display in the web browser, i.e. that "either the group Foo doesn't exist, or you are not allowed to restrict bugs to this group in this product". This way, we don't disclose the existence of the group.

Decreasing severity as Bug.create() works as the doc says.
Comment 4 Frédéric Buclin 2011-04-28 10:55:31 PDT
Created attachment 528902 [details] [diff] [review]
patch, v1
Comment 5 Frédéric Buclin 2011-04-28 10:57:24 PDT
Created attachment 528905 [details] [diff] [review]
patch, v1.1

The comment is now obsolete.
Comment 6 Max Kanat-Alexander 2011-04-28 19:38:44 PDT
Comment on attachment 528905 [details] [diff] [review]
patch, v1.1

Review of attachment 528905 [details] [diff] [review]:

Awesome, looks great. :-)
Comment 7 Max Kanat-Alexander 2011-04-28 19:39:03 PDT
We should also make sure that update() behaves similarly.
Comment 8 Frédéric Buclin 2011-04-29 04:34:46 PDT
Created attachment 529074 [details] [diff] [review]
patch, v1.2

We also have to update the API documentation as we no longer silently ignore illegal group names.
Comment 9 Frédéric Buclin 2011-04-29 04:41:49 PDT
Created attachment 529075 [details] [diff] [review]
patch, v1.3

Still another place to fix, to mention that Bug.create can now return error 120.
Comment 10 Gervase Markham [:gerv] 2011-05-02 04:38:06 PDT
(In reply to comment #1)
> I suppose this is my "I told you so" moment. :-)

Max: I thought you were the one arguing for the "don't reveal group names" behaviour? Or am I confused?

Gerv
Comment 11 Max Kanat-Alexander 2011-05-03 18:50:34 PDT
(In reply to comment #10)
> Max: I thought you were the one arguing for the "don't reveal group names"
> behaviour? Or am I confused?

  No, I heavily believed that Bugzilla should be as explicit and loud as possible about invalid groups.
Comment 12 Max Kanat-Alexander 2011-05-03 18:51:55 PDT
Comment on attachment 529075 [details] [diff] [review]
patch, v1.3

Review of attachment 529075 [details] [diff] [review]:

Looks great. :-)
Comment 13 Frédéric Buclin 2011-05-06 13:44:53 PDT
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Bug.pm
modified Bugzilla/WebService/Bug.pm
modified Bugzilla/WebService/Constants.pm
modified template/en/default/global/user-error.html.tmpl
Committed revision 7808.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.0/
modified Bugzilla/Bug.pm
modified Bugzilla/WebService/Bug.pm
modified Bugzilla/WebService/Constants.pm
modified template/en/default/global/user-error.html.tmpl
Committed revision 7593.
Comment 14 Frédéric Buclin 2011-09-23 17:14:20 PDT
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/qa/4.2/
modified t/webservice_bug_create.t
Committed revision 212.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/qa/4.0/
modified t/webservice_bug_create.t
Committed revision 201.

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