Skip to content

Commit

Permalink
Ensure LogEntry only saves safe data
Browse files Browse the repository at this point in the history
This will prevent from saving values that are now allowed in the
serialized column.
  • Loading branch information
elia committed Feb 20, 2023
1 parent 2d940d9 commit 273c68b
Show file tree
Hide file tree
Showing 2 changed files with 76 additions and 14 deletions.
32 changes: 23 additions & 9 deletions core/app/models/spree/log_entry.rb
Original file line number Diff line number Diff line change
Expand Up @@ -70,19 +70,33 @@ def self.permitted_classes
belongs_to :source, polymorphic: true, optional: true

def parsed_details
@details ||= YAML.safe_load(
details,
permitted_classes: self.class.permitted_classes,
aliases: Spree::Config.log_entry_allow_aliases
)
handle_psych_serialization_errors do
@details ||= YAML.safe_load(
details,
permitted_classes: self.class.permitted_classes,
aliases: Spree::Config.log_entry_allow_aliases,
)
end
end

def parsed_details=(value)
handle_psych_serialization_errors do
self.details = YAML.safe_dump(
value,
permitted_classes: self.class.permitted_classes,
aliases: Spree::Config.log_entry_allow_aliases,
)
end
end

private

def handle_psych_serialization_errors
yield
rescue Psych::DisallowedClass => e
raise DisallowedClass.new(psych_exception: e)
rescue Psych::BadAlias => e
raise BadAlias.new(psych_exception: e)
end

def parsed_details=(value)
self.details = value.to_yaml
end
end
end
58 changes: 53 additions & 5 deletions core/spec/models/spree/log_entry_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,61 @@

describe '#parsed_details=' do
it 'serializes the provided value to YAML' do
value = double("an object", to_yaml: "---\nfoo: bar")
log_entry = described_class.new(parsed_details: { "foo" => "bar" })

log_entry = described_class.new(parsed_details: value)
expect(log_entry.details).to eq("---\nfoo: bar\n")
expect(log_entry.parsed_details).to eq("foo" => "bar")
end

it 'allows aliases by default' do
x = []
x << x

log_entry = described_class.new

expect { log_entry.parsed_details = x }.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

expect { log_entry.parsed_details = x }.to raise_error(described_class::BadAlias, /log_entry_allow_aliases/)
end

it 'can dump ActiveMerchant::Billing::Response instances' do
response = ActiveMerchant::Billing::Response.new('success', 'message')

log_entry = described_class.new

expect { log_entry.parsed_details = response }.not_to raise_error
end

it 'can dump ActiveSupport::TimeWithZone instances' do
time = Time.zone.now

log_entry = described_class.new

expect { log_entry.parsed_details = time }.not_to raise_error
end

it 'can dump user specified class instances' do
stub_spree_preferences(log_entry_permitted_classes: ['Date'])

log_entry = described_class.new

expect { log_entry.parsed_details = Date.new }.not_to raise_error
end

it 'raises a meaningful exception when a disallowed class is found' do
log_entry = described_class.new

expect(value).to have_received(:to_yaml)
expect(log_entry.details).to eq("---\nfoo: bar")
expect(log_entry.parsed_details).to eq({"foo" => "bar"})
expect { log_entry.parsed_details = Date.new }.to raise_error(
described_class::DisallowedClass, /log_entry_permitted_classes/
)
end
end
end

0 comments on commit 273c68b

Please sign in to comment.