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

net/http: add Server.MaxBodyBytes int64 field #39567

Open
rolandshoemaker opened this issue Jun 12, 2020 · 18 comments · May be fixed by #48104
Open

net/http: add Server.MaxBodyBytes int64 field #39567

rolandshoemaker opened this issue Jun 12, 2020 · 18 comments · May be fixed by #48104

Comments

@rolandshoemaker
Copy link
Member

@rolandshoemaker rolandshoemaker commented Jun 12, 2020

Using ioutil.ReadAll on http.Request.Body is a rather common pattern (and one which is in fact used in at least one of the net/http examples) which can be somewhat dangerous as it can cause unbounded reads, leading to memory exhaustion and/or other funky behavior down the line when operating on the read contents (i.e. causing a stack overflow in encoding/json with massively nested structures being unmarshalled into an interface{}, see #31789).

The common solution to this problem is using http.MaxBytesReader (or less ideally ioutil.LimitedReader) either in a top level handler that wraps the http.Request.Body io.ReadCloser on all incoming requests (which is a bit boilerplate-y), or on each handler where you plan to read the request body (which is also quite verbose, and easy to forget to do leading to a vulnerable endpoint).

Ideally you would be able to set a field on http.Server, which when non-zero would automatically replace the request body reader with a MaxBytesReader on all incoming requests, preventing the user from having to either implement a top level handler, or a per handler reader replacement.

@andybons andybons changed the title net/http: allow setting a MaxBytesReader on all requests in http.Server proposal: net/http: allow setting a MaxBytesReader on all requests in http.Server Jun 15, 2020
@gopherbot gopherbot added this to the Proposal milestone Jun 15, 2020
@andybons
Copy link
Member

@andybons andybons commented Jun 15, 2020

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Nov 11, 2020
@rsc rsc moved this from Incoming to Active in Proposals Nov 18, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Nov 18, 2020

We do have MaxHeaderBytes in http.Server already.
Are you suggesting to add MaxBodyBytes int64?

@rsc rsc changed the title proposal: net/http: allow setting a MaxBytesReader on all requests in http.Server proposal: net/http: add Server.MaxBodyBytes int64 field Dec 2, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Dec 2, 2020

Based on the discussion above, this seems like a likely accept.

@rolandshoemaker
Copy link
Member Author

@rolandshoemaker rolandshoemaker commented Dec 2, 2020

Ah sorry, I completely missed the comment from two weeks ago

Are you suggesting to add MaxBodyBytes int64?

Yep, that is the meat of the proposal.

@rsc rsc moved this from Active to Likely Accept in Proposals Dec 2, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Dec 9, 2020

No change in consensus, so accepted.

@rsc rsc moved this from Likely Accept to Accepted in Proposals Dec 9, 2020
@rsc rsc changed the title proposal: net/http: add Server.MaxBodyBytes int64 field net/http: add Server.MaxBodyBytes int64 field Dec 9, 2020
@rsc rsc removed this from the Proposal milestone Dec 9, 2020
@rsc rsc added this to the Backlog milestone Dec 9, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 15, 2021

Change https://golang.org/cl/310529 mentions this issue: net/http: add MaxBodyBytes to Server

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Apr 18, 2021

Sorry, I didn't see this proposal until now, so some late feedback which I also left on Gerrit:

I just realized why this change feels a bit odd to me: this is the first http.Server tunable field that can be done with an http.Handler wrapper instead.

Historically we've only added knobs to Server when there was no other choice, but in this case anybody who can configure the http.Server can already wrap its Handler with a Handler that sets MaxBytesReader.

So I'm not super hot on this change. I don't dislike it enough to argue strongly against it, but I want to make sure others agree with this breaking of precedent before we do this.

The top comment on this issue touched on this ("a top level handler that wraps the http.Request.Body"), but I'm not sure others are aware that this is the first case of a Server tunable that's not really required.

We cool with that?

@jameshartig
Copy link
Contributor

@jameshartig jameshartig commented Apr 18, 2021

Doesn't a handler just need to call http.MaxBytesReader and replace the body on the Request? That seems simple enough that a server knob isn't necessary. The original proposal even mentions this. If that really feels like too much work then could a helper method that wraps a handler be added to the stdlib rather than another knob on Server?

Another downside of the Server knob is that you can't control it per path so anyone that needs to do that will end up using the handler method.

I also don't think the extra field on server adds much visibility to the problem if the common use case is ListenAndServe anyways.

Additionally, any library that wants to do this still has to do the handler method as long as they support older versions of Go.

@rolandshoemaker
Copy link
Member Author

@rolandshoemaker rolandshoemaker commented Apr 19, 2021

Sorry, I didn't see this proposal until now

No worries, I probably should've tagged a few HTTP folks initially.

The original rationale for this proposal was that one of the most common HTTP related security issues we see in internet facing servers is people either not limiting the request body size at all, or limiting request body size inconsistently. Part of fixing this problem is more strongly documenting the security issues of not limiting body size, but another is making fixing the problem as frictionless as possible for the user (one thing I've heard repeatedly is "if it's important that I do this, why isn't it enabled by default").

I agree this breaks precedent, and I don't really love it, but I think it's the least terrible, and probably most usable, solution to the issue. Another considered solution was to add a MaxBodySizeHandler as a complement to the other existing ...Handler functions, but it has the same downside as other existing solutions which is that without a default it is quite easy to forget to use when adding a new handler, leaving an unprotected route.

Another downside of the Server knob is that you can't control it per path so anyone that needs to do that will end up using the handler method.

I think this is a reasonable issue, which has similar considerations as the existing ReadTimeout field. It is quite common to have some endpoint where you'd like a significantly higher limit than other endpoints (i.e. an upload, streaming, or some other authenticated endpoint). Perhaps one solution would be adding a method to Request which allowed setting the max body size, SetMaxBodySize, which would wrap, or explicitly re-wrap, the Body with a MaxBytesReader allowing a single endpoint to override the default limit (one other proposal would be to add just this method, but I think the same arguments apply as previously, it is much easier for users to set a default and then override it once or twice than having to remember to set it in every handler).

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented May 14, 2021

I like the idea of a Handler wrapper func: it would fit nicely beside TimeoutHandler and StripPrefix in the godoc.

@networkimprov
Copy link

@networkimprov networkimprov commented May 16, 2021

@fraenkel
Copy link
Contributor

@fraenkel fraenkel commented May 16, 2021

It's easy to make this work for http. I am trying to figure out how this would work for http/2 since it doesn't have all the necessary plumbing.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented May 17, 2021

@fraenkel, per discussion above, this can all be done with an http.Handler wrapper. The http2 package doesn't need to be aware of this: we can just hand it a Handler that does the right thing.

@fraenkel
Copy link
Contributor

@fraenkel fraenkel commented May 17, 2021

I get that. However the entire body is sent and queued up in the pipe. The http case will add a connection close header.

@carlmjohnson
Copy link
Contributor

@carlmjohnson carlmjohnson commented Aug 12, 2021

Can this issue be renamed "net/http: add MaxByteHandler(h Handler, n int64) Handler"? ISTM, that's the consensus. Happy to open a CL for that if it is.

@programmer04
Copy link
Contributor

@programmer04 programmer04 commented Aug 28, 2021

It'd be beneficial to do #30715 too. Having the error defined for this case ensures that users can easily check the error returned from e.g. io.ReadAll and return nice information from their API to a client (e.g. proper error code and custom description) or log something.

@carlmjohnson
Copy link
Contributor

@carlmjohnson carlmjohnson commented Aug 28, 2021

Yes, they work well as a pair.

@carlmjohnson carlmjohnson linked a pull request that will close this issue Aug 31, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Aug 31, 2021

Change https://golang.org/cl/346569 mentions this issue: net/http: add MaxBytesHandler

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

Successfully merging a pull request may close this issue.

10 participants