[golang-dev] code review 6849092: net/http/cookiejar: new package. (issue 6849092)

LGTM



https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/jar.go
File src/pkg/net/http/cookiejar/jar.go (right):

https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/jar.go#newcode5
src/pkg/net/http/cookiejar/jar.go:5: // Package cookiejar provides a
http.CookieJar implementation that conforms
Package cookiejar implements an RFC 6265-compliant http.CookieJar.

https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/jar.go#newcode44
src/pkg/net/http/cookiejar/jar.go:44: // Jar implements the
http.CookieJar interface from the net/http package.
Do you want people to modify the jar after construction? I am wondering
if we should have a cookiejar.New(*Options) instead.

https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/jar.go#newcode56
src/pkg/net/http/cookiejar/jar.go:56: // TODO: how do we reject HttpOnly
cookies? Do we post-process the return
I don't understand this TODO. Both the Cookies and SetCookies methods
have a URL so it seems like they can tell HTTP from HTTPS.

https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/storage.go
File src/pkg/net/http/cookiejar/storage.go (right):

https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/storage.go#newcode17
src/pkg/net/http/cookiejar/storage.go:17: // A key is invalid if it
contains a byte that is not '-', not '.', is outside
A valid key must use only bytes in the character class [a-z0-9.-] and
must have at least one non-. byte. Note that this excludes any key
containing a capital ASCII letter as well as the empty string.

https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/storage.go#newcode23
src/pkg/net/http/cookiejar/storage.go:23: // The Add method has
undefined behavior if the entry's Data has zero length.
An added Entry will always have non-empty Data.
(maybe move down to the Add method comment)

https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/storage.go#newcode25
src/pkg/net/http/cookiejar/storage.go:25: // A Jar will not call a
Storage's methods, other than Lock, unless it has
I think this might work better as comments on the Lock and Unlock
methods, if you expand sync.Locker (which most people have probably not
seen):

// A client must call Lock before calling other methods and must call
// Unlock when finished. Between the calls to Lock and Unlock, a client
// can assume that other clients are not modifying the Storage.
Lock()
Unlock()

https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/storage.go#newcode68
src/pkg/net/http/cookiejar/storage.go:68: Data []byte
You spend a lot of time telling people not to mutate the []byte.
This is ASCII data, though, right? So maybe it would be okay to use
string?

https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/storage_test.go
File src/pkg/net/http/cookiejar/storage_test.go (right):

https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/storage_test.go#newcode12
src/pkg/net/http/cookiejar/storage_test.go:12: testCases :=
map[string]bool{
make global var validStorageKeyTests

https://codereview.appspot.com/6849092/

Nothing is fundamentally wrong with this API,
but I admit I don't see what the rationals for
this API are. Having a Storage with opaque Data
forces Jar to serialize Cookies and the filter
functions f to decode this serialization before
doing actual work. Is it about squeezing the
booleans HostOnly, Secure and HttpOnly into a
bitfield and compressing Value to minimize the
memory footprint of an in-memory Storage?
Or is it just a way to avoid an additional type
StoredCookie which would share several fields with
http.Cookie?
Having LastAccess exposed in Entry and managed by
Storage is nice, but only if LastAccess is used
somewhere: The only two task where LastAccess
is ever used is a) display in a GUI and b) during
compactification of the jar when LastAccess helps
to decide which cookies to drop from the jar.
Such functions should be part of the API, at least
if we are heading for a general "suits-everything"
cookie jar.

Just to be sure: The general design is fixed to
"Jar delegates all cookie storage to an interface
type Storage"? Or are other designs like "Package
cookiejar provides an efficent in-memory cookie
jar as well as functions to build specialized
cookie jars." or "The way chromium/firefox does."
still discussed?



https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/jar.go
File src/pkg/net/http/cookiejar/jar.go (right):

https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/jar.go#newcode44
src/pkg/net/http/cookiejar/jar.go:44: // Jar implements the
http.CookieJar interface from the net/http package.
Russ's Options could be used for stuff like allowing
host cookies on IP addresses and safeguarding against
excessive long Value.

https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/jar.go#newcode56
src/pkg/net/http/cookiejar/jar.go:56: // TODO: how do we reject HttpOnly
cookies? Do we post-process the return
It is (I assume) about telling HTTP(S) from FTP
and other protocols, not about telling HTTP from
HTTPS (which is controlled by Secure field).

