Build SQL with Arel instead of from strings (#2418)

In Administrate::Order, there are several places where we are building
SQL expressions using string interpolation, e.g. order =
"#{relation.table_name}.#{attribute} #{direction}". Doing this can be
dangerous, because it's easy to introduce a SQL injection vulnerability.
Thankfully, it seems like this class is carefully designed in a way that
prevents such a vulnerability. Nevertheless, it feels precarious; if
someone makes changes, they have to be very careful to keep the code
safe.

This change replaces all string-interpolated SQL expressions with Arel
expressions. The database adapter will then automatically quote &
sanitize the components of these expressions.

lib/administrate/order.rb

@@ -10,9 +10,9 @@ def apply(relation)
       return order_by_association(relation) unless
         reflect_association(relation).nil?
 
-      order = "#{relation.table_name}.#{attribute} #{direction}"
+      order = relation.arel_table[attribute].public_send(direction)
 
-      return relation.reorder(Arel.sql(order)) if
+      return relation.reorder(order) if
         column_exist?(relation, attribute)
 
       relation
@@ -66,11 +66,11 @@ def order_by_association(relation)
 
     def order_by_count(relation)
       klass = reflect_association(relation).klass
-      query = "COUNT(#{klass.table_name}.#{klass.primary_key}) #{direction}"
+      query = klass.arel_table[klass.primary_key].count.public_send(direction)
       relation.
         left_joins(attribute.to_sym).
         group(:id).
-        reorder(Arel.sql(query))
+        reorder(query)
     end
 
     def order_by_belongs_to(relation)
@@ -92,15 +92,15 @@ def order_by_has_one(relation)
     def order_by_attribute(relation)
       relation.joins(
         attribute.to_sym,
-      ).reorder(Arel.sql(order_by_attribute_query))
+      ).reorder(order_by_attribute_query)
     end
 
     def order_by_id(relation)
-      relation.reorder(Arel.sql(order_by_id_query(relation)))
+      relation.reorder(order_by_id_query(relation))
     end
 
     def order_by_association_id(relation)
-      relation.reorder(Arel.sql(order_by_association_id_query))
+      relation.reorder(order_by_association_id_query)
     end
 
     def ordering_by_association_column?(relation)
@@ -115,15 +115,16 @@ def column_exist?(table, column_name)
     end
 
     def order_by_id_query(relation)
-      "#{relation.table_name}.#{foreign_key(relation)} #{direction}"
+      relation.arel_table[foreign_key(relation)].public_send(direction)
     end
 
     def order_by_association_id_query
-      "#{association_table_name}.id #{direction}"
+      Arel::Table.new(association_table_name)[:id].public_send(direction)
     end
 
     def order_by_attribute_query
-      "#{association_table_name}.#{association_attribute} #{direction}"
+      table = Arel::Table.new(association_table_name)
+      table[association_attribute].public_send(direction)
     end
 
     def relation_type(relation)

spec/lib/administrate/order_spec.rb

@@ -1,4 +1,4 @@
-require "active_record"
+require "rails_helper"
 require "administrate/order"
 
 describe Administrate::Order do
@@ -37,7 +37,9 @@
 
         ordered = order.apply(relation)
 
-        expect(relation).to have_received(:reorder).with("table_name.name asc")
+        expect(relation).to have_received(:reorder).with(
+          to_sql('"table_name"."name" ASC'),
+        )
         expect(ordered).to eq(relation)
       end
 
@@ -48,7 +50,9 @@
 
         ordered = order.apply(relation)
 
-        expect(relation).to have_received(:reorder).with("table_name.name desc")
+        expect(relation).to have_received(:reorder).with(
+          to_sql('"table_name"."name" DESC'),
+        )
         expect(ordered).to eq(relation)
       end
 
@@ -59,7 +63,9 @@
 
         ordered = order.apply(relation)
 
-        expect(relation).to have_received(:reorder).with("table_name.name asc")
+        expect(relation).to have_received(:reorder).with(
+          to_sql('"table_name"."name" ASC'),
+        )
         expect(ordered).to eq(relation)
       end
     end
@@ -69,7 +75,11 @@
         order = Administrate::Order.new(:name)
         relation = relation_with_association(
           :has_many,
-          klass: double(table_name: "users", primary_key: "uid"),
+          klass: double(
+            table_name: "users",
+            arel_table: Arel::Table.new("users"),
+            primary_key: "uid",
+          ),
         )
         allow(relation).to receive(:reorder).and_return(relation)
         allow(relation).to receive(:left_joins).and_return(relation)
@@ -79,7 +89,9 @@
 
         expect(relation).to have_received(:left_joins).with(:name)
         expect(relation).to have_received(:group).with(:id)
-        expect(relation).to have_received(:reorder).with("COUNT(users.uid) asc")
+        expect(relation).to have_received(:reorder).with(
+          to_sql('COUNT("users"."uid") ASC'),
+        )
         expect(ordered).to eq(relation)
       end
     end
@@ -96,7 +108,7 @@
         ordered = order.apply(relation)
 
         expect(relation).to have_received(:reorder).with(
-          "table_name.some_foreign_key asc",
+          to_sql('"table_name"."some_foreign_key" ASC'),
         )
         expect(ordered).to eq(relation)
       end
@@ -120,7 +132,7 @@
 
           ordered = order.apply(relation)
           expect(relation).to have_received(:reorder).with(
-            "users.name asc",
+            to_sql('"users"."name" ASC'),
           )
           expect(ordered).to eq(relation)
         end
@@ -146,7 +158,7 @@
           ordered = order.apply(relation)
 
           expect(relation).to have_received(:reorder).with(
-            "table_name.belongs_to_id asc",
+            to_sql('"table_name"."belongs_to_id" ASC'),
           )
           expect(ordered).to eq(relation)
         end
@@ -164,7 +176,7 @@
         ordered = order.apply(relation)
 
         expect(relation).to have_received(:reorder).with(
-          "users.id asc",
+          to_sql('"users"."id" ASC'),
         )
         expect(ordered).to eq(relation)
       end
@@ -188,7 +200,7 @@
 
           ordered = order.apply(relation)
           expect(relation).to have_received(:reorder).with(
-            "users.name asc",
+            to_sql('"users"."name" ASC'),
           )
           expect(ordered).to eq(relation)
         end
@@ -214,7 +226,7 @@
           ordered = order.apply(relation)
 
           expect(relation).to have_received(:reorder).with(
-            "users.id asc",
+            to_sql('"users"."id" ASC'),
           )
           expect(ordered).to eq(relation)
         end
@@ -311,6 +323,7 @@ def relation_with_column(column)
       klass: double(reflect_on_association: nil),
       columns_hash: { column.to_s => :column_info },
       table_name: "table_name",
+      arel_table: Arel::Table.new("table_name"),
     )
   end
 
@@ -329,6 +342,7 @@ def relation_with_association(
         ),
       ),
       table_name: "table_name",
+      arel_table: Arel::Table.new("table_name"),
     )
   end
 end

spec/support/matchers/to_sql.rb

@@ -0,0 +1,5 @@
+RSpec::Matchers.define :to_sql do |sql|
+  match do |actual|
+    actual.to_sql == sql
+  end
+end