Bugfix: address unsafe YAML load on destroy actions

Addresses `Psych::DisallowedClass: Tried to load unspecified class`.
Reenables three skipped tests.

Exposes new `config.yaml_permitted_classes` option for legacy usages,
and starts serializing in secure-by-default JSON that doesn't need
configuration by hosts apps.

[fixes #365]

Author: tute

lib/merit.rb

@@ -26,6 +26,18 @@ def self.current_user_method
       "current_#{@config.user_model_name.downcase}".to_sym
   end
 
+  def self.yaml_safe_load_permitted_classes
+    [
+      ActiveModel::Attribute.const_get(:FromDatabase),
+      ActiveModel::Attribute.const_get(:FromUser),
+      ActiveModel::Type::String,
+      ActiveSupport::HashWithIndifferentAccess,
+      Array,
+      Comment,
+      Hash
+    ] + @config.yaml_safe_load_permitted_classes
+  end
+
   def self.observers
     @config.observers
   end
@@ -47,13 +59,14 @@ def self.remove_point_rules
 
   class Configuration
     attr_accessor :checks_on_each_request, :orm, :user_model_name, :observers,
-                  :current_user_method
+                  :current_user_method, :yaml_safe_load_permitted_classes
 
     def initialize
       @checks_on_each_request = true
       @orm = :active_record
       @user_model_name = 'User'
       @observers = []
+      @yaml_safe_load_permitted_classes = []
     end
 
     def add_observer(class_name)

lib/merit/base_target_finder.rb

@@ -25,7 +25,19 @@ def model_class
     end
 
     def reanimate_target_from_action
-      YAML.load(@action.target_data)
+      parse_target_data(@action.target_data)
+    end
+
+    private
+
+    def parse_target_data(target_data)
+      model_class.new JSON.parse(target_data)
+    rescue JSON::ParserError
+      YAML.safe_load(
+        target_data,
+        permitted_classes: Merit.yaml_safe_load_permitted_classes,
+        aliases: false
+      )
     end
   end
 end

lib/merit/controller_extensions.rb

@@ -27,7 +27,7 @@ def merit_action_hash
         had_errors:    had_errors?,
         target_model:  controller_path,
         target_id:     target_id,
-        target_data:   target_object.to_yaml,
+        target_data:   JSON.generate(target_object.as_json)
       }
     end
 
@@ -42,7 +42,7 @@ def had_errors?
     def target_object
       variable_name = :"@#{controller_name.singularize}"
       if instance_variable_defined?(variable_name)
-        if target_obj = instance_variable_get(variable_name)
+        if (target_obj = instance_variable_get(variable_name))
           target_obj
         else
           warn_no_object_found

test/dummy/config/initializers/merit.rb

@@ -2,7 +2,7 @@
 Merit.setup do |config|
   # Add application observers to get notifications any time merit changes reputation.
   config.add_observer 'DummyObserver'
-
+  config.yaml_safe_load_permitted_classes = %w(Comment)
   config.orm = ENV['ORM'].try(:to_sym)
 end
 

test/integration/navigation_test.rb

@@ -231,7 +231,6 @@ def teardown
     # Destroying a comment should remove points from the comment creator.
     comment_to_destroy = user.comments.last
     visit '/comments'
-    skip "see bug https://github.com/merit-gem/merit/issues/365"
     assert_difference lambda { user.reload.points }, -5 do
       within("tr#c_#{comment_to_destroy.id}") do
         click_link 'Destroy'

test/unit/action_test.rb

@@ -2,12 +2,11 @@
 
 describe Merit::Action do
   it 'saves correctly with a serialised model' do
-    skip "see bug https://github.com/merit-gem/merit/issues/365"
     comment = Comment.new(name: 'the comment name')
     action = Merit::Action.create(target_model: 'comment',
                                   target_id: 2,
-                                  target_data: comment.to_yaml)
-    comment_yaml = Merit::Action.find(action.id).target_data
-    assert_equal comment.name, YAML::load(comment_yaml).name
+                                  target_data: JSON.generate(comment.as_json))
+    comment_json = Merit::Action.find(action.id).target_data
+    assert_equal comment.name, JSON.parse(comment_json)['name']
   end
 end

test/unit/base_target_finder_test.rb

@@ -47,7 +47,20 @@
 
     describe 'target was destroyed' do
       it 'gets the object from the JSON data in the merit_actions table' do
-        skip "see bug https://github.com/merit-gem/merit/issues/365"
+        comment = Comment.new(name: 'the comment name')
+
+        rule = Merit::Rule.new
+        rule.to = :itself
+        rule.model_name = 'comment'
+        action = Merit::Action.new(target_model: 'comment',
+                                   target_id: 2,
+                                   target_data: JSON.generate(comment.as_json))
+
+        finder = Merit::BaseTargetFinder.new(rule, action)
+        _(finder.find.name).must_be :==, 'the comment name'
+      end
+
+      it 'gets the object from the legacy YAML data in the merit_actions table' do
         comment = Comment.new(name: 'the comment name')
 
         rule = Merit::Rule.new