Issue #829464 by Berdir, Github sync, sepgil: Orderby() should escape fields and verify direction.

core/lib/Drupal/Core/Database/Query/Select.php

@@ -541,6 +541,8 @@ class Select extends Query implements SelectInterface {
   }
 
   public function orderBy($field, $direction = 'ASC') {
+    // Only allow ASC and DESC, default to ASC.
+    $direction = strtoupper($direction) == 'DESC' ? 'DESC' : 'ASC';
     $this->order[$field] = $direction;
     return $this;
   }
@@ -748,7 +750,7 @@ class Select extends Query implements SelectInterface {
       $query .= "\nORDER BY ";
       $fields = array();
       foreach ($this->order as $field => $direction) {
-        $fields[] = $field . ' ' . $direction;
+        $fields[] = $this->connection->escapeField($field) . ' ' . $direction;
       }
       $query .= implode(', ', $fields);
     }

core/lib/Drupal/Core/Database/Query/SelectInterface.php

@@ -363,9 +363,19 @@ interface SelectInterface extends ConditionInterface, AlterableInterface, Extend
    * called.
    *
    * @param $field
-   *   The field on which to order.
+   *   The field on which to order. The field is escaped for security so only
+   *   valid field and alias names are possible. To order by an expression, add
+   *   the expression with addExpression() first and then use the alias to order
+   *   on.
+   *
+   *   Example:
+   *   <code>
+   *   $query->addExpression('SUBSTRING(thread, 1, (LENGTH(thread) - 1))', 'order_field');
+   *   $query->orderBy('order_field', 'ASC');
+   *   </code>
    * @param $direction
-   *   The direction to sort. Legal values are "ASC" and "DESC".
+   *   The direction to sort. Legal values are "ASC" and "DESC". Any other value
+   *   will be converted to "ASC".
    * @return \Drupal\Core\Database\Query\SelectInterface
    *   The called object.
    */

core/lib/Drupal/Core/Database/Query/TableSortExtender.php

@@ -46,10 +46,9 @@ class TableSortExtender extends SelectExtender {
       // Based on code from db_escape_table(), but this can also contain a dot.
       $field = preg_replace('/[^A-Za-z0-9_.]+/', '', $ts['sql']);
 
-      // Sort order can only be ASC or DESC.
-      $sort = drupal_strtoupper($ts['sort']);
-      $sort = in_array($sort, array('ASC', 'DESC')) ? $sort : '';
-      $this->orderBy($field, $sort);
+      // orderBy() will ensure that only ASC/DESC values are accepted, so we
+      // don't need to sanitize that here.
+      $this->orderBy($field, $ts['sort']);
     }
     return $this;
   }

core/lib/Drupal/Core/Entity/Query/Sql/Query.php

@@ -222,8 +222,8 @@ class Query extends QueryBase implements QueryInterface {
         // if the direction is descending.
         $function = $direction == 'ASC' ? 'min' : 'max';
         $expression = "$function($sql_alias)";
-        $this->sqlQuery->addExpression($expression);
-        $this->sqlQuery->orderBy($expression, $direction);
+        $expression_alias = $this->sqlQuery->addExpression($expression);
+        $this->sqlQuery->orderBy($expression_alias, $direction);
       }
     }
     return $this;

core/lib/Drupal/Core/Entity/Query/Sql/QueryAggregate.php

@@ -124,7 +124,7 @@ class QueryAggregate extends Query implements QueryAggregateInterface {
   protected function addSortAggregate() {
     if(!$this->count) {
       foreach ($this->sortAggregate as $alias => $sort) {
-        $this->sqlQuery->orderBy($this->sqlExpressions[$alias], $sort['direction']);
+        $this->sqlQuery->orderBy($alias, $sort['direction']);
       }
     }
     return $this;

core/modules/comment/comment.module

@@ -370,9 +370,11 @@ function comment_new_page_count($num_comments, $new_replies, EntityInterface $en
       ->range(0, $new_replies);
 
     // 2. Find the first thread.
-    $first_thread = db_select($unread_threads_query, 'thread')
+    $first_thread_query = db_select($unread_threads_query, 'thread');
+    $first_thread_query->addExpression('SUBSTRING(thread, 1, (LENGTH(thread) - 1))', 'torder');
+    $first_thread = $first_thread_query
       ->fields('thread', array('thread'))
-      ->orderBy('SUBSTRING(thread, 1, (LENGTH(thread) - 1))')
+      ->orderBy('torder')
       ->range(0, 1)
       ->execute()
       ->fetchField();

core/tests/Drupal/Tests/Core/Database/OrderByTest.php

@@ -0,0 +1,63 @@
+<?php
+
+/**
+ * @file
+ * Contains Drupal\Tests\Core\Database\OrderByTest.
+ */
+
+namespace Drupal\Tests\Core\Database;
+
+use Drupal\Core\Database\Query\Select;
+use Drupal\Tests\UnitTestCase;
+
+/**
+ * Tests the orderBy() method of select queries.
+ */
+class OrderByTest extends UnitTestCase {
+
+  /**
+   * The select query object to test.
+   *
+   * @var \Drupal\Core\Database\Query\Select
+   */
+  protected $query;
+
+  /**
+   * {@inheritdoc}
+   */
+  public static function getInfo() {
+    return array(
+      'name' => 'Order by',
+      'description' => 'Tests the orderBy() method of select queries.',
+      'group' => 'Database',
+    );
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function setUp() {
+    $connection = $this->getMockBuilder('Drupal\Core\Database\Connection')
+      ->disableOriginalConstructor()
+      ->getMockForAbstractClass();
+    $this->query = new Select('test', NULL, $connection);
+  }
+
+  /**
+   * Checks that invalid sort directions in ORDER BY get converted to ASC.
+   */
+  public function testInvalidDirection() {
+    $this->query->orderBy('test', 'invalid direction');
+    $order_bys = $this->query->getOrderBy();
+    $this->assertEquals($order_bys['test'], 'ASC', 'Invalid order by direction is converted to ASC.');
+  }
+
+  /**
+   * Tests that fields passed for ordering get escaped properly.
+   */
+  public function testFieldEscaping() {
+    $this->query->orderBy('x; DROP table node; --');
+    $sql = $this->query->__toString();
+    $this->assertStringEndsWith('ORDER BY xDROPtablenode ASC', $sql, 'Order by field is escaped correctly.');
+  }
+}