Handle nil in Rack::Utils.escape_html#2202
Conversation
ioquatix
left a comment
There was a problem hiding this comment.
I'm okay with this, but I'd like @jeremyevans's opinion on it.
jeremyevans
left a comment
There was a problem hiding this comment.
I would like to avoid the slowdown, and I think I have a way (at least for some Ruby versions).
fc474c2 to
2b2b6e7
Compare
|
Are you happy that |
jeremyevans
left a comment
There was a problem hiding this comment.
Looks good, one small change requested.
|
I was about to post some numbers (:
Benchmark script
require 'benchmark/ips'
require 'cgi'
require 'erb'
# Generated by ChatGPT
HTML_EXAMPLES = <<HTML.lines(chomp: true)
<p>It's a beautiful day.</p>
<a href="https://example.com/page?id=123&category=5">Link</a>
<button onclick="alert('Hello, world!')">Click me</button>
<input type="text" value="It's my text">
<div class='container'>Content goes here</div>
<span>5 × 10 = 50</span>
<img src='image.jpg' alt="Nature's beauty">
<p style='color: red;'>This text is red.</p>
<a href='javascript:alert("XSS attack!");'>Click here</a>
<input type='text' placeholder='Enter your name & email'>
HTML
HTML_NO_ESCAPING = <<HTML.lines(chomp: true)
Lorem ipsum dolor sit amet, consectetur adipiscing elit,
sed do eiusmod tempor incididunt ut labore et dolore magna
aliqua. Ut enim ad minim veniam, quis nostrud exercitation
ullamco laboris nisi ut aliquip ex ea commodo consequat.
Duis aute irure dolor in reprehenderit in voluptate velit
esse cillum dolore eu fugiat nulla pariatur. Excepteur sint
occaecat cupidatat non proident, sunt in culpa qui officia
deserunt mollit anim id est laborum. Lorem ipsum dolor sit
amet, consectetur adipiscing elit, sed do eiusmod tempor
incididunt ut labore et dolore magna aliqua. Ut enim ad
HTML
targets = (HTML_EXAMPLES * 10).shuffle
target_no_escaping = (HTML_NO_ESCAPING * 10).shuffle
num_iteration = 10_000
module Utils
module_function
define_method(:escape_html_current, CGI.method(:escapeHTML))
define_method(:escape_html_new, ERB::Escape.instance_method(:html_escape))
def escape_html_new_fallback(string)
CGI.escapeHTML(string.to_s)
end
def escape_html_to_s(string)
CGI.escapeHTML(string.to_s)
end
end
Benchmark.ips do |x|
x.report "current" do
num_iteration.times do
targets.each do |string|
Utils.escape_html_current(string)
end
end
end
x.report "to_s" do
num_iteration.times do
targets.each do |string|
Utils.escape_html_to_s(string)
end
end
end
x.report "erb" do
num_iteration.times do
targets.each do |string|
Utils.escape_html_new(string)
end
end
end
x.report "erb fallback" do
num_iteration.times do
targets.each do |string|
Utils.escape_html_new_fallback(string)
end
end
end
x.compare!
end;
puts "TEXT ONLY"
Benchmark.ips do |x|
x.report "current" do
num_iteration.times do
target_no_escaping.each do |string|
Utils.escape_html_current(string)
end
end
end
x.report "to_s" do
num_iteration.times do
target_no_escaping.each do |string|
Utils.escape_html_to_s(string)
end
end
end
x.report "erb" do
num_iteration.times do
target_no_escaping.each do |string|
Utils.escape_html_new(string)
end
end
end
x.report "erb fallback" do
num_iteration.times do
target_no_escaping.each do |string|
Utils.escape_html_new_fallback(string)
end
end
end
x.compare!
endYJIT |
This previously worked but unintentionally changed in rack#2099 This previously called `to_s` which made this accept almost anything (but especially `nil`)
2b2b6e7 to
9a3bcfc
Compare
jeremyevans
left a comment
There was a problem hiding this comment.
One difference between ERB::Escape#html_escape and CGI::Escape#escapeHTML is that if no changes are needed, ERB::Escape#html_escape does not allocate, it returns the argument. Are we OK with that change in behavior?
|
That seems reasonable to me. |
erb is required at the top of the file so if the required method is available (i.e. recent version of ruby, or erb in the gemfile) it should be deterministic. I don't think the situation you are describing is possible, or am I missing something here? |
|
Sorry, I deleted my comment, it was wrong. Yes, you are correct. |
|
One thing that is additionally nice here is that this function is now again documented. https://www.rubydoc.info/gems/rack/3.1.3/Rack%2FUtils:escape_html It didn't cope with |
This previously worked but unintentionally changed in #2099. It previously called
to_swhich made this accept almost anything (but especiallynil)This is of course a bit slower than the current implementation. Here's a benchmark:
Results for my machine:
With YJIT:
With YJIT there is barely any difference between these variants which I think is pretty great.
| ""would be an option and it does handlenilbut not non-strings.