From 09c25d9736b057250f8fbc27a4791a0ad61ade54 Mon Sep 17 00:00:00 2001 From: ydah Date: Wed, 9 Sep 2026 00:20:40 +0900 Subject: [PATCH] Prevent path parameters from masking invalid query parameters --- .../open_api_3/operation_wrapper.rb | 35 ++++-------- .../request_validation_open_api_3_test.rb | 4 +- .../open_api_3/operation_wrapper_test.rb | 57 +++++++++++++++++++ 3 files changed, 69 insertions(+), 27 deletions(-) diff --git a/lib/committee/schema_validator/open_api_3/operation_wrapper.rb b/lib/committee/schema_validator/open_api_3/operation_wrapper.rb index f565363b..19bc1afb 100644 --- a/lib/committee/schema_validator/open_api_3/operation_wrapper.rb +++ b/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 diff --git a/test/middleware/request_validation_open_api_3_test.rb b/test/middleware/request_validation_open_api_3_test.rb index 15e98179..340c662d 100644 --- a/test/middleware/request_validation_open_api_3_test.rb +++ b/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, {}, []] diff --git a/test/schema_validator/open_api_3/operation_wrapper_test.rb b/test/schema_validator/open_api_3/operation_wrapper_test.rb index 1a279618..14981213 100644 --- a/test/schema_validator/open_api_3/operation_wrapper_test.rb +++ b/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'