Added bind-style variable interpolation for the condition arrays that uses the adapter's quote method [Michael Koziarski]

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@56 5ecf4fe2-1ee6-0310-87b1-e25e094e27de

activerecord/CHANGELOG

@@ -1,5 +1,15 @@
 *CVS*
 
+* Added bind-style variable interpolation for the condition arrays that uses the adapter's quote method [Michael Koziarski]
+
+  Before:
+    find_first([ "user_name = '%s' AND password = '%s'", user_name, password ])]
+    find_first([ "firm_id = %s", firm_id ])] # unsafe!
+
+  After:
+    find_first([ "user_name = ? AND password = ?", user_name, password ])]
+    find_first([ "firm_id = ?", firm_id ])]
+
 * Added CSV format for fixtures #272 [what-a-day]. (See the new and expanded documentation on fixtures for more information)
 
 * Fixed fixtures using primary key fields called something else than "id" [dave]

activerecord/lib/active_record/base.rb

@@ -67,7 +67,7 @@ module ActiveRecord #:nodoc:
   #     end
   # 
   #     def self.authenticate_safely(user_name, password)
-  #       find_first([ "user_name = '%s' AND password = '%s'", user_name, password ])
+  #       find_first([ "user_name = ? AND password = ?", user_name, password ])
   #     end
   #   end
   # 
@@ -76,10 +76,6 @@ module ActiveRecord #:nodoc:
   # on the other hand, will sanitize the <tt>user_name</tt> and +password+ before inserting them in the query, which will ensure that
   # an attacker can't escape the query and fake the login (or worse).
   #
-  # Beware, that the approach used in <tt>authenticate_unsafely</tt> is basically just a wrapped call to sprintf. This means that you
-  # still have to quote when using %s or use %d instead. So find_first([ "firm_id = %s", firm_id ]) is _not_ safe while both
-  # find_first([ "firm_id = '%s'", firm_id ]) and find_first([ "firm_id = %d", firm_id ]) are.
-  # 
   # == Overwriting default accessors
   # 
   # All column values are automatically available through basic accessors on the Active Record object, but some times you
@@ -636,13 +632,21 @@ module ActiveRecord #:nodoc:
         # Accepts either a condition array or string. The string is returned untouched, but the array has each of
         # the condition values sanitized.
         def sanitize_conditions(conditions)
-          if Array === conditions
+          return conditions unless conditions.is_a?(Array)
+
           statement, values = conditions[0], conditions[1..-1]
-            values.collect! { |value| sanitize(value) }
+
-            conditions = statement % values
+          statement =~ /\?/ ?
+            replace_bind_variables(statement, values) :
+            statement % values.collect { |value| sanitize(value) }
         end
 
-          return conditions
+        def replace_bind_variables(statement, values)
+          while statement =~ /\?/
+            statement.sub!(/\?/, connection.quote(values.shift))
+          end
+	
+  		    return statement
         end
     end
 

activerecord/test/finder_test.rb

@@ -60,6 +60,13 @@ class FinderTest < Test::Unit::TestCase
     assert_kind_of Time, Topic.find_first(["id = %d", 1]).written_on
   end
 
+  def test_bind_variables
+    assert_kind_of Firm, Company.find_first(["name = ?", "37signals"])
+    assert_nil Company.find_first(["name = ?", "37signals!"])
+    assert_nil Company.find_first(["name = ?", "37signals!' OR 1=1"])
+    assert_kind_of Time, Topic.find_first(["id = ?", 1]).written_on
+  end
+	
   def test_string_sanitation
     assert_equal "something '' 1=1", ActiveRecord::Base.sanitize("something ' 1=1")
     assert_equal "something select table", ActiveRecord::Base.sanitize("something; select table")