diff --git a/core/app/models/spree/log_entry.rb b/core/app/models/spree/log_entry.rb index 2e3bd32a456..2334c3658b5 100644 --- a/core/app/models/spree/log_entry.rb +++ b/core/app/models/spree/log_entry.rb @@ -2,10 +2,83 @@ module Spree class LogEntry < Spree::Base + # Classes used in core that can be present in serialized details + # + # Users can add their own classes in + # `Spree::Config#log_entry_permitted_classes`. + # + # @see Spree::AppConfiguration#log_entry_permitted_classes + CORE_PERMITTED_CLASSES = [ + ActiveMerchant::Billing::Response, + ActiveSupport::TimeWithZone, + Time, + ActiveSupport::TimeZone + ].freeze + + # Raised when a disallowed class is tried to be loaded + class DisallowedClass < RuntimeError + attr_reader :psych_exception + + def initialize(psych_exception:) + @psych_exception = psych_exception + super(default_message) + end + + private + + def default_message + <<~MSG + #{psych_exception.message} + + You can specify custom classes to be loaded in config/initializers/spree.rb. E.g: + + Spree.config do |config| + config.log_entry_permitted_classes = ['MyClass'] + end + MSG + end + end + + # Raised when YAML contains aliases and they're not enabled + class BadAlias < RuntimeError + attr_reader :psych_exception + + def initialize(psych_exception:) + @psych_exception = psych_exception + super(default_message) + end + + private + + def default_message + <<~MSG + #{psych_exception.message} + + You can explicitly enable aliases in config/initializers/spree.rb. E.g: + + Spree.config do |config| + config.log_entry_allow_aliases = true + end + MSG + end + end + + def self.permitted_classes + CORE_PERMITTED_CLASSES + Spree::Config.log_entry_permitted_classes.map(&:constantize) + end + belongs_to :source, polymorphic: true, optional: true def parsed_details - @details ||= YAML.load(details) + @details ||= YAML.safe_load( + details, + permitted_classes: self.class.permitted_classes, + aliases: Spree::Config.log_entry_allow_aliases + ) + rescue Psych::DisallowedClass => e + raise DisallowedClass.new(psych_exception: e) + rescue Psych::BadAlias => e + raise BadAlias.new(psych_exception: e) end end end diff --git a/core/lib/spree/app_configuration.rb b/core/lib/spree/app_configuration.rb index e0276aa5ac1..9d9dada2709 100644 --- a/core/lib/spree/app_configuration.rb +++ b/core/lib/spree/app_configuration.rb @@ -165,6 +165,22 @@ class AppConfiguration < Preferences::Configuration # @return [String] URL of logo used on frontend (default: +'logo/solidus.svg'+) preference :logo, :string, default: 'logo/solidus.svg' + # @!attribute [rw] log_entry_permitted_classes + # @return [Array] An array of extra classes that are allowed to be + # loaded from a serialized YAML as details in {Spree::LogEntry} + # (defaults to a non-frozen empty array, so that extensions can add + # their own classes). + # @example + # config.log_entry_permitted_classes = ['Date'] + preference :log_entry_permitted_classes, :array, default: [] + + # @!attribute [rw] log_entry_allow_aliases + # @return [Boolean] Whether YAML aliases are allowed when loading + # serialized data in {Spree::LogEntry}. It defaults to true. Depending + # on the source of your data, you may consider disabling it to prevent + # entity expansion attacks. + preference :log_entry_allow_aliases, :boolean, default: true + # @!attribute [rw] mails_from # @return [String] Email address used as +From:+ field in transactional emails. preference :mails_from, :string, default: 'solidus@example.com' diff --git a/core/lib/spree/core/engine.rb b/core/lib/spree/core/engine.rb index d8a63235984..022463d1a25 100644 --- a/core/lib/spree/core/engine.rb +++ b/core/lib/spree/core/engine.rb @@ -15,6 +15,12 @@ class Engine < ::Rails::Engine generator.test_framework :rspec end + if ActiveRecord.respond_to?(:yaml_column_permitted_classes) || ActiveRecord::Base.respond_to?(:yaml_column_permitted_classes) + config.active_record.yaml_column_permitted_classes ||= [] + config.active_record.yaml_column_permitted_classes |= + [Symbol, BigDecimal, ActiveSupport::HashWithIndifferentAccess] + end + initializer "spree.environment", before: :load_config_initializers do |app| app.config.spree = Spree::Config.environment end diff --git a/core/solidus_core.gemspec b/core/solidus_core.gemspec index 30871b7bc84..42c3ba3f08a 100644 --- a/core/solidus_core.gemspec +++ b/core/solidus_core.gemspec @@ -40,6 +40,7 @@ Gem::Specification.new do |s| s.add_dependency 'mini_magick', '~> 4.10' s.add_dependency 'monetize', '~> 1.8' s.add_dependency 'kt-paperclip', '~> 6.3' + s.add_dependency 'psych', ['>= 3.1.0', '< 5.0'] s.add_dependency 'ransack', '~> 2.0' s.add_dependency 'state_machines-activerecord', '~> 0.6' diff --git a/core/spec/models/spree/log_entry_spec.rb b/core/spec/models/spree/log_entry_spec.rb new file mode 100644 index 00000000000..c9ae45d650f --- /dev/null +++ b/core/spec/models/spree/log_entry_spec.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Spree::LogEntry, type: :model do + describe '#parsed_details' do + it 'allow aliases by default' do + x = [] + x << x + + log_entry = described_class.new(details: x.to_yaml) + + expect { log_entry.parsed_details }.not_to raise_error + end + + it 'can disable aliases and raises a meaningful exception when used' do + stub_spree_preferences(log_entry_allow_aliases: false) + x = [] + x << x + + log_entry = described_class.new(details: x.to_yaml) + + expect { log_entry.parsed_details }.to raise_error(described_class::BadAlias, /log_entry_allow_aliases/) + end + + it 'can parse ActiveMerchant::Billing::Response instances' do + response = ActiveMerchant::Billing::Response.new('success', 'message') + + log_entry = described_class.new(details: response.to_yaml) + + expect { log_entry.parsed_details }.not_to raise_error + end + + it 'can parse ActiveSupport::TimeWithZone instances' do + time = Time.zone.now + + log_entry = described_class.new(details: time.to_yaml) + + expect { log_entry.parsed_details }.not_to raise_error + end + + it 'can parse user specified classes instances' do + stub_spree_preferences(log_entry_permitted_classes: ['Date']) + + log_entry = described_class.new(details: Date.today) + + expect { log_entry.parsed_details }.not_to raise_error + end + + it 'raises a meaningful exception when a disallowed class is found' do + log_entry = described_class.new(details: Date.today) + + expect { log_entry.parsed_details }.to raise_error(described_class::DisallowedClass, /log_entry_permitted_classes/) + end + end +end diff --git a/core/spec/models/spree/promotion/rules/user_role_spec.rb b/core/spec/models/spree/promotion/rules/user_role_spec.rb index 9dbbabe2408..baba3f51609 100644 --- a/core/spec/models/spree/promotion/rules/user_role_spec.rb +++ b/core/spec/models/spree/promotion/rules/user_role_spec.rb @@ -3,7 +3,7 @@ require 'rails_helper' RSpec.describe Spree::Promotion::Rules::UserRole, type: :model do - let(:rule) { described_class.new(preferred_role_ids: roles_for_rule) } + let(:rule) { described_class.new(preferred_role_ids: roles_for_rule.map(&:id)) } let(:user) { create(:user, spree_roles: roles_for_user) } let(:roles_for_rule) { [] } let(:roles_for_user) { [] }