Helper methods should escape HTML in parameters

[ryanb]

Since link_to and other helper methods return an html_safe string, wouldn't it make sense for them to escape the content passed to them? Otherwise the application is still vulnerable to XSS attacks. This is the current behavior:

<%= link_to @comment.name, @comment.url %> is vulnerable to XSS
<%= link_to h(@comment.name), h(@comment.url) %> is not vulnerable

I propose all helper methods which return html_safe strings should escape the parameters passed to them (unless they are html_safe already).

[minaguib]

This is legal:

link_to "", @comment.url

[ryanb]

Can you post that code snippet again in pre tags? I can't see the content in the quotes.

[minaguib]

@ryanb Created a gist instead: http://gist.github.com/244787

[ryanb]

@minaguib, your issue is present for normal output as well. If someone does this with the plugin:

<%= "" %>

It will escape the HTML unless you are using the raw method or have marked the string as html_safe.

<%= raw "" %>

The behavior should be consistent, so whether you're passing a string to helper method or straight out with ERB, the auto-escaping should take effect.

[minaguib]

@ryanb I understand the above, but, the current behavior of link_to allows you to either: link_to x, comments_path or link_to h(x), comments_path

Where x can be a trusted html chunk (maybe even the output of a render :partial), or an untrusted user-inputted string.

With the current implementation, the flexibility exists to do both. If link_to and friends auto-escape the first (content) argument, there would be no way to use it programmatically to surround a piece of trusted html with a link.

[nono]

@ryanb : I've just created a bug report on lighthouse with a patch for this bug -> https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/3518-link_to-doesnt-escape-its-input .

@minaguib : you can do that with: link_to x.html_safe!, comments_path Strings marked as html_safe are not escaped.

[nono]

By the way, I've also found the same flaw on label_tag (https://rails.lighthouseapp.com/projects/8994/tickets/3449-label-tag-doesnt-escape-its-input ) and field_set_tag (https://rails.lighthouseapp.com/projects/8994/tickets/3450-field_set_tag-doesnt-escape-the-legend ).

[NZKoz]

@minaguib: image_tab, and render :partial both return safe strings so will work fine.

The catch with this one is that we can't put these escape calls into 2-3-stable as it'll break people's apps. So we need to have two seperate fixes. A heavy monkey-patch for this plugin for 2-3-stable users, and just a patch against master.

[ryanb]

@NZKoz, could we add an escape method to 2-3-stable which does nothing unless enabled by this plugin or a config option? This way it will have no effect on existing apps.

However, that does seem like a significant core feature change, and I'm okay with waiting until 3.0 hits.

[minaguib]

@NZKoz

I just realized that there are actually 2 concerns.

• A String's notion of html_safe or not (slightly ill defined)
• Rendering strings, and possibly needing to escape them

The current behavior of h(), not altering html_safe! strings, is what was confusing me, though I'm not sure if my expectation or the current behavior makes more sense.

h() not touching safe strings is great for legacy apps - they will not need to go through all their views and clean up the now-useless h calls to prevent double-escaping.

However, h logically means "escape the input because I want to display it verbatim to the user" - the fact that the input is a html_safe, or not, IMHO, should not matter.

Perhaps this is a non-issue in the real world, or a simple naming issue whether with the historical h() or with the new html_safe!()

(What html_safe! currently does is more like .never_escape!)

[ryanb]

@minaguib, this behavior of the h() method is very useful outside of legacy apps. It should still be used frequently in helper methods to ensure the HTML is escaped if not already. For example, let's say I'm writing a strong helper method. It might go like this.

def strong(content)
  "<strong>#{h(content)}</strong>".html_safe!
end

Since we are returning an html_safe string, it should ensure the content is html_safe as well, and the "h" method is a good way to do this so existing html_safe strings are not modified.

[minaguib]

@ryanb

Don't get me wrong. I like what's being done. I'm just worried about the consistency.

