Prevent path parameters from masking invalid query parameters

lib/committee/schema_validator/open_api_3/operation_wrapper.rb

@@ -54,12 +54,12 @@ def validate_response_params(status_code, headers, response_data, strict, check_
           raise Committee::InvalidResponse.new(e.message, original_error: e)
         end
 
-        def validate_request_params(path_params, query_params, body_params, headers, validator_option)
+        def validate_request_params(_path_params, query_params, body_params, headers, validator_option)
           ret, err = case request_operation.http_method
                 when 'get', 'delete', 'head'
-                  validate_get_request_params(path_params, query_params, headers, validator_option)
+                  validate_get_request_params(query_params, headers, validator_option)
                 when 'post', 'put', 'patch', 'options'
-                  validate_post_request_params(path_params, query_params, body_params, headers, validator_option)
+                  validate_post_request_params(query_params, body_params, headers, validator_option)
                 else
                   raise "Committee OpenAPI3 not support #{request_operation.http_method} method"
                 end
@@ -126,42 +126,27 @@ def build_openapi_parser_option(validator_option, coerce_value)
           OpenAPIParser::SchemaValidator::Options.new(**parser_options)
         end
 
-        def validate_get_request_params(path_params, query_params, headers, validator_option)
-          # bad performance because when we coerce value, same check
-          validate_path_and_query_params(path_params, query_params, headers, validator_option)
+        def validate_get_request_params(query_params, headers, validator_option)
+          validate_query_params(query_params, headers, validator_option)
         rescue OpenAPIParser::OpenAPIError => e
           raise Committee::InvalidRequest.new(e.message, original_error: e)
         end
 
-        def validate_post_request_params(path_params, query_params, body_params, headers, validator_option)
+        def validate_post_request_params(query_params, body_params, headers, validator_option)
           content_type_key = headers.keys.detect { |k| k.casecmp?('Content-Type') }
           content_type = Rack::MediaType.type(headers[content_type_key])
 
-          # bad performance because when we coerce value, same check
-          validate_path_and_query_params(path_params, query_params, headers, validator_option)
+          validate_query_params(query_params, headers, validator_option)
           request_operation.validate_request_body(content_type, body_params, build_openapi_parser_body_option(validator_option))
         rescue => e
           raise Committee::InvalidRequest.new(e.message, original_error: e)
         end
 
-        def validate_path_and_query_params(path_params, query_params, headers, validator_option)
-          path_params ||= {}
+        def validate_query_params(query_params, headers, validator_option)
           query_params ||= {}
 
-          # it's currently impossible to validate path params and query params separately
-          # so we have to resort to this workaround
-
-          path_keys = path_params.keys.to_set
-          query_keys = query_params.keys.to_set
-
-          merged_params = query_params.merge(path_params)
-
-          request_operation.validate_request_parameter(merged_params, headers, build_openapi_parser_request_parameter_option(validator_option))
-
-          merged_params.each do |k, v|
-            path_params[k] = v if path_keys.include?(k)
-            query_params[k] = v if query_keys.include?(k)
-          end
+          # Path parameters have already been validated by coerce_path_parameter.
+          request_operation.validate_request_parameter(query_params, headers, build_openapi_parser_request_parameter_option(validator_option))
 
           validate_no_unknown_query_params(query_params) if validator_option.strict_query_params
         end

test/middleware/request_validation_open_api_3_test.rb

@@ -424,7 +424,7 @@ def app
         assert_equal env['committee.path_hash'][:integer], 84
         assert_equal env['committee.request_body_hash']['integer'], 21
         assert_equal env['committee.request_body_hash'][:integer], 21
-        assert_equal env['committee.query_hash']['integer'], 84 # we can't use query_parameter :(
+        assert_equal env['committee.query_hash']['integer'], 42
         # assert_equal env['rack.request.query_hash'][:integer], 21 # this isn't hash indifferent hash because we use rack.request.query_hash
         [204, {}, []]
       end, schema: open_api_3_schema, parameter_overwrite_by_rails_rule: false)
@@ -478,7 +478,7 @@ def app
         assert_equal env['committee.params'][:integer], 84
         assert_equal env['committee.request_body_hash']['integer'], 21
         assert_equal env['committee.request_body_hash'][:integer], 21
-        assert_equal env['committee.query_hash']['integer'], 84 # we can't use query_parameter :(
+        assert_equal env['committee.query_hash']['integer'], 42
         assert_equal env['committee.path_hash']['integer'], 84
         assert_equal env['committee.path_hash'][:integer], 84
         [204, {}, []]

test/schema_validator/open_api_3/operation_wrapper_test.rb

@@ -156,6 +156,63 @@ def operation_object
       assert_kind_of(OpenAPIParser::OpenAPIError, e.original_error)
     end
 
+    describe 'same-named path and query parameters' do
+      %w[get post].each do |method|
+        describe method do
+          before do
+            @path = '/overwrite_same_parameter/123'
+            @method = method
+            data = open_api_3_data
+            path_item = data['paths']['/overwrite_same_parameter/{integer}']
+            path_item[method] = path_item['post']
+            @open_api_3_schema = Committee::Drivers.load_from_data(data, open_api_3_schema_path, parser_options: { strict_reference_validation: true })
+          end
+
+          it 'rejects an invalid query value without overwriting it with the path value' do
+            path_params = { 'integer' => 123 }
+            query_params = { 'integer' => 'not-an-integer' }
+
+            error = assert_raises(Committee::InvalidRequest) do
+              operation_object.validate_request_params(path_params, query_params, {}, HEADER, @validator_option)
+            end
+
+            assert_match(/expected integer, but received String: "not-an-integer"/i, error.message)
+            assert_kind_of(OpenAPIParser::OpenAPIError, error.original_error)
+            assert_equal({ 'integer' => 'not-an-integer' }, query_params)
+            assert_equal({ 'integer' => 123 }, path_params)
+          end
+
+          it 'rejects a missing required query parameter even when the path parameter is present' do
+            error = assert_raises(Committee::InvalidRequest) do
+              operation_object.validate_request_params({ 'integer' => 123 }, {}, {}, HEADER, @validator_option)
+            end
+
+            assert_match(/missing required parameters: integer/i, error.message)
+          end
+
+          it 'coerces the query value independently of the path value' do
+            path_params = { 'integer' => 123 }
+            query_params = { 'integer' => '456' }
+
+            operation_object.validate_request_params(path_params, query_params, {}, HEADER, @validator_option)
+
+            assert_equal({ 'integer' => 123 }, path_params)
+            assert_equal({ 'integer' => 456 }, query_params)
+          end
+
+          it 'honors disabled query coercion even when the path value is already an integer' do
+            options = Committee::SchemaValidator::Option.new({ coerce_query_params: false }, open_api_3_schema, :open_api_3)
+
+            error = assert_raises(Committee::InvalidRequest) do
+              operation_object.validate_request_params({ 'integer' => 123 }, { 'integer' => '456' }, {}, HEADER, options)
+            end
+
+            assert_match(/expected integer, but received String: "456"/i, error.message)
+          end
+        end
+      end
+    end
+
     describe '#content_types' do
       it 'returns supported content types' do
         @path = '/validate_content_types'