feat: only log webhook response details when a webhook host whitelist has been configured

Author: bethesque

lib/pact_broker/api/decorators/webhook_execution_result_decorator.rb

@@ -1,21 +1,17 @@
 require_relative 'base_decorator'
 require 'json'
+require 'pact_broker/messages'
 
 module PactBroker
   module Api
     module Decorators
-
       class WebhookExecutionResultDecorator < BaseDecorator
-
         class ErrorDecorator < BaseDecorator
-
           property :message
           property :backtrace
-
         end
 
         class HTTPResponseDecorator < BaseDecorator
-
           property :status, :getter => lambda { |_| code.to_i }
           property :headers, exec_context: :decorator
           property :body, exec_context: :decorator
@@ -34,11 +30,11 @@ def body
               represented.body
             end
           end
-
         end
 
-        property :error, :extend => ErrorDecorator
-        property :response, :extend => HTTPResponseDecorator
+        property :error, :extend => ErrorDecorator, if: lambda { |context| context[:options][:user_options][:show_response] }
+        property :response, :extend => HTTPResponseDecorator, if: lambda { |context| context[:options][:user_options][:show_response] }
+        property :response_hidden_message, as: :message, exec_context: :decorator, if: lambda { |context| !context[:options][:user_options][:show_response] }
 
         link :webhook do | options |
           {
@@ -52,7 +48,11 @@ def body
             href: webhook_execution_url(options.fetch(:webhook), options.fetch(:base_url))
           }
         end
+
+        def response_hidden_message
+          PactBroker::Messages.message('messages.response_body_hidden')
+        end
       end
     end
   end
-end
+end

lib/pact_broker/api/resources/webhook_execution.rb

@@ -32,7 +32,7 @@ def resource_exists?
         private
 
         def post_response_body webhook_execution_result
-          Decorators::WebhookExecutionResultDecorator.new(webhook_execution_result).to_json(user_options: { base_url: base_url, webhook: webhook })
+          Decorators::WebhookExecutionResultDecorator.new(webhook_execution_result).to_json(user_options: user_options)
         end
 
         def webhook
@@ -46,8 +46,11 @@ def pact
         def uuid
           identifier_from_path[:uuid]
         end
+
+        def user_options
+          { base_url: base_url, webhook: webhook, show_response: PactBroker.configuration.show_webhook_response? }
+        end
       end
     end
   end
-
 end

lib/pact_broker/configuration.rb

@@ -146,6 +146,10 @@ def add_api_error_reporter &block
       end
     end
 
+    def show_webhook_response?
+      webhook_host_whitelist.any?
+    end
+
     def enable_badge_resources= enable_badge_resources
       puts "Pact Broker configuration property `enable_badge_resources` is deprecated. Please use `enable_public_badge_access`"
       self.enable_public_badge_access = enable_badge_resources

lib/pact_broker/doc/views/webhooks.markdown

@@ -59,10 +59,23 @@ A request body can be specified as well.
 
 To ensure that webhooks cannot be used maliciously to expose either data about your contracts or your internal network, the following validation rules are applied to webhooks via the Pact Broker configuration settings.
 
