code review 12204043: net/http: don't write out invalid cookie lines

net/http: don't write out invalid cookie lines

Fixes issue 3033

[bradfitz, 2013-07-31 23:05:35]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[fvb, 2013-08-01 07:20:39]

https://codereview.appspot.com/12204043/diff/6001/src/pkg/net/http/cookie.go
File src/pkg/net/http/cookie.go (right):

https://codereview.appspot.com/12204043/diff/6001/src/pkg/net/http/cookie.go#...
src/pkg/net/http/cookie.go:270: return b > 0x20 && b != 0x22 && b != 0x2c && b
!= 0x3b && b != 0x5c && b != 0x7f
You forgot to exclude non-ASCII characters (by adding "&& b < 0x80" or folding
it into the 0x7f case by using < instead of != there).

https://codereview.appspot.com/12204043/diff/6001/src/pkg/net/http/cookie.go#...
src/pkg/net/http/cookie.go:270: return b > 0x20 && b != 0x22 && b != 0x2c && b
!= 0x3b && b != 0x5c && b != 0x7f
Also, perhaps you should use rune constants here to make it more clear what it
does?
0x22, 0x2c, 0x3b and 0x5c are a bit less clear than '"', ',', ';' and '\\'. Not
sure about 0x20 <--> ' '.
This could be argued the other way given the RFC quote in the comment on
sanitizeCookieValue, though maybe in that case the comment in question should be
on *this* function instead of (or in addition to) there?

https://codereview.appspot.com/12204043/diff/6001/src/pkg/net/http/cookie.go#...
src/pkg/net/http/cookie.go:273: func validCookiePathByte(b byte) bool {
Most of what I wrote about the previous function doesn't apply here, but part of
the comment on sanitizeCookiePath could be moved or copied here.

https://codereview.appspot.com/12204043/diff/6001/src/pkg/net/http/cookie.go#...
src/pkg/net/http/cookie.go:273: func validCookiePathByte(b byte) bool {
Alternatively, since this function and the one above it are so short, they could
be inlined as function literals into the SanitizeCookie* functions that use
them.

[bradfitz, 2013-08-01 17:37:22]

PTAL

https://codereview.appspot.com/12204043/diff/6001/src/pkg/net/http/cookie.go
File src/pkg/net/http/cookie.go (right):

https://codereview.appspot.com/12204043/diff/6001/src/pkg/net/http/cookie.go#...
src/pkg/net/http/cookie.go:270: return b > 0x20 && b != 0x22 && b != 0x2c && b
!= 0x3b && b != 0x5c && b != 0x7f
On 2013/08/01 07:20:39, fvb wrote:
> You forgot to exclude non-ASCII characters (by adding "&& b < 0x80" or folding
> it into the 0x7f case by using < instead of != there).

Whoops, fixed. Thanks.

https://codereview.appspot.com/12204043/diff/6001/src/pkg/net/http/cookie.go#...
src/pkg/net/http/cookie.go:273: func validCookiePathByte(b byte) bool {
On 2013/08/01 07:20:39, fvb wrote:
> Most of what I wrote about the previous function doesn't apply here, but part
of
> the comment on sanitizeCookiePath could be moved or copied here.

Rearranged to put them next to their callers and docs.

[bradfitz, 2013-08-01 17:38:35]

Hello golang-dev@googlegroups.com, fvbommel@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[rsc, 2013-08-01 18:08:57]

LGTM

https://codereview.appspot.com/12204043/diff/13001/src/pkg/net/http/cookie.go
File src/pkg/net/http/cookie.go (right):

https://codereview.appspot.com/12204043/diff/13001/src/pkg/net/http/cookie.go...
src/pkg/net/http/cookie.go:242: return b > 0x20 && b != 0x22 && b != 0x2c && b
!= 0x3b && b != 0x5c && b < 0x7f
We usually make ranges clear by writing lo < x && x < hi (or <= as appropriate).
I don't much care whether the != are all in the middle or at the end, but this
line is different from the one in validCookiePathByte. I suggest doing the range
first, but really I care about the consistency.

return 0x20 < b && b < 0x7f && b != ...

makes clear that there's a range check and then some exceptions.

https://codereview.appspot.com/12204043/diff/13001/src/pkg/net/http/cookie.go...
src/pkg/net/http/cookie.go:252: return b >= 0x20 && b < 0x7f && b != ';'
0x20 <= b

are spaces really okay in the path?

https://codereview.appspot.com/12204043/diff/13001/src/pkg/net/http/cookie.go...
src/pkg/net/http/cookie.go:268: buf := bytes.NewBuffer(make([]byte, 0, len(v)))
bytes.Buffer seems like overkill when you could use a []byte and append.

[bradfitz, 2013-08-01 18:15:48]

https://codereview.appspot.com/12204043/diff/13001/src/pkg/net/http/cookie.go
File src/pkg/net/http/cookie.go (right):

https://codereview.appspot.com/12204043/diff/13001/src/pkg/net/http/cookie.go...
src/pkg/net/http/cookie.go:242: return b > 0x20 && b != 0x22 && b != 0x2c && b
!= 0x3b && b != 0x5c && b < 0x7f
On 2013/08/01 18:08:57, rsc wrote:
> We usually make ranges clear by writing lo < x && x < hi (or <= as
appropriate).
> I don't much care whether the != are all in the middle or at the end, but this
> line is different from the one in validCookiePathByte. I suggest doing the
range
> first, but really I care about the consistency.
> 
> return 0x20 < b && b < 0x7f && b != ...
> 
> makes clear that there's a range check and then some exceptions.

Will do.

https://codereview.appspot.com/12204043/diff/13001/src/pkg/net/http/cookie.go...
src/pkg/net/http/cookie.go:252: return b >= 0x20 && b < 0x7f && b != ';'
On 2013/08/01 18:08:57, rsc wrote:
> 0x20 <= b
> 
> are spaces really okay in the path?

That surprised me too. The spec sure makes it seems like it.

https://codereview.appspot.com/12204043/diff/13001/src/pkg/net/http/cookie.go...
src/pkg/net/http/cookie.go:268: buf := bytes.NewBuffer(make([]byte, 0, len(v)))
On 2013/08/01 18:08:57, rsc wrote:
> bytes.Buffer seems like overkill when you could use a []byte and append.

Yeah it does. Will change.

[bradfitz, 2013-08-01 19:16:41]

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

net/http: don't write out invalid cookie lines

Fixes issue 3033

R=golang-dev, fvbommel, rsc
CC=golang-dev
https://codereview.appspot.com/12204043