Merge pull request rails#42123 from Shopify/safe-buffer-implicit-casting

Stop implicitly coercing objects to string in Safebuffer

Author: byroot

actionview/lib/action_view/helpers/form_helper.rb

@@ -2610,7 +2610,9 @@ def fields_for_with_nested_attributes(association_name, association, options, bl
               else
                 options[:child_index] = nested_child_index(name)
               end
-              output << fields_for_nested_model("#{name}[#{options[:child_index]}]", child, options, block)
+              if content = fields_for_nested_model("#{name}[#{options[:child_index]}]", child, options, block)
+                output << content
+              end
             end
             output
           elsif association

actionview/lib/action_view/helpers/form_tag_helper.rb

@@ -923,7 +923,7 @@ def form_tag_html(html_options)
 
         def form_tag_with_body(html_options, content)
           output = form_tag_html(html_options)
-          output << content
+          output << content.to_s if content
           output.safe_concat("</form>")
         end
 

activesupport/CHANGELOG.md

@@ -1,3 +1,13 @@
+*   Deprecate `ActiveSupport::SafeBuffer`'s incorrect implicit conversion of objects into string.
+
+    Except for a few methods like `String#%`, objects must implement `#to_str`
+    to be implictly converted to a String in string operations. In some
+    circumstances `ActiveSupport::SafeBuffer` was incorrectly calling the
+    explicit conversion method (`#to_s`) on them. This behavior is now
+    deprecated.
+
+    *Jean Boussier*
+
 *   Allow nested access to keys on `Rails.application.credentials`
 
     Previously only top level keys in `credentials.yml.enc` could be accessed with method calls. Now any key can.

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

@@ -184,27 +184,27 @@ def clone_empty
     end
 
     def concat(value)
-      super(html_escape_interpolated_argument(value))
+      super(implicit_html_escape_interpolated_argument(value))
     end
     alias << concat
 
     def insert(index, value)
-      super(index, html_escape_interpolated_argument(value))
+      super(index, implicit_html_escape_interpolated_argument(value))
     end
 
     def prepend(value)
-      super(html_escape_interpolated_argument(value))
+      super(implicit_html_escape_interpolated_argument(value))
     end
 
     def replace(value)
-      super(html_escape_interpolated_argument(value))
+      super(implicit_html_escape_interpolated_argument(value))
     end
 
     def []=(*args)
       if args.length == 3
-        super(args[0], args[1], html_escape_interpolated_argument(args[2]))
+        super(args[0], args[1], implicit_html_escape_interpolated_argument(args[2]))
       else
-        super(args[0], html_escape_interpolated_argument(args[1]))
+        super(args[0], implicit_html_escape_interpolated_argument(args[1]))
       end
     end
 
@@ -222,9 +222,9 @@ def *(*)
     def %(args)
       case args
       when Hash
-        escaped_args = args.transform_values { |arg| html_escape_interpolated_argument(arg) }
+        escaped_args = args.transform_values { |arg| explicit_html_escape_interpolated_argument(arg) }
       else
-        escaped_args = Array(args).map { |arg| html_escape_interpolated_argument(arg) }
+        escaped_args = Array(args).map { |arg| explicit_html_escape_interpolated_argument(arg) }
       end
 
       self.class.new(super(escaped_args))
@@ -289,10 +289,33 @@ def #{unsafe_method}!(*args, &block)            # def gsub!(*args, &block)
     end
 
     private
-      def html_escape_interpolated_argument(arg)
+      def explicit_html_escape_interpolated_argument(arg)
         (!html_safe? || arg.html_safe?) ? arg : CGI.escapeHTML(arg.to_s)
       end
 
+      def implicit_html_escape_interpolated_argument(arg)
+        if !html_safe? || arg.html_safe?
+          arg
+        else
+          arg_string = begin
+            arg.to_str
+          rescue NoMethodError => error
+            if error.name == :to_str
+              str = arg.to_s
+              ActiveSupport::Deprecation.warn <<~MSG.squish
+                Implicit conversion of #{arg.class} into String by ActiveSupport::SafeBuffer
+                is deprecated and will be removed in Rails 7.1.
+                You must explicitly cast it to a String.
+              MSG
+              str
+            else
+              raise
+            end
+          end
+          CGI.escapeHTML(arg_string)
+        end
+      end
+
       def set_block_back_references(block, match_data)
         block.binding.eval("proc { |m| $~ = m }").call(match_data)
       rescue ArgumentError

activesupport/test/core_ext/string_ext_test.rb

@@ -786,10 +786,15 @@ class OutputSafetyTest < ActiveSupport::TestCase
   def setup
     @string = +"hello"
     @object = Class.new(Object) do
-      def to_s
+      def to_str
         "other"
       end
     end.new
+    @to_s_object = Class.new(Object) do
+      def to_s
+        "to_s"
+      end
+    end.new
   end
 
   test "A string is unsafe by default" do
@@ -817,6 +822,14 @@ def to_s
     assert_not_predicate @object, :html_safe?
   end
 
+  test "Adding an object not responding to `#to_str` to a safe string is deprecated" do
+    string = @string.html_safe
+    assert_deprecated("Implicit conversion of #{@to_s_object.class} into String by ActiveSupport::SafeBuffer is deprecated") do
+      string << @to_s_object
+    end
+    assert_equal "helloto_s", string
+  end
+
   test "Adding an object to a safe string returns a safe string" do
     string = @string.html_safe
     string << @object
@@ -913,6 +926,11 @@ def to_s
     assert_not_predicate @other_string, :html_safe?
   end
 
+  test "% method explicitly cast the argument to string" do
+    @other_string = "other%s"
+    assert_equal "otherto_s", @other_string % @to_s_object
+  end
+
   test "Concatting unsafe onto safe with % yields escaped safe" do
     @other_string = "other%s".html_safe
     string = @other_string % "<foo>"