-* **Scheme**: Must be included in the `webhook_scheme_whitelist`, which by default only includes `https`. You can change this to include `http` if absolutely necessary, however, keep in mind that the body of any http traffic is visible to the network. You can load a self signed certificate into the Pact Broker to be used for https connections using `script/insert-self-signed-certificate-from-url.rb` in the Pact Broker repository.
+* **Scheme**: Must be included in the `webhook_scheme_whitelist`, which by default only includes `https`. You can change this to include `http` if absolutely necessary, however, keep in mind that the body of any http traffic is visible to the network. You can load a self signed certificate into the Pact Broker to be used for https connections using [script/insert-self-signed-certificate-from-url.rb](https://github.com/pact-foundation/pact_broker/blob/master/script/insert-self-signed-certificate-from-url.rb) in the
+Pact Broker Github repository.
 
 * **HTTP method**: Must be included in the `webhook_http_method_whitelist`, which by default only includes `POST`. It is highly recommended that only `POST` requests are allowed to ensure that webhooks cannot be used to retrieve sensitive information from hosts within the same network.
 
+* **Host**: If the `webhook_host_whitelist` contains any entries, the host must match one or more of the entries. By default, it is empty. For security purposes, if the host whitelist is empty, the response details will not be logged to the UI (though they can be seen in the application logs at debug level).
+
+  The host whitelist may contain hostnames (eg `"github.com"`), IPs (eg `"192.0.345.4"`), network ranges (eg `"10.0.0.0/8"`) or regular expressions (eg `/.*\.foo\.com$/`). Note that IPs are not resolved, so if you specify an IP range, you need to use the IP in the webhook URL. If you wish to allow webhooks to any host (not recommended!), you can set `webhook_host_whitelist` to `[/.*/]`. Beware of any sensitive endpoints that may be exposed within the same network.
+
+  The recommended set of values to start with are:
+
+    * your CI server's hostname (for triggering builds)
+    * your company chat (eg. Slack, for publishing notifications)
+    * your code repository (eg. Github, for sending commit statuses)
+
+  Alternatively, you could use a regular expression to limit requests to your company's domain. eg `/.*\.foo\.com$/` (don't forget the end of string anchor). You can test Ruby regular expressions at [rubular.com](http://rubular.com).
+
 #### Event types
 
 `contract_content_changed:` triggered when the content of the contract has changed since the previous publication. Uses plain string equality, so changes to the ordering of hash keys, or whitespace changes will trigger this webhook.

lib/pact_broker/domain/webhook_request.rb

@@ -68,15 +68,14 @@ def execute_and_build_result pact, options, logs, execution_logger
         uri = build_uri(pact)
         req = build_request(uri, pact, execution_logger)
         response = do_request(uri, req)
-        log_response(response, execution_logger)
+        log_response(response, execution_logger, options)
         result = WebhookExecutionResult.new(response, logs.string)
         log_completion_message(options, execution_logger, result.success?)
         result
       end
 
       def handle_error_and_build_result e, options, logs, execution_logger
-        logger.error "Error executing webhook #{uuid} #{e.class.name} - #{e.message} #{e.backtrace.join("\n")}"
-        execution_logger.error "Error executing webhook #{uuid} #{e.class.name} - #{e.message}"
+        log_error(e, execution_logger, options)
         log_completion_message(options, execution_logger, false)
         WebhookExecutionResult.new(nil, logs.string, e)
       end
@@ -96,7 +95,7 @@ def build_request uri, pact, execution_logger
           req.body = PactBroker::Webhooks::Render.call(String === body ? body : body.to_json, pact)
         end
 
-        execution_logger.info req.body
+        execution_logger.info(req.body) if req.body
         req
       end
 
@@ -108,15 +107,33 @@ def do_request uri, req
         end
       end
 
-      def log_response response, execution_logger
-        execution_logger.info(" ")
+      def log_response response, execution_logger, options
+        log_response_to_application_logger(response)
+        if options[:show_response]
+          log_response_to_execution_logger(response, execution_logger)
+        else
+          execution_logger.info response_body_hidden_message
+        end
+      end
+
+      def response_body_hidden_message
+        PactBroker::Messages.message('messages.response_body_hidden')
+      end
+
+      def log_response_to_application_logger response
         logger.info "Received response for webhook #{uuid} status=#{response.code}"
+        logger.debug "headers=#{response.to_hash}"
+        logger.debug "body=#{response.body}"
+      end
+
+      def log_response_to_execution_logger response, execution_logger
         execution_logger.info "HTTP/#{response.http_version} #{response.code} #{response.message}"
         response.each_header do | header |
           execution_logger.info "#{header.split("-").collect(&:capitalize).join('-')}: #{response[header]}"
         end
-        logger.debug "body=#{response.body}"
+
         safe_body = nil
+
         if response.body
           safe_body = response.body.encode('UTF-8', 'binary', invalid: :replace, undef: :replace, replace: '')
           if response.body != safe_body
@@ -138,6 +155,16 @@ def log_completion_message options, execution_logger, success
         end
       end
 
+      def log_error e, execution_logger, options
+        logger.error "Error executing webhook #{uuid} #{e.class.name} - #{e.message} #{e.backtrace.join("\n")}"
+
+        if options[:show_response]
+          execution_logger.error "Error executing webhook #{uuid} #{e.class.name} - #{e.message}"
+        else
+          execution_logger.error "Error executing webhook #{uuid}. #{response_body_hidden_message}"
+        end
+      end
+
       def to_s
         "#{method.upcase} #{url}, username=#{username}, password=#{display_password}, headers=#{headers}, body=#{body}"
       end

lib/pact_broker/locale/en.yml

@@ -9,6 +9,8 @@ en:
     valid_consumer_version_number?: "Consumer version number '%{value}' cannot be parsed to a version number. The expected format (unless this configuration has been overridden) is a semantic version. eg. 1.3.0 or 2.0.4.rc1"
 
   pact_broker:
+    messages:
+      response_body_hidden: For security purposes, the response details are not logged. To enable response logging, configure the webhook_host_whitelist property. See /doc/webhooks#whitelist for more information.
     errors:
       validation:
         blank: "cannot be blank."

lib/pact_broker/webhooks/service.rb

@@ -61,7 +61,8 @@ def self.find_all
 
       def self.execute_webhook_now webhook, pact
         triggered_webhook = webhook_repository.create_triggered_webhook(next_uuid, webhook, pact, USER)
-        webhook_execution_result = execute_triggered_webhook_now triggered_webhook, failure_log_message: "Webhook execution failed"
+        options = { failure_log_message: "Webhook execution failed", show_response: PactBroker.configuration.show_webhook_response?}
+        webhook_execution_result = execute_triggered_webhook_now triggered_webhook, options
         if webhook_execution_result.success?
           webhook_repository.update_triggered_webhook_status triggered_webhook, TriggeredWebhook::STATUS_SUCCESS
         else

lib/pact_broker/webhooks/webhook.rb

@@ -5,7 +5,6 @@
 module PactBroker
   module Webhooks
     class Webhook < Sequel::Model
-
       set_primary_key :id
       associate(:many_to_one, :provider, :class => "PactBroker::Domain::Pacticipant", :key => :provider_id, :primary_key => :id)
       associate(:many_to_one, :consumer, :class => "PactBroker::Domain::Pacticipant", :key => :consumer_id, :primary_key => :id)
@@ -84,13 +83,11 @@ def self.properties_hash_from_domain webhook
           is_json_request_body: is_json_request_body
         }
       end
