From e1b2dd4720b88f987bed3a23a1b5ebb2ecc74407 Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Thu, 14 Jul 2016 14:55:22 -0400 Subject: [PATCH] API: Filter lists of IDs before sending them to the database People apparently have a tendency to typo the IDs somehow, and if you hand MySQL a stringified integer in a list that is out of range it decides it can't use sensible indexes. Bug: T140302 Change-Id: Ic1975220e55cb9daa16127ec0540e7ad16aad44e --- includes/api/ApiBase.php | 38 ++++++++++++++++++++++++++++ includes/api/ApiPageSet.php | 45 ++++++++++----------------------- includes/api/ApiQueryBase.php | 24 ++++++++++++++++++ includes/api/ApiQueryBlocks.php | 2 +- 4 files changed, 77 insertions(+), 32 deletions(-) diff --git a/includes/api/ApiBase.php b/includes/api/ApiBase.php index 32156d8b92..9ea8c6df43 100644 --- a/includes/api/ApiBase.php +++ b/includes/api/ApiBase.php @@ -268,6 +268,9 @@ abstract class ApiBase extends ContextSource { /** @var array Maps extension paths to info arrays */ private static $extensionInfo = null; + /** @var int[][][] Cache for self::filterIDs() */ + private static $filterIDsCache = []; + /** @var ApiMain */ private $mMainModule; /** @var string */ @@ -1832,6 +1835,41 @@ abstract class ApiBase extends ContextSource { } } + /** + * Filter out-of-range values from a list of positive integer IDs + * @since 1.33 + * @param array $fields Array of pairs of table and field to check + * @param (string|int)[] $ids IDs to filter. Strings in the array are + * expected to be stringified ints. + * @return (string|int)[] Filtered IDs. + */ + protected function filterIDs( $fields, array $ids ) { + $min = INF; + $max = 0; + foreach ( $fields as list( $table, $field ) ) { + if ( isset( self::$filterIDsCache[$table][$field] ) ) { + $row = self::$filterIDsCache[$table][$field]; + } else { + $row = $this->getDB()->selectRow( + $table, + [ + 'min_id' => "MIN($field)", + 'max_id' => "MAX($field)", + ], + null, + __METHOD__ + ); + self::$filterIDsCache[$table][$field] = $row; + } + $min = min( $min, $row->min_id ); + $max = max( $max, $row->max_id ); + } + return array_filter( $ids, function ( $id ) use ( $min, $max ) { + return ( is_int( $id ) && $id >= 0 || ctype_digit( $id ) ) + && $id >= $min && $id <= $max; + } ); + } + /**@}*/ /************************************************************************//** diff --git a/includes/api/ApiPageSet.php b/includes/api/ApiPageSet.php index 194a511061..913f45ba5f 100644 --- a/includes/api/ApiPageSet.php +++ b/includes/api/ApiPageSet.php @@ -819,8 +819,9 @@ class ApiPageSet extends ApiBase { /** * Does the same as initFromTitles(), but is based on page IDs instead * @param array $pageids Array of page IDs + * @param bool $filterIds Whether the IDs need filtering */ - private function initFromPageIds( $pageids ) { + private function initFromPageIds( $pageids, $filterIds = false ) { if ( !$pageids ) { return; } @@ -828,7 +829,9 @@ class ApiPageSet extends ApiBase { $pageids = array_map( 'intval', $pageids ); // paranoia $remaining = array_flip( $pageids ); - $pageids = self::getPositiveIntegers( $pageids ); + if ( $filterIds ) { + $pageids = $this->filterIDs( [ [ 'page', 'page_id' ] ], $pageids ); + } $res = null; if ( !empty( $pageids ) ) { @@ -939,9 +942,10 @@ class ApiPageSet extends ApiBase { $pageids = []; $remaining = array_flip( $revids ); - $revids = self::getPositiveIntegers( $revids ); + $revids = $this->filterIDs( [ [ 'revision', 'rev_id' ], [ 'archive', 'ar_rev_id' ] ], $revids ); + $goodRemaining = array_flip( $revids ); - if ( !empty( $revids ) ) { + if ( $revids ) { $tables = [ 'revision', 'page' ]; $fields = [ 'rev_id', 'rev_page' ]; $where = [ 'rev_id' => $revids, 'rev_page = page_id' ]; @@ -955,22 +959,20 @@ class ApiPageSet extends ApiBase { $this->mLiveRevIDs[$revid] = $pageid; $pageids[$pageid] = ''; unset( $remaining[$revid] ); + unset( $goodRemaining[$revid] ); } } - $this->mMissingRevIDs = array_keys( $remaining ); - // Populate all the page information - $this->initFromPageIds( array_keys( $pageids ) ); + $this->initFromPageIds( array_keys( $pageids ), false ); // If the user can see deleted revisions, pull out the corresponding // titles from the archive table and include them too. We ignore // ar_page_id because deleted revisions are tied by title, not page_id. - if ( !empty( $this->mMissingRevIDs ) && $this->getUser()->isAllowed( 'deletedhistory' ) ) { - $remaining = array_flip( $this->mMissingRevIDs ); + if ( $goodRemaining && $this->getUser()->isAllowed( 'deletedhistory' ) ) { $tables = [ 'archive' ]; $fields = [ 'ar_rev_id', 'ar_namespace', 'ar_title' ]; - $where = [ 'ar_rev_id' => $this->mMissingRevIDs ]; + $where = [ 'ar_rev_id' => array_keys( $goodRemaining ) ]; $res = $db->select( $tables, $fields, $where, __METHOD__ ); $titles = []; @@ -1002,9 +1004,9 @@ class ApiPageSet extends ApiBase { $remaining[$revid] = true; } } - - $this->mMissingRevIDs = array_keys( $remaining ); } + + $this->mMissingRevIDs = array_keys( $remaining ); } /** @@ -1416,25 +1418,6 @@ class ApiPageSet extends ApiBase { return $this->mDbSource->getDB(); } - /** - * Returns the input array of integers with all values < 0 removed - * - * @param array $array - * @return array - */ - private static function getPositiveIntegers( $array ) { - // T27734 API: possible issue with revids validation - // It seems with a load of revision rows, MySQL gets upset - // Remove any < 0 integers, as they can't be valid - foreach ( $array as $i => $int ) { - if ( $int < 0 ) { - unset( $array[$i] ); - } - } - - return $array; - } - public function getAllowedParams( $flags = 0 ) { $result = [ 'titles' => [ diff --git a/includes/api/ApiQueryBase.php b/includes/api/ApiQueryBase.php index fe01f03367..b243ceeb34 100644 --- a/includes/api/ApiQueryBase.php +++ b/includes/api/ApiQueryBase.php @@ -263,6 +263,30 @@ abstract class ApiQueryBase extends ApiBase { } } + /** + * Like addWhereFld for an integer list of IDs + * @since 1.33 + * @param string $table Table name + * @param string $field Field name + * @param int[] $ids IDs + * @return int Count of IDs actually included + */ + protected function addWhereIDsFld( $table, $field, $ids ) { + // Use count() to its full documented capabilities to simultaneously + // test for null, empty array or empty countable object + if ( count( $ids ) ) { + $ids = $this->filterIDs( [ [ $table, $field ] ], $ids ); + + if ( !count( $ids ) ) { + // Return nothing, no IDs are valid + $this->where[] = '0 = 1'; + } else { + $this->where[$field] = $ids; + } + } + return count( $ids ); + } + /** * Add a WHERE clause corresponding to a range, and an ORDER BY * clause to sort in the right direction diff --git a/includes/api/ApiQueryBlocks.php b/includes/api/ApiQueryBlocks.php index caaebc9194..95f8cda818 100644 --- a/includes/api/ApiQueryBlocks.php +++ b/includes/api/ApiQueryBlocks.php @@ -103,7 +103,7 @@ class ApiQueryBlocks extends ApiQueryBase { } if ( isset( $params['ids'] ) ) { - $this->addWhereFld( 'ipb_id', $params['ids'] ); + $this->addWhereIDsFld( 'ipblocks', 'ipb_id', $params['ids'] ); } if ( isset( $params['users'] ) ) { $usernames = []; -- 2.20.1