net/http: AddCookie fails to add a malformed cookie to a request

[mariusgrigaitis]

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

$ go version
go version go1.14.2 darwin/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=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/mgrigaitis/Library/Caches/go-build"
GOENV="/Users/mgrigaitis/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/mgrigaitis/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.14.2_1/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.14.2_1/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/xd/kmhy2qkd011g0cn7pf341c_w0000gn/T/go-build009702876=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

https://play.golang.org/p/yagiRaYwKRk
https://play.golang.org/p/UVW1Hh9O8Gc

What did you expect to see?

I would expect request.AddCookie("name", "value") followed by request.Cookie("name") to always return Cookie with name=value or at least Cookie with same key but previous value.

What did you see instead?

request.Cookie() does not see the Cookie.

Other considerations

Different programming languages / libraries behave differently in cookie parsing. If golang application is acting as "proxy" and adds cookies to the request, backend systems could parse cookies differently.

[katiehockman]

/cc @bradfitz @FiloSottile

[seankhliao]

the cookie delimiter (defined in RFC6265 Section 5.4.4.2) is "; " with a space, AddCookie works when the delimiter is properly used

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

[mariusgrigaitis]

the cookie delimiter (defined in RFC6265 Section 5.4.4.2) is "; " with a space, AddCookie works when the delimiter is properly used

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

Yes, ;; is not valid according to RFC.

However I would still expect AddCookie to work in this case by either

• adding cookie to "last valid place", before ;;
• appending as new Cookie: header (but comment on top of AddCookie states that it would not do so)
• "sanitizing" previous cookie value according to RFC and only then adding a new one.
• return an error in case cookie cannot be added (but that would mean a breaking change)

[seankhliao]

the code for AddCookie is very simplistic https://golang.org/src/net/http/request.go?s=15364:15402#L423
and the Cookie is added to the header https://play.golang.org/p/RqneuqIJ6cv
it's just req.Cookie() stops parsing after the first invalid cookie

the same RFC also says there must only be 1 cookie header

[mariusgrigaitis]

Why does request.Cookie() parse all Cookie: headers instead of one? This is not according to RFC? Feel free to report it as separate issue.

[odeke-em]

Kindly /ccing @vdobler, in addition to what @seankhliao recommends.

[vdobler]

The issue title is misleading, the problem is not in net/http.Request.AddCookie not adding the cookie to the request. AddCookie works properly.

The problem stems from mixing manual (via Request.Header) and automatic (via Request.AddCookie) cookie management using malformed manual cookies.

Unfortunately there is not much which can be done here. AddCookie can be used to properly add an arbitrary number of cookies but nothing prevents the user from manually adding extra Cookie entries or adding or changing existing entries. If this manual action is done properly net/http.Request.Cookies will properly parse the Cookie header. This behaviour is analog to how net/http.Request.Cookies works for server requests: If the client sends a malformed Cookie header it won't be able to parse that header.

mariusgrigaitis seems to understand that the problem is manual messing up the header and suggests a few things to do (different order):

• return an error in case cookie cannot be added (but that would mean a breaking change)

  We do not do breaking changes.

• appending as new Cookie: header (but comment on top of AddCookie states that it would not do so)

  This might be a breaking change and would violate RFC 6265. Things we try hard not to do.

• "sanitizing" previous cookie value according to RFC and only then adding a new one.

  There is a reason why we allow to manually set a the Cookie header value: Sometimes you must send a malformed Cookie header because the server requires it. A typical example is illegal characters in the cookie-value. There is no need to "sanitize" cookies added via AddCookie (as these are valid) and the reason to set the Cookie header manually is to use a "unsanitized" cookie. It can be done but this needs to be documented, something like "If you manually set a malformed Cookie header a later AddCookie might clean up or remove that header value". It boils down to "You cannot mix malformed manual-cookies and wellformed AddCookie-cookies." (See below).

• adding cookie to "last valid place", before ;;

  This could be done. AddCookie would have to parse the Cookie header, find where the header goes bogus, add the new cookie there and append the bogus rest. This would have to be documented. It boils down to "You can mix malformed manual-cookies and wellformed AddCookie-cookies: AddCookies will be sorted in front of the malformed manual ones."

So all "solutions" require adding (complicated) documentation which explains the various quirks if you mix malformed manual-cookies and wellformed AddCookie-cookies.

I think the underlying problem is the assumption you could safely mix malformed manual-cookies ("MMC") and wellformed AddCookie-cookies ("WAC"): You cannot. This should be documented. But it doesn't require any code change. The problem is not that you cannot mix MMC and WAC, the problem is the expectation that you could.

You can mix wellformed manual cookies and AddCookie cookies but that is almost a no-brianer.

There are valid use cases for manual fiddling with the Cookie header and setting strange values and there are valid uses for AddCookie. Both ways of setting cookies are provided and you are encouraged to use AddCookie only. But if your back is to the wall you can manually set Cookie headers in which case you must take great care: You can mix manual setting Cookies and AddCookies but results depend on what you do manually.

[vdobler]

@mariusgrigaitis

Why does request.Cookie() parse all Cookie: headers instead of one? This is not according to RFC? Feel free to report it as separate issue.

net/http tries to be strict what it sends but halfway liberal on what it accepts as input. RFC 6265 forbids sending multiple Cookie headers but doesn't disallow parsing several Cookie headers.

[odeke-em]

Thank you @vdobler for the analysis, and thank you too @seankhliao and @mariusgrigaitis!
Going by your recommendations, might you perhaps have some bandwidth @vdobler to send a documentation update for this issue to fix it? Thank you.

[gopherbot]

Change https://golang.org/cl/235141 mentions this issue: net/http: clarify that AddCookie appends to whatever the Cookie header contains