Skip to content
This repository
Browse code

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.
  • Loading branch information...
commit 53a2c0baf2b128dd4808eca313256f6f4bb8c4cd 1 parent ce23c6e
Michael Koziarski authored May 16, 2011 tenderlove committed June 07, 2011
13  activesupport/lib/active_support/core_ext/string/output_safety.rb
@@ -73,6 +73,7 @@ def html_safe?
73 73
 
74 74
 module ActiveSupport #:nodoc:
75 75
   class SafeBuffer < String
  76
+    UNSAFE_STRING_METHODS = ["capitalize", "chomp", "chop", "delete", "downcase", "gsub", "lstrip", "next", "reverse", "rstrip", "slice", "squeeze", "strip", "sub", "succ", "swapcase", "tr", "tr_s", "upcase"].freeze
76 77
     alias safe_concat concat
77 78
 
78 79
     def concat(value)
@@ -103,6 +104,18 @@ def to_s
103 104
     def to_yaml(*args)
104 105
       to_str.to_yaml(*args)
105 106
     end
  107
+
  108
+    for unsafe_method in UNSAFE_STRING_METHODS
  109
+      class_eval <<-EOT, __FILE__, __LINE__
  110
+        def #{unsafe_method}(*args)
  111
+          super.to_str
  112
+        end
  113
+
  114
+        def #{unsafe_method}!(*args)
  115
+          raise TypeError, "Cannot modify SafeBuffer in place"
  116
+        end
  117
+      EOT
  118
+    end
106 119
   end
107 120
 end
108 121
 
12  activesupport/test/safe_buffer_test.rb
@@ -38,4 +38,16 @@ def setup
38 38
     new_buffer = @buffer.to_s
39 39
     assert_equal ActiveSupport::SafeBuffer, new_buffer.class
40 40
   end
  41
+
  42
+  test "Should not return safe buffer from gsub" do
  43
+    altered_buffer = @buffer.gsub('', 'asdf')
  44
+    assert_equal 'asdf', altered_buffer
  45
+    assert !altered_buffer.html_safe?
  46
+  end
  47
+
  48
+  test "Should not allow gsub! on safe buffers" do
  49
+    assert_raise TypeError do
  50
+      @buffer.gsub!('', 'asdf')
  51
+    end
  52
+  end
41 53
 end

7 notes on commit 53a2c0b

Steve Hoeksema

This breaks String#squish as defined in activesupport/lib/active_support/core_ext/string/filters.rb for ActiveRecord attributes - is that intentional?

Nathan Weizenbaum
nex3 commented on 53a2c0b June 07, 2011

This also breaks #gsub with a block, using the $1 etc. to access captures:

SafeBuffer.new("fe fo fa").gsub(/f(.)/) do
  "z" + $1
end

The above should yield "ze zo za", but with this change instead yields "z z z".

Wojciech Wnętrzak

This change breaks escape_javascript helper somehow. I will try to debug more and give some feedback.
I'm using this in ajax request with rendering template as haml, so there is lot of layers.

Wojciech Wnętrzak

There is a problem with using escape_javascript and rendering partials.
You can see the problem at https://github.com/morgoth/escape_javascript_bug
Output from escape_javascript(render("partial")) is completely different after this commit (wrong escaping).

Bruno Michel
nono commented on 53a2c0b June 08, 2011

@nex3 : afaik, it's not possible to support this on ruby (ruby 1.8 / 1.9). You have to call #to_str before doing #gsub.

Paul Gallagher

We're facing the same issue @morgoth and I've confirmed that the escape_javascript is due to the same breakage of gsub with a $1 matcher reported by @nex3 .. within the gsub block, $1 is nil and does not contain the match.
It's possible to fix escape_javascript by just changing the usage of $1 to instead use explicit block variable i.e. gsub(..){ .. $1 } to gsub(..){ |m| .. m } , but the root cause seems to be that after super.to_str, you no longer have the context for an implicit usage of $1.

I've tested on 3.0-stable and the problem still exists. There doesn't seem to be an issue ticket for this yet .. has anyone logged one?

Wojciech Wnętrzak

I opened issue for this: #1553

Please sign in to comment.
Something went wrong with that request. Please try again.