Better handling of multipart parsing when request is missing body.

- Introduce `module Rack::BadRequest` for unified exception handling.
- Add `BadRequest` to query parser too.
- Make `env['rack.input']` optional.

# Conflicts:
#	lib/rack/request.rb

CHANGELOG.md

@@ -2,6 +2,16 @@
 
 All notable changes to this project will be documented in this file. For info on how to format all future additions to this file please reference [Keep A Changelog](https://keepachangelog.com/en/1.0.0/).
 
+## [Unreleased]
+
+### SPEC Changes
+
+- `rack.input` is now optional. ([#1997](https://github.com/rack/rack/pull/1997), [@ioquatix])
+
+### Changed
+
+- Improved handling of multipart requests. `rack.input` is now optional, and if missing, will raise an error which includes `module Rack::BadRequest`. Several other exceptions also include this module. ([#1997](https://github.com/rack/rack/pull/1997), [@ioquatix])
+
 ## [3.0.3] - 2022-12-07
 
 ### Fixed

SPEC.rdoc

@@ -121,7 +121,7 @@ If the string values for CGI keys contain non-ASCII characters,
 they should use ASCII-8BIT encoding.
 There are the following restrictions:
 * <tt>rack.url_scheme</tt> must either be +http+ or +https+.
-* There must be a valid input stream in <tt>rack.input</tt>.
+* There may be a valid input stream in <tt>rack.input</tt>.
 * There must be a valid error stream in <tt>rack.errors</tt>.
 * There may be a valid hijack callback in <tt>rack.hijack</tt>
 * The <tt>REQUEST_METHOD</tt> must be a valid token.

lib/rack.rb

@@ -15,6 +15,7 @@
 require_relative 'rack/constants'
 
 module Rack
+  autoload :BadRequest, "rack/bad_request"
   autoload :Builder, "rack/builder"
   autoload :BodyProxy, "rack/body_proxy"
   autoload :Cascade, "rack/cascade"

lib/rack/bad_request.rb

@@ -0,0 +1,4 @@
+module Rack
+  module BadRequest
+  end
+end

lib/rack/lint.rb

@@ -56,9 +56,6 @@ def response
         raise LintError, "No env given" unless @env
         check_environment(@env)
 
-        @env[RACK_INPUT] = InputWrapper.new(@env[RACK_INPUT])
-        @env[RACK_ERRORS] = ErrorWrapper.new(@env[RACK_ERRORS])
-
         ## and returns a non-frozen Array of exactly three values:
         @response = @app.call(@env)
         raise LintError, "response is not an Array, but #{@response.class}" unless @response.kind_of? Array
@@ -265,11 +262,9 @@ def check_environment(env)
         ## is reserved for use with the Rack core distribution and other
         ## accepted specifications and must not be used otherwise.
         ##
-
-        %w[REQUEST_METHOD SERVER_NAME QUERY_STRING SERVER_PROTOCOL
-           rack.input rack.errors].each { |header|
+        %w[REQUEST_METHOD SERVER_NAME QUERY_STRING SERVER_PROTOCOL rack.errors].each do |header|
           raise LintError, "env missing required key #{header}" unless env.include? header
-        }
+        end
 
         ## The <tt>SERVER_PORT</tt> must be an Integer if set.
         server_port = env["SERVER_PORT"]
@@ -328,10 +323,17 @@ def check_environment(env)
           raise LintError, "rack.url_scheme unknown: #{env[RACK_URL_SCHEME].inspect}"
         end
 
-        ## * There must be a valid input stream in <tt>rack.input</tt>.
-        check_input env[RACK_INPUT]
+        ## * There may be a valid input stream in <tt>rack.input</tt>.
+        if rack_input = env[RACK_INPUT]
+          check_input_stream(rack_input)
+          @env[RACK_INPUT] = InputWrapper.new(rack_input)
+        end
+
         ## * There must be a valid error stream in <tt>rack.errors</tt>.
-        check_error env[RACK_ERRORS]
+        rack_errors = env[RACK_ERRORS]
+        check_error_stream(rack_errors)
+        @env[RACK_ERRORS] = ErrorWrapper.new(rack_errors)
+
         ## * There may be a valid hijack callback in <tt>rack.hijack</tt>
         check_hijack env
 
@@ -384,7 +386,7 @@ def check_environment(env)
       ##
       ## The input stream is an IO-like object which contains the raw HTTP
       ## POST data.
-      def check_input(input)
+      def check_input_stream(input)
         ## When applicable, its external encoding must be "ASCII-8BIT" and it
         ## must be opened in binary mode, for Ruby 1.9 compatibility.
         if input.respond_to?(:external_encoding) && input.external_encoding != Encoding::ASCII_8BIT
@@ -488,7 +490,7 @@ def close(*args)
       ##
       ## === The Error Stream
       ##
-      def check_error(error)
+      def check_error_stream(error)
         ## The error stream must respond to +puts+, +write+ and +flush+.
         [:puts, :write, :flush].each { |method|
           unless error.respond_to? method

lib/rack/mock_request.rb

@@ -41,11 +41,6 @@ def string
       end
     end
 
-    DEFAULT_ENV = {
-      RACK_INPUT        => StringIO.new,
-      RACK_ERRORS       => StringIO.new,
-    }.freeze
-
     def initialize(app)
       @app = app
     end
@@ -104,7 +99,7 @@ def self.env_for(uri = "", opts = {})
       uri = parse_uri_rfc2396(uri)
       uri.path = "/#{uri.path}" unless uri.path[0] == ?/
 
-      env = DEFAULT_ENV.dup
+      env = {}
 
       env[REQUEST_METHOD]  = (opts[:method] ? opts[:method].to_s.upcase : GET).b
       env[SERVER_NAME]     = (uri.host || "example.org").b
@@ -144,20 +139,21 @@ def self.env_for(uri = "", opts = {})
         end
       end
 
-      opts[:input] ||= String.new
       if String === opts[:input]
         rack_input = StringIO.new(opts[:input])
       else
         rack_input = opts[:input]
       end
 
-      rack_input.set_encoding(Encoding::BINARY)
-      env[RACK_INPUT] = rack_input
+      if rack_input
+        rack_input.set_encoding(Encoding::BINARY)
+        env[RACK_INPUT] = rack_input
 
-      env["CONTENT_LENGTH"] ||= env[RACK_INPUT].size.to_s if env[RACK_INPUT].respond_to?(:size)
+        env["CONTENT_LENGTH"] ||= env[RACK_INPUT].size.to_s if env[RACK_INPUT].respond_to?(:size)
+      end
 
       opts.each { |field, value|
-        env[field] = value  if String === field
+        env[field] = value if String === field
       }
 
       env

lib/rack/multipart.rb

@@ -6,16 +6,24 @@
 require_relative 'multipart/parser'
 require_relative 'multipart/generator'
 
+require_relative 'bad_request'
+
 module Rack
   # A multipart form data parser, adapted from IOWA.
   #
   # Usually, Rack::Request#POST takes care of calling this.
   module Multipart
     MULTIPART_BOUNDARY = "AaB03x"
 
+    class MissingInputError < StandardError
+      include BadRequest
+    end
+
     class << self
       def parse_multipart(env, params = Rack::Utils.default_query_parser)
-        io = env[RACK_INPUT]
+        unless io = env[RACK_INPUT]
+          raise MissingInputError, "Missing input stream!"
+        end
 
         if content_length = env['CONTENT_LENGTH']
           content_length = content_length.to_i

lib/rack/multipart/parser.rb

@@ -3,18 +3,28 @@
 require 'strscan'
 
 require_relative '../utils'
+require_relative '../bad_request'
 
 module Rack
   module Multipart
-    class MultipartPartLimitError < Errno::EMFILE; end
+    class MultipartPartLimitError < Errno::EMFILE
+      include BadRequest
+    end
 
     # Use specific error class when parsing multipart request
     # that ends early.
-    class EmptyContentError < ::EOFError; end
+    class EmptyContentError < ::EOFError
+      include BadRequest
+    end
 
     # Base class for multipart exceptions that do not subclass from
     # other exception classes for backwards compatibility.
-    class Error < StandardError; end
+    class BoundaryTooLongError < StandardError
+      include BadRequest
+    end
+
+    Error = BoundaryTooLongError
+    deprecate_constant :Error
 
     EOL = "\r\n"
     MULTIPART = %r|\Amultipart/.*boundary=\"?([^\";,]+)\"?|ni
@@ -96,7 +106,7 @@ def self.parse(io, content_length, content_type, tmpfile, bufsize, qp)
         if boundary.length > 70
           # RFC 1521 Section 7.2.1 imposes a 70 character maximum for the boundary.
           # Most clients use no more than 55 characters.
-          raise Error, "multipart boundary size too large (#{boundary.length} characters)"
+          raise BoundaryTooLongError, "multipart boundary size too large (#{boundary.length} characters)"
         end
 
         io = BoundedIO.new(io, content_length) if content_length

lib/rack/query_parser.rb

@@ -1,22 +1,30 @@
 # frozen_string_literal: true
 
+require_relative 'bad_request'
+
 module Rack
   class QueryParser
     DEFAULT_SEP = /[&] */n
     COMMON_SEP = { ";" => /[;] */n, ";," => /[;,] */n, "&" => /[&] */n }
 
     # ParameterTypeError is the error that is raised when incoming structural
     # parameters (parsed by parse_nested_query) contain conflicting types.
-    class ParameterTypeError < TypeError; end
+    class ParameterTypeError < TypeError
+      include BadRequest
+    end
 
     # InvalidParameterError is the error that is raised when incoming structural
     # parameters (parsed by parse_nested_query) contain invalid format or byte
     # sequence.
-    class InvalidParameterError < ArgumentError; end
+    class InvalidParameterError < ArgumentError
+      include BadRequest
+    end
 
     # ParamsTooDeepError is the error that is raised when params are recursively
     # nested over the specified limit.
-    class ParamsTooDeepError < RangeError; end
+    class ParamsTooDeepError < RangeError
+      include BadRequest
+    end
 
     def self.make_default(param_depth_limit)
       new Params, param_depth_limit

lib/rack/request.rb

@@ -459,6 +459,11 @@ def content_charset
         media_type_params['charset']
       end
 
+      # Whether the request potentially has input.
+      def input?
+        !!get_header(RACK_INPUT)
+      end
+
       # Determine whether the request body contains form-data by checking
       # the request content-type for one of the media-types:
       # "application/x-www-form-urlencoded" or "multipart/form-data". The
@@ -469,9 +474,10 @@ def content_charset
       # content-type header is provided and the request_method is POST.
       def form_data?
         type = media_type
-        meth = get_header(RACK_METHODOVERRIDE_ORIGINAL_METHOD) || get_header(REQUEST_METHOD)
 
-        (meth == POST && type.nil?) || FORM_DATA_MEDIA_TYPES.include?(type)
+        request_method = get_header(RACK_METHODOVERRIDE_ORIGINAL_METHOD) || get_header(REQUEST_METHOD)
+
+        (request_method == POST && type.nil?) || FORM_DATA_MEDIA_TYPES.include?(type)
       end
 
       # Determine whether the request body contains data by checking
@@ -501,11 +507,13 @@ def POST
         end
 
         begin
-          if get_header(RACK_INPUT).nil?
-            raise "Missing rack.input"
-          elsif get_header(RACK_REQUEST_FORM_INPUT) == get_header(RACK_INPUT)
-            get_header(RACK_REQUEST_FORM_HASH)
-          elsif form_data? || parseable_data?
+          # If the form input was already memoized:
+          if get_header(RACK_REQUEST_FORM_INPUT) == get_header(RACK_INPUT)
+            return get_header(RACK_REQUEST_FORM_HASH)
+          end
+
+          # Otherwise, figure out how to parse the input:
+          if input? && (form_data? || parseable_data?)
             unless set_header(RACK_REQUEST_FORM_HASH, parse_multipart)
               form_vars = get_header(RACK_INPUT).read
 
@@ -516,6 +524,7 @@ def POST
               set_header RACK_REQUEST_FORM_VARS, form_vars
               set_header RACK_REQUEST_FORM_HASH, parse_query(form_vars, '&')
             end
+
             set_header RACK_REQUEST_FORM_INPUT, get_header(RACK_INPUT)
             get_header RACK_REQUEST_FORM_HASH
           else

test/spec_lint.rb

@@ -13,8 +13,12 @@
     [200, { "content-type" => "test/plain", "content-length" => "3" }, ["foo"]]
   end
 
-  def env(*args)
-    Rack::MockRequest.env_for("/", *args)
+  def env(options = {})
+    unless options.key?(:input)
+      options[:input] = String.new
+    end
+
+    Rack::MockRequest.env_for("/", options)
   end
 
   it "pass valid request" do

test/spec_mock_request.rb

@@ -14,7 +14,10 @@
 app = Rack::Lint.new(lambda { |env|
   req = Rack::Request.new(env)
 
-  env["mock.postdata"] = env["rack.input"].read
+  if input = env["rack.input"]
+    env["mock.postdata"] = input.read
+  end
+
   if req.GET["error"]
     env["rack.errors"].puts req.GET["error"]
     env["rack.errors"].flush
@@ -46,7 +49,7 @@
   end
 
   it "should handle a non-GET request with both :input and :params" do
-    env = Rack::MockRequest.env_for("/", method: :post, input: nil, params: {})
+    env = Rack::MockRequest.env_for("/", method: :post, input: "", params: {})
     env["PATH_INFO"].must_equal "/"
     env.must_be_kind_of Hash
     env['rack.input'].read.must_equal ''
@@ -71,7 +74,7 @@
     env["PATH_INFO"].must_equal "/"
     env["SCRIPT_NAME"].must_equal ""
     env["rack.url_scheme"].must_equal "http"
-    env["mock.postdata"].must_be :empty?
+    env["mock.postdata"].must_be_nil
   end
 
   it "allow GET/POST/PUT/DELETE/HEAD" do
@@ -194,7 +197,7 @@
     env["QUERY_STRING"].must_include "baz=2"
     env["QUERY_STRING"].must_include "foo%5Bbar%5D=1"
     env["PATH_INFO"].must_equal "/foo"
-    env["mock.postdata"].must_equal ""
+    env["mock.postdata"].must_be_nil
   end
 
   it "accept raw input in params for GET requests" do
@@ -204,7 +207,7 @@
     env["QUERY_STRING"].must_include "baz=2"
     env["QUERY_STRING"].must_include "foo%5Bbar%5D=1"
     env["PATH_INFO"].must_equal "/foo"
-    env["mock.postdata"].must_equal ""
+    env["mock.postdata"].must_be_nil
   end
 
   it "accept params and build url encoded params for POST requests" do

test/spec_mock_response.rb

@@ -14,7 +14,10 @@
 app = Rack::Lint.new(lambda { |env|
   req = Rack::Request.new(env)
 
-  env["mock.postdata"] = env["rack.input"].read
+  if input = env["rack.input"]
+    env["mock.postdata"] = input.read
+  end
+
   if req.GET["error"]
     env["rack.errors"].puts req.GET["error"]
     env["rack.errors"].flush