API Create orderBy() method to handle raw SQL

silverstripe · Dec 6, 2022 · 855e65a · 855e65a
1 parent 7860e46
commit 855e65a
Show file tree

Hide file tree

Showing 8 changed files with 286 additions and 65 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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([