Skip to content

Commit

Permalink
Merge the prerequisites for on-by-default XSS escaping into rails.
Browse files Browse the repository at this point in the history
This consists of:

* String#html_safe! a method to mark a string as 'safe'
* ActionView::SafeBuffer a string subclass which escapes anything unsafe which is concatenated to it
* Calls to String#html_safe! throughout the rails helpers
* a 'raw' helper which lets you concatenate trusted HTML from non-safety-aware sources (e.g. presantized strings in the DB)

Note, this does *not* give you on-by-default XSS escaping in 2.3 applications.  To get that you'll need to install a plugin:

http://github.com/nzkoz/rails_xss
  • Loading branch information
NZKoz committed Oct 8, 2009
1 parent a69316b commit 80da8eb
Show file tree
Hide file tree
Showing 26 changed files with 297 additions and 30 deletions.
6 changes: 3 additions & 3 deletions actionpack/lib/action_view.rb
Expand Up @@ -49,10 +49,10 @@ def self.load_all!
autoload :TemplateHandler, 'action_view/template_handler'
autoload :TemplateHandlers, 'action_view/template_handlers'
autoload :Helpers, 'action_view/helpers'
autoload :SafeBuffer, 'action_view/safe_buffer'
end

class ERB
autoload :Util, 'action_view/erb/util'
end
require 'action_view/erb/util'


