diff --git a/lib/pact_broker/api/contracts/webhook_contract.rb b/lib/pact_broker/api/contracts/webhook_contract.rb index 549c28bfa..1e94da42c 100644 --- a/lib/pact_broker/api/contracts/webhook_contract.rb +++ b/lib/pact_broker/api/contracts/webhook_contract.rb @@ -6,6 +6,19 @@ module Api module Contracts class WebhookContract < Reform::Form + def validate(*) + result = super + # I just cannot seem to get the validation to stop on the first error. + # If one rule fails, they all come back failed, and it's driving me nuts. + # Why on earth would I want that behaviour? + new_errors = Reform::Contract::Errors.new + errors.messages.each do | key, value | + new_errors.add(key, value.first) + end + @errors = new_errors + result + end + validation do configure do config.messages_file = File.expand_path("../../../locale/en.yml", __FILE__) @@ -23,20 +36,56 @@ class WebhookContract < Reform::Form configure do config.messages_file = File.expand_path("../../../locale/en.yml", __FILE__) - def valid_method?(value) - Net::HTTP.const_defined?(value.capitalize) + def self.messages + super.merge( + en: { + errors: { + allowed_webhook_method?: http_method_error_message, + allowed_webhook_scheme?: scheme_error_message + } + } + ) + end + + def self.http_method_error_message + if PactBroker.configuration.webhook_http_method_whitelist.size == 1 + "must be #{PactBroker.configuration.webhook_http_method_whitelist.first}. See /doc/webhooks#whitelist for more information." + else + "must be one of #{PactBroker.configuration.webhook_http_method_whitelist.join(", ")}. See /doc/webhooks#whitelist for more information." + end + end + + def self.scheme_error_message + "must be #{PactBroker.configuration.webhook_scheme_whitelist.join(", ")}. See /doc/webhooks#whitelist for more information." end - def valid_url?(value) - uri = URI(value) + def valid_method?(http_method) + Net::HTTP.const_defined?(http_method.capitalize) + end + + def valid_url?(url) + uri = URI(url) uri.scheme && uri.host rescue URI::InvalidURIError false end + + def allowed_webhook_method?(http_method) + PactBroker.configuration.webhook_http_method_whitelist.any? do | allowed_method | + http_method.downcase == allowed_method.downcase + end + end + + def allowed_webhook_scheme?(url) + scheme = URI(url).scheme + PactBroker.configuration.webhook_scheme_whitelist.any? do | allowed_scheme | + scheme.downcase == allowed_scheme.downcase + end + end end - required(:http_method).filled(:valid_method?) - required(:url).filled(:valid_url?) + required(:http_method).filled(:valid_method?, :allowed_webhook_method?) + required(:url).filled(:valid_url?, :allowed_webhook_scheme?) end end diff --git a/lib/pact_broker/configuration.rb b/lib/pact_broker/configuration.rb index c7a54c736..d3c076b34 100644 --- a/lib/pact_broker/configuration.rb +++ b/lib/pact_broker/configuration.rb @@ -30,6 +30,7 @@ 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 :semver_formats attr_accessor :enable_public_badge_access, :shields_io_base_url attr_accessor :webhook_retry_schedule @@ -74,6 +75,8 @@ def self.default_configuration config.webhook_retry_schedule = [10, 60, 120, 300, 600, 1200] #10 sec, 1 min, 2 min, 5 min, 10 min, 20 min => 38 minutes config.check_for_potential_duplicate_pacticipant_names = true config.disable_ssl_verification = false + config.webhook_http_method_whitelist = ['POST'] + config.webhook_scheme_whitelist = ['https'] config end diff --git a/lib/pact_broker/doc/views/webhooks.markdown b/lib/pact_broker/doc/views/webhooks.markdown index c55feddaa..7f4d6f607 100644 --- a/lib/pact_broker/doc/views/webhooks.markdown +++ b/lib/pact_broker/doc/views/webhooks.markdown @@ -54,6 +54,15 @@ A request body can be specified as well. **BEWARE** The password can be reverse engineered from the database, so make a separate account for the Pact Broker to use, don't use your personal account! + +#### Webhook Whitelist + +To ensure that webhooks cannot be used maliciously to expose either data about your contracts or your internal network, the following validation rules are applied to webhooks via the Pact Broker configuration settings. + +* **Scheme**: Must be included in the `webhook_scheme_whitelist`, which by default only includes `https`. You can change this to include `http` if absolutely necessary, however, keep in mind that the body of any http traffic is visible to the network. You can load a self signed certificate into the Pact Broker to be used for https connections using `script/insert-self-signed-certificate-from-url.rb` in the Pact Broker repository. + +* **HTTP method**: Must be included in the `webhook_http_method_whitelist`, which by default only includes `POST`. It is highly recommended that only `POST` requests are allowed to ensure that webhooks cannot be used to retrieve sensitive information from hosts within the same network. + #### Event types `contract_content_changed:` triggered when the content of the contract has changed since the previous publication. Uses plain string equality, so changes to the ordering of hash keys, or whitespace changes will trigger this webhook. diff --git a/spec/features/create_webhook_spec.rb b/spec/features/create_webhook_spec.rb index 0a06db136..b95853f60 100644 --- a/spec/features/create_webhook_spec.rb +++ b/spec/features/create_webhook_spec.rb @@ -43,7 +43,7 @@ }], request: { method: 'POST', - url: 'http://example.org', + url: 'https://example.org', headers: { :"Content-Type" => "application/json" }, diff --git a/spec/features/edit_webhook_spec.rb b/spec/features/edit_webhook_spec.rb index 95d499091..0fc39c54c 100644 --- a/spec/features/edit_webhook_spec.rb +++ b/spec/features/edit_webhook_spec.rb @@ -14,7 +14,7 @@ let(:response_body) { JSON.parse(last_response.body, symbolize_names: true)} let(:webhook_json) do h = load_json_fixture('webhook_valid.json') - h['request']['method'] = 'GET' + h['request']['url'] = 'https://bar.com' h.to_json end @@ -55,7 +55,7 @@ it "updates the webhook" do subject - expect(reloaded_webhook.request.method).to eq 'GET' + expect(reloaded_webhook.request.url).to eq 'https://bar.com' end end end diff --git a/spec/fixtures/webhook_valid.json b/spec/fixtures/webhook_valid.json index 8f224ebc0..ca4eb51cb 100644 --- a/spec/fixtures/webhook_valid.json +++ b/spec/fixtures/webhook_valid.json @@ -4,7 +4,7 @@ }], "request": { "method": "POST", - "url": "http://some.url", + "url": "HTTPS://some.url", "username": "username", "password": "password", "headers": { diff --git a/spec/lib/pact_broker/api/contracts/webhook_contract_spec.rb b/spec/lib/pact_broker/api/contracts/webhook_contract_spec.rb index 39fe4a278..ae3aeeab7 100644 --- a/spec/lib/pact_broker/api/contracts/webhook_contract_spec.rb +++ b/spec/lib/pact_broker/api/contracts/webhook_contract_spec.rb @@ -10,6 +10,7 @@ module Contracts let(:hash) { JSON.parse(json) } let(:webhook) { PactBroker::Api::Decorators::WebhookDecorator.new(Domain::Webhook.new).from_json(json) } let(:subject) { WebhookContract.new(webhook) } + let(:matching_hosts) { ['foo'] } def valid_webhook_with hash = load_json_fixture 'webhook_valid.json' @@ -18,11 +19,13 @@ def valid_webhook_with end describe "errors" do - before do + PactBroker.configuration.webhook_http_method_whitelist = webhook_http_method_whitelist subject.validate(hash) end + let(:webhook_http_method_whitelist) { ['POST'] } + context "with valid fields" do it "is empty" do expect(subject.errors).to be_empty @@ -81,6 +84,40 @@ def valid_webhook_with end end + context "with an invalid scheme" do + let(:json) do + valid_webhook_with do |hash| + hash['request']['url'] = 'ftp://foo' + end + 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." + end + end + + context "with an HTTP method that is not whitelisted" do + let(:json) do + valid_webhook_with do |hash| + hash['request']['method'] = 'DELETE' + end + end + + context "when there is only one allowed HTTP method" do + it "contains an error for invalid method" do + expect(subject.errors[:"request.http_method"]).to include "must be POST. See /doc/webhooks#whitelist for more information." + end + end + + context "when there is more than one allowed HTTP method", pending: "need to work out how to dynamically create this message" do + let(:webhook_http_method_whitelist) { ['POST', 'GET'] } + + it "contains an error for invalid method" do + expect(subject.errors[:"request.http_method"]).to include "must be one of POST, GET" + end + end + end + context "with no URL" do let(:json) do valid_webhook_with do |hash|