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

Already on GitHub? Sign in to your account

Keys in Money::Currency::TABLE should be string rather than symbol #132

Closed
kenn opened this Issue Dec 14, 2011 · 5 comments

Comments

Projects
None yet
3 participants
Member

kenn commented Dec 14, 2011

In Money::Currency#initialize, id argument is converted to symbol in the initialize method:

def initialize(id)
  @id  = id.to_s.downcase.to_sym
  data = TABLE[@id] || raise(UnknownCurrency, "Unknown currency `#{id}'")
  data.each_pair do |key, value|
    instance_variable_set(:"@#{key}", value)
  end
end

This could be problematic if you have a code like this in a Rails application:

Money::Currency.new(params[:currency])

or

params[:currency].to_currency

This could potentially create unlimited number of symbol entry by users that eventually causes symbol table overflow (RuntimeError) with 16Mi entries, which could be a source of problems like memory leak, slow symbol table lookup, or even DoS attack. It statically accumulates even when it ends up with the UnknownCurrency exception.

FWIW, for the same reason ActiveSupport::HashWithIndifferentAccess keeps hash keys as string internally.

https://github.com/rails/rails/blob/master/activesupport/lib/active_support/hash_with_indifferent_access.rb

Another way to get around this is provide some validator class to check user-provided strings before the string is passed to Money::Currency.new.

Owner

weppos commented Dec 15, 2011

I never faced this issue, but it's a very interesting topic to keep in mind. Thank you very much for point it out.

I think we need to find a solution. Handling the currency key as String might be the solution. However, changing the way we store currencies would break applications that access TABLE directly, though this is not recommended.

I'm +1 for changing keys to Strings. I'm thinking about a solution to minimize the impact.

Owner

semmons99 commented Dec 16, 2011

For this to affect someone, wouldn't they have to issue 16 million unique symbols? The only issue I could see is if you're throwing heaps of invalid currencies to Money::Currency#new.

Wouldn't this fix the problem?

ID_STRINGS = TABLE.keys.map{|k| k.to_s.downcase}

def initialize(id)
  id = id.to_s.downcase

  unless ID_STRINGS.include? id
    raise UnknownCurrency, "Unknown currency '#{id}'"
  end

  @id = id.to_sym
  data = TABLE[@id]
  data.each_pair do |key, value|
    instance_variable_set(:"@#{key}", value)
  end
end
Owner

semmons99 commented Dec 24, 2011

Since no response has been given in a week, I'm closing this issue.

@semmons99 semmons99 closed this Dec 24, 2011

Member

kenn commented Dec 24, 2011

@semmons99 sorry for my late reply - your fix does seem good, plus backward compatible. Can you apply the fix, or should I cook up a pull request? Thanks!

Owner

semmons99 commented Dec 24, 2011

@kenn if you have some time to make the pull request and add a test or two, that'd be great.

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