Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

mime/multipart: "filename" Content-Disposition parameter should be optional #19183

Closed
matiasanaya opened this issue Feb 19, 2017 · 10 comments
Closed

mime/multipart: "filename" Content-Disposition parameter should be optional #19183

matiasanaya opened this issue Feb 19, 2017 · 10 comments
Labels
FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Go1.10

Comments

@matiasanaya
Copy link

What version of Go are you using?

go version go1.7.4 darwin/amd64

What operating system and processor architecture are you using?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/matias.anaya/go"
GORACE=""
GOROOT="/Users/matias.anaya/.goenv/versions/1.7.4"
GOTOOLDIR="/Users/matias.anaya/.goenv/versions/1.7.4/pkg/tool/darwin_amd64"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"

What did you do?

Tried to read a "file" in a multipart/form-data which did not have a filename.

What did you expect to see?

multipart.File and multipart.FileHeader being correctly built and returned.

What did you see instead?

Error: http: no such file

Thoughts?

multipart ReadForm assumes a "filename" will be present. RFC2388 mentions that this parameter is optional.

Looks like multipart.Form (which has a multipart.Form.Value and a multipart.Form.File map) tries to figure out what's a "file" and what's a "value" at parse time without caller input. Is this a decision that can be revisited?

Also wouldn't it be more correct to have a multipart.Form.Part abstraction which can be used as a "Value" (string) or a "File" (*FileHeader)?
@bradfitz bradfitz added help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Mar 21, 2017
@bradfitz bradfitz added this to the Go1.9Maybe milestone Mar 21, 2017
@odeke-em
Copy link
Member

@matiasanaya if interested, please feel free to send a CL and we'll review it.

@bradfitz bradfitz modified the milestones: Go1.10, Go1.9Maybe Jun 25, 2017
@matiasanaya
Copy link
Author

@odeke-em perfect, I'll work on it.

@nilsmagnus
Copy link
Contributor

nilsmagnus commented Oct 15, 2017 •
edited

How can we know if there is a file present? Is it enough to know that there is a Content-type header in the Part? I cannot see any other way of differentiating values from files in a multipart form. Please enlighten me if I am mistaking.

@gopherbot
Copy link

Change https://golang.org/cl/70931 mentions this issue: Allow for nameless files in a multipart-form.

@ianlancetaylor
Copy link
Contributor

I don't understand how you can expect to use FormFile, which requires a file name, to look up a multipart that doesn't have a file name. Why not use MultipartReader instead?

@nilsmagnus
Copy link
Contributor

nilsmagnus commented Oct 16, 2017 •
edited

I am not sure I understand your comment @ianlancetaylor , patch only changes the behaviour when a filename is not specified. From what I can see, the multipart.Reader is already beeing used.

With the change, all Parts with the content-type header will be treated as a file, with or without filename. The existing tests are still passing, I added a test for a nameless-file-part.

@ianlancetaylor
Copy link
Contributor

I am not sure I understand your comment @ianlancetaylor , patch only changes the behaviour when a filename is not specified.

Is that true? The original code tests filename == "". The new code does not. With the new code, if filename is not the empty string, but there is a Content-Type header, then it seems like the behaviour changes. What am I missing?

@nilsmagnus
Copy link
Contributor

Thanks, I was thinking about this just before I went to sleep yesterday, but forgot to add it. I will revise the change.

@peczenyj peczenyj mentioned this issue Feb 23, 2018
Closed
@gopherbot
Copy link

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

@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Jun 26, 2018
@ianlancetaylor
mime/multipart: restore 1.9 handling of missing/empty form-data file …
f040e43 
…name

Revert the code changes of CL 96975 and CL 70931, but keep the tests,
appropriately modified for the code changes. This restores the 1.9
handling of form-data entries with missing or empty file names.

Changing the handling of this simply confused existing programs for no
useful benefit. Go back to the old behavior.

Updates #19183
Fixes #24041

Change-Id: I4ebc32433911e6360b9fd79d8f63a6d884822e0e
Reviewed-on: https://go-review.googlesource.com/121055
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
gopherbot pushed a commit that referenced this issue Jun 26, 2018
@ianlancetaylor
[release-branch.go1.10] mime/multipart: restore 1.9 handling of missi…
b5375d7 
…ng/empty form-data file name

Revert the code change of CL 70931, but keep the test, appropriately
modified for the code changes. Also add a new test. This restores the
1.9 handling of form-data entries with missing or empty file names.

Changing the handling of this simply confused existing programs for no
useful benefit. Go back to the old behavior.

Updates #19183
Fixes #24041

Change-Id: Ie7a0309a061218ceda3bbc2a7da85e6fb3dd016d
Reviewed-on: https://go-review.googlesource.com/121075
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@golang golang locked and limited conversation to collaborators Jun 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Go1.10
Development

No branches or pull requests
6 participants
@bradfitz @nilsmagnus @matiasanaya @ianlancetaylor @odeke-em @gopherbot