Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

SafeBuffer doesn't handle % #6352

Closed
jaroslawr opened this Issue · 8 comments

3 participants

@jaroslawr

This ERB code:

"<b>Foobar</b>".html_safe + "<b>foo</b>

Becomes this HTML:

<b>Foobar</b>&lt;b&gt;foo&lt;/b&gt;

But this ERB code:

<%= "<b>Foobar</b> %s".html_safe % "<b>foo</b>" %>

Becomes this HTML:

&lt;b&gt;Foobar&lt;/b&gt; &lt;b&gt;foo&lt;/b&gt;

In the second case, I would expect only <b>foo</b> to be escaped, but instead both strings are escaped.

Is there any deep reason for this or is it just on omission?

@jaroslawr

In my mind % should escape its arguments if those aren't .html_safe, then do the traditional Ruby String interpolation, then mark the result as .html_safe. I am right here?

@nashby

@jaroslawr what rails version are you using? Because for me (rails 3.2.3)

<%= "<b>Foobar</b> %s".html_safe % "<b>foo</b>" %>

became

<b>Foobar</b> <b>foo</b>
@jaroslawr

@nashby Sorry, my bad, I did a stupid thing and tested this with a monkey patch for the original problem I had. I updated the description accordingly.

@jaroslawr

I thought about this some more and the corner case here seems to be type coercion that the original Ruby implementation of % will do - even when you escape all the string arguments passed to SafeBuffers %, Ruby's % implementation type coercion can still inject unescaped strings into the SafeBuffer and you cannot blindly call to_s on everything passed to % since the format string might require argument of particular type.

It should be possible, though, to split the string in the SafeBuffer so that the interpolations ("%d", "%s") are separated from other text. Then Ruby's % implementation could be used to do type coercion and conversion to a string, this string could be escaped and glued back together with the normal text. So, if I have for example

"Something: %s, something else: %.2f" % [Foobar.new, 1.2]

The process would be the following:

  • The string would be split into an array: ["Something: ", "%s", ", something else: ", "%.2f"]

  • The percent-strings would be interpolated and escaped (only in case the result is not .html_safe?), so the array would look like the result of:

["Something: ", ERB::Util.html_escape("%s" % Foobar.new), ", something else: ", ERB::Util.html_escape("%.2f" % 1.2)]
  • The array would be joined back together into a single string.
@jaroslawr

Thanks for the fix, but I think there is a small problem with the implementation, it modifies the array passed as an argument:

irb(main):001:0> "foo".html_safe
=> "foo"
irb(main):002:0> x = ["<p>", "<b>", "<h1>"]
=> ["<p>", "<b>", "<h1>"]
irb(main):003:0> "%s %s %s".html_safe % x
=> "&lt;p&gt; &lt;b&gt; &lt;h1&gt;"
irb(main):004:0> y
=> ["&lt;p&gt;", "&lt;b&gt;", "&lt;h1&gt;"]
@nashby

Nice catch. I'll fix it tonight!

@jaroslawr

Thanks!

@nashby

@jaroslawr it was fixed on master. Thanks again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.