Multipart EOF don't have \r\n at the end

[eminarcissus]

I've used HTTPosion to do some multiparts uploads with hackney but found the server didn't recognized the request, and I've done quite a lot of tests in modifying src/lib/hackney_multiparts.erl to find out which part is the real culprit, finally figure out looks like the request issues from requests(python) have a \r\n at the end of request but not for the hackney lib.
here is my modification:

line 137:
mp_eof(Boundary) ->
<<"--", Boundary/binary, "--">>.

to
mp_eof(Boundary) ->
<<"--", Boundary/binary, "--\r\n">>.

And after the change everything works like a charm, don't know which one did really follow the rfc sheet, but If I'm remembered well every sentence communication via http should have a \r\n at the end, so maybe this is a bug over here.

Also for this function
mp_file_header({file, Path, ExtraHeaders}, Boundary) ->
FName = hackney_bstr:to_binary(filename:basename(Path)),
Disposition = {<<"form-data">>,
[{<<"name">>, <<"file">>},
{<<"filename">>, <<""", FName/binary, """>>}]},
mp_file_header({file, Path, Disposition, ExtraHeaders}, Boundary);
mp_file_header({file, Path, {Disposition, Params}, ExtraHeaders}, Boundary) ->
CType = hackney_mimetypes:filename(Path),
Len = filelib:file_size(Path),
ExtraHeaders0 = lists:map(fun ({K, V}) -> {hackney_bstr:to_lower(K), V} end, ExtraHeaders),
Headers = mp_filter_header([{<<"content-type">>, CType},
{<<"content-length">>, Len}],
[{<<"content-disposition">>, Disposition, Params} | ExtraHeaders0]),
BinHeader = mp_header(Headers, Boundary),
{BinHeader, Len}.

When sending Disposiion,ExtraHeaders, Disposition kv values didn't add quote " into the disposition,although it didn't make difference to the server, but it also is a point where it differs from what I get with python.requests.

from hackney
Content-Disposition: form-data; name=file; filename=avatar.jpg

from python
Content-Disposition: form-data; name="file"; filename="avatar.jpg"

so my temporary solution is add a quote into the value of name and filename temporary to make it works as expected.

Hope those can be merged and fixed soon, really appreciate your works on this lib :)

[benoitc]

Thanks! I will have a look on it, since it can break others lib like couchbeam we should be careful on such changes.

[jlouis]

I'm interested in this one as well. I think I have an upload which breaks cowboy due to this thing. I'll make a test with the above patch and see if it fixes the problem. I've also seen the {file, ...} api break due to this, but I'm guessing it is a buffering problem.

I'll make some investigation, but have you done any in advance on this problem?

[jlouis]

Relevant RFCs:

https://www.rfc-editor.org/rfc/rfc2388.txt
https://www.rfc-editor.org/rfc/rfc7578.txt (PROPOSED, Obsoletes 2388)

I haven't read through them yet, but will do so.

[jlouis]

RFC 7578 just punts to

http://www.rfc-editor.org/info/rfc2046 and
http://www.rfc-editor.org/info/rfc2047

Because the boundary is specificed inside those documents. You don't get any rules on boundary encoding from reading 7578 alone.

[jlouis]

RFC 2046, Section 5.1.1 Common Syntax states the following:

   The boundary delimiter MUST occur at the beginning of a line, i.e.,
   following a CRLF, and the initial CRLF is considered to be attached
   to the boundary delimiter line rather than part of the preceding
   part.  The boundary may be followed by zero or more characters of
   linear whitespace. It is then terminated by either another CRLF and
   the header fields for the next part, or by two CRLFs, in which case
   there are no header fields for the next part.  If no Content-Type
   field is present it is assumed to be "message/rfc822" in a
   "multipart/digest" and "text/plain" otherwise.

This sets up the rules of the general boundary. A later paragraph describes the last boundary:

   The boundary delimiter line following the last body part is a
   distinguished delimiter that indicates that no further body parts
   will follow.  Such a delimiter line is identical to the previous
   delimiter lines, with the addition of two more hyphens after the
   boundary parameter value.

     --gc0pJq0M:08jU534c0p--

Since the last boundary has to be identical to other boundaries, it is logical it ends with a CRLF to indicate end of the boundary. So this issue is at least correct with respect to the RFC as I read it. In other words, it is hackney and not cowboy that is wrong in my tests. I'll probably attempt making this issue into a PR.

[eminarcissus]

Interesting, so indeed this part does really have a bug here. I'm being kept patching this file by myself in order to use this lib, don't know why it didn't get confirmed yet(so I think there should be quite some of web servers does not recognize this kind of request).

[jlouis]

Well, I think @benoitc is at a conference right now, and I'm currently just investigating what the right™ thing to do is. I'll probably start working on the PR in earnest later this week -- for now I'm just trying to get a grip at who is right and who is wrong.

[eminarcissus]

Great thx for the help :)

[benoitc]

@jlouis thanks for the all the informations and research! I will have a look on the source later in the afternoon and provides a new release today with fixes if needed on it.

Anyway expect a new release, so people can use hackney again with erlang.mk

[jlouis]

@benoitc I'm working on a test case for it at the moment. I'll avoid actually hacking the fix up. Then we can check if the fix solves the problem.

[jlouis]

Ongoing work on https://github.com/jlouis/hackney/tree/jl/hackney-multipart-fix for interested parties.

[jlouis]

I'm now ready for testing when you get to the point where you have something which works. I'm not sure my code works in entirety, but I can reproduce the error I saw inside Cowboy with Hackney now, under the above knowledge Hackney is doing it wrong.