feat: add error reference to API error response and ensure potentially sensitive details from the exception message are not exposed

Author: bethesque

lib/pact_broker/api/resources/error_handler.rb

@@ -1,5 +1,6 @@
 require 'webmachine/convert_request_to_rack_env'
 require 'pact_broker/configuration'
+require 'securerandom'
 
 module PactBroker
   module Api
@@ -10,24 +11,46 @@ class ErrorHandler
         include PactBroker::Logging
 
         def self.call e, request, response
-          logger.error e
+          error_reference = SecureRandom.urlsafe_base64.gsub(/[^a-z]/i, '')[0,10]
+          logger.error "#{e.message} - error reference #{error_reference}"
           logger.error e.backtrace
-          response_body = { error: { :message => e.message } }
-          if PactBroker.configuration.show_backtrace_in_error_response?
-            response_body[:error][:backtrace] = e.backtrace
-          end
-          response.body = response_body.to_json
-          report(e, request) if reportable?(e)
+          response.body = response_body_hash(e, error_reference).to_json
+          report(e, error_reference, request) if reportable?(e)
         end
 
         def self.reportable? e
           !e.is_a?(PactBroker::Error)
         end
 
-        def self.report e, request
+        def self.display_message(e, error_reference)
+          if PactBroker.configuration.show_backtrace_in_error_response?
+            e.message
+          else
+           reportable?(e) ? obfuscated_error_message(error_reference) : e.message
+          end
+        end
+
+        def self.obfuscated_error_message error_reference
+          "An error has occurred. The details have been logged with the reference #{error_reference}"
+        end
+
+        def self.response_body_hash e, error_reference
+          response_body = {
+            error: {
+              message: display_message(e, error_reference),
+              reference: error_reference
+            }
+          }
+          if PactBroker.configuration.show_backtrace_in_error_response?
+            response_body[:error][:backtrace] = e.backtrace
+          end
+          response_body
+        end
+
+        def self.report e, error_reference, request
           PactBroker.configuration.api_error_reporters.each do | error_notifier |
             begin
-              error_notifier.call(e, env: Webmachine::ConvertRequestToRackEnv.call(request))
+              error_notifier.call(e, env: Webmachine::ConvertRequestToRackEnv.call(request), error_reference: error_reference)
             rescue StandardError => e
               log_error(e, "Error executing api_error_reporter")
             end

spec/lib/pact_broker/api/resources/error_handler_spec.rb

@@ -8,12 +8,13 @@ module Resources
 
           before do
             allow(ErrorHandler).to receive(:logger).and_return(logger)
+            allow(SecureRandom).to receive(:urlsafe_base64).and_return("bYWfn-+yWPlf")
           end
 
           let(:logger) { double('logger').as_null_object }
           let(:error) { StandardError.new('test error') }
           let(:thing) { double('thing', call: nil, another_call: nil) }
-          let(:options) { { env: env } }
+          let(:options) { { env: env, error_reference: "bYWfnyWPlf" } }
           let(:request) { double('request' ) }
           let(:response) { double('response', :body= => nil) }
           let(:env) { double('env') }
@@ -37,33 +38,59 @@ module Resources
             subject
           end
 
-          context "when the error is a PactBroker::Error or subclass" do
-            let(:error) { Class.new(PactBroker::Error).new('test error') }
-
-            it "does not invoke the api error reporters" do
-              expect(thing).to_not receive(:call).with(error, options)
-              subject
-            end
-          end
-
-          it "creates a json error response body" do
+          it "includes an error reference" do
             expect(response).to receive(:body=) do | body |
-              expect(JSON.parse(body)['error']).to include 'message' => 'test error'
+              expect(JSON.parse(body)['error']).to include 'reference' => "bYWfnyWPlf"
             end
             subject
           end
 
-
           context "when show_backtrace_in_error_response? is true" do
             before do
               allow(PactBroker.configuration).to receive(:show_backtrace_in_error_response?).and_return(true)
             end
 
-            it "includes the backtrace in the error response" do
-              expect(response).to receive(:body=) do | body |
-                expect(body).to include("backtrace")
+            context "when the error is a PactBroker::Error or subclass" do
+              let(:error) { Class.new(PactBroker::Error).new('test error') }
+
+              it "does not invoke the api error reporters" do
+                expect(thing).to_not receive(:call).with(error, options)
+                subject
+              end
+
+              it "uses the error message as the message" do
+                expect(response).to receive(:body=) do | body |
+                  expect(JSON.parse(body)['error']).to include 'message' => "test error"
+                end
+                subject
+              end
+
+              it "includes the backtrace in the error response" do
+                expect(response).to receive(:body=) do | body |
+                  expect(body).to include("backtrace")
+                end
+                subject
+              end
+            end
+            context "when the error is not a PactBroker::Error or subclass" do
+              it "invokes the api error reporters" do
+                expect(thing).to receive(:call).with(error, options)
+                subject
+              end
+
+              it "uses the error message as the message" do
+                expect(response).to receive(:body=) do | body |
+                  expect(JSON.parse(body)['error']).to include 'message' => "test error"
+                end
+                subject
+              end
+
+              it "includes the backtrace in the error response" do
+                expect(response).to receive(:body=) do | body |
+                  expect(body).to include("backtrace")
+                end
+                subject
               end
-              subject
             end
           end
 
@@ -72,11 +99,47 @@ module Resources
               allow(PactBroker.configuration).to receive(:show_backtrace_in_error_response?).and_return(false)
             end
 
-            it "does not include the backtrace in the error response" do
-              expect(response).to receive(:body=) do | body |
-                expect(body).to_not include("backtrace")
+            context "when the error is a PactBroker::Error or subclass" do
+              let(:error) { Class.new(PactBroker::Error).new('test error') }
+
+              it "does not invoke the api error reporters" do
+                expect(thing).to_not receive(:call).with(error, options)
+                subject
+              end
+
+              it "uses the error message as the message" do
+                expect(response).to receive(:body=) do | body |
+                  expect(JSON.parse(body)['error']).to include 'message' => "test error"
+                end
+                subject
+              end
+
+              it "does not include the backtrace in the error response" do
+                expect(response).to receive(:body=) do | body |
+                  expect(body).to_not include("backtrace")
+                end
+                subject
+              end
+            end
+            context "when the error is not a PactBroker::Error or subclass" do
+              it "invokes the api error reporters" do
+                expect(thing).to receive(:call).with(error, options)
+                subject
+              end
+
+              it "uses a hardcoded error message" do
+                expect(response).to receive(:body=) do | body |
+                  expect(JSON.parse(body)['error']['message']).to match /An error/
+                end
+                subject
+              end
+
+              it "does not include the backtrace in the error response" do
+                expect(response).to receive(:body=) do | body |
+                  expect(body).to_not include("backtrace")
+                end
+                subject
               end
-              subject
             end
           end