Take the strong helper, for example, if you're writing a tutorial site and want to show the actual HTML in a < code > block. You can no longer count on h() escaping HTML to be shown to the user verbatim. Sometimes it will. Sometimes it wont, depending on whether its input is marked html_safe! or not...

[jfine]

I just did a major palm to forehead. All this time I was working under the impression that link_to escaped the first param! The way it functions seems completely backwards. To want to wrap raw html with a link seems much more the exception rather than the rule.

Like everything in rails there should certainly be the ability to override so you can do whatever you want. There aren't even that many legal html elements you can have inside of an anchor.

So I guess instead of going off and adding hundreds if not thousands of h's all over the place I'll be creating a the_way_link_to_should_work plugin.

[jfine]

@minaguib

With the current implementation, the flexibility exists to do both. If link_to and friends auto-escape the first (content) argument, there would be no way to use it programmatically to surround a piece of trusted html with a link.

I was thinking something like link_to(username, user, :raw => true). There is probably a slightly better syntax but something like that would work great for me.

[NZKoz]

Guys, link_to can and will work this way in 3.0 We'll fix that.

However we can't just make that change in 2.3.x without busting people's apps. So I think what we should do is make this plugin fix link_to like you expect.

Anyone want to take a stab at this?

[jfine]

@NZKoz

Very cool and totally understandable with regard to <3.0. After considerable thought I think @ryanb put it best that any string passed to link_to (and siblings) not marked as html_safe should be escaped. Looks like @nono has already submitted a patch. I'll take a peek and see if it does want I think we're all talking about.

[ryanb]

@minaguib, good point, sometimes escaping safe html is intentional. Perhaps the escape_html method can be used for simple, consistent escaping.

@jfine, you can use the raw method.

link_to raw(username), ...

Here no escaping would happen. You can also use html_safe! to mark the string as safe so it won't be escaped.

@NZKoz, there are a lot of methods in addition to link_to which fall into this category. Any helper method which takes an argument and returns HTML should be considered.

I wonder if it's possible to handle this at a lower level? For example, if everything went through content_tag then the escaping could be handled there. It currently doesn't so maybe that's something to think about for Rails 3.

[NZKoz]

@ryanb: No need to overthink the problem here, you can just blindly escape the relevant parameters and rely on the idempotency to handle all the weird cases.

Using content_tag seems a little broken because it's expected to be passed big html strings...

[spastorino]

Please try using the latest 2-3-branch and http://github.com/rails/rails_xss and send me all the issues you find.

[nono]

I have a problem with button_to with the lastest 2-3-branch and http://github.com/rails/rails_xss: the button_to HTML code is escaped, so users see the HTML code instead of the button. There is a Rails ticket on lighthosue about that: https://rails.lighthouseapp.com/projects/8994/tickets/3448-button_to-does-not-return-an-html-safe-string. The problem is with Rails, not Rails_xss, but maybe can you do something about it ;-)

By the way, thanks for your good work on Railsx_xss.

[nono]

By the way, in Rails 2.3, content_tag doesn't escape its input, neither do functions that use it like label_tag. In Rails 3.0, my patch for that was accepted: https://rails.lighthouseapp.com/projects/8994/tickets/3883-content_tag-does-not-escape-its-input. Do you think that it should be backported to Rails 2.3 or in Rails_xss?

[spastorino]

That is supposed to be working now, you have to generate your app with Rails 2-3-stable branch from git. then vendor that 2-3 edge rails and use rails/rails_xss if it doesn't work please report to LH or give me some test or example. Thanks for your hard work Bruno, ;).

[nono]

With the current 2-3-stable branch for rails and the latest version of rails/rails_xss, I have a popup if I use any of these lines: <%= label_tag "" %> <%= content_tag :p, "" %>

[spastorino]

Thanks Michael, we just fixed that it's about to being pushed. Thanks again for your hard help.