diff --git a/core/lib/Drupal/Core/Database/Query/Select.php b/core/lib/Drupal/Core/Database/Query/Select.php index dde68082b73..1f5072b2974 100644 --- a/core/lib/Drupal/Core/Database/Query/Select.php +++ b/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); } diff --git a/core/lib/Drupal/Core/Database/Query/SelectInterface.php b/core/lib/Drupal/Core/Database/Query/SelectInterface.php index db66aefa27c..645fd78b07c 100644 --- a/core/lib/Drupal/Core/Database/Query/SelectInterface.php +++ b/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: + * + * $query->addExpression('SUBSTRING(thread, 1, (LENGTH(thread) - 1))', 'order_field'); + * $query->orderBy('order_field', 'ASC'); + * * @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. */ diff --git a/core/lib/Drupal/Core/Database/Query/TableSortExtender.php b/core/lib/Drupal/Core/Database/Query/TableSortExtender.php index a9bcd400c5e..4786f5973c5 100644 --- a/core/lib/Drupal/Core/Database/Query/TableSortExtender.php +++ b/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; } diff --git a/core/lib/Drupal/Core/Entity/Query/Sql/Query.php b/core/lib/Drupal/Core/Entity/Query/Sql/Query.php index f5c84b445de..883513e2a57 100644 --- a/core/lib/Drupal/Core/Entity/Query/Sql/Query.php +++ b/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; diff --git a/core/lib/Drupal/Core/Entity/Query/Sql/QueryAggregate.php b/core/lib/Drupal/Core/Entity/Query/Sql/QueryAggregate.php index 2658f0de642..c76a773e67d 100644 --- a/core/lib/Drupal/Core/Entity/Query/Sql/QueryAggregate.php +++ b/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; diff --git a/core/modules/comment/comment.module b/core/modules/comment/comment.module index fc4308ddce2..1f5201eba09 100644 --- a/core/modules/comment/comment.module +++ b/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(); diff --git a/core/tests/Drupal/Tests/Core/Database/OrderByTest.php b/core/tests/Drupal/Tests/Core/Database/OrderByTest.php new file mode 100644 index 00000000000..e69b11a25ad --- /dev/null +++ b/core/tests/Drupal/Tests/Core/Database/OrderByTest.php @@ -0,0 +1,63 @@ + '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.'); + } +}