diff --git a/src/ORM/DataList.php b/src/ORM/DataList.php index e9f65b04dc0..cb901bdc4d0 100644 --- a/src/ORM/DataList.php +++ b/src/ORM/DataList.php @@ -308,8 +308,10 @@ 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. + * + * sort() does not allow the use of raw SQL as a safe quard against SQL injection attacks + * Use sortRaw() instead if you must use raw SQL * * @see SS_List::sort() * @see SQLSelect::orderby @@ -321,46 +323,83 @@ public function distinct($value) * @param string|array Escaped SQL statement. If passed as array, all keys and values are assumed to be escaped. * @return static */ - public function sort() + public function sort(...$args) + { + return $this->sortInner(false, $args); + } + + /** + * Return a new DataList instance as a copy of this data list with the sort order set + * + * The methid is the same as sort(), though allows raw SQL to be used for sorting + * This method is open to SQL injection attacks so use with caution + * + * @see SS_List::sort() + * @see SQLSelect::orderby + * @example $list = $list->sort('Name'); // default ASC sorting + * @example $list = $list->sort('Mod(ID, 3) ASC, Name ASC'); // Raw SQL sort + * @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 + */ + public function sortRaw(...$args) { - $count = func_num_args(); + return $this->sortInner(true, $args); + } + private function sortInner(bool $allowRawSql, array $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 and not allowing raw sql, convert string to array to allow for validation + if (is_string($sort) && !$allowRawSql) { + $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('/^(.+) (asc|desc)$/i', trim($colDir), $matches)) { + list($column, $direction) = [$matches[1], $matches[2]]; + } else { + list($column, $direction) = [$colDir, 'ASC']; + } + $newSort[$column] = $direction; + } + $sort = $newSort; + } + } + // validation + if (is_array($sort)) { + foreach ($sort as $column => $direction) { + if (!$allowRawSql) { + $this->validateColumnNotRawSQL($column); + } + $this->validateDirection($direction); + } } - 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. @@ -371,6 +410,30 @@ public function sort() }); } + private function validateDirection(string $direction): void + { + $dir = strtolower($direction); + if ($dir !== 'asc' && $dir !== 'desc') { + throw new InvalidArgumentException('Second argument to sort must be either ASC or DESC'); + } + } + + private function validateColumnNotRawSQL(string $column): void + { + // Remove anything surronding by double quotes as it's used my MySQLiConnector and postgres native + // to quote table and column names. This does allow strange characters in column names such as spaces + preg_match_all('#"([^"]+)"#', $column, $matches); + foreach ($matches[1] as $match) { + $column = preg_replace('#"' . preg_quote($match, '#') . '"#', '', $column); + } + $column = trim($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('#[\(\)\[\]{}\+\-\*/=\:\?\\;|&\'"`<>^%, ]#', $column)) { + throw new InvalidArgumentException('Raw SQL cannot be used for sort(), use sortRaw() instead'); + } + } + /** * Return a copy of this list which only includes items with these characteristics * @@ -1190,7 +1253,7 @@ public function removeByFilter($filter) */ public function shuffle() { - return $this->sort(DB::get_conn()->random()); + return $this->sortRaw(DB::get_conn()->random()); } /** diff --git a/src/ORM/DataQuery.php b/src/ORM/DataQuery.php index 89e9dae7442..bd4b2a4a632 100644 --- a/src/ORM/DataQuery.php +++ b/src/ORM/DataQuery.php @@ -722,6 +722,7 @@ public function whereAny($filter) * @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..5ed79d956c7 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 testSortByRawComplexExpression() { // 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()->sortRaw( 'CASE WHEN "DataObjectTest_Team"."ClassName" = \'' . $teamClass . '\' THEN 0 ELSE 1 END, "Title" DESC' ); $this->assertEquals( @@ -1837,6 +1839,68 @@ public function testSortByComplexExpression() ); } + /** + * @dataProvider provideRawSqlSortException + */ + public function testRawSqlSortException(string $sort, string $failureType): void + { + if ($failureType === 'raw-sql') { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('Raw SQL cannot be used for sort(), use sortRaw() instead'); + } elseif ($failureType === 'unknown-column') { + if (!(DB::get_conn()->getConnector() instanceof MySQLiConnector)) { + $this->markTestSkipped('Database connector is not MySQLiConnector'); + } + // It's surprising this is mysqli_sql_exception when it probably should be DatabaseException + $this->expectException(\mysqli_sql_exception::class); + $this->expectExceptionMessageMatches('/Unknown column/'); + } elseif ($failureType === 'valid') { + $this->expectNotToPerformAssertions(); + } else { + throw new Exception('Invalid $failureType'); + } + // ->column('ID') is required to get the database query be actually fired off + Team::get()->sort($sort)->column('ID'); + } + + /** + * @dataProvider provideRawSqlSortException + */ + public function testRawSqlSortRawNoException(string $sort, string $failureType): void + { + if ($failureType === 'unknown-column') { + if (!(DB::get_conn()->getConnector() instanceof MySQLiConnector)) { + $this->markTestSkipped('Database connector is not MySQLiConnector'); + } + // It's surprising this is mysqli_sql_exception when it probably should be DatabaseException + $this->expectException(\mysqli_sql_exception::class); + $this->expectExceptionMessageMatches('/Unknown column/'); + } elseif ($failureType === 'valid' || $failureType == 'raw-sql') { + $this->expectNotToPerformAssertions(); + } else { + throw new Exception('Invalid $failureType'); + } + // ->column('ID') is required to get the database query be actually fired off + Team::get()->sortRaw($sort)->column('ID'); + } + + public function provideRawSqlSortException(): array + { + return [ + ['Title', 'valid'], + ['Title asc', 'valid'], + ['"Title" ASC', 'valid'], + ['Title ASC, "DatabaseField"', 'valid'], + ['"Title", "DatabaseField" DESC', 'valid'], + ['NonExistentColumn', 'unknown-column'], + ['"Strange non-existent column name"', 'unknown-column'], + ['Title, 1 = 1', 'raw-sql'], + ["Title,'abc' = 'abc'", 'raw-sql'], + ['Title,Mod(ID,3)=1', 'raw-sql'], + ['(CASE WHEN ID < 3 THEN 1 ELSE 0 END)', 'raw-sql'], + ]; + } + public function testShuffle() { $list = Team::get()->shuffle();