https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/jar.go#newcode56
src/pkg/net/http/cookiejar/jar.go:56: // TODO: how do we reject HttpOnly
cookies? Do we post-process the return
Allowing an ftp server to set a cookie which gets sent
to a HTTP request (and the other way around) might be
allowed in theory, but most probably this is a bad
idea. I would be strict (and safe): "Jar will neither
accept cookies from, nor send cookies to any URL which
is neither HTTP nor HTTPS."

https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/storage.go
File src/pkg/net/http/cookiejar/storage.go (right):

https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/storage.go#newcode66
src/pkg/net/http/cookiejar/storage.go:66: Creation time.Time
Why is Creation treated special here? Created is set once
like Name or Path and not used for retrieval (it is not
even used for cleanup or shrinking the jar). Just because
LastAccess is handled special?

https://codereview.appspot.com/6849092/

There is only one Jar implementation here. The Storage interface is
structured so that it is as simple as possible to implement and as
difficult as possible to implement incorrectly.

Russ

The rationale for this design, as bradfitz mused earlier, is that the
implementor of an SQLiteStorage or a LevelDBStorage shouldn't have to
know anything about HTTP cookies and all its subtleties. Note that in
this proposal, storage.go does not import "net/http".

I haven't decided yet whether Jar has its own in-memory storage. This
design does allow for a CachingStorage to wrap another Storage, so it
doesn't have to be the responsibility of Jar per se. On a related note,
a Jar may want its own in-memory storage for non-persistent cookies. I'm
still exploring this space.


https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/jar.go
File src/pkg/net/http/cookiejar/jar.go (right):

https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/jar.go#newcode56
src/pkg/net/http/cookiejar/jar.go:56: // TODO: how do we reject HttpOnly
cookies? Do we post-process the return
Heh, the HttpOnly name is unfortunate, but out of our control.

It has nothing to do with FTP, from Go's point of view. It means that
the cookie is used for HTTP(S) requests, but is not viewable from a
page's javascript (via document.cookie). It mitigates XSS attacks.

https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/storage.go
File src/pkg/net/http/cookiejar/storage.go (right):

https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/storage.go#newcode66
src/pkg/net/http/cookiejar/storage.go:66: Creation time.Time
It's here because when a newly set cookie replaces an existing cookie
(replaces means same name/domain/path), then the new cookie is stored
with the creation time of the old cookie. (RFC 6265 Section 5.3 Step
11).

On second thoughts, creation time could arguably be encoded in the Data
[]byte, but it could be easier to manage as a separate Entry field. This
API isn't set in stone yet; presumably implementing a MemStorage and
possibly a SQLiteStorage or LevelDBStorage will shake out the design.

LastAccess is a separate field because sending an HTTP request scans the
cookies and updates the LastAccess of each to time.Now. Being a separate
field means that the Data bytes don't need a decode-modify-encode step
for this common operation.

https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/storage.go#newcode68
src/pkg/net/http/cookiejar/storage.go:68: Data []byte
string?

Yeah, string might be better. I was trying to avoid []byte <--> string
conversions to and from storage, but that's possibly premature
optimization.

https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/storage_test.go
File src/pkg/net/http/cookiejar/storage_test.go (right):

