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 does not preserve url path #24366

Closed
cjellick opened this issue Mar 13, 2018 · 6 comments
Closed

net/http: StripPrefix does not preserve url path #24366

cjellick opened this issue Mar 13, 2018 · 6 comments
Assignees
Milestone

Comments

@cjellick
Copy link

@cjellick cjellick commented Mar 13, 2018

Please answer these questions before submitting your issue. Thanks!

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

go version go1.10 linux/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

$ go env
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"

What did you do?

The http.StripPrefix handler doesnt preserve the original path of the request. Specifically, it completely unescapes it, replacing %2F with /, for example.
It also causes url.Path, url.RawPath, and url.EncodedPath() to become inconsistent with each other.

You can reproduce with the following program:

package main

import (
	"fmt"
	"net/http"
)

type exampleHandler struct {}

func (h *exampleHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
	fmt.Fprintf(w, "url.Path: %v\n", req.URL.Path)
	fmt.Fprintf(w, "url.RawPath: %v\n", req.URL.RawPath)
	fmt.Fprintf(w, "url.EscapedPath(): %v\n", req.URL.EscapedPath())
}

func main() {
	sp := http.StripPrefix("/foo/", &exampleHandler{})

	// server on port 8888 uses StripPrefix
	server := &http.Server{
		Addr: ":8888",
		Handler: sp,
	}
	go server.ListenAndServe()

	// server on port 9999 does not use StripPrefix
	server2 := &http.Server{
		Addr: ":9999",
		Handler: &exampleHandler{},
	}
	server2.ListenAndServe()
}

What did you expect to see?

I expected to see

curl 'http://localhost:8888/foo/%2Fgoogle.com'
url.Path: %2Fgoogle.com
url.RawPath: %2Fgoogle.com
url.EscapedPath(): %2Fgoogle.com

What did you see instead?

This is the actual output:

curl 'http://localhost:8888/foo/%2Fgoogle.com'
url.Path: /google.com
url.RawPath: /foo/%2Fgoogle.com
url.EscapedPath(): /google.com

So to reiterate, I think EscapePath() should return a version of the path with all the original escaping and I think RawPath should be insync with Path and EscapedPath()

For a comparison, this is what the endpoint that doesnt use StripPrefix outputs:

curl 'http://localhost:9999/foo/%2Fgoogle.com'
url.Path: /foo//google.com
url.RawPath: /foo/%2Fgoogle.com
url.EscapedPath(): /foo/%2Fgoogle.com
@agnivade
Copy link
Contributor

@agnivade agnivade commented Mar 13, 2018

This has nothing to do with StripPrefix. The url parsing itself stores the path in its decoded form. As you can clearly see with the handler without the StripPrefix.

From the documentation -

Note that the Path field is stored in decoded form: /%47%6f%2f becomes /Go/. A consequence is that it is impossible to tell which slashes in the Path were slashes in the raw URL and which were %2f. This distinction is rarely important, but when it is, code must not use Path directly.

Regarding EscapedPath(), see -

EscapedPath returns the escaped form of u.Path. In general there are multiple possible escaped forms of any path. EscapedPath returns u.RawPath when it is a valid escaping of u.Path. Otherwise EscapedPath ignores u.RawPath and computes an escaped form on its own.

I don't see anything that needs change here. Everything works according to the documentation.

@cjellick
Copy link
Author

@cjellick cjellick commented Mar 13, 2018

In that case, url.EscapedPath() and by association url.String() should be considered unsafe to use if you use the StripPrefix handler and care about the original encoding because those methods will now never return the original encoding.

@agnivade
Copy link
Contributor

@agnivade agnivade commented Mar 13, 2018

Hmm .. on second thoughts I think EscapedPath() should return %2Fgoogle.com. I think it's a problem with %2F.

Because

curl 'http://localhost:8888/foo/%20google.com' gives - %20google.com
curl 'http://localhost:8888/foo/%21google.com' gives - %21google.com
curl 'http://localhost:8888/foo/%22google.com' gives - %22google.com

Whereas

curl 'http://localhost:8888/foo/%2Fgoogle.com' gives - /google.com.

Also, surprisingly, the RawPath vanishes for %20, %21, %22. I have just tested only these though.

/cc @bradfitz

@andybons andybons added this to the Go1.11 milestone Mar 13, 2018
@cjellick
Copy link
Author

@cjellick cjellick commented Mar 13, 2018

To be honest, maybe this will all be resolved with "worked as expected" because technically everything is behaving as documented. To me, the url's API around URL's Path, Rawpath, and EscapedPath() is just funky and confusing. There is a lot of code in the wild that manipulates request.URL.Path to make interesting things happen but all that I've seen does not touch u.RawPath, which causes it to be an invalid escaping of u.Path and thus to be ignored by u.EscapedPath() and subsequently u.String(). As you've pointed out, RawPath is sometimes set and it sometimes isn't. Sometimes u.EscapedPath() matches it, sometimes it doesn't. Again, the behavior all perfectly lines up with the documentation but it this all makes for a very confusing API that is difficult for devs to use correctly.

I'd also like to understand why u.Path stores the completely unescaped version of the path. I realize the documentation points that out, but by unescaping %2F, the meaning of the path is fundamentally changed, isn't it? I guess as the code is currently written, it is "developer error" to use reqeust.URL.Path for parsing and routing and they should be using either RawPath or EscapedPath() (provided some other code hasn't changed Path causing EscapedPath() to ignore RawPath). But it seems counter-intuitive to use u.RawPath over u.Path.

@dmitshur dmitshur changed the title http.StripPrefix does not preserve url path net/http: StripPrefix does not preserve url path Mar 20, 2018
@bradfitz bradfitz modified the milestones: Go1.11, Go1.12 Jul 9, 2018
@andybons andybons modified the milestones: Go1.12, Go1.13 Feb 12, 2019
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@gopherbot
Copy link

@gopherbot gopherbot commented May 13, 2020

Change https://golang.org/cl/233637 mentions this issue: net/http: handle RawPath in StripPrefix

@adg adg self-assigned this May 13, 2020
@cespare
Copy link
Contributor

@cespare cespare commented Aug 22, 2020

I stumbled upon this bug just now. @adg's fix looks good to me. Can an approver please take a look? It's a small CL.

@gopherbot gopherbot closed this in bb54a85 Aug 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants
You can’t perform that action at this time.