Issue 86050045: code review 86050045: net/http: allow commas and spaces in cookie values

Issue 86050045: code review 86050045: net/http: allow commas and spaces in cookie values (Closed)

Can't Edit
Can't Publish+Mail
Start Review

Created:
10 years ago by volker.dobler

Modified:
10 years ago

Reviewers:
nigeltao

CC:
nigeltao, bradfitz, golang-codereviews

Visibility:
Public.

Description

net/http: allow commas and spaces in cookie values According to RFC 6265 a cookie value may contain neither commas nor spaces but such values are very common in the wild and browsers handle them very well so we'll allow both commas and spaces. Values starting or ending in a comma or a space are sent in the quoted form to prevent missinterpetations. RFC 6265 conforming values are handled as before and semicolons, backslashes and double-quotes are still disallowed. Fixes issue 7243

Patch Set 1 #

Patch Set 2 : diff -r 1eaf29392348 https://code.google.com/p/go/ #

Patch Set 3 : diff -r 07e31caba5b6 https://code.google.com/p/go/ #

Patch Set 4 : diff -r 07e31caba5b6 https://code.google.com/p/go/ #

Patch Set 5 : diff -r 07e31caba5b6 https://code.google.com/p/go/ #

Total comments: 6

Patch Set 6 : diff -r 04ab9968c31b https://code.google.com/p/go/ #

Total comments: 4

Patch Set 7 : diff -r 04ab9968c31b https://code.google.com/p/go/ #

Created: 10 years ago

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+97 lines, -38 lines)			Patch
	M	src/pkg/net/http/cookie.go	View	1 2 3 4 5 6	3 chunks	+19 lines, -37 lines	0 comments	Download
	M	src/pkg/net/http/cookie_test.go	View	1 2 3	3 chunks	+78 lines, -1 line	0 comments	Download

Messages

Total messages: 9

Expand All Messages | Collapse All Messages

volker.dobler

