From becf20c5b8cbcac831ce88e3955b597dbad7ac1b Mon Sep 17 00:00:00 2001 From: Beth Skurrie Date: Tue, 29 May 2018 13:10:52 +1000 Subject: [PATCH] fix: temporarily redact webhook response body from UI for security purposes --- .../webhook_execution_result_decorator.rb | 10 +++++--- lib/pact_broker/domain/webhook_request.rb | 23 ++++++++++--------- ...webhook_execution_result_decorator_spec.rb | 8 +++++-- .../domain/webhook_request_spec.rb | 21 +++++++++++++---- 4 files changed, 41 insertions(+), 21 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..d8a8e681b 100644 --- a/lib/pact_broker/api/decorators/webhook_execution_result_decorator.rb +++ b/lib/pact_broker/api/decorators/webhook_execution_result_decorator.rb @@ -34,11 +34,15 @@ def body represented.body end end - end - property :error, :extend => ErrorDecorator - property :response, :extend => HTTPResponseDecorator + property :message, exec_context: :decorator + # property :error, :extend => ErrorDecorator + # property :response, :extend => HTTPResponseDecorator + + def message + "Webhook response has been redacted temporarily for security purposes. Please see the Pact Broker application logs for the response body." + end link :webhook do | options | { diff --git a/lib/pact_broker/domain/webhook_request.rb b/lib/pact_broker/domain/webhook_request.rb index 2572b7db5..9315ce4bb 100644 --- a/lib/pact_broker/domain/webhook_request.rb +++ b/lib/pact_broker/domain/webhook_request.rb @@ -114,18 +114,19 @@ def log_response response, execution_logger execution_logger.info(" ") logger.info "Received response for webhook #{uuid} status=#{response.code}" 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 + #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 - execution_logger.debug "Note that invalid UTF-8 byte sequences were removed from response body before saving the logs" - end - end - execution_logger.info safe_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 + # execution_logger.debug "Note that invalid UTF-8 byte sequences were removed from response body before saving the logs" + # end + # end + #execution_logger.info safe_body + execution_logger.info "Webhook response has been redacted temporarily for security purposes. Please see the Pact Broker application logs for the response body." end def log_completion_message options, execution_logger, success 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..1177eb2b2 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 @@ -30,7 +30,11 @@ module Decorators expect(subject[:_links][:webhook][:href]).to eq 'http://example.org/webhooks/some-uuid' end - context "when there is an error" do + it "includes a message about the response being redacted" do + expect(subject[:message]).to match /redacted/ + end + + context "when there is an error", pending: "temporarily disabled" do let(:error) { double('error', message: 'message', backtrace: ['blah','blah']) } it "includes the message" do @@ -42,7 +46,7 @@ module Decorators end end - context "when there is a response" do + context "when there is a response", pending: "temporarily disabled" do it "includes the response code" do expect(subject[:response][:status]).to eq 200 end diff --git a/spec/lib/pact_broker/domain/webhook_request_spec.rb b/spec/lib/pact_broker/domain/webhook_request_spec.rb index 3fa434c16..ddc4b9c51 100644 --- a/spec/lib/pact_broker/domain/webhook_request_spec.rb +++ b/spec/lib/pact_broker/domain/webhook_request_spec.rb @@ -8,6 +8,9 @@ module Domain before do allow(PactBroker::Api::PactBrokerUrls).to receive(:pact_url).and_return('http://example.org/pact-url') allow(PactBroker.configuration).to receive(:base_url).and_return('http://example.org') + allow(PactBroker.logger).to receive(:info).and_call_original + allow(PactBroker.logger).to receive(:debug).and_call_original + allow(PactBroker.logger).to receive(:warn).and_call_original end let(:username) { nil } @@ -121,6 +124,14 @@ module Domain subject.execute(pact, options) end + it "does not write the response body to the exeuction log for security purposes" do + expect(logs).to_not include "An error" + end + + it "logs a message about why there is no response information" do + expect(logs).to include "Webhook response has been redacted temporarily for security purposes" + end + describe "execution logs" do it "logs the request method and path" do @@ -143,12 +154,12 @@ module Domain expect(logs).to include "HTTP/1.0 200" end - it "logs the response headers" do - expect(logs).to include "Content-Type: text/foo, blah" + it "does not log the response headers" do + expect(logs).to_not include "Content-Type: text/foo, blah" end - it "logs the response body" do - expect(logs).to include "respbod" + it "does not log the response body" do + expect(logs).to_not include "respbod" end context "when the response code is a success" do @@ -280,7 +291,7 @@ module Domain end end - context "when the response body contains a non UTF-8 character" do + context "when the response body contains a non UTF-8 character", pending: "execution logs disabled temporarily for security purposes" do let!(:http_request) do stub_request(:post, "http://example.org/hook"). to_return(:status => 200, :body => "This has some \xC2 invalid chars")