From 855e65aa0bab346a4d95d9ad0ee45225250c3afd Mon Sep 17 00:00:00 2001 From: Steve Boyd Date: Tue, 6 Dec 2022 16:41:57 +1300 Subject: [PATCH] API Create orderBy() method to handle raw SQL --- src/ORM/DataList.php | 151 ++++++++++++++------- src/ORM/DataObject.php | 20 ++- src/ORM/DataQuery.php | 5 + src/ORM/Search/SearchContext.php | 6 +- src/Security/InheritedPermissions.php | 2 +- tests/php/ORM/DataListTest.php | 155 +++++++++++++++++++++- tests/php/ORM/DataObjectTest.php | 10 +- tests/php/ORM/ManyManyThroughListTest.php | 2 +- 8 files changed, 286 insertions(+), 65 deletions(-) diff --git a/src/ORM/DataList.php b/src/ORM/DataList.php index e9f65b04dc0..7eb7edf72a0 100644 --- a/src/ORM/DataList.php +++ b/src/ORM/DataList.php @@ -203,10 +203,14 @@ public function sql(&$parameters = []) /** * Return a new DataList instance with a WHERE clause added to this list's query. * + * This method accepts raw SQL so could be vulnerable to SQL injection attacks if used incorrectly, + * it's preferable to use filter() instead which does not allow raw SQL. + * * Supports parameterised queries. * See SQLSelect::addWhere() for syntax examples, although DataList * won't expand multiple method arguments as SQLSelect does. * + * * @param string|array|SQLConditionGroup $filter Predicate(s) to set, as escaped SQL statements or * paramaterised queries * @return static @@ -222,6 +226,9 @@ public function where($filter) * Return a new DataList instance with a WHERE clause added to this list's query. * All conditions provided in the filter will be joined with an OR * + * This method accepts raw SQL so could be vulnerable to SQL injection attacks if used incorrectly, + * it's preferable to use filterAny() instead which does not allow raw SQL + * * Supports parameterised queries. * See SQLSelect::addWhere() for syntax examples, although DataList * won't expand multiple method arguments as SQLSelect does. @@ -237,8 +244,6 @@ public function whereAny($filter) }); } - - /** * Returns true if this DataList can be sorted by the given field. * @@ -308,72 +313,118 @@ public function distinct($value) } /** - * Return a new DataList instance as a copy of this data list with the sort - * order set. + * Return a new DataList instance as a copy of this data list with the sort order set + * + * Raw SQL is not accepted, only actual field names can be passed * - * @see SS_List::sort() - * @see SQLSelect::orderby + * @param string|array $args * @example $list = $list->sort('Name'); // default ASC sorting - * @example $list = $list->sort('Name DESC'); // DESC sorting + * @example $list = $list->sort('Name ASC, Age DESC'); * @example $list = $list->sort('Name', 'ASC'); - * @example $list = $list->sort(array('Name'=>'ASC', 'Age'=>'DESC')); - * - * @param string|array Escaped SQL statement. If passed as array, all keys and values are assumed to be escaped. - * @return static + * @example $list = $list->sort(['Name' => 'ASC', 'Age' => 'DESC']); + * @example $list = $list->sort('MyRelation.MyColumn ASC') + * @example $list->sort(null); // wipe any existing sort */ - public function sort() + public function sort(...$args): static { - $count = func_num_args(); - + $count = count($args); if ($count == 0) { return $this; } - if ($count > 2) { throw new InvalidArgumentException('This method takes zero, one or two arguments'); } - if ($count == 2) { - $col = null; - $dir = null; - list($col, $dir) = func_get_args(); - - // Validate direction - if (!in_array(strtolower($dir ?? ''), ['desc', 'asc'])) { - user_error('Second argument to sort must be either ASC or DESC'); + list($column, $direction) = $args; + $sort = [$column => $direction]; + } else { + $sort = $args[0]; + if (!is_string($sort) && !is_array($sort) && !is_null($sort)) { + throw new InvalidArgumentException('sort() arguments must either be a string, an array, or null'); + } + if (is_null($sort)) { + // Set an an empty array here to cause any existing sort on the DataLists to be wiped + // later on in this method + $sort = []; + } elseif (empty($sort)) { + throw new InvalidArgumentException('Invalid sort paramater'); + } + // If $sort is string then convert string to array to allow for validation + if (is_string($sort)) { + $newSort = []; + // Making the assumption here there are no commas in column names + // Other parts of silverstripe will break if there are commas in column names + foreach (explode(',', $sort) as $colDir) { + // Using regex instead of explode(' ') in case column name includes spaces + if (preg_match('/^(.+) ([^"]+)$/i', trim($colDir), $matches)) { + list($column, $direction) = [$matches[1], $matches[2]]; + } else { + list($column, $direction) = [$colDir, 'ASC']; + } + $newSort[$column] = $direction; + } + $sort = $newSort; } + } + foreach ($sort as $column => $direction) { + $this->validateSortColumn($column); + $this->validateSortDirection($direction); + } + return $this->alterDataQuery(function (DataQuery $query, DataList $list) use ($sort) { + // Wipe the sort + $query->sort(null, null); + foreach ($sort as $column => $direction) { + $list->applyRelation($column, $relationColumn, true); + $query->sort($relationColumn, $direction, false); + } + }); + } - $sort = [$col => $dir]; - } else { - $sort = func_get_arg(0); + private function validateSortColumn(string $column): void + { + $col = trim($column); + // Strip slashes from single field names e.g. '"Title"' + if (preg_match('#^"[^"]+"$#', $col)) { + $col = str_replace('"', '', $col); } + // $columnName is a param that is passed by reference so is essentially as a return type + // it will be returned in quoted SQL "TableName"."ColumnName" notation + // if it's equal to $col however it means that it WAS orginally raw sql, which is disallowed for sort() + // + // applyRelation() will also throw an InvalidArgumentException if $column is not raw sql but + // the Relation.FieldName is not a valid model relationship + $this->applyRelation($col, $columnName, true); + if ($col === $columnName) { + throw new InvalidArgumentException("Invalid sort column $column"); + } + } - return $this->alterDataQuery(function (DataQuery $query, DataList $list) use ($sort) { + private function validateSortDirection(string $direction): void + { + $dir = strtolower($direction); + if ($dir !== 'asc' && $dir !== 'desc') { + throw new InvalidArgumentException("Invalid sort direction $direction"); + } + } - if (is_string($sort) && $sort) { - if (false !== stripos($sort ?? '', ' asc') || false !== stripos($sort ?? '', ' desc')) { - $query->sort($sort); - } else { - $list->applyRelation($sort, $column, true); - $query->sort($column, 'ASC'); - } - } elseif (is_array($sort)) { - // sort(array('Name'=>'desc')); - $query->sort(null, null); // wipe the sort - - foreach ($sort as $column => $direction) { - // Convert column expressions to SQL fragment, while still allowing the passing of raw SQL - // fragments. - $list->applyRelation($column, $relationColumn, true); - $query->sort($relationColumn, $direction, false); - } - } + /** + * Set an explicit ORDER BY statement using raw SQL + * + * This method accepts raw SQL so could be vulnerable to SQL injection attacks if used incorrectly, + * it's preferable to use sort() instead which does not allow raw SQL + */ + public function orderBy(string $orderBy): static + { + return $this->alterDataQuery(function (DataQuery $query) use ($orderBy) { + $query->sort($orderBy, null, true); }); } /** * Return a copy of this list which only includes items with these characteristics * + * Raw SQL is not accepted, only actual field names can be passed + * * @see Filterable::filter() * * @example $list = $list->filter('Name', 'bob'); // only bob in the list @@ -433,6 +484,8 @@ public function addFilter($filterArray) /** * Return a copy of this list which contains items matching any of these characteristics. * + * Raw SQL is not accepted, only actual field names can be passed + * * @example // only bob in the list * $list = $list->filterAny('Name', 'bob'); * // SQL: WHERE "Name" = 'bob' @@ -564,7 +617,7 @@ function (DataQuery $query) use ($field, &$columnName, $linearOnly) { */ protected function isValidRelationName($field) { - return preg_match('/^[A-Z0-9._]+$/i', $field ?? ''); + return preg_match('/^[A-Z0-9\._]+$/i', $field ?? ''); } /** @@ -608,6 +661,8 @@ protected function createSearchFilter($filter, $value) /** * Return a copy of this list which does not contain any items that match all params * + * Raw SQL is not accepted, only actual field names can be passed + * * @example $list = $list->exclude('Name', 'bob'); // exclude bob from list * @example $list = $list->exclude('Name', array('aziz', 'bob'); // exclude aziz and bob from list * @example $list = $list->exclude(array('Name'=>'bob, 'Age'=>21)); // exclude bob that has Age 21 @@ -648,6 +703,8 @@ public function exclude() /** * Return a copy of this list which does not contain any items with any of these params * + * Raw SQL is not accepted, only actual field names can be passed + * * @example $list = $list->excludeAny('Name', 'bob'); // exclude bob from list * @example $list = $list->excludeAny('Name', array('aziz', 'bob'); // exclude aziz and bob from list * @example $list = $list->excludeAny(array('Name'=>'bob, 'Age'=>21)); // exclude bob or Age 21 @@ -1190,7 +1247,7 @@ public function removeByFilter($filter) */ public function shuffle() { - return $this->sort(DB::get_conn()->random()); + return $this->orderBy(DB::get_conn()->random()); } /** diff --git a/src/ORM/DataObject.php b/src/ORM/DataObject.php index 25be4ab955a..22fb633c0eb 100644 --- a/src/ORM/DataObject.php +++ b/src/ORM/DataObject.php @@ -2254,7 +2254,7 @@ public function getManyManyComponents($componentName, $id = null) // If we have a default sort set for our "join" then we should overwrite any default already set. $joinSort = Config::inst()->get($manyManyComponent['join'], 'default_sort'); if (!empty($joinSort)) { - $result = $result->sort($joinSort); + $result = $result->orderBy($joinSort); } $this->extend('updateManyManyComponents', $result); @@ -3327,7 +3327,7 @@ public function getReverseAssociation($className) * @param string $callerClass The class of objects to be returned * @param string|array $filter A filter to be inserted into the WHERE clause. * Supports parameterised queries. See SQLSelect::addWhere() for syntax examples. - * @param string|array $sort A sort expression to be inserted into the ORDER + * @param string|array|null $sort Passed to DataList::sort() * BY clause. If omitted, self::$default_sort will be used. * @param string $join Deprecated 3.0 Join clause. Use leftJoin($table, $joinClause) instead. * @param string|array $limit A limit expression to be inserted into the LIMIT clause. @@ -3369,7 +3369,7 @@ public static function get( if ($filter) { $result = $result->where($filter); } - if ($sort) { + if ($sort || is_null($sort)) { $result = $result->sort($sort); } if ($limit && strpos($limit ?? '', ',') !== false) { @@ -3399,11 +3399,11 @@ public static function get( * e.g. MyObject::get_one() will return a MyObject * @param string|array $filter A filter to be inserted into the WHERE clause. * @param boolean $cache Use caching - * @param string $orderBy A sort expression to be inserted into the ORDER BY clause. + * @param string|array|null $sort Passed to DataList::sort() so that DataList::first() returns the desired item * * @return DataObject|null The first item matching the query */ - public static function get_one($callerClass = null, $filter = "", $cache = true, $orderBy = "") + public static function get_one($callerClass = null, $filter = "", $cache = true, $sort = "") { if ($callerClass === null) { $callerClass = static::class; @@ -3417,12 +3417,18 @@ public static function get_one($callerClass = null, $filter = "", $cache = true, /** @var DataObject $singleton */ $singleton = singleton($callerClass); - $cacheComponents = [$filter, $orderBy, $singleton->getUniqueKeyComponents()]; + $cacheComponents = [$filter, $sort, $singleton->getUniqueKeyComponents()]; $cacheKey = md5(serialize($cacheComponents)); $item = null; if (!$cache || !isset(self::$_cache_get_one[$callerClass][$cacheKey])) { - $dl = DataObject::get($callerClass)->where($filter)->sort($orderBy); + $dl = DataObject::get($callerClass); + if (!empty($filter)) { + $dl = $dl->where($filter); + } + if (!empty($sort) || is_null($sort)) { + $dl = $dl->sort($sort); + } $item = $dl->first(); if ($cache) { diff --git a/src/ORM/DataQuery.php b/src/ORM/DataQuery.php index 89e9dae7442..801f112a313 100644 --- a/src/ORM/DataQuery.php +++ b/src/ORM/DataQuery.php @@ -716,12 +716,17 @@ public function whereAny($filter) /** * Set the ORDER BY clause of this query * + * Note: while the similarly named DataList::sort() does not allow raw SQL, DataQuery::sort() does allow it + * + * Raw SQL can be vulnerable to SQL injection attacks if used incorrectly, so it's preferable not to use it + * * @see SQLSelect::orderby() * * @param string $sort Column to sort on (escaped SQL statement) * @param string $direction Direction ("ASC" or "DESC", escaped SQL statement) * @param bool $clear Clear existing values * @return $this + * */ public function sort($sort = null, $direction = null, $clear = true) { diff --git a/src/ORM/Search/SearchContext.php b/src/ORM/Search/SearchContext.php index 1b751de0031..e4ad3bd6d4e 100644 --- a/src/ORM/Search/SearchContext.php +++ b/src/ORM/Search/SearchContext.php @@ -204,8 +204,10 @@ private function prepareQuery($sort, $limit, ?DataList $existingQuery): DataList } else { $query = $query->limit($limit); } - - return $query->sort($sort); + if (!empty($sort) || is_null($sort)) { + $query = $query->sort($sort); + } + return $query; } /** diff --git a/src/Security/InheritedPermissions.php b/src/Security/InheritedPermissions.php index 1066d18de1f..a327cbf3b0d 100644 --- a/src/Security/InheritedPermissions.php +++ b/src/Security/InheritedPermissions.php @@ -393,7 +393,7 @@ protected function batchPermissionCheckForStage( // of whether the user has permission to edit this object. $groupedByParent = []; $potentiallyInherited = $stageRecords->filter($typeField, self::INHERIT) - ->sort("\"{$baseTable}\".\"ID\"") + ->orderBy("\"{$baseTable}\".\"ID\"") ->dataQuery() ->query() ->setSelect([ diff --git a/tests/php/ORM/DataListTest.php b/tests/php/ORM/DataListTest.php index 8ce1de790bc..9b1002448bd 100755 --- a/tests/php/ORM/DataListTest.php +++ b/tests/php/ORM/DataListTest.php @@ -2,10 +2,12 @@ namespace SilverStripe\ORM\Tests; +use Exception; use InvalidArgumentException; use SilverStripe\Core\Convert; use SilverStripe\Core\Injector\InjectorNotFoundException; use SilverStripe\Dev\SapphireTest; +use SilverStripe\ORM\Connect\MySQLiConnector; use SilverStripe\ORM\DataList; use SilverStripe\ORM\DataQuery; use SilverStripe\ORM\DB; @@ -1816,12 +1818,12 @@ public function testReverse() $this->assertEquals('Phil', $list->first()->Name, 'First comment should be from Phil'); } - public function testSortByComplexExpression() + public function testOrderByComplexExpression() { // Test an expression with both spaces and commas. This test also tests that column() can be called // with a complex sort expression, so keep using column() below $teamClass = Convert::raw2sql(SubTeam::class); - $list = Team::get()->sort( + $list = Team::get()->orderBy( 'CASE WHEN "DataObjectTest_Team"."ClassName" = \'' . $teamClass . '\' THEN 0 ELSE 1 END, "Title" DESC' ); $this->assertEquals( @@ -1837,6 +1839,155 @@ public function testSortByComplexExpression() ); } + /** + * @dataProvider provideRawSqlSortException + */ + public function testRawSqlSort(string $sort, string $type): void + { + $type = explode('|', $type)[0]; + if ($type === 'valid') { + $this->expectNotToPerformAssertions(); + } elseif ($type === 'invalid-direction') { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessageMatches('/Invalid sort direction/'); + } elseif ($type === 'unknown-column') { + if (!(DB::get_conn()->getConnector() instanceof MySQLiConnector)) { + $this->markTestSkipped('Database connector is not MySQLiConnector'); + } + $this->expectException(\mysqli_sql_exception::class); + $this->expectExceptionMessageMatches('/Unknown column/'); + } elseif ($type === 'invalid-column') { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessageMatches('/Invalid sort column/'); + } elseif ($type === 'unknown-relation') { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessageMatches('/is not a relation on model/'); + } else { + throw new \Exception("Invalid type $type"); + } + // column('ID') is required to get the database query be actually fired off + Team::get()->sort($sort)->column('ID'); + } + + /** + * @dataProvider provideRawSqlSortException + */ + public function testRawSqlOrderBy(string $sort, string $type): void + { + $type = explode('|', $type)[1]; + if ($type === 'valid') { + if (!str_contains($sort, '"') && !(DB::get_conn()->getConnector() instanceof MySQLiConnector)) { + // don't test unquoted things in non-mysql + $this->markTestSkipped('Database connector is not MySQLiConnector'); + } + $this->expectNotToPerformAssertions(); + } else { + if (!(DB::get_conn()->getConnector() instanceof MySQLiConnector)) { + $this->markTestSkipped('Database connector is not MySQLiConnector'); + } + $this->expectException(\mysqli_sql_exception::class); + if ($type === 'error-in-sql-syntax') { + $this->expectExceptionMessageMatches('/You have an error in your SQL syntax/'); + } else { + $this->expectExceptionMessageMatches('/Unknown column/'); + } + } + // column('ID') is required to get the database query be actually fired off + Team::get()->orderBy($sort)->column('ID'); + } + + public function provideRawSqlSortException(): array + { + return [ + ['Title', 'valid|valid'], + ['Title asc', 'valid|valid'], + ['"Title" ASC', 'valid|valid'], + ['Title ASC, "DatabaseField"', 'valid|valid'], + ['"Title", "DatabaseField" DESC', 'valid|valid'], + ['Title ASC, DatabaseField DESC', 'valid|valid'], + ['Title ASC, , DatabaseField DESC', 'invalid-column|unknown-column'], + ['Captain.ShirtNumber', 'valid|unknown-column'], + ['Captain.ShirtNumber ASC', 'valid|unknown-column'], + ['"Captain"."ShirtNumber"', 'invalid-column|unknown-column'], + ['"Captain"."ShirtNumber" DESC', 'invalid-column|unknown-column'], + ['Title BACKWARDS', 'invalid-direction|error-in-sql-syntax'], + ['"Strange non-existent column name"', 'invalid-column|unknown-column'], + ['NonExistentColumn', 'unknown-column|unknown-column'], + ['Team.NonExistentColumn', 'unknown-relation|unknown-column'], + ['"DataObjectTest_Team"."NonExistentColumn" ASC', 'invalid-column|unknown-column'], + ['"DataObjectTest_Team"."Title" ASC', 'invalid-column|valid'], + ['DataObjectTest_Team.Title', 'unknown-relation|valid'], + ['Title, 1 = 1', 'invalid-column|valid'], + ["Title,'abc' = 'abc'", 'invalid-column|valid'], + ['Title,Mod(ID,3)=1', 'invalid-column|valid'], + ['(CASE WHEN ID < 3 THEN 1 ELSE 0 END)', 'invalid-column|valid'], + ]; + } + + /** + * @dataProvider provideSortDirectionValidationTwoArgs + */ + public function testSortDirectionValidationTwoArgs(string $direction, string $type): void + { + if ($type === 'valid') { + $this->expectNotToPerformAssertions(); + } else { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessageMatches('/Invalid sort direction/'); + } + Team::get()->sort('Title', $direction)->column('ID'); + } + + public function provideSortDirectionValidationTwoArgs(): array + { + return [ + ['ASC', 'valid'], + ['asc', 'valid'], + ['DESC', 'valid'], + ['desc', 'valid'], + ['BACKWARDS', 'invalid'], + ]; + } + + /** + * Test passing scalar values to sort() + * + * Explicity tests that sort(null) will wipe any existing sort on a DataList + * + * @dataProvider provideSortScalarValues + */ + public function testSortScalarValues(mixed $emtpyValue, string $type): void + { + $this->assertSame(['Subteam 1'], Team::get()->limit(1)->column('Title')); + $list = Team::get()->sort('Title DESC'); + $this->assertSame(['Team 3'], $list->limit(1)->column('Title')); + if ($type !== 'wipes-existing') { + $this->expectException(InvalidArgumentException::class); + } + if ($type === 'invalid-scalar') { + $this->expectExceptionMessage('sort() arguments must either be a string, an array, or null'); + } + if ($type === 'empty-scalar') { + $this->expectExceptionMessage('Invalid sort paramater'); + } + // $type === 'wipes-existing' is valid + $list = $list->sort($emtpyValue); + $this->assertSame(['Subteam 1'], $list->limit(1)->column('Title')); + } + + public function provideSortScalarValues(): array + { + return [ + [null, 'wipes-existing'], + ['', 'empty-scalar'], + [[], 'empty-scalar'], + [false, 'invalid-scalar'], + [true, 'invalid-scalar'], + [0, 'invalid-scalar'], + [1, 'invalid-scalar'], + ]; + } + public function testShuffle() { $list = Team::get()->shuffle(); diff --git a/tests/php/ORM/DataObjectTest.php b/tests/php/ORM/DataObjectTest.php index 04787f883fe..cb6536f57ce 100644 --- a/tests/php/ORM/DataObjectTest.php +++ b/tests/php/ORM/DataObjectTest.php @@ -1034,10 +1034,10 @@ public function testRandomSort() $this->assertFalse($obj->isChanged()); /* If we perform the same random query twice, it shouldn't return the same results */ - $itemsA = DataObject::get(DataObjectTest\TeamComment::class, "", DB::get_conn()->random()); - $itemsB = DataObject::get(DataObjectTest\TeamComment::class, "", DB::get_conn()->random()); - $itemsC = DataObject::get(DataObjectTest\TeamComment::class, "", DB::get_conn()->random()); - $itemsD = DataObject::get(DataObjectTest\TeamComment::class, "", DB::get_conn()->random()); + $itemsA = DataObject::get(DataObjectTest\TeamComment::class, "")->orderBy(DB::get_conn()->random()); + $itemsB = DataObject::get(DataObjectTest\TeamComment::class)->orderBy(DB::get_conn()->random()); + $itemsC = DataObject::get(DataObjectTest\TeamComment::class)->orderBy(DB::get_conn()->random()); + $itemsD = DataObject::get(DataObjectTest\TeamComment::class)->orderBy(DB::get_conn()->random()); foreach ($itemsA as $item) { $keysA[] = $item->ID; } @@ -1914,7 +1914,7 @@ public function testManyManyExtraFields() // Check that ordering a many-many relation by an aggregate column doesn't fail $player = $this->objFromFixture(DataObjectTest\Player::class, 'player2'); - $player->Teams()->sort("count(DISTINCT \"DataObjectTest_Team_Players\".\"DataObjectTest_PlayerID\") DESC"); + $player->Teams()->orderBy("count(DISTINCT \"DataObjectTest_Team_Players\".\"DataObjectTest_PlayerID\") DESC"); } /** diff --git a/tests/php/ORM/ManyManyThroughListTest.php b/tests/php/ORM/ManyManyThroughListTest.php index 26211d7c207..168f762d4d2 100644 --- a/tests/php/ORM/ManyManyThroughListTest.php +++ b/tests/php/ORM/ManyManyThroughListTest.php @@ -102,7 +102,7 @@ public function testSorting($sort, $expected) $items = $parent->Items(); if ($sort) { - $items = $items->sort($sort); + $items = $items->orderBy($sort); } $this->assertSame($expected, $items->column('Title')); }