1
0
Fork 0
mirror of https://github.com/rails/rails.git synced 2022-11-09 12:12:34 -05:00

Switch to on-by-default XSS escaping for rails.

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)
  * New ERB implementation based on erubis which uses a SafeBuffer instead of a String

Hat tip to Django for the inspiration.
This commit is contained in:
Michael Koziarski 2009-10-08 09:31:20 +13:00
parent f27e7ebc0e
commit 9415935902
36 changed files with 368 additions and 42 deletions

View file

@ -4,6 +4,7 @@ gem "rack", "~> 1.0.0"
gem "rack-test", "~> 0.5.0"
gem "activesupport", "3.0.pre", :vendored_at => rails_root.join("activesupport")
gem "activemodel", "3.0.pre", :vendored_at => rails_root.join("activemodel")
gem "erubis", "~> 2.6.0"
only :test do
gem "mocha"

View file

@ -44,11 +44,11 @@ module ActionView
autoload :TextTemplate, 'action_view/template/text'
autoload :Helpers, 'action_view/helpers'
autoload :FileSystemResolverWithFallback, 'action_view/template/resolver'
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"

View file

@ -260,7 +260,7 @@ module ActionView #:nodoc:
@assigns = assigns_for_first_render.each { |key, value| instance_variable_set("@#{key}", value) }
@controller = controller
@helpers = self.class.helpers || Module.new
@_content_for = Hash.new {|h,k| h[k] = "" }
@_content_for = Hash.new {|h,k| h[k] = ActionView::SafeBuffer.new }
self.view_paths = view_paths
end

View file

