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: MaxBytesHandler overwrites request Context #62414

Closed
paul-leydier opened this issue Sep 1, 2023 · 3 comments
Closed

net/http: MaxBytesHandler overwrites request Context #62414

paul-leydier opened this issue Sep 1, 2023 · 3 comments

Comments

@paul-leydier
Copy link

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

$ go version
go version go1.20.7 windows/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
set GO111MODULE=on
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\PaulLEYDIER\AppData\Local\go-build
set GOENV=C:\Users\PaulLEYDIER\AppData\Roaming\go\env
set GOEXE=.exe
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Users\PaulLEYDIER\go\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Users\PaulLEYDIER\go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=C:/Users/PaulLEYDIER/go/go1.21.0
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLCHAIN=auto
set GOTOOLDIR=C:\Users\PaulLEYDIER\go\go1.21.0\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=go1.21.0
set GCCGO=gccgo
set GOAMD64=v1
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=0
set GOMOD=C:\Users\PaulLEYDIER\GitHub\maxBytesHandlerBug\go.mod
set GOWORK=
set CGO_CFLAGS=-O2 -g
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-O2 -g
set CGO_FFLAGS=-O2 -g
set CGO_LDFLAGS=-O2 -g
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -fno-caret-diagnostics -Qunused-arguments -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=C:\Users\PAULLE~1\AppData\Local\Temp\go-build3700301075=/tmp/go-build -gno-record-gcc-switches

What did you do?

The http.MaxBytesHandler function, which was created to be used as a middleware has an (IMO) undesirable side effect: it overwrites the request's Context.

Here's an example: go.dev/play link

In this example, we have a very simple HTTP server with a single endpoint. The endpoint writes some new value ({"myKey": "myValue}) in the request's Context. This handler is wrapper by two middlewares:

  • the standard library http.MaxBytesHandler
  • a custom logRequest middleware which checks if the request's Context new value written by the endpoint is still here. It prints it if it is here, but prints "Nothing found in context!" otherwise.

What did you expect to see?

Console output:

"In context: {myKey: myValue}"

What did you see instead?

Console output:

Nothing found in context!

Here is the very simple solution I adopted in the project in which I encountered this issue: go.dev/play link. It's basically just a rewrite of the standard library handler to not overwrite the context:

func MaxBytesHandler(h http.Handler, n int64) http.Handler {
	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		r.Body = http.MaxBytesReader(w, r.Body, n)
		h.ServeHTTP(w, r)
	})
}

Maybe this is not a bug, but is wanted. Maybe what I am doing is a bad practice and there are other ways for upstream middlewares to access data from downstream middlewares and endpoint? If that is the case, sorry about the issue and I would love to learn more about this.

However, if this turns out to be a real bug, I'd be happy to work on the fix!

@bboreham
Copy link
Contributor

bboreham commented Sep 1, 2023

You're not supposed to do that.

Except for reading the body, handlers should not modify the provided Request.

https://pkg.go.dev/net/http#Handler

@paul-leydier
Copy link
Author

You're not supposed to do that.

Except for reading the body, handlers should not modify the provided Request.

https://pkg.go.dev/net/http#Handler

Interesting, thanks for the link! How would you go about passing information from an endpoint to a logging middleware?

@seankhliao
Copy link
Member

working as intended.

@seankhliao seankhliao closed this as not planned Won't fix, can't repro, duplicate, stale Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants