Only log basic response information

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

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