From 3e1c562bb4658ba1cbf26d98a884d382a9c65696 Mon Sep 17 00:00:00 2001
From: Beth Skurrie <beth@bethesque.com>
Date: Sun, 3 Jun 2018 17:17:20 +1000
Subject: [PATCH] feat: only log webhook response details when a webhook host
 whitelist has been configured

---
 .../webhook_execution_result_decorator.rb     | 18 +++---
 .../api/resources/webhook_execution.rb        |  7 ++-
 lib/pact_broker/configuration.rb              |  4 ++
 lib/pact_broker/doc/views/webhooks.markdown   | 15 ++++-
 lib/pact_broker/domain/webhook_request.rb     | 41 ++++++++++---
 lib/pact_broker/locale/en.yml                 |  2 +
 lib/pact_broker/webhooks/service.rb           |  3 +-
 lib/pact_broker/webhooks/webhook.rb           |  5 --
 spec/features/create_webhook_spec.rb          |  1 -
 spec/features/execute_webhook_spec.rb         | 33 +++++++++-
 ...webhook_execution_result_decorator_spec.rb | 18 +++++-
 .../api/resources/webhook_execution_spec.rb   |  4 +-
 .../domain/webhook_request_spec.rb            | 60 ++++++++++++++++---
 spec/lib/pact_broker/webhooks/service_spec.rb |  7 +++
 14 files changed, 178 insertions(+), 40 deletions(-)

diff --git a/lib/pact_broker/api/decorators/webhook_execution_result_decorator.rb b/lib/pact_broker/api/decorators/webhook_execution_result_decorator.rb
index 6f7e64e93..e6ae1c916 100644
--- a/lib/pact_broker/api/decorators/webhook_execution_result_decorator.rb
+++ b/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
\ No newline at end of file
+end
diff --git a/lib/pact_broker/api/resources/webhook_execution.rb b/lib/pact_broker/api/resources/webhook_execution.rb
index d33236866..eae508b46 100644
--- a/lib/pact_broker/api/resources/webhook_execution.rb
+++ b/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
diff --git a/lib/pact_broker/configuration.rb b/lib/pact_broker/configuration.rb
index e91e40b0c..7ca24d34c 100644
--- a/lib/pact_broker/configuration.rb
+++ b/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
diff --git a/lib/pact_broker/doc/views/webhooks.markdown b/lib/pact_broker/doc/views/webhooks.markdown
index 7f4d6f607..94aee6e2a 100644
--- a/lib/pact_broker/doc/views/webhooks.markdown
+++ b/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.
diff --git a/lib/pact_broker/domain/webhook_request.rb b/lib/pact_broker/domain/webhook_request.rb
index be95eba78..663a68bdf 100644
--- a/lib/pact_broker/domain/webhook_request.rb
+++ b/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
diff --git a/lib/pact_broker/locale/en.yml b/lib/pact_broker/locale/en.yml
index 78f9e728a..cf7de260e 100644
--- a/lib/pact_broker/locale/en.yml
+++ b/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."
diff --git a/lib/pact_broker/webhooks/service.rb b/lib/pact_broker/webhooks/service.rb
index 5f02b7cbe..1e9fb9fc7 100644
--- a/lib/pact_broker/webhooks/service.rb
+++ b/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
diff --git a/lib/pact_broker/webhooks/webhook.rb b/lib/pact_broker/webhooks/webhook.rb
index 4165ae60b..14ba932e6 100644
--- a/lib/pact_broker/webhooks/webhook.rb
+++ b/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
diff --git a/spec/features/create_webhook_spec.rb b/spec/features/create_webhook_spec.rb
index b95853f60..0aea0bb4e 100644
--- a/spec/features/create_webhook_spec.rb
+++ b/spec/features/create_webhook_spec.rb
@@ -76,5 +76,4 @@
       expect(response_body).to include webhook_hash
     end
   end
-
 end
diff --git a/spec/features/execute_webhook_spec.rb b/spec/features/execute_webhook_spec.rb
index 2214c2738..af199572c 100644
--- a/spec/features/execute_webhook_spec.rb
+++ b/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
diff --git a/spec/lib/pact_broker/api/decorators/webhook_execution_result_decorator_spec.rb b/spec/lib/pact_broker/api/decorators/webhook_execution_result_decorator_spec.rb
index 7f8a57901..205efcda0 100644
--- a/spec/lib/pact_broker/api/decorators/webhook_execution_result_decorator_spec.rb
+++ b/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
diff --git a/spec/lib/pact_broker/api/resources/webhook_execution_spec.rb b/spec/lib/pact_broker/api/resources/webhook_execution_spec.rb
index 7617cf084..ab220c838 100644
--- a/spec/lib/pact_broker/api/resources/webhook_execution_spec.rb
+++ b/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
diff --git a/spec/lib/pact_broker/domain/webhook_request_spec.rb b/spec/lib/pact_broker/domain/webhook_request_spec.rb
index 69fd50299..2a4ca2a43 100644
--- a/spec/lib/pact_broker/domain/webhook_request_spec.rb
+++ b/spec/lib/pact_broker/domain/webhook_request_spec.rb
@@ -18,7 +18,8 @@ module Domain
       let(:body) { 'body' }
       let(:logs) { StringIO.new }
       let(:execution_logger) { Logger.new(logs) }