-
     end
 
     Webhook.plugin :timestamps, :update_on_create=>true
 
     class WebhookHeader < Sequel::Model
-
       associate(:many_to_one, :webhook, :class => "PactBroker::Repositories::Webhook", :key => :webhook_id, :primary_key => :id)
 
       def self.from_domain name, value, webhook_id
@@ -100,8 +97,6 @@ def self.from_domain name, value, webhook_id
         db_header.webhook_id = webhook_id
         db_header
       end
-
     end
   end
-
 end

spec/features/create_webhook_spec.rb

@@ -76,5 +76,4 @@
       expect(response_body).to include webhook_hash
     end
   end
-
 end

spec/features/execute_webhook_spec.rb

@@ -23,8 +23,9 @@
 
   context "when the execution is successful" do
     let!(:request) do
-      stub_request(:post, /http/).with(body: 'http://example.org/pacts/provider/Bar/consumer/Foo/version/1').to_return(:status => 200)
+      stub_request(:post, /http/).with(body: 'http://example.org/pacts/provider/Bar/consumer/Foo/version/1').to_return(:status => 200, body: response_body)
     end
+    let(:response_body) { "webhook-response-body" }
 
     it "performs the HTTP request" do
       subject
@@ -42,6 +43,36 @@
     it "saves a WebhookExecution" do
       expect { subject }.to change { PactBroker::Webhooks::Execution.count }.by(1)
     end
+
+    context "when a webhook host whitelist is not configured" do
+      before do
+        allow(PactBroker.configuration).to receive(:show_webhook_response?).and_return(false)
+      end
+
+      it "does not show any response details" do
+        expect(subject.body).to_not include response_body
+      end
+
+      it "does not log any response details" do
+        subject
+        expect(PactBroker::Webhooks::Execution.order(:id).last.logs).to_not include response_body
+      end
+    end
+
+    context "when a webhook host whitelist is configured" do
+      before do
+        allow(PactBroker.configuration).to receive(:show_webhook_response?).and_return(true)
+      end
+
+      it "includes response details" do
+        expect(subject.body).to include response_body
+      end
+
+      it "logs the response details" do
+        subject
+        expect(PactBroker::Webhooks::Execution.order(:id).last.logs).to include response_body
+      end
+    end
   end
 
   context "when an error occurs", no_db_clean: true do

spec/lib/pact_broker/api/decorators/webhook_execution_result_decorator_spec.rb

@@ -14,10 +14,11 @@ module Decorators
           let(:response) { double('http_response', code: '200', body: response_body, to_hash: headers) }
           let(:response_body) { 'body' }
           let(:error) { nil }
-          let(:webhook) { instance_double(PactBroker::Domain::Webhook, uuid: 'some-uuid')}
+          let(:webhook) { instance_double(PactBroker::Domain::Webhook, uuid: 'some-uuid') }
+          let(:show_response) { true }
           let(:json) {
             WebhookExecutionResultDecorator.new(webhook_execution_result)
-            .to_json(user_options: { base_url: 'http://example.org', webhook: webhook })
+            .to_json(user_options: { base_url: 'http://example.org', webhook: webhook, show_response: show_response })
           }
 
           let(:subject) { JSON.parse(json, symbolize_names: true)}
@@ -62,8 +63,19 @@ module Decorators
               end
             end
           end
-        end
 
+          context "when show_response is false" do
+            let(:show_response) { false }
+
+            it "does not include the response" do
+              expect(subject).to_not have_key(:response)
+            end
+
+            it "includes a message about why the response is hidden" do
+              expect(subject[:message]).to match /security purposes/
+            end
+          end
+        end
       end
     end
   end

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

@@ -57,7 +57,8 @@ module Resources
               end
 
               it "generates a JSON response body for the execution result" do
-                expect(decorator).to receive(:to_json).with(user_options: { base_url: 'http://example.org', webhook: webhook })
+                allow(PactBroker.configuration).to receive(:show_webhook_response?).and_return('foo')
+                expect(decorator).to receive(:to_json).with(user_options: { base_url: 'http://example.org', webhook: webhook, show_response: 'foo' })
                 subject
               end
 
@@ -69,6 +70,7 @@ module Resources
 
             context "when execution is not successful" do
               let(:success) { false }
+
               it "returns a 500 JSON response" do
                 subject
                 expect(last_response.status).to eq 500