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: StripPrefix regression from fix for #30165 #31622

Open
twmb opened this issue Apr 23, 2019 · 3 comments

Comments

Projects
None yet
2 participants
@twmb
Copy link
Contributor

commented Apr 23, 2019

The fix for #30165 breaks code relying on the stripped prefix to elide a leading /. If code relies on equality testing of r.URL.Path after stripping a leading /path/, that code will break due to the new leading /.

I raise this only because I have a test that ensures a response cannot change; that test failed on tip.

What did you do?

https://play.golang.org/p/84ozdbcJ8dm

What did you expect to see?

=== RUN   TestPathStrip
--- PASS: TestPathStrip (0.00s)
PASS

What did you see instead?

--- FAIL: TestPathStrip (0.00s)
    junk_test.go:26: got "/foo" != exp "foo"
FAIL
exit status 1

Does this issue reproduce with the latest release (go1.12.4)?

No, tip only.

System details

go version devel +e40dffe55a Mon Apr 22 23:03:04 2019 +0000 linux/amd64
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/twmb/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/twmb/go"
GOPROXY=""
GORACE=""
GOROOT="/home/twmb/go/go"
GOTMPDIR=""
GOTOOLDIR="/home/twmb/go/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/twmb/testing/go.mod"
GOROOT/bin/go version: go version devel +e40dffe55a Mon Apr 22 23:03:04 2019 +0000 linux/amd64
GOROOT/bin/go tool compile -V: compile version devel +e40dffe55a Mon Apr 22 23:03:04 2019 +0000
uname -sr: Linux 4.15.0-47-generic
Distributor ID:	Ubuntu
Description:	Ubuntu 18.04.2 LTS
Release:	18.04
Codename:	bionic
gdb --version: GNU gdb (Ubuntu 8.1-0ubuntu3) 8.1.0.20180409-git

@dmitshur dmitshur added this to the Go1.13 milestone Apr 23, 2019

@dmitshur

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

I suspect this is an unfortunate but intended consequence of fixing #30165. Code relying on that should be updated not to rely on it. That is, instead of:

http.Handle("/foo/", http.StripPrefix("/foo/", h))

It makes more sense to me to be writing:

http.Handle("/foo/", http.StripPrefix("/foo", h))

That leads to a more consistent behavior. Handlers almost always receive absolute request paths, and trying to handle or support paths that don't begin with a "/" is often going to be problematic or impossible.

But I'll let Brad see if anything else can/should be done. /cc @bradfitz

@twmb

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

I agree it's a consequence, but it it is misleading to start with a cleaned path, strip a known prefix, and end up with a result where the full prefix was not stripped. Handlers that are coded to only be behind StripPrefix now have to also know that maybe a slash is not stripped, even if requested.

The original problem exists because the file server is relying on r.URL.Path to be non-empty; the fix should be to check that it is non-empty before indexing into the path. The original code should also likely just be using FileServer.

@dmitshur

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

I think the original snippet be simplified to https://play.golang.org/p/qIYFprNaibv.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.