-      let(:options) { {failure_log_message: 'oops'}}
+      let(:options) { {failure_log_message: 'oops', show_response: show_response} }
+      let(:show_response) { true }
       let(:pact) { instance_double('PactBroker::Domain::Pact') }
 
       subject do
@@ -105,6 +106,7 @@ module Domain
           allow(PactBroker.logger).to receive(:info)
           allow(PactBroker.logger).to receive(:debug)
           expect(PactBroker.logger).to receive(:info).with(/response.*200/)
+          expect(PactBroker.logger).to receive(:debug).with(/content-type/)
           expect(PactBroker.logger).to receive(:debug).with(/respbod/)
           subject.execute(pact, options)
         end
@@ -135,16 +137,38 @@ module Domain
             expect(logs).to include body
           end
 
-          it "logs the response status" do
-            expect(logs).to include "HTTP/1.0 200"
-          end
+          context "when show_response is true" do
+            it "logs the response status" do
+              expect(logs).to include "HTTP/1.0 200"
+            end
+
+            it "logs the response headers" do
+              expect(logs).to include "Content-Type: text/foo, blah"
+            end
 
-          it "logs the response headers" do
-            expect(logs).to include "Content-Type: text/foo, blah"
+            it "logs the response body" do
+              expect(logs).to include "respbod"
+            end
           end
 
-          it "logs the response body" do
-            expect(logs).to include "respbod"
+          context "when show_response is false" do
+            let(:show_response) { false }
+
+            it "does not log the response status" do
+              expect(logs).to_not include "HTTP/1.0 200"
+            end
+
+            it "does not log the response headers" do
+              expect(logs).to_not include "Content-Type: text/foo, blah"
+            end
+
+            it "does not log the response body" do
+              expect(logs).to_not include "respbod"
+            end
+
+            it "logs a message about why the response is hidden" do
+              expect(logs).to include "security purposes"
+            end
           end
 
           context "when the response code is a success" do
@@ -286,10 +310,10 @@ class WebhookTestError < StandardError; end
 
           before do
             allow(subject).to receive(:http_request).and_raise(WebhookTestError.new("blah"))
+            allow(PactBroker.logger).to receive(:error)
           end
 
           it "logs the error" do
-            allow(PactBroker.logger).to receive(:error)
             expect(PactBroker.logger).to receive(:error).with(/Error.*WebhookTestError.*blah/)
             subject.execute(pact, options)
           end
@@ -305,6 +329,24 @@ class WebhookTestError < StandardError; end
           it "logs the failure_log_message" do
             expect(logs).to include "oops"
           end
+
+          context "when show_response is true" do
+            it "logs the exception information" do
+              expect(logs).to include "blah"
+            end
+          end
+
+          context "when show_response is false" do
+            let(:show_response) { false }
+
+            it "does not logs the exception information" do
+              expect(logs).to_not include "blah"
+            end
+
+            it "logs a message about why the response is hidden" do
+              expect(logs).to include "security purposes"
+            end
+          end
         end
       end
     end
diff --git a/spec/lib/pact_broker/webhooks/service_spec.rb b/spec/lib/pact_broker/webhooks/service_spec.rb
index abd062c31..7753b879a 100644
--- a/spec/lib/pact_broker/webhooks/service_spec.rb
+++ b/spec/lib/pact_broker/webhooks/service_spec.rb
@@ -102,6 +102,13 @@ module Webhooks
 
         subject { PactBroker::Webhooks::Service.execute_webhook_now td.webhook, pact }
 
+        it "executes the triggered webhook with the correct options" do
+          allow(PactBroker.configuration).to receive(:show_webhook_response?).and_return('foo')
+          expected_options = { :failure_log_message => "Webhook execution failed", :show_response => 'foo' }
+          expect_any_instance_of(PactBroker::Webhooks::TriggeredWebhook).to receive(:execute).with(expected_options).and_call_original
+          subject
+        end
+
         it "executes the HTTP request of the webhook" do
           subject
           expect(http_request).to have_been_made