Merge pull request #6671 from mrbrdo/master

Bad Regexp exploits "fix" master branch

4 files changed, 41 insertions, 9 deletions

diff --git a/activemodel/CHANGELOG.md b/activemodel/CHANGELOG.md
index 5ee439fa3f..847ae7f237 100644
--- a/activemodel/CHANGELOG.md
+++ b/activemodel/CHANGELOG.md
@@ -37,6 +37,11 @@
 
 *   Trim down Active Model API by removing `valid?` and `errors.full_messages` *José Valim*
 
+*   When `^` or `$` are used in the regular expression provided to `validates_format_of` and the :multiline option is not set to true, an exception will be raised. This is to prevent security vulnerabilities when using `validates_format_of`. The problem is described in detail in the Rails security guide.
+
+## Rails 3.2.6 (Jun 12, 2012) ##
+
+*   No changes.
 
 ## Rails 3.2.5 (Jun 1, 2012) ##
 
diff --git a/activemodel/lib/active_model/validations/format.rb b/activemodel/lib/active_model/validations/format.rb
index dd87e312f9..ffdf842d94 100644
--- a/activemodel/lib/active_model/validations/format.rb
+++ b/activemodel/lib/active_model/validations/format.rb
@@ -32,11 +32,21 @@ module ActiveModel
       def record_error(record, attribute, name, value)
         record.errors.add(attribute, :invalid, options.except(name).merge!(:value => value))
       end
-
+      
+      def regexp_using_multiline_anchors?(regexp)
+        regexp.source.start_with?("^") ||
+          (regexp.source.end_with?("$") && !regexp.source.end_with?("\\$"))
+      end
+      
       def check_options_validity(options, name)
         option = options[name]
         if option && !option.is_a?(Regexp) && !option.respond_to?(:call)
           raise ArgumentError, "A regular expression or a proc or lambda must be supplied as :#{name}"
+        elsif option && option.is_a?(Regexp) &&
+              regexp_using_multiline_anchors?(option) && options[:multiline] != true
+          raise ArgumentError, "The provided regular expression is using multiline anchors (^ or $), " \
+          "which may present a security risk. Did you mean to use \\A and \\z, or forgot to add the " \
+          ":multiline => true option?"
         end
       end
     end
@@ -47,7 +57,7 @@ module ActiveModel
       # attribute matches the regular expression:
       #
       #   class Person < ActiveRecord::Base
-      #     validates_format_of :email, :with => /\A([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})\Z/i, :on => :create
+      #     validates_format_of :email, :with => /\A([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})\z/i, :on => :create
       #   end
       #
       # Alternatively, you can require that the specified attribute does _not_
@@ -63,12 +73,16 @@ module ActiveModel
       #   class Person < ActiveRecord::Base
       #     # Admin can have number as a first letter in their screen name
       #     validates_format_of :screen_name,
-      #                         :with => lambda{ |person| person.admin? ? /\A[a-z0-9][a-z0-9_\-]*\Z/i : /\A[a-z][a-z0-9_\-]*\Z/i }
+      #                         :with => lambda{ |person| person.admin? ? /\A[a-z0-9][a-z0-9_\-]*\z/i : /\A[a-z][a-z0-9_\-]*\z/i }
       #   end
       #
       # Note: use <tt>\A</tt> and <tt>\Z</tt> to match the start and end of the
       # string, <tt>^</tt> and <tt>$</tt> match the start/end of a line.
       #
+      # Due to frequent misuse of <tt>^</tt> and <tt>$</tt>, you need to pass the
+      # :multiline => true option in case you use any of these two anchors in the provided
+      # regular expression. In most cases, you should be using <tt>\A</tt> and <tt>\z</tt>.
+      #
       # You must pass either <tt>:with</tt> or <tt>:without</tt> as an option.
       # In addition, both must be a regular expression or a proc or lambda, or
       # else an exception will be raised.
@@ -98,6 +112,9 @@ module ActiveModel
       #   method, proc or string should return or evaluate to a true or false value.
       # * <tt>:strict</tt> - Specifies whether validation should be strict. 
       #   See <tt>ActiveModel::Validation#validates!</tt> for more information.
+      # * <tt>:multiline</tt> - Set to true if your regular expression contains
+      #   anchors that match the beginning or end of lines as opposed to the
+      #   beginning or end of the string. These anchors are <tt>^</tt> and <tt>$</tt>.
       def validates_format_of(*attr_names)
         validates_with FormatValidator, _merge_attributes(attr_names)
       end
