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

proposal: net/http: define and export RequestBodyTooLargeError #30715

Open
sslavic opened this issue Mar 10, 2019 · 11 comments
Open

proposal: net/http: define and export RequestBodyTooLargeError #30715

sslavic opened this issue Mar 10, 2019 · 11 comments
Labels
Projects
Milestone

Comments

@sslavic
Copy link

@sslavic sslavic commented Mar 10, 2019

What version of Go are you using (go version)?

$ go version
go version go1.12 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOBIN="/Users/foo/go/bin"
GOCACHE="/Users/foo/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/foo/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/Cellar/go/1.12/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.12/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/lf/rbfblwvx6rx3xhm68yksmqjwdv1dsf/T/go-build261266046=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Upon limiting the maximum request size (by using MaxBytesReader as in https://stackoverflow.com/a/28292505/381140) I wanted to specially handle too large request body error in a clean way with error type assertion, and return custom response body.
More people asked for the same feature: https://groups.google.com/forum/#!topic/golang-nuts/gMzVGDgPyrY

What did you expect to see?

I expected net/http to have RequestBodyTooLargeError defined and returned by MaxBytesReader so type assertion can be done instead of error message comparison.

What did you see instead?

Trivial error string implementation is returned https://golang.org/src/net/http/request.go#L1114

@ALTree ALTree added this to the Go1.13 milestone Mar 10, 2019
@perillo
Copy link
Contributor

@perillo perillo commented Mar 10, 2019

There is a similar issue in the parsePostForm function: https://golang.org/src/net/http/request.go#L1158.

@rsc rsc removed this from the Go1.13 milestone May 1, 2019
@rsc rsc added this to the Go1.14 milestone May 1, 2019
@rsc rsc removed this from the Go1.14 milestone Oct 9, 2019
@rsc rsc added this to the Backlog milestone Oct 9, 2019
@s3rj1k
Copy link

@s3rj1k s3rj1k commented Dec 10, 2019

@rsc At a very least set those string as constants, current code looks ugly and stands against all the things that we love in golang.

const http.StatusRequestEntityTooLargeMessage = "http: request body too large"

@carlmjohnson
Copy link
Contributor

@carlmjohnson carlmjohnson commented Aug 11, 2021

@rsc, I think this issue fell off the tracker because it's from before the "Proposal" tag. Can this be tagged? If the proposal is accepted, I'm happy to file the CL.

@ianlancetaylor ianlancetaylor changed the title net/http: Define and export RequestBodyTooLargeError proposal: net/http: define and export RequestBodyTooLargeError Aug 11, 2021
@ianlancetaylor ianlancetaylor removed this from the Backlog milestone Aug 11, 2021
@ianlancetaylor ianlancetaylor added this to the Proposal milestone Aug 11, 2021
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Aug 11, 2021
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Aug 11, 2021

CC @neild

@carlmjohnson
Copy link
Contributor

@carlmjohnson carlmjohnson commented Aug 12, 2021

FWIW, #41493 is a dupe with all positive feedback. I propose that MaxByteReader should return errors of the type ErrorMaxBytesExceeded struct { MaxSize int64 } that can be checked with errors.As.

@neild
Copy link
Contributor

@neild neild commented Aug 12, 2021

A http.MaxBytesReader is defined as "similar to io.LimitReader", but "returns a non-EOF error for a Read beyond the limit".

Wrapping the incoming request body in a MaxBytesReader is the recommended way to set a limit on the size of an incoming request body. It interacts with other elements of HTTP request processing in some non-obvious ways: For example, (*Request).ParseForm sets a 10MiB limit on the size of parsed form data, except when the request Body is a MaxBytesReader.

This proposal, as I understand it, is to return a defined error when a MaxBytesReader hits the read limit.

This seems reasonable to me.

The net/http package defines only one exported error type, http.ProtocolError, which is deprecated. It defines a number of sentinel error values. Since the creator of a MaxBytesReader knows what maximum size they provided it, I think that providing this information in the error response is unnecessary, and this should be a sentinel error. Perhaps:

// ErrRequestBodyTooLarge is returned after a ReadCloser created by MaxBytesReader reads
// past its limit.
var ErrRequestBodyTooLarge = errors.New("http: request body too large")

A couple questions, and my proposed answers:

  • Does Close return ErrRequestBodyTooLarge if the limit is exceeded? Currently Close does not return an error in this case, but I think it should.
  • Does Read return ErrRequestBodyTooLarge, or an error wrapping this one? I don't have a strong opinion here. If this was a new package, I'd argue for wrapping, but given the extensive use of bare sentinels in net/http I think consistency argues for returning an unwrapped ErrRequestBodyTooLarge.

@carlmjohnson
Copy link
Contributor

@carlmjohnson carlmjohnson commented Aug 12, 2021

Since the creator of a MaxBytesReader knows what maximum size they provided it, I think that providing this information in the error response is unnecessary, and this should be a sentinel error.

Not necessarily. I want to wrap different handlers in different middleware, so that the processImage handler has a 25MB limit, but the processUser handler has a 1MB limit, for example. I want to be able to write generic error response middleware that can prompt the user with different messages for different overages.

@carlmjohnson
Copy link
Contributor

@carlmjohnson carlmjohnson commented Sep 4, 2021

What if instead of defining an error type, we add http.IsMaxBytesError(err error) (size int)? If the error isn’t from MaxBytesReader, it returns 0, otherwise it returns a positive number. It’s easier to use than a defined type and lets us hide the implementation details.

@rsc
Copy link
Contributor

@rsc rsc commented Oct 20, 2021

If it is going to be generated by MaxBytesReader, perhaps it should be a MaxBytesError?
@neild are you satisfied by the justification above for including the number of bytes?
Or should we just do ErrMaxBytes / ErrRequestBodyTooLarge?

@rsc
Copy link
Contributor

@rsc rsc commented Oct 20, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals Oct 20, 2021
@carlmjohnson
Copy link
Contributor

@carlmjohnson carlmjohnson commented Oct 20, 2021

I would be happy with either of

type MaxBytesError int64

type MaxBytesError struct {
    Limit int64
}

The struct version is more extensible, but I don't foresee it needing extension, so int64 version is fine too.

A side-effect of this change is the secret requestTooLarger interface could be retired. I think it would probably stick around.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants