From df9ab52ebc0088f16b2e7e8c98d932a42bd0e957 Mon Sep 17 00:00:00 2001 From: Elia Schito Date: Thu, 23 Feb 2023 11:18:06 +0100 Subject: [PATCH 1/2] Share a superclass for the LogEntry serialization errors Allow a rescue to catch both in one go --- core/app/models/spree/log_entry.rb | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/core/app/models/spree/log_entry.rb b/core/app/models/spree/log_entry.rb index c4d74776659..cdf181fc979 100644 --- a/core/app/models/spree/log_entry.rb +++ b/core/app/models/spree/log_entry.rb @@ -15,15 +15,17 @@ class LogEntry < Spree::Base ActiveSupport::TimeZone ].freeze - # Raised when a disallowed class is tried to be loaded - class DisallowedClass < RuntimeError + class SerializationError < RuntimeError attr_reader :psych_exception def initialize(psych_exception:) @psych_exception = psych_exception super(default_message) end + end + # Raised when a disallowed class is tried to be loaded + class DisallowedClass < SerializationError private def default_message @@ -40,14 +42,7 @@ def default_message 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 - + class BadAlias < SerializationError private def default_message From a36e68177a7f6ef36e53ab2e479a3d7e25393d70 Mon Sep 17 00:00:00 2001 From: Elia Schito Date: Thu, 23 Feb 2023 11:19:50 +0100 Subject: [PATCH 2/2] Ensure we keep recording payment log entries that cannot be deserialized Raising an exception at this time could prevent a payment from being properly processed. Any incompatible response will be wrapped in a valid one and saved with proper reporting. This allows it to be both recorded and reported without breaking the payment details page in the admin. --- core/app/models/spree/log_entry.rb | 11 +++++++++++ core/app/models/spree/payment/processing.rb | 2 +- core/app/models/spree/refund.rb | 2 +- core/spec/models/spree/log_entry_spec.rb | 19 +++++++++++++++++++ core/spec/models/spree/payment_spec.rb | 3 +-- 5 files changed, 33 insertions(+), 4 deletions(-) diff --git a/core/app/models/spree/log_entry.rb b/core/app/models/spree/log_entry.rb index cdf181fc979..67877c2a60b 100644 --- a/core/app/models/spree/log_entry.rb +++ b/core/app/models/spree/log_entry.rb @@ -84,6 +84,17 @@ def parsed_details=(value) end end + def parsed_payment_response_details_with_fallback=(response) + self.parsed_details = response + rescue SerializationError, YAML::Exception => e + # Fall back on wrapping the response and signaling the error to the end user. + self.parsed_details = ActiveMerchant::Billing::Response.new( + response.success?, + "[WARNING: An error occurred while trying to serialize the payment response] #{response.message}", + { 'data' => response.inspect, 'error' => e.message.to_s }, + ) + end + private def handle_psych_serialization_errors diff --git a/core/app/models/spree/payment/processing.rb b/core/app/models/spree/payment/processing.rb index d1b25229762..c2804258ed2 100644 --- a/core/app/models/spree/payment/processing.rb +++ b/core/app/models/spree/payment/processing.rb @@ -206,7 +206,7 @@ def handle_response(response) end def record_response(response) - log_entries.create!(parsed_details: response) + log_entries.create!(parsed_payment_response_details_with_fallback: response) end def protect_from_connection_error diff --git a/core/app/models/spree/refund.rb b/core/app/models/spree/refund.rb index 00158132562..b5329ee1ce7 100644 --- a/core/app/models/spree/refund.rb +++ b/core/app/models/spree/refund.rb @@ -53,7 +53,7 @@ def perform! credit_cents = money.cents @perform_response = process!(credit_cents) - log_entries.build(details: perform_response.to_yaml) + log_entries.build(parsed_payment_response_details_with_fallback: perform_response) self.transaction_id = perform_response.authorization save! diff --git a/core/spec/models/spree/log_entry_spec.rb b/core/spec/models/spree/log_entry_spec.rb index fd685bd5956..60d6ddec826 100644 --- a/core/spec/models/spree/log_entry_spec.rb +++ b/core/spec/models/spree/log_entry_spec.rb @@ -113,4 +113,23 @@ ) end end + + describe '#parsed_payment_response_details_with_fallback=' do + it 'wraps non serializable responses' do + log_entry = described_class.new + bad_response = ActiveMerchant::Billing::Response.new( + true, + 'FooBar', + { foo: { bar: "Symbol keys are not allowed" } } + ) + + log_entry.parsed_payment_response_details_with_fallback = bad_response + details = log_entry.parsed_details + + expect(details.success?).to eq(true) + expect(details.message).to eq("[WARNING: An error occurred while trying to serialize the payment response] FooBar") + expect(details.params['data']).to include(':bar=>"Symbol keys are not allowed"') + expect(details.params['error']).to include('Tried to dump unspecified class: Symbol') + end + end end diff --git a/core/spec/models/spree/payment_spec.rb b/core/spec/models/spree/payment_spec.rb index fc1c1b8a45f..5d71ce969ed 100644 --- a/core/spec/models/spree/payment_spec.rb +++ b/core/spec/models/spree/payment_spec.rb @@ -518,8 +518,7 @@ it "should do nothing" do expect(payment).not_to receive(:complete) expect(payment.payment_method).not_to receive(:capture) - expect(payment.log_entries).not_to receive(:create!) - subject + expect{ subject }.not_to change(payment.log_entries, :count) end end end