Skip to content
This repository

Multipart requests with content types raise an exception #186

Closed
jdelStrother opened this Issue · 6 comments

5 participants

Jonathan del Strother whataboutbob Jessica Suttles Jake Dempsey James Tucker
Jonathan del Strother

Hi,

I'm using Rack 1.2.3. If a client sends a multipart request where one of the (regular, non-file) fields has a content type, rack raises an error:

NoMethodError: undefined method `rewind' for "Very very blue":String
    ./lib/rack/utils.rb:575:in `parse_multipart': Rack::Utils::Multipart
    ./lib/rack/utils.rb:499:in `loop'
    ./lib/rack/utils.rb:499:in `parse_multipart'

At https://github.com/rack/rack/blob/1.2.3/lib/rack/utils.rb#L575, it calls body.rewind if there's a content type but not a filename. However, body is only going to respond to 'rewind' if there was a filename - otherwise we don't assign a Tempfile to the body (https://github.com/rack/rack/blob/1.2.3/lib/rack/utils.rb#L534) ... so you end up with "undefined method 'rewind' for String"

I created a failing test to demonstrate the problem at https://gist.github.com/28075f66b2a490431083. When we were using Rack 1.1, it was fine with this use case. There's also an old discussion on google groups that may be related, if you're interested in the background (https://groups.google.com/d/msg/rack-devel/JKD6j_Pc5Ic/q_cBPixY_hcJ).

Any thoughts?
-Jonathan

whataboutbob

I just came across this issue too and it's a big problem for me because my app basically chokes on this. I 'm on Heroku and can't obviously change Rack source code (not that I want to) and tried to add this to application_controller.rb without any luck

rescue_from NoMethodError do |exception|
if exception.name == :rewind
    logger.debug "rewind error" 
  else
    rescue_action_without_handler(exception)
  end
end

Any idea if I can override the Rack call or trap the error in my application somehow.

Anyone knows if Rack 1.3.2 solves this issue? I tried to bundle install 1.3.2 but it threw an error

Bundler could not find compatible versions for gem "rack":
In Gemfile:
rails (= 3.0.9) depends on
  rack (~> 1.2.1)

rack (1.3.2)

UPDATE: After a couple of days of hacking, I was able to monkey patch parse.multipart, it's 2 lines of code but it took me 2 days to debug and identify the fixes. If anyone faces this issue and wants to know what I did, message me.

Jonathan del Strother

Sorry I missed your message. FWIW, we have a file in config/initializers to patch things, looking something like

