diff --git a/src/ORM/DataList.php b/src/ORM/DataList.php index e9f65b04dc0..0ff0673b5f9 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 * - * @see SS_List::sort() - * @see SQLSelect::orderby + * Raw SQL is not accepted, only actual field names can be passed + * + * @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->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; + } + } + // Validate that column names are not raw sql + if (is_array($sort)) { + foreach ($sort as $column => $direction) { + $this->validateColumnHasNoRawSql($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 validateColumnHasNoRawSql(string $column): void + { + // Remove quotes around the column name + $col = trim(trim($column), '"'); + // Using regex because $this->canSortBy() failed on complex relations e.g. Team.Founder.Surname and + // also increased the number of db queries + // 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('#[()[\]{}+\-*/=:?\\;|&\'"`<>^%,\s]#', $col)) { + $message = implode(' ', [ + "Column name $column includes characters that could lead to SQL injection vulnerabilities.", + 'Rename this column or use DataList::orderBy() for sorting instead.' + ]); + throw new InvalidArgumentException($message); } + } - 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' @@ -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..0219bf087c8 100644 --- a/src/ORM/DataObject.php +++ b/src/ORM/DataObject.php @@ -3422,7 +3422,13 @@ public static function get_one($callerClass = null, $filter = "", $cache = true, $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($orderBy)) { + $dl = $dl->sort($orderBy); + } $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/tests/php/ORM/DataListTest.php b/tests/php/ORM/DataListTest.php index 8ce1de790bc..d3e326d2a30 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,135 @@ public function testSortByComplexExpression() ); } + /** + * @dataProvider provideRawSqlSortException + */ + public function testRawSqlSort(string $sort, string $type): void + { + 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/'); + } else { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessageMatches('/includes characters that could lead to SQL injection/'); + } + // 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 ($type === 'invalid-direction') { + $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 ASC, DatabaseField DESC', 'valid'], + ['Title BACKWARDS', 'invalid-direction'], + ['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'], + ]; + } + + /** + * @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"); } /**