diff --git a/activemodel/test/cases/validations/format_validation_test.rb b/activemodel/test/cases/validations/format_validation_test.rb
index 41a1131bcb..308a3c6cef 100644
--- a/activemodel/test/cases/validations/format_validation_test.rb
+++ b/activemodel/test/cases/validations/format_validation_test.rb
@@ -11,7 +11,7 @@ class PresenceValidationTest < ActiveModel::TestCase
   end
 
   def test_validate_format
-    Topic.validates_format_of(:title, :content, :with => /^Validation\smacros \w+!$/, :message => "is bad data")
+    Topic.validates_format_of(:title, :content, :with => /\AValidation\smacros \w+!\z/, :message => "is bad data")
 
     t = Topic.new("title" => "i'm incorrect", "content" => "Validation macros rule!")
     assert t.invalid?, "Shouldn't be valid"
@@ -27,7 +27,7 @@ class PresenceValidationTest < ActiveModel::TestCase
   end
 
   def test_validate_format_with_allow_blank
-    Topic.validates_format_of(:title, :with => /^Validation\smacros \w+!$/, :allow_blank => true)
+    Topic.validates_format_of(:title, :with => /\AValidation\smacros \w+!\z/, :allow_blank => true)
     assert Topic.new("title" => "Shouldn't be valid").invalid?
     assert Topic.new("title" => "").valid?
     assert Topic.new("title" => nil).valid?
@@ -36,7 +36,7 @@ class PresenceValidationTest < ActiveModel::TestCase
 
   # testing ticket #3142
   def test_validate_format_numeric
-    Topic.validates_format_of(:title, :content, :with => /^[1-9][0-9]*$/, :message => "is bad data")
+    Topic.validates_format_of(:title, :content, :with => /\A[1-9][0-9]*\z/, :message => "is bad data")
 
     t = Topic.new("title" => "72x", "content" => "6789")
     assert t.invalid?, "Shouldn't be valid"
@@ -63,11 +63,21 @@ class PresenceValidationTest < ActiveModel::TestCase
   end
 
   def test_validate_format_with_formatted_message
-    Topic.validates_format_of(:title, :with => /^Valid Title$/, :message => "can't be %{value}")
+    Topic.validates_format_of(:title, :with => /\AValid Title\z/, :message => "can't be %{value}")
     t = Topic.new(:title => 'Invalid title')
     assert t.invalid?
     assert_equal ["can't be Invalid title"], t.errors[:title]
   end
+  
+  def test_validate_format_of_with_multiline_regexp_should_raise_error
+    assert_raise(ArgumentError) { Topic.validates_format_of(:title, :with => /^Valid Title$/) }
+  end
+  
+  def test_validate_format_of_with_multiline_regexp_and_option
+    assert_nothing_raised(ArgumentError) do
+      Topic.validates_format_of(:title, :with => /^Valid Title$/, :multiline => true)
+    end
+  end
 
   def test_validate_format_with_not_option
     Topic.validates_format_of(:title, :without => /foo/, :message => "should not contain foo")
diff --git a/activemodel/test/cases/validations/i18n_validation_test.rb b/activemodel/test/cases/validations/i18n_validation_test.rb
index 6b6aad3bd1..bb751cf9c5 100644
--- a/activemodel/test/cases/validations/i18n_validation_test.rb
+++ b/activemodel/test/cases/validations/i18n_validation_test.rb
@@ -141,7 +141,7 @@ class I18nValidationTest < ActiveModel::TestCase
 
   COMMON_CASES.each do |name, validation_options, generate_message_options|
     test "validates_format_of on generated message #{name}" do
-      Person.validates_format_of :title, validation_options.merge(:with => /^[1-9][0-9]*$/)
+      Person.validates_format_of :title, validation_options.merge(:with => /\A[1-9][0-9]*\z/)
       @person.title = '72x'
       @person.errors.expects(:generate_message).with(:title, :invalid, generate_message_options.merge(:value => '72x'))
       @person.valid?
@@ -291,7 +291,7 @@ class I18nValidationTest < ActiveModel::TestCase
   # validates_format_of w/o mocha
 
   set_expectations_for_validation "validates_format_of", :invalid do |person, options_to_merge|
-    Person.validates_format_of :title, options_to_merge.merge(:with => /^[1-9][0-9]*$/)
+    Person.validates_format_of :title, options_to_merge.merge(:with => /\A[1-9][0-9]*\z/)
   end
 
   # validates_inclusion_of w/o mocha