# -*- encoding: binary -*-
require 'rack/utils'
module Rack
  module Utils
    module Multipart
      def self.parse_multipart(env)
        unless env['CONTENT_TYPE'] =~
            %r|\Amultipart/.*boundary=\"?([^\";,]+)\"?|n
          nil
        else
          boundary = "--#{$1}"

          params = {}
          buf = ""
          content_length = env['CONTENT_LENGTH'].to_i
          input = env['rack.input']
          input.rewind

          boundary_size = Utils.bytesize(boundary) + EOL.size
          bufsize = 16384

          content_length -= boundary_size

          read_buffer = ''

          status = input.read(boundary_size, read_buffer)
          raise EOFError, "bad content body"  unless status == boundary + EOL

          rx = /(?:#{EOL})?#{Regexp.quote boundary}(#{EOL}|--)/n

          loop {
            head = nil
            body = ''
            filename = content_type = name = nil

            until head && buf =~ rx
              if !head && i = buf.index(EOL+EOL)
                head = buf.slice!(0, i+2) # First \r\n
                buf.slice!(0, 2)          # Second \r\n

                token = /[^\s()<>,;:\\"\/\[\]?=]+/
                condisp = /Content-Disposition:\s*#{token}\s*/i
                dispparm = /;\s*(#{token})=("(?:\\"|[^"])*"|#{token})*/

                rfc2183 = /^#{condisp}(#{dispparm})+$/i
                broken_quoted = /^#{condisp}.*;\sfilename="(.*?)"(?:\s*$|\s*;\s*#{token}=)/i
                broken_unquoted = /^#{condisp}.*;\sfilename=(#{token})/i

                if head =~ rfc2183
                  filename = Hash[head.scan(dispparm)]['filename']
                  filename = $1 if filename and filename =~ /^"(.*)"$/
                elsif head =~ broken_quoted
                  filename = $1
                elsif head =~ broken_unquoted
                  filename = $1
                end

                if filename && filename !~ /\\[^\\"]/
                  filename = Utils.unescape(filename).gsub(/\\(.)/, '\1')
                end

                content_type = head[/Content-Type: (.*)#{EOL}/ni, 1]
                name = head[/Content-Disposition:.*\s+name="?([^\";]*)"?/ni, 1] || head[/Content-ID:\s*([^#{EOL}]*)/ni, 1]

                if filename
                  body = Tempfile.new("RackMultipart")
                  body.binmode  if body.respond_to?(:binmode)
                end

                next
              end

              # Save the read body part.
              if head && (boundary_size+4 < buf.size)
                body << buf.slice!(0, buf.size - (boundary_size+4))
              end

              c = input.read(bufsize < content_length ? bufsize : content_length, read_buffer)
              raise EOFError, "bad content body"  if c.nil? || c.empty?
              buf << c
              content_length -= c.size
            end

            # Save the rest.
            if i = buf.index(rx)
              body << buf.slice!(0, i)
              buf.slice!(0, boundary_size+2)

              content_length = -1  if $1 == "--"
            end

            if filename == ""
              # filename is blank which means no file has been selected
              data = nil
            elsif filename
              body.rewind

              # Take the basename of the upload's original filename.
              # This handles the full Windows paths given by Internet Explorer
              # (and perhaps other broken user agents) without affecting
              # those which give the lone filename.
              filename = filename.split(/[\/\\]/).last

              data = {:filename => filename, :type => content_type,
                      :name => name, :tempfile => body, :head => head}
            # elsif !filename && content_type
            #   body.rewind
            # 
            #   # Generic multipart cases, not coming from a form
            #   data = {:type => content_type,
            #           :name => name, :tempfile => body, :head => head}
            else
              data = body
            end

            Utils.normalize_params(params, name, data) unless data.nil?

            # break if we're at the end of a buffer, but not if it is the end of a field
            break if (buf.empty? && $1 != EOL) || content_length == -1
          }

          input.rewind

          params
        end
      end
    end
  end
end

The only real change there is

               data = {:filename => filename, :type => content_type,
                       :name => name, :tempfile => body, :head => head}
-            elsif !filename && content_type
-              body.rewind
-
-              # Generic multipart cases, not coming from a form
-              data = {:type => content_type,
-                      :name => name, :tempfile => body, :head => head}
+            # elsif !filename && content_type
+            #   body.rewind
+            # 
+            #   # Generic multipart cases, not coming from a form
+            #   data = {:type => content_type,
+            #           :name => name, :tempfile => body, :head => head}
             else
               data = body
             end

so if we're missing a filename parameter, we just treat it as a regular string value.

whataboutbob

No worries, thanks for sharing this, I did something similar though not quite the same. I noticed that you set the encoding as binary at the beginning, I used force_encoding. Also I decided to leave in the functionality of handling generic multipart cases with a small change, elsif !filename && !content_type.eql?("text/plain") && !content_type.nil?. Figures there might be a need for non-text/plain content type in the future.

Jessica Suttles

jdelStrother, thank you for sharing your solution. It just saved me some time :)

Jake Dempsey

i added the patch to fix an issue i had with the aurigma uploader... but has this been planned as a fix?

Jonathan del Strother

It's fixed in the version of Rack we use now (1.3.5). I think it was fixed back in 1.3 but couldn't say for sure off the top of my head.

James Tucker raggi closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.