Merge pull request rails#47344 from ggmichaelgo/main

maintain html_safe? on sliced HTML safe strings

Author: rafaelfranca

activesupport/CHANGELOG.md

@@ -1,3 +1,19 @@
+*   Maintain `html_safe?` on html_safe strings when sliced with `slice`, `slice!`, or `chr` method.
+
+    Previously, `html_safe?` was only maintained when the html_safe strings were sliced
+    with `[]` method. Now, `slice`, `slice!`, and `chr` methods will maintain `html_safe?` like `[]` method.
+
+    ```ruby
+    string = "<div>test</div>".html_safe
+    string.slice(0, 1).html_safe? # => true
+    string.slice!(0, 1).html_safe? # => true
+    # maintain html_safe? after the slice!
+    string.html_safe? # => true
+    string.chr # => true
+    ```
+
+    *Michael Go*
+
 *   `config.i18n.raise_on_missing_translations = true` now raises on any missing translation.
 
     Previously it would only raise when called in a view or controller. Now it will raise

activesupport/lib/active_support/core_ext/string/output_safety.rb

@@ -19,7 +19,7 @@ module ActiveSupport # :nodoc:
   class SafeBuffer < String
     UNSAFE_STRING_METHODS = %w(
       capitalize chomp chop delete delete_prefix delete_suffix
-      downcase lstrip next reverse rstrip scrub slice squeeze strip
+      downcase lstrip next reverse rstrip scrub squeeze strip
       succ swapcase tr tr_s unicode_normalize upcase
     )
 
@@ -41,13 +41,26 @@ def [](*args)
 
         return unless new_string
 
-        new_safe_buffer = new_string.is_a?(SafeBuffer) ? new_string : SafeBuffer.new(new_string)
-        new_safe_buffer.instance_variable_set :@html_safe, true
-        new_safe_buffer
+        string_into_safe_buffer(new_string, true)
       else
         to_str[*args]
       end
     end
+    alias_method :slice, :[]
+
+    def slice!(*args)
+      new_string = super
+
+      return new_string if !html_safe? || new_string.nil?
+
+      string_into_safe_buffer(new_string, true)
+    end
+
+    def chr
+      return super unless html_safe?
+
+      string_into_safe_buffer(super, true)
+    end
 
     def safe_concat(value)
       raise SafeConcatError unless html_safe?
@@ -209,6 +222,12 @@ def set_block_back_references(block, match_data)
       rescue ArgumentError
         # Can't create binding from C level Proc
       end
+
+      def string_into_safe_buffer(new_string, is_html_safe)
+        new_safe_buffer = new_string.is_a?(SafeBuffer) ? new_string : SafeBuffer.new(new_string)
+        new_safe_buffer.instance_variable_set :@html_safe, is_html_safe
+        new_safe_buffer
+      end
   end
 end
 

activesupport/test/safe_buffer_test.rb

@@ -90,7 +90,6 @@ def test_titleize
     reverse: nil,
     rstrip: nil,
     scrub: nil,
-    slice: "foo",
     squeeze: nil,
     strip: nil,
     sub: ["foo", "bar"],
@@ -212,28 +211,59 @@ def test_titleize
   end
 
   test "Should continue unsafe on slice" do
-    x = "foo".html_safe.gsub!("f", '<script>alert("lolpwnd");</script>')
+    safe_string = "foo".html_safe.gsub!("f", '<script>alert("lolpwnd");</script>')
 
     # calling gsub! makes the dirty flag true
-    assert_not x.html_safe?, "should not be safe"
-
-    # getting a slice of it
-    y = x[0..-1]
+    assert_not safe_string.html_safe?, "should not be safe"
 
     # should still be unsafe
-    assert_not y.html_safe?, "should not be safe"
+    assert_not safe_string[0..-1].html_safe?, "should not be safe"
+    assert_not safe_string.slice(0..-1).html_safe?, "should not be safe"
+    assert_not safe_string.slice!(0..-1).html_safe?, "should not be safe"
+    # even after slice! safe_string is still unsafe
+    assert_not safe_string.html_safe?, "should not be safe"
   end
 
   test "Should continue safe on slice" do
-    x = "<div>foo</div>".html_safe
-
-    assert_predicate x, :html_safe?
+    safe_string = "<div>foo</div>".html_safe
 
-    # getting a slice of it
-    y = x[0..-1]
+    assert_predicate safe_string, :html_safe?
 
     # should still be safe
-    assert_predicate y, :html_safe?
+    assert_predicate safe_string[0..-1], :html_safe?
+    assert_predicate safe_string.slice(0..-1), :html_safe?
+    assert_predicate safe_string.slice!(0...1), :html_safe?
+
+    # even after slice! safe_string is still safe
+    assert_predicate safe_string, :html_safe?
+  end
+
+  test "Should continue safe on chr" do
+    safe_string = "<div>foo</div>".html_safe
+
+    assert_predicate safe_string, :html_safe?
+    assert_predicate safe_string.chr, :html_safe?
+  end
+
+  test "Should continue unsafe on chr" do
+    safe_string = "<div>foo</div>"
+
+    assert_not safe_string.html_safe?, "should not be safe"
+    assert_not safe_string.chr.html_safe?, "should not be safe"
+  end
+
+  test "Should return a SafeBuffer on slice! if original value was safe" do
+    safe_string = "<div>foo</div>".html_safe
+
+    assert safe_string.slice!(0...1).is_a?(ActiveSupport::SafeBuffer)
+  end
+
+  test "Should return a String on slice! if original value was not safe" do
+    unsafe_string = +'<script>alert("XSS");</script>'
+
+    sliced_string = unsafe_string.slice!(0...1)
+    assert_not sliced_string.is_a?(ActiveSupport::SafeBuffer)
+    assert sliced_string.is_a?(String)
   end
 
   test "Should work with interpolation (array argument)" do