NZKoz / rails_xss

A plugin for rails 2.3.5 applications which switches the default to escape by default. Later versions should use rails/rails_xss
MIT License
215 stars 39 forks source link

will_paginate: one helper escaped, other one not #4

Closed mislav closed 14 years ago

mislav commented 14 years ago

I have 2 main view helpers in will_paginate: will_paginate and page_entries_info. A user reported that he can't use my library in Rails 2.3.5 with this plugin because XSS protection escapes HTML rendered by my helpers because they're not marked html_safe!.

Before fixing will_paginate I tried to reproduce the issue, and something weird happened. I created a blank Rails app, added latest stable will_paginate to gem list, installed rails_xss from git and created a HomeController with script/generate controller home index. This was my view:

<h1>Home#index</h1>
<p>Find me in app/views/home/index.html.erb</p>

<%= will_paginate WillPaginate::Collection.new(2, 5, 25) %>
<%= page_entries_info WillPaginate::Collection.new(2, 5, 25) %>

The unexpected result in the output was that will_paginate HTML output wasn't escaped, but page_entries_info was. Here are the rough implementations of these two view helpers:

def will_paginate(collection = nil, options = {})
  # ...
  # render HTML for pagination
  renderer.prepare collection, options, self
  renderer.to_html
end

def page_entries_info(collection, options = {})
  # ...
  %{Displaying #{entry_name.pluralize} <b>%d&nbsp;-&nbsp;%d</b> of <b>%d</b> in total} % [
      collection.offset + 1,
      collection.offset + collection.length,
      collection.total_entries
    ]
end

And here is the Erubis log I got in my development.log

** Erubis: src==<<'END'
@output_buffer = ActionView::SafeBuffer.new;
@output_buffer << (''.html_safe!); __in_erb_template=true ;@output_buffer << ('<h1>Home#index</h1>
<p>Find me in app/views/home/index.html.erb</p>

'.html_safe!);@output_buffer << (( will_paginate WillPaginate::Collection.new(2, 5, 25) ).to_s);@output_buffer << ('
'.html_safe!);@output_buffer << (( page_entries_info WillPaginate::Collection.new(2, 5, 25) ).to_s);@output_buffer << (''.html_safe!);@output_buffer.to_sEND
nono commented 14 years ago

If I understand correctly will_paginate code, the will_paginate helper uses only Rails helpers to construct its output, but the page_entries_info helper interpolates strings.

In the first case, each string generated by a rails helper is marked as html_safe!, and their concatenation is thus marked as html_safe!. So when Rails renders it, it doesn't escape it.

But, for the page_entries_info, the output is a string not marked as html_safe!, so Rails escape it. I think your problem is described as the third gotcha of http://github.com/NZKoz/rails_xss/blob/master/README.markdown (String interpolation won't be safe, even when it 'should' be).

mislav commented 14 years ago

Thanks for explaining. It's obvious to me why xss protection escapes page_entries_info, I just wasn't sure why it doesn't do the same for will_paginate output.

You might be on to something when you said that will_paginate uses Rails helpers: it does use link_to and content_tag internally in the link renderer class. Still, it creates an array of all pagination elements this way and joins them with a space using Array#join. I assumed that two safe strings joined with this method will result in an unsafe string, but looks like I was wrong.

Koz, is this wanted behavior or something you missed?

NZKoz commented 14 years ago

If you want to take a stab at making Array#join work I wouldn't object, but it seems just a little too magic to me. join handles arrays of 'things other than strings' so I can't see an easy way to reimplement it without entirely duplicating all the logic.

In rails itself I changed all those case to .join(" ").html_safe! seemed more explicity

NZKoz commented 14 years ago

Closing this ticket anyway, if you want to take a stab at Array#join that should probably done against rails itself rather than here.