Hello nigeltao@golang.org (cc: bradfitz@golang.org, golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go/

10 years ago (2014-04-10 08:09:23 UTC) #1

nigeltao

https://codereview.appspot.com/86050045/diff/80001/src/pkg/net/http/cookie.go File src/pkg/net/http/cookie.go (right): https://codereview.appspot.com/86050045/diff/80001/src/pkg/net/http/cookie.go#newcode298 src/pkg/net/http/cookie.go:298: // but we produce a quoted cookie-value in case ...

10 years ago (2014-04-11 06:54:47 UTC) #2

nigeltao

Just checking that I understand the intended behavior. After this CL: A Go server can ...

10 years ago (2014-04-11 07:21:10 UTC) #3

volker.dobler

On Fri, Apr 11, 2014 at 9:21 AM, <nigeltao@golang.org> wrote: > Just checking that I ...

10 years ago (2014-04-11 09:50:50 UTC) #4

On Fri, Apr 11, 2014 at 9:21 AM, <nigeltao@golang.org> wrote:

> Just checking that I understand the intended behavior. After this CL:
>
> A Go server can now set commas and spaces in cookies, adding quotes to
> write things like:
> Set-Cookie: foo=",z"
>
Yes. The quotes are added automatically if needed user of
net/http don't have to care about this.


> A Go server will not send
> Set-Cookie: foo=,z
> without quotes, but the Go client will still accept this.
>
Yes (for send) and yes (for accept)
(For send just almost: I think it is possible to manually add such
a header via Header.Add; but yes, not through http.SetCookie.)

>
> The Go client will strip quotes if present, so that it treats these two:
> Set-Cookie: foo=,z
> Set-Cookie: foo=",z"
> as equivalent.

Yes.
Unconditional yes for commas, for spaces
Set-Cookie: foo=" z"
Set-Cookie: foo= z
differ: The value in the second case is just "z" but this space
removal is mandated by RFC 6265: Step 4 of the Set-Cookie
parsing algorithm in section 5.2.


> Non-Go clients, and especially browsers (as inspected via
> Javascript via alert(document.cookie), see below) don't actually strip
> the quotes, but in a Go server -> browser -> Go server round trip, the
> Go code (above the net/http layer) won't see quotes at either end.
>
Yes. (Same for curl if invoked with -c it will store the quoted values.)

V.



>
> Test program:
> --------
> package main
>
> import (
>         "fmt"
>         "log"
>         "net/http"
>         "strconv"
>         "time"
> )
>
> func main() {
>         http.HandleFunc("/get", handleGet)
>         http.HandleFunc("/set", handleSet)
>         log.Fatal(http.ListenAndServe(":8080", nil))
> }
>
> func handleGet(w http.ResponseWriter, req *http.Request) {
>         // Edit the Cookie.String code in the net/http package's cookie.go
> to
> add quote marks.
>         c, _ := req.Cookie("foobar")
>         if c != nil {
>                 fmt.Printf("get %q\n", c.Value)
>         } else {
>                 fmt.Printf("get -\n")
>         }
>         w.Write([]byte(`<html><body>Go back to <a
> href="/set">/set</a>.</body></html>`))
> }
>
> func handleSet(w http.ResponseWriter, req *http.Request) {
>         nanos := strconv.Itoa(time.Now().Nanosecond())
>         fmt.Printf("set %q\n", nanos)
>         http.SetCookie(w, &http.Cookie{
>                 Name:    "foobar",
>                 Value:   nanos,
>                 Expires: time.Now().Add(1 * time.Hour),
>         })
>         fmt.Fprintf(w, `<html><head>`+
>                 `<script>alert(document.cookie)</script>`+
>                 `<meta http-equiv="refresh" content="0;
> url=/get"></head></html>`)
> }
> --------
>
> https://codereview.appspot.com/86050045/
>



-- 
Dr. Volker Dobler

volker.dobler

PTAL https://codereview.appspot.com/86050045/diff/80001/src/pkg/net/http/cookie.go File src/pkg/net/http/cookie.go (right): https://codereview.appspot.com/86050045/diff/80001/src/pkg/net/http/cookie.go#newcode298 src/pkg/net/http/cookie.go:298: // but we produce a quoted cookie-value in ...

10 years ago (2014-04-11 09:51:19 UTC) #5

nigeltao

https://codereview.appspot.com/86050045/diff/100001/src/pkg/net/http/cookie.go File src/pkg/net/http/cookie.go (right): https://codereview.appspot.com/86050045/diff/100001/src/pkg/net/http/cookie.go#newcode313 src/pkg/net/http/cookie.go:313: return 0x1f < b && b < 0x7f && ...

10 years ago (2014-04-14 00:21:49 UTC) #6

volker.dobler

PTAL https://codereview.appspot.com/86050045/diff/100001/src/pkg/net/http/cookie.go File src/pkg/net/http/cookie.go (right): https://codereview.appspot.com/86050045/diff/100001/src/pkg/net/http/cookie.go#newcode313 src/pkg/net/http/cookie.go:313: return 0x1f < b && b < 0x7f ...

10 years ago (2014-04-14 09:12:23 UTC) #7

nigeltao

10 years ago (2014-04-17 06:01:31 UTC) #9

*** Submitted as https://code.google.com/p/go/source/detail?r=f1f593b2b24e ***

net/http: allow commas and spaces in cookie values

According to RFC 6265 a cookie value may contain neither
commas nor spaces but such values are very common in the
wild and browsers handle them very well so we'll allow
both commas and spaces.
Values starting or ending in a comma or a space are
sent in the quoted form to prevent missinterpetations.

RFC 6265 conforming values are handled as before and
semicolons, backslashes and double-quotes are still
disallowed.

Fixes issue 7243

LGTM=nigeltao
R=nigeltao
CC=bradfitz, golang-codereviews
https://codereview.appspot.com/86050045

Committer: Nigel Tao <nigeltao@golang.org>

Expand All Messages | Collapse All Messages