From a63907808d14679c723e566cb83acc76fc8cafc2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nuno=20Gon=C3=A7alves?= Date: Thu, 18 Apr 2024 19:30:26 +0000 Subject: [PATCH] net/http: add field Cookie.Quoted bool The current implementation of the http package strips double quotes from the cookie-value during parsing, resulting in the serialized cookie not including them. This patch addresses this limitation by introducing a new field to track whether the original value was enclosed in quotes. Additionally, the internal representation of a cookie in the cookiejar package has been adjusted to align with the new representation. The syntax of cookies is outlined in RFC 6265 Section 4.1.1: https://datatracker.ietf.org/doc/html/rfc6265\#section-4.1.1 Fixes #46443 Change-Id: Iac12a56397d77a6060a75757ab0daeacc60457f3 GitHub-Last-Rev: a76440e741440cddaa05944b6828a14a32b5a44a GitHub-Pull-Request: golang/go#66752 Reviewed-on: https://go-review.googlesource.com/c/go/+/577755 Reviewed-by: Damien Neil LUCI-TryBot-Result: Go LUCI Reviewed-by: Cherry Mui --- api/next/46443.txt | 1 + doc/next/6-stdlib/99-minor/net/http/46443.md | 3 + src/net/http/cookie.go | 51 ++++++----- src/net/http/cookie_test.go | 90 +++++++++++++------- src/net/http/cookiejar/jar.go | 4 +- src/net/http/cookiejar/jar_test.go | 26 +++++- src/net/http/request.go | 2 +- 7 files changed, 124 insertions(+), 53 deletions(-) create mode 100644 api/next/46443.txt create mode 100644 doc/next/6-stdlib/99-minor/net/http/46443.md diff --git a/api/next/46443.txt b/api/next/46443.txt new file mode 100644 index 0000000000000..a4e6fc4d4be2b --- /dev/null +++ b/api/next/46443.txt @@ -0,0 +1 @@ +pkg net/http, type Cookie struct, Quoted bool #46443 diff --git a/doc/next/6-stdlib/99-minor/net/http/46443.md b/doc/next/6-stdlib/99-minor/net/http/46443.md new file mode 100644 index 0000000000000..730582008adc2 --- /dev/null +++ b/doc/next/6-stdlib/99-minor/net/http/46443.md @@ -0,0 +1,3 @@ +[`Cookie`](/pkg/net/http#Cookie) now preserves double quotes surrounding +a cookie value. The new `Cookie.Quoted` field indicates whether the +`Cookie.Value` was originally quoted. diff --git a/src/net/http/cookie.go b/src/net/http/cookie.go index ab84625ba0bdb..2a8170709b487 100644 --- a/src/net/http/cookie.go +++ b/src/net/http/cookie.go @@ -21,8 +21,9 @@ import ( // // See https://tools.ietf.org/html/rfc6265 for details. type Cookie struct { - Name string - Value string + Name string + Value string + Quoted bool // indicates whether the Value was originally quoted Path string // optional Domain string // optional @@ -80,11 +81,11 @@ func ParseCookie(line string) ([]*Cookie, error) { if !isCookieNameValid(name) { return nil, errInvalidCookieName } - value, found = parseCookieValue(value, true) + value, quoted, found := parseCookieValue(value, true) if !found { return nil, errInvalidCookieValue } - cookies = append(cookies, &Cookie{Name: name, Value: value}) + cookies = append(cookies, &Cookie{Name: name, Value: value, Quoted: quoted}) } return cookies, nil } @@ -105,14 +106,15 @@ func ParseSetCookie(line string) (*Cookie, error) { if !isCookieNameValid(name) { return nil, errInvalidCookieName } - value, ok = parseCookieValue(value, true) + value, quoted, ok := parseCookieValue(value, true) if !ok { return nil, errInvalidCookieValue } c := &Cookie{ - Name: name, - Value: value, - Raw: line, + Name: name, + Value: value, + Quoted: quoted, + Raw: line, } for i := 1; i < len(parts); i++ { parts[i] = textproto.TrimString(parts[i]) @@ -125,7 +127,7 @@ func ParseSetCookie(line string) (*Cookie, error) { if !isASCII { continue } - val, ok = parseCookieValue(val, false) + val, _, ok = parseCookieValue(val, false) if !ok { c.Unparsed = append(c.Unparsed, parts[i]) continue @@ -229,7 +231,7 @@ func (c *Cookie) String() string { b.Grow(len(c.Name) + len(c.Value) + len(c.Domain) + len(c.Path) + extraCookieLength) b.WriteString(c.Name) b.WriteRune('=') - b.WriteString(sanitizeCookieValue(c.Value)) + b.WriteString(sanitizeCookieValue(c.Value, c.Quoted)) if len(c.Path) > 0 { b.WriteString("; Path=") @@ -341,11 +343,11 @@ func readCookies(h Header, filter string) []*Cookie { if filter != "" && filter != name { continue } - val, ok := parseCookieValue(val, true) + val, quoted, ok := parseCookieValue(val, true) if !ok { continue } - cookies = append(cookies, &Cookie{Name: name, Value: val}) + cookies = append(cookies, &Cookie{Name: name, Value: val, Quoted: quoted}) } } return cookies @@ -430,6 +432,8 @@ func sanitizeCookieName(n string) string { } // sanitizeCookieValue produces a suitable cookie-value from v. +// It receives a quoted bool indicating whether the value was originally +// quoted. // https://tools.ietf.org/html/rfc6265#section-4.1.1 // // cookie-value = *cookie-octet / ( DQUOTE *cookie-octet DQUOTE ) @@ -439,15 +443,14 @@ func sanitizeCookieName(n string) string { // ; and backslash // // We loosen this as spaces and commas are common in cookie values -// but we produce a quoted cookie-value if and only if v contains -// commas or spaces. +// thus we produce a quoted cookie-value if v contains commas or spaces. // See https://golang.org/issue/7243 for the discussion. -func sanitizeCookieValue(v string) string { +func sanitizeCookieValue(v string, quoted bool) string { v = sanitizeOrWarn("Cookie.Value", validCookieValueByte, v) if len(v) == 0 { return v } - if strings.ContainsAny(v, " ,") { + if strings.ContainsAny(v, " ,") || quoted { return `"` + v + `"` } return v @@ -489,17 +492,27 @@ func sanitizeOrWarn(fieldName string, valid func(byte) bool, v string) string { return string(buf) } -func parseCookieValue(raw string, allowDoubleQuote bool) (string, bool) { +// parseCookieValue parses a cookie value according to RFC 6265. +// If allowDoubleQuote is true, parseCookieValue will consider that it +// is parsing the cookie-value; +// otherwise, it will consider that it is parsing a cookie-av value +// (cookie attribute-value). +// +// It returns the parsed cookie value, a boolean indicating whether the +// parsing was successful, and a boolean indicating whether the parsed +// value was enclosed in double quotes. +func parseCookieValue(raw string, allowDoubleQuote bool) (value string, quoted, ok bool) { // Strip the quotes, if present. if allowDoubleQuote && len(raw) > 1 && raw[0] == '"' && raw[len(raw)-1] == '"' { raw = raw[1 : len(raw)-1] + quoted = true } for i := 0; i < len(raw); i++ { if !validCookieValueByte(raw[i]) { - return "", false + return "", quoted, false } } - return raw, true + return raw, quoted, true } func isCookieNameValid(raw string) bool { diff --git a/src/net/http/cookie_test.go b/src/net/http/cookie_test.go index ce5093c2ea4fa..de476825cf089 100644 --- a/src/net/http/cookie_test.go +++ b/src/net/http/cookie_test.go @@ -147,6 +147,19 @@ var writeSetCookiesTests = []struct { &Cookie{Name: "a\rb", Value: "v"}, ``, }, + // Quoted values (issue #46443) + { + &Cookie{Name: "cookie", Value: "quoted", Quoted: true}, + `cookie="quoted"`, + }, + { + &Cookie{Name: "cookie", Value: "quoted with spaces", Quoted: true}, + `cookie="quoted with spaces"`, + }, + { + &Cookie{Name: "cookie", Value: "quoted,with,commas", Quoted: true}, + `cookie="quoted,with,commas"`, + }, } func TestWriteSetCookies(t *testing.T) { @@ -215,6 +228,15 @@ var addCookieTests = []struct { }, "cookie-1=v$1; cookie-2=v$2; cookie-3=v$3", }, + // Quoted values (issue #46443) + { + []*Cookie{ + {Name: "cookie-1", Value: "quoted", Quoted: true}, + {Name: "cookie-2", Value: "quoted with spaces", Quoted: true}, + {Name: "cookie-3", Value: "quoted,with,commas", Quoted: true}, + }, + `cookie-1="quoted"; cookie-2="quoted with spaces"; cookie-3="quoted,with,commas"`, + }, } func TestAddCookie(t *testing.T) { @@ -326,15 +348,15 @@ var readSetCookiesTests = []struct { }, { Header{"Set-Cookie": {`special-2=" z"`}}, - []*Cookie{{Name: "special-2", Value: " z", Raw: `special-2=" z"`}}, + []*Cookie{{Name: "special-2", Value: " z", Quoted: true, Raw: `special-2=" z"`}}, }, { Header{"Set-Cookie": {`special-3="a "`}}, - []*Cookie{{Name: "special-3", Value: "a ", Raw: `special-3="a "`}}, + []*Cookie{{Name: "special-3", Value: "a ", Quoted: true, Raw: `special-3="a "`}}, }, { Header{"Set-Cookie": {`special-4=" "`}}, - []*Cookie{{Name: "special-4", Value: " ", Raw: `special-4=" "`}}, + []*Cookie{{Name: "special-4", Value: " ", Quoted: true, Raw: `special-4=" "`}}, }, { Header{"Set-Cookie": {`special-5=a,z`}}, @@ -342,7 +364,7 @@ var readSetCookiesTests = []struct { }, { Header{"Set-Cookie": {`special-6=",z"`}}, - []*Cookie{{Name: "special-6", Value: ",z", Raw: `special-6=",z"`}}, + []*Cookie{{Name: "special-6", Value: ",z", Quoted: true, Raw: `special-6=",z"`}}, }, { Header{"Set-Cookie": {`special-7=a,`}}, @@ -350,13 +372,18 @@ var readSetCookiesTests = []struct { }, { Header{"Set-Cookie": {`special-8=","`}}, - []*Cookie{{Name: "special-8", Value: ",", Raw: `special-8=","`}}, + []*Cookie{{Name: "special-8", Value: ",", Quoted: true, Raw: `special-8=","`}}, }, // Make sure we can properly read back the Set-Cookie headers // for names containing spaces: { Header{"Set-Cookie": {`special-9 =","`}}, - []*Cookie{{Name: "special-9", Value: ",", Raw: `special-9 =","`}}, + []*Cookie{{Name: "special-9", Value: ",", Quoted: true, Raw: `special-9 =","`}}, + }, + // Quoted values (issue #46443) + { + Header{"Set-Cookie": {`cookie="quoted"`}}, + []*Cookie{{Name: "cookie", Value: "quoted", Quoted: true, Raw: `cookie="quoted"`}}, }, // TODO(bradfitz): users have reported seeing this in the @@ -425,15 +452,15 @@ var readCookiesTests = []struct { Header{"Cookie": {`Cookie-1="v$1"; c2="v2"`}}, "", []*Cookie{ - {Name: "Cookie-1", Value: "v$1"}, - {Name: "c2", Value: "v2"}, + {Name: "Cookie-1", Value: "v$1", Quoted: true}, + {Name: "c2", Value: "v2", Quoted: true}, }, }, { Header{"Cookie": {`Cookie-1="v$1"; c2=v2;`}}, "", []*Cookie{ - {Name: "Cookie-1", Value: "v$1"}, + {Name: "Cookie-1", Value: "v$1", Quoted: true}, {Name: "c2", Value: "v2"}, }, }, @@ -486,23 +513,26 @@ func TestCookieSanitizeValue(t *testing.T) { log.SetOutput(&logbuf) tests := []struct { - in, want string + in string + quoted bool + want string }{ - {"foo", "foo"}, - {"foo;bar", "foobar"}, - {"foo\\bar", "foobar"}, - {"foo\"bar", "foobar"}, - {"\x00\x7e\x7f\x80", "\x7e"}, - {`"withquotes"`, "withquotes"}, - {"a z", `"a z"`}, - {" z", `" z"`}, - {"a ", `"a "`}, - {"a,z", `"a,z"`}, - {",z", `",z"`}, - {"a,", `"a,"`}, + {"foo", false, "foo"}, + {"foo;bar", false, "foobar"}, + {"foo\\bar", false, "foobar"}, + {"foo\"bar", false, "foobar"}, + {"\x00\x7e\x7f\x80", false, "\x7e"}, + {`withquotes`, true, `"withquotes"`}, + {`"withquotes"`, true, `"withquotes"`}, // double quotes are not valid octets + {"a z", false, `"a z"`}, + {" z", false, `" z"`}, + {"a ", false, `"a "`}, + {"a,z", false, `"a,z"`}, + {",z", false, `",z"`}, + {"a,", false, `"a,"`}, } for _, tt := range tests { - if got := sanitizeCookieValue(tt.in); got != tt.want { + if got := sanitizeCookieValue(tt.in, tt.quoted); got != tt.want { t.Errorf("sanitizeCookieValue(%q) = %q; want %q", tt.in, got, tt.want) } } @@ -668,7 +698,7 @@ func TestParseCookie(t *testing.T) { }, { line: `Cookie-1="v$1";c2="v2"`, - cookies: []*Cookie{{Name: "Cookie-1", Value: "v$1"}, {Name: "c2", Value: "v2"}}, + cookies: []*Cookie{{Name: "Cookie-1", Value: "v$1", Quoted: true}, {Name: "c2", Value: "v2", Quoted: true}}, }, { line: "k1=", @@ -800,15 +830,15 @@ func TestParseSetCookie(t *testing.T) { }, { line: `special-2=" z"`, - cookie: &Cookie{Name: "special-2", Value: " z", Raw: `special-2=" z"`}, + cookie: &Cookie{Name: "special-2", Value: " z", Quoted: true, Raw: `special-2=" z"`}, }, { line: `special-3="a "`, - cookie: &Cookie{Name: "special-3", Value: "a ", Raw: `special-3="a "`}, + cookie: &Cookie{Name: "special-3", Value: "a ", Quoted: true, Raw: `special-3="a "`}, }, { line: `special-4=" "`, - cookie: &Cookie{Name: "special-4", Value: " ", Raw: `special-4=" "`}, + cookie: &Cookie{Name: "special-4", Value: " ", Quoted: true, Raw: `special-4=" "`}, }, { line: `special-5=a,z`, @@ -816,7 +846,7 @@ func TestParseSetCookie(t *testing.T) { }, { line: `special-6=",z"`, - cookie: &Cookie{Name: "special-6", Value: ",z", Raw: `special-6=",z"`}, + cookie: &Cookie{Name: "special-6", Value: ",z", Quoted: true, Raw: `special-6=",z"`}, }, { line: `special-7=a,`, @@ -824,13 +854,13 @@ func TestParseSetCookie(t *testing.T) { }, { line: `special-8=","`, - cookie: &Cookie{Name: "special-8", Value: ",", Raw: `special-8=","`}, + cookie: &Cookie{Name: "special-8", Value: ",", Quoted: true, Raw: `special-8=","`}, }, // Make sure we can properly read back the Set-Cookie headers // for names containing spaces: { line: `special-9 =","`, - cookie: &Cookie{Name: "special-9", Value: ",", Raw: `special-9 =","`}, + cookie: &Cookie{Name: "special-9", Value: ",", Quoted: true, Raw: `special-9 =","`}, }, { line: "", diff --git a/src/net/http/cookiejar/jar.go b/src/net/http/cookiejar/jar.go index e7f5ddd4d0096..280f4650c1815 100644 --- a/src/net/http/cookiejar/jar.go +++ b/src/net/http/cookiejar/jar.go @@ -92,6 +92,7 @@ func New(o *Options) (*Jar, error) { type entry struct { Name string Value string + Quoted bool Domain string Path string SameSite string @@ -220,7 +221,7 @@ func (j *Jar) cookies(u *url.URL, now time.Time) (cookies []*http.Cookie) { return s[i].seqNum < s[j].seqNum }) for _, e := range selected { - cookies = append(cookies, &http.Cookie{Name: e.Name, Value: e.Value}) + cookies = append(cookies, &http.Cookie{Name: e.Name, Value: e.Value, Quoted: e.Quoted}) } return cookies @@ -429,6 +430,7 @@ func (j *Jar) newEntry(c *http.Cookie, now time.Time, defPath, host string) (e e } e.Value = c.Value + e.Quoted = c.Quoted e.Secure = c.Secure e.HttpOnly = c.HttpOnly diff --git a/src/net/http/cookiejar/jar_test.go b/src/net/http/cookiejar/jar_test.go index 251f7c16171c3..93b351889f13c 100644 --- a/src/net/http/cookiejar/jar_test.go +++ b/src/net/http/cookiejar/jar_test.go @@ -404,7 +404,12 @@ func (test jarTest) run(t *testing.T, jar *Jar) { if !cookie.Expires.After(now) { continue } - cs = append(cs, cookie.Name+"="+cookie.Value) + + v := cookie.Value + if strings.ContainsAny(v, " ,") || cookie.Quoted { + v = `"` + v + `"` + } + cs = append(cs, cookie.Name+"="+v) } } sort.Strings(cs) @@ -421,7 +426,7 @@ func (test jarTest) run(t *testing.T, jar *Jar) { now = now.Add(1001 * time.Millisecond) var s []string for _, c := range jar.cookies(mustParseURL(query.toURL), now) { - s = append(s, c.Name+"="+c.Value) + s = append(s, c.String()) } if got := strings.Join(s, " "); got != query.want { t.Errorf("Test %q #%d\ngot %q\nwant %q", test.description, i, got, query.want) @@ -639,6 +644,23 @@ var basicsTests = [...]jarTest{ {"https://[::1%25.example.com]:80/", ""}, }, }, + { + "Retrieval of cookies with quoted values", // issue #46443 + "http://www.host.test/", + []string{ + `cookie-1="quoted"`, + `cookie-2="quoted with spaces"`, + `cookie-3="quoted,with,commas"`, + `cookie-4= ,`, + }, + `cookie-1="quoted" cookie-2="quoted with spaces" cookie-3="quoted,with,commas" cookie-4=" ,"`, + []query{ + { + "http://www.host.test", + `cookie-1="quoted" cookie-2="quoted with spaces" cookie-3="quoted,with,commas" cookie-4=" ,"`, + }, + }, + }, } func TestBasics(t *testing.T) { diff --git a/src/net/http/request.go b/src/net/http/request.go index 345ba3d4eb43c..bdd18adf3f3a9 100644 --- a/src/net/http/request.go +++ b/src/net/http/request.go @@ -464,7 +464,7 @@ func (r *Request) Cookie(name string) (*Cookie, error) { // AddCookie only sanitizes c's name and value, and does not sanitize // a Cookie header already present in the request. func (r *Request) AddCookie(c *Cookie) { - s := fmt.Sprintf("%s=%s", sanitizeCookieName(c.Name), sanitizeCookieValue(c.Value)) + s := fmt.Sprintf("%s=%s", sanitizeCookieName(c.Name), sanitizeCookieValue(c.Value, c.Quoted)) if c := r.Header.Get("Cookie"); c != "" { r.Header.Set("Cookie", c+"; "+s) } else {