From 2a1e9e507867236d0a3776ff0f568f09e74fca5d Mon Sep 17 00:00:00 2001 From: Jalyna Date: Wed, 18 Mar 2020 15:48:40 +0100 Subject: [PATCH 1/3] Allow path and request body to be parsed at the same time --- lib/committee/request_unpacker.rb | 48 ++++++++++++++++++- lib/committee/schema_validator/open_api_3.rb | 8 +++- .../open_api_3/operation_wrapper.rb | 20 ++++++-- test/data/openapi3/normal.yaml | 25 ++++++++++ .../request_validation_open_api_3_test.rb | 17 +++++-- test/request_unpacker_test.rb | 32 ++++++------- .../open_api_3/operation_wrapper_test.rb | 30 ++++++------ 7 files changed, 138 insertions(+), 42 deletions(-) diff --git a/lib/committee/request_unpacker.rb b/lib/committee/request_unpacker.rb index b61912d1..0056764b 100644 --- a/lib/committee/request_unpacker.rb +++ b/lib/committee/request_unpacker.rb @@ -14,6 +14,48 @@ def initialize(request, options={}) end def call + return call_hyperschema if hyperschema? + + # if Content-Type is empty or JSON, and there was a request body, try to + # interpret it as JSON + params = {} + + params['body'] = if !@request.media_type || @request.media_type =~ %r{application/.*json} + parse_json + elsif @optimistic_json + begin + parse_json + rescue JSON::ParserError + nil + end + else + {} + end + + params['form_data'] = if @allow_form_params && %w[application/x-www-form-urlencoded multipart/form-data].include?(@request.media_type) + # Actually, POST means anything in the request body, could be from + # PUT or PATCH too. Silly Rack. + p = @request.POST + + @schema_validator.coerce_form_params(p) if @coerce_form_params + + p + else + {} + end + + params['query'] = if @allow_query_params + indifferent_params(@request.GET) + else + {} + end + + [params, headers] + end + + private + + def call_hyperschema # if Content-Type is empty or JSON, and there was a request body, try to # interpret it as JSON params = if !@request.media_type || @request.media_type =~ %r{application/.*json} @@ -47,8 +89,6 @@ def call end end - private - # Creates a Hash with indifferent access. # # (Copied from Sinatra) @@ -56,6 +96,10 @@ def indifferent_hash Hash.new { |hash,key| hash[key.to_s] if Symbol === key } end + def hyperschema? + @schema_validator.is_a?(Committee::SchemaValidator::HyperSchema) + end + # Enable string or symbol key access to the nested params hash. # # (Copied from Sinatra) diff --git a/lib/committee/schema_validator/open_api_3.rb b/lib/committee/schema_validator/open_api_3.rb index c2a11551..65dee74b 100644 --- a/lib/committee/schema_validator/open_api_3.rb +++ b/lib/committee/schema_validator/open_api_3.rb @@ -18,7 +18,8 @@ def request_validate(request) request_unpack(request) - request.env[validator_option.params_key]&.merge!(path_params) unless path_params.empty? + # committee.params + request.env[validator_option.params_key]&.merge!('path' => path_params) unless path_params.empty? request_schema_validation(request) @@ -80,8 +81,11 @@ def request_unpack(request) def copy_coerced_data_to_query_hash(request) return if request.env["rack.request.query_hash"].nil? || request.env["rack.request.query_hash"].empty? + params = request.env[validator_option.params_key] + request_params = (params['path'] || {}).merge(params['query'] || {}).merge(params['form_data'] || {}).merge(params['body'] || {}) + request.env["rack.request.query_hash"].keys.each do |k| - request.env["rack.request.query_hash"][k] = request.env[validator_option.params_key][k] + request.env["rack.request.query_hash"][k] = request_params[k] end end end 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 7a3d5214..ffb6be42 100644 --- a/lib/committee/schema_validator/open_api_3/operation_wrapper.rb +++ b/lib/committee/schema_validator/open_api_3/operation_wrapper.rb @@ -107,7 +107,14 @@ def build_openapi_parser_get_option(validator_option) def validate_get_request_params(params, headers, validator_option) # bad performance because when we coerce value, same check - request_operation.validate_request_parameter(params, headers, build_openapi_parser_get_option(validator_option)) + request_params = (params['path'] || {}).merge(params['query'] || {}).merge(params['body'] || {}).merge(params['form_data'] || {}) + result = request_operation.validate_request_parameter(request_params, headers, build_openapi_parser_get_option(validator_option)) + # Copy coerced params + params['path'] = (params['path'] || {}).keys.map { |k| [k, request_params[k]] }.to_h + params['query'] = (params['query'] || {}).keys.map { |k| [k, request_params[k]] }.to_h + params['body'] = (params['body'] || {}).keys.map { |k| [k, request_params[k]] }.to_h + params['form_data'] = (params['form_data'] || {}).keys.map { |k| [k, request_params[k]] }.to_h + result rescue OpenAPIParser::OpenAPIError => e raise Committee::InvalidRequest.new(e.message) end @@ -117,8 +124,15 @@ def validate_post_request_params(params, headers, validator_option) # bad performance because when we coerce value, same check schema_validator_options = build_openapi_parser_post_option(validator_option) - request_operation.validate_request_parameter(params, headers, schema_validator_options) - request_operation.validate_request_body(content_type, params, schema_validator_options) + request_params = (params['path'] || {}).merge(params['query'] || {}).merge(params['body'] || {}).merge(params['form_data'] || {}) + request_operation.validate_request_parameter(request_params, headers, schema_validator_options) + result = request_operation.validate_request_body(content_type, params['body'], schema_validator_options) + # Copy coerced params + params['path'] = (params['path'] || {}).keys.map { |k| [k, request_params[k]] }.to_h + params['query'] = (params['query'] || {}).keys.map { |k| [k, request_params[k]] }.to_h + params['body'] = (params['body'] || {}).keys.map { |k| [k, request_params[k]] }.to_h + params['form_data'] = (params['form_data'] || {}).keys.map { |k| [k, request_params[k]] }.to_h + result rescue => e raise Committee::InvalidRequest.new(e.message) end diff --git a/test/data/openapi3/normal.yaml b/test/data/openapi3/normal.yaml index d72f3468..28cf95db 100644 --- a/test/data/openapi3/normal.yaml +++ b/test/data/openapi3/normal.yaml @@ -544,6 +544,31 @@ paths: '204': description: no content + /request_body_and_path_params/{id}: + post: + description: request body + requestBody: + required: true + content: + application/json: + schema: + type: object + additionalProperties: false + required: + - data + properties: + data: + type: string + parameters: + - name: id + in: path + required: true + schema: + type: string + responses: + '204': + description: no content + components: schemas: nested_array: diff --git a/test/middleware/request_validation_open_api_3_test.rb b/test/middleware/request_validation_open_api_3_test.rb index 7504df9f..1f86de64 100644 --- a/test/middleware/request_validation_open_api_3_test.rb +++ b/test/middleware/request_validation_open_api_3_test.rb @@ -44,6 +44,15 @@ def app assert_equal 200, last_response.status end + it "passes given a valid request body and path param" do + @app = new_rack_app(schema: open_api_3_schema) + params = { "data" => "abc" } + header "Content-Type", "application/json" + post "/request_body_and_path_params/abc", JSON.generate(params) + + assert_equal 200, last_response.status + end + it "passes given a valid parameter on GET endpoint with request body and allow_get_body=true" do params = { "data" => "abc" } @@ -66,7 +75,7 @@ def app params = { "datetime_string" => "2016-04-01T16:00:00.000+09:00" } check_parameter = lambda { |env| - assert_equal DateTime, env['committee.params']["datetime_string"].class + assert_equal DateTime, env['committee.params']['body']["datetime_string"].class [200, {}, []] } @@ -108,7 +117,7 @@ def app } check_parameter = lambda { |env| - nested_array = env['committee.params']["nested_array"] + nested_array = env['committee.params']['body']["nested_array"] first_data = nested_array[0] assert_kind_of DateTime, first_data["update_time"] @@ -220,7 +229,7 @@ def app } check_parameter = lambda { |env| - hash = env['committee.params'] + hash = env['committee.params']['body'] array = hash['nested_array'] assert_equal DateTime, array.first['update_time'].class @@ -388,7 +397,7 @@ def app it "coerce string to integer" do check_parameter_string = lambda { |env| - assert env['committee.params']['integer'].is_a?(Integer) + assert env['committee.params']['path']['integer'].is_a?(Integer) [200, {}, []] } diff --git a/test/request_unpacker_test.rb b/test/request_unpacker_test.rb index c78b1781..6a64ffb3 100644 --- a/test/request_unpacker_test.rb +++ b/test/request_unpacker_test.rb @@ -10,7 +10,7 @@ } request = Rack::Request.new(env) params, _ = Committee::RequestUnpacker.new(request).call - assert_equal({ "x" => "y" }, params) + assert_equal({ "x" => "y" }, params["body"]) end it "unpacks JSON on no Content-Type" do @@ -19,7 +19,7 @@ } request = Rack::Request.new(env) params, _ = Committee::RequestUnpacker.new(request).call - assert_equal({ "x" => "y" }, params) + assert_equal({ "x" => "y" }, params["body"]) end it "doesn't unpack JSON under other Content-Types" do @@ -30,7 +30,7 @@ } request = Rack::Request.new(env) params, _ = Committee::RequestUnpacker.new(request).call - assert_equal({}, params) + assert_equal({}, params["body"]) end end @@ -42,7 +42,7 @@ } request = Rack::Request.new(env) params, _ = Committee::RequestUnpacker.new(request, optimistic_json: true).call - assert_equal({ "x" => "y" }, params) + assert_equal({ "x" => "y" }, params["body"]) end end @@ -54,7 +54,7 @@ } request = Rack::Request.new(env) params, _ = Committee::RequestUnpacker.new(request, optimistic_json: true).call - assert_equal({}, params) + assert_equal(nil, params["body"]) end end @@ -65,7 +65,7 @@ } request = Rack::Request.new(env) params, _ = Committee::RequestUnpacker.new(request).call - assert_equal({}, params) + assert_nil params["body"] end it "doesn't unpack form params" do @@ -76,7 +76,7 @@ } request = Rack::Request.new(env) params, _ = Committee::RequestUnpacker.new(request).call - assert_equal({}, params) + assert_equal({}, params["body"]) end end @@ -88,7 +88,7 @@ } request = Rack::Request.new(env) params, _ = Committee::RequestUnpacker.new(request, allow_form_params: true).call - assert_equal({ "x" => "y" }, params) + assert_equal({ "x" => "y" }, params["form_data"]) end end @@ -143,7 +143,7 @@ schema_validator: validator, ).call # openapi3 not support coerce in request unpacker - assert_equal({ "limit" => '20' }, params) + assert_equal({ "limit" => '20' }, params["form_data"]) end end @@ -167,7 +167,7 @@ coerce_form_params: true, schema_validator: validator, ).call - assert_equal({ "limit" => "twenty" }, params) + assert_equal({ "limit" => "twenty" }, params["form_data"]) end end @@ -180,7 +180,7 @@ } request = Rack::Request.new(env) params, _ = Committee::RequestUnpacker.new(request, allow_form_params: true, allow_query_params: true).call - assert_equal({ "x" => "y", "a" => "b" }, params) + assert_equal({"body" => {}, "form_data" => {"x" => "y"}, "query" => {"a"=>"b"}}, params) end end @@ -191,7 +191,7 @@ } request = Rack::Request.new(env) params, _ = Committee::RequestUnpacker.new(request, allow_query_params: true).call - assert_equal({ "a" => "b" }, params) + assert_equal({ "a" => "b" }, params["query"]) end it "errors if JSON is not an object" do @@ -212,7 +212,7 @@ } request = Rack::Request.new(env) params, _ = Committee::RequestUnpacker.new(request).call - assert_equal({}, params) + assert_equal({"body" => {}, "form_data" => {}, "query" => {}}, params) end # this is mostly here for line coverage @@ -222,7 +222,7 @@ } request = Rack::Request.new(env) params, _ = Committee::RequestUnpacker.new(request).call - assert_equal({ "x" => [] }, params) + assert_equal({ "x" => [] }, params["body"]) end it "unpacks http header" do @@ -243,7 +243,7 @@ } request = Rack::Request.new(env) params, _ = Committee::RequestUnpacker.new(request, { allow_query_params: true, allow_get_body: true }).call - assert_equal({ 'data' => 'value', 'x' => 1, 'y' => 2 }, params) + assert_equal({"body" => {"x" => 1, "y" => 2}, "form_data" => {}, "query" => {"data" => "value", "x" => "aaa"}}, params) end it "doesn't include request body when `use_get_body` is false" do @@ -254,6 +254,6 @@ } request = Rack::Request.new(env) params, _ = Committee::RequestUnpacker.new(request, { allow_query_params: true, use_get_body: false }).call - assert_equal({ 'data' => 'value', 'x' => 'aaa' }, params) + assert_equal({ 'data' => 'value', 'x' => 'aaa' }, params["query"]) end end 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 b68d3b4b..11f0609b 100644 --- a/test/schema_validator/open_api_3/operation_wrapper_test.rb +++ b/test/schema_validator/open_api_3/operation_wrapper_test.rb @@ -27,12 +27,12 @@ def operation_object ] it 'correct data' do - operation_object.validate_request_params(SCHEMA_PROPERTIES_PAIR.to_h, HEADER, @validator_option) + operation_object.validate_request_params({ "body" => SCHEMA_PROPERTIES_PAIR.to_h }, HEADER, @validator_option) assert true end it 'correct object data' do - operation_object.validate_request_params({ + operation_object.validate_request_params({ "body" => { "object_1" => { "string_1" => nil, @@ -40,7 +40,7 @@ def operation_object "boolean_1" => nil, "number_1" => nil } - }, + }}, HEADER, @validator_option) @@ -49,7 +49,7 @@ def operation_object it 'invalid params' do e = assert_raises(Committee::InvalidRequest) { - operation_object.validate_request_params({"string" => 1}, HEADER, @validator_option) + operation_object.validate_request_params({ "body" => {"string" => 1}}, HEADER, @validator_option) } # FIXME: when ruby 2.3 dropped, fix because ruby 2.3 return Fixnum, ruby 2.4 or later return Integer @@ -58,10 +58,10 @@ def operation_object it 'support put method' do @method = "put" - operation_object.validate_request_params({"string" => "str"}, HEADER, @validator_option) + operation_object.validate_request_params({ "body" => {"string" => "str"}}, HEADER, @validator_option) e = assert_raises(Committee::InvalidRequest) { - operation_object.validate_request_params({"string" => 1}, HEADER, @validator_option) + operation_object.validate_request_params({ "body" => {"string" => 1}}, HEADER, @validator_option) } # FIXME: when ruby 2.3 dropped, fix because ruby 2.3 return Fixnum, ruby 2.4 or later return Integer @@ -70,17 +70,17 @@ def operation_object it 'support patch method' do @method = "patch" - operation_object.validate_request_params({"integer" => 1}, HEADER, @validator_option) + operation_object.validate_request_params({ "body" => {"integer" => 1}}, HEADER, @validator_option) e = assert_raises(Committee::InvalidRequest) { - operation_object.validate_request_params({"integer" => "str"}, HEADER, @validator_option) + operation_object.validate_request_params({ "body" => {"integer" => "str"}}, HEADER, @validator_option) } assert_match(/expected integer, but received String: str/i, e.message) end it 'unknown param' do - operation_object.validate_request_params({"unknown" => 1}, HEADER, @validator_option) + operation_object.validate_request_params({ "body" => {"unknown" => 1}}, HEADER, @validator_option) end describe 'support get method' do @@ -90,13 +90,13 @@ def operation_object it 'correct' do operation_object.validate_request_params( - {"query_string" => "query", "query_integer_list" => [1, 2]}, + { "query" => {"query_string" => "query", "query_integer_list" => [1, 2]}}, HEADER, @validator_option ) operation_object.validate_request_params( - {"query_string" => "query", "query_integer_list" => [1, 2], "optional_integer" => 1}, + { "query" => {"query_string" => "query", "query_integer_list" => [1, 2], "optional_integer" => 1}}, HEADER, @validator_option ) @@ -106,7 +106,7 @@ def operation_object it 'not exist required' do e = assert_raises(Committee::InvalidRequest) { - operation_object.validate_request_params({"query_integer_list" => [1, 2]}, HEADER, @validator_option) + operation_object.validate_request_params({ "query" => {"query_integer_list" => [1, 2]}}, HEADER, @validator_option) } assert_match(/missing required parameters: query_string/i, e.message) @@ -115,7 +115,7 @@ def operation_object it 'invalid type' do e = assert_raises(Committee::InvalidRequest) { operation_object.validate_request_params( - {"query_string" => 1, "query_integer_list" => [1, 2], "optional_integer" => 1}, + { "query" => {"query_string" => 1, "query_integer_list" => [1, 2], "optional_integer" => 1}}, HEADER, @validator_option ) @@ -133,14 +133,14 @@ def operation_object end it 'correct' do - operation_object.validate_request_params({"limit" => "1"}, HEADER, @validator_option) + operation_object.validate_request_params({ "form_data" => {"limit" => "1"}}, HEADER, @validator_option) assert true end it 'invalid type' do e = assert_raises(Committee::InvalidRequest) { - operation_object.validate_request_params({"limit" => "a"}, HEADER, @validator_option) + operation_object.validate_request_params({ "form_data" => {"limit" => "a"}}, HEADER, @validator_option) } assert_match(/expected integer, but received String: a/i, e.message) From 367ed89b4567e360eda4dbe6ae5f2d458ba2bd9a Mon Sep 17 00:00:00 2001 From: Nick Campbell Date: Mon, 10 Jan 2022 10:48:01 +0000 Subject: [PATCH 2/3] Use Request#media_type to silence Rails 7 warning Warning: DEPRECATION WARNING: Rails 7.1 will return Content-Type header without modification. If you want just the MIME type, please use #media_type instead. (called from request_media_type at /home/circleci/repo/vendor/bundle/ruby/3.0.0/bundler/gems/committee-2a1e9e507867/lib/committee/schema_validator.rb:7)) Since we don't need to strip any encoding etc from the string, we can just use this value unchanged. --- lib/committee/schema_validator.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/committee/schema_validator.rb b/lib/committee/schema_validator.rb index 4eb109de..6b64a61d 100644 --- a/lib/committee/schema_validator.rb +++ b/lib/committee/schema_validator.rb @@ -4,7 +4,7 @@ module Committee module SchemaValidator class << self def request_media_type(request) - request.content_type.to_s.split(";").first.to_s + request.media_type end # @param [String] prefix From 202244db3797d07f7f09c4693944a9023296931b Mon Sep 17 00:00:00 2001 From: Maurice Vogel Date: Thu, 10 Mar 2022 16:04:21 +0100 Subject: [PATCH 3/3] Update OpenAPIParser to v1.0.0, fix deprecation warning See https://github.com/ota42y/openapi_parser/pull/123 for context. --- committee.gemspec | 2 +- lib/committee/drivers.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/committee.gemspec b/committee.gemspec index 8ae7dd50..f7433d2a 100644 --- a/committee.gemspec +++ b/committee.gemspec @@ -19,7 +19,7 @@ Gem::Specification.new do |s| s.add_dependency "json_schema", "~> 0.14", ">= 0.14.3" s.add_dependency "rack", ">= 1.5" - s.add_dependency "openapi_parser", ">= 0.6.1" + s.add_dependency "openapi_parser", "~> 1.0" s.add_development_dependency "minitest", "~> 5.3" s.add_development_dependency "rack-test", "~> 0.6" diff --git a/lib/committee/drivers.rb b/lib/committee/drivers.rb index c4653d64..a24b0b80 100644 --- a/lib/committee/drivers.rb +++ b/lib/committee/drivers.rb @@ -50,7 +50,7 @@ def self.load_from_file(schema_path) # @return [Committee::Driver] def self.load_from_data(hash) if hash['openapi']&.start_with?('3.0.') - parser = OpenAPIParser.parse(hash) + parser = OpenAPIParser.parse(hash, { strict_reference_validation: false }) return Committee::Drivers::OpenAPI3::Driver.new.parse(parser) end