@ -15,9 +15,19 @@ class ERB
# puts html_escape("is a > 0 & a < 10?")
# # => is a &gt; 0 &amp; a &lt; 10?
def html_escape(s)
s.to_s.gsub(/[&"><]/) { |special| HTML_ESCAPE[special] }
s = s.to_s
if s.html_safe?
s
else
s.gsub(/[&"><]/) { |special| HTML_ESCAPE[special] }.html_safe!
end
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>.
#

View file

@ -15,6 +15,7 @@ module ActionView #: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'
@ -46,6 +47,7 @@ module ActionView #:nodoc:
include JavaScriptHelper
include NumberHelper
include PrototypeHelper
include RawOutputHelper
include RecordIdentificationHelper
include RecordTagHelper
include SanitizeHelper

View file

@ -91,6 +91,7 @@ module ActionView
yield contents if block_given?
contents << submit_tag(submit_value)
contents << '</form>'
contents.html_safe!
end
# Returns a string containing the error message attached to the +method+ on the +object+ if one exists.

View file

@ -289,7 +289,7 @@ module ActionView
else
sources = expand_javascript_sources(sources, recursive)
ensure_javascript_sources!(sources) if cache
sources.collect { |source| javascript_src_tag(source, options) }.join("\n")
sources.collect { |source| javascript_src_tag(source, options) }.join("\n").html_safe!
end
end
@ -440,7 +440,7 @@ module ActionView
else
sources = expand_stylesheet_sources(sources, recursive)
ensure_stylesheet_sources!(sources) if cache
sources.collect { |source| stylesheet_tag(source, options) }.join("\n")
sources.collect { |source| stylesheet_tag(source, options) }.join("\n").html_safe!
end
end
@ -584,7 +584,7 @@ module ActionView
if sources.is_a?(Array)
content_tag("video", options) do
sources.map { |source| tag("source", :src => source) }.join
sources.map { |source| tag("source", :src => source) }.join.html_safe!
end
else
options[:src] = path_to_video(sources)

View file

@ -143,7 +143,7 @@ module ActionView
# Defaults to a new empty string.
def with_output_buffer(buf = nil) #:nodoc:
unless buf
buf = ''
buf = ActionView::SafeBuffer.new
buf.force_encoding(output_buffer.encoding) if buf.respond_to?(:force_encoding)
end
self.output_buffer, old_buffer = buf, output_buffer

View file

@ -916,15 +916,15 @@ module ActionView
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

View file

@ -282,7 +282,7 @@ module ActionView
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:
@ -809,7 +809,7 @@ module ActionView
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 = {})

View file

@ -296,7 +296,7 @@ module ActionView
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

View file

@ -440,7 +440,7 @@ module ActionView
concat(tag(:fieldset, options, true))
concat(content_tag(:legend, legend)) unless legend.blank?
concat(content)
concat("</fieldset>")
concat("</fieldset>".html_safe!)
end
private
@ -467,14 +467,14 @@ module ActionView
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

View file

@ -395,7 +395,7 @@ module ActionView
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

View file

@ -0,0 +1,9 @@
module ActionView #:nodoc:
module Helpers #:nodoc:
module RawOutputHelper
def raw(stringish)
stringish.to_s.html_safe!
end
end
end
end

View file

@ -49,7 +49,11 @@ module ActionView
# 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.
@ -72,7 +76,11 @@ module ActionView
# 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.

View file

@ -41,7 +41,7 @@ module ActionView
# 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
@ -94,7 +94,7 @@ module ActionView
# 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.
@ -128,7 +128,7 @@ module ActionView
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)
@ -143,7 +143,7 @@ module ActionView
attrs << %(#{key}="#{final_value}")
end
end
" #{attrs.sort * ' '}" unless attrs.empty?
" #{attrs.sort * ' '}".html_safe! unless attrs.empty?
end
end
end

View file

@ -93,7 +93,7 @@ module ActionView
polymorphic_path(options)
end
escape ? escape_once(url) : url
(escape ? escape_once(url) : url).html_safe!
end
# Creates a link tag of the given +name+ using a URL created by the set
@ -220,7 +220,7 @@ module ActionView
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[0]
options = args[1] || {}
@ -238,7 +238,7 @@ module ActionView
end
href_attr = "href=\"#{url}\"" unless href
"<a #{href_attr}#{tag_options}>#{name || url}</a>"
"<a #{href_attr}#{tag_options}>#{ERB::Util.h(name || url)}</a>".html_safe!
end
end
@ -309,8 +309,8 @@ module ActionView
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>"
("<form method=\"#{form_method}\" action=\"#{escape_once url}\" class=\"button-to\"><div>" +
method_tag + tag("input", html_options) + request_token_tag + "</div></form>").html_safe!
end

View file

@ -223,7 +223,7 @@ module ActionView
end
result = template ? collection_with_template(template) : collection_without_template
result.join(spacer)
result.join(spacer).html_safe!
end
def collection_with_template(template)

View file

@ -0,0 +1,28 @@
module ActionView #:nodoc:
class SafeBuffer < String
def <<(value)
if value.html_safe?
super(value)
else
super(CGI.escapeHTML(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

View file

@ -1,7 +1,31 @@
require 'active_support/core_ext/class/attribute_accessors'
require 'active_support/core_ext/string/output_safety'
require 'erubis'
module ActionView
module TemplateHandlers
class Erubis < ::Erubis::Eruby
def add_preamble(src)
src << "@output_buffer = ActionView::SafeBuffer.new;"
end
def add_text(src, text)
src << "@output_buffer << ('" << escape_text(text) << "'.html_safe!);"
end
def add_expr_literal(src, code)
src << '@output_buffer << ((' << code << ').to_s);'
end
def add_expr_escaped(src, code)
src << '@output_buffer << ' << escaped_expr(code) << ';'
end
def add_postamble(src)
src << '@output_buffer.to_s'
end
end
class ERB < TemplateHandler
include Compilable
@ -15,11 +39,9 @@ module ActionView
self.default_format = Mime::HTML
def compile(template)
require 'erb'
magic = $1 if template.source =~ /\A(<%#.*coding[:=]\s*(\S+)\s*-?%>)/
erb = "#{magic}<% __in_erb_template=true %>#{template.source}"
::ERB.new(erb, nil, erb_trim_mode, '@output_buffer').src
Erubis.new(erb, :trim=>(self.class.erb_trim_mode == "-")).src
end
end
end

View file

@ -55,7 +55,7 @@ module ActionView
setup :setup_with_controller
def setup_with_controller
@controller = TestController.new
@output_buffer = ''
@output_buffer = ActionView::SafeBuffer.new
@rendered = ''
self.class.send(:include_helper_modules!)

View file

@ -0,0 +1,19 @@
require 'abstract_unit'
class OutputEscapingTest < ActiveSupport::TestCase
test "escape_html shouldn't die when passed nil" do
assert ERB::Util.h(nil).blank?
end
test "escapeHTML should escape strings" do
assert_equal "&lt;&gt;&quot;", ERB::Util.h("<>\"")
end
test "escapeHTML shouldn't touch explicitly safe strings" do
# TODO this seems easier to compose and reason about, but
# this should be verified
assert_equal "<", ERB::Util.h("<".html_safe!)
end
end

View file

@ -1050,7 +1050,7 @@ class RenderTest < ActionController::TestCase
def test_action_talk_to_layout
get :action_talk_to_layout
assert_equal "<title>Talking to the layout</title>\nAction was here!", @response.body
assert_equal "<title>Talking to the layout</title>\n\nAction was here!", @response.body
end
# :addressed:

View file

@ -231,6 +231,11 @@ class AssetTagHelperTest < ActionView::TestCase
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 'bank'
@ -285,6 +290,13 @@ class AssetTagHelperTest < ActionView::TestCase
}
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
ENV["RAILS_ASSET_ID"] = ''
ActionView::Helpers::AssetTagHelper::register_stylesheet_expansion :robbery => ["bank", "robber"]

View file

@ -15,6 +15,18 @@ class ErbUtilTest < Test::Unit::TestCase
end
end
def test_html_escape_is_html_safe
escaped = h("<p>")
assert_equal "&lt;p&gt;", escaped
assert escaped.html_safe?
end
def test_html_escape_passes_html_escpe_unmodified
escaped = h("<p>".html_safe!)
assert_equal "<p>", escaped
assert escaped.html_safe?
end
def test_rest_in_ascii
(0..127).to_a.map {|int| int.chr }.each do |chr|
next if %w(& " < >).include?(chr)

View file

@ -974,7 +974,7 @@ class FormHelperTest < ActionView::TestCase
(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__

View file

@ -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

View file

@ -229,7 +229,7 @@ module RenderTestCases
end
def test_render_with_nested_layout
assert_equal %(<title>title</title>\n<div id="column">column</div>\n<div id="content">content</div>\n),
assert_equal %(<title>title</title>\n\n\n<div id="column">column</div>\n<div id="content">content</div>\n),
@view.render(:file => "test/nested_layout.erb", :layout => "layouts/yield")
end

View file

@ -39,7 +39,16 @@ class SanitizeHelperTest < ActionView::TestCase
%{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)

View file

@ -34,6 +34,7 @@ class TagHelperTest < ActionView::TestCase
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

View file

@ -155,7 +155,7 @@ module ActionView
class AssertionsTest < ActionView::TestCase
def render_from_helper
form_tag('/foo') do
concat render(:text => '<ul><li>foo</li></ul>')
concat render(:text => '<ul><li>foo</li></ul>').html_safe!
end
end
helper_method :render_from_helper

View file

@ -139,7 +139,7 @@ class UrlHelperTest < ActionView::TestCase
end
def test_link_tag_with_img
assert_dom_equal "<a href=\"http://www.example.com\"><img src='/favicon.jpg' /></a>", link_to("<img src='/favicon.jpg' />", "http://www.example.com")
assert_dom_equal "<a href=\"http://www.example.com\"><img src='/favicon.jpg' alt=\"Favicon\" /></a>", link_to(image_tag("/favicon.jpg"), "http://www.example.com")
end
def test_link_with_nil_html_options

View file

@ -0,0 +1,41 @@
require 'abstract_unit'
class SafeBufferTest < ActionView::TestCase
def setup
@buffer = ActionView::SafeBuffer.new
end
test "Should look like a string" do
assert @buffer.is_a?(String)
assert_equal "", @buffer
end
test "Should escape a raw string which is passed to them" do
@buffer << "<script>"
assert_equal "&lt;script&gt;", @buffer
end
test "Should NOT escape a safe value passed to it" do
@buffer << "<script>".html_safe!
assert_equal "<script>", @buffer
end
test "Should not mess with an innocuous string" do
@buffer << "Hello"
assert_equal "Hello", @buffer
end
test "Should not mess with a previously escape test" do
@buffer << CGI.escapeHTML("<script>")
assert_equal "&lt;script&gt;", @buffer
end
test "Should be considered safe" do
assert @buffer.html_safe?
end
test "Should return a safe buffer when calling to_s" do
new_buffer = @buffer.to_s
assert_equal ActionView::SafeBuffer, new_buffer.class
end
end

View file

@ -7,4 +7,5 @@ require 'active_support/core_ext/string/access'
require 'active_support/core_ext/string/iterators'
require 'active_support/core_ext/string/xchar'
require 'active_support/core_ext/string/behavior'
require 'active_support/core_ext/string/interpolation'
require 'active_support/core_ext/string/interpolation'
require 'active_support/core_ext/string/output_safety'

View file

@ -0,0 +1,43 @@
class String
def html_safe?
defined?(@_rails_html_safe) && @_rails_html_safe
end
def html_safe!
@_rails_html_safe = true
self
end
def html_safe
dup.html_safe!
end
alias original_plus +
def +(other)
result = original_plus(other)
if html_safe? && also_html_safe?(other)
result.html_safe!
else
result
end
end
alias original_concat <<
def <<(other)
result = original_concat(other)
unless html_safe? && also_html_safe?(other)
@_rails_html_safe = false
end
result
end
def concat(other)
self << other
end
private
def also_html_safe?(other)
other.respond_to?(:html_safe?) && other.html_safe?
end
end

View file

@ -356,3 +356,89 @@ class StringBytesizeTest < Test::Unit::TestCase
assert_equal 3, 'foo'.bytesize
end
end
class OutputSafetyTest < ActiveSupport::TestCase
def setup
@string = "hello"
end
test "A string is unsafe by default" do
assert !@string.html_safe?
end
test "A string can be marked safe" do
@string.html_safe!
assert @string.html_safe?
end
test "Marking a string safe returns the string" do
assert_equal @string, @string.html_safe!
end
test "Adding a safe string to another safe string returns a safe string" do
@other_string = "other".html_safe!
@string.html_safe!
@combination = @other_string + @string
assert_equal "otherhello", @combination
assert @combination.html_safe?
end
test "Adding an unsafe string to a safe string returns an unsafe string" do
@other_string = "other".html_safe!
@combination = @other_string + @string
@other_combination = @string + @other_string
assert_equal "otherhello", @combination
assert_equal "helloother", @other_combination
assert !@combination.html_safe?
assert !@other_combination.html_safe?
end
test "Concatting safe onto unsafe yields unsafe" do
@other_string = "other"
@string.html_safe!
@other_string.concat(@string)
assert !@other_string.html_safe?
end
test "Concatting unsafe onto safe yields unsafe" do
@other_string = "other".html_safe!
@other_string.concat(@string)
assert !@other_string.html_safe?
end
test "Concatting safe onto safe yields safe" do
@other_string = "other".html_safe!
@string.html_safe!
@other_string.concat(@string)
assert @other_string.html_safe?
end
test "Concatting safe onto unsafe with << yields unsafe" do
@other_string = "other"
@string.html_safe!
@other_string << @string
assert !@other_string.html_safe?
end
test "Concatting unsafe onto safe with << yields unsafe" do
@other_string = "other".html_safe!
@other_string << @string
assert !@other_string.html_safe?
end
test "Concatting safe onto safe with << yields safe" do
@other_string = "other".html_safe!
@string.html_safe!
@other_string << @string
assert @other_string.html_safe?
end
end