recaptime.dev / phorge
@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge
phorge phabricator
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

Never raise policy exceptions for the omnipotent viewer

Summary:
Fixes T4109. If a revision has a bad `repositoryPHID` (for example, because the repository was deleted), `DifferentialRevisionQuery` calls `didRejectResult()` on it, which raises a policy exception, even if the viewer is omnipotent. This aborts the `MessageParser`, because it does not expect policy exceptions to be raised for an omnipotent viewer.

Fix this in two ways:

# Never raise a policy exception for an omnipotent viewer. I think this is the expected behavior and a reasonable rule.
# In this case, load the revision for an omnipotent viewer.

This feels a little gross, but it's the only place where we do this in the codebase right now. We can clean this up later on once it's more clear what the circumstances of checks like these are.

Test Plan: Set a revision to have an invalid `repositoryPHID`, ran message parser on it, got a clean parse.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4109

Differential Revision: https://secure.phabricator.com/D7603

epriestley c2079640 200b51df

+16
+7
src/applications/differential/query/DifferentialRevisionQuery.php
··· 429 429 continue; 430 430 } 431 431 432 + if ($this->getViewer()->isOmnipotent()) { 433 + // The viewer is omnipotent. Allow the revision to load even without 434 + // a repository. 435 + $revision->attachRepository(null); 436 + continue; 437 + } 438 + 432 439 // The revision has an associated repository, and the viewer can't see 433 440 // it, and the viewer has no special capabilities. Filter out this 434 441 // revision.
+9
src/applications/policy/filter/PhabricatorPolicyFilter.php
··· 259 259 return; 260 260 } 261 261 262 + if ($this->viewer->isOmnipotent()) { 263 + // Never raise policy exceptions for the omnipotent viewer. Although we 264 + // will never normally issue a policy rejection for the omnipotent 265 + // viewer, we can end up here when queries blanket reject objects that 266 + // have failed to load, without distinguishing between nonexistent and 267 + // nonvisible objects. 268 + return; 269 + } 270 + 262 271 $capobj = PhabricatorPolicyCapability::getCapabilityByKey($capability); 263 272 $rejection = null; 264 273 if ($capobj) {