diff --git a/lib/pact_broker/api/resources/all_webhooks.rb b/lib/pact_broker/api/resources/all_webhooks.rb index 1a6ca1478..59cfd15ad 100644 --- a/lib/pact_broker/api/resources/all_webhooks.rb +++ b/lib/pact_broker/api/resources/all_webhooks.rb @@ -30,10 +30,7 @@ def post_is_create? end def malformed_request? - if request.post? - return invalid_json? || validation_errors?(webhook) - end - false + super || (request.post? && validation_errors?(webhook)) end def to_json diff --git a/lib/pact_broker/api/resources/base_resource.rb b/lib/pact_broker/api/resources/base_resource.rb index 332932c86..769e8366e 100644 --- a/lib/pact_broker/api/resources/base_resource.rb +++ b/lib/pact_broker/api/resources/base_resource.rb @@ -15,6 +15,7 @@ module PactBroker module Api module Resources class InvalidJsonError < PactBroker::Error ; end + class NonUTF8CharacterFound < PactBroker::Error ; end class BaseResource < Webmachine::Resource include PactBroker::Services @@ -39,6 +40,10 @@ def known_methods super + ["PATCH"] end + def malformed_request? + content_type_is_json_but_invalid_json_provided? + end + def finish_request application_context.after_resource&.call(self) PactBroker.configuration.after_resource.call(self) @@ -125,17 +130,32 @@ def params(options = {}) else @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 JSON::JSONError => e - raise InvalidJsonError.new("Error parsing JSON - #{e.message}") + 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 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 def pact_params - @pact_params ||= PactBroker::Pacts::PactParams.from_request request, identifier_from_path + @pact_params ||= PactBroker::Pacts::PactParams.from_request(request, identifier_from_path) end def set_json_error_message message @@ -192,9 +212,12 @@ def invalid_json? begin params false + rescue NonUTF8CharacterFound => e + set_json_error_message(e.message) + response.headers["Content-Type"] = "application/hal+json;charset=utf-8" + true rescue StandardError => e - logger.info "Error parsing JSON #{e} - #{request_body}" - set_json_error_message "Error parsing JSON - #{e.message}" + set_json_error_message("#{e.cause ? e.cause.class.name : e.class.name} - #{e.message}") response.headers["Content-Type"] = "application/hal+json;charset=utf-8" true end @@ -279,6 +302,10 @@ def validation_errors_for_schema?(schema_to_use = schema, params_to_validate = p def malformed_request_for_json_with_schema?(schema_to_use = schema, params_to_validate = params) invalid_json? || validation_errors_for_schema?(schema_to_use, params_to_validate) end + + def content_type_is_json_but_invalid_json_provided? + request.content_type&.include?("json") && any_request_body? && invalid_json? + end end end end diff --git a/lib/pact_broker/api/resources/currently_deployed_versions_for_environment.rb b/lib/pact_broker/api/resources/currently_deployed_versions_for_environment.rb index 45a1f48a3..b51013c7a 100644 --- a/lib/pact_broker/api/resources/currently_deployed_versions_for_environment.rb +++ b/lib/pact_broker/api/resources/currently_deployed_versions_for_environment.rb @@ -8,10 +8,6 @@ module Resources class CurrentlyDeployedVersionsForEnvironment < BaseResource using PactBroker::StringRefinements - def content_types_accepted - [["application/json", :from_json]] - end - def content_types_provided [["application/hal+json", :to_json]] end diff --git a/lib/pact_broker/api/resources/currently_supported_versions_for_environment.rb b/lib/pact_broker/api/resources/currently_supported_versions_for_environment.rb index 1e93b639f..c6a65a8f3 100644 --- a/lib/pact_broker/api/resources/currently_supported_versions_for_environment.rb +++ b/lib/pact_broker/api/resources/currently_supported_versions_for_environment.rb @@ -8,10 +8,6 @@ module Resources class CurrentlySupportedVersionsForEnvironment < BaseResource using PactBroker::StringRefinements - def content_types_accepted - [["application/json", :from_json]] - end - def content_types_provided [["application/hal+json", :to_json]] end diff --git a/lib/pact_broker/api/resources/deployed_version.rb b/lib/pact_broker/api/resources/deployed_version.rb index 94c09eeeb..e1071c808 100644 --- a/lib/pact_broker/api/resources/deployed_version.rb +++ b/lib/pact_broker/api/resources/deployed_version.rb @@ -8,11 +8,6 @@ module Resources class DeployedVersion < BaseResource include PactBroker::Messages - def initialize - super - @currently_deployed_param = params(default: {})[:currentlyDeployed] - end - def content_types_provided [ ["application/hal+json", :to_json] @@ -33,14 +28,6 @@ def resource_exists? !!deployed_version end - def malformed_request? - if request.patch? - return invalid_json? - else - false - end - end - def to_json decorator_class(:deployed_version_decorator).new(deployed_version).to_json(decorator_options) end @@ -73,7 +60,14 @@ def policy_record private - attr_reader :currently_deployed_param + # can't use ||= with a potentially nil value + def currently_deployed_param + if defined?(@currently_deployed_param) + @currently_deployed_param + else + @currently_deployed_param = params(default: {})[:currentlyDeployed] + end + end def process_currently_deployed_param if currently_deployed_param == false diff --git a/lib/pact_broker/api/resources/environment.rb b/lib/pact_broker/api/resources/environment.rb index 8eb62ca24..3cfbf3c95 100644 --- a/lib/pact_broker/api/resources/environment.rb +++ b/lib/pact_broker/api/resources/environment.rb @@ -22,11 +22,7 @@ def resource_exists? end def malformed_request? - if request.put? && environment - invalid_json? || validation_errors_for_schema?(schema, params.merge(uuid: uuid)) - else - false - end + super || (request.put? && environment && validation_errors_for_schema?(schema, params.merge(uuid: uuid))) end def from_json diff --git a/lib/pact_broker/api/resources/environments.rb b/lib/pact_broker/api/resources/environments.rb index 6fe2646ce..ff20ef38d 100644 --- a/lib/pact_broker/api/resources/environments.rb +++ b/lib/pact_broker/api/resources/environments.rb @@ -27,11 +27,7 @@ def post_is_create? end def malformed_request? - if request.post? - invalid_json? || validation_errors_for_schema?(schema, params.merge(uuid: uuid)) - else - false - end + super || (request.post? && validation_errors_for_schema?(schema, params.merge(uuid: uuid))) end def create_path diff --git a/lib/pact_broker/api/resources/pact.rb b/lib/pact_broker/api/resources/pact.rb index 3e4257466..212068156 100644 --- a/lib/pact_broker/api/resources/pact.rb +++ b/lib/pact_broker/api/resources/pact.rb @@ -42,12 +42,9 @@ def is_conflict? potential_duplicate_pacticipants?(pact_params.pacticipant_names) || merge_conflict || disallowed_modification? end + def malformed_request? - if request.patch? || request.put? - invalid_json? || contract_validation_errors?(Contracts::PutPactParamsContract.new(pact_params), pact_params) - else - false - end + super || ((request.patch? || request.really_put?) && contract_validation_errors?(Contracts::PutPactParamsContract.new(pact_params), pact_params)) end def resource_exists? diff --git a/lib/pact_broker/api/resources/pact_webhooks.rb b/lib/pact_broker/api/resources/pact_webhooks.rb index 26c23e7d6..cb44abcbb 100644 --- a/lib/pact_broker/api/resources/pact_webhooks.rb +++ b/lib/pact_broker/api/resources/pact_webhooks.rb @@ -25,10 +25,7 @@ def resource_exists? end def malformed_request? - if request.post? - return invalid_json? || validation_errors?(webhook) - end - false + super || (request.post? && validation_errors?(webhook)) end def validation_errors? webhook diff --git a/lib/pact_broker/api/resources/pacticipant.rb b/lib/pact_broker/api/resources/pacticipant.rb index 0958af03b..4045a4d28 100644 --- a/lib/pact_broker/api/resources/pacticipant.rb +++ b/lib/pact_broker/api/resources/pacticipant.rb @@ -26,11 +26,7 @@ def known_methods end def malformed_request? - if request.patch? || request.put? - invalid_json? || validation_errors_for_schema? - else - false - end + super || ((request.patch? || request.really_put?) && validation_errors_for_schema?) end def from_json diff --git a/lib/pact_broker/api/resources/pacticipant_webhooks.rb b/lib/pact_broker/api/resources/pacticipant_webhooks.rb index df40aedde..9610d171d 100644 --- a/lib/pact_broker/api/resources/pacticipant_webhooks.rb +++ b/lib/pact_broker/api/resources/pacticipant_webhooks.rb @@ -27,10 +27,7 @@ def resource_exists? end def malformed_request? - if request.post? - return invalid_json? || webhook_validation_errors?(webhook) - end - false + super || (request.post? && webhook_validation_errors?(webhook)) end def create_path diff --git a/lib/pact_broker/api/resources/pacticipants.rb b/lib/pact_broker/api/resources/pacticipants.rb index f8b4bf387..80e6a4a94 100644 --- a/lib/pact_broker/api/resources/pacticipants.rb +++ b/lib/pact_broker/api/resources/pacticipants.rb @@ -23,10 +23,7 @@ def allowed_methods end def malformed_request? - if request.post? - return invalid_json? || validation_errors_for_schema? - end - false + super || (request.post? && validation_errors_for_schema?) end def post_is_create? diff --git a/lib/pact_broker/api/resources/provider_pacts_for_verification.rb b/lib/pact_broker/api/resources/provider_pacts_for_verification.rb index f474984f3..9de99b124 100644 --- a/lib/pact_broker/api/resources/provider_pacts_for_verification.rb +++ b/lib/pact_broker/api/resources/provider_pacts_for_verification.rb @@ -20,12 +20,7 @@ def content_types_accepted end def malformed_request? - if (errors = query_schema.call(query)).any? - set_json_validation_error_messages(errors) - true - else - false - end + super || ((request.get? || request.post?) && schema_validation_errors?) end def process_post @@ -96,6 +91,15 @@ def log_request def nested_query @nested_query ||= Rack::Utils.parse_nested_query(request.uri.query) end + + def schema_validation_errors? + if (errors = query_schema.call(query)).any? + set_json_validation_error_messages(errors) + true + else + false + end + end end end end diff --git a/lib/pact_broker/api/resources/publish_contracts.rb b/lib/pact_broker/api/resources/publish_contracts.rb index 8bc125a5d..9fa063a6f 100644 --- a/lib/pact_broker/api/resources/publish_contracts.rb +++ b/lib/pact_broker/api/resources/publish_contracts.rb @@ -23,11 +23,7 @@ def allowed_methods end def malformed_request? - if request.post? - invalid_json? || validation_errors_for_schema? - else - false - end + super || (request.post? && validation_errors_for_schema?) end def process_post diff --git a/lib/pact_broker/api/resources/released_version.rb b/lib/pact_broker/api/resources/released_version.rb index e14f611e6..df82f597f 100644 --- a/lib/pact_broker/api/resources/released_version.rb +++ b/lib/pact_broker/api/resources/released_version.rb @@ -8,11 +8,6 @@ module Resources class ReleasedVersion < BaseResource include PactBroker::Messages - def initialize - super - @currently_supported_param = params(default: {})[:currentlySupported] - end - def content_types_provided [["application/hal+json", :to_json]] end @@ -63,7 +58,14 @@ def policy_record private - attr_reader :currently_supported_param + # can't use ||= with a potentially nil value + def currently_supported_param + if defined?(@currently_deployed_param) + @currently_supported_param + else + @currently_supported_param = params(default: {})[:currentlySupported] + end + end def process_currently_supported_param if currently_supported_param == false diff --git a/lib/pact_broker/api/resources/verifications.rb b/lib/pact_broker/api/resources/verifications.rb index bc1fa0275..6d323d537 100644 --- a/lib/pact_broker/api/resources/verifications.rb +++ b/lib/pact_broker/api/resources/verifications.rb @@ -34,15 +34,7 @@ def resource_exists? end def malformed_request? - if request.post? - return true if invalid_json? - errors = verification_service.errors(params) - if !errors.empty? - set_json_validation_error_messages(errors.messages) - return true - end - end - false + super || (request.post? && any_validation_errors?) end def create_path @@ -91,6 +83,12 @@ def event_context def verification_params params(symbolize_names: false).merge("wip" => wip?, "pending" => pending?) end + + def any_validation_errors? + errors = verification_service.errors(params) + set_json_validation_error_messages(errors.messages) if !errors.empty? + !errors.empty? + end end end end diff --git a/lib/pact_broker/api/resources/version.rb b/lib/pact_broker/api/resources/version.rb index 99e540bbf..cf23c8069 100644 --- a/lib/pact_broker/api/resources/version.rb +++ b/lib/pact_broker/api/resources/version.rb @@ -25,14 +25,6 @@ def resource_exists? !!version end - def malformed_request? - if request.put? && any_request_body? - invalid_json? - else - false - end - end - def from_json if request.really_put? handle_request do diff --git a/lib/pact_broker/api/resources/webhook.rb b/lib/pact_broker/api/resources/webhook.rb index 417e6a79c..2b717df2a 100644 --- a/lib/pact_broker/api/resources/webhook.rb +++ b/lib/pact_broker/api/resources/webhook.rb @@ -27,10 +27,7 @@ def resource_exists? end def malformed_request? - if request.put? - return invalid_json? || webhook_validation_errors?(parsed_webhook, uuid) - end - false + super || (request.put? && webhook_validation_errors?(parsed_webhook, uuid)) end def from_json diff --git a/lib/pact_broker/api/resources/webhook_execution.rb b/lib/pact_broker/api/resources/webhook_execution.rb index 8fd631950..656e2519a 100644 --- a/lib/pact_broker/api/resources/webhook_execution.rb +++ b/lib/pact_broker/api/resources/webhook_execution.rb @@ -37,14 +37,16 @@ def resource_exists? end def malformed_request? - if request.post? + if super + true + elsif request.post? if uuid false else webhook_validation_errors?(webhook) end else - super + false end end diff --git a/lib/pact_broker/locale/en.yml b/lib/pact_broker/locale/en.yml index a44b852d1..3b34628b5 100644 --- a/lib/pact_broker/locale/en.yml +++ b/lib/pact_broker/locale/en.yml @@ -107,6 +107,7 @@ en: To disable this check, set `check_for_potential_duplicate_pacticipant_names` to false in the configuration. 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}'" "400": title: 400 Malformed Request diff --git a/spec/integration/endpoints/non_utf_8_spec.rb b/spec/integration/endpoints/non_utf_8_spec.rb new file mode 100644 index 000000000..3534f0dfe --- /dev/null +++ b/spec/integration/endpoints/non_utf_8_spec.rb @@ -0,0 +1,40 @@ +RSpec.describe "a request to publish a pact with non-utf-8 chars" do + + subject { put("/pacts/provider/Bar/consumer/Foo/version/2", pact_content, { "CONTENT_TYPE" => "application/json"}) } + + context "with less than 100 chars preceding the invalid char" do + let(:pact_content) do + "ABCDEFG\x8FDEF" + end + + its(:status) { is_expected.to eq 400 } + + it "returns an error indicating where the non UTF-8 character is" do + expect(JSON.parse(subject.body)).to eq("error" => "Request body has a non UTF-8 character at char 8 and cannot be parsed as JSON. Fragment preceding invalid character is: 'ABCDEFG'") + end + end + + context "with more than 100 chars preceding the invalid char" do + let(:pact_content) do + "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890\x8FDEF" + 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'") + end + end +end + +RSpec.describe "a request to publish a pact with invalid JSON" do + let(:pact_content) do + "{" + end + + subject { put("/pacts/provider/Bar/consumer/Foo/version/2", pact_content, { "CONTENT_TYPE" => "application/json"}) } + + its(:status) { is_expected.to eq 400 } + + it "returns an error message" do + expect(JSON.parse(subject.body)).to eq("error" => "JSON::ParserError - 859: unexpected token at '{'") + end +end diff --git a/spec/lib/pact_broker/api/resources/base_resource_spec.rb b/spec/lib/pact_broker/api/resources/base_resource_spec.rb index 20822f111..f2991b74e 100644 --- a/spec/lib/pact_broker/api/resources/base_resource_spec.rb +++ b/spec/lib/pact_broker/api/resources/base_resource_spec.rb @@ -191,7 +191,8 @@ module Resources allow(path_info).to receive(:[]).with(:application_context).and_return(application_context) end let(:application_context) { PactBroker::ApplicationContext.default_application_context(before_resource: before_resource, after_resource: after_resource) } - let(:request) { double("request", uri: URI("http://example.org"), path_info: path_info, body: "{}").as_null_object } + let(:request) { double("request", uri: URI("http://example.org"), path_info: path_info, body: body).as_null_object } + let(:body) { "{}" } let(:path_info) { { pacticipant_name: "foo", pacticipant_version_number: "1" } } let(:response) { double("response").as_null_object } let(:resource) { resource_class.new(request, response) } @@ -217,6 +218,25 @@ module Resources it "has a policy_name method" do expect(resource).to respond_to(:policy_name) end + + describe "malformed_request?" do + context "an invalid UTF-8 character is used in the request body" do + before do + allow(request).to receive(:put?).and_return(true) + allow(request).to receive(:really_put?).and_return(true) + allow(request).to receive(:post?).and_return(true) + allow(request).to receive(:patch?).and_return(true) + end + + let(:body) { "ABCDEFG\x8FDEF" } + + it "returns true when it accepts a content type that includes json" do + if resource.content_types_accepted.collect(&:first).any?{ |ct| ct.include?("json") } + expect(resource.malformed_request?).to eq true + end + end + end + end end end end diff --git a/spec/lib/pact_broker/api/resources/pact_spec.rb b/spec/lib/pact_broker/api/resources/pact_spec.rb index 3dd88d245..3ce94e304 100644 --- a/spec/lib/pact_broker/api/resources/pact_spec.rb +++ b/spec/lib/pact_broker/api/resources/pact_spec.rb @@ -75,7 +75,7 @@ module Resources end it "returns an error message" do - expect(JSON.parse(response.body)["error"]).to match(/Error parsing JSON/) + expect(JSON.parse(response.body)["error"]).to match(/JSON/) end end