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.Handler does not populate named path wildcards #69623

Open
chressie opened this issue Sep 25, 2024 · 12 comments
Open

net/http: ServeMux.Handler does not populate named path wildcards #69623

chressie opened this issue Sep 25, 2024 · 12 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@chressie
Copy link
Contributor

Go version

go version devel go1.24-b17a55d095 Tue Sep 24 23:20:50 2024 +0000 linux/amd64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/chressie/.cache/go-build'
GOENV='/home/chressie/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/chressie/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/chressie/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/home/chressie/src/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/home/chressie/src/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='devel go1.24-b17a55d095 Tue Sep 24 23:20:50 2024 +0000'
GODEBUG=''
GOTELEMETRY='on'
GOTELEMETRYDIR='/home/chressie/.config/go/telemetry'
GCCGO='/usr/bin/gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/home/chressie/src/go/src/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-build549655017=/tmp/go-build -gno-record-gcc-switches'

What did you do?

The http.ServeMux.Handler method currently discards patterns and matches that findHandler returns.

This behavior makes it impossible to use named path wildcards in http.Handler implementations that want to use an http.ServeMux as the underlying request multiplexer.

The following program shows a common pattern that demonstrates this:

package main

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

func main() {
	mux := http.NewServeMux()
	mux.HandleFunc("/{foo}", func(w http.ResponseWriter, r *http.Request) {
		fmt.Printf("handler1: {foo}==%q\n", r.PathValue("foo"))
	})
	mux.HandleFunc("/x/{bar}", func(w http.ResponseWriter, r *http.Request) {
		fmt.Printf("handler2: {bar}==%q\n", r.PathValue("bar"))
	})

	// Typical middleware that routes all requests to mux.
	http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
		h, _ := mux.Handler(r)
		h.ServeHTTP(w, r)
	})

	ln, _ := net.Listen("tcp", ":0")
	go (&http.Server{Handler: http.DefaultServeMux, Addr: ln.Addr().String()}).Serve(ln)
	http.Get("http://" + ln.Addr().String() + "/wildcard-foo")
	http.Get("http://" + ln.Addr().String() + "/x/wildcard-bar")
}

What did you see happen?

Executing above program yields

% go run t.go
handler1: {foo}==""
handler2: {bar}==""

The wildcards were not populated.

What did you expect to see?

I'd expect to see the wildcards populated.

% go run t.go
handler1: {foo}=="wildcard-foo"
handler2: {bar}=="wildcard-bar"
@chressie
Copy link
Contributor Author

I drafted a possible fix in https://go.dev/cl/615735.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/615735 mentions this issue: net/http: populate named path wildcards into Request in ServeMux.Handler

@seankhliao
Copy link
Member

cc @jba @neild

imo this is working as intended and finding a handler shouldn't modify the request.

@neild
Copy link
Contributor

neild commented Sep 25, 2024

It doesn't seem unreasonable for ServeMux.Handler to populate the request path. But on the other hand, it doesn't seem unreasonable to say that it's surprising for a lookup operation to modify the lookup key. So I don't know.

Unless I'm missing something, the example middleware can be written more simply using ServeMux.ServeHTTP, which will populate the path as desired:

http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
	mux.ServeHTTP(w, r)
})

@chressie
Copy link
Contributor Author

Thanks for your thoughts. If one thinks of ServeMux.Handler as a pure lookup function, then i agree that populating the request is surprising.

In my mind the bug is that the returned handler can no longer be used as a regular HTTP handler. That means it's no longer safe to call ServeHTTP on it, because the request doesn't carry the information it needs (at least when using patterns with wildcards).

The example middleware in the initial description was a bit too short to demonstrate this, so here's a slightly extended version that shows how to subtly hold it wrong:

http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
	h, _ := mux.Handler(r)
	if _, ok := h.(*internalAccessHandler); ok {
		h = ensureAuthenticationHandler{h}
	}
	h.ServeHTTP(w, r)
})

At a first glance the code looks fine, but assuming ensureAuthenticationHandler.ServeHTTP eventually calls h.ServeHTTP it would fail on patterns with wildcards. The fix is, as you suggest, to re-use the mux (e.g. ensureAuthenticationHandler{mux}), but that's very subtle.

I think it would be better to populate the request and avoid subtle bugs like the above.

@seankhliao
Copy link
Member

i still think Handler should be a pure lookup function, and it's up to the middleware to use the second return parameter to fill in the request, as is expected of any other third party router.

@dr2chase dr2chase added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 26, 2024
@dr2chase
Copy link
Contributor

unclear if this "needs investigation" or WAI.

@jba
Copy link
Contributor

jba commented Sep 27, 2024

Definitely "needs investigation." It's not clear to me what the implications are of populating the request in the presence of further mux nesting and http.StripPrefix. Also, to echo what @seankhliao said, there may be other uses of Handler that are just pure lookup, perhaps with variations on the request. The fundamental problem is that you're modifying a shared resource. Furthermore, so far in all examples, there is a workaround that is not too far from the problematic code (though perhaps a bit subtle).

@neild
Copy link
Contributor

neild commented Sep 27, 2024

Maybe the handler returned by mux.Handler should modify the request passed to it?

http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
	// mux.Handler does not modify r.
	h, _ := mux.Handler(r)

	// But h.ServeHTTP does.
	h.ServeHTTP(w, r)
})

@chressie
Copy link
Contributor Author

Wrapping the registered handler into another handler (which then modifies the request to populate the fields on demand) would break code that expects the returned handler to match the registered one (e.g. uses type assertion).

The options that i'm seeing right now:

  • Accept the current state and document that calling ServeHTTP doesn't work with patterns with wildcards and that the original mux should be used.
  • Populate the request.
    • A variant could be to just modify the request when there actually was a wildcard in the pattern and otherwise leave it as is.
  • Have an extra API that allows the request to be populated.

I don't find any of these particularly appealing. Maybe there are more options that i'm not seeing though.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/618096 mentions this issue: net/http: document ServeMux.Handler as a pure lookup function

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

7 participants