Hello

Seems from Go 1.9 to 1.10, the behavior of mime/multipart Reader method ReadForm changes, you can see the new code here:

https://golang.org/src/mime/multipart/formdata.go#L61

the presence of a "Content-Type" header force the read interpret this part as a Form File, instead a Form Value

https://play.golang.org/p/lykpJ6Q01t5

I think it is linked to issue #19183 - Investigate this because I think it is interesting, I am not involving in the golang development ( at least, not yet )

thanks for looking into it, that looks like the culprit! There's examples of specifying a Content-Type on non-file fields in the spec https://www.w3.org/Protocols/rfc1341/7_2_Multipart.html

this seems the case when the filename is empty versus absense of filename

The data is still there, and is accessible via req.FormFile. I don't know whether there is something to fix here or not.

CC @nilsmagnus

Since the change doesn't appear to be intentional, it seems that we should restore the old behavior (while keeping whatever fix was intended). CLs welcome.

More details in #24060

Change https://golang.org/cl/96975 mentions this issue: mime/multipart: test for presence of filename instead of content-type

@ianlancetaylor

Moving to 1.10.2 due to discussions about this not potentially being the right fix.

hey @andybons I'd love to help out, any chance of more details? thanks!

I'll let @ianlancetaylor chime in on that one.

My concern is simply that I don't know how to be sure that this is the right fix, and I want to be careful that we don't jerk people around in the 1.10 release cycle. I would open to simply reverting the whole thing in 1.10.1 so that we can try again in 1.11.

Totally understand, as it stands this means upgrading to 1.10 will break codebases, I guess it depends on how many people you feel are left to update vs have already updated.

FWIW as an implementer and someone effected by this bug I wouldn't mind reverting code I wrote to work around a regression.

If we're not patching perhaps we could add a "known regressions" note to 1.10?

I had this one bite me as well. I have a bit of production code that I was testing under go1.10 failing, which wasn't in previous versions.

The gist of the issue is that I have a form file that is optional. I test for the existence of the file by going:

 _, _, err = r.FormFile("my_file")  
if err != nil {  
    if err != http.ErrMissingFile {
    // etc
   }
}

Omitting the file from the form doesn't yield an error any longer. This causes my application to fail later on when it tries to validate that file. I can work around this, but it doesn't seem like the intended behaviour to me.

It is this code snippet that concerns me the most: f69ad10#diff-418b3661e18738914329ca17445a46a8

I'm normally silent on github, so I apologise if I'm intruding. Please kindly tell me to be quiet if I'm sticking my nose where I don't belong.

@gopherbot please open a 1.10 backport tracking issue. See the discussion about what to backport in #24041.

Backport issue(s) opened: #25040 (for 1.10).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

I imagine my team is not the only one holding out before upgrading to 1.10 until this is resolved. The milestone has shifted from 1.10.1 to 1.10.2 and now 1.11. Are there strong opinions against reverting to the 1.9 behaviour or is a CL/PR what is required?

I'm dowgrading to 1.8, and apparently when 2 go installed 1.10 and 1.8 this problem still arise '__')
even when go version command shows 1.8
crazy when only 1 line changed for this year in my codebase, built with my deployment server cause a problem a week later..
then finding out that this was the culprit..

So, this was closed but it seems that it should not have been since we’re not clear on the fix? @ianlancetaylor

I've applied the "patch" from the backport PR and it seems to work form me. If it helps.

Frankly I have no idea what the right thing to do is here. I regret approving the original CL (https://golang.org/cl/70931).

As I understand it, and I probably don't, the issue is about where we store the data associated with a part of a multipart/form-data. We record it in either the Value field or the File field of the returned multipart.Form value. When used with net/http, this plays into whether the code should use FormValue or FormFile.

In Go 1.9, if there was no filename, or if the filename was the empty string, we used the Value field.

In Go 1.10, this was changed (https://golang.org/cl/70931) so that we use the File field if there is a Content-Type header. In other words, previously we used the File field if there was a filename. In Go 1.10, we also use the File field if there a Content-Type header, even if the filename is missing. The argument for this change (in #19183) was that RFC 2388 says that the filename is optional. Note that the multipart.Form Value and File fields, and therefore with the http.Request FormValue and FormFile methods, the value is stored not using the filename, but using the name of the part. So even if the filename is missing, it is still possible to look up the value. In 1.10, looking up a value with a missing filename using File or FormFile will return a multipart.File with an empty Filename.

That change let to this issue being filed. Basically, cases that used to be in Value are now in File, causing an unexpected change.

So in the future 1.11, the behavior was changed again, by https://golang.org/cl/96975. Now if there is no filename, we use the Value field. This is closer to the 1.9 behavior, but the behavior differs if there is a filename that is the empty string. In 1.9 an empty filename used the Value field. In 1.11, an empty filename uses the File field. In both cases a missing filename uses the Value field.

So CL 96975 essentially undoes the argument in #19183, in that once again the filename is relevant to the choice of representation, even though the RFC says that the filename is optional. However, it does so in a way that preserves the behavior of the test added in CL 70931, which has a filename that is the empty string.

As far as I can tell none of this matters very much. The data is available either way. It's just that you have to access it in different ways. People writing new code can probably figure it out. For existing code, we shouldn't change things.

So I think there is a solid argument to be made for reverting CL 96975, then reverting CL 70931, which will get us back to the state we were in in 1.9, then closing #19183 as "sorry," and closing this one because we've gone back to the old behavior. And then, for 1.10.3, reverting 70931 to go back to the 1.9 state.

And then we can try to forget the whole thing.

@nilsmagnus @matiasanaya see previous comment.

I think Ian's suggestion makes sense but it looks like this did not make it into 1.10.3?

@ianlancetaylor I agree with you on reverting this change. I had no idea that this issue would cause that much trouble in the first place.

@gopherbot please backport to 1.10

Change https://golang.org/cl/121055 mentions this issue: mime/multipart: restore 1.9 handling of missing/empty form-data file name

Change https://golang.org/cl/121075 mentions this issue: [release-branch.go1.10] mime/multipart: restore 1.9 handling of missing/empty form-data file name