code review 5544082: net/http/cookiejar: A cookie jar implementation.

net/http/cookiejar: A cookie jar implementation.

This implementation is a very simple one: Cookies are
stored in a unsorted, flat slice.  All operations
(insert, update, delete, retrieve) are painfully slow
at the moment but the test cases should cover all
aspects of cookie handling.

Still missing are:
  - host/domain name canonicalization (this will need
    additional tables)
  - an exported method to persist the persistent cookies

[volker.dobler, 2012-01-17 00:49:14]

This got rather lengthy even in its unfinished state.

Any comments welcome.

The non-http stuff could be simplified (just ignoring
any non-http/https protocol) but I wanted to try all
the testcases first.

V.

[jmhodges, 2012-01-17 04:10:29]

Could you uncomment the tests in TestRedirectCookiesOnRequest in
net/http/client_test.go and make sure they pass? They seem to be failing when I
run them with your code.

[volker.dobler, 2012-01-17 10:18:18]

I think the testcase is broken: If the second c..Do uses the same
client as the first, it will use the same cookie jar. And this jar
contains cookies from the redirect chain triggered by the first
c.Do.

func TestRedirectCookiesOnRequest(t *testing.T) {
	var ts *httptest.Server
	ts = httptest.NewServer(echoCookiesRedirectHandler)
	defer ts.Close()
	c := &Client{}
	c.Jar = cookiejar.New(100,100,100)
	req, _ := NewRequest("GET", ts.URL, nil)
	req.AddCookie(expectedCookies[0])
	_ = c
	resp, _ := c.Do(req)
	matchReturnedCookies(t, expectedCookies, resp.Cookies())

	req, _ = NewRequest("GET", ts.URL, nil)
	resp, _ = c.Do(req)
	matchReturnedCookies(t, expectedCookies[1:], resp.Cookies())  // BROKEN

	/*
	 CC: ChocolateChips; F: First; S: Second

	 jar is empty
	 req1.AddCookie(CC)                         // manualy added CC to request
	 c.Do(req1):
	     -->  GET / (CC)                              // CC cookie from to requets
             <--  302 to /second (Set CC; F)        // CC echoed back,
additional F
	     store CC and F in jar
	     -->  GET /second (CC; F)               // both CC and F from jar
	     <--  200 (Set CC; F; S)                  // echo CC and F, add S
	     update CC and F, store S
	 3 cookies recieved

  	 jar contains CC; F; S
	
	 c.Do(fresh_req):                                     // will use same jar!
	     -->  GET / (CC; F; S)                          // CC, F and S from jar!
             <--  302 to /second (Set CC; F; S; F)  // echo all and add F
             update jar                                         // no
real change, still contains CC,F,S
	     -->  GET /second (CC; F; S)              // all from jar
	     <--  200 (Set CC; F; S; S)                 // echo CC,F,S add S
	     update CC,F,S and S a second time
  	 here 4 cookies are recieved: CC, F, S and S
	 */
}

I am not sure if the whole second (fresh) request can test anything
useful if it uses the same, filled jar without further modification (cookie
restricted to path, expired, etc.)

Correct me if I am wrong!

Volker

On 1/17/12, jeff@somethingsimilar.com <jeff@somethingsimilar.com> wrote:
> Could you uncomment the tests in TestRedirectCookiesOnRequest in
> net/http/client_test.go and make sure they pass? They seem to be failing
> when I run them with your code.
>
> http://codereview.appspot.com/5544082/
>


-- 
Dr. Volker Dobler

[jmhodges, 2012-01-18 05:53:21]

Volker and I accidentally went offline. We agreed that this test was
wrong and that a nil Jar should mean "don't pass cookies". It should
probably be removed.

