In this proposal every method which is considered as "unsafe" for
`ActiveSupport::SafeBuffer` is going to be deprecated, both bang and
non-destructive versions.
At first glance, it seems that this would violate Liskov substitution
principle, because `ActiveSupport::SafeBuffer` is a subclass of `String`.
However, let's step back and remember what's the purpose of safe strings.
Safebuffer strings were introduced in Rails 3 in order to distinguish
between strings that are already sanitized, that don't need to be sanitized
and non-safe strings. Consider this ERB template:
<div>Hello!</div>
<%= params[:evil_input] %>
<%= link_to params[:evil_input], users_path %>
The first string is static therefore it is completely safe and shouldn't be
sanitized at all.
The second one is definitely unsafe and should be sanitized.
The third one is made by Rails helper which returns safe strings and
shouldn't be sanitized. However, parameter itself is unsafe and should be
sanitized by Rails before wrapping it into <a></a>.
When ERB is compiled into a Ruby method, it wraps each dynamic string into
a sanitizing method call, which in turn must have a way to distinguish
between second and third row. This is when Safebuffers are really used.
They respond with +true+ to +html_safe?+, while other strings and objects
(except numbers) respond with +false+.
So, this is completely a view concept and even could be considered as a
part of ActionView private API, because nobody uses safebuffer to pass data
throughout whole application. SafeBuffers were not designed to enhance Ruby
String in some way that other parsing/converter libraries could benefit
from. As for subclassing String, [here][yehudakatz] explains that this was
performance only decision ("Because SafeBuffer inherits from String, Ruby
creates this wrapper extremely efficiently (just sharing the internal char
* storage).") Should we bother if some methods from String are missing then?
Back to modifications and "unsafe" methods. It turned out that if we would
perform, for example, +gsub+ on already sanitized string that was wrapped
into a SafeBuffer, we couldn't call it safe anymore. This is because one
could revert any changes made by a sanitizer by another series of
+gsub("<", "<")+ etc. So the decision was made to return raw string for
non-destructive versions of such unsafe methods and mark SafeBuffer as
non-safe for destructive (bang) ones.
As for +gsub!+ and its friends. Not only "unsafe SafeBuffer" sounds
ridiculous, but this highly complicated the class implementation! Let's
look how it could look like:
module ActiveSupport #:nodoc:
class SafeBuffer < String
UNSAFE_STRING_METHODS = %w(
capitalize chomp chop delete downcase gsub lstrip next reverse rstrip
slice squeeze strip sub succ swapcase tr tr_s upcase
)
alias_method :safe_concat, :concat
class SafeConcatError < StandardError
def initialize
super 'Could not concatenate to the buffer because it is not html
safe.'
end
end
def initialize_copy(other)
raise SafeConcatError unless other.html_safe?
super
end
def clone_empty
self[0, 0]
end
def concat(value)
super(ERB.unwrapped_html_escape(value))
end
alias << concat
def prepend(value)
super(ERB.unwrapped_html_escape(value))
end
def +(other)
dup.concat(other)
end
def %(args)
case args
when Hash
escaped_args = Hash[args.map { |k,arg| [k,
ERB.unwrapped_html_escape(arg)] }]
else
escaped_args = Array(args).map { |arg|
ERB.unwrapped_html_escape(arg) }
end
self.class.new(super(escaped_args))
end
def html_safe?
true
end
def to_s
self
end
alias_method :html_safe, :to_s
def to_param
to_str
end
def encode_with(coder)
coder.represent_scalar nil, to_str
end
UNSAFE_STRING_METHODS.each do |unsafe_method|
if unsafe_method.respond_to?(unsafe_method)
class_eval <<-EOT, __FILE__, __LINE__ + 1
def #{unsafe_method}(*args, &block)
raise NoMethodError
end
def #{unsafe_method}!(*args)
raise NoMethodError
end
EOT
end
end
end
end
Now, aint' that nice? [Here][output_safety] you can compare it with the
current implementation. By declaring safebuffers *always* safe we make it
easier to understand, and we have less code to perform!
([see][tenderlove_rant] for more information about +#safe_concat+). We also
always return +self+ for +html_safe+ (there were a
[couple][html_safe_pull_1] rejected [approaches][html_safe_pull_2] to
enhance that).
One would agree that destructive modifications is true evil, but what's
wrong with +gsub+? Well, first of all, it [doesn't work as
expected][broken_gsub]. [Here][deprecate_gsub_block] is a proposal to fix
that, but we see that this doesn't solve the problem entirely. Moreover, it
leads to writing less performant and (possibly) vulnerable code. Consider
this example code:
user_input = ERB.unwrapped_html_escape(params[:evil_input]
string_that_is_going_to_view = "<div>#{user_input}</div>".html_safe
This code is 100% safe and efficient. Suddenly, a programmer decides to
change something
patched_string = string_that_is_going_to_view.gsub(",", ".")
Now, his "<div"> is escaped by ERB, this is no good. What's the solution?
Throw another +html_safe+ !
patched_string = string_that_is_going_to_view.gsub(",", ".").html_safe
Wha'ts wrong with this code? First of all, it creates two SafeBuffer
objects while first is disposed immediately. [Here][tenderlove_unwrapped]
@tenderlove clearly states that this is wrong approach and every object
counts! Next, it can possibly lead to XSS attack. Consider this variant of
code above:
patched_string = string_that_is_going_to_view.gsub("[", "<").gsub("]",
">").html_safe
Now, even if +user_input+ was previously sanitized it can be turned into
unsafe again.
One could ask "isn't it a user decision to take care of such situations?" I
would answer that Rails is opinionated and should force a user to write
better code. When I turned deprecation warnings on all unsafe methods and
run Rails test suite, I saw *a lot* of deprecations on my screen. There're
even specially written tests that checks if SafeBuffer works correctly with
+#titleize+ and its family (which obviously modies the passed string). Not
to mention that Rails views itself use +gsub!+ as well.
So, if those methods are being deprecated what should programmer do? He/she
must realise that any change should be made *before* wrapping a string into
Safebuffer. This would be fast and efficient. If it's impossible, then call
+to_str+, do a modification and instantiate a new safebuffer. No changes
with current approach, except it is explicit now (so a user would feel the
pain and maybe decide to rewrite the code). For a library writer (who
doesn't know beforehand what kind of data will be passed into a method) I
would propose ActiveSupport to have monkeypatched +Kernel.String+ method,
so it would return string from a safebuffer, otherwise call +to_s_ as usual.
[yehudakatz]: http://yehudakatz.com/2010/02/01/safebuffers-and-rails-3-0/
[output_safety]:
https://github.com/rails/rails/blob/master/activesupport/lib/active_support/core_ext/string/output_safety.rb#L131
[tenderlove_rant]:
http://tenderlovemaking.com/2014/06/04/yagni-methods-slow-us-down.html
[html_safe_pull_1]: https://github.com/rails/rails/pull/17206
[html_safe_pull_2]: https://github.com/rails/rails/pull/17250
[broken_gsub]: https://github.com/rails/rails/issues/1555
[deprecate_gsub_block]: https://github.com/rails/rails/pull/16957
[tenderlove_unwrapped]:
https://github.com/rails/rails/commit/8899503f62a556a918c45b3e7b5c2effaaa943f4
--
You received this message because you are subscribed to the Google Groups "Ruby
on Rails: Core" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to rubyonrails-core+unsubscr...@googlegroups.com.
To post to this group, send email to rubyonrails-core@googlegroups.com.
Visit this group at http://groups.google.com/group/rubyonrails-core.
For more options, visit https://groups.google.com/d/optout.
