Page MenuHomePhabricator

When performing complex edits, pause sub-editors before they publish to propagate "Must Encrypt" and other state
ClosedPublic

Authored by epriestley on Mar 13 2019, 4:07 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Apr 11, 7:59 AM
Unknown Object (File)
Fri, Apr 5, 4:42 AM
Unknown Object (File)
Wed, Apr 3, 6:40 PM
Unknown Object (File)
Thu, Mar 28, 8:56 PM
Unknown Object (File)
Feb 19 2024, 11:43 AM
Unknown Object (File)
Feb 16 2024, 3:07 AM
Unknown Object (File)
Feb 15 2024, 1:08 AM
Unknown Object (File)
Feb 13 2024, 3:35 PM
Subscribers
None

Details

Summary

See PHI1134. Previously, see T13082 and D19969 for some sort-of-related stuff.

Currently, edits work roughly like this:

  • Suppose we're editing object X, and we're also going to edit some other object, Y, because X mentioned Y or the edit is making X a child or parent of Y, or unblocking Y.
  • Do the actual edit to X, including inverse edits ("alice mentioned Y on X.", "alice added a child revision: X", etc) which apply to Y.
  • Run Herald rules on X.
  • Publish the edit to X.

The "inverse edits" currently do this whole process inline, in a sub-editor. So the flow expands like this:

  • Begin editing X.
  • Update properties on X.
    • Begin inverse-edge editing Y.
    • Update properties on Y.
    • Run (actually, skip) Herald rules on Y.
    • Publish edits to Y.
  • Run Herald rules on X.
  • Publish edits to X.

Notably, the "Y" stuff publishes before the "X" Herald rules run. This creates potential problems:

  • Herald rules may change the name or visibility policy of "X", but we'll publish mail about it via the edits to Y before those edits apply. This is a problem only in theory, we don't ship any upstream rules like this today.
  • Herald rules may "Require Secure Mail", but we won't know that at the time we're building mail about the indirect change to "Y". This is a problem in practice.

Instead, switch to this new flow, where we stop the sub-editors before they publish, then publish everything at the very end once all the edits are complete:

  • Begin editing X.
  • Update properties on X.
    • Begin inverse-edge editing Y.
    • Update properties on Y.
    • Skip Herald on Y.
  • Run Herald rules on X.
  • Publish X.
    • Publish all child-editors of X.
      • Publish Y.
Test Plan
  • Created "Must Encrypt" Herald rules for Tasks and Revisions.
  • Edited object "A", an object which the rules applied to directly, and set object "B" (a different object which the rules did not hit) as its parent/child and/or unblocked it.
  • In bin/mail list-outbound, saw:
    • Mail about object "A" all flagged as "Must Encrypt".
    • Normal mail from object B not flagged "Must Encrypt".
    • Mail from object B about changing relationships to object A flagged as "Must Encrypt".

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Herald rules may "Require Secure Mail", but we won't know that at the time we're building mail about the indirect change to "Y". This is a problem in practice.

Specifically, the problem this creates is that we may send mail from object T999 which says: alice added a subtask: T123 Description of an important security issue.

Although mail about object T123 is correctly flagged as "Must Encrypt", the indirect mail from T999 is not, and can leak information about T123.

This also probably fixes a couple of minor bugs. For example, I suspect this is currently a bug, although I haven't verified it:

If you use a silent bulk edit to close tasks, parents of those tasks still send non-silent notification email that a subtask was closed.

Moving everything to use a single method for generating a new sub/related editor should help solve this stuff in the future.

amckinley added inline comments.
src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
1339

All hail Herlad!

This revision is now accepted and ready to land.Mar 18 2019, 6:48 PM
This revision was automatically updated to reflect the committed changes.