net/http: Client should scope cookie to Request.Host before Request.URL

colinclerk commented 4 years ago

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

$ go version
go version go1.14.2 linux/amd64

Does this issue reproduce with the latest release?

Yes.

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

go env Output

$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/root/.cache/go-build"
GOENV="/root/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
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-build866802505=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Created a httptest.Server with a handler that sets a cookie, then issued a request from a http.Client with a custom Host field.

https://play.golang.org/p/78mh9ZwYI_t

What did you expect to see?

I expected to see the cookie scoped to the Host field.

What did you see instead?

The cookie was scoped to httptest.Server.URL

I was seeing unexpected cookie behavior while testing and I believe this is the root of it: https://github.com/golang/go/blob/57e32c4fbd4f20d567d1767dfc2d94bec828a8dc/src/net/http/client.go#L170-L186

Instead of the client always using req.URL for the cookie jar, I believe it should first look at req.Host, then req.Header.Get("Host"), and then finally req.URL.

I think this would be consistent with the behavior of Request.Write described on the Request struct: https://github.com/golang/go/blob/57e32c4fbd4f20d567d1767dfc2d94bec828a8dc/src/net/http/request.go#L231-L235

toothrot commented 4 years ago

/cc @vdobler @bradfitz

vdobler commented 4 years ago

@colinclerk Thanks for this bug report. There might be a real issue here.

From https://tools.ietf.org/html/rfc6265#section-2.3

The request-host is the name of the host, as known by the user agent, to which the user agent is sending an HTTP request or from which it is receiving an HTTP response (i.e., the name of the host to which it sent the corresponding HTTP request).

The term request-uri is defined in Section 5.1.2 of [RFC2616].

According to https://tools.ietf.org/html/rfc6265#section-5.4 domain matching works on the request-host (with the unclear definition above). The request-uri consists of the Host header and the abs_path, and the request-host sounds more like the r.URL.Host.

On the other hand: curl seems to use the Host header....

colinclerk commented 4 years ago

@vdobler What is the intended purpose of allowing outbound requests where r.Host != r.URL.Host?

When I issue outbound requests where r.Host != r.URL.Host, my intent is to mimic DNS resolution:

r.Host is what my user has in their address bar
r.URL.Host is where the DNS for r.Host resolves

Having this separation allows me to mimic a server that has multiple hosts pointed at it, and generate different responses depending on the Host.

Since I'm imagining r.Host is what my user has in their address bar, it's also where I'm expecting cookies to be set. But maybe I'm thinking about this all wrong?

Another exercise that might be helpful is to think about things from the server's perspective. r.URL.Host doesn't exist on the server, so when it issues SetCookie it expects the cookie to be set on the incoming request's r.Host.

That's not what happens when using Client and setting r.Host manually: https://play.golang.org/p/NJIuiMhPP55

chrisguiney commented 1 year ago

I just ran into this issue. It seems pretty clear to me that a new *url.URL using r.Host should be used to set/get cookies.

Say my application does routing based on subdomain, and I'm testing with httptest.Server:

httptest.Server creates some url like http://localhost:21345.

I want any request to connect to localhost:21345, so to accomplish that I set r.URL = server.URL()

I still need to let the server know what domain is being requested, and so I set r.Host = "customer1.example.com"

The request is issued, and everything looks like it works fine. Except cookies are written with domain=.localhost instead of .customer1.example.com. This still appears to work for subsequent requests to customer1.exmaple.com.

When doing a subsequent request for customer2.example.com however, http.Client still loads cookies from localhost, and so all the cookies intended to be scoped to customer1.example.com are now being sent to customer2.example.com.

Currently the only workaround is to maintain your own cookiejar, and load/store the cookies before and after the request manually.

Is this something the net/http maintainers would welcome a patch for? What would fixing the behavior mean for the compatibility promise? It seems to me that leaking data from one domain to another is a clear security concern. It doesn't seem like this is something that would be pervasive in a production application.

golang / go