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: ServeMux not validating methods for redirected paths #69690

Closed
Zaba505 opened this issue Sep 28, 2024 · 2 comments
Closed

net/http: ServeMux not validating methods for redirected paths #69690

Zaba505 opened this issue Sep 28, 2024 · 2 comments

Comments

@Zaba505
Copy link

Zaba505 commented Sep 28, 2024

Go version

go version go1.23.0 linux/amd64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='* REMOVED TO PREVENT LOCAL PATHS FROM BEING LEAKED */go-build'
GOENV='* REMOVED TO PREVENT LOCAL PATHS FROM BEING LEAKED */go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='* REMOVED TO PREVENT LOCAL PATHS FROM BEING LEAKED */pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='* REMOVED TO PREVENT LOCAL PATHS FROM BEING LEAKED *'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='* REMOVED TO PREVENT LOCAL PATHS FROM BEING LEAKED *'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='* REMOVED TO PREVENT LOCAL PATHS FROM BEING LEAKED */pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.23.0'
GODEBUG=''
GOTELEMETRY='local'
GOTELEMETRYDIR='* REMOVED TO PREVENT LOCAL PATHS FROM BEING LEAKED *'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='servemuxpoc/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build618692677=/tmp/go-build -gno-record-gcc-switches'

What did you do?

https://go.dev/play/p/_3usVyrcn1V

In the above Go Playground example, I'm registering a single http.HandleFunc with a http.ServeMux and leveraging
the new pattern format to require both the path, /hello, and method, HTTP GET to match for the request to be handled.

Once registered, I create an http.Server and run it in a goroutine. Afterward, I begin sending 2 requests to the server.
The first request contains // at the beginning of it's path and the second request does not. Note, both requests are sent
with the HTTP POST method.

What did you see happen?

with duplicated slash in url path
request POST http://[::]:29142//hello
received request POST //hello
writing response 301
received request GET /hello
writing response 500
response 500 GET

without duplicated slash in url path
request POST http://[::]:29142/hello
received request POST /hello
writing response 405
response 405 Method Not Allowed

The first request actually executes the HTTP GET pattern handler which results in a HTTP 500 status code being returned.

The second request did not execute the HTTP GET pattern handler and received a HTTP 405 status code.

What did you expect to see?

For the both requests, I expected to receive a HTTP 405 Method Not Allowed to be returned since each request method is HTTP POST.

After investigating, I came across the following:

which along with the logs seen in the output of the Go Playground example all lead to the first request being matched to http.RedirectHandler.

Now, I understand that http.RedirectHandler is expected behaviour but I believe this scenario to be a particular edge case
that leads to 2 expectations to collide. That being the request method matching of http.ServeMux and the http.RedirectHandler
behviour. As a user, I was geniunely surprised to not receive a 405 for POST //hello since cleaning //hello makes it /hello.
I think in this situation the HTTP method should be validated before http.RedirectHandler is returned but I'd love to hear others
thoughts on this behaviour.

NOTE: this is an edge case that only happens for patterns whose method for matching happens to be HTTP GET since
http.RedirectHandler redirects via HTTP GET i.e. incoming HTTP POST //hello -> redirect to HTTP GET /hello

@gazerro
Copy link
Contributor

gazerro commented Sep 28, 2024

The behavior you're experiencing is the one described in issue #60769. The current behavior is correct. Returning a 308 status instead of 301 would be more desirable, however, as discussed in issue #60769, there is a compatibility issue with older clients that do not recognize the 308 status.

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

4 participants