On Tue, Jan 17, 2012 at 2:18 AM, Volker Dobler
<dr.volker.dobler@gmail.com> wrote:
> I think the testcase is broken: If the second c..Do uses the same
> client as the first, it will use the same cookie jar. And this jar
> contains cookies from the redirect chain triggered by the first
> c.Do.
>
> func TestRedirectCookiesOnRequest(t *testing.T) {
>        var ts *httptest.Server
>        ts = httptest.NewServer(echoCookiesRedirectHandler)
>        defer ts.Close()
>        c := &Client{}
>        c.Jar = cookiejar.New(100,100,100)
>        req, _ := NewRequest("GET", ts.URL, nil)
>        req.AddCookie(expectedCookies[0])
>        _ = c
>        resp, _ := c.Do(req)
>        matchReturnedCookies(t, expectedCookies, resp.Cookies())
>
>        req, _ = NewRequest("GET", ts.URL, nil)
>        resp, _ = c.Do(req)
>        matchReturnedCookies(t, expectedCookies[1:], resp.Cookies())  // BROKEN
>
>        /*
>         CC: ChocolateChips; F: First; S: Second
>
>         jar is empty
>         req1.AddCookie(CC)                         // manualy added CC to
request
>         c.Do(req1):
>             -->  GET / (CC)                              // CC cookie from to
requets
>             <--  302 to /second (Set CC; F)        // CC echoed back,
> additional F
>             store CC and F in jar
>             -->  GET /second (CC; F)               // both CC and F from jar
>             <--  200 (Set CC; F; S)                  // echo CC and F, add S
>             update CC and F, store S
>         3 cookies recieved
>
>         jar contains CC; F; S
>
>         c.Do(fresh_req):                                     // will use same
jar!
>             -->  GET / (CC; F; S)                          // CC, F and S from
jar!
>             <--  302 to /second (Set CC; F; S; F)  // echo all and add F
>             update jar                                         // no
> real change, still contains CC,F,S
>             -->  GET /second (CC; F; S)              // all from jar
>             <--  200 (Set CC; F; S; S)                 // echo CC,F,S add S
>             update CC,F,S and S a second time
>         here 4 cookies are recieved: CC, F, S and S
>         */
> }
>
> I am not sure if the whole second (fresh) request can test anything
> useful if it uses the same, filled jar without further modification (cookie
> restricted to path, expired, etc.)
>
> Correct me if I am wrong!
>
> Volker
>
> On 1/17/12, jeff@somethingsimilar.com <jeff@somethingsimilar.com> wrote:
>> Could you uncomment the tests in TestRedirectCookiesOnRequest in
>> net/http/client_test.go and make sure they pass? They seem to be failing
>> when I run them with your code.
>>
>> http://codereview.appspot.com/5544082/
>>
>
>
> --
> Dr. Volker Dobler

[rsc, 2012-01-19 21:10:38]

R=bradfitz?

[jmhodges, 2012-01-20 00:13:03]

I haven't had the chance to review more beyond the first few files. Sending this
before I forget to.

http://codereview.appspot.com/5544082/diff/2001/src/pkg/net/http/cookiejar/co...
File src/pkg/net/http/cookiejar/cookie.go (right):

http://codereview.appspot.com/5544082/diff/2001/src/pkg/net/http/cookiejar/co...
src/pkg/net/http/cookiejar/cookie.go:38: // shouldSendCookie determines whether
to send cookie via a secure/nonHttp request
s/shouldSendCookie/shouldSend/

The other way may be more clear.

http://codereview.appspot.com/5544082/diff/2001/src/pkg/net/http/cookiejar/co...
src/pkg/net/http/cookiejar/cookie.go:45: httpProtocol(c.HttpOnly, !nonHttp)
This double negative is troublesome.

