From fd58410c5ea82d342720c8f3df0570a0729540cd Mon Sep 17 00:00:00 2001 From: Aaron J. Bedra Date: Sat, 31 May 2008 16:18:15 -0700 Subject: [PATCH] adding sql injection fixes for limit and offset --- .../abstract/database_statements.rb | 9 +++++++-- activerecord/test/cases/adapter_test.rb | 20 ++++++++++++++++++++ .../test/connections/native_mysql/connection.rb | 4 ++-- 3 files changed, 29 insertions(+), 4 deletions(-) diff --git a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb index 16d405d..5358491 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb @@ -106,11 +106,16 @@ module ActiveRecord # SELECT * FROM suppliers LIMIT 10 OFFSET 50 def add_limit_offset!(sql, options) if limit = options[:limit] - sql << " LIMIT #{limit}" + sql << " LIMIT #{sanitize_limit(limit)}" if offset = options[:offset] - sql << " OFFSET #{offset}" + sql << " OFFSET #{offset.to_i}" end end + sql + end + + def sanitize_limit(limit) + limit.to_s[/,/] ? limit.split(',').map{ |i| i.to_i }.join(',') : limit.to_i end # Appends a locking clause to an SQL statement. diff --git a/activerecord/test/cases/adapter_test.rb b/activerecord/test/cases/adapter_test.rb index 91504af..c77446f 100644 --- a/activerecord/test/cases/adapter_test.rb +++ b/activerecord/test/cases/adapter_test.rb @@ -104,4 +104,24 @@ class AdapterTest < ActiveRecord::TestCase end end + def test_add_limit_offset_should_sanitize_sql_injection_for_limit_without_comas + sql_inject = "1 select * from schema" + assert_equal " LIMIT 1", @connection.add_limit_offset!("", :limit=>sql_inject) + if current_adapter?(:MysqlAdapter) + assert_equal " LIMIT 7, 1", @connection.add_limit_offset!("", :limit=>sql_inject, :offset=>7) + else + assert_equal " LIMIT 1 OFFSET 7", @connection.add_limit_offset!("", :limit=>sql_inject, :offset=>7) + end + end + + def test_add_limit_offset_should_sanitize_sql_injection_for_limit_with_comas + sql_inject = "1, 7 procedure help()" + if current_adapter?(:MysqlAdapter) + assert_equal " LIMIT 1,7", @connection.add_limit_offset!("", :limit=>sql_inject) + assert_equal " LIMIT 7, 1", @connection.add_limit_offset!("", :limit=>sql_inject, :offset=>7) + else + assert_equal " LIMIT 1,7", @connection.add_limit_offset!("", :limit=>sql_inject) + assert_equal " LIMIT 1,7 OFFSET 7", @connection.add_limit_offset!("", :limit=>sql_inject, :offset=>7) + end + end end diff --git a/activerecord/test/connections/native_mysql/connection.rb b/activerecord/test/connections/native_mysql/connection.rb index 1fab444..3ee031f 100644 --- a/activerecord/test/connections/native_mysql/connection.rb +++ b/activerecord/test/connections/native_mysql/connection.rb @@ -12,13 +12,13 @@ ActiveRecord::Base.logger = RAILS_DEFAULT_LOGGER ActiveRecord::Base.configurations = { 'arunit' => { :adapter => 'mysql', - :username => 'rails', + :username => 'root', :encoding => 'utf8', :database => 'activerecord_unittest', }, 'arunit2' => { :adapter => 'mysql', - :username => 'rails', + :username => 'root', :database => 'activerecord_unittest2' } } -- 1.5.4.4