diff --git a/CHANGELOG.md b/CHANGELOG.md index 87ede926..4719478d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,10 @@ ## Unreleased - Added: OpenAPI 3.2 documents are accepted, but not fully supported yet. They are handled using the OpenAPI 3.1 rules, so features introduced in 3.2 may be ignored. Loading such a document prints a warning. Operations defined under `additionalOperations` are routed. See #469. +- **Breaking**: Uploaded files are no longer read during request validation. Before, the whole content of every `multipart/form-data` part that was sent as a file was read into memory, which allowed a single large upload to any documented multipart route to exhaust the memory of the server process. Such a field is now passed through as Rack parsed it (`{ filename:, type:, name:, tempfile:, head: }`), which is the same shape that Sinatra and Hanami hand to your application. Use `parsed_body['file'][:tempfile]` to read or stream the file. + - The content of these fields is not validated anymore, so `minLength`, `maxLength` or `pattern` on a field that was sent as a file are ignored. + - An `after_request_body_property_validation` hook sees an empty String instead of the file. + - Fields that were not sent as a file, and fields with a JSON `contentType` in the `encoding` map, are read and validated as before. - Changed: Don't hide covered endpoints in HTML coverage reporter - Added: Filter un/covered endpoints in HTML coverage reporter - Changed: Reduced memory retained by a loaded `Definition`. Response headers with a schema no longer keep the whole raw document node alive, and a couple of build-time-only hashes were replaced with more compact structures. diff --git a/README.md b/README.md index e334fdab..1e667335 100644 --- a/README.md +++ b/README.md @@ -166,8 +166,25 @@ use OpenapiFirst::Middlewares::RequestValidation, 'openapi.yaml', error_response You can build your own custom error response with `error_response: MyCustomClass` that implements `OpenapiFirst::ErrorResponse`. You can define custom error responses globally by including / implementing `OpenapiFirst::ErrorResponse` and register it via `OpenapiFirst.register_error_response(my_name, MyCustomErrorResponse)` and set `error_response: my_name`. +#### Multipart file uploads + +Uploaded files are not read during request validation. A `multipart/form-data` field that was sent as a file is passed through as Rack parsed it – the same shape that Sinatra and Hanami hand to your application: + +```ruby +file = validated_request.parsed_body['file'] +file[:filename] # => "cat.jpg" +file[:type] # => "image/jpeg" +file[:tempfile] # => # Read or stream this in your application. +``` + +The tempfile is only usable while the request is being handled, because Rack removes it afterwards. + +This means the _content_ of these fields is not validated, so `minLength`, `maxLength` or `pattern` on a field that was sent as a file are ignored. Fields that were not sent as a file are read and validated as usual, and a field with `contentType: application/json` in the `encoding` map is still parsed as JSON. + ### Response validation +You should use [Contract Testing](#contract-testing) instead of running the response validation middleware. + This middleware raises an error by default if the response is not valid. This can be useful in a test or staging environment, especially if you are adopting OpenAPI for an existing implementation. diff --git a/lib/openapi_first/request.rb b/lib/openapi_first/request.rb index ed02ce5e..5ca853a6 100644 --- a/lib/openapi_first/request.rb +++ b/lib/openapi_first/request.rb @@ -28,6 +28,7 @@ def initialize(path:, request_method:, operation_object:, # rubocop:disable Metr @body_parsers = build_body_parser(content_type, encoding) if content_type @validator = RequestValidator.new( content_schema:, + content_type:, required_request_body: required_body == true, path_schema: parameters.path_schema, query_schema: parameters.query_schema, diff --git a/lib/openapi_first/request_body_parsers.rb b/lib/openapi_first/request_body_parsers.rb index 1588951b..1740b97e 100644 --- a/lib/openapi_first/request_body_parsers.rb +++ b/lib/openapi_first/request_body_parsers.rb @@ -39,10 +39,6 @@ def self.read_body(request) Failure.new(:invalid_body, message: 'Failed to parse request body as JSON') end) - # Parses multipart/form-data requests and currently puts the contents of a file upload at the parsed hash values. - # NOTE: This behavior will probably change in the next major version. - # The uploaded file should not be read during request validation. - # # Honors the OpenAPI `encoding` map: when a top-level field has # `contentType: application/json` (or any */json), the field's raw value # is JSON-parsed before schema validation. @@ -65,9 +61,11 @@ def call(request) private def decode_field(name, value) - raw = unpack_value(value) content_type = @encoding.dig(name, 'contentType') - return raw unless content_type && raw.is_a?(String) && json?(content_type) + return unpack_value(value) unless content_type && json?(content_type) + + raw = read_raw(value) + return unpack_value(value) if raw.nil? JSON.parse(raw) rescue JSON::ParserError => e @@ -79,10 +77,16 @@ def json?(content_type) content_type.match?(%r{[/+]json\b}i) end + def read_raw(value) + return value if value.is_a?(String) + + value[:tempfile]&.read if value.is_a?(Hash) && value.key?(:tempfile) + end + def unpack_value(value) return value.map { unpack_value(_1) } if value.is_a?(Array) return value unless value.is_a?(Hash) - return value[:tempfile]&.read if value.key?(:tempfile) + return value if value.key?(:tempfile) value.transform_values { unpack_value(_1) } end diff --git a/lib/openapi_first/request_validator.rb b/lib/openapi_first/request_validator.rb index 84cb6d50..7d5069a6 100644 --- a/lib/openapi_first/request_validator.rb +++ b/lib/openapi_first/request_validator.rb @@ -9,6 +9,7 @@ module OpenapiFirst class RequestValidator def initialize( content_schema:, + content_type:, required_request_body:, path_schema:, query_schema:, @@ -16,7 +17,9 @@ def initialize( cookie_schema: ) @validators = [] - @validators << Validators::RequestBody.new(content_schema:, required_request_body:) if content_schema + if content_schema + @validators.concat Validators::RequestBody.for(content_schema:, required_request_body:, content_type:) + end @validators.concat Validators::RequestParameters.for( path_schema:, query_schema:, diff --git a/lib/openapi_first/validators/multipart_request_body.rb b/lib/openapi_first/validators/multipart_request_body.rb new file mode 100644 index 00000000..1575e022 --- /dev/null +++ b/lib/openapi_first/validators/multipart_request_body.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +require_relative '../schema/validation_result' + +module OpenapiFirst + module Validators + class MultipartRequestBody + FILE_UPLOAD_PLACEHOLDER = String.new('', encoding: Encoding::BINARY).freeze + + def initialize(content_schema:) + @schema = content_schema + end + + def call(parsed_request) + body = parsed_request.body + return if body.nil? + + uploads = collect_file_uploads(body) + uploads.each_key { write_at(body, _1, FILE_UPLOAD_PLACEHOLDER) } + begin + validate(body) + ensure + uploads.each { |path, upload| write_at(body, path, upload) } + end + end + + private + + def validate(body) + validation = Schema::ValidationResult.new( + @schema.validate(body, access_mode: 'write') + ) + Failure.new(:invalid_body, errors: validation.errors) if validation.error? + end + + def collect_file_uploads(value, path = [], result = {}) + case value + when ::Hash + if value.key?(:tempfile) + result[path] = value unless path.empty? + else + value.each { |key, item| collect_file_uploads(item, path + [key], result) } + end + when ::Array + value.each_with_index { |item, index| collect_file_uploads(item, path + [index], result) } + end + result + end + + def write_at(root, path, value) + *parents, key = path + container = parents.empty? ? root : root.dig(*parents) + container[key] = value if container + end + end + end +end diff --git a/lib/openapi_first/validators/request_body.rb b/lib/openapi_first/validators/request_body.rb index ebe54b5c..026346a5 100644 --- a/lib/openapi_first/validators/request_body.rb +++ b/lib/openapi_first/validators/request_body.rb @@ -1,20 +1,29 @@ # frozen_string_literal: true +require_relative 'multipart_request_body' +require_relative 'required_request_body' + module OpenapiFirst module Validators class RequestBody - def initialize(content_schema:, required_request_body:) + MULTIPART = %r{\Amultipart/}i + private_constant :MULTIPART + + def self.for(content_schema:, required_request_body:, content_type:) + validators = [] + validators << RequiredRequestBody.new if required_request_body + klass = MULTIPART.match?(content_type.to_s) ? MultipartRequestBody : self + validators << klass.new(content_schema:) + validators + end + + def initialize(content_schema:) @schema = content_schema - @required = required_request_body end def call(parsed_request) body = parsed_request.body - if body.nil? - return Failure.new(:invalid_body, message: 'Request body must not be empty') if @required - - return - end + return if body.nil? validation = Schema::ValidationResult.new( @schema.validate(body, access_mode: 'write') diff --git a/lib/openapi_first/validators/required_request_body.rb b/lib/openapi_first/validators/required_request_body.rb new file mode 100644 index 00000000..444768a3 --- /dev/null +++ b/lib/openapi_first/validators/required_request_body.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module OpenapiFirst + module Validators + class RequiredRequestBody + def call(parsed_request) + Failure.new(:invalid_body, message: 'Request body must not be empty') if parsed_request.body.nil? + end + end + end +end diff --git a/spec/middlewares/request_validation/request_body_validation_spec.rb b/spec/middlewares/request_validation/request_body_validation_spec.rb index 436f4c32..0cb60b57 100644 --- a/spec/middlewares/request_validation/request_body_validation_spec.rb +++ b/spec/middlewares/request_validation/request_body_validation_spec.rb @@ -60,8 +60,18 @@ def fixture_path(name) post '/multipart-with-file', 'file' => uploaded_file expect(last_response.status).to eq(200) - uploaded_file = last_request.env[OpenapiFirst::REQUEST].parsed_body['file'] - expect(uploaded_file).to eq File.read(fixture_path('foo.txt')) + part = last_request.env[OpenapiFirst::REQUEST].parsed_body['file'] + expect(part[:filename]).to eq('foo.txt') + expect(part[:tempfile].read).to eq File.read(fixture_path('foo.txt')) + end + + it 'does not read the uploaded file during request validation' do + uploaded_file = Rack::Test::UploadedFile.new(fixture_path('foo.txt')) + + expect_any_instance_of(Tempfile).not_to receive(:read) + post '/multipart-with-file', 'file' => uploaded_file + + expect(last_response.status).to eq(200), last_response.body end it 'succeeds with nested multipart form data file binary upload' do @@ -70,8 +80,8 @@ def fixture_path(name) post '/nested-multipart-with-file', 'user' => { 'avatar' => uploaded_file } expect(last_response.status).to eq(200), last_response.body - uploaded_file = last_request.env[OpenapiFirst::REQUEST].parsed_body.dig('user', 'avatar') - expect(uploaded_file).to eq File.read(fixture_path('foo.txt')) + part = last_request.env[OpenapiFirst::REQUEST].parsed_body.dig('user', 'avatar') + expect(part[:tempfile].read).to eq File.read(fixture_path('foo.txt')) end it 'succeeds list of binary fields in multipart/form-data' do @@ -80,8 +90,31 @@ def fixture_path(name) post '/users-with-avatars', 'data' => [{ 'avatar' => uploaded_file, 'name' => 'Quentin' }] expect(last_response.status).to eq(200), last_response.body - names = last_request.env[OpenapiFirst::REQUEST].parsed_body.fetch('data').map { _1['name'] } - expect(names).to eq(['Quentin']) + data = last_request.env[OpenapiFirst::REQUEST].parsed_body.fetch('data') + expect(data.map { _1['name'] }).to eq(['Quentin']) + expect(data.first['avatar'][:tempfile].read).to eq File.read(fixture_path('foo.txt')) + end + + it 'fails when a required file part is missing' do + data_part = Rack::Test::UploadedFile.new( + StringIO.new(JSON.generate(name: 'Quentin', description: 'Cat')), + 'application/json', original_filename: 'data.json' + ) + + post '/multipart-with-encoding', 'data' => data_part + + expect(last_response.status).to eq(400), last_response.body + end + + it 'still validates non-file fields next to a file upload' do + uploaded_file = Rack::Test::UploadedFile.new(fixture_path('foo.txt')) + + post '/multipart-with-file', 'file' => uploaded_file, 'petId' => 'not-a-number' + + expect(last_response.status).to eq(400), last_response.body + + part = last_request.env[OpenapiFirst::REQUEST].parsed_body['file'] + expect(part[:tempfile].read).to eq File.read(fixture_path('foo.txt')) end context 'when raise_error is true and a multipart JSON-encoded part is malformed' do @@ -113,7 +146,37 @@ def fixture_path(name) expect(last_response.status).to eq(200), last_response.body parsed = last_request.env[OpenapiFirst::REQUEST].parsed_body expect(parsed['data']).to eq('name' => 'Quentin', 'description' => 'Cat') - expect(parsed['file']).to eq(File.read(fixture_path('foo.txt'))) + expect(parsed['file'][:tempfile].read).to eq(File.read(fixture_path('foo.txt'))) + end + + context 'with an after_request_body_property_validation hook' do + let(:seen) { [] } + + let(:app) do + properties = seen + definition = OpenapiFirst.load('./spec/data/request-body-validation.yaml') do |config| + config.after_request_body_property_validation do |data, property, _property_schema| + properties << [property, data[property]] + end + end + Rack::Builder.new do + use(OpenapiFirst::Middlewares::RequestValidation, spec: definition) + run lambda { |_env| + Rack::Response.new('hello', 200).finish + } + end + end + + it 'restores a nested file upload after validation and shows the hook a placeholder' do + uploaded_file = Rack::Test::UploadedFile.new(fixture_path('foo.txt')) + + post '/nested-multipart-with-file', 'user' => { 'avatar' => uploaded_file } + expect(last_response.status).to eq(200), last_response.body + + part = last_request.env[OpenapiFirst::REQUEST].parsed_body.dig('user', 'avatar') + expect(part[:tempfile].read).to eq File.read(fixture_path('foo.txt')) + expect(seen).to include(['avatar', '']) + end end it 'succeeds without optional file upload' do @@ -336,6 +399,8 @@ def fixture_path(name) post path expect(last_response.status).to be 400 + error = last_request.env[OpenapiFirst::REQUEST].error + expect(error.message).to eq 'Request body must not be empty' end it 'returns 415 if request content-type does not match' do diff --git a/spec/request_body_parsers_spec.rb b/spec/request_body_parsers_spec.rb index 6f711633..78e18119 100644 --- a/spec/request_body_parsers_spec.rb +++ b/spec/request_body_parsers_spec.rb @@ -24,7 +24,16 @@ def app = ->(_env) { Rack::Response.new.finish } post '/', 'file' => uploaded_file body = parser.call(last_request) - expect(body['file']).to eq(File.read('./spec/data/foo.txt')) + expect(body['file'][:filename]).to eq('foo.txt') + expect(body['file'][:tempfile].read).to eq(File.read('./spec/data/foo.txt')) + end + + it 'does not read uploaded files' do + uploaded_file = Rack::Test::UploadedFile.new('./spec/data/foo.txt') + post '/', 'file' => uploaded_file + + expect_any_instance_of(Tempfile).not_to receive(:read) + parser.call(last_request) end context 'with an encoding map' do