Skip to content

Loading…

NumberHelper.number_with_delimiter should html_escape both delimiters and separators #6355

Merged
merged 1 commit into from

5 participants

@amutz

Recently, the dependency on safe_join was removed from number_helper. When this happened, delimiters and separators were no longer being html_escaped.

For example,

number_with_delimiter(1000, :delimiter =>'<script></script>')

will produce the string

"1<script></script>000" 

marked as html_safe.

This pull request makes both delimiters and separators html_escaped, and adds tests for this behavior.

See ecfb32c for more info

Likely interested parties: @carlosantoniodasilva and @josevalim

@rafaelfranca
Ruby on Rails member

Do we really need this change? :separator and :delimiter are values set by the programmer. I think we only need to escape values that can be set by users.

@drogus
Ruby on Rails member

@rafaelfranca generally it's true, but I guess it won't hurt to escape it if it's tainted and I can easily see how developers can allow to for example choose delimiters in HTML view based on user preference (I know, edge case, but still)

@rafaelfranca
Ruby on Rails member

I don't know. Still feel like we should not escape it. If we are going to escape all the strings that are set by programmers we will have some troubles.

For example:

content_tag("<script>alert('OMG!')</script>", "text")

will allow XSS.

@amutz

Just to be clear, this pull request is just to restore it to its previous behavior before three days ago. The delimiters and separators were being correctly escaped prior to that. The author of the commit that changed this, @carlosantoniodasilva, said this was not intentional, and said it should be fixed. You can read the details here:

ecfb32c

@josevalim josevalim commented on the diff
actionpack/lib/action_view/helpers/number_helper.rb
@@ -254,7 +255,7 @@ def number_with_delimiter(number, options = {})
parts = number.to_s.to_str.split('.')
parts[0].gsub!(/(\d)(?=(\d\d\d)+(?!\d))/, "\\1#{options[:delimiter]}")
- parts.join(options[:separator]).html_safe
@josevalim Ruby on Rails member

why not simply use safe_join back again?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@amutz

@josevalim, I assumed the decoupling of NumberHelper from safe_join was necessary for some other reason (@carlosantoniodasilva, was this the case?).

If it wasn't the case, I can change it back to safe_join.

@josevalim
Ruby on Rails member
@amutz

Ok, I've changed the pull request to add the tests and revert the change back to the old implementation.

The requires are strange, but they always were (active_support/testing requires actionview numberhelper, etc.) but that can be fixed in a future pull request.

Let me know if you want any other changes, thanks!

@rafaelfranca
Ruby on Rails member

Great. Please squash the commits in one and we can merge it.

@rafaelfranca
Ruby on Rails member

Do the following commands:

git fetch rails_origin
git rebase -i rails_origin/master
# squash the commits in one
git push -f your_origin fix_number_with_delimiter_escaping
@amutz

Thanks for the git tip... should be a single commit now.

Thanks!

@rafaelfranca rafaelfranca merged commit a6dbc3f into rails:master
@carlosantoniodasilva
Ruby on Rails member

@amutz hey.. thank you a lot for your work on this, and sorry I couldn't get back here before. There was actually no "real reason" to change that, I was restoring the previous version as I mentioned in the commit, which seemed ok based on the comments there. Well, thanks again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 18, 2012
  1. @amutz

    reverting decoupling of NumberHelper from safe_join and adding tests …

    amutz committed
    …for escaping of delimiters and separators
This page is out of date. Refresh to see the latest.
View
2 actionpack/lib/action_view/helpers/number_helper.rb
@@ -254,7 +254,7 @@ def number_with_delimiter(number, options = {})
parts = number.to_s.to_str.split('.')
parts[0].gsub!(/(\d)(?=(\d\d\d)+(?!\d))/, "\\1#{options[:delimiter]}")
- parts.join(options[:separator]).html_safe
@josevalim Ruby on Rails member

why not simply use safe_join back again?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ safe_join(parts, options[:separator])
end
# Formats a +number+ with the specified level of
View
2 actionpack/test/template/number_helper_test.rb
@@ -78,6 +78,8 @@ def test_number_with_delimiter_with_options_hash
assert_equal '12,345,678-05', number_with_delimiter(12345678.05, :separator => '-')
assert_equal '12.345.678,05', number_with_delimiter(12345678.05, :separator => ',', :delimiter => '.')
assert_equal '12.345.678,05', number_with_delimiter(12345678.05, :delimiter => '.', :separator => ',')
+ assert_equal '1&lt;script&gt;&lt;/script&gt;01', number_with_delimiter(1.01, :separator => "<script></script>")
+ assert_equal '1&lt;script&gt;&lt;/script&gt;000', number_with_delimiter(1000, :delimiter => "<script></script>")
end
def test_number_with_precision
View
1 activesupport/lib/active_support/testing/performance.rb
@@ -196,6 +196,7 @@ def self.[](name)
class Base
include ActionView::Helpers::NumberHelper
+ include ActionView::Helpers::OutputSafetyHelper
attr_reader :total
Something went wrong with that request. Please try again.