Skip to content

Commit

Permalink
store enum mapping using Strings instead of Symbols.
Browse files Browse the repository at this point in the history
This allows to assign both `String` and `Symbol` values to the enum
without having to call `to_sym`, which is a security problem.
  • Loading branch information
senny committed Nov 5, 2013
1 parent 6c720d1 commit 44406d1
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 3 deletions.
3 changes: 2 additions & 1 deletion activerecord/lib/active_record/enum.rb
Expand Up @@ -43,6 +43,7 @@ def enum(definitions)
_enum_methods_module.module_eval do
# def direction=(value) self[:direction] = DIRECTION[value] end
define_method("#{name}=") { |value|
value = value.to_s
unless enum_values.has_key?(value)
raise ArgumentError, "'#{value}' is not a valid #{name}"
end
Expand All @@ -54,7 +55,7 @@ def enum(definitions)

pairs = values.respond_to?(:each_pair) ? values.each_pair : values.each_with_index
pairs.each do |value, i|
enum_values[value] = i
enum_values[value.to_s] = i

# scope :incoming, -> { where direction: 0 }
klass.scope value, -> { klass.where name => i }
Expand Down
9 changes: 7 additions & 2 deletions activerecord/test/cases/enum_test.rb
Expand Up @@ -17,8 +17,8 @@ class EnumTest < ActiveRecord::TestCase
end

test "query state with symbol" do
assert_equal :proposed, @book.status
assert_equal :unread, @book.read_status
assert_equal "proposed", @book.status
assert_equal "unread", @book.read_status
end

test "find via scope" do
Expand Down Expand Up @@ -46,6 +46,11 @@ class EnumTest < ActiveRecord::TestCase
assert @book.written?
end

test "assign string value" do
@book.status = "written"
assert @book.written?
end

test "assign non existing value raises an error" do
e = assert_raises(ArgumentError) do
@book.status = :unknown
Expand Down

4 comments on commit 44406d1

@jordimassaguerpla
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've found this commit with the gems-status software (github.com/jordimassaguerpla/gems-status) why is that a security problem? And if so, why has not yet been released and assigned a CVE entry?

@rafaelfranca
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jordimassaguerpla normally to_sym can be a security problem, in this case it is not. The commit message talk about this possibility, not about a problem.

@carlosantoniodasilva
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jordimassaguerpla this is a master feature only, so there's nothing possibly wrong with it to worry about. Thanks.

@jordimassaguerpla
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for your comments.

Please sign in to comment.