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

Escaping is being done on static HTML in results of <%= cache... %> calls and tabnav plugin output #11

Open weyus opened 14 years ago

weyus commented 14 years ago

Rails 2.3.5

I installed this plugin and saw most of my page start displaying HTML source. It appears that the static HTML in the output of a call like:

<% cache "header-#{current_user.id}", 1.day.from_now do -%> <%= render :partial => '/layouts/header' %> <% end -%>

or

<%= start_tabnav :customer %> <%= show_flash(:success) %> <%= show_flash(:notice) %> <%= show_flash(:error) %> <%= yield %> <%= end_tabnav %>

is being escaped, thus showing up as raw text in the browser.

NZKoz commented 14 years ago

the cache case looks like it could be a bug

Could you paste the implementation of your tab / flash helpers into a gist for me to take a look at? That's either a bug in rails_xss or a change you need to make to your code.

weyus commented 14 years ago

Here's the gist: http://gist.github.com/286289

It looks like I need to call raw in front of each of these, as in:

<%= raw show_flash... %> <%= raw start_tabnav... %>

NZKoz commented 14 years ago

Yes, you'll need to either call raw in your views, or modify your helpers to pre-mark those strings as safe. e.g.

(result + css).html_safe!

A String is considered safe ONLY if we're told it's safe, we don't try to guess based on the content as this is likely to lead to security bugs. We've made efforts to ensure that all helpers which use the built-in tag helpers will be 'magically' safe, but for your kind of helpers where you're manipulating big strings, you'll have to do the marking yourself

However the cache case seems like it should work out of the box.

weyus commented 14 years ago

Many thanks.

BTW, I think this is a great feature to add to the default set of features in Rails 3.

NZKoz commented 14 years ago

Yes, it's already been included ;)