Rails core - Sep 2009 - Rails form helpers uses "escape_once" causing undesirable behavior

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 "<" 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.

> I would argue that the current behavior is broken.  If I pass in the
> string "<" 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.

http://github.com/NZKoz/rails_xss
http://github.com/NZKoz/rails/tree/rails_xss



-- 
Cheers

Koz

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.

On Sep 30, 4:09 pm, Michael Koziarski <mich...@koziarski.com>
wrote:
> > 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.
>
>
http://github.com/NZKoz/rails_xsshttp://github.com/NZKoz/rails/tree/rails_xss
>
> --
> Cheers
>
> Koz

On Fri, Oct 2, 2009 at 6:45 AM, stephenjudkins
<stephen.judkins@gmail.com> wrote:
>
> 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 :)

> Keep up the good work!  We appreciate it.
>
> On Sep 30, 4:09 pm, Michael Koziarski <mich...@koziarski.com> wrote:
>> > 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.
>>
>>
http://github.com/NZKoz/rails_xsshttp://github.com/NZKoz/rails/tree/rails_xss
>>
>> --
>> Cheers
>>
>> Koz
> >
>


-- 
Cheers

Koz