From cc978582e39cdcb5e9b9ab1235dc60052f60acdf Mon Sep 17 00:00:00 2001 From: Beth Skurrie Date: Tue, 28 May 2019 11:54:48 +1000 Subject: [PATCH] feat(webhooks): maintain starred out Authorization header value --- lib/pact_broker/domain/webhook_request.rb | 1 + lib/pact_broker/webhooks/service.rb | 33 +++++++--- .../webhooks/webhook_request_template.rb | 6 +- spec/lib/pact_broker/webhooks/service_spec.rb | 62 ++++++++++++++++++- 4 files changed, 93 insertions(+), 9 deletions(-) diff --git a/lib/pact_broker/domain/webhook_request.rb b/lib/pact_broker/domain/webhook_request.rb index 31db5960a..a637378df 100644 --- a/lib/pact_broker/domain/webhook_request.rb +++ b/lib/pact_broker/domain/webhook_request.rb @@ -9,6 +9,7 @@ require 'pact_broker/build_http_options' require 'cgi' require 'delegate' +require 'rack/utils' module PactBroker diff --git a/lib/pact_broker/webhooks/service.rb b/lib/pact_broker/webhooks/service.rb index 4e60ef699..d57bb494a 100644 --- a/lib/pact_broker/webhooks/service.rb +++ b/lib/pact_broker/webhooks/service.rb @@ -43,13 +43,7 @@ def self.find_by_uuid uuid def self.update_by_uuid uuid, params webhook = webhook_repository.find_by_uuid(uuid) - # Dirty hack to maintain existing password if it is not submitted - # This is required because the password is not returned in the API response - # for security purposes, so it needs to be re-entered with every response. - # TODO implement proper 'secrets' management. - if webhook.request.password && !params['request'].key?('password') - params['request']['password'] = webhook.request.password - end + maintain_redacted_params(webhook, params) PactBroker::Api::Decorators::WebhookDecorator.new(webhook).from_hash(params) webhook_repository.update_by_uuid uuid, webhook end @@ -171,6 +165,31 @@ def self.find_triggered_webhooks_for_pact pact def self.find_triggered_webhooks_for_verification verification webhook_repository.find_triggered_webhooks_for_verification(verification) end + + private + + # Dirty hack to maintain existing password or Authorization header if it is submitted with value **** + # This is required because the password and Authorization header is **** out in the API response + # for security purposes, so it would need to be re-entered with every response. + # TODO implement proper 'secrets' management. + def self.maintain_redacted_params(webhook, params) + if webhook.request.password && password_key_does_not_exist_or_is_starred?(params) + params['request']['password'] = webhook.request.password + end + + new_headers = params['request']['headers'] ||= {} + existing_headers = webhook.request.headers + starred_new_headers = new_headers.select { |key, value| value =~ /^\**$/ } + starred_new_headers.each do | (key, value) | + new_headers[key] = existing_headers[key] + end + params['request']['headers'] = new_headers + params + end + + def self.password_key_does_not_exist_or_is_starred?(params) + !params['request'].key?('password') || params.dig('request','password') =~ /^\**$/ + end end end end diff --git a/lib/pact_broker/webhooks/webhook_request_template.rb b/lib/pact_broker/webhooks/webhook_request_template.rb index 9ef3aa35e..9bbec569e 100644 --- a/lib/pact_broker/webhooks/webhook_request_template.rb +++ b/lib/pact_broker/webhooks/webhook_request_template.rb @@ -22,7 +22,7 @@ def initialize attributes = {} @url = attributes[:url] @username = attributes[:username] @password = attributes[:password] - @headers = attributes[:headers] || {} + @headers = Rack::Utils::HeaderHash.new(attributes[:headers] || {}) @body = attributes[:body] @uuid = attributes[:uuid] end @@ -64,6 +64,10 @@ def redacted_headers end end + def headers= headers + @headers = Rack::Utils::HeaderHash.new(headers) + end + private def to_s diff --git a/spec/lib/pact_broker/webhooks/service_spec.rb b/spec/lib/pact_broker/webhooks/service_spec.rb index d1efab48e..5854a84bb 100644 --- a/spec/lib/pact_broker/webhooks/service_spec.rb +++ b/spec/lib/pact_broker/webhooks/service_spec.rb @@ -38,7 +38,9 @@ module Webhooks allow_any_instance_of(PactBroker::Webhooks::Repository).to receive(:find_by_uuid).and_return(existing_webhook) end - let(:request) { PactBroker::Webhooks::WebhookRequestTemplate.new(password: 'password')} + let(:request) { PactBroker::Webhooks::WebhookRequestTemplate.new(password: existing_password, headers: headers)} + let(:existing_password) { nil } + let(:headers) { {} } let(:existing_webhook) { PactBroker::Domain::Webhook.new(request: request) } let(:params) do { @@ -50,7 +52,19 @@ module Webhooks subject { Service.update_by_uuid("1234", params) } + it "sends through the params to the repository" do + updated_webhook = nil + allow_any_instance_of(PactBroker::Webhooks::Repository).to receive(:update_by_uuid) do | instance, uuid, webhook | + updated_webhook = webhook + true + end + subject + expect(updated_webhook.request.url).to eq 'http://url' + end + context "when the webhook has a password and the incoming parameters do not contain a password" do + let(:existing_password) { 'password' } + it "does not overwite the password" do updated_webhook = nil allow_any_instance_of(PactBroker::Webhooks::Repository).to receive(:update_by_uuid) do | instance, uuid, webhook | @@ -62,6 +76,52 @@ module Webhooks end end + context "when the webhook has a password and the incoming parameters contain a *** password" do + let(:existing_password) { 'password' } + let(:params) do + { + 'request' => { + 'url' => 'http://url', + 'password' => '*******' + } + } + end + + it "does not overwite the password" do + updated_webhook = nil + allow_any_instance_of(PactBroker::Webhooks::Repository).to receive(:update_by_uuid) do | instance, uuid, webhook | + updated_webhook = webhook + true + end + subject + expect(updated_webhook.request.password).to eq 'password' + end + end + + context "when the webhook has an authorization header and the incoming parameters contain a *** authorization header" do + let(:headers) { { 'Authorization' => 'existing'} } + let(:params) do + { + 'request' => { + 'url' => "http://url", + 'headers' => { + 'authorization' => "***********" + } + } + } + end + + it "does not overwite the authorization header" do + updated_webhook = nil + allow_any_instance_of(PactBroker::Webhooks::Repository).to receive(:update_by_uuid) do | instance, uuid, webhook | + updated_webhook = webhook + true + end + subject + expect(updated_webhook.request.headers['Authorization']).to eq 'existing' + end + end + context "the incoming parameters contain a password" do let(:params) do {