Page MenuHomePhabricator
Create Task
Maniphest T13628

Provide "Foist Upon", an inverse operation to "Commandeer"
Closed, ResolvedPublic
Actions

Description

See https://twitter.com/evanpriestley/status/1367180657458176001.

Primarily, I think "Foist Upon" is funny. However, it's also inconsistent that "Commandeer" is a one-way trap door: modern Phabricator generally tries to make it easy to undo mistakes, and "Commandeer" can't be undone.

There are probably also some off-label use cases where, for example, bots create revisions on behalf of users. I'm not thrilled about these, but "ability to undo a reasonable error" is good enough on its own.

My most significant concern here is that the "Scuttle Task" button in Maniphest was previously a source of significant confusion because of the obscurity of the word "Scuttle". "Scuttle" is likely more obscure than "Foist", and the most common use of "scuttle" as a verb is "scuttle across the floor", not "scuttle the ship", so this may not be an issue. There would also be a better contextual hint (you'd be prompted for a new revision author).

Still, this could perhaps be mitigated by turning "phabricator.serious-business" into a "Tone/Flavor" option in Config. The only major issue with this is that a handful of effects are global (notably, "silly" task statuses), but I think these could be removed without any real loss.

Revisions and Commits

rP Phabricator
D21595rPfceb9a3f9e1d Hide the "Author" field in Differential on the web create workflow
D21592rPf3caf7e9fcd6 Improve rendering behavior for "foist upon" with an empty tokenizer
D21591rPdec14136844d Provide a more general "Author" transaction for Differential

Related Objects

Event Timeline

epriestley triaged this task as Low priority.Mar 3 2021, 6:51 PM
epriestley created this task.
epriestley added a revision: D21591: Provide a more general "Author" transaction for Differential.Mar 3 2021, 11:47 PM
epriestley added a commit: rPdec14136844d: Provide a more general "Author" transaction for Differential.Mar 3 2021, 11:49 PM
epriestley added a revision: D21592: Improve rendering behavior for "foist upon" with an empty tokenizer.Mar 4 2021, 12:01 AM
epriestley added a commit: rPf3caf7e9fcd6: Improve rendering behavior for "foist upon" with an empty tokenizer.Mar 4 2021, 12:08 AM
epriestley added a revision: D21595: Hide the "Author" field in Differential on the web create workflow.Mar 4 2021, 5:44 PM
epriestley added a commit: rPfceb9a3f9e1d: Hide the "Author" field in Differential on the web create workflow.Mar 4 2021, 11:50 PM
epriestley mentioned this in 2021 Week 10 (Early March).Mar 5 2021, 9:29 PM
epriestley closed this task as Resolved.Mar 10 2021, 6:21 PM
epriestley claimed this task.

This promoted and has been in the Phacility cluster for a few days without issues.

cspeckmim added a subscriber: cspeckmim.Mar 11 2021, 7:38 PM

Adding a data point, "Foist Upon" was a surprise hit among the developers here and additionally we have successfully utilized the conduit API to take advantage of foisting

There are probably also some off-label use cases where, for example, bots create revisions on behalf of users. I'm not thrilled about these, ...

oops 😬

epriestley added a comment.Mar 11 2021, 7:46 PM

I don't think those use cases are necessarily bad, there's just a little bit of a slippery slope to, say, "Git sure looks a lot like a database" and then support issues like "it's hard to manage my 3,500 automated review requests".

There are probably more good use cases for API "Foist Upon" than bad use cases, the bad ones just might make my life harder some day.

ptarjan added a subscriber: ptarjan.Oct 19 2021, 5:48 PM

Is there a way to disable this feature? Our security team has noticed that with this feature we can land code with just a single person's machine being compromised (we rely on an attacker needing two machines to deploy code as a safety mechanism). I.e. You make a revision, Foist it on someone, Approve it, then arc land it as the other person (saying y to the prompt).

epriestley added a comment.Oct 19 2021, 6:18 PM

I believe it is extremely difficult to configure Phabricator to provide the assurance you describe, particularly if arc land does anything. If you are actually providing this guarantee ("an attacker needs two machines"), you can likely add a clause to the large amount of custom code you've written to prevent self-foisting while still supporting other foisting use cases. If you haven't written a large amount of custom code, I suspect an attacker can fairly easily deploy with one machine without using "Foist Upon".

You may want to try these non-foisting workflows:

  1. Make a revision, skip the foisting and approval, git push it.
  2. Make a revision, edit the commit message to say Differential Revision: D1234, where D1234 is a previous, valid, accepted and landed revision legitimately authored by owner of the laptop you've stolen, git push it.
  3. Make a trivial revision ("typo fix"), get approval in the normal way from an unwitting reviewer, edit it to do arbitrarily bad things, git push it.

This class of guarantee is discussed somewhat in T182. There was never much customer interest in actually providing systems which could resist adversarial engineers or attackers who can steal laptops, and Phabricator generally does not provide any defense against these classes of attacks.

epriestley added a comment.Oct 19 2021, 6:25 PM

...and:

  1. As Alice, commandeer a revision authored by Baliey and reviewed by Claire. Edit it locally to do arbitrary bad things, then git push it.
  2. Make a commit, edit the commit message to say Differential Revision: D1234, where D1234 is a current, valid, accepted revision authored by anyone, then git push it.

(Narrowly, no, there is no way to disable "Foist Upon" or the mutability of revision authors provided alongside it, short of reverting those changes or commenting them out, etc.)

Phabricator · Built by Phacility