cleanly handle http.MaxBytesError for enforcing req payload size

[bgentry]

One more feature building on #4: making it easy to enforce a max request payload size.

If a middleware applies http.MaxBytesReader and we encounter an
*http.MaxBytesError when reading the request body, return an HTTP 413
with a clean error.

This does not actually have a function for applying a limit, only for
handling those errors when they occur.

[brandur]

Hm, the only thing I don't love about this is that it'd have to be configured on every endpoint — so you'd end up adding quite a bit of noise to each one, and it'd really easy to accidentally forget.

I've traditionally implemented this with middleware:

type MaxBytesReaderMiddleware struct {
	pservice.BaseMiddleware
	maxBytes int64
}

func NewMaxBytesReaderMiddleware() *MaxBytesReaderMiddleware {
	return &MaxBytesReaderMiddleware{
		// 1 megabyte
		maxBytes: 1 * 1024 * 1024,
	}
}

func (m *MaxBytesReaderMiddleware) Wrapper(next http.Handler) http.Handler {
	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		if r.Body != nil {
			r.Body = http.MaxBytesReader(w, r.Body, m.maxBytes)
		}

		next.ServeHTTP(w, r)
	})
}

This works, but doesn't produce the nice 413.

Looking at this again, I'm kind of wondering if it might not be a bad idea to have something like a "high level" apiframe middleware variant that's able to intercept a request at the top of the stack, modify it (i.e. make req.Body a max reader), and then potentially change out an error after response to something better (like a custom API error instead of an internal error). Maybe this would even look like River hooks ... (i.e. functions instead of a wrapper, hmm).

[bgentry]

Yeah, I was also thinking it doesn’t make sense to configure for every endpoint and could probably just be a global safety check. Really you just want to put a reasonable cap on most payload sizes to avoid the DoS vector, and unless you have some endpoints that take large payloads (file uploads & such) you can probably get away with a single limit everywhere.

Do you want to take a crack at the hooks option?

[brandur]

Yeah, I was also thinking it doesn’t make sense to configure for every endpoint and could probably just be a global safety check. Really you just want to put a reasonable cap on most payload sizes to avoid the DoS vector, and unless you have some endpoints that take large payloads (file uploads & such) you can probably get away with a single limit everywhere.

Do you want to take a crack at the hooks option?

What do you think about repurposing this to just put in with a hard-coded don't-DDOS-us-bro limit to the core API infrastructure, then I could go and try to extract it to a separate middleware later. (Or even not, we could probably just get away with the global limit for a while.)

[bgentry]

@brandur had another idea here and reworked it. If a middleware applies http.MaxBytesReader and we encounter an *http.MaxBytesError when reading the request body, return an HTTP 413 with a clean error. This does not actually have a function for applying a limit, only for handling those errors when they occur.

[brandur]

@brandur had another idea here and reworked it. If a middleware applies http.MaxBytesReader and we encounter an *http.MaxBytesError when reading the request body, return an HTTP 413 with a clean error. This does not actually have a function for applying a limit, only for handling those errors when they occur.

Cool, but wait, who applies the limit then? A middleware/hook we put in later?

[brandur]

Maybe just keep punctuation as a convention on errors in here (the others have it already).

Suggested change

	return apierror.NewRequestEntityTooLarge("Request entity too large")
	return apierror.NewRequestEntityTooLarge("Request entity too large.")

[bgentry]

Thanks!

[bgentry]

Cool, but wait, who applies the limit then? A middleware/hook we put in later?

Yes, I did this in https://github.com/riverqueue/riverapi/pull/36/files#diff-0eb779b9e49d8e44b0f36923fdb8d87d5ee024f886eefc45deec4ec88380a087R57-R63

Basically this adds some built-in handling as part of apiframe for when users happen to use http.MaxBytesReader, only handling the specific error it produces and only when reading the request body.

I'm not sure if there's a cleaner way to do this globally given the current structure of this package—there's not really a global place where you could even configure it, only individual endpoints/handlers that mount themselves to a mux. But the error specifically shows up when reading the request body, which is something the framework does handle, so it sort of needs to be the thing that detects the specific error.