Only log basic response information

When logging a gateway error response there is a params field,
which provides extra information about the request. That information
can include useful information like the payment amount, but it can
also include PII such as a full billing address.

By logging the full error response in yaml that PII can end up in your
logs, which is not desirable and potentially against the law. Therefore
we should only log the minimal information needed from the response.

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