290229 - attachment.cgi action=enter does not do groupset check on flag requestee

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

attachment.cgi does a check that attachment flag requestees can access
group-confidential bugs when the attachment flag request is being set while
editing an attachment (action=edit), but it does not do the same check when the
attachment flag request is being set while creating a new attachment
(action=enter).  It probably should do the check for action=enter as well, and
it should certainly be consistent.

This created the bizarre situation in bug 290038 (atachment 180621) that dveditz
was able to make a request to mconnor while mconnor was not capable of viewing
the bug (and thus probably not capable of responding to the request).  It also
meant that when I went to deal with the superreview request at the same time,
*I* got the error message from
https://bugzilla.mozilla.org/attachment.cgi?id=180621&action=edit even though I
wasn't changing the problematic request at all -- I was plussing the separate
superreview request.  (I had to open the bug in a new tab, cc: mconnor, and
resubmit my superreview+ change and comments.)


Steps to reproduce (I expect, didn't actually test this)
 1. create a group-confidential bug
 2. create an attachment to the bug and in the attachment creation form, request
review on the attachment from somebody who can't access the bug

Expected results: error saying that the requestee can't access the bug (just
like for making the same request for action=edit)

Actual results: No error, but anybody who edits the attachment without changing
the problematic request does get the error.

Comment 1

[Frédéric Buclin]

I cannot reproduce this bug using the latest CVS version. And I cannot have a
look at the bug activity table in 290038 due to restrictions. :(

It's strange, looking at attachment.cgi and FlagType.pm, the check is there,
both in 2.19.1 and on the trunk. Maybe is there some local customization on
b.m.o? justdave, which version of these two files are we using on b.m.o? myk, do
you remember having fixed such a bug about requestee?

dbaron, are you sure mconnor has been requested to review the patch *before* the
bug got the security flag?

Comment 2

[Frédéric Buclin]

(In reply to comment #1)
> dbaron, are you sure mconnor has been requested to review the patch *before* the
> bug got the security flag?

err... I meant *after*.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The bug was filed with the security flag, so yes.

Comment 4

[Dave Miller [:justdave]]

File: FlagType.pm       Status: Up-to-date
   Working revision:    1.10
File: attachment.cgi    Status: Up-to-date
   Working revision:    1.63

There were four flags set at once on that bug, is it by any chance passing the
check if any one of the addresses passes?

[e-mail] changed:

 Attachment #180621 [details] [diff] |                           |review?(mconnor@steelgryphom.
               Flag |                           |com),
                    |                           |superreview?(dbaron@mozillafo
                    |                           |undation.org), approval1.7.7?,
                    |                           |approval-aviary1.0.3?

Comment 5

[Frédéric Buclin]

I (indirectly) discussed that problem with myk a few days ago (April 5). But I
did not make the relation with the requestee validation problem; sorry for that:

(22:05:25) LpSolit: myk, do you agree with me when I say that $bugid is
undefined in attachment.cgi, line 131?
(22:06:12) LpSolit: myk, validateID is the function which defines $bugid, but
this function is not called with $action==insert
(22:07:08) LpSolit: myk, this makes Flag::validate() and FlagType::validate()...
invalid
(22:11:50) myk: LpSolit: looks like you are right
(22:12:28) myk: LpSolit: does wicked's patch in bug 238870 fix the problem?


After several tests I did locally, I can now answer myk's question: YES! :)

I checked this patch in one week ago. Marking fixed!