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/cookie: readSetCookies does not strip whitespace for key/value pair received #40414

Open
jixunmoe opened this issue Jul 26, 2020 · 3 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@jixunmoe
Copy link

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

$ go version
go version go1.14.6 windows/amd64

Does this issue reproduce with the latest release?

Yes. I'm using the latest go release for Windows.

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

go env Output
$ go env
set GO111MODULE=on
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\-snip-\AppData\Local\go-build
set GOENV=C:\Users\-snip-\AppData\Roaming\go\env
set GOEXE=.exe
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Users\-snip-\go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=C:\Go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=C:\Go\pkg\tool\windows_amd64
set GCCGO=gccgo
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=-snip-
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=C:\Users\-snip-\AppData\Local\Temp\go-build817710326=/tmp/go-build -gno-record-gcc-switches

What did you do?

Use http.Client with cookie jar to access site with valid Set-Cookie header, but the cookie did not persist.

The first two requests:

GET http://192.168.2.1/html/footer.html HTTP/1.1
Host: 192.168.2.1
User-Agent: Go-http-client/1.1
Accept-Encoding: gzip

HTTP/1.1 200 OK
Date: Thu, 01 Jan 1970 00:00:00 GMT
Server: webserver
Connection: keep-alive
Keep-Alive: timeout=10, max=100
X-Download-Options: noopen
X-Frame-Options: deny
X-XSS-Protection: 1; mode=block
Strict-Transport-Security: max-age=31536000; includeSubdomains
Content-Length: 185
Content-Type: text/html
Expires: 0
Cache-Control: no-cache
Set-Cookie: SessionID=some_value;path =/;HttpOnly;

GET http://192.168.2.1/api/user/state-login HTTP/1.1
Host: 192.168.2.1
User-Agent: Go-http-client/1.1
Accept-Encoding: gzip

In the second request, the cookie did not persist. When looking at the cookie jar after the first request, the cookie has been stored under the path /html instead of expected /, which applies to the whole domain.

When I further examine the response header from the initial request, there's a white-space after the path key. This extra whitespace has caused readSetCookies to not recognise the key-pair, and the cookie has been stored relative to the document path (i.e. /html).

A quick read in RFC6265#5.2, step 4 states that the user-agent should strip leading or trailing whitespaces for the key/value string.

An attempt to fix this behaviour:

--- cookie.go   2020-07-16 22:30:44.000000000 +0100
+++ cookie_2.go 2020-07-26 11:33:31.710704300 +0100
@@ -92,8 +92,8 @@
                        if j := strings.Index(attr, "="); j >= 0 {
                                attr, val = attr[:j], attr[j+1:]
                        }
-                       lowerAttr := strings.ToLower(attr)
-                       val, ok = parseCookieValue(val, false)
+                       lowerAttr := strings.TrimSpace(strings.ToLower(attr))
+                       val, ok = parseCookieValue(strings.TrimSpace(val), false)
                        if !ok {
                                c.Unparsed = append(c.Unparsed, parts[i])
                                continue

What did you expect to see?

The cookie is carried with subsequent requests.

What did you see instead?

The cookie was not set in subsequent requests, due to path mismatch.

@jixunmoe
Copy link
Author

jixunmoe commented Jul 26, 2020

As a workaround, I've created an alternative cookie jar which forces the path to be /.

package cookiejar

import (
	"net/http"
	"net/http/cookiejar"
	"net/url"
)

type Jar struct {
	*cookiejar.Jar
}

func (j *Jar) SetCookies(u *url.URL, cookies []*http.Cookie) {
	for _, cookie := range cookies {
		cookie.Path = "/"
	}

	j.Jar.SetCookies(u, cookies)
}

func New(opts *cookiejar.Options) (*Jar, error) {
	var err error
	jar := &Jar{}
	jar.Jar, err = cookiejar.New(opts)
	return jar, err
}

@cagedmantis cagedmantis added this to the Backlog milestone Jul 31, 2020
@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 31, 2020
@cagedmantis
Copy link
Contributor

/cc @bradfitz

@vdobler
Copy link
Contributor

vdobler commented Aug 21, 2020

/cc @nigeltao

This seems like a real bug. The "Note" in RFC 6265 5.2 makes it pretty clear:

NOTE: The algorithm below is more permissive than the grammar in
Section 4.1. For example, the algorithm strips leading and trailing
whitespace from the cookie name and value (but maintains internal
whitespace), whereas the grammar in Section 4.1 forbids whitespace in
these positions. User agents use this algorithm so as to
interoperate with servers that do not follow the recommendations in
Section 4.

So while sending a Set-Cookie header of the form
Set-Cookie: SessionID=some_value;path =/;HttpOnly;
is a violation of RFC 6265 we probably should accept it.

@jixunmoe You write

with valid Set-Cookie header
The Set-Cookie header is technically not valid.

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

3 participants