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.

Fix an error in the PolicyFilter algorithm

Summary:
`PhabricatorPolicyFilter` has a bug right now where it lets through objects incorrectly if:

- the query requests two or more policies;
- the object satisfies at least one of those policies; and
- policy exceptions are not enabled.

This would be bad, but there's only one call in the codebase which satisfies all of these conditions, in the Maniphest batch editor. And it's moot anyway because edit operations get another policy check slightly later. So there is no policy/security impact from this flaw.

(The next diff relies on this behavior, which is how I caught it.)

Test Plan:
- Added a failing unit test and made it pass.
- Grepped the codebase for `requireCapabilities()` and verified that there is no security impact. Basically, 99% of callsites use `executeOne()`, which throws anyway and moots the filtering.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

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

epriestley faaaff0b 5ca84589

+33 -4
+29
src/applications/policy/__tests__/PhabricatorPolicyTestCase.php
··· 227 227 } 228 228 } 229 229 230 + public function testMultipleCapabilities() { 231 + $object = new PhabricatorPolicyTestObject(); 232 + $object->setCapabilities( 233 + array( 234 + PhabricatorPolicyCapability::CAN_VIEW, 235 + PhabricatorPolicyCapability::CAN_EDIT, 236 + )); 237 + $object->setPolicies( 238 + array( 239 + PhabricatorPolicyCapability::CAN_VIEW 240 + => PhabricatorPolicies::POLICY_USER, 241 + PhabricatorPolicyCapability::CAN_EDIT 242 + => PhabricatorPolicies::POLICY_NOONE, 243 + )); 244 + 245 + $filter = new PhabricatorPolicyFilter(); 246 + $filter->requireCapabilities( 247 + array( 248 + PhabricatorPolicyCapability::CAN_VIEW, 249 + PhabricatorPolicyCapability::CAN_EDIT, 250 + )); 251 + $filter->setViewer($this->buildUser('user')); 252 + 253 + $result = $filter->apply(array($object)); 254 + 255 + $this->assertEqual(array(), $result); 256 + } 257 + 258 + 230 259 /** 231 260 * Test an object for visibility across multiple user specifications. 232 261 */
+3 -3
src/applications/policy/filter/PhabricatorPolicyFilter.php
··· 156 156 // If we're missing any capability, move on to the next object. 157 157 continue 2; 158 158 } 159 - 160 - // If we make it here, we have all of the required capabilities. 161 - $filtered[$key] = $object; 162 159 } 160 + 161 + // If we make it here, we have all of the required capabilities. 162 + $filtered[$key] = $object; 163 163 } 164 164 165 165 return $filtered;
+1 -1
src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php
··· 109 109 * @task config 110 110 */ 111 111 final public function shouldRaisePolicyExceptions() { 112 - return (bool) $this->raisePolicyExceptions; 112 + return (bool)$this->raisePolicyExceptions; 113 113 } 114 114 115 115