code review 6212046: mime/multipart: fix handling of empty parts without CRL...

mime/multipart: fix handling of empty parts without CRLF before next part

Empty parts can be either of the form:

a) "--separator\r\n", header (w/ trailing 2xCRLF), \r\n "--separator"...
or
b) "--separator\r\n", header (w/ trailing 2xCRLF), "--separator"...

We never handled case b).  In fact the RFC seems kinda vague about
it, but browsers seem to do a), and App Engine's synthetic POST
bodies after blob uploads is of form b).

So handle them both, and add a bunch of tests.

(I can't promise these are the last fixes to multipart, especially
considering its history, but I'm growing increasingly confident at
least, and I've never submitted a multipart CL with known bugs
outstanding, including this time.)

[bradfitz, 2012-05-14 19:11:12]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2012-05-14 19:36:25]

Hold on reviewing.

Reading the RFC, we don't handle optional LWSP after boundaries before CRLF.

I confirmed we fail in that case.  Will fix in this CL too.


On Tue, May 15, 2012 at 5:11 AM, <bradfitz@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> mime/multipart: fix handling of empty parts without CRLF before next
> part
>
> Empty parts can be either of the form:
>
> a) "--separator\r\n", header (w/ trailing 2xCRLF), \r\n "--separator"...
> or
> b) "--separator\r\n", header (w/ trailing 2xCRLF), "--separator"...
>
> We never handled case b).  In fact the RFC seems kinda vague about
> it, but browsers seem to do a), and App Engine's synthetic POST
> bodies after blob uploads is of form b).
>
> So handle them both, and add a bunch of tests.
>
> (I can't promise these are the last fixes to multipart, especially
> considering its history, but I'm growing increasingly confident at
> least, and I've never submitted a multipart CL with known bugs
> outstanding, including this time.)
>
> Please review this at
http://codereview.appspot.com/**6212046/<http://codereview.appspot.com/6212046/>
>
> Affected files:
>  M src/pkg/mime/multipart/**multipart.go
>  M src/pkg/mime/multipart/**multipart_test.go
>
>
>

[bradfitz, 2012-05-14 23:25:36]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2012-05-14 23:27:05]

This is ready for review now.

I think it's simpler than it was before now, as well as less (not?) buggy &
better tested.

On Mon, May 14, 2012 at 4:25 PM, <bradfitz@golang.org> wrote:

> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
>
http://codereview.appspot.com/**6212046/<http://codereview.appspot.com/6212046/>
>

[bradfitz, 2012-05-14 23:36:04]

FYI

http://codereview.appspot.com/6212046/diff/6001/src/pkg/mime/multipart/multip...
File src/pkg/mime/multipart/multipart_test.go (left):

http://codereview.appspot.com/6212046/diff/6001/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart_test.go:18: func TestHorizontalWhitespace(t
*testing.T) {
and this function is just gone now.

http://codereview.appspot.com/6212046/diff/6001/src/pkg/mime/multipart/multip...
src/pkg/mime/multipart/multipart_test.go:322: func TestZeroLengthBody(t
*testing.T) {
FYI: this is folded into new table-based tests below.

[adg, 2012-05-15 00:56:22]

LGTM

Wow.

[bradfitz, 2012-05-15 01:16:57]

*** Submitted as http://code.google.com/p/go/source/detail?r=32a8b0e41031 ***

mime/multipart: fix handling of empty parts without CRLF before next part

Empty parts can be either of the form:

a) "--separator\r\n", header (w/ trailing 2xCRLF), \r\n "--separator"...
or
b) "--separator\r\n", header (w/ trailing 2xCRLF), "--separator"...

We never handled case b).  In fact the RFC seems kinda vague about
it, but browsers seem to do a), and App Engine's synthetic POST
bodies after blob uploads is of form b).

So handle them both, and add a bunch of tests.

(I can't promise these are the last fixes to multipart, especially
considering its history, but I'm growing increasingly confident at
least, and I've never submitted a multipart CL with known bugs
outstanding, including this time.)

R=golang-dev, adg
CC=golang-dev
http://codereview.appspot.com/6212046