Rails form helpers uses "escape_once" causing undesirable behavior

rubyonrails-core
1

Hi, I posted this on lighthouse a few weeks ago: https://rails.lighthouseapp.com/projects/8994/tickets/3213-html-literals-passed-into-tag-helpers-dont-escape

A patch, with tests, is included. I'd like to use form helpers but we're forced not to in some situations because of this bug popping up. However, this patch may break some behavior that people rely upon.

I would argue that the current behavior is broken. If I pass in the string "&lt;" to a form helper, I expect it to show up in the browser as "&lt;", not "<". If some people are passing pre-escaped HTML into form helpers, that is an ambiguity in their code that they should fix.

Note that applying my patch might cause undesirable behavior in some people's (broken) applications, but could not possibly introduce any XSS vulnerabilities. This is because it's replacing a semi-broken version of HTML escaping with one that's stricter.

If this has been discussed before, I would appreciate a pointer to the discussion. If it merits further discussion, I would be happy to argue my case. If this patch is going to be accepted, consider this message a gentle prod for it to be merged in.

2

I would argue that the current behavior is broken. If I pass in the string "&lt;" to a form helper, I expect it to show up in the browser as "&lt;", not "<". If some people are passing pre-escaped HTML into form helpers, that is an ambiguity in their code that they should fix.

Unfortunately it's not *quite* that simple because there are places where we're blindly escaping and then re-escaping those strings.

escape_once is a gross hack and my goal is to remove it in 3.0 in favour of using the rails_xss stuff I have in a branch / plugin at present. That will let us distinguish between "&lt;", and h("<"), and only escape the &lt; case.

So my gut feeling here is that we favour a proper fix in 3.0, and leave 2-3-stable 'consistently weird' rather than run the risk of complicating things.

3

Thanks for the feedback.

I'm not sure when Rails 3 is going to come out---and I'm not going to pester you about it--but I noticed that your rails_xss branch is merging in changes from 2-3-stable. (It looks like nice work, by the way.) What's the chance of getting the XSS stuff out in a Rails 2.3.5 release or something like that? If the XSS branch could work with Rails 2.3.x, it would make for an easier migration path. I expect we'll have to do a good deal of work to get our app to run in Rails 3, so any potentially difficult changes that can be merged in before that would be helpful.

Keep up the good work! We appreciate it.

4

Thanks for the feedback.

I'm not sure when Rails 3 is going to come out---and I'm not going to pester you about it--but I noticed that your rails_xss branch is merging in changes from 2-3-stable. (It looks like nice work, by the way.) What's the chance of getting the XSS stuff out in a Rails 2.3.5 release or something like that? If the XSS branch could work with Rails 2.3.x, it would make for an easier migration path. I expect we'll have to do a good deal of work to get our app to run in Rails 3, so any potentially difficult changes that can be merged in before that would be helpful.

The current plan is to merge the String#html_safe! and the helper changes into 2.3.5 (or .6, depending on when I get the time), and leave the 'escape by default' stuff in a plugin. Then for 3.0 the escape by default stuff is in core.

So people should be able to beta test the functionality in 2.3.x, and let us fix any surprises before 3.0.

Having said all that, I have a newborn due within 3 weeks, so it's entirely possible my plans will change and I'll need someone else to wrap it up. Hope not though :slight_smile: