fix: check that request body does not contain any invalid UTF-8 chara…

…cters before JSON parsing

JSON parse allows characters to be deserialised that cannot be serialised.

lib/pact_broker/api/contracts/publish_contracts_schema.rb

@@ -2,6 +2,7 @@
 require "pact_broker/api/contracts/dry_validation_workarounds"
 require "pact_broker/api/contracts/dry_validation_predicates"
 require "pact_broker/messages"
+require "pact_broker/api/contracts/utf_8_validation"
 
 module PactBroker
   module Api
@@ -11,6 +12,10 @@ class PublishContractsSchema
         using PactBroker::HashRefinements
         extend PactBroker::Messages
 
+        class << self
+          include PactBroker::Api::Contracts::UTF8Validation
+        end
+
         SCHEMA = Dry::Validation.Schema do
           configure do
             predicates(DryValidationPredicates)
@@ -83,6 +88,14 @@ def self.validate_encoding(contract, i, errors)
           if contract[:decodedContent].nil?
             add_contract_error(:content, message("errors.base64?", scope: nil), i, errors)
           end
+
+          if contract[:decodedContent]
+            char_number, fragment = fragment_before_invalid_utf_8_char(contract[:decodedContent])
+            if char_number
+              error_message = message("errors.non_utf_8_char_in_contract", char_number: char_number, fragment: fragment)
+              add_contract_error(:content, error_message, i, errors)
+            end
+          end
         end
 
         def self.validate_content_matches_content_type(contract, i, errors)
@@ -91,7 +104,6 @@ def self.validate_content_matches_content_type(contract, i, errors)
           end
         end
 
-
         def self.add_contract_error(field, message, i, errors)
           errors[:contracts] ||= {}
           errors[:contracts][i] ||= {}

lib/pact_broker/api/contracts/utf_8_validation.rb

@@ -0,0 +1,17 @@
+module PactBroker
+  module Api
+    module Contracts
+      module UTF8Validation
+        def fragment_before_invalid_utf_8_char(string)
+          string.force_encoding('UTF-8').each_char.with_index do | char, index |
+            if !char.valid_encoding?
+              fragment = index < 100 ? string[0...index] : string[index-100...index]
+              return index + 1, fragment
+            end
+          end
+          nil
+        end
+      end
+    end
+  end
+end

lib/pact_broker/api/resources/base_resource.rb

@@ -11,6 +11,7 @@
 require "pact_broker/api/resources/authorization"
 require "pact_broker/errors"
 require "pact_broker/api/resources/error_handling_methods"
+require "pact_broker/api/contracts/utf_8_validation"
 
 module PactBroker
   module Api
@@ -24,6 +25,7 @@ class BaseResource < Webmachine::Resource
         include PactBroker::Api::Resources::Authentication
         include PactBroker::Api::Resources::Authorization
         include PactBroker::Api::Resources::ErrorHandlingMethods
+        include PactBroker::Api::Contracts::UTF8Validation
 
         include PactBroker::Logging
 
@@ -124,25 +126,10 @@ def params(options = {})
             @params_with_string_keys ||= JSON.parse(request_body, { symbolize_names: false }.merge(PACT_PARSING_OPTIONS)) #Not load! Otherwise it will try to load Ruby classes.
           end
         rescue StandardError => e
-          fragment = fragment_before_invalid_utf_8_char
-
-          if fragment
-            raise NonUTF8CharacterFound.new(message("errors.non_utf_8_char_in_request_body", char_number: fragment.length + 1, fragment: fragment))
-          else
-            raise InvalidJsonError.new(e.message)
-          end
+          raise InvalidJsonError.new(e.message)
         end
         # rubocop: enable Metrics/CyclomaticComplexity
 
-        def fragment_before_invalid_utf_8_char
-          request_body.each_char.with_index do | char, index |
-            if !char.valid_encoding?
-              return index < 100 ? request_body[0...index] : request_body[index-100...index]
-            end
-          end
-          nil
-        end
-
         def params_with_string_keys
           params(symbolize_names: false)
         end
@@ -193,13 +180,17 @@ def pacticipant_specified?
 
         def invalid_json?
           begin
-            params
-            false
-          rescue NonUTF8CharacterFound => e
-            logger.info(e.message) # Don't use the default SemanticLogger error logging method because it will try and print out the cause which will contain non UTF-8 chars in the message
-            set_json_error_message(e.message)
-            response.headers["Content-Type"] = error_response_content_type
-            true
+            char_number, fragment = fragment_before_invalid_utf_8_char(request_body)
+            if char_number
+              error_message = message("errors.non_utf_8_char_in_request_body", char_number: char_number, fragment: fragment)
+              logger.info(error_message)
+              set_json_error_message(error_message)
+              response.headers["Content-Type"] = error_response_content_type
+              true
+            else
+              params
+              false
+            end
           rescue StandardError => e
             message = "#{e.cause ? e.cause.class.name : e.class.name} - #{e.message}"
             logger.info(message)

lib/pact_broker/locale/en.yml

@@ -109,6 +109,7 @@ en:
       new_line_in_url_path: URL path cannot contain a new line character.
       tab_in_url_path: URL path cannot contain a tab character.
       non_utf_8_char_in_request_body: "Request body has a non UTF-8 character at char %{char_number} and cannot be parsed as JSON. Fragment preceding invalid character is: '%{fragment}'"
+      non_utf_8_char_in_contract: "Contract has a non UTF-8 character at char %{char_number} and cannot be parsed as JSON. Fragment preceding invalid character is: '%{fragment}'"
 
       "400":
         title: 400 Malformed Request

spec/features/publish_pact_all_in_one_spec.rb

@@ -40,6 +40,12 @@
     it { is_expected.to be_a_json_error_response("missing") }
   end
 
+  context "with a contract that contains a non utf-8 char" do
+    let(:contract) { "{\"key\": \"ABCDEFG\x8FDEF\" }" }
+
+    its(:status) { is_expected.to eq 400 }
+  end
+
   context "with a conflicting pact" do
     before do
       allow(PactBroker.configuration).to receive(:allow_dangerous_contract_modification).and_return(false)

spec/integration/endpoints/non_utf_8_spec.rb

@@ -20,7 +20,7 @@
     end
 
     it "truncates the fragement included in the error message" do
-      expect(JSON.parse(subject.body)).to eq("error" => "Request body has a non UTF-8 character at char 101 and cannot be parsed as JSON. Fragment preceding invalid character is: '1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890'")
+      expect(JSON.parse(subject.body)).to eq("error" => "Request body has a non UTF-8 character at char 111 and cannot be parsed as JSON. Fragment preceding invalid character is: '1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890'")
     end
   end
 end

spec/lib/pact_broker/api/contracts/publish_contracts_schema_spec.rb

@@ -121,6 +121,16 @@ module Contracts
             expect(subject[:contracts]).to include "specification is missing at index 0"
           end
         end
+
+        context "when there is a non UTF-8 character in the base64 decoded contract" do
+          let(:encoded_contract) { Base64.strict_encode64(decoded_content) }
+          let(:decoded_content) { "{\"key\": \"ABCDEFG\x8FDEF\" }" }
+          let(:decoded_parsed_content) { PactBroker::Pacts::Parse.call(decoded_content) }
+
+          it "returns an error" do
+            expect(subject[:contracts].first).to include "UTF-8 character at char 17"
+          end
+        end
       end
     end
   end