From 102b1930bd306330bb322f5c76e048106d420981 Mon Sep 17 00:00:00 2001 From: Beth Skurrie Date: Fri, 7 Oct 2022 11:26:59 +1100 Subject: [PATCH] feat(webhooks): allow auth headers to be logged for debugging purposes (#575) --- .../config/runtime_configuration.rb | 1 + .../runtime_configuration_logging_methods.rb | 3 + .../test/http_test_data_builder.rb | 60 +++++++++++-------- .../webhooks/execution_configuration.rb | 4 ++ .../execution_configuration_creator.rb | 1 + .../webhooks/webhook_request_logger.rb | 2 +- script/data/webhook.rb | 1 + ...untime_configuration_documentation_spec.rb | 3 +- .../webhooks/webhook_request_logger_spec.rb | 11 +++- 9 files changed, 57 insertions(+), 29 deletions(-) diff --git a/lib/pact_broker/config/runtime_configuration.rb b/lib/pact_broker/config/runtime_configuration.rb index e2a3a7152..86a0a78da 100644 --- a/lib/pact_broker/config/runtime_configuration.rb +++ b/lib/pact_broker/config/runtime_configuration.rb @@ -52,6 +52,7 @@ class RuntimeConfiguration < Anyway::Config webhook_http_code_success: [200, 201, 202, 203, 204, 205, 206], webhook_scheme_whitelist: ["https"], webhook_host_whitelist: [], + webhook_redact_sensitive_data: true, disable_ssl_verification: false, user_agent: "Pact Broker v#{PactBroker::VERSION}" ) diff --git a/lib/pact_broker/config/runtime_configuration_logging_methods.rb b/lib/pact_broker/config/runtime_configuration_logging_methods.rb index 145339533..1b574606a 100644 --- a/lib/pact_broker/config/runtime_configuration_logging_methods.rb +++ b/lib/pact_broker/config/runtime_configuration_logging_methods.rb @@ -31,6 +31,9 @@ def log_configuration(logger) source: source_info.dig(key, :source) || {:type=>:defaults} } end.sort_by { |key, _| key }.each { |key, value| log_config_inner(key, value, logger) } + if self.webhook_redact_sensitive_data == false + logger.warn("WARNING!!! webhook_redact_sensitive_data is set to false. This will allow authentication information to be included in the webhook logs. This should only be used for debugging purposes. Do not run the application permanently in production with this value.") + end end def log_config_inner(key, value, logger) diff --git a/lib/pact_broker/test/http_test_data_builder.rb b/lib/pact_broker/test/http_test_data_builder.rb index d84d2565b..9964ef99e 100644 --- a/lib/pact_broker/test/http_test_data_builder.rb +++ b/lib/pact_broker/test/http_test_data_builder.rb @@ -265,7 +265,9 @@ def create_webhook_for_event(uuid: nil, url: "https://postman-echo.com/post", bo "request" => { "method" => "POST", "url" => url, - "body" => body || default_body + "body" => body || default_body, + "username" => "user", + "password" => "pass" } }.compact path = "webhooks/#{uuid}" @@ -286,32 +288,9 @@ def create_global_webhook_for_contract_requiring_verification_published(uuid: ni def create_global_webhook_for_anything_published(uuid: nil, url: "https://postman-echo.com/post") puts "Creating global webhook for contract changed event with uuid #{uuid}" uuid ||= SecureRandom.uuid - request_body = { - "description" => "A webhook for all consumers and providers", - "events" => [{ - "name" => "contract_published" - },{ - "name" => "provider_verification_published" - }], - "request" => { - "method" => "POST", - "url" => url, - "headers" => { "Content-Type" => "application/json"}, - "body" => { - "eventName" => "${pactbroker.eventName}", - "consumerVersionNumber" => "${pactbroker.consumerVersionNumber}", - "consumerVersionTags" => "${pactbroker.consumerVersionTags}", - "consumerVersionBranch" => "${pactbroker.consumerVersionBranch}", - "githubVerificationStatus" => "${pactbroker.githubVerificationStatus}", - "providerVersionNumber" => "${pactbroker.providerVersionNumber}", - "providerVersionTags" => "${pactbroker.providerVersionTags}", - "providerVersionBranch" => "${pactbroker.providerVersionBranch}", - "canIMerge" => "${pactbroker.providerMainBranchGithubVerificationStatus}" - } - } - } + path = "webhooks/#{uuid}" - client.put(path, request_body.to_json).tap { |response| check_for_error(response) } + client.put(path, webhook_body_with_all_parameters(url).to_json).tap { |response| check_for_error(response) } separate self end @@ -438,6 +417,35 @@ def check_for_error(response) puts response.body end end + + def webhook_body_with_all_parameters(url) + { + "description" => "A webhook for all consumers and providers", + "events" => [{ + "name" => "contract_published" + },{ + "name" => "provider_verification_published" + }], + "request" => { + "method" => "POST", + "url" => url, + "headers" => { "Content-Type" => "application/json"}, + "username" => "user", + "password" => "pass", + "body" => { + "eventName" => "${pactbroker.eventName}", + "consumerVersionNumber" => "${pactbroker.consumerVersionNumber}", + "consumerVersionTags" => "${pactbroker.consumerVersionTags}", + "consumerVersionBranch" => "${pactbroker.consumerVersionBranch}", + "githubVerificationStatus" => "${pactbroker.githubVerificationStatus}", + "providerVersionNumber" => "${pactbroker.providerVersionNumber}", + "providerVersionTags" => "${pactbroker.providerVersionTags}", + "providerVersionBranch" => "${pactbroker.providerVersionBranch}", + "canIMerge" => "${pactbroker.providerMainBranchGithubVerificationStatus}" + } + } + } + end end end end diff --git a/lib/pact_broker/webhooks/execution_configuration.rb b/lib/pact_broker/webhooks/execution_configuration.rb index 599c4b243..0c31dff70 100644 --- a/lib/pact_broker/webhooks/execution_configuration.rb +++ b/lib/pact_broker/webhooks/execution_configuration.rb @@ -25,6 +25,10 @@ def with_failure_log_message(value) with_updated_attribute(logging_options: { failure_log_message: value }) end + def with_redact_sensitive_data(value) + with_updated_attribute(logging_options: { redact_sensitive_data: value }) + end + def with_retry_schedule(value) with_updated_attribute(retry_schedule: value) end diff --git a/lib/pact_broker/webhooks/execution_configuration_creator.rb b/lib/pact_broker/webhooks/execution_configuration_creator.rb index f05c165a9..accbc10c7 100644 --- a/lib/pact_broker/webhooks/execution_configuration_creator.rb +++ b/lib/pact_broker/webhooks/execution_configuration_creator.rb @@ -10,6 +10,7 @@ class ExecutionConfigurationCreator def self.call(resource) PactBroker::Webhooks::ExecutionConfiguration.new .with_show_response(PactBroker.configuration.show_webhook_response?) + .with_redact_sensitive_data(PactBroker.configuration.webhook_redact_sensitive_data) .with_retry_schedule(PactBroker.configuration.webhook_retry_schedule) .with_http_success_codes(PactBroker.configuration.webhook_http_code_success) .with_user_agent(PactBroker.configuration.user_agent) diff --git a/lib/pact_broker/webhooks/webhook_request_logger.rb b/lib/pact_broker/webhooks/webhook_request_logger.rb index 3928128bf..bea501564 100644 --- a/lib/pact_broker/webhooks/webhook_request_logger.rb +++ b/lib/pact_broker/webhooks/webhook_request_logger.rb @@ -51,7 +51,7 @@ def log_webhook_context(webhook_context) end def log_request(webhook_request) - http_request = HttpRequestWithRedactedHeaders.new(webhook_request.http_request) + http_request = options[:redact_sensitive_data] ? HttpRequestWithRedactedHeaders.new(webhook_request.http_request) : webhook_request.http_request logger.info "Making webhook #{webhook_request.uuid} request #{http_request.method.upcase} URI=#{webhook_request.url} (headers and body in debug logs)" logger.debug "Webhook #{webhook_request.uuid} request headers=#{http_request.to_hash}" logger.debug "Webhook #{webhook_request.uuid} request body=#{http_request.body}" diff --git a/script/data/webhook.rb b/script/data/webhook.rb index ac57f93ef..014b08430 100755 --- a/script/data/webhook.rb +++ b/script/data/webhook.rb @@ -12,6 +12,7 @@ .delete_pacticipant("bar-provider") .create_pacticipant("foo-consumer") .create_pacticipant("bar-provider") + .create_version(pacticipant: "bar-provider", branch: "main", version: "1") .publish_pact_the_old_way(consumer: "foo-consumer", consumer_version: "1", provider: "bar-provider", content_id: "111", tag: "main") .get_pacts_for_verification( provider: "bar-provider", diff --git a/spec/lib/pact_broker/config/runtime_configuration_documentation_spec.rb b/spec/lib/pact_broker/config/runtime_configuration_documentation_spec.rb index 6388aef7f..7ba26a4de 100644 --- a/spec/lib/pact_broker/config/runtime_configuration_documentation_spec.rb +++ b/spec/lib/pact_broker/config/runtime_configuration_documentation_spec.rb @@ -18,7 +18,8 @@ module Config :semver_formats, :seed_example_data, :use_first_tag_as_branch_time_limit, - :validate_database_connection_config + :validate_database_connection_config, + :webhook_redact_sensitive_data ] (ATTRIBUTES - DELIBERATELY_UNDOCUMENTED_ATTRIBUTES).each do | attribute_name | diff --git a/spec/lib/pact_broker/webhooks/webhook_request_logger_spec.rb b/spec/lib/pact_broker/webhooks/webhook_request_logger_spec.rb index 0f9d4696a..5c7e668f9 100644 --- a/spec/lib/pact_broker/webhooks/webhook_request_logger_spec.rb +++ b/spec/lib/pact_broker/webhooks/webhook_request_logger_spec.rb @@ -16,7 +16,8 @@ module Webhooks let(:logger) { double("logger").as_null_object } let(:uuid) { "uuid" } - let(:options) { { failure_log_message: "oops", show_response: show_response, http_code_success: [200] } } + let(:options) { { failure_log_message: "oops", show_response: show_response, http_code_success: [200], redact_sensitive_data: redact_sensitive_data } } + let(:redact_sensitive_data) { true } let(:show_response) { true } let(:username) { nil } let(:password) { nil } @@ -144,6 +145,14 @@ module Webhooks it "logs the Authorization header with a starred value" do expect(logs).to include "authorization: **********" end + + context "when redact_sensitive_data is false" do + let(:redact_sensitive_data) { false } + + it "logs the real value of the Authorization header" do + expect(logs).to include "authorization: foo" + end + end end context "when the response body contains a non UTF-8 character" do