This project is archived and is in readonly mode.
Content_tag does not escape its input!
Reported by Bruno Michel | February 6th, 2010 @ 10:31 PM
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>
<script>evil_js</script>
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
"<#{name}#{tag_options}>#{ERB::Util.h content}</#{name}>".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: https://rails.lighthouseapp.com/projects/8994/tickets/3450-field_se... and https://rails.lighthouseapp.com/projects/8994/tickets/3449-label-ta....
Comments and changes to this ticket
-
Bruno Michel February 6th, 2010 @ 11:47 PM
- Tag changed from “3.0pre, xss” to “3.0pre, patch, xss”
I really think that
content_tagshould be public, so I've patched it. Fixing the failing tests was not that hard. So here's the patch. -
Santiago Pastorino February 12th, 2010 @ 07:33 PM
Why did you remove to_s from select_html << select_options_as_html.to_s ?
-
Bruno Michel February 13th, 2010 @ 02:28 PM
I remove it because it's useless.
select_options_as_htmlis an argument passed tobuild_select.build_selectis called two times in the whole Rails codebase, and for those two times,select_options_as_htmlis already a string. Moreover, considering the means ofbuild_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 :-)
-
Santiago Pastorino February 13th, 2010 @ 03:27 PM
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. -
Santiago Pastorino February 13th, 2010 @ 08:57 PM
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.
-
-
Santiago Pastorino February 13th, 2010 @ 08:59 PM
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) -
Santiago Pastorino February 13th, 2010 @ 10:00 PM
- no changes were found...
-
Bruno Michel February 13th, 2010 @ 10:08 PM
I'm OK with this change.
By the way, my firstname is Bruno and my lastname Michel, not Michael ;-)
-
Santiago Pastorino February 13th, 2010 @ 10:17 PM
Sorry Bruno, Yehuda is about to apply it, great work.
-
Prem Sichanugrist (sikachu) February 15th, 2010 @ 03:09 PM
- State changed from “new” to “resolved”
-
-
Santiago Pastorino March 23rd, 2010 @ 07:19 PM
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. -
thedarkone March 24th, 2010 @ 12:49 AM
Yes, you are right indeed. My bad, I missed your yesterday's commit. Thanks.
Create your profile
Help contribute to this project by taking a few moments to create your personal profile. Create your profile »
<h2 style="font-size: 14px">Tickets have moved to Github</h2>
The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>
People watching this ticket
Bruno Michel
Prem Sichanugrist (sikachu)
Santiago Pastorino
Yehuda Katz (wycats)
Attachments
Referenced by
-
3957
Explicit html_escape usage removed when not needed
This patch should be applied after that one
https://rail...