Adding tests and code to ensure that adapter is not vulnerable to limit and offset SQL injection.

Author: snowblink

lib/active_record/connection_adapters/sqlserver_adapter.rb

@@ -386,12 +386,32 @@ def quote_column_name(name)
       end
 
       def add_limit_offset!(sql, options)
+        if options[:offset]
+          raise ArgumentError, "offset should have a limit" unless options[:limit]
+          unless options[:offset].kind_of?Integer
+            if options[:offset] =~ /^\d+$/
+              options[:offset] = options[:offset].to_i 
+            else
+              raise ArgumentError, "offset should be an integer"
+            end
+          end
+        end
+
+        unless options[:limit].kind_of?Integer
+          # is it just a string which should be an integer?
+          if options[:limit] =~ /^\d+$/
+            options[:limit] = options[:limit].to_i 
+          else
+            raise ArgumentError, "limit should be an integer"
+          end
+        end
+
         if options[:limit] and options[:offset]
           total_rows = @connection.select_all("SELECT count(*) as TotalRows from (#{sql.gsub(/\bSELECT(\s+DISTINCT)?\b/i, "SELECT#{$1} TOP 1000000000")}) tally")[0][:TotalRows].to_i
           if (options[:limit] + options[:offset]) >= total_rows
             options[:limit] = (total_rows - options[:offset] >= 0) ? (total_rows - options[:offset]) : 0
           end
-          sql.sub!(/^\s*SELECT(\s+DISTINCT)?/i, "SELECT * FROM (SELECT TOP #{options[:limit]} * FROM (SELECT#{$1} TOP #{options[:limit] + options[:offset]} ")
+          sql.sub!(/^\s*SELECT(\s+DISTINCT)?/i, "SELECT * FROM (SELECT TOP #{options[:limit]} * FROM (SELECT#{$1} TOP #{options[:limit] + options[:offset]}")
           sql << ") AS tmp1"
           if options[:order]
             order = options[:order].split(',').map do |field|
@@ -410,7 +430,7 @@ def add_limit_offset!(sql, options)
             end.join(', ')
             sql << " ORDER BY #{change_order_direction(order)}) AS tmp2 ORDER BY #{order}"
           else
-            sql << " ) AS tmp2"
+            sql << ") AS tmp2"
           end
         elsif sql !~ /^\s*SELECT (@@|COUNT\()/i
           sql.sub!(/^\s*SELECT(\s+DISTINCT)?/i) do

test/cases/aaaa_create_tables_test_sqlserver.rb

@@ -5,7 +5,7 @@ class AAAACreateTablesTestSqlserver < ActiveRecord::TestCase
   self.use_transactional_fixtures = false
 
   def setup
-    @ar_path = "#{File.dirname(__FILE__)}/../schema"
+    @ar_path = File.expand_path(File.join(File.dirname(__FILE__), '..', '..', '..', '..', '..', 'rails/activerecord/test/schema'))
     @base_path = "#{File.dirname(__FILE__)}/../fixtures/db_definitions"
   end
 

test/cases/adapter_test_sqlserver.rb

@@ -1,4 +1,4 @@
-require "cases/helper"
+require 'cases/helper'
 require 'models/default'
 require 'models/post'
 require 'models/task'

test/cases/offset_and_limit_test.rb

@@ -0,0 +1,75 @@
+require 'cases/helper'
+# require File.dirname(__FILE__) + '/../../lib/shoulda'
+require 'rubygems'
+require 'shoulda'
+
+class OffsetAndLimitTest < ActiveRecord::TestCase
+  def setup
+    @connection = ActiveRecord::Base.connection
+  end
+
+  context "selecting with limit" do
+    setup do
+      @select_sql = 'SELECT * FROM schema'
+    end
+
+    should "alter SQL to limit number of records returned" do
+      options = { :limit => 10 }
+      assert_equal('SELECT TOP 10 * FROM schema', @connection.add_limit_offset!(@select_sql, options))
+    end
+    
+    should "only allow integers for limit" do
+      options = { :limit => 'ten' }
+      assert_raise(ArgumentError) {@connection.add_limit_offset!(@select_sql, options) }
+    end
+
+    should "convert strings which look like integers to integers" do
+      options = { :limit => '42' }
+      assert_nothing_raised(ArgumentError) {@connection.add_limit_offset!(@select_sql, options)}
+    end
+
+    should "not allow sql injection via limit" do
+      options = { :limit => '1 * FROM schema; DELETE * FROM table; SELECT TOP 10 *'}
+      assert_raise(ArgumentError) { @connection.add_limit_offset!(@select_sql, options) }
+    end
+  end
+  
+  context "selecting with limit and offset" do
+    setup do
+      # we have to use a real table as we need the counts
+      @select_sql = 'SELECT * FROM accounts'
+      class Account < ActiveRecord::Base; end
+      
+      # create 10 Accounts
+      (1..10).each {|i| Account.create!}
+    end
+    
+    should "have limit if offset is passed" do
+      options = { :offset => 1 }
+      assert_raise(ArgumentError) { @connection.add_limit_offset!(@select_sql, options) }
+    end
+    
+    should "only allow integers for offset" do
+      options = { :limit => 10, :offset => 'five' }
+      assert_raise(ArgumentError) { @connection.add_limit_offset!(@select_sql, options)}
+    end
+    
+    should "convert strings which look like integers to integers" do
+      options = { :limit => 10, :offset => '5' }
+      assert_nothing_raised(ArgumentError) {@connection.add_limit_offset!(@select_sql, options)}
+    end
+    
+    should "alter SQL to limit number of records returned offset by specified amount" do
+      options = { :limit => 3, :offset => 5 }
+      expected_sql = %&SELECT * FROM (SELECT TOP 3 * FROM (SELECT TOP 8 * FROM accounts) AS tmp1) AS tmp2&
+      assert_equal(expected_sql, @connection.add_limit_offset!(@select_sql, options))
+    end
+
+    # Not really sure what an offset sql injection might look like
+    should "not allow sql injection via offset" do
+      options = { :limit => 10, :offset => '1 * FROM schema; DELETE * FROM table; SELECT TOP 10 *'}
+      assert_raise(ArgumentError) { @connection.add_limit_offset!(@select_sql, options) }
+    end
+  end
+
+end