Merge pull request #494 from ahx/dont-load-binary-file-uploads

Don't read uploaded files during request validation

Author: ahx

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.

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] # => #<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.
 

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,

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

lib/openapi_first/request_validator.rb

@@ -9,14 +9,17 @@ module OpenapiFirst
   class RequestValidator
     def initialize(
       content_schema:,
+      content_type:,
       required_request_body:,
       path_schema:,
       query_schema:,
       header_schema:,
       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:,

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

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')

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

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

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