https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/storage_test.go#newcode12
src/pkg/net/http/cookiejar/storage_test.go:12: testCases :=
map[string]bool{
Why? I prefer local variables over global variables, and I think it's
just as readable.

https://codereview.appspot.com/6849092/

I see.

Why not taking the identifying (name,domain,path)-tuple
out of the opaque Data like this:
type Entry {
Creation, LastAccess time.Time
Id string // e.g. Name + ";" + Domain + ";" + Path
Data []byte // just Value and flags left
}
and Storage must treat any Entry with the same Id
as identical in the set of a given key? This wouldn't
make a DB-backed Storage much more complicated and
Storage can be required to manage Creation as it know
knows if two cookies are the same (even if it doesn't
need to know anything about cookies at all).
This would remove the need of Jar to query Storage
first before Add'ing a cookie.




https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/jar.go
File src/pkg/net/http/cookiejar/jar.go (right):

https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/jar.go#newcode56
src/pkg/net/http/cookiejar/jar.go:56: // TODO: how do we reject HttpOnly
cookies? Do we post-process the return
methods have a
Yes, access through document.cookie is one of those
"non-HTTP" APIs RFC 6265 forbids in 5.4.1 bullet 4.
Am I wrong in assuming that every other (incl. FTP or
jet-to-invent protocols) are excluded too?

https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/storage.go
File src/pkg/net/http/cookiejar/storage.go (right):

https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/storage.go#newcode66
src/pkg/net/http/cookiejar/storage.go:66: Creation time.Time
LastAccess as a separate field is perfectly fine,
It is managed by Storage. But who is going to manage
Creation? If Storage is responsible for updating
than Storage must have know-how about the opaque Data;
otherwise it cannot know if two cookies are the same.
If Jar manages this field, then it can be envoded in Data
as well. If Jar manages Creation, then Jar will have
to look up any cookie with Entries before Add'ing
them to choose the right Creation time.

https://codereview.appspot.com/6849092/

https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/jar.go
File src/pkg/net/http/cookiejar/jar.go (right):

https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/jar.go#newcode56
src/pkg/net/http/cookiejar/jar.go:56: // TODO: how do we reject HttpOnly
cookies? Do we post-process the return
I think that net/http/cookiejar would only care about HTTP(S). FTP and
other transport protocols are out of scope.

https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/storage.go
File src/pkg/net/http/cookiejar/storage.go (right):

https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/storage.go#newcode66
src/pkg/net/http/cookiejar/storage.go:66: Creation time.Time
I think the principle is that Storage is as dumb as possible. Again, I'd
like the programmer who implements SQLiteStorage not to have to know
anything about HTTP cookies.

Yes, a Jar will need to scan existing cookies before Add'ing new ones.
We could move name/domain/path into an Entry instead, but it would make
a Storage more complicated. Hmm...

https://codereview.appspot.com/6849092/

I've uploaded a version that puts the domain/name/path (which I've
called a SubKey) into an Entry. PTAL at
https://codereview.appspot.com/6849092/

I haven't thought enough about whether the Subkey needs to be exposed.



https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/storage_test.go
File src/pkg/net/http/cookiejar/storage_test.go (right):

https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/storage_test.go#newcode12
src/pkg/net/http/cookiejar/storage_test.go:12: testCases :=
map[string]bool{
Because 99% of the other tests do that. Some of the reasons: it avoids
some indentation, it makes it easier to scan forward to find where the
table is done (completely unindented lines), and it makes the table
available to some future test function that might be able to reuse the
cases.

When I see a local variable I like this I expect it contains a value
that can only be computed once the test has started running. There's no
such value here.

https://codereview.appspot.com/6849092/diff/10002/src/pkg/net/http/cookiejar/storage.go
File src/pkg/net/http/cookiejar/storage.go (right):

https://codereview.appspot.com/6849092/diff/10002/src/pkg/net/http/cookiejar/storage.go#newcode12
src/pkg/net/http/cookiejar/storage.go:12: // entries. Each entry
consists of a sub-key, creation time, last access time,
s/sub-key/subkey/
subkey is a fine English word without needing a hyphen.
Because of that fact, the name below should be Subkey not SubKey.

https://codereview.appspot.com/6849092/

https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/storage_test.go
File src/pkg/net/http/cookiejar/storage_test.go (right):

https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/storage_test.go#newcode12
src/pkg/net/http/cookiejar/storage_test.go:12: testCases :=
map[string]bool{
it's just as
value here.

Done.

https://codereview.appspot.com/6849092/diff/10002/src/pkg/net/http/cookiejar/storage.go
File src/pkg/net/http/cookiejar/storage.go (right):

https://codereview.appspot.com/6849092/diff/10002/src/pkg/net/http/cookiejar/storage.go#newcode12
src/pkg/net/http/cookiejar/storage.go:12: // entries. Each entry
consists of a sub-key, creation time, last access time,
Done.

https://codereview.appspot.com/6849092/

LGTM

I wonder if cookiejar should be located under exp:
Handling IDNs properly will require exp/norm.

https://codereview.appspot.com/6849092/

Hmm... I'll move it to exp/cookiejar for now.

*** Submitted as
http://code.google.com/p/go/source/detail?r=69e2f3d33300 ***

exp/cookiejar: new package.

This CL defines the API. Implementation will come in follow-up CLs.

Update issue 1960.

R=bradfitz, dr.volker.dobler, rsc
CC=golang-dev
http://codereview.appspot.com/6849092


http://codereview.appspot.com/6849092/