Bug in Rack::Utils::Multipart.parse_multipart, dropped one of the parts

[Jacob Burkhart]

It seems to be an oddity related to the way a particular file ends and
then having more multipart content afterwards. see
http://gist.github.com/73561

It seems rack is making some unsafe assumptions about how much to slice off in:

# 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

change it to:

if i = buf.index(rx)
body << result = buf.slice!(0, i)
puts "result of first slice #{result}"
r2 = buf.slice!(0, boundary_size+2)
puts "result of second slice #{r2}"

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

And the debug output will show you that something is off...

[candlerb]

Have you got a test case for this that you can share? How does it
differ from those in test/multipart/* ?

[Jacob]

The gist was supposed to be a quasi-test to demonstrate the problem.
I'll fork and write up a more formal test case...

[Jacob]

Ok, So I've finally tracked it down.

It relates both to the Regular expression rack was using in 0.9.1 and
the fact that Rails sets $KCODE='u' which changes how strings with
unicode characters in them will respond to the method "index"

So the latest rack has some changes to parse_multipart and doesn't
fail the test.

So I forked rack, added this test, and then reverted the
parse_multipart method to the code from my 0.9.1 gem.

The result is here http://github.com/jacobo/rack

But... after all of that... I figured out that this has already been
fixed in http://github.com/rack/rack/commit/69b24cb7afceb0805ae6abe48575eb6c70190b7d
and there's already a test for it.

So I guess the only thing left to ask is.... when is the next gem
release of rack? (which includes this fix)

[Christian Neukirchen]

Jacob <igot...@gmail.com> writes:

> So I guess the only thing left to ask is.... when is the next gem
> release of rack? (which includes this fix)

It's time for 1.0 release candidates I think. Let's see what I can
get together this week. (Which version should they be?)

--
Christian Neukirchen <chneuk...@gmail.com> http://chneukirchen.org

[Scytrin dai Kinthra]

I can devote time on friday and saturday (PST) for some revision
checks and testing.

--
stadik.net

[candlerb]

Aside - the regexp used looks a bit dodgy to me:

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

This says "it can start with EOL, or not, it doesn't make any
difference". However that means it will match a boundary string which
occurs in the middle of a line. Any MIME body which did that would be
broken.

I think a clearer and safer regexp is this:

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

Making this change doesn't break any tests. It would however make the
decoding more strict, because EOL = "\r\n". If anyone has been wrongly
terminating their lines with only \n, then it may break them.

I say "safer" because there appears to be some logic which assumes
that a 2-byte EOL is always present:

boundary_size = boundary.size + EOL.size

But I think I see a separate bug in MIME decoding, since at first
glance I think it won't skip any preamble. If so, I'll make a separate
test case for that to demonstrate.

[candlerb]

> But I think I see a separate bug in MIME decoding, since at first
> glance I think it won't skip any preamble.

Whilst technically a bug (see example in RFC 2046 5.1.1), turns out
Rack's MIME parser is hard-coded only to handle multipart form posts,
and browsers don't add preamble/epilogue. So it doesn't really matter.

Shame, because I was looking for a lightweight MIME parser for
multipart/mixed :-)

I'll probably end up hacking either this one, or Ruby's soap/
mimemessage.rb

[Yehuda Katz]

I believe that at one point Josh told me that no multipart/mixed parser existed at all in Ruby. Weird.

-- Yehuda

2009/3/5 candlerb <b.ca...@pobox.com>

--
Yehuda Katz
Developer | Engine Yard
(ph) 718.877.1325