Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Escape attributes regardless of whether it's SafeBuffer or not #1028

Merged
merged 2 commits into from
Jul 15, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/haml/attribute_builder.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,9 @@ def build_attributes(is_html, attr_wrapper, escape_attrs, hyphenate_data_attrs,

value =
if escape_attrs == :once
Haml::Helpers.escape_once(value.to_s)
Haml::Helpers.escape_once_without_haml_xss(value.to_s)
elsif escape_attrs
Haml::Helpers.html_escape(value.to_s)
Haml::Helpers.html_escape_without_haml_xss(value.to_s)
else
value.to_s
end
Expand Down
4 changes: 2 additions & 2 deletions lib/haml/attribute_compiler.rb
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ def compile_id_or_class_attribute(id_or_class, values)
['false, nil', [:multi]],
[:else, [:multi,
[:static, " #{id_or_class}=#{@attr_wrapper}"],
[:escape, @escape_attrs, [:dynamic, var]],
[:escape, Escapable::EscapeSafeBuffer.new(@escape_attrs), [:dynamic, var]],
[:static, @attr_wrapper]],
]
],
Expand All @@ -191,7 +191,7 @@ def compile_common_attribute(key, values)
['false, nil', [:multi]],
[:else, [:multi,
[:static, " #{key}=#{@attr_wrapper}"],
[:escape, @escape_attrs, [:dynamic, var]],
[:escape, Escapable::EscapeSafeBuffer.new(@escape_attrs), [:dynamic, var]],
[:static, @attr_wrapper]],
]
],
Expand Down
49 changes: 38 additions & 11 deletions lib/haml/escapable.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,30 +4,31 @@ module Haml
# Like Temple::Filters::Escapable, but with support for escaping by
# Haml::Herlpers.html_escape and Haml::Herlpers.escape_once.
class Escapable < Temple::Filter
k0kubun marked this conversation as resolved.
Show resolved Hide resolved
# Special value of `flag` to ignore html_safe?
EscapeSafeBuffer = Struct.new(:value)

def initialize(*)
super
@escape_code = "::Haml::Helpers.html_escape((%s))"
@escaper = eval("proc {|v| #{@escape_code % 'v'} }")
@once_escape_code = "::Haml::Helpers.escape_once((%s))"
@once_escaper = eval("proc {|v| #{@once_escape_code % 'v'} }")
@escape = false
@escape_safe_buffer = false
end

def on_escape(flag, exp)
old = @escape
@escape = flag
old_escape, old_escape_safe_buffer = @escape, @escape_safe_buffer
@escape_safe_buffer = flag.is_a?(EscapeSafeBuffer)
@escape = @escape_safe_buffer ? flag.value : flag
compile(exp)
ensure
@escape = old
@escape, @escape_safe_buffer = old_escape, old_escape_safe_buffer
end

# The same as Haml::AttributeBuilder.build_attributes
def on_static(value)
[:static,
if @escape == :once
@once_escaper[value]
escape_once(value)
elsif @escape
@escaper[value]
escape(value)
else
value
end
Expand All @@ -38,13 +39,39 @@ def on_static(value)
def on_dynamic(value)
[:dynamic,
if @escape == :once
@once_escape_code % value
escape_once_code(value)
elsif @escape
@escape_code % value
escape_code(value)
else
"(#{value}).to_s"
end
]
end

private

def escape_once(value)
if @escape_safe_buffer
::Haml::Helpers.escape_once_without_haml_xss(value)
else
::Haml::Helpers.escape_once(value)
end
end

def escape(value)
if @escape_safe_buffer
::Haml::Helpers.html_escape_without_haml_xss(value)
else
::Haml::Helpers.html_escape(value)
end
end

def escape_once_code(value)
"::Haml::Helpers.escape_once#{('_without_haml_xss' if @escape_safe_buffer)}((#{value}))"
end

def escape_code(value)
"::Haml::Helpers.html_escape#{('_without_haml_xss' if @escape_safe_buffer)}((#{value}))"
end
end
end
8 changes: 7 additions & 1 deletion lib/haml/helpers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -607,9 +607,12 @@ def haml_tag_if(condition, *tag)
# @param text [String] The string to sanitize
# @return [String] The sanitized string
def html_escape(text)
ERB::Util.html_escape(text)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ActiveSupport monkey-patches this method for html_safe?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely like seeing the CGI version vs leaning on ActiveSupport

CGI.escapeHTML(text.to_s)
end

# Always escape text regardless of html_safe?
alias_method :html_escape_without_haml_xss, :html_escape

HTML_ESCAPE_ONCE_REGEX = /['"><]|&(?!(?:[a-zA-Z]+|#(?:\d+|[xX][0-9a-fA-F]+));)/

# Escapes HTML entities in `text`, but without escaping an ampersand
Expand All @@ -622,6 +625,9 @@ def escape_once(text)
text.gsub(HTML_ESCAPE_ONCE_REGEX, HTML_ESCAPE)
end

# Always escape text once regardless of html_safe?
alias_method :escape_once_without_haml_xss, :escape_once

# Returns whether or not the current template is a Haml template.
#
# This function, unlike other {Haml::Helpers} functions,
Expand Down
9 changes: 6 additions & 3 deletions lib/haml/helpers/xss_mods.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,15 @@ module Helpers
# to work with Rails' XSS protection methods.
module XssMods
def self.included(base)
%w[html_escape find_and_preserve preserve list_of surround
precede succeed capture_haml haml_concat haml_internal_concat haml_indent
escape_once].each do |name|
%w[find_and_preserve preserve list_of surround
precede succeed capture_haml haml_concat haml_internal_concat haml_indent].each do |name|
base.send(:alias_method, "#{name}_without_haml_xss", name)
base.send(:alias_method, name, "#{name}_with_haml_xss")
end
# Those two always have _without_haml_xss
%w[html_escape escape_once].each do |name|
base.send(:alias_method, name, "#{name}_with_haml_xss")
end
end

# Don't escape text that's already safe,
Expand Down
4 changes: 4 additions & 0 deletions test/results/escape_safe_buffer.xhtml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
<div data-html='&lt;foo&gt;bar&lt;/foo&gt;'></div>
<meta content='&#39;&quot;' />
<meta content='&#39;&quot;' />
<meta content='&#39;&quot;' />
4 changes: 3 additions & 1 deletion test/template_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,9 @@ def test_xss_protection_in_attributes
end

def test_xss_protection_in_attributes_with_safe_strings
assert_equal("<div data-html='<foo>bar</foo>'></div>\n", render('%div{ "data-html" => "<foo>bar</foo>".html_safe }', :action_view))
assert_renders_correctly('escape_safe_buffer') do |name|
render(File.read(File.expand_path("templates/#{name}.haml", __dir__)), :action_view)
end
end

def test_xss_protection_with_bang_in_interpolation
Expand Down
6 changes: 6 additions & 0 deletions test/templates/escape_safe_buffer.haml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
%div{ 'data-html' => '<foo>bar</foo>'.html_safe }
%meta{ content: %{'"}.html_safe }
- val = %{'"}.html_safe
%meta{ content: val }
- hash = { content: val }
%meta{ hash }