From 619c7e9374b6768e5775ac265da2d4a0590c8ba4 Mon Sep 17 00:00:00 2001 From: Beth Skurrie Date: Sat, 9 Jun 2018 20:38:00 +1000 Subject: [PATCH] feat: redact potentially sensitive headers in the webhook resource --- .../decorators/webhook_request_decorator.rb | 6 +++- lib/pact_broker/domain/webhook_request.rb | 14 ++++++-- .../api/decorators/webhook_decorator_spec.rb | 7 ++++ .../domain/webhook_request_spec.rb | 35 +++++++++++++++++-- 4 files changed, 56 insertions(+), 6 deletions(-) diff --git a/lib/pact_broker/api/decorators/webhook_request_decorator.rb b/lib/pact_broker/api/decorators/webhook_request_decorator.rb index 907094410..0413a6b90 100644 --- a/lib/pact_broker/api/decorators/webhook_request_decorator.rb +++ b/lib/pact_broker/api/decorators/webhook_request_decorator.rb @@ -7,11 +7,15 @@ class WebhookRequestDecorator < BaseDecorator property :method property :url - property :headers, getter: lambda { | _ | headers.empty? ? nil : headers } + property :headers, getter: lambda { | _ | headers.empty? ? nil : self.redacted_headers } property :body property :username property :password, getter: lambda { | _ | display_password } + + def redacted_headers + represented.headers + end end end end diff --git a/lib/pact_broker/domain/webhook_request.rb b/lib/pact_broker/domain/webhook_request.rb index 7f9d2b800..a8cce4c36 100644 --- a/lib/pact_broker/domain/webhook_request.rb +++ b/lib/pact_broker/domain/webhook_request.rb @@ -4,7 +4,6 @@ require 'pact_broker/logging' require 'pact_broker/messages' require 'net/http' -require 'pact_broker/webhooks/redact_logs' require 'pact_broker/webhooks/render' require 'pact_broker/api/pact_broker_urls' require 'pact_broker/build_http_options' @@ -27,6 +26,7 @@ class WebhookRequest include PactBroker::Logging include PactBroker::Messages + HEADERS_TO_REDACT = [/authorization/i, /token/i] attr_accessor :method, :url, :headers, :body, :username, :password, :uuid @@ -52,6 +52,13 @@ def display_password password.nil? ? nil : "**********" end + def redacted_headers + headers.each_with_object({}) do | (name, value), new_headers | + redact = HEADERS_TO_REDACT.any?{ | pattern | name =~ pattern } + new_headers[name] = redact ? "**********" : value + end + end + def execute pact, verification, options = {} logs = StringIO.new execution_logger = Logger.new(logs) @@ -84,8 +91,9 @@ def build_request uri, pact, verification, execution_logger req = http_request(uri) execution_logger.info "HTTP/1.1 #{method.upcase} #{url_with_credentials(pact, verification)}" + headers_to_log = redacted_headers headers.each_pair do | name, value | - execution_logger.info Webhooks::RedactLogs.call("#{name}: #{value}") + execution_logger.info "#{name}: #{headers_to_log[name]}" req[name] = value end @@ -166,7 +174,7 @@ def log_error e, execution_logger, options end def to_s - "#{method.upcase} #{url}, username=#{username}, password=#{display_password}, headers=#{headers}, body=#{body}" + "#{method.upcase} #{url}, username=#{username}, password=#{display_password}, headers=#{redacted_headers}, body=#{body}" end def http_request(uri) diff --git a/spec/lib/pact_broker/api/decorators/webhook_decorator_spec.rb b/spec/lib/pact_broker/api/decorators/webhook_decorator_spec.rb index f28957315..d1f20700f 100644 --- a/spec/lib/pact_broker/api/decorators/webhook_decorator_spec.rb +++ b/spec/lib/pact_broker/api/decorators/webhook_decorator_spec.rb @@ -88,6 +88,13 @@ module Decorators expect(parsed_json[:request]).to_not have_key :headers end end + + context 'when the headers contain sensitve information' do + let(:headers) { { 'Authorization' => 'foo' } } + it 'redacts them' do + expect(parsed_json[:request][:headers][:'Authorization']).to eq "**********" + end + end end describe 'from_json' do diff --git a/spec/lib/pact_broker/domain/webhook_request_spec.rb b/spec/lib/pact_broker/domain/webhook_request_spec.rb index 7b49bb6c0..cc88829fd 100644 --- a/spec/lib/pact_broker/domain/webhook_request_spec.rb +++ b/spec/lib/pact_broker/domain/webhook_request_spec.rb @@ -16,6 +16,7 @@ module Domain let(:username) { nil } let(:password) { nil } let(:url) { 'http://example.org/hook' } + let(:headers) { {'Content-Type' => 'text/plain', 'Authorization' => 'foo'} } let(:body) { 'body' } let(:logs) { StringIO.new } let(:execution_logger) { Logger.new(logs) } @@ -29,7 +30,7 @@ module Domain WebhookRequest.new( method: 'post', url: url, - headers: {'Content-Type' => 'text/plain', 'Authorization' => 'foo'}, + headers: headers, username: username, password: password, body: body) @@ -58,6 +59,36 @@ module Domain end end + describe "redacted_headers" do + let(:headers) do + { + 'Authorization' => 'foo', + 'X-authorization' => 'bar', + 'token' => 'bar', + 'Token' => 'bar', + 'X-Auth-Token' => 'bar', + 'X-Authorization-Token' => 'bar', + 'OK' => 'ok' + } + end + + let(:expected_headers) do + { + 'Authorization' => '**********', + 'X-authorization' => '**********', + 'token' => '**********', + 'Token' => '**********', + 'X-Auth-Token' => '**********', + 'X-Authorization-Token' => '**********', + 'OK' => 'ok' + } + end + + it "redacts sensitive headers" do + expect(subject.redacted_headers).to eq expected_headers + end + end + describe "execute" do let!(:http_request) do stub_request(:post, "http://example.org/hook"). @@ -126,7 +157,7 @@ module Domain end it "redacts potentially sensitive headers" do - expect(logs).to include "Authorization: [REDACTED]" + expect(logs).to include "Authorization: **********" end it "logs the request body" do