Fix incorrect key handling in extended policy filtering

Summary:
Via HackerOne. The use of `$key` here should be `$extended_key`.

Exploiting this requires a very unusual group of objects to be subjected to extended policy checks. I believe there is no way to actually get anything bad through the policy filter today, but this could have been an issue in the future.

Test Plan:
  - Added a unit test which snuck something through the policy filter.
  - Fixed use of `$extended_key`.
  - Test now passes.

Reviewers: chad

Reviewed By: chad

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

Author: epriestley

src/applications/policy/__tests__/PhabricatorPolicyTestCase.php

@@ -285,6 +285,76 @@ public function testExtendedPolicyCycles() {
       pht('Extended Policy with Cycle'));
   }
 
+
+  /**
+   * Test bulk checks of extended policies.
+   *
+   * This is testing an issue with extended policy filtering which allowed
+   * unusual inputs to slip objects through the filter. See D14993.
+   */
+  public function testBulkExtendedPolicies() {
+    $object1 = $this->buildObject(PhabricatorPolicies::POLICY_USER)
+      ->setPHID('PHID-TEST-1');
+    $object2 = $this->buildObject(PhabricatorPolicies::POLICY_USER)
+      ->setPHID('PHID-TEST-2');
+    $object3 = $this->buildObject(PhabricatorPolicies::POLICY_USER)
+      ->setPHID('PHID-TEST-3');
+
+    $extended = $this->buildObject(PhabricatorPolicies::POLICY_ADMIN)
+      ->setPHID('PHID-TEST-999');
+
+    $object1->setExtendedPolicies(
+      array(
+        PhabricatorPolicyCapability::CAN_VIEW => array(
+          array(
+            $extended,
+            array(
+              PhabricatorPolicyCapability::CAN_VIEW,
+              PhabricatorPolicyCapability::CAN_EDIT,
+            ),
+          ),
+        ),
+      ));
+
+    $object2->setExtendedPolicies(
+      array(
+        PhabricatorPolicyCapability::CAN_VIEW => array(
+          array($extended, PhabricatorPolicyCapability::CAN_VIEW),
+        ),
+      ));
+
+    $object3->setExtendedPolicies(
+      array(
+        PhabricatorPolicyCapability::CAN_VIEW => array(
+          array(
+            $extended,
+            array(
+              PhabricatorPolicyCapability::CAN_VIEW,
+              PhabricatorPolicyCapability::CAN_EDIT,
+            ),
+          ),
+        ),
+      ));
+
+    $user = $this->buildUser('user');
+
+    $visible = id(new PhabricatorPolicyFilter())
+      ->setViewer($user)
+      ->requireCapabilities(
+        array(
+          PhabricatorPolicyCapability::CAN_VIEW,
+        ))
+      ->apply(
+        array(
+          $object1,
+          $object2,
+          $object3,
+        ));
+
+    $this->assertEqual(array(), $visible);
+  }
+
+
   /**
    * An omnipotent user should be able to see even objects with invalid
    * policies.
@@ -430,10 +500,12 @@ private function buildObject($policy) {
     $object->setCapabilities(
       array(
         PhabricatorPolicyCapability::CAN_VIEW,
+        PhabricatorPolicyCapability::CAN_EDIT,
       ));
     $object->setPolicies(
       array(
         PhabricatorPolicyCapability::CAN_VIEW => $policy,
+        PhabricatorPolicyCapability::CAN_EDIT => $policy,
       ));
 
     return $object;

src/applications/policy/filter/PhabricatorPolicyFilter.php

@@ -321,7 +321,7 @@ private function applyExtendedPolicyChecks(array $extended_objects) {
       $objects_in = array();
       foreach ($structs as $struct) {
         $extended_key = $struct['key'];
-        if (empty($extended_objects[$key])) {
+        if (empty($extended_objects[$extended_key])) {
           // If this object has already been rejected by an earlier filtering
           // pass, we don't need to do any tests on it.
           continue;
@@ -335,8 +335,8 @@ private function applyExtendedPolicyChecks(array $extended_objects) {
             // We weren't able to load the corresponding object, so just
             // reject this result outright.
 
-            $reject = $extended_objects[$key];
-            unset($extended_objects[$key]);
+            $reject = $extended_objects[$extended_key];
+            unset($extended_objects[$extended_key]);
 
             // TODO: This could be friendlier.
             $this->rejectObject($reject, false, '<bad-ref>');