From 18db311c572cd976d447d5610383f55c28ec1a19 Mon Sep 17 00:00:00 2001 From: Tute Costa Date: Fri, 8 Aug 2025 21:50:11 -0300 Subject: [PATCH 1/4] 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] --- lib/merit.rb | 15 ++++++++++++++- lib/merit/base_target_finder.rb | 14 +++++++++++++- lib/merit/controller_extensions.rb | 4 ++-- test/dummy/config/initializers/merit.rb | 2 +- test/integration/navigation_test.rb | 1 - test/unit/action_test.rb | 7 +++---- test/unit/base_target_finder_test.rb | 15 ++++++++++++++- 7 files changed, 47 insertions(+), 11 deletions(-) diff --git a/lib/merit.rb b/lib/merit.rb index 15043354..0cd624ac 100644 --- a/lib/merit.rb +++ b/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) diff --git a/lib/merit/base_target_finder.rb b/lib/merit/base_target_finder.rb index b1c68f46..823f692f 100644 --- a/lib/merit/base_target_finder.rb +++ b/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 diff --git a/lib/merit/controller_extensions.rb b/lib/merit/controller_extensions.rb index 9696c3a9..475a4e3b 100644 --- a/lib/merit/controller_extensions.rb +++ b/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 diff --git a/test/dummy/config/initializers/merit.rb b/test/dummy/config/initializers/merit.rb index 5f543019..627d3e46 100644 --- a/test/dummy/config/initializers/merit.rb +++ b/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 diff --git a/test/integration/navigation_test.rb b/test/integration/navigation_test.rb index 579bb23c..6a848943 100644 --- a/test/integration/navigation_test.rb +++ b/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' diff --git a/test/unit/action_test.rb b/test/unit/action_test.rb index 93782c65..91dee69a 100644 --- a/test/unit/action_test.rb +++ b/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 diff --git a/test/unit/base_target_finder_test.rb b/test/unit/base_target_finder_test.rb index e423ef34..3f6b2d14 100644 --- a/test/unit/base_target_finder_test.rb +++ b/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 From c9f52b9b58c606d5decc4fb9a6e1decc79494bdd Mon Sep 17 00:00:00 2001 From: Tute Costa Date: Fri, 8 Aug 2025 23:23:41 -0300 Subject: [PATCH 2/4] update merit.rb --- lib/merit.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/merit.rb b/lib/merit.rb index 0cd624ac..984a1d65 100644 --- a/lib/merit.rb +++ b/lib/merit.rb @@ -33,7 +33,6 @@ def self.yaml_safe_load_permitted_classes ActiveModel::Type::String, ActiveSupport::HashWithIndifferentAccess, Array, - Comment, Hash ] + @config.yaml_safe_load_permitted_classes end From 9d593b899ca92a6317c47e4469d99689d2949370 Mon Sep 17 00:00:00 2001 From: Tute Costa Date: Fri, 8 Aug 2025 23:26:17 -0300 Subject: [PATCH 3/4] base_target_finder.rb --- lib/merit/base_target_finder.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/merit/base_target_finder.rb b/lib/merit/base_target_finder.rb index 823f692f..3e638db2 100644 --- a/lib/merit/base_target_finder.rb +++ b/lib/merit/base_target_finder.rb @@ -35,8 +35,7 @@ def parse_target_data(target_data) rescue JSON::ParserError YAML.safe_load( target_data, - permitted_classes: Merit.yaml_safe_load_permitted_classes, - aliases: false + permitted_classes: Merit.yaml_safe_load_permitted_classes ) end end From c38c5e2690ab09726b400747eb7b67d63ae6798a Mon Sep 17 00:00:00 2001 From: Tute Costa Date: Fri, 8 Aug 2025 23:28:29 -0300 Subject: [PATCH 4/4] merit.rb --- lib/merit.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/merit.rb b/lib/merit.rb index 984a1d65..c072ee7e 100644 --- a/lib/merit.rb +++ b/lib/merit.rb @@ -31,9 +31,7 @@ def self.yaml_safe_load_permitted_classes ActiveModel::Attribute.const_get(:FromDatabase), ActiveModel::Attribute.const_get(:FromUser), ActiveModel::Type::String, - ActiveSupport::HashWithIndifferentAccess, - Array, - Hash + ActiveSupport::HashWithIndifferentAccess ] + @config.yaml_safe_load_permitted_classes end