1567526 - For security group bugs, Phabricator should CC reviewers to the bug

Status: NEW

(Conduit :: Phabricator, enhancement, P2)

Description

[:Gijs (he/him)]

Previously: https://groups.google.com/forum/#!msg/mozilla.dev.platform/En5hlbApMI0/AMoKw4lcAAAJ

Previously: https://bugzilla.mozilla.org/show_bug.cgi?id=1440864

Also: https://phabricator.services.mozilla.com/D33959#1160188

While we initially have gone with adding BMO CC list folks as subscribers on patches, the unfortunate consequence is that if you moz-phab submit a patch with r?foo, and foo is not CC'd, now they can see the patch but not the bug (unless they have sec group access). That's often a problem for the review.

I'd like phab to CC reviewers. Although we previously decided on a BMO->phab sync only, the newsgroup thread does say "We can and will revisit this after Phabricator goes live for everyone.".

I find that in practice, I often get review requests where I end up never being CC'd on the bug. This saves me some bugmail, but is occasionally annoying - but I can always CC myself if I'm interested in developments on the bug. For sec bugs however, this is not possible for most folks, which is particularly inconvenient. Given the commandline integration we have with phab at the moment, it would be nice if patch submission immediately took care of ensuring the reviewer could see the bug. If we don't want to do this in phab.s.m.c, we could potentially do it in moz-phab (in which case I guess the bug needs moving, but that's not my call).

Comment 1

[David Lawrence [:dkl]]

Will take a look at implementing this. Technically we do not need Phab to send anything to BMO for this to work so it will not be 2 way sync. When a revision is created or updated, BMO will notice the change and at that time we can look at the author and reviewer and add them to the cc list of the bug if they are not already. We will however not sync the subscriber list both ways. For that we will still require a user to be added to the cc list of the bug first.

Comment 2

[Daniel Veditz [:dveditz]]

Could a person without access to a bug create a revision and claim it's for a secure bug and get themselves CC'd to arbitrary bugs that way? If the author doesn't already have bug access I don't know what's going on. How did they know what to patch?

Adding reviewers to a bug makes sense since the author (assuming they have access) could just add them first. This saves some steps and prevent problems when this step gets forgotten. A confirmation for security bugs might be nice, conceptually similar to the question about CC'ing the reporter of a dupe bug to a security bug.

Comment 3

[David Lawrence [:dkl]]

(In reply to Daniel Veditz [:dveditz] from comment #2)

Could a person without access to a bug create a revision and claim it's for a secure bug and get themselves CC'd to arbitrary bugs that way? If the author doesn't already have bug access I don't know what's going on. How did they know what to patch?

Adding reviewers to a bug makes sense since the author (assuming they have access) could just add them first. This saves some steps and prevent problems when this step gets forgotten. A confirmation for security bugs might be nice, conceptually similar to the question about CC'ing the reporter of a dupe bug to a security bug.

Currently when a user submits a new revision and adds a bug id to it either through the Phab UI or via command line, Phab will check with BMO to see if the author can see the bug and will reject the bug id if they cannot or it doesn't exist. The check uses a special bespoke API call in BMO that only the phab bot user can access using an API key.

For this bug, I am fine with limiting to just adding reviewers to the cc list and not bother with the author. They author should already be able to see the bug since we do the bug id check at creation time and they are more than likely already attached to the bug somehow.

Sounds OK?

Comment 4

[Daniel Veditz [:dveditz]]

They author should already be able to see the bug since we do the bug id check at creation time and they are more than likely already attached to the bug somehow.

Ah, that's right. The author doesn't have to be the assignee, they could be someone who only has access to the bug by being CC'd. So if I get accidentally CC'd on a bug I shouldn't be able to see, or get removed later because the bug veers into security issues that were not originally apparent, If I've added a patch I can always request a review from myself to get added back to the bug? Or from a confederate account if we have a simple check that reviewer != author?

Is it hard/expensive to verify that the author still currently has access before CC'ing reviewers to the bug?

As you said "more than likely" this won't be the case, but it is a hole. If it can't easily be fixed please file a separate security bug about it (and I guess make this comment private) so we can fend off future bug bounty hunters who rediscover it.

Comment 5

[David Lawrence [:dkl]]

So should we just not do the automation and just leave it to the revision author to add the reviewer to the cc list of the bug on request? This removes the ability for someone to use the ho,e to gain access to a secure bug.

Comment 6

[Daniel Veditz [:dveditz]]

0. "Manually add reviewers to bug CC list" (comment 5) is the current state; that amounts to WONTFIX this bug.

1. "Always CC reviewers on bug" is this enhancement. Mostly OK because the author had to have bug access at the time the patch was added, and probably still does. Edge case where they don't, which opens a small security hole where they could regain access to the bug from which they may have been explicitly removed for good reason.

2. "CC reviewers on bug if author still has access". Would satisfy this enhancement and close the hole. (comment 4)

#2 is my strong preference. Since there's code that makes the author check when patches are created could we re-use or copy that check for this case before proceeding to CC the reviewers?

I'm sympathetic to Gijs' request so I'm reluctant to wontfix if we don't have to. Is there a devs involved in a bug can disassociate a revision from a bug, so that if we do un-CC someone because of bad behavior/risk we can also manually get rid of old bad patches? The only real control I see is marking the associated bugzilla patch "Obsolete", and I'm guessing checking for that won't be any easier than checking if the patch author has bug access in the first place (maybe more work, in fact).

If the check is not feasible we could weigh how often this situation comes up and how likely a hole this represents if we go with #1. It's not going to come up often. The patch author knows whatever is in the bug up to the point we want to make it a secure bug. Is it feasible to train developers that if they start un-CC'ing people from a security bug they should check they haven't authored a patch (in most cases they haven't) and if they have, we have to close the bug and fork further work into a new bug. That all seems error prone and inconvenient.

Comment 9

[David Lawrence [:dkl]]

In an attempt to get my work load down to a manageable amount, I am un-assigning some bugs from my list. In the future when I assign a bug to myself, it will mean I am actively working on it and has become a priority. This action doesn't mean you bug will not be addressed, but will reassessed as time allows. Please comment if this is not acceptable. Thanks.