Order/reorder changed in Rails 6.1

lib/brakeman/checks/check_sql.rb

@@ -28,11 +28,11 @@ def run_check
 
       @sql_targets.delete :delete_all
       @sql_targets.delete :destroy_all
-      @sql_targets.delete :order
-      @sql_targets.delete :reorder
     end
 
     if version_between?("6.1.0", "9.9.9")
+      @sql_targets.delete :order
+      @sql_targets.delete :reorder
       @sql_targets.delete :pluck
     end
 

test/apps/rails6/app/controllers/groups_controller.rb

@@ -66,5 +66,7 @@ def test_rails6_sqli
     User.select("stuff").reselect(params[:columns])
     User.where("x = 1").rewhere("x = #{params[:x]}")
     User.pluck(params[:column]) # Warn in 6.0, not in 6.1
+    User.order("name #{params[:direction]}") # Warn in 6.0, not in 6.1
+    User.order(:name).reorder(params[:column]) # Warn in 6.0, not in 6.1
   end
 end

test/tests/github_output.rb

@@ -6,7 +6,7 @@ def setup
   end
 
   def test_report_format
-    assert_equal 38, @@report.lines.count, "Did you add vulnerabilities to the Rails 6 app? Update this test please!"
+    assert_equal 40, @@report.lines.count, "Did you add vulnerabilities to the Rails 6 app? Update this test please!"
     @@report.lines.each do |line|
       assert line.start_with?('::'), 'Every line must start with `::`'
       assert_equal 2, line.scan('::').count, 'Every line must have exactly 2 `::`'

test/tests/rails6.rb

@@ -13,7 +13,7 @@ def expected
       :controller => 0,
       :model => 0,
       :template => 4,
-      :generic => 32
+      :generic => 34
     }
   end
 
@@ -213,6 +213,7 @@ def test_sql_injection_reselect
   end
 
   def test_sql_injection_pluck
+    # Not in Rails 6.1 though
     assert_warning :type => :warning,
       :warning_code => 0,
       :fingerprint => "69a7e516b2b409dc8d74f6a26b44d62f4b842ce9c73e96c3910f9206c6fc50f5",
@@ -225,6 +226,34 @@ def test_sql_injection_pluck
       :user_input => s(:call, s(:params), :[], s(:lit, :column))
   end
 
+  def test_sql_injection_order
+    # Not in Rails 6.1 though
+    assert_warning :type => :warning,
+      :warning_code => 0,
+      :fingerprint => "47e9c6316ae9b2937121298ebc095bac4c4c8682779a0be95ce32c3fc4ba3118",
+      :warning_type => "SQL Injection",
+      :line => 69,
+      :message => /^Possible\ SQL\ injection/,
+      :confidence => 0,
+      :relative_path => "app/controllers/groups_controller.rb",
+      :code => s(:call, s(:const, :User), :order, s(:dstr, "name ", s(:evstr, s(:call, s(:params), :[], s(:lit, :direction))))),
+      :user_input => s(:call, s(:params), :[], s(:lit, :direction))
+  end
+
+  def test_sql_injection_reorder
+    # Not in Rails 6.1 though
+    assert_warning :type => :warning,
+      :warning_code => 0,
+      :fingerprint => "c6b303b67a5de261d9faaa84e02b29987b57fb443691d7ad77956bbecf41a1d0",
+      :warning_type => "SQL Injection",
+      :line => 70,
+      :message => /^Possible\ SQL\ injection/,
+      :confidence => 0,
+      :relative_path => "app/controllers/groups_controller.rb",
+      :code => s(:call, s(:call, s(:const, :User), :order, s(:lit, :name)), :reorder, s(:call, s(:params), :[], s(:lit, :column))),
+      :user_input => s(:call, s(:params), :[], s(:lit, :column))
+  end
+
   def test_cross_site_scripting_sanity
     assert_warning :type => :template,
       :warning_code => 2,

test/tests/pluck.rb → test/tests/rails_61_sql.rb

@@ -1,17 +1,17 @@
 require_relative '../test'
 require 'brakeman/rescanner'
 
-class PluckTests < Minitest::Test
+class Rails61SQLTests < Minitest::Test
   include BrakemanTester::RescanTestHelper
 
-  # Test that `pluck` no longer warns about SQL injection
+  # Test that `pluck`/`order`/`reorder` no longer warn about SQL injection
   # in Rails 6.1
   def test_pluck_safe_in_rails_6_1
     before_rescan_of ['app/controllers/groups_controller.rb', 'Gemfile'], 'rails6' do
       replace "Gemfile", "gem 'rails', '~> 6.0.0.beta2'", "gem 'rails', '6.1.0'"
     end
 
     assert_equal '6.1.0', @rescanner.tracker.config.rails_version
-    assert_fixed 1
+    assert_fixed 3
   end
 end