http://codereview.appspot.com/5544082/diff/2001/src/pkg/net/http/cookiejar/co...
src/pkg/net/http/cookiejar/cookie.go:75: return !c.HostOnly &&
strings.HasSuffix(host, "."+c.Domain)
What happens if c.Domain has a dot at the front? (I think it's expected to?)

http://codereview.appspot.com/5544082/diff/2001/src/pkg/net/http/cookiejar/co...
src/pkg/net/http/cookiejar/cookie.go:97: } else if requestPath[len(c.Path)] ==
'/' {
When requestPath and c.Path are the same size, this will error.

A better way might be to use strings.LastIndex and reuse that for both of these
conditionals.

http://codereview.appspot.com/5544082/diff/2001/src/pkg/net/http/cookiejar/ja...
File src/pkg/net/http/cookiejar/jar.go (right):

http://codereview.appspot.com/5544082/diff/2001/src/pkg/net/http/cookiejar/ja...
src/pkg/net/http/cookiejar/jar.go:109: for _, cookie := range jar.cookies {
This seems a little much just to get access to the cookies. Why not do this
inline and return an array?

Also, I don't see a jar.lock.Lock() here. Are you sure we don't need it?

http://codereview.appspot.com/5544082/diff/2001/src/pkg/net/http/cookiejar/ja...
src/pkg/net/http/cookiejar/jar.go:369: runtime.GC()
Remove.

http://codereview.appspot.com/5544082/diff/2001/src/pkg/net/http/cookiejar/ja...
src/pkg/net/http/cookiejar/jar.go:425: nonHttp := isNonHttp(u)
Double negatives are a pain to deal with. 

Also, Go conventions are to capitalize all the letters in an acronym.

http://codereview.appspot.com/5544082/diff/2001/src/pkg/net/http/cookiejar/ja...
src/pkg/net/http/cookiejar/jar.go:426: /*
Remove.

[bradfitz, 2012-01-20 00:16:56]

I'll review this also.
 On Jan 19, 2012 1:10 PM, <rsc@golang.org> wrote:

> R=bradfitz?
>
>
>
http://codereview.appspot.com/**5544082/<http://codereview.appspot.com/5544082/>
>

[volker.dobler, 2012-01-20 17:17:17]

Hello jeff@somethingsimilar.com, bradfitz@golang.org (cc:
golang-dev@googlegroups.com),

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

[volker.dobler, 2012-01-20 17:30:30]

The main questions which IMHO have to be answered before
diging into details of the code are:

A) API of Jar:
    - naming of constructors
    - Access to cookies, currently the unsuitable All()
    - persistence/saving/reading a jar
    - additional exported methods?

B) Behaviour: is the strict/non-strict switch
   the proper way to be a bit more browser like?
   Is this (host cookies for IP addresses) needed?

C) Do the test cases cover all the necessary cases
   and are they correct?

D) Are the benchmarks meaningful?

Once C and D are setteled I'll replace the dead
stupid implementation with something less embarrassing...
But i'd like to have a testsuite ready which proves
the impl works.

Volker

http://codereview.appspot.com/5544082/diff/2001/src/pkg/net/http/cookiejar/co...
File src/pkg/net/http/cookiejar/cookie.go (right):

http://codereview.appspot.com/5544082/diff/2001/src/pkg/net/http/cookiejar/co...
src/pkg/net/http/cookiejar/cookie.go:38: // shouldSendCookie determines whether
to send cookie via a secure/nonHttp request
On 2012/01/20 00:13:04, jmhodges wrote:
> s/shouldSendCookie/shouldSend/
> 
> The other way may be more clear.

Done.

http://codereview.appspot.com/5544082/diff/2001/src/pkg/net/http/cookiejar/co...
src/pkg/net/http/cookiejar/cookie.go:45: httpProtocol(c.HttpOnly, !nonHttp)
On 2012/01/20 00:13:04, jmhodges wrote:
> This double negative is troublesome.

Done.

http://codereview.appspot.com/5544082/diff/2001/src/pkg/net/http/cookiejar/co...
src/pkg/net/http/cookiejar/cookie.go:75: return !c.HostOnly &&
strings.HasSuffix(host, "."+c.Domain)
On 2012/01/20 00:13:04, jmhodges wrote:
> What happens if c.Domain has a dot at the front? (I think it's expected to?)

cookiejar.Cookie.Domain never starts with a dot, it is
stripped (if present) in domainAndType().
Leading dots for domain cookies are not required:
http://tools.ietf.org/html/rfc6265#section-5.2.3

http://codereview.appspot.com/5544082/diff/2001/src/pkg/net/http/cookiejar/co...
src/pkg/net/http/cookiejar/cookie.go:97: } else if requestPath[len(c.Path)] ==
'/' {
On 2012/01/20 00:13:04, jmhodges wrote:
> When requestPath and c.Path are the same size, this will error.

No: If this line is reached, than requestPath has c.Path
as a prefix. So they are identiacal if there length are
the same. But requestPath==c.Path is handled above, so
this will not fail. (I know: such brain twisters are
not good code, but see below)

> A better way might be to use strings.LastIndex and reuse that for both of
these
> conditionals.

Yes! The algorithm is a direct translation of the spec.
I thought about having an algorithm which is
spec-conforming, set up the testcases and optimise
the algos/ds afterwards. 
Added your comment as TODO.

http://codereview.appspot.com/5544082/diff/2001/src/pkg/net/http/cookiejar/ja...
File src/pkg/net/http/cookiejar/jar.go (right):

http://codereview.appspot.com/5544082/diff/2001/src/pkg/net/http/cookiejar/ja...
src/pkg/net/http/cookiejar/jar.go:109: for _, cookie := range jar.cookies {
On 2012/01/20 00:13:04, jmhodges wrote:
> This seems a little much just to get access to the cookies. Why not do this
> inline and return an array?
> 
> Also, I don't see a jar.lock.Lock() here. Are you sure we don't need it?
No, I forgot it (as well as close(ch)). 
I totaly aggree that this function is bad. 

We should have A) a way to persist the
(non-expired) persistent cookies and read them back.
And we should have B) a way to get access to all (valid
i.e. non-expired) cookies in the jar.
For the first issue I was thinking about gob, the
second resulted in this broken hack.

How should the interface look like? 
For the access to all valid cookies: It might be
tricky if a copy is returned as the cookies
in the jar might change while some goroutine decides
based on the copy what to do. Maybe we have too add
extra methods like Delete(domain, path, name string)
or DeleteAll(domain string)...

For the persistence i thought of providing 
Gob{En,De}coder. Encoding could be simple,
but during decoding there is no need to read all
persisted cookies back, ony the non-expired
need to be processed.

http://codereview.appspot.com/5544082/diff/2001/src/pkg/net/http/cookiejar/ja...
src/pkg/net/http/cookiejar/jar.go:369: runtime.GC()
On 2012/01/20 00:13:04, jmhodges wrote:
> Remove.

Done.

http://codereview.appspot.com/5544082/diff/2001/src/pkg/net/http/cookiejar/ja...
src/pkg/net/http/cookiejar/jar.go:425: nonHttp := isNonHttp(u)
On 2012/01/20 00:13:04, jmhodges wrote:
> Double negatives are a pain to deal with. 
> 
> Also, Go conventions are to capitalize all the letters in an acronym.

Done.

http://codereview.appspot.com/5544082/diff/2001/src/pkg/net/http/cookiejar/ja...
src/pkg/net/http/cookiejar/jar.go:426: /*
On 2012/01/20 00:13:04, jmhodges wrote:
> Remove.

Brad suggested to have an "http only" cookie jar,
so I just uncommented it (elsewhere too) and commented
out the http/nonhttp testcases.

[bradfitz, 2012-01-25 16:47:55]

http://codereview.appspot.com/5544082/diff/7002/src/pkg/net/http/cookiejar/ja...
File src/pkg/net/http/cookiejar/jar.go (right):

http://codereview.appspot.com/5544082/diff/7002/src/pkg/net/http/cookiejar/ja...
src/pkg/net/http/cookiejar/jar.go:24: var RejectPublicSuffixes = true
I don't want this controlled by a global variable. It should be a property of
the jar.

http://codereview.appspot.com/5544082/diff/7002/src/pkg/net/http/cookiejar/ja...
src/pkg/net/http/cookiejar/jar.go:31: lock    sync.Mutex // the BKL of our jar.
TODO(vodo) replace by RWMutex
this guards everything? the AllCookies method below doesn't use it.

http://codereview.appspot.com/5544082/diff/7002/src/pkg/net/http/cookiejar/ja...
src/pkg/net/http/cookiejar/jar.go:50: func NewCustomJar(maxCookiesPerDomain,
maxTotalCookies, maxBytesPerCookie int, strict bool) *Jar {
too many position parameters of the same type.

could users just create a &cookiejar.Jar{MaxCookiesPerDomain: 5} and have the
zero values mean some defaults, specified as consts?

then you can remove NewCustomJar.

http://codereview.appspot.com/5544082/diff/7002/src/pkg/net/http/cookiejar/ja...
src/pkg/net/http/cookiejar/jar.go:53: jar.cookies = make([]Cookie, 0, 16)
this can just be lazily initialized.

[rsc, 2012-01-25 21:12:36]

Hi.

I think it's important to have a package that provides a
reasonable in-memory-only implementation of http.CookieJar
and also provides helper functions that might allow someone
to implement persistent jars without duplicating work.
Thanks very much for working on this.

We're getting to the point in the Go 1 release process,
where we only want to be fixing bugs, not adding new code,
and not designing new APIs that we won't have time to
use in anger before committing to them.

I think we should postpone this CL until after Go 1 is done.
Volker, if you want to run this code before then, my suggestion
would be to post it on code.google.com or github so that we
can use go get to install and use it.  Please ping us once 
Go 1 is out so that we can continue this discussion.

I have updated issue 1960 to point to this CL and to note
that we're waiting until after Go 1.  If you do post the code
in an external repository, please add a link to that issue.
http://code.google.com/p/go/issues/detail?id=1960

Thanks.
Russ

[volker.dobler, 2012-01-25 21:29:16]

Hi,

Currently this is the only suitable way.
As an external library I can use go-idn
(http://code.google.com/p/go-idn/) to get 
canonical host names which will make the jar
really RFC 6265 conforming.  Expect a version
on github which should be usable after my 
holidays.

Volker

[bradfitz, 2012-01-25 22:18:58]

excellent!

On Wed, Jan 25, 2012 at 1:29 PM, <dr.volker.dobler@gmail.com> wrote:

> Hi,
>
> Currently this is the only suitable way.
> As an external library I can use go-idn
> (http://code.google.com/p/go-**idn/ <http://code.google.com/p/go-idn/>)
> to get
> canonical host names which will make the jar
> really RFC 6265 conforming.  Expect a version
> on github which should be usable after my
> holidays.
>
> Volker
>
>
http://codereview.appspot.com/**5544082/<http://codereview.appspot.com/5544082/>
>