diff --git a/src/applications/differential/query/DifferentialRevisionQuery.php b/src/applications/differential/query/DifferentialRevisionQuery.php index a64e2efea3..85013e4a65 100644 --- a/src/applications/differential/query/DifferentialRevisionQuery.php +++ b/src/applications/differential/query/DifferentialRevisionQuery.php @@ -730,7 +730,7 @@ final class DifferentialRevisionQuery private function loadCursorObject($id) { $results = id(new DifferentialRevisionQuery()) - ->setViewer($this->getViewer()) + ->setViewer($this->getPagingViewer()) ->withIDs(array((int)$id)) ->execute(); return head($results); diff --git a/src/applications/maniphest/query/ManiphestTaskQuery.php b/src/applications/maniphest/query/ManiphestTaskQuery.php index e3aa8075fb..670fd23779 100644 --- a/src/applications/maniphest/query/ManiphestTaskQuery.php +++ b/src/applications/maniphest/query/ManiphestTaskQuery.php @@ -782,7 +782,7 @@ final class ManiphestTaskQuery private function loadCursorObject($id) { $results = id(new ManiphestTaskQuery()) - ->setViewer($this->getViewer()) + ->setViewer($this->getPagingViewer()) ->withIDs(array((int)$id)) ->execute(); return head($results); diff --git a/src/applications/repository/query/PhabricatorRepositoryQuery.php b/src/applications/repository/query/PhabricatorRepositoryQuery.php index 8662f682ff..5bfe60e2da 100644 --- a/src/applications/repository/query/PhabricatorRepositoryQuery.php +++ b/src/applications/repository/query/PhabricatorRepositoryQuery.php @@ -164,7 +164,7 @@ final class PhabricatorRepositoryQuery private function loadCursorObject($id) { $results = id(new PhabricatorRepositoryQuery()) - ->setViewer($this->getViewer()) + ->setViewer($this->getPagingViewer()) ->withIDs(array((int)$id)) ->execute(); return head($results); diff --git a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php index b05ea23d32..75d325d5fe 100644 --- a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php +++ b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php @@ -12,6 +12,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery private $afterID; private $beforeID; private $applicationSearchConstraints = array(); + private $internalPaging; protected function getPagingColumn() { return 'id'; @@ -26,6 +27,9 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery } protected function nextPage(array $page) { + // See getPagingViewer() for a description of this flag. + $this->internalPaging = true; + if ($this->beforeID) { $this->beforeID = $this->getPagingValue(last($page)); } else { @@ -51,6 +55,46 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery return $this->beforeID; } + + /** + * Get the viewer for making cursor paging queries. + * + * NOTE: You should ONLY use this viewer to load cursor objects while + * building paging queries. + * + * Cursor paging can happen in two ways. First, the user can request a page + * like `/stuff/?after=33`, which explicitly causes paging. Otherwise, we + * can fall back to implicit paging if we filter some results out of a + * result list because the user can't see them and need to go fetch some more + * results to generate a large enough result list. + * + * In the first case, want to use the viewer's policies to load the object. + * This prevents an attacker from figuring out information about an object + * they can't see by executing queries like `/stuff/?after=33&order=name`, + * which would otherwise give them a hint about the name of the object. + * Generally, if a user can't see an object, they can't use it to page. + * + * In the second case, we need to load the object whether the user can see + * it or not, because we need to examine new results. For example, if a user + * loads `/stuff/` and we run a query for the first 100 items that they can + * see, but the first 100 rows in the database aren't visible, we need to + * be able to issue a query for the next 100 results. If we can't load the + * cursor object, we'll fail or issue the same query over and over again. + * So, generally, internal paging must bypass policy controls. + * + * This method returns the appropriate viewer, based on the context in which + * the paging is occuring. + * + * @return PhabricatorUser Viewer for executing paging queries. + */ + final protected function getPagingViewer() { + if ($this->internalPaging) { + return PhabricatorUser::getOmnipotentUser(); + } else { + return $this->getViewer(); + } + } + final protected function buildLimitClause(AphrontDatabaseConnection $conn_r) { if ($this->getRawResultLimit()) { return qsprintf($conn_r, 'LIMIT %d', $this->getRawResultLimit());