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: double escape of URL #25208

Closed
djgilcrease opened this issue May 1, 2018 · 6 comments
Closed

net/http: double escape of URL #25208

djgilcrease opened this issue May 1, 2018 · 6 comments

Comments

@djgilcrease
Copy link

@djgilcrease djgilcrease commented May 1, 2018

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

$ go version
go version go1.10.1 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=""
GOCACHE="/home/digitalxero/.cache/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/opt/projects/f5/go"
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build355493993=/tmp/go-build -gno-record-gcc-switches"

What did you do?

url := "http://some.example.com/foo%2Fbar"
req, _ := http.NewRequest("POST", url, body)
resp, _ := client.Do(req)
fmt.Println(resp.Request.RequestURI)
# 

What did you expect to see?

"http://some.example.com/foo%2Fbar"

What did you see instead?

"http://some.example.com/foo%252Fbar"

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 1, 2018

"foo%252Fbar" is of course the escaped form of "foo%2Fbar".

This is a question probably better directed to a forum rather than to the issue tracker. See https://golang.org/wiki/Questions . Thanks.

@djgilcrease

This comment has been minimized.

Copy link
Author

@djgilcrease djgilcrease commented May 1, 2018

This is a bug because foo%2Fbar is a valid path as is and does not need to be escaped again. It even passes the internal validEncodedPath check in url.go, but because it does a secondary check of unescaping the path and comparing it to the valid escaped path you get a double escaped path when you should not

@ianlancetaylor ianlancetaylor changed the title EscapedPath logic seems odd net/http: double escape of URL May 2, 2018
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 2, 2018

Can you show us a complete program demonstrating the bug? Thanks.

CC @bradfitz

@ianlancetaylor ianlancetaylor reopened this May 2, 2018
@ianlancetaylor ianlancetaylor added this to the Go1.11 milestone May 2, 2018
@djgilcrease

This comment has been minimized.

Copy link
Author

@djgilcrease djgilcrease commented May 2, 2018

https://play.golang.org/p/DlQWHrX1cuD

package main

import (
	"fmt"
	"net/url"
)

func main() {
	expected := "https://httpbin.org/anything/foo%2Fbar/bla"
	good, _ := url.Parse(fmt.Sprintf("https://httpbin.org/anything/%s/bla", url.PathEscape("foo/bar")))
	bad, _ := url.Parse("https://httpbin.org")
	bad.Path = fmt.Sprintf("anything/%s/bla", url.PathEscape("foo/bar"))
	alsoBad, _ := url.Parse("https://httpbin.org")
	alsoBad.RawPath = fmt.Sprintf("anything/%s/bla", url.PathEscape("foo/bar"))

	workAround, _ := url.Parse("https://httpbin.org")
	workAround.Path = fmt.Sprintf("anything/%s/bla", url.PathEscape("foo/bar"))
	workAround.RawPath = workAround.Path
	workAround.Path, _ = url.PathUnescape(workAround.Path)

	if expected != good.String() {
		fmt.Printf("GOOD: %s != %s", expected, good.String())
	}

	if expected != bad.String() {
		fmt.Printf("BAD: %s != %s\n", expected, bad.String())
	}

	if expected != alsoBad.String() {
		fmt.Printf("ALSO BAD: %s != %s\n", expected, alsoBad.String())
	}

	if expected != workAround.String() {
		fmt.Printf("Worked Around: %s != %s", expected, workAround.String())
	}
}
@crvv

This comment has been minimized.

Copy link
Contributor

@crvv crvv commented May 9, 2018

I think the example doesn't show any bug.
The document(https://golang.org/pkg/net/url/#URL) says

the Path field is stored in decoded form: /%47%6f%2f becomes /Go/

If url.Path is /foo%2Fbar/bla, its encoded form is /foo%252Fbar/bla.

And the "workAround" is the right way to do this.
It is documented clearly at https://golang.org/pkg/net/url/#URL.EscapedPath

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Jun 5, 2018

The first example shows fmt.Println(resp.Request.RequestURI) but that Request.RequestURI field is documented as:

        // RequestURI is the unmodified Request-URI of the
        // Request-Line (RFC 2616, Section 5.1) as sent by the client
        // to a server. Usually the URL field should be used instead.
        // It is an error to set this field in an HTTP client request.
        RequestURI string

Note that last sentence: "It is an error to set this field in an HTTP client request".

And indeed, the net/http code returns an error if it's set.

I don't see how that's possible. The sample code as provided seems like it would never happen or wouldn't return the results described.

Then the follow-up code shows completely unrelated (and working as expected) code that doesn't involve the RequestURI field.

I'm going to close this. If you have a better example of the bug, please post and we can reopen.

@bradfitz bradfitz closed this Jun 5, 2018
@golang golang locked and limited conversation to collaborators Jun 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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