This project is archived and is in readonly mode.

#4066 ✓invalid
Greg Hazel

auto_link output should be marked .html_safe!

Reported by Greg Hazel | February 26th, 2010 @ 09:18 PM | in 3.0.2

x = h("<foo> http://foo.com </foo>")
o = auto_link(x)

In this example, "x" is marked as html_safe, but "o" is not. This is different from something like link_to:

x = h("<foo> wee </foo>")
o = link_to(x, "http://foo.com")

Where both are html_safe.

Obviously auto_link does not escape elements, so it should only mark its output as html_safe if the input was html_safe.

Comments and changes to this ticket

  • Greg Hazel

    Greg Hazel February 26th, 2010 @ 09:47 PM

    Wow, actually. Is there a right way to use auto_link with escaping at all?

    Say you have a blob of text like:

    b = "<foo> http://foo.com?p=1&v=2 </foo>"
    

    Passing it through auto_link gives you:

    auto_link(b)
    "<foo> <a href=&quot;http://foo.com?p=1&amp;v=2&quot;>http://foo.com?p=1&amp;v=2</a> </foo>"
    

    Which is right, except you have the unescaped tags in there. Bad. Escaping that string would escape your tag as well, and that would be bad.

    Escaping the contents first gives you:

    h(b)
    "&lt;foo&gt; http://foo.com?p=1&amp;v=2 &lt;/foo&gt;"
    

    So far so good, but passing that to auto_link:

    auto_link(h(b))
    "&lt;foo&gt; <a href=&quot;http://foo.com?p=1&amp;v=2&quot;>http://foo.com?p=1&amp;amp;v=2</a> &lt;/foo&gt;"
    

    produces a string with &amp;amp; in it. Clearly wrong.

    How should this be done?

  • Mislav

    Mislav April 17th, 2010 @ 03:29 AM

    • Tag changed from xss auto_link to auto_link, html, sanitize, xss

    There are use cases where developers would pass HTML content to be auto-linked (think HTML comments) and some where they want plaintext content with an occasional link where applicable.

    I think that the responsibility of the developer to use a sanitize helper on the final output if they want to be sure there are only allowed tags in the output.

    I would like to get some more opinions on this, though

  • Santiago Pastorino

    Santiago Pastorino April 19th, 2010 @ 07:06 PM

    • Milestone cleared.
    • State changed from “new” to “verified”
    • Assigned user set to “Santiago Pastorino”
  • Michael Koziarski

    Michael Koziarski April 19th, 2010 @ 10:19 PM

    • State changed from “verified” to “invalid”

    auto_link doesn't return safe strings. It makes no effort to remove potentially hostile content.

    If you want to use auto_link in a guaranteed safe manner the only option you have is:

    <%= sanitize(auto_link(x)) %>

    If you believe your particular situation is safe you can use the raw helper, but odds are it's not as safe as you think it is :)

  • Greg Hazel

    Greg Hazel April 19th, 2010 @ 10:23 PM

    What about the case I was initially reporting, where the input string is html_safe already?

  • Michael Koziarski

    Michael Koziarski April 19th, 2010 @ 10:32 PM

    What's the scenario you have in mind? I'm a little bit hesitant to introduce the expectation that the helper will behave differently based on what its inputs are as it breaks the simple-but-provable model that we have now where a helper will only return a safe string if it knows that the string is safe.

    Strictly speaking we don't know that with the working of auto_link and the person who does (you) can just as easily add raw to your views?

  • Greg Hazel

    Greg Hazel April 19th, 2010 @ 10:49 PM

    Well, auto_link(h(b)) is particularly the use-case I had in mind. I suppose I can write raw(auto_link(h(b))), but that feels a bit odd - as if auto_link doesn't know if it's producing safe html or not..

    Honestly though, because that has the above problem I mentioned, I would prefer a auto_link_and_escape function which split up the buffer, escaped the segments which were not urls and linkified the sections which were (marking those html_safe!) then added the whole thing back together. Is that not something anyone else wants?

  • Neeraj

    Neeraj April 19th, 2010 @ 11:08 PM

    I would say have two methods: auto_link and auto_link_safe.

  • Michael Koziarski

    Michael Koziarski April 20th, 2010 @ 12:04 AM

    I think the only reliable way to actually implement auto_link_and_escape would be to implement it as sanitize(auto_link). Users can do that for now so I'm leaving this closed

  • Jeremy Kemper

    Jeremy Kemper October 15th, 2010 @ 11:01 PM

    • Milestone set to 3.0.2
    • Importance changed from “” to “Low”

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Pages