Skip to content

net/http: infinite redirect on path variable followed by trailing slash #67657

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

Closed
marwan-at-work opened this issue May 26, 2024 · 3 comments
Closed
Labels
FixPending Issues that have a fix which has not yet been reviewed or submitted. NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@marwan-at-work
Copy link
Contributor

marwan-at-work commented May 26, 2024

Go version

go version go1.22.3 darwin/arm64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/marwansulaiman/Library/Caches/go-build'
GOENV='/Users/marwansulaiman/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/marwansulaiman/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/marwansulaiman/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.22.3'
GCCGO='gccgo'
AR='ar'
CC='clang'
CXX='clang++'
CGO_ENABLED='1'
GOMOD='/Users/marwansulaiman/marwan/muxw/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 -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/ms/n54rxytd6612dh1dfyqzx30w0000gn/T/go-build2482118027=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

Run the following program:

package main

import (
	"fmt"
	"net/http"
)`

func main() {
	m := http.NewServeMux()
	m.HandleFunc("GET /users/{user}/{$}", func(w http.ResponseWriter, r *http.Request) {
		fmt.Fprintln(w, "RECEIVED", r.PathValue("user"))
	})
	http.ListenAndServe(":9191", m)
}

What did you see happen?

Run the following curls (following redirects)

1. Works as expected
curl -L localhost:9191/users/one
RECEIVED one

2. Works as expected
curl -L localhost:9191/users/one/
RECEIVED one

4. Infinite redirect
curl -L localhost:9191/users/    
curl: (47) Maximum (50) redirects followed

What did you expect to see?

The very last one should either 404 as it is not a registered command, or at the very least call the handler with an empty string for the {user} path value.

Note that I built Go from source, and the latest build gives me a "MethodNotAllowed" instead of infinite redirects. It certainly is an improvement, but it feels like that's the wrong error code? The method is in fact allowed, but just not the path?

@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 2, 2024
@neild
Copy link
Contributor

neild commented Jun 17, 2024

The redirect loop was fixed by https://go.dev/cl/562557.

I'm not sure what's going on with the 405 Method Not Allowed response after that CL. I'd expect a 404. The 405 is definitely wrong:

GET /users/ HTTP/1.1
Host: localhost

HTTP/1.1 405 Method Not Allowed
Allow: GET, HEAD

Sorry, we don't allow GET, only GET or HEAD. That's not right.

@neild neild added the NeedsFix The path to resolution is known, but the work has not been done. label Jun 17, 2024
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 17, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/594175 mentions this issue: net/http: matchingMethods: do not add "/" twice to the path

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/594737 mentions this issue: net/http: avoid appending an existing trailing slash to path again

@dmitshur dmitshur added this to the Go1.23 milestone Jun 27, 2024
@dmitshur dmitshur added the FixPending Issues that have a fix which has not yet been reviewed or submitted. label Jun 27, 2024
Mchnan pushed a commit to Mchnan/go-sylixos that referenced this issue Jul 9, 2024
This CL is similar to CL 562557, and it takes over CL 594175.

While here, unrelatedly remove mapKeys function, use slices.Sorted(maps.Keys(ms))
to simplify code.

Fixes golang#67657

Change-Id: Id8b99216f87a6dcfd6d5fa61407b515324c79112
Reviewed-on: https://go-review.googlesource.com/c/go/+/594737
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Jonathan Amsterdam <[email protected]>
Reviewed-by: Joedian Reid <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FixPending Issues that have a fix which has not yet been reviewed or submitted. NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants