This project is archived and is in readonly mode.

Content_tag does not escape its input!

#3883

Hi,

I've read http://yehudakatz.com/2010/02/01/safebuffers-and-rails-3-0/ and I've tried the example. Obviously, tag has to be remplaced by content_tag (error 500 else). But, the output is not the expected one, evil_js can be executed:

Hello <strong>friends</strong>!
 
<p><script>evil_js</script></p>
&lt;script&gt;evil_js&lt;/script&gt;

I've tried to fix content_tag_string like this:

        def content_tag_string(name, content, options, escape = true)

      tag_options = tag_options(options, escape) if options
      &quot;&lt;#{name}#{tag_options}&gt;#{ERB::Util.h content}&lt;/#{name}&gt;&quot;.html_safe
    end </code>

But this breaks a lot of unit tests (184 failures in actionpack). So, I wonder if content_tag is really a public API that should escape its input, or if it's only for internal usage.

In the first case, it will also fix 2 others of my tickets: #3450 Field_set_tag doesn't escape the legend and #3449 Label tag doesn't escape its input.

Reported by Bruno Michel · February 6th, 2010 @ 10:31 PM

Activity

Bruno Michel
- Tag changed from 3.0pre, xss to 3.0pre, patch, xss
I really think that content_tag should be public, so I've patched it. Fixing the failing tests was not that hard. So here's the patch.
- escape_input_of_content_tag.diff 13.1 KB
February 6th, 2010 @ 11:47 PM
Santiago Pastorino
Why did you remove to_s from select_html << select_options_as_html.to_s ?

February 12th, 2010 @ 07:33 PM
Bruno Michel
I remove it because it's useless. select_options_as_html is an argument passed to build_select. build_select is called two times in the whole Rails codebase, and for those two times, select_options_as_html is already a string. Moreover, considering the means of build_select, I can't see why you someone should want to give it anything else than a String (or a SafeBuffer, of course). So, I remove it as I think it's a ugly artifact.

But, if it's a problem, I can redo the patch with the to_s.

Thanks for your review :-)

February 13th, 2010 @ 02:28 PM
Santiago Pastorino
Yes, you're right and is a privare method.
So +1 for the patch.
I have a complementary patch for this one so i'll talk to yehuda to apply both.

February 13th, 2010 @ 03:27 PM

Santiago Pastorino

Hey Michael the last thing, I'd do that to your patch

 content = content.html_safe unless escape

 content = html_escape(content) if escape

 content_tag :textarea, content, { "name" => name, "id" => sanitize_to_id(name) }.update(options)

 content_tag :textarea, content.html_safe, { "name" => name, "id" => sanitize_to_id(name) }.update(options)

Are you agree? I think it's cleaner.

February 13th, 2010 @ 08:57 PM

Santiago Pastorino
change ... content = content.html_safe unless escape to content = html_escape(content) if escape (that was the original line)
and change ... content_tag :textarea, content, { "name" => name, "id" => sanitize_to_id(name) }.update(options) to content_tag :textarea, content.html_safe, { "name" => name, "id" => sanitize_to_id(name) }.update(options)

February 13th, 2010 @ 08:59 PM
Santiago Pastorino
- escape_input_of_content_tag.diff 15.5 KB
February 13th, 2010 @ 10:00 PM
Bruno Michel
I'm OK with this change.

By the way, my firstname is Bruno and my lastname Michel, not Michael ;-)

February 13th, 2010 @ 10:08 PM
Santiago Pastorino
Sorry Bruno, Yehuda is about to apply it, great work.

February 13th, 2010 @ 10:17 PM
Prem Sichanugrist (sikachu)
- State changed from new to resolved
February 15th, 2010 @ 03:09 PM
thedarkone
Shouldn't this also go into the current 2-3-stable?

March 23rd, 2010 @ 06:37 PM
Santiago Pastorino
thedarkone: The current 2-3-stable has all the necessary things regarding html_safe, or at least the known ones.
If you use http://github.com/rails/rails_xss with a 2-3-stable application you have all the protections against xss.

March 23rd, 2010 @ 07:19 PM
thedarkone
Yes, you are right indeed. My bad, I missed your yesterday's commit. Thanks.

March 24th, 2010 @ 12:49 AM