@Qgil: Yes, just as safe. Once the patch lands we can enable herald without worry of leaking any pre-existing private tasks.

The patch has been merged, is this task completed?

In T493#962222, @Gilles wrote:

The patch has been merged, is this task completed?

It was reverted and there is ongoing progress to try again

This will be fixed when the next phabricator release goes live.

In T493#975222, @mmodell wrote:

This will be fixed when the next phabricator release goes live.

I don't know man, I think we probably can't enable herald for now. At least some overview and discussion of the remaining herald loopholes needs to happen. T76564: The options of the Security dropdown in Phabricator need to be clear and documented

What's stalling this?

In T493#1025120, @scfc wrote:

What's stalling this?

@scfc AFAIK it is essentially

https://phabricator.wikimedia.org/T76564

• Prevents users from CCing themselves via Herald [1] (on new creation yes but not if a user tries to public an issue accidentally)

Prevents users from CCing themselves via Herald on a security issue someone tries to set as public (this is done via a last check herald rule for now that means regular users can CC themselves on a security issue that someone tries to set as public. The public will be rescinded but their CC will remain, and because of intended CC functionality they will have edit/view permissions all from a sneaky herald rule. This is especially strange as setting an issue for view to public allows a user who has an "always cc me herald rule" to sneak in, but setting to edit seems not to. Bug in phab?) This also creates an issue with security-bug issues where if a user has an "always cc me" rule they cannot be removed from CC as the herald processing works before the permissions are removed continually readding them. That mean a user can effectively create a herald action that makes it impossible to remove their access to a security isue as CC == policy perms. Without this we cannot enable herald for all users.`

I know the problem, but what is stalling fixing it? Herald was proposed as a solution for Bugzilla functionality missing in Phabricator, and watching the Phabricator project is probably the most prominent of these, so there is certainly demand to enable Herald as soon as possible. Does something need to be done upstream? Are there UIs that need to be defined?

In T493#1032333, @scfc wrote:

I know the problem, but what is stalling fixing it? Herald was proposed as a solution for Bugzilla functionality missing in Phabricator, and watching the Phabricator project is probably the most prominent of these, so there is certainly demand to enable Herald as soon as possible. Does something need to be done upstream? Are there UIs that need to be defined?

Ah, in that case paging @mmodell

My understanding is that during the course of extracting Herald from the security extension for this purpose there were issues and a significant amount more time would need to be allocated to fully get it done. We could figure out stakeholders and say that the herald cc on hidden task on accidental public setting is not enough of a big deal to prevent herald. But mukunda has the best view of the effort side of things.

In T493#1032333, @scfc wrote:

Herald was proposed as a solution for Bugzilla functionality missing in Phabricator

What functionality specifically, and where was it proposed?

I don't have the time to search, but in the past 9 months I have replied in several occasions that even if Phabricator didn't have exactly XYZ Bugzilla functionality, with Herald you could accomplish more or less that or something better.

I think we should try to solve this task indeed, or at least assess what is the risk today. Then look at T630: Enabling Herald and see what can be done within these constraints. If enabling it for everybody is not an option today, we could start enabling it to some trusted users, who are likely to be basically the ones we trust more. (T630#1014570)

I don't know of a way to resolve this really, it'll require patching phabricator and it's not at all straightforward. I've spent countless hours (probably 60+) trying to resolve this one problem and it's mostly intractable.

Personally I don't think the "accidentally made public" case is worth considering, I would just enable herald and be done with it.

The only other route I can see to fully closing that hole would be to police the herald rules and prevent people (other than @Aklapper) from creating "CC Me on everything" rules. Even more feasible than policing the rules directly: just have a cron job that periodically deletes any such rules that get created.

I don't think that policing the herald rules is the way to go. I would probably try to game the system just ouf of curiosity ("Hey! Someone wants to hide something!"), and if someone files a security bug, the promise that information is not leaked is (much) more important than others' convenience.

@scfc: Software enforcement of those rules would provide some level of confidence that information is not leaked.

Does an upstream bugreport for this issue exist?

To answer my own question:

See-Also: https://secure.phabricator.com/T6367
See-Also: https://secure.phabricator.com/T4411

To follow up from IRC, I think the root problem behavior is this:

• Phabricator evaluates which Herald rules are permitted to run first, then runs all the rules.
• Herald performs the correct policy checks (mostly, see [1]), but checks the state of the object before rules execute.
• When tasks are filed here, they initially have open policy settings, so Herald evaluates those and allows a broad set of rules to run (i.e., most users can view most tasks, so most users can also run Herald rules on most tasks).

For most installs, this is fine and produces correct/safe behavior, because Herald rules can not normally change the object's policy settings, so evaluating policy rules first is correct.

On this (WMF's) install, a custom action may change the policy rules after they've already been evaluated. This issue ("herald does not reevaluate runnable rules mid-flight") doesn't have an upstream task, and also doesn't have a obvious remedy. In theory, we could maybe let rules tell the Adapter that policies have changed, and trigger re-evaluation of which remaining rules are permitted to run. However, global rules explicitly run last (in order to have the strongest effects for actions like "assign to") so this is generally a tangled mess. In the general case, it would also probably be very difficult for users to understand (the existing UI could not easily help them answer a question like "why didn't my rule run?" if the answer depends on execution order and reevaluation of policy settings, and building such a UI seems challenging). Broadly, this is probably something we don't want to pursue in the upstream without a much stronger set of use cases driving it.

I think a more correct fix is really to let custom fields (or some other early step) force policy (and likely Space) settings, rather than trying to drive this through Herald. Herald fundamentally acts in the wrong order.

At the lowest end, this might look like adding a hide parameter to the prefill settings in https://secure.phabricator.com/T3320 to hide specific fields. That would let you create a link like:

/task/create/?viewPolicy=A&editPolicy=B&space=C&projects=security&hide=viewPolicy,editPolicy,space,projects,custom:security

...which would hypothetically generate a form with "Space", "View Policy", "Edit Policy", "Projects" and "Security" correctly configured and not adjustable. The custom field ("custom:security") would then just render a piece of text like "If this is a security concern, click here to file a security issue" (rather than being a dropdown), and other documentation leading into this workflow could funnel users to the "security" form as well.

(You could sort of fake your way through this today by copying the form HTML somewhere and tweaking it, but obviously that's not super scalable/general.)

At a slightly less low-end, it's possible we could let selecting certain values in controls lock the values of other controls; that's relatively easy to do on the server side but I'm not especially excited about the amount of work involved on the client side, and the feature would be confusing without UI to support it (e.g., some of your inputs would be discarded without the interaction being obvious).

It generally feels better to me to push this toward being more of a modal choice before we show the user the form. Generally, reframe this two separate workflows ("file a security issue", "file something which isn't a security issue"), not as a single workflow where a lot of the parameters change midstream depending on a field setting.

Outside of this, there are quasi-related issues in the upstream, as per above:

• https://secure.phabricator.com/T6367 is about incorrectly emailing users who have been CC'd even if they can't see an object -- but they have to get on the CC list first. They normally can not do this with Herald (they can on this install).
• https://secure.phabricator.com/T4411 is about possibly letting "CC" grant view permission, which is related but not precisely material.

[1] Some object types currently perform incomplete checks, but mostly in ways that aren't hugely important, and this does not affect tasks; see: https://secure.phabricator.com/T7703

Note that you may not be able to specify "A", "B", and "C" correctly today, even if we magic "hide" into existence:

• Default view policy: probably not settable until http://secure.phabricator.com/T6860 allows you parameterize 'Acting User' as part of a policy template, since the user filing the task won't be able to see the thing otherwise.
• Default edit policy: probably settable.
• Spaces: don't exist yet.

@epriestley: Thank you once again for the very thorough and insightful response. I will have to buy you food and beers next time I'm in SF.

I like the idea of pre-configuring the form. If it was possible to parameterize the policy fields, we'd be in business.

Note: I wrote a custom herald rule that matches "users who are CC'd on task: T#, T#" which is what allows CC to grant view permission, though it has to be specified on each task. That's what the 'security' event listener does - it simply modifies the task policy to include the right set of rules during task creation on the server side.

Send email with recipient's language and access levels, not sender's (one of the moving pieces here) has been resolved upstream.

I don't think that necessarily solves this issue?

The only remaining issue, and it's really not solvable, is this scenario:

1. Task T is submitted as public
2. Some user U has a herald rule that adds them to the CC'd list of all new tasks
   1. since T is a public task, this is allowed
   2. now U is subscribed to T
3. Later on, someone edits T to make it private, but forgets to remove CC'd users (such as U) who maybe should not not have access to T
4. Due to the intentional "CC'd users can view" policy, U will still have access to the task after making it private unless the subscription is manually removed.

Note: This is only true for tasks marked as "Software Security bug." Other values of the security setting do not add "CC'd users can view" to the ACL. So "Sensitive" and "Access Request" tasks are not susceptible to this edge case.

@Krenair: ^

Now, my opinion is that forgetting to remove someone from the CC'd list is user error and not a software bug. The way to fix it is to forget the whole "CC'd users can view" magic which is the source of all this trouble. (That is, remove that "feature" and have policies that aren't so easy to get wrong)... Or we can just live with the consequences and be sure to remember to remove CCs when making a public task into a private one. The possibility of not realizing that CC'd users can view the task is a good example of why magic behavior is sdometimes/often harmful.

If a task is created as Security !None (not public) and regular users cannot catch it via Herald rules, then this task is fixed.

Tasks created as public and then changing policies is another story that should not block Herald for everybody, I agree.

Yeah, I agree that's basically a non-issue here, IIRC you had to do similar things in BZ with the default wikibugs-l subscriber etc.?

Anyway, I don't understand how you believe the actual issue here (user's Herald rules kicking in and granting user access before global security herald rule can act) is resolved.

In T493#1355583, @mmodell wrote:

The only remaining issue, and it's really not solvable, is this scenario:

1. Task T is submitted as public
2. Some user U has a herald rule that adds them to the CC'd list of all new tasks
   1. since T is a public task, this is allowed
   2. now U is subscribed to T
3. Later on, someone edits T to make it private, but forgets to remove CC'd users (such as U) who maybe should not not have access to T
4. Due to the intentional "CC'd users can view" policy, U will still have access to the task after making it private unless the subscription is manually removed.

Agree with Krenair, this exact scenario isn't a problem. When a person makes a public issue private (at least, anyone who used bugzilla previously), they expect all CC's to have access, and can remove anyone if it's an issue.

There is definitely a problem if it's possible for a user to setup a Herald rule that adds them to issues, and when someone else submits a new task with the security flag set and the first user's Herald rule gets run first so they get visibility to the task before Herald sets the ACL. From what I can tell of Evan's comments, and the absence of any link to where it's fixed, I don't think this is resolved, right? If that's the case, then this task still needs to block T630.

@csteipp: Herald no longer preempts the security setting - I tested thoroughly and the security policy gets set before herald ever touches the task. This is one of the main reasons we went with an event listener which is discouraged by upstream, instead of depending on a herald rule alone. The event listener fires before the task is saved so it gets to modify the security policy before herald has a chance to see the task.

Ah, okay, thanks for clarifying @mmodell. Let's test it on phab-01.wmflabs.org (assuming it all works the same there?) and, if all goes fine, resolve this task, and enable Herald?

... alternatively, production, where it still seems broken: T102150

@Krenair: I tested on phab-01 and everything looked good, but I just tested here in production and it's still adding @Aklapper via herald...

So obviously I've missed something. I'll test further and see if I can get consistently reproducible behavior.

In T493#1357543, @Krenair wrote:

... alternatively, production, where it still seems broken: T102150

@Aklapper is a member of the security group, so his herald rule is valid and he's allowed to be CC'd.

We need an unprivileged user to test this properly.

In T493#1357511, @mmodell wrote:

@csteipp: Herald no longer preempts the security setting - I tested thoroughly and the security policy gets set before herald ever touches the task. This is one of the main reasons we went with an event listener which is discouraged by upstream, instead of depending on a herald rule alone. The event listener fires before the task is saved so it gets to modify the security policy before herald has a chance to see the task.

Cool, can you link to the patches that did this, so we have that documented?

In T493#1357561, @mmodell wrote:

We need an unprivileged user to test this properly.

I have Stype_and_Co.-WMF (unprivileged account) for testing stuff like that. Let me know if you need that account to add some rules.

In T493#1357569, @csteipp wrote:

Cool, can you link to the patches that did this, so we have that documented?

I'm pretty sure it's been fixed since 120aab85a7f81dc691a7828b2ba1bb3fd9a25345

I hadn't thought about this in a while until I was testing the other day and I noticed that the herald rules were blocked on new secure tasks (in my dev environment) so that reminded me to revive this discussion. After further testing it seemed pretty solid and that's when I brought it up here.

@Aklapper: I'm removing you from the security project so that we'll have a definitive test in production. I'll add you back afterward.

{T102153}

Great success! herald was blocked.

I'd really like to see a clear test of this. @Aklapper, I'm going to remove you again and rerun the test. Sorry to pick on you :)

Ok, T102189 shows that Andre wasn't added while he was removed from the Security group.