Fixes double escaping of HTML-safe strings by the truncate command #13730

Open
wants to merge 3 commits into from

3 participants

@pallan

Resolves #13721

@dmathieu dmathieu and 1 other commented on an outdated diff Jan 16, 2014
actionview/lib/action_view/helpers/text_helper.rb
content = text.truncate(length, options)
+
+ options[:escape] = false if text.html_safe? && !content.html_safe? && options[:escape] != true

Why are you checking !content.html_safe?

@pallan
pallan added a note Jan 16, 2014

If String#truncate does modify text then it returns a new string instance which is not considered HTML-safe. I want the method to consider the content html_safe if the original already was html_safe so it doesn't double escape it unless told to explicitly

You should add a test for it then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pallan pallan commented on the diff Jan 16, 2014
actionview/test/template/text_helper_test.rb
@@ -129,6 +129,18 @@ def test_truncate_should_escape_the_input
assert_equal "Hello &lt;sc...", truncate("Hello <script>code!</script>World!!", :length => 12)
end
+ def test_truncate_should_always_escape_if_true
+ assert_equal "Hello &amp;gt; W...", truncate("Hello &gt; World!", :length => 15, :escape => true)
+ end
+
+ def test_truncate_should_not_escape_a_html_safe_input
@pallan
pallan added a note Jan 16, 2014

@dmathieu this is the test that is supposed to check for that. I also added an additional test for the inverse behaviour when the supplied input is html_safe and not truncated.

Yes, you're testing that a text which is safe and a content which is not safe will not be escaped.
What you're not testing is that a text which is safe and a content which is also safe will be escaped.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pallan

As it stands, this PR updates truncate so that it no longer escapes inputs that were already html_safe. If, for some reason, you want to escape and html_safe input you must explicitly direct it to do so by adding escape: true as an option.

This is a bit confusing due to the existing behaviour of ERB::Util.html_escape which won't escape an html_safe string and truncate docs which say all input will be escaped by default. This is complicated by the String#truncate method which may or may not modify the input which may or may not toggle the return value's html_safe-ness (which then affects how ERB::Util.html_escape handles it).

That aside, this method should now behave as it did under Rails 3.2, which may or may not be desirable.

@robin850 robin850 added this to the 4.2.0 milestone May 3, 2014
@anitagraham anitagraham referenced this pull request in refinery/refinerycms Jul 15, 2014
Closed

Changes to specs to remove deprecated syntax #2621

@pallan

Merged to latest master and tests are still passing.

@rafaelfranca rafaelfranca modified the milestone: 4.2.0, 5.0.0 Aug 18, 2014
@pallan

Rebased against latest master.

@rafaelfranca rafaelfranca removed this from the 5.0.0 [temp] milestone Apr 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment