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 a bug where policy queries with cursor-based pagers and non-ID orders can go into infinite loops

Summary:
Ref T603. See inlines for an explanation. The case where I hit this was loading the "Pending Differential Revisions" panel in Diffusion when logged out, after making a repository public.

What happens is that we load 10 revisions (say, D1 .. D10) but the user can't see any of them. We then try to load the next 10, but since the pagination is ordered by date modified, we need to base the next query on the modified date of the last thing we loaded (D10). However, since we use the viewer's policies to load that cursor object, it fails to load, and then we just issue the same query over and over again, loading D1 .. D10 until we run out of execution time.

Test Plan: Interface now loads correctly.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

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

epriestley e7a7e431 63818818

+47 -3
+1 -1
src/applications/differential/query/DifferentialRevisionQuery.php
··· 730 730 731 731 private function loadCursorObject($id) { 732 732 $results = id(new DifferentialRevisionQuery()) 733 - ->setViewer($this->getViewer()) 733 + ->setViewer($this->getPagingViewer()) 734 734 ->withIDs(array((int)$id)) 735 735 ->execute(); 736 736 return head($results);
+1 -1
src/applications/maniphest/query/ManiphestTaskQuery.php
··· 782 782 783 783 private function loadCursorObject($id) { 784 784 $results = id(new ManiphestTaskQuery()) 785 - ->setViewer($this->getViewer()) 785 + ->setViewer($this->getPagingViewer()) 786 786 ->withIDs(array((int)$id)) 787 787 ->execute(); 788 788 return head($results);
+1 -1
src/applications/repository/query/PhabricatorRepositoryQuery.php
··· 164 164 165 165 private function loadCursorObject($id) { 166 166 $results = id(new PhabricatorRepositoryQuery()) 167 - ->setViewer($this->getViewer()) 167 + ->setViewer($this->getPagingViewer()) 168 168 ->withIDs(array((int)$id)) 169 169 ->execute(); 170 170 return head($results);
+44
src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php
··· 12 12 private $afterID; 13 13 private $beforeID; 14 14 private $applicationSearchConstraints = array(); 15 + private $internalPaging; 15 16 16 17 protected function getPagingColumn() { 17 18 return 'id'; ··· 26 27 } 27 28 28 29 protected function nextPage(array $page) { 30 + // See getPagingViewer() for a description of this flag. 31 + $this->internalPaging = true; 32 + 29 33 if ($this->beforeID) { 30 34 $this->beforeID = $this->getPagingValue(last($page)); 31 35 } else { ··· 49 53 50 54 final protected function getBeforeID() { 51 55 return $this->beforeID; 56 + } 57 + 58 + 59 + /** 60 + * Get the viewer for making cursor paging queries. 61 + * 62 + * NOTE: You should ONLY use this viewer to load cursor objects while 63 + * building paging queries. 64 + * 65 + * Cursor paging can happen in two ways. First, the user can request a page 66 + * like `/stuff/?after=33`, which explicitly causes paging. Otherwise, we 67 + * can fall back to implicit paging if we filter some results out of a 68 + * result list because the user can't see them and need to go fetch some more 69 + * results to generate a large enough result list. 70 + * 71 + * In the first case, want to use the viewer's policies to load the object. 72 + * This prevents an attacker from figuring out information about an object 73 + * they can't see by executing queries like `/stuff/?after=33&order=name`, 74 + * which would otherwise give them a hint about the name of the object. 75 + * Generally, if a user can't see an object, they can't use it to page. 76 + * 77 + * In the second case, we need to load the object whether the user can see 78 + * it or not, because we need to examine new results. For example, if a user 79 + * loads `/stuff/` and we run a query for the first 100 items that they can 80 + * see, but the first 100 rows in the database aren't visible, we need to 81 + * be able to issue a query for the next 100 results. If we can't load the 82 + * cursor object, we'll fail or issue the same query over and over again. 83 + * So, generally, internal paging must bypass policy controls. 84 + * 85 + * This method returns the appropriate viewer, based on the context in which 86 + * the paging is occuring. 87 + * 88 + * @return PhabricatorUser Viewer for executing paging queries. 89 + */ 90 + final protected function getPagingViewer() { 91 + if ($this->internalPaging) { 92 + return PhabricatorUser::getOmnipotentUser(); 93 + } else { 94 + return $this->getViewer(); 95 + } 52 96 } 53 97 54 98 final protected function buildLimitClause(AphrontDatabaseConnection $conn_r) {