I18n.load_path << "#{File.dirname(__FILE__)}/action_view/locale/en.yml"
5 changes: 5 additions & 0 deletions actionpack/lib/action_view/erb/util.rb
Expand Up @@ -18,6 +18,11 @@ def html_escape(s)
s.to_s.gsub(/[&"><]/) { |special| HTML_ESCAPE[special] }
end

alias h html_escape

module_function :html_escape
module_function :h

# A utility method for escaping HTML entities in JSON strings.
# This method is also aliased as <tt>j</tt>.
#
Expand Down
2 changes: 2 additions & 0 deletions actionpack/lib/action_view/helpers.rb
Expand Up @@ -14,6 +14,7 @@ module Helpers #:nodoc:
autoload :JavaScriptHelper, 'action_view/helpers/javascript_helper'
autoload :NumberHelper, 'action_view/helpers/number_helper'
autoload :PrototypeHelper, 'action_view/helpers/prototype_helper'
autoload :RawOutputHelper, 'action_view/helpers/raw_output_helper'
autoload :RecordIdentificationHelper, 'action_view/helpers/record_identification_helper'
autoload :RecordTagHelper, 'action_view/helpers/record_tag_helper'
autoload :SanitizeHelper, 'action_view/helpers/sanitize_helper'
Expand Down Expand Up @@ -45,6 +46,7 @@ module ClassMethods
include JavaScriptHelper
include NumberHelper
include PrototypeHelper
include RawOutputHelper
include RecordIdentificationHelper
include RecordTagHelper
include SanitizeHelper
Expand Down
2 changes: 1 addition & 1 deletion actionpack/lib/action_view/helpers/active_record_helper.rb
Expand Up @@ -290,7 +290,7 @@ def to_time_select_tag(options = {}, html_options = {})
end

def error_wrapping(html_tag, has_error)
has_error ? Base.field_error_proc.call(html_tag, self) : html_tag
has_error ? Base.field_error_proc.call(html_tag, self).html_safe! : html_tag
end

def error_message
Expand Down
4 changes: 2 additions & 2 deletions actionpack/lib/action_view/helpers/asset_tag_helper.rb
Expand Up @@ -285,7 +285,7 @@ def javascript_include_tag(*sources)
end
javascript_src_tag(joined_javascript_name, options)
else
expand_javascript_sources(sources, recursive).collect { |source| javascript_src_tag(source, options) }.join("\n")
expand_javascript_sources(sources, recursive).collect { |source| javascript_src_tag(source, options) }.join("\n").html_safe!
end
end

Expand Down Expand Up @@ -434,7 +434,7 @@ def stylesheet_link_tag(*sources)
end
stylesheet_tag(joined_stylesheet_name, options)
else
expand_stylesheet_sources(sources, recursive).collect { |source| stylesheet_tag(source, options) }.join("\n")
expand_stylesheet_sources(sources, recursive).collect { |source| stylesheet_tag(source, options) }.join("\n").html_safe!
end
end

Expand Down
4 changes: 2 additions & 2 deletions actionpack/lib/action_view/helpers/capture_helper.rb
Expand Up @@ -118,13 +118,13 @@ def capture(*args, &block)
def content_for(name, content = nil, &block)
ivar = "@content_for_#{name}"
content = capture(&block) if block_given?
instance_variable_set(ivar, "#{instance_variable_get(ivar)}#{content}")
instance_variable_set(ivar, "#{instance_variable_get(ivar)}#{content}".html_safe!)
nil
end

# Use an alternate output buffer for the duration of the block.
# Defaults to a new empty string.
def with_output_buffer(buf = '') #:nodoc:
def with_output_buffer(buf = "") #:nodoc:
self.output_buffer, old_buffer = buf, output_buffer
yield
output_buffer
Expand Down
6 changes: 3 additions & 3 deletions actionpack/lib/action_view/helpers/date_helper.rb
Expand Up @@ -916,15 +916,15 @@ def separator(type)

class InstanceTag #:nodoc:
def to_date_select_tag(options = {}, html_options = {})
datetime_selector(options, html_options).select_date
datetime_selector(options, html_options).select_date.html_safe!
end

def to_time_select_tag(options = {}, html_options = {})
datetime_selector(options, html_options).select_time
datetime_selector(options, html_options).select_time.html_safe!
end

def to_datetime_select_tag(options = {}, html_options = {})
datetime_selector(options, html_options).select_datetime
datetime_selector(options, html_options).select_datetime.html_safe!
end

private
Expand Down
4 changes: 2 additions & 2 deletions actionpack/lib/action_view/helpers/form_helper.rb
Expand Up @@ -280,7 +280,7 @@ def form_for(record_or_name_or_array, *args, &proc)

concat(form_tag(options.delete(:url) || {}, options.delete(:html) || {}))
fields_for(object_name, *(args << options), &proc)
concat('</form>')
concat('</form>'.html_safe!)
end

def apply_form_for_options!(object_or_array, options) #:nodoc:
Expand Down Expand Up @@ -797,7 +797,7 @@ def to_check_box_tag(options = {}, checked_value = "1", unchecked_value = "0")
add_default_name_and_id(options)
hidden = tag("input", "name" => options["name"], "type" => "hidden", "value" => options['disabled'] && checked ? checked_value : unchecked_value)
checkbox = tag("input", options)
hidden + checkbox
(hidden + checkbox).html_safe!
end

def to_boolean_select_tag(options = {})
Expand Down
2 changes: 1 addition & 1 deletion actionpack/lib/action_view/helpers/form_options_helper.rb
Expand Up @@ -296,7 +296,7 @@ def options_for_select(container, selected = nil)
options << %(<option value="#{html_escape(value.to_s)}"#{selected_attribute}#{disabled_attribute}>#{html_escape(text.to_s)}</option>)
end

options_for_select.join("\n")
options_for_select.join("\n").html_safe!
end

# Returns a string of option tags that have been compiled by iterating over the +collection+ and assigning the
Expand Down
6 changes: 3 additions & 3 deletions actionpack/lib/action_view/helpers/form_tag_helper.rb
Expand Up @@ -432,7 +432,7 @@ def field_set_tag(legend = nil, options = nil, &block)
concat(tag(:fieldset, options, true))
concat(content_tag(:legend, legend)) unless legend.blank?
concat(content)
concat("</fieldset>")
concat("</fieldset>".html_safe!)
end

private
Expand All @@ -459,14 +459,14 @@ def extra_tags_for_form(html_options)

def form_tag_html(html_options)
extra_tags = extra_tags_for_form(html_options)
tag(:form, html_options, true) + extra_tags
(tag(:form, html_options, true) + extra_tags).html_safe!
end

def form_tag_in_block(html_options, &block)
content = capture(&block)
concat(form_tag_html(html_options))
concat(content)
concat("</form>")
concat("</form>".html_safe!)
end

def token_tag
Expand Down
2 changes: 1 addition & 1 deletion actionpack/lib/action_view/helpers/prototype_helper.rb
Expand Up @@ -393,7 +393,7 @@ def remote_form_for(record_or_name_or_array, *args, &proc)

concat(form_remote_tag(options))
fields_for(object_name, *(args << options), &proc)
concat('</form>')
concat('</form>'.html_safe!)
end
alias_method :form_remote_for, :remote_form_for

Expand Down
9 changes: 9 additions & 0 deletions actionpack/lib/action_view/helpers/raw_output_helper.rb
@@ -0,0 +1,9 @@
module ActionView #:nodoc:
module Helpers #:nodoc:
module RawOutputHelper
def raw(stringish)
stringish.to_s.html_safe!
end
end
end
end
12 changes: 10 additions & 2 deletions actionpack/lib/action_view/helpers/sanitize_helper.rb
Expand Up @@ -49,7 +49,11 @@ module SanitizeHelper
# confuse browsers.
#
def sanitize(html, options = {})
self.class.white_list_sanitizer.sanitize(html, options)
returning self.class.white_list_sanitizer.sanitize(html, options) do |sanitized|
if sanitized
sanitized.html_safe!
end
end
end

# Sanitizes a block of CSS code. Used by +sanitize+ when it comes across a style attribute.
Expand All @@ -72,7 +76,11 @@ def sanitize_css(style)
# strip_tags("<div id='top-bar'>Welcome to my website!</div>")
# # => Welcome to my website!
def strip_tags(html)
self.class.full_sanitizer.sanitize(html)
returning self.class.full_sanitizer.sanitize(html) do |sanitized|
if sanitized
sanitized.html_safe!
end
end
end

# Strips all link tags from +text+ leaving just the link text.
Expand Down
8 changes: 4 additions & 4 deletions actionpack/lib/action_view/helpers/tag_helper.rb
Expand Up @@ -38,7 +38,7 @@ module TagHelper
# tag("img", { :src => "open &amp; shut.png" }, false, false)
# # => <img src="open &amp; shut.png" />
def tag(name, options = nil, open = false, escape = true)
"<#{name}#{tag_options(options, escape) if options}#{open ? ">" : " />"}"
"<#{name}#{tag_options(options, escape) if options}#{open ? ">" : " />"}".html_safe!
end

# Returns an HTML block tag of type +name+ surrounding the +content+. Add
Expand Down Expand Up @@ -91,7 +91,7 @@ def content_tag(name, content_or_options_with_block = nil, options = nil, escape
# cdata_section(File.read("hello_world.txt"))
# # => <![CDATA[<hello from a text file]]>
def cdata_section(content)
"<![CDATA[#{content}]]>"
"<![CDATA[#{content}]]>".html_safe!
end

# Returns an escaped version of +html+ without affecting existing escaped entities.
Expand Down Expand Up @@ -125,7 +125,7 @@ def block_called_from_erb?(block)

def content_tag_string(name, content, options, escape = true)
tag_options = tag_options(options, escape) if options
"<#{name}#{tag_options}>#{content}</#{name}>"
"<#{name}#{tag_options}>#{content}</#{name}>".html_safe!
end

def tag_options(options, escape = true)
Expand All @@ -142,7 +142,7 @@ def tag_options(options, escape = true)
else
attrs = options.map { |key, value| %(#{key}="#{value}") }
end
" #{attrs.sort * ' '}" unless attrs.empty?
" #{attrs.sort * ' '}".html_safe! unless attrs.empty?
end
end
end
Expand Down
6 changes: 3 additions & 3 deletions actionpack/lib/action_view/helpers/url_helper.rb
Expand Up @@ -219,7 +219,7 @@ def link_to(*args, &block)
if block_given?
options = args.first || {}
html_options = args.second
concat(link_to(capture(&block), options, html_options))
concat(link_to(capture(&block), options, html_options).html_safe!)
else
name = args.first
options = args.second || {}
Expand All @@ -237,7 +237,7 @@ def link_to(*args, &block)
end

href_attr = "href=\"#{url}\"" unless href
"<a #{href_attr}#{tag_options}>#{name || url}</a>"
"<a #{href_attr}#{tag_options}>#{name || url}</a>".html_safe!
end
end

Expand Down Expand Up @@ -309,7 +309,7 @@ def button_to(name, options = {}, html_options = {})
html_options.merge!("type" => "submit", "value" => name)

"<form method=\"#{form_method}\" action=\"#{escape_once url}\" class=\"button-to\"><div>" +
method_tag + tag("input", html_options) + request_token_tag + "</div></form>"
method_tag + tag("input", html_options) + request_token_tag + "</div></form>".html_safe!
end


Expand Down
2 changes: 1 addition & 1 deletion actionpack/lib/action_view/partials.rb
Expand Up @@ -221,7 +221,7 @@ def render_partial_collection(options = {}) #:nodoc:
result = template.render_partial(self, object, local_assigns.dup, as)
index += 1
result
end.join(spacer)
end.join(spacer).html_safe!
end

def _pick_partial_template(partial_path) #:nodoc:
Expand Down
28 changes: 28 additions & 0 deletions actionpack/lib/action_view/safe_buffer.rb
@@ -0,0 +1,28 @@

module ActionView #:nodoc:
class SafeBuffer < String
def <<(value)
if value.html_safe?
super(value)
else
super(ERB::Util.h(value))
end
end

def concat(value)
self << value
end

def html_safe?
true
end

def html_safe!
self
end

def to_s
self
end
end
end
12 changes: 12 additions & 0 deletions actionpack/test/template/asset_tag_helper_test.rb
Expand Up @@ -164,6 +164,11 @@ def test_javascript_include_tag_with_given_asset_id
assert_dom_equal(%(<script src="/javascripts/prototype.js?1" type="text/javascript"></script>\n<script src="/javascripts/effects.js?1" type="text/javascript"></script>\n<script src="/javascripts/dragdrop.js?1" type="text/javascript"></script>\n<script src="/javascripts/controls.js?1" type="text/javascript"></script>\n<script src="/javascripts/application.js?1" type="text/javascript"></script>), javascript_include_tag(:defaults))
end

def test_javascript_include_tag_is_html_safe
assert javascript_include_tag(:defaults).html_safe?
assert javascript_include_tag("prototype").html_safe?
end

def test_register_javascript_include_default
ENV["RAILS_ASSET_ID"] = ""
ActionView::Helpers::AssetTagHelper::register_javascript_include_default 'slider'
Expand Down Expand Up @@ -206,6 +211,13 @@ def test_stylesheet_link_tag
StyleLinkToTag.each { |method, tag| assert_dom_equal(tag, eval(method)) }
end

def test_stylesheet_link_tag_is_html_safe
ENV["RAILS_ASSET_ID"] = ""
assert stylesheet_link_tag('dir/file').html_safe?
assert stylesheet_link_tag('dir/other/file', 'dir/file2').html_safe?
assert stylesheet_tag('dir/file', {}).html_safe?
end

def test_custom_stylesheet_expansions
ActionView::Helpers::AssetTagHelper::register_stylesheet_expansion :monkey => ["head", "body", "tail"]
assert_dom_equal %(<link href="/stylesheets/first.css" media="screen" rel="stylesheet" type="text/css" />\n<link href="/stylesheets/head.css" media="screen" rel="stylesheet" type="text/css" />\n<link href="/stylesheets/body.css" media="screen" rel="stylesheet" type="text/css" />\n<link href="/stylesheets/tail.css" media="screen" rel="stylesheet" type="text/css" />\n<link href="/stylesheets/last.css" media="screen" rel="stylesheet" type="text/css" />), stylesheet_link_tag('first', :monkey, 'last')
Expand Down
2 changes: 1 addition & 1 deletion actionpack/test/template/form_helper_test.rb
Expand Up @@ -1060,7 +1060,7 @@ class LabelledFormBuilder < ActionView::Helpers::FormBuilder
(field_helpers - %w(hidden_field)).each do |selector|
src = <<-END_SRC
def #{selector}(field, *args, &proc)
"<label for='\#{field}'>\#{field.to_s.humanize}:</label> " + super + "<br/>"
("<label for='\#{field}'>\#{field.to_s.humanize}:</label> " + super + "<br/>").html_safe!
end
END_SRC
class_eval src, __FILE__, __LINE__
Expand Down
21 changes: 21 additions & 0 deletions actionpack/test/template/raw_output_helper_test.rb
@@ -0,0 +1,21 @@
require 'abstract_unit'
require 'testing_sandbox'

class RawOutputHelperTest < ActionView::TestCase
tests ActionView::Helpers::RawOutputHelper
include TestingSandbox

def setup
@string = "hello"
end

test "raw returns the safe string" do
result = raw(@string)
assert_equal @string, result
assert result.html_safe?
end

test "raw handles nil values correctly" do
assert_equal "", raw(nil)
end
end
11 changes: 10 additions & 1 deletion actionpack/test/template/sanitize_helper_test.rb
Expand Up @@ -39,7 +39,16 @@ def test_strip_tags
%{This is a test.\n\n\nIt no longer contains any HTML.\n}, strip_tags(
%{<title>This is <b>a <a href="" target="_blank">test</a></b>.</title>\n\n<!-- it has a comment -->\n\n<p>It no <b>longer <strong>contains <em>any <strike>HTML</strike></em>.</strong></b></p>\n}))
assert_equal "This has a here.", strip_tags("This has a <!-- comment --> here.")
[nil, '', ' '].each { |blank| assert_equal blank, strip_tags(blank) }
[nil, '', ' '].each do |blank|
stripped = strip_tags(blank)
assert_equal blank, stripped
assert stripped.html_safe? unless blank.nil?
end
assert strip_tags("<script>").html_safe?
end

def test_sanitize_is_marked_safe
assert sanitize("<html><script></script></html>").html_safe?
end

def assert_sanitized(text, expected = nil)
Expand Down
1 change: 1 addition & 0 deletions actionpack/test/template/tag_helper_test.rb
Expand Up @@ -34,6 +34,7 @@ def test_tag_options_converts_boolean_option

def test_content_tag
assert_equal "<a href=\"create\">Create</a>", content_tag("a", "Create", "href" => "create")
assert content_tag("a", "Create", "href" => "create").html_safe?
assert_equal content_tag("a", "Create", "href" => "create"),
content_tag("a", "Create", :href => "create")
end
Expand Down

0 comments on commit 80da8eb

Please sign in to comment.