From 147f207a57a03fc7a52040aa1f6878cf70ee0db7 Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Sun, 2 May 2021 11:58:54 +0200 Subject: [PATCH] Deprecate implicitly coercing objects to string in ActiveSupport::SafeBuffer --- .../lib/action_view/helpers/form_helper.rb | 4 +- .../action_view/helpers/form_tag_helper.rb | 2 +- activesupport/CHANGELOG.md | 10 +++++ .../core_ext/string/output_safety.rb | 41 +++++++++++++++---- .../test/core_ext/string_ext_test.rb | 20 ++++++++- 5 files changed, 65 insertions(+), 12 deletions(-) diff --git a/actionview/lib/action_view/helpers/form_helper.rb b/actionview/lib/action_view/helpers/form_helper.rb index 3c4fdeade2..38a4e257d4 100644 --- a/actionview/lib/action_view/helpers/form_helper.rb +++ b/actionview/lib/action_view/helpers/form_helper.rb @@ -2610,7 +2610,9 @@ module ActionView 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 diff --git a/actionview/lib/action_view/helpers/form_tag_helper.rb b/actionview/lib/action_view/helpers/form_tag_helper.rb index a4b91671cd..94c7bccfdb 100644 --- a/actionview/lib/action_view/helpers/form_tag_helper.rb +++ b/actionview/lib/action_view/helpers/form_tag_helper.rb @@ -923,7 +923,7 @@ module ActionView 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("") end diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md index b9ad7f2344..0bd3d019f8 100644 --- a/activesupport/CHANGELOG.md +++ b/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. 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 c9e6735cb9..3e023e3c07 100644 --- a/activesupport/lib/active_support/core_ext/string/output_safety.rb +++ b/activesupport/lib/active_support/core_ext/string/output_safety.rb @@ -184,27 +184,27 @@ module ActiveSupport #:nodoc: 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 @@ module ActiveSupport #:nodoc: 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 @@ module ActiveSupport #:nodoc: 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 diff --git a/activesupport/test/core_ext/string_ext_test.rb b/activesupport/test/core_ext/string_ext_test.rb index aef045e85e..efbe7fb607 100644 --- a/activesupport/test/core_ext/string_ext_test.rb +++ b/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 @@ class OutputSafetyTest < ActiveSupport::TestCase 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 @@ class OutputSafetyTest < ActiveSupport::TestCase 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 % ""