71767 - Provide summary view for secure bugs

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Jacob Steenhagen]

The addition of the bug summary to the page <title> and the tooltips of bug
links initially had the unwanted side-effect of leaking this information for
secure bugs.  The "quick fix" was to not show the summary if the bug is secure,
regardless of the user's permissions.

Effected files: show_bug.cgi (v1.14), duplicates.cgi (v1.4), globals.pl (v1.85)

This should be fixed for 2.14 to allow users with proper permissions to benefit
from this information.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch to bug_form.pl and show_bug.cgi that fixes problem

Comment 2

[Myk Melez [:myk] [@mykmelez]]

The only downside to this patch is that the ValidateBugID function, which this
patch uses to determine if the user is authorized to view the bug, does not
provide an error message to the user which is as helpful as the one currently
being provided.  In particular, the current error message provides a link for
logging in if the user isn't logged in.  The solution to this is probably to
make ValidateBugID provide a friendlier error message.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch w/better "unauthorized" error message

Comment 4

[Myk Melez [:myk] [@mykmelez]]

The other thing this latest patch does is have &ValidateBugID use local
variables $usergroupset and $userid instead of potentially setting the values of
the global variables $::usergroupset and $::userid.  The former approach isn't
terrible, but the new way conforms to better coding practices.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Adding "review" keyword to get these on the radars of reviewers (if they aren't
already).

Comment 6

[Jacob Steenhagen]

Two comments. (I always seem to have two comments :)

By removing all checks from bug_form.pl this has the potential to leak the
information from process_bug.cgi.  If a user somehow gets a secure bug into
thier buglist cookie and then modifies the bug before it in the list (or any bug
if the secure bug is the first/only bug in the cookie) then process_bug.cgi will
gladly spit the entire contents of the bug out to the user (at least I'm 90%
sure that's the case, although I haven't looked at the code yet to be sure).

The other is whitespace ;) 
I just noticed that you seem to be using two space indents, but the rest of
bugzilla (and the emacs modeline in the files) use 4 spaces.

Comment 7

[Myk Melez [:myk] [@mykmelez]]

-----
By removing all checks from bug_form.pl this has the potential to leak the
information from process_bug.cgi.
-----

The solution to this problem is to patch process_bug.cgi to validate the next ID
on the user's bug list.

-----
The other is whitespace ;) 
I just noticed that you seem to be using two space indents, but the rest of
bugzilla (and the emacs modeline in the files) use 4 spaces.
-----

Darn, I was hoping no one would notice. :-)  Ok, the next patch will indent
blocks 4 spaces.  I'll leave multi-line statements indented two spaces since
there seems to be no consensus among the code and because it seems to work well
to distinguish those statements from blocks.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Attachment: also patches process_bug.cgi

Comment 9

[Jacob Steenhagen]

Patching process_bug.cgi sounds like the right solution, but I think I'd rather see 
ValidateBugID($::next_bug);
Added at line 1038.  To me that seems more obvious when looking at the code that
this has been though of and also seems less error prone (such as modifications
in the future).

Multi-line statements seem to just indent to whatever seems right at the time
(most of the time they line up w/the '(' if it's a sub or similar).  I'd say the
ones you're doing are fine w/2 spaces :)

Comment 10

[Tara Hernandez]

This looks good.  I couldn't reproduce the behaviour Jake was concerned about, 
though if someone can, feel free to reopen the bug.

Checking in Myk's patch and closing out.

Comment 11

[Dave Miller [:justdave]]

Moving to Bugzilla product