Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

`truncate' on html-safe input will escape only when truncating #13721

Open
afn opened this Issue Jan 15, 2014 · 8 comments

Comments

Projects
None yet
4 participants
Contributor

afn commented Jan 15, 2014

If the text passed to truncate is an html-safe string, the result gets an extra level of encoding if (and only if) truncation is performed. This seems unintuitive, although it's not clear from the documentation whether the expected behavior is to encode in both cases or neither.

Loading development environment (Rails 4.1.0.beta1)
2.1.0p0 :001 > include ActionView::Helpers
 => Object
2.1.0p0 :002 > truncate('foo&bar blahblahblah'.html_safe, length: 20, separator: ' ')
 => "foo&bar..."
2.1.0p0 :003 > truncate('foo&bar blahblahblah'.html_safe, length: 30, separator: ' ')
 => "foo&bar blahblahblah"

If I pass escape: true the behavior is unchanged.

pallan commented Jan 16, 2014

The current docs for truncate seem to indicate, to me at least, that I should expect whatever I pass to it to be encoded and that if I don't want to encode it I should pass encode: false.

As for why this only happens when truncation is performed, its because the core extension String#truncate returns a duplicate of the passed text when no truncation is necessary. Since it is a duplicate it is already been marked as HTML-safe. When it does have to modify the original string it creates a new instance that is not marked HTML-safe. When ActionView::Helper#truncate goes to escape the string it recognizes the duplicate as already being safe and does not change it. The new copy is modified as it is not considered HTML safe and you get duplicate encoding. Do you think that make sense?

If this needs to be fixed this all that would be needed is to check to see if the original string was HTML safe. If it was, consider the modified string HTML-safe and don't do the conversion on it.

Contributor

afn commented Jan 16, 2014

I think you can make a reasonable argument for either behavior:

  • if the text parameter is already HTML-safe, then do not do any escaping (even if :escape is true), _OR_
  • if :escape is true, then always escape the string, even if it is already marked HTML-safe

I think the former is more in line with what I would expect, so I think your proposed solution makes sense (if the original string was HTML-safe, then consider the modified string HTML-safe as well and don't do the conversion). Either way, though, the current behavior is surprising to say the least. The presence or absence of encoding should not depend on whether or not the input string happens to be within the threshold length.

pallan commented Jan 16, 2014

This code has been rewritten in Rails 4. In Rails 3 the escape option did not exist and the duplicate escaping did not happen. I am going to see if I can fix it.

@afn afn added the stale label Apr 23, 2014

Owner

rafaelfranca commented May 1, 2014

This issue has been automatically marked as stale because it has not been commented on for at least
three months.

The resources of the Rails team are limited, and so we are asking for your help.

If you can still reproduce this error on the 4-1-stable, 4-0-stable branches or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

Contributor

afn commented May 1, 2014

@pallan There hasn't been any activity on this since your pull request. Are we still waiting for feedback, or can this change be merged?

Thanks!
Tony

@rafaelfranca rafaelfranca added attached PR and removed stale labels May 1, 2014

pallan commented May 1, 2014

I was waiting for feedback. I will have to verify that it still works with HEAD but otherwise I think its ready to go.

@anitagraham anitagraham referenced this issue in refinery/refinerycms Jul 15, 2014

Closed

Changes to specs to remove deprecated syntax #2621

listx commented Jan 5, 2015

The behavior originally described by @afn still exists on 4.2.0.

listx commented Jan 8, 2015

I just want to make clear/summarize the discussion so far, but with slightly
adjusted input methods to help ease readability for others looking at this
issue. My goal is to help others understand the issue better (maybe we need more
help on this thing?). I hope it helps!

I am using the input string >>>>> because they are just as HTML-unsafe as
@afn's original example, and I can still reproduce the same kind of behavior,
when html_safe is involved. Also, I'll be using omission: 'X' because I
think it makes the output easier to read.

The following is the block of code to paste into your Rails console to
cross-check against my machine.

.irbrc:

include ActionView::Helpers::TextHelper
truncate('>>>>>',           omission: 'X', length: 5)
truncate('>>>>>',           omission: 'X', length: 4)
truncate('>>>>>',           omission: 'X', length: 5, escape: true)
truncate('>>>>>',           omission: 'X', length: 5, escape: false)
truncate('>>>>>',           omission: 'X', length: 4, escape: true)
truncate('>>>>>',           omission: 'X', length: 4, escape: false)
truncate('>>>>>'.html_safe, omission: 'X', length: 5)
truncate('>>>>>'.html_safe, omission: 'X', length: 4)
truncate('>>>>>'.html_safe, omission: 'X', length: 5, escape: true)
truncate('>>>>>'.html_safe, omission: 'X', length: 5, escape: false)
truncate('>>>>>'.html_safe, omission: 'X', length: 4, escape: true)
truncate('>>>>>'.html_safe, omission: 'X', length: 4, escape: false)
Loading development environment (Rails 4.2.0)
irb(main):001:0> include ActionView::Helpers::TextHelper
=> Object
irb(main):002:0> truncate('>>>>>',           omission: 'X', length: 5)
=> ">>>>>"

    (A) OK; we escape the string to make it HTML-safe.

irb(main):003:0> truncate('>>>>>',           omission: 'X', length: 4)
=> ">>>X"

    (B) OK; we escape the string to make it HTML-safe, then remove the last
    character with an 'X', our omission character. We get a total length of 4
    characters rendered on-screen, which is great!

irb(main):004:0> truncate('>>>>>',           omission: 'X', length: 5, escape: true)
=> ">>>>>"

    (C) OK; no change from (A); the only difference is that we escape
    explicitly.

irb(main):005:0> truncate('>>>>>',           omission: 'X', length: 5, escape: false)
=> ">>>>>"

    (D) OK; escaping is disabled so we get the plain string back.

irb(main):006:0> truncate('>>>>>',           omission: 'X', length: 4, escape: true)
=> ">>>X"

    (E) OK; this is just like (B), but with explicity escaping with `escape`.

irb(main):007:0> truncate('>>>>>',           omission: 'X', length: 4, escape: false)
=> ">>>X"

    (F) OK; we disable escaping, and we also truncate.


irb(main):008:0> truncate('>>>>>'.html_safe, omission: 'X', length: 5)
=> ">>>>>"

    (G) OK; this is expected. It is the functional equivalent of (D). The only
    difference is that the string is marked HTML-safe, instead of passing in
    `escape: false`.

irb(main):009:0> truncate('>>>>>'.html_safe, omission: 'X', length: 4)
=> ">>>X"

    (H) ERROR 1; this is the bug as originally described by @afn.

irb(main):010:0> truncate('>>>>>'.html_safe, omission: 'X', length: 5, escape: true)
=> ">>>>>"

    (I) ERROR 2?; this is a special case. Although we marked the string as
    HTML-safe, we still asked Rails to escape it, but it did not. This is what
    @pallan's first comment was addressing. Whether this is a bug itself or not
    depends on how you want to define the precedence of `escape` vs the
    `html_safe` method.

irb(main):011:0> truncate('>>>>>'.html_safe, omission: 'X', length: 5, escape: false)
=> ">>>>>"

    (J) OK; however, the coder cannot tell whether the string was left as-is,
    because of the call to `.html_safe` or because we told it explicitly not to
    escape with `escape: false`, or both!

irb(main):012:0> truncate('>>>>>'.html_safe, omission: 'X', length: 4, escape: true)
=> ">>>X"

    (K) OK? This is just like (I) in that it is a special case. We already
    marked the string as HTML-safe, but we explicitly asked Rails to escape it.
    Personally, I think this is not an error and is expected behavior.

irb(main):013:0> truncate('>>>>>'.html_safe, omission: 'X', length: 4, escape: false)
=> ">>>X"

    (L) OK; however, just like (J), we cannot tell whether it was `.html_safe`
    or `escape: false` that made the resulting string not HTML-escaped.

I think the discussion so far boils down to (H) and (I). @afn's second comment
is about cases (J) and (K), where the precedence of .html_safe vs. escape: true/false is ambiguous.

I disagree with @afn's 2nd comment; I think that the escape option should take
precedence over .html_safe, because it is explicit. That is, even if a string
is marked HTML-safe, passing along escape: true should still make it
HTML-escaped one level. After all, whether the string is marked HTML-safe with
.html_safe is something that might happen elsewhere in the application; if
we're calling truncate with escape now, it makes sense to uphold that
request.

I believe resolving the ambiguity between passing in
already-HTML-safe-marked-strings vs. the escape option is just as important
here as the original bug report.

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