feat: validate webhook scheme and http method against configurable lists on creation

Author: bethesque

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
 

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
 

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!
 
+<a name="whitelist"></a>
+#### 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.

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"
           },

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

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": {

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|