3-0-8-security.diff

gistfile1.diff

From 13475b3186d8d4613e390907c1a233a8c012c253 Mon Sep 17 00:00:00 2001
From: Michael Koziarski <michael@koziarski.com>
Date: Mon, 16 May 2011 12:07:20 -0400
Subject: [PATCH 1/2] Ensure that the strings returned by SafeBuffer#gsub and friends aren't considered html_safe?

Also make sure that the versions of those methods which modify a string in place such as gsub! can't be called on safe buffers at all.
---
 .../core_ext/string/output_safety.rb               |   13 +++++++++++++
 activesupport/test/safe_buffer_test.rb             |   12 ++++++++++++
 2 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/activesupport/lib/active_support/core_ext/string/output_safety.rb b/activesupport/lib/active_support/core_ext/string/output_safety.rb
index f6c2402..15924ad 100644
--- a/activesupport/lib/active_support/core_ext/string/output_safety.rb
+++ b/activesupport/lib/active_support/core_ext/string/output_safety.rb
@@ -73,6 +73,7 @@ end
 
 module ActiveSupport #:nodoc:
   class SafeBuffer < String
+    UNSAFE_STRING_METHODS = ["capitalize", "chomp", "chop", "delete", "downcase", "gsub", "lstrip", "next", "reverse", "rstrip", "slice", "squeeze", "strip", "sub", "succ", "swapcase", "tr", "tr_s", "upcase"].freeze
     alias safe_concat concat
 
     def concat(value)
@@ -103,6 +104,18 @@ module ActiveSupport #:nodoc:
     def to_yaml(*args)
       to_str.to_yaml(*args)
     end
+
+    for unsafe_method in UNSAFE_STRING_METHODS
+      class_eval <<-EOT, __FILE__, __LINE__
+        def #{unsafe_method}(*args)
+          super.to_str
+        end
+
+        def #{unsafe_method}!(*args)
+          raise TypeError, "Cannot modify SafeBuffer in place"
+        end
+      EOT
+    end
   end
 end
 
diff --git a/activesupport/test/safe_buffer_test.rb b/activesupport/test/safe_buffer_test.rb
index bf61f9e..4371f35 100644
--- a/activesupport/test/safe_buffer_test.rb
+++ b/activesupport/test/safe_buffer_test.rb
@@ -38,4 +38,16 @@ class SafeBufferTest < ActiveSupport::TestCase
     new_buffer = @buffer.to_s
     assert_equal ActiveSupport::SafeBuffer, new_buffer.class
   end
+
+  test "Should not return safe buffer from gsub" do
+    altered_buffer = @buffer.gsub('', 'asdf')
+    assert_equal 'asdf', altered_buffer
+    assert !altered_buffer.html_safe?
+  end
+
+  test "Should not allow gsub! on safe buffers" do
+    assert_raise TypeError do
+      @buffer.gsub!('', 'asdf')
+    end
+  end
 end
-- 
1.7.2


From f55bf81e64371eaedc7a1044562f5d3b906f5976 Mon Sep 17 00:00:00 2001
From: Bruno Michel <bmichel@menfin.info>
Date: Wed, 25 May 2011 02:42:13 +0200
Subject: [PATCH 2/2] Do not modify a safe buffer in helpers

Signed-off-by: Michael Koziarski <michael@koziarski.com>
---
 actionpack/lib/action_view/helpers/text_helper.rb |   40 +++++++++------------
 actionpack/test/template/text_helper_test.rb      |   26 +++++++++----
 2 files changed, 35 insertions(+), 31 deletions(-)

diff --git a/actionpack/lib/action_view/helpers/text_helper.rb b/actionpack/lib/action_view/helpers/text_helper.rb
index 5e01306..c5ac1eb 100644
--- a/actionpack/lib/action_view/helpers/text_helper.rb
+++ b/actionpack/lib/action_view/helpers/text_helper.rb
@@ -115,13 +115,12 @@ module ActionView
         end
         options.reverse_merge!(:highlighter => '<strong class="highlight">\1</strong>')
 
-        text = sanitize(text) unless options[:sanitize] == false
-        if text.blank? || phrases.blank?
-          text
-        else
+        if text.present? && phrases.present?
           match = Array(phrases).map { |p| Regexp.escape(p) }.join('|')
