socketry / multipart-post

Adds multipart POST capability to net/http
MIT License
293 stars 72 forks source link

About the end of the line have two CRLFs #40

Closed huacnlee closed 7 years ago

huacnlee commented 9 years ago

https://github.com/nicksieger/multipart-post/commit/0af8dde1343b2d0a2cb4993d4cde2de099b11b98#diff-9d2b9709d7d6d6706a5d7de795405083R62

Current version of multipart-post have two \r\n in the end of line:

class EpiloguePart
  include Part
  def initialize(boundary)
    @part = "--#{boundary}--\r\n\r\n"
    @io = StringIO.new(@part)
  end
end

But the same case in other programming languages stdlib there just have one For example Go:

http://golang.org/src/mime/multipart/writer.go?s=619:653#L160

153 func (w *Writer) Close() error {
154     if w.lastpart != nil {
155         if err := w.lastpart.close(); err != nil {
156             return err
157         }
158         w.lastpart = nil
159     }
160     _, err := fmt.Fprintf(w.w, "\r\n--%s--\r\n", w.boundary)
161     return err
162 }

Is \r\n\r\n in the end of the line is correct? This will not let our project upload success, becuase upload server care about that.

pavel-jurasek commented 9 years ago

@huacnlee

http://www.w3.org/Protocols/rfc1341/7_2_Multipart.html

The requirement that the encapsulation boundary begins with a CRLF implies that the body of a multipart entity must itself begin with a CRLF before the first encapsulation line -- that is, if the "preamble" area is not used, the entity headers must be followed by TWO CRLFs

pavel-jurasek commented 9 years ago

@huacnlee so i would say this should be change since python use the same as Go!

pavel-jurasek commented 9 years ago

https://github.com/andrew-d/python-multipart/blob/master/multipart/multipart.py#L1020-L1031

lukec commented 7 years ago

got bit by this bug today..... must stop using this library when the server has strict multi-part parsing.

mcolyer commented 7 years ago

I also just hit this issue, fixed in my fork: https://github.com/mcolyer/multipart-post.

ioquatix commented 7 years ago

What is the conclusion of this?

Can we either:

mcolyer commented 7 years ago

Change the default behaviour?

I'd vote for this, strict in what you send and liberal in what you interpret.

ioquatix commented 7 years ago

@mcolyer can you submit a PR?

ioquatix commented 7 years ago

Also, if you submit a PR, can you include first a failing test case(s), so we can track any regressions in the future. Ideally in the test case can you refer to the behaviour in the RFC so it's clear we are testing/doing the right thing? Don't forget to rebase on the latest master either :) Thanks so much in advance :D

ioquatix commented 7 years ago

Also, please forgive me, but doesn't:

The requirement that the encapsulation boundary begins with a CRLF implies that the body of a multipart entity must itself begin with a CRLF before the first encapsulation line -- that is, if the "preamble" area is not used, the entity headers must be followed by TWO CRLFs

Mean that we are doing the right thing already?

mcolyer commented 7 years ago

Also, please forgive me, but doesn't:

The issue I had was with the epilogue and not the preamble. And yes according to the spec, "This is the epilogue. It is also to be ignored.".

However ModSecurity doesn't ignore it and it fails their strict multipart check, more specifically MULTIPART_DATA_AFTER.

can you submit a PR?

Happy to open up a pull with my current commit but I'd be looking for some help with the test case as I can't devote much time to this currently.

ioquatix commented 7 years ago

It would be great if you could start the ball rolling on the PR.

ioquatix commented 7 years ago

This will be fixed on the next release.