diff --git a/core/app/models/spree/payment/processing.rb b/core/app/models/spree/payment/processing.rb index 806dd808906..cbab1238bf4 100644 --- a/core/app/models/spree/payment/processing.rb +++ b/core/app/models/spree/payment/processing.rb @@ -204,22 +204,42 @@ def protect_from_connection_error end def gateway_error(error) - if error.is_a? ActiveMerchant::Billing::Response - text = error.params['message'] || error.params['response_reason_text'] || error.message - elsif error.is_a? ActiveMerchant::ConnectionError - text = I18n.t('spree.unable_to_connect_to_gateway') - else - text = error.to_s - end - logger.error(I18n.t('spree.gateway_error')) - logger.error(" #{error.to_yaml}") - raise Core::GatewayError.new(text) + message, log = case error + when ActiveMerchant::Billing::Response + [ + error.params['message'] || error.params['response_reason_text'] || error.message, + basic_response_info(error) + ] + when ActiveMerchant::ConnectionError + [I18n.t('spree.unable_to_connect_to_gateway')] * 2 + else + [error.to_s, error] + end + + logger.error("#{I18n.t('spree.gateway_error')}: #{log}") + raise Core::GatewayError.new(message) end # The unique identifier to be passed in to the payment gateway def gateway_order_id "#{order.number}-#{number}" end + + # The gateway response information without the params since the params + # can contain PII. + def basic_response_info(response) + { + message: response.message, + test: response.test, + authorization: response.authorization, + avs_result: response.avs_result, + cvv_result: response.cvv_result, + error_code: response.error_code, + emv_authorization: response.emv_authorization, + gateway_order_id: gateway_order_id, + order_number: order.number + } + end end end end diff --git a/core/spec/models/spree/payment_spec.rb b/core/spec/models/spree/payment_spec.rb index d4f0f6831a1..15fb31c870b 100644 --- a/core/spec/models/spree/payment_spec.rb +++ b/core/spec/models/spree/payment_spec.rb @@ -38,7 +38,12 @@ end let(:failed_response) do - ActiveMerchant::Billing::Response.new(false, '', {}, {}) + ActiveMerchant::Billing::Response.new( + false, + 'Declined', + { transaction: {} }, + {} + ) end context '.risky' do @@ -318,14 +323,25 @@ end context "if unsuccessful" do - it "should mark payment as failed" do + before do allow(gateway).to receive(:authorize).and_return(failed_response) + end + + it "should mark payment as failed" do expect(payment).to receive(:failure) expect(payment).not_to receive(:pend) expect { payment.authorize! }.to raise_error(Spree::Core::GatewayError) end + + it "should not log response params" do + expect(Rails.logger).to receive(:error).with(/Gateway Error/) + expect(Rails.logger).to_not receive(:error).with(/transaction/) + expect { + payment.authorize! + }.to raise_error(Spree::Core::GatewayError) + end end end @@ -387,6 +403,14 @@ expect { payment.purchase! }.to raise_error(Spree::Core::GatewayError) expect(payment.capture_events.count).to eq(0) end + + it "should not log response params" do + expect(Rails.logger).to receive(:error).with(/Gateway Error/) + expect(Rails.logger).to_not receive(:error).with(/transaction/) + expect { + payment.purchase! + }.to raise_error(Spree::Core::GatewayError) + end end end @@ -442,12 +466,23 @@ end context "if unsuccessful" do - it "should not make payment complete" do + before do allow(gateway).to receive_messages capture: failed_response + end + + it "should not make payment complete" do expect(payment).to receive(:failure) expect(payment).not_to receive(:complete) expect { payment.capture! }.to raise_error(Spree::Core::GatewayError) end + + it "should not log response params" do + expect(Rails.logger).to receive(:error).with(/Gateway Error/) + expect(Rails.logger).to_not receive(:error).with(/transaction/) + expect { + payment.capture! + }.to raise_error(Spree::Core::GatewayError) + end end end @@ -498,6 +533,14 @@ expect(payment.state).to eq('pending') expect(payment.response_code).to eq('abc') end + + it "should not log response params" do + expect(Rails.logger).to receive(:error).with(/Gateway Error/) + expect(Rails.logger).to_not receive(:error).with(/transaction/) + expect { + subject + }.to raise_error(Spree::Core::GatewayError) + end end end @@ -539,11 +582,22 @@ end context "if unsuccessful" do - it "should not void the payment" do + before do allow(gateway).to receive_messages void: failed_response + end + + it "should not void the payment" do expect(payment).not_to receive(:void) expect { payment.void_transaction! }.to raise_error(Spree::Core::GatewayError) end + + it "should not log response params" do + expect(Rails.logger).to receive(:error).with(/Gateway Error/) + expect(Rails.logger).to_not receive(:error).with(/transaction/) + expect { + payment.void_transaction! + }.to raise_error(Spree::Core::GatewayError) + end end # Regression test for https://github.com/spree/spree/issues/2119