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: does not handle semicolon-terminated cookie strings very well #52349

Closed
zjclyz opened this issue Apr 14, 2022 · 7 comments
Closed

net/http: does not handle semicolon-terminated cookie strings very well #52349

zjclyz opened this issue Apr 14, 2022 · 7 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@zjclyz
Copy link

zjclyz commented Apr 14, 2022

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

$ go version
1.16.3

Does this issue reproduce with the latest release?

I tried version 1.18 and it did not solve my problem completely.

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

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN="/Users/xxx/go/bin"
GOCACHE="/Users/xxx/Library/Caches/go-build"
GOENV="/Users/xxx/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/xxx/go/pkg/mod"
GONOPROXY="git.xx.oa.com"
GONOSUMDB="git.xx.oa.com"
GOOS="darwin"
GOPATH="/Users/xxx/go"
GOPRIVATE="git.xx.oa.com"
GOPROXY="https://goproxy.cn,direct"
GOROOT="/Users/xxx/go/go1.16.3"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/xxx/go/go1.16.3/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.16.3"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/xxx/develop/doc_pro1/xxx-xxx/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/gg/4vpkn5v518g_bdwnnq4w6zpm0000gn/T/go-build3887804640=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

The following code has reproduction problems:

import (
	"fmt"
	"net/http"
)

func main() {
	r := &http.Request{Header: map[string][]string{"Cookie": {"test1=1;"}}} // This test cookie string ends with a semicolon
	r.AddCookie(&http.Cookie{Name: "test2",Value: "2"}) // Append a cookie
	for _, c := range r.Cookies() { // Only test1 is printed, test2 is missing
		fmt.Println("cookie:", c.Name, c.Value)
	}
        fmt.Println(r.Header.Get("Cookie"))
}

What did you expect to see?

The actual printed results of the demo code:

cookie: test1 1
test1=1;; test2=2

The result I expect:

cookie: test1 1
cookie: test2 2
test1=1; test2=2

Why can't the http.Request.AddCookie() method remove the semicolon at the end and then append the cookie string?

@dmitshur dmitshur changed the title affected/package: http, http library does not handle semicolon-terminated cookie strings very well net/http: does not handle semicolon-terminated cookie strings very well Apr 14, 2022
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 14, 2022
@dmitshur dmitshur added this to the Backlog milestone Apr 14, 2022
@dmitshur
Copy link
Contributor

Thanks for the report. Do you know if the relevant specification says anything about this, or is this more related to nuances of the net/http API?

CC @neild.

@zjclyz
Copy link
Author

zjclyz commented Apr 19, 2022

Thanks for the report. Do you know if the relevant specification says anything about this, or is this more related to nuances of the net/http API?

CC @neild.

Sorry, I didn't find the specification either, it's just that we had a problem with it in our production environment, we were using version 1.16 and found that some of the cookie strings ended in a semicolon and then some of the cookies were lost after appending a new cookie. i looked at the http.Request.AddCookie() implementation and it doesn't start with Instead of removing the extra semicolon first, it goes on to add another one. Has the standard library considered correcting this beforehand to make it conform to the standard before adding a new cookie?

@ZekeLu
Copy link
Contributor

ZekeLu commented Apr 19, 2022

FYI, here is the spec for the Cookie header: https://datatracker.ietf.org/doc/html/rfc6265#section-4.2.1

cookie-header = "Cookie:" OWS cookie-string OWS
cookie-string = cookie-pair *( ";" SP cookie-pair )

And cookie-pair is described here: https://datatracker.ietf.org/doc/html/rfc6265#section-4.1

cookie-pair       = cookie-name "=" cookie-value
cookie-name       = token
cookie-value      = *cookie-octet / ( DQUOTE *cookie-octet DQUOTE )
cookie-octet      = %x21 / %x23-2B / %x2D-3A / %x3C-5B / %x5D-7E
                       ; US-ASCII characters excluding CTLs,
                       ; whitespace DQUOTE, comma, semicolon,
                       ; and backslash

According to the spec, semicolon should be excluded from the cookie-value (US-ASCII characters excluding CTLs, whitespace DQUOTE, comma, semicolon, and backslash). And there is not trailing semicolon in the cookie-string.

@zjclyz
Copy link
Author

zjclyz commented Apr 20, 2022

FYI, here is the spec for the Cookie header: https://datatracker.ietf.org/doc/html/rfc6265#section-4.2.1

cookie-header = "Cookie:" OWS cookie-string OWS
cookie-string = cookie-pair *( ";" SP cookie-pair )

And cookie-pair is described here: https://datatracker.ietf.org/doc/html/rfc6265#section-4.1

cookie-pair       = cookie-name "=" cookie-value
cookie-name       = token
cookie-value      = *cookie-octet / ( DQUOTE *cookie-octet DQUOTE )
cookie-octet      = %x21 / %x23-2B / %x2D-3A / %x3C-5B / %x5D-7E
                       ; US-ASCII characters excluding CTLs,
                       ; whitespace DQUOTE, comma, semicolon,
                       ; and backslash

According to the spec, semicolon should be excluded from the cookie-value (US-ASCII characters excluding CTLs, whitespace DQUOTE, comma, semicolon, and backslash). And there is not trailing semicolon in the cookie-string.

In theory, it should indeed be as specified in the draft, but our production environment did encounter a cookie string ending in a semicolon, which caused us problems; I was hoping that the standard library would be compatible with this situation, after all, go 1.16 would simply lose the newly appended cookie, and although subsequent versions have fixed this problem, they have never removed that extra semicolon.

@neild
Copy link
Contributor

neild commented Apr 20, 2022

Request.AddCookie is very simple in how it appends cookie values: If an existing Cookie header is present, it appends a semicolon and space before the new cookie value. This works correctly, so long as the existing Cookie header is valid.

It is not valid for a Cookie header to end in a semicolon. I don't think it makes sense for AddCookie to check for a specific form of invalid Cookie header. If it does make sense, then we need a cohesive understanding of what forms of invalid Cookie headers AddCookie will correct; would we also convert a=b;;b=c into a=b;b=c, for example? What do we do if the existing Cookie header is entirely unparsable?

Go 1.18's Request.Cookies function is more permissive about extraneous semicolons in cookies. If I run the example above with 1.18 (playground link), I see:

cookie: test1 1
cookie: test2 2
test1=1;; test2=2

@seankhliao
Copy link
Member

This should also be considered a dup of #38437

@zjclyz
Copy link
Author

zjclyz commented May 5, 2022

Ok, thanks for the replies!

@zjclyz zjclyz closed this as completed May 5, 2022
@golang golang locked and limited conversation to collaborators May 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge 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

6 participants