Skip to content

Commit

Permalink
feat: validate webhook host against configurable list on creation
Browse files Browse the repository at this point in the history
  • Loading branch information
bethesque committed Jun 7, 2018
1 parent d7a2b0a commit 077e37f
Show file tree
Hide file tree
Showing 5 changed files with 120 additions and 6 deletions.
24 changes: 21 additions & 3 deletions lib/pact_broker/api/contracts/webhook_contract.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
require 'reform'
require 'reform/form'
require 'pact_broker/webhooks/check_host_whitelist'

module PactBroker
module Api
Expand Down Expand Up @@ -41,7 +42,8 @@ def self.messages
en: {
errors: {
allowed_webhook_method?: http_method_error_message,
allowed_webhook_scheme?: scheme_error_message
allowed_webhook_scheme?: scheme_error_message,
allowed_webhook_host?: host_error_message
}
}
)
Expand All @@ -56,7 +58,11 @@ def self.http_method_error_message
end

def self.scheme_error_message
"must be #{PactBroker.configuration.webhook_scheme_whitelist.join(", ")}. See /doc/webhooks#whitelist for more information."
"scheme must be #{PactBroker.configuration.webhook_scheme_whitelist.join(", ")}. See /doc/webhooks#whitelist for more information."
end

def self.host_error_message
"host must be in the whitelist #{PactBroker.configuration.webhook_host_whitelist.join(",")}. See /doc/webhooks#whitelist for more information."
end

def valid_method?(http_method)
Expand All @@ -82,10 +88,22 @@ def allowed_webhook_scheme?(url)
scheme.downcase == allowed_scheme.downcase
end
end

def allowed_webhook_host?(url)
if host_whitelist.any?
PactBroker::Webhooks::CheckHostWhitelist.call(URI(url).host, host_whitelist).any?
else
true
end
end

def host_whitelist
PactBroker.configuration.webhook_host_whitelist
end
end

required(:http_method).filled(:valid_method?, :allowed_webhook_method?)
required(:url).filled(:valid_url?, :allowed_webhook_scheme?)
required(:url).filled(:valid_url?, :allowed_webhook_scheme?, :allowed_webhook_host?)
end
end

Expand Down
5 changes: 3 additions & 2 deletions lib/pact_broker/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,10 @@ class Configuration
attr_accessor :validate_database_connection_config, :enable_diagnostic_endpoints, :version_parser, :sha_generator
attr_accessor :use_case_sensitive_resource_names, :order_versions_by_date
attr_accessor :check_for_potential_duplicate_pacticipant_names
attr_accessor :webhook_http_method_whitelist, :webhook_scheme_whitelist
attr_accessor :webhook_http_method_whitelist, :webhook_scheme_whitelist, :webhook_host_whitelist
attr_accessor :webhook_retry_schedule
attr_accessor :semver_formats
attr_accessor :enable_public_badge_access, :shields_io_base_url
attr_accessor :webhook_retry_schedule
attr_accessor :disable_ssl_verification
attr_accessor :base_equality_only_on_content_that_affects_verification_results
attr_reader :api_error_reporters
Expand Down Expand Up @@ -77,6 +77,7 @@ def self.default_configuration
config.disable_ssl_verification = false
config.webhook_http_method_whitelist = ['POST']
config.webhook_scheme_whitelist = ['https']
config.webhook_host_whitelist = []
config
end

Expand Down
22 changes: 22 additions & 0 deletions lib/pact_broker/webhooks/check_host_whitelist.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
module PactBroker
module Webhooks
class CheckHostWhitelist

def self.call(host, whitelist = PactBroker.configuration.webhook_host_whitelist)
whitelist.select{ | whitelist_host | match?(host, whitelist_host) }
end

def self.match?(host, whitelist_host)
if whitelist_host.is_a?(Regexp)
host =~ whitelist_host
else
begin
IPAddr.new(whitelist_host) === IPAddr.new(host)
rescue IPAddr::Error
host == whitelist_host
end
end
end
end
end
end
28 changes: 27 additions & 1 deletion spec/lib/pact_broker/api/contracts/webhook_contract_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,14 @@ def valid_webhook_with
describe "errors" do
before do
PactBroker.configuration.webhook_http_method_whitelist = webhook_http_method_whitelist
PactBroker.configuration.webhook_host_whitelist = webhook_host_whitelist
allow(PactBroker::Webhooks::CheckHostWhitelist).to receive(:call).and_return(whitelist_matches)
subject.validate(hash)
end

let(:webhook_http_method_whitelist) { ['POST'] }
let(:whitelist_matches) { ['foo'] }
let(:webhook_host_whitelist) { [] }

context "with valid fields" do
it "is empty" do
Expand Down Expand Up @@ -92,7 +96,7 @@ def valid_webhook_with
end

it "contains an error for the URL" do
expect(subject.errors[:"request.url"]).to include "must be https. See /doc/webhooks#whitelist for more information."
expect(subject.errors[:"request.url"]).to include "scheme must be https. See /doc/webhooks#whitelist for more information."
end
end

Expand All @@ -118,6 +122,28 @@ def valid_webhook_with
end
end

context "when the host whitelist is empty" do
it "does not attempt to validate the host" do
expect(PactBroker::Webhooks::CheckHostWhitelist).to_not have_received(:call)
end
end

context "when the host whitelist is populated" do
let(:webhook_host_whitelist) { [/foo/, "bar"] }

it "validates the host" do
expect(PactBroker::Webhooks::CheckHostWhitelist).to have_received(:call).with("some.url", webhook_host_whitelist)
end

context "when the host does not match the whitelist" do
let(:whitelist_matches) { [] }

it "contains an error", pending: "need to work out how to do dynamic messages" do
expect(subject.errors[:"request.url"]).to include "host must be in the whitelist /foo/, \"bar\" . See /doc/webhooks#whitelist for more information."
end
end
end

context "with no URL" do
let(:json) do
valid_webhook_with do |hash|
Expand Down
47 changes: 47 additions & 0 deletions spec/lib/pact_broker/webhooks/check_host_whitelist_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
require 'pact_broker/webhooks/check_host_whitelist'

module PactBroker
module Webhooks
describe CheckHostWhitelist do
context "when the host is 10.0.0.7" do
let(:host) { "10.0.1.0" }

it "matches 10.0.0.0/8" do
expect(CheckHostWhitelist.call(host, ["10.0.0.0/8"])).to eq ["10.0.0.0/8"]
end

it "matches 10.0.1.0" do
expect(CheckHostWhitelist.call(host, [host])).to eq [host]
end

it "does not match 10.0.0.2" do
expect(CheckHostWhitelist.call(host, ["10.0.0.2"])).to eq []
end

it "does not match 10.0.0.0/28" do
expect(CheckHostWhitelist.call(host, ["10.0.0.0/28"])).to eq []
end
end

context "when the host is localhost" do
let(:host) { "localhost" }

it "matches localhost" do
expect(CheckHostWhitelist.call(host, [host])).to eq [host]
end

it "matches /local.*/" do
expect(CheckHostWhitelist.call(host, [/local*/])).to eq [/local*/]
end

it "does not match foo" do
expect(CheckHostWhitelist.call(host, ["foo"])).to eq []
end

it "does not match /foo.*/" do
expect(CheckHostWhitelist.call(host, [/foo*/])).to eq []
end
end
end
end
end

0 comments on commit 077e37f

Please sign in to comment.