TypeError w/ arc diff when build plan view policy does not contain user

$ arc diff
Linting...
No lint engine configured for this project.
Running unit tests...
No unit test engine is configured for this project.
 SKIP STAGING  No staging area is configured for this repository.
 Exception
TypeError: Argument 1 passed to HarbormasterBuildPlanBehavior::getPlanOption() must be an instance of HarbormasterBuildPlan, null given, called in .../phabricator/src/applications/differential/storage/DifferentialRevision.php on line 873

I also got bug reports from multiple users that they couldn’t access their draft until the builds finished. I guess it is pretty much the same thing at another location:

Argument 1 passed to HarbormasterBuildPlanBehavior::getPlanOption() must be an instance of HarbormasterBuildPlan, null given, called in .../phabricator/src/applications/differential/storage/DifferentialRevision.php on line 881

edit: I should add that I would like to keep the view policy of the build plans strict as they contain HTTP requests with tokens in the URL that I prefer not to make “public”.

Reproduction Instructions
Do a arc diff for a repo with a bulid plan that does not contain you in its view policy.

Phabricator/Arcanist Version

Client:

arcanist ce7bd679804c7a334fc3411e8e8d0da175bf24f4 (30 Apr 2020)
PHP 7.3.14

Server:

phabricator fc018dfaadaf Sun, Apr 26 d05d8f655896
arcanist ce7bd679804c Thu, Apr 30 68f050bd14e0

Other Version Information

Binary Version Path
php 7.2.30 fpm-fcgi
diff 3.3 /usr/bin/diff
git 2.16.6 /usr/share/phabricator/support/bin/git
hg Not Available
pygmentize 1.4 /usr/bin/pygmentize
svn 1.7.14 /usr/bin/svn

Ah, thanks! I think this is likely the cause for https://secure.phabricator.com/T13526, which I caught in the logs but couldn’t immediately figure out how to reproduce.

I may have stabilized this behavior in https://secure.phabricator.com/D21194.

The way this works right now (the code allows you to load a Build even if you can not see the Build Plan) is generally surprising and not consistent with how other modern applications and objects work. Some aspects of Harbormaster itself are already inconsistent in handling this case.

In the future, I suspect I’ll make these changes to try to make this stuff easier to work with:

  • Provide better tools for hiding tokens/credentials when using generic build plans, so you don’t need to restrict read access to a build plan purely to hide credentials. Non-generic plans already use Passphrase to store tokens, but the “Run Command” and “Make HTTP Request” build plans are complicated to make work in the same way (see also https://secure.phabricator.com/T9608).
  • Make it so that users who can not see a Build Plan can not (generally) see the Builds it generates. Confusingly, the “Build” page already works like this.
  • Make it so that users who can not see a Build can still see that it exists and its effects on a Buildable. For example, if your revision is being held in draft by a build you don’t have permission to see, the UI would communicate that you’re being held up by “Restricted Build (Build Plan 123)”. Confusingly, the “Buildable” page already sort of works like this.

In the meantime, I stabilized the three ways to reach this that I could find.

Two are mostly straightforward: you can’t issue commands to a Build if you can’t see the Build Plan. This is the correct result.

One has a slightly weird side effect: if you can not see a Build Plan, your revisions can not be affected by the “Hold Draft” setting in “Plan Behaviors”, and things will act as though the behaviors have the default values. This may not be ideal, but should be better than fataling.

This stabilization fix is now available in master and stable. Let me know if you’re still seeing other issues after upgrading – there may be additional ways to reach this state which I wasn’t immediately able to hunt down.

1 Like

Thanks! I will let you know in case we still reach this state somehow.