diff --git a/src/ORM/DataList.php b/src/ORM/DataList.php index e9f65b04dc0..7a718b536de 100644 --- a/src/ORM/DataList.php +++ b/src/ORM/DataList.php @@ -12,6 +12,7 @@ use Exception; use InvalidArgumentException; use LogicException; +use SilverStripe\ORM\Connect\DatabaseException; /** * Implements a "lazy loading" DataObjectSet. @@ -237,8 +238,6 @@ public function whereAny($filter) }); } - - /** * Returns true if this DataList can be sorted by the given field. * @@ -308,62 +307,67 @@ 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 columns 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']); */ - 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'); - } - - $sort = [$col => $dir]; + list($column, $direction) = $args; + $sort = [$column => $direction]; } else { - $sort = func_get_arg(0); + $sort = $args[0]; + // 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) { + if (empty($colDir)) { + continue; + } + // Using regex instead of explode(' ') in case column name includes spaces + if (preg_match('/^(.+) (asc|desc)$/i', trim($colDir), $matches)) { + list($column, $direction) = [$matches[1], $matches[2]]; + } else { + list($column, $direction) = [$colDir, 'ASC']; + } + $newSort[$column] = $direction; + } + $sort = $newSort; + } + } + // Validate that column names are not raw sql + if (is_array($sort)) { + foreach (array_keys($sort) as $column) { + $this->validateColumnHasNoRawSql($column); + } } - return $this->alterDataQuery(function (DataQuery $query, DataList $list) use ($sort) { - if (is_string($sort) && $sort) { - if (false !== stripos($sort ?? '', ' asc') || false !== stripos($sort ?? '', ' desc')) { + if (preg_match('/^(.+) (asc|desc)$/i', $sort)) { $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 - + // Wipe the sort + $query->sort(null, null); 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); } @@ -371,6 +375,29 @@ public function sort() }); } + private function validateColumnHasNoRawSql(string $column): void + { + $col = trim(str_replace('"', '', $column)); + // Search for non-alphanumeric characters in the column name rather than ^[^a-zA-Z0-9_]$ to account + // for column names that include macrons etc + if (preg_match('#[\(\)\[\]{}\+\-\*/=\:\?\\;|&\'"`<>^%, ]#', $col)) { + throw new InvalidArgumentException("Invalid column name $column"); + } + } + + /** + * Set an explicit ORDER BY statement using raw SQL + * + * Note: this may be vulnerable to SQL injection so most of the time you should + * use sort() instead which doesn't 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 * @@ -1190,7 +1217,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/DataQuery.php b/src/ORM/DataQuery.php index 89e9dae7442..9c2690f494a 100644 --- a/src/ORM/DataQuery.php +++ b/src/ORM/DataQuery.php @@ -716,12 +716,15 @@ 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 + * * @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/tests/php/ORM/DataListTest.php b/tests/php/ORM/DataListTest.php index 8ce1de790bc..7a527d99dbc 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,67 @@ public function testSortByComplexExpression() ); } + /** + * @dataProvider provideRawSqlSortException + */ + public function testRawSqlSort(string $sort, string $type): void + { + if ($type === 'valid') { + $this->expectNotToPerformAssertions(); + } 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/'); + } else { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessageMatches('/Invalid column name/'); + } + // 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 + { + if ($type === 'valid' || $type === 'order-by-only') { + $this->expectNotToPerformAssertions(); + } else { + if (!(DB::get_conn()->getConnector() instanceof MySQLiConnector)) { + $this->markTestSkipped('Database connector is not MySQLiConnector'); + } + $this->expectException(\mysqli_sql_exception::class); + if ($sort === 'Title BACKWARDS') { + $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'], + ['Title asc', 'valid'], + ['"Title" ASC', 'valid'], + ['Title ASC, "DatabaseField"', 'valid'], + ['"Title", "DatabaseField" DESC', 'valid'], + ['Title BACKWARDS', 'invalid'], + ['NonExistentColumn', 'unknown-column'], + ['"Strange non-existent column name"', 'invalid'], + ['Title, 1 = 1', 'order-by-only'], + ["Title,'abc' = 'abc'", 'order-by-only'], + ['Title,Mod(ID,3)=1', 'order-by-only'], + ['(CASE WHEN ID < 3 THEN 1 ELSE 0 END)', 'order-by-only'], + ]; + } + public function testShuffle() { $list = Team::get()->shuffle(); diff --git a/tests/php/ORM/DataObjectTest.php b/tests/php/ORM/DataObjectTest.php index 04787f883fe..8bfa6cc511c 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; }