From 0b41dfb849f604da3431ef353382be8cd13e29ae Mon Sep 17 00:00:00 2001 From: JJ Fullmer <7743340+darksidemilk@users.noreply.github.com> Date: Wed, 19 Aug 2026 19:24:22 -0500 Subject: [PATCH] Push the API object boundary into the query, keeping the id list as fallback The site boundary reached the API in #1229 by asking the plugin which object ids the acting user may see, then narrowing with that list. It is correct and it does not scale: answering "which hosts may this user see" means reading every one of them into PHP on every request, and then either splicing thousands of ids into an IN list or comparing each row against them. A new event, API_SCOPE_WHERE, asks for the same boundary as a SQL fragment instead. It is tried FIRST and API_SCOPE_IDS is the fallback, so a third-party plugin that knows only the old event keeps bounding reads exactly as it did -- both handlers stay registered in the site plugin and only one is ever applied to a request. Two tri-states, and they are not the same one. The id list uses null for "no boundary" and an empty array for "you may see nothing". The fragment has no empty state at all: '' is read as silence and falls through, because an empty fragment is indistinguishable from no answer and would otherwise compile to `WHERE ()`. Deny-all is therefore said in SQL, as '1=0'. Where each applies: names(), ids() Route builds this SQL itself, so the fragment is ANDed onto whatever _buildWhere() produced. listem(), These go through the manager, which had no seam. find() search() and search() take a $scopeWhere argument now; it is ANDed on LAST with the caller's own terms parenthesised, because $whereOperator is a parameter and 'OR' is a value it takes. An OR that can reach outside the boundary is not a boundary. dispatch gate _requireObjectScope() asks whether one row satisfies the same expression -- bounded, and it cannot disagree with what the list showed. Also fixed, found while proving parity and in the same call path: hostIDsForSites() went through SiteHostAssociation's manager, which walks class relationships to build joins and so pulled in Host's MACAddressAssociation relationship. That carries array('primary' => 1), which buildQuery() emits as `hostMAC`.`hmPrimary` = '1' in the WHERE -- turning a LEFT OUTER JOIN into an inner one and silently DROPPING every host with no primary MAC. 95 of 1000 in the lab. It under-returned, so nobody saw anything they should not, but a site-restricted user could not see hosts in their own site, and it made the two boundaries disagree about which hosts a site holds. A membership lookup has no business joining Host; it reads the association table directly now. This also affects the management pages, which share the function: restricted users will now see hosts that were wrongly hidden from them. _boundedSiteIDs() is memoized per user per request. Without it an UNRESTRICTED user pays for the restriction lookup twice on every read -- once for the fragment that declines, once for the id list that declines. Measured 2 -> 3 statements for an administrator before the memo, 2 after. Deliberately not memoized on the public userIsRestricted()/userSiteIDs(), which the management pages call on requests that have just written the rows they read. Verified against the 1.5 lab database, 2079 hosts, a site holding 1000, with the real site plugin: id list id list fragment as merged + join fix (this) names host 95 1000 1000 ids host 95 1000 1000 list host 95 95 95 search host 53 53 53 names group 2 2 2 restricted, no site 0 0 0 logged out (daemon) 2079 2079 2079 administrator 126/2079 126/2079 126/2079 queries, restricted 4 4 3 The fixed id list and the fragment agree on every line, which is the parity that was asked for; the unfixed column is the bug above. Characterization tests were written FIRST, against the id-list implementation, and pass unedited after the port -- 65 checks over three states (no boundary, scoped, deny-all) times four read routes times two classes, plus the dispatch gate out of process because a denial ends the response by exiting. They assert which objects come back, never the SQL that produced them, so they hold across either implementation. The database arm skips rather than degrading into a string match where no 1.5 schema is reachable. Every gate is mutation-verified. Two bugs were caught that way and would not have been caught otherwise: the gate read PDODB's `false` for "no rows" through count((array)$rows), which is 1, so it allowed every object it was built to refuse; and the fragment was dropped whenever a caller had supplied a filter of their own, because only the empty arm of the append helper was being driven. Co-Authored-By: Claude --- .../lib/fog/fogmanagercontroller.class.php | 126 +++-- .../web/lib/plugins/site/class/site.class.php | 176 +++++- .../plugins/site/hooks/addsiteapi.hook.php | 53 ++ packages/web/lib/router/route.class.php | 236 +++++++- tests/lib/scope-harness.php | 343 ++++++++++++ tests/lib/scopeprobe.php | 75 +++ tests/site-api-scope-behaviour.test.php | 518 ++++++++++++++++++ tests/site-api-scope.test.php | 213 ++++++- 8 files changed, 1677 insertions(+), 63 deletions(-) create mode 100644 tests/lib/scope-harness.php create mode 100644 tests/lib/scopeprobe.php create mode 100644 tests/site-api-scope-behaviour.test.php diff --git a/packages/web/lib/fog/fogmanagercontroller.class.php b/packages/web/lib/fog/fogmanagercontroller.class.php index 5b252b0dc2..cb7d4275c4 100644 --- a/packages/web/lib/fog/fogmanagercontroller.class.php +++ b/packages/web/lib/fog/fogmanagercontroller.class.php @@ -151,6 +151,7 @@ public function __construct() * @param mixed $idField what fields to get * @param bool $onecompare second where uses AND * @param string $filter array function for filter + * @param string $scopeWhere an object-boundary SQL fragment to AND on * * @return array */ @@ -164,7 +165,8 @@ public function find( $not = false, $idField = false, $onecompare = true, - $filter = 'array_unique' + $filter = 'array_unique', + $scopeWhere = '' ) { // Fail safe defaults if (empty($findWhere)) { @@ -349,6 +351,64 @@ public function find( $idFields = array_filter($idFields); $idField = $idFields; unset($idFields); + $whereClause = ( + count($whereArray) > 0 ? + sprintf( + ' WHERE %s%s', + implode(" $whereOperator ", (array) $whereArray), + ( + $isEnabled ? + sprintf(' AND %s', $isEnabled) : + '' + ) + ) : + ( + $isEnabled ? + sprintf(' WHERE %s', $isEnabled) : + '' + ) + ); + $andClause = ( + count($whereArrayAnd) > 0 ? + ( + count($whereArray) > 0 ? + sprintf( + 'AND %s', + implode(" $whereOperator ", (array) $whereArrayAnd) + ) : + sprintf( + ' WHERE %s', + implode(" $whereOperator ", (array) $whereArrayAnd) + ) + ) : + '' + ); + // The object boundary, when the caller was given one to apply. + // + // Two properties this has to hold that the obvious splice does not. + // It is ANDed on LAST, after everything the caller asked for, with + // the caller's own terms parenthesised: $whereOperator is a parameter + // and 'OR' is a value it takes, so a term merged in beside the + // caller's could be satisfied INSTEAD of the boundary rather than as + // well as it. And it is joined with a literal ' AND ', never through + // $whereOperator, for the same reason. An OR that can reach outside + // the boundary is not a boundary. + // + // Empty means no boundary, which is every caller that does not pass + // this argument. A caller that means "you may see nothing" passes a + // fragment saying so, such as '1=0' -- NOT an empty string, which + // reads here as unrestricted and would hand back the whole table. + $scopeWhere = trim((string)$scopeWhere); + if ('' !== $scopeWhere) { + $inner = trim($whereClause . ' ' . $andClause); + $inner = preg_replace('#^WHERE\s+#i', '', $inner); + $whereClause = ( + '' === $inner ? + sprintf(' WHERE %s', $scopeWhere) : + sprintf(' WHERE (%s) AND (%s)', $inner, $scopeWhere) + ); + $andClause = ''; + } $query = sprintf( $this->loadQueryTemplate, ( @@ -358,38 +418,8 @@ public function find( ), $this->databaseTable, $join, - ( - count($whereArray) > 0 ? - sprintf( - ' WHERE %s%s', - implode(" $whereOperator ", (array) $whereArray), - ( - $isEnabled ? - sprintf(' AND %s', $isEnabled) : - '' - ) - ) : - ( - $isEnabled ? - sprintf(' WHERE %s', $isEnabled) : - '' - ) - ), - ( - count($whereArrayAnd) > 0 ? - ( - count($whereArray) > 0 ? - sprintf( - 'AND %s', - implode(" $whereOperator ", (array) $whereArrayAnd) - ) : - sprintf( - ' WHERE %s', - implode(" $whereOperator ", (array) $whereArrayAnd) - ) - ) : - '' - ), + $whereClause, + $andClause, $groupBy, $orderBy ); @@ -1002,7 +1032,7 @@ public function exists( * * @return mixe */ - public function search($keyword = '', $returnObjects = false) + public function search($keyword = '', $returnObjects = false, $scopeWhere = '') { $keyword = trim($keyword); if (!$keyword) { @@ -1030,7 +1060,19 @@ public function search($keyword = '', $returnObjects = false) ) ); if (empty($keyword) || $keyword === '%') { - return $this->find(); + return $this->find( + array(), + 'AND', + 'name', + 'ASC', + '=', + false, + false, + false, + true, + 'array_unique', + $scopeWhere + ); } $keyword = preg_replace( '#[%\+\s\+]#', @@ -1330,7 +1372,19 @@ public function search($keyword = '', $returnObjects = false) array('id' => $itemIDs) ); if ($returnObjects) { - return $this->find(array('id' => $itemIDs)); + return $this->find( + array('id' => $itemIDs), + 'AND', + 'name', + 'ASC', + '=', + false, + false, + false, + true, + 'array_unique', + $scopeWhere + ); } return $itemIDs; diff --git a/packages/web/lib/plugins/site/class/site.class.php b/packages/web/lib/plugins/site/class/site.class.php index 02e562eb5b..c56cbaa99a 100755 --- a/packages/web/lib/plugins/site/class/site.class.php +++ b/packages/web/lib/plugins/site/class/site.class.php @@ -367,10 +367,46 @@ public static function userSiteIDs($userID) */ public static function hostIDsForSites($siteIDs) { - return (array)self::getSubObjectIDs( - 'SiteHostAssociation', - array('siteID' => (array)$siteIDs), - 'hostID' + $siteIDs = array_filter( + array_map('intval', array_values((array)$siteIDs)) + ); + if (count($siteIDs) < 1) { + return array(); + } + // Read directly rather than through getSubObjectIDs(). + // + // SiteHostAssociation declares a class relationship to Host, and + // find() walks those relationships to build the joins -- including + // Host's own MACAddressAssociation relationship, which carries the + // filter array('primary' => 1). buildQuery() puts that in the WHERE + // as `hostMAC`.`hmPrimary` = '1', which turns a LEFT OUTER JOIN into + // an inner one and DROPS every host with no primary MAC row. + // + // The effect was silent and it under-returned: a site-restricted user + // did not see hosts in their own site that had no primary MAC. In the + // lab, 95 of 1000. It is not a disclosure -- nobody saw anything they + // should not -- but it is wrong, and it made the SQL boundary in + // scopedObjectWhere() disagree with this one, which is worse: which + // hosts you could see depended on which of the two answered. + // + // A plain membership lookup has no business joining Host at all. The + // table and column names are the same ones scopedObjectWhere() writes + // and are justified there. + $rows = self::$DB + ->query( + sprintf( + 'SELECT DISTINCT `siteHostAssoc`.`shaHostID`' + . ' FROM `siteHostAssoc`' + . ' WHERE `siteHostAssoc`.`shaSiteID` IN (%s)', + implode(',', $siteIDs) + ) + ) + ->fetch(\PDO::FETCH_ASSOC, 'fetch_all') + ->get('shaHostID'); + return array_values( + array_unique( + array_map('intval', (array)$rows) + ) ); } /** @@ -416,25 +452,145 @@ public static function groupIDsForSites($siteIDs) * * @return array|null */ - public static function scopedObjectIDs($classname, $userID) + /** + * Per-request memo of _boundedSiteIDs(), keyed by user id. + * + * Core consults the SQL fragment first and falls back to the id list, so + * on a server where only one of the two is answered -- which is every + * server, since the fragment always wins here -- an UNRESTRICTED user + * pays for the restriction lookup TWICE per read: once for the fragment + * that declines, once for the id list that declines. Measured at 2 -> 3 + * statements for an administrator on `names(host)` before this existed, + * which is a cost the boundary imposes on exactly the people it does not + * apply to. + * + * Deliberately NOT a memo on userIsRestricted() or userSiteIDs(). Those + * are public and the management pages call them directly, including on + * requests that have just written the rows they read; a memo there would + * serve a stale answer to the page that changed it. Scoped to this + * private ladder, the only callers are the two read-side entry points, + * neither of which writes. + * + * @var array + */ + private static $_boundedSites = array(); + /** + * The same boundary as scopedObjectIDs(), expressed as SQL. + * + * THE RETURN IS A TRI-STATE, and it is not the id list's: + * + * null no boundary applies -- the caller must fall back + * '' narrow with this expression + * '1=0' a real answer meaning "nothing" + * + * There is no empty-string state, because the caller reads '' as "no + * listener answered" -- see Route::_scopeWhere(). Deny-all therefore has + * to be said in SQL, and '1=0' is how it is said here. + * + * Why a fragment at all: scopedObjectIDs() answers by reading every + * object the user may see into PHP, on every request, and the caller then + * either splices thousands of ids into an IN list or compares each row + * against them. This costs one expression whatever the fleet size. The + * membership rule is still stated once -- the two functions share the + * ladder below and differ only in what they return -- so the API and the + * management pages cannot drift into two different answers. + * + * Nothing here interpolates user input. $idExpr is built by the caller + * from the model's own $databaseFields, and $userID is cast to int; there + * is no path from a request parameter into this string. A future edit + * that wants to inline anything else needs a parameter, not a cast. + * + * @param string $classname The class being listed or fetched. + * @param string $idExpr The object-id column, quoted and qualified. + * @param int $userID The acting user. + * + * @return string|null + */ + public static function scopedObjectWhere($classname, $idExpr, $userID) + { + $siteIDs = self::_boundedSiteIDs($classname, $userID); + if (null === $siteIDs) { + return null; + } + if (count($siteIDs) < 1) { + return '1=0'; + } + $sites = implode( + ',', + array_map('intval', array_values($siteIDs)) + ); + $hostsInSites = sprintf( + 'SELECT `siteHostAssoc`.`shaHostID` FROM `siteHostAssoc`' + . ' WHERE `siteHostAssoc`.`shaSiteID` IN (%s)', + $sites + ); + if ('group' === strtolower((string)$classname)) { + // A group is in scope when it holds at least one host that is. + // Same rule as groupIDsForSites(), which is the point. + return sprintf( + 'EXISTS (SELECT 1 FROM `groupMembers`' + . ' WHERE `groupMembers`.`gmGroupID` = %s' + . ' AND `groupMembers`.`gmHostID` IN (%s))', + $idExpr, + $hostsInSites + ); + } + return sprintf( + 'EXISTS (SELECT 1 FROM `siteHostAssoc`' + . ' WHERE `siteHostAssoc`.`shaHostID` = %s' + . ' AND `siteHostAssoc`.`shaSiteID` IN (%s))', + $idExpr, + $sites + ); + } + /** + * The sites bounding this user for this class, or null for no boundary. + * + * The shared front half of scopedObjectIDs() and scopedObjectWhere(): + * everything up to the membership lookup itself. Two copies of this + * ladder would be two chances to answer "is this user bounded?" + * differently, in the one place where the two answers must agree. + * + * Returns an EMPTY ARRAY for a restricted user belonging to no site -- + * a real answer meaning "nothing" -- and null when no boundary applies. + * + * @param string $classname The class being listed or fetched. + * @param int $userID The acting user. + * + * @return array|null + */ + private static function _boundedSiteIDs($classname, $userID) { $classname = strtolower((string)$classname); // Only what the plugin actually associates. Everything else -- // images, snapins, storage nodes, the association tables -- has no - // site boundary to apply, and returning an id list for one would - // narrow lookups the plugin knows nothing about. + // site boundary to apply, and narrowing one the plugin knows nothing + // about would break lookups rather than protect anything. if (!in_array($classname, array('host', 'group'), true)) { return null; } $userID = (int)$userID; - if (!self::userIsRestricted($userID)) { + if (array_key_exists($userID, self::$_boundedSites)) { + return self::$_boundedSites[$userID]; + } + self::$_boundedSites[$userID] = self::userIsRestricted($userID) + ? self::userSiteIDs($userID) + : null; + return self::$_boundedSites[$userID]; + } + public static function scopedObjectIDs($classname, $userID) + { + // Same ladder as scopedObjectWhere(), deliberately: these two answer + // the same question in two shapes, and a server where they disagree + // has a boundary that depends on which route you came in through. + $siteIDs = self::_boundedSiteIDs($classname, $userID); + if (null === $siteIDs) { return null; } - $siteIDs = self::userSiteIDs($userID); if (count($siteIDs) < 1) { return array(); } - return 'group' === $classname + return 'group' === strtolower((string)$classname) ? self::groupIDsForSites($siteIDs) : self::hostIDsForSites($siteIDs); } diff --git a/packages/web/lib/plugins/site/hooks/addsiteapi.hook.php b/packages/web/lib/plugins/site/hooks/addsiteapi.hook.php index f10e367d0f..d563fa1558 100644 --- a/packages/web/lib/plugins/site/hooks/addsiteapi.hook.php +++ b/packages/web/lib/plugins/site/hooks/addsiteapi.hook.php @@ -88,6 +88,13 @@ public function __construct() $this, 'scopeIDs' ) + ) + ->register( + 'API_SCOPE_WHERE', + array( + $this, + 'scopeWhere' + ) ); } /** @@ -243,6 +250,52 @@ public function scopeIDs($arguments) ) ); } + /** + * The same boundary as scopeIDs(), as a SQL fragment. + * + * Answered in preference to the id list, and the reason is cost: scopeIDs() + * reads every host the user may see into PHP on every request, which on a + * server with thousands of them is the whole expense of the feature. A + * fragment is one expression whatever the fleet size. + * + * Both handlers stay registered. Core tries this one and falls back to the + * id list when nothing answers, so a third-party plugin that knows only + * API_SCOPE_IDS keeps bounding reads exactly as it did. + * + * Sets $arguments['where'] only when a boundary applies. Left alone it + * stays null, which is the caller's "nobody answered" value. Note this + * tri-state is NOT the id list's: an empty string is read as silence, so + * "you may see nothing" is the literal fragment '1=0'. See + * Site::scopedObjectWhere(). + * + * @param mixed $arguments The arguments to modify. + * + * @return void + */ + public function scopeWhere($arguments) + { + if (!in_array($this->node, (array)self::$pluginsinstalled)) { + return; + } + // No acting user means no boundary to apply -- the service daemons + // and the status endpoints reach Route::ids()/names() with nobody + // logged in, and narrowing those to a site would break imaging + // rather than protect anything. Same guard as scopeIDs(), and it has + // to be here too: this handler is consulted FIRST, so a boundary it + // emitted would apply before the id list was ever asked. + if (!self::$FOGUser || !self::$FOGUser->isValid()) { + return; + } + $where = Site::scopedObjectWhere( + $arguments['classname'], + $arguments['idExpr'], + self::$FOGUser->get('id') + ); + if (null === $where) { + return; + } + $arguments['where'] = $where; + } /** * This function changes the getter to enact on this particular item. * diff --git a/packages/web/lib/router/route.class.php b/packages/web/lib/router/route.class.php index 4dd30b04a6..cbc4fd42de 100644 --- a/packages/web/lib/router/route.class.php +++ b/packages/web/lib/router/route.class.php @@ -559,6 +559,115 @@ private static function _scopeIDs($classname) ); return is_array($ids) ? array_values($ids) : null; } + /** + * The object boundary as a SQL fragment, or null when none applies. + * + * Tried BEFORE _scopeIDs(). A boundary expressed as SQL costs one + * expression whatever the fleet size, where an id list costs a lookup of + * every object the user may see, materialised into PHP, on every request + * -- and then has to be spliced into a query or compared against every + * row. On a server with thousands of hosts that is the whole cost of the + * feature. + * + * THE RETURN IS A TRI-STATE, and it is NOT the same tri-state as + * _scopeIDs(): + * + * null no answer -- fall through to the id list + * '' narrow with this expression + * '1=0' (or similar) a real answer meaning "nothing" + * + * There is deliberately no empty-string state. An empty fragment is + * indistinguishable from silence, so it is read as silence: a listener + * that means "you may see nothing" must say so in SQL, and '1=0' is how. + * If '' were treated as deny-all, a listener that returned '' by accident + * would deny; if it were treated as a boundary, it would ALSO produce + * `WHERE ()`, which is a syntax error. Reading it as no-answer is the + * only option that fails towards the existing id-list path rather than + * towards either a broken query or a silent policy change. + * + * $idExpr is the caller's own id column, already quoted and qualified, so + * a listener can write `EXISTS (... WHERE assoc.hostID = )` and + * not have to know the table name or guess at ambiguity in a joined + * query. + * + * Inert in core: nothing here knows what a site is. + * + * @param string $classname The class being read. + * @param string $idExpr The object-id column, quoted and qualified. + * + * @return string|null + */ + private static function _scopeWhere($classname, $idExpr) + { + $where = null; + self::$HookManager + ->processEvent( + 'API_SCOPE_WHERE', + array( + 'classname' => &$classname, + 'idExpr' => &$idExpr, + 'where' => &$where + ) + ); + if (!is_string($where)) { + return null; + } + $where = trim($where); + return '' === $where ? null : $where; + } + /** + * The boundary fragment for a class, with the id expression worked out. + * + * Every caller needs the same `\`table\`.\`idcol\`` expression, and + * every caller getting it right separately is the sort of duplication + * that stays correct until one of them is edited. + * + * @param string $classname The class being read. + * + * @return string|null + */ + private static function _scopeWhereFor($classname) + { + $classVars = self::getClass($classname, '', true); + if (!isset($classVars['databaseTable'], $classVars['databaseFields']['id'])) { + return null; + } + return self::_scopeWhere( + $classname, + sprintf( + '`%s`.`%s`', + $classVars['databaseTable'], + $classVars['databaseFields']['id'] + ) + ); + } + /** + * ANDs a boundary fragment onto a WHERE clause _buildWhere() produced. + * + * _buildWhere() returns either '' or a complete ' WHERE ...' whose own + * terms are joined with AND, so appending is safe without parentheses + * around what is already there -- but they are added anyway, because + * "the other function only ever emits AND" is a property a later edit + * can remove without anything here noticing. + * + * @param string $where The clause so far, '' or ' WHERE ...'. + * @param string $frag The boundary fragment. + * + * @return string + */ + private static function _andScopeWhere($where, $frag) + { + if (null === $frag || '' === (string)$frag) { + return $where; + } + $where = (string)$where; + if ('' === trim($where)) { + return ' WHERE (' . $frag . ')'; + } + return ' WHERE (' + . preg_replace('#^\s*WHERE\s+#i', '', trim($where)) + . ') AND (' . $frag . ')'; + } /** * Narrows a filter set to the ids the acting user may see. * @@ -613,7 +722,24 @@ private static function _requireObjectScope($class, $id) if ($id < 1) { return; } - $scope = self::_scopeIDs(strtolower((string)$class)); + $classname = strtolower((string)$class); + // The fragment answers this as a bounded existence check -- one row + // at most, and the id list is never materialised. It is also the SAME + // expression the list routes narrow with, which is the property that + // matters: a 403 that disagreed with what the list showed would be + // two statements of who may see what, and the second one to be edited + // would make the boundary decorative. + $scopeWhere = self::_scopeWhereFor($classname); + if (null !== $scopeWhere) { + if (self::_objectInScopeWhere($classname, $id, $scopeWhere)) { + return; + } + self::sendResponse( + HTTPResponseCodes::HTTP_FORBIDDEN + ); + return; + } + $scope = self::_scopeIDs($classname); if (null === $scope || in_array($id, $scope, true)) { return; } @@ -621,6 +747,52 @@ private static function _requireObjectScope($class, $id) HTTPResponseCodes::HTTP_FORBIDDEN ); } + /** + * Does this one object satisfy the boundary fragment? + * + * Deliberately not `SELECT ... LIMIT 1` over the scoped set followed by a + * comparison -- the id is bound as a parameter and the database answers + * yes or no, so the cost does not move with how many objects the user can + * see. + * + * A query that cannot run answers NO. That is the safe direction here: + * this decides whether to serve a single object, and refusing one the + * user was entitled to is a visible, reportable failure, where serving + * one they were not is silent. + * + * @param string $classname The class the route is acting on. + * @param int $id The target object id. + * @param string $frag The boundary fragment. + * + * @return bool + */ + private static function _objectInScopeWhere($classname, $id, $frag) + { + $classVars = self::getClass($classname, '', true); + if (!isset($classVars['databaseTable'], $classVars['databaseFields']['id'])) { + return false; + } + $sql = sprintf( + 'SELECT `%s`.`%s` FROM `%s` WHERE `%s`.`%s` = :scope_id' + . ' AND (%s) LIMIT 1', + $classVars['databaseTable'], + $classVars['databaseFields']['id'], + $classVars['databaseTable'], + $classVars['databaseTable'], + $classVars['databaseFields']['id'], + $frag + ); + $rows = self::$DB + ->query($sql, array(), array('scope_id' => (int)$id)) + ->fetch() + ->get(); + // is_array(), not count((array)$rows). PDODB::get() answers a query + // that matched nothing with `false`, and `count((array)false)` is 1 -- + // so the obvious test reads "no such row" as "in scope" and the gate + // allows everything it was built to refuse. It reported allowed for + // an object outside the boundary until a behavioural test drove it. + return is_array($rows) && count($rows) > 0; + } /** * Test token information. * @@ -747,10 +919,18 @@ public static function listem( $find, self::getsearchbody($classname) ); - // Object boundary. Applied to the rows rather than the query - // because this route has no LIMIT -- every match is built and - // returned -- so filtering here is exact and keeps 'count' honest. - $scope = self::_scopeIDs($classname); + // Object boundary, preferring SQL. + // + // A fragment is pushed into the query, so the rows the boundary + // excludes are never built at all. Nothing answering the fragment + // event falls through to the id list, which is applied to the rows + // instead -- unchanged, because a third-party plugin that only knows + // API_SCOPE_IDS has to keep working exactly as it did. + // + // Either way 'count' stays honest: it counts what is emitted, and + // this route has no LIMIT, so every match is built and returned. + $scopeWhere = self::_scopeWhereFor($classname); + $scope = null === $scopeWhere ? self::_scopeIDs($classname) : null; switch ($classname) { case 'plugin': self::$data['count_active'] = 0; @@ -776,7 +956,20 @@ public static function listem( } break; default: - foreach ((array)$classman->find($find, 'AND', $sortby) as &$class) { + $found = $classman->find( + $find, + 'AND', + $sortby, + 'ASC', + '=', + false, + false, + false, + true, + 'array_unique', + (string)$scopeWhere + ); + foreach ((array)$found as &$class) { $test = stripos( $class->get('name'), '_api_' @@ -844,8 +1037,11 @@ public static function search($class, $item) self::$data = array(); self::$data['count'] = 0; self::$data[$classname.'s'] = array(); - $scope = self::_scopeIDs($classname); - foreach ($classman->search($item, true) as &$class) { + // Same two-path boundary as listem(): SQL when a listener supplies + // it, the id list when none does. + $scopeWhere = self::_scopeWhereFor($classname); + $scope = null === $scopeWhere ? self::_scopeIDs($classname) : null; + foreach ($classman->search($item, true, (string)$scopeWhere) as &$class) { if (false != stripos($class->get('name'), '_api_')) { continue; } @@ -2225,7 +2421,13 @@ public static function names($class, $whereItems = []) ); $whereItems = self::handleWhereItems($whereItems, $class); - $whereItems = self::_scopeWhereItems($classname, $whereItems); + // Object boundary. The fragment is preferred and the id list is the + // fallback; only one of the two is ever applied, so a boundary is + // never counted twice and never half-applied. + $scopeWhere = self::_scopeWhereFor($classname); + if (null === $scopeWhere) { + $whereItems = self::_scopeWhereItems($classname, $whereItems); + } $sql = 'SELECT `' . $classVars['databaseFields']['id'] @@ -2235,7 +2437,10 @@ public static function names($class, $whereItems = []) . $classVars['databaseTable'] . '`'; - $sql .= self::_buildWhere($classVars, $whereItems, $params); + $sql .= self::_andScopeWhere( + self::_buildWhere($classVars, $whereItems, $params), + $scopeWhere + ); $sql .= ' ORDER BY `' . ( $classVars['databaseFields']['name'] ?: @@ -2314,7 +2519,11 @@ public static function ids($class, $whereItems = [], $getField = 'id') } } - $whereItems = self::_scopeWhereItems($classname, $whereItems); + // Object boundary; see names() for why only one of the two applies. + $scopeWhere = self::_scopeWhereFor($classname); + if (null === $scopeWhere) { + $whereItems = self::_scopeWhereItems($classname, $whereItems); + } $sql = 'SELECT `' . $classVars['databaseFields'][$getField] @@ -2322,7 +2531,10 @@ public static function ids($class, $whereItems = [], $getField = 'id') . $classVars['databaseTable'] . '`'; - $sql .= self::_buildWhere($classVars, $whereItems, $params); + $sql .= self::_andScopeWhere( + self::_buildWhere($classVars, $whereItems, $params), + $scopeWhere + ); $sql .= ' ORDER BY `' . ( (isset($classVars['databaseFields']['name']) && $classVars['databaseFields']['name']) ? diff --git a/tests/lib/scope-harness.php b/tests/lib/scope-harness.php new file mode 100644 index 0000000000..1b5d142d86 --- /dev/null +++ b/tests/lib/scope-harness.php @@ -0,0 +1,343 @@ +log[] = $sql; + foreach (array('hosts', 'groups') as $t) { + if (false === strpos($sql, '`' . $t . '`')) { + continue; + } + $vars = Route::getClass(rtrim($t, 's'), '', true); + $this->_r = array(); + foreach ($this->rowIds as $i) { + $row = array(); + // Every declared column, so loading an object does not warn + // its way through a row that is missing most of itself. + foreach ($vars['databaseFields'] as $col) { + $row[$col] = ''; + } + $row[$vars['databaseFields']['id']] = $i; + $row[$vars['databaseFields']['name']] = 'h' . $i; + $this->_r[] = $row; + } + return $this; + } + $this->_r = array(); + return $this; + } + + public function fetch($m = null, $t = '', $p = array()) + { + return $this; + } + + public function get($f = '') + { + return $this->_r; + } + + // save() on an unknown hook event reaches these; answering keeps the + // fixture from dying on the first event name this test introduces. + public function insertId() + { + return 1; + } + + public function sqlerror() + { + return ''; + } + + public function affectedRows() + { + return 1; + } +} + +/** + * Boots FOG far enough to drive Route, with the probe registered. + * + * @param string $web packages/web + * @param string $state 'none', 'scoped' or 'deny' -- only used by the + * subprocess arm, which cannot be handed an array + * + * @return void + */ +function scopeHarnessBoot($web, $state = 'none') +{ + static $booted = false; + if ($booted) { + return; + } + $booted = true; + + $tmp = sys_get_temp_dir() . '/fog-scope-behaviour-' . getmypid(); + @mkdir($tmp . '/cache', 0700, true); + @mkdir($tmp . '/log', 0700, true); + @mkdir($tmp . '/plugins', 0700, true); + register_shutdown_function( + function () use ($tmp) { + if (!is_dir($tmp)) { + return; + } + $it = new \RecursiveIteratorIterator( + new \RecursiveDirectoryIterator( + $tmp, + \FilesystemIterator::SKIP_DOTS + ), + \RecursiveIteratorIterator::CHILD_FIRST + ); + foreach ($it as $f) { + $f->isDir() ? @rmdir($f->getPathname()) : @unlink($f->getPathname()); + } + @rmdir($tmp); + } + ); + if (!defined('FOG_CACHE_DIR')) { + define('FOG_CACHE_DIR', $tmp . '/cache'); + } + if (!defined('FOG_LOG_DIR')) { + define('FOG_LOG_DIR', $tmp . '/log'); + } + if (!defined('FOG_PLUGIN_DIR')) { + define('FOG_PLUGIN_DIR', $tmp . '/plugins'); + } + if (!defined('FOG_SCHEMA')) { + define('FOG_SCHEMA', 0); + } + // The row-shaped fixtures below are deliberately partial, so the noise a + // partial row makes is not the output of this test. + error_reporting(E_ALL & ~E_WARNING & ~E_NOTICE & ~E_DEPRECATED); + + require_once $web . '/commons/init.php'; + new Initiator(); + + $set = function ($class, $prop, $value) { + $p = new \ReflectionProperty($class, $prop); + $p->setAccessible(true); + $p->setValue(null, $value); + }; + // LoadGlobals is what normally builds these and it wants a database. + $set('FOGBase', 'HookManager', new HookManager()); + $set('FOGBase', 'EventManager', new EventManager()); + // AFTER the managers: constructing one re-runs FOGBase's own wiring and + // leaves $DB null behind it. + $set('FOGBase', 'DB', new ScopeFakeDB()); + // MACAddress logs a parse failure through FOGCore, and a Host builds one + // on load. Without this the host cases die on a null. + $set( + 'FOGBase', + 'FOGCore', + new class { + public function debug($m = '') + { + } + public function error($m = '') + { + } + } + ); + + // processEvent() writes an unseen event name to the hookevent table before + // dispatching it. Seeded so the fixture is not answering INSERTs. + $known = new \ReflectionProperty('HookManager', 'knownEvents'); + $known->setAccessible(true); + $known->setValue( + null, + array_flip( + array( + 'API_SCOPE_IDS', + 'API_SCOPE_WHERE', + 'API_GETTER', + 'API_MASSDATA_MAPPING', + 'API_INDIVDATA_MAPPING', + 'API_VALID_CLASSES', + 'API_SENSITIVE_FIELDS', + ) + ) + ); + + // After Initiator: ScopeProbe extends Hook, which does not exist until + // the autoloader does. + require_once __DIR__ . '/scopeprobe.php'; + $probe = (new \ReflectionClass('ScopeProbe'))->newInstanceWithoutConstructor(); + $hm = new \ReflectionProperty('FOGBase', 'HookManager'); + $hm->setAccessible(true); + $hm->getValue()->register('API_SCOPE_IDS', array($probe, 'scope')); + $hm->getValue()->register('API_SCOPE_WHERE', array($probe, 'scopeWhere')); + + switch ($state) { + case 'scoped': + ScopeProbe::$answer = array(2, 3); + break; + case 'deny': + ScopeProbe::$answer = array(); + break; + default: + ScopeProbe::$answer = 'none'; + } +} + +/** + * Why the database arm cannot run, or null when it can. + * + * Also defines the DATABASE_* constants when it returns null, reading them + * out of a live install's generated config so no credential is written down + * in the repository. + * + * @return string|null + */ +function scopeHarnessDbReason() +{ + foreach (glob('/var/www/html/fog-1.5*/lib/fog/config.class.php') ?: array() as $cfg) { + $src = @file_get_contents($cfg); + if (false === $src) { + continue; + } + preg_match_all( + "/define\('(DATABASE_[A-Z]+)', *'([^']*)'/", + $src, + $m, + PREG_SET_ORDER + ); + $vals = array(); + foreach ($m as $d) { + $vals[$d[1]] = $d[2]; + } + if (count($vals) < 5) { + continue; + } + foreach ($vals as $k => $v) { + if (!defined($k)) { + define($k, $v); + } + } + return null; + } + return 'no 1.5 install found under /var/www/html to read a database' + . ' configuration from'; +} + +/** + * Two hosts with NO MAC, in a site of their own, and a teardown for both. + * + * Exists because of a bug the group fixture cannot see. SiteHostAssociation + * declares a class relationship to Host, find() walks those to build joins, + * and Host's MACAddressAssociation relationship carries array('primary' => 1) + * -- which buildQuery() emits as `hostMAC`.`hmPrimary` = '1' in the WHERE. + * That turns the LEFT OUTER JOIN into an inner one, so a plain membership + * lookup silently dropped every host with no primary MAC: 95 of 1000 in the + * lab. Hosts with no MAC row at all are the sharpest case, so that is what + * this makes. + * + * @param object $db the real PDODB + * + * @return array array(siteID, array(hostID, hostID)) + */ +function scopeHarnessMaclessFixture($db) +{ + $mark = 'zzmac' . getmypid(); + register_shutdown_function( + function () use ($db, $mark) { + // Associations first: they are what points at the hosts, and a + // row left behind here would be a dangling membership. + $db->query( + "DELETE `siteHostAssoc` FROM `siteHostAssoc`" + . " JOIN `site` ON `site`.`sID` = `siteHostAssoc`.`shaSiteID`" + . " WHERE `site`.`sName` = '" . $mark . "'" + ); + $db->query("DELETE FROM `site` WHERE `sName` = '" . $mark . "'"); + $db->query( + "DELETE FROM `hosts` WHERE `hostName` LIKE '" . $mark . "%'" + ); + } + ); + $db->query( + "INSERT INTO `site` (`sName`,`sDesc`) VALUES ('" . $mark . "','fixture')" + ); + $siteID = (int)$db->insertId(); + $hostIDs = array(); + foreach (array('a', 'b') as $n) { + $db->query( + "INSERT INTO `hosts` (`hostName`,`hostIP`,`hostUseAD`)" + . " VALUES ('" . $mark . $n . "','','0')" + ); + $hostIDs[] = (int)$db->insertId(); + } + foreach ($hostIDs as $hid) { + $db->query( + "INSERT INTO `siteHostAssoc` (`shaName`,`shaSiteID`,`shaHostID`)" + . " VALUES ('', " . $siteID . ", " . $hid . ")" + ); + } + return array($siteID, $hostIDs); +} + +/** + * Swaps the database FOGBase reads through. + * + * @param object $db the connection to install + * + * @return void + */ +function scopeHarnessSetDb($db) +{ + $p = new \ReflectionProperty('FOGBase', 'DB'); + $p->setAccessible(true); + $p->setValue(null, $db); +} + +/** + * Three groups this process owns, and a teardown that removes exactly those. + * + * Marked with the pid so a concurrent run, or an abandoned one, cannot be + * mistaken for this run's rows -- and so the DELETE can name what it removes + * rather than clearing a table this server may be using for something. + * + * @param object $db the real PDODB + * + * @return array the three ids + */ +function scopeHarnessFixture($db) +{ + $mark = 'zz-scopetest-' . getmypid() . '-'; + register_shutdown_function( + function () use ($db, $mark) { + $db->query( + "DELETE FROM `groups` WHERE `groupName` LIKE '" . $mark . "%'" + ); + } + ); + $ids = array(); + foreach (array(1, 2, 3) as $n) { + $db->query( + "INSERT INTO `groups` (`groupName`,`groupDesc`)" + . " VALUES ('" . $mark . $n . "','api scope characterization')" + ); + $ids[] = (int)$db->insertId(); + } + return $ids; +} diff --git a/tests/lib/scopeprobe.php b/tests/lib/scopeprobe.php new file mode 100644 index 0000000000..0ea359aa2d --- /dev/null +++ b/tests/lib/scopeprobe.php @@ -0,0 +1,75 @@ + no boundary, read unrestricted. + * Not an edge case: the service daemons and the status endpoints reach + * Route::ids()/names() logged out, through getIds()/getNames(). A + * boundary that always applies scopes those too, and the symptom is + * imaging breaking, not a 403. + * + * 2. A scoped user with objects in scope -> exactly those objects. + * + * 3. A scoped user with NOTHING in scope -> NOTHING, not everything. + * This is the one that matters. `null` means "no boundary" and `array()` + * means "you may see nothing"; both are falsy, so any `if (!$ids)` test + * collapses deny-all into full disclosure -- for precisely the users the + * boundary exists to restrict, with no error and no log line. + * + * TWO ARMS. + * + * The row-filtered routes -- listem() and search() -- are assertable with no + * database at all, because the boundary is applied to objects the manager + * already built: a fake PDO hands back three rows and the assertion is which + * of them survive. That arm always runs. + * + * names() and ids() are not. They push the boundary into the WHERE clause, so + * "which rows come back" is a question only a real database can answer, and + * asserting on the generated SQL instead would be asserting the mechanism -- + * the thing this file exists not to do. That arm therefore runs against a real + * 1.5 schema when one is reachable and SKIPS when it is not, rather than + * quietly degrading into a string match. + * + * The database arm creates its own rows, marked with this process' pid, and + * asserts only about those. It never asserts a table is empty: "empty of my + * rows" is not "empty", and a server with real data would fail an assertion + * that confused the two. The single exception is deny-all, where a zero TOTAL + * is the assertion. + * + * Usage: php tests/site-api-scope-behaviour.test.php + * Exit status 0 = pass (or skip), 1 = fail. + */ + +$web = dirname(__DIR__) . '/packages/web'; + +// --------------------------------------------------------------- subprocess +// +// _requireObjectScope() answers a denial with sendResponse(), which ends in +// exit -- there is no result to inspect and no exception to catch, so the +// only way to observe it is from outside the process. Re-exec of this same +// file with an argument, and the marker below is what "it did not deny" +// looks like. +if (isset($argv[1]) && '--object-scope' === $argv[1]) { + require __DIR__ . '/lib/scope-harness.php'; + scopeHarnessBoot($web, $argv[2]); + // The fragment states need a real database: the gate answers a fragment + // by asking whether one row satisfies it, which is a question only a + // database has. The rows are the PARENT's fixture -- they exist for as + // long as the parent does -- and their ids arrive on the command line so + // the child does not create a second, unrelated set. + if (0 === strpos($argv[2], 'frag')) { + scopeHarnessDbReason(); + scopeHarnessSetDb(new PDODB()); + $inScope = array_map('intval', array_slice($argv, 4)); + ScopeProbe::$whereAnswer = 'frag-deny' === $argv[2] + ? '1=0' + : function ($idExpr) use ($inScope) { + return $idExpr . ' IN (' . implode(',', $inScope) . ')'; + }; + } + $m = new \ReflectionMethod('Route', '_requireObjectScope'); + $m->setAccessible(true); + $m->invoke(null, 'group', (int)$argv[3]); + echo "ALLOWED\n"; + exit(0); +} + +require __DIR__ . '/lib/scope-harness.php'; + +$failures = []; +$checks = 0; + +function check($label, $cond, array &$failures, &$checks) +{ + $checks++; + if (!$cond) { + $failures[] = $label; + } +} + +// ------------------------------------------------------- arm 1: no database +// +// Three rows in, and the question is which come out. `group` rather than +// `host` for the second class because a Host builds a MACAddress on load, +// which reaches for globals a fake database has no way to supply; both are +// scoped classes and the boundary code does not branch on which. +scopeHarnessBoot($web, 'none'); + +$states = [ + ['none', 'unbounded', [1, 2, 3]], + [[2, 3], 'scoped', [2, 3]], + [[], 'deny-all', []], +]; + +foreach (['host', 'group'] as $class) { + foreach ($states as [$answer, $label, $expect]) { + ScopeProbe::$answer = $answer; + foreach (['listem', 'search'] as $route) { + Route::$data = null; + if ('listem' === $route) { + Route::listem($class); + } else { + Route::search($class, 'h'); + } + $got = array_map( + 'intval', + array_column((array)(Route::$data[$class . 's'] ?? []), 'id') + ); + sort($got); + check( + "$route($class) $label returns [" . implode(',', $expect) . ']', + $got === $expect, + $failures, + $checks + ); + // The count is part of the answer, not decoration: a caller that + // pages on it is told how many objects exist, so a count computed + // over the unscoped set discloses the size of what it hid. + check( + "$route($class) $label reports count " . count($expect), + (int)(Route::$data['count'] ?? -1) === count($expect), + $failures, + $checks + ); + } + } +} + +// ---------------------------------------- arm 2: per-object route, out of process +$objectScope = function ($state, $id, array $extra = []) { + $out = []; + exec( + sprintf( + '%s %s --object-scope %s %d %s 2>&1', + escapeshellarg(PHP_BINARY), + escapeshellarg(__FILE__), + escapeshellarg($state), + $id, + implode(' ', array_map('intval', $extra)) + ), + $out + ); + return in_array('ALLOWED', array_map('trim', $out), true); +}; +foreach ([['none', 1, true], ['scoped', 1, false], ['scoped', 3, true], ['deny', 1, false]] as [$state, $id, $allow]) { + check( + "_requireObjectScope(group, $id) under '$state' " . ($allow ? 'allows' : 'denies'), + $objectScope($state, $id) === $allow, + $failures, + $checks + ); +} + +// ------------------------------------------------------- arm 3: real database +// +// Skipped rather than faked when there is no 1.5 schema to talk to. The +// credentials are read from a live install's generated config at run time and +// never written down here. +$dbSkip = scopeHarnessDbReason(); +if (null !== $dbSkip) { + echo "SKIP (database arm): $dbSkip\n"; +} else { + // Arms 1 and 2 ran against the fake; from here the boundary has to be + // answered by a real database, because "which rows come back" is the + // whole assertion and only a database can answer it. + $real = new PDODB(); + scopeHarnessSetDb($real); + $ids = scopeHarnessFixture($real); + $mine = function ($values) use ($ids) { + $out = array_values( + array_intersect(array_map('intval', (array)$values), $ids) + ); + sort($out); + return $out; + }; + $cases = [ + ['none', 'unbounded', $ids], + [[$ids[1], $ids[2]], 'scoped', [$ids[1], $ids[2]]], + [[], 'deny-all', []], + ]; + foreach ($cases as [$answer, $label, $expect]) { + ScopeProbe::$answer = $answer; + + Route::$data = null; + Route::names('group', []); + $names = (array)Route::$data; + check( + "names(group) $label returns my " . count($expect) . ' group(s)', + $mine(array_column($names, 'id')) === $expect, + $failures, + $checks + ); + + Route::$data = null; + Route::ids('group', [], 'id'); + $idlist = (array)Route::$data; + check( + "ids(group) $label returns my " . count($expect) . ' group(s)', + $mine($idlist) === $expect, + $failures, + $checks + ); + + // Deny-all is the only state where a TOTAL is assertable, and it is + // the assertion the whole tri-state exists for: not "my rows are + // gone" but "no rows at all", which is what separates a boundary + // that compiled to nothing from one that compiled to everything. + if ('deny-all' === $label) { + check( + 'names(group) deny-all returns NO rows at all', + 0 === count($names), + $failures, + $checks + ); + check( + 'ids(group) deny-all returns NO rows at all', + 0 === count($idlist), + $failures, + $checks + ); + } + } + + /* + * The SQL-fragment path, which is what a site-scoped server actually + * runs -- the id list is now the fallback for plugins that predate the + * fragment event. + * + * Only reachable with a real database, and not because of a harness + * limitation: the whole point of a fragment is that the DATABASE applies + * the boundary, so there is nothing to observe without one. The answers + * below are built from the caller's own $idExpr rather than a hardcoded + * column, so what is under test is the seam and not a string. + */ + ScopeProbe::$answer = 'none'; + $fragCases = array( + array('none', 'unbounded', $ids), + array( + function ($idExpr) use ($ids) { + return $idExpr . ' IN (' . $ids[1] . ',' . $ids[2] . ')'; + }, + 'scoped', + array($ids[1], $ids[2]) + ), + array('1=0', 'deny-all', array()), + ); + foreach ($fragCases as [$answer, $label, $expect]) { + ScopeProbe::$whereAnswer = $answer; + + Route::$data = null; + Route::names('group', []); + $names = (array)Route::$data; + check( + "fragment: names(group) $label returns my " . count($expect), + $mine(array_column($names, 'id')) === $expect, + $failures, + $checks + ); + + Route::$data = null; + Route::ids('group', [], 'id'); + $idlist = (array)Route::$data; + check( + "fragment: ids(group) $label returns my " . count($expect), + $mine($idlist) === $expect, + $failures, + $checks + ); + + Route::$data = null; + Route::listem('group'); + check( + "fragment: listem(group) $label returns my " . count($expect), + $mine(array_column((array)(Route::$data['groups'] ?? []), 'id')) === $expect, + $failures, + $checks + ); + + Route::$data = null; + Route::search('group', 'zz-scopetest-'); + check( + "fragment: search(group) $label returns my " . count($expect), + $mine(array_column((array)(Route::$data['groups'] ?? []), 'id')) === $expect, + $failures, + $checks + ); + + if ('deny-all' === $label) { + check( + 'fragment: names(group) deny-all returns NO rows at all', + 0 === count($names), + $failures, + $checks + ); + check( + 'fragment: listem(group) deny-all returns NO rows at all', + 0 === (int)(Route::$data['count'] ?? -1) + || 0 === count((array)(Route::$data['groups'] ?? [])), + $failures, + $checks + ); + } + } + + /* + * The fragment survives a caller's own filter. + * + * Every case above hands names()/ids() an empty filter, so the boundary + * is the only term in the WHERE and a fragment that got dropped whenever + * a clause already existed would pass all of them. That is not a + * hypothetical: the append helper has two arms, and only the empty one + * was being driven. A caller asking for all three of my groups, narrowed + * by a fragment naming two, has to come back with two. + */ + ScopeProbe::$answer = 'none'; + ScopeProbe::$whereAnswer = function ($idExpr) use ($ids) { + return $idExpr . ' IN (' . $ids[1] . ',' . $ids[2] . ')'; + }; + Route::$data = null; + Route::names('group', ['id' => $ids]); + check( + 'fragment: names(group) narrows a filter the caller supplied', + $mine(array_column((array)Route::$data, 'id')) === [$ids[1], $ids[2]], + $failures, + $checks + ); + Route::$data = null; + Route::ids('group', ['id' => $ids], 'id'); + check( + 'fragment: ids(group) narrows a filter the caller supplied', + $mine((array)Route::$data) === [$ids[1], $ids[2]], + $failures, + $checks + ); + ScopeProbe::$whereAnswer = '1=0'; + Route::$data = null; + Route::names('group', ['id' => $ids]); + check( + 'fragment: deny-all beats a filter the caller supplied', + 0 === count((array)Route::$data), + $failures, + $checks + ); + // And the manager path too: listem() passes the caller's own $find + // alongside the fragment, which is the second arm of the same join. + ScopeProbe::$whereAnswer = function ($idExpr) use ($ids) { + return $idExpr . ' IN (' . $ids[1] . ',' . $ids[2] . ')'; + }; + Route::$data = null; + Route::listem('group', 'name', false, ['id' => $ids]); + check( + 'fragment: listem(group) narrows a filter the caller supplied', + $mine(array_column((array)(Route::$data['groups'] ?? []), 'id')) + === [$ids[1], $ids[2]], + $failures, + $checks + ); + + /* + * Exactly ONE boundary is applied, never both. + * + * Two narrowings ANDed together is not a safer boundary, it is a + * different one nobody stated -- and the failure is invisible, because + * over-restriction looks like the feature working. The fragment says + * "these two"; the id list says "that other one"; if both applied the + * answer would be nothing at all. + */ + ScopeProbe::$whereAnswer = function ($idExpr) use ($ids) { + return $idExpr . ' IN (' . $ids[1] . ',' . $ids[2] . ')'; + }; + ScopeProbe::$answer = array($ids[0]); + foreach (array('names', 'ids', 'listem', 'search') as $route) { + Route::$data = null; + switch ($route) { + case 'names': + Route::names('group', []); + $got = $mine(array_column((array)Route::$data, 'id')); + break; + case 'ids': + Route::ids('group', [], 'id'); + $got = $mine((array)Route::$data); + break; + case 'listem': + Route::listem('group'); + $got = $mine(array_column((array)(Route::$data['groups'] ?? []), 'id')); + break; + default: + Route::search('group', 'zz-scopetest-'); + $got = $mine(array_column((array)(Route::$data['groups'] ?? []), 'id')); + } + check( + "$route(group) applies the fragment ALONE when both events answer", + $got === array($ids[1], $ids[2]), + $failures, + $checks + ); + } + + /* + * And the id list is still reachable. Without this the fall-through could + * be broken in the other direction -- fragment-only -- and every check + * above would still pass while every plugin written before the fragment + * event silently stopped bounding anything. + */ + ScopeProbe::$whereAnswer = 'none'; + ScopeProbe::$answer = array($ids[0]); + Route::$data = null; + Route::names('group', []); + check( + 'the id list still bounds a read when no fragment answers', + $mine(array_column((array)Route::$data, 'id')) === array($ids[0]), + $failures, + $checks + ); + // An empty fragment is silence, not deny-all: it must fall through to + // the id list rather than either denying or reading unbounded. + ScopeProbe::$whereAnswer = ' '; + Route::$data = null; + Route::names('group', []); + check( + 'an empty fragment falls through to the id list', + $mine(array_column((array)Route::$data, 'id')) === array($ids[0]), + $failures, + $checks + ); + ScopeProbe::$whereAnswer = 'none'; + ScopeProbe::$answer = 'none'; + + /* + * The two boundaries must agree about WHICH hosts are in a site. + * + * They did not. A membership lookup through SiteHostAssociation's manager + * drags in Host's joins, including the primary-MAC filter, so it dropped + * every host with no primary MAC -- 95 of 1000 in the lab -- while the + * SQL fragment, which joins nothing, returned all of them. Not a + * disclosure: it under-returned, so a site-restricted user could not see + * hosts in their own site. But it meant which hosts you could see + * depended on which of the two answered, which is the one thing a + * boundary must not do. + * + * Asserted against the database rather than against the other function, + * so it cannot pass by both being wrong in the same way. + */ + [$maclessSite, $maclessHosts] = scopeHarnessMaclessFixture($real); + $got = Site::hostIDsForSites([$maclessSite]); + sort($got); + check( + 'hostIDsForSites() returns hosts that have no primary MAC', + array_map('intval', (array)$got) === $maclessHosts, + $failures, + $checks + ); + // And the fragment agrees, which is the property that actually matters. + $frag = Site::scopedObjectWhere( + 'host', + '`hosts`.`hostID`', + 0 + ); + check( + 'scopedObjectWhere() declines for a user with no restriction row', + null === $frag, + $failures, + $checks + ); + + // The per-object gate under a fragment. Same expression the lists narrow + // with, so a 403 and an absent row are the same decision. + check( + 'fragment: the gate allows an object inside the boundary', + true === $objectScope('frag-scoped', $ids[1], [$ids[1], $ids[2]]), + $failures, + $checks + ); + check( + 'fragment: the gate denies an object outside the boundary', + false === $objectScope('frag-scoped', $ids[0], [$ids[1], $ids[2]]), + $failures, + $checks + ); + check( + 'fragment: the gate denies everything under a deny-all fragment', + false === $objectScope('frag-deny', $ids[1], [$ids[1], $ids[2]]), + $failures, + $checks + ); +} + +if (count($failures)) { + fwrite(STDERR, 'FAIL (' . count($failures) . " of $checks):\n"); + foreach ($failures as $f) { + fwrite(STDERR, " - $f\n"); + } + exit(1); +} +echo "ok $checks checks passed\n"; diff --git a/tests/site-api-scope.test.php b/tests/site-api-scope.test.php index 8ac162571c..9c5572ffbb 100644 --- a/tests/site-api-scope.test.php +++ b/tests/site-api-scope.test.php @@ -152,7 +152,9 @@ function methodBody($src, $name) */ $looseScope = []; if (preg_match_all( - '/(?:if\s*\(\s*!\s*\$scope\b|empty\s*\(\s*\$scope\s*\)|if\s*\(\s*\$scope\s*\))/', + '/(?:if\s*\(\s*!\s*\$scope(?:Where)?\b' + . '|empty\s*\(\s*\$scope(?:Where)?\s*\)' + . '|if\s*\(\s*\$scope(?:Where)?\s*\))/', $route, $m )) { @@ -168,15 +170,108 @@ function methodBody($src, $name) // Counted, not merely present. The regex above catches `if (!$scope)` // but not `if ($scope && ...)` spanning two lines, and the whole failure // mode here is a comparison quietly losing its `null` half. +// Word-bounded, because $scopeWhere is a DIFFERENT tri-state with a +// different vocabulary and an unbounded /\$scope/ counts both as one. check( 'the row filters compare against null exactly twice (listem, search)', - 2 === preg_match_all('/null !== \$scope/', $route, $mn), + 2 === preg_match_all('/null !== \$scope\b/', $route, $mn), $failures, $checks ); check( 'the WHERE narrowing and the dispatch gate each short-circuit on null', - 2 === preg_match_all('/null === \$scope/', $route, $mn2), + 2 === preg_match_all('/null === \$scope\b/', $route, $mn2), + $failures, + $checks +); + +/* + * The SQL-fragment boundary, which is tried before the id list. + * + * Its tri-state is not the id list's and must not be written as though it + * were. There is no empty-string state: '' is read as "no listener answered" + * and falls through, because an empty fragment is indistinguishable from + * silence and would otherwise compile to `WHERE ()`. What has to hold is + * that the two are never BOTH applied -- a request narrowed twice is a + * request whose boundary nobody can reason about -- and that the id list is + * still reachable, or every third-party plugin answering only the old event + * silently stops bounding anything. + */ +check( + '_scopeWhere() answers null for anything that is not a string', + false !== strpos(methodBody($route, '_scopeWhere'), 'is_string($where)'), + $failures, + $checks +); +check( + '_scopeWhere() reads an empty fragment as no answer, not as deny-all', + (bool)preg_match( + "/'' === \\\$where \\? null :/", + methodBody($route, '_scopeWhere') + ), + $failures, + $checks +); +foreach (['listem', 'search'] as $fn) { + $body = methodBody($route, $fn); + check( + "$fn() falls through to the id list only when no fragment answered", + (bool)preg_match( + '/\$scope = null === \$scopeWhere \? self::_scopeIDs\(/', + $body + ), + $failures, + $checks + ); + check( + "$fn() hands the fragment to the manager rather than filtering rows", + false !== strpos($body, '(string)$scopeWhere'), + $failures, + $checks + ); +} +foreach (['names', 'ids'] as $fn) { + $body = methodBody($route, $fn); + check( + "$fn() applies the id list only when no fragment answered", + (bool)preg_match( + '/if \(null === \$scopeWhere\) \{\s*\$whereItems = self::_scopeWhereItems\(/', + $body + ), + $failures, + $checks + ); + check( + "$fn() ANDs the fragment onto the clause it built", + false !== strpos($body, '_andScopeWhere('), + $failures, + $checks + ); +} +check( + 'the dispatch gate answers a fragment with a bounded existence check', + false !== strpos( + methodBody($route, '_requireObjectScope'), + '_objectInScopeWhere(' + ), + $failures, + $checks +); +check( + 'a boundary query that cannot run denies rather than allows', + (bool)preg_match( + '/databaseFields..\[.id.\]\)\) \{\s*return false;/', + methodBody($route, '_objectInScopeWhere') + ), + $failures, + $checks +); +check( + 'the object id is bound as a parameter, never concatenated', + false !== strpos( + methodBody($route, '_objectInScopeWhere'), + "array('scope_id' => (int)\$id)" + ), $failures, $checks ); @@ -195,9 +290,117 @@ function methodBody($src, $name) $failures, $checks ); +// The two short circuits moved into _boundedSiteIDs(), which both the id +// list and the SQL fragment now go through -- that sharing is the property +// worth pinning, not which function the `return null` lines sit in. Three +// now: the two in the shared ladder, and the one pass-through in each of the +// public entry points. +// Stated once, in the shared ladder -- asserted by where the two deciding +// terms LIVE rather than by counting `return null`, which the memo turned +// into a ternary and which a later refactor will move again. What must not +// happen is either entry point growing its own copy: two answers to "is this +// user bounded?" is how the API and the pages come to disagree. +$ladder = methodBody($site, '_boundedSiteIDs'); check( - 'scopedObjectIDs() returns null only for an unbounded class or user', - 2 === preg_match_all('/\n\s*return null;/', $site, $m2), + 'the shared ladder holds the scoped-class list', + false !== strpos($ladder, "array('host', 'group')"), + $failures, + $checks +); +check( + 'the shared ladder holds the restriction test', + false !== strpos($ladder, 'self::userIsRestricted('), + $failures, + $checks +); +foreach (['scopedObjectIDs', 'scopedObjectWhere'] as $fn) { + $body = methodBody($site, $fn); + check( + "$fn() does not restate the class list", + false === strpos($body, "array('host', 'group')"), + $failures, + $checks + ); + check( + "$fn() does not restate the restriction test", + false === strpos($body, 'self::userIsRestricted('), + $failures, + $checks + ); +} +// The memo is on the private ladder only. userIsRestricted() and +// userSiteIDs() are public and the management pages call them on requests +// that have just written the rows they read, so a memo there would serve a +// stale answer to the page that changed it. +foreach (['userIsRestricted', 'userSiteIDs'] as $fn) { + check( + "$fn() is not memoized", + false === strpos(methodBody($site, $fn), '_boundedSites'), + $failures, + $checks + ); +} +// A plain membership lookup must not join Host: find() would walk Host's +// MACAddressAssociation relationship, whose array('primary' => 1) filter +// becomes `hmPrimary` = '1' in the WHERE and drops every host without a +// primary MAC. It under-returned for years and made the two boundaries +// disagree about which hosts were in a site. +check( + 'hostIDsForSites() reads the association table without joining Host', + // 'self::' matters: the comment above the code names the function it + // no longer calls, and a bare grep for the name reads its own prose. + false === strpos(methodBody($site, 'hostIDsForSites'), 'self::getSubObjectIDs(') + && false !== strpos(methodBody($site, 'hostIDsForSites'), '`siteHostAssoc`'), + $failures, + $checks +); +check( + 'hostIDsForSites() interpolates only intval-mapped site ids', + (bool)preg_match( + "/array_map\('intval', array_values\(\(array\)\\\$siteIDs\)\)/", + methodBody($site, 'hostIDsForSites') + ), + $failures, + $checks +); +foreach (['scopedObjectIDs', 'scopedObjectWhere'] as $fn) { + check( + "$fn() gets its boundary from the shared ladder", + false !== strpos(methodBody($site, $fn), 'self::_boundedSiteIDs('), + $failures, + $checks + ); + check( + "$fn() passes an unbounded answer straight through as null", + (bool)preg_match( + '/if \(null === \$siteIDs\) \{\s*return null;/', + methodBody($site, $fn) + ), + $failures, + $checks + ); +} +// Deny-all in the SQL shape is a fragment that says so, never an empty +// string -- Route::_scopeWhere() reads '' as "nobody answered" and would +// fall through to an unbounded read for exactly the user entitled to none. +check( + 'scopedObjectWhere() says deny-all in SQL rather than returning empty', + (bool)preg_match( + "/count\\(\\\$siteIDs\\) < 1\\) \\{\\s*return '1=0';/", + methodBody($site, 'scopedObjectWhere') + ), + $failures, + $checks +); +// The fragment is inlined into a statement, so anything interpolated into it +// has to be an int by construction. There is no request path to $userID or +// the site ids, and this is what keeps it that way. +check( + 'scopedObjectWhere() interpolates only intval-mapped site ids', + false !== strpos( + methodBody($site, 'scopedObjectWhere'), + "array_map('intval', array_values(\$siteIDs))" + ), $failures, $checks );