-          text.gsub(/(#{match})(?!(?:[^<]*?)(?:["'])[^<>]*>)/i, options[:highlighter])
-        end.html_safe
+          text = text.to_str.gsub(/(#{match})(?!(?:[^<]*?)(?:["'])[^<>]*>)/i, options[:highlighter])
+        end
+        text = sanitize(text) unless options[:sanitize] == false
+        text
       end
 
       # Extracts an excerpt from +text+ that matches the first instance of +phrase+.
@@ -251,14 +250,16 @@ module ActionView
       #   simple_format("Look ma! A class!", :class => 'description')
       #   # => "<p class='description'>Look ma! A class!</p>"
       def simple_format(text, html_options={}, options={})
-        text = ''.html_safe if text.nil?
+        text = text ? text.to_str : ''
+        text = text.dup if text.frozen?
         start_tag = tag('p', html_options, true)
-        text = sanitize(text) unless options[:sanitize] == false
         text.gsub!(/\r\n?/, "\n")                    # \r\n and \r -> \n
         text.gsub!(/\n\n+/, "</p>\n\n#{start_tag}")  # 2+ newline  -> paragraph
         text.gsub!(/([^\n]\n)(?=[^\n])/, '\1<br />') # 1 newline   -> br
         text.insert 0, start_tag
-        text.html_safe.safe_concat("</p>")
+        text.concat("</p>")
+        text = sanitize(text) unless options[:sanitize] == false
+        text
       end
 
       # Turns all URLs and e-mail addresses into clickable links. The <tt>:link</tt> option
@@ -477,7 +478,7 @@ module ActionView
         # is yielded and the result is used as the link text.
         def auto_link_urls(text, html_options = {}, options = {})
           link_attributes = html_options.stringify_keys
-          text.gsub(AUTO_LINK_RE) do
+          text.to_str.gsub(AUTO_LINK_RE) do
             scheme, href = $1, $&
             punctuation = []
 
@@ -494,14 +495,11 @@ module ActionView
                 end
               end
 
-              link_text = block_given?? yield(href) : href
+              link_text = block_given? ? yield(href) : href
               href = 'http://' + href unless scheme
 
-              unless options[:sanitize] == false
-                link_text = sanitize(link_text)
-                href      = sanitize(href)
-              end
-              content_tag(:a, link_text, link_attributes.merge('href' => href), !!options[:sanitize]) + punctuation.reverse.join('')
+              sanitize = options[:sanitize] != false
+              content_tag(:a, link_text, link_attributes.merge('href' => href), sanitize) + punctuation.reverse.join('')
             end
           end
         end
@@ -509,18 +507,14 @@ module ActionView
         # Turns all email addresses into clickable links.  If a block is given,
         # each email is yielded and the result is used as the link text.
         def auto_link_email_addresses(text, html_options = {}, options = {})
-          text.gsub(AUTO_EMAIL_RE) do
+          text.to_str.gsub(AUTO_EMAIL_RE) do
             text = $&
 
             if auto_linked?($`, $')
               text.html_safe
             else
-              display_text = (block_given?) ? yield(text) : text
-
-              unless options[:sanitize] == false
-                text         = sanitize(text)
-                display_text = sanitize(display_text) unless text == display_text
-              end
+              display_text = block_given? ? yield(text) : text
+              display_text = sanitize(display_text) unless options[:sanitize] == false
               mail_to text, display_text, html_options
             end
           end
diff --git a/actionpack/test/template/text_helper_test.rb b/actionpack/test/template/text_helper_test.rb
index f0d99a0..9bc9cea 100644
--- a/actionpack/test/template/text_helper_test.rb
+++ b/actionpack/test/template/text_helper_test.rb
@@ -48,6 +48,10 @@ class TextHelperTest < ActionView::TestCase
     assert_equal "<p><b> test with unsafe string </b><script>code!</script></p>", simple_format("<b> test with unsafe string </b><script>code!</script>", {}, :sanitize => false)
   end
 
+  def test_simple_format_should_not_be_html_safe_when_sanitize_option_is_false
+    assert !simple_format("<b> test with unsafe string </b><script>code!</script>", {}, :sanitize => false).html_safe?
+  end
+
   def test_truncate_should_not_be_html_safe
     assert !truncate("Hello World!", :length => 12).html_safe?
   end
@@ -166,6 +170,13 @@ class TextHelperTest < ActionView::TestCase
     )
   end
 
+  def test_highlight_on_an_html_safe_string
+    assert_equal(
+      "<p>This is a <b>beautiful</b> morning, but also a <b>beautiful</b> day</p>",
+      highlight("<p>This is a beautiful morning, but also a beautiful day</p>".html_safe, "beautiful", :highlighter => '<b>\1</b>')
+    )
+  end
+
   def test_highlight_with_html
     assert_equal(
       "<p>This is a <strong class=\"highlight\">beautiful</strong> morning, but also a <strong class=\"highlight\">beautiful</strong> day</p>",
@@ -306,13 +317,10 @@ class TextHelperTest < ActionView::TestCase
     end
   end
 
-  def generate_result(link_text, href = nil, escape = false)
-    href ||= link_text
-    if escape
-      %{<a href="#{CGI::escapeHTML href}">#{CGI::escapeHTML link_text}</a>}
-    else
-      %{<a href="#{href}">#{link_text}</a>}
-    end
+  def generate_result(link_text, href = nil)
+    href = CGI::escapeHTML(href || link_text)
+    text = CGI::escapeHTML(link_text)
+    %{<a href="#{href}">#{text}</a>}
   end
 
   def test_auto_link_should_not_be_html_safe
@@ -323,6 +331,8 @@ class TextHelperTest < ActionView::TestCase
     assert !auto_link('').html_safe?, 'should not be html safe'
     assert !auto_link("#{link_raw} #{link_raw} #{link_raw}").html_safe?, 'should not be html safe'
     assert !auto_link("hello #{email_raw}").html_safe?, 'should not be html safe'
+    assert !auto_link(link_raw.html_safe).html_safe?, 'should not be html safe'
+    assert !auto_link(email_raw.html_safe).html_safe?, 'should not be html safe'
   end
 
   def test_auto_link_email_address
@@ -425,7 +435,7 @@ class TextHelperTest < ActionView::TestCase
 
   def test_auto_link_should_sanitize_input_when_sanitize_option_is_not_false
     link_raw     = %{http://www.rubyonrails.com?id=1&num=2}
-    assert_equal %{<a href="http://www.rubyonrails.com?id=1&num=2">http://www.rubyonrails.com?id=1&num=2</a>}, auto_link(link_raw)
+    assert_equal %{<a href="http://www.rubyonrails.com?id=1&amp;num=2">http://www.rubyonrails.com?id=1&amp;num=2</a>}, auto_link(link_raw)
   end
 
   def test_auto_link_should_not_sanitize_input_when_sanitize_option_is_false
-- 
1.7.2